Ascending timestamps #558
Ascending timestamps #558
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. |
eoghanmurray
force-pushed
the
ascending-timestamps
branch
3 times, most recently
from
d01b44f to
17c9efd
Compare
eoghanmurray
force-pushed
the
ascending-timestamps
branch
2 times, most recently
from
146cb14 to
0f1780b
Compare
packages/rrweb/src/record/index.ts
Outdated
| @@ -22,7 +22,10 @@ import { | |||
| import { IframeManager } from './iframe-manager'; | |||
| import { ShadowDomManager } from './shadow-dom-manager'; | |||
|
|
|||
| function wrapEvent(e: event): eventWithTime { | |||
| function wrapEvent(e: event | eventWithTime): eventWithTime { | |||
| if ((e as eventWithTime).timestamp) { | |||
There was a problem hiding this comment.
Does this get rid of type assertion?
if ('timestamp' in e) {}There was a problem hiding this comment.
Yes, thanks that is better
packages/rrweb/src/record/index.ts
Outdated
| 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?
eoghanmurray
force-pushed
the
ascending-timestamps
branch
from
0f1780b to
9c582aa
Compare
|
I definitely need to create some proper tests for this PR as it involves intricate interactions between ongoing mouse movement and interrupting mutations. Actually I haven't fully thought about whether splitting a single mouse movement event in two will have any differences in replay rendering? Even with this PR, I still think it's a good idea to emit an event at the start of mousemovent for the use of live replayers and for cleaner processing (even though that starting timestamp info is also contained in the eventual throttled event). |
| if ('positions' in e.data && e.data.positions[0].timeOffset) { | ||
| // assign the mutation timestamp to the beginning | ||
| // of mouse/touch movement | ||
| mtimestamp += e.data.positions[0].timeOffset; |
There was a problem hiding this comment.
@eoghanmurray What I get confused about is this part.
Why the position data need get timestamp from a mutation event, instead of recording by itself as the emitted?
There was a problem hiding this comment.
The scenario here is some frozen mutations being interrupted by a mouse move event.
So the mutations happened at some prior time and continued to be frozen when the mouse started moving; but were only unfrozen after the buffered mouse move event is emitted. So we are ensuring that the 'unfreeze' time is prior to the start of the first mouse movement.
I've just realized that this wouldn't be necessary if we added that placeholder event for 'mouse movement started' as briefly discussed previously.
There was a problem hiding this comment.
Yes, that will be a little neater.
Would like to do it in this PR or in the future?
…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
…n to a mutation 'unfreeze' event takes into account any negative timeOffset because of mouse movement buffering
…r the timestamp attribute
…mousemove initiated 3. mouseclick, then ensure that the mutation emission gets the timestamp of when the mousemove started
eoghanmurray
force-pushed
the
ascending-timestamps
branch
from
9c582aa to
35286c1
Compare
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.
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. Any advice on how to increase the test coverage for this commit appreciated!