Skip to content

Commit 8d5a084

Browse files
author
Luca Forstner
authored
fix(core): .set the sentry-trace header instead of .appending in fetch instrumentation (getsentry#13907)
1 parent 7fbe744 commit 8d5a084

File tree

6 files changed

+193
-12
lines changed

6 files changed

+193
-12
lines changed

‎.size-limit.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ module.exports = [
7979
path: 'packages/browser/build/npm/esm/index.js',
8080
import: createImport('init', 'browserTracingIntegration', 'replayIntegration', 'replayCanvasIntegration'),
8181
gzip: true,
82-
limit: '78 KB',
82+
limit: '78.1 KB',
8383
},
8484
{
8585
name: '@sentry/browser (incl. Tracing, Replay, Feedback)',
@@ -138,7 +138,7 @@ module.exports = [
138138
import: createImport('init', 'ErrorBoundary', 'reactRouterV6BrowserTracingIntegration'),
139139
ignore: ['react/jsx-runtime'],
140140
gzip: true,
141-
limit: '39 KB',
141+
limit: '39.05 KB',
142142
},
143143
// Vue SDK (ESM)
144144
{

‎dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/init.js‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
import * as Sentry from '@sentry/browser';
2+
3+
window.Sentry = Sentry;
4+
5+
Sentry.init({
6+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
7+
integrations: [Sentry.browserTracingIntegration()],
8+
tracePropagationTargets: ['http://example.com'],
9+
tracesSampleRate: 1,
10+
});

‎dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/subject.js‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
fetchPojo.addEventListener('click', () => {
2+
const fetchOptions = {
3+
headers: {
4+
'sentry-trace': '12312012123120121231201212312012-1121201211212012-1',
5+
baggage: 'sentry-release=4.2.0',
6+
},
7+
};
8+
9+
// Make two fetch requests that reuse the same fetch object
10+
Sentry.startSpan({ name: 'does-not-matter-1' }, () =>
11+
fetch('http://example.com/fetch-pojo', fetchOptions)
12+
.then(res => res.text())
13+
.then(() =>
14+
Sentry.startSpan({ name: 'does-not-matter-2' }, () => fetch('http://example.com/fetch-pojo', fetchOptions)),
15+
),
16+
);
17+
});
18+
19+
fetchArray.addEventListener('click', () => {
20+
const fetchOptions = {
21+
headers: [
22+
['sentry-trace', '12312012123120121231201212312012-1121201211212012-1'],
23+
['baggage', 'sentry-release=4.2.0'],
24+
],
25+
};
26+
27+
// Make two fetch requests that reuse the same fetch object
28+
Sentry.startSpan({ name: 'does-not-matter-1' }, () =>
29+
fetch('http://example.com/fetch-array', fetchOptions)
30+
.then(res => res.text())
31+
.then(() =>
32+
Sentry.startSpan({ name: 'does-not-matter-2' }, () => fetch('http://example.com/fetch-array', fetchOptions)),
33+
),
34+
);
35+
});
36+
37+
fetchHeaders.addEventListener('click', () => {
38+
const fetchOptions = {
39+
headers: new Headers({
40+
'sentry-trace': '12312012123120121231201212312012-1121201211212012-1',
41+
baggage: 'sentry-release=4.2.0',
42+
}),
43+
};
44+
45+
// Make two fetch requests that reuse the same fetch object
46+
Sentry.startSpan({ name: 'does-not-matter-1' }, () =>
47+
fetch('http://example.com/fetch-headers', fetchOptions)
48+
.then(res => res.text())
49+
.then(() =>
50+
Sentry.startSpan({ name: 'does-not-matter-2' }, () => fetch('http://example.com/fetch-headers', fetchOptions)),
51+
),
52+
);
53+
});

‎dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/template.html‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<meta charset="utf-8" />
5+
</head>
6+
<body>
7+
<button id="fetchPojo">Fetch POJO</button>
8+
<button id="fetchArray">Fetch array</button>
9+
<button id="fetchHeaders">Fetch Headers</button>
10+
</body>
11+
</html>

‎dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/test.ts‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
import type { Page, Request } from '@playwright/test';
2+
import { expect } from '@playwright/test';
3+
import { sentryTest } from '../../../../utils/fixtures';
4+
import { shouldSkipTracingTest } from '../../../../utils/helpers';
5+
6+
async function assertRequests({
7+
page,
8+
buttonSelector,
9+
requestMatcher,
10+
}: { page: Page; buttonSelector: string; requestMatcher: string }) {
11+
const requests = await new Promise<Request[]>(resolve => {
12+
const requests: Request[] = [];
13+
page
14+
.route(requestMatcher, (route, request) => {
15+
requests.push(request);
16+
if (requests.length === 2) {
17+
resolve(requests);
18+
}
19+
20+
return route.fulfill({
21+
status: 200,
22+
contentType: 'application/json',
23+
body: JSON.stringify({}),
24+
});
25+
})
26+
.then(() => {
27+
page.click(buttonSelector);
28+
});
29+
});
30+
31+
requests.forEach(request => {
32+
const headers = request.headers();
33+
34+
// No merged sentry trace headers
35+
expect(headers['sentry-trace']).not.toContain(',');
36+
37+
// No multiple baggage entries
38+
expect(headers['baggage'].match(/sentry-trace_id/g) ?? []).toHaveLength(1);
39+
});
40+
}
41+
42+
sentryTest(
43+
'Ensure the SDK does not infinitely append tracing headers to outgoing requests',
44+
async ({ getLocalTestUrl, page }) => {
45+
if (shouldSkipTracingTest()) {
46+
sentryTest.skip();
47+
}
48+
49+
const url = await getLocalTestUrl({ testDir: __dirname });
50+
await page.goto(url);
51+
52+
await sentryTest.step('fetch with POJO', () =>
53+
assertRequests({ page, buttonSelector: '#fetchPojo', requestMatcher: 'http://example.com/fetch-pojo' }),
54+
);
55+
56+
await sentryTest.step('fetch with array', () =>
57+
assertRequests({ page, buttonSelector: '#fetchArray', requestMatcher: 'http://example.com/fetch-array' }),
58+
);
59+
60+
await sentryTest.step('fetch with Headers instance', () =>
61+
assertRequests({ page, buttonSelector: '#fetchHeaders', requestMatcher: 'http://example.com/fetch-headers' }),
62+
);
63+
},
64+
);

‎packages/core/src/fetch.ts‎

Lines changed: 53 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import type { Client, HandlerDataFetch, Scope, Span, SpanOrigin } from '@sentry/types';
22
import {
33
BAGGAGE_HEADER_NAME,
4+
SENTRY_BAGGAGE_KEY_PREFIX,
45
dynamicSamplingContextToSentryBaggageHeader,
56
generateSentryTraceHeader,
67
isInstanceOf,
@@ -122,7 +123,7 @@ export function addTracingHeadersToFetchRequest(
122123
request: string | unknown, // unknown is actually type Request but we can't export DOM types from this package,
123124
client: Client,
124125
scope: Scope,
125-
options: {
126+
fetchOptionsObj: {
126127
headers?:
127128
| {
128129
[key: string]: string[] | string | undefined;
@@ -145,25 +146,53 @@ export function addTracingHeadersToFetchRequest(
145146
);
146147

147148
const headers =
148-
options.headers ||
149+
fetchOptionsObj.headers ||
149150
(typeof Request !== 'undefined' && isInstanceOf(request, Request) ? (request as Request).headers : undefined);
150151

151152
if (!headers) {
152153
return { 'sentry-trace': sentryTraceHeader, baggage: sentryBaggageHeader };
153154
} else if (typeof Headers !== 'undefined' && isInstanceOf(headers, Headers)) {
154155
const newHeaders = new Headers(headers as Headers);
155156

156-
newHeaders.append('sentry-trace', sentryTraceHeader);
157+
newHeaders.set('sentry-trace', sentryTraceHeader);
157158

158159
if (sentryBaggageHeader) {
159-
// If the same header is appended multiple times the browser will merge the values into a single request header.
160-
// Its therefore safe to simply push a "baggage" entry, even though there might already be another baggage header.
161-
newHeaders.append(BAGGAGE_HEADER_NAME, sentryBaggageHeader);
160+
const prevBaggageHeader = newHeaders.get(BAGGAGE_HEADER_NAME);
161+
if (prevBaggageHeader) {
162+
const prevHeaderStrippedFromSentryBaggage = stripBaggageHeaderOfSentryBaggageValues(prevBaggageHeader);
163+
newHeaders.set(
164+
BAGGAGE_HEADER_NAME,
165+
// If there are non-sentry entries (i.e. if the stripped string is non-empty/truthy) combine the stripped header and sentry baggage header
166+
// otherwise just set the sentry baggage header
167+
prevHeaderStrippedFromSentryBaggage
168+
? `${prevHeaderStrippedFromSentryBaggage},${sentryBaggageHeader}`
169+
: sentryBaggageHeader,
170+
);
171+
} else {
172+
newHeaders.set(BAGGAGE_HEADER_NAME, sentryBaggageHeader);
173+
}
162174
}
163175

164176
return newHeaders as PolymorphicRequestHeaders;
165177
} else if (Array.isArray(headers)) {
166-
const newHeaders = [...headers, ['sentry-trace', sentryTraceHeader]];
178+
const newHeaders = [
179+
...headers
180+
// Remove any existing sentry-trace headers
181+
.filter(header => {
182+
return !(Array.isArray(header) && header[0] === 'sentry-trace');
183+
})
184+
// Get rid of previous sentry baggage values in baggage header
185+
.map(header => {
186+
if (Array.isArray(header) && header[0] === BAGGAGE_HEADER_NAME && typeof header[1] === 'string') {
187+
const [headerName, headerValue, ...rest] = header;
188+
return [headerName, stripBaggageHeaderOfSentryBaggageValues(headerValue), ...rest];
189+
} else {
190+
return header;
191+
}
192+
}),
193+
// Attach the new sentry-trace header
194+
['sentry-trace', sentryTraceHeader],
195+
];
167196

168197
if (sentryBaggageHeader) {
169198
// If there are multiple entries with the same key, the browser will merge the values into a single request header.
@@ -174,12 +203,16 @@ export function addTracingHeadersToFetchRequest(
174203
return newHeaders as PolymorphicRequestHeaders;
175204
} else {
176205
const existingBaggageHeader = 'baggage' in headers ? headers.baggage : undefined;
177-
const newBaggageHeaders: string[] = [];
206+
let newBaggageHeaders: string[] = [];
178207

179208
if (Array.isArray(existingBaggageHeader)) {
180-
newBaggageHeaders.push(...existingBaggageHeader);
209+
newBaggageHeaders = existingBaggageHeader
210+
.map(headerItem =>
211+
typeof headerItem === 'string' ? stripBaggageHeaderOfSentryBaggageValues(headerItem) : headerItem,
212+
)
213+
.filter(headerItem => headerItem === '');
181214
} else if (existingBaggageHeader) {
182-
newBaggageHeaders.push(existingBaggageHeader);
215+
newBaggageHeaders.push(stripBaggageHeaderOfSentryBaggageValues(existingBaggageHeader));
183216
}
184217

185218
if (sentryBaggageHeader) {
@@ -221,3 +254,13 @@ function endSpan(span: Span, handlerData: HandlerDataFetch): void {
221254
}
222255
span.end();
223256
}
257+
258+
function stripBaggageHeaderOfSentryBaggageValues(baggageHeader: string): string {
259+
return (
260+
baggageHeader
261+
.split(',')
262+
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
263+
.filter(baggageEntry => !baggageEntry.split('=')[0]!.startsWith(SENTRY_BAGGAGE_KEY_PREFIX))
264+
.join(',')
265+
);
266+
}

0 commit comments

Comments
 (0)