From 44ae0487fd6cf807a1a01a0756601c6a1186490d Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 23 May 2023 14:06:22 -0400 Subject: [PATCH] fix(replay): Fix buffered replays creating replay w/o error occuring (#8168) Introduced in #7741. This happens when session replay rate is > 0 and the session is unsampled, it calls `stop()` which ends up flushing without checking the recording mode (session vs buffer). Closes #8054 --- packages/replay/jest.setup.ts | 26 +++++++++++++++++-- packages/replay/src/replay.ts | 4 ++- .../errorSampleRate-delayFlush.test.ts | 17 +++++++++++- .../test/integration/errorSampleRate.test.ts | 18 +++++++++++-- 4 files changed, 59 insertions(+), 6 deletions(-) diff --git a/packages/replay/jest.setup.ts b/packages/replay/jest.setup.ts index b485f3c882d1..665d29e2386a 100644 --- a/packages/replay/jest.setup.ts +++ b/packages/replay/jest.setup.ts @@ -128,6 +128,24 @@ function checkCallForSentReplay( }; } +/** +* Only want calls that send replay events, i.e. ignore error events +*/ +function getReplayCalls(calls: any[][][]): any[][][] { + return calls.map(call => { + const arg = call[0]; + if (arg.length !== 2) { + return []; + } + + if (!arg[1][0].find(({type}: {type: string}) => ['replay_event', 'replay_recording'].includes(type))) { + return []; + } + + return [ arg ]; + }).filter(Boolean); +} + /** * Checks all calls to `fetch` and ensures a replay was uploaded by * checking the `fetch()` request's body. @@ -143,7 +161,9 @@ const toHaveSentReplay = function ( const expectedKeysLength = expected ? ('sample' in expected ? Object.keys(expected.sample) : Object.keys(expected)).length : 0; - for (const currentCall of calls) { + const replayCalls = getReplayCalls(calls) + + for (const currentCall of replayCalls) { result = checkCallForSentReplay.call(this, currentCall[0], expected); if (result.pass) { break; @@ -193,7 +213,9 @@ const toHaveLastSentReplay = function ( expected?: SentReplayExpected | { sample: SentReplayExpected; inverse: boolean }, ) { const { calls } = (getCurrentHub().getClient()?.getTransport()?.send as MockTransport).mock; - const lastCall = calls[calls.length - 1]?.[0]; + const replayCalls = getReplayCalls(calls) + + const lastCall = replayCalls[calls.length - 1]?.[0]; const { results, call, pass } = checkCallForSentReplay.call(this, lastCall, expected); diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index e45830e9fdde..52c772f57c21 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -327,7 +327,9 @@ export class ReplayContainer implements ReplayContainerInterface { this._debouncedFlush.cancel(); // See comment above re: `_isEnabled`, we "force" a flush, ignoring the // `_isEnabled` state of the plugin since it was disabled above. - await this._flush({ force: true }); + if (this.recordingMode === 'session') { + await this._flush({ force: true }); + } // After flush, destroy event buffer this.eventBuffer && this.eventBuffer.destroy(); diff --git a/packages/replay/test/integration/errorSampleRate-delayFlush.test.ts b/packages/replay/test/integration/errorSampleRate-delayFlush.test.ts index c0b711c028f8..20645b1b85a4 100644 --- a/packages/replay/test/integration/errorSampleRate-delayFlush.test.ts +++ b/packages/replay/test/integration/errorSampleRate-delayFlush.test.ts @@ -229,6 +229,21 @@ describe('Integration | errorSampleRate with delayed flush', () => { }); }); + // This tests a regression where we were calling flush indiscriminantly in `stop()` + it('does not upload a replay event if error is not sampled', async () => { + // We are trying to replicate the case where error rate is 0 and session + // rate is > 0, we can't set them both to 0 otherwise + // `_loadAndCheckSession` is not called when initializing the plugin. + replay.stop(); + replay['_options']['errorSampleRate'] = 0; + replay['_loadAndCheckSession'](); + + jest.runAllTimers(); + await new Promise(process.nextTick); + expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); + expect(replay).not.toHaveLastSentReplay(); + }); + it('does not send a replay when triggering a full dom snapshot when document becomes visible after [SESSION_IDLE_EXPIRE_DURATION]ms', async () => { Object.defineProperty(document, 'visibilityState', { configurable: true, @@ -664,7 +679,7 @@ describe('Integration | errorSampleRate with delayed flush', () => { jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY); await new Promise(process.nextTick); - expect(replay).toHaveLastSentReplay(); + expect(replay).not.toHaveLastSentReplay(); // Wait a bit, shortly before session expires jest.advanceTimersByTime(MAX_SESSION_LIFE - 1000); diff --git a/packages/replay/test/integration/errorSampleRate.test.ts b/packages/replay/test/integration/errorSampleRate.test.ts index 74fde11f50f0..3145ba37e7f9 100644 --- a/packages/replay/test/integration/errorSampleRate.test.ts +++ b/packages/replay/test/integration/errorSampleRate.test.ts @@ -234,6 +234,21 @@ describe('Integration | errorSampleRate', () => { }); }); + // This tests a regression where we were calling flush indiscriminantly in `stop()` + it('does not upload a replay event if error is not sampled', async () => { + // We are trying to replicate the case where error rate is 0 and session + // rate is > 0, we can't set them both to 0 otherwise + // `_loadAndCheckSession` is not called when initializing the plugin. + replay.stop(); + replay['_options']['errorSampleRate'] = 0; + replay['_loadAndCheckSession'](); + + jest.runAllTimers(); + await new Promise(process.nextTick); + expect(mockRecord.takeFullSnapshot).not.toHaveBeenCalled(); + expect(replay).not.toHaveLastSentReplay(); + }); + it('does not send a replay when triggering a full dom snapshot when document becomes visible after [SESSION_IDLE_EXPIRE_DURATION]ms', async () => { Object.defineProperty(document, 'visibilityState', { configurable: true, @@ -668,8 +683,7 @@ describe('Integration | errorSampleRate', () => { jest.advanceTimersByTime(DEFAULT_FLUSH_MIN_DELAY); await new Promise(process.nextTick); - - expect(replay).toHaveLastSentReplay(); + expect(replay).not.toHaveLastSentReplay(); // Wait a bit, shortly before session expires jest.advanceTimersByTime(MAX_SESSION_LIFE - 1000);