Fix baggage header merging and networkContext propagation in TracingURLSessionHandler#2683
Fix baggage header merging and networkContext propagation in TracingURLSessionHandler#2683Valpertui wants to merge 5 commits into
Conversation
20320e8 to
21e6420
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21e6420a28
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| request.setValue(mergedValue, forHTTPHeaderField: field) | ||
| } else { | ||
| request.setValue(value, forHTTPHeaderField: field) | ||
| } | ||
| hasSetAnyHeader = true |
There was a problem hiding this comment.
Don’t return TraceContext when only baggage is injected
When a request already has W3C traceparent/tracestate set by the caller, this branch still sets hasSetAnyHeader = true after merging (or adding) baggage. That causes modify() to return a TraceContext, which NetworkInstrumentationFeature registers and later uses to build spans. The outgoing request keeps the caller’s original trace headers, so the span created from the returned TraceContext can end up on a different trace ID than the actual request, breaking distributed trace correlation whenever the app supplies its own trace headers but expects the SDK to only merge baggage. Consider tracking “trace header injected” separately from “baggage injected,” or avoid returning TraceContext when only baggage was modified.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I believe this is normal that there is a trace context returned if we inject any header.
But currently if at least 1 header doesn't get overwritten because it's already there (eg traceparent), we will propagate a traceContext with traceID and spanID set back to the SDK. While the network request sent will not contain those specific traceID/spanID, breaking distributed tracing in an unexpected way.
Sure it looks like a developer mistake to fix. But we need to remember that those header fields could also be set automatically by other integrations like Datadog.
Should we keep the current behavior? Should we revamp it ?
793327a to
f7649d3
Compare
This comment has been minimized.
This comment has been minimized.
BaggageHeaderMerger is needed by TracingURLSessionHandler for merging W3C baggage headers during trace context injection. Move it to DatadogInternal so it can be used across modules. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nHandler Two changes to the modify() method: 1. Merge W3C baggage headers instead of skipping them when already present on the request. This uses BaggageHeaderMerger to combine existing baggage values with SDK-injected ones (session.id, user.id, account.id), with new values taking precedence. 2. Use the networkContext parameter for populating rumSessionId, userId, and accountId in the TraceContext, falling back to contextReceiver.context. Previously networkContext was accepted but ignored, so these values were never propagated into baggage headers when provided via networkContext. The traceContext nil assertion in the "does not overwrite headers" test was removed because the presence of a returned traceContext depends on whether any header was set (e.g. baggage), which is unrelated to the non-overwriting behavior that test verifies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f7649d3 to
86d019d
Compare
What and why?
TracingURLSessionHandler.modify()had two issues:W3C baggage headers were not merged — when a request already had a
baggageheader, the handler skipped it entirely (same "do not overwrite" logic as trace propagation headers). Baggage headers should be merged instead, combining existing user-set values with SDK-injected ones (session.id,user.id,account.id).networkContextparameter was ignored — themodify()method accepted aNetworkContext?parameter but never used it for populatingrumSessionId,userId, andaccountIdin theTraceContext. It read exclusively fromcontextReceiver.context, so values provided vianetworkContextwere never propagated into baggage headers.How?
BaggageHeaderMergerfromDatadogRUMtoDatadogInternalso it can be used byTracingURLSessionHandler.modify(): when a writer produces abaggageheader, merge it with any existing value usingBaggageHeaderMergerinstead of skipping.networkContextas the primary source forrumSessionId,userId, andaccountIdwhen creating theTraceContext, falling back tocontextReceiver.context.Review checklist
make api-surfacewhen adding new APIs