Skip to content

feat(browser): Update propagationContext on spanEnd to keep trace consistent #11631

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.

Sign up for GitHub

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 4 commits into from
Apr 17, 2024
Merged

feat(browser): Update propagationContext on spanEnd to keep trace consistent #11631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cb77638
feat(browser): Update `propagationContext` on `spanEnd` to keep trace…
Lms24 Apr 16, 2024
7197b8c
add unit test
Lms24 Apr 16, 2024
62f5e22
clean up DSC and sampled value logic
Lms24 Apr 17, 2024
a0692e5
revert reading sampled from DSC
Lms24 Apr 17, 2024
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
12 changes: 6 additions & 6 deletions dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,11 @@ sentryTest(
const request = await requestPromise;
const headers = request.headers();

// sampling decision is deferred b/c of no active span at the time of request
// sampling decision and DSC are continued from navigation span, even after it ended
const navigationTraceId = navigationTraceContext?.trace_id;
expect(headers['sentry-trace']).toMatch(new RegExp(`^${navigationTraceId}-[0-9a-f]{16}$`));
expect(headers['sentry-trace']).toMatch(new RegExp(`^${navigationTraceId}-[0-9a-f]{16}-1$`));
expect(headers['baggage']).toEqual(
`sentry-environment=production,sentry-public_key=public,sentry-trace_id=${navigationTraceId}`,
`sentry-environment=production,sentry-public_key=public,sentry-trace_id=${navigationTraceId},sentry-sample_rate=1,sentry-sampled=true`,
);
},
);
Expand Down Expand Up @@ -203,11 +203,11 @@ sentryTest(
const request = await xhrPromise;
const headers = request.headers();

// sampling decision is deferred b/c of no active span at the time of request
// sampling decision and DSC are continued from navigation span, even after it ended
const navigationTraceId = navigationTraceContext?.trace_id;
expect(headers['sentry-trace']).toMatch(new RegExp(`^${navigationTraceId}-[0-9a-f]{16}$`));
expect(headers['sentry-trace']).toMatch(new RegExp(`^${navigationTraceId}-[0-9a-f]{16}-1$`));
expect(headers['baggage']).toEqual(
`sentry-environment=production,sentry-public_key=public,sentry-trace_id=${navigationTraceId}`,
`sentry-environment=production,sentry-public_key=public,sentry-trace_id=${navigationTraceId},sentry-sample_rate=1,sentry-sampled=true`,
);
},
);
Expand Down
2 changes: 1 addition & 1 deletion ...kages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/template.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<meta charset="utf-8" />
<meta name="sentry-trace" content="12345678901234567890123456789012-1234567890123456-1" />
<meta name="baggage"
content="sentry-trace_id=12345678901234567890123456789012,sentry-sample_rate=0.2,sentry-transaction=my-transaction,sentry-public_key=public,sentry-release=1.0.0,sentry-environment=prod"/>
content="sentry-trace_id=12345678901234567890123456789012,sentry-sample_rate=0.2,sentry-sampled=true,sentry-transaction=my-transaction,sentry-public_key=public,sentry-release=1.0.0,sentry-environment=prod"/>
</head>
<body>
<button id="errorBtn">Throw Error</button>
Expand Down
2 changes: 1 addition & 1 deletion dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload-meta/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
const META_TAG_TRACE_ID = '12345678901234567890123456789012';
const META_TAG_PARENT_SPAN_ID = '1234567890123456';
const META_TAG_BAGGAGE =
'sentry-trace_id=12345678901234567890123456789012,sentry-sample_rate=0.2,sentry-transaction=my-transaction,sentry-public_key=public,sentry-release=1.0.0,sentry-environment=prod';
'sentry-trace_id=12345678901234567890123456789012,sentry-sample_rate=0.2,sentry-sampled=true,sentry-transaction=my-transaction,sentry-public_key=public,sentry-release=1.0.0,sentry-environment=prod';

sentryTest(
'create a new trace for a navigation after the <meta> tag pageload trace',
Expand Down
12 changes: 6 additions & 6 deletions dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/pageload/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,11 +124,11 @@ sentryTest(
const request = await requestPromise;
const headers = request.headers();

// sampling decision is deferred b/c of no active span at the time of request
// sampling decision and DSC are continued from the pageload span even after it ended
const pageloadTraceId = pageloadTraceContext?.trace_id;
expect(headers['sentry-trace']).toMatch(new RegExp(`^${pageloadTraceId}-[0-9a-f]{16}$`));
expect(headers['sentry-trace']).toMatch(new RegExp(`^${pageloadTraceId}-[0-9a-f]{16}-1$`));
expect(headers['baggage']).toEqual(
`sentry-environment=production,sentry-public_key=public,sentry-trace_id=${pageloadTraceId}`,
`sentry-environment=production,sentry-public_key=public,sentry-trace_id=${pageloadTraceId},sentry-sample_rate=1,sentry-sampled=true`,
);
},
);
Expand Down Expand Up @@ -191,11 +191,11 @@ sentryTest(
const request = await requestPromise;
const headers = request.headers();

// sampling decision is deferred b/c of no active span at the time of request
// sampling decision and DSC are continued from the pageload span even after it ended
const pageloadTraceId = pageloadTraceContext?.trace_id;
expect(headers['sentry-trace']).toMatch(new RegExp(`^${pageloadTraceId}-[0-9a-f]{16}$`));
expect(headers['sentry-trace']).toMatch(new RegExp(`^${pageloadTraceId}-[0-9a-f]{16}-1$`));
expect(headers['baggage']).toEqual(
`sentry-environment=production,sentry-public_key=public,sentry-trace_id=${pageloadTraceId}`,
`sentry-environment=production,sentry-public_key=public,sentry-trace_id=${pageloadTraceId},sentry-sample_rate=1,sentry-sampled=true`,
);
},
);
Expand Down
25 changes: 25 additions & 0 deletions packages/browser/src/tracing/browserTracingIntegration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@ import {
getActiveSpan,
getClient,
getCurrentScope,
getDynamicSamplingContextFromSpan,
getIsolationScope,
getRootSpan,
registerSpanErrorInstrumentation,
spanIsSampled,
spanToJSON,
startIdleSpan,
withScope,
Expand Down Expand Up @@ -282,6 +284,29 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
});
});

