From fb32963ee408579468184b2476bcac3136e022b6 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 7 May 2025 16:35:00 +0200 Subject: [PATCH 1/5] WIP add test --- .../http-no-tracing-no-spans/instrument.mjs | 17 +++ .../http-no-tracing-no-spans/scenario.mjs | 43 ++++++++ .../requests/http-no-tracing-no-spans/test.ts | 102 ++++++++++++++++++ 3 files changed, 162 insertions(+) create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/instrument.mjs create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/scenario.mjs create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/instrument.mjs new file mode 100644 index 000000000000..9f713557b30a --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/instrument.mjs @@ -0,0 +1,17 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracePropagationTargets: [/\/v0/, 'v1'], + integrations: [Sentry.httpIntegration({ spans: false })], + transport: loggingTransport, + // Ensure this gets a correct hint + beforeBreadcrumb(breadcrumb, hint) { + breadcrumb.data = breadcrumb.data || {}; + const req = hint?.request; + breadcrumb.data.ADDED_PATH = req?.path; + return breadcrumb; + }, +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/scenario.mjs b/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/scenario.mjs new file mode 100644 index 000000000000..2ee57c8651e0 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/scenario.mjs @@ -0,0 +1,43 @@ +import * as Sentry from '@sentry/node'; +import * as http from 'http'; + +async function run() { + Sentry.addBreadcrumb({ message: 'manual breadcrumb' }); + + await makeHttpRequest(`${process.env.SERVER_URL}/api/v0`); + await makeHttpGet(`${process.env.SERVER_URL}/api/v1`); + await makeHttpRequest(`${process.env.SERVER_URL}/api/v2`); + await makeHttpRequest(`${process.env.SERVER_URL}/api/v3`); + + Sentry.captureException(new Error('foo')); +} + +run(); + +function makeHttpRequest(url) { + return new Promise(resolve => { + http + .request(url, httpRes => { + httpRes.on('data', () => { + // we don't care about data + }); + httpRes.on('end', () => { + resolve(); + }); + }) + .end(); + }); +} + +function makeHttpGet(url) { + return new Promise(resolve => { + http.get(url, httpRes => { + httpRes.on('data', () => { + // we don't care about data + }); + httpRes.on('end', () => { + resolve(); + }); + }); + }); +} diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts new file mode 100644 index 000000000000..cb635920442b --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts @@ -0,0 +1,102 @@ +import { describe,expect } from 'vitest'; +import { createEsmAndCjsTests } from '../../../../utils/runner'; +import { createTestServer } from '../../../../utils/server'; + +describe('outgoing http requests with tracing & spans disabled xxx', () => { + createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { + test('outgoing http requests are correctly instrumented with tracing & spans disabled', async () => { + expect.assertions(11); + + const [SERVER_URL, closeTestServer] = await createTestServer() + .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(); + + await createRunner() + .withEnv({ SERVER_URL }) + .ensureNoErrorOutput() + .expect({ + event: { + exception: { + values: [ + { + type: 'Error', + value: 'foo', + }, + ], + }, + breadcrumbs: [ + { + message: 'manual breadcrumb', + timestamp: expect.any(Number), + }, + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v0`, + status_code: 200, + ADDED_PATH: '/api/v0', + }, + timestamp: expect.any(Number), + type: 'http', + }, + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v1`, + status_code: 200, + ADDED_PATH: '/api/v1', + }, + timestamp: expect.any(Number), + type: 'http', + }, + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v2`, + status_code: 200, + ADDED_PATH: '/api/v2', + }, + timestamp: expect.any(Number), + type: 'http', + }, + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v3`, + status_code: 200, + ADDED_PATH: '/api/v3', + }, + timestamp: expect.any(Number), + type: 'http', + }, + ], + }, + }) + .start() + .completed(); + + closeTestServer(); + }); + }); +}); From f314fdbd21f09376254a1b6993b1071b423a9753 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 7 May 2025 16:35:17 +0200 Subject: [PATCH 2/5] fix(node): Ensure trace propagation works without spans in Node 23+ --- .../requests/http-no-tracing-no-spans/test.ts | 268 ++++++++++++------ .../http/SentryHttpInstrumentation.ts | 72 +++++ packages/node/src/integrations/http/index.ts | 3 + 3 files changed, 260 insertions(+), 83 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts index cb635920442b..689c1d79591d 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts @@ -1,102 +1,204 @@ -import { describe,expect } from 'vitest'; +import { describe, expect } from 'vitest'; +import { conditionalTest } from '../../../../utils'; import { createEsmAndCjsTests } from '../../../../utils/runner'; import { createTestServer } from '../../../../utils/server'; -describe('outgoing http requests with tracing & spans disabled xxx', () => { +describe('outgoing http requests with tracing & spans disabled', () => { createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { - test('outgoing http requests are correctly instrumented with tracing & spans disabled', async () => { - expect.assertions(11); + conditionalTest({ min: 23 })('node >=23', () => { + test('outgoing http requests are correctly instrumented with tracing & spans disabled', async () => { + expect.assertions(11); - const [SERVER_URL, closeTestServer] = await createTestServer() - .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(); + const [SERVER_URL, closeTestServer] = await createTestServer() + .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(); - await createRunner() - .withEnv({ SERVER_URL }) - .ensureNoErrorOutput() - .expect({ - event: { - exception: { - values: [ + await createRunner() + .withEnv({ SERVER_URL }) + .ensureNoErrorOutput() + .expect({ + event: { + exception: { + values: [ + { + type: 'Error', + value: 'foo', + }, + ], + }, + breadcrumbs: [ + { + message: 'manual breadcrumb', + timestamp: expect.any(Number), + }, + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v0`, + status_code: 200, + ADDED_PATH: '/api/v0', + }, + timestamp: expect.any(Number), + type: 'http', + }, + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v1`, + status_code: 200, + ADDED_PATH: '/api/v1', + }, + timestamp: expect.any(Number), + type: 'http', + }, { - type: 'Error', - value: 'foo', + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v2`, + status_code: 200, + ADDED_PATH: '/api/v2', + }, + timestamp: expect.any(Number), + type: 'http', + }, + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v3`, + status_code: 200, + ADDED_PATH: '/api/v3', + }, + timestamp: expect.any(Number), + type: 'http', }, ], }, - breadcrumbs: [ - { - message: 'manual breadcrumb', - timestamp: expect.any(Number), + }) + .start() + .completed(); + + closeTestServer(); + }); + }); + + // On older node versions, outgoing requests do not get trace-headers injected, sadly + // This is because the necessary diagnostics channel hook is not available yet + conditionalTest({ max: 22 })('node <=22', () => { + test('outgoing http requests generate breadcrumbs correctly with tracing & spans disabled', async () => { + expect.assertions(9); + + const [SERVER_URL, closeTestServer] = await createTestServer() + .get('/api/v0', headers => { + // This is not instrumented, sadly + expect(headers['baggage']).toBeUndefined(); + expect(headers['sentry-trace']).toBeUndefined(); + }) + .get('/api/v1', headers => { + // This is not instrumented, sadly + expect(headers['baggage']).toBeUndefined(); + expect(headers['sentry-trace']).toBeUndefined(); + }) + .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(); + + await createRunner() + .withEnv({ SERVER_URL }) + .ensureNoErrorOutput() + .expect({ + event: { + exception: { + values: [ + { + type: 'Error', + value: 'foo', + }, + ], }, - { - category: 'http', - data: { - 'http.method': 'GET', - url: `${SERVER_URL}/api/v0`, - status_code: 200, - ADDED_PATH: '/api/v0', + breadcrumbs: [ + { + message: 'manual breadcrumb', + timestamp: expect.any(Number), }, - timestamp: expect.any(Number), - type: 'http', - }, - { - category: 'http', - data: { - 'http.method': 'GET', - url: `${SERVER_URL}/api/v1`, - status_code: 200, - ADDED_PATH: '/api/v1', + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v0`, + status_code: 200, + ADDED_PATH: '/api/v0', + }, + timestamp: expect.any(Number), + type: 'http', }, - timestamp: expect.any(Number), - type: 'http', - }, - { - category: 'http', - data: { - 'http.method': 'GET', - url: `${SERVER_URL}/api/v2`, - status_code: 200, - ADDED_PATH: '/api/v2', + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v1`, + status_code: 200, + ADDED_PATH: '/api/v1', + }, + timestamp: expect.any(Number), + type: 'http', }, - timestamp: expect.any(Number), - type: 'http', - }, - { - category: 'http', - data: { - 'http.method': 'GET', - url: `${SERVER_URL}/api/v3`, - status_code: 200, - ADDED_PATH: '/api/v3', + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v2`, + status_code: 200, + ADDED_PATH: '/api/v2', + }, + timestamp: expect.any(Number), + type: 'http', }, - timestamp: expect.any(Number), - type: 'http', - }, - ], - }, - }) - .start() - .completed(); + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v3`, + status_code: 200, + ADDED_PATH: '/api/v3', + }, + timestamp: expect.any(Number), + type: 'http', + }, + ], + }, + }) + .start() + .completed(); - closeTestServer(); + closeTestServer(); + }); }); }); }); diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index 4e044879d2aa..260f040f18da 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -18,13 +18,17 @@ import { getCurrentScope, getIsolationScope, getSanitizedUrlString, + getTraceData, httpRequestToRequestData, logger, + LRUMap, parseUrl, stripUrlQueryAndFragment, withIsolationScope, } from '@sentry/core'; +import { shouldPropagateTraceForUrl } from '@sentry/opentelemetry'; import { DEBUG_BUILD } from '../../debug-build'; +import { mergeBaggageHeaders } from '../../utils/baggage'; import { getRequestUrl } from '../../utils/getRequestUrl'; const INSTRUMENTATION_NAME = '@sentry/instrumentation-http'; @@ -49,6 +53,15 @@ export type SentryHttpInstrumentationOptions = InstrumentationConfig & { */ extractIncomingTraceFromHeader?: boolean; + /** + * Whether to propagate Sentry trace headers in ougoing requests. + * By default this is done by the HttpInstrumentation, but if that is not added (e.g. because tracing is disabled, ...) + * then this instrumentation can take over. + * + * @default `false` + */ + propagateTraceInOutgoingRequests?: boolean; + /** * 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. @@ -102,8 +115,12 @@ const MAX_BODY_BYTE_LENGTH = 1024 * 1024; * https://github.com/open-telemetry/opentelemetry-js/blob/f8ab5592ddea5cba0a3b33bf8d74f27872c0367f/experimental/packages/opentelemetry-instrumentation-http/src/http.ts */ export class SentryHttpInstrumentation extends InstrumentationBase { + private _propagationDecisionMap: LRUMap; + public constructor(config: SentryHttpInstrumentationOptions = {}) { super(INSTRUMENTATION_NAME, VERSION, config); + + this._propagationDecisionMap = new LRUMap(100); } /** @inheritdoc */ @@ -127,6 +144,11 @@ export class SentryHttpInstrumentation extends InstrumentationBase { + const data = _data as { request: http.ClientRequest }; + this._onOutgoingRequestCreated(data.request); + }) satisfies ChannelListener; + /** * You may be wondering why we register these diagnostics-channel listeners * in such a convoluted way (as InstrumentationNodeModuleDefinition...)˝, @@ -153,12 +175,19 @@ export class SentryHttpInstrumentation extends InstrumentationBase { unsubscribe('http.server.request.start', onHttpServerRequestStart); unsubscribe('http.client.response.finish', onHttpClientResponseFinish); unsubscribe('http.client.request.error', onHttpClientRequestError); + unsubscribe('http.client.request.created', onHttpClientRequestCreated); }, ), new InstrumentationNodeModuleDefinition( @@ -209,6 +238,49 @@ export class SentryHttpInstrumentation extends InstrumentationBase // 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, + // If spans are not instrumented, it means the HttpInstrumentation has not been added + // In that case, we want to handle trace propagation ourselves + propagateTraceInOutgoingRequests: !instrumentSpans, }); // This is the "regular" OTEL instrumentation that emits spans From cd388a6001a858b353b263be930a4ed27e72c988 Mon Sep 17 00:00:00 2001 From: Francesco Gringl-Novy Date: Fri, 9 May 2025 09:11:20 +0200 Subject: [PATCH 3/5] Apply suggestions from code review Co-authored-by: Abhijeet Prasad --- .../node/src/integrations/http/SentryHttpInstrumentation.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index 260f040f18da..6b46958e7829 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -54,8 +54,8 @@ export type SentryHttpInstrumentationOptions = InstrumentationConfig & { extractIncomingTraceFromHeader?: boolean; /** - * Whether to propagate Sentry trace headers in ougoing requests. - * By default this is done by the HttpInstrumentation, but if that is not added (e.g. because tracing is disabled, ...) + * Whether to propagate Sentry trace headers in outgoing requests. + * By default this is done by the HttpInstrumentation, but if that is not added (e.g. because tracing is disabled) * then this instrumentation can take over. * * @default `false` From 4825f47489596ceb32deb98d4de6a21cdef29403 Mon Sep 17 00:00:00 2001 From: Francesco Gringl-Novy Date: Mon, 12 May 2025 12:36:47 +0200 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com> --- .../node/src/integrations/http/SentryHttpInstrumentation.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index 6b46958e7829..6b8f615479e4 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -176,7 +176,8 @@ export class SentryHttpInstrumentation extends InstrumentationBase Date: Mon, 12 May 2025 13:27:37 +0200 Subject: [PATCH 5/5] works on node 22 --- .../suites/tracing/requests/http-no-tracing-no-spans/test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts index 689c1d79591d..41f688178d1c 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts @@ -5,7 +5,7 @@ import { createTestServer } from '../../../../utils/server'; describe('outgoing http requests with tracing & spans disabled', () => { createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { - conditionalTest({ min: 23 })('node >=23', () => { + conditionalTest({ min: 22 })('node >=22', () => { test('outgoing http requests are correctly instrumented with tracing & spans disabled', async () => { expect.assertions(11); @@ -104,7 +104,7 @@ describe('outgoing http requests with tracing & spans disabled', () => { // On older node versions, outgoing requests do not get trace-headers injected, sadly // This is because the necessary diagnostics channel hook is not available yet - conditionalTest({ max: 22 })('node <=22', () => { + conditionalTest({ max: 21 })('node <22', () => { test('outgoing http requests generate breadcrumbs correctly with tracing & spans disabled', async () => { expect.assertions(9);