diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index f4ab605be9c2..2b4af867acc1 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -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, @@ -24,7 +25,6 @@ import { getDynamicSamplingContextFromSpan, getIsolationScope, getLocationHref, - getRootSpan, logger, propagationContextFromHeaders, registerSpanErrorInstrumentation, @@ -276,6 +276,17 @@ 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, { @@ -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); diff --git a/packages/browser/test/tracing/browserTracingIntegration.test.ts b/packages/browser/test/tracing/browserTracingIntegration.test.ts index 3e9d2354a532..4ca1f785a895 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(); @@ -650,6 +658,7 @@ describe('browserTracingIntegration', () => { }); expect(newCurrentScopePropCtx).toEqual({ traceId: expect.stringMatching(/[a-f0-9]{32}/), + sampled: false, }); expect(newIsolationScopePropCtx).toEqual({ traceId: expect.stringMatching(/[a-f0-9]{32}/), @@ -657,6 +666,31 @@ 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, + }); + + 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", () => { @@ -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(); @@ -690,7 +726,7 @@ describe('browserTracingIntegration', () => { public_key: 'examplePublicKey', sample_rate: '1', sampled: 'true', - transaction: 'mySpan', + transaction: 'mySpan2', trace_id: propCtxBeforeEnd.traceId, }, }); @@ -714,6 +750,7 @@ describe('browserTracingIntegration', () => { const propCtxBeforeEnd = getCurrentScope().getPropagationContext(); expect(propCtxBeforeEnd).toStrictEqual({ traceId: expect.stringMatching(/[a-f0-9]{32}/), + sampled: false, }); navigationSpan!.end(); @@ -758,11 +795,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 +804,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 +833,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 +842,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/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. 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.');