-
Notifications
You must be signed in to change notification settings - Fork 391
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 INP breakdown timings and LoAF attribution #442
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.
In general LGTM. But do have some comments/questions
Co-authored-by: Barry Pollard <barrypollard@google.com>
Co-authored-by: Barry Pollard <barrypollard@google.com>
const processingEnd = group.processingEnd; | ||
|
||
const longAnimationFrameEntries: PerformanceLongAnimationFrameTiming[] = | ||
getIntersectingLoAFs(firstEntry.startTime, processingEnd); |
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.
@mmocny FYI the logic to select which LoAF entries get attributed to INP is here.
Hey all, thanks for the extensive reviews. Based on feedback (and internal discussions) I've updated the logic quite a bit. Here are the main changes:
@mmocny @tunetheweb Please take a look and let me know if you have any additional feedback. |
Co-authored-by: Barry Pollard <barrypollard@google.com>
} | ||
}; | ||
|
||
const cleanupEntries = () => { |
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.
@mmocny if you have time to review this function, that would be helpful. This function isn't part of the public API, but if any part of this logic fails (or is incorrect) then it could result in issues, so I'd love a second pair of eyes on it.
In short, this function tries to remove references to event
and LoAF entries that are no longer needed (so we're not just storing everything in memory), but there's the potential that I could remove a reference to something that will later been needed and then it won't get reported in the attribution object (or worse, there will be an error because other code will try to reference a property of an object that no longer exists).
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 with two outstanding questions.
Awesome work here!
Co-authored-by: Barry Pollard <barrypollard@google.com>
Co-authored-by: Brendan Kenny <bckenny@gmail.com>
This PR updates the INP attribution logic from an "interaction-based" model to a "frame-based" model. This better aligns this library's attribution data with what's exposed in the Interactions track in DevTools, as well as with the Long Animation Frame API.
The following are the most important changes that PR introduces:
In
/src/onINP.ts
:metric.entries
array would only include entries with the sameinteractionId
value. Now,metric.entries
will include allevent
entries whose processing time occurs between the user interaction and the next frame painted. This allows developers to more accurately attribute the full "processing time" of the interaction.In
/src/attribution/onINP.ts
:longAnimationFrameEntry
property will look for a correspondinglong-animation-frame
entry, and report it if found (i.e. if the browser supports the API and the frame was long enough for the entry to be dispatched).inputDelay
,processingTime
, andpresentationDelay
have been added (see the README for detailed description of each).event*
names in theINPAttribution
interface have been renamed tointeraction*
since now there is no singleevent
entry that gets reported (so the term "interaction" now is much more suitable).To see these changes visualized, below are two screenshots of traces from the INP Demo page showing the new INP attribution data in action. Notice how the "Interaction" and "Event" measures in the Timings track line up perfectly with what DevTools displays in the Interactions track (including the breakdown timing whiskers), as well as with the events in the main thread flame chart below: