Uh oh!
There was an error while loading. Please reload this page.
fix(core): Allow non-recording spans to carry explicit sampling decisions - #21406
fix(core): Allow non-recording spans to carry explicit sampling decisions#21406andreiborza wants to merge 15 commits into
Conversation
Uh oh!
There was an error while loading. Please reload this page.
size-limit report 📦
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
9a455d0 to
69d87c1Compare| * @internal | ||
| */ | ||
| public recordException(_exception: unknown, _time?: number | undefined): void { | ||
| public recordException(_exception: unknown, _time?: SpanTimeInput | undefined): void { |
There was a problem hiding this comment.
This is a widening change to the more correct SpanTimeInput type, it contains number.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
isaacs
left a comment
There was a problem hiding this comment.
Found some small suggestions that might improve or tidy it up, defend against future mistakes, etc. But generally, this looks great :)
| dropUndefinedKeys({ | ||
| ...getDynamicSamplingContextFromSpan(span), | ||
| transaction: source === 'url' ? undefined : spanArguments.name, | ||
| })) satisfies Partial<DynamicSamplingContext>; |
There was a problem hiding this comment.
This block is almost identical with the bit in packages/core/src/tracing/idleSpan.ts, looks like they only differ in the default name. Can that be abstracted out to a helper function, like a fancier version of freezeDscOnSpan for TwP root spans?
| return new SentryNonRecordingSpan({ | ||
| dropReason: 'ignored', | ||
| sampled: false, |
There was a problem hiding this comment.
The root span leaves sampled: undefined, and this one (intentionally) sets it false. It might be a good idea to add a comment so that we don't come along later and ""fix"" the inconsistency. (Also, below, on line 563.)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…ions Non-recording spans can now carry a `sampled` flag and `parentSpanId` on their span context, so `spanToTraceHeader`/`spanToTraceparentHeader` and `spanToJSON` reflect the real decision. In Tracing without Performance mode, root non-recording spans keep the sampling decision deferred (no `sentry-trace` flag, no `sentry-sampled`/`sample_rate` in the DSC) instead of asserting a negative decision that would suppress downstream sampling. Spans created for an unsampled trace (or ignored spans) carry an explicit `sampled: false`.
b539317 to
07dfbf4Compare
Lms24
left a comment
There was a problem hiding this comment.
Just some minor qs and a request for streamed spans. Good change overall (even in isolation without the traceprovider switch :) )
| event: Event, | ||
| { includeSampleRand = false, sdk = 'cloudflare' }: { includeSampleRand?: boolean; sdk?: 'cloudflare' | 'hono' } = {}, | ||
| { | ||
| includeSamplingFields = false, |
There was a problem hiding this comment.
super-l: Not a fan of this tbh as it still limits what we're expecting in the specific values. Why not directly assert on the envelope headers?
This is fine for the PR though, so no need to change it :)
There was a problem hiding this comment.
Agreed, it would be better to assert actual values.
I had a go at this but it snowballs quite a bit so I'll extract that out into a separate PR later.
| * Sentry-specific sampling decision for this span context. | ||
| * `undefined` means no local sampling decision was made yet. | ||
| */ | ||
| sampled?: boolean | undefined; |
There was a problem hiding this comment.
l: not opposed to this but just to double check: We're fine with diverging from OTel here? My understanding is we can do it because our tracer implementation will just create SentrySpans, so this should be fine.
There was a problem hiding this comment.
Right, I walked back on this and go for the same _sampled + tracestate flag approach to be in line with SentrySpan.
| return { | ||
| span_id, | ||
| trace_id, | ||
| parent_span_id: (span as { parentSpanId?: string }).parentSpanId, |
There was a problem hiding this comment.
m: I think we can also update this on spanToStreamedSpanJSON, right?
| startSpan({ name: 'ignored-child' }, span => { | ||
| expect(span).toBeInstanceOf(SentryNonRecordingSpan); | ||
| // The ignored span still links to its parent so `spanToJSON` can surface it. | ||
| expect(spanToJSON(span).parent_span_id).toBe(rootSpan.spanContext().spanId); |
There was a problem hiding this comment.
m: spanToJSON does not change based on traceLifecycle: stream. i think we need to assert against spanToStreamedSpanJSON here.
| // In TwP mode, a new trace's sampling decision stays deferred (like `startSpan`) while a | ||
| // continued trace carries the upstream decision, so baggage and the `sentry-trace` header | ||
| // agree. Idle spans are always trace roots, so we freeze the DSC here. | ||
| freezeDscOnTwpRootSpan(span, { |
There was a problem hiding this comment.
I am not a fan of Twp as it is not super clear. Can we just call this e.g. freezeDscOnRootSpanWithoutSampling or something along these lines?
There was a problem hiding this comment.
Right, renamed to freezeDscOnRootSpanWithoutSampling in ba51d24
| spanId: this._spanId, | ||
| traceId: this._traceId, | ||
| traceFlags: TRACE_FLAG_NONE, | ||
| traceFlags: this._sampled ? TRACE_FLAG_SAMPLED : TRACE_FLAG_NONE, |
There was a problem hiding this comment.
This is weird and Imho not ideal, that a non recording span can be sampled = true? We should enforce that this is either false or undefined. If it is true we should not create a non recording span I suppose...
There was a problem hiding this comment.
I reworked this to be in line with SentrySpan, using _sampled + a flag in trace state and using the same helpers across both span types in ba51d24
| parentSpanId?: string; | ||
| sampled?: boolean; | ||
| dsc?: Partial<DynamicSamplingContext>; | ||
| } = parentSpan |
There was a problem hiding this comment.
Can we not use an existing utility like spantodsc or similar here?
There was a problem hiding this comment.
Updated in ba51d24, if I understand your comment correctly 😅
| ...getDynamicSamplingContextFromSpan(span), | ||
| } satisfies Partial<DynamicSamplingContext>; | ||
| freezeDscOnSpan(span, dsc); | ||
| freezeDscOnTwpRootSpan(span, { |
There was a problem hiding this comment.
This is not a twp span, is it? Just a forced transaction?
Just verify correct span nesting for redis
ca1ccfb to
ba51d24Compareandreiborza
commented
Jun 14, 2026
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d5887e7. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
andreiborza
commented
Jun 15, 2026
Closing in favor of #21549 which keeps SentryNonRecordingSpan a thin container and uses the scope to handle the sampling decision in cases where it's needed. |
…21549) 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), so the outgoing `sentry-trace` header omits the flag instead of asserting `-0`. The span id comes from the scope's `propagationSpanId` (a fresh id is generated when the scope has none). - `getDynamicSamplingContextFromSpan` resolves 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 `-0` via `spanToTraceHeader`. 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. This is an alternative to #21406
What
Non-recording spans can now carry a
sampledflag andparentSpanIdon their span context, sospanToTraceHeader/spanToTraceparentHeaderandspanToJSONreflect the real decision.In Tracing without Performance mode, root non-recording spans keep the sampling decision deferred (no
sentry-traceflag, nosentry-sampled/sample_ratein the DSC) instead of asserting a negative decision that would suppress downstream sampling.Spans created for an unsampled trace (or ignored spans) carry an explicit
sampled: false.Placeholder/idle spans also inherit
traceId/parentSpanIdfrom the propagation context and capture their scopes.Why
These changes are important for when we switch over to our own TracerProvider and Tracer because we will create native Sentry spans (including
SentryNonRecordingSpan) and they need to be on par with the previously used OTel spans.