RUM-15138: Ignore dropped local active span in RUM-to-APM path#3406
Conversation
e575fc2 to
2135aec
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3406 +/- ##
===========================================
+ Coverage 72.09% 72.14% +0.05%
===========================================
Files 962 962
Lines 35440 35491 +51
Branches 5895 5919 +24
===========================================
+ Hits 25548 25602 +54
+ Misses 8274 8264 -10
- Partials 1618 1625 +7
🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2135aec15d
ℹ️ 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".
| } | ||
|
|
||
| private fun isParentDropped(tracer: DatadogTracer, explicitParent: DatadogSpanContext?): Boolean { | ||
| val priority = explicitParent?.samplingPriority ?: tracer.activeSpan()?.samplingPriority |
There was a problem hiding this comment.
In the RUM/headerPropagationOnly path, an active manual span normally has samplingPriority == null until propagation forces the trace sampler (CorePropagation.inject() calls setSamplingPriorityIfNecessary()). With that state this check treats the active parent as not dropped, so the network span remains its child; when updateRequest() later injects headers, the APM sampler can still set SAMPLER_DROP and propagate the dropped trace, which is the scenario this change is meant to prevent. Please force/read the active parent's sampling decision before deciding not to ignore it, and apply the same fix to the duplicated helper in ApmNetworkInstrumentationExt.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Looks like a valid concern.
Manual spans created via the Trace SDK have samplingPriority = UNSET until propagation forces the decision (CorePropagation.inject() → setSamplingPriorityIfNecessary()), so isParentDropped was always returning false for them — defeating the purpose of this change.
Commit 647ed6d fixes this by calling _TraceInternalProxy.setTracingSamplingPriorityIfNecessary(parentContext) before reading the priority, which is the same forcing mechanism already used in extractSamplingDecision and during inject(). It's CAS-guarded and idempotent so I think it should be safe. Applied in both TracingInterceptor.isParentDropped() and ApmNetworkInstrumentationExt.isParentDropped().
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 647ed6d786
ℹ️ 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".
| ignoreDroppedParent: Boolean | ||
| ): DatadogSpan { | ||
| val parentContext = propagationHelper.extractParentContext(this, request) | ||
| val shouldIgnoreParent = ignoreDroppedParent && isParentDropped(this, parentContext) |
There was a problem hiding this comment.
When the dropped parent was extracted from request headers or a request tag, parentContext is non-null so this branch ignores it, but DatadogSpan.sample(request) still calls propagationHelper.extractSamplingDecision(request) and rereads the same DROP decision before traceSampler can run; the OkHttp path added the same pattern. In the RUM/header-propagation path this means RUM still cannot make an independent sampling decision for dropped extracted parents, so correlation is still lost except for active-span parents with no request-carried context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I've being thinking about this comment because it's a bit complex and I think, it describes a scenario where tracing headers on the request (e.g., x-datadog-sampling-priority: 0) re-poison the sampling decision after we ignore a dropped parent.
However, our PR specifically targets the case where a local active manual span from the Trace SDK is dropped and drags down RUM network spans — in that scenario, the request carries no tracing headers, so extractSamplingDecision(request) returns null and the traceSampler runs independently as intended. If a request does carry explicit DROP headers, that represents a deliberate upstream decision about that specific trace, not the accidental inheritance from an unrelated manual span — respecting that DROP looks like is correct behavior.
Anyway, happy to listen to other opinions.
37c5f51 to
ff4f037
Compare
647ed6d to
e849395
Compare
hamorillo
changed the title
45062e0 to
7c7046f
Compare
…ow independent sampling
The check for a dropped parent in the RUM-to-APM path read samplingPriority on the active span without first resolving it. Manual spans created via the Trace SDK keep their priority as UNSET until propagation/inject runs the sampler, so the check would treat them as not-dropped, attach the network span as a child, and then watch the APM sampler propagate a DROP at inject time — exactly the scenario this code is meant to prevent. isParentDropped now calls _TraceInternalProxy.setTracingSamplingPriorityIfNecessary on the parent context before reading its priority. This is the same forcing mechanism already used in extractSamplingDecision and during inject; it is idempotent (CAS-guarded) and produces the exact decision the trace would have received later. Same fix applied in TracingInterceptor (okhttp) and ApmNetworkInstrumentationExt (trace). Existing tests updated to stub the parent context's priority (not just the span's), and new tests cover the "context resolves to DROP/KEEP after forcing" cases.
e849395 to
6f2854c
Compare
|
Are you sure it won't break RUM-8051? worth to discuss with @0xnm as well |
| val activeContext = if (explicitParent != null) null else tracer.activeSpan()?.context() | ||
| // Force resolution of the active span's sampling priority — a manual span backed | ||
| // by a PendingTrace can read UNSET until the sampler commits at inject time. | ||
| activeContext?.let { _TraceInternalProxy.setTracingSamplingPriorityIfNecessary(it) } |
There was a problem hiding this comment.
this is a side-effect of the function and it's unclear from the caller side. we should highlight that this priority would be set in a function name, or (it's better) to move this logic out of this getter
There was a problem hiding this comment.
Good point. I've refactored this.
Renames ignoreDroppedParent to ignoreLocalDroppedParent so the flag name matches its actual scope (only local active spans can be ignored), and lifts the setTracingSamplingPriorityIfNecessary side effect out of the predicate so it is visible at the buildSpan call site, mirroring the pattern in extractSamplingDecision. The isParentDropped helper is replaced by a pure DatadogSpanContext?.isDropped() extension.
hamorillo
left a comment
hamorillo
left a comment
There was a problem hiding this comment.
Are you sure it won't break RUM-8051? worth to discuss with @0xnm as well
@satween Good question. I would say that's the main goal of this PR. When there is an active span (and trace) from APM that is not sampled, RUM should ignore it and act as if the trace did not exist. Basically, it should make its own decision. This is the Android counterpart of iOS RUM-15310 (DataDog/dd-sdk-ios#2807).
In any other cases, the implementation from RUM-8051 should remain identical.
Anyway, let's wait for @0xnm's insight.
| val activeContext = if (explicitParent != null) null else tracer.activeSpan()?.context() | ||
| // Force resolution of the active span's sampling priority — a manual span backed | ||
| // by a PendingTrace can read UNSET until the sampler commits at inject time. | ||
| activeContext?.let { _TraceInternalProxy.setTracingSamplingPriorityIfNecessary(it) } |
There was a problem hiding this comment.
Good point. I've refactored this.
| // Only consult the local active span. Explicit parents (request tags or propagated | ||
| // headers) represent developer intent and must be honored regardless of priority. |
There was a problem hiding this comment.
Is it the same on iOS? The spec written by @qsellem says:
But the spans are not attached to the manually created trace and so the sampling decision is taken by the RUM SDK, not the Trace SDK
I guess there is a blind spot here, because this PR says that spans added by something like Request.Builder.addParentSpan are unaffected.
Normally, this becomes a change maybe for Cronet only then? Because in case of OkHttp active span is thread-local and this code is going to be executed from the thread created by OkHttp, if enqueue is used (so customer cannot associate its own span with this thread unless done somewhere in the upstream interceptor).
When a customer attaches a DatadogSpan or TraceContext as a parent via Request.Builder.parentSpan(span) (OkHttp) or addRequestAnnotation(span) (Cronet), the resulting tag-attached parent is now subject to the same dropped-parent ignore logic as the local active span on the RUM-to-APM path. This aligns Android's behavior with iOS RUM-15310 for the case of an explicit developer-attached parent that gets sampled out by the Trace SDK. The change spans two coordinated layers. First, extractParentContext returns a ParentContextSource sealed type (FromHeaders or FromTag) so buildSpan can discriminate the source and call ignoreActiveSpan() when a tag parent is dropped. Second, extractSamplingDecision now takes an ignoreLocalDroppedParent flag (threaded through DatadogSpan.sample(...) from canSendSpan) that returns null for dropped tag parents so the trace sampler decides instead of inheriting the drop. Inbound trace headers remain authoritative in both layers to preserve head-based sampling of upstream propagation.
| private fun DatadogSpanContext?.isDropped(): Boolean { | ||
| val priority = this?.samplingPriority | ||
| return priority.isDroppedPriority() | ||
| } | ||
|
|
||
| private fun Int?.isDroppedPriority(): Boolean = | ||
| this == PrioritySampling.SAMPLER_DROP || this == PrioritySampling.USER_DROP |
There was a problem hiding this comment.
there is duplication between here and similar code in ApmNetworkInstrumentation, but I guess it will be gone at some point with further changes.
There was a problem hiding this comment.
I was aware about it but yeah, let's remove it. I've refactor a bit the code in d7dd316
This comment has been minimized.
This comment has been minimized.
5 participants
What does this PR do?
When a local manual span (created via the Trace SDK) is active but has been dropped by the trace sampler, the RUM-to-APM path (DatadogInterceptor /
headerPropagationOnly=true) now ignores it and creates an independent trace with its own sampling decision. This prevents the Trace SDK's drop decision from killing RUM-APM correlation for network requests where the RUM SDK would have sampled the trace.The fix only affects the case where the dropped parent is a local active span with no explicit parent context. Explicit parent contexts — set via
Request.Builder().parentSpan(...), request tags, or inbound trace propagation headers (Datadog, W3C, B3) — are always honored regardless of their sampling priority. This preserves distributed-trace continuity when an upstream service or developer code has explicitly chosen a parent.Motivation
When a customer creates a manual span via the Trace SDK that gets dropped (e.g., 5% trace rate), and then makes a network request through DatadogInterceptor, HBS previously propagated the drop — the RUM resource lost all trace correlation. The customer configured the RUM SDK to sample at a higher rate, but the Trace SDK's lower rate overrode it. This change lets the RUM SDK make its own independent sampling decision in that scenario, while still honoring any explicit parent the developer or upstream service has propagated.
Behavior matrix (RUM path only —
canSendSpan()=false)tracer.activeSpan())SAMPLER_DROP/USER_DROPSAMPLER_KEEP/USER_KEEP/UNSETRequest.Builder().parentSpan(...))The
TracingInterceptorAPM-only path (canSendSpan()=true) is unaffected — HBS still propagates dropped parents as before.Implementation Notes
TracingInterceptor.buildSpan()andApmNetworkInstrumentationExt.buildSpan()callisParentDropped(), which inspects only the local active span when no explicit parent context is present. When that active span resolves toSAMPLER_DROPorUSER_DROP,ignoreActiveSpan()is called on the span builder, creating a root span with a fresh trace ID.PendingTracecan carryUNSETpriority until propagation / inject runs the sampler. Without forcing resolution, the check would treat such a span as not-dropped, attach the network span as a child, and then watch the APM sampler propagate aDROPat inject time — exactly the scenario this code is meant to prevent.isParentDroppedcalls_TraceInternalProxy.setTracingSamplingPriorityIfNecessaryon the active span's context before reading its priority. This is the same forcing mechanism already used inextractSamplingDecisionand during inject; it is idempotent (CAS-guarded) and produces the exact decision the trace would have received later.isParentDroppedhelpers areprivate.SessionRebasedSamplerfrom that PR is what makes the independent sampling decision meaningful.Review checklist (to be filled by reviewers)