From 36d4cfecaf792c45b4a0b4acc8d1847471c49758 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 25 Jan 2024 14:40:48 +0100 Subject: [PATCH 1/5] ref: Read propagation context off of scope and isolation scope when propagating --- .../tests/propagation.test.ts | 1 + .../baggage-header-assign/test.ts | 27 +++++++-- .../test.ts | 2 +- .../baggage-other-vendors/test.ts | 2 +- packages/core/src/baseclient.ts | 12 ++-- packages/core/src/tracing/trace.ts | 2 +- .../core/src/utils/applyScopeDataToEvent.ts | 13 ++--- packages/core/test/lib/prepareEvent.test.ts | 17 ++---- packages/core/test/lib/scope.test.ts | 16 ++--- packages/core/test/lib/tracing/trace.test.ts | 1 + packages/hub/test/scope.test.ts | 2 - .../test/integration/transactions.test.ts | 5 -- .../node-experimental/test/sdk/scope.test.ts | 16 ++--- packages/node/src/integrations/http.ts | 52 ++++++++--------- .../node/src/integrations/undici/index.ts | 29 ++++++---- packages/node/test/handlers.test.ts | 1 + packages/opentelemetry/src/propagator.ts | 2 +- packages/opentelemetry/src/spanExporter.ts | 6 +- .../test/integration/scope.test.ts | 2 - .../test/integration/transactions.test.ts | 5 -- .../opentelemetry/test/propagator.test.ts | 7 ++- .../src/browser/browsertracing.ts | 58 +++++++------------ .../tracing-internal/src/browser/request.ts | 34 ++++++----- packages/tracing-internal/src/common/fetch.ts | 21 +++---- packages/types/src/tracing.ts | 35 ++++++++++- packages/utils/src/tracing.ts | 41 +++++++------ packages/utils/test/tracing.test.ts | 9 +++ 27 files changed, 219 insertions(+), 199 deletions(-) create mode 100644 packages/utils/test/tracing.test.ts diff --git a/dev-packages/e2e-tests/test-applications/node-experimental-fastify-app/tests/propagation.test.ts b/dev-packages/e2e-tests/test-applications/node-experimental-fastify-app/tests/propagation.test.ts index 9b83e882a739..fe395b1ccce8 100644 --- a/dev-packages/e2e-tests/test-applications/node-experimental-fastify-app/tests/propagation.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-experimental-fastify-app/tests/propagation.test.ts @@ -164,6 +164,7 @@ test('Propagates trace for outgoing fetch requests', async ({ baseURL }) => { 'sentry.origin': 'auto.http.otel.http', }, op: 'http.server', + parent_span_id: expect.any(String), span_id: expect.any(String), status: 'ok', tags: { diff --git a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts index 190f458ea76c..4ec29414868c 100644 --- a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts +++ b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts @@ -5,7 +5,7 @@ afterAll(() => { cleanupChildProcesses(); }); -test('Should not overwrite baggage if the incoming request already has Sentry baggage data.', async () => { +test('Should overwrite baggage if the incoming request already has Sentry baggage data but no sentry-trace', async () => { const runner = createRunner(__dirname, '..', 'server.ts').start(); const response = await runner.makeRequest('get', '/test/express', { @@ -13,7 +13,7 @@ test('Should not overwrite baggage if the incoming request already has Sentry ba }); expect(response).toBeDefined(); - expect(response).toMatchObject({ + expect(response).not.toMatchObject({ test_data: { host: 'somewhere.not.sentry', baggage: 'sentry-release=2.0.0,sentry-environment=myEnv', @@ -25,7 +25,7 @@ test('Should propagate sentry trace baggage data from an incoming to an outgoing const runner = createRunner(__dirname, '..', 'server.ts').start(); const response = await runner.makeRequest('get', '/test/express', { - 'sentry-trace': '', + 'sentry-trace': '12312012123120121231201212312012-1121201211212012-1', baggage: 'sentry-release=2.0.0,sentry-environment=myEnv,dogs=great', }); @@ -38,11 +38,28 @@ test('Should propagate sentry trace baggage data from an incoming to an outgoing }); }); -test('Should not propagate baggage if sentry-trace header is present in incoming request but no baggage header', async () => { +test('Should not propagate baggage data from an incoming to an outgoing request if sentry-trace is faulty.', async () => { const runner = createRunner(__dirname, '..', 'server.ts').start(); const response = await runner.makeRequest('get', '/test/express', { 'sentry-trace': '', + baggage: 'sentry-release=2.0.0,sentry-environment=myEnv,dogs=great', + }); + + expect(response).toBeDefined(); + expect(response).not.toMatchObject({ + test_data: { + host: 'somewhere.not.sentry', + baggage: 'sentry-release=2.0.0,sentry-environment=myEnv', + }, + }); +}); + +test('Should not propagate baggage if sentry-trace header is present in incoming request but no baggage header', async () => { + const runner = createRunner(__dirname, '..', 'server.ts').start(); + + const response = await runner.makeRequest('get', '/test/express', { + 'sentry-trace': '12312012123120121231201212312012-1121201211212012-1', }); expect(response).toBeDefined(); @@ -57,7 +74,7 @@ test('Should not propagate baggage and ignore original 3rd party baggage entries const runner = createRunner(__dirname, '..', 'server.ts').start(); const response = await runner.makeRequest('get', '/test/express', { - 'sentry-trace': '', + 'sentry-trace': '12312012123120121231201212312012-1121201211212012-1', baggage: 'foo=bar', }); diff --git a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts index 1accfce01316..b7b6c08c0f3e 100644 --- a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts +++ b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts @@ -9,7 +9,7 @@ test('should ignore sentry-values in `baggage` header of a third party vendor an const runner = createRunner(__dirname, 'server.ts').start(); const response = await runner.makeRequest('get', '/test/express', { - 'sentry-trace': '', + 'sentry-trace': '12312012123120121231201212312012-1121201211212012-1', baggage: 'sentry-release=2.1.0,sentry-environment=myEnv', }); diff --git a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts index bc93b2886a19..41b2b9d7cf19 100644 --- a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts +++ b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts @@ -9,7 +9,7 @@ test('should merge `baggage` header of a third party vendor with the Sentry DSC const runner = createRunner(__dirname, 'server.ts').start(); const response = await runner.makeRequest('get', '/test/express', { - 'sentry-trace': '', + 'sentry-trace': '12312012123120121231201212312012-1121201211212012-1', baggage: 'sentry-release=2.0.0,sentry-environment=myEnv', }); diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index d59d596e0b82..21bd9d9d4eb1 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -20,7 +20,6 @@ import type { MetricsAggregator, Outcome, ParameterizedString, - PropagationContext, SdkMetadata, Session, SessionAggregates, @@ -638,13 +637,14 @@ export abstract class BaseClient implements Client { return evt; } - // If a trace context is not set on the event, we use the propagationContext set on the event to - // generate a trace context. If the propagationContext does not have a dynamic sampling context, we - // also generate one for it. - const { propagationContext } = evt.sdkProcessingMetadata || {}; + const propagationContext = { + ...isolationScope.getPropagationContext(), + ...(scope ? scope.getPropagationContext() : undefined), + }; + const trace = evt.contexts && evt.contexts.trace; if (!trace && propagationContext) { - const { traceId: trace_id, spanId, parentSpanId, dsc } = propagationContext as PropagationContext; + const { traceId: trace_id, spanId, parentSpanId, dsc } = propagationContext; evt.contexts = { trace: { trace_id, diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index eb070510a3b7..1488cb4075da 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -346,7 +346,7 @@ export function continueTrace( const transactionContext: Partial = { ...traceparentData, metadata: dropUndefinedKeys({ - dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, + dynamicSamplingContext, }), }; diff --git a/packages/core/src/utils/applyScopeDataToEvent.ts b/packages/core/src/utils/applyScopeDataToEvent.ts index bdf60497cff1..92a5e6747cd8 100644 --- a/packages/core/src/utils/applyScopeDataToEvent.ts +++ b/packages/core/src/utils/applyScopeDataToEvent.ts @@ -1,4 +1,4 @@ -import type { Breadcrumb, Event, PropagationContext, ScopeData, Span } from '@sentry/types'; +import type { Breadcrumb, Event, ScopeData, Span } from '@sentry/types'; import { arrayify, dropUndefinedKeys } from '@sentry/utils'; import { getDynamicSamplingContextFromSpan } from '../tracing/dynamicSamplingContext'; import { getRootSpan } from './getRootSpan'; @@ -8,7 +8,7 @@ import { spanToJSON, spanToTraceContext } from './spanUtils'; * Applies data from the scope to the event and runs all event processors on it. */ export function applyScopeDataToEvent(event: Event, data: ScopeData): void { - const { fingerprint, span, breadcrumbs, sdkProcessingMetadata, propagationContext } = data; + const { fingerprint, span, breadcrumbs, sdkProcessingMetadata } = data; // Apply general data applyDataToEvent(event, data); @@ -22,7 +22,7 @@ export function applyScopeDataToEvent(event: Event, data: ScopeData): void { applyFingerprintToEvent(event, fingerprint); applyBreadcrumbsToEvent(event, breadcrumbs); - applySdkMetadataToEvent(event, sdkProcessingMetadata, propagationContext); + applySdkMetadataToEvent(event, sdkProcessingMetadata); } /** Merge data of two scopes together. */ @@ -163,15 +163,10 @@ function applyBreadcrumbsToEvent(event: Event, breadcrumbs: Breadcrumb[]): void event.breadcrumbs = mergedBreadcrumbs.length ? mergedBreadcrumbs : undefined; } -function applySdkMetadataToEvent( - event: Event, - sdkProcessingMetadata: ScopeData['sdkProcessingMetadata'], - propagationContext: PropagationContext, -): void { +function applySdkMetadataToEvent(event: Event, sdkProcessingMetadata: ScopeData['sdkProcessingMetadata']): void { event.sdkProcessingMetadata = { ...event.sdkProcessingMetadata, ...sdkProcessingMetadata, - propagationContext: propagationContext, }; } diff --git a/packages/core/test/lib/prepareEvent.test.ts b/packages/core/test/lib/prepareEvent.test.ts index 22f62bbcf89c..cb3c616b7390 100644 --- a/packages/core/test/lib/prepareEvent.test.ts +++ b/packages/core/test/lib/prepareEvent.test.ts @@ -228,12 +228,7 @@ describe('prepareEvent', () => { event_id: expect.any(String), environment: 'production', message: 'foo', - sdkProcessingMetadata: { - propagationContext: { - spanId: expect.any(String), - traceId: expect.any(String), - }, - }, + sdkProcessingMetadata: {}, }); }); @@ -309,16 +304,15 @@ describe('prepareEvent', () => { user: { id: '1', email: 'test@example.com' }, tags: { tag1: 'aa', tag2: 'aa' }, extra: { extra1: 'aa', extra2: 'aa' }, - contexts: { os: { name: 'os1' }, culture: { display_name: 'name1' } }, + contexts: { + os: { name: 'os1' }, + culture: { display_name: 'name1' }, + }, fingerprint: ['dd', 'aa'], breadcrumbs: [breadcrumb4, breadcrumb2, breadcrumb3, breadcrumb1], sdkProcessingMetadata: { aa: 'aa', bb: 'bb', - propagationContext: { - spanId: '1', - traceId: '1', - }, }, }); }); @@ -382,7 +376,6 @@ describe('prepareEvent', () => { sdkProcessingMetadata: { aa: 'aa', bb: 'bb', - propagationContext: isolationScope.getPropagationContext(), }, }); }); diff --git a/packages/core/test/lib/scope.test.ts b/packages/core/test/lib/scope.test.ts index e4510ef58e3b..4b4242ce7dc6 100644 --- a/packages/core/test/lib/scope.test.ts +++ b/packages/core/test/lib/scope.test.ts @@ -132,12 +132,7 @@ describe('Scope', () => { expect(event).toEqual({ message: 'foo', - sdkProcessingMetadata: { - propagationContext: { - spanId: expect.any(String), - traceId: expect.any(String), - }, - }, + sdkProcessingMetadata: {}, }); }); @@ -166,15 +161,14 @@ describe('Scope', () => { user: { id: '1', email: 'test@example.com' }, tags: { tag1: 'aa', tag2: 'aa' }, extra: { extra1: 'aa', extra2: 'aa' }, - contexts: { os: { name: 'os1' }, culture: { display_name: 'name1' } }, + contexts: { + os: { name: 'os1' }, + culture: { display_name: 'name1' }, + }, fingerprint: ['dd', 'aa'], breadcrumbs: [breadcrumb2, breadcrumb1], sdkProcessingMetadata: { aa: 'aa', - propagationContext: { - spanId: '1', - traceId: '1', - }, }, }); }); diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index a2bdac43b6dd..16ed09c2f818 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -455,6 +455,7 @@ describe('continueTrace', () => { const scope = getCurrentScope(); expect(scope.getPropagationContext()).toEqual({ + dsc: {}, // DSC should be an empty object (frozen), because there was an incoming trace sampled: false, parentSpanId: '1121201211212012', spanId: expect.any(String), diff --git a/packages/hub/test/scope.test.ts b/packages/hub/test/scope.test.ts index b8cfaf1914e0..08f4a73abcb2 100644 --- a/packages/hub/test/scope.test.ts +++ b/packages/hub/test/scope.test.ts @@ -261,8 +261,6 @@ describe('Scope', () => { expect(processedEvent!.contexts).toEqual({ os: { id: '1' } }); expect(processedEvent!.sdkProcessingMetadata).toEqual({ dogs: 'are great!', - // @ts-expect-error accessing private property for test - propagationContext: scope._propagationContext, }); }); diff --git a/packages/node-experimental/test/integration/transactions.test.ts b/packages/node-experimental/test/integration/transactions.test.ts index b1038a53a276..929b286452f3 100644 --- a/packages/node-experimental/test/integration/transactions.test.ts +++ b/packages/node-experimental/test/integration/transactions.test.ts @@ -108,11 +108,6 @@ describe('Integration | Transactions', () => { trace_id: expect.any(String), transaction: 'test name', }), - propagationContext: { - sampled: undefined, - spanId: expect.any(String), - traceId: expect.any(String), - }, sampleRate: 1, source: 'task', spanMetadata: expect.any(Object), diff --git a/packages/node-experimental/test/sdk/scope.test.ts b/packages/node-experimental/test/sdk/scope.test.ts index e3919654e920..076188d9c9b7 100644 --- a/packages/node-experimental/test/sdk/scope.test.ts +++ b/packages/node-experimental/test/sdk/scope.test.ts @@ -130,12 +130,7 @@ describe('Unit | Scope', () => { event_id: expect.any(String), environment: 'production', message: 'foo', - sdkProcessingMetadata: { - propagationContext: { - spanId: expect.any(String), - traceId: expect.any(String), - }, - }, + sdkProcessingMetadata: {}, }); }); @@ -213,16 +208,15 @@ describe('Unit | Scope', () => { user: { id: '1', email: 'test@example.com' }, tags: { tag1: 'aa', tag2: 'aa' }, extra: { extra1: 'aa', extra2: 'aa' }, - contexts: { os: { name: 'os1' }, culture: { display_name: 'name1' } }, + contexts: { + os: { name: 'os1' }, + culture: { display_name: 'name1' }, + }, fingerprint: ['dd', 'aa'], breadcrumbs: [breadcrumb4, breadcrumb2, breadcrumb3, breadcrumb1], sdkProcessingMetadata: { aa: 'aa', bb: 'bb', - propagationContext: { - spanId: '1', - traceId: '1', - }, }, }); }); diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index e3f6d164d991..cd7a93d79bcc 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -1,6 +1,7 @@ import type * as http from 'http'; import type * as https from 'https'; import type { Hub } from '@sentry/core'; +import { getIsolationScope } from '@sentry/core'; import { addBreadcrumb, getActiveSpan, @@ -13,13 +14,7 @@ import { spanToJSON, spanToTraceHeader, } from '@sentry/core'; -import type { - DynamicSamplingContext, - EventProcessor, - Integration, - SanitizedRequestData, - TracePropagationTargets, -} from '@sentry/types'; +import type { EventProcessor, Integration, SanitizedRequestData, TracePropagationTargets } from '@sentry/types'; import { LRUMap, dynamicSamplingContextToSentryBaggageHeader, @@ -250,13 +245,15 @@ function _createWrappedRequestMethodFactory( // eslint-disable-next-line deprecation/deprecation const rawRequestUrl = extractRawUrl(requestOptions); const requestUrl = extractUrl(requestOptions); + const client = getClient(); // we don't want to record requests to Sentry as either breadcrumbs or spans, so just use the original method - if (isSentryRequestUrl(requestUrl, getClient())) { + if (isSentryRequestUrl(requestUrl, client)) { return originalRequestMethod.apply(httpModule, requestArgs); } const scope = getCurrentScope(); + const isolationScope = getIsolationScope(); const parentSpan = getActiveSpan(); const data = getRequestSpanData(requestUrl, requestOptions); @@ -271,19 +268,24 @@ function _createWrappedRequestMethodFactory( }) : undefined; - if (shouldAttachTraceData(rawRequestUrl)) { - if (requestSpan) { - const sentryTraceHeader = spanToTraceHeader(requestSpan); - const dynamicSamplingContext = getDynamicSamplingContextFromSpan(requestSpan); - addHeadersToRequestOptions(requestOptions, requestUrl, sentryTraceHeader, dynamicSamplingContext); - } else { - const client = getClient(); - const { traceId, sampled, dsc } = scope.getPropagationContext(); - const sentryTraceHeader = generateSentryTraceHeader(traceId, undefined, sampled); - const dynamicSamplingContext = - dsc || (client ? getDynamicSamplingContextFromClient(traceId, client, scope) : undefined); - addHeadersToRequestOptions(requestOptions, requestUrl, sentryTraceHeader, dynamicSamplingContext); - } + if (client && shouldAttachTraceData(rawRequestUrl)) { + const { traceId, spanId, sampled, dsc } = { + ...isolationScope.getPropagationContext(), + ...scope.getPropagationContext(), + }; + + const sentryTraceHeader = requestSpan + ? spanToTraceHeader(requestSpan) + : generateSentryTraceHeader(traceId, spanId, sampled); + + const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader( + dsc || + (requestSpan + ? getDynamicSamplingContextFromSpan(requestSpan) + : getDynamicSamplingContextFromClient(traceId, client, scope)), + ); + + addHeadersToRequestOptions(requestOptions, requestUrl, sentryTraceHeader, sentryBaggageHeader); } else { DEBUG_BUILD && logger.log( @@ -333,7 +335,7 @@ function addHeadersToRequestOptions( requestOptions: RequestOptions, requestUrl: string, sentryTraceHeader: string, - dynamicSamplingContext: Partial | undefined, + sentryBaggageHeader: string | undefined, ): void { // Don't overwrite sentry-trace and baggage header if it's already set. const headers = requestOptions.headers || {}; @@ -343,15 +345,13 @@ function addHeadersToRequestOptions( DEBUG_BUILD && logger.log(`[Tracing] Adding sentry-trace header ${sentryTraceHeader} to outgoing request to "${requestUrl}": `); - const sentryBaggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); - const sentryBaggageHeader = - sentryBaggage && sentryBaggage.length > 0 ? normalizeBaggageHeader(requestOptions, sentryBaggage) : undefined; requestOptions.headers = { ...requestOptions.headers, 'sentry-trace': sentryTraceHeader, // Setting a header to `undefined` will crash in node so we only set the baggage header when it's defined - ...(sentryBaggageHeader && { baggage: sentryBaggageHeader }), + ...(sentryBaggageHeader && + sentryBaggageHeader.length > 0 && { baggage: normalizeBaggageHeader(requestOptions, sentryBaggageHeader) }), }; } diff --git a/packages/node/src/integrations/undici/index.ts b/packages/node/src/integrations/undici/index.ts index ef875891b3ef..ff3cca79ead6 100644 --- a/packages/node/src/integrations/undici/index.ts +++ b/packages/node/src/integrations/undici/index.ts @@ -5,6 +5,7 @@ import { getCurrentScope, getDynamicSamplingContextFromClient, getDynamicSamplingContextFromSpan, + getIsolationScope, isSentryRequestUrl, spanToTraceHeader, } from '@sentry/core'; @@ -157,6 +158,7 @@ export class Undici implements Integration { const clientOptions = client.getOptions(); const scope = getCurrentScope(); + const isolationScope = getIsolationScope(); const parentSpan = getActiveSpan(); const span = this._shouldCreateSpan(stringUrl) ? createRequestSpan(parentSpan, request, stringUrl) : undefined; @@ -180,18 +182,21 @@ export class Undici implements Integration { }; if (shouldAttachTraceData(stringUrl)) { - if (span) { - const dynamicSamplingContext = getDynamicSamplingContextFromSpan(span); - const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); - - setHeadersOnRequest(request, spanToTraceHeader(span), sentryBaggageHeader); - } else { - const { traceId, sampled, dsc } = scope.getPropagationContext(); - const sentryTrace = generateSentryTraceHeader(traceId, undefined, sampled); - const dynamicSamplingContext = dsc || getDynamicSamplingContextFromClient(traceId, client, scope); - const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); - setHeadersOnRequest(request, sentryTrace, sentryBaggageHeader); - } + const { traceId, spanId, sampled, dsc } = { + ...isolationScope.getPropagationContext(), + ...scope.getPropagationContext(), + }; + + const sentryTraceHeader = span ? spanToTraceHeader(span) : generateSentryTraceHeader(traceId, spanId, sampled); + + const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader( + dsc || + (span + ? getDynamicSamplingContextFromSpan(span) + : getDynamicSamplingContextFromClient(traceId, client, scope)), + ); + + setHeadersOnRequest(request, sentryTraceHeader, sentryBaggageHeader); } }; diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index d9001f6ffdd9..8438a3fc2acd 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -270,6 +270,7 @@ describe('tracingHandler', () => { parentSpanId: '1121201211212012', spanId: expect.any(String), sampled: false, + dsc: {}, // There is an incoming trace but no baggage header, so the DSC must be frozen (empty object) }); // since we have no tracesSampler defined, the default behavior (inherit if possible) applies diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts index 0768541fbfb9..bbeb2744e501 100644 --- a/packages/opentelemetry/src/propagator.ts +++ b/packages/opentelemetry/src/propagator.ts @@ -83,7 +83,7 @@ function getDsc( context: Context, propagationContext: PropagationContext, traceId: string | undefined, -): DynamicSamplingContext | undefined { +): Partial | undefined { // If we have a DSC on the propagation context, we just use it if (propagationContext.dsc) { return propagationContext.dsc; diff --git a/packages/opentelemetry/src/spanExporter.ts b/packages/opentelemetry/src/spanExporter.ts index ca2f1deb4fc0..0d57d1009e31 100644 --- a/packages/opentelemetry/src/spanExporter.ts +++ b/packages/opentelemetry/src/spanExporter.ts @@ -4,7 +4,7 @@ import { ExportResultCode } from '@opentelemetry/core'; import type { ReadableSpan, SpanExporter } from '@opentelemetry/sdk-trace-base'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; import { flush, getCurrentScope } from '@sentry/core'; -import type { DynamicSamplingContext, Scope, Span as SentrySpan, SpanOrigin, TransactionSource } from '@sentry/types'; +import type { Scope, Span as SentrySpan, SpanOrigin, TransactionSource } from '@sentry/types'; import { logger } from '@sentry/utils'; import { getCurrentHub } from './custom/hub'; @@ -158,9 +158,7 @@ function createTransactionForOtelSpan(span: ReadableSpan): OpenTelemetryTransact const parentSpanId = span.parentSpanId; const parentSampled = span.attributes[InternalSentrySemanticAttributes.PARENT_SAMPLED] as boolean | undefined; - const dynamicSamplingContext: DynamicSamplingContext | undefined = scope - ? scope.getPropagationContext().dsc - : undefined; + const dynamicSamplingContext = scope ? scope.getPropagationContext().dsc : undefined; const { op, description, tags, data, origin, source } = getSpanData(span); const metadata = getSpanMetadata(span); diff --git a/packages/opentelemetry/test/integration/scope.test.ts b/packages/opentelemetry/test/integration/scope.test.ts index 6ab7c6d35621..d0234b27a140 100644 --- a/packages/opentelemetry/test/integration/scope.test.ts +++ b/packages/opentelemetry/test/integration/scope.test.ts @@ -69,7 +69,6 @@ describe('Integration | Scope', () => { ? { span_id: spanId, trace_id: traceId, - parent_span_id: undefined, } : expect.any(Object), }), @@ -190,7 +189,6 @@ describe('Integration | Scope', () => { ? { span_id: spanId1, trace_id: traceId1, - parent_span_id: undefined, } : expect.any(Object), }), diff --git a/packages/opentelemetry/test/integration/transactions.test.ts b/packages/opentelemetry/test/integration/transactions.test.ts index a87816691a45..5c4742df5f97 100644 --- a/packages/opentelemetry/test/integration/transactions.test.ts +++ b/packages/opentelemetry/test/integration/transactions.test.ts @@ -106,11 +106,6 @@ describe('Integration | Transactions', () => { trace_id: expect.any(String), transaction: 'test name', }), - propagationContext: { - sampled: undefined, - spanId: expect.any(String), - traceId: expect.any(String), - }, sampleRate: 1, source: 'task', spanMetadata: expect.any(Object), diff --git a/packages/opentelemetry/test/propagator.test.ts b/packages/opentelemetry/test/propagator.test.ts index 1663021b6224..a4eb98ab2126 100644 --- a/packages/opentelemetry/test/propagator.test.ts +++ b/packages/opentelemetry/test/propagator.test.ts @@ -310,6 +310,7 @@ describe('SentryPropagator', () => { parentSpanId: '6e0c63257de34c92', spanId: expect.any(String), traceId: 'd4cda95b652f4a1592b449d5929fda1b', + dsc: {}, // Frozen DSC }); // Ensure spanId !== parentSpanId - it should be a new random ID @@ -321,19 +322,21 @@ describe('SentryPropagator', () => { carrier[SENTRY_TRACE_HEADER] = sentryTraceHeader; const context = propagator.extract(ROOT_CONTEXT, carrier, defaultTextMapGetter); expect(getPropagationContextFromContext(context)).toEqual({ - sampled: undefined, spanId: expect.any(String), traceId: expect.any(String), }); }); it('sets defined dynamic sampling context on context', () => { + const sentryTraceHeader = 'd4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-1'; const baggage = 'sentry-environment=production,sentry-release=1.0.0,sentry-public_key=abc,sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b,sentry-transaction=dsc-transaction'; + carrier[SENTRY_TRACE_HEADER] = sentryTraceHeader; carrier[SENTRY_BAGGAGE_HEADER] = baggage; const context = propagator.extract(ROOT_CONTEXT, carrier, defaultTextMapGetter); expect(getPropagationContextFromContext(context)).toEqual({ - sampled: undefined, + sampled: true, + parentSpanId: expect.any(String), spanId: expect.any(String), traceId: expect.any(String), // Note: This is not automatically taken from the DSC (in reality, this should be aligned) dsc: { diff --git a/packages/tracing-internal/src/browser/browsertracing.ts b/packages/tracing-internal/src/browser/browsertracing.ts index e9f61c73c0f3..276f306e4c49 100644 --- a/packages/tracing-internal/src/browser/browsertracing.ts +++ b/packages/tracing-internal/src/browser/browsertracing.ts @@ -5,8 +5,6 @@ import { TRACING_DEFAULTS, addTracingExtensions, getActiveTransaction, - spanIsSampled, - spanToJSON, startIdleTransaction, } from '@sentry/core'; import type { EventProcessor, Integration, Transaction, TransactionContext, TransactionSource } from '@sentry/types'; @@ -310,23 +308,27 @@ export class BrowserTracing implements Integration { const isPageloadTransaction = context.op === 'pageload'; - const sentryTrace = isPageloadTransaction ? getMetaContent('sentry-trace') : ''; - const baggage = isPageloadTransaction ? getMetaContent('baggage') : ''; - const { traceparentData, dynamicSamplingContext, propagationContext } = tracingContextFromHeaders( - sentryTrace, - baggage, - ); - - const expandedContext: TransactionContext = { - ...context, - ...traceparentData, - metadata: { - // eslint-disable-next-line deprecation/deprecation - ...context.metadata, - dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, - }, - trimEnd: true, - }; + let expandedContext: TransactionContext; + if (isPageloadTransaction) { + const sentryTrace = isPageloadTransaction ? getMetaContent('sentry-trace') : ''; + const baggage = isPageloadTransaction ? getMetaContent('baggage') : undefined; + const { traceparentData, dynamicSamplingContext } = tracingContextFromHeaders(sentryTrace, baggage); + expandedContext = { + ...context, + ...traceparentData, + metadata: { + // eslint-disable-next-line deprecation/deprecation + ...context.metadata, + dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, + }, + trimEnd: true, + }; + } else { + expandedContext = { + ...context, + trimEnd: true, + }; + } const modifiedContext = typeof beforeNavigate === 'function' ? beforeNavigate(expandedContext) : expandedContext; @@ -384,24 +386,6 @@ export class BrowserTracing implements Integration { } } - // eslint-disable-next-line deprecation/deprecation - const scope = hub.getScope(); - - // If it's a pageload and there is a meta tag set - // use the traceparentData as the propagation context - if (isPageloadTransaction && traceparentData) { - scope.setPropagationContext(propagationContext); - } else { - // Navigation transactions should set a new propagation context based on the - // created idle transaction. - scope.setPropagationContext({ - traceId: idleTransaction.spanContext().traceId, - spanId: idleTransaction.spanContext().spanId, - parentSpanId: spanToJSON(idleTransaction).parent_span_id, - sampled: spanIsSampled(idleTransaction), - }); - } - idleTransaction.registerBeforeFinishCallback(transaction => { this._collectWebVitals(); addPerformanceEntries(transaction); diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index c84d0545054b..99930a990fe3 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -4,7 +4,7 @@ import { getCurrentScope, getDynamicSamplingContextFromClient, getDynamicSamplingContextFromSpan, - getRootSpan, + getIsolationScope, hasTracingEnabled, spanToJSON, spanToTraceHeader, @@ -275,6 +275,7 @@ export function xhrCallback( } const scope = getCurrentScope(); + const isolationScope = getIsolationScope(); const span = shouldCreateSpanResult ? startInactiveSpan({ @@ -294,21 +295,22 @@ export function xhrCallback( spans[xhr.__sentry_xhr_span_id__] = span; } - if (xhr.setRequestHeader && shouldAttachHeaders(sentryXhrData.url)) { - if (span) { - const transaction = span && getRootSpan(span); - const dynamicSamplingContext = transaction && getDynamicSamplingContextFromSpan(transaction); - const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); - setHeaderOnXhr(xhr, spanToTraceHeader(span), sentryBaggageHeader); - } else { - const client = getClient(); - const { traceId, sampled, dsc } = scope.getPropagationContext(); - const sentryTraceHeader = generateSentryTraceHeader(traceId, undefined, sampled); - const dynamicSamplingContext = - dsc || (client ? getDynamicSamplingContextFromClient(traceId, client, scope) : undefined); - const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); - setHeaderOnXhr(xhr, sentryTraceHeader, sentryBaggageHeader); - } + const client = getClient(); + + if (xhr.setRequestHeader && shouldAttachHeaders(sentryXhrData.url) && client) { + const { traceId, spanId, sampled, dsc } = { + ...isolationScope.getPropagationContext(), + ...scope.getPropagationContext(), + }; + + const sentryTraceHeader = span ? spanToTraceHeader(span) : generateSentryTraceHeader(traceId, spanId, sampled); + + const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader( + dsc || + (span ? getDynamicSamplingContextFromSpan(span) : getDynamicSamplingContextFromClient(traceId, client, scope)), + ); + + setHeaderOnXhr(xhr, sentryTraceHeader, sentryBaggageHeader); } return span; diff --git a/packages/tracing-internal/src/common/fetch.ts b/packages/tracing-internal/src/common/fetch.ts index c96778f8cd35..272fa8a96836 100644 --- a/packages/tracing-internal/src/common/fetch.ts +++ b/packages/tracing-internal/src/common/fetch.ts @@ -3,7 +3,7 @@ import { getCurrentScope, getDynamicSamplingContextFromClient, getDynamicSamplingContextFromSpan, - getRootSpan, + getIsolationScope, hasTracingEnabled, spanToTraceHeader, startInactiveSpan, @@ -132,18 +132,19 @@ export function addTracingHeadersToFetchRequest( // eslint-disable-next-line deprecation/deprecation const span = requestSpan || scope.getSpan(); - const transaction = span && getRootSpan(span); + const isolationScope = getIsolationScope(); - const { traceId, sampled, dsc } = scope.getPropagationContext(); + const { traceId, spanId, sampled, dsc } = { + ...isolationScope.getPropagationContext(), + ...scope.getPropagationContext(), + }; - const sentryTraceHeader = span ? spanToTraceHeader(span) : generateSentryTraceHeader(traceId, undefined, sampled); - const dynamicSamplingContext = transaction - ? getDynamicSamplingContextFromSpan(transaction) - : dsc - ? dsc - : getDynamicSamplingContextFromClient(traceId, client, scope); + const sentryTraceHeader = span ? spanToTraceHeader(span) : generateSentryTraceHeader(traceId, spanId, sampled); - const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); + const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader( + dsc || + (span ? getDynamicSamplingContextFromSpan(span) : getDynamicSamplingContextFromClient(traceId, client, scope)), + ); const headers = options.headers || diff --git a/packages/types/src/tracing.ts b/packages/types/src/tracing.ts index 7c5b02c45596..701f9930d314 100644 --- a/packages/types/src/tracing.ts +++ b/packages/types/src/tracing.ts @@ -2,10 +2,43 @@ import type { DynamicSamplingContext } from './envelope'; export type TracePropagationTargets = (string | RegExp)[]; +/** + * `PropagationContext` represents the data from an incoming trace. It should be constructed from incoming trace data, + * usually represented by `sentry-trace` and `baggage` HTTP headers. + * + * There is always a propagation context present in the SDK (or rather on Scopes), holding at least a `traceId`. This is + * to ensure that there is always a trace we can attach events onto, even if performance monitoring is disabled. If + * there was no incoming `traceId`, the `traceId` will be generated by the current SDK. + */ export interface PropagationContext { + /** + * Either represents the incoming `traceId` or the `traceId` generated by the current SDK, if there was no incoming trace. + */ traceId: string; + /** + * Represents the execution context of the current SDK. This acts as a fallback value to associate events with a + * particular execution context when performance monitoring is disabled. + * + * The ID of a current span (if one exists) should have precedence over this value when propagating trace data. + */ spanId: string; + /** + * Represents the sampling decision of the incoming trace. + * + * The current SDK should not modify this value! + */ sampled?: boolean; + /** + * The `parentSpanId` denotes the ID of the incoming client span. If there is no `parentSpanId` on the propagation + * context, it means that the the incoming trace didn't come from a span. + * + * The current SDK should not modify this value! + */ parentSpanId?: string; - dsc?: DynamicSamplingContext; + /** + * An undefined dsc in the propagation context means that the current SDK invocation is the head of trace and still free to modify and set the DSC for outgoing requests. + * + * The current SDK should not modify this value! + */ + dsc?: Partial; } diff --git a/packages/utils/src/tracing.ts b/packages/utils/src/tracing.ts index 438af21ac744..b0c25e7f1935 100644 --- a/packages/utils/src/tracing.ts +++ b/packages/utils/src/tracing.ts @@ -1,4 +1,4 @@ -import type { DynamicSamplingContext, PropagationContext, TraceparentData } from '@sentry/types'; +import type { PropagationContext, TraceparentData } from '@sentry/types'; import { baggageHeaderToDynamicSamplingContext } from './baggage'; import { uuid4 } from './misc'; @@ -59,25 +59,28 @@ export function tracingContextFromHeaders( const { traceId, parentSpanId, parentSampled } = traceparentData || {}; - const propagationContext: PropagationContext = { - traceId: traceId || uuid4(), - spanId: uuid4().substring(16), - sampled: parentSampled, - }; - - if (parentSpanId) { - propagationContext.parentSpanId = parentSpanId; - } - - if (dynamicSamplingContext) { - propagationContext.dsc = dynamicSamplingContext as DynamicSamplingContext; + if (!traceparentData) { + return { + traceparentData, + dynamicSamplingContext: undefined, + propagationContext: { + traceId: traceId || uuid4(), + spanId: uuid4().substring(16), + }, + }; + } else { + return { + traceparentData, + dynamicSamplingContext: dynamicSamplingContext || {}, // If we have traceparent data but no DSC it means we are not head of trace and we must freeze it + propagationContext: { + traceId: traceId || uuid4(), + parentSpanId: parentSpanId || uuid4().substring(16), + spanId: uuid4().substring(16), + sampled: parentSampled, + dsc: dynamicSamplingContext || {}, // If we have traceparent data but no DSC it means we are not head of trace and we must freeze it + }, + }; } - - return { - traceparentData, - dynamicSamplingContext, - propagationContext, - }; } /** diff --git a/packages/utils/test/tracing.test.ts b/packages/utils/test/tracing.test.ts new file mode 100644 index 000000000000..2e7cc4d3d5a5 --- /dev/null +++ b/packages/utils/test/tracing.test.ts @@ -0,0 +1,9 @@ +import { tracingContextFromHeaders } from '../src/tracing'; + +describe('tracingContextFromHeaders()', () => { + it('should produce a frozen baggage (empty object) when there is an incoming trace but no baggage header', () => { + const tracingContext = tracingContextFromHeaders('12312012123120121231201212312012-1121201211212012-1', undefined); + expect(tracingContext.dynamicSamplingContext).toEqual({}); + expect(tracingContext.propagationContext.dsc).toEqual({}); + }); +}); From 168083764fede3e13b54debec98d6834b09f7e8d Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 25 Jan 2024 15:13:55 +0100 Subject: [PATCH 2/5] last test= --- .../node-experimental-fastify-app/tests/propagation.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/dev-packages/e2e-tests/test-applications/node-experimental-fastify-app/tests/propagation.test.ts b/dev-packages/e2e-tests/test-applications/node-experimental-fastify-app/tests/propagation.test.ts index fe395b1ccce8..9b83e882a739 100644 --- a/dev-packages/e2e-tests/test-applications/node-experimental-fastify-app/tests/propagation.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-experimental-fastify-app/tests/propagation.test.ts @@ -164,7 +164,6 @@ test('Propagates trace for outgoing fetch requests', async ({ baseURL }) => { 'sentry.origin': 'auto.http.otel.http', }, op: 'http.server', - parent_span_id: expect.any(String), span_id: expect.any(String), status: 'ok', tags: { From 331489297f62042cba794385bb31020212f9ea80 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 29 Jan 2024 08:30:57 +0000 Subject: [PATCH 3/5] Lint --- CONTRIBUTING.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9ab617cdeffe..812aaa870df5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -37,10 +37,11 @@ able to use it. From the top level of the repo, there are three commands availab dependencies (`utils`, `core`, `browser`, etc), and all packages which depend on it (currently `gatsby` and `nextjs`)) - `yarn build:dev:watch`, which runs `yarn build:dev` in watch mode (recommended) -You can also run a production build via `yarn build`, which will build everything except for the tarballs for publishing to NPM. -You can use this if you want to bundle Sentry yourself. The build output can be found in the packages `build/` folder, e.g. `packages/browser/build`. -Bundled files can be found in `packages/browser/build/bundles`. -Note that there are no guarantees about the produced file names etc., so make sure to double check which files are generated after upgrading. +You can also run a production build via `yarn build`, which will build everything except for the tarballs for publishing +to NPM. You can use this if you want to bundle Sentry yourself. The build output can be found in the packages `build/` +folder, e.g. `packages/browser/build`. Bundled files can be found in `packages/browser/build/bundles`. Note that there +are no guarantees about the produced file names etc., so make sure to double check which files are generated after +upgrading. ## Testing SDK Packages Locally From 290137287f4f30b1fd3aecd073627408750cf49a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 29 Jan 2024 13:33:22 +0000 Subject: [PATCH 4/5] Update new browser tracing integration --- .../src/browser/browserTracingIntegration.ts | 56 +++++++------------ 1 file changed, 21 insertions(+), 35 deletions(-) diff --git a/packages/tracing-internal/src/browser/browserTracingIntegration.ts b/packages/tracing-internal/src/browser/browserTracingIntegration.ts index 8184ad058039..7df259405611 100644 --- a/packages/tracing-internal/src/browser/browserTracingIntegration.ts +++ b/packages/tracing-internal/src/browser/browserTracingIntegration.ts @@ -199,23 +199,27 @@ export const _browserTracingIntegration = ((_options: Partial { _collectWebVitals(); addPerformanceEntries(transaction); From 560cdc4a195d246567556cbb0c503e470f0ef507 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 29 Jan 2024 14:00:48 +0000 Subject: [PATCH 5/5] lint --- .../tracing-internal/src/browser/browserTracingIntegration.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/tracing-internal/src/browser/browserTracingIntegration.ts b/packages/tracing-internal/src/browser/browserTracingIntegration.ts index 7df259405611..59d15d9588ee 100644 --- a/packages/tracing-internal/src/browser/browserTracingIntegration.ts +++ b/packages/tracing-internal/src/browser/browserTracingIntegration.ts @@ -7,7 +7,6 @@ import { TRACING_DEFAULTS, addTracingExtensions, getActiveTransaction, - spanIsSampled, spanToJSON, startIdleTransaction, } from '@sentry/core';