RUM-15138: Rebase trace sample rate against RUM session sample rate for correlated cross-product sampling by hamorillo · Pull Request #3342 · DataDog/dd-sdk-android

hamorillo · 2026-04-07T12:16:54Z

What does this PR do?

DeterministicTraceSampler now applies cross-product rebasing using sessionSampleRate × traceSampleRate / 100 whenever a valid session sample rate is available. This rebasing is provider-driven and no longer gated by rum.session.id on the span.

The session sample rate is propagated from RumSessionScope → RumContext → SDK feature context. ApmNetworkInstrumentationConfiguration reads it at instrumentation setup time via a sessionSampleRateProvider lambda and passes it to DeterministicTraceSampler. A compatibility fallback still reads session_sample_rate from span tags for integrations that bypass ApmNetworkInstrumentationConfiguration.

_dd.agent_psr now reports the same effective sample rate used by sampling decisions in instrumentation paths, including rebased rates when applicable.

Motivation

Previously, rebasing behavior and effective-rate reporting were inconsistent across paths. This change makes rebasing policy explicit and consistent: when a session sample rate is available, trace sampling is rebased to preserve the intended cross-product relationship between session and trace sampling.

Additional Notes

Session rate stability: RumSessionScope snapshots sessionSampleRate once at construction time rather than calling getSampleRate() on every getRumContext() invocation, so session-level metadata remains stable for the scope's lifetime.

RumContextKeys: A new internal object centralises the session_sample_rate key in the trace module.

Follow-up

❓ 💡 We should consider, in the next major version, making DeterministicTraceSampler non-public so all instances are SDK-controlled. That would let us remove the span-tag compatibility fallback (session_sample_rate) and simplify sampling to a single internal effective-rate source.

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

codecov-commenter · 2026-04-07T13:40:49Z

Codecov Report

❌ Patch coverage is 91.48936% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.11%. Comparing base (e293f09) to head (eb2e1e8).
⚠️ Report is 4 commits behind head on develop.

Files with missing lines	Patch %	Lines
...n/com/datadog/android/okhttp/DatadogInterceptor.kt	71.43%	1 Missing and 1 partial ⚠️
...android/internal/sampling/DeterministicSampling.kt	83.33%	0 Missing and 1 partial ⚠️
...trace/internal/net/ApmNetworkInstrumentationExt.kt	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3342      +/-   ##
===========================================
+ Coverage    72.01%   72.11%   +0.09%     
===========================================
  Files          956      957       +1     
  Lines        35307    35339      +32     
  Branches      5862     5868       +6     
===========================================
+ Hits         25426    25482      +56     
+ Misses        8271     8252      -19     
+ Partials      1610     1605       -5

Files with missing lines	Coverage Δ
...adog/android/core/sampling/DeterministicSampler.kt	`90.48% <100.00%> (+5.29%)`	⬆️
.../datadog/android/rum/internal/domain/RumContext.kt	`100.00% <100.00%> (ø)`
...droid/rum/internal/domain/scope/RumSessionScope.kt	`91.72% <100.00%> (+0.75%)`	⬆️
...id/trace/ApmNetworkInstrumentationConfiguration.kt	`73.81% <ø> (ø)`
...datadog/android/trace/DeterministicTraceSampler.kt	`100.00% <100.00%> (ø)`
...ndroid/trace/internal/ApmNetworkInstrumentation.kt	`60.98% <100.00%> (ø)`
...dog/android/trace/internal/RumContextPropagator.kt	`77.97% <100.00%> (+2.97%)`	⬆️
...datadog/android/okhttp/trace/TracingInterceptor.kt	`84.47% <100.00%> (+2.10%)`	⬆️
...android/internal/sampling/DeterministicSampling.kt	`83.33% <83.33%> (ø)`
...trace/internal/net/ApmNetworkInstrumentationExt.kt	`82.76% <75.00%> (+13.53%)`	⬆️
... and 1 more

... and 36 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b6a78e8689

ℹ️ 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".

