fix(node): Avoid double-wrapping http module

mydea · mydea · commit 72ddee3cf653 · 2025-04-30T16:05:45.000+02:00
diff --git a/dev-packages/node-integration-tests/suites/express/with-http/test.ts b/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();
+    });
+  });
 });
diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts
@@ -1,15 +1,14 @@
-/* eslint-disable max-lines */
+import { subscribe } from 'node:diagnostics_channel';
 import type * as http from 'node:http';
-import type { IncomingMessage, RequestOptions } from 'node:http';
-import type * as https from 'node:https';
 import type { EventEmitter } from 'node:stream';
 import { context, propagation } from '@opentelemetry/api';
 import { VERSION } from '@opentelemetry/core';
 import type { InstrumentationConfig } from '@opentelemetry/instrumentation';
-import { InstrumentationBase, InstrumentationNodeModuleDefinition } from '@opentelemetry/instrumentation';
+import { InstrumentationBase } from '@opentelemetry/instrumentation';
 import type { AggregationCounts, Client, SanitizedRequestData, Scope } from '@sentry/core';
 import {
   addBreadcrumb,
+  addNonEnumerableProperty,
   generateSpanId,
   getBreadcrumbLogLevelFromHttpStatusCode,
   getClient,
@@ -24,11 +23,6 @@ import {
 } from '@sentry/core';
 import { DEBUG_BUILD } from '../../debug-build';
 import { getRequestUrl } from '../../utils/getRequestUrl';
-import { stealthWrap } from './utils';
-import { getRequestInfo } from './vendor/getRequestInfo';
-
-type Http = typeof http;
-type Https = typeof https;
 
 const INSTRUMENTATION_NAME = '@sentry/instrumentation-http';
 
@@ -58,7 +52,7 @@ export type SentryHttpInstrumentationOptions = InstrumentationConfig & {
    * @param url Contains the entire URL, including query string (if any), protocol, host, etc. of the outgoing request.
    * @param request Contains the {@type RequestOptions} object used to make the outgoing request.
    */
-  ignoreOutgoingRequests?: (url: string, request: RequestOptions) => boolean;
+  ignoreOutgoingRequests?: (url: string, request: http.RequestOptions) => boolean;
 
   /**
    * Do not capture the request body for incoming HTTP requests to URLs where the given callback returns `true`.
@@ -67,7 +61,7 @@ export type SentryHttpInstrumentationOptions = InstrumentationConfig & {
    * @param url Contains the entire URL, including query string (if any), protocol, host, etc. of the outgoing request.
    * @param request Contains the {@type RequestOptions} object used to make the outgoing request.
    */
-  ignoreIncomingRequestBody?: (url: string, request: RequestOptions) => boolean;
+  ignoreIncomingRequestBody?: (url: string, request: http.RequestOptions) => boolean;
 
   /**
    * Whether the integration should create [Sessions](https://docs.sentry.io/product/releases/health/#sessions) for incoming requests to track the health and crash-free rate of your releases in Sentry.
@@ -107,72 +101,65 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns
   }
 
   /** @inheritdoc */
-  public init(): [InstrumentationNodeModuleDefinition, InstrumentationNodeModuleDefinition] {
-    return [this._getHttpsInstrumentation(), this._getHttpInstrumentation()];
-  }
+  public init(): [] {
+    subscribe('http.server.request.start', data => {
+      const server = (data as { server: http.Server }).server;
+      this._patchServerEmit(server);
+    });
 
-  /** Get the instrumentation for the http module. */
-  private _getHttpInstrumentation(): InstrumentationNodeModuleDefinition {
-    return new InstrumentationNodeModuleDefinition(
-      'http',
-      ['*'],
-      (moduleExports: Http): Http => {
-        // Patch incoming requests for request isolation
-        stealthWrap(moduleExports.Server.prototype, 'emit', this._getPatchIncomingRequestFunction());
+    subscribe('http.client.response.finish', data => {
+      const request = (data as { request: http.ClientRequest }).request;
+      const response = (data as { response: http.IncomingMessage }).response;
 
-        // Patch outgoing requests for breadcrumbs
-        const patchedRequest = stealthWrap(moduleExports, 'request', this._getPatchOutgoingRequestFunction());
-        stealthWrap(moduleExports, 'get', this._getPatchOutgoingGetFunction(patchedRequest));
+      this._onOutgoingRequestFinish(request, response);
+    });
 
-        return moduleExports;
-      },
-      () => {
-        // no unwrap here
-      },
-    );
+    return [];
   }
 
-  /** Get the instrumentation for the https module. */
-  private _getHttpsInstrumentation(): InstrumentationNodeModuleDefinition {
-    return new InstrumentationNodeModuleDefinition(
-      'https',
-      ['*'],
-      (moduleExports: Https): Https => {
-        // Patch incoming requests for request isolation
-        stealthWrap(moduleExports.Server.prototype, 'emit', this._getPatchIncomingRequestFunction());
+  /**
+   * This is triggered when an outgoing request finishes.
+   * It has access to the final request and response objects.
+   */
+  private _onOutgoingRequestFinish(request: http.ClientRequest, response: http.IncomingMessage): void {
+    const _breadcrumbs = this.getConfig().breadcrumbs;
+    const breadCrumbsEnabled = typeof _breadcrumbs === 'undefined' ? true : _breadcrumbs;
+    const options = getRequestOptions(request);
 
-        // Patch outgoing requests for breadcrumbs
-        const patchedRequest = stealthWrap(moduleExports, 'request', this._getPatchOutgoingRequestFunction());
-        stealthWrap(moduleExports, 'get', this._getPatchOutgoingGetFunction(patchedRequest));
+    const _ignoreOutgoingRequests = this.getConfig().ignoreOutgoingRequests;
+    const shouldCreateBreadcrumb =
+      typeof _ignoreOutgoingRequests === 'function' ? !_ignoreOutgoingRequests(getRequestUrl(request), options) : true;
 
-        return moduleExports;
-      },
-      () => {
-        // no unwrap here
-      },
-    );
+    if (breadCrumbsEnabled && shouldCreateBreadcrumb) {
+      addRequestBreadcrumb(request, response);
+    }
   }
 
   /**
-   * Patch the incoming request function for request isolation.
+   * Patch a server.emit function to handle process isolation for incoming requests.
+   * This will only patch the emit function if it was not already patched.
    */
-  private _getPatchIncomingRequestFunction(): (
-    original: (event: string, ...args: unknown[]) => boolean,
-  ) => (this: unknown, event: string, ...args: unknown[]) => boolean {
+  private _patchServerEmit(server: http.Server): void {
+    // eslint-disable-next-line @typescript-eslint/unbound-method
+    const originalEmit = server.emit;
+
+    // This means it was already patched, do nothing
+    if ((originalEmit as { __sentry_patched__?: boolean }).__sentry_patched__) {
+      return;
+    }
+
     // eslint-disable-next-line @typescript-eslint/no-this-alias
     const instrumentation = this;
     const { ignoreIncomingRequestBody } = instrumentation.getConfig();
 
-    return (
-      original: (event: string, ...args: unknown[]) => boolean,
-    ): ((this: unknown, event: string, ...args: unknown[]) => boolean) => {
-      return function incomingRequest(this: unknown, ...args: [event: string, ...args: unknown[]]): boolean {
+    const newEmit = new Proxy(originalEmit, {
+      apply(target, thisArg, args: [event: string, ...args: unknown[]]) {
         // Only traces request events
         if (args[0] !== 'request') {
-          return original.apply(this, args);
+          return target.apply(thisArg, args);
         }
 
-        instrumentation._diag.debug('http instrumentation for incoming request');
+        DEBUG_BUILD && logger.log('http instrumentation for incoming request');
 
         const isolationScope = getIsolationScope().clone();
         const request = args[1] as http.IncomingMessage;
@@ -217,89 +204,20 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns
 
           // If we don't want to extract the trace from the header, we can skip this
           if (!instrumentation.getConfig().extractIncomingTraceFromHeader) {
-            return original.apply(this, args);
+            return target.apply(thisArg, args);
           }
 
           const ctx = propagation.extract(context.active(), normalizedRequest.headers);
           return context.with(ctx, () => {
-            return original.apply(this, args);
+            return target.apply(thisArg, args);
           });
         });
-      };
-    };
-  }
-
-  /**
-   * Patch the outgoing request function for breadcrumbs.
-   */
-  private _getPatchOutgoingRequestFunction(): (
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    original: (...args: any[]) => http.ClientRequest,
-  ) => (options: URL | http.RequestOptions | string, ...args: unknown[]) => http.ClientRequest {
-    // eslint-disable-next-line @typescript-eslint/no-this-alias
-    const instrumentation = this;
-
-    return (original: (...args: unknown[]) => http.ClientRequest): ((...args: unknown[]) => http.ClientRequest) => {
-      return function outgoingRequest(this: unknown, ...args: unknown[]): http.ClientRequest {
-        instrumentation._diag.debug('http instrumentation for outgoing requests');
-
-        // Making a copy to avoid mutating the original args array
-        // We need to access and reconstruct the request options object passed to `ignoreOutgoingRequests`
-        // so that it matches what Otel instrumentation passes to `ignoreOutgoingRequestHook`.
-        // @see https://github.com/open-telemetry/opentelemetry-js/blob/7293e69c1e55ca62e15d0724d22605e61bd58952/experimental/packages/opentelemetry-instrumentation-http/src/http.ts#L756-L789
-        const argsCopy = [...args];
-
-        const options = argsCopy.shift() as URL | http.RequestOptions | string;
-
-        const extraOptions =
-          typeof argsCopy[0] === 'object' && (typeof options === 'string' || options instanceof URL)
-            ? (argsCopy.shift() as http.RequestOptions)
-            : undefined;
-
-        const { optionsParsed } = getRequestInfo(instrumentation._diag, options, extraOptions);
-
-        const request = original.apply(this, args) as ReturnType<typeof http.request>;
-
-        request.prependListener('response', (response: http.IncomingMessage) => {
-          const _breadcrumbs = instrumentation.getConfig().breadcrumbs;
-          const breadCrumbsEnabled = typeof _breadcrumbs === 'undefined' ? true : _breadcrumbs;
-
-          const _ignoreOutgoingRequests = instrumentation.getConfig().ignoreOutgoingRequests;
-          const shouldCreateBreadcrumb =
-            typeof _ignoreOutgoingRequests === 'function'
-              ? !_ignoreOutgoingRequests(getRequestUrl(request), optionsParsed)
-              : true;
-
-          if (breadCrumbsEnabled && shouldCreateBreadcrumb) {
-            addRequestBreadcrumb(request, response);
-          }
-        });
+      },
+    });
 
-        return request;
-      };
-    };
-  }
+    addNonEnumerableProperty(newEmit, '__sentry_patched__', true);
 
-  /** Path the outgoing get function for breadcrumbs. */
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  private _getPatchOutgoingGetFunction(clientRequest: (...args: any[]) => http.ClientRequest) {
-    return (_original: unknown): ((...args: unknown[]) => http.ClientRequest) => {
-      // Re-implement http.get. This needs to be done (instead of using
-      // getPatchOutgoingRequestFunction to patch it) because we need to
-      // set the trace context header before the returned http.ClientRequest is
-      // ended. The Node.js docs state that the only differences between
-      // request and get are that (1) get defaults to the HTTP GET method and
-      // (2) the returned request object is ended immediately. The former is
-      // already true (at least in supported Node versions up to v10), so we
-      // simply follow the latter. Ref:
-      // https://nodejs.org/dist/latest/docs/api/http.html#http_http_get_options_callback
-      // https://github.com/googleapis/cloud-trace-nodejs/blob/master/src/instrumentations/instrumentation-http.ts#L198
-      return function outgoingGetRequest(...args: unknown[]): http.ClientRequest {
-        const req = clientRequest(...args);
-        req.end();
-        return req;
-      };
-    };
+    server.emit = newEmit;
   }
 }
 
@@ -359,7 +277,7 @@ function getBreadcrumbData(request: http.ClientRequest): Partial<SanitizedReques
  * we monkey patch `req.on('data')` to intercept the body chunks.
  * This way, we only read the body if the user also consumes the body, ensuring we do not change any behavior in unexpected ways.
  */
-function patchRequestToCaptureBody(req: IncomingMessage, isolationScope: Scope): void {
+function patchRequestToCaptureBody(req: http.IncomingMessage, isolationScope: Scope): void {
   let bodyByteLength = 0;
   const chunks: Buffer[] = [];
 
@@ -451,6 +369,17 @@ function patchRequestToCaptureBody(req: IncomingMessage, isolationScope: Scope):
   }
 }
 
+function getRequestOptions(request: http.ClientRequest): http.RequestOptions {
+  return {
+    method: request.method,
+    protocol: request.protocol,
+    host: request.host,
+    hostname: request.host,
+    path: request.path,
+    headers: request.getHeaders(),
+  };
+}
+
 /**
  * Starts a session and tracks it in the context of a given isolation scope.
  * When the passed response is finished, the session is put into a task and is
diff --git a/packages/node/src/integrations/http/vendor/getRequestInfo.ts b/packages/node/src/integrations/http/vendor/getRequestInfo.ts
