fix(tracing): Ensure web vitals are correctly stopped/captured (#10323)

mydea · web-flow · commit b72a681e3e14 · 2024-01-24T16:48:44.000-05:00
This was identified as a problem by @edwardgou-sentry , and was incorrectly changed in 7.75.0. I think this should work now as expected 🤔 I added some tests for LCP to hopefully very this works kind of as expected (=even after stopping it you can still get a LCP value).
diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/handlers-lcp/assets/sentry-logo-600x179.png b/dev-packages/browser-integration-tests/suites/tracing/metrics/handlers-lcp/assets/sentry-logo-600x179.png
diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/handlers-lcp/subject.js b/dev-packages/browser-integration-tests/suites/tracing/metrics/handlers-lcp/subject.js
@@ -0,0 +1,18 @@
+import { addLcpInstrumentationHandler } from '@sentry-internal/tracing';
+
+addLcpInstrumentationHandler(({ metric }) => {
+  const entry = metric.entries[metric.entries.length - 1];
+  window._LCP = entry.size;
+});
+
+addLcpInstrumentationHandler(({ metric }) => {
+  const entry = metric.entries[metric.entries.length - 1];
+  window._LCP2 = entry.size;
+});
+
+window.ADD_HANDLER = () => {
+  addLcpInstrumentationHandler(({ metric }) => {
+    const entry = metric.entries[metric.entries.length - 1];
+    window._LCP3 = entry.size;
+  });
+};
diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/handlers-lcp/template.html b/dev-packages/browser-integration-tests/suites/tracing/metrics/handlers-lcp/template.html
@@ -0,0 +1,11 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+  </head>
+  <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/dev-packages/browser-integration-tests/suites/tracing/metrics/handlers-lcp/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/handlers-lcp/test.ts
@@ -0,0 +1,51 @@
+import type { Route } from '@playwright/test';
+import { expect } from '@playwright/test';
+import type { Event } from '@sentry/types';
+
+import { sentryTest } from '../../../../utils/fixtures';
+import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers';
+
+const bundle = process.env.PW_BUNDLE || '';
+
+sentryTest(
+  'should capture metrics for LCP instrumentation handlers',
+  async ({ browserName, getLocalTestPath, page }) => {
+    // This uses a utility that is not exported in CDN bundles
+    if (shouldSkipTracingTest() || browserName !== 'chromium' || bundle.startsWith('bundle')) {
+      sentryTest.skip();
+    }
+
+    await page.route('**/path/to/image.png', (route: Route) =>
+      route.fulfill({ path: `${__dirname}/assets/sentry-logo-600x179.png` }),
+    );
+
+    const url = await getLocalTestPath({ testDir: __dirname });
+    const [eventData] = await Promise.all([
+      getFirstSentryEnvelopeRequest<Event>(page),
+      page.goto(url),
+      page.click('button'),
+    ]);
+
+    expect(eventData.measurements).toBeDefined();
+    expect(eventData.measurements?.lcp?.value).toBeDefined();
+
+    expect(eventData.tags?.['lcp.element']).toBe('body > img');
+    expect(eventData.tags?.['lcp.size']).toBe(107400);
+    expect(eventData.tags?.['lcp.url']).toBe('https://example.com/path/to/image.png');
+
+    const lcp = await (await page.waitForFunction('window._LCP')).jsonValue();
+    const lcp2 = await (await page.waitForFunction('window._LCP2')).jsonValue();
+    const lcp3 = await page.evaluate('window._LCP3');
+
+    expect(lcp).toEqual(107400);
+    expect(lcp2).toEqual(107400);
+    // this has not been triggered yet
+    expect(lcp3).toEqual(undefined);
+
+    // Adding a handler after LCP is completed still triggers the handler
+    await page.evaluate('window.ADD_HANDLER()');
+    const lcp3_2 = await (await page.waitForFunction('window._LCP3')).jsonValue();
+
+    expect(lcp3_2).toEqual(107400);
+  },
+);
diff --git a/dev-packages/e2e-tests/test-applications/react-create-hash-router/tests/fixtures/ReplayRecordingData.ts b/dev-packages/e2e-tests/test-applications/react-create-hash-router/tests/fixtures/ReplayRecordingData.ts
@@ -215,7 +215,11 @@ export const ReplayRecordingData = [
         description: 'largest-contentful-paint',
         startTimestamp: expect.any(Number),
         endTimestamp: expect.any(Number),
-        data: { value: expect.any(Number), size: expect.any(Number) },
+        data: {
+          value: expect.any(Number),
+          size: expect.any(Number),
+          nodeId: 16,
+        },
       },
     },
   },
