Skip to content

ref(node): Extract propagation context in tracingHandler #8425

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jul 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions packages/node/src/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,12 @@ import type { Span } from '@sentry/types';
import type { AddRequestDataToEventOptions } from '@sentry/utils';
import {
addRequestDataToTransaction,
baggageHeaderToDynamicSamplingContext,
dropUndefinedKeys,
extractPathForTransaction,
extractTraceparentData,
isString,
logger,
normalize,
tracingContextFromHeaders,
} from '@sentry/utils';
import type * as http from 'http';

Expand Down Expand Up @@ -62,11 +61,13 @@ export function tracingHandler(): (
return next();
}

// If there is a trace header set, we extract the data from it (parentSpanId, traceId, and sampling decision)
const traceparentData =
req.headers && isString(req.headers['sentry-trace']) && extractTraceparentData(req.headers['sentry-trace']);
const incomingBaggageHeaders = req.headers?.baggage;
const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(incomingBaggageHeaders);
const sentryTrace = req.headers && isString(req.headers['sentry-trace']) ? req.headers['sentry-trace'] : undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: could be slightly simplified to:

Suggested change
const sentryTrace = req.headers && isString(req.headers['sentry-trace']) ? req.headers['sentry-trace'] : undefined;
const sentryTrace = req.headers?.['sentry-trace'] || undefined;

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have the isString check here because req.headers['sentry-trace'] can provide an array, and we currently don't consider that valid.

I'd rather come back and re-evaluate that decision for every header usage, but for now don't want to unblock this change in particular. Adding as a TODO to the GH issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I see, makes sense. 👍

const baggage = req.headers?.baggage;
const { traceparentData, dynamicSamplingContext, propagationContext } = tracingContextFromHeaders(
sentryTrace,
baggage,
);
hub.getScope().setPropagationContext(propagationContext);

const [name, source] = extractPathForTransaction(req, { path: true, method: true });
const transaction = startTransaction(
Expand Down
28 changes: 25 additions & 3 deletions packages/node/test/handlers.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { Hub } from '@sentry/core';
import * as sentryCore from '@sentry/core';
import { setAsyncContextStrategy, Transaction } from '@sentry/core';
import type { Event } from '@sentry/types';
import type { Event, PropagationContext } from '@sentry/types';
import { SentryError } from '@sentry/utils';
import * as http from 'http';

Expand Down Expand Up @@ -209,6 +209,11 @@ describe('tracingHandler', () => {
jest.restoreAllMocks();
});

function getPropagationContext(): PropagationContext {
// @ts-expect-error accesing private property for test
return hub.getScope()._propagationContext;
Comment on lines +213 to +214
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// @ts-expect-error accesing private property for test
return hub.getScope()._propagationContext;
return hub.getScope()['_propagationContext'];

You can access private properties without TS complaining and without @ts- comment this way - we are using this in replay tests quite a lot :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh TIL! good call.

}

it('creates a transaction when handling a request', () => {
const startTransaction = jest.spyOn(sentryCore, 'startTransaction');

Expand Down Expand Up @@ -251,6 +256,13 @@ describe('tracingHandler', () => {

const transaction = (res as any).__sentry_transaction;

expect(getPropagationContext()).toEqual({
traceId: '12312012123120121231201212312012',
parentSpanId: '1121201211212012',
spanId: expect.any(String),
sampled: false,
});

// since we have no tracesSampler defined, the default behavior (inherit if possible) applies
expect(transaction.traceId).toEqual('12312012123120121231201212312012');
expect(transaction.parentSpanId).toEqual('1121201211212012');
Expand All @@ -260,18 +272,26 @@ describe('tracingHandler', () => {

it("pulls parent's data from tracing and baggage headers on the request", () => {
req.headers = {
'sentry-trace': '12312012123120121231201212312012-1121201211212012-0',
'sentry-trace': '12312012123120121231201212312012-1121201211212012-1',
baggage: 'sentry-version=1.0,sentry-environment=production',
};

sentryTracingMiddleware(req, res, next);

expect(getPropagationContext()).toEqual({
traceId: '12312012123120121231201212312012',
parentSpanId: '1121201211212012',
spanId: expect.any(String),
sampled: true,
dsc: { version: '1.0', environment: 'production' },
});

const transaction = (res as any).__sentry_transaction;

// since we have no tracesSampler defined, the default behavior (inherit if possible) applies
expect(transaction.traceId).toEqual('12312012123120121231201212312012');
expect(transaction.parentSpanId).toEqual('1121201211212012');
expect(transaction.sampled).toEqual(false);
expect(transaction.sampled).toEqual(true);
expect(transaction.metadata?.dynamicSamplingContext).toStrictEqual({ version: '1.0', environment: 'production' });
});

Expand All @@ -283,6 +303,8 @@ describe('tracingHandler', () => {

sentryTracingMiddleware(req, res, next);

expect(getPropagationContext().dsc).toEqual({ version: '1.0', environment: 'production' });

const transaction = (res as any).__sentry_transaction;
expect(transaction.metadata?.dynamicSamplingContext).toStrictEqual({ version: '1.0', environment: 'production' });
});
Expand Down