From 7f35f7cfbda36714c487696e4b58470226ddd855 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 30 Apr 2025 14:30:52 +0200 Subject: [PATCH 01/10] fix(node): Avoid double-wrapping http module --- .../suites/express/with-http/test.ts | 56 +++-- .../http/SentryHttpInstrumentation.ts | 197 ++++++------------ .../http/vendor/getRequestInfo.ts | 157 -------------- 3 files changed, 87 insertions(+), 323 deletions(-) delete mode 100644 packages/node/src/integrations/http/vendor/getRequestInfo.ts 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 index ef27c3129020..10dbefa74a9a 100644 --- 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 index 5dbdeae2f925..5ac4a9b238f2 100644 --- 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 { + 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 { - 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; - - 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 { - let pathname: string; - let origin: string; - let optionsParsed: RequestOptions; - let invalidUrl = false; - if (typeof options === 'string') { - try { - const convertedOptions = stringUrlToHttpOptions(options); - optionsParsed = convertedOptions; - pathname = convertedOptions.pathname || '/'; - } catch (e) { - invalidUrl = true; - logger.verbose( - 'Unable to parse URL provided to HTTP request, using fallback to determine path. Original error:', - e, - ); - // for backward compatibility with how url.parse() behaved. - optionsParsed = { - path: options, - }; - pathname = optionsParsed.path || '/'; - } - - origin = `${optionsParsed.protocol || 'http:'}//${optionsParsed.host}`; - if (extraOptions !== undefined) { - Object.assign(optionsParsed, extraOptions); - } - } else if (options instanceof url.URL) { - optionsParsed = { - protocol: options.protocol, - hostname: - typeof options.hostname === 'string' && options.hostname.startsWith('[') - ? options.hostname.slice(1, -1) - : options.hostname, - path: `${options.pathname || ''}${options.search || ''}`, - }; - if (options.port !== '') { - optionsParsed.port = Number(options.port); - } - if (options.username || options.password) { - optionsParsed.auth = `${options.username}:${options.password}`; - } - pathname = options.pathname; - origin = options.origin; - if (extraOptions !== undefined) { - Object.assign(optionsParsed, extraOptions); - } - } else { - optionsParsed = Object.assign({ protocol: options.host ? 'http:' : undefined }, options); - - const hostname = - optionsParsed.host || - (optionsParsed.port != null ? `${optionsParsed.hostname}${optionsParsed.port}` : optionsParsed.hostname); - origin = `${optionsParsed.protocol || 'http:'}//${hostname}`; - - pathname = (options as url.URL).pathname; - if (!pathname && optionsParsed.path) { - try { - const parsedUrl = new URL(optionsParsed.path, origin); - pathname = parsedUrl.pathname || '/'; - } catch (e) { - pathname = '/'; - } - } - } - - // some packages return method in lowercase.. - // ensure upperCase for consistency - const method = optionsParsed.method ? optionsParsed.method.toUpperCase() : 'GET'; - - return { origin, pathname, method, optionsParsed, invalidUrl }; -}; - -/** - * Mimics Node.js conversion of URL strings to RequestOptions expected by - * `http.request` and `https.request` APIs. - * - * See https://github.com/nodejs/node/blob/2505e217bba05fc581b572c685c5cf280a16c5a3/lib/internal/url.js#L1415-L1437 - * - * @param stringUrl - * @throws TypeError if the URL is not valid. - */ -function stringUrlToHttpOptions(stringUrl: string): RequestOptions & { pathname: string } { - // This is heavily inspired by Node.js handling of the same situation, trying - // to follow it as closely as possible while keeping in mind that we only - // deal with string URLs, not URL objects. - const { hostname, pathname, port, username, password, search, protocol, hash, href, origin, host } = new URL( - stringUrl, - ); - - const options: RequestOptions & { - pathname: string; - hash: string; - search: string; - href: string; - origin: string; - } = { - protocol: protocol, - hostname: hostname && hostname[0] === '[' ? hostname.slice(1, -1) : hostname, - hash: hash, - search: search, - pathname: pathname, - path: `${pathname || ''}${search || ''}`, - href: href, - origin: origin, - host: host, - }; - if (port !== '') { - options.port = Number(port); - } - if (username || password) { - options.auth = `${decodeURIComponent(username)}:${decodeURIComponent(password)}`; - } - return options; -} From 6ad5c03ed7fde72fe4d9a0890397551a09543877 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 30 Apr 2025 16:59:13 +0200 Subject: [PATCH 02/10] fix astro stuff --- dev-packages/e2e-tests/package.json | 2 +- .../astro-5/tests/tracing.dynamic.test.ts | 1 - .../http/SentryHttpInstrumentation.ts | 28 +++++++++++++------ 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/dev-packages/e2e-tests/package.json b/dev-packages/e2e-tests/package.json index a1dceaeebfda..957589c1fd91 100644 --- a/dev-packages/e2e-tests/package.json +++ b/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,pnpm-lock.yaml,.last-run.json,test-results}", "clean:pnpm": "pnpm store prune" }, "devDependencies": { diff --git a/dev-packages/e2e-tests/test-applications/astro-5/tests/tracing.dynamic.test.ts b/dev-packages/e2e-tests/test-applications/astro-5/tests/tracing.dynamic.test.ts index 2bcf6cbf2362..eb70f7362e63 100644 --- a/dev-packages/e2e-tests/test-applications/astro-5/tests/tracing.dynamic.test.ts +++ b/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), diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index 5ac4a9b238f2..d89a4a69e25c 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -114,6 +114,15 @@ export class SentryHttpInstrumentation extends InstrumentationBase { + const request = (data as { request: http.ClientRequest }).request; + // there is no response object here, we only have access to request & error :( + + this._onOutgoingRequestFinish(request, undefined); + }); + return []; } @@ -121,7 +130,9 @@ export class SentryHttpInstrumentation extends InstrumentationBase) => { const [event, listener, ...restArgs] = args; - if (DEBUG_BUILD) { - logger.log(INSTRUMENTATION_NAME, 'Patching request.on', event); - } - if (event === 'data') { + DEBUG_BUILD && logger.log(INSTRUMENTATION_NAME, 'Handling request.on', event); const callback = new Proxy(listener, { apply: (target, thisArg, args: Parameters) => { try { From 5c99873fa7198624ac2c9086f6eee2eb06a4140c Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 30 Apr 2025 17:46:09 +0200 Subject: [PATCH 03/10] one more cleanup --- dev-packages/e2e-tests/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-packages/e2e-tests/package.json b/dev-packages/e2e-tests/package.json index 957589c1fd91..718fdf478053 100644 --- a/dev-packages/e2e-tests/package.json +++ b/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,.astro,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": { From 030f222429b3826202ebfa6cd52edb9a9409154a Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 2 May 2025 09:48:45 +0200 Subject: [PATCH 04/10] fix order with otel instrumentation --- .../http/SentryHttpInstrumentation.ts | 103 ++++++++++++++---- packages/node/src/integrations/http/index.ts | 15 +-- 2 files changed, 88 insertions(+), 30 deletions(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index d89a4a69e25c..5c88b26bf72b 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -1,10 +1,13 @@ -import { subscribe } from 'node:diagnostics_channel'; +/* eslint-disable max-lines */ +import type { ChannelListener } from 'node:diagnostics_channel'; +import { subscribe, unsubscribe } from 'node:diagnostics_channel'; import type * as http 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 } from '@opentelemetry/instrumentation'; +import { InstrumentationBase, InstrumentationNodeModuleDefinition } from '@opentelemetry/instrumentation'; import type { AggregationCounts, Client, SanitizedRequestData, Scope } from '@sentry/core'; import { addBreadcrumb, @@ -26,6 +29,9 @@ import { getRequestUrl } from '../../utils/getRequestUrl'; const INSTRUMENTATION_NAME = '@sentry/instrumentation-http'; +type Http = typeof http; +type Https = typeof https; + export type SentryHttpInstrumentationOptions = InstrumentationConfig & { /** * Whether breadcrumbs should be recorded for requests. @@ -101,29 +107,80 @@ export class SentryHttpInstrumentation extends InstrumentationBase { - const server = (data as { server: http.Server }).server; - this._patchServerEmit(server); - }); - - subscribe('http.client.response.finish', data => { - const request = (data as { request: http.ClientRequest }).request; - const response = (data as { response: http.IncomingMessage }).response; + public init(): [InstrumentationNodeModuleDefinition, InstrumentationNodeModuleDefinition] { + // Nothing to do here - this._onOutgoingRequestFinish(request, response); - }); + const handledRequests = new WeakSet(); - // When an error happens, we still want to have a breadcrumb - // In this case, `http.client.response.finish` is not triggered - subscribe('http.client.request.error', data => { - const request = (data as { request: http.ClientRequest }).request; - // there is no response object here, we only have access to request & error :( + const handleOutgoingRequestFinishOnce = (request: http.ClientRequest, response?: http.IncomingMessage): void => { + if (handledRequests.has(request)) { + return; + } - this._onOutgoingRequestFinish(request, undefined); - }); + handledRequests.add(request); + this._onOutgoingRequestFinish(request, response); + }; - return []; + const onHttpServerRequestStart = ((_data: unknown) => { + const data = _data as { server: http.Server }; + this._patchServerEmitOnce(data.server); + }) satisfies ChannelListener; + + const onHttpsServerRequestStart = ((_data: unknown) => { + const data = _data as { server: http.Server }; + this._patchServerEmitOnce(data.server); + }) satisfies ChannelListener; + + const onHttpClientResponseFinish = ((_data: unknown) => { + const data = _data as { request: http.ClientRequest; response: http.IncomingMessage }; + handleOutgoingRequestFinishOnce(data.request, data.response); + }) satisfies ChannelListener; + + const onHttpClientRequestError = ((_data: unknown) => { + const data = _data as { request: http.ClientRequest }; + handleOutgoingRequestFinishOnce(data.request, undefined); + }) satisfies ChannelListener; + + return [ + new InstrumentationNodeModuleDefinition( + 'http', + ['*'], + (moduleExports: Http): Http => { + subscribe('http.server.request.start', onHttpServerRequestStart); + subscribe('http.client.response.finish', onHttpClientResponseFinish); + + // When an error happens, we still want to have a breadcrumb + // In this case, `http.client.response.finish` is not triggered + subscribe('http.client.request.error', onHttpClientRequestError); + + return moduleExports; + }, + () => { + unsubscribe('http.server.request.start', onHttpServerRequestStart); + unsubscribe('http.client.response.finish', onHttpClientResponseFinish); + unsubscribe('http.client.request.error', onHttpClientRequestError); + }, + ), + new InstrumentationNodeModuleDefinition( + 'https', + ['*'], + (moduleExports: Https): Https => { + subscribe('http.server.request.start', onHttpsServerRequestStart); + subscribe('http.client.response.finish', onHttpClientResponseFinish); + + // When an error happens, we still want to have a breadcrumb + // In this case, `http.client.response.finish` is not triggered + subscribe('http.client.request.error', onHttpClientRequestError); + + return moduleExports; + }, + () => { + unsubscribe('http.server.request.start', onHttpsServerRequestStart); + unsubscribe('http.client.response.finish', onHttpClientResponseFinish); + unsubscribe('http.client.request.error', onHttpClientRequestError); + }, + ), + ]; } /** @@ -150,7 +207,7 @@ export class SentryHttpInstrumentation extends InstrumentationBase return { name: INTEGRATION_NAME, setupOnce() { + // TODO: get rid of this too // Below, we instrument the Node.js HTTP API three times. 2 times Sentry-specific, 1 time OTEL specific. // Due to timing reasons, we sometimes need to apply Sentry instrumentation _before_ we apply the OTEL // instrumentation (e.g. to flush on serverless platforms), and sometimes we need to apply Sentry instrumentation @@ -165,19 +166,19 @@ export const httpIntegration = defineIntegration((options: HttpOptions = {}) => const instrumentSpans = _shouldInstrumentSpans(options, getClient()?.getOptions()); - // This is the "regular" OTEL instrumentation that emits spans - if (instrumentSpans) { - const instrumentationConfig = getConfigWithDefaults(options); - instrumentOtelHttp(instrumentationConfig); - } - - // This is Sentry-specific instrumentation that is applied _after_ any OTEL instrumentation. + // This is Sentry-specific instrumentation for request isolation and breadcrumbs instrumentSentryHttp({ ...options, // If spans are not instrumented, it means the HttpInstrumentation has not been added // In that case, we want to handle incoming trace extraction ourselves extractIncomingTraceFromHeader: !instrumentSpans, }); + + // This is the "regular" OTEL instrumentation that emits spans + if (instrumentSpans) { + const instrumentationConfig = getConfigWithDefaults(options); + instrumentOtelHttp(instrumentationConfig); + } }, }; }); From 3a6317e83eb16e4732cc54abb76878dc5519880e Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 2 May 2025 13:16:32 +0200 Subject: [PATCH 05/10] fix comment --- .../node/src/integrations/http/SentryHttpInstrumentation.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index 5c88b26bf72b..3ab962e11f51 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -108,8 +108,6 @@ export class SentryHttpInstrumentation extends InstrumentationBase(); const handleOutgoingRequestFinishOnce = (request: http.ClientRequest, response?: http.IncomingMessage): void => { From db3b34084476047fcb67bf058384e2bac9ca0b83 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 2 May 2025 13:19:37 +0200 Subject: [PATCH 06/10] improve dedupe logic --- .../http/SentryHttpInstrumentation.ts | 45 +++++++++++-------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index 3ab962e11f51..b529fa7f689b 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -108,42 +108,43 @@ export class SentryHttpInstrumentation extends InstrumentationBase(); - - const handleOutgoingRequestFinishOnce = (request: http.ClientRequest, response?: http.IncomingMessage): void => { - if (handledRequests.has(request)) { - return; - } - - handledRequests.add(request); - this._onOutgoingRequestFinish(request, response); - }; + // We register handlers when either http or https is instrumented + // but we only want to register them once, whichever is instrumented first + let hasRegisteredHandlers = false; const onHttpServerRequestStart = ((_data: unknown) => { const data = _data as { server: http.Server }; this._patchServerEmitOnce(data.server); }) satisfies ChannelListener; - const onHttpsServerRequestStart = ((_data: unknown) => { - const data = _data as { server: http.Server }; - this._patchServerEmitOnce(data.server); - }) satisfies ChannelListener; - const onHttpClientResponseFinish = ((_data: unknown) => { const data = _data as { request: http.ClientRequest; response: http.IncomingMessage }; - handleOutgoingRequestFinishOnce(data.request, data.response); + this._onOutgoingRequestFinish(data.request, data.response); }) satisfies ChannelListener; const onHttpClientRequestError = ((_data: unknown) => { const data = _data as { request: http.ClientRequest }; - handleOutgoingRequestFinishOnce(data.request, undefined); + this._onOutgoingRequestFinish(data.request, undefined); }) satisfies ChannelListener; + /** + * You may be wondering why we register these diagnostics-channel listenrers in such InstrumentationNodeModuleDefinition, + * instead of simply subscribing to the events once in here. + * The reason for this is timing semantics: These functions are called once the http or https module is loaded. + * If we'd subscribe before that, there seem to be conflicts with the OTEL native instrumentation in some scenarios, + * especially the "import-on-top" pattern of setting up ESM applications. + */ return [ new InstrumentationNodeModuleDefinition( 'http', ['*'], (moduleExports: Http): Http => { + if (hasRegisteredHandlers) { + return moduleExports; + } + + hasRegisteredHandlers = true; + subscribe('http.server.request.start', onHttpServerRequestStart); subscribe('http.client.response.finish', onHttpClientResponseFinish); @@ -163,7 +164,13 @@ export class SentryHttpInstrumentation extends InstrumentationBase { - subscribe('http.server.request.start', onHttpsServerRequestStart); + if (hasRegisteredHandlers) { + return moduleExports; + } + + hasRegisteredHandlers = true; + + subscribe('http.server.request.start', onHttpServerRequestStart); subscribe('http.client.response.finish', onHttpClientResponseFinish); // When an error happens, we still want to have a breadcrumb @@ -173,7 +180,7 @@ export class SentryHttpInstrumentation extends InstrumentationBase { - unsubscribe('http.server.request.start', onHttpsServerRequestStart); + unsubscribe('http.server.request.start', onHttpServerRequestStart); unsubscribe('http.client.response.finish', onHttpClientResponseFinish); unsubscribe('http.client.request.error', onHttpClientRequestError); }, From 1f8247124eb57576e827167343d7a0e310c50e70 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 2 May 2025 13:35:08 +0200 Subject: [PATCH 07/10] fix comment --- .../node/src/integrations/http/SentryHttpInstrumentation.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index b529fa7f689b..b79a279ba0c3 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -109,7 +109,7 @@ export class SentryHttpInstrumentation extends InstrumentationBase { @@ -128,7 +128,8 @@ export class SentryHttpInstrumentation extends InstrumentationBase Date: Fri, 2 May 2025 13:45:30 +0200 Subject: [PATCH 08/10] fix logger statements --- .../src/integrations/http/SentryHttpInstrumentation.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index b79a279ba0c3..74ae85ba84ed 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -194,7 +194,7 @@ export class SentryHttpInstrumentation extends InstrumentationBase) => { try { From 56825c156464452dff976c39a382b3a4700db262 Mon Sep 17 00:00:00 2001 From: Francesco Gringl-Novy Date: Mon, 5 May 2025 10:18:53 +0200 Subject: [PATCH 09/10] Apply suggestions from code review Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com> --- .../node/src/integrations/http/SentryHttpInstrumentation.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index 74ae85ba84ed..4e044879d2aa 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -222,7 +222,7 @@ export class SentryHttpInstrumentation extends InstrumentationBase Date: Mon, 5 May 2025 10:27:08 +0200 Subject: [PATCH 10/10] add changelog entry --- CHANGELOG.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fce135710fb8..9554dca0f2bc 100644 --- a/CHANGELOG.md +++ b/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