-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: Upgrade to web-vitals@4.0.0 #152
Conversation
4f5991c
to
a95522b
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.
🎉 Amazing! Can we these new attributes to our docs for web vitals attributes? Up to you if you want to do it as a separate PR, that works too!
[`${attrPrefix}.presentation_delay`]: presentationDelay, | ||
[`${attrPrefix}.processing_duration`]: processingDuration, | ||
// These will be deprecated in a future version | ||
[`${attrPrefix}.element`]: interactionTarget, |
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.
definitely defer to purvi & other experts here, but i'd like to vote to keep {attrPrefix}.element
as the designated field (or at least dual write) — one struggle for folks less familiar with the specific data shape of various events is trying to remember what field name goes with what event type. ie instead of making people remember for inp
, the element is listed under interaction_target
, for cls
, it's the largest_shift_target
, and for lcp
, it's element
, keep all targets referenced under the same name — the element
more in-depth writeup/thoughts here: #61
For metrics which include reference to a specific element — largest contentful paint, cumulative layout score, interaction to next paint — that element is always included in [metric].element, instead of a metric-specific key (inp.element instead of inp.event_target, cls.element instead of cls.largest_shift_target. lcp.element remains lcp.element). Although we do lose specific signal around why the element is being referenced, I think there's benefit to folks only needing to remember one field name. Each metric has only one specific, metric-related reason for referencing an element; I don't think we're losing too much signal by shedding the specificity in the key.
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.
That's great additional context! I think that makes me more inclined to dual write. My _evolved thinking is that we should map 1-1 with the shape of web vitals data, and we can _additionally _ in a separate namespace provide more ergonomic access and/or derived data. Since we're still in EA and not removing any fields, let's merge this, and we open up a follow-up PR to tackle any changes as our thinking evolves.
Which problem is this PR solving?
Web vitals but released 4.0.0 which includes an updates to field names and additional for
LCPAttribution
,INPAttribution
andTTFBAttribution
and deprecatedonFID
. See the full changes here.Short description of the changes
Upgrade to
web-vitals@4.0.0
. Full change log of the underlying changes can be found here.For this release, we'll send both the updated field names and the deprecated field names, which will be removed in the next major release.
Additionally,
onFID
has been deprecated and will removed in the next major release.For this instrumentation we have updated attribute payloads as follows:
LCP
:resource_load_time
->resource_load_duration
resource_load_time
INP
:element
->interaction_target
element
event_type
->interaction_type
event_type
interaction_time
input_delay
interaction_target_element
next_paint_time
presentation_delay
processing_duration
TTFB
:waiting_time
->waiting_duration
waiting_time
dns_time
->dns_duration
dns_time
connection_time
->connection_duration
connection_time
request_time
->request_duration
request_time
cache_duration
How to verify that this has the expected result
See that the new new and updated properties are being reported.