From cb77638c92d70572fe1ba0fd8f0a61a69f22ee6f Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 16 Apr 2024 15:51:53 +0200 Subject: [PATCH 1/4] feat(browser): Update `propagationContext` on `spanEnd` to keep trace consistent --- .../tracing/trace-lifetime/navigation/test.ts | 12 ++++----- .../tracing/trace-lifetime/pageload/test.ts | 12 ++++----- .../src/tracing/browserTracingIntegration.ts | 25 +++++++++++++++++++ 3 files changed, 37 insertions(+), 12 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts index 4bf1d1f7d49a..f24ca0507c66 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts @@ -130,11 +130,11 @@ sentryTest( const request = await requestPromise; const headers = request.headers(); - // sampling decision is deferred b/c of no active span at the time of request + // sampling decision and DSC are continued from navigation span, even after it ended const navigationTraceId = navigationTraceContext?.trace_id; - expect(headers['sentry-trace']).toMatch(new RegExp(`^${navigationTraceId}-[0-9a-f]{16}$`)); + expect(headers['sentry-trace']).toMatch(new RegExp(`^${navigationTraceId}-[0-9a-f]{16}-1$`)); expect(headers['baggage']).toEqual( - `sentry-environment=production,sentry-public_key=public,sentry-trace_id=${navigationTraceId}`, + `sentry-environment=production,sentry-public_key=public,sentry-trace_id=${navigationTraceId},sentry-sample_rate=1,sentry-sampled=true`, ); }, ); @@ -203,11 +203,11 @@ sentryTest( const request = await xhrPromise; const headers = request.headers(); - // sampling decision is deferred b/c of no active span at the time of request + // sampling decision and DSC are continued from navigation span, even after it ended const navigationTraceId = navigationTraceContext?.trace_id; - expect(headers['sentry-trace']).toMatch(new RegExp(`^${navigationTraceId}-[0-9a-f]{16}$`)); + expect(headers['sentry-trace']).toMatch(new RegExp(`^${navigationTraceId}-[0-9a-f]{16}-1$`)); expect(headers['baggage']).toEqual( - `sentry-environment=production,sentry-public_key=public,sentry-trace_id=${navigationTraceId}`, + `sentry-environment=production,sentry-public_key=public,sentry-trace_id=${navigationTraceId},sentry-sample_rate=1,sentry-sampled=true`, ); }, ); diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts index 333381d28b9a..dc87dea9760b 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts @@ -124,11 +124,11 @@ sentryTest( const request = await requestPromise; const headers = request.headers(); - // sampling decision is deferred b/c of no active span at the time of request + // sampling decision and DSC are continued from the pageload span even after it ended const pageloadTraceId = pageloadTraceContext?.trace_id; - expect(headers['sentry-trace']).toMatch(new RegExp(`^${pageloadTraceId}-[0-9a-f]{16}$`)); + expect(headers['sentry-trace']).toMatch(new RegExp(`^${pageloadTraceId}-[0-9a-f]{16}-1$`)); expect(headers['baggage']).toEqual( - `sentry-environment=production,sentry-public_key=public,sentry-trace_id=${pageloadTraceId}`, + `sentry-environment=production,sentry-public_key=public,sentry-trace_id=${pageloadTraceId},sentry-sample_rate=1,sentry-sampled=true`, ); }, ); @@ -191,11 +191,11 @@ sentryTest( const request = await requestPromise; const headers = request.headers(); - // sampling decision is deferred b/c of no active span at the time of request + // sampling decision and DSC are continued from the pageload span even after it ended const pageloadTraceId = pageloadTraceContext?.trace_id; - expect(headers['sentry-trace']).toMatch(new RegExp(`^${pageloadTraceId}-[0-9a-f]{16}$`)); + expect(headers['sentry-trace']).toMatch(new RegExp(`^${pageloadTraceId}-[0-9a-f]{16}-1$`)); expect(headers['baggage']).toEqual( - `sentry-environment=production,sentry-public_key=public,sentry-trace_id=${pageloadTraceId}`, + `sentry-environment=production,sentry-public_key=public,sentry-trace_id=${pageloadTraceId},sentry-sample_rate=1,sentry-sampled=true`, ); }, ); diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index 64510fc0b93f..b6d42f6da8a8 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -14,9 +14,11 @@ import { getActiveSpan, getClient, getCurrentScope, + getDynamicSamplingContextFromSpan, getIsolationScope, getRootSpan, registerSpanErrorInstrumentation, + spanIsSampled, spanToJSON, startIdleSpan, withScope, @@ -282,6 +284,29 @@ export const browserTracingIntegration = ((_options: Partial { + const op = spanToJSON(span).op; + if (span !== getRootSpan(span) || (op !== 'navigation' && op !== 'pageload')) { + return; + } + + const scope = getCurrentScope(); + + // A trace should to stay the same for the entire time span of one route. + // Therefore, when the initial pageload or navigation transaction is ended, we update the + // scope's propagation context to keep span-specific attributes like the `sampled` decision and + // the dynamic sampling context valid, even after the transaction has ended. + // This ensures that the trace data is consistent for the entire duration of the route. + const newPropagationContext = { + ...scope.getPropagationContext(), + // it's okay to always set sampled: true/false here. If we have a span, it cannot be un-sampled. + sampled: spanIsSampled(span), + dsc: getDynamicSamplingContextFromSpan(span), + }; + + scope.setPropagationContext(newPropagationContext); + }); + if (options.instrumentPageLoad && WINDOW.location) { const startSpanOptions: StartSpanOptions = { name: WINDOW.location.pathname, From 7197b8c213bc42f883f5c09798bf4c1c04ae65f6 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 16 Apr 2024 16:12:51 +0200 Subject: [PATCH 2/4] add unit test --- .../src/tracing/browserTracingIntegration.ts | 18 ++++---- .../tracing/browserTracingIntegration.test.ts | 41 ++++++++++++++++++- 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index b6d42f6da8a8..8e61c95106e1 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -284,6 +284,11 @@ export const browserTracingIntegration = ((_options: Partial { const op = spanToJSON(span).op; if (span !== getRootSpan(span) || (op !== 'navigation' && op !== 'pageload')) { @@ -291,17 +296,12 @@ export const browserTracingIntegration = ((_options: Partial { expect(getCurrentScope().getScopeData().transactionName).toBe('test navigation span'); }); - it("resets the scopes' propagationContexts", () => { + it("updates the scopes' propagationContexts on a navigation", () => { const client = new BrowserClient( getDefaultBrowserClientOptions({ integrations: [browserTracingIntegration()], @@ -675,6 +675,45 @@ describe('browserTracingIntegration', () => { expect(newIsolationScopePropCtx?.traceId).not.toEqual(oldIsolationScopePropCtx?.traceId); expect(newCurrentScopePropCtx?.traceId).not.toEqual(oldCurrentScopePropCtx?.traceId); }); + + it("saves the span's sampling decision and its DSC on the propagationContext when the span finishes", () => { + const client = new BrowserClient( + getDefaultBrowserClientOptions({ + tracesSampleRate: 1, + integrations: [browserTracingIntegration({ instrumentPageLoad: false })], + }), + ); + setCurrentClient(client); + client.init(); + + const navigationSpan = startBrowserTracingNavigationSpan(client, { + name: 'mySpan', + attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route' }, + }); + + const propCtxBeforeEnd = getCurrentScope().getPropagationContext(); + expect(propCtxBeforeEnd).toStrictEqual({ + spanId: expect.stringMatching(/[a-f0-9]{16}/), + traceId: expect.stringMatching(/[a-f0-9]{32}/), + }); + + navigationSpan!.end(); + + const propCtxAfterEnd = getCurrentScope().getPropagationContext(); + expect(propCtxAfterEnd).toStrictEqual({ + spanId: propCtxBeforeEnd?.spanId, + traceId: propCtxBeforeEnd?.traceId, + sampled: true, + dsc: { + environment: 'production', + public_key: 'examplePublicKey', + sample_rate: '1', + sampled: 'true', + transaction: 'mySpan', + trace_id: propCtxBeforeEnd?.traceId, + }, + }); + }); }); describe('using the tag data', () => { From 62f5e22e35fa560bec2eab991c363c3f62354025 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 17 Apr 2024 09:58:27 +0200 Subject: [PATCH 3/4] clean up DSC and sampled value logic fix incorrect `baggage` header in integration test --- .../tracing/trace-lifetime/pageload-meta/template.html | 2 +- .../suites/tracing/trace-lifetime/pageload-meta/test.ts | 2 +- packages/browser/src/tracing/browserTracingIntegration.ts | 8 +++++--- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/template.html b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/template.html index dd44e58fbb4a..0dee204aef16 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/template.html +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/template.html @@ -4,7 +4,7 @@ + content="sentry-trace_id=12345678901234567890123456789012,sentry-sample_rate=0.2,sentry-sampled=true,sentry-transaction=my-transaction,sentry-public_key=public,sentry-release=1.0.0,sentry-environment=prod"/> diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts index d037bac433aa..75e1d4f1c3b6 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts @@ -10,7 +10,7 @@ import { const META_TAG_TRACE_ID = '12345678901234567890123456789012'; const META_TAG_PARENT_SPAN_ID = '1234567890123456'; const META_TAG_BAGGAGE = - 'sentry-trace_id=12345678901234567890123456789012,sentry-sample_rate=0.2,sentry-transaction=my-transaction,sentry-public_key=public,sentry-release=1.0.0,sentry-environment=prod'; + 'sentry-trace_id=12345678901234567890123456789012,sentry-sample_rate=0.2,sentry-sampled=true,sentry-transaction=my-transaction,sentry-public_key=public,sentry-release=1.0.0,sentry-environment=prod'; sentryTest( 'create a new trace for a navigation after the tag pageload trace', diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index 8e61c95106e1..b438c4d97da5 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -18,7 +18,6 @@ import { getIsolationScope, getRootSpan, registerSpanErrorInstrumentation, - spanIsSampled, spanToJSON, startIdleSpan, withScope, @@ -298,10 +297,13 @@ export const browserTracingIntegration = ((_options: Partial Date: Wed, 17 Apr 2024 12:13:25 +0200 Subject: [PATCH 4/4] revert reading sampled from DSC --- packages/browser/src/tracing/browserTracingIntegration.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index b438c4d97da5..8e61c95106e1 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -18,6 +18,7 @@ import { getIsolationScope, getRootSpan, registerSpanErrorInstrumentation, + spanIsSampled, spanToJSON, startIdleSpan, withScope, @@ -297,13 +298,10 @@ export const browserTracingIntegration = ((_options: Partial