Skip to content

Commit 7309b5d

Browse files
Lms24Lms24
authored
ref(tracing): Check and set mutability of baggage (#5205)
Introduce a third item to our `Baggage` tuple which is a mutability flag. The reason for adding this flag is to set the mutability of the baggage object when we receive one, either from incoming requests or `<meta>` tags in the browser. It's necessary to decide about mutability at this time because the decision depends on the `sentry-trace` header. Since we're lazily populating baggage when it gets sent (propagated), we have to know at that time, if we are allowed to add `sentry-` entries to it, or if we should just pass it along as is. The only way to cover all cases of when we're allowed to make changes is to set the flag at the incoming time and check it at population time. Specifically, we identify and handle mutability in the following cases: * Incoming request w/ baggage that contains `sentry-*` items (and possibly 3rd party items): * At incoming request time (IRT): Parse and store baggage on the span. Set it immutable * At baggage propagation time (BPT): Propagate baggage * Incoming request w/o baggage header: * if `sentry-trace` is present * IRT: create empty baggage object and set it to immutable * BPT: propagate baggage * else, * IRT: create empty baggage object (to be populated later) and leave it mutable * BPT: populate, set it immutable, propagate it * Incoming request with baggage header that only contains 3rd party items: Same as above. In both cases, we propagate the 3rd party baggage. * Incoming request w/o any of the two headers: * IRT: create empty baggage object (to be populated later) and leave it mutable * BPT: populate, set it immutable, propagate it * No baggage set on the span at BPT: populate, set it immutable, propagate it Checking for the presence of the `sentry-trace` header is important because if it is present, we know that the trace was already started which means that whatever handler is receiving it, cannot modify baggage anymore. Getting a `sentry-trace` but no `baggage` header, can (and will) for example occur, when the SDK that started the trace does not yet support and propagate baggage.
1 parent f6c87dd commit 7309b5d

File tree

16 files changed

+324
-72
lines changed

16 files changed

+324
-72
lines changed

‎packages/nextjs/src/utils/instrumentServer.ts‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import {
1313
fill,
1414
isString,
1515
logger,
16-
parseBaggageString,
16+
parseBaggageSetMutability,
1717
stripUrlQueryAndFragment,
1818
} from '@sentry/utils';
1919
import * as domain from 'domain';
@@ -257,8 +257,8 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler {
257257
__DEBUG_BUILD__ && logger.log(`[Tracing] Continuing trace ${traceparentData?.traceId}.`);
258258
}
259259

260-
const baggage =
261-
nextReq.headers && isString(nextReq.headers.baggage) && parseBaggageString(nextReq.headers.baggage);
260+
const rawBaggageString = nextReq.headers && isString(nextReq.headers.baggage) && nextReq.headers.baggage;
261+
const baggage = parseBaggageSetMutability(rawBaggageString, traceparentData);
262262

263263
// pull off query string, if any
264264
const reqPath = stripUrlQueryAndFragment(nextReq.url);
@@ -271,9 +271,8 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler {
271271
{
272272
name: `${namePrefix}${reqPath}`,
273273
op: 'http.server',
274-
metadata: { requestPath: reqPath },
274+
metadata: { requestPath: reqPath, baggage },
275275
...traceparentData,
276-
...(baggage && { metadata: { baggage: baggage } }),
277276
},
278277
// Extra context passed to the `tracesSampler` (Note: We're combining `nextReq` and `req` this way in order
279278
// to not break people's `tracesSampler` functions, even though the format of `nextReq` has changed (see

‎packages/nextjs/src/utils/withSentry.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import {
66
isString,
77
logger,
88
objectify,
9-
parseBaggageString,
9+
parseBaggageSetMutability,
1010
stripUrlQueryAndFragment,
1111
} from '@sentry/utils';
1212
import * as domain from 'domain';
@@ -53,7 +53,8 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
5353
__DEBUG_BUILD__ && logger.log(`[Tracing] Continuing trace ${traceparentData?.traceId}.`);
5454
}
5555

56-
const baggage = req.headers && isString(req.headers.baggage) && parseBaggageString(req.headers.baggage);
56+
const rawBaggageString = req.headers && isString(req.headers.baggage) && req.headers.baggage;
57+
const baggage = parseBaggageSetMutability(rawBaggageString, traceparentData);
5758

5859
const url = `${req.url}`;
5960
// pull off query string, if any
@@ -73,7 +74,7 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
7374
name: `${reqMethod}${reqPath}`,
7475
op: 'http.server',
7576
...traceparentData,
76-
...(baggage && { metadata: { baggage: baggage } }),
77+
metadata: { baggage: baggage },
7778
},
7879
// extra context passed to the `tracesSampler`
7980
{ request: req },

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

