RUM-17709: Apply remote configuration to Trace module at SDK instance ready - #3663
Conversation
|
@codex review |
![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: 9ae030be9e
ℹ️ 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".
| if (rcHosts.isEmpty()) { | ||
| return DefaultFirstPartyHostHeaderTypeResolver(emptyMap()) |
There was a problem hiding this comment.
When RC sends an explicit empty tracedHosts, this only clears the local resolver. Requests can still be traced through sdkCore.firstPartyHostResolver: both trace paths check the global resolver for traceability and fall back to its headerTypesForUrl(...) when the local set is empty, so apps that configured global first-party hosts keep creating spans and propagating headers even though the RC payload is intended to replace/clear developer hosts. Please make a present RC host list authoritative, or skip the global resolver fallback for this instrumentation once RC has supplied tracedHosts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is different from the iOS SDK, which doesn't have a global firstPartyHostResolver concept — all host configuration lives directly on Trace.Configuration. If we want RC to also control global first-party hosts on Android, that would need a dedicated Android-specific RC field. For now, tracedHosts correctly controls the per-interceptor local resolver as intended.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/remote-config #3663 +/- ##
========================================================
Coverage ? 73.47%
========================================================
Files ? 1002
Lines ? 36572
Branches ? 6195
========================================================
Hits ? 26869
Misses ? 7982
Partials ? 1721
🚀 New features to boost your workflow:
|
9ae030b to
6e16895
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e16895354
ℹ️ 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".
| val types = tracedHost.propagatorTypes | ||
| ?.map { it.toSdkHeaderType() } | ||
| ?.toSet() |
There was a problem hiding this comment.
When RC sends a traced host with propagatorTypes: [], these lines store an empty header set. The downstream injection paths (ApmNetworkInstrumentation.traceRequest and TracingInterceptor.updateRequest) treat an empty local header set as “no local match” via .ifEmpty { sdkCore.firstPartyHostResolver.headerTypesForUrl(...) }, so apps that also configured the same host globally still inject those global headers even though the RC payload explicitly asked for no propagators. Fresh evidence is the new test/comment that defines empty propagatorTypes as “trace host with no headers”; please carry an authoritative/no-fallback state instead of representing it as an empty set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is similar to #3663 (comment). I would say that If we want RC to suppress global header injection for a host, that would require a dedicated Android-specific approach. For now, propagatorTypes: [] correctly controls the per-interceptor local resolver as intended.
8e30c6f
4 participants
What does this PR do?
Applies the RC
tracenamespace to both Android tracing paths when the SDK instance becomes available:TraceRemoteConfigExt.kt(shared helpers:applyRcSampleRate(),toSdkInjection(),buildRcHostResolver()) and wires RC intoTracingInterceptor.onSdkInstanceReady()ApmNetworkInstrumentation.onSdkInstanceReady()(the modern OkHttp/Cronet path)RC is applied once per interceptor lifetime, guarded by
AtomicBoolean.compareAndSet, on both the legacy (TracingInterceptor/DatadogInterceptor) and modern (ApmNetworkInstrumentation) paths.Motivation
RUM-17709 — applies the RC
tracenamespace on Android. Mirrors the iOS implementation in dd-sdk-ios#3047.Additional Notes
RC fields mapped from
RemoteConfiguration.Trace:trace.sampleRatetraceSamplerwithDeterministicTraceSampler(rcRate); preservesSessionRebasedSamplerwrapper when present (used byDatadogInterceptorfor RUM × APM cross-product rebasing)trace.traceContextInjectiontraceContextInjection/injectionTypetrace.tracedHosts+trace.tracingHeaderTypeslocalFirstPartyHostHeaderTypeResolverviabuildRcHostResolver(). Semantics mirror iOS PR #3047: null → no-op;[]→ clear hosts; non-empty → replacepropagatorTypes/tracingHeaderTypessemantics (matching iOS):null(absent) → fall back to global types / SDK default[](explicit empty list) → no headers for that host / no global fallback — not treated as absentReview checklist (to be filled by reviewers)