diff --git a/dev-packages/browser-integration-tests/suites/tracing/request/fetch-trace-header-merging/test.ts b/dev-packages/browser-integration-tests/suites/tracing/request/fetch-trace-header-merging/test.ts index 7c0f6db3483b..dd57b0f6ad84 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/request/fetch-trace-header-merging/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/request/fetch-trace-header-merging/test.ts @@ -39,9 +39,10 @@ async function assertRequests({ // No merged sentry trace headers expect(headers['sentry-trace']).not.toContain(','); + expect(headers['sentry-trace']).toBe('12312012123120121231201212312012-1121201211212012-1'); // No multiple baggage entries - expect(headers['baggage'].match(/sentry-release/g) ?? []).toHaveLength(1); + expect(headers['baggage']).toBe('sentry-release=4.2.0'); }); } diff --git a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts index dbdd69ffb45b..eebafa06bfd1 100644 --- a/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts +++ b/dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts @@ -1,6 +1,7 @@ import { afterAll, expect, test } from 'vitest'; import { cleanupChildProcesses, createRunner } from '../../../../utils/runner'; import type { TestAPIResponse } from '../server'; +import { extractTraceparentData } from '@sentry/core'; afterAll(() => { cleanupChildProcesses(); @@ -33,7 +34,6 @@ test('should ignore sentry-values in `baggage` header of a third party vendor an 'sentry-environment=myEnv', 'sentry-release=2.1.0', expect.stringMatching(/sentry-sample_rand=\d+/), - 'sentry-sample_rate=0.54', 'third=party', ]); }); @@ -46,6 +46,11 @@ test('should ignore sentry-values in `baggage` header of a third party vendor an expect(response).toBeDefined(); const baggage = response?.test_data.baggage?.split(',').sort(); + const sentryTraceHeader = response?.test_data['sentry-trace']; + + const sentryTrace = extractTraceparentData(sentryTraceHeader); + + expect(sentryTrace?.traceId).toMatch(/^[0-9a-f]{32}$/); expect(response).toMatchObject({ test_data: { @@ -63,7 +68,7 @@ test('should ignore sentry-values in `baggage` header of a third party vendor an expect.stringMatching(/sentry-sample_rand=\d+/), 'sentry-sample_rate=1', 'sentry-sampled=true', - expect.stringMatching(/sentry-trace_id=[\da-f]{32}/), + `sentry-trace_id=${sentryTrace?.traceId}`, 'sentry-transaction=GET%20%2Ftest%2Fexpress', 'third=party', ]); diff --git a/dev-packages/node-integration-tests/suites/tracing/double-baggage/expects.ts b/dev-packages/node-integration-tests/suites/tracing/double-baggage/expects.ts new file mode 100644 index 000000000000..e092f29c1e65 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/double-baggage/expects.ts @@ -0,0 +1,37 @@ +import { expect } from 'vitest'; +import { extractTraceparentData, parseBaggageHeader, TRACEPARENT_REGEXP } from '@sentry/core'; + +export function expectNoDuplicateSentryBaggageKeys(baggage: string | string[] | undefined): void { + expect(baggage).toBeDefined(); + const baggageStr = Array.isArray(baggage) ? baggage.join(',') : (baggage as string); + const sentryEntries = baggageStr.split(',').filter(entry => entry.trim().startsWith('sentry-')); + const sentryKeyNames = sentryEntries.map(entry => entry.trim().split('=')[0]); + const uniqueKeyNames = [...new Set(sentryKeyNames)]; + expect(sentryKeyNames).toEqual(uniqueKeyNames); +} + +export function expectConsistentTraceId(headers: Record): void { + const sentryTrace = headers['sentry-trace']; + expect(sentryTrace).toMatch(TRACEPARENT_REGEXP); + + const sentryTraceData = extractTraceparentData(sentryTrace as string)!; + expect(sentryTraceData.traceId).toMatch(/^[a-f\d]{32}$/); + + const baggage = parseBaggageHeader(headers['baggage']); + + const baggageTraceId = baggage!['sentry-trace_id']; + expect(baggageTraceId).toBeDefined(); + expect(baggageTraceId).toMatch(/^[a-f\d]{32}$/); + + expect(sentryTraceData.traceId).toEqual(baggageTraceId); +} + +export function expectUserSetTraceId(headers: Record): void { + const xSentryTrace = extractTraceparentData(headers['x-tracedata-sentry-trace'] as string); + const sentryTrace = extractTraceparentData(headers['sentry-trace'] as string); + expect(xSentryTrace?.traceId).toBe(sentryTrace?.traceId); + + const xBaggage = parseBaggageHeader(headers['x-tracedata-baggage']); + const baggage = parseBaggageHeader(headers['baggage']); + expect(xBaggage).toEqual(baggage); +} diff --git a/dev-packages/node-integration-tests/suites/tracing/double-baggage/no-spans/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/double-baggage/no-spans/instrument.mjs new file mode 100644 index 000000000000..7acd36926f22 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/double-baggage/no-spans/instrument.mjs @@ -0,0 +1,9 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + // explicitly not setting tracesSampleRate, + transport: loggingTransport, +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/double-baggage/no-spans/scenario.mjs b/dev-packages/node-integration-tests/suites/tracing/double-baggage/no-spans/scenario.mjs new file mode 100644 index 000000000000..046e980c5fe2 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/double-baggage/no-spans/scenario.mjs @@ -0,0 +1,45 @@ +import * as Sentry from '@sentry/node'; +import http from 'http'; + +async function run() { + const traceData = Sentry.getTraceData(); + // fetch with manual getTraceData() headers - the core reproduction case from #19158 + await fetch(`${process.env.SERVER_URL}/api/fetch-custom-headers`, { + headers: { + ...traceData, + 'x-tracedata-sentry-trace': traceData['sentry-trace'], + 'x-tracedata-baggage': traceData.baggage, + }, + }).then(res => res.text()); + + // fetch without manual headers (baseline - auto-instrumentation only) + await fetch(`${process.env.SERVER_URL}/api/fetch`).then(res => res.text()); + + // http.request with manual getTraceData() headers + await new Promise((resolve, reject) => { + const url = new URL(`${process.env.SERVER_URL}/api/http-custom-headers`); + const req = http.request( + { + hostname: url.hostname, + port: url.port, + path: url.pathname, + method: 'GET', + headers: { + ...traceData, + 'x-tracedata-sentry-trace': traceData['sentry-trace'], + 'x-tracedata-baggage': traceData.baggage, + }, + }, + res => { + res.on('data', () => {}); + res.on('end', () => resolve()); + }, + ); + req.on('error', reject); + req.end(); + }); + + Sentry.captureException(new Error('done')); +} + +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/double-baggage/no-spans/test.ts b/dev-packages/node-integration-tests/suites/tracing/double-baggage/no-spans/test.ts new file mode 100644 index 000000000000..c517bca0d25b --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/double-baggage/no-spans/test.ts @@ -0,0 +1,47 @@ +import { createTestServer } from '@sentry-internal/test-utils'; +import { describe, expect } from 'vitest'; +import { createEsmAndCjsTests } from '../../../../utils/runner'; +import { expectConsistentTraceId, expectNoDuplicateSentryBaggageKeys, expectUserSetTraceId } from '../expects'; + +describe('double baggage prevention', () => { + createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { + test('fetch with manual getTraceData() does not duplicate sentry baggage entries', async () => { + const [SERVER_URL, closeTestServer] = await createTestServer() + .get('/api/fetch-custom-headers', headers => { + // fetch with manual getTraceData() headers + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^[a-f\d]{32}-[a-f\d]{16}$/)); + expectNoDuplicateSentryBaggageKeys(headers['baggage']); + expectConsistentTraceId(headers); + expectUserSetTraceId(headers); + }) + .get('/api/fetch', headers => { + // fetch without manual headers (baseline) + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^[a-f\d]{32}-[a-f\d]{16}$/)); + expectNoDuplicateSentryBaggageKeys(headers['baggage']); + expectConsistentTraceId(headers); + }) + .get('/api/http-custom-headers', headers => { + // http.request with manual getTraceData() headers + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^[a-f\d]{32}-[a-f\d]{16}$/)); + expectNoDuplicateSentryBaggageKeys(headers['baggage']); + expectConsistentTraceId(headers); + expectUserSetTraceId(headers); + }) + .start(); + + await createRunner() + .withEnv({ SERVER_URL }) + .ignore('transaction') + .expect({ + event: { + exception: { + values: [{ type: 'Error', value: 'done' }], + }, + }, + }) + .start() + .completed(); + closeTestServer(); + }); + }); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-no-parent/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-no-parent/instrument.mjs new file mode 100644 index 000000000000..46a27dd03b74 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-no-parent/instrument.mjs @@ -0,0 +1,9 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + transport: loggingTransport, +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-no-parent/scenario.mjs b/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-no-parent/scenario.mjs new file mode 100644 index 000000000000..dd5841685463 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-no-parent/scenario.mjs @@ -0,0 +1,45 @@ +import * as Sentry from '@sentry/node'; +import http from 'http'; + +async function run() { + const traceData = Sentry.getTraceData(); + // fetch with manual getTraceData() headers - the core reproduction case from #19158 + await fetch(`${process.env.SERVER_URL}/api/fetch-custom-headers`, { + headers: { + ...traceData, + 'x-tracedata-sentry-trace': traceData['sentry-trace'], + 'x-tracedata-baggage': traceData.baggage, + }, + }).then(res => res.text()); + + // fetch without manual headers (baseline - auto-instrumentation only) + await fetch(`${process.env.SERVER_URL}/api/fetch`, {}).then(res => res.text()); + + // http.request with manual getTraceData() headers + await new Promise((resolve, reject) => { + const url = new URL(`${process.env.SERVER_URL}/api/http-custom-headers`); + const req = http.request( + { + hostname: url.hostname, + port: url.port, + path: url.pathname, + method: 'GET', + headers: { + ...traceData, + 'x-tracedata-sentry-trace': traceData['sentry-trace'], + 'x-tracedata-baggage': traceData.baggage, + }, + }, + res => { + res.on('data', () => {}); + res.on('end', () => resolve()); + }, + ); + req.on('error', reject); + req.end(); + }); + + Sentry.captureException(new Error('done')); +} + +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-no-parent/test.ts b/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-no-parent/test.ts new file mode 100644 index 000000000000..6b5d20cdb5f7 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-no-parent/test.ts @@ -0,0 +1,46 @@ +import { createTestServer } from '@sentry-internal/test-utils'; +import { describe, expect } from 'vitest'; +import { createEsmAndCjsTests } from '../../../../utils/runner'; +import { expectConsistentTraceId, expectNoDuplicateSentryBaggageKeys, expectUserSetTraceId } from '../expects'; + +describe('double baggage prevention', () => { + createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { + test('fetch with manual getTraceData() does not duplicate sentry baggage entries', async () => { + const [SERVER_URL, closeTestServer] = await createTestServer() + .get('/api/fetch-custom-headers', headers => { + // fetch with manual getTraceData() headers + expect(headers['sentry-trace']).not.toContain(','); + expectNoDuplicateSentryBaggageKeys(headers['baggage']); + expectConsistentTraceId(headers); + expectUserSetTraceId(headers); + }) + .get('/api/fetch', headers => { + // fetch without manual headers (baseline) + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^[a-f\d]{32}-[a-f\d]{16}(-[01])?$/)); + expectNoDuplicateSentryBaggageKeys(headers['baggage']); + expectConsistentTraceId(headers); + }) + .get('/api/http-custom-headers', headers => { + // http.request with manual getTraceData() headers + expect(headers['sentry-trace']).not.toContain(','); + expectNoDuplicateSentryBaggageKeys(headers['baggage']); + expectConsistentTraceId(headers); + expectUserSetTraceId(headers); + }) + .start(); + + await createRunner() + .withEnv({ SERVER_URL }) + .expect({ + event: { + exception: { + values: [{ type: 'Error', value: 'done' }], + }, + }, + }) + .start() + .completed(); + closeTestServer(); + }); + }); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-parent/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-parent/instrument.mjs new file mode 100644 index 000000000000..46a27dd03b74 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-parent/instrument.mjs @@ -0,0 +1,9 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + transport: loggingTransport, +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-parent/scenario.mjs b/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-parent/scenario.mjs new file mode 100644 index 000000000000..a577bad62333 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-parent/scenario.mjs @@ -0,0 +1,50 @@ +import * as Sentry from '@sentry/node'; +import http from 'http'; + +async function run() { + const traceData = Sentry.getTraceData(); + // fetch with manual getTraceData() headers - the core reproduction case from #19158 + await fetch(`${process.env.SERVER_URL}/api/fetch-custom-headers`, { + headers: { + ...traceData, + 'x-tracedata-sentry-trace': traceData['sentry-trace'], + 'x-tracedata-baggage': traceData.baggage, + }, + }).then(res => res.text()); + + // fetch without manual headers (baseline - auto-instrumentation only) + await fetch(`${process.env.SERVER_URL}/api/fetch`, { + headers: { + 'x-tracedata-sentry-trace': traceData['sentry-trace'], + 'x-tracedata-baggage': traceData.baggage, + }, + }).then(res => res.text()); + + // http.request with manual getTraceData() headers + await new Promise((resolve, reject) => { + const url = new URL(`${process.env.SERVER_URL}/api/http-custom-headers`); + const req = http.request( + { + hostname: url.hostname, + port: url.port, + path: url.pathname, + method: 'GET', + headers: { + ...traceData, + 'x-tracedata-sentry-trace': traceData['sentry-trace'], + 'x-tracedata-baggage': traceData.baggage, + }, + }, + res => { + res.on('data', () => {}); + res.on('end', () => resolve()); + }, + ); + req.on('error', reject); + req.end(); + }); + + Sentry.captureException(new Error('done')); +} + +Sentry.startSpan({ name: 'parent_span' }, () => run()); diff --git a/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-parent/test.ts b/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-parent/test.ts new file mode 100644 index 000000000000..22de5cb285b3 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/double-baggage/spans-parent/test.ts @@ -0,0 +1,92 @@ +import { createTestServer } from '@sentry-internal/test-utils'; +import { describe, expect } from 'vitest'; +import { createEsmAndCjsTests } from '../../../../utils/runner'; +import { extractTraceparentData } from '@sentry/core'; +import { expectConsistentTraceId, expectNoDuplicateSentryBaggageKeys, expectUserSetTraceId } from '../expects'; + +describe('double baggage prevention - http.client spans with parent span', () => { + createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { + let transactionTraceId = '000'; + let fetchSpanId = '000'; + let httpCustomHeadersSpanId = '000'; + let fetchCustomHeadersSpanId = '000'; + + test('fetch with manual getTraceData() does not duplicate sentry baggage entries', async () => { + const [SERVER_URL, closeTestServer] = await createTestServer() + .get('/api/fetch-custom-headers', headers => { + // fetch with manual getTraceData() headers — core reproduction case + const sentryTrace = extractTraceparentData(headers['sentry-trace'] as string); + transactionTraceId = sentryTrace!.traceId!; + fetchCustomHeadersSpanId = sentryTrace!.parentSpanId!; + expectNoDuplicateSentryBaggageKeys(headers['baggage']); + expect(headers['sentry-trace']).not.toContain(','); + expectConsistentTraceId(headers); + expectUserSetTraceId(headers); + }) + .get('/api/fetch', headers => { + // fetch without manual headers (baseline) + expectNoDuplicateSentryBaggageKeys(headers['baggage']); + expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^[a-f\d]{32}-[a-f\d]{16}(-[01])?$/)); + expectConsistentTraceId(headers); + const sentryTrace = extractTraceparentData(headers['sentry-trace'] as string); + fetchSpanId = sentryTrace!.parentSpanId!; + }) + .get('/api/http-custom-headers', headers => { + // http.request with manual getTraceData() headers + expectNoDuplicateSentryBaggageKeys(headers['baggage']); + expect(headers['sentry-trace']).not.toContain(','); + expectConsistentTraceId(headers); + expectUserSetTraceId(headers); + const sentryTrace = extractTraceparentData(headers['sentry-trace'] as string); + httpCustomHeadersSpanId = sentryTrace!.parentSpanId!; + }) + .start(); + + await createRunner() + .withEnv({ SERVER_URL }) + .ignore('event') + .expect({ + transaction: txn => { + expect(transactionTraceId).toMatch(/^[a-f0-9]{32}$/); + + expect(txn).toMatchObject({ + transaction: 'parent_span', + spans: [ + { + op: 'http.client', + description: expect.stringMatching(/^GET .*\/api\/fetch-custom-headers$/), + data: {}, + // span id is expected to be different since users call getTraceData() before the + // http.client span is created + span_id: expect.not.stringContaining(fetchCustomHeadersSpanId), + start_timestamp: expect.any(Number), + trace_id: transactionTraceId, + }, + { + op: 'http.client', + description: expect.stringMatching(/^GET .*\/api\/fetch$/), + data: {}, + span_id: fetchSpanId, + start_timestamp: expect.any(Number), + trace_id: transactionTraceId, + }, + { + op: 'http.client', + description: expect.stringMatching(/^GET .*\/api\/http-custom-headers$/), + data: {}, + // span id is expected to be different since users call getTraceData() before the + // http.client span is created + span_id: expect.not.stringContaining(httpCustomHeadersSpanId), + start_timestamp: expect.any(Number), + trace_id: transactionTraceId, + }, + ], + }); + }, + }) + .start() + .completed(); + closeTestServer(); + }); + }); +}); diff --git a/packages/node-core/src/utils/baggage.ts b/packages/node-core/src/utils/baggage.ts index d236851559db..496c834d5c23 100644 --- a/packages/node-core/src/utils/baggage.ts +++ b/packages/node-core/src/utils/baggage.ts @@ -1,4 +1,4 @@ -import { objectToBaggageHeader, parseBaggageHeader } from '@sentry/core'; +import { objectToBaggageHeader, parseBaggageHeader, SENTRY_BAGGAGE_KEY_PREFIX } from '@sentry/core'; /** * Merge two baggage headers into one. @@ -24,15 +24,39 @@ export function mergeBaggageHeaders { - // Sentry-specific keys always take precedence from new baggage - // Non-Sentry keys only added if not already present - if (key.startsWith('sentry-') || !mergedBaggageEntries[key]) { + // Single pass over new entries to partition sentry vs non-sentry + const newSentryEntries: Record = {}; + const newNonSentryEntries: Record = {}; + for (const [key, value] of Object.entries(newBaggageEntries)) { + if (key.startsWith(SENTRY_BAGGAGE_KEY_PREFIX)) { + newSentryEntries[key] = value; + } else { + newNonSentryEntries[key] = value; + } + } + + const hasNewSentryEntries = Object.keys(newSentryEntries).length > 0; + + // If new baggage contains at least one sentry- value, we remove all old sentry- values + // otherwise, we keep old sentry- values. If we don't remove old sentry- values, we end + // up with an inconsistent dynamic sampling context propagation. + const mergedBaggageEntries: Record = {}; + if (existingBaggageEntries) { + for (const [key, value] of Object.entries(existingBaggageEntries)) { + if (hasNewSentryEntries && key.startsWith(SENTRY_BAGGAGE_KEY_PREFIX)) { + continue; + } mergedBaggageEntries[key] = value; } - }); + } + + // Sentry entries from new baggage always overwrite; non-sentry only if not already present + Object.assign(mergedBaggageEntries, newSentryEntries); + for (const [key, value] of Object.entries(newNonSentryEntries)) { + if (!mergedBaggageEntries[key]) { + mergedBaggageEntries[key] = value; + } + } return objectToBaggageHeader(mergedBaggageEntries); } diff --git a/packages/node-core/src/utils/outgoingFetchRequest.ts b/packages/node-core/src/utils/outgoingFetchRequest.ts index ce04a26db1e0..cad20496e478 100644 --- a/packages/node-core/src/utils/outgoingFetchRequest.ts +++ b/packages/node-core/src/utils/outgoingFetchRequest.ts @@ -10,13 +10,10 @@ import { } from '@sentry/core'; import type { UndiciRequest, UndiciResponse } from '../integrations/node-fetch/types'; import { mergeBaggageHeaders } from './baggage'; - +import { debug } from '@sentry/core'; const SENTRY_TRACE_HEADER = 'sentry-trace'; const SENTRY_BAGGAGE_HEADER = 'baggage'; - -// For baggage, we make sure to merge this into a possibly existing header -const BAGGAGE_HEADER_REGEX = /baggage: (.*)\r\n/; - +const W3C_TRACEPARENT_HEADER = 'traceparent'; /** * Add trace propagation headers to an outgoing fetch/undici request. * @@ -45,55 +42,137 @@ export function addTracePropagationHeadersToFetchRequest( const { 'sentry-trace': sentryTrace, baggage, traceparent } = addedHeaders; + const requestHeaders = Array.isArray(request.headers) ? request.headers : stringToArrayHeaders(request.headers); + + // OTel's UndiciInstrumentation calls propagation.inject() which unconditionally + // appends headers to the request. When the user also sets headers via getTraceData(), + // this results in duplicate sentry-trace and baggage (and optionally traceparent) entries. + // We clean these up before applying our own logic. + _deduplicateArrayHeader(requestHeaders, SENTRY_TRACE_HEADER); + _deduplicateArrayHeader(requestHeaders, SENTRY_BAGGAGE_HEADER); + if (propagateTraceparent) { + _deduplicateArrayHeader(requestHeaders, W3C_TRACEPARENT_HEADER); + } + // We do not want to overwrite existing headers here // If the core UndiciInstrumentation is registered, it will already have set the headers // We do not want to add any then - if (Array.isArray(request.headers)) { - const requestHeaders = request.headers; + const hasExistingSentryTraceHeader = _findExistingHeaderIndex(requestHeaders, SENTRY_TRACE_HEADER) !== -1; - // We do not want to overwrite existing header here, if it was already set - if (sentryTrace && !requestHeaders.includes(SENTRY_TRACE_HEADER)) { + // We do not want to set any headers if we already have an existing sentry-trace header. + // sentry-trace is still the source of truth, otherwise we risk mixing up baggage and sentry-trace values. + if (!hasExistingSentryTraceHeader) { + if (sentryTrace) { requestHeaders.push(SENTRY_TRACE_HEADER, sentryTrace); } - if (traceparent && !requestHeaders.includes('traceparent')) { + if (traceparent && _findExistingHeaderIndex(requestHeaders, 'traceparent') === -1) { requestHeaders.push('traceparent', traceparent); } // For baggage, we make sure to merge this into a possibly existing header - const existingBaggagePos = requestHeaders.findIndex(header => header === SENTRY_BAGGAGE_HEADER); - if (baggage && existingBaggagePos === -1) { + const existingBaggageIndex = _findExistingHeaderIndex(requestHeaders, SENTRY_BAGGAGE_HEADER); + if (baggage && existingBaggageIndex === -1) { requestHeaders.push(SENTRY_BAGGAGE_HEADER, baggage); } else if (baggage) { - const existingBaggage = requestHeaders[existingBaggagePos + 1]; - const merged = mergeBaggageHeaders(existingBaggage, baggage); + // headers in format [key_0, value_0, key_1, value_1, ...], hence the +1 here + const existingBaggageValue = requestHeaders[existingBaggageIndex + 1]; + const merged = mergeBaggageHeaders(existingBaggageValue, baggage); if (merged) { - requestHeaders[existingBaggagePos + 1] = merged; + requestHeaders[existingBaggageIndex + 1] = merged; + } + } + } + + if (!Array.isArray(request.headers)) { + // For original string request headers, we need to write them back to the request + request.headers = arrayToStringHeaders(requestHeaders); + } +} + +function stringToArrayHeaders(requestHeaders: string): string[] { + const headersArray = requestHeaders.split('\r\n'); + const headers: string[] = []; + for (const header of headersArray) { + try { + const colonIndex = header.indexOf(':'); + if (colonIndex === -1) { + continue; + } + const key = header.slice(0, colonIndex).trim(); + const value = header.slice(colonIndex + 1).trim(); + if (key) { + headers.push(key, value); } + } catch { + debug.warn(`Failed to convert string request header to array header: ${header}`); + } + } + return headers; +} + +function arrayToStringHeaders(headers: string[]): string { + const headerPairs: string[] = []; + + for (let i = 0; i < headers.length; i += 2) { + const key = headers[i]; + const value = headers[i + 1]; + if (!key || value == null) { + // skip falsy keys but only null/undefined values + continue; } - } else { - const requestHeaders = request.headers; - // We do not want to overwrite existing header here, if it was already set - if (sentryTrace && !requestHeaders.includes(`${SENTRY_TRACE_HEADER}:`)) { - request.headers += `${SENTRY_TRACE_HEADER}: ${sentryTrace}\r\n`; + headerPairs.push(`${key}: ${value}`); + } + + if (!headerPairs.length) { + return ''; + } + + return headerPairs.join('\r\n').concat('\r\n'); +} + +/** + * For a given header name, if there are multiple entries in the [key, value, key, value, ...] array, + * keep the first entry and remove the rest. + * For baggage, values are merged to preserve all entries but to dedupe sentry- values, and always + * keep the first occurrence of them + */ +function _deduplicateArrayHeader(headers: string[], headerName: string): void { + let firstIndex = -1; + for (let i = 0; i < headers.length; i += 2) { + if (headers[i] !== headerName) { + continue; } - if (traceparent && !requestHeaders.includes('traceparent:')) { - request.headers += `traceparent: ${traceparent}\r\n`; + if (firstIndex === -1) { + firstIndex = i; + continue; } - const existingBaggage = request.headers.match(BAGGAGE_HEADER_REGEX)?.[1]; - if (baggage && !existingBaggage) { - request.headers += `${SENTRY_BAGGAGE_HEADER}: ${baggage}\r\n`; - } else if (baggage) { - const merged = mergeBaggageHeaders(existingBaggage, baggage); + const firstHeaderValue = headers[firstIndex + 1]; + if (headerName === SENTRY_BAGGAGE_HEADER && firstHeaderValue) { + // mergeBaggageHeaders always takes sentry- values from the new baggage (2nd param) and merges + // it with the existing one (1st param). Here, we want to keep the first header's existing + // sentry- values in favor of the new ones. Hence we swap the parameters. + const merged = mergeBaggageHeaders(headers[i + 1], firstHeaderValue); if (merged) { - request.headers = request.headers.replace(BAGGAGE_HEADER_REGEX, `baggage: ${merged}\r\n`); + headers[firstIndex + 1] = merged; } } + headers.splice(i, 2); + i -= 2; } } +/** + * Find the index of an existing header in an array of headers. + * Only take even indices, because headers are in format [key_0, value_0, key_1, value_1, ...] + * otherwise we could match a header _value_ with @param name + */ +function _findExistingHeaderIndex(headers: string[], name: string): number { + return headers.findIndex((header, i) => i % 2 === 0 && header === name); +} + /** Add a breadcrumb for an outgoing fetch/undici request. */ export function addFetchRequestBreadcrumb(request: UndiciRequest, response: UndiciResponse): void { const data = getBreadcrumbData(request); diff --git a/packages/node-core/src/utils/outgoingHttpRequest.ts b/packages/node-core/src/utils/outgoingHttpRequest.ts index 7eafa941286a..34624900b472 100644 --- a/packages/node-core/src/utils/outgoingHttpRequest.ts +++ b/packages/node-core/src/utils/outgoingHttpRequest.ts @@ -63,7 +63,13 @@ export function addTracePropagationHeadersToOutgoingRequest( const { 'sentry-trace': sentryTrace, baggage, traceparent } = headersToAdd; - if (sentryTrace && !request.getHeader('sentry-trace')) { + const hasExistingSentryTraceHeader = !!request.getHeader('sentry-trace'); + + if (hasExistingSentryTraceHeader) { + return; + } + + if (sentryTrace) { try { request.setHeader('sentry-trace', sentryTrace); DEBUG_BUILD && debug.log(LOG_PREFIX, 'Added sentry-trace header to outgoing request'); @@ -92,7 +98,8 @@ export function addTracePropagationHeadersToOutgoingRequest( } if (baggage) { - const newBaggage = mergeBaggageHeaders(request.getHeader('baggage'), baggage); + const existingBaggage = request.getHeader('baggage'); + const newBaggage = mergeBaggageHeaders(existingBaggage, baggage); if (newBaggage) { try { request.setHeader('baggage', newBaggage); diff --git a/packages/node-core/test/utils/baggage.test.ts b/packages/node-core/test/utils/baggage.test.ts index aae5c48d6068..0d7ff5f757d5 100644 --- a/packages/node-core/test/utils/baggage.test.ts +++ b/packages/node-core/test/utils/baggage.test.ts @@ -69,7 +69,6 @@ describe('mergeBaggageHeaders', () => { expect(entries).toContain('third=party'); expect(entries).toContain('sentry-environment=myEnv'); expect(entries).toContain('sentry-release=2.1.0'); - expect(entries).toContain('sentry-sample_rate=0.54'); expect(entries).not.toContain('sentry-environment=staging'); expect(entries).not.toContain('sentry-release=9.9.9'); }); @@ -87,7 +86,7 @@ describe('mergeBaggageHeaders', () => { it('handles array-type existing baggage', () => { const result = mergeBaggageHeaders(['foo=bar', 'other=vendor'], 'sentry-release=1.0.0'); - const entries = result?.split(','); + const entries = (result as string)?.split(','); expect(entries).toContain('foo=bar'); expect(entries).toContain('other=vendor'); expect(entries).toContain('sentry-release=1.0.0'); @@ -115,7 +114,7 @@ describe('mergeBaggageHeaders', () => { expect(entries).not.toContain('sentry-environment=old'); }); - it('matches OTEL propagation.inject() behavior for Sentry keys', () => { + it('overwrites existing Sentry entries with new SDK values', () => { const result = mergeBaggageHeaders( 'sentry-trace_id=abc123,sentry-sampled=false,non-sentry=keep', 'sentry-trace_id=xyz789,sentry-sampled=true', @@ -128,4 +127,41 @@ describe('mergeBaggageHeaders', () => { expect(entries).not.toContain('sentry-trace_id=abc123'); expect(entries).not.toContain('sentry-sampled=false'); }); + + it('merges non-conflicting baggage entries', () => { + const existing = 'custom-key=value'; + const newBaggage = 'sentry-environment=production'; + const result = mergeBaggageHeaders(existing, newBaggage); + expect(result).toBe('custom-key=value,sentry-environment=production'); + }); + + it('overwrites existing Sentry entries when keys conflict', () => { + const existing = 'sentry-environment=staging'; + const newBaggage = 'sentry-environment=production'; + const result = mergeBaggageHeaders(existing, newBaggage); + expect(result).toBe('sentry-environment=production'); + }); + + it('handles multiple entries with Sentry conflicts', () => { + const existing = 'custom-key=value1,sentry-environment=staging'; + const newBaggage = 'sentry-environment=production,sentry-trace_id=123'; + const result = mergeBaggageHeaders(existing, newBaggage); + expect(result).toContain('custom-key=value1'); + expect(result).toContain('sentry-environment=production'); + expect(result).toContain('sentry-trace_id=123'); + expect(result).not.toContain('sentry-environment=staging'); + }); + + it('removes all sentry- values from old baggage and only adds new ones (if at least one new sentry- value is present)', () => { + const existing = 'sentry-trace_id=old,sentry-sampled=false,non-sentry=keep'; + const newBaggage = 'sentry-trace_id=new,sentry-environment=new'; + const result = mergeBaggageHeaders(existing, newBaggage); + expect(result).toBe('non-sentry=keep,sentry-trace_id=new,sentry-environment=new'); + }); + + it('preserves existing sentry entries when new baggage has no sentry entries', () => { + const result = mergeBaggageHeaders('sentry-release=1.0.0,foo=bar', 'baz=qux'); + + expect(result).toBe('sentry-release=1.0.0,foo=bar,baz=qux'); + }); }); diff --git a/packages/node-core/test/utils/outgoingFetchRequest.test.ts b/packages/node-core/test/utils/outgoingFetchRequest.test.ts new file mode 100644 index 000000000000..f023c2cab268 --- /dev/null +++ b/packages/node-core/test/utils/outgoingFetchRequest.test.ts @@ -0,0 +1,478 @@ +import type { MockedFunction } from 'vitest'; +import { describe, beforeEach, vi, expect, it } from 'vitest'; +import type { UndiciRequest } from '../../src/integrations/node-fetch/types'; +import { addTracePropagationHeadersToFetchRequest } from '../../src/utils/outgoingFetchRequest'; +import { LRUMap } from '@sentry/core'; +import * as SentryCore from '@sentry/core'; + +const mockedGetTraceData: MockedFunction<() => ReturnType> = vi.hoisted(() => + vi.fn(() => ({ + 'sentry-trace': 'trace_id_1-span_id_1-1', + baggage: 'sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging', + })), +); + +const mockedClientGetOptions: MockedFunction<() => Partial> = vi.hoisted(() => + vi.fn(() => ({ + tracePropagationTargets: ['https://example.com'], + propagateTraceparent: true, + })), +); + +vi.mock('@sentry/core', async () => { + const actual = await vi.importActual('@sentry/core'); + return { + ...actual, + getClient: vi.fn(() => ({ + getOptions: mockedClientGetOptions, + })), + shouldPropagateTraceForUrl: () => true, + getTraceData: mockedGetTraceData, + }; +}); + +describe('addTracePropagationHeadersToFetchRequest', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("doesn't add headers if shouldPropagateTraceForUrl returns false", () => { + vi.spyOn(SentryCore, 'shouldPropagateTraceForUrl').mockReturnValueOnce(false); + + const request = { + headers: [] as string[], + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toEqual([]); + }); + + describe('when headers are an array', () => { + it('adds sentry-trace and baggage headers to request', () => { + const request = { + headers: [] as string[], + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toEqual([ + 'sentry-trace', + 'trace_id_1-span_id_1-1', + 'baggage', + 'sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging', + ]); + }); + + it('adds sentry-trace, baggage and traceparent headers to request', () => { + mockedGetTraceData.mockReturnValueOnce({ + 'sentry-trace': 'trace_id_1-span_id_1-1', + baggage: 'sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging', + traceparent: '00-trace_id_1-span_id_1-01', + }); + + const request = { + headers: [] as string[], + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toEqual([ + 'sentry-trace', + 'trace_id_1-span_id_1-1', + 'traceparent', + '00-trace_id_1-span_id_1-01', + 'baggage', + 'sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging', + ]); + }); + + it('preserves non-sentry entries in existing baggage header', () => { + const request = { + headers: ['baggage', 'other=entry,not=sentry'], + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toEqual([ + 'baggage', + 'other=entry,not=sentry,sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging', + 'sentry-trace', + 'trace_id_1-span_id_1-1', + ]); + }); + + it('preserves pre-existing traceparent header', () => { + mockedGetTraceData.mockReturnValueOnce({ + 'sentry-trace': 'trace_id_1-span_id_1-1', + baggage: 'sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging', + traceparent: '00-trace_id_1-span_id_1-01', + }); + + const request = { + headers: ['traceparent', '00-some-other-trace_id-span_id_x-01'], + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toEqual([ + 'traceparent', + '00-some-other-trace_id-span_id_x-01', + 'sentry-trace', + 'trace_id_1-span_id_1-1', + 'baggage', + 'sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging', + ]); + }); + + describe('when sentry-trace is already set', () => { + it("preserves original sentry-trace header doesn't add baggage", () => { + const request = { + headers: ['sentry-trace', 'trace_id_2-span_id_2-1'], + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toEqual(['sentry-trace', 'trace_id_2-span_id_2-1']); + }); + + it('preserves original baggage header', () => { + const request = { + headers: [ + 'sentry-trace', + 'trace_id_2-span_id_2-1', + 'baggage', + 'sentry-trace_id=trace_id_2,sentry-sampled=true,sentry-environment=staging', + ], + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toEqual([ + 'sentry-trace', + 'trace_id_2-span_id_2-1', + 'baggage', + 'sentry-trace_id=trace_id_2,sentry-sampled=true,sentry-environment=staging', + ]); + }); + + it("doesn't add traceparent header even if propagateTraceparent is true", () => { + mockedGetTraceData.mockReturnValueOnce({ + 'sentry-trace': 'trace_id_2-span_id_2-1', + baggage: 'sentry-trace_id=trace_id_2,sentry-sampled=true,sentry-environment=staging', + traceparent: '00-trace_id_2-span_id_2-01', + }); + + const request = { + headers: ['sentry-trace', 'trace_id_2-span_id_2-1'], + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toEqual(['sentry-trace', 'trace_id_2-span_id_2-1']); + }); + }); + + describe('pre-existing header deduplication', () => { + it('deduplicates sentry-trace and baggage headers', () => { + const request = { + headers: [ + 'sentry-trace', + 'user-trace_id-xyz-1', + 'baggage', + 'sentry-trace_id=user-trace_id-xyz-1,sentry-sampled=true,sentry-environment=user', + 'sentry-trace', + 'undici-trace_id-abc-1', + 'baggage', + 'sentry-trace_id=undici-trace_id-abc-1,sentry-sampled=true,sentry-environment=undici', + ], + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toEqual([ + 'sentry-trace', + 'user-trace_id-xyz-1', + 'baggage', + 'sentry-trace_id=user-trace_id-xyz-1,sentry-sampled=true,sentry-environment=user', + ]); + }); + + it('deduplicates traceparent headers if propagateTraceparent is true', () => { + mockedClientGetOptions.mockReturnValueOnce({ + tracePropagationTargets: ['https://example.com'], + propagateTraceparent: true, + }); + + const request = { + headers: [ + 'sentry-trace', + 'user-trace_id-xyz-1', + 'baggage', + 'sentry-trace_id=user-trace_id-xyz-1,sentry-sampled=true,sentry-environment=user', + 'traceparent', + '00-user-trace_id-xyz-1-01', + 'sentry-trace', + 'undici-trace_id-abc-1', + 'baggage', + 'sentry-trace_id=undici-trace_id-abc-1,sentry-sampled=true,sentry-environment=undici', + 'traceparent', + '00-undici-trace_id-abc-1-01', + ], + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toEqual([ + 'sentry-trace', + 'user-trace_id-xyz-1', + 'baggage', + 'sentry-trace_id=user-trace_id-xyz-1,sentry-sampled=true,sentry-environment=user', + 'traceparent', + '00-user-trace_id-xyz-1-01', + ]); + }); + + // admittedly an unrealistic edge case but doesn't hurt to test it + it("doesn't crash with incomplete headers array", () => { + const request = { + headers: [ + 'sentry-trace', + 'user-trace_id-xyz-1', + 'baggage', + 'sentry-trace_id=user-trace_id-xyz-1,sentry-sampled=true,sentry-environment=user', + 'sentry-trace', + 'undici-trace_id-abc-1', + 'baggage', + 'sentry-trace_id=undici-trace_id-abc-1,sentry-sampled=true,sentry-environment=undici', + 'baggage', // only the key, no value + ], + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toEqual([ + 'sentry-trace', + 'user-trace_id-xyz-1', + 'baggage', + 'sentry-trace_id=user-trace_id-xyz-1,sentry-sampled=true,sentry-environment=user', + ]); + }); + + it('dedupes multiple baggage headers with sentry- values keeps non-sentry values around', () => { + const request = { + headers: [ + 'sentry-trace', + 'user-trace_id-xyz-1', + 'baggage', + 'user-added=value,another=one', + 'baggage', + 'yet-another=value,another=two', + 'sentry-trace', + 'undici-trace_id-abc-1', + 'baggage', + 'sentry-trace_id=undici-trace_id-abc-1,sentry-sampled=true,sentry-environment=undici,', + ], + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toEqual([ + 'sentry-trace', + 'user-trace_id-xyz-1', + 'baggage', + 'sentry-trace_id=undici-trace_id-abc-1,sentry-sampled=true,sentry-environment=undici,yet-another=value,another=two,user-added=value', + ]); + }); + + it('dedupes multiple baggage headers keeps non-sentry values around', () => { + const request = { + headers: ['baggage', 'user-added=value,another=one', 'baggage', 'yet-another=value,another=two'], + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toEqual([ + 'baggage', + 'yet-another=value,another=two,user-added=value,sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging', + 'sentry-trace', + 'trace_id_1-span_id_1-1', + ]); + }); + }); + + it('doesn\'t mistake a header value with "sentry-trace" for a sentry-trace header', () => { + const request = { + headers: ['x-allow-header', 'sentry-trace'], + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toEqual([ + 'x-allow-header', + 'sentry-trace', + 'sentry-trace', + 'trace_id_1-span_id_1-1', + 'baggage', + 'sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging', + ]); + }); + + it('doesn\'t mistake a header value with "baggage" for a sentry-trace header', () => { + const request = { + headers: ['x-allow-header', 'baggage'], + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toEqual([ + 'x-allow-header', + 'baggage', + 'sentry-trace', + 'trace_id_1-span_id_1-1', + 'baggage', + 'sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging', + ]); + }); + }); + + describe('when headers are a string', () => { + it('adds sentry-trace and baggage headers to request', () => { + const request = { + headers: '', + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toBe( + 'sentry-trace: trace_id_1-span_id_1-1\r\n' + + 'baggage: sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging\r\n', + ); + }); + + describe('when sentry-trace is already set', () => { + it("preserves original sentry-trace header doesn't add baggage", () => { + const request = { + headers: 'sentry-trace: trace_id_2-span_id_2-1\r\n', + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toBe('sentry-trace: trace_id_2-span_id_2-1\r\n'); + }); + + it('preserves the original baggage header', () => { + const request = { + headers: + 'sentry-trace: trace_id_2-span_id_2-1\r\n' + + 'baggage: sentry-trace_id=trace_id_2,sentry-sampled=true,sentry-environment=staging\r\n', + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toBe( + 'sentry-trace: trace_id_2-span_id_2-1\r\n' + + 'baggage: sentry-trace_id=trace_id_2,sentry-sampled=true,sentry-environment=staging\r\n', + ); + }); + }); + + describe('pre-existing header deduplication', () => { + it('deduplicates sentry-trace and baggage headers', () => { + const request = { + headers: + 'sentry-trace: user-trace_id-xyz-1\r\n' + + 'baggage: sentry-trace_id=user-trace_id,sentry-sampled=true,sentry-environment=user\r\n' + + 'sentry-trace: undici-trace_id-abc-1\r\n' + + 'baggage: sentry-trace_id=undici-trace_id-abc-1,sentry-sampled=true,sentry-environment=undici\r\n', + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toBe( + 'sentry-trace: user-trace_id-xyz-1\r\n' + + 'baggage: sentry-trace_id=user-trace_id,sentry-sampled=true,sentry-environment=user\r\n', + ); + }); + + it("doesn't crash with incomplete headers string", () => { + const request = { + headers: + 'sentry-trace: user-trace_id-xyz-1\r\n' + + 'baggage: sentry-trace_id=user-trace_id,sentry-sampled=true,sentry-environment=user\r\n' + + 'sentry-trace: undici-trace_id-abc-1\r\n' + + 'baggage: sentry-trace_id=undici-trace_id-abc-1,sentry-sampled=true,sentry-environment=undici\r\n' + + 'baggage: \r\n', + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toBe( + 'sentry-trace: user-trace_id-xyz-1\r\n' + + 'baggage: sentry-trace_id=user-trace_id,sentry-sampled=true,sentry-environment=user\r\n', + ); + }); + }); + + it("doesn't dedupe nearly-sentry-tracing headers", () => { + const request = { + headers: + 'sentry-trace: user-trace_id-xyz-1\r\n' + + 'baggage: sentry-trace_id=user-trace_id,sentry-sampled=true,sentry-environment=user\r\n' + + 'x-sentry-trace: custom-trace_id-abc-1\r\n' + + 'x-baggage: sentry-trace_id=undici-trace_id-abc-1,sentry-sampled=true,sentry-environment=undici\r\n', + origin: 'https://some-service.com', + path: '/api/test', + } as UndiciRequest; + + addTracePropagationHeadersToFetchRequest(request, new LRUMap(100)); + + expect(request.headers).toBe( + 'sentry-trace: user-trace_id-xyz-1\r\n' + + 'baggage: sentry-trace_id=user-trace_id,sentry-sampled=true,sentry-environment=user\r\n' + + 'x-sentry-trace: custom-trace_id-abc-1\r\n' + + 'x-baggage: sentry-trace_id=undici-trace_id-abc-1,sentry-sampled=true,sentry-environment=undici\r\n', + ); + }); + }); +}); diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts index 2b5873658706..b8582b37d964 100644 --- a/packages/opentelemetry/src/propagator.ts +++ b/packages/opentelemetry/src/propagator.ts @@ -63,6 +63,8 @@ export class SentryPropagator extends W3CBaggagePropagator { } const existingBaggageHeader = getExistingBaggage(carrier); + const existingSentryTraceHeader = getExistingSentryTrace(carrier); + let baggage = propagation.getBaggage(context) || propagation.createBaggage({}); const { dynamicSamplingContext, traceId, spanId, sampled } = getInjectionData(context); @@ -72,12 +74,18 @@ export class SentryPropagator extends W3CBaggagePropagator { if (baggageEntries) { Object.entries(baggageEntries).forEach(([key, value]) => { + if (!existingSentryTraceHeader && key.startsWith(SENTRY_BAGGAGE_KEY_PREFIX)) { + // Edge case: A baggage header with sentry- keys was added previously but no + // sentry-trace header. In this case we remove the old sentry-keys and add new + // ones below. + return; + } baggage = baggage.setEntry(key, { value }); }); } } - if (dynamicSamplingContext) { + if (!existingSentryTraceHeader && dynamicSamplingContext) { baggage = Object.entries(dynamicSamplingContext).reduce((b, [dscKey, dscValue]) => { if (dscValue) { return b.setEntry(`${SENTRY_BAGGAGE_KEY_PREFIX}${dscKey}`, { value: dscValue }); @@ -87,7 +95,7 @@ export class SentryPropagator extends W3CBaggagePropagator { } // We also want to avoid setting the default OTEL trace ID, if we get that for whatever reason - if (traceId && traceId !== INVALID_TRACEID) { + if (!existingSentryTraceHeader && traceId && traceId !== INVALID_TRACEID) { setter.set(carrier, SENTRY_TRACE_HEADER, generateSentryTraceHeader(traceId, spanId, sampled)); if (propagateTraceparent) { @@ -248,6 +256,14 @@ function getExistingBaggage(carrier: unknown): string | undefined { } } +function getExistingSentryTrace(carrier: unknown): string | string[] | undefined { + try { + return (carrier as Record)[SENTRY_TRACE_HEADER]; + } catch { + return undefined; + } +} + /** * It is pretty tricky to get access to the outgoing request URL of a request in the propagator. * As we only have access to the context of the span to be sent and the carrier (=headers), diff --git a/packages/opentelemetry/test/propagator.test.ts b/packages/opentelemetry/test/propagator.test.ts index ef8223b5974a..7610f040a72d 100644 --- a/packages/opentelemetry/test/propagator.test.ts +++ b/packages/opentelemetry/test/propagator.test.ts @@ -500,14 +500,17 @@ describe('SentryPropagator', () => { ); }); - it('should overwrite existing sentry baggage header', () => { + it('overwrites existing sentry baggage values and add sentry-trace header if sentry-trace is not set yet', () => { + // This is an edeg case where someone set a baggage header with existing sentry- values but no sentry-trace header. + // There's no evidence this occurs in real-life but if it does, we can assume that this must be some kind of error + // Hence, we overwrite the existing sentry- values with our new ones but keep all other non-sentry values. const spanContext = { traceId: 'd4cda95b652f4a1592b449d5929fda1b', spanId: '6e0c63257de34c92', traceFlags: TraceFlags.SAMPLED, }; - const carrier = { + const carrier: Record = { baggage: 'foo=bar,other=yes,sentry-release=9.9.9,sentry-other=yes', }; const context = trace.setSpanContext(ROOT_CONTEXT, spanContext); @@ -520,11 +523,11 @@ describe('SentryPropagator', () => { 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', 'sentry-public_key=abc', 'sentry-environment=production', - 'sentry-other=yes', 'sentry-release=1.0.0', 'sentry-sampled=true', ].sort(), ); + expect(carrier[SENTRY_TRACE_HEADER]).toBe('d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-1'); }); it('should create baggage without propagation context', () => { @@ -537,6 +540,7 @@ describe('SentryPropagator', () => { expect(carrier[SENTRY_BAGGAGE_HEADER]).toBe( `foo=bar,sentry-environment=production,sentry-release=1.0.0,sentry-public_key=abc,sentry-trace_id=${traceId}`, ); + expect(carrier[SENTRY_TRACE_HEADER]).toBeDefined(); // whenever we set baggage, we must also set sentry-trace }); it('should NOT set baggage and sentry-trace header if instrumentation is suppressed', () => { @@ -551,6 +555,86 @@ describe('SentryPropagator', () => { expect(carrier[SENTRY_TRACE_HEADER]).toBe(undefined); expect(carrier[SENTRY_BAGGAGE_HEADER]).toBe(undefined); }); + + it("doesn't set baggage header if sentry-trace header is already set", () => { + const carrier: Record = { + [SENTRY_TRACE_HEADER]: 'abcdef-xyz-1', + }; + propagator.inject(ROOT_CONTEXT, carrier, defaultTextMapSetter); + + expect(carrier[SENTRY_BAGGAGE_HEADER]).toBe(undefined); + expect(carrier[SENTRY_TRACE_HEADER]).toBe('abcdef-xyz-1'); + }); + + describe('traceparent header', () => { + it("doesn't change baggage header if sentry-trace header is already set", () => { + const carrier: Record = { + [SENTRY_TRACE_HEADER]: 'abcdef-xyz-1', + [SENTRY_BAGGAGE_HEADER]: 'foo=bar,other=yes,sentry-release=9.9.9', + }; + propagator.inject(ROOT_CONTEXT, carrier, defaultTextMapSetter); + + expect(carrier[SENTRY_BAGGAGE_HEADER]).toBe('foo=bar,other=yes,sentry-release=9.9.9'); + expect(carrier[SENTRY_TRACE_HEADER]).toBe('abcdef-xyz-1'); + }); + + it("doesn't set traceparent header if sentry-trace header is already set", () => { + mockSdkInit({ propagateTraceparent: true }); + const carrier: Record = { + [SENTRY_TRACE_HEADER]: 'abcdef-xyz-1', + }; + propagator.inject(ROOT_CONTEXT, carrier, defaultTextMapSetter); + + expect(carrier['traceparent']).toBe(undefined); + expect(carrier[SENTRY_TRACE_HEADER]).toBe('abcdef-xyz-1'); + }); + + it('sets traceparent header if propagateTraceparent is true', () => { + mockSdkInit({ + environment: 'production', + release: '1.0.0', + tracesSampleRate: 1, + dsn: 'https://abc@domain/123', + propagateTraceparent: true, + }); + + const spanContext = { + traceId: 'd4cda95b652f4a1592b449d5929fda1b', + spanId: '6e0c63257de34c92', + traceFlags: TraceFlags.SAMPLED, + }; + const context = trace.setSpanContext(ROOT_CONTEXT, spanContext); + const baggage = propagation.createBaggage({ foo: { value: 'bar' } }); + propagator.inject(propagation.setBaggage(context, baggage), carrier, defaultTextMapSetter); + + expect(baggageToArray(carrier[SENTRY_BAGGAGE_HEADER])).toEqual( + [ + 'foo=bar', + 'sentry-trace_id=d4cda95b652f4a1592b449d5929fda1b', + 'sentry-public_key=abc', + 'sentry-environment=production', + 'sentry-release=1.0.0', + 'sentry-sampled=true', + ].sort(), + ); + expect(carrier['traceparent']).toBe('00-d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-01'); + }); + + it("doesn't set traceparent header if propagateTraceparent is false", () => { + mockSdkInit({ + environment: 'production', + release: '1.0.0', + tracesSampleRate: 1, + dsn: 'https://abc@domain/123', + propagateTraceparent: false, + }); + const carrier: Record = {}; + propagator.inject(ROOT_CONTEXT, carrier, defaultTextMapSetter); + + expect(carrier['traceparent']).toBe(undefined); + expect(carrier[SENTRY_TRACE_HEADER]).toBeDefined(); + }); + }); }); describe('extract', () => {