From 5179f09e7d1f06ea9507813df75df7496fc54bf3 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 30 Jan 2025 09:03:49 +0100 Subject: [PATCH 1/8] ref: move to folder --- .../node/src/integrations/{node-fetch.ts => node-fetch/index.ts} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename packages/node/src/integrations/{node-fetch.ts => node-fetch/index.ts} (100%) diff --git a/packages/node/src/integrations/node-fetch.ts b/packages/node/src/integrations/node-fetch/index.ts similarity index 100% rename from packages/node/src/integrations/node-fetch.ts rename to packages/node/src/integrations/node-fetch/index.ts From 2459be65f4282831d79b65fd86ed6618e63d41a7 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 30 Jan 2025 10:27:37 +0100 Subject: [PATCH 2/8] feat(node): Extract Sentry-specific node-fetch instrumentation To allow users to opt-out of using the otel instrumentation. --- .../fetch-no-tracing-no-spans/scenario.ts | 24 ++ .../fetch-no-tracing-no-spans/test.ts | 47 ++++ .../SentryNodeFetchInstrumentation.ts | 266 ++++++++++++++++++ .../node/src/integrations/node-fetch/index.ts | 182 +++++------- .../node/src/integrations/node-fetch/types.ts | 47 ++++ packages/node/src/nodeVersion.ts | 1 + packages/opentelemetry/src/propagator.ts | 10 + 7 files changed, 463 insertions(+), 114 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/fetch-no-tracing-no-spans/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/fetch-no-tracing-no-spans/test.ts create mode 100644 packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts create mode 100644 packages/node/src/integrations/node-fetch/types.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-no-tracing-no-spans/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-no-tracing-no-spans/scenario.ts new file mode 100644 index 000000000000..8c2ed31ee1f8 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-no-tracing-no-spans/scenario.ts @@ -0,0 +1,24 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracePropagationTargets: [/\/v0/, 'v1'], + integrations: [Sentry.nativeNodeFetchIntegration({ spans: false })], + transport: loggingTransport, +}); + +async function run(): Promise { + // Since fetch is lazy loaded, we need to wait a bit until it's fully instrumented + await new Promise(resolve => setTimeout(resolve, 100)); + await fetch(`${process.env.SERVER_URL}/api/v0`).then(res => res.text()); + await fetch(`${process.env.SERVER_URL}/api/v1`).then(res => res.text()); + await fetch(`${process.env.SERVER_URL}/api/v2`).then(res => res.text()); + await fetch(`${process.env.SERVER_URL}/api/v3`).then(res => res.text()); + + Sentry.captureException(new Error('foo')); +} + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-no-tracing-no-spans/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-no-tracing-no-spans/test.ts new file mode 100644 index 000000000000..98fc6bd38c52 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-no-tracing-no-spans/test.ts @@ -0,0 +1,47 @@ +import { createRunner } from '../../../../utils/runner'; +import { createTestServer } from '../../../../utils/server'; + +describe('outgoing fetch', () => { + test('outgoing fetch requests are correctly instrumented with tracing & spans are disabled', done => { + expect.assertions(11); + + createTestServer(done) + .get('/api/v0', headers => { + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/)); + expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000'); + expect(headers['baggage']).toEqual(expect.any(String)); + }) + .get('/api/v1', headers => { + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/)); + expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000'); + expect(headers['baggage']).toEqual(expect.any(String)); + }) + .get('/api/v2', headers => { + expect(headers['baggage']).toBeUndefined(); + expect(headers['sentry-trace']).toBeUndefined(); + }) + .get('/api/v3', headers => { + expect(headers['baggage']).toBeUndefined(); + expect(headers['sentry-trace']).toBeUndefined(); + }) + .start() + .then(([SERVER_URL, closeTestServer]) => { + createRunner(__dirname, 'scenario.ts') + .withEnv({ SERVER_URL }) + .ensureNoErrorOutput() + .expect({ + event: { + exception: { + values: [ + { + type: 'Error', + value: 'foo', + }, + ], + }, + }, + }) + .start(closeTestServer); + }); + }); +}); diff --git a/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts b/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts new file mode 100644 index 000000000000..f6787a9126d2 --- /dev/null +++ b/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts @@ -0,0 +1,266 @@ +import { VERSION } from '@opentelemetry/core'; +import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; +import { InstrumentationBase } from '@opentelemetry/instrumentation'; +import type { SanitizedRequestData } from '@sentry/core'; +import { LRUMap, getClient, getTraceData } from '@sentry/core'; +import { addBreadcrumb, getBreadcrumbLogLevelFromHttpStatusCode, getSanitizedUrlString, parseUrl } from '@sentry/core'; +import { shouldPropagateTraceForUrl } from '@sentry/opentelemetry'; +import * as diagch from 'diagnostics_channel'; +import { NODE_MAJOR, NODE_MINOR } from '../../nodeVersion'; +import type { UndiciRequest, UndiciResponse } from './types'; + +export type SentryNodeFetchInstrumentationOptions = InstrumentationConfig & { + /** + * Whether breadcrumbs should be recorded for requests. + * + * @default `true` + */ + breadcrumbs?: boolean; + + /** + * Do not capture breadcrumbs for outgoing fetch requests to URLs where the given callback returns `true`. + * For the scope of this instrumentation, this callback only controls breadcrumb creation. + * The same option can be passed to the top-level httpIntegration where it controls both, breadcrumb and + * span creation. + * + * @param url Contains the entire URL, including query string (if any), protocol, host, etc. of the outgoing request. + */ + ignoreOutgoingRequests?: (url: string) => boolean; +}; + +interface ListenerRecord { + name: string; + unsubscribe: () => void; +} + +/** + * This custom node-fetch instrumentation is used to instrument outgoing fetch requests. + * It does not emit any spans. + * + * The reason this is isolated from the OpenTelemetry instrumentation is that users may overwrite this, + * which would lead to Sentry not working as expected. + * + * This is heavily inspired & adapted from: + * https://github.com/open-telemetry/opentelemetry-js-contrib/blob/28e209a9da36bc4e1f8c2b0db7360170ed46cb80/plugins/node/instrumentation-undici/src/undici.ts + */ +export class SentryNodeFetchInstrumentation extends InstrumentationBase { + // Keep ref to avoid https://github.com/nodejs/node/issues/42170 bug and for + // unsubscribing. + private _channelSubs: Array; + private _propagationDecisionMap: LRUMap; + + public constructor(config: SentryNodeFetchInstrumentationOptions = {}) { + super('@sentry/instrumentation-node-fetch', VERSION, config); + this._channelSubs = []; + this._propagationDecisionMap = new LRUMap(100); + } + + /** No need to instrument files/modules. */ + public init(): void { + return undefined; + } + + /** Disable the instrumentation. */ + public disable(): void { + super.disable(); + this._channelSubs.forEach(sub => sub.unsubscribe()); + this._channelSubs = []; + } + + /** Enable the instrumentation. */ + public enable(): void { + // "enabled" handling is currently a bit messy with InstrumentationBase. + // If constructed with `{enabled: false}`, this `.enable()` is still called, + // and `this.getConfig().enabled !== this.isEnabled()`, creating confusion. + // + // For now, this class will setup for instrumenting if `.enable()` is + // called, but use `this.getConfig().enabled` to determine if + // instrumentation should be generated. This covers the more likely common + // case of config being given a construction time, rather than later via + // `instance.enable()`, `.disable()`, or `.setConfig()` calls. + super.enable(); + + // This method is called by the super-class constructor before ours is + // called. So we need to ensure the property is initalized. + this._channelSubs = this._channelSubs || []; + + // Avoid to duplicate subscriptions + if (this._channelSubs.length > 0) { + return; + } + + this._subscribeToChannel('undici:request:create', this._onRequestCreated.bind(this)); + this._subscribeToChannel('undici:request:headers', this._onResponseHeaders.bind(this)); + } + + /** + * This method is called when a request is created. + * You can still mutate the request here before it is sent. + */ + private _onRequestCreated({ request }: { request: UndiciRequest }): void { + const config = this.getConfig(); + const enabled = config.enabled !== false; + + if (!enabled) { + return; + } + + // Add trace propagation headers + const url = getAbsoluteUrl(request.origin, request.path); + const _ignoreOutgoingRequests = config.ignoreOutgoingRequests; + const shouldIgnore = _ignoreOutgoingRequests && url && _ignoreOutgoingRequests(url); + + if (shouldIgnore) { + return; + } + + // Manually add the trace headers, if it applies + // Note: We do not use `propagation.inject()` here, because our propagator relies on an active span + // Which we do not have in this case + // The propagator _may_ overwrite this, but this should be fine as it is the same data + const tracePropagationTargets = getClient()?.getOptions().tracePropagationTargets; + const addedHeaders = shouldPropagateTraceForUrl(url, tracePropagationTargets, this._propagationDecisionMap) + ? getTraceData() + : {}; + + // We do not want to overwrite existing headers here + // If the core UndiciInstrumentation is registered, it will already have set the headers + // We do not want to add any then + if (Array.isArray(request.headers)) { + const requestHeaders = request.headers; + Object.entries(addedHeaders) + .filter(([k]) => { + // If the header already exists, we do not want to set it again + return !requestHeaders.includes(`${k}:`); + }) + .forEach(headers => requestHeaders.push(...headers)); + } else { + const requestHeaders = request.headers; + request.headers += Object.entries(addedHeaders) + .filter(([k]) => { + // If the header already exists, we do not want to set it again + return !requestHeaders.includes(`${k}:`); + }) + .map(([k, v]) => `${k}: ${v}\r\n`) + .join(''); + } + } + + /** + * This method is called when a response is received. + */ + private _onResponseHeaders({ request, response }: { request: UndiciRequest; response: UndiciResponse }): void { + const config = this.getConfig(); + const enabled = config.enabled !== false; + + if (!enabled) { + return; + } + + const _breadcrumbs = config.breadcrumbs; + const breadCrumbsEnabled = typeof _breadcrumbs === 'undefined' ? true : _breadcrumbs; + + const _ignoreOutgoingRequests = config.ignoreOutgoingRequests; + const shouldCreateBreadcrumb = + typeof _ignoreOutgoingRequests === 'function' + ? !_ignoreOutgoingRequests(getAbsoluteUrl(request.origin, request.path)) + : true; + + if (breadCrumbsEnabled && shouldCreateBreadcrumb) { + addRequestBreadcrumb(request, response); + } + } + + /** Subscribe to a diagnostics channel. */ + private _subscribeToChannel( + diagnosticChannel: string, + onMessage: (message: unknown, name: string | symbol) => void, + ): void { + // `diagnostics_channel` had a ref counting bug until v18.19.0. + // https://github.com/nodejs/node/pull/47520 + const useNewSubscribe = NODE_MAJOR > 18 || (NODE_MAJOR === 18 && NODE_MINOR >= 19); + + let unsubscribe: () => void; + if (useNewSubscribe) { + diagch.subscribe?.(diagnosticChannel, onMessage); + unsubscribe = () => diagch.unsubscribe?.(diagnosticChannel, onMessage); + } else { + const channel = diagch.channel(diagnosticChannel); + channel.subscribe(onMessage); + unsubscribe = () => channel.unsubscribe(onMessage); + } + + this._channelSubs.push({ + name: diagnosticChannel, + unsubscribe, + }); + } +} + +/** Add a breadcrumb for outgoing requests. */ +function addRequestBreadcrumb(request: UndiciRequest, response: UndiciResponse): void { + const data = getBreadcrumbData(request); + + const statusCode = response.statusCode; + const level = getBreadcrumbLogLevelFromHttpStatusCode(statusCode); + + addBreadcrumb( + { + category: 'http', + data: { + status_code: statusCode, + ...data, + }, + type: 'http', + level, + }, + { + event: 'response', + request, + response, + }, + ); +} + +function getBreadcrumbData(request: UndiciRequest): Partial { + try { + const url = getAbsoluteUrl(request.origin, request.path); + const parsedUrl = parseUrl(url); + + const data: Partial = { + url: getSanitizedUrlString(parsedUrl), + 'http.method': request.method || 'GET', + }; + + if (parsedUrl.search) { + data['http.query'] = parsedUrl.search; + } + if (parsedUrl.hash) { + data['http.fragment'] = parsedUrl.hash; + } + + return data; + } catch { + return {}; + } +} + +function getAbsoluteUrl(origin: string, path: string = '/'): string { + try { + const url = new URL(path, origin); + return url.toString(); + } catch { + // fallback: Construct it on our own + const url = `${origin}`; + + if (url.endsWith('/') && path.startsWith('/')) { + return `${url}${path.slice(1)}`; + } + + if (!url.endsWith('/') && !path.startsWith('/')) { + return `${url}/${path.slice(1)}`; + } + + return `${url}${path}`; + } +} diff --git a/packages/node/src/integrations/node-fetch/index.ts b/packages/node/src/integrations/node-fetch/index.ts index a4678be7239a..dc0df9b5ef57 100644 --- a/packages/node/src/integrations/node-fetch/index.ts +++ b/packages/node/src/integrations/node-fetch/index.ts @@ -1,20 +1,14 @@ -import { registerInstrumentations } from '@opentelemetry/instrumentation'; -import type { UndiciRequest, UndiciResponse } from '@opentelemetry/instrumentation-undici'; +import type { UndiciInstrumentationConfig } from '@opentelemetry/instrumentation-undici'; import { UndiciInstrumentation } from '@opentelemetry/instrumentation-undici'; -import type { IntegrationFn, SanitizedRequestData } from '@sentry/core'; -import { - LRUMap, - SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, - addBreadcrumb, - defineIntegration, - getBreadcrumbLogLevelFromHttpStatusCode, - getClient, - getSanitizedUrlString, - getTraceData, - hasTracingEnabled, - parseUrl, -} from '@sentry/core'; -import { shouldPropagateTraceForUrl } from '@sentry/opentelemetry'; +import type { IntegrationFn } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, defineIntegration, getClient } from '@sentry/core'; +import { generateInstrumentOnce } from '../../otel/instrument'; +import type { NodeClient } from '../../sdk/client'; +import type { NodeClientOptions } from '../../types'; +import type { SentryNodeFetchInstrumentationOptions } from './SentryNodeFetchInstrumentation'; +import { SentryNodeFetchInstrumentation } from './SentryNodeFetchInstrumentation'; + +const INTEGRATION_NAME = 'NodeFetch'; interface NodeFetchOptions { /** @@ -23,6 +17,15 @@ interface NodeFetchOptions { */ breadcrumbs?: boolean; + /** + * If set to false, do not emit any spans. + * This will ensure that the default UndiciInstrumentation from OpenTelemetry is not setup, + * only the Sentry-specific instrumentation for breadcrumbs & trace propagation is applied. + * + * If `skipOpenTelemetrySetup: true` is configured, this defaults to `false`, otherwise it defaults to `true`. + */ + spans?: boolean; + /** * Do not capture spans or breadcrumbs for outgoing fetch requests to URLs where the given callback returns `true`. * This controls both span & breadcrumb creation - spans will be non recording if tracing is disabled. @@ -30,114 +33,39 @@ interface NodeFetchOptions { ignoreOutgoingRequests?: (url: string) => boolean; } -const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => { - const _breadcrumbs = typeof options.breadcrumbs === 'undefined' ? true : options.breadcrumbs; - const _ignoreOutgoingRequests = options.ignoreOutgoingRequests; +const instrumentOtelNodeFetch = generateInstrumentOnce(INTEGRATION_NAME, config => { + return new UndiciInstrumentation(config); +}); + +const instrumentSentryNodeFetch = generateInstrumentOnce( + `${INTEGRATION_NAME}.sentry`, + config => { + return new SentryNodeFetchInstrumentation(config); + }, +); +const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => { return { name: 'NodeFetch', setupOnce() { - const propagationDecisionMap = new LRUMap(100); - - const instrumentation = new UndiciInstrumentation({ - requireParentforSpans: false, - ignoreRequestHook: request => { - const url = getAbsoluteUrl(request.origin, request.path); - const shouldIgnore = _ignoreOutgoingRequests && url && _ignoreOutgoingRequests(url); - - if (shouldIgnore) { - return true; - } - - // If tracing is disabled, we still want to propagate traces - // So we do that manually here, matching what the instrumentation does otherwise - if (!hasTracingEnabled()) { - const tracePropagationTargets = getClient()?.getOptions().tracePropagationTargets; - const addedHeaders = shouldPropagateTraceForUrl(url, tracePropagationTargets, propagationDecisionMap) - ? getTraceData() - : {}; - - const requestHeaders = request.headers; - if (Array.isArray(requestHeaders)) { - Object.entries(addedHeaders).forEach(headers => requestHeaders.push(...headers)); - } else { - request.headers += Object.entries(addedHeaders) - .map(([k, v]) => `${k}: ${v}\r\n`) - .join(''); - } - - // Prevent starting a span for this request - return true; - } - - return false; - }, - startSpanHook: () => { - return { - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.otel.node_fetch', - }; - }, - responseHook: (_, { request, response }) => { - if (_breadcrumbs) { - addRequestBreadcrumb(request, response); - } - }, - }); - - registerInstrumentations({ instrumentations: [instrumentation] }); + const instrumentSpans = _shouldInstrumentSpans(options, getClient()?.getOptions()); + + // This is the "regular" OTEL instrumentation that emits spans + if (instrumentSpans) { + const instrumentationConfig = getConfigWithDefaults(options); + instrumentOtelNodeFetch(instrumentationConfig); + } + + // This is the Sentry-specific instrumentation that creates breadcrumbs & propagates traces + // This must be registered after the OTEL one, to ensure that the core trace propagation logic takes presedence + // Otherwise, the sentry-trace header may be set multiple times + instrumentSentryNodeFetch(options); }, }; }) satisfies IntegrationFn; export const nativeNodeFetchIntegration = defineIntegration(_nativeNodeFetchIntegration); -/** Add a breadcrumb for outgoing requests. */ -function addRequestBreadcrumb(request: UndiciRequest, response: UndiciResponse): void { - const data = getBreadcrumbData(request); - const statusCode = response.statusCode; - const level = getBreadcrumbLogLevelFromHttpStatusCode(statusCode); - - addBreadcrumb( - { - category: 'http', - data: { - status_code: statusCode, - ...data, - }, - type: 'http', - level, - }, - { - event: 'response', - request, - response, - }, - ); -} - -function getBreadcrumbData(request: UndiciRequest): Partial { - try { - const url = new URL(request.path, request.origin); - const parsedUrl = parseUrl(url.toString()); - - const data: Partial = { - url: getSanitizedUrlString(parsedUrl), - 'http.method': request.method || 'GET', - }; - - if (parsedUrl.search) { - data['http.query'] = parsedUrl.search; - } - if (parsedUrl.hash) { - data['http.fragment'] = parsedUrl.hash; - } - - return data; - } catch { - return {}; - } -} - // Matching the behavior of the base instrumentation function getAbsoluteUrl(origin: string, path: string = '/'): string { const url = `${origin}`; @@ -152,3 +80,29 @@ function getAbsoluteUrl(origin: string, path: string = '/'): string { return `${url}${path}`; } + +function _shouldInstrumentSpans(options: NodeFetchOptions, clientOptions: Partial = {}): boolean { + // If `spans` is passed in, it takes precedence + // Else, we by default emit spans, unless `skipOpenTelemetrySetup` is set to `true` + return typeof options.spans === 'boolean' ? options.spans : !clientOptions.skipOpenTelemetrySetup; +} + +function getConfigWithDefaults(options: Partial = {}): UndiciInstrumentationConfig { + const instrumentationConfig = { + requireParentforSpans: false, + ignoreRequestHook: request => { + const url = getAbsoluteUrl(request.origin, request.path); + const _ignoreOutgoingRequests = options.ignoreOutgoingRequests; + const shouldIgnore = _ignoreOutgoingRequests && url && _ignoreOutgoingRequests(url); + + return !!shouldIgnore; + }, + startSpanHook: () => { + return { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.otel.node_fetch', + }; + }, + } satisfies UndiciInstrumentationConfig; + + return instrumentationConfig; +} diff --git a/packages/node/src/integrations/node-fetch/types.ts b/packages/node/src/integrations/node-fetch/types.ts new file mode 100644 index 000000000000..0139dadde413 --- /dev/null +++ b/packages/node/src/integrations/node-fetch/types.ts @@ -0,0 +1,47 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * Vendored from https://github.com/open-telemetry/opentelemetry-js-contrib/blob/28e209a9da36bc4e1f8c2b0db7360170ed46cb80/plugins/node/instrumentation-undici/src/types.ts + */ + +export interface UndiciRequest { + origin: string; + method: string; + path: string; + /** + * Serialized string of headers in the form `name: value\r\n` for v5 + * Array of strings v6 + */ + headers: string | string[]; + /** + * Helper method to add headers (from v6) + */ + addHeader: (name: string, value: string) => void; + throwOnError: boolean; + completed: boolean; + aborted: boolean; + idempotent: boolean; + contentLength: number | null; + contentType: string | null; + body: unknown; +} + +export interface UndiciResponse { + headers: Buffer[]; + statusCode: number; + statusText: string; +} diff --git a/packages/node/src/nodeVersion.ts b/packages/node/src/nodeVersion.ts index 1ec745743620..86c761543de7 100644 --- a/packages/node/src/nodeVersion.ts +++ b/packages/node/src/nodeVersion.ts @@ -2,3 +2,4 @@ import { parseSemver } from '@sentry/core'; export const NODE_VERSION = parseSemver(process.versions.node) as { major: number; minor: number; patch: number }; export const NODE_MAJOR = NODE_VERSION.major; +export const NODE_MINOR = NODE_VERSION.minor; diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts index f92331d7739f..024b30255f35 100644 --- a/packages/opentelemetry/src/propagator.ts +++ b/packages/opentelemetry/src/propagator.ts @@ -264,6 +264,16 @@ function getExistingBaggage(carrier: unknown): string | undefined { } } +/** Try to get the existing sentry-trace header so we can merge this in. */ +function getExistingSentryTrace(carrier: unknown): string | undefined { + try { + const sentryTrace = (carrier as Record)[SENTRY_TRACE_HEADER]; + return Array.isArray(sentryTrace) ? sentryTrace.join(',') : sentryTrace; + } catch { + return undefined; + } +} + /** * It is pretty tricky to get access to the outgoing request URL of a request in the propagator. * As we only have access to the context of the span to be sent and the carrier (=headers), From 5f735492ca8b78caa6ad1acf02d8e2105051966d Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 30 Jan 2025 14:20:37 +0100 Subject: [PATCH 3/8] fix lint --- packages/opentelemetry/src/propagator.ts | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts index 024b30255f35..f92331d7739f 100644 --- a/packages/opentelemetry/src/propagator.ts +++ b/packages/opentelemetry/src/propagator.ts @@ -264,16 +264,6 @@ function getExistingBaggage(carrier: unknown): string | undefined { } } -/** Try to get the existing sentry-trace header so we can merge this in. */ -function getExistingSentryTrace(carrier: unknown): string | undefined { - try { - const sentryTrace = (carrier as Record)[SENTRY_TRACE_HEADER]; - return Array.isArray(sentryTrace) ? sentryTrace.join(',') : sentryTrace; - } catch { - return undefined; - } -} - /** * It is pretty tricky to get access to the outgoing request URL of a request in the propagator. * As we only have access to the context of the span to be sent and the carrier (=headers), From 522b9c726dccc1b6dafa46f8d489dc9dae97214c Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 30 Jan 2025 14:53:23 +0100 Subject: [PATCH 4/8] small ref --- .../node-fetch/SentryNodeFetchInstrumentation.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts b/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts index f6787a9126d2..df9da513e74e 100644 --- a/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts +++ b/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts @@ -121,7 +121,11 @@ export class SentryNodeFetchInstrumentation extends InstrumentationBase Date: Thu, 30 Jan 2025 14:53:54 +0100 Subject: [PATCH 5/8] wip debug it? --- .../integrations/node-fetch/SentryNodeFetchInstrumentation.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts b/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts index df9da513e74e..c3b6d54bf7f9 100644 --- a/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts +++ b/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts @@ -127,6 +127,8 @@ export class SentryNodeFetchInstrumentation extends InstrumentationBase Date: Thu, 30 Jan 2025 15:05:17 +0100 Subject: [PATCH 6/8] fix it actually --- .../node-fetch/SentryNodeFetchInstrumentation.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts b/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts index c3b6d54bf7f9..987b32fd5af5 100644 --- a/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts +++ b/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts @@ -127,8 +127,6 @@ export class SentryNodeFetchInstrumentation extends InstrumentationBase { // If the header already exists, we do not want to set it again - return !requestHeaders.includes(`${k}:`); + return !requestHeaders.includes(k); }) - .forEach(headers => requestHeaders.push(...headers)); + .forEach(keyValuePair => requestHeaders.push(...keyValuePair)); } else { const requestHeaders = request.headers; request.headers += Object.entries(addedHeaders) From 07e8184421e65a0c6d4de02256ece4415f347011 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 31 Jan 2025 11:19:35 +0100 Subject: [PATCH 7/8] merge fetch baggage --- .../http/SentryHttpInstrumentation.ts | 29 +--------- .../SentryNodeFetchInstrumentation.ts | 54 +++++++++++++------ packages/node/src/utils/baggage.ts | 31 +++++++++++ 3 files changed, 71 insertions(+), 43 deletions(-) create mode 100644 packages/node/src/utils/baggage.ts diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index 60fec28a434c..7d3f6daeb58d 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -18,8 +18,6 @@ import { getTraceData, httpRequestToRequestData, logger, - objectToBaggageHeader, - parseBaggageHeader, parseUrl, stripUrlQueryAndFragment, withIsolationScope, @@ -27,6 +25,7 @@ import { } from '@sentry/core'; import { shouldPropagateTraceForUrl } from '@sentry/opentelemetry'; import { DEBUG_BUILD } from '../../debug-build'; +import { mergeBaggageHeaders } from '../../utils/baggage'; import { getRequestUrl } from '../../utils/getRequestUrl'; import { getRequestInfo } from './vendor/getRequestInfo'; @@ -602,29 +601,3 @@ function getAbsoluteUrl(origin: string, path: string = '/'): string { return `${url}${path}`; } } - -function mergeBaggageHeaders( - existing: string | string[] | number | undefined, - baggage: string, -): string | string[] | number | undefined { - if (!existing) { - return baggage; - } - - const existingBaggageEntries = parseBaggageHeader(existing); - const newBaggageEntries = parseBaggageHeader(baggage); - - if (!newBaggageEntries) { - return existing; - } - - // Existing entries take precedence, ensuring order remains stable for minimal changes - const mergedBaggageEntries = { ...existingBaggageEntries }; - Object.entries(newBaggageEntries).forEach(([key, value]) => { - if (!mergedBaggageEntries[key]) { - mergedBaggageEntries[key] = value; - } - }); - - return objectToBaggageHeader(mergedBaggageEntries); -} diff --git a/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts b/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts index 987b32fd5af5..57d4e4bde947 100644 --- a/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts +++ b/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts @@ -7,8 +7,12 @@ import { addBreadcrumb, getBreadcrumbLogLevelFromHttpStatusCode, getSanitizedUrl import { shouldPropagateTraceForUrl } from '@sentry/opentelemetry'; import * as diagch from 'diagnostics_channel'; import { NODE_MAJOR, NODE_MINOR } from '../../nodeVersion'; +import { mergeBaggageHeaders } from '../../utils/baggage'; import type { UndiciRequest, UndiciResponse } from './types'; +const SENTRY_TRACE_HEADER = 'sentry-trace'; +const SENTRY_BAGGAGE_HEADER = 'baggage'; + export type SentryNodeFetchInstrumentationOptions = InstrumentationConfig & { /** * Whether breadcrumbs should be recorded for requests. @@ -18,8 +22,7 @@ export type SentryNodeFetchInstrumentationOptions = InstrumentationConfig & { breadcrumbs?: boolean; /** - * Do not capture breadcrumbs for outgoing fetch requests to URLs where the given callback returns `true`. - * For the scope of this instrumentation, this callback only controls breadcrumb creation. + * Do not capture breadcrumbs or inject headers for outgoing fetch requests to URLs where the given callback returns `true`. * The same option can be passed to the top-level httpIntegration where it controls both, breadcrumb and * span creation. * @@ -127,26 +130,47 @@ export class SentryNodeFetchInstrumentation extends InstrumentationBase { - // If the header already exists, we do not want to set it again - return !requestHeaders.includes(k); - }) - .forEach(keyValuePair => requestHeaders.push(...keyValuePair)); + + // We do not want to overwrite existing header here, if it was already set + if (sentryTrace && !requestHeaders.includes(SENTRY_TRACE_HEADER)) { + requestHeaders.push(SENTRY_TRACE_HEADER, sentryTrace); + } + + // For baggage, we make sure to merge this into a possibly existing header + const existingBaggagePos = requestHeaders.findIndex(header => header === SENTRY_BAGGAGE_HEADER); + if (baggage && existingBaggagePos === -1) { + requestHeaders.push(SENTRY_BAGGAGE_HEADER, baggage); + } else if (baggage) { + const existingBaggage = requestHeaders[existingBaggagePos + 1]; + const merged = mergeBaggageHeaders(existingBaggage, baggage); + if (merged) { + requestHeaders[existingBaggagePos + 1] = merged; + } + } } else { const requestHeaders = request.headers; - request.headers += Object.entries(addedHeaders) - .filter(([k]) => { - // If the header already exists, we do not want to set it again - return !requestHeaders.includes(`${k}:`); - }) - .map(([k, v]) => `${k}: ${v}\r\n`) - .join(''); + // We do not want to overwrite existing header here, if it was already set + if (sentryTrace && !requestHeaders.includes(`${SENTRY_TRACE_HEADER}:`)) { + request.headers += `${SENTRY_TRACE_HEADER}: ${sentryTrace}\r\n`; + } + + // For baggage, we make sure to merge this into a possibly existing header + const existingBaggage = request.headers.match(/baggage: (.*)\r\n/)?.[1]; + if (baggage && !existingBaggage) { + request.headers += `${SENTRY_BAGGAGE_HEADER}: ${baggage}\r\n`; + } else if (baggage) { + const merged = mergeBaggageHeaders(existingBaggage, baggage); + if (merged) { + request.headers = request.headers.replace(/baggage: (.*)\r\n/, `baggage: ${merged}\r\n`); + } + } } } diff --git a/packages/node/src/utils/baggage.ts b/packages/node/src/utils/baggage.ts new file mode 100644 index 000000000000..be8e62b9497b --- /dev/null +++ b/packages/node/src/utils/baggage.ts @@ -0,0 +1,31 @@ +import { objectToBaggageHeader, parseBaggageHeader } from '@sentry/core'; + +/** + * Merge two baggage headers into one, where the existing one takes precedence. + * The order of the existing baggage will be preserved, and new entries will be added to the end. + */ +export function mergeBaggageHeaders( + existing: Existing, + baggage: string, +): string | undefined | Existing { + if (!existing) { + return baggage; + } + + const existingBaggageEntries = parseBaggageHeader(existing); + const newBaggageEntries = parseBaggageHeader(baggage); + + if (!newBaggageEntries) { + return existing; + } + + // Existing entries take precedence, ensuring order remains stable for minimal changes + const mergedBaggageEntries = { ...existingBaggageEntries }; + Object.entries(newBaggageEntries).forEach(([key, value]) => { + if (!mergedBaggageEntries[key]) { + mergedBaggageEntries[key] = value; + } + }); + + return objectToBaggageHeader(mergedBaggageEntries); +} From 73af3a1ba546b94b26aad11c400b0fa5779d5885 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 31 Jan 2025 11:56:44 +0100 Subject: [PATCH 8/8] small ref --- .../node-fetch/SentryNodeFetchInstrumentation.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts b/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts index 57d4e4bde947..858eb84cf012 100644 --- a/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts +++ b/packages/node/src/integrations/node-fetch/SentryNodeFetchInstrumentation.ts @@ -162,13 +162,14 @@ export class SentryNodeFetchInstrumentation extends InstrumentationBase