-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: Replace performance.timeOrigin in favour of browserPerformanceTimeOrigin #3397
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
|
rhcarvalho
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.
One suggestion for simplification and we should see less weird transaction durations :) Thanks @dashed ❤️
| logger.log('[Measurements] Adding LCP'); | ||
| this._measurements['lcp'] = { value: metric.value }; | ||
| this._measurements['mark.lcp'] = { value: timeOrigin + startTime }; | ||
| if (browserPerformanceTimeOrigin !== undefined) { |
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.
browserPerformanceTimeOrigin should always exist when we can actually report LCP and FID.
I'd just replace performance.timeOrigin above with browserPerformanceTimeOrigin.
I see we use performance.timeOrigin directly in ember... maybe we change that too, cc @k-fish.
sentry-javascript/packages/ember/addon/instance-initializers/sentry-performance.ts
Line 302 in 3eb9e05
| performance.measure && performance.getEntriesByName && performance.timeOrigin !== undefined; |
sentry-javascript/packages/ember/addon/instance-initializers/sentry-performance.ts
Line 312 in 3eb9e05
| const startTimestamp = (measure.startTime + performance.timeOrigin) / 1000; |
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.
@rhcarvalho typescript told me that browserPerformanceTimeOrigin could be undefined, but I'll coerce it as a number
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.
Yeah typescript... it is only undefined if performance is also undefined.
830c67e to
c016947
Compare
rhcarvalho
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.
Thanks @dashed
Use
browserPerformanceTimeOriginin favour ofperformance.timeOriginwhen generating timestamps for the LCP and FID web vitals.