fix(core): Defer TwP sampling by reading trace state from the scope#21549
fix(core): Defer TwP sampling by reading trace state from the scope#21549andreiborza wants to merge 5 commits into
Conversation
In Tracing-without-Performance (spans disabled), a root placeholder previously froze a negative sampling decision in the DSC, which suppressed downstream sampling instead of leaving the decision to a performance-enabled service further along the trace. The scope is the source of truth for a TwP placeholder's trace state: - `getTraceData` reads the sampling decision from the scope (deferred for a new trace, the upstream decision for a continued trace) while keeping the placeholder's stable span id, so the outgoing `sentry-trace` header omits the flag instead of asserting `-0`. - `getDynamicSamplingContextFromSpan` resolves a placeholder's DSC from its captured scope (continued traces keep the incoming DSC; new traces derive it from the client). A new (head-of-trace) TwP trace does not stamp a local `transaction` in its DSC; continued traces still propagate the upstream decision and DSC. No DSC is written to the scope at span start, preserving the browser's "scope stays DSC-free between navigations" behavior.
size-limit report 📦
|
|
|
||
| if (!client || !hasSpansEnabled()) { | ||
| const span = new SentryNonRecordingSpan(); | ||
| const propagationContext = { |
There was a problem hiding this comment.
I don't think this is actually necessary, do we ever put traceId/spanId/sampled/dsc on the isolation scope? I think this is always on the same type of scope, but not 100% sure...
There was a problem hiding this comment.
Right, we only ever need the traceId from there, which isn't affected. Removed in d1d9d2d
| // A non-recording span is a Tracing-without-Performance placeholder that carries no sampling | ||
| // decision of its own — the scope is the source of truth. We keep the placeholder's (stable) | ||
| // span id but read the sampling decision from the scope. | ||
| const isNonRecordingSpan = span instanceof SentryNonRecordingSpan; |
There was a problem hiding this comment.
this is fine but possibly brittle, if packages are badly deduped or similar. if we can find a different, non-identidy based way to check this that would possibly be more robust, but not necessarily required here IMHO
| traceData.traceparent = | ||
| span && !isNonRecordingSpan | ||
| ? spanToTraceparentHeader(span) | ||
| : scopeToTraceparentHeader(scope, span?.spanContext().spanId); |
There was a problem hiding this comment.
does this make sense? if it is non-recording the span.spanContext should not have a valid spanId, I guess...?
There was a problem hiding this comment.
It does have a valid id, but I simplified this to not pass a span id. This does mean in some cases the span id might differ, e.g. when starting a manual span in TwP
Sentry.startSpan({...}), () => {
Sentry.captureException(...);
fetch(...)
})The span id of startSpan and the error event trace are the same, but the fetch call ends up with a new span id. This shouldn't be a problem since we aren't sending spans anyway in this mode.
Updated in 6a7c654
| // For a non-recording placeholder (Tracing without Performance), the DSC is not carried on the | ||
| // span — the scope is the source of truth. Resolve it from the span's captured scope: continued | ||
| // traces keep the incoming DSC, new traces derive it from the client (without a local transaction). | ||
| if (rootSpan instanceof SentryNonRecordingSpan) { |
There was a problem hiding this comment.
do we need this, actually? Would we not skip even calling this if the span is a non recording span?
There was a problem hiding this comment.
Yes this is needed, this is the one unifying place where we can get the DSC from. This simplifies not having to add checks for SentryNonRecordingSpans at all other callsites (e.g. in getTraceData, applySpanToEvent etc).
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6a7c654. Configure here.
In Tracing-without-Performance (spans disabled), a root placeholder previously froze a negative sampling decision in the DSC, which suppressed downstream sampling instead of leaving the decision to a performance-enabled service further along the trace.
The scope is the source of truth for a TwP placeholder's trace state:
getTraceDatareads the sampling decision from the scope (deferred for a new trace, the upstream decision for acontinued trace), so the outgoing
sentry-traceheader omits the flag instead of asserting-0. The span id comes from the scope'spropagationSpanId(a fresh id is generated when the scope has none).getDynamicSamplingContextFromSpanresolves a placeholder's DSC from its captured scope (continued traces keep the incoming DSC; new traces derive it from the client).The scope is only consulted for genuine TwP placeholders. A non-recording span in tracing mode, the child of an unsampled span, or an ignored span carries an explicit negative decision and keeps propagating
-0viaspanToTraceHeader.A new (head-of-trace) TwP trace does not stamp a local
transactionin its DSC; continued traces still propagate the upstream decision and DSC.No DSC is written to the scope at span start, preserving the browser's "scope stays DSC-free between navigations" behavior.
This is an alternative to #21406