Lines changed: 56 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,50 +3,97 @@ import * as path from 'path';
33
import { getAPIResponse, runServer } from '../../../../utils/index';
44
import { TestAPIResponse } from '../server';
55

6-
test('Should assign `baggage` header which contains 3rd party trace baggage data to an outgoing request.', async () => {
6+
test('Should not overwrite baggage if the incoming request already has Sentry baggage data.', async () => {
77
const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
88

99
const response = (await getAPIResponse(new URL(`${url}/express`), {
10-
baggage: 'foo=bar,bar=baz',
10+
baggage: 'sentry-version=2.0.0,sentry-environment=myEnv',
1111
})) as TestAPIResponse;
1212

1313
expect(response).toBeDefined();
1414
expect(response).toMatchObject({
1515
test_data: {
1616
host: 'somewhere.not.sentry',
17-
baggage: 'foo=bar,bar=baz,sentry-environment=prod,sentry-release=1.0',
17+
baggage: 'sentry-version=2.0.0,sentry-environment=myEnv',
1818
},
1919
});
2020
});
2121

22-
test('Should not overwrite baggage if the incoming request already has Sentry baggage data.', async () => {
22+
test('Should pass along sentry and 3rd party trace baggage data from an incoming to an outgoing request.', async () => {
2323
const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
2424

2525
const response = (await getAPIResponse(new URL(`${url}/express`), {
26-
baggage: 'sentry-version=2.0.0,sentry-environment=myEnv',
26+
baggage: 'sentry-version=2.0.0,sentry-environment=myEnv,dogs=great',
2727
})) as TestAPIResponse;
2828

2929
expect(response).toBeDefined();
3030
expect(response).toMatchObject({
3131
test_data: {
3232
host: 'somewhere.not.sentry',
33-
baggage: 'sentry-version=2.0.0,sentry-environment=myEnv',
33+
baggage: expect.stringContaining('dogs=great,sentry-version=2.0.0,sentry-environment=myEnv'),
3434
},
3535
});
3636
});
3737

38-
test('Should pass along sentry and 3rd party trace baggage data from an incoming to an outgoing request.', async () => {
38+
test('Should propagate empty baggage if sentry-trace header is present in incoming request but no baggage header', async () => {
3939
const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
4040

4141
const response = (await getAPIResponse(new URL(`${url}/express`), {
42-
baggage: 'sentry-version=2.0.0,sentry-environment=myEnv,dogs=great',
42+
'sentry-trace': '',
4343
})) as TestAPIResponse;
4444

4545
expect(response).toBeDefined();
4646
expect(response).toMatchObject({
4747
test_data: {
4848
host: 'somewhere.not.sentry',
49-
baggage: expect.stringContaining('dogs=great,sentry-version=2.0.0,sentry-environment=myEnv'),
49+
baggage: '',
50+
},
51+
});
52+
});
53+
54+
test('Should propagate empty sentry and original 3rd party baggage if sentry-trace header is present', async () => {
55+
const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
56+
57+
const response = (await getAPIResponse(new URL(`${url}/express`), {
58+
'sentry-trace': '',
59+
baggage: 'foo=bar',
60+
})) as TestAPIResponse;
61+
62+
expect(response).toBeDefined();
63+
expect(response).toMatchObject({
64+
test_data: {
65+
host: 'somewhere.not.sentry',
66+
baggage: 'foo=bar',
67+
},
68+
});
69+
});
70+
71+
test('Should populate and propagate sentry baggage if sentry-trace header does not exist', async () => {
72+
const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
73+
74+
const response = (await getAPIResponse(new URL(`${url}/express`), {})) as TestAPIResponse;
75+
76+
expect(response).toBeDefined();
77+
expect(response).toMatchObject({
78+
test_data: {
79+
host: 'somewhere.not.sentry',
80+
baggage: 'sentry-environment=prod,sentry-release=1.0',
81+
},
82+
});
83+
});
84+
85+
test('Should populate Sentry and propagate 3rd party content if sentry-trace header does not exist', async () => {
86+
const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
87+
88+
const response = (await getAPIResponse(new URL(`${url}/express`), {
89+
baggage: 'foo=bar,bar=baz',
90+
})) as TestAPIResponse;
91+
92+
expect(response).toBeDefined();
93+
expect(response).toMatchObject({
94+
test_data: {
95+
host: 'somewhere.not.sentry',
96+
baggage: 'foo=bar,bar=baz,sentry-environment=prod,sentry-release=1.0',
5097
},
5198
});
5299
});

‎packages/node/src/handlers.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import {
88
isString,
99
logger,
1010
normalize,
11-
parseBaggageString,
11+
parseBaggageSetMutability,
1212
stripUrlQueryAndFragment,
1313
} from '@sentry/utils';
1414
import * as cookie from 'cookie';
@@ -63,14 +63,16 @@ export function tracingHandler(): (
6363
// If there is a trace header set, we extract the data from it (parentSpanId, traceId, and sampling decision)
6464
const traceparentData =
6565
req.headers && isString(req.headers['sentry-trace']) && extractTraceparentData(req.headers['sentry-trace']);
66-
const baggage = req.headers && isString(req.headers.baggage) && parseBaggageString(req.headers.baggage);
66+
const rawBaggageString = req.headers && isString(req.headers.baggage) && req.headers.baggage;
67+
68+
const baggage = parseBaggageSetMutability(rawBaggageString, traceparentData);
6769

6870
const transaction = startTransaction(
6971
{
7072
name: extractExpressTransactionName(req, { path: true, method: true }),
7173
op: 'http.server',
7274
...traceparentData,
73-
...(baggage && { metadata: { baggage: baggage } }),
75+
metadata: { baggage: baggage },
7476
},
7577
// extra context passed to the tracesSampler
7678
{ request: extractRequestData(req) },

‎packages/node/test/handlers.test.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import * as sentryHub from '@sentry/hub';
33
import { Hub } from '@sentry/hub';
44
import { Transaction } from '@sentry/tracing';
55
import { Baggage, Runtime } from '@sentry/types';
6-
import { SentryError } from '@sentry/utils';
6+
import { isBaggageEmpty, isBaggageMutable, SentryError } from '@sentry/utils';
77
import * as http from 'http';
88
import * as net from 'net';
99

@@ -368,7 +368,9 @@ describe('tracingHandler', () => {
368368
expect(transaction.traceId).toEqual('12312012123120121231201212312012');
369369
expect(transaction.parentSpanId).toEqual('1121201211212012');
370370
expect(transaction.sampled).toEqual(false);
371-
expect(transaction.metadata?.baggage).toBeUndefined();
371+
expect(transaction.metadata?.baggage).toBeDefined();
372+
expect(isBaggageEmpty(transaction.metadata?.baggage)).toBe(true);
373+
expect(isBaggageMutable(transaction.metadata?.baggage)).toBe(false);
372374
});
373375

374376
it("pulls parent's data from tracing and baggage headers on the request", () => {
@@ -386,7 +388,11 @@ describe('tracingHandler', () => {
386388
expect(transaction.parentSpanId).toEqual('1121201211212012');
387389
expect(transaction.sampled).toEqual(false);
388390
expect(transaction.metadata?.baggage).toBeDefined();
389-
expect(transaction.metadata?.baggage).toEqual([{ version: '1.0', environment: 'production' }, ''] as Baggage);
391+
expect(transaction.metadata?.baggage).toEqual([
392+
{ version: '1.0', environment: 'production' },
393+
'',
394+
false,
395+
] as Baggage);
390396
});
391397

392398
it("pulls parent's baggage (sentry + third party entries) headers on the request", () => {
@@ -402,6 +408,7 @@ describe('tracingHandler', () => {
402408
expect(transaction.metadata?.baggage).toEqual([
403409
{ version: '1.0', environment: 'production' },
404410
'dogs=great,cats=boring',
411+
false,
405412
] as Baggage);
406413
});
407414

‎packages/serverless/src/awslambda.ts‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import {
1111
} from '@sentry/node';
1212
import { extractTraceparentData } from '@sentry/tracing';
1313
import { Integration } from '@sentry/types';
14-
import { isString, logger, parseBaggageString } from '@sentry/utils';
14+
import { isString, logger, parseBaggageSetMutability } from '@sentry/utils';
1515
// NOTE: I have no idea how to fix this right now, and don't want to waste more time, as it builds just fine — Kamil
1616
// eslint-disable-next-line import/no-unresolved
1717
import { Context, Handler } from 'aws-lambda';
@@ -286,16 +286,15 @@ export function wrapHandler<TEvent, TResult>(
286286
traceparentData = extractTraceparentData(eventWithHeaders.headers['sentry-trace']);
287287
}
288288

289-
const baggage =
290-
eventWithHeaders.headers &&
291-
isString(eventWithHeaders.headers.baggage) &&
292-
parseBaggageString(eventWithHeaders.headers.baggage);
289+
const rawBaggageString =
290+
eventWithHeaders.headers && isString(eventWithHeaders.headers.baggage) && eventWithHeaders.headers.baggage;
291+
const baggage = parseBaggageSetMutability(rawBaggageString, traceparentData);
293292

294293
const transaction = startTransaction({
295294
name: context.functionName,
296295
op: 'awslambda.handler',
297296
...traceparentData,
298-
...(baggage && { metadata: { baggage: baggage } }),
297+
metadata: { baggage: baggage },
299298
});
300299

301300
const hub = getCurrentHub();

‎packages/serverless/src/gcpfunction/http.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { captureException, flush, getCurrentHub, Handlers, startTransaction } from '@sentry/node';
22
import { extractTraceparentData } from '@sentry/tracing';
3-
import { isString, logger, parseBaggageString, stripUrlQueryAndFragment } from '@sentry/utils';
3+
import { isString, logger, parseBaggageSetMutability, stripUrlQueryAndFragment } from '@sentry/utils';
44

55
import { domainify, getActiveDomain, proxyFunction } from './../utils';
66
import { HttpFunction, WrapperOptions } from './general';
@@ -56,16 +56,16 @@ function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial<HttpFunctionWr
5656
traceparentData = extractTraceparentData(reqWithHeaders.headers['sentry-trace']);
5757
}
5858

59-
const baggage =
60-
reqWithHeaders.headers &&
61-
isString(reqWithHeaders.headers.baggage) &&
62-
parseBaggageString(reqWithHeaders.headers.baggage);
59+
const rawBaggageString =
60+
reqWithHeaders.headers && isString(reqWithHeaders.headers.baggage) && reqWithHeaders.headers.baggage;
61+
62+
const baggage = parseBaggageSetMutability(rawBaggageString, traceparentData);
6363

6464
const transaction = startTransaction({
6565
name: `${reqMethod} ${reqUrl}`,
6666
op: 'gcp.function.http',
6767
...traceparentData,
68-
...(baggage && { metadata: { baggage: baggage } }),
68+
metadata: { baggage: baggage },
6969
});
7070

7171
// getCurrentHub() is expected to use current active domain as a carrier

‎packages/serverless/test/awslambda.test.ts‎

Lines changed: 32 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,11 @@ describe('AWSLambda', () => {
189189
const wrappedHandler = wrapHandler(handler);
190190
const rv = await wrappedHandler(fakeEvent, fakeContext, fakeCallback);
191191
expect(rv).toStrictEqual(42);
192-
expect(Sentry.startTransaction).toBeCalledWith({ name: 'functionName', op: 'awslambda.handler' });
192+
expect(Sentry.startTransaction).toBeCalledWith({
193+
name: 'functionName',
194+
op: 'awslambda.handler',
195+
metadata: { baggage: [{}, '', true] },
196+
});
193197
expectScopeSettings();
194198
// @ts-ignore see "Why @ts-ignore" note
195199
expect(Sentry.fakeTransaction.finish).toBeCalled();
@@ -208,7 +212,11 @@ describe('AWSLambda', () => {
208212
try {
209213
await wrappedHandler(fakeEvent, fakeContext, fakeCallback);
210214
} catch (e) {
211-
expect(Sentry.startTransaction).toBeCalledWith({ name: 'functionName', op: 'awslambda.handler' });
215+
expect(Sentry.startTransaction).toBeCalledWith({
216+
name: 'functionName',
217+
op: 'awslambda.handler',
218+
metadata: { baggage: [{}, '', true] },
219+
});
212220
expectScopeSettings();
213221
expect(Sentry.captureException).toBeCalledWith(error);
214222
// @ts-ignore see "Why @ts-ignore" note
@@ -251,6 +259,7 @@ describe('AWSLambda', () => {
251259
release: '2.12.1',
252260
},
253261
'maisey=silly,charlie=goofy',
262+
false,
254263
],
255264
},
256265
}),
@@ -282,6 +291,7 @@ describe('AWSLambda', () => {
282291
traceId: '12312012123120121231201212312012',
283292
parentSpanId: '1121201211212012',
284293
parentSampled: false,
294+
metadata: { baggage: [{}, '', false] },
285295
});
286296
expectScopeSettings();
287297
expect(Sentry.captureException).toBeCalledWith(e);
@@ -302,7 +312,11 @@ describe('AWSLambda', () => {
302312
const wrappedHandler = wrapHandler(handler);
303313
const rv = await wrappedHandler(fakeEvent, fakeContext, fakeCallback);
304314
expect(rv).toStrictEqual(42);
305-
expect(Sentry.startTransaction).toBeCalledWith({ name: 'functionName', op: 'awslambda.handler' });
315+
expect(Sentry.startTransaction).toBeCalledWith({
316+
name: 'functionName',
317+
op: 'awslambda.handler',
318+
metadata: { baggage: [{}, '', true] },
319+
});
306320
expectScopeSettings();
307321
// @ts-ignore see "Why @ts-ignore" note
308322
expect(Sentry.fakeTransaction.finish).toBeCalled();
@@ -332,7 +346,11 @@ describe('AWSLambda', () => {
332346
try {
333347
await wrappedHandler(fakeEvent, fakeContext, fakeCallback);
334348
} catch (e) {
335-
expect(Sentry.startTransaction).toBeCalledWith({ name: 'functionName', op: 'awslambda.handler' });
349+
expect(Sentry.startTransaction).toBeCalledWith({
350+
name: 'functionName',
351+
op: 'awslambda.handler',
352+
metadata: { baggage: [{}, '', true] },
353+
});
336354
expectScopeSettings();
337355
expect(Sentry.captureException).toBeCalledWith(error);
338356
// @ts-ignore see "Why @ts-ignore" note
@@ -367,7 +385,11 @@ describe('AWSLambda', () => {
367385
const wrappedHandler = wrapHandler(handler);
368386
const rv = await wrappedHandler(fakeEvent, fakeContext, fakeCallback);
369387
expect(rv).toStrictEqual(42);
370-
expect(Sentry.startTransaction).toBeCalledWith({ name: 'functionName', op: 'awslambda.handler' });
388+
expect(Sentry.startTransaction).toBeCalledWith({
389+
name: 'functionName',
390+
op: 'awslambda.handler',
391+
metadata: { baggage: [{}, '', true] },
392+
});
371393
expectScopeSettings();
372394
// @ts-ignore see "Why @ts-ignore" note
373395
expect(Sentry.fakeTransaction.finish).toBeCalled();
@@ -397,7 +419,11 @@ describe('AWSLambda', () => {
397419
try {
398420
await wrappedHandler(fakeEvent, fakeContext, fakeCallback);
399421
} catch (e) {
400-
expect(Sentry.startTransaction).toBeCalledWith({ name: 'functionName', op: 'awslambda.handler' });
422+
expect(Sentry.startTransaction).toBeCalledWith({
423+
name: 'functionName',
424+
op: 'awslambda.handler',
425+
metadata: { baggage: [{}, '', true] },
426+
});
401427
expectScopeSettings();
402428
expect(Sentry.captureException).toBeCalledWith(error);
403429
// @ts-ignore see "Why @ts-ignore" note

0 commit comments

Comments
 (0)