From 5d3f587d577f327e37dbdcc7cf5c487998aa57d6 Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Wed, 26 Aug 2026 21:59:28 -0400 Subject: [PATCH 1/8] test(core): Cover error-to-span attribution for errors escaping a span Adds failing coverage for #16206: an error is attributed to whatever span is active at captureException time rather than the span it was thrown in. Three cases fail on develop (sync nested span, concurrent group, deepest escaped span). The fourth asserts the cross-trace bail-out, which passes today and must keep passing. --- .../lib/tracing/errorSpanAttribution.test.ts | 146 ++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 packages/core/test/lib/tracing/errorSpanAttribution.test.ts diff --git a/packages/core/test/lib/tracing/errorSpanAttribution.test.ts b/packages/core/test/lib/tracing/errorSpanAttribution.test.ts new file mode 100644 index 000000000000..f02b91b1835f --- /dev/null +++ b/packages/core/test/lib/tracing/errorSpanAttribution.test.ts @@ -0,0 +1,146 @@ +import { beforeEach, describe, expect, it } from 'vitest'; +import { captureException, setAsyncContextStrategy, setCurrentClient, startNewTrace, startSpan } from '../../../src'; +import type { Event } from '../../../src/types/event'; +import { getDefaultTestClientOptions, TestClient } from '../../mocks/client'; +import { resetGlobals } from '../../testutils'; + +const tick = (): Promise => new Promise(resolve => setTimeout(resolve, 0)); + +let client: TestClient; +let events: Event[]; + +describe('error span attribution', () => { + beforeEach(() => { + resetGlobals(); + setAsyncContextStrategy(undefined); + + events = []; + + const options = getDefaultTestClientOptions({ + tracesSampleRate: 1, + beforeSend: event => { + events.push(event); + return event; + }, + }); + client = new TestClient(options); + setCurrentClient(client); + client.init(); + }); + + it('attributes an error to the span it escaped, not the span it was caught in', async () => { + let innerSpanId: string | undefined; + let outerSpanId: string | undefined; + + startSpan({ name: 'outer' }, outerSpan => { + outerSpanId = outerSpan.spanContext().spanId; + + try { + startSpan({ name: 'inner' }, innerSpan => { + innerSpanId = innerSpan.spanContext().spanId; + throw new Error('inner failed'); + }); + } catch (error) { + captureException(error); + } + }); + + await client.flush(); + + expect(innerSpanId).not.toBe(outerSpanId); + expect(events).toHaveLength(1); + expect(events[0]?.contexts?.trace?.span_id).toBe(innerSpanId); + }); + + it('attributes an error to the failing branch of a concurrent group', async () => { + let failingSpanId: string | undefined; + let succeedingSpanId: string | undefined; + let reportingSpanId: string | undefined; + + await startSpan({ name: 'root' }, async () => { + let escapedError: unknown; + + try { + await Promise.all([ + startSpan({ name: 'failing' }, async span => { + failingSpanId = span.spanContext().spanId; + await tick(); + throw new Error('branch failed'); + }), + startSpan({ name: 'succeeding' }, async span => { + succeedingSpanId = span.spanContext().spanId; + await tick(); + }), + ]); + } catch (error) { + escapedError = error; + } + + // Report from a span that is unambiguously active, so the assertion does not depend on + // which scope the stack strategy happens to leak after the branches resume. + startSpan({ name: 'reporting' }, span => { + reportingSpanId = span.spanContext().spanId; + captureException(escapedError); + }); + }); + + await client.flush(); + + expect(failingSpanId).not.toBe(succeedingSpanId); + expect(failingSpanId).not.toBe(reportingSpanId); + expect(events).toHaveLength(1); + expect(events[0]?.contexts?.trace?.span_id).toBe(failingSpanId); + }); + + it('attributes an error to the deepest span it escaped', async () => { + let deepestSpanId: string | undefined; + + startSpan({ name: 'level-1' }, () => { + try { + startSpan({ name: 'level-2' }, () => { + startSpan({ name: 'level-3' }, span => { + deepestSpanId = span.spanContext().spanId; + throw new Error('level 3 failed'); + }); + }); + } catch (error) { + captureException(error); + } + }); + + await client.flush(); + + expect(events).toHaveLength(1); + expect(events[0]?.contexts?.trace?.span_id).toBe(deepestSpanId); + }); + + // The bail-out described in the design: an error that outlives its trace keeps today's + // behaviour, so the event never mixes a stale trace with the current scope's data. + it('does not attribute an error to a span from a previous trace', async () => { + let escapedError: unknown; + let currentTraceId: string | undefined; + let currentSpanId: string | undefined; + + try { + startSpan({ name: 'previous-trace' }, () => { + throw new Error('escaped its trace'); + }); + } catch (error) { + escapedError = error; + } + + startNewTrace(() => { + startSpan({ name: 'current-trace' }, span => { + currentTraceId = span.spanContext().traceId; + currentSpanId = span.spanContext().spanId; + captureException(escapedError); + }); + }); + + await client.flush(); + + expect(events).toHaveLength(1); + expect(events[0]?.contexts?.trace?.trace_id).toBe(currentTraceId); + expect(events[0]?.contexts?.trace?.span_id).toBe(currentSpanId); + }); +}); From dbe138cfc1ec6c39e4e071742539827e3ded784f Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Wed, 26 Aug 2026 22:13:03 -0400 Subject: [PATCH 2/8] fix(core): Attribute errors to the span they escaped An error was attributed to whichever span happened to be active when captureException ran, not to the span that actually failed. Record the span's trace context in a WeakMap keyed on the error as it unwinds, and prefer that when building the error event. The first (deepest) span wins, non-recording spans are skipped, and the attribution only applies within the error's own trace so the envelope header and body can never name different traces. --- packages/core/src/client.ts | 5 + packages/core/src/tracing/trace.ts | 5 +- .../core/src/utils/errorSpanAttribution.ts | 62 ++++++++ .../lib/tracing/errorSpanAttribution.test.ts | 133 +++++++++++++++--- 4 files changed, 187 insertions(+), 18 deletions(-) create mode 100644 packages/core/src/utils/errorSpanAttribution.ts diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index 12d02b348bb2..5443bb682036 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -4,6 +4,7 @@ import { DEFAULT_ENVIRONMENT } from './constants'; import { getCurrentScope, getIsolationScope, getTraceContextFromScope } from './currentScopes'; import { DEBUG_BUILD } from './debug-build'; import { createEventEnvelope, createSessionEnvelope } from './envelope'; +import { applyEscapedErrorSpanToEvent } from './utils/errorSpanAttribution'; import type { IntegrationIndex } from './integration'; import { afterSetupIntegrations, setupIntegration, setupIntegrations } from './integration'; import { _INTERNAL_flushLogsBuffer } from './logs/internal'; @@ -1443,6 +1444,10 @@ export abstract class Client { ...evt.contexts, }; + // Runs once the trace context is settled, so it also corrects events captured with no active + // span, whose trace context only exists as of the merge above. + applyEscapedErrorSpanToEvent(evt, hint); + const dynamicSamplingContext = getDynamicSamplingContextFromScope(this, currentScope); evt.sdkProcessingMetadata = { diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index fdfaf51d4057..1aa3818a9f1c 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -14,6 +14,7 @@ import type { StartSpanOptions } from '../types/startSpanOptions'; import { baggageHeaderToDynamicSamplingContext } from '../utils/baggage'; import { debug } from '../utils/debug-logger'; import { handleCallbackErrors } from '../utils/handleCallbackErrors'; +import { recordEscapedErrorSpan } from '../utils/errorSpanAttribution'; import { hasSpansEnabled } from '../utils/hasSpansEnabled'; import { shouldIgnoreSpan } from '../utils/should-ignore-span'; import { hasSpanStreamingEnabled } from './spans/hasSpanStreamingEnabled'; @@ -670,7 +671,9 @@ function runCallback(span: Span, makeSpanActive: boolean, callback: () => T, return wrapper(() => handleCallbackErrors( () => callback(), - () => { + error => { + recordEscapedErrorSpan(error, span); + // Only update the span status if it hasn't been changed yet, and the span is not yet finished const { status } = spanToStaticSpanJSON(span); if (span.isRecording() && status === 'ok') { diff --git a/packages/core/src/utils/errorSpanAttribution.ts b/packages/core/src/utils/errorSpanAttribution.ts new file mode 100644 index 000000000000..54b517d212cf --- /dev/null +++ b/packages/core/src/utils/errorSpanAttribution.ts @@ -0,0 +1,62 @@ +import type { TraceContext } from '../types/context'; +import type { Event, EventHint } from '../types/event'; +import type { Span } from '../types/span'; +import { isPrimitive } from './is'; +import { spanToTraceContext } from './spanUtils'; + +/** + * The trace context of the span an error escaped, keyed by the error itself. + * + * We store the plain trace context rather than the span, so that an error object cannot keep a + * whole span tree alive for as long as it is referenced. + */ +const escapedSpanTraceContexts = new WeakMap(); + +function toKey(error: unknown): object | undefined { + return isPrimitive(error) ? undefined : error; +} + +/** + * Remember which span an error escaped, so a later `captureException` can attribute the error to + * the span that actually failed instead of whichever span happens to be active at capture time. + * + * The first span to see the error wins: as an error unwinds through nested spans, the innermost + * one is the one that failed. Non-recording spans are skipped because they are never sent, so + * their span id would point at a span that does not exist. + */ +export function recordEscapedErrorSpan(error: unknown, span: Span): void { + const key = toKey(error); + + if (!key || !span.isRecording() || escapedSpanTraceContexts.has(key)) { + return; + } + + escapedSpanTraceContexts.set(key, spanToTraceContext(span)); +} + +/** + * Attribute an error event to the span the error escaped, if we recorded one. + * + * This only applies within the error's own trace. The stored span id is meaningless in another + * trace, and the event's dynamic sampling context (which the envelope header is built from) is + * derived from the root span of the trace the event is already on. Rewriting the trace id here + * would leave the envelope header and body naming different traces. + */ +export function applyEscapedErrorSpanToEvent(event: Event, hint: EventHint): void { + const key = toKey(hint.originalException); + const traceContext = key && escapedSpanTraceContexts.get(key); + const eventTraceContext = event.contexts?.trace; + + if (!traceContext || !eventTraceContext || eventTraceContext.trace_id !== traceContext.trace_id) { + return; + } + + event.contexts = { + ...event.contexts, + trace: { + ...eventTraceContext, + span_id: traceContext.span_id, + parent_span_id: traceContext.parent_span_id, + }, + }; +} diff --git a/packages/core/test/lib/tracing/errorSpanAttribution.test.ts b/packages/core/test/lib/tracing/errorSpanAttribution.test.ts index f02b91b1835f..0b3ea97d0cd5 100644 --- a/packages/core/test/lib/tracing/errorSpanAttribution.test.ts +++ b/packages/core/test/lib/tracing/errorSpanAttribution.test.ts @@ -1,6 +1,14 @@ import { beforeEach, describe, expect, it } from 'vitest'; -import { captureException, setAsyncContextStrategy, setCurrentClient, startNewTrace, startSpan } from '../../../src'; +import { + captureException, + getActiveSpan, + setAsyncContextStrategy, + setCurrentClient, + startNewTrace, + startSpan, +} from '../../../src'; import type { Event } from '../../../src/types/event'; +import type { TestClientOptions } from '../../mocks/client'; import { getDefaultTestClientOptions, TestClient } from '../../mocks/client'; import { resetGlobals } from '../../testutils'; @@ -9,23 +17,28 @@ const tick = (): Promise => new Promise(resolve => setTimeout(resolve, 0)) let client: TestClient; let events: Event[]; +function initClient(extraOptions: Partial = {}): void { + events = []; + + const options = getDefaultTestClientOptions({ + tracesSampleRate: 1, + beforeSend: event => { + // The test client strips `sdkProcessingMetadata` when it sends, so snapshot the event here. + events.push({ ...event }); + return event; + }, + ...extraOptions, + }); + client = new TestClient(options); + setCurrentClient(client); + client.init(); +} + describe('error span attribution', () => { beforeEach(() => { resetGlobals(); setAsyncContextStrategy(undefined); - - events = []; - - const options = getDefaultTestClientOptions({ - tracesSampleRate: 1, - beforeSend: event => { - events.push(event); - return event; - }, - }); - client = new TestClient(options); - setCurrentClient(client); - client.init(); + initClient(); }); it('attributes an error to the span it escaped, not the span it was caught in', async () => { @@ -77,7 +90,7 @@ describe('error span attribution', () => { } // Report from a span that is unambiguously active, so the assertion does not depend on - // which scope the stack strategy happens to leak after the branches resume. + // which scope the stack strategy happens to leak once the branches resume. startSpan({ name: 'reporting' }, span => { reportingSpanId = span.spanContext().spanId; captureException(escapedError); @@ -92,6 +105,28 @@ describe('error span attribution', () => { expect(events[0]?.contexts?.trace?.span_id).toBe(failingSpanId); }); + it('attributes an error captured with no active span, in the same trace', async () => { + let escapedError: unknown; + let escapedSpanId: string | undefined; + + try { + startSpan({ name: 'failing' }, span => { + escapedSpanId = span.spanContext().spanId; + throw new Error('boom'); + }); + } catch (error) { + escapedError = error; + } + + expect(getActiveSpan()).toBeUndefined(); + captureException(escapedError); + + await client.flush(); + + expect(events).toHaveLength(1); + expect(events[0]?.contexts?.trace?.span_id).toBe(escapedSpanId); + }); + it('attributes an error to the deepest span it escaped', async () => { let deepestSpanId: string | undefined; @@ -114,8 +149,8 @@ describe('error span attribution', () => { expect(events[0]?.contexts?.trace?.span_id).toBe(deepestSpanId); }); - // The bail-out described in the design: an error that outlives its trace keeps today's - // behaviour, so the event never mixes a stale trace with the current scope's data. + // The stored span id is only meaningful inside its own trace, so an error that outlives its + // trace keeps today's behaviour rather than mixing a stale trace into the current scope's data. it('does not attribute an error to a span from a previous trace', async () => { let escapedError: unknown; let currentTraceId: string | undefined; @@ -143,4 +178,68 @@ describe('error span attribution', () => { expect(events[0]?.contexts?.trace?.trace_id).toBe(currentTraceId); expect(events[0]?.contexts?.trace?.span_id).toBe(currentSpanId); }); + + it('falls back to the active span when a non-object is thrown', async () => { + let outerSpanId: string | undefined; + + startSpan({ name: 'outer' }, outerSpan => { + outerSpanId = outerSpan.spanContext().spanId; + + try { + startSpan({ name: 'inner' }, () => { + throw 'a string, which cannot key a WeakMap'; + }); + } catch (error) { + captureException(error); + } + }); + + await client.flush(); + + expect(events).toHaveLength(1); + expect(events[0]?.contexts?.trace?.span_id).toBe(outerSpanId); + }); + + it('does not attribute an error to an ignored span, which is never sent', async () => { + initClient({ traceLifecycle: 'stream', ignoreSpans: ['ignored'] }); + + let outerSpanId: string | undefined; + + startSpan({ name: 'outer' }, outerSpan => { + outerSpanId = outerSpan.spanContext().spanId; + + try { + startSpan({ name: 'ignored' }, () => { + throw new Error('ignored span failed'); + }); + } catch (error) { + captureException(error); + } + }); + + await client.flush(); + + expect(events).toHaveLength(1); + expect(events[0]?.contexts?.trace?.span_id).toBe(outerSpanId); + }); + + // Why the attribution is gated on the trace: the envelope header is built from the dynamic + // sampling context, so it must never name a different trace than the trace context does. + it('keeps the dynamic sampling context in agreement with the trace context', async () => { + startSpan({ name: 'outer' }, () => { + try { + startSpan({ name: 'inner' }, () => { + throw new Error('inner failed'); + }); + } catch (error) { + captureException(error); + } + }); + + await client.flush(); + + const traceContext = events[0]?.contexts?.trace; + expect(traceContext?.trace_id).toBeDefined(); + expect(events[0]?.sdkProcessingMetadata?.dynamicSamplingContext?.trace_id).toBe(traceContext?.trace_id); + }); }); From d1a5baf7fd52775d52615cd90157a87f47ccacbf Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Wed, 26 Aug 2026 22:24:40 -0400 Subject: [PATCH 3/8] test(e2e): Assert hapi errors are attributed to the route handler span The error thrown in a hapi route handler escapes the router span, so it is now attributed to that span rather than to the request span. Assert the new relationship (error span is a child of the transaction's span, and is the router span) instead of the old identity. --- .../node-hapi/tests/errors.test.ts | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/node-hapi/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/node-hapi/tests/errors.test.ts index f355f253b53e..f1f2aaf8aafb 100644 --- a/dev-packages/e2e-tests/test-applications/node-hapi/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-hapi/tests/errors.test.ts @@ -1,23 +1,26 @@ import { expect, test } from '@playwright/test'; -import { waitForError, waitForStreamedSpan } from '@sentry-internal/test-utils'; +import { + collectStreamedSpansUntilSegment, + getSpanOp, + waitForError, + waitForStreamedSpan, +} from '@sentry-internal/test-utils'; test('Sends thrown error to Sentry', async ({ baseURL }) => { const errorEventPromise = waitForError('node-hapi', errorEvent => { return errorEvent?.exception?.values?.[0]?.value === 'This is an error'; }); - const segmentEventPromise = waitForStreamedSpan( - 'node-hapi', - segment => segment.is_segment && segment.name === 'GET /test-failure', - ); + const spansPromise = collectStreamedSpansUntilSegment('node-hapi', 'GET /test-failure'); await fetch(`${baseURL}/test-failure`); const errorEvent = await errorEventPromise; - const segmentEvent = await segmentEventPromise; + const spans = await spansPromise; + const segmentSpan = spans.find(span => span.is_segment); - expect(segmentEvent.name).toBe('GET /test-failure'); - expect(segmentEvent).toMatchObject({ + expect(segmentSpan?.name).toBe('GET /test-failure'); + expect(segmentSpan).toMatchObject({ trace_id: expect.stringMatching(/[a-f0-9]{32}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), }); @@ -42,10 +45,15 @@ test('Sends thrown error to Sentry', async ({ baseURL }) => { expect(errorEvent.contexts?.trace).toEqual({ trace_id: expect.stringMatching(/[a-f0-9]{32}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), + parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), }); - expect(errorEvent.contexts?.trace?.trace_id).toBe(segmentEvent?.trace_id); - expect(errorEvent.contexts?.trace?.span_id).toBe(segmentEvent?.span_id); + // The error is attributed to the route handler span that threw, which is a child of the request + // span the segment is built from. + const routeHandlerSpan = spans.find(span => getSpanOp(span) === 'router'); + expect(errorEvent.contexts?.trace?.trace_id).toBe(segmentSpan?.trace_id); + expect(errorEvent.contexts?.trace?.span_id).toBe(routeHandlerSpan?.span_id); + expect(errorEvent.contexts?.trace?.parent_span_id).toBe(segmentSpan?.span_id); }); test('sends error with parameterized transaction name', async ({ baseURL }) => { From b9b5cd5591f0395bf92de02f558a586c5e3417ca Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Thu, 27 Aug 2026 10:57:10 -0400 Subject: [PATCH 4/8] docs(core): Clarify why error span attribution stores a trace context Rename `toKey` to `toWeakMapKey` and explain that thrown primitives cannot be keyed. The previous WeakMap comment claimed a GC reason that does not hold. --- packages/core/src/utils/errorSpanAttribution.ts | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/packages/core/src/utils/errorSpanAttribution.ts b/packages/core/src/utils/errorSpanAttribution.ts index 54b517d212cf..c3015cc660cd 100644 --- a/packages/core/src/utils/errorSpanAttribution.ts +++ b/packages/core/src/utils/errorSpanAttribution.ts @@ -7,12 +7,16 @@ import { spanToTraceContext } from './spanUtils'; /** * The trace context of the span an error escaped, keyed by the error itself. * - * We store the plain trace context rather than the span, so that an error object cannot keep a - * whole span tree alive for as long as it is referenced. + * We store the trace context rather than the span because that is the shape we apply to the event + * later, and it snapshots the span as it failed instead of reading it back once it has ended. */ const escapedSpanTraceContexts = new WeakMap(); -function toKey(error: unknown): object | undefined { +/** + * A `WeakMap` can only be keyed by an object, so an error thrown as a primitive (`throw 'boom'`) + * has nothing we can hang the span on and is left unattributed. + */ +function toWeakMapKey(error: unknown): object | undefined { return isPrimitive(error) ? undefined : error; } @@ -25,7 +29,7 @@ function toKey(error: unknown): object | undefined { * their span id would point at a span that does not exist. */ export function recordEscapedErrorSpan(error: unknown, span: Span): void { - const key = toKey(error); + const key = toWeakMapKey(error); if (!key || !span.isRecording() || escapedSpanTraceContexts.has(key)) { return; @@ -43,7 +47,7 @@ export function recordEscapedErrorSpan(error: unknown, span: Span): void { * would leave the envelope header and body naming different traces. */ export function applyEscapedErrorSpanToEvent(event: Event, hint: EventHint): void { - const key = toKey(hint.originalException); + const key = toWeakMapKey(hint.originalException); const traceContext = key && escapedSpanTraceContexts.get(key); const eventTraceContext = event.contexts?.trace; From b1d8b5dda49a9b633b44dfcb346cf214b0a9681f Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Thu, 27 Aug 2026 11:10:25 -0400 Subject: [PATCH 5/8] docs(core): Explain why escaped error span attribution runs after the merge The trace context of an event captured with no active span only exists as of that merge, and without its trace id we cannot check the recorded span belongs to the same trace. --- packages/core/src/client.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index 5443bb682036..5e930932319d 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -1444,8 +1444,9 @@ export abstract class Client { ...evt.contexts, }; - // Runs once the trace context is settled, so it also corrects events captured with no active - // span, whose trace context only exists as of the merge above. + // Deliberately after the merge above: an error captured with no active span has no trace + // context until then, and without its trace id we cannot tell whether the span we recorded + // belongs to the same trace, which risks the event disagreeing with the DSC we build below. applyEscapedErrorSpanToEvent(evt, hint); const dynamicSamplingContext = getDynamicSamplingContextFromScope(this, currentScope); From 1285e11adb6a015e371f88ddbbc65f313e5d0c2a Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Tue, 8 Sep 2026 12:32:46 -0400 Subject: [PATCH 6/8] fix(core): Attribute the escaped error span before event processors run Attribution ran once the event was fully assembled, so scope and client event processors, along with the `postprocessEvent` hook, still saw the span that happened to be active at capture time. Move the lookup next to `applySpanToEvent` in `prepareEvent`. The same trace guard stays, but the trace id now comes from the scope when the event has no trace context of its own, which is what pushed the call downstream in the first place. The stored context is applied whole rather than spliced in: at this point the event may have no trace context at all, and the merge in `_prepareEvent` lets an existing one win outright, so a partial context would never get its trace id filled in. --- packages/core/src/client.ts | 6 -- .../core/src/utils/errorSpanAttribution.ts | 18 ++++-- packages/core/src/utils/prepareEvent.ts | 6 ++ .../lib/tracing/errorSpanAttribution.test.ts | 55 +++++++++++++++++++ 4 files changed, 75 insertions(+), 10 deletions(-) diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index 5e930932319d..12d02b348bb2 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -4,7 +4,6 @@ import { DEFAULT_ENVIRONMENT } from './constants'; import { getCurrentScope, getIsolationScope, getTraceContextFromScope } from './currentScopes'; import { DEBUG_BUILD } from './debug-build'; import { createEventEnvelope, createSessionEnvelope } from './envelope'; -import { applyEscapedErrorSpanToEvent } from './utils/errorSpanAttribution'; import type { IntegrationIndex } from './integration'; import { afterSetupIntegrations, setupIntegration, setupIntegrations } from './integration'; import { _INTERNAL_flushLogsBuffer } from './logs/internal'; @@ -1444,11 +1443,6 @@ export abstract class Client { ...evt.contexts, }; - // Deliberately after the merge above: an error captured with no active span has no trace - // context until then, and without its trace id we cannot tell whether the span we recorded - // belongs to the same trace, which risks the event disagreeing with the DSC we build below. - applyEscapedErrorSpanToEvent(evt, hint); - const dynamicSamplingContext = getDynamicSamplingContextFromScope(this, currentScope); evt.sdkProcessingMetadata = { diff --git a/packages/core/src/utils/errorSpanAttribution.ts b/packages/core/src/utils/errorSpanAttribution.ts index c3015cc660cd..1e6779be6244 100644 --- a/packages/core/src/utils/errorSpanAttribution.ts +++ b/packages/core/src/utils/errorSpanAttribution.ts @@ -1,3 +1,5 @@ +import { getTraceContextFromScope } from '../currentScopes'; +import type { Scope } from '../scope'; import type { TraceContext } from '../types/context'; import type { Event, EventHint } from '../types/event'; import type { Span } from '../types/span'; @@ -46,12 +48,21 @@ export function recordEscapedErrorSpan(error: unknown, span: Span): void { * derived from the root span of the trace the event is already on. Rewriting the trace id here * would leave the envelope header and body naming different traces. */ -export function applyEscapedErrorSpanToEvent(event: Event, hint: EventHint): void { +export function applyEscapedErrorSpanToEvent(event: Event, hint: EventHint, scope: Scope | undefined): void { const key = toWeakMapKey(hint.originalException); const traceContext = key && escapedSpanTraceContexts.get(key); + + if (!traceContext) { + return; + } + + // An error captured with no active span has no trace context yet: the scope's is merged in + // further downstream. Resolve the trace the event will end up on the same way that merge does, + // so the check below still knows which trace we are on. const eventTraceContext = event.contexts?.trace; + const eventTraceId = eventTraceContext?.trace_id ?? (scope && getTraceContextFromScope(scope).trace_id); - if (!traceContext || !eventTraceContext || eventTraceContext.trace_id !== traceContext.trace_id) { + if (eventTraceId !== traceContext.trace_id) { return; } @@ -59,8 +70,7 @@ export function applyEscapedErrorSpanToEvent(event: Event, hint: EventHint): voi ...event.contexts, trace: { ...eventTraceContext, - span_id: traceContext.span_id, - parent_span_id: traceContext.parent_span_id, + ...traceContext, }, }; } diff --git a/packages/core/src/utils/prepareEvent.ts b/packages/core/src/utils/prepareEvent.ts index 080f1ab752d1..f00e90932135 100644 --- a/packages/core/src/utils/prepareEvent.ts +++ b/packages/core/src/utils/prepareEvent.ts @@ -8,6 +8,7 @@ import type { ClientOptions } from '../types/options'; import type { StackParser } from '../types/stacktrace'; import { getFilenameToDebugIdMap } from './debug-ids'; import { getDataCategoryByType } from './envelope'; +import { applyEscapedErrorSpanToEvent } from './errorSpanAttribution'; import { addExceptionMechanismToCapturedException, uuid4 } from './misc'; import { normalize } from './normalize'; import { applyScopeDataToEvent, applySpanToEvent, getCombinedScopeData } from './scopeData'; @@ -95,6 +96,11 @@ export function prepareEvent( applySpanToEvent(prepared, span); } + // After the active span, so an error that escaped a span is attributed to that span rather than + // to whichever one happened to be active at capture time. Done here rather than once the event is + // assembled so that event processors and the `postprocessEvent` hook see the corrected span id. + applyEscapedErrorSpanToEvent(prepared, hint, finalScope); + const eventProcessors = [ ...clientEventProcessors, // Run scope event processors _after_ all other processors diff --git a/packages/core/test/lib/tracing/errorSpanAttribution.test.ts b/packages/core/test/lib/tracing/errorSpanAttribution.test.ts index 0b3ea97d0cd5..673bedfa9598 100644 --- a/packages/core/test/lib/tracing/errorSpanAttribution.test.ts +++ b/packages/core/test/lib/tracing/errorSpanAttribution.test.ts @@ -1,5 +1,6 @@ import { beforeEach, describe, expect, it } from 'vitest'; import { + addEventProcessor, captureException, getActiveSpan, setAsyncContextStrategy, @@ -242,4 +243,58 @@ describe('error span attribution', () => { expect(traceContext?.trace_id).toBeDefined(); expect(events[0]?.sdkProcessingMetadata?.dynamicSamplingContext?.trace_id).toBe(traceContext?.trace_id); }); + // Attribution used to happen once the event was fully assembled, which left event processors + // looking at the span that was active at capture time. + it('has attributed the error before event processors run', async () => { + let seenSpanId: string | undefined; + let innerSpanId: string | undefined; + + addEventProcessor(event => { + if (!event.type) { + seenSpanId = event.contexts?.trace?.span_id; + } + return event; + }); + + startSpan({ name: 'outer' }, () => { + try { + startSpan({ name: 'inner' }, innerSpan => { + innerSpanId = innerSpan.spanContext().spanId; + throw new Error('inner failed'); + }); + } catch (error) { + captureException(error); + } + }); + + await client.flush(); + + expect(seenSpanId).toBe(innerSpanId); + }); + + it('leaves the event with a complete trace context when nothing is active at capture time', async () => { + let innerSpanId: string | undefined; + let traceId: string | undefined; + let caught: unknown; + + startSpan({ name: 'outer' }, outerSpan => { + traceId = outerSpan.spanContext().traceId; + + try { + startSpan({ name: 'inner' }, innerSpan => { + innerSpanId = innerSpan.spanContext().spanId; + throw new Error('inner failed'); + }); + } catch (error) { + caught = error; + } + }); + + expect(getActiveSpan()).toBeUndefined(); + captureException(caught); + + await client.flush(); + + expect(events[0]?.contexts?.trace).toEqual(expect.objectContaining({ trace_id: traceId, span_id: innerSpanId })); + }); }); From 4099fde735a5755757f386ed560b0b84be18d401 Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Tue, 8 Sep 2026 12:32:54 -0400 Subject: [PATCH 7/8] fix(core): Attribute errors escaping a span that already ended `recordEscapedErrorSpan` skipped spans that were not recording, which bundles two conditions: the span is sampled, and it has not ended. Only sampling is relevant, since an unsampled span is never sent and its id would point at nothing. `handleCallbackErrors` runs its error handler after the callback's own `finally` block, and `startSpanManual` leaves ending the span to the caller. So the usual manual span idiom, ending the span in a `finally` and letting the error propagate, ended the span before the error unwound past it and was skipped entirely, despite being sampled and sent. Check `spanIsSampled` instead. --- .../core/src/utils/errorSpanAttribution.ts | 10 ++++--- .../lib/tracing/errorSpanAttribution.test.ts | 27 +++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/packages/core/src/utils/errorSpanAttribution.ts b/packages/core/src/utils/errorSpanAttribution.ts index 1e6779be6244..729a9bf0c1c6 100644 --- a/packages/core/src/utils/errorSpanAttribution.ts +++ b/packages/core/src/utils/errorSpanAttribution.ts @@ -4,7 +4,7 @@ import type { TraceContext } from '../types/context'; import type { Event, EventHint } from '../types/event'; import type { Span } from '../types/span'; import { isPrimitive } from './is'; -import { spanToTraceContext } from './spanUtils'; +import { spanIsSampled, spanToTraceContext } from './spanUtils'; /** * The trace context of the span an error escaped, keyed by the error itself. @@ -27,13 +27,15 @@ function toWeakMapKey(error: unknown): object | undefined { * the span that actually failed instead of whichever span happens to be active at capture time. * * The first span to see the error wins: as an error unwinds through nested spans, the innermost - * one is the one that failed. Non-recording spans are skipped because they are never sent, so - * their span id would point at a span that does not exist. + * one is the one that failed. Unsampled spans are skipped because they are never sent, so their + * span id would point at a span that does not exist. Sampling rather than `isRecording()` is what + * matters here: a span ended before the error escaped it, which is the norm for `startSpanManual`, + * has stopped recording but is still sent. */ export function recordEscapedErrorSpan(error: unknown, span: Span): void { const key = toWeakMapKey(error); - if (!key || !span.isRecording() || escapedSpanTraceContexts.has(key)) { + if (!key || !spanIsSampled(span) || escapedSpanTraceContexts.has(key)) { return; } diff --git a/packages/core/test/lib/tracing/errorSpanAttribution.test.ts b/packages/core/test/lib/tracing/errorSpanAttribution.test.ts index 673bedfa9598..600ba9d118a5 100644 --- a/packages/core/test/lib/tracing/errorSpanAttribution.test.ts +++ b/packages/core/test/lib/tracing/errorSpanAttribution.test.ts @@ -7,6 +7,7 @@ import { setCurrentClient, startNewTrace, startSpan, + startSpanManual, } from '../../../src'; import type { Event } from '../../../src/types/event'; import type { TestClientOptions } from '../../mocks/client'; @@ -297,4 +298,30 @@ describe('error span attribution', () => { expect(events[0]?.contexts?.trace).toEqual(expect.objectContaining({ trace_id: traceId, span_id: innerSpanId })); }); + // `startSpanManual` ends the span inside the callback, so by the time the error unwinds past it + // the span has stopped recording. It is still sampled and still sent, so it is still the span + // the error escaped. + it('attributes an error to a span that was ended before the error escaped it', async () => { + let innerSpanId: string | undefined; + + startSpan({ name: 'outer' }, () => { + try { + startSpanManual({ name: 'inner' }, span => { + innerSpanId = span.spanContext().spanId; + try { + throw new Error('inner failed'); + } finally { + span.end(); + } + }); + } catch (error) { + captureException(error); + } + }); + + await client.flush(); + + expect(events).toHaveLength(1); + expect(events[0]?.contexts?.trace?.span_id).toBe(innerSpanId); + }); }); From 86c65874a8355484ae1f612c7bca81e105359a59 Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Wed, 9 Sep 2026 15:58:10 -0400 Subject: [PATCH 8/8] test(browser): Cover error-to-span attribution in the browser Co-Authored-By: Claude Opus 5 (1M context) --- .../tracing/errorSpanAttribution/init.js | 10 ++ .../tracing/errorSpanAttribution/test.ts | 103 ++++++++++++++++++ 2 files changed, 113 insertions(+) create mode 100644 dev-packages/browser-integration-tests/suites/tracing/errorSpanAttribution/init.js create mode 100644 dev-packages/browser-integration-tests/suites/tracing/errorSpanAttribution/test.ts diff --git a/dev-packages/browser-integration-tests/suites/tracing/errorSpanAttribution/init.js b/dev-packages/browser-integration-tests/suites/tracing/errorSpanAttribution/init.js new file mode 100644 index 000000000000..bb6d4918d1bf --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/errorSpanAttribution/init.js @@ -0,0 +1,10 @@ +import * as Sentry from '@sentry/browser'; + +window.Sentry = Sentry; + +Sentry.init({ + traceLifecycle: 'static', + dsn: 'https://public@dsn.ingest.sentry.io/1337', + integrations: [Sentry.browserTracingIntegration()], + tracesSampleRate: 1, +}); diff --git a/dev-packages/browser-integration-tests/suites/tracing/errorSpanAttribution/test.ts b/dev-packages/browser-integration-tests/suites/tracing/errorSpanAttribution/test.ts new file mode 100644 index 000000000000..8477d838a613 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/errorSpanAttribution/test.ts @@ -0,0 +1,103 @@ +import type { Page } from '@playwright/test'; +import { expect } from '@playwright/test'; +import type { Event } from '@sentry/core'; +import { sentryTest } from '../../../utils/fixtures'; +import { + envelopeRequestParser, + runScriptInSandbox, + shouldSkipTracingTest, + waitForErrorRequest, + waitForTransactionRequest, +} from '../../../utils/helpers'; + +// The browser SDK parents every span to the root span, so `outer` and `inner` are siblings under the +// pageload span rather than a chain. Attribution still has to pick the span the error escaped. +const waitForPageloadWithSpans = (page: Page) => + waitForTransactionRequest( + page, + event => event.contexts?.trace?.op === 'pageload' && !!event.spans?.some(span => span.description === 'inner'), + ); + +sentryTest( + 'attributes an uncaught error to the span it escaped, not the pageload span active when it surfaces', + async ({ getLocalTestUrl, page, browserName }) => { + if (browserName === 'webkit') { + // Errors thrown from `runScriptInSandbox` are Script Errors on Webkit and skipped by Sentry + sentryTest.skip(); + } + + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const url = await getLocalTestUrl({ testDir: __dirname }); + + const errorRequestPromise = waitForErrorRequest(page); + const transactionRequestPromise = waitForPageloadWithSpans(page); + + await page.goto(url); + + await runScriptInSandbox(page, { + content: ` + setTimeout(() => { + Sentry.startSpan({ name: 'outer' }, () => { + Sentry.startSpan({ name: 'inner' }, () => { + throw new Error('Escaped Error'); + }); + }); + }); + `, + }); + + const errorEvent = envelopeRequestParser(await errorRequestPromise); + const transactionEvent = envelopeRequestParser(await transactionRequestPromise); + + const innerSpan = transactionEvent.spans?.find(span => span.description === 'inner'); + + expect(errorEvent.exception?.values?.[0]?.value).toBe('Escaped Error'); + expect(errorEvent.contexts?.trace?.trace_id).toBe(transactionEvent.contexts?.trace?.trace_id); + expect(errorEvent.contexts?.trace?.span_id).toBe(innerSpan?.span_id); + expect(errorEvent.contexts?.trace?.span_id).not.toBe(transactionEvent.contexts?.trace?.span_id); + }, +); + +sentryTest( + 'attributes a caught error to the span it escaped, not the span it was caught in', + async ({ getLocalTestUrl, page }) => { + if (shouldSkipTracingTest()) { + sentryTest.skip(); + } + + const url = await getLocalTestUrl({ testDir: __dirname }); + + const errorRequestPromise = waitForErrorRequest(page); + const transactionRequestPromise = waitForPageloadWithSpans(page); + + await page.goto(url); + + await runScriptInSandbox(page, { + content: ` + Sentry.startSpan({ name: 'outer' }, () => { + try { + Sentry.startSpan({ name: 'inner' }, () => { + throw new Error('Caught Error'); + }); + } catch (error) { + Sentry.captureException(error); + } + }); + `, + }); + + const errorEvent = envelopeRequestParser(await errorRequestPromise); + const transactionEvent = envelopeRequestParser(await transactionRequestPromise); + + const innerSpan = transactionEvent.spans?.find(span => span.description === 'inner'); + const outerSpan = transactionEvent.spans?.find(span => span.description === 'outer'); + + expect(errorEvent.exception?.values?.[0]?.value).toBe('Caught Error'); + expect(errorEvent.contexts?.trace?.trace_id).toBe(transactionEvent.contexts?.trace?.trace_id); + expect(errorEvent.contexts?.trace?.span_id).toBe(innerSpan?.span_id); + expect(errorEvent.contexts?.trace?.span_id).not.toBe(outerSpan?.span_id); + }, +);