// A trace should to stay the consistent over the entire time span of one route.
// Therefore, when the initial pageload or navigation transaction ends, we update the
// scope's propagation context to keep span-specific attributes like the `sampled` decision and
// the dynamic sampling context valid, even after the transaction has ended.
// This ensures that the trace data is consistent for the entire duration of the route.
client.on('spanEnd', span => {
const op = spanToJSON(span).op;
if (span !== getRootSpan(span) || (op !== 'navigation' && op !== 'pageload')) {
return;
}

const scope = getCurrentScope();
const oldPropagationContext = scope.getPropagationContext();

const newPropagationContext = {
...oldPropagationContext,
sampled: oldPropagationContext.sampled !== undefined ? oldPropagationContext.sampled : spanIsSampled(span),
Copy link
Member

Choose a reason for hiding this comment

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

l/m: Not 100% sure, but maybe it would be safer to pick this from the DSC, to ensure this is aligned? So something like this:

const dsc = oldPropagationContext.dsc || getDynamicSamplingContextFromSpan(span);
const dscSampled = dsc.sampled === 'true' ? true : dsc.sampled === 'false' : false : undefined;
const sampled = oldPropagationContext.sampled !== undefined ? oldPropagationContext.sampled : dscSampled;

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, can change it! I'm just wondering about a concrete scenario where these two might diverge; like spanIsSampled returning false while the dsc.sampled being true or undefined.

Copy link
Member

Choose a reason for hiding this comment

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

Please do not use any values from the DSC in the SDK. This has potential to cause a lot of issues later on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, so we quickly discussed this in person with @cleptric and settled on updating propagationContext.sampled from the propagation context and spanIsSampled instead of the DSC. The reason is that there are talks around removing sampled from the DSC again because it's (likely) unused in ingest/product.

Both of the approaches currently lead to the same outcome, as demonstrated by the integration tests but it's safer in this case to read from propagationContext (i.e. sentry-trace) instead of DSC/baggage.

dsc: oldPropagationContext.dsc || getDynamicSamplingContextFromSpan(span),
};

scope.setPropagationContext(newPropagationContext);
});

if (options.instrumentPageLoad && WINDOW.location) {
const startSpanOptions: StartSpanOptions = {
name: WINDOW.location.pathname,
Expand Down
41 changes: 40 additions & 1 deletion packages/browser/test/unit/tracing/browserTracingIntegration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,7 @@ describe('browserTracingIntegration', () => {
expect(getCurrentScope().getScopeData().transactionName).toBe('test navigation span');
});

it("resets the scopes' propagationContexts", () => {
it("updates the scopes' propagationContexts on a navigation", () => {
const client = new BrowserClient(
getDefaultBrowserClientOptions({
integrations: [browserTracingIntegration()],
Expand Down Expand Up @@ -675,6 +675,45 @@ describe('browserTracingIntegration', () => {
expect(newIsolationScopePropCtx?.traceId).not.toEqual(oldIsolationScopePropCtx?.traceId);
expect(newCurrentScopePropCtx?.traceId).not.toEqual(oldCurrentScopePropCtx?.traceId);
});

it("saves the span's sampling decision and its DSC on the propagationContext when the span finishes", () => {
const client = new BrowserClient(
getDefaultBrowserClientOptions({
tracesSampleRate: 1,
integrations: [browserTracingIntegration({ instrumentPageLoad: false })],
}),
);
setCurrentClient(client);
client.init();

const navigationSpan = startBrowserTracingNavigationSpan(client, {
name: 'mySpan',
attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route' },
});

const propCtxBeforeEnd = getCurrentScope().getPropagationContext();
expect(propCtxBeforeEnd).toStrictEqual({
spanId: expect.stringMatching(/[a-f0-9]{16}/),
traceId: expect.stringMatching(/[a-f0-9]{32}/),
});

navigationSpan!.end();

const propCtxAfterEnd = getCurrentScope().getPropagationContext();
expect(propCtxAfterEnd).toStrictEqual({
spanId: propCtxBeforeEnd?.spanId,
traceId: propCtxBeforeEnd?.traceId,
sampled: true,
dsc: {
environment: 'production',
public_key: 'examplePublicKey',
sample_rate: '1',
sampled: 'true',
transaction: 'mySpan',
trace_id: propCtxBeforeEnd?.traceId,
},
});
});
});

describe('using the <meta> tag data', () => {
Expand Down