fix: Propagate the incoming Dynamic Sampling Context when starting a transaction from ContinueTrace - #5569
fix: Propagate the incoming Dynamic Sampling Context when starting a transaction from ContinueTrace#5569cuva wants to merge 2 commits into
Conversation
…transaction from ContinueTrace ContinueTrace parses the baggage header into a DynamicSamplingContext and stores it on the scope's propagation context, but the public StartTransaction overload always passed null as the DSC to the internal overload. A transaction started from the returned TransactionContext therefore created a fresh DSC from itself, with a locally generated sample_rand and the local public_key, so Relay could reach a different keep-or-drop decision for it than for the rest of the trace. Only Sentry.AspNetCore honoured the incoming DSC, through the internal overload. Attach the DSC parsed from the baggage to the TransactionContext returned by ContinueTrace (internal property) and have the public StartTransaction overload pass it on, so the incoming DSC is propagated unchanged as the spec requires. Without a baggage header, or when the baggage yields no valid DSC, the behaviour is unchanged.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
…ader A sentry-trace header without a baggage header comes from an SDK without dynamic sampling support. The ASP.NET Core middleware freezes the DSC as empty in that case, as the spec requires, so ContinueTrace now does the same instead of letting the transaction create a DSC from itself. Reuse the propagation context's parsed DSC via Clone() rather than parsing the baggage a second time.
| DynamicSamplingContext = (traceHeader, baggageHeader) switch | ||
| { | ||
| (null, _) => null, | ||
| (_, null) => DynamicSamplingContext.Empty(), |
There was a problem hiding this comment.
Bug: An empty DynamicSamplingContext from ContinueTrace is incorrectly mutated in StartTransaction, violating the spec's requirement for it to remain frozen.
Severity: MEDIUM
Suggested Fix
Prevent the mutation of the empty DynamicSamplingContext within the StartTransaction overload. Before calling SetSampleRate() or SetReplayId(), add a check to ensure the context is not the specific 'frozen' empty instance that is created when a trace header is received without baggage.
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: src/Sentry/Internal/Hub.cs#L422
Potential issue: When `ContinueTrace` is called with a `sentry-trace` header but no
`baggage` header, an empty `DynamicSamplingContext` is created. This context is intended
to be treated as frozen, per Sentry specification, to ensure compatibility with older
SDKs. However, the internal `StartTransaction` overload later mutates this context by
calling `SetSampleRate()` if a global `TracesSampleRate` is configured. This
modification violates the requirement that a received Dynamic Sampling Context (DSC) be
propagated unchanged, causing the context to no longer be empty and breaking the
'frozen' state assumption.
Also affects:
src/Sentry/Internal/Hub.cs:171~178
There was a problem hiding this comment.
This one is pre-existing behaviour of the internal StartTransaction overload rather than something this PR introduces. SentryTracingMiddleware has passed DynamicSamplingContext.Empty() into the same overload since the freeze was added, and writing sample_rate onto a provided DSC once a sampling decision is made was done deliberately in #4374. StartTransaction_DynamicSamplingContextWithoutSampleRand_SampleRandNotPropagated covers exactly this path with an empty DSC and only requires that sample_rand is not added.
Changing that here would also change the ASP.NET Core behaviour, so I'd rather keep this PR limited to making ContinueTrace + StartTransaction(context) behave like the middleware. Happy to follow up separately if the maintainers want the empty DSC to stay strictly untouched in both paths.
I'm not sure that's true... that PR was closed - never merged. To be honest, I'm not sure if we have an explicit way to continue traces from a non-HTTP boundary. The sentry-dotnet/src/Sentry/Internal/Hub.cs Lines 677 to 678 in 9683c36 Propagating from non-HTTP boundaries should be possible but it requires a bit of investigation/planning. I won't have time for this immediately as we need to get the SDK ready for .NET 11. I also don't have time to review any moderately complex PRs at the moment. I've created the following issue so that we can priorities and schedule in this work once the work for .NET 11 is complete: |
|
Closing for now. Will reopen if this PR turns out to be an appropriate solution when we implement this. |
2 participants
Description
SentrySdk.ContinueTrace(traceHeader, baggageHeader)followed bySentrySdk.StartTransaction(context)is the documented way to continue a distributed trace from a non-HTTP boundary (#2646). The baggage is parsed into aDynamicSamplingContextand stored on the scope's propagation context, butHub.StartTransaction(ITransactionContext, customSamplingContext)always passesnullas the DSC to the internal overload, so the transaction creates a fresh DSC from itself: a locally generatedsample_rand, the localpublic_key, the localsample_rate, and so on. The incoming DSC is only honoured bySentry.AspNetCore, which reaches the internalStartTransaction(context, customSamplingContext, dynamicSamplingContext)overload throughInternalsVisibleTo.This change lets
ContinueTraceattach the DSC parsed from the baggage to theTransactionContextit returns (as an internal property), and makes the publicStartTransaction(context, customSamplingContext)overload pass that DSC to the internal overload. A transaction started from a continued trace now propagates the incoming DSC unchanged, andsample_randfollows from it, exactly like the ASP.NET Core middleware already does. No public API is added or changed.When
ContinueTracereceives asentry-traceheader without abaggageheader, the request comes from an SDK without dynamic sampling support and the DSC is frozen as empty, again matching the ASP.NET Core middleware and the spec. When the baggage does not yield a valid DSC, or when there is no trace header to continue, the behaviour is unchanged and the transaction creates its own DSC.Motivation and Context
The dynamic sampling context spec says a received DSC MUST be treated as frozen and propagated unchanged, and the propagated random value section requires
sample_randto stay the same across a trace so that Relay reaches one consistent keep-or-drop decision. Today a .NET transaction that continues a trace from headers reports its ownsample_randandpublic_key, so Relay can sample it independently of the rest of the trace.Our use case is a desktop app where a web view (JavaScript SDK) starts the trace and the .NET host continues it for the requests it handles, the same situation as #4021. The only workaround is reflection into the internal overload, since
DynamicSamplingContext,BaggageHeader.TryParseand the DSC-takingStartTransactionoverload are all internal.Related: #2646, #4021, #3838.
How did you test it?
HubTests: sampled and sampled-out continued traces propagate the incoming DSC andsample_rand, the string-header overload does the same, a trace header without baggage freezes an empty DSC, and invalid baggage or no trace header still create a DSC from the transaction. The three baggage tests and the empty-freeze test fail onmainand pass with this change.dotnet test test/Sentry.Tests -f net10.0passes, including the public API approval tests (no public API change).Changelog Entry
TransactionContextreturned byContinueTrace, sosample_randand the other frozen DSC items stay consistent across the trace