Skip to content

ref(core): Ensure non-sampled spans are NonRecordingSpans #14955

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

Closed
wants to merge 7 commits into from
Closed

ref(core): Ensure non-sampled spans are NonRecordingSpans #14955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d25345e
ref(core): Ensure non-sampled spans are NonRecordingSpans
mydea Jan 9, 2025
85b92cd
revert tests
mydea Jan 10, 2025
afc1878
do not set on span start??
mydea Jan 10, 2025
67c175e
WIP changed tests
mydea Jan 10, 2025
a332237
fixes
mydea Jan 10, 2025
373ff2c
fix linting
mydea Jan 10, 2025
6bac717
fix it
mydea Jan 10, 2025
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
52 changes: 29 additions & 23 deletions packages/browser/src/tracing/browserTracingIntegration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
startTrackingWebVitals,
} from '@sentry-internal/browser-utils';
import type { Client, IntegrationFn, Span, StartSpanOptions, TransactionSource } from '@sentry/core';
import { dropUndefinedKeys } from '@sentry/core';
import {
GLOBAL_OBJ,
SEMANTIC_ATTRIBUTE_SENTRY_IDLE_SPAN_FINISH_REASON,
Expand All @@ -24,7 +25,6 @@ import {
getDynamicSamplingContextFromSpan,
getIsolationScope,
getLocationHref,
getRootSpan,
logger,
propagationContextFromHeaders,
registerSpanErrorInstrumentation,
Expand Down Expand Up @@ -276,6 +276,17 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
_collectWebVitals();
addPerformanceEntries(span, { recordClsOnPageloadSpan: !enableStandaloneClsSpans });
setActiveIdleSpan(client, undefined);

// Ensure that DSC is updated with possibly final transaction etc.
const scope = getCurrentScope();
const oldPropagationContext = scope.getPropagationContext();

scope.setPropagationContext({
...oldPropagationContext,
traceId: idleSpan.spanContext().traceId,
sampled: spanIsSampled(idleSpan),
dsc: getDynamicSamplingContextFromSpan(span),
});
},
});
setActiveIdleSpan(client, idleSpan);
Expand All @@ -293,6 +304,22 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio

emitFinish();
}

// A trace should to stay the consistent over the entire time span of one route.
// Therefore, we update the traceId/sampled properties on the propagation context.
// The DSC is only set once the span has ended
const scope = getCurrentScope();
const oldPropagationContext = scope.getPropagationContext();

scope.setPropagationContext(
dropUndefinedKeys({
...oldPropagationContext,
traceId: idleSpan.spanContext().traceId,
sampled: spanIsSampled(idleSpan),
// Reset the DSC, it should be read from the span while it is active
dsc: undefined,
}),
);
}

return {
Expand Down Expand Up @@ -341,27 +368,6 @@ 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 root span 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 root span 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();

scope.setPropagationContext({
...oldPropagationContext,
sampled: oldPropagationContext.sampled !== undefined ? oldPropagationContext.sampled : spanIsSampled(span),
dsc: oldPropagationContext.dsc || getDynamicSamplingContextFromSpan(span),
});
});

