From e0776078b2fa257dbe864e7d1afbb6788190d949 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Wed, 7 Dec 2022 12:15:50 -0500 Subject: [PATCH 1/4] feat(replay): Flush immediately on DOM checkouts We originally added this delay to flushing to prevent recording short (low value) recordings. However, this causes some issues if the user were to refresh/leave the page before the first recording segment is sent as the backend will consider the replay invalid if it does not have segment id 0 present. Change to flush immediately to try to reduce the number of replays with missing first segments. Our UI has a default duration filter to hide replays < 5 seconds. --- packages/replay/src/replay.ts | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 576ae1181a46..17cf5930dbd9 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -485,21 +485,11 @@ export class ReplayContainer implements ReplayContainerInterface { this._maybeSaveSession(); } - // If the full snapshot is due to an initial load, we will not have - // a previous session ID. In this case, we want to buffer events - // for a set amount of time before flushing. This can help avoid - // capturing replays of users that immediately close the window. - setTimeout(() => this.conditionalFlush(), this._options.initialFlushDelay); - - // Cancel any previously debounced flushes to ensure there are no [near] - // simultaneous flushes happening. The latter request should be - // insignificant in this case, so wait for additional user interaction to - // trigger a new flush. - // - // This can happen because there's no guarantee that a recording event - // happens first. e.g. a mouse click can happen and trigger a debounced - // flush before the checkout. - this._debouncedFlush?.cancel(); + // Flush immediately so that we do not miss the first segment, otherwise + // it can prevent loading on the UI. This will cause an increase in short + // replays (e.g. opening and closing a tab quickly), but these can be + // filtered on the UI. + this.flushImmediate(); return true; }); From eef0c77aae5be36ccf8b2eb6ef0e1f542a8bf703 Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Thu, 8 Dec 2022 17:45:58 -0500 Subject: [PATCH 2/4] fix tests --- packages/replay/src/replay.ts | 9 ++-- .../test/unit/index-errorSampleRate.test.ts | 52 ++++++++----------- 2 files changed, 29 insertions(+), 32 deletions(-) diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 17cf5930dbd9..098d6b0af369 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -195,9 +195,10 @@ export class ReplayContainer implements ReplayContainerInterface { this.addListeners(); - this.startRecording(); - + // Need to set as enabled before we start recording, as `record()` can trigger a flush with a new checkout this._isEnabled = true; + + this.startRecording(); } /** @@ -489,7 +490,9 @@ export class ReplayContainer implements ReplayContainerInterface { // it can prevent loading on the UI. This will cause an increase in short // replays (e.g. opening and closing a tab quickly), but these can be // filtered on the UI. - this.flushImmediate(); + if (!this._waitForError) { + void this.flushImmediate(); + } return true; }); diff --git a/packages/replay/test/unit/index-errorSampleRate.test.ts b/packages/replay/test/unit/index-errorSampleRate.test.ts index 867bf229ee1d..5284777be9d6 100644 --- a/packages/replay/test/unit/index-errorSampleRate.test.ts +++ b/packages/replay/test/unit/index-errorSampleRate.test.ts @@ -19,7 +19,6 @@ async function advanceTimers(time: number) { describe('Replay (errorSampleRate)', () => { let replay: ReplayContainer; let mockRecord: RecordMock; - let domHandler: DomHandler; beforeEach(async () => { @@ -59,6 +58,7 @@ describe('Replay (errorSampleRate)', () => { await new Promise(process.nextTick); expect(replay).toHaveSentReplay({ + recordingPayloadHeader: { segment_id: 0 }, replayEventPayload: expect.objectContaining({ tags: expect.objectContaining({ errorSampleRate: 1, @@ -86,6 +86,19 @@ describe('Replay (errorSampleRate)', () => { ]), }); + // This is from when we stop recording and start a session recording + expect(replay).toHaveLastSentReplay({ + recordingPayloadHeader: { segment_id: 1 }, + replayEventPayload: expect.objectContaining({ + tags: expect.objectContaining({ + errorSampleRate: 1, + replayType: 'error', + sessionSampleRate: 0, + }), + }), + events: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP + 5020, type: 2 }]), + }); + jest.runAllTimers(); await new Promise(process.nextTick); @@ -111,11 +124,11 @@ describe('Replay (errorSampleRate)', () => { events: JSON.stringify([ { type: 5, - timestamp: BASE_TIMESTAMP + 15000 + 40, + timestamp: BASE_TIMESTAMP + 10000 + 40, data: { tag: 'breadcrumb', payload: { - timestamp: (BASE_TIMESTAMP + 15000 + 40) / 1000, + timestamp: (BASE_TIMESTAMP + 10000 + 40) / 1000, type: 'default', category: 'ui.click', message: '', @@ -287,7 +300,7 @@ describe('Replay (errorSampleRate)', () => { jest.runAllTimers(); await new Promise(process.nextTick); - expect(replay).toHaveLastSentReplay({ + expect(replay).toHaveSentReplay({ events: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }, TEST_EVENT]), replayEventPayload: expect.objectContaining({ replay_start_timestamp: BASE_TIMESTAMP / 1000, @@ -338,7 +351,8 @@ describe('Replay (errorSampleRate)', () => { expect(replay.session?.started).toBe(BASE_TIMESTAMP + ELAPSED + 20); // Does not capture mouse click - expect(replay).toHaveLastSentReplay({ + expect(replay).toHaveSentReplay({ + recordingPayloadHeader: { segment_id: 0 }, replayEventPayload: expect.objectContaining({ // Make sure the old performance event is thrown out replay_start_timestamp: (BASE_TIMESTAMP + ELAPSED + 20) / 1000, @@ -373,12 +387,6 @@ it('sends a replay after loading the session multiple times', async () => { }, autoStart: false, }); - - const fn = getCurrentHub()?.getClient()?.getTransport()?.send; - const mockTransportSend = fn - ? (jest.spyOn(getCurrentHub().getClient()!.getTransport()!, 'send') as jest.MockedFunction) - : jest.fn(); - replay.start(); jest.runAllTimers(); @@ -393,27 +401,13 @@ it('sends a replay after loading the session multiple times', async () => { jest.runAllTimers(); await new Promise(process.nextTick); - expect(replay).toHaveLastSentReplay({ + expect(replay).toHaveentReplay({ events: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }, TEST_EVENT]), }); - mockTransportSend.mockClear(); - expect(replay).not.toHaveLastSentReplay(); - - jest.runAllTimers(); - await new Promise(process.nextTick); - jest.runAllTimers(); - await new Promise(process.nextTick); - - // New checkout when we call `startRecording` again after uploading segment - // after an error occurs + // Latest checkout when we call `startRecording` again after uploading segment + // after an error occurs (e.g. when we switch to session replay recording) expect(replay).toHaveLastSentReplay({ - events: JSON.stringify([ - { - data: { isCheckout: true }, - timestamp: BASE_TIMESTAMP + 10000 + 20, - type: 2, - }, - ]), + events: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP + 5020, type: 2 }]), }); }); From b9f73de3cc09a6a9a82d99a66b98bbfa93b3abbf Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Mon, 12 Dec 2022 14:30:53 -0500 Subject: [PATCH 3/4] update removed `waitForError` --- packages/replay/src/replay.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 098d6b0af369..9feba644a45a 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -490,7 +490,7 @@ export class ReplayContainer implements ReplayContainerInterface { // it can prevent loading on the UI. This will cause an increase in short // replays (e.g. opening and closing a tab quickly), but these can be // filtered on the UI. - if (!this._waitForError) { + if (this.recordingMode === 'session') { void this.flushImmediate(); } From ac771029adc7aefcf04499113184632122dea2ec Mon Sep 17 00:00:00 2001 From: Billy Vong Date: Tue, 13 Dec 2022 13:19:38 -0500 Subject: [PATCH 4/4] fix tests --- packages/replay/test/unit/index-errorSampleRate.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/replay/test/unit/index-errorSampleRate.test.ts b/packages/replay/test/unit/index-errorSampleRate.test.ts index 5284777be9d6..14f392aef74b 100644 --- a/packages/replay/test/unit/index-errorSampleRate.test.ts +++ b/packages/replay/test/unit/index-errorSampleRate.test.ts @@ -1,4 +1,4 @@ -import { captureException, getCurrentHub } from '@sentry/core'; +import { captureException } from '@sentry/core'; import { REPLAY_SESSION_KEY, VISIBILITY_CHANGE_TIMEOUT, WINDOW } from '../../src/constants'; import { addEvent } from '../../src/util/addEvent'; @@ -401,7 +401,7 @@ it('sends a replay after loading the session multiple times', async () => { jest.runAllTimers(); await new Promise(process.nextTick); - expect(replay).toHaveentReplay({ + expect(replay).toHaveSentReplay({ events: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }, TEST_EVENT]), });