fix(replay): Ensure handleRecordingEmit aborts when event is not added
#8938
Conversation
| // Skip all further steps | ||
| if (!addEventSync(replay, event, isCheckout)) { | ||
| // Return true to skip scheduling a debounced flush | ||
| return true; |
There was a problem hiding this comment.
This API for addUpdate is always confusing for me, I always have to double check what I should return to do what 😅 Nothing to do but maybe we can find a clearer API for this at some point.
size-limit report 📦
|
ae7e13e to
470cc36
Compare
| @@ -816,73 +816,6 @@ describe('Integration | errorSampleRate', () => { | |||
| expect(replay).not.toHaveLastSentReplay(); | |||
| }); | |||
|
|
|||
| it('does not stop replay based on earliest event in buffer', async () => { | |||
There was a problem hiding this comment.
I removed this test as honestly I am not 100% sure what it is trying to prove/achieve 😅 it failed here due to timing issues, I first tried to change the timing etc. but ended up not really testing that much meaningful stuff.
There was a problem hiding this comment.
I think it's testing that old contents in the buffer do not stop the replay recording
There was a problem hiding this comment.
I think it's OK to delete this test as this is not really so relevant nowadays I think - as generally the earliest event in the buffer will be the initial timestamp that we check against...? Also this is not related to error sampled sessions anymore, at this point where a session may be stopped it is already a "session" session. Or do you want to keep this test around somehow?
There was a problem hiding this comment.
Was trying to explain the intention behind it -- I'm okay w/ removing it!
| @@ -816,73 +816,6 @@ describe('Integration | errorSampleRate', () => { | |||
| expect(replay).not.toHaveLastSentReplay(); | |||
| }); | |||
|
|
|||
| it('does not stop replay based on earliest event in buffer', async () => { | |||
There was a problem hiding this comment.
I think it's testing that old contents in the buffer do not stop the replay recording
packages/replay/src/util/addEvent.ts
Outdated
| @@ -81,6 +88,33 @@ export async function addEvent( | |||
| } | |||
| } | |||
|
|
|||
| function shouldAddEvent(replay: ReplayContainer, event: RecordingEvent): boolean { | |||
There was a problem hiding this comment.
Might be good to have a unit test for this
b828899 to
460595a
Compare
460595a to
0c1b036
Compare
I noticed that in
handleRecordingEmit, due to the async nature ofaddEventit could happen that an event is actually not added (because it is discarded due to timestamp etc.). But we are still updating the initial session timestamp ([Replay] Updating session start time to earliest event in buffer to...), as we don't actually abort there.This PR changes this to actually abort
handleRecordingEmitin this case. I added anaddEventSyncmethod for this that just returns true/false instead of a promise, which should not change anything there as we haven't been waiting for the result of the promise anyhow.