Allow Web Vitals metrics reconfiguration

Author: petyosi

packages/logfire-browser/src/index.test.ts

@@ -240,7 +240,6 @@ vi.mock('@opentelemetry/sdk-trace-web', () => ({
 }))
 
 vi.mock('./webVitals', () => ({
-  assertBrowserWebVitalsMetricsCanStart: () => undefined,
   startBrowserWebVitals: async (options: unknown) => mocks.startBrowserWebVitals(options),
 }))
 

packages/logfire-browser/src/index.ts

@@ -63,7 +63,7 @@ import type { RUMOptions } from './browserSession'
 import type { BrowserMetricsOptions, BrowserWebVitalsMetricOptions } from './browserMetrics'
 import { BrowserSessionReplayState, startBrowserSessionReplay } from './sessionReplay'
 import type { BrowserSessionReplayOptions } from './sessionReplay'
-import { assertBrowserWebVitalsMetricsCanStart, startBrowserWebVitals } from './webVitals'
+import { startBrowserWebVitals } from './webVitals'
 import type { BrowserWebVitalsOptions } from './webVitals'
 import { LogfireSpanProcessor } from './LogfireSpanProcessor'
 export { DiagLogLevel } from '@opentelemetry/api'
@@ -285,9 +285,6 @@ export function configure(options: LogfireConfigOptions): () => Promise<void> {
   if (webVitalsMetricOptions !== undefined && browserMetricsOptions === undefined) {
     throw new Error('logfire-browser: rum.webVitals.metrics requires top-level metrics.metricUrl')
   }
-  if (webVitalsMetricOptions !== undefined) {
-    assertBrowserWebVitalsMetricsCanStart()
-  }
   const browserSessionOptions = resolveBrowserSessionOptions(options.rum, sessionReplayOptions)
 
   const apiConfig: LogfireApiConfigOptions = {

packages/logfire-browser/src/webVitals.test.ts

@@ -221,7 +221,7 @@ describe('browser Web Vitals reporting', () => {
     expect(metricRecorder.shutdown).toHaveBeenCalledTimes(1)
   })
 
-  it('uses the first metric recorder when duplicate startup is requested', async () => {
+  it('uses the latest metric recorder when duplicate startup is requested', async () => {
     const firstMetricRecorder = createMetricRecorder()
     const secondMetricRecorder = createMetricRecorder()
     const metric = createMetric('FCP', { firstByteToFCP: 85, loadState: 'dom-interactive', timeToFirstByte: 42 })
@@ -233,16 +233,53 @@ describe('browser Web Vitals reporting', () => {
     for (const name of webVitalNames) {
       expect(mocks.registrations[name]).toHaveLength(1)
     }
-    expect(firstMetricRecorder.record).toHaveBeenCalledWith(metric)
-    expect(secondMetricRecorder.record).not.toHaveBeenCalled()
+    expect(firstMetricRecorder.record).not.toHaveBeenCalled()
+    expect(secondMetricRecorder.record).toHaveBeenCalledWith(metric)
   })
 
-  it('rejects enabling metrics after Web Vitals already started without metrics', async () => {
+  it('attaches a metric recorder after Web Vitals already started without metrics', async () => {
+    const metricRecorder = createMetricRecorder()
+    const beforeRecorder = createMetric('FCP', { firstByteToFCP: 85, loadState: 'dom-interactive', timeToFirstByte: 42 })
+    const afterRecorder = createMetric('TTFB', { waitingDuration: 1 })
+
     await startBrowserWebVitals()
+    report('FCP', beforeRecorder)
 
-    await expect(startBrowserWebVitals({ metricRecorder: createMetricRecorder() })).rejects.toThrow(
-      'Web Vitals were already started without metrics'
-    )
+    await startBrowserWebVitals({ metricRecorder })
+    report('TTFB', afterRecorder)
+
+    for (const name of webVitalNames) {
+      expect(mocks.registrations[name]).toHaveLength(1)
+    }
+    expect(metricRecorder.record).toHaveBeenCalledTimes(1)
+    expect(metricRecorder.record).toHaveBeenCalledWith(afterRecorder)
+  })
+
+  it('does not let an older handle shutdown clear a newer metric recorder', async () => {
+    const firstMetricRecorder = createMetricRecorder()
+    const secondMetricRecorder = createMetricRecorder()
+    const metric = createMetric('LCP', {
+      resourceLoadDelay: 10,
+      resourceLoadDuration: 20,
+      target: '#hero img',
+      timeToFirstByte: 40,
+    })
+
+    const firstHandle = await startBrowserWebVitals({ metricRecorder: firstMetricRecorder })
+    const secondHandle = await startBrowserWebVitals({ metricRecorder: secondMetricRecorder })
+
+    await firstHandle.shutdown()
+    report('LCP', metric)
+
+    expect(firstMetricRecorder.shutdown).toHaveBeenCalledTimes(1)
+    expect(firstMetricRecorder.record).not.toHaveBeenCalled()
+    expect(secondMetricRecorder.record).toHaveBeenCalledWith(metric)
+
+    await secondHandle.shutdown()
+    report('LCP', metric)
+
+    expect(secondMetricRecorder.shutdown).toHaveBeenCalledTimes(1)
+    expect(secondMetricRecorder.record).toHaveBeenCalledTimes(1)
   })
 
   it('creates a span with base attributes for each report', async () => {

packages/logfire-browser/src/webVitals.ts

@@ -52,20 +52,26 @@ interface BrowserWebVitalsStartOptions extends BrowserWebVitalsOptions {
   metricRecorder?: BrowserWebVitalsMetricRecorder
 }
 
+let startupPromise: Promise<void> | undefined
+let currentMetricRecorder: BrowserWebVitalsMetricRecorder | undefined
+
 function createHandle(metricRecorder: BrowserWebVitalsMetricRecorder | undefined): BrowserWebVitalsHandle {
+  let shutdownCalled = false
   return {
     async shutdown() {
+      if (shutdownCalled) {
+        return Promise.resolve()
+      }
+      shutdownCalled = true
       metricRecorder?.shutdown()
+      if (currentMetricRecorder === metricRecorder) {
+        currentMetricRecorder = undefined
+      }
       return Promise.resolve()
     },
   }
 }
 
-const noopHandle = createHandle(undefined)
-
-let startupPromise: Promise<BrowserWebVitalsHandle> | undefined
-let startupHasMetricRecorder = false
-
 function setPrimitiveAttribute(attributes: Attributes, key: string, value: unknown): void {
   if (typeof value === 'string' || typeof value === 'boolean') {
     attributes[key] = value
@@ -194,44 +200,36 @@ function reportWebVital(metric: MetricWithAttribution, metricRecorder: BrowserWe
   reportWebVitalMetric(metric, metricRecorder)
 }
 
-function registerWebVitals(webVitals: WebVitalsAttributionModule, options: BrowserWebVitalsStartOptions = {}): BrowserWebVitalsHandle {
+function registerWebVitals(webVitals: WebVitalsAttributionModule, options: BrowserWebVitalsStartOptions = {}): void {
   const reportOptions = createBaseReportOptions(options)
   const report = (metric: MetricWithAttribution) => {
-    reportWebVital(metric, options.metricRecorder)
+    reportWebVital(metric, currentMetricRecorder)
   }
   webVitals.onLCP(report, reportOptions)
   webVitals.onINP(report, createInpReportOptions(options))
   webVitals.onCLS(report, reportOptions)
   webVitals.onFCP(report, reportOptions)
   webVitals.onTTFB(report, reportOptions)
-  return createHandle(options.metricRecorder)
-}
-
-export function assertBrowserWebVitalsMetricsCanStart(): void {
-  if (startupPromise !== undefined && !startupHasMetricRecorder) {
-    throw new Error(
-      'logfire-browser: Web Vitals were already started without metrics in this page lifecycle; configure rum.webVitals.metrics before the first Web Vitals startup'
-    )
-  }
 }
 
 export async function startBrowserWebVitals(options: BrowserWebVitalsStartOptions = {}): Promise<BrowserWebVitalsHandle> {
   if (options.metricRecorder !== undefined) {
-    assertBrowserWebVitalsMetricsCanStart()
+    currentMetricRecorder = options.metricRecorder
   }
 
   startupPromise ??= import('web-vitals/attribution')
-    .then((webVitals) => registerWebVitals(webVitals, options))
+    .then((webVitals) => {
+      registerWebVitals(webVitals, options)
+    })
     .catch((error: unknown) => {
       diag.error('logfire-browser: failed to start Web Vitals reporting', error)
-      return noopHandle
     })
-  startupHasMetricRecorder ||= options.metricRecorder !== undefined
 
-  return startupPromise
+  await startupPromise
+  return createHandle(options.metricRecorder)
 }
 
 export function resetBrowserWebVitalsForTests(): void {
   startupPromise = undefined
-  startupHasMetricRecorder = false
+  currentMetricRecorder = undefined
 }

plans/022-browser-rum-web-vitals-metrics.md

@@ -133,11 +133,10 @@ automatic route-template extraction, or per-route soft-navigation Web Vitals.
 - Web Vitals callbacks cannot be unregistered and should not be registered more
   than once per page lifecycle. Metric recording must reuse PRP 021 callback
   registration rather than calling `onLCP()`, `onINP()`, etc. again.
-- The current `startBrowserWebVitals()` implementation is guarded by a
-  module-level startup promise. If a page first configures Web Vitals without
-  metrics and later reconfigures with metrics, later metric enablement cannot
-  add new observers. Either reject this transition clearly or design the module
-  to register sinks before first startup and keep later cleanup no-op.
+- `startBrowserWebVitals()` is guarded by a module-level startup promise because
+  Web Vitals observers are page-lifetime callbacks. Do not add duplicate
+  observers on reconfigure; keep metric recording behind a mutable recorder
+  reference so later configure calls can attach or replace the active sink.
 - Browser metrics should use a local `MeterProvider`. Do not set the global
   OpenTelemetry meter provider from `@pydantic/logfire-browser` unless a future
   explicit option asks for that. Replacing the global provider can break app
@@ -442,8 +441,8 @@ environment issue in the execution summary.
 - [x] Metric attributes are low-cardinality and exclude session/exact
       URL/selector/raw entry attributes.
 - [x] Existing Web Vital spans still include PRP 021 attributes.
-- [x] Duplicate Web Vitals startup does not duplicate observers or metric
-      records.
+- [x] Duplicate Web Vitals startup does not duplicate observers, and later
+      metric startup can attach or replace the active metric recorder.
 - [x] Browser example builds after metric config/docs changes.
 
 ## Clarifications
@@ -473,8 +472,9 @@ environment issue in the execution summary.
   proxy/example in this PRP so user-perspective metric testing is possible.
 - Q: What happens if Web Vitals were already started without metrics and a
   later configure call tries to enable Web Vitals metrics in the same page
-  lifecycle? -> A: Reject this transition clearly. Web Vitals observers are
-  page-lifetime callbacks and should not be re-registered.
+  lifecycle? -> A: Do not re-register Web Vitals observers. Attach the later
+  metric recorder through the module-level mutable recorder reference; metrics
+  emitted before the recorder exists are not backfilled.
 - Q: What metric instrument shape should be used? -> A: Use one histogram per
   Web Vital, with unit `ms` for LCP, INP, FCP, and TTFB, and unit `1` for CLS.
 

reports/browser-rum-replay-follow-ups.md

@@ -50,12 +50,12 @@ page URLs, so custom-event redaction alone does not fully solve URL privacy.
 
 ## Web Vitals Reconfiguration Lifecycle
 
-`webVitals.ts` intentionally uses module-level startup state because the
-`web-vitals` library registers page-lifecycle observers without a public
-unregister API. After shutdown, a second `configure()` call in the same page can
-reuse the original startup promise and fail to attach a new metrics recorder.
-Decide whether to support HMR/reconfigure by introducing a mutable recorder
-reference, or document Web Vitals startup as one-shot per page lifecycle.
+Addressed in this branch: `webVitals.ts` still uses module-level startup state
+because the `web-vitals` library registers page-lifecycle observers without a
+public unregister API, but the callbacks now read a mutable metric recorder
+reference. A later `configure()` call can attach or replace the active recorder
+without duplicate observers. Metrics emitted before a recorder exists are not
+backfilled.
 
 ## Replay Session Expiry During Replay-only Activity