Skip to content

Commit d25345e

Browse files
mydeamydea
committed
ref(core): Ensure non-sampled spans are NonRecordingSpans
1 parent 46ac778 commit d25345e

File tree

3 files changed

+124
-57
lines changed

3 files changed

+124
-57
lines changed

‎packages/browser/src/tracing/browserTracingIntegration.ts‎

Lines changed: 24 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ import {
2424
getDynamicSamplingContextFromSpan,
2525
getIsolationScope,
2626
getLocationHref,
27-
getRootSpan,
2827
logger,
2928
propagationContextFromHeaders,
3029
registerSpanErrorInstrumentation,
@@ -276,6 +275,15 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
276275
_collectWebVitals();
277276
addPerformanceEntries(span, { recordClsOnPageloadSpan: !enableStandaloneClsSpans });
278277
setActiveIdleSpan(client, undefined);
278+
279+
// Ensure that DSC is updated with possibly final transaction etc.
280+
const scope = getCurrentScope();
281+
const oldPropagationContext = scope.getPropagationContext();
282+
283+
scope.setPropagationContext({
284+
...oldPropagationContext,
285+
dsc: getDynamicSamplingContextFromSpan(span),
286+
});
279287
},
280288
});
281289
setActiveIdleSpan(client, idleSpan);
@@ -293,6 +301,21 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
293301

294302
emitFinish();
295303
}
304+
305+
// A trace should to stay the consistent over the entire time span of one route.
306+
// Therefore, when the initial pageload or navigation root span ends, we update the
307+
// scope's propagation context to keep span-specific attributes like the `sampled` decision and
308+
// the dynamic sampling context valid, even after the root span has ended.
309+
// This ensures that the trace data is consistent for the entire duration of the route.
310+
const scope = getCurrentScope();
311+
const oldPropagationContext = scope.getPropagationContext();
312+
313+
scope.setPropagationContext({
314+
...oldPropagationContext,
315+
traceId: idleSpan.spanContext().traceId,
316+
sampled: spanIsSampled(idleSpan),
317+
dsc: getDynamicSamplingContextFromSpan(idleSpan),
318+
});
296319
}
297320

298321
return {
@@ -341,27 +364,6 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
341364
});
342365
});
343366

