fix(core): Propagate sample_rand when continuing a trace without incoming baggage - #22980
Conversation
…ming baggage When a trace is continued from a `sentry-trace` header with no (or only 3rd-party) baggage, the frozen Dynamic Sampling Context is empty and previously dropped the `sample_rand`. Downstream services then received baggage without `sentry-sample_rand`, so sampling decisions could diverge across the trace. Fold the scope's `sample_rand` into the otherwise-empty DSC at freeze time; a populated incoming DSC is left untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 fbfdb3d. Configure here.
size-limit report 📦
|
…n tests Compare the frozen DSC's sample_rand against the scope's propagation-context value instead of only format-checking it, so an implementation that minted a fresh random would fail.
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
hmm does this solve anything? If we receive a sentry-trace w/o baggage, the head of trace is incompatible with DS or our baggage propagation mechanism. If we, now as a downstream SDK, freeze our sample rand onto the DSC, isn't this just as wrong? 😅
I'm referring to the dev spec where we defined that incoming sentry-trace but no baggage (or baggege but w/o sentry headers) should lead to instant baggage freezing.
|
@Lms24 this mimics how it behaves today when we have a tracer provider. The PR is preparation work for moving most SDKs off of using tracer providers at all and when doing so existing tests fail. Here's how this same behavior gets applied on develop, where it also ignores that a DSC might be frozen if I'm not mistaken? sentry-javascript/packages/core/src/tracing/dynamicSamplingContext.ts Lines 159 to 167 in f9c83f8 I think there might be a clash in spec, not sure tho 😬 For reference, here's the PR where not having this ended in test failures: #22957 It's not super apparent from this PR alone because tests are using tracer providers still. |
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Thanks for explaining! Makes sense to me to merge this for now for parity. We should follow up what the expected behaviour is for this case and adjust ours if necessary.
2 participants
What
Fold the scope's
sample_randinto the (otherwise empty) frozen Dynamic Sampling Context when continuing a trace that arrived with asentry-traceheader but no Sentry baggage.sentry-sample_rand.Why
Without the propagated
sample_rand, downstream services generate their own, so sampling decisions can diverge across a single trace. Propagating it keeps the decision consistent end to end.