Repository navigation
Ascending timestamps - #558
eoghanmurray wants to merge 7 commits into
Conversation
|
The downside of this pull request is that now long mouse movements can potentially be split up into many shorter events. A danger would be on a page which has many ongoing background mutations ... let me know if that is a concern. |
d01b44f to
17c9efd
Compare
146cb14 to
0f1780b
Compare
| const wrappedMutationEmit = (m: mutationCallbackParam) => { | ||
| const wrappedMutationEmit = (m: mutationCallbackParam, timestamp?: number) => { | ||
| if (typeof timestamp === 'undefined') { | ||
| timestamp = Date.now() |
There was a problem hiding this comment.
why mutation emit need to create timestamp by itself while other callback still using the wrappedEmit method?
There was a problem hiding this comment.
This should be written without a timestamp = Date.now(); in this function; I will rewrite it now and force push this and prior change.
There was a problem hiding this comment.
I've made that change now, but maybe you meant something else?
0f1780b to
9c582aa
Compare
Done in updated PR
In replay, every position is queued independently regardless of whether positions [P1,P2,P3] arrive as one event or three, each P lands in the timer at the same instant with the same (x,y)"
This is actually how it always(?) worked; the first mouse movement is emitted immediately with one position |
9c582aa to
35286c1
Compare
|
Can retry (rebase on master) after #1475 is merged as I incorporated some of this PR into that PR |
…sumed had been merged - it allows the timestamps to be set outside of the emission event
…crementalSnapshot (mutation) that generated them. The logic is that this is the time they were created, even if capture was delayed at record time. If sorting by timestamp is performed before replay, then so long as the sort algorithm is stable, this will bring the asset events closer to the snapshot that they are associated with, where they are needed for replay Note: This is a rework of a subset of the code from unmerged rrweb-io#558 allowing timestamps to be set outside of the emission event
…crementalSnapshot (mutation) that generated them. The logic is that this is the time they were created, even if capture was delayed at record time. If sorting by timestamp is performed before replay, then so long as the sort algorithm is stable, this will bring the asset events closer to the snapshot that they are associated with, where they are needed for replay Note: This is a rework of a subset of the code from unmerged rrweb-io#558 allowing timestamps to be set outside of the emission event
…crementalSnapshot (mutation) that generated them. The logic is that this is the time they were created, even if capture was delayed at record time. If sorting by timestamp is performed before replay, then so long as the sort algorithm is stable, this will bring the asset events closer to the snapshot that they are associated with, where they are needed for replay Note: This is a rework of a subset of the code from unmerged rrweb-io#558 allowing timestamps to be set outside of the emission event
35286c1 to
fe697c9
Compare
🦋 Changeset detectedLatest commit: 614c50f The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@rrweb/all
@rrweb/browser-client
@rrweb/packer
@rrweb/record
@rrweb/replay
rrdom
rrdom-nodejs
rrvideo
rrweb
rrweb-player
rrweb-snapshot
@rrweb/types
@rrweb/utils
@rrweb/rrweb-plugin-canvas-webrtc-record
@rrweb/rrweb-plugin-canvas-webrtc-replay
@rrweb/rrweb-plugin-console-record
@rrweb/rrweb-plugin-console-replay
@rrweb/rrweb-plugin-network-record
@rrweb/rrweb-plugin-network-replay
@rrweb/rrweb-plugin-sequential-id-record
@rrweb/rrweb-plugin-sequential-id-replay
commit: |
Answering my own 2021 comment above: I did some quantitative analysis on this on a corpus of rrweb events with this PR deployed, and got the AI to reconstruct how many batched events had been split up that would not have been before (based on timestamp adjacency). Bottom line: this PR generated an estimated 54% more mouse move events (as anticipated) by sometimes preventing the full 500ms batching that has been there since |
…he mouse move event immediately so that there is negative timeOffsets never result in an out of sequence event stream
…timestamp sequence is (weakly) ascending. - Overruling the timestamp of `wrapEvent` here also ensures that the `Date.now()` used to calculate the negative `totalOffset` matches the event timestamp, so is presumably slightly more accurate
…akly ascending - mutationBuffer emission can be triggered by another event, so a new `Date.time()` could otherwise be larger than the timestamp of the event that triggered it
…tions unfreeze on a clean boundary Instead of back-dating the unfrozen mutation's timestamp to the start of an in-progress mouse move, make freezePage() interrupt the move like any other event: flush the pending (throttled) move immediately and reset the move emission throttle so the next movement starts on a fresh leading edge (a single position at timeOffset 0). Any later unfreeze is then triggered either by that leading-edge move or by a non-move event, neither of which needs the timestamp adjustment. Adds a cancel() to throttle() (clears the pending trailing timeout and resets its window) and threads a reset flag through ongoingMove/moveEmission. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ascending Two dedicated tests exercise the guarantee that a throttled mouse move, when flushed by an interrupting event, is emitted before that event with its (negative) position timeOffsets, so the effective timestamp sequence stays weakly ascending: - a move interrupted by a keystroke - buffered (frozen) mutations plus a move interrupted by a click These replace the assertion previously embedded in the shared stringifySnapshots helper, which no longer exercised the mouse-move path (that helper filters MouseMove events out) and imposed the check on every snapshot test. Each test fails without the interrupting-flush fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fe697c9 to
71bde89
Compare
… a delayed mutation emission is short circuit triggered by another event (another type of forcing)
2 participants
This started with the idea that we don't want 'overlapping' events, i.e. a mouse click event followed by a mousemove event whose negative timestamps put the start of the mouse movement before the click.
This is important in the logical processing of a series of events so that we don't have to consider the next event when processing the current event e.g. in building up a graphical timeline.
It could also be important in streaming 'live' events to a player, as although a mousemove can still introduce up to 500ms lag, it now can't result in an out-of-order situation where that click is played back before a mousemove that should precede it.
2026 Edit: tests added in this PR in packages/rrweb/test/record.test.ts
There seem to be no tests that recreate the mouse movement due to the mentioned difficulty with 'random' mouse movements in the headless browser.I've added a test (via util.py) to check that timestamps are weakly ascending before we strip them out, but that didn't catch anything prior to these changes.