fix(node): Avoid double-wrapping http module (#16177)

@s1gr1d and @Lms24 figured out that the duplicate spans we sometimes see
are related to esm and the http module. Especially, it seems to be
related to us using the `stealthWrap` function to wrap `server.emit` for
request isolation purposes.

While we still don't really know _why_ this is making such problems,
this PR seems to fix it (at least in integration tests) by avoiding
using import-in-the-middle here, and instead using diagnostics channel
with good old-fashioned monkey patching on the passed-in `server`
instance.

Some note: We need to make sure to still call this in the otel-wrapping
code of `init()`, otherwise there are weird timing issues in
top-level-import scenarios 😬

Hopefully fixes
#15830, and fixes
#15803

---------

Co-authored-by: Andrei <[email protected]>

Author: mydea

CHANGELOG.md

@@ -10,6 +10,28 @@
 
 - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott
 
+### Important Changes
+
+- **fix(node): Avoid double-wrapping http module ([#16177](https://github.com/getsentry/sentry-javascript/pull/16177))**
+
+When running your application in ESM mode, there have been scenarios that resulted in the `http`/`https` emitting duplicate spans for incoming requests. This was apparently caused by us double-wrapping the modules for incoming request isolation.
+
+In order to solve this problem, the modules are no longer monkey patched by us for request isolation. Instead, we register diagnostics*channel hooks to handle request isolation now.
+While this is generally not expected to break anything, there is one tiny change that \_may* affect you if you have been relying on very specific functionality:
+
+The `ignoreOutgoingRequests` option of `httpIntegration` receives the `RequestOptions` as second argument. This type is not changed, however due to how the wrapping now works, we no longer pass through the full RequestOptions, but re-construct this partially based on the generated request. For the vast majority of cases, this should be fine, but for the sake of completeness, these are the only fields that may be available there going forward - other fields that _may_ have existed before may no longer be set:
+
+```ts
+ignoreOutgoingRequests(url: string, {
+  method: string;
+  protocol: string;
+  host: string;
+  hostname: string; // same as host
+  path: string;
+  headers: OutgoingHttpHeaders;
+})
+```
+
 ## 9.15.0
 
 ### Important Changes

dev-packages/e2e-tests/package.json

@@ -16,7 +16,7 @@
     "clean": "rimraf tmp node_modules && yarn clean:test-applications && yarn clean:pnpm",
     "ci:build-matrix": "ts-node ./lib/getTestMatrix.ts",
     "ci:build-matrix-optional": "ts-node ./lib/getTestMatrix.ts --optional=true",
-    "clean:test-applications": "rimraf --glob test-applications/**/{node_modules,dist,build,.next,.nuxt,.sveltekit,.react-router,pnpm-lock.yaml,.last-run.json,test-results}",
+    "clean:test-applications": "rimraf --glob test-applications/**/{node_modules,dist,build,.next,.nuxt,.sveltekit,.react-router,.astro,.output,pnpm-lock.yaml,.last-run.json,test-results}",
     "clean:pnpm": "pnpm store prune"
   },
   "devDependencies": {

dev-packages/e2e-tests/test-applications/astro-5/tests/tracing.dynamic.test.ts

@@ -62,7 +62,6 @@ test.describe('tracing in dynamically rendered (ssr) routes', () => {
     });
 
     expect(serverPageRequestTxn).toMatchObject({
-      breadcrumbs: expect.any(Array),
       contexts: {
         app: expect.any(Object),
         cloud_resource: expect.any(Object),

dev-packages/node-integration-tests/suites/express/with-http/test.ts

@@ -6,36 +6,28 @@ describe('express with http import', () => {
     cleanupChildProcesses();
   });
 
-  createEsmAndCjsTests(
-    __dirname,
-    'scenario.mjs',
-    'instrument.mjs',
-    (createRunner, test) => {
-      test('it works when importing the http module', async () => {
-        const runner = createRunner()
-          .expect({
-            transaction: {
-              transaction: 'GET /test2',
-            },
-          })
-          .expect({
-            transaction: {
-              transaction: 'GET /test',
-            },
-          })
-          .expect({
-            transaction: {
-              transaction: 'GET /test3',
-            },
-          })
-          .start();
-        await runner.makeRequest('get', '/test');
-        await runner.makeRequest('get', '/test3');
-        await runner.completed();
-      });
-      // TODO: This is failing on ESM because importing http is triggering the http spans twice :(
-      // We need to fix this!
-    },
-    { failsOnEsm: true },
-  );
+  createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => {
+    test('it works when importing the http module', async () => {
+      const runner = createRunner()
+        .expect({
+          transaction: {
+            transaction: 'GET /test2',
+          },
+        })
+        .expect({
+          transaction: {
+            transaction: 'GET /test',
+          },
+        })
+        .expect({
+          transaction: {
+            transaction: 'GET /test3',
+          },
+        })
+        .start();
+      await runner.makeRequest('get', '/test');
+      await runner.makeRequest('get', '/test3');
+      await runner.completed();
+    });
+  });
 });