Skip to content

RUM-10064: Read RUM context in Session Replay in non-blocking manner#2666

Merged
0xnm merged 2 commits into
nogorodnikov/rum-8170/feature-context-syncfrom
nogorodnikov/rum-10064/read-rum-context-in-non-blocking-manner-in-session-replay
May 22, 2025
Merged

RUM-10064: Read RUM context in Session Replay in non-blocking manner#2666
0xnm merged 2 commits into
nogorodnikov/rum-8170/feature-context-syncfrom
nogorodnikov/rum-10064/read-rum-context-in-non-blocking-manner-in-session-replay

Conversation

@0xnm

@0xnm 0xnm commented May 21, 2025

Copy link
Copy Markdown
Member

What does this PR do?

The main goal of this PR is to move away from the getFeatureContext() calls in Session Replay to read RUM context, because it will become a blocking call and we cannot allow ourselves waiting in case if this call happens on the main thread.

This is achieved by listening to the RUM context updates instead and this is the same way iOS Session Replay it taking RUM context (link).

It can be a bit of the miss when associating Session Replay wireframes to the RUM views due to the following:

Session Replay receives updates of the RUM context and this is the only way of getting it there. This means that if we started a new view and captured a new snapshot belonging to this view, but by the time we need to associate this snapshot with a RUM context new RUM startView didn’t finish processing yet (since it is a dedicated context queue, not main), we will associate this wireframe with a wrong RUM context, from the previous RUM view.

However, it is not a problem, because a miss will be very small and is not visible in the Session Replay player when switching between RUM views on the timeline.

Moreover, even with the current approach (by using getFeatureContext()) such mismatch is possible due to the concurrent nature of the event processing.

In general, this cannot be solved, because Session Replay and RUM tracking strategies are using different callbacks to report the data, so it cannot be aligned perfectly.

Also this PR does a minor change to the context update subscription mechanism: if there is a context available for the feature, it will be immediately emitted to the listener upon subscription.

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)

@0xnm
0xnm force-pushed the nogorodnikov/rum-10064/read-rum-context-in-non-blocking-manner-in-session-replay branch from aa32f36 to 3eadf7d Compare May 21, 2025 12:58
@0xnm
0xnm marked this pull request as ready for review May 21, 2025 13:16
@0xnm
0xnm requested review from a team as code owners May 21, 2025 13:16
@codecov-commenter

codecov-commenter commented May 21, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.09%. Comparing base (8893ced) to head (3eadf7d).
⚠️ Report is 165 commits behind head on nogorodnikov/rum-8170/feature-context-sync.

Files with missing lines Patch % Lines
...n/com/datadog/android/core/internal/DatadogCore.kt 75.00% 0 Missing and 1 partial ⚠️
...replay/internal/SessionReplayRumContextProvider.kt 90.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@                              Coverage Diff                               @@
##           nogorodnikov/rum-8170/feature-context-sync    #2666      +/-   ##
==============================================================================
+ Coverage                                       70.08%   70.09%   +0.02%     
==============================================================================
  Files                                             820      819       -1     
  Lines                                           30630    30620      -10     
  Branches                                         5118     5119       +1     
==============================================================================
- Hits                                            21464    21462       -2     
+ Misses                                           7754     7731      -23     
- Partials                                         1412     1427      +15     
Files with missing lines Coverage Δ
.../sessionreplay/internal/DefaultRecorderProvider.kt 93.80% <100.00%> (ø)
...oid/sessionreplay/internal/SessionReplayFeature.kt 98.84% <100.00%> (+0.03%) ⬆️
...replay/internal/processor/RumContextDataHandler.kt 100.00% <100.00%> (ø)
...nreplay/internal/recorder/SessionReplayRecorder.kt 95.28% <100.00%> (+0.31%) ⬆️
...lay/internal/recorder/WindowCallbackInterceptor.kt 100.00% <100.00%> (ø)
...ternal/recorder/callback/RecorderWindowCallback.kt 92.05% <100.00%> (+0.09%) ⬆️
...onreplay/internal/utils/SessionReplayRumContext.kt 90.00% <100.00%> (+8.18%) ⬆️
.../android/webview/internal/rum/WebViewRumFeature.kt 100.00% <ø> (ø)
...n/com/datadog/android/core/internal/DatadogCore.kt 82.28% <75.00%> (-0.12%) ⬇️
...replay/internal/SessionReplayRumContextProvider.kt 90.91% <90.00%> (-9.09%) ⬇️

... and 38 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ambushwork ambushwork left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@0xnm
0xnm merged commit 7737285 into nogorodnikov/rum-8170/feature-context-sync May 22, 2025
@0xnm
0xnm deleted the nogorodnikov/rum-10064/read-rum-context-in-non-blocking-manner-in-session-replay branch May 22, 2025 14:07

override fun onStop() {
stopRecording()
sdkCore.removeContextUpdateReceiver(Feature.RUM_FEATURE_NAME, rumContextProvider)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't there a mistake here ? I think you are not using the same key right to register and remove ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants