From d86cca98f3a0a93a687ad8f7b9dd1b4426c3251a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 8 Oct 2024 09:45:43 +0000 Subject: [PATCH 01/10] fix(core): `.set` the `sentry-trace` header instead of `.append`ing in fetch instrumentation --- packages/core/src/fetch.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/fetch.ts b/packages/core/src/fetch.ts index 9b9d2ece836b..23570f8c2921 100644 --- a/packages/core/src/fetch.ts +++ b/packages/core/src/fetch.ts @@ -153,7 +153,7 @@ export function addTracingHeadersToFetchRequest( } else if (typeof Headers !== 'undefined' && isInstanceOf(headers, Headers)) { const newHeaders = new Headers(headers as Headers); - newHeaders.append('sentry-trace', sentryTraceHeader); + newHeaders.set('sentry-trace', sentryTraceHeader); if (sentryBaggageHeader) { // If the same header is appended multiple times the browser will merge the values into a single request header. From 5206c77b04a66b5dcf1602e00bff5a1d45dd937a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 8 Oct 2024 11:02:58 +0000 Subject: [PATCH 02/10] Remove old ones and append new sentry baggage values --- packages/core/src/fetch.ts | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/packages/core/src/fetch.ts b/packages/core/src/fetch.ts index 23570f8c2921..b7926fc2c037 100644 --- a/packages/core/src/fetch.ts +++ b/packages/core/src/fetch.ts @@ -1,9 +1,12 @@ import type { Client, HandlerDataFetch, Scope, Span, SpanOrigin } from '@sentry/types'; import { BAGGAGE_HEADER_NAME, + SENTRY_BAGGAGE_KEY_PREFIX, + SENTRY_BAGGAGE_KEY_PREFIX_REGEX, dynamicSamplingContextToSentryBaggageHeader, generateSentryTraceHeader, isInstanceOf, + parseBaggageHeader, parseUrl, } from '@sentry/utils'; import { getClient, getCurrentScope, getIsolationScope } from './currentScopes'; @@ -156,9 +159,23 @@ export function addTracingHeadersToFetchRequest( newHeaders.set('sentry-trace', sentryTraceHeader); if (sentryBaggageHeader) { - // If the same header is appended multiple times the browser will merge the values into a single request header. - // Its therefore safe to simply push a "baggage" entry, even though there might already be another baggage header. - newHeaders.append(BAGGAGE_HEADER_NAME, sentryBaggageHeader); + const prevBaggageHeader = newHeaders.get(BAGGAGE_HEADER_NAME); + if (prevBaggageHeader) { + const prevHeaderStrippedFromSentryBaggage = sentryBaggageHeader + .split(',') + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + .filter(baggageEntry => !baggageEntry.split('=')[0]!.startsWith(SENTRY_BAGGAGE_KEY_PREFIX)) + .join(','); + + const mergedHeaders = [sentryBaggageHeader]; + if (prevHeaderStrippedFromSentryBaggage) { + mergedHeaders.unshift(prevHeaderStrippedFromSentryBaggage); + } + + newHeaders.set(BAGGAGE_HEADER_NAME, mergedHeaders.join(',')); + } else { + newHeaders.set(BAGGAGE_HEADER_NAME, sentryBaggageHeader); + } } return newHeaders as PolymorphicRequestHeaders; From 596eda416dea6b6d0a3b620468becb0d8ad94e96 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 8 Oct 2024 11:56:58 +0000 Subject: [PATCH 03/10] lint & size limit --- .size-limit.js | 2 +- packages/core/src/fetch.ts | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/.size-limit.js b/.size-limit.js index 5da293511976..23bb6c58eb98 100644 --- a/.size-limit.js +++ b/.size-limit.js @@ -180,7 +180,7 @@ module.exports = [ name: 'CDN Bundle (incl. Tracing, Replay)', path: createCDNPath('bundle.tracing.replay.min.js'), gzip: true, - limit: '73 KB', + limit: '73.1 KB', }, { name: 'CDN Bundle (incl. Tracing, Replay, Feedback)', diff --git a/packages/core/src/fetch.ts b/packages/core/src/fetch.ts index b7926fc2c037..cdbfffcdad6b 100644 --- a/packages/core/src/fetch.ts +++ b/packages/core/src/fetch.ts @@ -2,11 +2,9 @@ import type { Client, HandlerDataFetch, Scope, Span, SpanOrigin } from '@sentry/ import { BAGGAGE_HEADER_NAME, SENTRY_BAGGAGE_KEY_PREFIX, - SENTRY_BAGGAGE_KEY_PREFIX_REGEX, dynamicSamplingContextToSentryBaggageHeader, generateSentryTraceHeader, isInstanceOf, - parseBaggageHeader, parseUrl, } from '@sentry/utils'; import { getClient, getCurrentScope, getIsolationScope } from './currentScopes'; From 1ad1b5337ff9cf64543ff78f7ff249977b8e3778 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 8 Oct 2024 12:00:45 +0000 Subject: [PATCH 04/10] Simplify and add comment --- packages/core/src/fetch.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/core/src/fetch.ts b/packages/core/src/fetch.ts index cdbfffcdad6b..204243414272 100644 --- a/packages/core/src/fetch.ts +++ b/packages/core/src/fetch.ts @@ -165,12 +165,14 @@ export function addTracingHeadersToFetchRequest( .filter(baggageEntry => !baggageEntry.split('=')[0]!.startsWith(SENTRY_BAGGAGE_KEY_PREFIX)) .join(','); - const mergedHeaders = [sentryBaggageHeader]; - if (prevHeaderStrippedFromSentryBaggage) { - mergedHeaders.unshift(prevHeaderStrippedFromSentryBaggage); - } - - newHeaders.set(BAGGAGE_HEADER_NAME, mergedHeaders.join(',')); + newHeaders.set( + BAGGAGE_HEADER_NAME, + // If there are non-sentry entries (i.e. if the stripped string is non-empty/truthy) combine the stripped header and sentry baggage header + // otherwise just set the sentry baggage header + prevHeaderStrippedFromSentryBaggage + ? `${prevHeaderStrippedFromSentryBaggage},${sentryBaggageHeader}` + : sentryBaggageHeader, + ); } else { newHeaders.set(BAGGAGE_HEADER_NAME, sentryBaggageHeader); } From c9e8f387538e97632ce55d57e01c87b01abe94cc Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 8 Oct 2024 12:22:42 +0000 Subject: [PATCH 05/10] Override values for all the different variants of passing headers --- packages/core/src/fetch.ts | 47 ++++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 10 deletions(-) diff --git a/packages/core/src/fetch.ts b/packages/core/src/fetch.ts index 204243414272..863dffce6efe 100644 --- a/packages/core/src/fetch.ts +++ b/packages/core/src/fetch.ts @@ -159,12 +159,7 @@ export function addTracingHeadersToFetchRequest( if (sentryBaggageHeader) { const prevBaggageHeader = newHeaders.get(BAGGAGE_HEADER_NAME); if (prevBaggageHeader) { - const prevHeaderStrippedFromSentryBaggage = sentryBaggageHeader - .split(',') - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - .filter(baggageEntry => !baggageEntry.split('=')[0]!.startsWith(SENTRY_BAGGAGE_KEY_PREFIX)) - .join(','); - + const prevHeaderStrippedFromSentryBaggage = stripBaggageHeaderOfSentryBaggageValues(prevBaggageHeader); newHeaders.set( BAGGAGE_HEADER_NAME, // If there are non-sentry entries (i.e. if the stripped string is non-empty/truthy) combine the stripped header and sentry baggage header @@ -180,7 +175,25 @@ export function addTracingHeadersToFetchRequest( return newHeaders as PolymorphicRequestHeaders; } else if (Array.isArray(headers)) { - const newHeaders = [...headers, ['sentry-trace', sentryTraceHeader]]; + const newHeaders = headers + .filter(header => { + // Remove any existing sentry-trace headers + return !(Array.isArray(header) && header[0] === 'sentry-trace'); + }) + .map(header => { + if (Array.isArray(header) && header[0] === BAGGAGE_HEADER_NAME) { + return [ + BAGGAGE_HEADER_NAME, + ...header.map(headerValue => + typeof headerValue === 'string' ? stripBaggageHeaderOfSentryBaggageValues(headerValue) : headerValue, + ), + ]; + } else { + return header; + } + }) + // Attach the new sentry-trace header + .concat(['sentry-trace', sentryTraceHeader]); if (sentryBaggageHeader) { // If there are multiple entries with the same key, the browser will merge the values into a single request header. @@ -191,12 +204,16 @@ export function addTracingHeadersToFetchRequest( return newHeaders as PolymorphicRequestHeaders; } else { const existingBaggageHeader = 'baggage' in headers ? headers.baggage : undefined; - const newBaggageHeaders: string[] = []; + let newBaggageHeaders: string[] = []; if (Array.isArray(existingBaggageHeader)) { - newBaggageHeaders.push(...existingBaggageHeader); + newBaggageHeaders = existingBaggageHeader + .map(headerItem => + typeof headerItem === 'string' ? stripBaggageHeaderOfSentryBaggageValues(headerItem) : headerItem, + ) + .filter(headerItem => headerItem === ''); } else if (existingBaggageHeader) { - newBaggageHeaders.push(existingBaggageHeader); + newBaggageHeaders.push(stripBaggageHeaderOfSentryBaggageValues(existingBaggageHeader)); } if (sentryBaggageHeader) { @@ -238,3 +255,13 @@ function endSpan(span: Span, handlerData: HandlerDataFetch): void { } span.end(); } + +function stripBaggageHeaderOfSentryBaggageValues(baggageHeader: string): string { + return ( + baggageHeader + .split(',') + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + .filter(baggageEntry => !baggageEntry.split('=')[0]!.startsWith(SENTRY_BAGGAGE_KEY_PREFIX)) + .join(',') + ); +} From 5e01be35a86ce626f5138123986ca94fac5adbe9 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 8 Oct 2024 12:29:31 +0000 Subject: [PATCH 06/10] smol rename --- packages/core/src/fetch.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/fetch.ts b/packages/core/src/fetch.ts index 863dffce6efe..62193ee1e7fe 100644 --- a/packages/core/src/fetch.ts +++ b/packages/core/src/fetch.ts @@ -123,7 +123,7 @@ export function addTracingHeadersToFetchRequest( request: string | unknown, // unknown is actually type Request but we can't export DOM types from this package, client: Client, scope: Scope, - options: { + fetchOptionsObj: { headers?: | { [key: string]: string[] | string | undefined; @@ -146,7 +146,7 @@ export function addTracingHeadersToFetchRequest( ); const headers = - options.headers || + fetchOptionsObj.headers || (typeof Request !== 'undefined' && isInstanceOf(request, Request) ? (request as Request).headers : undefined); if (!headers) { From e672a379697db2a46ec06bd4bc8c853a82079610 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 15 Oct 2024 14:11:35 +0000 Subject: [PATCH 07/10] Boi am I glad I added tests --- .../trace-header-merging/init.js | 10 +++ .../trace-header-merging/subject.js | 53 +++++++++++++++ .../trace-header-merging/template.html | 11 ++++ .../trace-header-merging/test.ts | 64 +++++++++++++++++++ packages/core/src/fetch.ts | 33 +++++----- 5 files changed, 154 insertions(+), 17 deletions(-) create mode 100644 dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/init.js create mode 100644 dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/subject.js create mode 100644 dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/template.html create mode 100644 dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/test.ts diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/init.js b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/init.js new file mode 100644 index 000000000000..7cd076a052e5 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/init.js @@ -0,0 +1,10 @@ +import * as Sentry from '@sentry/browser'; + +window.Sentry = Sentry; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + integrations: [Sentry.browserTracingIntegration()], + tracePropagationTargets: ['http://example.com'], + tracesSampleRate: 1, +}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/subject.js b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/subject.js new file mode 100644 index 000000000000..5951d6d33411 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/subject.js @@ -0,0 +1,53 @@ +fetchPojo.addEventListener('click', () => { + const fetchOptions = { + headers: { + 'sentry-trace': '12312012123120121231201212312012-1121201211212012-1', + baggage: 'sentry-release=4.2.0', + }, + }; + + // Make two fetch requests that reuse the same fetch object + Sentry.startSpan({ name: 'does-not-matter-1' }, () => + fetch('http://example.com/fetch-pojo', fetchOptions) + .then(res => res.text()) + .then(() => + Sentry.startSpan({ name: 'does-not-matter-2' }, () => fetch('http://example.com/fetch-pojo', fetchOptions)), + ), + ); +}); + +fetchArray.addEventListener('click', () => { + const fetchOptions = { + headers: [ + ['sentry-trace', '12312012123120121231201212312012-1121201211212012-1'], + ['baggage', 'sentry-release=4.2.0'], + ], + }; + + // Make two fetch requests that reuse the same fetch object + Sentry.startSpan({ name: 'does-not-matter-1' }, () => + fetch('http://example.com/fetch-array', fetchOptions) + .then(res => res.text()) + .then(() => + Sentry.startSpan({ name: 'does-not-matter-2' }, () => fetch('http://example.com/fetch-array', fetchOptions)), + ), + ); +}); + +fetchHeaders.addEventListener('click', () => { + const fetchOptions = { + headers: new Headers({ + 'sentry-trace': '12312012123120121231201212312012-1121201211212012-1', + baggage: 'sentry-release=4.2.0', + }), + }; + + // Make two fetch requests that reuse the same fetch object + Sentry.startSpan({ name: 'does-not-matter-1' }, () => + fetch('http://example.com/fetch-headers', fetchOptions) + .then(res => res.text()) + .then(() => + Sentry.startSpan({ name: 'does-not-matter-2' }, () => fetch('http://example.com/fetch-headers', fetchOptions)), + ), + ); +}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/template.html b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/template.html new file mode 100644 index 000000000000..dc60d6d83808 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/template.html @@ -0,0 +1,11 @@ + + + + + + + + + + + diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/test.ts new file mode 100644 index 000000000000..5ae2f83924d0 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/trace-header-merging/test.ts @@ -0,0 +1,64 @@ +import type { Page, Request } from '@playwright/test'; +import { expect } from '@playwright/test'; +import { sentryTest } from '../../../../utils/fixtures'; +import { shouldSkipTracingTest } from '../../../../utils/helpers'; + +async function assertRequests({ + page, + buttonSelector, + requestMatcher, +}: { page: Page; buttonSelector: string; requestMatcher: string }) { + const requests = await new Promise(resolve => { + const requests: Request[] = []; + page + .route(requestMatcher, (route, request) => { + requests.push(request); + if (requests.length === 2) { + resolve(requests); + } + + return route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({}), + }); + }) + .then(() => { + page.click(buttonSelector); + }); + }); + + requests.forEach(request => { + const headers = request.headers(); + + // No merged sentry trace headers + expect(headers['sentry-trace']).not.toContain(','); + + // No multiple baggage entries + expect(headers['baggage'].match(/sentry-trace_id/g) ?? []).toHaveLength(1); + }); +} + +sentryTest( + 'Ensure the SDK does not infinitely append tracing headers to outgoing requests', + async ({ getLocalTestUrl, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const url = await getLocalTestUrl({ testDir: __dirname }); + await page.goto(url); + + await sentryTest.step('fetch with POJO', () => + assertRequests({ page, buttonSelector: '#fetchPojo', requestMatcher: 'http://example.com/fetch-pojo' }), + ); + + await sentryTest.step('fetch with array', () => + assertRequests({ page, buttonSelector: '#fetchArray', requestMatcher: 'http://example.com/fetch-array' }), + ); + + await sentryTest.step('fetch with Headers instance', () => + assertRequests({ page, buttonSelector: '#fetchHeaders', requestMatcher: 'http://example.com/fetch-headers' }), + ); + }, +); diff --git a/packages/core/src/fetch.ts b/packages/core/src/fetch.ts index 62193ee1e7fe..26a993ff0e12 100644 --- a/packages/core/src/fetch.ts +++ b/packages/core/src/fetch.ts @@ -175,25 +175,24 @@ export function addTracingHeadersToFetchRequest( return newHeaders as PolymorphicRequestHeaders; } else if (Array.isArray(headers)) { - const newHeaders = headers - .filter(header => { + const newHeaders = [ + ...headers // Remove any existing sentry-trace headers - return !(Array.isArray(header) && header[0] === 'sentry-trace'); - }) - .map(header => { - if (Array.isArray(header) && header[0] === BAGGAGE_HEADER_NAME) { - return [ - BAGGAGE_HEADER_NAME, - ...header.map(headerValue => - typeof headerValue === 'string' ? stripBaggageHeaderOfSentryBaggageValues(headerValue) : headerValue, - ), - ]; - } else { - return header; - } - }) + .filter(header => { + return !(Array.isArray(header) && header[0] === 'sentry-trace'); + }) + // Get rid of previous sentry baggage values in baggage header + .map(header => { + if (Array.isArray(header) && header[0] === BAGGAGE_HEADER_NAME && typeof header[1] === 'string') { + const [headerName, headerValue, ...rest] = header; + return [headerName, stripBaggageHeaderOfSentryBaggageValues(headerValue), ...rest]; + } else { + return header; + } + }), // Attach the new sentry-trace header - .concat(['sentry-trace', sentryTraceHeader]); + ['sentry-trace', sentryTraceHeader], + ]; if (sentryBaggageHeader) { // If there are multiple entries with the same key, the browser will merge the values into a single request header. From 7d16405672c9b860f1f99f6d72ef5532ff4e0de3 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 15 Oct 2024 14:29:11 +0000 Subject: [PATCH 08/10] size limit --- .size-limit.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.size-limit.js b/.size-limit.js index a244ccb2a2a3..27114ecd0aa7 100644 --- a/.size-limit.js +++ b/.size-limit.js @@ -138,7 +138,7 @@ module.exports = [ import: createImport('init', 'ErrorBoundary', 'reactRouterV6BrowserTracingIntegration'), ignore: ['react/jsx-runtime'], gzip: true, - limit: '39 KB', + limit: '39.01 KB', }, // Vue SDK (ESM) { From 66d1153cab7c6ffa3bd5444b76669155d12d5565 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 16 Oct 2024 07:29:31 +0000 Subject: [PATCH 09/10] size limit --- .size-limit.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.size-limit.js b/.size-limit.js index 27114ecd0aa7..f128739c9fd0 100644 --- a/.size-limit.js +++ b/.size-limit.js @@ -138,7 +138,7 @@ module.exports = [ import: createImport('init', 'ErrorBoundary', 'reactRouterV6BrowserTracingIntegration'), ignore: ['react/jsx-runtime'], gzip: true, - limit: '39.01 KB', + limit: '39.05 KB', }, // Vue SDK (ESM) { From 28ace04eb81f86d0c3cc13fd76a746bf3794b6bf Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 16 Oct 2024 08:06:21 +0000 Subject: [PATCH 10/10] size limit --- .size-limit.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.size-limit.js b/.size-limit.js index f128739c9fd0..bdfe8a4397e2 100644 --- a/.size-limit.js +++ b/.size-limit.js @@ -79,7 +79,7 @@ module.exports = [ path: 'packages/browser/build/npm/esm/index.js', import: createImport('init', 'browserTracingIntegration', 'replayIntegration', 'replayCanvasIntegration'), gzip: true, - limit: '78 KB', + limit: '78.1 KB', }, { name: '@sentry/browser (incl. Tracing, Replay, Feedback)',