From d25345e39c18a344b1b8c0a44c4a11ea4cfaaa0f Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 9 Jan 2025 15:11:21 +0100 Subject: [PATCH 1/7] ref(core): Ensure non-sampled spans are NonRecordingSpans --- .../src/tracing/browserTracingIntegration.ts | 48 ++++----- .../tracing/browserTracingIntegration.test.ts | 100 +++++++++++++----- packages/core/src/tracing/trace.ts | 33 ++++-- 3 files changed, 124 insertions(+), 57 deletions(-) diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index f4ab605be9c2..2e761d263424 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -24,7 +24,6 @@ import { getDynamicSamplingContextFromSpan, getIsolationScope, getLocationHref, - getRootSpan, logger, propagationContextFromHeaders, registerSpanErrorInstrumentation, @@ -276,6 +275,15 @@ export const browserTracingIntegration = ((_options: Partial { - 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, { @@ -453,10 +455,8 @@ export function startBrowserTracingPageLoadSpan( */ export function startBrowserTracingNavigationSpan(client: Client, spanOptions: StartSpanOptions): Span | undefined { getIsolationScope().setPropagationContext({ traceId: generateTraceId() }); - getCurrentScope().setPropagationContext({ traceId: generateTraceId() }); client.emit('startNavigationSpan', spanOptions); - getCurrentScope().setTransactionName(spanOptions.name); return getActiveIdleSpan(client); diff --git a/packages/browser/test/tracing/browserTracingIntegration.test.ts b/packages/browser/test/tracing/browserTracingIntegration.test.ts index 3e9d2354a532..11495eae8ae1 100644 --- a/packages/browser/test/tracing/browserTracingIntegration.test.ts +++ b/packages/browser/test/tracing/browserTracingIntegration.test.ts @@ -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'; @@ -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', () => { @@ -365,7 +372,7 @@ describe('browserTracingIntegration', () => { const client = new BrowserClient( getDefaultBrowserClientOptions({ - tracesSampleRate: 0, + tracesSampleRate: 1, integrations: [ browserTracingIntegration({ instrumentPageLoad: false, @@ -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'); }); @@ -408,7 +413,7 @@ describe('browserTracingIntegration', () => { const client = new BrowserClient( getDefaultBrowserClientOptions({ - tracesSampleRate: 0, + tracesSampleRate: 1, integrations: [ browserTracingIntegration({ instrumentPageLoad: false, @@ -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', () => { @@ -562,7 +571,7 @@ describe('browserTracingIntegration', () => { const client = new BrowserClient( getDefaultBrowserClientOptions({ - tracesSampleRate: 0, + tracesSampleRate: 1, integrations: [ browserTracingIntegration({ instrumentPageLoad: false, @@ -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'); }); @@ -590,7 +597,7 @@ describe('browserTracingIntegration', () => { const client = new BrowserClient( getDefaultBrowserClientOptions({ - tracesSampleRate: 0, + tracesSampleRate: 1, integrations: [ browserTracingIntegration({ instrumentPageLoad: false, @@ -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); @@ -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(); @@ -649,7 +657,14 @@ describe('browserTracingIntegration', () => { traceId: expect.stringMatching(/[a-f0-9]{32}/), }); expect(newCurrentScopePropCtx).toEqual({ - traceId: expect.stringMatching(/[a-f0-9]{32}/), + traceId, + sampled: false, + dsc: { + environment: 'production', + public_key: 'examplePublicKey', + sample_rate: '0', + trace_id: traceId, + }, }); expect(newIsolationScopePropCtx).toEqual({ traceId: expect.stringMatching(/[a-f0-9]{32}/), @@ -657,6 +672,22 @@ describe('browserTracingIntegration', () => { 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, + dsc: { + environment: 'production', + public_key: 'examplePublicKey', + sample_rate: '0', + trace_id: traceId2, + }, + }); }); it("saves the span's positive sampling decision and its DSC on the propagationContext when the span finishes", () => { @@ -677,8 +708,18 @@ describe('browserTracingIntegration', () => { const propCtxBeforeEnd = getCurrentScope().getPropagationContext(); expect(propCtxBeforeEnd).toStrictEqual({ traceId: expect.stringMatching(/[a-f0-9]{32}/), + sampled: true, + dsc: { + environment: 'production', + public_key: 'examplePublicKey', + sample_rate: '1', + sampled: 'true', + transaction: 'mySpan', + trace_id: propCtxBeforeEnd.traceId, + }, }); + updateSpanName(navigationSpan!, 'mySpan2'); navigationSpan!.end(); const propCtxAfterEnd = getCurrentScope().getPropagationContext(); @@ -690,7 +731,7 @@ describe('browserTracingIntegration', () => { public_key: 'examplePublicKey', sample_rate: '1', sampled: 'true', - transaction: 'mySpan', + transaction: 'mySpan2', trace_id: propCtxBeforeEnd.traceId, }, }); @@ -714,6 +755,15 @@ describe('browserTracingIntegration', () => { const propCtxBeforeEnd = getCurrentScope().getPropagationContext(); expect(propCtxBeforeEnd).toStrictEqual({ traceId: expect.stringMatching(/[a-f0-9]{32}/), + sampled: false, + dsc: { + environment: 'production', + public_key: 'examplePublicKey', + sample_rate: '0', + sampled: 'false', + transaction: 'mySpan', + trace_id: propCtxBeforeEnd.traceId, + }, }); navigationSpan!.end(); @@ -758,11 +808,8 @@ 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' }); @@ -770,6 +817,10 @@ describe('browserTracingIntegration', () => { // 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', () => { @@ -795,11 +846,8 @@ 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({}); @@ -807,6 +855,10 @@ describe('browserTracingIntegration', () => { // 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', () => { diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 28b9654d5266..d75c15ee1b77 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -391,7 +391,11 @@ function getAcs(): AsyncContextStrategy { return getAsyncContextStrategy(carrier); } -function _startRootSpan(spanArguments: SentrySpanArguments, scope: Scope, parentSampled?: boolean): SentrySpan { +function _startRootSpan( + spanArguments: SentrySpanArguments, + scope: Scope, + parentSampled?: boolean, +): SentrySpan | SentryNonRecordingSpan { const client = getClient(); const options: Partial = client?.getOptions() || {}; @@ -408,14 +412,25 @@ function _startRootSpan(spanArguments: SentrySpanArguments, scope: Scope, parent }, }); - const rootSpan = new SentrySpan({ - ...spanArguments, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', - ...spanArguments.attributes, - }, - sampled, - }); + const rootSpan = sampled + ? new SentrySpan({ + ...spanArguments, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom', + ...spanArguments.attributes, + }, + sampled, + }) + : new SentryNonRecordingSpan({ traceId: spanArguments.traceId }); + + if (!sampled) { + const dsc = { + sample_rate: sampleRate !== undefined ? `${sampleRate}` : undefined, + transaction: name, + ...getDynamicSamplingContextFromSpan(rootSpan), + } satisfies Partial; + freezeDscOnSpan(rootSpan, dsc); + } if (!sampled && client) { DEBUG_BUILD && logger.log('[Tracing] Discarding root span because its trace was not chosen to be sampled.'); From 85b92cde55a69119017f94e4f318d7447d1f0b48 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 10 Jan 2025 10:52:29 +0100 Subject: [PATCH 2/7] revert tests --- .../tracing/browserTracingIntegration.test.ts | 100 +++++------------- 1 file changed, 24 insertions(+), 76 deletions(-) diff --git a/packages/browser/test/tracing/browserTracingIntegration.test.ts b/packages/browser/test/tracing/browserTracingIntegration.test.ts index 11495eae8ae1..3e9d2354a532 100644 --- a/packages/browser/test/tracing/browserTracingIntegration.test.ts +++ b/packages/browser/test/tracing/browserTracingIntegration.test.ts @@ -20,18 +20,15 @@ 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'; @@ -268,10 +265,6 @@ 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', () => { @@ -372,7 +365,7 @@ describe('browserTracingIntegration', () => { const client = new BrowserClient( getDefaultBrowserClientOptions({ - tracesSampleRate: 1, + tracesSampleRate: 0, integrations: [ browserTracingIntegration({ instrumentPageLoad: false, @@ -385,7 +378,9 @@ describe('browserTracingIntegration', () => { setCurrentClient(client); client.init(); - const pageloadSpan = startBrowserTracingPageLoadSpan(client, { name: 'test span' }); + startBrowserTracingPageLoadSpan(client, { name: 'test span' }); + + const pageloadSpan = getActiveSpan(); expect(spanToJSON(pageloadSpan!).op).toBe('test op'); }); @@ -413,7 +408,7 @@ describe('browserTracingIntegration', () => { const client = new BrowserClient( getDefaultBrowserClientOptions({ - tracesSampleRate: 1, + tracesSampleRate: 0, integrations: [ browserTracingIntegration({ instrumentPageLoad: false, @@ -463,10 +458,6 @@ 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', () => { @@ -571,7 +562,7 @@ describe('browserTracingIntegration', () => { const client = new BrowserClient( getDefaultBrowserClientOptions({ - tracesSampleRate: 1, + tracesSampleRate: 0, integrations: [ browserTracingIntegration({ instrumentPageLoad: false, @@ -584,7 +575,9 @@ describe('browserTracingIntegration', () => { setCurrentClient(client); client.init(); - const navigationSpan = startBrowserTracingNavigationSpan(client, { name: 'test span' }); + startBrowserTracingNavigationSpan(client, { name: 'test span' }); + + const navigationSpan = getActiveSpan(); expect(spanToJSON(navigationSpan!).op).toBe('test op'); }); @@ -597,7 +590,7 @@ describe('browserTracingIntegration', () => { const client = new BrowserClient( getDefaultBrowserClientOptions({ - tracesSampleRate: 1, + tracesSampleRate: 0, integrations: [ browserTracingIntegration({ instrumentPageLoad: false, @@ -635,7 +628,7 @@ describe('browserTracingIntegration', () => { it("updates the scopes' propagationContexts on a navigation", () => { const client = new BrowserClient( getDefaultBrowserClientOptions({ - integrations: [browserTracingIntegration({ instrumentPageLoad: false })], + integrations: [browserTracingIntegration()], }), ); setCurrentClient(client); @@ -644,8 +637,7 @@ describe('browserTracingIntegration', () => { const oldIsolationScopePropCtx = getIsolationScope().getPropagationContext(); const oldCurrentScopePropCtx = getCurrentScope().getPropagationContext(); - const span = startBrowserTracingNavigationSpan(client, { name: 'test navigation span' }); - const traceId = span!.spanContext().traceId; + startBrowserTracingNavigationSpan(client, { name: 'test navigation span' }); const newIsolationScopePropCtx = getIsolationScope().getPropagationContext(); const newCurrentScopePropCtx = getCurrentScope().getPropagationContext(); @@ -657,14 +649,7 @@ describe('browserTracingIntegration', () => { traceId: expect.stringMatching(/[a-f0-9]{32}/), }); expect(newCurrentScopePropCtx).toEqual({ - traceId, - sampled: false, - dsc: { - environment: 'production', - public_key: 'examplePublicKey', - sample_rate: '0', - trace_id: traceId, - }, + traceId: expect.stringMatching(/[a-f0-9]{32}/), }); expect(newIsolationScopePropCtx).toEqual({ traceId: expect.stringMatching(/[a-f0-9]{32}/), @@ -672,22 +657,6 @@ describe('browserTracingIntegration', () => { 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, - dsc: { - environment: 'production', - public_key: 'examplePublicKey', - sample_rate: '0', - trace_id: traceId2, - }, - }); }); it("saves the span's positive sampling decision and its DSC on the propagationContext when the span finishes", () => { @@ -708,18 +677,8 @@ describe('browserTracingIntegration', () => { const propCtxBeforeEnd = getCurrentScope().getPropagationContext(); expect(propCtxBeforeEnd).toStrictEqual({ traceId: expect.stringMatching(/[a-f0-9]{32}/), - sampled: true, - dsc: { - environment: 'production', - public_key: 'examplePublicKey', - sample_rate: '1', - sampled: 'true', - transaction: 'mySpan', - trace_id: propCtxBeforeEnd.traceId, - }, }); - updateSpanName(navigationSpan!, 'mySpan2'); navigationSpan!.end(); const propCtxAfterEnd = getCurrentScope().getPropagationContext(); @@ -731,7 +690,7 @@ describe('browserTracingIntegration', () => { public_key: 'examplePublicKey', sample_rate: '1', sampled: 'true', - transaction: 'mySpan2', + transaction: 'mySpan', trace_id: propCtxBeforeEnd.traceId, }, }); @@ -755,15 +714,6 @@ describe('browserTracingIntegration', () => { const propCtxBeforeEnd = getCurrentScope().getPropagationContext(); expect(propCtxBeforeEnd).toStrictEqual({ traceId: expect.stringMatching(/[a-f0-9]{32}/), - sampled: false, - dsc: { - environment: 'production', - public_key: 'examplePublicKey', - sample_rate: '0', - sampled: 'false', - transaction: 'mySpan', - trace_id: propCtxBeforeEnd.traceId, - }, }); navigationSpan!.end(); @@ -808,8 +758,11 @@ describe('browserTracingIntegration', () => { const dynamicSamplingContext = getDynamicSamplingContextFromSpan(idleSpan!); const propagationContext = getCurrentScope().getPropagationContext(); - // Span is non-recording - expect(idleSpan instanceof SentryNonRecordingSpan).toBe(true); + // 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); expect(dynamicSamplingContext).toBeDefined(); expect(dynamicSamplingContext).toStrictEqual({ release: '2.1.14' }); @@ -817,10 +770,6 @@ describe('browserTracingIntegration', () => { // 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', () => { @@ -846,8 +795,11 @@ describe('browserTracingIntegration', () => { const dynamicSamplingContext = getDynamicSamplingContextFromSpan(idleSpan); const propagationContext = getCurrentScope().getPropagationContext(); - // Span is NonRecordingSpan - expect(idleSpan instanceof SentryNonRecordingSpan).toBe(true); + // 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); expect(dynamicSamplingContext).toBeDefined(); expect(dynamicSamplingContext).toStrictEqual({}); @@ -855,10 +807,6 @@ describe('browserTracingIntegration', () => { // 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', () => { From afc1878e7e0c26af5cc1e4e2bc54da2683427670 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 10 Jan 2025 10:56:39 +0100 Subject: [PATCH 3/7] do not set on span start?? --- .../src/tracing/browserTracingIntegration.ts | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index 2e761d263424..bbe0706c6959 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -282,6 +282,8 @@ export const browserTracingIntegration = ((_options: Partial Date: Fri, 10 Jan 2025 10:58:16 +0100 Subject: [PATCH 4/7] WIP changed tests --- .../tracing/browserTracingIntegration.test.ts | 73 +++++++++++++------ 1 file changed, 50 insertions(+), 23 deletions(-) diff --git a/packages/browser/test/tracing/browserTracingIntegration.test.ts b/packages/browser/test/tracing/browserTracingIntegration.test.ts index 3e9d2354a532..ba840f535a00 100644 --- a/packages/browser/test/tracing/browserTracingIntegration.test.ts +++ b/packages/browser/test/tracing/browserTracingIntegration.test.ts @@ -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'; @@ -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', () => { @@ -365,7 +372,7 @@ describe('browserTracingIntegration', () => { const client = new BrowserClient( getDefaultBrowserClientOptions({ - tracesSampleRate: 0, + tracesSampleRate: 1, integrations: [ browserTracingIntegration({ instrumentPageLoad: false, @@ -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'); }); @@ -408,7 +413,7 @@ describe('browserTracingIntegration', () => { const client = new BrowserClient( getDefaultBrowserClientOptions({ - tracesSampleRate: 0, + tracesSampleRate: 1, integrations: [ browserTracingIntegration({ instrumentPageLoad: false, @@ -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', () => { @@ -562,7 +571,7 @@ describe('browserTracingIntegration', () => { const client = new BrowserClient( getDefaultBrowserClientOptions({ - tracesSampleRate: 0, + tracesSampleRate: 1, integrations: [ browserTracingIntegration({ instrumentPageLoad: false, @@ -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'); }); @@ -590,7 +597,7 @@ describe('browserTracingIntegration', () => { const client = new BrowserClient( getDefaultBrowserClientOptions({ - tracesSampleRate: 0, + tracesSampleRate: 1, integrations: [ browserTracingIntegration({ instrumentPageLoad: false, @@ -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); @@ -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(); @@ -657,6 +665,22 @@ describe('browserTracingIntegration', () => { 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, + dsc: { + environment: 'production', + public_key: 'examplePublicKey', + sample_rate: '0', + trace_id: traceId2, + }, + }); }); it("saves the span's positive sampling decision and its DSC on the propagationContext when the span finishes", () => { @@ -679,6 +703,7 @@ describe('browserTracingIntegration', () => { traceId: expect.stringMatching(/[a-f0-9]{32}/), }); + updateSpanName(navigationSpan!, 'mySpan2'); navigationSpan!.end(); const propCtxAfterEnd = getCurrentScope().getPropagationContext(); @@ -690,7 +715,7 @@ describe('browserTracingIntegration', () => { public_key: 'examplePublicKey', sample_rate: '1', sampled: 'true', - transaction: 'mySpan', + transaction: 'mySpan2', trace_id: propCtxBeforeEnd.traceId, }, }); @@ -758,11 +783,8 @@ 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' }); @@ -770,6 +792,10 @@ describe('browserTracingIntegration', () => { // 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', () => { @@ -795,11 +821,8 @@ 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({}); @@ -807,6 +830,10 @@ describe('browserTracingIntegration', () => { // 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', () => { From a3322370fdc38a419636ef1543ef40a5a8778621 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 10 Jan 2025 11:24:49 +0100 Subject: [PATCH 5/7] fixes --- .../src/tracing/browserTracingIntegration.ts | 19 +++- .../tracing/browserTracingIntegration.test.ts | 12 ++ packages/core/src/tracing/idleSpan.ts | 106 +++++++++--------- 3 files changed, 85 insertions(+), 52 deletions(-) diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index bbe0706c6959..ebd143bc877d 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -9,7 +9,8 @@ import { startTrackingLongTasks, startTrackingWebVitals, } from '@sentry-internal/browser-utils'; -import type { Client, IntegrationFn, Span, StartSpanOptions, TransactionSource } from '@sentry/core'; +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, @@ -303,6 +304,22 @@ export const browserTracingIntegration = ((_options: Partial { }); expect(newCurrentScopePropCtx).toEqual({ traceId: expect.stringMatching(/[a-f0-9]{32}/), + sampled: false, }); expect(newIsolationScopePropCtx).toEqual({ traceId: expect.stringMatching(/[a-f0-9]{32}/), @@ -674,10 +675,19 @@ describe('browserTracingIntegration', () => { 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, }, }); @@ -701,6 +711,7 @@ describe('browserTracingIntegration', () => { const propCtxBeforeEnd = getCurrentScope().getPropagationContext(); expect(propCtxBeforeEnd).toStrictEqual({ traceId: expect.stringMatching(/[a-f0-9]{32}/), + sampled: true, }); updateSpanName(navigationSpan!, 'mySpan2'); @@ -739,6 +750,7 @@ describe('browserTracingIntegration', () => { const propCtxBeforeEnd = getCurrentScope().getPropagationContext(); expect(propCtxBeforeEnd).toStrictEqual({ traceId: expect.stringMatching(/[a-f0-9]{32}/), + sampled: false, }); navigationSpan!.end(); diff --git a/packages/core/src/tracing/idleSpan.ts b/packages/core/src/tracing/idleSpan.ts index 41e336677d2b..a9eab29786d0 100644 --- a/packages/core/src/tracing/idleSpan.ts +++ b/packages/core/src/tracing/idleSpan.ts @@ -109,6 +109,59 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti const client = getClient(); + function patchSpanEnd(span: Span): void { + // We patch span.end to ensure we can run some things before the span is ended + // eslint-disable-next-line @typescript-eslint/unbound-method + span.end = new Proxy(span.end, { + apply(target, thisArg, args: Parameters) { + if (beforeSpanEnd) { + beforeSpanEnd(span); + } + + // If the span is non-recording, nothing more to do here... + // This is the case if tracing is enabled but this specific span was not sampled + if (thisArg instanceof SentryNonRecordingSpan) { + return; + } + + // Just ensuring that this keeps working, even if we ever have more arguments here + const [definedEndTimestamp, ...rest] = args; + const timestamp = definedEndTimestamp || timestampInSeconds(); + const spanEndTimestamp = spanTimeInputToSeconds(timestamp); + + // Ensure we end with the last span timestamp, if possible + const spans = getSpanDescendants(span).filter(child => child !== span); + + // If we have no spans, we just end, nothing else to do here + if (!spans.length) { + onIdleSpanEnded(spanEndTimestamp); + return Reflect.apply(target, thisArg, [spanEndTimestamp, ...rest]); + } + + const childEndTimestamps = spans + .map(span => spanToJSON(span).timestamp) + .filter(timestamp => !!timestamp) as number[]; + const latestSpanEndTimestamp = childEndTimestamps.length ? Math.max(...childEndTimestamps) : undefined; + + // In reality this should always exist here, but type-wise it may be undefined... + const spanStartTimestamp = spanToJSON(span).start_timestamp; + + // The final endTimestamp should: + // * Never be before the span start timestamp + // * Be the latestSpanEndTimestamp, if there is one, and it is smaller than the passed span end timestamp + // * Otherwise be the passed end timestamp + // Final timestamp can never be after finalTimeout + const endTimestamp = Math.min( + spanStartTimestamp ? spanStartTimestamp + finalTimeout / 1000 : Infinity, + Math.max(spanStartTimestamp || -Infinity, Math.min(spanEndTimestamp, latestSpanEndTimestamp || Infinity)), + ); + + onIdleSpanEnded(endTimestamp); + return Reflect.apply(target, thisArg, [endTimestamp, ...rest]); + }, + }); + } + if (!client || !hasTracingEnabled()) { const span = new SentryNonRecordingSpan(); @@ -118,7 +171,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti ...getDynamicSamplingContextFromSpan(span), } satisfies Partial; freezeDscOnSpan(span, dsc); - + patchSpanEnd(span); return span; } @@ -126,56 +179,7 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti const previousActiveSpan = getActiveSpan(); const span = _startIdleSpan(startSpanOptions); - // We patch span.end to ensure we can run some things before the span is ended - // eslint-disable-next-line @typescript-eslint/unbound-method - span.end = new Proxy(span.end, { - apply(target, thisArg, args: Parameters) { - if (beforeSpanEnd) { - beforeSpanEnd(span); - } - - // If the span is non-recording, nothing more to do here... - // This is the case if tracing is enabled but this specific span was not sampled - if (thisArg instanceof SentryNonRecordingSpan) { - return; - } - - // Just ensuring that this keeps working, even if we ever have more arguments here - const [definedEndTimestamp, ...rest] = args; - const timestamp = definedEndTimestamp || timestampInSeconds(); - const spanEndTimestamp = spanTimeInputToSeconds(timestamp); - - // Ensure we end with the last span timestamp, if possible - const spans = getSpanDescendants(span).filter(child => child !== span); - - // If we have no spans, we just end, nothing else to do here - if (!spans.length) { - onIdleSpanEnded(spanEndTimestamp); - return Reflect.apply(target, thisArg, [spanEndTimestamp, ...rest]); - } - - const childEndTimestamps = spans - .map(span => spanToJSON(span).timestamp) - .filter(timestamp => !!timestamp) as number[]; - const latestSpanEndTimestamp = childEndTimestamps.length ? Math.max(...childEndTimestamps) : undefined; - - // In reality this should always exist here, but type-wise it may be undefined... - const spanStartTimestamp = spanToJSON(span).start_timestamp; - - // The final endTimestamp should: - // * Never be before the span start timestamp - // * Be the latestSpanEndTimestamp, if there is one, and it is smaller than the passed span end timestamp - // * Otherwise be the passed end timestamp - // Final timestamp can never be after finalTimeout - const endTimestamp = Math.min( - spanStartTimestamp ? spanStartTimestamp + finalTimeout / 1000 : Infinity, - Math.max(spanStartTimestamp || -Infinity, Math.min(spanEndTimestamp, latestSpanEndTimestamp || Infinity)), - ); - - onIdleSpanEnded(endTimestamp); - return Reflect.apply(target, thisArg, [endTimestamp, ...rest]); - }, - }); + patchSpanEnd(span); /** * Cancels the existing idle timeout, if there is one. From 373ff2c0be3d9993496a00426747f622efd9d25d Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 10 Jan 2025 11:39:09 +0100 Subject: [PATCH 6/7] fix linting --- packages/browser/src/tracing/browserTracingIntegration.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index ebd143bc877d..ab9489b2d314 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -9,7 +9,7 @@ import { startTrackingLongTasks, startTrackingWebVitals, } from '@sentry-internal/browser-utils'; -import type { Client, IntegrationFn, Span, StartSpanOptions, TransactionSource} from '@sentry/core'; +import type { Client, IntegrationFn, Span, StartSpanOptions, TransactionSource } from '@sentry/core'; import { dropUndefinedKeys } from '@sentry/core'; import { GLOBAL_OBJ, From 6bac717f342c2cf4e43fb7c6efd5d83e7751f1e8 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 10 Jan 2025 12:40:40 +0100 Subject: [PATCH 7/7] fix it --- packages/browser/src/tracing/browserTracingIntegration.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index ab9489b2d314..2b4af867acc1 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -458,7 +458,9 @@ 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);