fix(browser): Remove faulty LCP, FCP and FP normalization logic (#13502)

Remove incorrect normalization logic we applied to LCP, FCP and FP web vital measurements. With this change, we no longer alter the three web vital values but report directly what we received from the web-vitals library. Add a span attribute, `performance.timeOrigin` (feel free to suggest better names) to the pageload root span. This attribute contains the `timeOrigin` value we determine in the SDK. This value [should be used](https://developer.mozilla.org/en-US/docs/Web/API/Performance/timeOrigin) to base performance measurements on.
getsentry · Aug 30, 2024 · bb45dd2 · bb45dd2
1 parent 09622d6
commit bb45dd2
Show file tree

Hide file tree

Showing 8 changed files with 111 additions and 23 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,17 @@
 
 - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott
 
+### Important Changes
+
+- **fix(browser): Remove faulty LCP, FCP and FP normalization logic (#13502)**
+
+This release fixes a bug in the `@sentry/browser` package and all SDKs depending on this package (e.g. `@sentry/react`
+or `@sentry/nextjs`) that caused the SDK to send incorrect web vital values for the LCP, FCP and FP vitals. The SDK
+previously incorrectly processed the original values as they were reported from the browser. When updating your SDK to
+this version, you might experience an increase in LCP, FCP and FP values, which potentially leads to a decrease in your
+performance score in the Web Vitals Insights module in Sentry. This is because the previously reported values were
+smaller than the actually measured values. We apologize for the inconvenience!
+
 Work in this release was contributed by @leopoldkristjansson and @filips123. Thank you for your contributions!
 
 ## 8.27.0

diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/template.html b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/template.html
@@ -5,7 +5,7 @@
   </head>
   <body>
     <div id="content"></div>
-    <img src="https://example.com/path/to/image.png" />
+    <img src="https://example.com/my/image.png" />
     <button type="button">Test button</button>
   </body>
 </html>
diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts
@@ -11,7 +11,7 @@ sentryTest('should capture a LCP vital with element details.', async ({ browserN
   }
 
   page.route('**', route => route.continue());
-  page.route('**/path/to/image.png', async (route: Route) => {
+  page.route('**/my/image.png', async (route: Route) => {
     return route.fulfill({ path: `${__dirname}/assets/sentry-logo-600x179.png` });
   });
 

diff --git a/...egration-tests/suites/tracing/metrics/web-vitals/assets/sentry-logo-600x179.png b/...egration-tests/suites/tracing/metrics/web-vitals/assets/sentry-logo-600x179.png
diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals/template.html b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals/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/library/image.png" />
+    <button type="button">Test button</button>
+  </body>
+</html>
diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals/test.ts
@@ -0,0 +1,81 @@
+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';
+
+/**
+ * Bit of an odd test but we previously ran into cases where we would report TTFB > (LCP, FCP, FP)
+ * This should never happen and this test serves as a regression test for that.
+ *
+ * The problem is: We don't always get valid TTFB from the web-vitals library, so we skip the test if that's the case.
+ * Note: There is another test that covers that we actually report TTFB if it is valid (@see ../web-vitals-lcp/test.ts).
+ */
+sentryTest('paint web vitals values are greater than TTFB', async ({ browserName, getLocalTestPath, page }) => {
+  // Only run in chromium to ensure all vitals are present
+  if (shouldSkipTracingTest() || browserName !== 'chromium') {
+    sentryTest.skip();
+  }
+
+  page.route('**', route => route.continue());
+  page.route('**/library/image.png', async (route: Route) => {
+    return 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.locator('button').click(),
+  ]);
+
+  expect(eventData.measurements).toBeDefined();
+
+  const ttfbValue = eventData.measurements?.ttfb?.value;
+
+  if (!ttfbValue) {
+    // TTFB is unfortunately quite flaky. Sometimes, the web-vitals library doesn't report TTFB because
+    // responseStart is 0. This seems to happen somewhat randomly, so we just ignore this in that case.
+    // @see packages/browser-utils/src/metrics/web-vitals/onTTFB
+
+    // logging the skip reason so that we at least can check for that in CI logs
+    // eslint-disable-next-line no-console
+    console.log('SKIPPING: TTFB is not reported');
+    sentryTest.skip();
+  }
+
+  const lcpValue = eventData.measurements?.lcp?.value;
+  const fcpValue = eventData.measurements?.fcp?.value;
+  const fpValue = eventData.measurements?.fp?.value;
+
+  expect(lcpValue).toBeDefined();
+  expect(fcpValue).toBeDefined();
+  expect(fpValue).toBeDefined();
+
+  // (LCP, FCP, FP) >= TTFB
+  expect(lcpValue).toBeGreaterThanOrEqual(ttfbValue!);
+  expect(fcpValue).toBeGreaterThanOrEqual(ttfbValue!);
+  expect(fpValue).toBeGreaterThanOrEqual(ttfbValue!);
+});
+
+sentryTest('captures time origin as span attribute', async ({ getLocalTestPath, page }) => {
+  // Only run in chromium to ensure all vitals are present
+  if (shouldSkipTracingTest()) {
+    sentryTest.skip();
+  }
+
+  const url = await getLocalTestPath({ testDir: __dirname });
+  const [eventData] = await Promise.all([getFirstSentryEnvelopeRequest<Event>(page), page.goto(url)]);
+
+  const timeOriginAttribute = eventData.contexts?.trace?.data?.['performance.timeOrigin'];
+  const transactionStartTimestamp = eventData.start_timestamp;
+
+  expect(timeOriginAttribute).toBeDefined();
+  expect(transactionStartTimestamp).toBeDefined();
+
+  const delta = Math.abs(transactionStartTimestamp! - timeOriginAttribute);
+
+  // The delta should be less than 1ms if this flakes, we should increase the threshold
+  expect(delta).toBeLessThanOrEqual(1);
+});
diff --git a/dev-packages/e2e-tests/test-applications/react-create-hash-router/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/react-create-hash-router/tests/transactions.test.ts
@@ -22,6 +22,7 @@ test('Captures a pageload transaction', async ({ page }) => {
       'sentry.origin': 'auto.pageload.react.reactrouter_v6',
       'sentry.sample_rate': 1,
       'sentry.source': 'route',
+      'performance.timeOrigin': expect.any(Number),
     },
     op: 'pageload',
     span_id: expect.any(String),

diff --git a/packages/browser-utils/src/metrics/browserMetrics.ts b/packages/browser-utils/src/metrics/browserMetrics.ts
@@ -354,25 +354,6 @@ export function addPerformanceEntries(span: Span, options: AddPerformanceEntries
   if (op === 'pageload') {
     _addTtfbRequestTimeToMeasurements(_measurements);
 
-    ['fcp', 'fp', 'lcp'].forEach(name => {
-      const measurement = _measurements[name];
-      if (!measurement || !transactionStartTime || timeOrigin >= transactionStartTime) {
-        return;
-      }
-      // The web vitals, fcp, fp, lcp, and ttfb, all measure relative to timeOrigin.
-      // Unfortunately, timeOrigin is not captured within the span span data, so these web vitals will need
-      // to be adjusted to be relative to span.startTimestamp.
-      const oldValue = measurement.value;
-      const measurementTimestamp = timeOrigin + msToSec(oldValue);
-
-      // normalizedValue should be in milliseconds
-      const normalizedValue = Math.abs((measurementTimestamp - transactionStartTime) * 1000);
-      const delta = normalizedValue - oldValue;
-
-      DEBUG_BUILD && logger.log(`[Measurements] Normalized ${name} from ${oldValue} to ${normalizedValue} (${delta})`);
-      measurement.value = normalizedValue;
-    });
-
     const fidMark = _measurements['mark.fid'];
     if (fidMark && _measurements['fid']) {
       // create span for FID
@@ -399,7 +380,10 @@ export function addPerformanceEntries(span: Span, options: AddPerformanceEntries
       setMeasurement(measurementName, measurement.value, measurement.unit);
     });
 
-    _tagMetricInfo(span);
+    // Set timeOrigin which denotes the timestamp which to base the LCP/FCP/FP/TTFB measurements on
+    span.setAttribute('performance.timeOrigin', timeOrigin);
+
+    _setWebVitalAttributes(span);
   }
 
   _lcpEntry = undefined;
@@ -604,7 +588,7 @@ function _trackNavigator(span: Span): void {
 }
 
 /** Add LCP / CLS data to span to allow debugging */
-function _tagMetricInfo(span: Span): void {
+function _setWebVitalAttributes(span: Span): void {
   if (_lcpEntry) {
     DEBUG_BUILD && logger.log('[Measurements] Adding LCP Data');