-
Notifications
You must be signed in to change notification settings - Fork 136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
⚡ [RUM-116] On view change, take the full snapshot asynchronously #2887
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2887 +/- ##
==========================================
+ Coverage 93.68% 93.71% +0.03%
==========================================
Files 266 268 +2
Lines 7584 7623 +39
Branches 1687 1692 +5
==========================================
+ Hits 7105 7144 +39
Misses 479 479 ☔ View full report in Codecov by Sentry. |
/to-staging |
🚂 Branch Integration: starting soon, median merge time is 12m Commit 479736c960 will soon be integrated into staging-30. Use |
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
…ng-30 Integrated commit sha: 479736c Co-authored-by: roman.gaignault <[email protected]>
🚂 Branch Integration: This commit was successfully integrated Commit 479736c960 has been merged into staging-30 in merge commit cc5e316eee. Check out the triggered pipeline on Gitlab 🦊 |
|
||
let cancelIdleCallback: (() => void) | undefined | ||
const { unsubscribe } = lifeCycle.subscribe(LifeCycleEventType.VIEW_CREATED, (view) => { | ||
flushMutations() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ question: Is this flushMutations()
necessary?
sendToExtension('record', { record }) | ||
const view = options.viewContexts.findView()! | ||
replayStats.addRecord(view.id) | ||
if (!isFullSnapshotPending) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ question: By discarding records during the fullsnapshot are we confident not to miss important ones like scroll, mouseInteraction, move?
Motivation
In the Datadog App, we are starting a new view when the URL changes (via the onChange react-router callback). But at this time, the old page component is still displayed, and we take a full snapshot of the ending view instead of the new view.
Changes
A solution would be to take the fullsnapshot asynchronously. This would let a bit of time to the application UI to be updated. While waiting for the FS to be taken, we could ignore any emitted record, since we would know that a FS is coming to capture the whole state:
startView() is called
ignore any emitted record
[async] wait a bit
if stopSessionReplayRecording() was not called in the meantime
full snapshot is taken
start collecting emitted record again
For the asynchronous delay, we could even use requestIdleCallback to limit our impact on the application perceptible performance.
Testing
I have gone over the contributing documentation.