344-
// A trace should to stay the consistent over the entire time span of one route.
345-
// Therefore, when the initial pageload or navigation root span ends, we update the
346-
// scope's propagation context to keep span-specific attributes like the `sampled` decision and
347-
// the dynamic sampling context valid, even after the root span has ended.
348-
// This ensures that the trace data is consistent for the entire duration of the route.
349-
client.on('spanEnd', span => {
350-
const op = spanToJSON(span).op;
351-
if (span !== getRootSpan(span) || (op !== 'navigation' && op !== 'pageload')) {
352-
return;
353-
}
354-
355-
const scope = getCurrentScope();
356-
const oldPropagationContext = scope.getPropagationContext();
357-
358-
scope.setPropagationContext({
359-
...oldPropagationContext,
360-
sampled: oldPropagationContext.sampled !== undefined ? oldPropagationContext.sampled : spanIsSampled(span),
361-
dsc: oldPropagationContext.dsc || getDynamicSamplingContextFromSpan(span),
362-
});
363-
});
364-
365367
if (WINDOW.location) {
366368
if (instrumentPageLoad) {
367369
startBrowserTracingPageLoadSpan(client, {
@@ -453,10 +455,8 @@ export function startBrowserTracingPageLoadSpan(
453455
*/
454456
export function startBrowserTracingNavigationSpan(client: Client, spanOptions: StartSpanOptions): Span | undefined {
455457
getIsolationScope().setPropagationContext({ traceId: generateTraceId() });
456-
getCurrentScope().setPropagationContext({ traceId: generateTraceId() });
457458

458459
client.emit('startNavigationSpan', spanOptions);
459-
460460
getCurrentScope().setTransactionName(spanOptions.name);
461461

462462
return getActiveIdleSpan(client);

‎packages/browser/test/tracing/browserTracingIntegration.test.ts‎

Lines changed: 76 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -20,15 +20,18 @@ import {
2020
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
2121
SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
2222
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
23+
SentryNonRecordingSpan,
2324
TRACING_DEFAULTS,
2425
getActiveSpan,
2526
getCurrentScope,
2627
getDynamicSamplingContextFromSpan,
2728
getIsolationScope,
29+
getTraceData,
2830
setCurrentClient,
2931
spanIsSampled,
3032
spanToJSON,
3133
startInactiveSpan,
34+
updateSpanName,
3235
} from '@sentry/core';
3336
import type { Span, StartSpanOptions } from '@sentry/core';
3437
import { JSDOM } from 'jsdom';
@@ -265,6 +268,10 @@ describe('browserTracingIntegration', () => {
265268

266269
expect(span).toBeDefined();
267270
expect(spanIsSampled(span!)).toBe(false);
271+
272+
// Ensure getTraceData is correct in this case
273+
const traceData = getTraceData();
274+
expect(traceData['sentry-trace']).toEqual(`${span?.spanContext().traceId}-${span?.spanContext().spanId}-0`);
268275
});
269276

270277
it('works with integration setup', () => {
@@ -365,7 +372,7 @@ describe('browserTracingIntegration', () => {
365372

366373
const client = new BrowserClient(
367374
getDefaultBrowserClientOptions({
368-
tracesSampleRate: 0,
375+
tracesSampleRate: 1,
369376
integrations: [
370377
browserTracingIntegration({
371378
instrumentPageLoad: false,
@@ -378,9 +385,7 @@ describe('browserTracingIntegration', () => {
378385
setCurrentClient(client);
379386
client.init();
380387

381-
startBrowserTracingPageLoadSpan(client, { name: 'test span' });
382-
383-
const pageloadSpan = getActiveSpan();
388+
const pageloadSpan = startBrowserTracingPageLoadSpan(client, { name: 'test span' });
384389

385390
expect(spanToJSON(pageloadSpan!).op).toBe('test op');
386391
});
@@ -408,7 +413,7 @@ describe('browserTracingIntegration', () => {
408413

409414
const client = new BrowserClient(
410415
getDefaultBrowserClientOptions({
411-
tracesSampleRate: 0,
416+
tracesSampleRate: 1,
412417
integrations: [
413418
browserTracingIntegration({
414419
instrumentPageLoad: false,
@@ -458,6 +463,10 @@ describe('browserTracingIntegration', () => {
458463

459464
expect(span).toBeDefined();
460465
expect(spanIsSampled(span!)).toBe(false);
466+
467+
// Ensure getTraceData is correct in this case
468+
const traceData = getTraceData();
469+
expect(traceData['sentry-trace']).toEqual(`${span?.spanContext().traceId}-${span?.spanContext().spanId}-0`);
461470
});
462471

463472
it('works with integration setup', () => {
@@ -562,7 +571,7 @@ describe('browserTracingIntegration', () => {
562571

563572
const client = new BrowserClient(
564573
getDefaultBrowserClientOptions({
565-
tracesSampleRate: 0,
574+
tracesSampleRate: 1,
566575
integrations: [
567576
browserTracingIntegration({
568577
instrumentPageLoad: false,
@@ -575,9 +584,7 @@ describe('browserTracingIntegration', () => {
575584
setCurrentClient(client);
576585
client.init();
577586

578-
startBrowserTracingNavigationSpan(client, { name: 'test span' });
579-
580-
const navigationSpan = getActiveSpan();
587+
const navigationSpan = startBrowserTracingNavigationSpan(client, { name: 'test span' });
581588

582589
expect(spanToJSON(navigationSpan!).op).toBe('test op');
583590
});
@@ -590,7 +597,7 @@ describe('browserTracingIntegration', () => {
590597

591598
const client = new BrowserClient(
592599
getDefaultBrowserClientOptions({
593-
tracesSampleRate: 0,
600+
tracesSampleRate: 1,
594601
integrations: [
595602
browserTracingIntegration({
596603
instrumentPageLoad: false,
@@ -628,7 +635,7 @@ describe('browserTracingIntegration', () => {
628635
it("updates the scopes' propagationContexts on a navigation", () => {
629636
const client = new BrowserClient(
630637
getDefaultBrowserClientOptions({
631-
integrations: [browserTracingIntegration()],
638+
integrations: [browserTracingIntegration({ instrumentPageLoad: false })],
632639
}),
633640
);
634641
setCurrentClient(client);
@@ -637,7 +644,8 @@ describe('browserTracingIntegration', () => {
637644
const oldIsolationScopePropCtx = getIsolationScope().getPropagationContext();
638645
const oldCurrentScopePropCtx = getCurrentScope().getPropagationContext();
639646

640-
startBrowserTracingNavigationSpan(client, { name: 'test navigation span' });
647+
const span = startBrowserTracingNavigationSpan(client, { name: 'test navigation span' });
648+
const traceId = span!.spanContext().traceId;
641649

642650
const newIsolationScopePropCtx = getIsolationScope().getPropagationContext();
643651
const newCurrentScopePropCtx = getCurrentScope().getPropagationContext();
@@ -649,14 +657,37 @@ describe('browserTracingIntegration', () => {
649657
traceId: expect.stringMatching(/[a-f0-9]{32}/),
650658
});
651659
expect(newCurrentScopePropCtx).toEqual({
652-
traceId: expect.stringMatching(/[a-f0-9]{32}/),
660+
traceId,
661+
sampled: false,
662+
dsc: {
663+
environment: 'production',
664+
public_key: 'examplePublicKey',
665+
sample_rate: '0',
666+
trace_id: traceId,
667+
},
653668
});
654669
expect(newIsolationScopePropCtx).toEqual({
655670
traceId: expect.stringMatching(/[a-f0-9]{32}/),
656671
});
657672

658673
expect(newIsolationScopePropCtx.traceId).not.toEqual(oldIsolationScopePropCtx.traceId);
659674
expect(newCurrentScopePropCtx.traceId).not.toEqual(oldCurrentScopePropCtx.traceId);
675+
676+
const span2 = startBrowserTracingNavigationSpan(client, { name: 'test navigation span 2' });
677+
const traceId2 = span2!.spanContext().traceId;
678+
expect(traceId2).not.toEqual(traceId);
679+
680+
const newCurrentScopePropCtx2 = getCurrentScope().getPropagationContext();
681+
expect(newCurrentScopePropCtx2).toEqual({
682+
traceId: traceId2,
683+
sampled: false,
684+
dsc: {
685+
environment: 'production',
686+
public_key: 'examplePublicKey',
687+
sample_rate: '0',
688+
trace_id: traceId2,
689+
},
690+
});
660691
});
661692

662693
it("saves the span's positive sampling decision and its DSC on the propagationContext when the span finishes", () => {
@@ -677,8 +708,18 @@ describe('browserTracingIntegration', () => {
677708
const propCtxBeforeEnd = getCurrentScope().getPropagationContext();
678709
expect(propCtxBeforeEnd).toStrictEqual({
679710
traceId: expect.stringMatching(/[a-f0-9]{32}/),
711+
sampled: true,
712+
dsc: {
713+
environment: 'production',
714+
public_key: 'examplePublicKey',
715+
sample_rate: '1',
716+
sampled: 'true',
717+
transaction: 'mySpan',
718+
trace_id: propCtxBeforeEnd.traceId,
719+
},
680720
});
681721

722+
updateSpanName(navigationSpan!, 'mySpan2');
682723
navigationSpan!.end();
683724

684725
const propCtxAfterEnd = getCurrentScope().getPropagationContext();
@@ -690,7 +731,7 @@ describe('browserTracingIntegration', () => {
690731
public_key: 'examplePublicKey',
691732
sample_rate: '1',
692733
sampled: 'true',
693-
transaction: 'mySpan',
734+
transaction: 'mySpan2',
694735
trace_id: propCtxBeforeEnd.traceId,
695736
},
696737
});
@@ -714,6 +755,15 @@ describe('browserTracingIntegration', () => {
714755
const propCtxBeforeEnd = getCurrentScope().getPropagationContext();
715756
expect(propCtxBeforeEnd).toStrictEqual({
716757
traceId: expect.stringMatching(/[a-f0-9]{32}/),
758+
sampled: false,
759+
dsc: {
760+
environment: 'production',
761+
public_key: 'examplePublicKey',
762+
sample_rate: '0',
763+
sampled: 'false',
764+
transaction: 'mySpan',
765+
trace_id: propCtxBeforeEnd.traceId,
766+
},
717767
});
718768

719769
navigationSpan!.end();
@@ -758,18 +808,19 @@ describe('browserTracingIntegration', () => {
758808
const dynamicSamplingContext = getDynamicSamplingContextFromSpan(idleSpan!);
759809
const propagationContext = getCurrentScope().getPropagationContext();
760810

761-
// Span is correct
762-
expect(spanToJSON(idleSpan).op).toBe('pageload');
763-
expect(spanToJSON(idleSpan).trace_id).toEqual('12312012123120121231201212312012');
764-
expect(spanToJSON(idleSpan).parent_span_id).toEqual('1121201211212012');
765-
expect(spanIsSampled(idleSpan)).toBe(false);
811+
// Span is non-recording
812+
expect(idleSpan instanceof SentryNonRecordingSpan).toBe(true);
766813

767814
expect(dynamicSamplingContext).toBeDefined();
768815
expect(dynamicSamplingContext).toStrictEqual({ release: '2.1.14' });
769816

770817
// Propagation context keeps the meta tag trace data for later events on the same route to add them to the trace
771818
expect(propagationContext.traceId).toEqual('12312012123120121231201212312012');
772819
expect(propagationContext.parentSpanId).toEqual('1121201211212012');
820+
821+
// Ensure getTraceData is correct in this case
822+
const traceData = getTraceData();
823+
expect(traceData['sentry-trace']).toMatch(/12312012123120121231201212312012-[a-f0-9]{16}-0/);
773824
});
774825

775826
it('puts frozen Dynamic Sampling Context on pageload span if sentry-trace data and only 3rd party baggage is present', () => {
@@ -795,18 +846,19 @@ describe('browserTracingIntegration', () => {
795846
const dynamicSamplingContext = getDynamicSamplingContextFromSpan(idleSpan);
796847
const propagationContext = getCurrentScope().getPropagationContext();
797848

798-
// Span is correct
799-
expect(spanToJSON(idleSpan).op).toBe('pageload');
800-
expect(spanToJSON(idleSpan).trace_id).toEqual('12312012123120121231201212312012');
801-
expect(spanToJSON(idleSpan).parent_span_id).toEqual('1121201211212012');
802-
expect(spanIsSampled(idleSpan)).toBe(false);
849+
// Span is NonRecordingSpan
850+
expect(idleSpan instanceof SentryNonRecordingSpan).toBe(true);
803851

804852
expect(dynamicSamplingContext).toBeDefined();
805853
expect(dynamicSamplingContext).toStrictEqual({});
806854

807855
// Propagation context keeps the meta tag trace data for later events on the same route to add them to the trace
808856
expect(propagationContext.traceId).toEqual('12312012123120121231201212312012');
809857
expect(propagationContext.parentSpanId).toEqual('1121201211212012');
858+
859+
// Ensure getTraceData is correct in this case
860+
const traceData = getTraceData();
861+
expect(traceData['sentry-trace']).toMatch(/12312012123120121231201212312012-[a-f0-9]{16}-0/);
810862
});
811863

812864
it('ignores the meta tag data for navigation spans', () => {

‎packages/core/src/tracing/trace.ts‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -391,7 +391,11 @@ function getAcs(): AsyncContextStrategy {
391391
return getAsyncContextStrategy(carrier);
392392
}
393393

394-
function _startRootSpan(spanArguments: SentrySpanArguments, scope: Scope, parentSampled?: boolean): SentrySpan {
394+
function _startRootSpan(
395+
spanArguments: SentrySpanArguments,
396+
scope: Scope,
397+
parentSampled?: boolean,
398+
): SentrySpan | SentryNonRecordingSpan {
395399
const client = getClient();
396400
const options: Partial<ClientOptions> = client?.getOptions() || {};
397401

@@ -408,14 +412,25 @@ function _startRootSpan(spanArguments: SentrySpanArguments, scope: Scope, parent
408412
},
409413
});
410414

411-
const rootSpan = new SentrySpan({
412-
...spanArguments,
413-
attributes: {
414-
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom',
415-
...spanArguments.attributes,
416-
},
417-
sampled,
418-
});
415+
const rootSpan = sampled
416+
? new SentrySpan({
417+
...spanArguments,
418+
attributes: {
419+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom',
420+
...spanArguments.attributes,
421+
},
422+
sampled,
423+
})
424+
: new SentryNonRecordingSpan({ traceId: spanArguments.traceId });
425+
426+
if (!sampled) {
427+
const dsc = {
428+
sample_rate: sampleRate !== undefined ? `${sampleRate}` : undefined,
429+
transaction: name,
430+
...getDynamicSamplingContextFromSpan(rootSpan),
431+
} satisfies Partial<DynamicSamplingContext>;
432+
freezeDscOnSpan(rootSpan, dsc);
433+
}
419434

420435
if (!sampled && client) {
421436
DEBUG_BUILD && logger.log('[Tracing] Discarding root span because its trace was not chosen to be sampled.');

0 commit comments

Comments
 (0)