From 0b3788e3c86bc3d1f0fbdccef67a05e5c89eb2c1 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 11 Apr 2024 15:56:55 +0200 Subject: [PATCH 01/13] feat(node): Ensure tracing without performance (TWP) works TODO.... --- .../tracing/tracePropagationTargets/test.ts | 12 +++-- .../tracing/tracingNoSampleRate/scenario.ts | 39 ++++++++++++++++ .../tracing/tracingNoSampleRate/test.ts | 46 +++++++++++++++++++ .../tracing/tracingUnsampled/scenario.ts | 40 ++++++++++++++++ .../suites/tracing/tracingUnsampled/test.ts | 45 ++++++++++++++++++ packages/opentelemetry/src/propagator.ts | 5 +- 6 files changed, 180 insertions(+), 7 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/tracingNoSampleRate/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/tracingNoSampleRate/test.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/test.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/tracePropagationTargets/test.ts b/dev-packages/node-integration-tests/suites/tracing/tracePropagationTargets/test.ts index e87e9f3df1bc..c43b5607ef52 100644 --- a/dev-packages/node-integration-tests/suites/tracing/tracePropagationTargets/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/tracePropagationTargets/test.ts @@ -2,16 +2,18 @@ import { createRunner } from '../../../utils/runner'; import { createTestServer } from '../../../utils/server'; test('HttpIntegration should instrument correct requests when tracePropagationTargets option is provided', done => { - expect.assertions(9); + expect.assertions(11); createTestServer(done) .get('/api/v0', headers => { - expect(typeof headers['baggage']).toBe('string'); - expect(typeof headers['sentry-trace']).toBe('string'); + expect(headers['baggage']).toEqual(expect.any(String)); + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/)); + expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-1'); }) .get('/api/v1', headers => { - expect(typeof headers['baggage']).toBe('string'); - expect(typeof headers['sentry-trace']).toBe('string'); + expect(headers['baggage']).toEqual(expect.any(String)); + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/)); + expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-1'); }) .get('/api/v2', headers => { expect(headers['baggage']).toBeUndefined(); diff --git a/dev-packages/node-integration-tests/suites/tracing/tracingNoSampleRate/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/tracingNoSampleRate/scenario.ts new file mode 100644 index 000000000000..8213ddf7034e --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/tracingNoSampleRate/scenario.ts @@ -0,0 +1,39 @@ +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: [], + transport: loggingTransport, +}); + +import * as http from 'http'; + +async function run(): Promise { + await makeHttpRequest(`${process.env.SERVER_URL}/api/v0`); + await makeHttpRequest(`${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')); +} + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +run(); + +function makeHttpRequest(url: string): Promise { + return new Promise(resolve => { + http + .request(url, httpRes => { + httpRes.on('data', () => { + // we don't care about data + }); + httpRes.on('end', () => { + resolve(); + }); + }) + .end(); + }); +} diff --git a/dev-packages/node-integration-tests/suites/tracing/tracingNoSampleRate/test.ts b/dev-packages/node-integration-tests/suites/tracing/tracingNoSampleRate/test.ts new file mode 100644 index 000000000000..f558f15c6d4f --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/tracingNoSampleRate/test.ts @@ -0,0 +1,46 @@ +import { createRunner } from '../../../utils/runner'; +import { createTestServer } from '../../../utils/server'; + +test('HttpIntegration should instrument correct requests even without tracesSampleRate xxx', done => { + expect.assertions(11); + + createTestServer(done) + .get('/api/v0', headers => { + expect(headers['baggage']).toBeUndefined(); + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/)); + expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000'); + }) + .get('/api/v1', headers => { + expect(headers['baggage']).toBeUndefined(); + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/)); + expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000'); + }) + .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 => { + createRunner(__dirname, 'scenario.ts') + .withEnv({ SERVER_URL }) + .ensureNoErrorOutput() + .ignore('session', 'sessions') + .expect({ + event: { + exception: { + values: [ + { + type: 'Error', + value: 'foo', + }, + ], + }, + }, + }) + .start(done); + }); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/scenario.ts new file mode 100644 index 000000000000..3b1bdfff1b39 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/scenario.ts @@ -0,0 +1,40 @@ +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'], + tracesSampleRate: 0, + integrations: [], + transport: loggingTransport, +}); + +import * as http from 'http'; + +async function run(): Promise { + await makeHttpRequest(`${process.env.SERVER_URL}/api/v0`); + await makeHttpRequest(`${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')); +} + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +run(); + +function makeHttpRequest(url: string): Promise { + return new Promise(resolve => { + http + .request(url, httpRes => { + httpRes.on('data', () => { + // we don't care about data + }); + httpRes.on('end', () => { + resolve(); + }); + }) + .end(); + }); +} diff --git a/dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/test.ts b/dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/test.ts new file mode 100644 index 000000000000..b95d2e1254c4 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/test.ts @@ -0,0 +1,45 @@ +import { createRunner } from '../../../utils/runner'; +import { createTestServer } from '../../../utils/server'; + +test('HttpIntegration should instrument correct requests even when not sampled', done => { + expect.assertions(11); + + createTestServer(done) + .get('/api/v0', headers => { + expect(headers['baggage']).toBeUndefined(); + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-0$/)); + expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-0'); + }) + .get('/api/v1', headers => { + expect(headers['baggage']).toBeUndefined(); + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-0$/)); + expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-0'); + }) + .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 => { + createRunner(__dirname, 'scenario.ts') + .withEnv({ SERVER_URL }) + .ignore('session', 'sessions') + .expect({ + event: { + exception: { + values: [ + { + type: 'Error', + value: 'foo', + }, + ], + }, + }, + }) + .start(done); + }); +}); diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts index d96f19c16b7c..77b3b5993199 100644 --- a/packages/opentelemetry/src/propagator.ts +++ b/packages/opentelemetry/src/propagator.ts @@ -3,7 +3,7 @@ import { context } from '@opentelemetry/api'; import { TraceFlags, propagation, trace } from '@opentelemetry/api'; import { TraceState, W3CBaggagePropagator, isTracingSuppressed } from '@opentelemetry/core'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; -import type { continueTrace } from '@sentry/core'; +import { continueTrace, hasTracingEnabled } from '@sentry/core'; import { getRootSpan } from '@sentry/core'; import { spanToJSON } from '@sentry/core'; import { getClient, getCurrentScope, getDynamicSamplingContextFromClient, getIsolationScope } from '@sentry/core'; @@ -212,7 +212,7 @@ function getInjectionData(context: Context): { spanId: string | undefined; sampled: boolean | undefined; } { - const span = trace.getSpan(context); + const span = hasTracingEnabled() ? trace.getSpan(context) : undefined; const spanIsRemote = span?.spanContext().isRemote; // If we have a local span, we can just pick everything from it @@ -231,6 +231,7 @@ function getInjectionData(context: Context): { // Else we try to use the propagation context from the scope const scope = getScopesFromContext(context)?.scope; + if (scope) { const propagationContext = scope.getPropagationContext(); const dynamicSamplingContext = getDynamicSamplingContext(propagationContext, propagationContext.traceId); From 0c6cede0b098e06c44258e43782411b82d2f10a9 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 11 Apr 2024 17:34:56 +0200 Subject: [PATCH 02/13] SUPER HACK: fix it --- .../trace-header-assign/server.ts | 35 +++++++++ .../sentry-trace/trace-header-assign/test.ts | 2 +- .../tracing/tracingNoSampleRate/test.ts | 14 ++-- .../tracing/tracingUnsampled/scenario.ts | 11 ++- .../suites/tracing/tracingUnsampled/test.ts | 4 +- packages/node/src/integrations/http.ts | 21 +++-- packages/opentelemetry/src/propagator.ts | 76 ++++++++++--------- 7 files changed, 108 insertions(+), 55 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/express/sentry-trace/trace-header-assign/server.ts diff --git a/dev-packages/node-integration-tests/suites/express/sentry-trace/trace-header-assign/server.ts b/dev-packages/node-integration-tests/suites/express/sentry-trace/trace-header-assign/server.ts new file mode 100644 index 000000000000..f1393c3cfc5b --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/sentry-trace/trace-header-assign/server.ts @@ -0,0 +1,35 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +export type TestAPIResponse = { test_data: { host: string; 'sentry-trace': string; baggage: string } }; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + environment: 'prod', + integrations: [ + // TODO: This used to use the Express integration + ], + tracesSampleRate: 1.0, + transport: loggingTransport, +}); + +import http from 'http'; +import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; +import cors from 'cors'; +import express from 'express'; + +const app = express(); + +app.use(cors()); + +app.get('/test/express', (_req, res) => { + const headers = http.get('http://somewhere.not.sentry/').getHeaders(); + + // Responding with the headers outgoing request headers back to the assertions. + res.send({ test_data: headers }); +}); + +Sentry.setupExpressErrorHandler(app); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/express/sentry-trace/trace-header-assign/test.ts b/dev-packages/node-integration-tests/suites/express/sentry-trace/trace-header-assign/test.ts index 2870f0a8b51c..071f02f83647 100644 --- a/dev-packages/node-integration-tests/suites/express/sentry-trace/trace-header-assign/test.ts +++ b/dev-packages/node-integration-tests/suites/express/sentry-trace/trace-header-assign/test.ts @@ -7,7 +7,7 @@ afterAll(() => { }); test('Should assign `sentry-trace` header which sets parent trace id of an outgoing request.', async () => { - const runner = createRunner(__dirname, '..', 'server.ts').start(); + const runner = createRunner(__dirname, 'server.ts').start(); const response = await runner.makeRequest('get', '/test/express', { 'sentry-trace': '12312012123120121231201212312012-1121201211212012-0', diff --git a/dev-packages/node-integration-tests/suites/tracing/tracingNoSampleRate/test.ts b/dev-packages/node-integration-tests/suites/tracing/tracingNoSampleRate/test.ts index f558f15c6d4f..a24e6ea05c20 100644 --- a/dev-packages/node-integration-tests/suites/tracing/tracingNoSampleRate/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/tracingNoSampleRate/test.ts @@ -1,27 +1,31 @@ import { createRunner } from '../../../utils/runner'; import { createTestServer } from '../../../utils/server'; -test('HttpIntegration should instrument correct requests even without tracesSampleRate xxx', done => { - expect.assertions(11); +test('HttpIntegration should instrument correct requests even without tracesSampleRate', done => { + expect.assertions(15); - createTestServer(done) + createTestServer(() => {}) .get('/api/v0', headers => { - expect(headers['baggage']).toBeUndefined(); 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)); + expect(headers['__requestUrl']).toBeUndefined(); }) .get('/api/v1', headers => { - expect(headers['baggage']).toBeUndefined(); 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)); + expect(headers['__requestUrl']).toBeUndefined(); }) .get('/api/v2', headers => { expect(headers['baggage']).toBeUndefined(); expect(headers['sentry-trace']).toBeUndefined(); + expect(headers['__requestUrl']).toBeUndefined(); }) .get('/api/v3', headers => { expect(headers['baggage']).toBeUndefined(); expect(headers['sentry-trace']).toBeUndefined(); + expect(headers['__requestUrl']).toBeUndefined(); }) .start() .then(SERVER_URL => { diff --git a/dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/scenario.ts index 3b1bdfff1b39..6b66b11b8ffb 100644 --- a/dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/scenario.ts +++ b/dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/scenario.ts @@ -13,10 +13,13 @@ Sentry.init({ import * as http from 'http'; async function run(): Promise { - await makeHttpRequest(`${process.env.SERVER_URL}/api/v0`); - await makeHttpRequest(`${process.env.SERVER_URL}/api/v1`); - await makeHttpRequest(`${process.env.SERVER_URL}/api/v2`); - await makeHttpRequest(`${process.env.SERVER_URL}/api/v3`); + // Wrap in span that is not sampled + await Sentry.startSpan({ name: 'outer' }, async () => { + await makeHttpRequest(`${process.env.SERVER_URL}/api/v0`); + await makeHttpRequest(`${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')); } diff --git a/dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/test.ts b/dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/test.ts index b95d2e1254c4..6b546a062f63 100644 --- a/dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/test.ts @@ -6,12 +6,12 @@ test('HttpIntegration should instrument correct requests even when not sampled', createTestServer(done) .get('/api/v0', headers => { - expect(headers['baggage']).toBeUndefined(); + expect(headers['baggage']).toEqual(expect.any(String)); expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-0$/)); expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-0'); }) .get('/api/v1', headers => { - expect(headers['baggage']).toBeUndefined(); + expect(headers['baggage']).toEqual(expect.any(String)); expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-0$/)); expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-0'); }) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 633f27c7b64c..a5d85e2a6edd 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -17,7 +17,7 @@ import { import { getClient, getRequestSpanData, getSpanKind } from '@sentry/opentelemetry'; import type { IntegrationFn } from '@sentry/types'; -import { stripUrlQueryAndFragment } from '@sentry/utils'; +import { addNonEnumerableProperty, stripUrlQueryAndFragment } from '@sentry/utils'; import type { NodeClient } from '../sdk/client'; import { setIsolationScope } from '../sdk/scope'; import type { HTTPModuleRequestIncomingMessage } from '../transports/http-module'; @@ -65,6 +65,15 @@ const _httpIntegration = ((options: HttpOptions = {}) => { return true; } + // SUPER HACK: + // We store the URL on the headers object, because this is passed to the propagator + // Where we can pick the URL off to deterime if we should attach trace headers. + const headers = request.headers || {}; + // Can't use a non-enumerable property because http instrumentation clones this + // We remove this in the propagator + headers['__requestUrl'] = url; + request.headers = headers; + if (_ignoreOutgoingRequests && _ignoreOutgoingRequests(url)) { return true; } @@ -93,18 +102,18 @@ const _httpIntegration = ((options: HttpOptions = {}) => { requestHook: (span, req) => { addOriginToSpan(span, 'auto.http.otel.http'); + const scopes = getCapturedScopesOnSpan(span); + + const isolationScope = (scopes.isolationScope || getIsolationScope()).clone(); + const scope = scopes.scope || getCurrentScope(); + // both, incoming requests and "client" requests made within the app trigger the requestHook // we only want to isolate and further annotate incoming requests (IncomingMessage) if (_isClientRequest(req)) { return; } - const scopes = getCapturedScopesOnSpan(span); - // Update the isolation scope, isolate this request - const isolationScope = (scopes.isolationScope || getIsolationScope()).clone(); - const scope = scopes.scope || getCurrentScope(); - isolationScope.setSDKProcessingMetadata({ request: req }); const client = getClient(); diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts index 77b3b5993199..e33d9d3ec004 100644 --- a/packages/opentelemetry/src/propagator.ts +++ b/packages/opentelemetry/src/propagator.ts @@ -1,18 +1,19 @@ -import type { Baggage, Context, Span, SpanContext, TextMapGetter, TextMapSetter } from '@opentelemetry/api'; +import { Baggage, Context, ROOT_CONTEXT, Span, SpanContext, TextMapGetter, TextMapSetter } from '@opentelemetry/api'; import { context } from '@opentelemetry/api'; import { TraceFlags, propagation, trace } from '@opentelemetry/api'; import { TraceState, W3CBaggagePropagator, isTracingSuppressed } from '@opentelemetry/core'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; -import { continueTrace, hasTracingEnabled } from '@sentry/core'; +import { continueTrace, getDefaultCurrentScope, getDefaultIsolationScope, hasTracingEnabled } from '@sentry/core'; import { getRootSpan } from '@sentry/core'; import { spanToJSON } from '@sentry/core'; import { getClient, getCurrentScope, getDynamicSamplingContextFromClient, getIsolationScope } from '@sentry/core'; -import type { DynamicSamplingContext, Options, PropagationContext } from '@sentry/types'; +import type { DynamicSamplingContext, Options, PolymorphicRequest, PropagationContext } from '@sentry/types'; import { LRUMap, SENTRY_BAGGAGE_KEY_PREFIX, baggageHeaderToDynamicSamplingContext, dynamicSamplingContextToSentryBaggageHeader, + extractRequestData, generateSentryTraceHeader, logger, parseBaggageHeader, @@ -84,7 +85,8 @@ export class SentryPropagator extends W3CBaggagePropagator { } const activeSpan = trace.getSpan(context); - const url = activeSpan && spanToJSON(activeSpan).data?.[SemanticAttributes.HTTP_URL]; + const url = getCurrentURL(carrier as { __requestUrl?: string }, activeSpan); + const tracePropagationTargets = getClient()?.getOptions()?.tracePropagationTargets; if ( typeof url === 'string' && @@ -120,7 +122,10 @@ export class SentryPropagator extends W3CBaggagePropagator { }, baggage); } - setter.set(carrier, SENTRY_TRACE_HEADER, generateSentryTraceHeader(traceId, spanId, sampled)); + // We also want to avoid setting the default OTEL trace ID, if we get that for whatever reason + if (traceId && traceId !== '00000000000000000000000000000000') { + setter.set(carrier, SENTRY_TRACE_HEADER, generateSentryTraceHeader(traceId, spanId, sampled)); + } super.inject(propagation.setBaggage(context, baggage), carrier, setter); } @@ -230,40 +235,15 @@ function getInjectionData(context: Context): { } // Else we try to use the propagation context from the scope - const scope = getScopesFromContext(context)?.scope; - - if (scope) { - const propagationContext = scope.getPropagationContext(); - const dynamicSamplingContext = getDynamicSamplingContext(propagationContext, propagationContext.traceId); - return { - dynamicSamplingContext, - traceId: propagationContext.traceId, - spanId: propagationContext.spanId, - sampled: propagationContext.sampled, - }; - } - - // Else, we look at the remote span context - if (span) { - const spanContext = span.spanContext(); - const propagationContext = getPropagationContextFromSpan(span); - const dynamicSamplingContext = getDynamicSamplingContext(propagationContext, spanContext.traceId); + const scope = getScopesFromContext(context)?.scope || getCurrentScope(); - return { - dynamicSamplingContext, - traceId: spanContext.traceId, - spanId: spanContext.spanId, - sampled: getSamplingDecision(spanContext), - }; - } - - // If we have neither, there is nothing much we can do, but that should not happen usually - // Unless there is a detached OTEL context being passed around + const propagationContext = scope.getPropagationContext(); + const dynamicSamplingContext = getDynamicSamplingContext(propagationContext, propagationContext.traceId); return { - dynamicSamplingContext: undefined, - traceId: undefined, - spanId: undefined, - sampled: undefined, + dynamicSamplingContext, + traceId: propagationContext.traceId, + spanId: propagationContext.spanId, + sampled: propagationContext.sampled, }; } @@ -334,3 +314,25 @@ function getExistingBaggage(carrier: unknown): string | undefined { return undefined; } } + +function getCurrentURL(carrier: { __requestUrl?: string }, span: Span | undefined): string | undefined { + try { + if (carrier.__requestUrl) { + const url = carrier.__requestUrl; + delete carrier.__requestUrl; + return url; + } + } catch { + // ignore errors here + } + + if (span) { + const url = spanToJSON(span).data?.[SemanticAttributes.HTTP_URL]; + + if (url) { + return url; + } + } + + return undefined; +} From 1dcad0cc38c74f89931ecc93be43b904f57a78a2 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 12 Apr 2024 09:44:36 +0200 Subject: [PATCH 03/13] fix fetch --- .../requests/fetch-noSampleRate/scenario.ts | 24 +++++++++ .../requests/fetch-noSampleRate/test.ts | 53 ++++++++++++++++++ .../requests/fetch-sampled/scenario.ts | 25 +++++++++ .../tracing/requests/fetch-sampled/test.ts | 40 ++++++++++++++ .../requests/fetch-unsampled/scenario.ts | 28 ++++++++++ .../tracing/requests/fetch-unsampled/test.ts | 48 +++++++++++++++++ .../http-noSampleRate}/scenario.ts | 0 .../http-noSampleRate}/test.ts | 8 +-- .../tracing/requests/http-sampled/scenario.ts | 20 +++++++ .../tracing/requests/http-sampled/test.ts | 37 +++++++++++++ .../http-unsampled}/scenario.ts | 0 .../http-unsampled}/test.ts | 6 +-- packages/node/src/integrations/http.ts | 12 ++--- packages/opentelemetry/src/constants.ts | 1 + packages/opentelemetry/src/index.ts | 1 + packages/opentelemetry/src/propagator.ts | 54 ++++++++++++------- packages/opentelemetry/src/sampler.ts | 20 ++++--- .../utils/storeRequestUrlForPropagation.ts | 32 +++++++++++ 18 files changed, 370 insertions(+), 39 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/fetch-noSampleRate/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/fetch-noSampleRate/test.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled/test.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/test.ts rename dev-packages/node-integration-tests/suites/tracing/{tracingNoSampleRate => requests/http-noSampleRate}/scenario.ts (100%) rename dev-packages/node-integration-tests/suites/tracing/{tracingNoSampleRate => requests/http-noSampleRate}/test.ts (87%) create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/http-sampled/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/http-sampled/test.ts rename dev-packages/node-integration-tests/suites/tracing/{tracingUnsampled => requests/http-unsampled}/scenario.ts (100%) rename dev-packages/node-integration-tests/suites/tracing/{tracingUnsampled => requests/http-unsampled}/test.ts (87%) create mode 100644 packages/opentelemetry/src/utils/storeRequestUrlForPropagation.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-noSampleRate/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-noSampleRate/scenario.ts new file mode 100644 index 000000000000..10c17266cfb0 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-noSampleRate/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: [], + 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`); + await fetch(`${process.env.SERVER_URL}/api/v1`); + await fetch(`${process.env.SERVER_URL}/api/v2`); + await fetch(`${process.env.SERVER_URL}/api/v3`); + + 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-noSampleRate/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-noSampleRate/test.ts new file mode 100644 index 000000000000..2de3dcfea66b --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-noSampleRate/test.ts @@ -0,0 +1,53 @@ +import { conditionalTest } from '../../../../utils'; +import { createRunner } from '../../../../utils/runner'; +import { createTestServer } from '../../../../utils/server'; + +conditionalTest({ min: 18 })('outgoing fetch', () => { + test('outgoing fetch requests are correctly instrumented without tracesSampleRate', done => { + expect.assertions(15); + + 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)); + expect(headers['__requestUrl']).toBeUndefined(); + }) + .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)); + expect(headers['__requestUrl']).toBeUndefined(); + }) + .get('/api/v2', headers => { + expect(headers['baggage']).toBeUndefined(); + expect(headers['sentry-trace']).toBeUndefined(); + expect(headers['__requestUrl']).toBeUndefined(); + }) + .get('/api/v3', headers => { + expect(headers['baggage']).toBeUndefined(); + expect(headers['sentry-trace']).toBeUndefined(); + expect(headers['__requestUrl']).toBeUndefined(); + }) + .start() + .then(SERVER_URL => { + createRunner(__dirname, 'scenario.ts') + .withEnv({ SERVER_URL }) + .ensureNoErrorOutput() + .ignore('session', 'sessions') + .expect({ + event: { + exception: { + values: [ + { + type: 'Error', + value: 'foo', + }, + ], + }, + }, + }) + .start(done); + }); + }); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled/scenario.ts new file mode 100644 index 000000000000..b75c3f0ee92b --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled/scenario.ts @@ -0,0 +1,25 @@ +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', + tracesSampleRate: 1.0, + tracePropagationTargets: [/\/v0/, 'v1'], + integrations: [], + transport: loggingTransport, +}); + +async function run(): Promise { + await Sentry.startSpan({ name: 'test_span' }, async () => { + // 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`); + await fetch(`${process.env.SERVER_URL}/api/v1`); + await fetch(`${process.env.SERVER_URL}/api/v2`); + await fetch(`${process.env.SERVER_URL}/api/v3`); + }); +} + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled/test.ts new file mode 100644 index 000000000000..70beaa6b331a --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled/test.ts @@ -0,0 +1,40 @@ +import { conditionalTest } from '../../../../utils'; +import { createRunner } from '../../../../utils/runner'; +import { createTestServer } from '../../../../utils/server'; + +conditionalTest({ min: 18 })('outgoing fetch', () => { + test('outgoing sampled fetch requests are correctly instrumented xxx', 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})-1$/)); + expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-1'); + 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})-1$/)); + expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-1'); + 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 => { + createRunner(__dirname, 'scenario.ts') + .withEnv({ SERVER_URL }) + .expect({ + transaction: { + // we're not too concerned with the actual transaction here since this is tested elsewhere + }, + }) + .start(done); + }); + }); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/scenario.ts new file mode 100644 index 000000000000..c8d8996d6e23 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/scenario.ts @@ -0,0 +1,28 @@ +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'], + tracesSampleRate: 0, + integrations: [], + transport: loggingTransport, +}); + +async function run(): Promise { + // Wrap in span that is not sampled + await Sentry.startSpan({ name: 'outer' }, async () => { + // 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`); + await fetch(`${process.env.SERVER_URL}/api/v1`); + await fetch(`${process.env.SERVER_URL}/api/v2`); + await fetch(`${process.env.SERVER_URL}/api/v3`); + }); + + 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-unsampled/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/test.ts new file mode 100644 index 000000000000..0b0ceeaa499c --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/test.ts @@ -0,0 +1,48 @@ +import { conditionalTest } from '../../../../utils'; +import { createRunner } from '../../../../utils/runner'; +import { createTestServer } from '../../../../utils/server'; + +conditionalTest({ min: 18 })('outgoing fetch', () => { + test('outgoing fetch requests are correctly instrumented when not sampled', done => { + expect.assertions(11); + + createTestServer(done) + .get('/api/v0', headers => { + expect(headers['baggage']).toEqual(expect.any(String)); + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-0$/)); + expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-0'); + }) + .get('/api/v1', headers => { + expect(headers['baggage']).toEqual(expect.any(String)); + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-0$/)); + expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-0'); + }) + .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 => { + createRunner(__dirname, 'scenario.ts') + .withEnv({ SERVER_URL }) + .ignore('session', 'sessions') + .expect({ + event: { + exception: { + values: [ + { + type: 'Error', + value: 'foo', + }, + ], + }, + }, + }) + .start(done); + }); + }); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/tracingNoSampleRate/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-noSampleRate/scenario.ts similarity index 100% rename from dev-packages/node-integration-tests/suites/tracing/tracingNoSampleRate/scenario.ts rename to dev-packages/node-integration-tests/suites/tracing/requests/http-noSampleRate/scenario.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/tracingNoSampleRate/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-noSampleRate/test.ts similarity index 87% rename from dev-packages/node-integration-tests/suites/tracing/tracingNoSampleRate/test.ts rename to dev-packages/node-integration-tests/suites/tracing/requests/http-noSampleRate/test.ts index a24e6ea05c20..570d9f4865a6 100644 --- a/dev-packages/node-integration-tests/suites/tracing/tracingNoSampleRate/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-noSampleRate/test.ts @@ -1,10 +1,10 @@ -import { createRunner } from '../../../utils/runner'; -import { createTestServer } from '../../../utils/server'; +import { createRunner } from '../../../../utils/runner'; +import { createTestServer } from '../../../../utils/server'; -test('HttpIntegration should instrument correct requests even without tracesSampleRate', done => { +test('outgoing http requests are correctly instrumented without tracesSampleRate', done => { expect.assertions(15); - createTestServer(() => {}) + 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'); diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-sampled/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-sampled/scenario.ts new file mode 100644 index 000000000000..c346b617b9e6 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-sampled/scenario.ts @@ -0,0 +1,20 @@ +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', + tracesSampleRate: 1.0, + tracePropagationTargets: [/\/v0/, 'v1'], + integrations: [], + transport: loggingTransport, +}); + +import * as http from 'http'; + +Sentry.startSpan({ name: 'test_span' }, () => { + http.get(`${process.env.SERVER_URL}/api/v0`); + http.get(`${process.env.SERVER_URL}/api/v1`); + http.get(`${process.env.SERVER_URL}/api/v2`); + http.get(`${process.env.SERVER_URL}/api/v3`); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-sampled/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-sampled/test.ts new file mode 100644 index 000000000000..fd939bc4458c --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-sampled/test.ts @@ -0,0 +1,37 @@ +import { createRunner } from '../../../../utils/runner'; +import { createTestServer } from '../../../../utils/server'; + +test('outgoing sampled http requests are correctly instrumented', done => { + expect.assertions(11); + + createTestServer(done) + .get('/api/v0', headers => { + expect(headers['baggage']).toEqual(expect.any(String)); + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/)); + expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-1'); + }) + .get('/api/v1', headers => { + expect(headers['baggage']).toEqual(expect.any(String)); + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/)); + expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-1'); + }) + .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 => { + createRunner(__dirname, 'scenario.ts') + .withEnv({ SERVER_URL }) + .expect({ + transaction: { + // we're not too concerned with the actual transaction here since this is tested elsewhere + }, + }) + .start(done); + }); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-unsampled/scenario.ts similarity index 100% rename from dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/scenario.ts rename to dev-packages/node-integration-tests/suites/tracing/requests/http-unsampled/scenario.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-unsampled/test.ts similarity index 87% rename from dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/test.ts rename to dev-packages/node-integration-tests/suites/tracing/requests/http-unsampled/test.ts index 6b546a062f63..25f4aea15b3a 100644 --- a/dev-packages/node-integration-tests/suites/tracing/tracingUnsampled/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-unsampled/test.ts @@ -1,7 +1,7 @@ -import { createRunner } from '../../../utils/runner'; -import { createTestServer } from '../../../utils/server'; +import { createRunner } from '../../../../utils/runner'; +import { createTestServer } from '../../../../utils/server'; -test('HttpIntegration should instrument correct requests even when not sampled', done => { +test('outgoing http requests are correctly instrumented when not sampled', done => { expect.assertions(11); createTestServer(done) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index a5d85e2a6edd..7e114aa035fb 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -3,6 +3,7 @@ import type { Span } from '@opentelemetry/api'; import { SpanKind } from '@opentelemetry/api'; import { registerInstrumentations } from '@opentelemetry/instrumentation'; import { HttpInstrumentation } from '@opentelemetry/instrumentation-http'; +import { storeRequestUrlOnPropagationCarrier } from '@sentry/opentelemetry'; import { addBreadcrumb, @@ -17,7 +18,7 @@ import { import { getClient, getRequestSpanData, getSpanKind } from '@sentry/opentelemetry'; import type { IntegrationFn } from '@sentry/types'; -import { addNonEnumerableProperty, stripUrlQueryAndFragment } from '@sentry/utils'; +import { stripUrlQueryAndFragment } from '@sentry/utils'; import type { NodeClient } from '../sdk/client'; import { setIsolationScope } from '../sdk/scope'; import type { HTTPModuleRequestIncomingMessage } from '../transports/http-module'; @@ -65,13 +66,10 @@ const _httpIntegration = ((options: HttpOptions = {}) => { return true; } - // SUPER HACK: - // We store the URL on the headers object, because this is passed to the propagator - // Where we can pick the URL off to deterime if we should attach trace headers. + // We keep the URL on the headers (which are the carrier for the propagator) + // so we can access it in the propagator to check against tracePropagationTargets const headers = request.headers || {}; - // Can't use a non-enumerable property because http instrumentation clones this - // We remove this in the propagator - headers['__requestUrl'] = url; + storeRequestUrlOnPropagationCarrier(headers, url); request.headers = headers; if (_ignoreOutgoingRequests && _ignoreOutgoingRequests(url)) { diff --git a/packages/opentelemetry/src/constants.ts b/packages/opentelemetry/src/constants.ts index e153d50b0180..0f056d09a4ea 100644 --- a/packages/opentelemetry/src/constants.ts +++ b/packages/opentelemetry/src/constants.ts @@ -6,6 +6,7 @@ export const SENTRY_BAGGAGE_HEADER = 'baggage'; export const SENTRY_TRACE_STATE_DSC = 'sentry.dsc'; export const SENTRY_TRACE_STATE_PARENT_SPAN_ID = 'sentry.parent_span_id'; export const SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING = 'sentry.sampled_not_recording'; +export const SENTRY_TRACE_STATE_URL = 'sentry.url'; export const SENTRY_SCOPES_CONTEXT_KEY = createContextKey('sentry_scopes'); diff --git a/packages/opentelemetry/src/index.ts b/packages/opentelemetry/src/index.ts index db5abb951f4c..14f8cbbc12a8 100644 --- a/packages/opentelemetry/src/index.ts +++ b/packages/opentelemetry/src/index.ts @@ -17,6 +17,7 @@ export { } from './utils/spanTypes'; export { getDynamicSamplingContextFromSpan } from './utils/dynamicSamplingContext'; +export { storeRequestUrlOnPropagationCarrier } from './utils/storeRequestUrlForPropagation'; export { isSentryRequestSpan } from './utils/isSentryRequest'; diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts index e33d9d3ec004..78b9d3f6c25c 100644 --- a/packages/opentelemetry/src/propagator.ts +++ b/packages/opentelemetry/src/propagator.ts @@ -1,19 +1,19 @@ -import { Baggage, Context, ROOT_CONTEXT, Span, SpanContext, TextMapGetter, TextMapSetter } from '@opentelemetry/api'; +import type { Baggage, Context, Span, SpanContext, TextMapGetter, TextMapSetter } from '@opentelemetry/api'; import { context } from '@opentelemetry/api'; import { TraceFlags, propagation, trace } from '@opentelemetry/api'; import { TraceState, W3CBaggagePropagator, isTracingSuppressed } from '@opentelemetry/core'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; -import { continueTrace, getDefaultCurrentScope, getDefaultIsolationScope, hasTracingEnabled } from '@sentry/core'; +import type { continueTrace } from '@sentry/core'; +import { hasTracingEnabled } from '@sentry/core'; import { getRootSpan } from '@sentry/core'; import { spanToJSON } from '@sentry/core'; import { getClient, getCurrentScope, getDynamicSamplingContextFromClient, getIsolationScope } from '@sentry/core'; -import type { DynamicSamplingContext, Options, PolymorphicRequest, PropagationContext } from '@sentry/types'; +import type { DynamicSamplingContext, Options, PropagationContext } from '@sentry/types'; import { LRUMap, SENTRY_BAGGAGE_KEY_PREFIX, baggageHeaderToDynamicSamplingContext, dynamicSamplingContextToSentryBaggageHeader, - extractRequestData, generateSentryTraceHeader, logger, parseBaggageHeader, @@ -27,12 +27,14 @@ import { SENTRY_TRACE_STATE_DSC, SENTRY_TRACE_STATE_PARENT_SPAN_ID, SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, + SENTRY_TRACE_STATE_URL, } from './constants'; import { DEBUG_BUILD } from './debug-build'; import { getScopesFromContext, setScopesOnContext } from './utils/contextData'; import { getDynamicSamplingContextFromSpan } from './utils/dynamicSamplingContext'; import { getSamplingDecision } from './utils/getSamplingDecision'; import { setIsSetup } from './utils/setupCheck'; +import { getAndCleanRequestUrlFromPropagationCarrier } from './utils/storeRequestUrlForPropagation'; /** Get the Sentry propagation context from a span context. */ export function getPropagationContextFromSpan(span: Span): PropagationContext { @@ -94,7 +96,10 @@ export class SentryPropagator extends W3CBaggagePropagator { !this._shouldInjectTraceData(tracePropagationTargets, url) ) { DEBUG_BUILD && - logger.log('[Tracing] Not injecting trace data for url because it does not matchTracePropagationTargets:', url); + logger.log( + '[Tracing] Not injecting trace data for url because it does not match tracePropagationTargets:', + url, + ); return; } @@ -315,24 +320,35 @@ function getExistingBaggage(carrier: unknown): string | undefined { } } -function getCurrentURL(carrier: { __requestUrl?: string }, span: Span | undefined): string | undefined { - try { - if (carrier.__requestUrl) { - const url = carrier.__requestUrl; - delete carrier.__requestUrl; - return url; - } - } catch { - // ignore errors here - } - +/** + * 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), + * but the span may be unsampled and thus have no attributes. + * + * So we use the following logic: + * 1. If we have an active span, we check if it has a URL attribute. + * 2. Else, if the active span has no URL attribute (e.g. it is unsampled), we check a special trace state (which we set in our sampler). + * 3. Finally, we look at a special header on the carrier, which we set in the http integration. + */ +function getCurrentURL(carrier: Record, span: Span | undefined): string | undefined { if (span) { - const url = spanToJSON(span).data?.[SemanticAttributes.HTTP_URL]; + const urlAttribute = spanToJSON(span).data?.[SemanticAttributes.HTTP_URL]; + if (urlAttribute) { + return urlAttribute; + } - if (url) { - return url; + // Also look at the traceState, which we may set in the sampler even for unsampled spans + const urlTraceState = span.spanContext().traceState?.get(SENTRY_TRACE_STATE_URL); + if (urlTraceState) { + return urlTraceState; } } + // This may be set in the http integration + const url = getAndCleanRequestUrlFromPropagationCarrier(carrier); + if (url) { + return url; + } + return undefined; } diff --git a/packages/opentelemetry/src/sampler.ts b/packages/opentelemetry/src/sampler.ts index e611d6a43395..bb78001ca6c6 100644 --- a/packages/opentelemetry/src/sampler.ts +++ b/packages/opentelemetry/src/sampler.ts @@ -6,7 +6,7 @@ import { SamplingDecision } from '@opentelemetry/sdk-trace-base'; import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, hasTracingEnabled, sampleSpan } from '@sentry/core'; import type { Client, SpanAttributes } from '@sentry/types'; import { logger } from '@sentry/utils'; -import { SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING } from './constants'; +import { SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, SENTRY_TRACE_STATE_URL } from './constants'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; import { DEBUG_BUILD } from './debug-build'; @@ -36,13 +36,20 @@ export class SentrySampler implements Sampler { ): SamplingResult { const options = this._client.getOptions(); - if (!hasTracingEnabled(options)) { - return { decision: SamplingDecision.NOT_RECORD }; - } - const parentSpan = trace.getSpan(context); const parentContext = parentSpan?.spanContext(); - const traceState = parentContext?.traceState || new TraceState(); + + let traceState = parentContext?.traceState || new TraceState(); + + // We always keep the URL on the trace state, so we can access it in the propagator + const url = spanAttributes[SemanticAttributes.HTTP_URL]; + if (url && typeof url === 'string') { + traceState = traceState.set(SENTRY_TRACE_STATE_URL, url); + } + + if (!hasTracingEnabled(options)) { + return { decision: SamplingDecision.NOT_RECORD, traceState }; + } let parentSampled: boolean | undefined = undefined; @@ -94,6 +101,7 @@ export class SentrySampler implements Sampler { return { decision: SamplingDecision.RECORD_AND_SAMPLED, attributes, + traceState, }; } diff --git a/packages/opentelemetry/src/utils/storeRequestUrlForPropagation.ts b/packages/opentelemetry/src/utils/storeRequestUrlForPropagation.ts new file mode 100644 index 000000000000..85f1a5d05b70 --- /dev/null +++ b/packages/opentelemetry/src/utils/storeRequestUrlForPropagation.ts @@ -0,0 +1,32 @@ +const REQUEST_URL_PROP = '__requestUrl'; + +/** + * This mutates a given carrier object (usually an headers POJO) and sets a special property on it that the propagator can use + * to get the URL for the request. + * This is pretty hacky but necessary for us to check outgoing requests agains tracePropagationTargets. + * Without this, we cannot access the URL for the outgoing request in the propagator, sadly - we only have access to the carrier. + * We need to make sure to remove this again in the propagator, to not send this header out. + */ +export function storeRequestUrlOnPropagationCarrier(carrier: { [key: string]: unknown }, url: string): void { + // Can't use a non-enumerable property because http instrumentation clones this + // We remove this in the propagator + carrier[REQUEST_URL_PROP] = url; +} + +/** + * Get the request URL from the carrier object, if it was set by storeRequestUrlOnPropagationCarrier. + * Additionally, also remove the property from the carrier object, to avoid it being sent out. + */ +export function getAndCleanRequestUrlFromPropagationCarrier(carrier: { [key: string]: unknown }): string | undefined { + try { + if (typeof carrier[REQUEST_URL_PROP] === 'string') { + const url = carrier[REQUEST_URL_PROP]; + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete carrier[REQUEST_URL_PROP]; + return url; + } + } catch { + // ignore errors here + } + return undefined; +} From 762307c8ecc53751cb0f0e48b85bde22352bdd9f Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 12 Apr 2024 09:53:59 +0200 Subject: [PATCH 04/13] better http tests --- .../suites/tracing/requests/http-sampled/test.ts | 6 +++++- .../suites/tracing/requests/http-unsampled/test.ts | 6 +++++- packages/opentelemetry/src/propagator.ts | 12 +++++------- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-sampled/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-sampled/test.ts index fd939bc4458c..f3ad8bc5728e 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/http-sampled/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-sampled/test.ts @@ -2,26 +2,30 @@ import { createRunner } from '../../../../utils/runner'; import { createTestServer } from '../../../../utils/server'; test('outgoing sampled http requests are correctly instrumented', done => { - expect.assertions(11); + expect.assertions(15); createTestServer(done) .get('/api/v0', headers => { expect(headers['baggage']).toEqual(expect.any(String)); expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/)); expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-1'); + expect(headers['__requestUrl']).toBeUndefined(); }) .get('/api/v1', headers => { expect(headers['baggage']).toEqual(expect.any(String)); expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/)); expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-1'); + expect(headers['__requestUrl']).toBeUndefined(); }) .get('/api/v2', headers => { expect(headers['baggage']).toBeUndefined(); expect(headers['sentry-trace']).toBeUndefined(); + expect(headers['__requestUrl']).toBeUndefined(); }) .get('/api/v3', headers => { expect(headers['baggage']).toBeUndefined(); expect(headers['sentry-trace']).toBeUndefined(); + expect(headers['__requestUrl']).toBeUndefined(); }) .start() .then(SERVER_URL => { diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-unsampled/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-unsampled/test.ts index 25f4aea15b3a..c860958622fa 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/http-unsampled/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-unsampled/test.ts @@ -2,26 +2,30 @@ import { createRunner } from '../../../../utils/runner'; import { createTestServer } from '../../../../utils/server'; test('outgoing http requests are correctly instrumented when not sampled', done => { - expect.assertions(11); + expect.assertions(15); createTestServer(done) .get('/api/v0', headers => { expect(headers['baggage']).toEqual(expect.any(String)); expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-0$/)); expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-0'); + expect(headers['__requestUrl']).toBeUndefined(); }) .get('/api/v1', headers => { expect(headers['baggage']).toEqual(expect.any(String)); expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-0$/)); expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-0'); + expect(headers['__requestUrl']).toBeUndefined(); }) .get('/api/v2', headers => { expect(headers['baggage']).toBeUndefined(); expect(headers['sentry-trace']).toBeUndefined(); + expect(headers['__requestUrl']).toBeUndefined(); }) .get('/api/v3', headers => { expect(headers['baggage']).toBeUndefined(); expect(headers['sentry-trace']).toBeUndefined(); + expect(headers['__requestUrl']).toBeUndefined(); }) .start() .then(SERVER_URL => { diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts index 78b9d3f6c25c..8f0ce251b4c5 100644 --- a/packages/opentelemetry/src/propagator.ts +++ b/packages/opentelemetry/src/propagator.ts @@ -331,6 +331,10 @@ function getExistingBaggage(carrier: unknown): string | undefined { * 3. Finally, we look at a special header on the carrier, which we set in the http integration. */ function getCurrentURL(carrier: Record, span: Span | undefined): string | undefined { + // This may be set in the http integration + // We call this first to ensure the carrier is always cleaned, but we only use it if we don't have any url on the span + const urlFromCarrier = getAndCleanRequestUrlFromPropagationCarrier(carrier); + if (span) { const urlAttribute = spanToJSON(span).data?.[SemanticAttributes.HTTP_URL]; if (urlAttribute) { @@ -344,11 +348,5 @@ function getCurrentURL(carrier: Record, span: Span | undefined) } } - // This may be set in the http integration - const url = getAndCleanRequestUrlFromPropagationCarrier(carrier); - if (url) { - return url; - } - - return undefined; + return urlFromCarrier; } From c1d0b6c858236776bf1acec329cbb3ffc05ccef5 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 12 Apr 2024 09:59:23 +0200 Subject: [PATCH 05/13] fix propagator tests --- .../opentelemetry/test/propagator.test.ts | 149 ++---------------- 1 file changed, 17 insertions(+), 132 deletions(-) diff --git a/packages/opentelemetry/test/propagator.test.ts b/packages/opentelemetry/test/propagator.test.ts index da9f2d30fc98..90d490378d97 100644 --- a/packages/opentelemetry/test/propagator.test.ts +++ b/packages/opentelemetry/test/propagator.test.ts @@ -40,134 +40,6 @@ describe('SentryPropagator', () => { describe('inject', () => { describe('without active local span', () => { - it.each([ - [ - 'uses remote spanContext without DSC for sampled remote span', - { - traceId: 'd4cda95b652f4a1592b449d5929fda1b', - spanId: '6e0c63257de34c92', - traceFlags: TraceFlags.SAMPLED, - isRemote: true, - }, - [ - 'sentry-environment=production', - 'sentry-release=1.0.0', - 'sentry-public_key=abc', - 'sentry-sampled=true', - 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', - ], - 'd4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-1', - ], - [ - 'uses remote spanContext without DSC for unsampled remote span', - { - traceId: 'd4cda95b652f4a1592b449d5929fda1b', - spanId: '6e0c63257de34c92', - traceFlags: TraceFlags.NONE, - isRemote: true, - }, - [ - 'sentry-environment=production', - 'sentry-release=1.0.0', - 'sentry-public_key=abc', - 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', - ], - 'd4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92', - ], - [ - 'uses remote spanContext with trace state & without DSC for unsampled remote span', - { - traceId: 'd4cda95b652f4a1592b449d5929fda1b', - spanId: '6e0c63257de34c92', - traceFlags: TraceFlags.NONE, - isRemote: true, - traceState: makeTraceState({ - sampled: false, - }), - }, - [ - 'sentry-environment=production', - 'sentry-release=1.0.0', - 'sentry-public_key=abc', - 'sentry-sampled=false', - 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', - ], - 'd4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-0', - ], - [ - 'uses remote spanContext with DSC for sampled remote span', - { - traceId: 'd4cda95b652f4a1592b449d5929fda1b', - spanId: '6e0c63257de34c92', - traceFlags: TraceFlags.SAMPLED, - traceState: makeTraceState({ - parentSpanId: '6e0c63257de34c92', - dsc: { - transaction: 'sampled-transaction', - sampled: 'true', - trace_id: 'dsc_trace_id', - public_key: 'dsc_public_key', - environment: 'dsc_environment', - release: 'dsc_release', - sample_rate: '0.5', - replay_id: 'dsc_replay_id', - }, - }), - isRemote: true, - }, - [ - 'sentry-environment=dsc_environment', - 'sentry-release=dsc_release', - 'sentry-public_key=dsc_public_key', - 'sentry-trace_id=dsc_trace_id', - 'sentry-transaction=sampled-transaction', - 'sentry-sampled=true', - 'sentry-sample_rate=0.5', - 'sentry-replay_id=dsc_replay_id', - ], - 'd4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-1', - ], - [ - 'uses remote spanContext with DSC for unsampled remote span', - { - traceId: 'd4cda95b652f4a1592b449d5929fda1b', - spanId: '6e0c63257de34c92', - traceFlags: TraceFlags.NONE, - traceState: makeTraceState({ - parentSpanId: '6e0c63257de34c92', - dsc: { - transaction: 'sampled-transaction', - sampled: 'false', - trace_id: 'dsc_trace_id', - public_key: 'dsc_public_key', - environment: 'dsc_environment', - release: 'dsc_release', - sample_rate: '0.5', - replay_id: 'dsc_replay_id', - }, - }), - isRemote: true, - }, - [ - 'sentry-environment=dsc_environment', - 'sentry-release=dsc_release', - 'sentry-public_key=dsc_public_key', - 'sentry-trace_id=dsc_trace_id', - 'sentry-transaction=sampled-transaction', - 'sentry-sampled=false', - 'sentry-sample_rate=0.5', - 'sentry-replay_id=dsc_replay_id', - ], - 'd4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-0', - ], - ])('%s', (_name, spanContext, baggage, sentryTrace) => { - const ctx = trace.setSpanContext(ROOT_CONTEXT, spanContext); - propagator.inject(ctx, carrier, defaultTextMapSetter); - - expect(baggageToArray(carrier[SENTRY_BAGGAGE_HEADER])).toEqual(baggage.sort()); - expect(carrier[SENTRY_TRACE_HEADER]).toBe(sentryTrace); - }); - it('uses scope propagation context without DSC if no span is found', () => { withScope(scope => { scope.setPropagationContext({ @@ -261,12 +133,20 @@ describe('SentryPropagator', () => { ); }); - it('creates random traceId & spanId if no scope & span is found', () => { + it('uses propagation data from current scope if no scope & span is found', () => { + const scope = getCurrentScope(); + const traceId = scope.getPropagationContext().traceId; + const ctx = trace.deleteSpan(ROOT_CONTEXT).deleteValue(SENTRY_SCOPES_CONTEXT_KEY); propagator.inject(ctx, carrier, defaultTextMapSetter); - expect(baggageToArray(carrier[SENTRY_BAGGAGE_HEADER])).toEqual([]); - expect(carrier[SENTRY_TRACE_HEADER]).toMatch(/^\w{32}-\w{16}$/); + expect(baggageToArray(carrier[SENTRY_BAGGAGE_HEADER])).toEqual([ + 'sentry-environment=production', + 'sentry-public_key=abc', + 'sentry-release=1.0.0', + `sentry-trace_id=${traceId}`, + ]); + expect(carrier[SENTRY_TRACE_HEADER]).toMatch(traceId); }); }); @@ -641,10 +521,15 @@ describe('SentryPropagator', () => { }); it('should create baggage without propagation context', () => { + const scope = getCurrentScope(); + const traceId = scope.getPropagationContext().traceId; + const context = ROOT_CONTEXT; const baggage = propagation.createBaggage({ foo: { value: 'bar' } }); propagator.inject(propagation.setBaggage(context, baggage), carrier, defaultTextMapSetter); - expect(carrier[SENTRY_BAGGAGE_HEADER]).toBe('foo=bar'); + expect(carrier[SENTRY_BAGGAGE_HEADER]).toBe( + `foo=bar,sentry-environment=production,sentry-release=1.0.0,sentry-public_key=abc,sentry-trace_id=${traceId}`, + ); }); it('should NOT set baggage and sentry-trace header if instrumentation is supressed', () => { From 1e625c608335ebe1db71e1e87498f42a400144de Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 12 Apr 2024 12:17:13 +0200 Subject: [PATCH 06/13] fix standalone http.client requests --- .../fetch-sampled-no-active-span/scenario.ts | 25 ++++++++++ .../fetch-sampled-no-active-span/test.ts | 48 ++++++++++++++++++ .../http-sampled-no-active-span/scenario.ts | 40 +++++++++++++++ .../http-sampled-no-active-span/test.ts | 49 +++++++++++++++++++ packages/node/src/integrations/http.ts | 1 - packages/opentelemetry/src/sampler.ts | 10 +++- 6 files changed, 170 insertions(+), 3 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/test.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/http-sampled-no-active-span/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/http-sampled-no-active-span/test.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/scenario.ts new file mode 100644 index 000000000000..588fbb3ed318 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/scenario.ts @@ -0,0 +1,25 @@ +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'], + tracesSampleRate: 1.0, + integrations: [], + 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`); + await fetch(`${process.env.SERVER_URL}/api/v1`); + await fetch(`${process.env.SERVER_URL}/api/v2`); + await fetch(`${process.env.SERVER_URL}/api/v3`); + + 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-sampled-no-active-span/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/test.ts new file mode 100644 index 000000000000..40e14fe648f8 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/test.ts @@ -0,0 +1,48 @@ +import { conditionalTest } from '../../../../utils'; +import { createRunner } from '../../../../utils/runner'; +import { createTestServer } from '../../../../utils/server'; + +conditionalTest({ min: 18 })('outgoing fetch', () => { + test('outgoing sampled fetch requests without active span are correctly instrumented', done => { + expect.assertions(11); + + createTestServer(done) + .get('/api/v0', headers => { + expect(headers['baggage']).toEqual(expect.any(String)); + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/)); + expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000'); + }) + .get('/api/v1', headers => { + expect(headers['baggage']).toEqual(expect.any(String)); + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/)); + expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000'); + }) + .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 => { + createRunner(__dirname, 'scenario.ts') + .withEnv({ SERVER_URL }) + .ignore('session', 'sessions') + .expect({ + event: { + exception: { + values: [ + { + type: 'Error', + value: 'foo', + }, + ], + }, + }, + }) + .start(done); + }); + }); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-sampled-no-active-span/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-sampled-no-active-span/scenario.ts new file mode 100644 index 000000000000..e98a9e9aea80 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-sampled-no-active-span/scenario.ts @@ -0,0 +1,40 @@ +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', + tracesSampleRate: 1.0, + tracePropagationTargets: [/\/v0/, 'v1'], + integrations: [], + transport: loggingTransport, +}); + +import * as http from 'http'; + +async function run(): Promise { + await makeHttpRequest(`${process.env.SERVER_URL}/api/v0`); + await makeHttpRequest(`${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')); +} + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +run(); + +function makeHttpRequest(url: string): Promise { + return new Promise(resolve => { + http + .request(url, httpRes => { + httpRes.on('data', () => { + // we don't care about data + }); + httpRes.on('end', () => { + resolve(); + }); + }) + .end(); + }); +} diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-sampled-no-active-span/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-sampled-no-active-span/test.ts new file mode 100644 index 000000000000..ee0151cd6297 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-sampled-no-active-span/test.ts @@ -0,0 +1,49 @@ +import { createRunner } from '../../../../utils/runner'; +import { createTestServer } from '../../../../utils/server'; + +test('outgoing sampled http requests without active span are correctly instrumented', done => { + expect.assertions(15); + + createTestServer(done) + .get('/api/v0', headers => { + expect(headers['baggage']).toEqual(expect.any(String)); + 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['__requestUrl']).toBeUndefined(); + }) + .get('/api/v1', headers => { + expect(headers['baggage']).toEqual(expect.any(String)); + 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['__requestUrl']).toBeUndefined(); + }) + .get('/api/v2', headers => { + expect(headers['baggage']).toBeUndefined(); + expect(headers['sentry-trace']).toBeUndefined(); + expect(headers['__requestUrl']).toBeUndefined(); + }) + .get('/api/v3', headers => { + expect(headers['baggage']).toBeUndefined(); + expect(headers['sentry-trace']).toBeUndefined(); + expect(headers['__requestUrl']).toBeUndefined(); + }) + .start() + .then(SERVER_URL => { + createRunner(__dirname, 'scenario.ts') + .withEnv({ SERVER_URL }) + .ignore('session', 'sessions') + .expect({ + event: { + exception: { + values: [ + { + type: 'Error', + value: 'foo', + }, + ], + }, + }, + }) + .start(done); + }); +}); diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 7e114aa035fb..908e6dbb4215 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -95,7 +95,6 @@ const _httpIntegration = ((options: HttpOptions = {}) => { return false; }, - requireParentforOutgoingSpans: true, requireParentforIncomingSpans: false, requestHook: (span, req) => { addOriginToSpan(span, 'auto.http.otel.http'); diff --git a/packages/opentelemetry/src/sampler.ts b/packages/opentelemetry/src/sampler.ts index bb78001ca6c6..8f1ca62fea70 100644 --- a/packages/opentelemetry/src/sampler.ts +++ b/packages/opentelemetry/src/sampler.ts @@ -1,4 +1,4 @@ -import type { Attributes, Context, Span } from '@opentelemetry/api'; +import { Attributes, Context, Span, SpanKind } from '@opentelemetry/api'; import { isSpanContextValid, trace } from '@opentelemetry/api'; import { TraceState } from '@opentelemetry/core'; import type { Sampler, SamplingResult } from '@opentelemetry/sdk-trace-base'; @@ -30,7 +30,7 @@ export class SentrySampler implements Sampler { context: Context, traceId: string, spanName: string, - _spanKind: unknown, + spanKind: SpanKind, spanAttributes: SpanAttributes, _links: unknown, ): SamplingResult { @@ -51,6 +51,12 @@ export class SentrySampler implements Sampler { return { decision: SamplingDecision.NOT_RECORD, traceState }; } + // If we have a http.client span that has no local parent, we never want to sample it + // but we want to leave downstream sampling decisions up to the server + if (spanKind === SpanKind.CLIENT && (!parentSpan || parentContext?.isRemote)) { + return { decision: SamplingDecision.NOT_RECORD, traceState }; + } + let parentSampled: boolean | undefined = undefined; // Only inherit sample rate if `traceId` is the same From 309242eac610a811427509d0fa406ac10b0ba610 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 12 Apr 2024 14:57:47 +0200 Subject: [PATCH 07/13] fix sampler --- packages/opentelemetry/src/sampler.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry/src/sampler.ts b/packages/opentelemetry/src/sampler.ts index 8f1ca62fea70..b726662aee0e 100644 --- a/packages/opentelemetry/src/sampler.ts +++ b/packages/opentelemetry/src/sampler.ts @@ -1,4 +1,5 @@ -import { Attributes, Context, Span, SpanKind } from '@opentelemetry/api'; +import type { Attributes, Context, Span} from '@opentelemetry/api'; +import { SpanKind } from '@opentelemetry/api'; import { isSpanContextValid, trace } from '@opentelemetry/api'; import { TraceState } from '@opentelemetry/core'; import type { Sampler, SamplingResult } from '@opentelemetry/sdk-trace-base'; @@ -53,7 +54,11 @@ export class SentrySampler implements Sampler { // If we have a http.client span that has no local parent, we never want to sample it // but we want to leave downstream sampling decisions up to the server - if (spanKind === SpanKind.CLIENT && (!parentSpan || parentContext?.isRemote)) { + if ( + spanKind === SpanKind.CLIENT && + spanAttributes[SemanticAttributes.HTTP_METHOD] && + (!parentSpan || parentContext?.isRemote) + ) { return { decision: SamplingDecision.NOT_RECORD, traceState }; } From 1da510dd063dcc3e7875273ac057a850d81e6e19 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 12 Apr 2024 15:02:20 +0200 Subject: [PATCH 08/13] remove carrier hack --- packages/node/src/integrations/http.ts | 7 ---- packages/opentelemetry/src/index.ts | 1 - packages/opentelemetry/src/propagator.ts | 30 +++++++---------- .../utils/storeRequestUrlForPropagation.ts | 32 ------------------- 4 files changed, 11 insertions(+), 59 deletions(-) delete mode 100644 packages/opentelemetry/src/utils/storeRequestUrlForPropagation.ts diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 908e6dbb4215..2c9dbfd6749c 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -3,7 +3,6 @@ import type { Span } from '@opentelemetry/api'; import { SpanKind } from '@opentelemetry/api'; import { registerInstrumentations } from '@opentelemetry/instrumentation'; import { HttpInstrumentation } from '@opentelemetry/instrumentation-http'; -import { storeRequestUrlOnPropagationCarrier } from '@sentry/opentelemetry'; import { addBreadcrumb, @@ -66,12 +65,6 @@ const _httpIntegration = ((options: HttpOptions = {}) => { return true; } - // We keep the URL on the headers (which are the carrier for the propagator) - // so we can access it in the propagator to check against tracePropagationTargets - const headers = request.headers || {}; - storeRequestUrlOnPropagationCarrier(headers, url); - request.headers = headers; - if (_ignoreOutgoingRequests && _ignoreOutgoingRequests(url)) { return true; } diff --git a/packages/opentelemetry/src/index.ts b/packages/opentelemetry/src/index.ts index 14f8cbbc12a8..db5abb951f4c 100644 --- a/packages/opentelemetry/src/index.ts +++ b/packages/opentelemetry/src/index.ts @@ -17,7 +17,6 @@ export { } from './utils/spanTypes'; export { getDynamicSamplingContextFromSpan } from './utils/dynamicSamplingContext'; -export { storeRequestUrlOnPropagationCarrier } from './utils/storeRequestUrlForPropagation'; export { isSentryRequestSpan } from './utils/isSentryRequest'; diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts index 8f0ce251b4c5..6640a0f4e0f1 100644 --- a/packages/opentelemetry/src/propagator.ts +++ b/packages/opentelemetry/src/propagator.ts @@ -34,7 +34,6 @@ import { getScopesFromContext, setScopesOnContext } from './utils/contextData'; import { getDynamicSamplingContextFromSpan } from './utils/dynamicSamplingContext'; import { getSamplingDecision } from './utils/getSamplingDecision'; import { setIsSetup } from './utils/setupCheck'; -import { getAndCleanRequestUrlFromPropagationCarrier } from './utils/storeRequestUrlForPropagation'; /** Get the Sentry propagation context from a span context. */ export function getPropagationContextFromSpan(span: Span): PropagationContext { @@ -87,7 +86,7 @@ export class SentryPropagator extends W3CBaggagePropagator { } const activeSpan = trace.getSpan(context); - const url = getCurrentURL(carrier as { __requestUrl?: string }, activeSpan); + const url = activeSpan && getCurrentURL(activeSpan); const tracePropagationTargets = getClient()?.getOptions()?.tracePropagationTargets; if ( @@ -328,25 +327,18 @@ function getExistingBaggage(carrier: unknown): string | undefined { * So we use the following logic: * 1. If we have an active span, we check if it has a URL attribute. * 2. Else, if the active span has no URL attribute (e.g. it is unsampled), we check a special trace state (which we set in our sampler). - * 3. Finally, we look at a special header on the carrier, which we set in the http integration. */ -function getCurrentURL(carrier: Record, span: Span | undefined): string | undefined { - // This may be set in the http integration - // We call this first to ensure the carrier is always cleaned, but we only use it if we don't have any url on the span - const urlFromCarrier = getAndCleanRequestUrlFromPropagationCarrier(carrier); - - if (span) { - const urlAttribute = spanToJSON(span).data?.[SemanticAttributes.HTTP_URL]; - if (urlAttribute) { - return urlAttribute; - } +function getCurrentURL(span: Span): string | undefined { + const urlAttribute = spanToJSON(span).data?.[SemanticAttributes.HTTP_URL]; + if (urlAttribute) { + return urlAttribute; + } - // Also look at the traceState, which we may set in the sampler even for unsampled spans - const urlTraceState = span.spanContext().traceState?.get(SENTRY_TRACE_STATE_URL); - if (urlTraceState) { - return urlTraceState; - } + // Also look at the traceState, which we may set in the sampler even for unsampled spans + const urlTraceState = span.spanContext().traceState?.get(SENTRY_TRACE_STATE_URL); + if (urlTraceState) { + return urlTraceState; } - return urlFromCarrier; + return undefined; } diff --git a/packages/opentelemetry/src/utils/storeRequestUrlForPropagation.ts b/packages/opentelemetry/src/utils/storeRequestUrlForPropagation.ts deleted file mode 100644 index 85f1a5d05b70..000000000000 --- a/packages/opentelemetry/src/utils/storeRequestUrlForPropagation.ts +++ /dev/null @@ -1,32 +0,0 @@ -const REQUEST_URL_PROP = '__requestUrl'; - -/** - * This mutates a given carrier object (usually an headers POJO) and sets a special property on it that the propagator can use - * to get the URL for the request. - * This is pretty hacky but necessary for us to check outgoing requests agains tracePropagationTargets. - * Without this, we cannot access the URL for the outgoing request in the propagator, sadly - we only have access to the carrier. - * We need to make sure to remove this again in the propagator, to not send this header out. - */ -export function storeRequestUrlOnPropagationCarrier(carrier: { [key: string]: unknown }, url: string): void { - // Can't use a non-enumerable property because http instrumentation clones this - // We remove this in the propagator - carrier[REQUEST_URL_PROP] = url; -} - -/** - * Get the request URL from the carrier object, if it was set by storeRequestUrlOnPropagationCarrier. - * Additionally, also remove the property from the carrier object, to avoid it being sent out. - */ -export function getAndCleanRequestUrlFromPropagationCarrier(carrier: { [key: string]: unknown }): string | undefined { - try { - if (typeof carrier[REQUEST_URL_PROP] === 'string') { - const url = carrier[REQUEST_URL_PROP]; - // eslint-disable-next-line @typescript-eslint/no-dynamic-delete - delete carrier[REQUEST_URL_PROP]; - return url; - } - } catch { - // ignore errors here - } - return undefined; -} From 80cbb9aae15958bdffc0603c68dbe34c1039d890 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 12 Apr 2024 15:05:40 +0200 Subject: [PATCH 09/13] fix linting --- packages/opentelemetry/src/sampler.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry/src/sampler.ts b/packages/opentelemetry/src/sampler.ts index b726662aee0e..024f802c5ae6 100644 --- a/packages/opentelemetry/src/sampler.ts +++ b/packages/opentelemetry/src/sampler.ts @@ -1,4 +1,4 @@ -import type { Attributes, Context, Span} from '@opentelemetry/api'; +import type { Attributes, Context, Span } from '@opentelemetry/api'; import { SpanKind } from '@opentelemetry/api'; import { isSpanContextValid, trace } from '@opentelemetry/api'; import { TraceState } from '@opentelemetry/core'; From f0c57117fc936efee44e7f8bbc5541e7ceb8262f Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 12 Apr 2024 15:24:55 +0200 Subject: [PATCH 10/13] fix test leftovers --- .../suites/tracing/requests/fetch-sampled/test.ts | 2 +- packages/browser/test/unit/utils/lazyLoadIntegration.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled/test.ts index 70beaa6b331a..40d05d2131eb 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled/test.ts @@ -3,7 +3,7 @@ import { createRunner } from '../../../../utils/runner'; import { createTestServer } from '../../../../utils/server'; conditionalTest({ min: 18 })('outgoing fetch', () => { - test('outgoing sampled fetch requests are correctly instrumented xxx', done => { + test('outgoing sampled fetch requests are correctly instrumented', done => { expect.assertions(11); createTestServer(done) diff --git a/packages/browser/test/unit/utils/lazyLoadIntegration.test.ts b/packages/browser/test/unit/utils/lazyLoadIntegration.test.ts index a9a86cbf4374..f2afefcbe9a2 100644 --- a/packages/browser/test/unit/utils/lazyLoadIntegration.test.ts +++ b/packages/browser/test/unit/utils/lazyLoadIntegration.test.ts @@ -64,7 +64,7 @@ describe('lazyLoadIntegration', () => { expect(global.document.querySelectorAll('script')).toHaveLength(0); }); - test('it injects a script tag if integration is not yet loaded xxx', async () => { + test('it injects a script tag if integration is not yet loaded', async () => { // @ts-expect-error For testing sake global.Sentry = { ...Sentry, From 8845366e202fcef79fb99ba83f3cdcaeb0a2c369 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 12 Apr 2024 15:27:29 +0200 Subject: [PATCH 11/13] bump opentelemetry-node-fetch instrumentation to 1.2.0 --- packages/node/package.json | 2 +- yarn.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/node/package.json b/packages/node/package.json index c711fc35b620..e4019a00e6e5 100644 --- a/packages/node/package.json +++ b/packages/node/package.json @@ -82,7 +82,7 @@ "@types/node": "^14.18.0" }, "optionalDependencies": { - "opentelemetry-instrumentation-fetch-node": "1.1.2" + "opentelemetry-instrumentation-fetch-node": "1.2.0" }, "scripts": { "build": "run-p build:transpile build:types", diff --git a/yarn.lock b/yarn.lock index 64bc3a06b8bb..2db9b7009ea8 100644 --- a/yarn.lock +++ b/yarn.lock @@ -23178,10 +23178,10 @@ opener@^1.5.2: resolved "https://registry.yarnpkg.com/opener/-/opener-1.5.2.tgz#5d37e1f35077b9dcac4301372271afdeb2a13598" integrity sha512-ur5UIdyw5Y7yEj9wLzhqXiy6GZ3Mwx0yGI+5sMn2r0N0v3cKJvUmFH5yPP+WXh9e0xfyzyJX95D8l088DNFj7A== -opentelemetry-instrumentation-fetch-node@1.1.2: - version "1.1.2" - resolved "https://registry.yarnpkg.com/opentelemetry-instrumentation-fetch-node/-/opentelemetry-instrumentation-fetch-node-1.1.2.tgz#ba18648b8e1273c5e801a1d9d7a5e4c6f1daf6df" - integrity sha512-w5KYAw/X/F0smj1v67VQUhnWusS6+0y/v7W/H5iY6s1Zf7uOCCm0fiffY8t2H+4wXui2AOjcfEM77S6AvSTAJg== +opentelemetry-instrumentation-fetch-node@1.2.0: + version "1.2.0" + resolved "https://registry.yarnpkg.com/opentelemetry-instrumentation-fetch-node/-/opentelemetry-instrumentation-fetch-node-1.2.0.tgz#5beaad33b622f7021c61733af864fb505cd35626" + integrity sha512-aiSt/4ubOTyb1N5C2ZbGrBvaJOXIZhZvpRPYuUVxQJe27wJZqf/o65iPrqgLcgfeOLaQ8cS2Q+762jrYvniTrA== dependencies: "@opentelemetry/api" "^1.6.0" "@opentelemetry/instrumentation" "^0.43.0" From 8b0227524e9a5fd7d42c98b399d787397a815906 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 15 Apr 2024 10:59:13 +0200 Subject: [PATCH 12/13] PR feedback --- .../tracing/requests/fetch-noSampleRate/scenario.ts | 8 ++++---- .../requests/fetch-sampled-no-active-span/scenario.ts | 8 ++++---- .../suites/tracing/requests/fetch-sampled/scenario.ts | 8 ++++---- .../tracing/requests/fetch-unsampled/scenario.ts | 8 ++++---- packages/node/src/integrations/http.ts | 11 ++++++----- 5 files changed, 22 insertions(+), 21 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-noSampleRate/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-noSampleRate/scenario.ts index 10c17266cfb0..55b0a04c6784 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-noSampleRate/scenario.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-noSampleRate/scenario.ts @@ -12,10 +12,10 @@ Sentry.init({ 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`); - await fetch(`${process.env.SERVER_URL}/api/v1`); - await fetch(`${process.env.SERVER_URL}/api/v2`); - await fetch(`${process.env.SERVER_URL}/api/v3`); + 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')); } diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/scenario.ts index 588fbb3ed318..191797a10c15 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/scenario.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/scenario.ts @@ -13,10 +13,10 @@ Sentry.init({ 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`); - await fetch(`${process.env.SERVER_URL}/api/v1`); - await fetch(`${process.env.SERVER_URL}/api/v2`); - await fetch(`${process.env.SERVER_URL}/api/v3`); + 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')); } diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled/scenario.ts index b75c3f0ee92b..4d47e16fd42f 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled/scenario.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled/scenario.ts @@ -14,10 +14,10 @@ async function run(): Promise { await Sentry.startSpan({ name: 'test_span' }, async () => { // 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`); - await fetch(`${process.env.SERVER_URL}/api/v1`); - await fetch(`${process.env.SERVER_URL}/api/v2`); - await fetch(`${process.env.SERVER_URL}/api/v3`); + 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()); }); } diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/scenario.ts index c8d8996d6e23..91c38bf2b23a 100644 --- a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/scenario.ts +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/scenario.ts @@ -15,10 +15,10 @@ async function run(): Promise { await Sentry.startSpan({ name: 'outer' }, async () => { // 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`); - await fetch(`${process.env.SERVER_URL}/api/v1`); - await fetch(`${process.env.SERVER_URL}/api/v2`); - await fetch(`${process.env.SERVER_URL}/api/v3`); + 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')); diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 2c9dbfd6749c..89aa8d90b997 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -88,21 +88,22 @@ const _httpIntegration = ((options: HttpOptions = {}) => { return false; }, + requireParentforOutgoingSpans: false, requireParentforIncomingSpans: false, requestHook: (span, req) => { addOriginToSpan(span, 'auto.http.otel.http'); - const scopes = getCapturedScopesOnSpan(span); - - const isolationScope = (scopes.isolationScope || getIsolationScope()).clone(); - const scope = scopes.scope || getCurrentScope(); - // both, incoming requests and "client" requests made within the app trigger the requestHook // we only want to isolate and further annotate incoming requests (IncomingMessage) if (_isClientRequest(req)) { return; } + const scopes = getCapturedScopesOnSpan(span); + + const isolationScope = (scopes.isolationScope || getIsolationScope()).clone(); + const scope = scopes.scope || getCurrentScope(); + // Update the isolation scope, isolate this request isolationScope.setSDKProcessingMetadata({ request: req }); From ec1bac740e4f2a0425c4a27050f4c05722c147f5 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 15 Apr 2024 11:19:14 +0200 Subject: [PATCH 13/13] fix test --- packages/opentelemetry/test/propagator.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry/test/propagator.test.ts b/packages/opentelemetry/test/propagator.test.ts index 90d490378d97..0d84da98e4f6 100644 --- a/packages/opentelemetry/test/propagator.test.ts +++ b/packages/opentelemetry/test/propagator.test.ts @@ -8,7 +8,7 @@ import { trace, } from '@opentelemetry/api'; import { suppressTracing } from '@opentelemetry/core'; -import { withScope } from '@sentry/core'; +import { getCurrentScope, withScope } from '@sentry/core'; import { SENTRY_BAGGAGE_HEADER, SENTRY_SCOPES_CONTEXT_KEY, SENTRY_TRACE_HEADER } from '../src/constants'; import { SentryPropagator, makeTraceState } from '../src/propagator';