From bc1b8837bd14e56eb7133c4049ae87a4364dbb1c Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 10 Aug 2023 11:11:16 +0200 Subject: [PATCH 1/7] fix(replay): Ensure we do not try to flush when we force stop replay --- .../replay/eventBufferError/template.html | 10 +++ .../suites/replay/eventBufferError/test.ts | 80 +++++++++++++++++++ packages/replay/src/integration.ts | 2 +- packages/replay/src/replay.ts | 10 +-- packages/replay/src/types/replay.ts | 2 +- packages/replay/src/util/addEvent.ts | 2 +- .../replay/test/integration/flush.test.ts | 3 +- 7 files changed, 99 insertions(+), 10 deletions(-) create mode 100644 packages/browser-integration-tests/suites/replay/eventBufferError/template.html create mode 100644 packages/browser-integration-tests/suites/replay/eventBufferError/test.ts diff --git a/packages/browser-integration-tests/suites/replay/eventBufferError/template.html b/packages/browser-integration-tests/suites/replay/eventBufferError/template.html new file mode 100644 index 000000000000..24fc4828baf1 --- /dev/null +++ b/packages/browser-integration-tests/suites/replay/eventBufferError/template.html @@ -0,0 +1,10 @@ + + + + + + + + + + diff --git a/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts b/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts new file mode 100644 index 000000000000..8e80e4b33532 --- /dev/null +++ b/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts @@ -0,0 +1,80 @@ +import { expect } from '@playwright/test'; + +import { sentryTest } from '../../../utils/fixtures'; +import { envelopeRequestParser } from '../../../utils/helpers'; +import { + getReplaySnapshot, + isReplayEvent, + REPLAY_DEFAULT_FLUSH_MAX_DELAY, + shouldSkipReplayTest, + waitForReplayRequest, +} from '../../../utils/replayHelpers'; + +sentryTest( + 'should stop recording when running into eventBuffer error', + async ({ getLocalTestPath, page, forceFlushReplay }) => { + if (shouldSkipReplayTest()) { + sentryTest.skip(); + } + + let called = 0; + + await page.route('https://dsn.ingest.sentry.io/**/*', route => { + const event = envelopeRequestParser(route.request()); + + // We only want to count replays here + if (event && isReplayEvent(event)) { + called++; + } + + return route.fulfill({ + status: 200, + }); + }); + + const url = await getLocalTestPath({ testDir: __dirname }); + await page.goto(url); + + await waitForReplayRequest(page); + + expect(called).toBe(1); + const replay = await getReplaySnapshot(page); + expect(replay._isEnabled).toBe(true); + + await forceFlushReplay(); + + called = 0; + + /** + * We test the following here: + * 1. First click should add an event (so the eventbuffer is not empty) + * 2. Second click should throw an error in eventBuffer (which should lead to stopping the replay) + * 3. Nothing should be sent to API, as we stop the replay due to the eventBuffer error. + */ + await page.evaluate(` +window._count = 0; +window._addEvent = window.Replay._replay.eventBuffer.addEvent.bind(window.Replay._replay.eventBuffer); +window.Replay._replay.eventBuffer.addEvent = (...args) => { + window._count++; + if (window._count === 2) { + throw new Error('provoked error'); + } + window._addEvent(...args); +}; +`); + + void page.click('#button1'); + void page.click('#button2'); + + // Should immediately skip retrying and just cancel, no backoff + // This waitForTimeout call should be okay, as we're not checking for any + // further network requests afterwards. + await page.waitForTimeout(REPLAY_DEFAULT_FLUSH_MAX_DELAY + 100); + + expect(called).toBe(0); + + const replay2 = await getReplaySnapshot(page); + + expect(replay2._isEnabled).toBe(false); + }, +); diff --git a/packages/replay/src/integration.ts b/packages/replay/src/integration.ts index 15a39391636c..e4c17ea04ba1 100644 --- a/packages/replay/src/integration.ts +++ b/packages/replay/src/integration.ts @@ -263,7 +263,7 @@ Sentry.init({ replaysOnErrorSampleRate: ${errorSampleRate} })`, return Promise.resolve(); } - return this._replay.stop(); + return this._replay.stop({ forceFlush: this._replay.recordingMode === 'session' }); } /** diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts index 4a0a7e91d97d..6bba4eb66a0a 100644 --- a/packages/replay/src/replay.ts +++ b/packages/replay/src/replay.ts @@ -368,7 +368,7 @@ export class ReplayContainer implements ReplayContainerInterface { * Currently, this needs to be manually called (e.g. for tests). Sentry SDK * does not support a teardown */ - public async stop(reason?: string): Promise { + public async stop({ forceFlush = false, reason }: { forceFlush?: boolean; reason?: string } = {}): Promise { if (!this._isEnabled) { return; } @@ -388,7 +388,7 @@ 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. - if (this.recordingMode === 'session') { + if (forceFlush) { await this._flush({ force: true }); } @@ -777,7 +777,7 @@ export class ReplayContainer implements ReplayContainerInterface { this.session = session; if (!this.session.sampled) { - void this.stop('session not refreshed'); + void this.stop({ reason: 'session not refreshed' }); return false; } @@ -1099,7 +1099,7 @@ export class ReplayContainer implements ReplayContainerInterface { // This means we retried 3 times and all of them failed, // or we ran into a problem we don't want to retry, like rate limiting. // In this case, we want to completely stop the replay - otherwise, we may get inconsistent segments - void this.stop('sendReplay'); + void this.stop({ reason: 'sendReplay' }); const client = getCurrentHub().getClient(); @@ -1223,7 +1223,7 @@ export class ReplayContainer implements ReplayContainerInterface { // Stop replay if over the mutation limit if (overMutationLimit) { - void this.stop('mutationLimit'); + void this.stop({ reason: 'mutationLimit', forceFlush: this.recordingMode === 'session' }); return false; } diff --git a/packages/replay/src/types/replay.ts b/packages/replay/src/types/replay.ts index 1fbf44aa1b95..00f363003979 100644 --- a/packages/replay/src/types/replay.ts +++ b/packages/replay/src/types/replay.ts @@ -451,7 +451,7 @@ export interface ReplayContainer { getContext(): InternalEventContext; initializeSampling(): void; start(): void; - stop(reason?: string): Promise; + stop(options?: { reason?: string; forceflush?: boolean }): Promise; pause(): void; resume(): void; startRecording(): void; diff --git a/packages/replay/src/util/addEvent.ts b/packages/replay/src/util/addEvent.ts index e9898ee9461c..9024c3cbb6bd 100644 --- a/packages/replay/src/util/addEvent.ts +++ b/packages/replay/src/util/addEvent.ts @@ -71,7 +71,7 @@ export async function addEvent( const reason = error && error instanceof EventBufferSizeExceededError ? 'addEventSizeExceeded' : 'addEvent'; __DEBUG_BUILD__ && logger.error(error); - await replay.stop(reason); + await replay.stop({ reason }); const client = getCurrentHub().getClient(); diff --git a/packages/replay/test/integration/flush.test.ts b/packages/replay/test/integration/flush.test.ts index 1e0b3b1366d6..29ce2ba527fd 100644 --- a/packages/replay/test/integration/flush.test.ts +++ b/packages/replay/test/integration/flush.test.ts @@ -472,10 +472,9 @@ describe('Integration | flush', () => { const TEST_EVENT = { data: {}, timestamp: BASE_TIMESTAMP + 100, type: 2 }; mockRecord._emitter(TEST_EVENT); - await advanceTimers(DEFAULT_FLUSH_MIN_DELAY); await advanceTimers(160_000); - expect(mockFlush).toHaveBeenCalledTimes(2); + expect(mockFlush).toHaveBeenCalledTimes(1); expect(mockSendReplay).toHaveBeenCalledTimes(0); expect(replay.isEnabled()).toBe(false); From afa9602fe39158a095744cb382330ee3f11c74b6 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 10 Aug 2023 14:52:09 +0200 Subject: [PATCH 2/7] add debugging... --- .../suites/replay/eventBufferError/test.ts | 32 ++++++++++++------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts b/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts index 8e80e4b33532..e0958dbc9399 100644 --- a/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts +++ b/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts @@ -17,16 +17,7 @@ sentryTest( sentryTest.skip(); } - let called = 0; - await page.route('https://dsn.ingest.sentry.io/**/*', route => { - const event = envelopeRequestParser(route.request()); - - // We only want to count replays here - if (event && isReplayEvent(event)) { - called++; - } - return route.fulfill({ status: 200, }); @@ -36,13 +27,32 @@ sentryTest( await page.goto(url); await waitForReplayRequest(page); - - expect(called).toBe(1); const replay = await getReplaySnapshot(page); expect(replay._isEnabled).toBe(true); await forceFlushReplay(); + let called = 0; + + await page.route('https://dsn.ingest.sentry.io/**/*', route => { + const event = envelopeRequestParser(route.request()); + + // We only want to count replays here + if (event && isReplayEvent(event)) { + called++; + } + + // TODO FN: Just for debugging.... + if (called > 0) { + // eslint-disable-next-line no-console + console.log(event); + } + + return route.fulfill({ + status: 200, + }); + }); + called = 0; /** From 088449916e41c5aee4b2d04612d4f92338ecab9b Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 10 Aug 2023 15:06:23 +0200 Subject: [PATCH 3/7] moare debugging --- .../suites/replay/eventBufferError/test.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts b/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts index e0958dbc9399..3d42abf21d16 100644 --- a/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts +++ b/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts @@ -8,6 +8,7 @@ import { REPLAY_DEFAULT_FLUSH_MAX_DELAY, shouldSkipReplayTest, waitForReplayRequest, + getReplayRecordingContent, } from '../../../utils/replayHelpers'; sentryTest( @@ -46,6 +47,10 @@ sentryTest( if (called > 0) { // eslint-disable-next-line no-console console.log(event); + + const recordingEvents = getReplayRecordingContent(route.request()); + // eslint-disable-next-line no-console + console.log(recordingEvents); } return route.fulfill({ From 703eb0896a5dd12cc8a517a339213b7ab774bbb9 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 10 Aug 2023 15:18:01 +0200 Subject: [PATCH 4/7] even more debugging --- .../suites/replay/eventBufferError/test.ts | 4 ++-- packages/browser-integration-tests/utils/replayHelpers.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts b/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts index 3d42abf21d16..b7cff3210fe6 100644 --- a/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts +++ b/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts @@ -8,7 +8,7 @@ import { REPLAY_DEFAULT_FLUSH_MAX_DELAY, shouldSkipReplayTest, waitForReplayRequest, - getReplayRecordingContent, + getDecompressedRecordingEvents, } from '../../../utils/replayHelpers'; sentryTest( @@ -48,7 +48,7 @@ sentryTest( // eslint-disable-next-line no-console console.log(event); - const recordingEvents = getReplayRecordingContent(route.request()); + const recordingEvents = getDecompressedRecordingEvents(route.request()); // eslint-disable-next-line no-console console.log(recordingEvents); } diff --git a/packages/browser-integration-tests/utils/replayHelpers.ts b/packages/browser-integration-tests/utils/replayHelpers.ts index cb05baffb62b..f74f3e8e4313 100644 --- a/packages/browser-integration-tests/utils/replayHelpers.ts +++ b/packages/browser-integration-tests/utils/replayHelpers.ts @@ -267,7 +267,7 @@ function getOptionsEvents(replayRequest: Request): CustomRecordingEvent[] { return getAllCustomRrwebRecordingEvents(events).filter(data => data.tag === 'options'); } -function getDecompressedRecordingEvents(resOrReq: Request | Response): RecordingSnapshot[] { +export function getDecompressedRecordingEvents(resOrReq: Request | Response): RecordingSnapshot[] { const replayRequest = getRequest(resOrReq); return ( (replayEnvelopeRequestParser(replayRequest, 5) as eventWithTime[]) From 278b12dc57dbdf8f9def2c872e293225625bf65a Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 10 Aug 2023 15:37:42 +0200 Subject: [PATCH 5/7] again the debugging... --- .../suites/replay/eventBufferError/test.ts | 4 ++-- packages/browser-integration-tests/utils/replayHelpers.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts b/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts index b7cff3210fe6..592ccdb45202 100644 --- a/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts +++ b/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts @@ -8,7 +8,7 @@ import { REPLAY_DEFAULT_FLUSH_MAX_DELAY, shouldSkipReplayTest, waitForReplayRequest, - getDecompressedRecordingEvents, + replayEnvelopeParser, } from '../../../utils/replayHelpers'; sentryTest( @@ -48,7 +48,7 @@ sentryTest( // eslint-disable-next-line no-console console.log(event); - const recordingEvents = getDecompressedRecordingEvents(route.request()); + const recordingEvents = replayEnvelopeParser(route.request()); // eslint-disable-next-line no-console console.log(recordingEvents); } diff --git a/packages/browser-integration-tests/utils/replayHelpers.ts b/packages/browser-integration-tests/utils/replayHelpers.ts index f74f3e8e4313..0e8b20028b0d 100644 --- a/packages/browser-integration-tests/utils/replayHelpers.ts +++ b/packages/browser-integration-tests/utils/replayHelpers.ts @@ -267,7 +267,7 @@ function getOptionsEvents(replayRequest: Request): CustomRecordingEvent[] { return getAllCustomRrwebRecordingEvents(events).filter(data => data.tag === 'options'); } -export function getDecompressedRecordingEvents(resOrReq: Request | Response): RecordingSnapshot[] { +function getDecompressedRecordingEvents(resOrReq: Request | Response): RecordingSnapshot[] { const replayRequest = getRequest(resOrReq); return ( (replayEnvelopeRequestParser(replayRequest, 5) as eventWithTime[]) @@ -302,7 +302,7 @@ const replayEnvelopeRequestParser = (request: Request | null, envelopeIndex = 2) return envelope[envelopeIndex] as Event; }; -const replayEnvelopeParser = (request: Request | null): unknown[] => { +export const replayEnvelopeParser = (request: Request | null): unknown[] => { // https://develop.sentry.dev/sdk/envelopes/ const envelopeBytes = request?.postDataBuffer() || ''; From 26cb98b04915ff2086248e52502c3ba5cb317670 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 10 Aug 2023 15:56:13 +0200 Subject: [PATCH 6/7] MORE logging... --- .../suites/replay/eventBufferError/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts b/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts index 592ccdb45202..b93f8cce4968 100644 --- a/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts +++ b/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts @@ -50,7 +50,7 @@ sentryTest( const recordingEvents = replayEnvelopeParser(route.request()); // eslint-disable-next-line no-console - console.log(recordingEvents); + console.log(JSON.stringify(recordingEvents, null, 2)); } return route.fulfill({ From 81ed9bdb8aa9ce12bb7bd6369eb34d8265f92d64 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 10 Aug 2023 16:35:22 +0200 Subject: [PATCH 7/7] fix it?? --- .../suites/replay/eventBufferError/test.ts | 18 ++++++------------ .../utils/replayHelpers.ts | 2 +- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts b/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts index b93f8cce4968..10e9ad6f7196 100644 --- a/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts +++ b/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts @@ -3,12 +3,12 @@ import { expect } from '@playwright/test'; import { sentryTest } from '../../../utils/fixtures'; import { envelopeRequestParser } from '../../../utils/helpers'; import { + getDecompressedRecordingEvents, getReplaySnapshot, isReplayEvent, REPLAY_DEFAULT_FLUSH_MAX_DELAY, shouldSkipReplayTest, waitForReplayRequest, - replayEnvelopeParser, } from '../../../utils/replayHelpers'; sentryTest( @@ -40,17 +40,11 @@ sentryTest( // We only want to count replays here if (event && isReplayEvent(event)) { - called++; - } - - // TODO FN: Just for debugging.... - if (called > 0) { - // eslint-disable-next-line no-console - console.log(event); - - const recordingEvents = replayEnvelopeParser(route.request()); - // eslint-disable-next-line no-console - console.log(JSON.stringify(recordingEvents, null, 2)); + const events = getDecompressedRecordingEvents(route.request()); + // this makes sure we ignore e.g. mouse move events which can otherwise lead to flakes + if (events.length > 0) { + called++; + } } return route.fulfill({ diff --git a/packages/browser-integration-tests/utils/replayHelpers.ts b/packages/browser-integration-tests/utils/replayHelpers.ts index 0e8b20028b0d..5ddcf0c46e05 100644 --- a/packages/browser-integration-tests/utils/replayHelpers.ts +++ b/packages/browser-integration-tests/utils/replayHelpers.ts @@ -267,7 +267,7 @@ function getOptionsEvents(replayRequest: Request): CustomRecordingEvent[] { return getAllCustomRrwebRecordingEvents(events).filter(data => data.tag === 'options'); } -function getDecompressedRecordingEvents(resOrReq: Request | Response): RecordingSnapshot[] { +export function getDecompressedRecordingEvents(resOrReq: Request | Response): RecordingSnapshot[] { const replayRequest = getRequest(resOrReq); return ( (replayEnvelopeRequestParser(replayRequest, 5) as eventWithTime[])