From b6e2c3441cc0eb2d2fcb1478d3a2f9bd271c6362 Mon Sep 17 00:00:00 2001 From: Abdelrahman Awad Date: Fri, 28 Aug 2026 13:31:10 -0400 Subject: [PATCH] feat(browser): Make bfcache web vitals configurable instead of always dropped `withoutBfcache` dropped every metric web-vitals reported after a back/forward-cache restore. That was the right call while there was nothing to attach them to: a restore reuses the frozen document, so the values would have landed on the span the page had before it was frozen. Now that a restore gets its own navigation span, they have a correct parent, so the drop becomes an option rather than a hard rule: webVitals: { bfcache: true } Off by default. A restore is near-instant, so its vitals are a different population from page load vitals, and the earlier concern about skewing aggregates still applies to anyone who has not decided how to treat them. `browser.navigation.type: bfcache` makes them separable once enabled. Reporting per navigation rather than per page load is now what the tracker flag means, since bfcache restores need it for the same reason soft navigations do. `reportAllChanges` is switched off for either, since the per-navigation path relies on each reported value already being final for its navigation. Verified end to end in Chrome 152: a restore emits LCP and CLS parented to the bfcache navigation span on the restore's own trace, and a bfcache-ineligible back navigation still falls back to a page load. --- packages/browser-utils/src/index.ts | 1 + .../instrumentation/performanceObserver.ts | 42 ++++++---- .../browser-utils/src/web-vitals/spans.ts | 43 ++++++++--- .../test/web-vitals/spans.test.ts | 77 ++++++++++++++++++- .../browser/src/integrations/webVitals.ts | 38 +++++++-- .../test/integrations/webVitals.test.ts | 43 +++++++++++ 6 files changed, 207 insertions(+), 37 deletions(-) diff --git a/packages/browser-utils/src/index.ts b/packages/browser-utils/src/index.ts index 297990dc5400..733129d52f5f 100644 --- a/packages/browser-utils/src/index.ts +++ b/packages/browser-utils/src/index.ts @@ -5,6 +5,7 @@ export { addLcpInstrumentationHandler, addInpInstrumentationHandler, addFcpInstrumentationHandler, + enableBfcacheReporting, enableSoftNavigationReporting, } from './instrumentation/performanceObserver'; diff --git a/packages/browser-utils/src/instrumentation/performanceObserver.ts b/packages/browser-utils/src/instrumentation/performanceObserver.ts index 5c2b761b742e..7bdefc299c20 100644 --- a/packages/browser-utils/src/instrumentation/performanceObserver.ts +++ b/packages/browser-utils/src/instrumentation/performanceObserver.ts @@ -166,6 +166,7 @@ let _previousFcp: Metric | undefined; const stopListeners: Partial> = {}; let _reportSoftNavs = false; +let _reportBfcache = false; /** * Opt the CLS, LCP and INP observers into reporting metrics for soft navigations. @@ -188,6 +189,21 @@ export function enableSoftNavigationReporting(): void { _reportSoftNavs = true; } +/** + * Opt the CLS, LCP and INP observers into reporting metrics for back/forward-cache restores. + * + * web-vitals re-reports each metric after a restore, tagged with a `back-forward-cache` navigation + * type. A restore is a new page view measured against a document that was never reloaded, so the + * values only mean anything if there is a fresh root span for them to belong to. Without one they + * would attach to the span the page had before it was frozen, which is why this is off by default. + * + * Like `enableSoftNavigationReporting`, this only affects observers instrumented after it is + * called. + */ +export function enableBfcacheReporting(): void { + _reportBfcache = true; +} + /** * 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. @@ -294,16 +310,12 @@ function triggerHandlers(type: InstrumentHandlerType, data: unknown): void { } /** - * Wraps a metric callback so that metrics reported after a back/forward-cache restore are ignored. - * - * web-vitals re-reports each metric after a bfcache restore (tagged with a `back-forward-cache` - * navigation type). We intentionally drop those for now: our reporting assumes one set of vitals - * per page load, so surfacing bfcache re-reports would skew the data until we're ready to model - * and communicate them. + * Wraps a metric callback so that metrics reported after a back/forward-cache restore are dropped + * unless `enableBfcacheReporting` was called. See there for why they are off by default. */ -function withoutBfcache(callback: (metric: Metric) => void): (metric: Metric) => void { +function unlessBfcacheDisabled(callback: (metric: Metric) => void): (metric: Metric) => void { return metric => { - if (metric.navigationType === 'back-forward-cache') { + if (!_reportBfcache && metric.navigationType === 'back-forward-cache') { return; } callback(metric); @@ -312,7 +324,7 @@ function withoutBfcache(callback: (metric: Metric) => void): (metric: Metric) => function instrumentCls(): StopListening { return onCLS( - withoutBfcache(metric => { + unlessBfcacheDisabled(metric => { triggerHandlers('cls', { metric, }); @@ -320,13 +332,13 @@ function instrumentCls(): StopListening { }), // We want the callback to be called whenever the CLS value updates. // By default, the callback is only called when the tab goes to the background. - { reportAllChanges: !_reportSoftNavs, reportSoftNavs: _reportSoftNavs }, + { reportAllChanges: !_reportSoftNavs && !_reportBfcache, reportSoftNavs: _reportSoftNavs }, ); } function instrumentLcp(): StopListening { return onLCP( - withoutBfcache(metric => { + unlessBfcacheDisabled(metric => { triggerHandlers('lcp', { metric, }); @@ -334,13 +346,13 @@ function instrumentLcp(): StopListening { }), // We want the callback to be called whenever the LCP value updates. // By default, the callback is only called when the tab goes to the background. - { reportAllChanges: !_reportSoftNavs, reportSoftNavs: _reportSoftNavs }, + { reportAllChanges: !_reportSoftNavs && !_reportBfcache, reportSoftNavs: _reportSoftNavs }, ); } function instrumentTtfb(): StopListening { return onTTFB( - withoutBfcache(metric => { + unlessBfcacheDisabled(metric => { triggerHandlers('ttfb', { metric, }); @@ -351,7 +363,7 @@ function instrumentTtfb(): StopListening { function instrumentFcp(): StopListening { return onFCP( - withoutBfcache(metric => { + unlessBfcacheDisabled(metric => { triggerHandlers('fcp', { metric, }); @@ -362,7 +374,7 @@ function instrumentFcp(): StopListening { function instrumentInp(): StopListening { return onINP( - withoutBfcache(metric => { + unlessBfcacheDisabled(metric => { triggerHandlers('inp', { metric, }); diff --git a/packages/browser-utils/src/web-vitals/spans.ts b/packages/browser-utils/src/web-vitals/spans.ts index 4a0f22e95cf5..95ef5a14978d 100644 --- a/packages/browser-utils/src/web-vitals/spans.ts +++ b/packages/browser-utils/src/web-vitals/spans.ts @@ -6,6 +6,7 @@ import { getRootSpan, hasSpanStreamingEnabled, SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME, + spanToJSON, timestampInSeconds, } from '@sentry/core'; import { DEBUG_BUILD } from '../debug-build'; @@ -19,7 +20,7 @@ import { addLcpInstrumentationHandler, } from '../instrumentation/performanceObserver'; import type { LargestContentfulPaint, LayoutShift } from './emitSpan'; -import { _emitWebVitalSpan } from './emitSpan'; +import { BROWSER_NAVIGATION_TYPE_ATTRIBUTE, _emitWebVitalSpan } from './emitSpan'; import { isValidLcpMetric } from './lcp'; import type { WebVitalReportEvent } from './reportEvents'; import { listenForWebVitalReportEvents } from './reportEvents'; @@ -46,12 +47,12 @@ type WebVitalMetric = Parameters type InpMetric = Parameters[0]['metric']; /** - * Reports a web vital once per navigation, for browsers reporting soft navigations. + * Reports a web vital once per navigation, rather than once per page load. * - * With `reportSoftNavs`, web-vitals restarts the metric on every soft navigation and force-reports - * the previous one just before it does (and again on pagehide). Since we also drop - * `reportAllChanges` in this mode, every value we're handed is already the final one for its - * navigation, so there is nothing to accumulate: each report is a span. + * web-vitals restarts the metric on every soft navigation and force-reports the previous one just + * before it does (and again on pagehide), and re-reports every metric after a bfcache restore. + * Since `reportAllChanges` is off in this mode, every value we're handed is already the final one + * for its navigation, so there is nothing to accumulate: each report is a span. */ function trackWebVitalPerNavigation( client: Client, @@ -63,6 +64,16 @@ function trackWebVitalPerNavigation( pageloadSpan = span; }); + // Remembered when the restore happens rather than read back at report time: the restore + // navigation span is an idle span, and CLS and INP are only finalized on pagehide, by which point + // it has long ended and is no longer what is active. + let bfcacheNavigationSpan: Span | undefined; + client.on('spanStart', span => { + if (spanToJSON(span).attributes?.[BROWSER_NAVIGATION_TYPE_ATTRIBUTE] === 'bfcache') { + bfcacheNavigationSpan = span; + } + }); + addInstrumentationHandler(({ metric }) => { const navigationSpan = getNavigationSpanForMetric(metric); if (metric.navigationType === 'soft-navigation') { @@ -77,6 +88,14 @@ function trackWebVitalPerNavigation( return; } + if (metric.navigationType === 'back-forward-cache') { + // A restore reuses the frozen document, so the pageload span above belongs to the page view + // from before the freeze. The navigation span started for the restore is the page view these + // values were actually measured on. + send(metric, bfcacheNavigationSpan, undefined); + return; + } + send(metric, pageloadSpan, undefined); }); } @@ -84,12 +103,12 @@ function trackWebVitalPerNavigation( /** * Tracks LCP as a streamed span. */ -export function trackLcpAsSpan(client: Client, reportSoftNavs = false): void { +export function trackLcpAsSpan(client: Client, perNavigation = false): void { if (!supportsWebVital('largest-contentful-paint')) { return; } - if (reportSoftNavs) { + if (perNavigation) { trackWebVitalPerNavigation(client, addLcpInstrumentationHandler, (metric, parentSpan, softNavigationId) => { const entry = metric.entries[metric.entries.length - 1] as LargestContentfulPaint | undefined; _sendLcpSpan( @@ -184,12 +203,12 @@ export function _sendLcpSpan( /** * Tracks CLS as a streamed span. */ -export function trackClsAsSpan(client: Client, reportSoftNavs = false): void { +export function trackClsAsSpan(client: Client, perNavigation = false): void { if (!supportsWebVital('layout-shift')) { return; } - if (reportSoftNavs) { + if (perNavigation) { trackWebVitalPerNavigation(client, addClsInstrumentationHandler, (metric, parentSpan, softNavigationId) => { const entry = metric.entries[metric.entries.length - 1] as LayoutShift | undefined; _sendClsSpan( @@ -278,7 +297,7 @@ export function _sendClsSpan( * Requires `registerInpInteractionListener()` to be called separately for cached element names and * root spans per interaction. */ -export function trackInpAsSpan(client: Client, reportSoftNavs = false): void { +export function trackInpAsSpan(client: Client, perNavigation = false): void { const performance = getBrowserPerformanceAPI(); if (!performance || !browserPerformanceTimeOrigin()) { return; @@ -291,7 +310,7 @@ export function trackInpAsSpan(client: Client, reportSoftNavs = false): void { // TODO(standalone): once the static trace lifecycle is dropped, INP always streams; drop this flag. const standalone = !hasSpanStreamingEnabled(client); - if (reportSoftNavs) { + if (perNavigation) { // INP restarts per navigation and reports once that navigation is over, by which point the // navigation span has ended and the interaction cache no longer knows about it. The metric // says which navigation it belongs to, so INP is attributed exactly like LCP and CLS. diff --git a/packages/browser-utils/test/web-vitals/spans.test.ts b/packages/browser-utils/test/web-vitals/spans.test.ts index f48077199cb7..1fd0b5f2e529 100644 --- a/packages/browser-utils/test/web-vitals/spans.test.ts +++ b/packages/browser-utils/test/web-vitals/spans.test.ts @@ -76,7 +76,12 @@ describe('_emitWebVitalSpan', () => { beforeEach(() => { vi.mocked(SentryCore.getCurrentScope).mockReturnValue(mockScope as any); vi.mocked(SentryCoreBrowser.startInactiveSpan).mockReturnValue(mockSpan as any); - vi.mocked(SentryCore.spanToJSON).mockReturnValue({ attributes: {} } as any); + vi.mocked(SentryCore.spanToJSON).mockImplementation( + (span: any) => + (span === bfcacheNavigationSpan + ? { attributes: { 'browser.navigation.type': 'bfcache' } } + : { attributes: {} }) as any, + ); // A root span is its own root, which is what the web vital spans are parented to. vi.mocked(SentryCore.getRootSpan).mockImplementation(span => span); vi.mocked(SentryCore.getClient).mockReturnValue({ getIntegrationByName: () => undefined } as any); @@ -585,7 +590,12 @@ describe('_sendInpSpan', () => { vi.mocked(htmlTreeAsString).mockReturnValue('