feat(replay): Share performance instrumentation with tracing (#9296)

This streamlines web-vital & performance observer handling, by exposing a new `addPerformanceInstrumentationHandler` method from `@sentry-internal/tracing`. This works similar to the instrumentation in utils, where the first time you add instrumentation for a given type, it will add a performance observer. And any further calls will just add more callbacks. This way, we avoid having multiple of the same performance observers. Furthermore, this also aligns the handling of LCP capturing for replay. We used to do this separately, now we use the same data as for performance. Finally, while doing this I noticed that a whole bunch of performance observer stuff we used to capture in Replay, was actually discarded 😬 so no need to capture these anymore at all. (We can always add it back later, if needed) Some integration tests needed slight adjustments for this, probably due to minor timing semantics. But I think all the changes are good/"correct". I _also_ got rid of the event deduplication in replay. Closes #9246
getsentry · Oct 24, 2023 · a0ff516 · a0ff516
1 parent 921b8df
commit a0ff516
Show file tree

Hide file tree

Showing 33 changed files with 1,108 additions and 1,010 deletions.
diff --git a/packages/browser-integration-tests/scripts/detectFlakyTests.ts b/packages/browser-integration-tests/scripts/detectFlakyTests.ts
@@ -1,7 +1,6 @@
 import * as glob from 'glob';
 import * as path from 'path';
 import * as childProcess from 'child_process';
-import { promisify } from 'util';
 
 async function run(): Promise<void> {
   let testPaths: string[] = [];

diff --git a/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts b/packages/browser-integration-tests/suites/replay/eventBufferError/test.ts
@@ -5,6 +5,7 @@ import { envelopeRequestParser } from '../../../utils/helpers';
 import {
   getDecompressedRecordingEvents,
   getReplaySnapshot,
+  isCustomSnapshot,
   isReplayEvent,
   REPLAY_DEFAULT_FLUSH_MAX_DELAY,
   shouldSkipReplayTest,
@@ -41,8 +42,8 @@ sentryTest(
       // We only want to count replays here
       if (event && isReplayEvent(event)) {
         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) {
+        // Make sure to not count mouse moves or performance spans
+        if (events.filter(event => !isCustomSnapshot(event) || event.data.tag !== 'performanceSpan').length > 0) {
           called++;
         }
       }

diff --git a/packages/browser-integration-tests/suites/replay/largeMutations/defaultOptions/template.html b/packages/browser-integration-tests/suites/replay/largeMutations/defaultOptions/template.html
@@ -4,9 +4,10 @@
     <meta charset="utf-8" />
   </head>
   <body>
-    <button id="button-add">Add items</button>
-    <button id="button-modify">Modify items</button>
-    <button id="button-remove">Remove items</button>
+    <button id="noop" type="button">Noop</button>
+    <button id="button-add" type="button">Add items</button>
+    <button id="button-modify" type="button">Modify items</button>
+    <button id="button-remove" type="button">Remove items</button>
     <ul class="list"></ul>
 
     <script>

diff --git a/packages/browser-integration-tests/suites/replay/largeMutations/defaultOptions/test.ts b/packages/browser-integration-tests/suites/replay/largeMutations/defaultOptions/test.ts
@@ -20,25 +20,47 @@ sentryTest(
 
     const url = await getLocalTestPath({ testDir: __dirname });
 
-    const [res0] = await Promise.all([waitForReplayRequest(page, 0), page.goto(url)]);
+    // We have to click in order to ensure the LCP is generated, leading to consistent results
+    async function gotoPageAndClick() {
+      await page.goto(url);
+      await page.click('#noop');
+    }
+    const [res0] = await Promise.all([waitForReplayRequest(page, 0), gotoPageAndClick()]);
     await forceFlushReplay();
 
-    const [res1] = await Promise.all([waitForReplayRequest(page), page.click('#button-add')]);
-    await forceFlushReplay();
+    const [res1] = await Promise.all([
+      waitForReplayRequest(page, (_event, res) => {
+        const parsed = getReplayRecordingContent(res);
+        return !!parsed.incrementalSnapshots.length || !!parsed.fullSnapshots.length;
+      }),
+      page.click('#button-add'),
+      forceFlushReplay(),
+    ]);
 
-    const [res2] = await Promise.all([waitForReplayRequest(page), page.click('#button-modify')]);
-    await forceFlushReplay();
+    const [res2] = await Promise.all([
+      waitForReplayRequest(page, (_event, res) => {
+        const parsed = getReplayRecordingContent(res);
+        return !!parsed.incrementalSnapshots.length || !!parsed.fullSnapshots.length;
+      }),
+      page.click('#button-modify'),
+      forceFlushReplay(),
+    ]);
 
-    const [res3] = await Promise.all([waitForReplayRequest(page), page.click('#button-remove')]);
-    await forceFlushReplay();
+    const [res3] = await Promise.all([
+      waitForReplayRequest(page, (_event, res) => {
+        const parsed = getReplayRecordingContent(res);
+        return !!parsed.incrementalSnapshots.length || !!parsed.fullSnapshots.length;
+      }),
+      page.click('#button-remove'),
+      forceFlushReplay(),
+    ]);
 
     const replayData0 = getReplayRecordingContent(res0);
     const replayData1 = getReplayRecordingContent(res1);
     const replayData2 = getReplayRecordingContent(res2);
     const replayData3 = getReplayRecordingContent(res3);
 
     expect(replayData0.fullSnapshots.length).toBe(1);
-    expect(replayData0.incrementalSnapshots.length).toBe(0);
 
     expect(replayData1.fullSnapshots.length).toBe(0);
     expect(replayData1.incrementalSnapshots.length).toBeGreaterThan(0);

diff --git a/packages/browser-integration-tests/suites/replay/largeMutations/mutationLimit/template.html b/packages/browser-integration-tests/suites/replay/largeMutations/mutationLimit/template.html
@@ -4,9 +4,10 @@
     <meta charset="utf-8" />
   </head>
   <body>
-    <button id="button-add">Add items</button>
-    <button id="button-modify">Modify items</button>
-    <button id="button-remove">Remove items</button>
+    <button id="noop" type="button">Noop</button>
+    <button id="button-add" type="button">Add items</button>
+    <button id="button-modify" type="button">Modify items</button>
+    <button id="button-remove" type="button">Remove items</button>
     <ul class="list"></ul>
 
     <script>

diff --git a/packages/browser-integration-tests/suites/replay/largeMutations/mutationLimit/test.ts b/packages/browser-integration-tests/suites/replay/largeMutations/mutationLimit/test.ts
@@ -23,18 +23,26 @@ sentryTest(
       });
     });
 
-    const reqPromise0 = waitForReplayRequest(page, 0);
-
     const url = await getLocalTestPath({ testDir: __dirname });
 
-    const [res0] = await Promise.all([reqPromise0, page.goto(url)]);
-    await forceFlushReplay();
-
-    const reqPromise1 = waitForReplayRequest(page);
+    // We have to click in order to ensure the LCP is generated, leading to consistent results
+    async function gotoPageAndClick() {
+      await page.goto(url);
+      await page.click('#noop');
+    }
 
-    const [res1] = await Promise.all([reqPromise1, page.click('#button-add')]);
+    const [res0] = await Promise.all([waitForReplayRequest(page, 0), gotoPageAndClick()]);
     await forceFlushReplay();
 
+    const [res1] = await Promise.all([
+      waitForReplayRequest(page, (_event, res) => {
+        const parsed = getReplayRecordingContent(res);
+        return !!parsed.incrementalSnapshots.length || !!parsed.fullSnapshots.length;
+      }),
+      page.click('#button-add'),
+      forceFlushReplay(),
+    ]);
+
     // replay should be stopped due to mutation limit
     let replay = await getReplaySnapshot(page);
     expect(replay.session).toBe(undefined);
@@ -48,7 +56,6 @@ sentryTest(
 
     const replayData0 = getReplayRecordingContent(res0);
     expect(replayData0.fullSnapshots.length).toBe(1);
-    expect(replayData0.incrementalSnapshots.length).toBe(0);
 
     // Breadcrumbs (click and mutation);
     const replayData1 = getReplayRecordingContent(res1);

diff --git a/packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/template.html b/packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/template.html
@@ -6,5 +6,6 @@
   <body>
     <div id="content"></div>
     <img src="https://example.com/path/to/image.png" />
+    <button type="button">Test button</button>
   </body>
 </html>
diff --git a/packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts b/packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts
@@ -15,9 +15,11 @@ sentryTest('should capture a LCP vital with element details.', async ({ browserN
   );
 
   const url = await getLocalTestPath({ testDir: __dirname });
-  await page.goto(url);
-
-  const eventData = await getFirstSentryEnvelopeRequest<Event>(page);
+  const [eventData] = await Promise.all([
+    getFirstSentryEnvelopeRequest<Event>(page),
+    page.goto(url),
+    page.click('button'),
+  ]);
 
   expect(eventData.measurements).toBeDefined();
   expect(eventData.measurements?.lcp?.value).toBeDefined();

diff --git a/packages/browser-integration-tests/utils/replayHelpers.ts b/packages/browser-integration-tests/utils/replayHelpers.ts
@@ -153,7 +153,7 @@ function isFullSnapshot(event: RecordingEvent): event is FullRecordingSnapshot {
   return event.type === EventType.FullSnapshot;
 }
 
-function isCustomSnapshot(event: RecordingEvent): event is RecordingEvent & { data: CustomRecordingEvent } {
+export function isCustomSnapshot(event: RecordingEvent): event is RecordingEvent & { data: CustomRecordingEvent } {
   return event.type === EventType.Custom;
 }
 

diff --git a/packages/e2e-tests/test-applications/react-create-hash-router/tests/behaviour-test.spec.ts b/packages/e2e-tests/test-applications/react-create-hash-router/tests/behaviour-test.spec.ts
@@ -183,6 +183,9 @@ test('Sends a Replay recording to Sentry', async ({ browser }) => {
     return window.sentryReplayId;
   });
 
+  // Keypress event ensures LCP is finished
+  await page.type('body', 'Y');
+
   // Wait for replay to be sent
 
   if (replayId === undefined) {
@@ -229,10 +232,7 @@ test('Sends a Replay recording to Sentry', async ({ browser }) => {
             { headers: { Authorization: `Bearer ${authToken}` } },
           );
 
-          return {
-            status: response.status,
-            data: response.data,
-          };
+          return response.status === 200 ? response.data[0] : response.status;
         } catch (e) {
           if (e instanceof AxiosError && e.response) {
             if (e.response.status !== 404) {
@@ -249,8 +249,5 @@ test('Sends a Replay recording to Sentry', async ({ browser }) => {
         timeout: EVENT_POLLING_TIMEOUT,
       },
     )
-    .toEqual({
-      status: 200,
-      data: ReplayRecordingData,
-    });
+    .toEqual(ReplayRecordingData);
 });