RUM-15138: Add cross-product sampling rebasing for RUM-to-APM correlation#3402
Conversation
3ca0886 to
d54fca2
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3402 +/- ##
===========================================
- Coverage 72.07% 72.07% -0.00%
===========================================
Files 961 962 +1
Lines 35400 35435 +35
Branches 5883 5890 +7
===========================================
+ Hits 25513 25538 +25
- Misses 8270 8277 +7
- Partials 1617 1620 +3
🚀 New features to boost your workflow:
|
2bb9ee6 to
37c5f51
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
37c5f51 to
ff4f037
Compare
![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: ff4f037099
ℹ️ 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".
| class com.datadog.android.trace.internal.net.SessionRebasedSampler : com.datadog.android.core.sampling.Sampler<com.datadog.android.trace.api.span.DatadogSpan>, SpanAwareSampler | ||
| constructor(com.datadog.android.core.sampling.Sampler<com.datadog.android.trace.api.span.DatadogSpan>) | ||
| override fun sample(com.datadog.android.trace.api.span.DatadogSpan): Boolean | ||
| override fun getSampleRate(): Float? | ||
| override fun getSampleRate(com.datadog.android.trace.api.span.DatadogSpan): Float? |
There was a problem hiding this comment.
side thought: we need to refactor this in v4, because otherwise we have a relation to DatadogSpan in 2 interfaces: com.datadog.android.core.sampling.Sampler<com.datadog.android.trace.api.span.DatadogSpan> and in SpanAwareSampler (which just brings utility method getSampleRate wrt the span).
| val tracerProvider = TracerProvider(localTracerFactory, globalTracerProvider) | ||
|
|
||
| val effectiveSampler = if (headerPropagationOnly) { | ||
| SessionRebasedSampler(traceSampler) |
There was a problem hiding this comment.
So we are wrapping sampler inside another sampler just because we have setTraceSampler API? we need to address it in v4, removing this API, probably.
Also I think it is worth to add in the documentation of different trace sampling rate method (setTraceSampleRate / setTraceSampler) that there is a dependency on the RUM session sampling where applicable. I'm wondering if iOS SDK updated docs already.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
we need to address it in v4, removing this API, probably.
I agree. If I'm not wrong, iOS doesn't allow customers to set their own sampler so we probably should align with it in this case and keep only the setTraceSampleRate.
| override fun getSampleRate(span: DatadogSpan): Float? { | ||
| if (delegate !is DeterministicTraceSampler) return delegate.effectiveSampleRate(span) | ||
| val rawRate = delegate.getSampleRate() | ||
| return computeEffectiveRate(span, rawRate) | ||
| } | ||
|
|
||
| private fun computeEffectiveRate(item: DatadogSpan, rawRate: Float): Float { | ||
| val sessionRate = (item.context().tags[LogAttributes.RUM_SESSION_SAMPLE_RATE] as? Number)?.toFloat() | ||
| return if (sessionRate != null) { | ||
| DeterministicSampling.combinedSampleRate(sessionRate, rawRate) |
There was a problem hiding this comment.
I think we may have confusion here: combinedSampleRate vs effectiveSampleRate. What is the difference? It seems that the former is coming from iOS SDK?
There was a problem hiding this comment.
It seems that the former is coming from iOS SDK?
Yes, I used that name to maintain parity (as much as possible) with iOS. The combinedSampleRate is essentially the rebased sample rate over the session sample rate.
What is the difference?
effectiveSampleRate is the sample rate that we'll use for sampling, but it is not necessarily a combined or rebased one, as we may not have a session sample rate. In that case, effectiveSampleRate returns the raw configured sample rate.
| val tracerProvider = TracerProvider(localTracerFactory, globalTracerProvider) | ||
|
|
||
| val effectiveSampler = if (headerPropagationOnly) { | ||
| SessionRebasedSampler(traceSampler) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
27c84c2 to
45062e0
Compare
| * When `headerPropagationOnly` is enabled, the effective trace sample rate is automatically combined with | ||
| * the active RUM session sample rate, ensuring the backend receives correct sampling metadata | ||
| * (`_dd.agent_psr` / `_dd.rule_psr`) for RUM-to-APM correlation. |
There was a problem hiding this comment.
That leads me to the interesting idea: is it even useful to expose HeaderPropagationOnly API to the customer? @satween I believe this API shouldn't be controlled by the customer. I cannot imagine a situation where we need to give such control.
Current API doc says:
* This is useful when you want distributed tracing visibility without the overhead
* of client-side network spans.
But this is controlled by the RUM network instrumentation. If there is no RUM it makes no sense to set this to true, I think.
There was a problem hiding this comment.
@satween, any thoughts about this comment?
There was a problem hiding this comment.
Hey there. HeaderPropagationOnly API has been added for the backward compatibility with the previous instrumentation. For example if customer set DatadogInterceptor but not TracingInterceptor. With Rum enabled DatadogInterceptor will not send any spans, only add the traceId/spanId into the request headers and send RumEvents.
With the new instrumentation if customer wants to add any APM related logic into the networking instrumentation - he have to add ApmInstrumentationConfiguration. But adding it will enable tracing event reporting by-default. So to replicate the behaviour from the configuration when only DatadogInterceptor is provided i added HeaderPropagationOnly. We could make it internal for sure if we believe that nobody needs such set-up.
There was a problem hiding this comment.
IIUC, in that case, we should keep the HeaderPropagationOnly public; if not, we'll be forcing clients to send APM spans when they only want to add the HTTP headers. Right?
There was a problem hiding this comment.
I still think having this API can be quite confusing and not very obvious for the customer (we discussed this before though, with other API name), hopefully the documentation is well read and acknowledged.
In our code we use it in the sample app
And later this ApmInstrumentationConfiguration is a source for both apmNetworkInstrumentation and distributedTracingInstrumentation
So what was done by the DatadogInterceptor and TracingInterceptor without the need of the user (because of two different network instrumentations with a clear logical boundary), now needed to be done by the user, since we have a single entrypoint to the network instrumentation of Cronet - CronetPlugin.
I don't have a solution for that though. But maybe worth checking how iOS SDK handles this (enabling/disabling APM spans send depending on the instrumentation approach).
There was a problem hiding this comment.
I agree, it is confusing, and we may want to revisit the public API for v4 to make it simpler for the user because, as you mentioned, DatadogInterceptor and TracingInterceptor are easier to understand.
There was a problem hiding this comment.
DatadogInterceptor and TracingInterceptor are easier to understand.
Can't agree here. DatadogInterceptor extends TracerInrerceptor, both sharing same APM logic, but DatadogInterceptor basically does RUM integration, where TracerInterceptor does APM. You can set DatadogInterceptor as addNetworkInterceptor and if Rum feature is disabled it starts behave like a TracerInterceptor. Plus, as a user you cannot re-use same configuration for cronet. You have to set-up DataadogEventListener and if customer accidentally add their listener later in builder chain - it will break Rum resource timings.
The confusion with setHeadersPropagationOnly comes from backward compatibility with an existing logic. For v4 - we should disable most of those API's, reducing the shape of public API and decreasing the combinations of the setups, so there will be only few possible ways to use the API. ( for example if customer wants the APM headers it means spans also should be sent). But this is product decision as well so for the dogfooding period whole set of API's is open to make sure that new instrumentation behaves same way as previous and with all of them we could compare them 1 by 1 . Keep also in mind that our SDK should behave same way for both Cronet and OkHttp (as much as possible) so most features available for OkHttp sould be available for Cronet (if it's even possible), meanwhile Cronet has completley different architecture and current implementation - is a set of minimal common subset of logic, that could be shared between this two networking libraries.
| // region sample() | ||
|
|
||
| @Test | ||
| fun `M use rebased rate W sample() { session_sample_rate tag present }`(forge: Forge) { |
There was a problem hiding this comment.
maybe we can do in another PR: wdyt about some integration test which asserts the effective sample rate? would be useful to control our expectations with "close-to-real" setup. Although I guess it will be quite a big volume of events to generate to have some trustworthy value.
There was a problem hiding this comment.
Sounds good! I can work on it in a different PR. 🙂 I'll file a follow-up ticket so we don't lose it.
One thing to note is that the decision to sample is now deterministic based on sessionId (with traceId as fallback when there's no RUM session) and not random, so it should make it easier to test.
There's already a HeadBasedSamplingTest in reliability/single-fit/okhttp/ so something similar could work.
0xnm
left a comment
0xnm
left a comment
There was a problem hiding this comment.
LGTM. But with caution: we really need to add integration tests for that, so that we are sure that outcome is matching our expectations and spec. Luckily, we have enough time to do it and also do dogfooding, since we are just in the start of the next SDK dev cycle.
…o-APM paths # Conflicts: # integrations/dd-sdk-android-okhttp/src/main/kotlin/com/datadog/android/okhttp/DatadogInterceptor.kt
45062e0 to
7c7046f
Compare
I've created RUM-16331, and I'll work on it ASAP. |
5 participants
What does this PR do?
Uses the rebased sample rate (
traceSampleRate * sessionSampleRate / 100) when RUM-to-APM correlation requires it. ASessionRebasedSamplerdecorator wraps the trace sampler and adjusts the effective rate based on the RUM session sample rate tag on the span.The rebasing is scoped exclusively to the RUM-to-APM path:
DatadogInterceptor(OkHttp) wraps the sampler inSessionRebasedSamplerat build timeApmNetworkInstrumentationConfigurationwraps whenheaderPropagationOnly = trueTracingInterceptor(pure APM) andheaderPropagationOnly = falseuse the raw trace rate unchangedMotivation
When both RUM session sampling and trace sampling are active, the backend needs
_dd.agent_psr/_dd.rule_psrto reflect the actual combined probability for correct statistical reweighting. Without rebasing, these metrics report only the trace rate, leading to incorrect trace counts on the platform.Additional Notes
@InternalApitypes indd-sdk-android-trace:SessionRebasedSampler,SpanAwareSamplerinterface, andeffectiveSampleRate()extension. NewLogAttributes.RUM_SESSION_SAMPLE_RATEconstant indd-sdk-android-core. All are internal-use types in*.internal.*packages.DeterministicTraceSampleris NOT modified. The decorator wraps it externally, avoiding changes to the open public class.setTraceSampler()) pass through without rebasing — the decorator only rebasesDeterministicTraceSamplerdelegates.DatadogInterceptorshould ignore it and create an independent trace so the RUM SDK can make its own sampling decision. Related iOS work: dd-sdk-ios#2726.Review checklist (to be filled by reviewers)