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 13 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
size-limit report 📦
|
andreiborza
force-pushed
the
ab/nonrecording-span-sampling
branch
from
9a455d0 to
69d87c1
Compare
| * @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.
andreiborza
requested review from
JPeer264,
Lms24,
chargome,
logaretm,
mydea and
nicohrubec
and removed request for
a team and
JPeer264
isaacs
left a comment
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.)
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 c394cf7. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
…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`.
andreiborza
force-pushed
the
ab/nonrecording-span-sampling
branch
from
b539317 to
07dfbf4
Compare
Lms24
left a comment
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.
| 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?
| 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...
| 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?
| ...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?
5 participants
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.