Skip to content

Commit ad6fa1f

Browse files
mydeamydea
committed
fix(node): Fix baggage propagation
1 parent 0889751 commit ad6fa1f

File tree

14 files changed

+243
-78
lines changed

14 files changed

+243
-78
lines changed

‎dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -86,9 +86,7 @@ test('Should not propagate baggage and ignore original 3rd party baggage entries
8686
});
8787
});
8888

89-
// TODO(v8): Fix this test
90-
// eslint-disable-next-line jest/no-disabled-tests
91-
test.skip('Should populate and propagate sentry baggage if sentry-trace header does not exist', async () => {
89+
test('Should populate and propagate sentry baggage if sentry-trace header does not exist', async () => {
9290
const runner = createRunner(__dirname, '..', 'server.ts').start();
9391

9492
const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express');
@@ -105,9 +103,7 @@ test.skip('Should populate and propagate sentry baggage if sentry-trace header d
105103
});
106104
});
107105

108-
// TODO(v8): Fix this test
109-
// eslint-disable-next-line jest/no-disabled-tests
110-
test.skip('Should populate Sentry and ignore 3rd party content if sentry-trace header does not exist', async () => {
106+
test('Should populate Sentry and ignore 3rd party content if sentry-trace header does not exist', async () => {
111107
const runner = createRunner(__dirname, '..', 'server.ts').start();
112108

113109
const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express', {

‎dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,8 @@ Sentry.setUser({ id: 'user123' });
2626
app.use(cors());
2727

2828
app.get('/test/express', (_req, res) => {
29-
// eslint-disable-next-line deprecation/deprecation
30-
const transaction = Sentry.getCurrentScope().getTransaction();
31-
const traceId = transaction?.spanContext().traceId;
29+
const span = Sentry.getActiveSpan();
30+
const traceId = span?.spanContext().traceId;
3231
const headers = http.get('http://somewhere.not.sentry/').getHeaders();
3332
if (traceId) {
3433
headers['baggage'] = (headers['baggage'] as string).replace(traceId, '__SENTRY_TRACE_ID__');

‎dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/test.ts

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,21 +5,28 @@ afterAll(() => {
55
cleanupChildProcesses();
66
});
77

8-
// TODO(v8): Fix this test
9-
// eslint-disable-next-line jest/no-disabled-tests
10-
test.skip('should attach a baggage header to an outgoing request.', async () => {
8+
test('should attach a baggage header to an outgoing request.', async () => {
119
const runner = createRunner(__dirname, 'server.ts').start();
1210

1311
const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express');
1412

1513
expect(response).toBeDefined();
14+
15+
const baggage = response?.test_data.baggage?.split(',').sort();
16+
17+
expect(baggage).toEqual([
18+
'sentry-environment=prod',
19+
'sentry-public_key=public',
20+
'sentry-release=1.0',
21+
'sentry-sample_rate=1',
22+
'sentry-sampled=true',
23+
'sentry-trace_id=__SENTRY_TRACE_ID__',
24+
'sentry-transaction=GET%20%2Ftest%2Fexpress',
25+
]);
26+
1627
expect(response).toMatchObject({
1728
test_data: {
1829
host: 'somewhere.not.sentry',
19-
baggage:
20-
'sentry-environment=prod,sentry-release=1.0,sentry-public_key=public' +
21-
',sentry-trace_id=__SENTRY_TRACE_ID__,sentry-sample_rate=1,sentry-transaction=GET%20%2Ftest%2Fexpress' +
22-
',sentry-sampled=true',
2330
},
2431
});
2532
});

‎dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts

Lines changed: 32 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,7 @@ afterAll(() => {
55
cleanupChildProcesses();
66
});
77

8-
// TODO(v8): Fix this test
9-
// eslint-disable-next-line jest/no-disabled-tests
10-
test.skip('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with incoming DSC', async () => {
8+
test('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with incoming DSC', async () => {
119
const runner = createRunner(__dirname, 'server.ts').start();
1210

1311
const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express', {
@@ -16,34 +14,52 @@ test.skip('should ignore sentry-values in `baggage` header of a third party vend
1614
});
1715

1816
expect(response).toBeDefined();
17+
18+
const baggage = response?.test_data.baggage?.split(',').sort();
19+
1920
expect(response).toMatchObject({
2021
test_data: {
2122
host: 'somewhere.not.sentry',
22-
baggage: [
23-
'other=vendor,foo=bar,third=party,sentry-release=9.9.9,sentry-environment=staging,sentry-sample_rate=0.54,last=item',
24-
'sentry-release=2.1.0,sentry-environment=myEnv',
25-
],
2623
},
2724
});
25+
26+
expect(baggage).toEqual([
27+
'foo=bar',
28+
'last=item',
29+
'other=vendor',
30+
'sentry-environment=myEnv',
31+
'sentry-release=2.1.0',
32+
'sentry-sample_rate=0.54',
33+
'third=party',
34+
]);
2835
});
2936

30-
// TODO(v8): Fix this test
31-
// eslint-disable-next-line jest/no-disabled-tests
32-
test.skip('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with new DSC', async () => {
37+
test('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with new DSC', async () => {
3338
const runner = createRunner(__dirname, 'server.ts').start();
3439

3540
const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express');
3641

3742
expect(response).toBeDefined();
43+
44+
const baggage = response?.test_data.baggage?.split(',').sort();
45+
3846
expect(response).toMatchObject({
3947
test_data: {
4048
host: 'somewhere.not.sentry',
41-
baggage: [
42-
'other=vendor,foo=bar,third=party,sentry-release=9.9.9,sentry-environment=staging,sentry-sample_rate=0.54,last=item',
43-
expect.stringMatching(
44-
/sentry-environment=prod,sentry-release=1\.0,sentry-public_key=public,sentry-trace_id=[0-9a-f]{32},sentry-sample_rate=1,sentry-transaction=GET%20%2Ftest%2Fexpress/,
45-
),
46-
],
4749
},
4850
});
51+
52+
expect(baggage).toEqual([
53+
'foo=bar',
54+
'last=item',
55+
'other=vendor',
56+
'sentry-environment=prod',
57+
'sentry-public_key=public',
58+
'sentry-release=1.0',
59+
'sentry-sample_rate=1',
60+
'sentry-sampled=true',
61+
expect.stringMatching(/sentry-trace_id=[0-9a-f]{32}/),
62+
'sentry-transaction=GET%20%2Ftest%2Fexpress',
63+
'third=party',
64+
]);
4965
});

‎dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,7 @@ afterAll(() => {
55
cleanupChildProcesses();
66
});
77

8-
// TODO(v8): Fix this test
9-
// eslint-disable-next-line jest/no-disabled-tests
10-
test.skip('should merge `baggage` header of a third party vendor with the Sentry DSC baggage items', async () => {
8+
test('should merge `baggage` header of a third party vendor with the Sentry DSC baggage items', async () => {
119
const runner = createRunner(__dirname, 'server.ts').start();
1210

1311
const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express', {
@@ -19,7 +17,7 @@ test.skip('should merge `baggage` header of a third party vendor with the Sentry
1917
expect(response).toMatchObject({
2018
test_data: {
2119
host: 'somewhere.not.sentry',
22-
baggage: ['other=vendor,foo=bar,third=party', 'sentry-release=2.0.0,sentry-environment=myEnv'],
20+
baggage: 'other=vendor,foo=bar,third=party,sentry-release=2.0.0,sentry-environment=myEnv',
2321
},
2422
});
2523
});

‎dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/server.ts

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import { loggingTransport } from '@sentry-internal/node-integration-tests';
2-
import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core';
32
import * as Sentry from '@sentry/node';
43

54
export type TestAPIResponse = { test_data: { host: string; 'sentry-trace': string; baggage: string } };
@@ -31,10 +30,6 @@ Sentry.setUser({ id: 'user123' });
3130
app.use(cors());
3231

3332
app.get('/test/express', (_req, res) => {
34-
// eslint-disable-next-line deprecation/deprecation
35-
const transaction = Sentry.getCurrentScope().getTransaction();
36-
transaction?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route');
37-
3833
const headers = http.get('http://somewhere.not.sentry/').getHeaders();
3934
// Responding with the headers outgoing request headers back to the assertions.
4035
res.send({ test_data: headers });

‎dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/test.ts

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,7 @@ afterAll(() => {
55
cleanupChildProcesses();
66
});
77

8-
// TODO(v8): Fix this test
9-
// eslint-disable-next-line jest/no-disabled-tests
10-
test.skip('Includes transaction in baggage if the transaction name is parameterized', async () => {
8+
test('Includes transaction in baggage if the transaction name is parameterized', async () => {
119
const runner = createRunner(__dirname, 'server.ts').start();
1210

1311
const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express');

‎dev-packages/node-integration-tests/suites/tracing/spans/scenario.ts

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,21 @@ import * as http from 'http';
1212

1313
// eslint-disable-next-line @typescript-eslint/no-floating-promises
1414
Sentry.startSpan({ name: 'test_transaction' }, async () => {
15-
http.get(`${process.env.SERVER_URL}/api/v0`);
16-
http.get(`${process.env.SERVER_URL}/api/v1`);
17-
18-
// Give it a tick to resolve...
19-
await new Promise(resolve => setTimeout(resolve, 100));
15+
await makeHttpRequest(`${process.env.SERVER_URL}/api/v0`);
16+
await makeHttpRequest(`${process.env.SERVER_URL}/api/v1`);
2017
});
18+
19+
function makeHttpRequest(url: string): Promise<void> {
20+
return new Promise<void>(resolve => {
21+
http
22+
.request(url, httpRes => {
23+
httpRes.on('data', () => {
24+
// we don't care about data
25+
});
26+
httpRes.on('end', () => {
27+
resolve();
28+
});
29+
})
30+
.end();
31+
});
32+
}

‎packages/opentelemetry/src/propagator.ts

Lines changed: 46 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
1-
import type { Baggage, Context, SpanContext, TextMapGetter, TextMapSetter } from '@opentelemetry/api';
1+
import type { Baggage, Context, Span, SpanContext, TextMapGetter, TextMapSetter } from '@opentelemetry/api';
22
import { context } from '@opentelemetry/api';
33
import { TraceFlags, propagation, trace } from '@opentelemetry/api';
44
import { TraceState, W3CBaggagePropagator, isTracingSuppressed } from '@opentelemetry/core';
55
import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
66
import type { continueTrace } from '@sentry/core';
7+
import { getRootSpan } from '@sentry/core';
78
import { spanToJSON } from '@sentry/core';
89
import { getClient, getCurrentScope, getDynamicSamplingContextFromClient, getIsolationScope } from '@sentry/core';
910
import type { DynamicSamplingContext, Options, PropagationContext } from '@sentry/types';
@@ -27,18 +28,26 @@ import {
2728
} from './constants';
2829
import { DEBUG_BUILD } from './debug-build';
2930
import { getScopesFromContext, setScopesOnContext } from './utils/contextData';
31+
import { getDynamicSamplingContextFromSpan } from './utils/dynamicSamplingContext';
3032
import { setIsSetup } from './utils/setupCheck';
3133

3234
/** Get the Sentry propagation context from a span context. */
33-
export function getPropagationContextFromSpanContext(spanContext: SpanContext): PropagationContext {
35+
export function getPropagationContextFromSpan(span: Span): PropagationContext {
36+
const spanContext = span.spanContext();
3437
const { traceId, spanId, traceState } = spanContext;
3538

39+
// When we have a dsc trace state, it means this came from the incoming trace
40+
// Then this takes presedence over the root span
3641
const dscString = traceState ? traceState.get(SENTRY_TRACE_STATE_DSC) : undefined;
37-
const dsc = dscString ? baggageHeaderToDynamicSamplingContext(dscString) : undefined;
42+
const traceStateDsc = dscString ? baggageHeaderToDynamicSamplingContext(dscString) : undefined;
43+
3844
const parentSpanId = traceState ? traceState.get(SENTRY_TRACE_STATE_PARENT_SPAN_ID) : undefined;
3945

4046
const sampled = getSamplingDecision(spanContext);
4147

48+
// No trace state? --> Take DSC from root span
49+
const dsc = traceStateDsc || getDynamicSamplingContextFromSpan(getRootSpan(span));
50+
4251
return {
4352
traceId,
4453
spanId,
@@ -85,10 +94,18 @@ export class SentryPropagator extends W3CBaggagePropagator {
8594
return;
8695
}
8796

97+
const existingBaggageHeader = getExistingBaggage(carrier);
8898
let baggage = propagation.getBaggage(context) || propagation.createBaggage({});
8999

90100
const { dynamicSamplingContext, traceId, spanId, sampled } = getInjectionData(context);
91101

102+
if (existingBaggageHeader) {
103+
const baggageEntries = parseBaggageHeaderString(existingBaggageHeader);
104+
for (const [key, value] of baggageEntries) {
105+
baggage = baggage.setEntry(key, { value });
106+
}
107+
}
108+
92109
if (dynamicSamplingContext) {
93110
baggage = Object.entries(dynamicSamplingContext).reduce<Baggage>((b, [dscKey, dscValue]) => {
94111
if (dscValue) {
@@ -196,7 +213,8 @@ function getInjectionData(context: Context): {
196213
// If we have a local span, we can just pick everything from it
197214
if (span && !spanIsRemote) {
198215
const spanContext = span.spanContext();
199-
const propagationContext = getPropagationContextFromSpanContext(spanContext);
216+
217+
const propagationContext = getPropagationContextFromSpan(span);
200218
const dynamicSamplingContext = getDynamicSamplingContext(propagationContext, spanContext.traceId);
201219
return {
202220
dynamicSamplingContext,
@@ -220,9 +238,9 @@ function getInjectionData(context: Context): {
220238
}
221239

222240
// Else, we look at the remote span context
223-
const spanContext = trace.getSpanContext(context);
224-
if (spanContext) {
225-
const propagationContext = getPropagationContextFromSpanContext(spanContext);
241+
if (span) {
242+
const spanContext = span.spanContext();
243+
const propagationContext = getPropagationContextFromSpan(span);
226244
const dynamicSamplingContext = getDynamicSamplingContext(propagationContext, spanContext.traceId);
227245

228246
return {
@@ -338,3 +356,24 @@ export function getSamplingDecision(spanContext: SpanContext): boolean | undefin
338356

339357
return undefined;
340358
}
359+
360+
/** Try to get the existing baggage header so we can merge this in. */
361+
function getExistingBaggage(carrier: unknown): string | undefined {
362+
try {
363+
const baggage = (carrier as Record<string, string | string[]>)[SENTRY_BAGGAGE_HEADER];
364+
return Array.isArray(baggage) ? baggage.join(',') : baggage;
365+
} catch {
366+
return undefined;
367+
}
368+
}
369+
370+
/**
371+
* Will parse a baggage header into an array of tuples,
372+
* where the first item is the baggage name, the second the baggage value.
373+
*/
374+
function parseBaggageHeaderString(baggageHeader: string): [string, string][] {
375+
return baggageHeader.split(',').map(baggageEntry => {
376+
const [key, value] = baggageEntry.split('=');
377+
return [decodeURIComponent(key.trim()), decodeURIComponent(value.trim())];
378+
});
379+
}

‎packages/opentelemetry/src/sampler.ts

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import type { Attributes, Context, SpanContext } from '@opentelemetry/api';
1+
import type { Attributes, Context, Span } from '@opentelemetry/api';
22
import { isSpanContextValid, trace } from '@opentelemetry/api';
33
import { TraceState } from '@opentelemetry/core';
44
import type { Sampler, SamplingResult } from '@opentelemetry/sdk-trace-base';
@@ -9,7 +9,7 @@ import { logger } from '@sentry/utils';
99
import { SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING } from './constants';
1010

1111
import { DEBUG_BUILD } from './debug-build';
12-
import { getPropagationContextFromSpanContext, getSamplingDecision } from './propagator';
12+
import { getPropagationContextFromSpan, getSamplingDecision } from './propagator';
1313
import { setIsSetup } from './utils/setupCheck';
1414

1515
/**
@@ -38,16 +38,17 @@ export class SentrySampler implements Sampler {
3838
return { decision: SamplingDecision.NOT_RECORD };
3939
}
4040

41-
const parentContext = trace.getSpanContext(context);
41+
const parentSpan = trace.getSpan(context);
42+
const parentContext = parentSpan?.spanContext();
4243
const traceState = parentContext?.traceState || new TraceState();
4344

4445
let parentSampled: boolean | undefined = undefined;
4546

4647
// Only inherit sample rate if `traceId` is the same
4748
// Note for testing: `isSpanContextValid()` checks the format of the traceId/spanId, so we need to pass valid ones
48-
if (parentContext && isSpanContextValid(parentContext) && parentContext.traceId === traceId) {
49+
if (parentSpan && parentContext && isSpanContextValid(parentContext) && parentContext.traceId === traceId) {
4950
if (parentContext.isRemote) {
50-
parentSampled = getParentRemoteSampled(parentContext);
51+
parentSampled = getParentRemoteSampled(parentSpan);
5152
DEBUG_BUILD &&
5253
logger.log(`[Tracing] Inheriting remote parent's sampled decision for ${spanName}: ${parentSampled}`);
5354
} else {
@@ -90,9 +91,9 @@ export class SentrySampler implements Sampler {
9091
}
9192
}
9293

93-
function getParentRemoteSampled(spanContext: SpanContext): boolean | undefined {
94-
const traceId = spanContext.traceId;
95-
const traceparentData = getPropagationContextFromSpanContext(spanContext);
94+
function getParentRemoteSampled(parentSpan: Span): boolean | undefined {
95+
const traceId = parentSpan.spanContext().traceId;
96+
const traceparentData = getPropagationContextFromSpan(parentSpan);
9697

9798
// Only inherit sampled if `traceId` is the same
9899
return traceparentData && traceId === traceparentData.traceId ? traceparentData.sampled : undefined;

0 commit comments

Comments
 (0)