Skip to content

Commit 2e60318

Browse files
committed
fix(browser): Ensure parentSpanId stays consistent in TwP mode
1 parent af4a72d commit 2e60318

File tree

8 files changed

+207
-7
lines changed

8 files changed

+207
-7
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: 16 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,9 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
541552

542553
const scope = getCurrentScope();
543554
scope.setPropagationContext(propagationContext);
555+
if (!hasSpansEnabled()) {
556+
scope.getPropagationContext().propagationSpanId = generateSpanId();
557+
}
544558

545559
// We store the normalized request data on the scope, so we get the request data at time of span creation
546560
// 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: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -723,23 +723,28 @@ 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({
729730
traceId: expect.stringMatching(/[a-f0-9]{32}/),
731+
propagationSpanId: expect.stringMatching(/[a-f0-9]{16}/),
730732
sampleRand: expect.any(Number),
731733
});
732734
expect(newCurrentScopePropCtx).toEqual({
733735
traceId: expect.stringMatching(/[a-f0-9]{32}/),
736+
propagationSpanId: expect.stringMatching(/[a-f0-9]{16}/),
734737
sampleRand: expect.any(Number),
735738
});
736739
expect(newIsolationScopePropCtx).toEqual({
737740
traceId: expect.stringMatching(/[a-f0-9]{32}/),
741+
propagationSpanId: expect.stringMatching(/[a-f0-9]{16}/),
738742
sampleRand: expect.any(Number),
739743
});
740744

741745
expect(newIsolationScopePropCtx.traceId).not.toEqual(oldIsolationScopePropCtx.traceId);
742746
expect(newCurrentScopePropCtx.traceId).not.toEqual(oldCurrentScopePropCtx.traceId);
747+
expect(newIsolationScopePropCtx.propagationSpanId).not.toEqual(oldIsolationScopePropCtx.propagationSpanId);
743748
});
744749

745750
it("saves the span's positive sampling decision and its DSC on the propagationContext when the span finishes", () => {
@@ -761,6 +766,7 @@ describe('browserTracingIntegration', () => {
761766
expect(propCtxBeforeEnd).toStrictEqual({
762767
sampleRand: expect.any(Number),
763768
traceId: expect.stringMatching(/[a-f0-9]{32}/),
769+
propagationSpanId: expect.stringMatching(/[a-f0-9]{16}/),
764770
});
765771

766772
navigationSpan!.end();
@@ -770,6 +776,7 @@ describe('browserTracingIntegration', () => {
770776
traceId: propCtxBeforeEnd.traceId,
771777
sampled: true,
772778
sampleRand: expect.any(Number),
779+
propagationSpanId: expect.stringMatching(/[a-f0-9]{16}/),
773780
dsc: {
774781
release: undefined,
775782
org_id: undefined,
@@ -803,6 +810,7 @@ describe('browserTracingIntegration', () => {
803810
expect(propCtxBeforeEnd).toStrictEqual({
804811
traceId: expect.stringMatching(/[a-f0-9]{32}/),
805812
sampleRand: expect.any(Number),
813+
propagationSpanId: expect.stringMatching(/[a-f0-9]{16}/),
806814
});
807815

808816
navigationSpan!.end();
@@ -812,6 +820,7 @@ describe('browserTracingIntegration', () => {
812820
traceId: propCtxBeforeEnd.traceId,
813821
sampled: false,
814822
sampleRand: expect.any(Number),
823+
propagationSpanId: expect.stringMatching(/[a-f0-9]{16}/),
815824
dsc: {
816825
release: undefined,
817826
org_id: undefined,

0 commit comments

Comments
 (0)