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
Add a new attribution build for debugging issues in the field #237
Conversation
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.
I love the idea of adding attribution directly to the library!
It's one of the things I always have to look up, and also means that code is likely stale and not updated for any improvements. Including it in the library means users of the library will automatically benefit from those improvements (assuming you keep the library up to date of course).
Some early comments as not gone through all the actual code changes.
+ import {onLCP, onFID, onCLS} from 'web-vitals/attribution'; | ||
``` | ||
|
||
Usage for each of the imported function is identical to the standard build, but when importing from the attribution build, the [`Metric`](#metric) object will contain an additional [`attribution`](#metricwithattribution) property. |
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 we mention WHY not to use the attribution build all the time? Presumably it's more computation and a larger download so a waste if you won't load the attributions? This may be obvious, but perhaps no harm to be explicit here.
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.
Good suggestion. Will add.
Excited to see this change! I think the list of attributes for each metric is good. |
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.
I love this! Thanks for the awesome work here.
One overall Nit and a few comment in-line:
- The attribution values use the term
Delay
andTime
interchangeably. I think all of these represent durations not timestamps, but I assumed some of the "Time" attribution were timestamps because of the name differences.
Looking at the attributions, I think "Time" means "actively working" and "delay" means "waiting around", but it's a bit of a loose fit for things like TTFB.
Perhaps some more of these "Time" names should be "Delay" by those criteria, and perhaps "Time" should be renamed something like "Duration"?
Very soft opinion on that feedback.
huzzah!
…On Wed, Jul 13, 2022 at 3:17 PM Philip Walton ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/attribution/onFCP.ts
<#237 (comment)>
:
> +import {getNavigationEntry} from '../lib/getNavigationEntry.js';
+import {onFCP as unattributedOnFCP} from '../onFCP.js';
+import {FCPMetricWithAttribution, FCPReportCallback, FCPReportCallbackWithAttribution, ReportOpts} from '../types.js';
+
+
+const attributeFCP = (metric: FCPMetricWithAttribution): void => {
+ if (metric.entries.length) {
+ const navigationEntry = getNavigationEntry();
+
+ if (navigationEntry) {
+ const activationStart = navigationEntry.activationStart || 0;
+ const ttfb = Math.max(0, navigationEntry.responseStart - activationStart);
+
+ metric.attribution = {
+ timeToFirstByte: ttfb,
+ renderDelay: metric.value - ttfb,
Let's go with firstByteToFCP. It's *very* clear what timestan it covers,
and if in the future we're able to break it down further, we could add
additional sub-parts without causing too much confusion or forcing a
breaking change.
—
Reply to this email directly, view it on GitHub
<#237 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADTZKUJCPBVQIBT3XJSQA3VT4I6HANCNFSM53FK7R6A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@mmocny @tunetheweb I believe I addressed all your feedback in 7b04ae6. If you have time to take another look, that would be great. |
Very much LGTM! |
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.
LGTM
A couple of suggestions for < GA4 (since many people use this library for web-vitals-report which doesn't support GA4 yet).
be593e2
to
fd994ef
Compare
Addresses #203.
This PR adds a new "attribution" build to this library. When used, it augments the
Metric
object with anattribution
property that contains useful information to debug issues that occur in the field.This is essentially a codification of the recommendations outlined in the post Debug Web Vitals in the field, and it also includes some additional diagnostic information like outlined in the updated Optimize LCP guide.
The API to use this library doesn't change, to get the attribution data you have to important from
web-vitals/attribution
rather thanweb-vitals
:Then, when the metric callback functions are invokes, the
Metric
object passed to them contain a newattribution
property:And as an example of how you might use this in code, where's how you would send data about what element was involved in the largest layout shift contributing to CLS on the page:
The following type definitions gives an overview of what attribution data could be set on each object, depending on which metric is being observed.
CLS:
FCP:
FID:
INP:
LCP:
One downside of exposing the attribution data via a separate build is it means you either have to get attribution for all metrics or none of them—you can't just get attribution for one, e.g.
onCLS()
, and then use the unattributed versions of all the other metrics (well, you can, but doing so would import both versions and double the file size).However, given the full attribution code for all metrics only adds 500 bytes (brotli'd) to the download size, I think that's worth the convenience of not changing the API. Note: in the future, if the pipeline operator comes to JavaScript, it may be worth revisiting this decision.
Another downside is there's no easy way to make the attribution build work with the base+polyfill build without massively increasing complexity (to support ESM, UMD, and IIFE versions of each build, making a base+polyfill+attribution build would double the number of builds).
Based on the analysis done in #238, in this initial PR the attribution build does not make use of the polyfill.
In addition to the above, this PR also updates this package to use
type: module
as well as package exports, now that they are widely supported. As a result, there are additional changes fromrequire()
toimport
in this PR that are not related to the above changes.