-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(web-vitals): Adjust some web vitals to be relative to fetchStart and some other improvements #3019
Conversation
size-limit report
|
_startChild(transaction, { | ||
description: 'first input delay', | ||
endTimestamp: this._measurements['mark.fid'].value + msToSec(this._measurements['fid'].value), | ||
op: 'web vitals', |
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.
Should this be 'vitals' to match the more generic name we'll likely use in the future?
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.
Right now we've not committed to using that term as a generic umbrella.
We've only used "web vitals" thus far.
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.
Is the fact that the op is currently two words going to mean that when searching for it, you have to put it in quotes? (I would guess that the answer is yes.) If so, can we make this one word (even if it's webVitals
or web.vitals
or webvitals
) so that users don't have to do that?
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.
web.vitals
works 👍
f214c72
to
8ba3e0a
Compare
After talking this over with @dashed , this makes good sense to me. By adding in a |
Co-authored-by: Tony <Zylphrex@users.noreply.github.com>
@Zylphrex feel free to push commits to this branch as you see fit. |
why |
The |
It is not found. Actually, I am seeing significant differences between the data(LCP/FCP) I am getting from Sentry and what I am getting from Google Search Console. I don't know what to do in the above situation. |
@Dante-dan found here now:
Under the hood we use Google's web vitals library to grab LCP/FCP data. This is what the web vitals team themselves recommend:
It is strange there is a significant difference though. @Dante-dan could you provide some kind of reproduction and open an issue on the JS SDK repo? That will help us dig in further! |
Fine, I think I have found the problem.
|
Some Web Vitals that are based on timings, specifically FCP, FP, LCP, and TTFB are measured relative to
timeOrigin
(https://www.w3.org/TR/hr-time-2/#dfn-time-origin). Unfortunately, this conflicts with the generated span data sourced from the level 2 Navigation Timings as captured from the JS SDK (https://www.w3.org/TR/navigation-timing-2/).As an example:
One approach would be to measure relative to other timings such as
fetchStart
. See: GoogleChrome/web-vitals#19Intuitively, it's more practical to adjust some web vitals to be relative to the starting point of the transaction.
This PR addresses the following:
fetchStart
todomainLookupStart
. This adds more completeness to the processing model. By creating this span, this will usually adjusttransaction.startTimestamp
for somepageload
transactions.See: https://www.w3.org/TR/navigation-timing-2/#processing-model
What it looks like:
transaction.startTimestamp
. This makes an assumption thattransaction.startTimestamp
is the earliest known starting point.What the adjustment looks like with
debug: true
: