Skip to content

ref(browser): split web vitals integration - #21210

Merged
logaretm merged 9 commits into
developfrom
awad/js-2628-split-web-vitals-into-their-own-integration
Jun 3, 2026
Merged

ref(browser): split web vitals integration#21210
logaretm merged 9 commits into
developfrom
awad/js-2628-split-web-vitals-into-their-own-integration

Conversation

@logaretm

@logaretmlogaretm commented May 28, 2026

Copy link
Copy Markdown
Member

Splits out the web vitals recording logic to its own integration that is added by the browser tracing integration by default.

Closes#21209

@logaretm
logaretm requested a review from a team as a code ownerMay 28, 2026 11:55
@logaretm
logaretm requested review from Lms24 and mydea and removed request for a teamMay 28, 2026 11:56
@linear-code

Copy link
Copy Markdown

JS-2628

@github-actions

github-actionsBot commented May 28, 2026

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize% ChangeChange
@sentry/browser27.2 kB--
@sentry/browser - with treeshaking flags25.65 kB--
@sentry/browser (incl. Tracing)45.5 kB+0.56%+250 B 🔺
@sentry/browser (incl. Tracing + Span Streaming)47.73 kB+0.53%+247 B 🔺
@sentry/browser (incl. Tracing, Profiling)50.28 kB+0.08%+40 B 🔺
@sentry/browser (incl. Tracing, Replay)84.71 kB-0.2%-164 B 🔽
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags74.33 kB-0.15%-105 B 🔽
@sentry/browser (incl. Tracing, Replay with Canvas)89.4 kB-0.2%-177 B 🔽
@sentry/browser (incl. Tracing, Replay, Feedback)102.1 kB-0.1%-98 B 🔽
@sentry/browser (incl. Feedback)44.36 kB--
@sentry/browser (incl. sendFeedback)32.02 kB--
@sentry/browser (incl. FeedbackAsync)37.11 kB--
@sentry/browser (incl. Metrics)28.27 kB--
@sentry/browser (incl. Logs)28.51 kB--
@sentry/browser (incl. Metrics & Logs)29.22 kB--
@sentry/react29.03 kB--
@sentry/react (incl. Tracing)47.79 kB+0.44%+208 B 🔺
@sentry/vue32.22 kB--
@sentry/vue (incl. Tracing)47.4 kB+0.48%+225 B 🔺
@sentry/svelte27.23 kB--
CDN Bundle29.57 kB--
CDN Bundle (incl. Tracing)47.95 kB+0.27%+126 B 🔺
CDN Bundle (incl. Logs, Metrics)31.08 kB--
CDN Bundle (incl. Tracing, Logs, Metrics)49.19 kB+0.27%+132 B 🔺
CDN Bundle (incl. Replay, Logs, Metrics)70.36 kB--
CDN Bundle (incl. Tracing, Replay)85.33 kB+0.14%+114 B 🔺
CDN Bundle (incl. Tracing, Replay, Logs, Metrics)86.51 kB+0.15%+123 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback)91.15 kB+0.08%+69 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback, Logs, Metrics)92.33 kB+0.11%+101 B 🔺
CDN Bundle - uncompressed87.7 kB--
CDN Bundle (incl. Tracing) - uncompressed144.9 kB+0.54%+769 B 🔺
CDN Bundle (incl. Logs, Metrics) - uncompressed92.19 kB--
CDN Bundle (incl. Tracing, Logs, Metrics) - uncompressed148.65 kB+0.52%+769 B 🔺
CDN Bundle (incl. Replay, Logs, Metrics) - uncompressed217 kB--
CDN Bundle (incl. Tracing, Replay) - uncompressed263.76 kB+0.3%+772 B 🔺
CDN Bundle (incl. Tracing, Replay, Logs, Metrics) - uncompressed267.51 kB+0.29%+772 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed277.46 kB+0.28%+772 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback, Logs, Metrics) - uncompressed281.2 kB+0.28%+772 B 🔺
@sentry/nextjs (client)50.26 kB+0.42%+207 B 🔺
@sentry/sveltekit (client)45.92 kB+0.47%+213 B 🔺
@sentry/core/server75.94 kB--
@sentry/core/browser63.09 kB--
@sentry/node-core61.7 kB+0.01%+1 B 🔺
@sentry/node130.4 kB+0.01%+1 B 🔺
@sentry/node - without tracing74.06 kB+0.01%+1 B 🔺
@sentry/aws-serverless86.26 kB+0.01%+1 B 🔺
@sentry/cloudflare (withSentry) - minified173.59 kB--
@sentry/cloudflare (withSentry)433.61 kB--

View base workflow run

@logaretm
logaretmforce-pushed the awad/js-2628-split-web-vitals-into-their-own-integration branch 2 times, most recently from 99e8359 to 433f30aCompareMay 28, 2026 12:10
Comment threadpackages/browser/test/integrations/webVitals.test.ts
/**
* Web vitals to skip.
*/
disable?: WebVitalName[];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignore works better here and similar to other stuff we have

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think we can maybe have it its own performance observer loop so we can split them out?

How do you mean that? Collect what we collect in _measurements in webVitalsIntegration? Yeah I think we could do that.

@logaretmlogaretmJun 3, 2026

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea sorry, I decided to move FP/FCP as well later so there should be no measurements left to collect on the browser tracing integration, it's all now in web vitals integration. That comment was outdated.

The only coupled part is the collectWebVitals call which I want to get rid of now and move away from tracing integration. Doing that now since I think it is within the scope of this PR.

Sorry for the back and forth.

@logaretm
logaretmforce-pushed the awad/js-2628-split-web-vitals-into-their-own-integration branch from 0eccc6c to fb0e9d0CompareMay 29, 2026 21:57
Comment threadpackages/browser-utils/src/metrics/browserMetrics.ts
logaretmand others added 4 commits June 1, 2026 11:12
Co-Authored-By: Codex <codex@openai.com>
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.
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.
@logaretm
logaretmforce-pushed the awad/js-2628-split-web-vitals-into-their-own-integration branch from 98a76d7 to 00cd99bCompareJune 1, 2026 15:12
@logaretm
logaretm requested a review from Lms24June 1, 2026 15:13
Comment threadpackages/browser/src/tracing/browserTracingIntegration.ts
Comment threadpackages/browser-utils/src/metrics/browserMetrics.ts
Comment threadpackages/browser/src/tracing/browserTracingIntegration.ts Outdated
@github-actions

Copy link
Copy Markdown
Contributor

👋 @mydea — Please review this PR when you get a chance!

@logaretmlogaretm changed the title feat(browser): split web vitals integrationref(browser): split web vitals integrationJun 2, 2026

@Lms24Lms24 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gonna approve this since I think we can either merge this as-is and pull out more measurements collecting stuff in a follow-up (if this is easily doable, otherwise feel free to disregard).

logaretmand others added 3 commits June 3, 2026 14:15
Co-Authored-By: OpenAI GPT-5 <codex@openai.com>
Co-Authored-By: OpenAI GPT-5 <codex@openai.com>
Co-Authored-By: OpenAI GPT-5 <codex@openai.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 1b58aa7. Configure here.

Comment threadpackages/browser-utils/src/metrics/browserMetrics.ts
Co-Authored-By: OpenAI GPT-5 <codex@openai.com>
@logaretm
logaretm merged commit 983fbac into developJun 3, 2026
526 of 529 checks passed
@logaretm
logaretm deleted the awad/js-2628-split-web-vitals-into-their-own-integration branch June 3, 2026 19:49
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Split Web Vitals into their own integration

2 participants

@logaretm@Lms24