-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
ref(browser): Unify CLS/LCP recording and add recording event attribute #16866
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
size-limit report 📦
|
3454549 to
5cc3c90
Compare
32ca000 to
4b7a123
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Race Condition Causes Web Vital Data Loss
A race condition in listenForWebVitalReportEvents leads to web vital data collection failures and memory leaks. If a report event (e.g., pagehide) occurs before pageloadSpanId is asynchronously set, the _runCollectorCallbackOnce function incorrectly marks collection as complete (collected = true) without actually invoking the collectorCallback. This permanently prevents web vital reporting and, unlike previous behavior, leaves instrumentation cleanup handlers uncalled, causing resource leaks.
packages/browser-utils/src/metrics/utils.ts#L211-L218
sentry-javascript/packages/browser-utils/src/metrics/utils.ts
Lines 211 to 218 in 006b7a6
| let collected = false; | |
| function _runCollectorCallbackOnce(event: WebVitalReportEvent) { | |
| if (!collected && pageloadSpanId) { | |
| collectorCallback(event, pageloadSpanId); | |
| } | |
| collected = true; | |
| } |
Was this report helpful? Give feedback by reacting with 👍 or 👎
s1gr1d
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice changes
This PR refactors our CLS and LCP tracking and recording logic. We now have a
listenForWebVitalReportEventshelper function which listens to page hides and navigation span starts and ensures that the standalone span logic is only called once per web vital. Previously, we had this logic in the CLS and LCP function and it was quite a lot of duplicated and identical code.Despite this change adding an attribute, bundle size should remain constant due to the extraction (🤞)
Note: There's still an edge case with the pageloadSpanId not being obtained correctly in this PR but I'll fix this in a follow-up PR to keep things atomic(ish).