Skip to content

Commit

Permalink
fix(replay): Fix buffered replays creating replay w/o error occuring (#…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
billyvg committed May 23, 2023
1 parent ce8fc32 commit 44ae048
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 6 deletions.
26 changes: 24 additions & 2 deletions packages/replay/jest.setup.ts
Expand Up @@ -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.
Expand All @@ -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;
Expand Down Expand Up @@ -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);

Expand Down
4 changes: 3 additions & 1 deletion packages/replay/src/replay.ts
Expand Up @@ -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();
Expand Down
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down
18 changes: 16 additions & 2 deletions packages/replay/test/integration/errorSampleRate.test.ts
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 44ae048

Please sign in to comment.