-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(node): Avoid OTEL instrumentation for outgoing requests on Node 22+ #17355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
9c68295
1dad574
81ba2cf
52cdf7d
a798420
e4246f3
db7c0e2
acf83b6
ca643e8
c6df808
c12e32e
3ef2033
bdcf180
177b26f
8c7d414
5fc2a89
54dae22
4ef95fd
9c09bfd
f8e3cad
9cb0bcf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,35 @@ | ||
| import type { ChannelListener } from 'node:diagnostics_channel'; | ||
| import { subscribe, unsubscribe } from 'node:diagnostics_channel'; | ||
| import { errorMonitor } from 'node:events'; | ||
| import type * as http from 'node:http'; | ||
| import type * as https from 'node:https'; | ||
| import { context } from '@opentelemetry/api'; | ||
| import { context, SpanStatusCode, trace } from '@opentelemetry/api'; | ||
| import { isTracingSuppressed } from '@opentelemetry/core'; | ||
| import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; | ||
| import { InstrumentationBase, InstrumentationNodeModuleDefinition } from '@opentelemetry/instrumentation'; | ||
| import type { Span } from '@sentry/core'; | ||
| import { debug, LRUMap, SDK_VERSION } from '@sentry/core'; | ||
| import { | ||
| ATTR_HTTP_RESPONSE_STATUS_CODE, | ||
| ATTR_NETWORK_PEER_ADDRESS, | ||
| ATTR_NETWORK_PEER_PORT, | ||
| ATTR_NETWORK_PROTOCOL_VERSION, | ||
| ATTR_NETWORK_TRANSPORT, | ||
| ATTR_URL_FULL, | ||
| ATTR_USER_AGENT_ORIGINAL, | ||
| SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH, | ||
| SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH_UNCOMPRESSED, | ||
| } from '@opentelemetry/semantic-conventions'; | ||
| import type { Span, SpanAttributes, SpanStatus } from '@sentry/core'; | ||
| import { | ||
| debug, | ||
| getHttpSpanDetailsFromUrlObject, | ||
| getSpanStatusFromHttpCode, | ||
| LRUMap, | ||
| parseStringToURLObject, | ||
| SDK_VERSION, | ||
| SEMANTIC_ATTRIBUTE_SENTRY_OP, | ||
| startInactiveSpan, | ||
| } from '@sentry/core'; | ||
| import { DEBUG_BUILD } from '../../debug-build'; | ||
| import { getRequestUrl } from '../../utils/getRequestUrl'; | ||
| import { INSTRUMENTATION_NAME } from './constants'; | ||
| import { | ||
| addRequestBreadcrumb, | ||
|
|
@@ -19,6 +39,8 @@ import { | |
|
|
||
| type Http = typeof http; | ||
| type Https = typeof https; | ||
| type IncomingHttpHeaders = http.IncomingHttpHeaders; | ||
| type OutgoingHttpHeaders = http.OutgoingHttpHeaders; | ||
|
|
||
| export type SentryHttpInstrumentationOptions = InstrumentationConfig & { | ||
| /** | ||
|
|
@@ -37,6 +59,27 @@ export type SentryHttpInstrumentationOptions = InstrumentationConfig & { | |
| */ | ||
| propagateTraceInOutgoingRequests?: boolean; | ||
|
|
||
| /** | ||
| * Whether to enable the capability to create spans for outgoing requests via diagnostic channels. | ||
| * This controls whether the instrumentation subscribes to the `http.client.request.start` channel. | ||
| * If enabled, spans will only be created if the `spans` option is also enabled (default: true). | ||
| * | ||
| * This is a feature flag that should be enabled by SDKs when the runtime supports it (Node 22+). | ||
| * Individual users should not need to configure this directly. | ||
| * | ||
| * @default `false` | ||
| */ | ||
| createSpansForOutgoingRequests?: boolean; | ||
|
|
||
| /** | ||
| * Whether to create spans for outgoing requests (user preference). | ||
| * This only takes effect if `createSpansForOutgoingRequests` is also enabled. | ||
| * If `createSpansForOutgoingRequests` is not enabled, this option is ignored. | ||
| * | ||
| * @default `true` | ||
| */ | ||
| spans?: boolean; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: I'm wondering why we need this second option...would someone ever want to set this to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Users with custom OTel setups that add
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh, thanks! |
||
|
|
||
| /** | ||
| * Do not capture breadcrumbs for outgoing HTTP requests to URLs where the given callback returns `true`. | ||
| * For the scope of this instrumentation, this callback only controls breadcrumb creation. | ||
|
|
@@ -51,11 +94,6 @@ export type SentryHttpInstrumentationOptions = InstrumentationConfig & { | |
| // All options below do not do anything anymore in this instrumentation, and will be removed in the future. | ||
| // They are only kept here for backwards compatibility - the respective functionality is now handled by the httpServerIntegration/httpServerSpansIntegration. | ||
|
|
||
| /** | ||
| * @deprecated This no longer does anything. | ||
| */ | ||
| spans?: boolean; | ||
|
|
||
| /** | ||
| * @depreacted This no longer does anything. | ||
| */ | ||
|
|
@@ -111,14 +149,17 @@ export type SentryHttpInstrumentationOptions = InstrumentationConfig & { | |
| }; | ||
|
|
||
| /** | ||
| * This custom HTTP instrumentation is used to isolate incoming requests and annotate them with additional information. | ||
| * It does not emit any spans. | ||
| * This custom HTTP instrumentation handles outgoing HTTP requests. | ||
| * | ||
| * It provides: | ||
| * - Breadcrumbs for all outgoing requests | ||
| * - Trace propagation headers (when enabled) | ||
| * - Span creation for outgoing requests (when createSpansForOutgoingRequests is enabled) | ||
| * | ||
| * The reason this is isolated from the OpenTelemetry instrumentation is that users may overwrite this, | ||
| * which would lead to Sentry not working as expected. | ||
| * Span creation requires Node 22+ and uses diagnostic channels to avoid monkey-patching. | ||
| * By default, this is only enabled in the node SDK, not in node-core or other runtime SDKs. | ||
| * | ||
| * Important note: Contrary to other OTEL instrumentation, this one cannot be unwrapped. | ||
| * It only does minimal things though and does not emit any spans. | ||
| * | ||
| * This is heavily inspired & adapted from: | ||
| * https://github.com/open-telemetry/opentelemetry-js/blob/f8ab5592ddea5cba0a3b33bf8d74f27872c0367f/experimental/packages/opentelemetry-instrumentation-http/src/http.ts | ||
|
|
@@ -155,6 +196,11 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns | |
| this._onOutgoingRequestCreated(data.request); | ||
| }) satisfies ChannelListener; | ||
|
|
||
| const onHttpClientRequestStart = ((_data: unknown) => { | ||
| const data = _data as { request: http.ClientRequest }; | ||
| this._onOutgoingRequestStart(data.request); | ||
| }) satisfies ChannelListener; | ||
|
|
||
| const wrap = <T extends Http | Https>(moduleExports: T): T => { | ||
| if (hasRegisteredHandlers) { | ||
| return moduleExports; | ||
|
|
@@ -168,20 +214,23 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns | |
| // In this case, `http.client.response.finish` is not triggered | ||
| subscribe('http.client.request.error', onHttpClientRequestError); | ||
|
|
||
| if (this.getConfig().createSpansForOutgoingRequests) { | ||
|
||
| subscribe('http.client.request.start', onHttpClientRequestStart); | ||
| } | ||
cursor[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // NOTE: This channel only exist since Node 22 | ||
| // Before that, outgoing requests are not patched | ||
| // and trace headers are not propagated, sadly. | ||
| if (this.getConfig().propagateTraceInOutgoingRequests) { | ||
| subscribe('http.client.request.created', onHttpClientRequestCreated); | ||
| } | ||
|
|
||
| return moduleExports; | ||
| }; | ||
|
|
||
| const unwrap = (): void => { | ||
| unsubscribe('http.client.response.finish', onHttpClientResponseFinish); | ||
| unsubscribe('http.client.request.error', onHttpClientRequestError); | ||
| unsubscribe('http.client.request.created', onHttpClientRequestCreated); | ||
| unsubscribe('http.client.request.start', onHttpClientRequestStart); | ||
| }; | ||
|
|
||
| /** | ||
|
|
@@ -198,6 +247,115 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns | |
| ]; | ||
| } | ||
|
|
||
| /** | ||
| * This is triggered when an outgoing request starts. | ||
| * It has access to the request object, and can mutate it before the request is sent. | ||
| */ | ||
| private _onOutgoingRequestStart(request: http.ClientRequest): void { | ||
| DEBUG_BUILD && debug.log(INSTRUMENTATION_NAME, 'Handling started outgoing request'); | ||
|
|
||
| const spansEnabled = this.getConfig().spans ?? true; | ||
|
|
||
| const shouldIgnore = this._ignoreOutgoingRequestsMap.get(request) ?? this._shouldIgnoreOutgoingRequest(request); | ||
| this._ignoreOutgoingRequestsMap.set(request, shouldIgnore); | ||
|
|
||
| if (spansEnabled && !shouldIgnore) { | ||
| this._startSpanForOutgoingRequest(request); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Start a span for an outgoing request. | ||
| * The span wraps the callback of the request, and ends when the response is finished. | ||
| */ | ||
| private _startSpanForOutgoingRequest(request: http.ClientRequest): void { | ||
| // We monkey-patch `req.once('response'), which is used to trigger the callback of the request | ||
| // eslint-disable-next-line @typescript-eslint/unbound-method, deprecation/deprecation | ||
| const originalOnce = request.once; | ||
|
|
||
| const [name, attributes] = _getOutgoingRequestSpanData(request); | ||
|
|
||
| const span = startInactiveSpan({ | ||
| name, | ||
| attributes, | ||
| onlyIfParent: true, | ||
| }); | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| const newOnce = new Proxy(originalOnce, { | ||
| apply(target, thisArg, args: Parameters<typeof originalOnce>) { | ||
| const [event] = args; | ||
| if (event !== 'response') { | ||
| return target.apply(thisArg, args); | ||
| } | ||
|
|
||
| const parentContext = context.active(); | ||
| const requestContext = trace.setSpan(parentContext, span); | ||
|
|
||
| return context.with(requestContext, () => { | ||
| return target.apply(thisArg, args); | ||
| }); | ||
| }, | ||
| }); | ||
|
|
||
| // eslint-disable-next-line deprecation/deprecation | ||
| request.once = newOnce; | ||
|
|
||
| /** | ||
| * Determines if the request has errored or the response has ended/errored. | ||
| */ | ||
| let responseFinished = false; | ||
|
|
||
| const endSpan = (status: SpanStatus): void => { | ||
| if (responseFinished) { | ||
| return; | ||
| } | ||
| responseFinished = true; | ||
|
|
||
| span.setStatus(status); | ||
| span.end(); | ||
| }; | ||
|
|
||
| request.prependListener('response', response => { | ||
| if (request.listenerCount('response') <= 1) { | ||
| response.resume(); | ||
| } | ||
|
|
||
| context.bind(context.active(), response); | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| const additionalAttributes = _getOutgoingRequestEndedSpanData(response); | ||
| span.setAttributes(additionalAttributes); | ||
|
|
||
| const endHandler = (forceError: boolean = false): void => { | ||
| this._diag.debug('outgoingRequest on end()'); | ||
|
|
||
| const status = | ||
| // eslint-disable-next-line deprecation/deprecation | ||
| forceError || typeof response.statusCode !== 'number' || (response.aborted && !response.complete) | ||
| ? { code: SpanStatusCode.ERROR } | ||
| : getSpanStatusFromHttpCode(response.statusCode); | ||
|
|
||
| endSpan(status); | ||
| }; | ||
|
|
||
| response.on('end', () => { | ||
| endHandler(); | ||
| }); | ||
| response.on(errorMonitor, error => { | ||
| this._diag.debug('outgoingRequest on response error()', error); | ||
| endHandler(true); | ||
| }); | ||
| }); | ||
|
|
||
| // Fallback if proper response end handling above fails | ||
| request.on('close', () => { | ||
| endSpan({ code: SpanStatusCode.UNSET }); | ||
| }); | ||
| request.on(errorMonitor, error => { | ||
| this._diag.debug('outgoingRequest on request error()', error); | ||
| endSpan({ code: SpanStatusCode.ERROR }); | ||
| }); | ||
| } | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing integration or E2E test for new featureLow Severity This is a Additional Locations (1)Triggered by project rule: PR Review Guidelines for Cursor Bot |
||
|
|
||
| /** | ||
| * This is triggered when an outgoing request finishes. | ||
| * It has access to the final request and response objects. | ||
|
|
@@ -251,3 +409,103 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns | |
| return ignoreOutgoingRequests(url, options); | ||
| } | ||
| } | ||
|
|
||
| function _getOutgoingRequestSpanData(request: http.ClientRequest): [string, SpanAttributes] { | ||
| const url = getRequestUrl(request); | ||
|
|
||
| const [name, attributes] = getHttpSpanDetailsFromUrlObject( | ||
| parseStringToURLObject(url), | ||
| 'client', | ||
| 'auto.http.otel.http', | ||
| request, | ||
| ); | ||
|
|
||
| const userAgent = request.getHeader('user-agent'); | ||
|
|
||
| return [ | ||
| name, | ||
| { | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.client', | ||
| 'otel.kind': 'CLIENT', | ||
| [ATTR_USER_AGENT_ORIGINAL]: userAgent, | ||
| [ATTR_URL_FULL]: url, | ||
| 'http.url': url, | ||
| 'http.method': request.method, | ||
| 'http.target': request.path || '/', | ||
| 'net.peer.name': request.host, | ||
| 'http.host': request.getHeader('host'), | ||
| ...attributes, | ||
| }, | ||
| ]; | ||
| } | ||
|
|
||
| function getRequestUrl(request: http.ClientRequest): string { | ||
| const hostname = request.getHeader('host') || request.host; | ||
| const protocol = request.protocol; | ||
| const path = request.path; | ||
|
|
||
| return `${protocol}//${hostname}${path}`; | ||
| } | ||
cursor[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| function _getOutgoingRequestEndedSpanData(response: http.IncomingMessage): SpanAttributes { | ||
| const { statusCode, statusMessage, httpVersion, socket } = response; | ||
|
|
||
| const transport = httpVersion.toUpperCase() !== 'QUIC' ? 'ip_tcp' : 'ip_udp'; | ||
|
|
||
| const additionalAttributes: SpanAttributes = { | ||
| [ATTR_HTTP_RESPONSE_STATUS_CODE]: statusCode, | ||
| [ATTR_NETWORK_PROTOCOL_VERSION]: httpVersion, | ||
| 'http.flavor': httpVersion, | ||
| [ATTR_NETWORK_TRANSPORT]: transport, | ||
| 'net.transport': transport, | ||
| ['http.status_text']: statusMessage?.toUpperCase(), | ||
| 'http.status_code': statusCode, | ||
| ...getResponseContentLengthAttributes(response), | ||
| }; | ||
|
|
||
| if (socket) { | ||
| const { remoteAddress, remotePort } = socket; | ||
|
|
||
| additionalAttributes[ATTR_NETWORK_PEER_ADDRESS] = remoteAddress; | ||
| additionalAttributes[ATTR_NETWORK_PEER_PORT] = remotePort; | ||
| additionalAttributes['net.peer.ip'] = remoteAddress; | ||
| additionalAttributes['net.peer.port'] = remotePort; | ||
| } | ||
|
|
||
| return additionalAttributes; | ||
| } | ||
|
|
||
| function getResponseContentLengthAttributes(response: http.IncomingMessage): SpanAttributes { | ||
| const length = getContentLength(response.headers); | ||
| if (length == null) { | ||
| return {}; | ||
| } | ||
|
|
||
| if (isCompressed(response.headers)) { | ||
| // eslint-disable-next-line deprecation/deprecation | ||
| return { [SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH]: length }; | ||
| } else { | ||
| // eslint-disable-next-line deprecation/deprecation | ||
| return { [SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH_UNCOMPRESSED]: length }; | ||
| } | ||
| } | ||
|
|
||
| function getContentLength(headers: http.OutgoingHttpHeaders): number | undefined { | ||
| const contentLengthHeader = headers['content-length']; | ||
| if (typeof contentLengthHeader !== 'string') { | ||
| return contentLengthHeader; | ||
| } | ||
sentry[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| const contentLength = parseInt(contentLengthHeader, 10); | ||
| if (isNaN(contentLength)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return contentLength; | ||
| } | ||
|
|
||
| function isCompressed(headers: OutgoingHttpHeaders | IncomingHttpHeaders): boolean { | ||
| const encoding = headers['content-encoding']; | ||
|
|
||
| return !!encoding && encoding !== 'identity'; | ||
| } | ||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment sound very directed to us as SDK maintainers. As this comment is public-facing I would write that a bit differently as it can be confusing how to act on this as a user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a suggestion? The comment already calls out individual users should not set this.