diff --git a/dev-packages/e2e-tests/test-applications/react-router-6-use-routes/tests/fixtures/ReplayRecordingData.ts b/dev-packages/e2e-tests/test-applications/react-router-6-use-routes/tests/fixtures/ReplayRecordingData.ts
@@ -215,7 +215,11 @@ export const ReplayRecordingData = [
         description: 'largest-contentful-paint',
         startTimestamp: expect.any(Number),
         endTimestamp: expect.any(Number),
-        data: { value: expect.any(Number), size: expect.any(Number) },
+        data: {
+          value: expect.any(Number),
+          size: expect.any(Number),
+          nodeId: 16,
+        },
       },
     },
   },
diff --git a/dev-packages/e2e-tests/test-applications/standard-frontend-react/tests/fixtures/ReplayRecordingData.ts b/dev-packages/e2e-tests/test-applications/standard-frontend-react/tests/fixtures/ReplayRecordingData.ts
@@ -215,7 +215,11 @@ export const ReplayRecordingData = [
         description: 'largest-contentful-paint',
         startTimestamp: expect.any(Number),
         endTimestamp: expect.any(Number),
-        data: { value: expect.any(Number), size: expect.any(Number) },
+        data: {
+          value: expect.any(Number),
+          size: expect.any(Number),
+          nodeId: 16,
+        },
       },
     },
   },
