feat(replay): Do not capture replays < 5 seconds (GA) (#8277)

getsentry · Jun 19, 2023 · df4f4ab · df4f4ab
1 parent 4edc429
commit df4f4ab
Show file tree

Hide file tree

Showing 13 changed files with 222 additions and 1,070 deletions.
diff --git a/packages/browser-integration-tests/suites/replay/bufferMode/test.ts b/packages/browser-integration-tests/suites/replay/bufferMode/test.ts
@@ -25,7 +25,6 @@ sentryTest(
     let errorEventId: string | undefined;
     const reqPromise0 = waitForReplayRequest(page, 0);
     const reqPromise1 = waitForReplayRequest(page, 1);
-    const reqPromise2 = waitForReplayRequest(page, 2);
     const reqErrorPromise = waitForErrorRequest(page);
 
     await page.route('https://dsn.ingest.sentry.io/**/*', route => {
@@ -101,18 +100,14 @@ sentryTest(
 
     // Switches to session mode and then goes to background
     const req1 = await reqPromise1;
-    const req2 = await reqPromise2;
-    expect(callsToSentry).toBeGreaterThanOrEqual(5);
+    expect(callsToSentry).toBeGreaterThanOrEqual(4);
 
     const event0 = getReplayEvent(req0);
     const content0 = getReplayRecordingContent(req0);
 
     const event1 = getReplayEvent(req1);
     const content1 = getReplayRecordingContent(req1);
 
-    const event2 = getReplayEvent(req2);
-    const content2 = getReplayRecordingContent(req2);
-
     expect(event0).toEqual(
       getExpectedReplayEvent({
         error_ids: [errorEventId!],
@@ -157,17 +152,7 @@ sentryTest(
 
     // From switching to session mode
     expect(content1.fullSnapshots).toHaveLength(1);
-
-    expect(event2).toEqual(
-      getExpectedReplayEvent({
-        replay_type: 'buffer', // although we're in session mode, we still send 'buffer' as replay_type
-        segment_id: 2,
-        urls: [],
-      }),
-    );
-
-    expect(content2.fullSnapshots).toHaveLength(0);
-    expect(content2.breadcrumbs).toEqual(expect.arrayContaining([expectedClickBreadcrumb]));
+    expect(content1.breadcrumbs).toEqual(expect.arrayContaining([expectedClickBreadcrumb]));
   },
 );
 

diff --git a/packages/browser-integration-tests/suites/replay/fileInput/test.ts b/packages/browser-integration-tests/suites/replay/fileInput/test.ts
@@ -25,7 +25,6 @@ sentryTest(
     }
 
     const reqPromise0 = waitForReplayRequest(page, 0);
-    const reqPromise1 = waitForReplayRequest(page, 1);
 
     await page.route('https://dsn.ingest.sentry.io/**/*', route => {
       return route.fulfill({
@@ -39,7 +38,7 @@ sentryTest(
 
     await page.goto(url);
 
-    await reqPromise0;
+    const res = await reqPromise0;
 
     await page.setInputFiles('#file-input', {
       name: 'file.csv',
@@ -49,9 +48,7 @@ sentryTest(
 
     await forceFlushReplay();
 
-    const res1 = await reqPromise1;
-
-    const snapshots = getIncrementalRecordingSnapshots(res1).filter(isInputMutation);
+    const snapshots = getIncrementalRecordingSnapshots(res).filter(isInputMutation);
 
     expect(snapshots).toEqual([]);
   },

diff --git a/packages/browser-integration-tests/suites/replay/largeMutations/defaultOptions/test.ts b/packages/browser-integration-tests/suites/replay/largeMutations/defaultOptions/test.ts
@@ -11,7 +11,6 @@ sentryTest(
     }
 
     const reqPromise0 = waitForReplayRequest(page, 0);
-    const reqPromise0b = waitForReplayRequest(page, 1);
 
     await page.route('https://dsn.ingest.sentry.io/**/*', route => {
       return route.fulfill({
@@ -24,10 +23,7 @@ sentryTest(
     const url = await getLocalTestPath({ testDir: __dirname });
 
     await page.goto(url);
-    await forceFlushReplay();
     const res0 = await reqPromise0;
-    await reqPromise0b;
-    // A second request is sent right after initial snapshot, we want to wait for that to settle before we continue
 
     const reqPromise1 = waitForReplayRequest(page);
 

diff --git a/packages/browser-integration-tests/suites/replay/largeMutations/mutationLimit/test.ts b/packages/browser-integration-tests/suites/replay/largeMutations/mutationLimit/test.ts
@@ -16,7 +16,6 @@ sentryTest(
     }
 
     const reqPromise0 = waitForReplayRequest(page, 0);
-    const reqPromise0b = waitForReplayRequest(page, 1);
 
     await page.route('https://dsn.ingest.sentry.io/**/*', route => {
       return route.fulfill({
@@ -30,8 +29,6 @@ sentryTest(
 
     await page.goto(url);
     const res0 = await reqPromise0;
-    await reqPromise0b;
-    // A second request is sent right after initial snapshot, we want to wait for that to settle before we continue
 
     const reqPromise1 = waitForReplayRequest(page);
 

diff --git a/packages/browser-integration-tests/suites/replay/slowClick/mutation/test.ts b/packages/browser-integration-tests/suites/replay/slowClick/mutation/test.ts
@@ -3,7 +3,7 @@ import { expect } from '@playwright/test';
 import { sentryTest } from '../../../../utils/fixtures';
 import { getCustomRecordingEvents, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers';
 
-sentryTest('mutation after threshold results in slow click', async ({ getLocalTestUrl, page }) => {
+sentryTest('mutation after threshold results in slow click', async ({ forceFlushReplay, getLocalTestUrl, page }) => {
   if (shouldSkipReplayTest()) {
     sentryTest.skip();
   }
@@ -21,6 +21,7 @@ sentryTest('mutation after threshold results in slow click', async ({ getLocalTe
   const url = await getLocalTestUrl({ testDir: __dirname });
 
   await page.goto(url);
+  await forceFlushReplay();
   await reqPromise0;
 
   const reqPromise1 = waitForReplayRequest(page, (event, res) => {
@@ -125,59 +126,63 @@ sentryTest('multiple clicks are counted', async ({ getLocalTestUrl, page }) => {
   expect(slowClickBreadcrumbs[0]?.data?.timeAfterClickMs).toBeLessThan(3100);
 });
 
-sentryTest('immediate mutation does not trigger slow click', async ({ browserName, getLocalTestUrl, page }) => {
-  // This test seems to only be flakey on firefox
-  if (shouldSkipReplayTest() || ['firefox'].includes(browserName)) {
-    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' }),
+sentryTest(
+  'immediate mutation does not trigger slow click',
+  async ({ forceFlushReplay, browserName, getLocalTestUrl, page }) => {
+    // This test seems to only be flakey on firefox
+    if (shouldSkipReplayTest() || ['firefox'].includes(browserName)) {
+      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 });
+    const url = await getLocalTestUrl({ testDir: __dirname });
 
-  await page.goto(url);
-  await reqPromise0;
+    await page.goto(url);
+    await forceFlushReplay();
+    await reqPromise0;
 
-  const reqPromise1 = waitForReplayRequest(page, (event, res) => {
-    const { breadcrumbs } = getCustomRecordingEvents(res);
+    const reqPromise1 = waitForReplayRequest(page, (event, res) => {
+      const { breadcrumbs } = getCustomRecordingEvents(res);
 
-    return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.click');
-  });
-
-  await page.click('#mutationButtonImmediately');
-
-  const { breadcrumbs } = getCustomRecordingEvents(await reqPromise1);
+      return breadcrumbs.some(breadcrumb => breadcrumb.category === 'ui.click');
+    });
 
-  expect(breadcrumbs).toEqual([
-    {
-      category: 'ui.click',
-      data: {
-        node: {
-          attributes: {
-            id: 'mutationButtonImmediately',
+    await page.click('#mutationButtonImmediately');
+
+    const { breadcrumbs } = getCustomRecordingEvents(await reqPromise1);
+
+    expect(breadcrumbs).toEqual([
+      {
+        category: 'ui.click',
+        data: {
+          node: {
+            attributes: {
+              id: 'mutationButtonImmediately',
+            },
+            id: expect.any(Number),
+            tagName: 'button',
+            textContent: '******* ******** ***********',
           },
-          id: expect.any(Number),
-          tagName: 'button',
-          textContent: '******* ******** ***********',
+          nodeId: expect.any(Number),
         },
-        nodeId: expect.any(Number),
+        message: 'body > button#mutationButtonImmediately',
+        timestamp: expect.any(Number),
+        type: 'default',
       },
-      message: 'body > button#mutationButtonImmediately',
-      timestamp: expect.any(Number),
-      type: 'default',
-    },
-  ]);
-});
+    ]);
+  },
+);
 
-sentryTest('inline click handler does not trigger slow click', async ({ getLocalTestUrl, page }) => {
+sentryTest('inline click handler does not trigger slow click', async ({ forceFlushReplay, getLocalTestUrl, page }) => {
   if (shouldSkipReplayTest()) {
     sentryTest.skip();
   }
@@ -195,6 +200,7 @@ sentryTest('inline click handler does not trigger slow click', async ({ getLocal
   const url = await getLocalTestUrl({ testDir: __dirname });
 
   await page.goto(url);
+  await forceFlushReplay();
   await reqPromise0;
 
   const reqPromise1 = waitForReplayRequest(page, (event, res) => {

diff --git a/packages/browser-integration-tests/suites/replay/throttleBreadcrumbs/test.ts b/packages/browser-integration-tests/suites/replay/throttleBreadcrumbs/test.ts
@@ -26,16 +26,19 @@ sentryTest(
     const url = await getLocalTestUrl({ testDir: __dirname });
 
     await page.goto(url);
-    await reqPromise0;
+    await forceFlushReplay();
+    const res0 = getCustomRecordingEvents(await reqPromise0);
 
     await page.click('[data-console]');
     await forceFlushReplay();
 
-    const { breadcrumbs } = getCustomRecordingEvents(await reqPromise1);
+    const res1 = getCustomRecordingEvents(await reqPromise1);
 
-    // 1 click breadcrumb + 1 throttled breadcrumb is why console logs are less
-    // than throttle limit
-    expect(breadcrumbs.length).toBe(THROTTLE_LIMIT);
+    const breadcrumbs = [...res0.breadcrumbs, ...res1.breadcrumbs];
+    const spans = [...res0.performanceSpans, ...res1.performanceSpans];
     expect(breadcrumbs.filter(breadcrumb => breadcrumb.category === 'replay.throttled').length).toBe(1);
+    // replay.throttled breadcrumb does *not* use the throttledAddEvent as we
+    // alwants want that breadcrumb to be present in replay
+    expect(breadcrumbs.length + spans.length).toBe(THROTTLE_LIMIT + 1);
   },
 );
diff --git a/...e2e-tests/test-applications/standard-frontend-react/tests/fixtures/ReplayRecordingData.ts b/...e2e-tests/test-applications/standard-frontend-react/tests/fixtures/ReplayRecordingData.ts
@@ -95,26 +95,6 @@ export const ReplayRecordingData = [
       },
       timestamp: expect.any(Number),
     },
-    {
-      type: 5,
-      timestamp: expect.any(Number),
-      data: {
-        tag: 'performanceSpan',
-        payload: {
-          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),
-            },
-          },
-        },
-      },
-    },
     {
       type: 3,
       data: {
@@ -155,8 +135,6 @@ export const ReplayRecordingData = [
       data: { source: 5, text: 'Capture Exception', isChecked: false, id: 16 },
       timestamp: expect.any(Number),
     },
-  ],
-  [
     {
       type: 5,
       timestamp: expect.any(Number),

diff --git a/packages/replay/src/replay.ts b/packages/replay/src/replay.ts
@@ -547,6 +547,13 @@ export class ReplayContainer implements ReplayContainerInterface {
     return this.flushImmediate();
   }
 
+  /**
+   * Flush using debounce flush
+   */
+  public flush(): Promise<void> {
+    return this._debouncedFlush() as Promise<void>;
+  }
+
   /**
    * Always flush via `_debouncedFlush` so that we do not have flushes triggered
    * from calling both `flush` and `_debouncedFlush`. Otherwise, there could be

diff --git a/packages/replay/src/types/replay.ts b/packages/replay/src/types/replay.ts
@@ -194,7 +194,6 @@ export interface ReplayPluginOptions extends ReplayNetworkOptions {
   _experiments: Partial<{
     captureExceptions: boolean;
     traceInternals: boolean;
-    delayFlushOnCheckout: number;
   }>;
 }
 
@@ -438,6 +437,7 @@ export interface ReplayContainer {
   stopRecording(): boolean;
   sendBufferedReplayOrFlush(options?: SendBufferedReplayOptions): Promise<void>;
   conditionalFlush(): Promise<void>;
+  flush(): Promise<void>;
   flushImmediate(): Promise<void>;
   cancelFlush(): void;
   triggerUserActivity(): void;

diff --git a/packages/replay/src/util/handleRecordingEmit.ts b/packages/replay/src/util/handleRecordingEmit.ts
@@ -80,41 +80,12 @@ export function getHandleRecordingEmit(replay: ReplayContainer): RecordingEmitCa
         }
       }
 
-      const options = replay.getOptions();
-
-      // TODO: We want this as an experiment so that we can test
-      // internally and create metrics before making this the default
-      if (options._experiments.delayFlushOnCheckout) {
+      if (replay.recordingMode === 'session') {
         // 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.
-        // TODO: We should check `recordingMode` here and do nothing if it's
-        // buffer, instead of checking inside of timeout, this will make our
-        // tests a bit cleaner as we will need to wait on the delay in order to
-        // do nothing.
-        setTimeout(() => replay.conditionalFlush(), options._experiments.delayFlushOnCheckout);
-
-        // 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.
-        replay.cancelFlush();
-
-        return true;
-      }
-
-      // 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.
-      if (replay.recordingMode === 'session') {
-        // We want to ensure the worker is ready, as otherwise we'd always send the first event uncompressed
-        void replay.flushImmediate();
+        void replay.flush();
       }
 
       return true;

diff --git a/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts b/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts
@@ -148,9 +148,13 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => {
 
     jest.runAllTimers();
     await new Promise(process.nextTick);
-
     // Send twice, one for the error & one right after for the session conversion
+    expect(mockSend).toHaveBeenCalledTimes(1);
+
+    jest.runAllTimers();
+    await new Promise(process.nextTick);
     expect(mockSend).toHaveBeenCalledTimes(2);
+
     // This is removed now, because it has been converted to a "session" session
     expect(Array.from(replay.getContext().errorIds)).toEqual([]);
     expect(replay.isEnabled()).toBe(true);