Skip to content

Commit ebe6a67

Browse files
authored
fix(browser): Ensure propagated parentSpanId stays consistent during trace in TwP mode (#17526)
This PR adjusts how we set treat the `parentSpanId` when propagating it in an SDK configured for [tracing without performance](https://develop.sentry.dev/sdk/telemetry/traces/tracing-without-performance). Previously, we didn't set the `propagationSpanId` on the `propagationContext` (which is supposed to be serialized to the parent span id in the `sentry-trace` (and `traceparent`) header, if no active span is present. This had the consequence that every time `getTraceData()` was called, we'd return a new, random (non-existing) parent span id. Meaning, that if we generate both, `sentry-trace` and `traceparent` headers via `getTraceData()` they'd end up with different parent span ids in the respective hedaers. With this change, we now set a `propagationSpanId` on the PC, which has two effects: 1. consistent parent span id in `sentry-trace` and `traceparent` headers for the same outgoing request 2. consistent parent span ids in all outgoing requests until the next navigation when the PC is recycled So this prolongs the time span in which we propagate the same parent span id. Since this span doesn't exist, I _think_ this is fine. Though happy to drop this if reviewers have concerns. The main benefit with this change is 1 in combination with us generating `traceparent` from raw data rather than parsing the already generated `sentry-trace` header and converting its parts into `traceparent`. (TwP was a mistake)
1 parent 3faa74a commit ebe6a67

File tree

13 files changed

+259
-66
lines changed

13 files changed

+259
-66
lines changed

dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/linked-traces/consistent-sampling/meta-precedence/test.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,13 @@ sentryTest.describe('When `consistentTraceSampling` is `true` and page contains
3030
const clientReportPromise = waitForClientReportRequest(page);
3131

3232
await sentryTest.step('Initial pageload', async () => {
33+
// negative sampling decision -> no pageload txn
3334
await page.goto(url);
3435
});
3536

3637
await sentryTest.step('Make fetch request', async () => {
38+
// The fetch requests starts a new trace on purpose. So we only want the
39+
// sampling decision and rand to be the same as from the meta tag but not the trace id or DSC
3740
const tracingHeadersPromise = waitForTracingHeadersOnUrl(page, 'http://sentry-test-external.io');
3841

3942
await page.locator('#btn2').click();
Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
1-
fetch('http://sentry-test-site.example/0').then();
1+
fetch('http://sentry-test-site.example/0');
2+
fetch('http://sentry-test-site.example/1');

dev-packages/browser-integration-tests/suites/tracing/request/fetch-tracing-without-performance-propagateTraceparent/test.ts

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,21 +12,36 @@ sentryTest(
1212

1313
const url = await getLocalTestUrl({ testDir: __dirname });
1414

15-
const [, request] = await Promise.all([page.goto(url), page.waitForRequest('http://sentry-test-site.example/0')]);
15+
const [, request0, request1] = await Promise.all([
16+
page.goto(url),
17+
page.waitForRequest('http://sentry-test-site.example/0'),
18+
page.waitForRequest('http://sentry-test-site.example/1'),
19+
]);
1620

17-
const requestHeaders = request.headers();
21+
const requestHeaders0 = request0.headers();
1822

19-
const traceparentData = extractTraceparentData(requestHeaders['sentry-trace']);
23+
const traceparentData = extractTraceparentData(requestHeaders0['sentry-trace']);
2024
expect(traceparentData).toMatchObject({
2125
traceId: expect.stringMatching(/^([a-f0-9]{32})$/),
2226
parentSpanId: expect.stringMatching(/^([a-f0-9]{16})$/),
2327
parentSampled: undefined,
2428
});
2529

26-
expect(requestHeaders).toMatchObject({
30+
expect(requestHeaders0).toMatchObject({
2731
'sentry-trace': `${traceparentData?.traceId}-${traceparentData?.parentSpanId}`,
2832
baggage: expect.stringContaining(`sentry-trace_id=${traceparentData?.traceId}`),
2933
traceparent: `00-${traceparentData?.traceId}-${traceparentData?.parentSpanId}-00`,
3034
});
35+
36+
const requestHeaders1 = request1.headers();
37+
expect(requestHeaders1).toMatchObject({
38+
'sentry-trace': `${traceparentData?.traceId}-${traceparentData?.parentSpanId}`,
39+
baggage: expect.stringContaining(`sentry-trace_id=${traceparentData?.traceId}`),
40+
traceparent: `00-${traceparentData?.traceId}-${traceparentData?.parentSpanId}-00`,
41+
});
42+
43+
expect(requestHeaders1['sentry-trace']).toBe(requestHeaders0['sentry-trace']);
44+
expect(requestHeaders1['baggage']).toBe(requestHeaders0['baggage']);
45+
expect(requestHeaders1['traceparent']).toBe(requestHeaders0['traceparent']);
3146
},
3247
);
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
import * as Sentry from '@sentry/browser';
2+
3+
window.Sentry = Sentry;
4+
5+
Sentry.init({
6+
// in browser TwP means not setting tracesSampleRate but adding browserTracingIntegration,
7+
dsn: 'https://[email protected]/1337',
8+
integrations: [Sentry.browserTracingIntegration()],
9+
tracePropagationTargets: ['http://sentry-test-site.example'],
10+
propagateTraceparent: true,
11+
});
Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
<!doctype html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8" />
5+
<!-- Purposefully emitting the `sampled` flag in the sentry-trace tag -->
6+
<meta name="sentry-trace" content="12345678901234567890123456789012-1234567890123456" />
7+
<meta
8+
name="baggage"
9+
content="sentry-trace_id=12345678901234567890123456789012,sentry-public_key=public,sentry-release=1.0.0,sentry-environment=prod,sentry-sample_rand=0.42"
10+
/>
11+
</head>
12+
<body>
13+
<button id="errorBtn">Throw Error</button>
14+
<button id="fetchBtn">Fetch Request</button>
15+
<button id="xhrBtn">XHR Request</button>
16+
</body>
17+
</html>
Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
import { expect } from '@playwright/test';
2+
import { extractTraceparentData } from '@sentry/core';
3+
import { sentryTest } from '../../../../utils/fixtures';
4+
import { shouldSkipTracingTest } from '../../../../utils/helpers';
5+
6+
const META_TAG_TRACE_ID = '12345678901234567890123456789012';
7+
const META_TAG_PARENT_SPAN_ID = '1234567890123456';
8+
const META_TAG_BAGGAGE =
9+
'sentry-trace_id=12345678901234567890123456789012,sentry-public_key=public,sentry-release=1.0.0,sentry-environment=prod,sentry-sample_rand=0.42';
10+
11+
sentryTest(
12+
'outgoing fetch requests have new traceId after navigation (with propagateTraceparent)',
13+
async ({ getLocalTestUrl, page }) => {
14+
if (shouldSkipTracingTest()) {
15+
sentryTest.skip();
16+
}
17+
18+
const url = await getLocalTestUrl({ testDir: __dirname });
19+
20+
await page.route('http://sentry-test-site.example/**', route => {
21+
return route.fulfill({
22+
status: 200,
23+
contentType: 'application/json',
24+
body: JSON.stringify({}),
25+
});
26+
});
27+
28+
await page.goto(url);
29+
30+
const requestPromise = page.waitForRequest('http://sentry-test-site.example/*');
31+
await page.locator('#fetchBtn').click();
32+
const request = await requestPromise;
33+
const headers = request.headers();
34+
35+
const sentryTraceParentData = extractTraceparentData(headers['sentry-trace']);
36+
// sampling decision is deferred because TwP means we didn't sample any span
37+
expect(sentryTraceParentData).toEqual({
38+
traceId: META_TAG_TRACE_ID,
39+
parentSpanId: expect.stringMatching(/^[0-9a-f]{16}$/),
40+
parentSampled: undefined,
41+
});
42+
expect(headers['baggage']).toBe(META_TAG_BAGGAGE);
43+
// but traceparent propagates a negative sampling decision because it has no concept of deferred sampling
44+
expect(headers['traceparent']).toBe(
45+
`00-${sentryTraceParentData?.traceId}-${sentryTraceParentData?.parentSpanId}-00`,
46+
);
47+
48+
const requestPromise2 = page.waitForRequest('http://sentry-test-site.example/*');
49+
await page.locator('#fetchBtn').click();
50+
const request2 = await requestPromise2;
51+
const headers2 = request2.headers();
52+
53+
const sentryTraceParentData2 = extractTraceparentData(headers2['sentry-trace']);
54+
expect(sentryTraceParentData2).toEqual(sentryTraceParentData);
55+
56+
await page.goto(`${url}#navigation`);
57+
58+
const requestPromise3 = page.waitForRequest('http://sentry-test-site.example/*');
59+
await page.locator('#fetchBtn').click();
60+
const request3 = await requestPromise3;
61+
const headers3 = request3.headers();
62+
63+
const sentryTraceParentData3 = extractTraceparentData(headers3['sentry-trace']);
64+
// sampling decision is deferred because TwP means we didn't sample any span
65+
expect(sentryTraceParentData3).toEqual({
66+
traceId: expect.not.stringContaining(sentryTraceParentData!.traceId!),
67+
parentSpanId: expect.not.stringContaining(sentryTraceParentData!.parentSpanId!),
68+
parentSampled: undefined,
69+
});
70+
71+
expect(headers3['baggage']).toMatch(
72+
/sentry-environment=production,sentry-public_key=public,sentry-trace_id=[0-9a-f]{32}/,
73+
);
74+
expect(headers3['baggage']).not.toContain(`sentry-trace_id=${META_TAG_TRACE_ID}`);
75+
// but traceparent propagates a negative sampling decision because it has no concept of deferred sampling
76+
expect(headers3['traceparent']).toBe(
77+
`00-${sentryTraceParentData3!.traceId}-${sentryTraceParentData3!.parentSpanId}-00`,
78+
);
79+
80+
const requestPromise4 = page.waitForRequest('http://sentry-test-site.example/*');
81+
await page.locator('#fetchBtn').click();
82+
const request4 = await requestPromise4;
83+
const headers4 = request4.headers();
84+
85+
const sentryTraceParentData4 = extractTraceparentData(headers4['sentry-trace']);
86+
expect(sentryTraceParentData4).toEqual(sentryTraceParentData3);
87+
},
88+
);
89+
90+
sentryTest('outgoing XHR requests have new traceId after navigation', async ({ getLocalTestUrl, page }) => {
91+
if (shouldSkipTracingTest()) {
92+
sentryTest.skip();
93+
}
94+
95+
const url = await getLocalTestUrl({ testDir: __dirname });
96+
97+
await page.route('http://sentry-test-site.example/**', route => {
98+
return route.fulfill({
99+
status: 200,
100+
contentType: 'application/json',
101+
body: JSON.stringify({}),
102+
});
103+
});
104+
105+
await page.goto(url);
106+
107+
const requestPromise = page.waitForRequest('http://sentry-test-site.example/*');
108+
await page.locator('#xhrBtn').click();
109+
const request = await requestPromise;
110+
const headers = request.headers();
111+
112+
// sampling decision is deferred because TwP means we didn't sample any span
113+
expect(headers['sentry-trace']).toMatch(new RegExp(`^${META_TAG_TRACE_ID}-[0-9a-f]{16}$`));
114+
expect(headers['baggage']).toBe(META_TAG_BAGGAGE);
115+
116+
await page.goto(`${url}#navigation`);
117+
118+
const requestPromise2 = page.waitForRequest('http://sentry-test-site.example/*');
119+
await page.locator('#xhrBtn').click();
120+
const request2 = await requestPromise2;
121+
const headers2 = request2.headers();
122+
123+
// sampling decision is deferred because TwP means we didn't sample any span
124+
expect(headers2['sentry-trace']).toMatch(/^[0-9a-f]{32}-[0-9a-f]{16}$/);
125+
expect(headers2['baggage']).not.toBe(`${META_TAG_TRACE_ID}-${META_TAG_PARENT_SPAN_ID}`);
126+
expect(headers2['baggage']).toMatch(
127+
/sentry-environment=production,sentry-public_key=public,sentry-trace_id=[0-9a-f]{32}/,
128+
);
129+
expect(headers2['baggage']).not.toContain(`sentry-trace_id=${META_TAG_TRACE_ID}`);
130+
});

packages/browser/src/tracing/browserTracingIntegration.ts

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,15 @@ import {
55
browserPerformanceTimeOrigin,
66
dateTimestampInSeconds,
77
debug,
8+
generateSpanId,
89
generateTraceId,
910
getClient,
1011
getCurrentScope,
1112
getDynamicSamplingContextFromSpan,
1213
getIsolationScope,
1314
getLocationHref,
1415
GLOBAL_OBJ,
16+
hasSpansEnabled,
1517
parseStringToURLObject,
1618
propagationContextFromHeaders,
1719
registerSpanErrorInstrumentation,
@@ -512,10 +514,19 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
512514

513515
maybeEndActiveSpan();
514516

515-
getIsolationScope().setPropagationContext({ traceId: generateTraceId(), sampleRand: Math.random() });
517+
getIsolationScope().setPropagationContext({
518+
traceId: generateTraceId(),
519+
sampleRand: Math.random(),
520+
propagationSpanId: hasSpansEnabled() ? undefined : generateSpanId(),
521+
});
516522

517523
const scope = getCurrentScope();
518-
scope.setPropagationContext({ traceId: generateTraceId(), sampleRand: Math.random() });
524+
scope.setPropagationContext({
525+
traceId: generateTraceId(),
526+
sampleRand: Math.random(),
527+
propagationSpanId: hasSpansEnabled() ? undefined : generateSpanId(),
528+
});
529+
519530
// We reset this to ensure we do not have lingering incorrect data here
520531
// places that call this hook may set this where appropriate - else, the URL at span sending time is used
521532
scope.setSDKProcessingMetadata({
@@ -541,6 +552,12 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
541552

542553
const scope = getCurrentScope();
543554
scope.setPropagationContext(propagationContext);
555+
if (!hasSpansEnabled()) {
556+
// for browser, we wanna keep the spanIds consistent during the entire lifetime of the trace
557+
// this works by setting the propagationSpanId to a random spanId so that we have a consistent
558+
// span id to propagate in TwP mode (!hasSpansEnabled())
559+
scope.getPropagationContext().propagationSpanId = generateSpanId();
560+
}
544561

545562
// We store the normalized request data on the scope, so we get the request data at time of span creation
546563
// otherwise, the URL etc. may already be of the following navigation, and we'd report the wrong URL

packages/browser/test/tracing/browserTracingIntegration.test.ts

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -723,6 +723,7 @@ describe('browserTracingIntegration', () => {
723723

724724
expect(oldCurrentScopePropCtx).toEqual({
725725
traceId: expect.stringMatching(/[a-f0-9]{32}/),
726+
propagationSpanId: expect.stringMatching(/[a-f0-9]{16}/),
726727
sampleRand: expect.any(Number),
727728
});
728729
expect(oldIsolationScopePropCtx).toEqual({
@@ -731,15 +732,18 @@ describe('browserTracingIntegration', () => {
731732
});
732733
expect(newCurrentScopePropCtx).toEqual({
733734
traceId: expect.stringMatching(/[a-f0-9]{32}/),
735+
propagationSpanId: expect.stringMatching(/[a-f0-9]{16}/),
734736
sampleRand: expect.any(Number),
735737
});
736738
expect(newIsolationScopePropCtx).toEqual({
737739
traceId: expect.stringMatching(/[a-f0-9]{32}/),
740+
propagationSpanId: expect.stringMatching(/[a-f0-9]{16}/),
738741
sampleRand: expect.any(Number),
739742
});
740743

741744
expect(newIsolationScopePropCtx.traceId).not.toEqual(oldIsolationScopePropCtx.traceId);
742745
expect(newCurrentScopePropCtx.traceId).not.toEqual(oldCurrentScopePropCtx.traceId);
746+
expect(newIsolationScopePropCtx.propagationSpanId).not.toEqual(oldIsolationScopePropCtx.propagationSpanId);
743747
});
744748

745749
it("saves the span's positive sampling decision and its DSC on the propagationContext when the span finishes", () => {
@@ -758,15 +762,15 @@ describe('browserTracingIntegration', () => {
758762
});
759763

760764
const propCtxBeforeEnd = getCurrentScope().getPropagationContext();
761-
expect(propCtxBeforeEnd).toStrictEqual({
765+
expect(propCtxBeforeEnd).toEqual({
762766
sampleRand: expect.any(Number),
763767
traceId: expect.stringMatching(/[a-f0-9]{32}/),
764768
});
765769

766770
navigationSpan!.end();
767771

768772
const propCtxAfterEnd = getCurrentScope().getPropagationContext();
769-
expect(propCtxAfterEnd).toStrictEqual({
773+
expect(propCtxAfterEnd).toEqual({
770774
traceId: propCtxBeforeEnd.traceId,
771775
sampled: true,
772776
sampleRand: expect.any(Number),
@@ -800,15 +804,15 @@ describe('browserTracingIntegration', () => {
800804
});
801805

802806
const propCtxBeforeEnd = getCurrentScope().getPropagationContext();
803-
expect(propCtxBeforeEnd).toStrictEqual({
807+
expect(propCtxBeforeEnd).toEqual({
804808
traceId: expect.stringMatching(/[a-f0-9]{32}/),
805809
sampleRand: expect.any(Number),
806810
});
807811

808812
navigationSpan!.end();
809813

810814
const propCtxAfterEnd = getCurrentScope().getPropagationContext();
811-
expect(propCtxAfterEnd).toStrictEqual({
815+
expect(propCtxAfterEnd).toEqual({
812816
traceId: propCtxBeforeEnd.traceId,
813817
sampled: false,
814818
sampleRand: expect.any(Number),

packages/core/src/utils/spanUtils.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import type { SpanStatus } from '../types-hoist/spanStatus';
1717
import { addNonEnumerableProperty } from '../utils/object';
1818
import { generateSpanId } from '../utils/propagationContext';
1919
import { timestampInSeconds } from '../utils/time';
20-
import { generateSentryTraceHeader } from '../utils/tracing';
20+
import { generateSentryTraceHeader, generateTraceparentHeader } from '../utils/tracing';
2121
import { consoleSandbox } from './debug-logger';
2222
import { _getSpanForScope } from './spanOnScope';
2323

@@ -77,6 +77,15 @@ export function spanToTraceHeader(span: Span): string {
7777
return generateSentryTraceHeader(traceId, spanId, sampled);
7878
}
7979

80+
/**
81+
* Convert a Span to a W3C traceparent header.
82+
*/
83+
export function spanToTraceparentHeader(span: Span): string {
84+
const { traceId, spanId } = span.spanContext();
85+
const sampled = spanIsSampled(span);
86+
return generateTraceparentHeader(traceId, spanId, sampled);
87+
}
88+
8089
/**
8190
* Converts the span links array to a flattened version to be sent within an envelope.
8291
*

0 commit comments

Comments
 (0)