diff --git a/CHANGELOG.md b/CHANGELOG.md index c4fb77f95837..d330fdf29e78 100644 --- 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 index caf4b8f2deab..502f4dde80c2 100644 --- 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 @@
- + 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 index f79505c6105a..2e215c728ecf 100644 --- 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/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals/assets/sentry-logo-600x179.png b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals/assets/sentry-logo-600x179.png new file mode 100644 index 000000000000..353b7233d6bf Binary files /dev/null and b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals/assets/sentry-logo-600x179.png differ 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 new file mode 100644 index 000000000000..d4c01b121bf7 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals/template.html @@ -0,0 +1,11 @@ + + + + + + +
+ + + + 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 new file mode 100644 index 000000000000..3ff09a2862c5 --- /dev/null +++ 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(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(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 index 10442a4a2bde..861b6c420fbb 100644 --- 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 index b71f80df1ff2..066eba1e6839 100644 --- 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');