diff --git a/packages/tracing-internal/src/browser/instrument.ts b/packages/tracing-internal/src/browser/instrument.ts
@@ -73,6 +73,8 @@ interface Metric {
 
 type InstrumentHandlerType = InstrumentHandlerTypeMetric | InstrumentHandlerTypePerformanceObserver;
 
+type StopListening = undefined | void | (() => void);
+
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
 type InstrumentHandlerCallback = (data: any) => void;
 
@@ -88,17 +90,29 @@ let _previousLcp: Metric | undefined;
 /**
  * Add a callback that will be triggered when a CLS metric is available.
  * Returns a cleanup callback which can be called to remove the instrumentation handler.
+ *
+ * Pass `stopOnCallback = true` to stop listening for CLS when the cleanup callback is called.
+ * This will lead to the CLS being finalized and frozen.
  */
-export function addClsInstrumentationHandler(callback: (data: { metric: Metric }) => void): CleanupHandlerCallback {
-  return addMetricObserver('cls', callback, instrumentCls, _previousCls);
+export function addClsInstrumentationHandler(
+  callback: (data: { metric: Metric }) => void,
+  stopOnCallback = false,
+): CleanupHandlerCallback {
+  return addMetricObserver('cls', callback, instrumentCls, _previousCls, stopOnCallback);
 }
 
 /**
  * Add a callback that will be triggered when a LCP metric is available.
  * Returns a cleanup callback which can be called to remove the instrumentation handler.
+ *
+ * Pass `stopOnCallback = true` to stop listening for LCP when the cleanup callback is called.
+ * This will lead to the LCP being finalized and frozen.
  */
-export function addLcpInstrumentationHandler(callback: (data: { metric: Metric }) => void): CleanupHandlerCallback {
-  return addMetricObserver('lcp', callback, instrumentLcp, _previousLcp);
+export function addLcpInstrumentationHandler(
+  callback: (data: { metric: Metric }) => void,
+  stopOnCallback = false,
+): CleanupHandlerCallback {
+  return addMetricObserver('lcp', callback, instrumentLcp, _previousLcp, stopOnCallback);
 }
 
 /**
@@ -158,8 +172,8 @@ function triggerHandlers(type: InstrumentHandlerType, data: unknown): void {
   }
 }
 
-function instrumentCls(): void {
-  onCLS(metric => {
+function instrumentCls(): StopListening {
+  return onCLS(metric => {
     triggerHandlers('cls', {
       metric,
     });
@@ -168,16 +182,16 @@ function instrumentCls(): void {
 }
 
 function instrumentFid(): void {
-  onFID(metric => {
+  return onFID(metric => {
     triggerHandlers('fid', {
       metric,
     });
     _previousFid = metric;
   });
 }
 
-function instrumentLcp(): void {
-  onLCP(metric => {
+function instrumentLcp(): StopListening {
+  return onLCP(metric => {
     triggerHandlers('lcp', {
       metric,
     });
@@ -188,21 +202,24 @@ function instrumentLcp(): void {
 function addMetricObserver(
   type: InstrumentHandlerTypeMetric,
   callback: InstrumentHandlerCallback,
-  instrumentFn: () => void,
+  instrumentFn: () => StopListening,
   previousValue: Metric | undefined,
+  stopOnCallback = false,
 ): CleanupHandlerCallback {
   addHandler(type, callback);
 
+  let stopListening: StopListening | undefined;
+
   if (!instrumented[type]) {
-    instrumentFn();
+    stopListening = instrumentFn();
     instrumented[type] = true;
   }
 
   if (previousValue) {
     callback({ metric: previousValue });
   }
 
-  return getCleanupCallback(type, callback);
+  return getCleanupCallback(type, callback, stopOnCallback ? stopListening : undefined);
 }
 
 function instrumentPerformanceObserver(type: InstrumentHandlerTypePerformanceObserver): void {
@@ -228,8 +245,16 @@ function addHandler(type: InstrumentHandlerType, handler: InstrumentHandlerCallb
 }
 
 // Get a callback which can be called to remove the instrumentation handler
-function getCleanupCallback(type: InstrumentHandlerType, callback: InstrumentHandlerCallback): CleanupHandlerCallback {
+function getCleanupCallback(
+  type: InstrumentHandlerType,
+  callback: InstrumentHandlerCallback,
+  stopListening: StopListening,
+): CleanupHandlerCallback {
   return () => {
+    if (stopListening) {
+      stopListening();
+    }
+
     const typeHandlers = handlers[type];
 
     if (!typeHandlers) {
diff --git a/packages/tracing-internal/src/browser/metrics/index.ts b/packages/tracing-internal/src/browser/metrics/index.ts
@@ -39,7 +39,8 @@ let _lcpEntry: LargestContentfulPaint | undefined;
 let _clsEntry: LayoutShift | undefined;
 
 /**
- * Start tracking web vitals
+ * Start tracking web vitals.
+ * The callback returned by this function can be used to stop tracking & ensure all measurements are final & captured.
  *
  * @returns A function that forces web vitals collection
  */
@@ -129,35 +130,35 @@ export function startTrackingInteractions(): void {
 /** Starts tracking the Cumulative Layout Shift on the current page. */
 function _trackCLS(): () => void {
   return addClsInstrumentationHandler(({ metric }) => {
-    const entry = metric.entries.pop();
+    const entry = metric.entries[metric.entries.length - 1];
     if (!entry) {
       return;
     }
 
     DEBUG_BUILD && logger.log('[Measurements] Adding CLS');
     _measurements['cls'] = { value: metric.value, unit: '' };
     _clsEntry = entry as LayoutShift;
-  });
+  }, true);
 }
 
 /** Starts tracking the Largest Contentful Paint on the current page. */
 function _trackLCP(): () => void {
   return addLcpInstrumentationHandler(({ metric }) => {
-    const entry = metric.entries.pop();
+    const entry = metric.entries[metric.entries.length - 1];
     if (!entry) {
       return;
     }
 
     DEBUG_BUILD && logger.log('[Measurements] Adding LCP');
     _measurements['lcp'] = { value: metric.value, unit: 'millisecond' };
     _lcpEntry = entry as LargestContentfulPaint;
-  });
+  }, true);
 }
 
 /** Starts tracking the First Input Delay on the current page. */
 function _trackFID(): () => void {
   return addFidInstrumentationHandler(({ metric }) => {
-    const entry = metric.entries.pop();
+    const entry = metric.entries[metric.entries.length - 1];
     if (!entry) {
       return;
     }
