Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/core/src/tracing/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export {
spanIsTracerProviderSpan,
} from './utils';
export { startIdleSpan, TRACING_DEFAULTS } from './idleSpan';
export { SentrySpan } from './sentrySpan';
export { SentrySpan, _INTERNAL_setDeferSegmentSpanCapture } from './sentrySpan';
export { SentryNonRecordingSpan } from './sentryNonRecordingSpan';
export { setHttpStatus, getSpanStatusFromHttpCode } from './spanstatus';
export { SPAN_STATUS_ERROR, SPAN_STATUS_OK, SPAN_STATUS_UNSET } from './spanstatus';
Expand Down
134 changes: 122 additions & 12 deletions packages/core/src/tracing/sentrySpan.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* eslint-disable max-lines */
import type { Client } from '../client';
import { getClient, getCurrentScope } from '../currentScopes';
import { DEBUG_BUILD } from '../debug-build';
import { createSpanEnvelope } from '../envelope';
Expand Down Expand Up @@ -26,7 +27,9 @@ import type {
} from '../types/span';
import type { SpanStatus } from '../types/spanStatus';
import type { TimedEvent } from '../types/timedEvent';
import { debounce } from '../utils/debounce';
import { debug } from '../utils/debug-logger';
import { isBrowser } from '../utils/isBrowser';
import { generateSpanId, generateTraceId } from '../utils/propagationContext';
import {
addStatusMessageAttribute,
Expand All @@ -51,6 +54,59 @@ import { getCapturedScopesOnSpan, markSpanSourceAsExplicit, spanShouldInferOtelS

const MAX_SPAN_COUNT = 1000;

// Clients whose segment-span transaction capture should be deferred (rather than run synchronously on
// span end), mapped to the function that queues a deferred capture. Tracked per client rather than as
// a process-wide flag so pending captures and their timer cannot leak across `Sentry.init()` calls —
// a client that never opts in is simply absent. Enabled per client by SDKs that assemble transactions
// from the live span tree on root-span end (e.g. the Node SDK), which would otherwise drop children
// that close after it. Every other setup keeps its synchronous capture.
Comment on lines +57 to +62

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Clients whose segment-span transaction capture should be deferred (rather than run synchronously on
// span end), mapped to the function that queues a deferred capture. Tracked per client rather than as
// a process-wide flag so pending captures and their timer cannot leak across `Sentry.init()` calls —
// a client that never opts in is simply absent. Enabled per client by SDKs that assemble transactions
// from the live span tree on root-span end (e.g. the Node SDK), which would otherwise drop children
// that close after it. Every other setup keeps its synchronous capture.
// Clients that opted into deferred segment-span capture (see `_INTERNAL_se
tDeferSegmentSpanCapture`),
// mapped to the function that queues a deferred capture. Keyed by client r
ather than a process-wide
// flag so pending captures and their timer cannot leak across `Sentry.init
()` calls.

just a suggestion but I am wondering if we could cut this comment down a bit, reads quite lengthy 😅

const DEFERRED_SEGMENT_SPAN_CAPTURES = new WeakMap<Client, (capture: () => void) => void>();

// Spans already included in a captured transaction. Used so a child that ends after its root segment
// was captured can be emitted as its own orphan transaction (see `_onSpanEnded`) without any span ever
// being sent in more than one transaction.
Comment on lines +65 to +67

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Spans already included in a captured transaction. Used so a child that ends after its root segment
// was captured can be emitted as its own orphan transaction (see `_onSpanEnded`) without any span ever
// being sent in more than one transaction.
// Track spans already sent in a transaction, so we can emit spans ending after the root in a separate transaction

const CAPTURED_SPANS = new WeakSet<Span>();

/**
* Defer a client's segment-span transaction capture. Set once by the SDK during setup (e.g. the Node

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, maybe we can cut this down a bit

* SDK); see {@link DEFERRED_SEGMENT_SPAN_CAPTURES}. Idempotent, and deferral stays on for the client's
* lifetime (there is no opt-out: deferral is a set-once-at-setup property, never toggled mid-session).
*
* The transaction is otherwise assembled from the live span tree the instant a root span ends, which
* drops children whose async instrumentation closes them later (a diagnostics-channel `asyncEnd`
* callback in the same tick, or engine spans replayed on a later tick). A debounced timer (the same one
* the OpenTelemetry span exporter uses) delays the snapshot just enough for those later span ends to
* land first. Pending captures are drained synchronously on the client's `flush` hook so
* `Sentry.flush()` / `client.close()` cannot resolve before they run.
*/
export function _INTERNAL_setDeferSegmentSpanCapture(client: Client): void {
if (DEFERRED_SEGMENT_SPAN_CAPTURES.has(client)) {
return;
}

const pendingCaptures = new Set<() => void>();
const debouncedDrain = debounce(
() => {
const captures = [...pendingCaptures];
pendingCaptures.clear();
for (const capture of captures) {
capture();
}
},
1,
{ maxWait: 100 },
);
Comment thread
cursor[bot] marked this conversation as resolved.

client.on('flush', () => {
debouncedDrain.flush();
});
Comment thread
cursor[bot] marked this conversation as resolved.

DEFERRED_SEGMENT_SPAN_CAPTURES.set(client, capture => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed offline: maybe we can think about simplifying this by not doing this per client or using an explicit queue class or something like that. if not also fine just think this is quite hard to understand at first pass

pendingCaptures.add(capture);
debouncedDrain();
});
}
Comment thread
andreiborza marked this conversation as resolved.

/**
* Span contains all data about a span
*/
Expand Down Expand Up @@ -343,9 +399,24 @@ export class SentrySpan implements Span {
// A segment span is basically the root span of a local span tree.
// So for now, this is either what we previously refer to as the root span,
// or a standalone span.
const isSegmentSpan = this._isStandaloneSpan || this === getRootSpan(this);

if (!isSegmentSpan) {
const rootSpan = getRootSpan(this);
const isSegmentSpan = this._isStandaloneSpan || this === rootSpan;

// A child span that ends after its root segment's transaction was already captured can no longer be
// part of it. Mirror the OpenTelemetry span exporter, which emits such a late child as its own
// (orphan) transaction in the same trace instead of dropping it. Only for clients that defer the
// segment capture (the SentryTracerProvider, the no-exporter native-assembly path); other setups
// keep the synchronous drop. `CAPTURED_SPANS` is only populated during a non-streaming capture, so
// this stays inert under span streaming (where late children stream individually).
const isOrphanSegment =
!isSegmentSpan &&
!!client &&
!!DEFERRED_SEGMENT_SPAN_CAPTURES.get(client) &&
!isBrowser() &&
!CAPTURED_SPANS.has(this) &&
CAPTURED_SPANS.has(rootSpan);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: If a root span is a SentryNonRecordingSpan, the check CAPTURED_SPANS.has(rootSpan) will always be false, silently disabling orphan segment handling for its children.
Severity: MEDIUM

Suggested Fix

The check for orphan segments should not depend on the root span being a SentrySpan that has been converted to a transaction. Re-evaluate the CAPTURED_SPANS.has(rootSpan) condition. The logic could be adjusted to correctly identify orphans even when the root span is non-recording, perhaps by tracking sent transactions differently.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: packages/core/src/tracing/sentrySpan.ts#L417

Potential issue: The logic to detect orphan segments relies on checking if the root span
is present in the `CAPTURED_SPANS` set. However, a span is only added to this set when
`_convertSpanToTransaction` is called, a method exclusive to `SentrySpan`. If the root
span of a trace is a `SentryNonRecordingSpan`, it will never be added to
`CAPTURED_SPANS`. Consequently, any child `SentrySpan` that finishes after the root will
fail the `isOrphanSegment` check at `sentrySpan.ts:417`, and its data will be lost
instead of being sent as an orphan segment.


if (!isSegmentSpan && !isOrphanSegment) {
return;
}

Expand All @@ -367,17 +438,38 @@ export class SentrySpan implements Span {
return;
}

const transactionEvent = this._convertSpanToTransaction();
if (transactionEvent) {
const scope = getCapturedScopesOnSpan(this).scope || getCurrentScope();
scope.captureEvent(transactionEvent);
const scope = getCapturedScopesOnSpan(this).scope || getCurrentScope();

// The transaction is assembled synchronously from the live span tree the instant the root span
// ends, dropping children whose async instrumentation closes them after it (a diagnostics-channel
// `asyncEnd` callback in the same tick, or engine spans replayed on a later tick). Clients that
// opted in defer the snapshot via a debounced timer so those later span ends land first; every
// other setup keeps its synchronous capture. Never deferred in the browser, where there is no such
// pattern and a deferred capture could be lost on page unload.
const deferCapture = client && DEFERRED_SEGMENT_SPAN_CAPTURES.get(client);
if (client && deferCapture && !isBrowser()) {
deferCapture(() => {
const transactionEvent = this._convertSpanToTransaction({ orphanedFromSentParent: isOrphanSegment });
if (transactionEvent) {
// Capture through the client resolved when the span ended, not the scope: a capture that
// fires on a later tick must reach the client active at span end and never whatever client
// is current when the timer fires (e.g. a different client after re-init), and the scope's
// client reference can be reassigned. Only the snapshot is deferred, so late children land.
client.captureEvent(transactionEvent, undefined, scope);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: the comment says do not resolve from the scope but then you pass the scope here, how does that work?

}
});
} else {
const transactionEvent = this._convertSpanToTransaction();
if (transactionEvent) {
scope.captureEvent(transactionEvent);
}
}
}

/**
* Finish the transaction & prepare the event to send to Sentry.
*/
private _convertSpanToTransaction(): TransactionEvent | undefined {
private _convertSpanToTransaction(options: { orphanedFromSentParent?: boolean } = {}): TransactionEvent | undefined {
// We can only convert finished spans
if (!isFullFinishedSpan(spanToJSON(this))) {
return undefined;
Expand All @@ -396,10 +488,22 @@ export class SentrySpan implements Span {
return undefined;
}

// The transaction span itself as well as any potential standalone spans should be filtered out
const finishedSpans = getSpanDescendants(this).filter(span => span !== this && !isStandaloneSpan(span));

const spans = finishedSpans.map(span => spanToJSON(span)).filter(isFullFinishedSpan);
// Skip the transaction span itself, standalone spans, and spans already sent in another transaction.
// Marking everything we send as captured lets a child that ends later be emitted as its own orphan
// transaction (see `_onSpanEnded`) instead of being dropped or sent twice.
CAPTURED_SPANS.add(this);
Comment thread
sentry[bot] marked this conversation as resolved.
const spans: SpanJSON[] = [];
for (const descendant of getSpanDescendants(this)) {
if (descendant === this || isStandaloneSpan(descendant) || CAPTURED_SPANS.has(descendant)) {
continue;
}
const spanJSON = spanToJSON(descendant);
if (!isFullFinishedSpan(spanJSON)) {
continue;
}
CAPTURED_SPANS.add(descendant);
spans.push(spanJSON);
}

const source = this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE];

Expand Down Expand Up @@ -443,6 +547,12 @@ export class SentrySpan implements Span {
}),
};

// Mirror the OpenTelemetry span exporter: tag a transaction whose parent span was already sent (an
// orphan emitted from `_onSpanEnded`) so it can be distinguished downstream.
if (options.orphanedFromSentParent && transaction.contexts?.trace?.data) {
transaction.contexts.trace.data['sentry.parent_span_already_sent'] = true;
}

const measurements = timedEventsToMeasurements(this._events);
const hasMeasurements = measurements && Object.keys(measurements).length;

Expand Down
14 changes: 13 additions & 1 deletion packages/core/test/lib/tracing/sentrySpan.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { describe, expect, it, test, vi } from 'vitest';
import { getCurrentScope } from '../../../src/currentScopes';
import { setCurrentClient } from '../../../src/sdk';
import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '../../../src/semanticAttributes';
import { SentrySpan } from '../../../src/tracing/sentrySpan';
import { _INTERNAL_setDeferSegmentSpanCapture, SentrySpan } from '../../../src/tracing/sentrySpan';
import { SPAN_STATUS_ERROR } from '../../../src/tracing/spanstatus';
import { markSpanForOtelSourceInference, spanSourceWasExplicitlySet } from '../../../src/tracing/utils';
import type { SpanJSON } from '../../../src/types/span';
Expand Down Expand Up @@ -132,6 +132,18 @@ describe('SentrySpan', () => {
});
});

describe('_INTERNAL_setDeferSegmentSpanCapture', () => {
it('registers the flush listener once and is idempotent on repeated enable', () => {
const client = new TestClient(getDefaultTestClientOptions());
const onSpy = vi.spyOn(client, 'on');

_INTERNAL_setDeferSegmentSpanCapture(client);
_INTERNAL_setDeferSegmentSpanCapture(client);

expect(onSpy.mock.calls.filter(([hook]) => hook === 'flush')).toHaveLength(1);
});
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing defer behavior tests

Low Severity

This feat change adds deferred transaction capture and orphan emission, but the diff only adds a unit test that the flush hook registers once. There is no integration or E2E test here exercising deferred assembly, flush draining, or orphan transactions.

Fix in Cursor Fix in Web

Triggered by project rule: PR Review Guidelines for Cursor Bot

Reviewed by Cursor Bugbot for commit 2df53ad. Configure here.


describe('end', () => {
test('simple', () => {
const span = new SentrySpan({});
Expand Down
Loading