From 77b3355b2f2327ce5b761855d877b2e88e9215d7 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 17 Oct 2024 12:40:26 -0700 Subject: [PATCH] fix(replay): Ignore older performance entries when starting manually (#13969) For replay, performance entries are captured separately, and added to the replay before we send it. Generally, this works just fine, because when we buffer, we clear the captured performance events whenever the buffer is cleared. However, when we manually start the replay, it can happen that we capture performane entries from way before we started the replay manually, which will in turn extend the timeline of the replay potentially drastically. To fix this, we now drop any performance events, when starting manually, that happened more than a second before we manually started the replay. --- .../suites/replay/bufferModeManual/test.ts | 119 ++++++++++++++++++ packages/replay-internal/src/replay.ts | 13 +- 2 files changed, 131 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..4ac7b9ea9cf1 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,125 @@ 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 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') { + 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; + + 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), + }, + }, + }, + ]); + }, +); + // 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..cfd8a2a45c33 100644 --- a/packages/replay-internal/src/replay.ts +++ b/packages/replay-internal/src/replay.ts @@ -1046,11 +1046,22 @@ 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 + // 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) { + const initialTimestampInSeconds = this._context.initialTimestamp / 1000; + performanceEntries = performanceEntries.filter(entry => entry.start >= initialTimestampInSeconds); + } + return Promise.all(createPerformanceSpans(this, performanceEntries)); }