RUM-15139: Use rebased sessionReplaySampleRate for deterministic Session Replay sampling#3372
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ 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". |
![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: 2dbea616fb
ℹ️ 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".
8f392fd to
0f30157
Compare
2dbea61 to
1217c89
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3372 +/- ##
===========================================
- Coverage 72.03% 71.97% -0.06%
===========================================
Files 956 957 +1
Lines 35307 35311 +4
Branches 5861 5859 -2
===========================================
- Hits 25432 25415 -17
- Misses 8271 8280 +9
- Partials 1604 1616 +12
🚀 New features to boost your workflow:
|
4d02ee3 to
1dfbfca
Compare
1217c89 to
5d9fa7c
Compare
…ion Replay sampling Also, extract SessionSamplingIdProvider to dd-sdk-android-internal
5d9fa7c to
f9fa86c
Compare
|
@kikoveiga Thanks for your initial review. Since #3342 is still being redesigned and discussed, I’ve removed this change’s dependencies on that code. As discussed with @0xnm, Session Replay is an independent feature with a different sampling decision point, so we can move forward with this PR. |
| sdkCore.getFeature(Feature.SESSION_REPLAY_FEATURE_NAME)?.sendEvent( | ||
| mapOf( | ||
| SESSION_REPLAY_BUS_MESSAGE_TYPE_KEY to RUM_SESSION_RENEWED_BUS_MESSAGE, | ||
| RUM_KEEP_SESSION_BUS_MESSAGE_KEY to keepSession, |
There was a problem hiding this comment.
should we keep it? I'm thinking about the setup where RUM and SR artifacts version mismatch. Unlikely it can be the case, but just to be sure that we don't break such "wrong" setups.
There was a problem hiding this comment.
Good point! I didn't consider that case. I've added it again with a comment to remove it at V4.
| private fun applySampling(alreadySeenSession: Boolean, sessionData: SessionData) { | ||
| if (!alreadySeenSession) { | ||
| isSessionSampledIn.set(rateBasedSampler.sample(Unit)) | ||
| isSessionSampledIn.set( |
There was a problem hiding this comment.
I'm wondering if we need to handle this case somehow:
Normally we don't expect ingestion of SR data without RUM session linked, but worth checking.
There was a problem hiding this comment.
I would say that on Android this case can't happen — SR only starts recording when it receives a rum_session_renewed bus event from RUM via onReceive(). Also parseSessionMetadata returns null when no RUM sessionId available so it will never reach the applySampling method.
4 participants
What does this PR do?
Replaces the independent random Session Replay sampling with deterministic sampling based on the RUM session sample rate. Instead of two independent coin flips (RUM + SR), the SR sampling decision now uses a
DeterministicSamplerwith a combined rate (sessionSampleRate * srSampleRate / 100), keyed on the session ID. This guarantees that any session sampled in for SR is also sampled in for RUM.Key changes:
RumSessionScope: SendssessionSampleRate(instead ofkeepSession) to Session Replay via the inter-feature busSessionReplayFeature: ReplacesRateBasedSamplerwithDeterministicSamplerusingcombinedSampleRate(). Removes thekeepSessiongate — it's now redundant because the combined threshold is always <= the RUM thresholdDeterministicSampling.combinedSampleRate(): New utility indd-sdk-android-internal(equivalent to iOS'sSampleRate.composed(with:)) for computing the effective rate when composing parent + child samplingSessionSamplingIdProvider: Moved fromdd-sdk-android-rumtodd-sdk-android-internalso it can be shared across featuresMotivation
RUM-15139. Aligns Android SR sampling with the deterministic approach used by the iOS SDK (dd-sdk-ios#2714). The previous random sampler meant SR decisions were independent of RUM decisions, requiring the
keepSessionflag as a gate. With deterministic sampling, the math guarantees consistency — makingkeepSessionunnecessary and simplifying the code.Additional Notes
E2E Verification — Deterministic SR Sampling
The deterministic sampling decision can be verified on a real device using the
android-sdk-event-inspectionskill. Ask Claude Code to:Tested with 8 sessions across 5 rate combinations — all matched the Knuth hash math:
e3bbb091-4cb2-444b-8c00-5b45b410a3a5has_replay: false4d8b7e7b-5b29-45af-8381-fdaeaf470fd0has_replay: falsec5254296-9dd5-4f27-9f9d-3cccddfaa22773edb0a2-ede8-4b6f-8c6f-f16c95f0fcbdhas_replay: false735ba6e4-4ea0-4a9a-8505-1bbebd3cea31has_replay: true9ba59950-fa30-459e-bdd5-20b1b8140d5b11274730-b7fd-4982-b426-185c176d9413ddd7b75d-ecc8-4ef7-a4c2-fc34db9230b1has_replay: trueReview checklist (to be filled by reviewers)