getsentry · Lms24 · Aug 30, 2024 · Aug 28, 2024 · Aug 28, 2024 · Aug 28, 2024
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');