From 745d848b59990b7211996f69d8d816121d94369f Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 14 Oct 2024 10:00:43 +0200 Subject: [PATCH 1/2] fix(replay): Ignore older performance entries when starting manually --- .../suites/replay/bufferModeManual/test.ts | 106 ++++++++++++++++++ packages/replay-internal/src/replay.ts | 14 ++- 2 files changed, 119 insertions(+), 1 deletion(-) diff --git a/dev-packages/browser-integration-tests/suites/replay/bufferModeManual/test.ts b/dev-packages/browser-integration-tests/suites/replay/bufferModeManual/test.ts index 09fc602d92b7..1de3e800999d 100644 --- a/dev-packages/browser-integration-tests/suites/replay/bufferModeManual/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/bufferModeManual/test.ts @@ -287,6 +287,112 @@ sentryTest( }, ); +sentryTest( + '[buffer-mode] manually starting replay ignores earlier performance entries', + async ({ getLocalTestUrl, page, browserName }) => { + // This was sometimes flaky on webkit, so skipping for now + if (shouldSkipReplayTest() || browserName === 'webkit') { + sentryTest.skip(); + } + + const reqPromise0 = waitForReplayRequest(page, 0); + + await page.route('https://dsn.ingest.sentry.io/**/*', route => { + return route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ id: 'test-id' }), + }); + }); + + const url = await getLocalTestUrl({ testDir: __dirname }); + + await page.goto(url); + + // Wait for everything to be initialized - Replay is not running yet + await page.waitForFunction('!!window.Replay'); + + // Wait for a second, then start replay + await new Promise(resolve => setTimeout(resolve, 1000)); + await page.evaluate('window.Replay.start()'); + + const req0 = await reqPromise0; + + const event0 = getReplayEvent(req0); + const content0 = getReplayRecordingContent(req0); + + expect(event0).toEqual( + getExpectedReplayEvent({ + replay_type: 'session', + }), + ); + + const { performanceSpans } = content0; + + // Here, we test that this does not contain any web-vital etc. performance spans + // as these have been emitted _before_ the replay was manually started + expect(performanceSpans).toEqual([ + { + op: 'memory', + description: 'memory', + startTimestamp: expect.any(Number), + endTimestamp: expect.any(Number), + data: { + memory: { + jsHeapSizeLimit: expect.any(Number), + totalJSHeapSize: expect.any(Number), + usedJSHeapSize: expect.any(Number), + }, + }, + }, + ]); + }, +); + +sentryTest( + '[buffer-mode] manually starting replay includes performance entries with 1s wiggle room', + async ({ getLocalTestUrl, page, browserName }) => { + // This was sometimes flaky on webkit, so skipping for now + if (shouldSkipReplayTest() || browserName === 'webkit') { + sentryTest.skip(); + } + + const reqPromise0 = waitForReplayRequest(page, 0); + + await page.route('https://dsn.ingest.sentry.io/**/*', route => { + return route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ id: 'test-id' }), + }); + }); + + const url = await getLocalTestUrl({ testDir: __dirname }); + + page.goto(url); + + // Wait for everything to be initialized, then start replay as soon as possible + await page.waitForFunction('!!window.Replay'); + await page.evaluate('window.Replay.start()'); + + const req0 = await reqPromise0; + + const event0 = getReplayEvent(req0); + const content0 = getReplayRecordingContent(req0); + + expect(event0).toEqual( + getExpectedReplayEvent({ + replay_type: 'session', + }), + ); + + const { performanceSpans } = content0; + + // web vitals etc. are included with 1s wiggle room, to accomodate "immediate" start + expect(performanceSpans.length).toBeGreaterThan(1); + }, +); + // Doing this in buffer mode to test changing error sample rate after first // error happens. sentryTest( diff --git a/packages/replay-internal/src/replay.ts b/packages/replay-internal/src/replay.ts index 563938fe4d43..6148bf2c7528 100644 --- a/packages/replay-internal/src/replay.ts +++ b/packages/replay-internal/src/replay.ts @@ -1046,11 +1046,23 @@ export class ReplayContainer implements ReplayContainerInterface { * are included in the replay event before it is finished and sent to Sentry. */ private _addPerformanceEntries(): Promise> { - const performanceEntries = createPerformanceEntries(this.performanceEntries).concat(this.replayPerformanceEntries); + let performanceEntries = createPerformanceEntries(this.performanceEntries).concat(this.replayPerformanceEntries); this.performanceEntries = []; this.replayPerformanceEntries = []; + // If we are manually starting, we want to ensure we only include performance entries + // that are after the initial timestamp, with 1s wiggle room. + // The reason for this is that we may have performance entries from the page load, but may decide to start + // the replay later on, in which case we do not want to include these entries. + // without this, manually started replays can have events long before the actual replay recording starts, + // which messes with the timeline etc. + if (this._requiresManualStart) { + // We leave 1s wiggle room to accomodate timing differences for "immedidate" manual starts + const initialTimestampInSeconds = this._context.initialTimestamp / 1000 - 1; + performanceEntries = performanceEntries.filter(entry => entry.start >= initialTimestampInSeconds); + } + return Promise.all(createPerformanceSpans(this, performanceEntries)); } From e861f4672f62c21993db2397ed8f001ca7280036 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 17 Oct 2024 11:31:13 -0700 Subject: [PATCH 2/2] no wiggle room --- .../suites/replay/bufferModeManual/test.ts | 19 ++++++++++++++++--- packages/replay-internal/src/replay.ts | 5 ++--- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/replay/bufferModeManual/test.ts b/dev-packages/browser-integration-tests/suites/replay/bufferModeManual/test.ts index 1de3e800999d..4ac7b9ea9cf1 100644 --- a/dev-packages/browser-integration-tests/suites/replay/bufferModeManual/test.ts +++ b/dev-packages/browser-integration-tests/suites/replay/bufferModeManual/test.ts @@ -350,7 +350,7 @@ sentryTest( ); sentryTest( - '[buffer-mode] manually starting replay includes performance entries with 1s wiggle room', + '[buffer-mode] manually starting replay ignores earlier performance entries when starting immediately', async ({ getLocalTestUrl, page, browserName }) => { // This was sometimes flaky on webkit, so skipping for now if (shouldSkipReplayTest() || browserName === 'webkit') { @@ -388,8 +388,21 @@ sentryTest( const { performanceSpans } = content0; - // web vitals etc. are included with 1s wiggle room, to accomodate "immediate" start - expect(performanceSpans.length).toBeGreaterThan(1); + expect(performanceSpans).toEqual([ + { + op: 'memory', + description: 'memory', + startTimestamp: expect.any(Number), + endTimestamp: expect.any(Number), + data: { + memory: { + jsHeapSizeLimit: expect.any(Number), + totalJSHeapSize: expect.any(Number), + usedJSHeapSize: expect.any(Number), + }, + }, + }, + ]); }, ); diff --git a/packages/replay-internal/src/replay.ts b/packages/replay-internal/src/replay.ts index 6148bf2c7528..cfd8a2a45c33 100644 --- a/packages/replay-internal/src/replay.ts +++ b/packages/replay-internal/src/replay.ts @@ -1052,14 +1052,13 @@ export class ReplayContainer implements ReplayContainerInterface { this.replayPerformanceEntries = []; // If we are manually starting, we want to ensure we only include performance entries - // that are after the initial timestamp, with 1s wiggle room. + // that are after the initial timestamp // The reason for this is that we may have performance entries from the page load, but may decide to start // the replay later on, in which case we do not want to include these entries. // without this, manually started replays can have events long before the actual replay recording starts, // which messes with the timeline etc. if (this._requiresManualStart) { - // We leave 1s wiggle room to accomodate timing differences for "immedidate" manual starts - const initialTimestampInSeconds = this._context.initialTimestamp / 1000 - 1; + const initialTimestampInSeconds = this._context.initialTimestamp / 1000; performanceEntries = performanceEntries.filter(entry => entry.start >= initialTimestampInSeconds); }