feat(browser): split web vitals integration#21210
Conversation
size-limit report 📦
|
logaretm
force-pushed
the
awad/js-2628-split-web-vitals-into-their-own-integration
branch
from
c7e4357 to
99e8359
Compare
Co-Authored-By: Codex <codex@openai.com>
logaretm
force-pushed
the
awad/js-2628-split-web-vitals-into-their-own-integration
branch
from
99e8359 to
433f30a
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| /** | ||
| * Web vitals to skip. | ||
| */ | ||
| disable?: WebVitalName[]; |
There was a problem hiding this comment.
I like this API much more than the enableInp flag we had in browserTracingIntegration. Just wondering if we should have a different name for this because we rarely use disable as an option. WDYT about ignore? Kinda goes with the same "denylist" pattern but is a bit more established in our options.
I was also thinking if instead of the "disable" mentality, we should have an enable: WebVitalName[] option that by default includes everything. But then I realized, if users set this to e.g. disable INP but have LCP and CLS, they'd never get a new vital we'd add here (soft nav vitals) unless they explicitly add it. So I think the denylist approach in this case is better suited.
There was a problem hiding this comment.
ignore works better here and similar to other stuff we have
There was a problem hiding this comment.
I think one thing where browserTracing and webVitalsIntegration are still a bit interwoven is the addPerformanceEntries call where we write the web vitals as measurements onto the pageload span. We would still need this code if v11 ships with transactions and span streaming (most likely). Do you think it makes sense for us to also pull this out of browserTracing?
I guess one can make the argument that since this requires the pageload span to be present, it can also live in browserTracing. So in a way this is already a separation of concerns. No strong opinions!
There was a problem hiding this comment.
I think pageload is the transport only in this case, so ideally we should move it out. It is too tightly coupled to the other stuff collected like fp/fcp, do you think we can maybe have it its own performance observer loop so we can split them out?
Aligns the webVitalsIntegration denylist option with the more established `ignore` naming used elsewhere in our options. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Previously `addPerformanceEntries` (driven by `browserTracingIntegration`) both emitted pageload child spans (resource/navigation/measure) *and* wrote the web vital values onto the pageload span. This interleaved web vital logic with browserTracing's transport concerns. This fully consolidates web vital logic into `webVitalsIntegration`: - FP/FCP are now collected via their own `paint` PerformanceObserver in `startTrackingWebVitals`, instead of being scavenged from the shared `addPerformanceEntries` loop. All six vitals are now collected in one place. - The "write vitals onto the pageload span" block is extracted into a new `addWebVitalsToSpan` helper, which the integration drives via `collectWebVitalsForClient(client, span)` at span end. The integration owns the decision of whether/how to record each vital. - `connection.rtt` is decoupled from the web vital measurement flush and emitted directly by `_trackNavigator` (behavior-preserving: `setMeasurement` in v1, span attribute in span streaming). - `addPerformanceEntries` slims to resource/navigation/measure spans + navigator; its `_performanceCursor` is no longer shared with web vitals. `browserTracingIntegration` now only provides the pageload span and the span-end trigger — it makes no web vital decisions.
logaretm
force-pushed
the
awad/js-2628-split-web-vitals-into-their-own-integration
branch
from
0eccc6c to
fb0e9d0
Compare
The web vital refactor moved `connection.rtt` out of the shared `_measurements` object and emitted it directly via `setMeasurement` in `_trackNavigator`. Since `_trackNavigator` runs from `addPerformanceEntries` for every idle span (pageload and navigation), this regressed `connection.rtt` onto navigation transactions too. Previously it was only flushed inside the `op === 'pageload'` guard. Restore the pageload-only scope for the non-streaming measurement (the span-streaming attribute path was already emitted for all spans and is unchanged) and add a navigation regression test.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 98a76d7. Configure here.
| const url = await getLocalTestUrl({ testDir: __dirname }); | ||
|
|
||
| const pageloadRequest = await getFirstSentryEnvelopeRequest<Event>(page, url); | ||
| const navigationRequest = await getFirstSentryEnvelopeRequest<Event>(page, `${url}#foo`); |
There was a problem hiding this comment.
E2E test uses unreliable getFirstSentryEnvelopeRequest helper
Medium Severity
The new E2E test uses getFirstSentryEnvelopeRequest which is flagged as unreliable by the project rules. The sequential pattern of awaiting the pageload request and then immediately navigating and awaiting the navigation request is prone to race conditions — stale or unrelated envelopes (e.g. session updates) could be captured instead of the expected transaction. Helpers like waitForTransaction with appropriate filtering would be more reliable.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 98a76d7. Configure here.
2 participants
Splits out the web vitals recording logic to its own integration that is added by the browser tracing integration by default.
Closes #21209