if (WINDOW.location) {
if (instrumentPageLoad) {
startBrowserTracingPageLoadSpan(client, {
Expand Down Expand Up @@ -452,11 +458,11 @@ export function startBrowserTracingPageLoadSpan(
* This will only do something if a browser tracing integration has been setup.
*/
export function startBrowserTracingNavigationSpan(client: Client, spanOptions: StartSpanOptions): Span | undefined {
// Reset this to ensure we start a new trace, instead of continuing the last pageload/navigation trace
getIsolationScope().setPropagationContext({ traceId: generateTraceId() });
getCurrentScope().setPropagationContext({ traceId: generateTraceId() });

client.emit('startNavigationSpan', spanOptions);

getCurrentScope().setTransactionName(spanOptions.name);

return getActiveIdleSpan(client);
Expand Down
85 changes: 62 additions & 23 deletions packages/browser/test/tracing/browserTracingIntegration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,18 @@ import {
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
SentryNonRecordingSpan,
TRACING_DEFAULTS,
getActiveSpan,
getCurrentScope,
getDynamicSamplingContextFromSpan,
getIsolationScope,
getTraceData,
setCurrentClient,
spanIsSampled,
spanToJSON,
startInactiveSpan,
updateSpanName,
} from '@sentry/core';
import type { Span, StartSpanOptions } from '@sentry/core';
import { JSDOM } from 'jsdom';
Expand Down Expand Up @@ -265,6 +268,10 @@ describe('browserTracingIntegration', () => {

expect(span).toBeDefined();
expect(spanIsSampled(span!)).toBe(false);

// Ensure getTraceData is correct in this case
const traceData = getTraceData();
expect(traceData['sentry-trace']).toEqual(`${span?.spanContext().traceId}-${span?.spanContext().spanId}-0`);
});

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

const client = new BrowserClient(
getDefaultBrowserClientOptions({
tracesSampleRate: 0,
tracesSampleRate: 1,
integrations: [
browserTracingIntegration({
instrumentPageLoad: false,
Expand All @@ -378,9 +385,7 @@ describe('browserTracingIntegration', () => {
setCurrentClient(client);
client.init();

startBrowserTracingPageLoadSpan(client, { name: 'test span' });

const pageloadSpan = getActiveSpan();
const pageloadSpan = startBrowserTracingPageLoadSpan(client, { name: 'test span' });

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

const client = new BrowserClient(
getDefaultBrowserClientOptions({
tracesSampleRate: 0,
tracesSampleRate: 1,
integrations: [
browserTracingIntegration({
instrumentPageLoad: false,
Expand Down Expand Up @@ -458,6 +463,10 @@ describe('browserTracingIntegration', () => {

expect(span).toBeDefined();
expect(spanIsSampled(span!)).toBe(false);

// Ensure getTraceData is correct in this case
const traceData = getTraceData();
expect(traceData['sentry-trace']).toEqual(`${span?.spanContext().traceId}-${span?.spanContext().spanId}-0`);
});

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

const client = new BrowserClient(
getDefaultBrowserClientOptions({
tracesSampleRate: 0,
tracesSampleRate: 1,
integrations: [
browserTracingIntegration({
instrumentPageLoad: false,
Expand All @@ -575,9 +584,7 @@ describe('browserTracingIntegration', () => {
setCurrentClient(client);
client.init();

startBrowserTracingNavigationSpan(client, { name: 'test span' });

const navigationSpan = getActiveSpan();
const navigationSpan = startBrowserTracingNavigationSpan(client, { name: 'test span' });

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

const client = new BrowserClient(
getDefaultBrowserClientOptions({
tracesSampleRate: 0,
tracesSampleRate: 1,
integrations: [
browserTracingIntegration({
instrumentPageLoad: false,
Expand Down Expand Up @@ -628,7 +635,7 @@ describe('browserTracingIntegration', () => {
it("updates the scopes' propagationContexts on a navigation", () => {
const client = new BrowserClient(
getDefaultBrowserClientOptions({
integrations: [browserTracingIntegration()],
integrations: [browserTracingIntegration({ instrumentPageLoad: false })],
}),
);
setCurrentClient(client);
Expand All @@ -637,7 +644,8 @@ describe('browserTracingIntegration', () => {
const oldIsolationScopePropCtx = getIsolationScope().getPropagationContext();
const oldCurrentScopePropCtx = getCurrentScope().getPropagationContext();

startBrowserTracingNavigationSpan(client, { name: 'test navigation span' });
const span = startBrowserTracingNavigationSpan(client, { name: 'test navigation span' });
const traceId = span!.spanContext().traceId;

const newIsolationScopePropCtx = getIsolationScope().getPropagationContext();
const newCurrentScopePropCtx = getCurrentScope().getPropagationContext();
Expand All @@ -650,13 +658,39 @@ describe('browserTracingIntegration', () => {
});
expect(newCurrentScopePropCtx).toEqual({
traceId: expect.stringMatching(/[a-f0-9]{32}/),
sampled: false,
});
expect(newIsolationScopePropCtx).toEqual({
traceId: expect.stringMatching(/[a-f0-9]{32}/),
});

expect(newIsolationScopePropCtx.traceId).not.toEqual(oldIsolationScopePropCtx.traceId);
expect(newCurrentScopePropCtx.traceId).not.toEqual(oldCurrentScopePropCtx.traceId);

const span2 = startBrowserTracingNavigationSpan(client, { name: 'test navigation span 2' });
const traceId2 = span2!.spanContext().traceId;
expect(traceId2).not.toEqual(traceId);

const newCurrentScopePropCtx2 = getCurrentScope().getPropagationContext();
expect(newCurrentScopePropCtx2).toEqual({
traceId: traceId2,
sampled: false,
});

span2?.end();

const newCurrentScopePropCtx2After = getCurrentScope().getPropagationContext();
expect(newCurrentScopePropCtx2After).toEqual({
traceId: traceId2,
sampled: false,
dsc: {
environment: 'production',
public_key: 'examplePublicKey',
sample_rate: '0',
sampled: 'false',
trace_id: traceId2,
},
});
});

it("saves the span's positive sampling decision and its DSC on the propagationContext when the span finishes", () => {
Expand All @@ -677,8 +711,10 @@ describe('browserTracingIntegration', () => {
const propCtxBeforeEnd = getCurrentScope().getPropagationContext();
expect(propCtxBeforeEnd).toStrictEqual({
traceId: expect.stringMatching(/[a-f0-9]{32}/),
sampled: true,
});

updateSpanName(navigationSpan!, 'mySpan2');
navigationSpan!.end();

const propCtxAfterEnd = getCurrentScope().getPropagationContext();
Expand All @@ -690,7 +726,7 @@ describe('browserTracingIntegration', () => {
public_key: 'examplePublicKey',
sample_rate: '1',
sampled: 'true',
transaction: 'mySpan',
transaction: 'mySpan2',
trace_id: propCtxBeforeEnd.traceId,
},
});
Expand All @@ -714,6 +750,7 @@ describe('browserTracingIntegration', () => {
const propCtxBeforeEnd = getCurrentScope().getPropagationContext();
expect(propCtxBeforeEnd).toStrictEqual({
traceId: expect.stringMatching(/[a-f0-9]{32}/),
sampled: false,
});

navigationSpan!.end();
Expand Down Expand Up @@ -758,18 +795,19 @@ describe('browserTracingIntegration', () => {
const dynamicSamplingContext = getDynamicSamplingContextFromSpan(idleSpan!);
const propagationContext = getCurrentScope().getPropagationContext();

// Span is correct
expect(spanToJSON(idleSpan).op).toBe('pageload');
expect(spanToJSON(idleSpan).trace_id).toEqual('12312012123120121231201212312012');
expect(spanToJSON(idleSpan).parent_span_id).toEqual('1121201211212012');
expect(spanIsSampled(idleSpan)).toBe(false);
// Span is non-recording
expect(idleSpan instanceof SentryNonRecordingSpan).toBe(true);

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

// Propagation context keeps the meta tag trace data for later events on the same route to add them to the trace
expect(propagationContext.traceId).toEqual('12312012123120121231201212312012');
expect(propagationContext.parentSpanId).toEqual('1121201211212012');

// Ensure getTraceData is correct in this case
const traceData = getTraceData();
expect(traceData['sentry-trace']).toMatch(/12312012123120121231201212312012-[a-f0-9]{16}-0/);
});

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

// Span is correct
expect(spanToJSON(idleSpan).op).toBe('pageload');
expect(spanToJSON(idleSpan).trace_id).toEqual('12312012123120121231201212312012');
expect(spanToJSON(idleSpan).parent_span_id).toEqual('1121201211212012');
expect(spanIsSampled(idleSpan)).toBe(false);
// Span is NonRecordingSpan
expect(idleSpan instanceof SentryNonRecordingSpan).toBe(true);

expect(dynamicSamplingContext).toBeDefined();
expect(dynamicSamplingContext).toStrictEqual({});

// Propagation context keeps the meta tag trace data for later events on the same route to add them to the trace
expect(propagationContext.traceId).toEqual('12312012123120121231201212312012');
expect(propagationContext.parentSpanId).toEqual('1121201211212012');

// Ensure getTraceData is correct in this case
const traceData = getTraceData();
expect(traceData['sentry-trace']).toMatch(/12312012123120121231201212312012-[a-f0-9]{16}-0/);
});

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