hamorillo · 2026-04-09T16:41:17Z

 */
-open class DeterministicTraceSampler(
-    sampleRateProvider: () -> Float
+open class DeterministicTraceSampler private constructor(


❓ 💡 open on this class created some friction — external subclasses silently bypass the sessionSampleRateProvider wiring and can't access getSampleRate(DatadogSpan), both of which are needed for correct rebasing. Worth making this internal or at least final in the next major version. Or do we have a good reason to have it open. Wdyt?

@satween might be the best person to discuss this with.

open is the legacy came from the V2 version of the SDK. Please add todo with link to V4 ticket.

hamorillo · 2026-04-09T16:41:42Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4ca59be245

ℹ️ 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".

chatgpt-codex-connector · 2026-04-09T21:56:08Z

Codex Review: Didn't find any major issues. Keep them coming!

ℹ️ 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".

hamorillo · 2026-04-10T06:49:17Z

Thanks for your initial review, @ambushwork! While reviewing this comment, I tried to simplify and improve the changes, so it would be nice if you could take a second pass. 🙏

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a7b9b0b93b

ℹ️ 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".

aleksandr-gringauz · 2026-04-10T09:23:21Z

+
+    /** @inheritDoc */
+    override fun sample(item: DatadogSpan): Boolean {
+        val sampleRate = getSampleRate(item)


This looks like exactly the same code as DeterministicSampler.sample. Is it possible not to have the same algorithm implemented twice?

Good catch!

I would say to do that we need to extract that to a "public" place (or at least accesible for subclasses) for example:

DeterministicSampler.kt protected fun computeSamplingDecision(sampleRate: Float, id: ULong): Boolean { return when { sampleRate >= SAMPLE_ALL_RATE -> true sampleRate <= 0f -> false else -> { val hash = id * SAMPLER_HASHER val threshold = (MAX_ID.toDouble() * sampleRate / SAMPLE_ALL_RATE).toULong() hash < threshold } } } --- DeterministicSampler.kt override fun sample(item: T): Boolean = computeSamplingDecision(getSampleRate(), idConverter(item)) --- DeterministicTraceSampler.kt override fun sample(item: DatadogSpan): Boolean = computeSamplingDecision(getSampleRate(item), SpanSamplingIdProvider.provideId(item))

The reason the sample override exists at all is to call getSampleRate(item) instead of getSampleRate() — the item-aware version is needed to support the legacy instantiation path where no sessionSampleRateProvider is wired up and the session rate is read from the span tag instead. The parent's sample would silently miss that fallback.

Unless you consider this a blocker, I'd prefer to keep the duplication for now and avoid widening the API surface. In a future major version, if the legacy constructors can be removed, the override could be dropped entirely and the parent's sample would suffice.

Wdyt?

I'd prefer to keep the duplication for now and avoid widening the API surface

Can you make computeSamplingDecision in your proposed solution internal instead of protected and make it some standalone function not tied to DeterministicSampler or DeterministicTraceSampler? This way you don't have to change the API of DeterministicSampler at all.

As discussed I've moved the computeSamplingDecision to dd-sdk-android-internal.

satween

Two additional things to check before proceed:

ApmInstrumentation is an experimental API that we gonna move to from TracingInterceptor/DatadogInterceptor and that already used in Cronet. But untill then - you have to support TracingInterceptor/DatadogInterceptor as well.
Note that there is another effective sampling calculation logic that is applied to the telemetry event. Make sure that your changes properly works with that one

satween · 2026-04-13T15:05:03Z

 */
-open class DeterministicTraceSampler(
-    sampleRateProvider: () -> Float
+open class DeterministicTraceSampler private constructor(


open is the legacy came from the V2 version of the SDK. Please add todo with link to V4 ticket.

satween · 2026-04-13T15:12:08Z


+internal fun Sampler<DatadogSpan>.effectiveSampleRate(span: DatadogSpan): Float? {
+    return when (this) {
+        is DeterministicTraceSampler -> getSampleRate(span)


I think this is not the best API design, as this layer becomes aware of the implementation of the Sampler interface.

Maybe we should add getSampleRate(T) method with the default implementation with fallback to getSampleRate()?

this will keep implementation isolated from the caller side + wont break backward compatibility. And in DeterministicTraceSampler you could implement independent behaviour. WDYT?

This is a good point. I totally agree that this is far from an ideal solution.

I considered that option before implementation but adding getSampleRate(T) on the Sampler interface sounds like an unnecessary refactor considering that and some point we'll be able to get rid of that method. We only need getSampleRate(T) due to the legacy classes but when remove and make those private/internal we won't need that method anymore.

The extension function with the when (this is ...) check is admittedly not clean, but it keeps the coupling contained and co-located with the V4 TODO, rather than leaving a footprint on the interface.

satween · 2026-04-13T15:19:04Z

+ * @param id a stable numerical identifier derived from the item being sampled.
+ * @return true if the item should be sampled, false otherwise.
+ */
+fun computeSamplingDecision(sampleRate: Float, id: ULong): Boolean {


why did you moved this code out of the DeterministicSampler ? It's seems like only descendants of it are using it...

You can see the discussion: #3342 (comment)

aleksandr-gringauz · 2026-04-16T15:32:34Z

+        val providerRateIsValid = providerRate >= 0f && providerRate < DeterministicSampler.SAMPLE_ALL_RATE
+        val sessionSampleRate = when {
+            providerRateIsValid -> providerRate
+            else -> (item.context().tags[LogAttributes.RUM_SESSION_SAMPLE_RATE] as? Number)?.toFloat()


Question.

Why don't we always extract the session_sample_rate from context().tags? If we do it this way, we won't have to deal with #3342 (comment).

Or am I missing something?

This is a good question.

I don’t like the approach of always relying on context().tags, because in that case the sampler would depend on the span internals. Based on how we’ve designed sampler, I think they should always have the sample rate available, not only when they receive a span.

For that reason, I only implemented the span path to keep backward compatibility, with the idea of removing it in V4 when we can drop the previously mentioned public APIs.

Anyway, I’m happy to discuss further if you think the two paths are unnecessary.

Just some thoughts, for now no strong opinion.

The way RUM context is passed to the Tracing spans is through injectRumContext which is called when the span is created. It is done this way so that we capture the RUM Context at exactly the moment the span is created.

If you pass some items of RUM Context through the FeatureContextUpdateReceiver and some through the injectRumContext mechanism, theoretically they might get out of sync. In our particular case with sessionSamplingRate probably this will not happen, because sessionSamplingRate is a constant, but still.

The way I see it. We have a span, we need to compute the sampling decision for it.

We extract the RUM Session ID and RUM Sampling rate from the RUM Context that is attached to it.

do the rebasing of sampling rates (simple division of 2 numbers).

call computeSamplingDecision.

DONE.

we can put the sampling rate from step 2 into AGENT_PSR_ATTRIBUTE.

We don't even need to use DeterministicTraceSampler and the Sampler interface in general for this.

However, there is a publicsetTraceSampler in ApmNetworkInstrumentationConfiguration and TracingInterceptor and the public DeterministicTraceSampler. As far as I see it makes my proposal impossible to implement without breaking the public API.

What if a customer sets their custom sampler? In general it breaks our deterministic sampling and cross-feature sampling rebasing logic. Also mentioned here RUM-15138: Rebase trace sample rate against RUM session sample rate for correlated cross-product sampling #3342 (comment).

What is the point of public DeterministicTraceSampler?. Tbh I don't understand how exactly the customers might want to customize this the way it is now in this PR (even the way it is now in develop). IIUC there is a plan to at least make it internal in v4 RUM-15138: Rebase trace sample rate against RUM session sample rate for correlated cross-product sampling #3342 (comment).

without breaking the public API

I would say that’s the main inconvenience of adding cross-sampling in this PR. I haven’t thought deeply about your proposal, but I think it’s doable, as you mentioned, once we get rid of what we’ve been discussing in different threads within the PR.

Anyway, when we reach that point, I think it’s worth considering your suggestion. Thanks for sharing.

I think context.tags is really handy for that, we already pass RUM session ID there, so having sample rate is a simple extension then. And we use RUM Session ID in the sticky sampling decision (in the SpanSamplingIdProvider).

Indeed, traceSampler makes the things more complicated, but this is already the case with RUM session ID I believe (I may be wrong, I didn't check the code in details): if we provide custom sampler, we already lose the stickiness-to-RUM-session property we have right now? So nothing new, we can acknowledge such behavior as existing. But then I guess it will require changing DeterministicSampler API, because we need to register RUM session provider as well in the constructor signature?

Relying on context.tags will simplify the change layout. But we can keep both paths if relying on span internals is a concern.

Based on how we’ve designed sampler, I think they should always have the sample rate available, not only when they receive a span.

However, sampling decision is made per-span. With HBS (Head-based sampling) the root span gets the sampling decision and child spans follow it.

But then I guess it will require changing DeterministicSampler API, because we need to register RUM session provider as well in the constructor signature?

One of the problems IIUC is that we have to implement DeterministicTraceSampler.getSampleRate(). This method doesn't accept the span, so the only way to have the RUM sample rate there is by passing it into the constructor of DeterministicTraceSampler.

Relying on context.tags

You both mentioned this approach, so I’m exploring that option again. It’s probably doable by returning traceSampleRate in DeterministicTraceSampler.getSampleRate(), which could be correct.

We don't even need to use DeterministicTraceSampler and the Sampler interface in general for this.

We’ll need to keep the casting at least until V4, where we can replace the Sampler with a different interface (or remove it, as @aleksandr-gringauz mentioned).

Let me explore that path. Thanks, both.

I've switched to always reading session_sample_rate from context.tags (the same mechanism already used for RUM_SESSION_ID by SpanSamplingIdProvider). This allowed removing SessionSampleRateRegistrationEvent, the FeatureContextUpdateReceiver/FeatureEventReceiveron TracingFeature, the AtomicReference caching, and the sendEvent wiring.

DeterministicTraceSampler.getSampleRate()(no-arg) now returns the raw traceSampleRate. All paths that need the rebased rate (_dd.agent_psr, _dd.rule_psr) already dispatch to getSampleRate(span) via the effectiveSampleRate() bridge, so no call site is affected.

In V4, we can revisit the Sampler<DatadogSpan> interface to support per-span rate calculation natively, removing the need for the effectiveSampleRate() casting dispatch.

DeterministicTraceSampler.getSampleRate()(no-arg) now returns the raw traceSampleRate

IIUC in your new implementation, if the user overrides DeterministicTraceSampler.getSampleRate it will be useless, because it isn't used anywhere. So if some user relies on overriding getSampleRate this way, we are breaking it.

But tbh I think we should do it regardless. The probability of someone doing this is really small. And in V4 we should clean-up our public API, get rid of custom samplers in network request tracing entirely.

if the user overrides DeterministicTraceSampler.getSampleRate it will be useless, because it isn't used anywhere.

Good point @aleksandr-gringauz . You are right.

But tbh I think we should do it regardless. The probability of someone doing this is really small. And in V4 we should clean-up our public API, get rid of custom samplers in network request tracing entirely.

I agree. As you mentioned, solving that probably wouldn’t help anyone, and it would make everything even more complex to implement. I would say this is acceptable.

0xnm

I went quickly through the main files and added some comments.

0xnm · 2026-04-16T15:18:23Z

            val viewTimestamp = featureContext[VIEW_TIMESTAMP] as? Long ?: 0L
            val viewTimestampOffset = featureContext[VIEW_TIMESTAMP_OFFSET] as? Long ?: 0L
+            val sessionSampleRate = (featureContext[SESSION_SAMPLE_RATE] as? Number)
+                ?.toFloat() ?: FULL_SESSION_SAMPLE_RATE


just curious: is there a motivation behind using 100% as a fallback, and not 0%?

The main reason was that if we use 0% as a fallback, the rebased sampling would drop everything, so it seemed safer to me. However, now that you’ve mentioned it, I’m not sure if this is what we expect.

0xnm · 2026-04-16T15:22:04Z


    private fun renewSession(time: Time, reason: StartReason) {
        val newSessionId = UUID.randomUUID().toString()
+        sessionSampleRate = sessionSampler.getSampleRate() ?: RumContext.FULL_SESSION_SAMPLE_RATE


Why do we need to set it here? normally, session sample rate is static in RUM and is defined in the configuration, there is no API to provide something dynamic.

Is it some protection for the future?

Good catch. We don't need it.

0xnm · 2026-04-16T15:25:14Z

     * By default it is a sampler accepting 100% of the traces.
     */
    fun setTraceSampler(traceSampler: Sampler<DatadogSpan>) = apply {
-        this.traceSampler = traceSampler


I'm wondering if we need to expose this API long-term. Probably, there are some users, but I'm curious if it is worth to expose it given we have cross-feature sampling requirement now and also if iOS SDK has something similar.

Totally agree. We can’t remove it right now since it’s part of the public API, but we could consider removing it in V4. I’m not sure if we have data on how many users are using it, though.

If we agree, I'll create a ticket.

0xnm · 2026-04-16T15:29:53Z

            val tracerProvider = TracerProvider(localTracerFactory, globalTracerProvider)

+            @Suppress("UnsafeThirdPartyFunctionCall") // AtomicReference initialized with non-null Float constant
+            val cachedSessionSampleRate = AtomicReference(DEFAULT_TRACE_SAMPLE_RATE)


It seems it means here that while RUM session context update has happened, we say "assume that RUM session sampling rate is DEFAULT_TRACE_SAMPLE_RATE". Unless I miss something, this is wrong? Shouldn't we just say that in this case we use traceSampleRate alone and shouldn't set any value in cachedSessionSampleRate by default and set it only when context arrived?

Shouldn't we just say that in this case we use traceSampleRate alone and shouldn't set any value in cachedSessionSampleRate by default and set it only when context arrived?

This is exactly what the code does; however, the naming is clearly misleading.

We initialize cachedSessionSampleRate to 100f, which, after rebasing, essentially means using traceSampleRate alone.

0xnm · 2026-04-16T15:33:22Z

+    private val isClosed = AtomicBoolean(false)
+    private val isRegistered = AtomicBoolean(false)


if I'm not wrong, AtomicBoolean are not really needed here, because these two properties are always called inside synchronized block, which guarantees the thread-safety for any properties used inside. On another hand, then these properties would be var, so we can keep it.

Good catch! This was a leftover from a previous approach.

0xnm · 2026-04-16T15:35:06Z

+    /**
+     * Releases this instrumentation and unregisters context receivers from SDK core.
+     */
+    fun close() {


does it make sense to implement Closeable interface to be explicit? Alternatively we may put FeatureContextUpdateReceiver on TracingFeature, so we can unsubscribe there on the lifecycle method. Otherwise it is easy to miss close call.

Great suggestion! Thanks!

I've applied the FeatureContextUpdateReceiver on TracingFeature. Let me know what you think.

…or correlated cross-product sampling DeterministicTraceSampler now applies a cross-product rebasing formula when a span carries a RUM session ID tag: rebasedRate = sessionSampleRate × traceSampleRate / 100. Previously, the raw traceSampleRate was used as the sampling threshold regardless of whether the session ID was the hash key, causing the effective trace rate among RUM-tracked sessions to be incorrect. The session sample rate is propagated via RumContext (new sessionSampleRate field) into the SDK feature context, then written onto spans as a tag by RumContextPropagator alongside the existing RUM tags. DeterministicTraceSampler reads the tag at sampling time and applies the rebasing; spans without a session ID tag continue to use the raw trace rate unchanged. RumSessionScope snapshots sessionSampleRate in renewSession() at the moment the sampling decision is made, ensuring the rate is immutable for the lifetime of that session.

…hing the actual sampling decision applyPriority() and RequestTracingState.sampleRate now use a span-aware effectiveSampleRate() lookup instead of the argument-less getSampleRate(). For spans linked to an active RUM session the rebased rate is reported; for spans without a session ID the raw trace rate is reported, eliminating the mismatch where the backend received a rebased PSR for non-session spans sampled at the raw trace rate. DeterministicTraceSampler gains an internal getSampleRate(DatadogSpan) method that resolves the effective rate per span, with a compatibility fallback to the session_sample_rate span tag for integrations that do not use ApmNetworkInstrumentationConfiguration. RumContextKeys centralises the session_sample_rate string constant across the trace module. # Conflicts: # features/dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/internal/ApmNetworkInstrumentation.kt

…ernal Moves the core hash-and-threshold computation into a shared top-level function `computeSamplingDecision` in `dd-sdk-android-internal`, removing the duplicated implementation from both `DeterministicSampler` and `DeterministicTraceSampler`. Adds unit tests for the extracted function.

…pdateReceiver for session sample rate caching

…in legacy interceptors Make DeterministicTraceSampler.getSampleRate(DatadogSpan) public so the legacy TracingInterceptor and DatadogInterceptor paths can call the span-aware overload at the PSR reporting sites. The type-check pattern mirrors the existing effectiveSampleRate approach in ApmNetworkInstrumentationExt. The session_sample_rate tag is already written onto the span by extractRumContext before the PSR metric is set, so the tag-based fallback fires correctly for samplers created via the legacy public constructors. getSampleRate(DatadogSpan) will be removed when the legacy interceptor path is retired in v4.

Replace the internal RumContextKeys object (single-constant file in dd-sdk-android-trace) with LogAttributes.RUM_SESSION_SAMPLE_RATE in dd-sdk-android-core, consistent with where the other RUM context propagation keys (RUM_SESSION_ID, RUM_VIEW_ID, etc.) are defined.

Instead of each ApmNetworkInstrumentation instance creating and managing its own FeatureContextUpdateReceiver (with manual close()/synchronized lifecycle), TracingFeature now owns a single receiver that listens for RUM context changes and broadcasts the session sample rate to all registered AtomicReference<Float> instances. ApmNetworkInstrumentation sends a SessionSampleRateRegistrationEvent to TracingFeature on SDK resolution, which registers the ref and initializes it with the current rate. TracingFeature unregisters the receiver and clears all refs on onStop(), tying the lifecycle to the feature itself. This eliminates the need for close() on ApmNetworkInstrumentation, fixing the leak where the OkHttp integration path never called close() while the Cronet path did. # Conflicts: # features/dd-sdk-android-trace/src/test/kotlin/com/datadog/android/trace/internal/net/ApmNetworkInstrumentationTest.kt

0xnm · 2026-04-20T15:07:26Z

+        val providerRateIsValid = providerRate >= 0f && providerRate < DeterministicSampler.SAMPLE_ALL_RATE
+        val sessionSampleRate = when {
+            providerRateIsValid -> providerRate
+            else -> (item.context().tags[LogAttributes.RUM_SESSION_SAMPLE_RATE] as? Number)?.toFloat()


I think context.tags is really handy for that, we already pass RUM session ID there, so having sample rate is a simple extension then. And we use RUM Session ID in the sticky sampling decision (in the SpanSamplingIdProvider).

Indeed, traceSampler makes the things more complicated, but this is already the case with RUM session ID I believe (I may be wrong, I didn't check the code in details): if we provide custom sampler, we already lose the stickiness-to-RUM-session property we have right now? So nothing new, we can acknowledge such behavior as existing. But then I guess it will require changing DeterministicSampler API, because we need to register RUM session provider as well in the constructor signature?

Relying on context.tags will simplify the change layout. But we can keep both paths if relying on span internals is a concern.

Based on how we’ve designed sampler, I think they should always have the sample rate available, not only when they receive a span.

However, sampling decision is made per-span. With HBS (Head-based sampling) the root span gets the sampling decision and child spans follow it.

0xnm · 2026-04-20T15:09:00Z

+            sessionSampleRate < DeterministicSampler.SAMPLE_ALL_RATE
+        ) {
+            val rebased = super.getSampleRate() * sessionSampleRate / DeterministicSampler.SAMPLE_ALL_RATE
+            if (rebased > DeterministicSampler.SAMPLE_ALL_RATE) DeterministicSampler.SAMPLE_ALL_RATE else rebased


very nit: maybe use coerceAtMost as above?

…r-based plumbing Replace the multi-layer sessionSampleRateProvider chain (FeatureContextUpdateReceiver → TracingFeature → AtomicReference → DeterministicTraceSampler) with direct reads from the session_sample_rate span tag that RumContextPropagator already writes before sampling runs. This eliminates SessionSampleRateRegistrationEvent, FeatureContextUpdateReceiver on TracingFeature, AtomicReference caching, and sendEvent cross-module wiring — resolving locking concerns, timing races, and misleading defaults flagged during review. getSampleRate() (no-arg) now returns the raw configured trace rate. Per-span rebasing lives in sample(item) and the deprecated getSampleRate(item), accessed through the effectiveSampleRate() casting bridge until V4 allows replacing the Sampler<DatadogSpan> interface.

0xnm

overall lgtm, I just added some questions

0xnm · 2026-04-22T07:28:43Z

-        this.traceSampler = DeterministicTraceSampler(sampleRate)
+        this.traceSampleRate = sampleRate


Do we still need this change and customTraceSampler ?: DeterministicTraceSampler(traceSampleRate) below? by looking on the effectiveSampleRate implementation I think it is not needed?

Also we may want to document the difference in the behaviour of setTraceSampleRate vs setTraceSampler that the former relies on the RUM session sampling and latter may drop this connection.

Good catch! I’ve reverted those changes, as they’re no longer needed.

…en test constants

…e self-call through deprecated API

hamorillo · 2026-05-04T10:04:00Z

After several discussions, we noticed that this wasn't the right approach.

Closing this PR in favor of #3402

This comment has been minimized.

Sign in to view

hamorillo force-pushed the hector.morilloprieto/RUM-15138 branch from 6d2dcd7 to de100fe Compare April 7, 2026 13:06

hamorillo force-pushed the hector.morilloprieto/RUM-15138 branch 3 times, most recently from 5320e34 to b6a78e8 Compare April 8, 2026 09:41

hamorillo marked this pull request as ready for review April 8, 2026 12:23

hamorillo requested review from a team as code owners April 8, 2026 12:23

chatgpt-codex-connector Bot reviewed Apr 8, 2026

View reviewed changes

Comment thread .../dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/DeterministicTraceSampler.kt Outdated

Comment thread ...android-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumSessionScope.kt Outdated

ambushwork previously approved these changes Apr 8, 2026

View reviewed changes

hamorillo marked this pull request as draft April 8, 2026 13:17

hamorillo dismissed ambushwork’s stale review via 4ca59be April 9, 2026 16:31

hamorillo commented Apr 9, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Apr 9, 2026

View reviewed changes

Comment thread .../dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/DeterministicTraceSampler.kt Outdated

hamorillo force-pushed the hector.morilloprieto/RUM-15138 branch from 4ca59be to a7b9b0b Compare April 9, 2026 21:38

hamorillo marked this pull request as ready for review April 10, 2026 06:49

hamorillo requested a review from ambushwork April 10, 2026 06:49

chatgpt-codex-connector Bot reviewed Apr 10, 2026

View reviewed changes

Comment thread ...id-trace/src/main/kotlin/com/datadog/android/trace/ApmNetworkInstrumentationConfiguration.kt Outdated

hamorillo force-pushed the hector.morilloprieto/RUM-15138 branch from a7b9b0b to e50d3bf Compare April 10, 2026 07:17

aleksandr-gringauz reviewed Apr 10, 2026

View reviewed changes

hamorillo requested a review from aleksandr-gringauz April 13, 2026 14:35

aleksandr-gringauz previously approved these changes Apr 13, 2026

View reviewed changes

0xnm reviewed Apr 13, 2026

View reviewed changes

Comment thread ...id-trace/src/main/kotlin/com/datadog/android/trace/ApmNetworkInstrumentationConfiguration.kt Outdated

satween reviewed Apr 13, 2026

View reviewed changes

hamorillo dismissed aleksandr-gringauz’s stale review via c8f50f9 April 14, 2026 14:06

hamorillo force-pushed the hector.morilloprieto/RUM-15138 branch from c8f50f9 to d5d716f Compare April 14, 2026 14:27

hamorillo requested a review from aleksandr-gringauz April 14, 2026 18:35

hamorillo force-pushed the hector.morilloprieto/RUM-15138 branch from 8f392fd to 0f30157 Compare April 16, 2026 09:05

aleksandr-gringauz reviewed Apr 16, 2026

View reviewed changes

0xnm reviewed Apr 16, 2026

View reviewed changes

hamorillo added 8 commits April 17, 2026 15:06

RUM-15138: Replace per-request getFeatureContext with FeatureContextU…

3597376

…pdateReceiver for session sample rate caching

RUM-15138: PR feedback

b7272e5

hamorillo force-pushed the hector.morilloprieto/RUM-15138 branch from 82293f2 to a985cb8 Compare April 17, 2026 13:43

hamorillo requested review from 0xnm and aleksandr-gringauz April 20, 2026 07:01

0xnm reviewed Apr 20, 2026

View reviewed changes

hamorillo force-pushed the hector.morilloprieto/RUM-15138 branch from 7b18845 to 4d02ee3 Compare April 21, 2026 12:42

hamorillo force-pushed the hector.morilloprieto/RUM-15138 branch from 4d02ee3 to 1dfbfca Compare April 21, 2026 14:01

hamorillo requested a review from 0xnm April 22, 2026 06:35

0xnm reviewed Apr 22, 2026

View reviewed changes

RUM-15138: Address PR review feedback — simplify sampler config, hard…

6f012e4

…en test constants

hamorillo force-pushed the hector.morilloprieto/RUM-15138 branch from 5feeb98 to 6f012e4 Compare April 22, 2026 09:57

aleksandr-gringauz reviewed Apr 22, 2026

View reviewed changes

Comment thread dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/sampling/DeterministicSampler.kt

Comment thread .../dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/DeterministicTraceSampler.kt Outdated

RUM-15138: Deprecate DeterministicSampler hashing constants and remov…

eb2e1e8

…e self-call through deprecated API

hamorillo mentioned this pull request Apr 23, 2026

RUM-15139: Use rebased sessionReplaySampleRate for deterministic Session Replay sampling #3372

Merged

3 tasks

hamorillo marked this pull request as draft April 27, 2026 08:26

hamorillo closed this May 4, 2026

hamorillo mentioned this pull request Jun 4, 2026

RUM-15702 Add trace sampling decision to DatadogEventBridge #3495

Merged

3 tasks

		private val isClosed = AtomicBoolean(false)
		private val isRegistered = AtomicBoolean(false)

		this.traceSampler = DeterministicTraceSampler(sampleRate)
		this.traceSampleRate = sampleRate

Uh oh!

Conversation

hamorillo commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Additional Notes

Follow-up

Review checklist (to be filled by reviewers)

Uh oh!

This comment has been minimized.

codecov-commenter commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hamorillo commented Apr 9, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot commented Apr 9, 2026

Uh oh!

hamorillo commented Apr 10, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aleksandr-gringauz Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

satween left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aleksandr-gringauz Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hamorillo commented Apr 7, 2026 •

edited

Loading

codecov-commenter commented Apr 7, 2026 •

edited

Loading

aleksandr-gringauz Apr 10, 2026 •

edited

Loading

aleksandr-gringauz Apr 17, 2026 •

edited

Loading

hamorillo Apr 21, 2026 •

edited

Loading