-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
core: add hidden layout-shift variant metrics #12046
Conversation
1c392b6
to
e9d6d49
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.
neat!
some speculation that since the window is already limited by load time there may be minimal impact for Lighthouse when switching LayoutShift metrics
I have a strong suspicion of this as well, at least for the longer windowed variants (and the tests seems to agree 😉 ). Getting this into some FR timespan testing is another story though, big game changer there :D
effect of simulated vs applied throttling (do we need some level of Lantern support for the windowed LayoutShift variants?)
Do we have reason to suspect this will be any different than for CLS? AFAIK, Lantern CLS is almost always matching or underreporting real CLS and it's essentially impossible to predict the magnitude of the shifts would have occurred when network is stretched out in any principled way.
* @param {LH.Artifacts.TraceOfTab} traceOfTab | ||
* @return {Array<LayoutShiftEvent>} | ||
*/ | ||
static getLayoutShiftEvents(traceOfTab) { |
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.
cc @adamraine maybe there's interplay between your planned refactor and this PR? seems like CLS itself could be a "layout shift variant" maxSession(allEvents, Infinity, Infinity)
heck our current impl should probably use this :)
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.
My plan for the refractor was to add the layout shift events for all frames to TraceOfTab
and then filter/aggregate the events in CLS and CLS-AF. The had_recent_input
logic would be in TraceProcessor
.
I could use this getLayoutShiftEvents
instead if it wasn't specific to main frame events. Maybe pick out the main frame events in _compute
? It would also make sense to move this function elsewhere since it wouldn't be specific to this file.
Either way, we should probably be refactoring main frame CLS so we don't repeat the had_recent_input
logic.
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 could use this
getLayoutShiftEvents
instead if it wasn't specific to main frame events. Maybe pick out the main frame events in_compute
?
yeah, that would work fine
My plan for the refractor was to add the layout shift events for all frames to
TraceOfTab
and then filter/aggregate the events in CLS and CLS-AF. Thehad_recent_input
logic would be inTraceProcessor
.
@patrickhulce are we just making our job harder for FR, though? I wonder if any correction we do should be pushed back even further...like should the correction be done during gathering, when everything knows whether or not it was a navigation
and so can confidently correct had_recent_input
?
OTOH for timeOrigin
stuff, TraceProcessor is going to have to be given some level of knowledge about what kind of run the trace comes from anyways...
(edit: or we just share a getLayoutShiftEvents
function between the various CLS impls and don't try to correct the trace at all)
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.
are we just making our job harder for FR, though? I wonder if any correction we do should be pushed back even further...like should the correction be done during gathering, when everything knows whether or not it was a navigation and so can confidently correct had_recent_input?
When we get there, the workaround will need to be an option for sure. Unfortunately, I think we're just kinda screwed for FR no matter what we do. I called this out in @adamraine's PR too that if we're auto enabling emulation with timespans we'll be stuck in between both of these (we want to ignore had_recent_input for the emulation change just like navigation but the user could have had real input too :/)
Either way, I'm fine with whatever is most comfortable for these two immediate PRs because FR trace processing is a massive project I don't expect to be significantly affected by this decision here.
edit: or we just share a getLayoutShiftEvents function between the various CLS impls and don't try to correct the trace at all
This was roughly what I had in mind originally but with options: {ignoreInitialHadRecentInputSequence: boolean}
or something
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.
Was this settled? It seems like we're still just duplicating at the moment, and I'm a little unclear of how it will be resolved in a followup.
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.
Was this settled? It seems like we're still just duplicating at the moment, and I'm a little unclear of how it will be resolved in a followup.
this is duplicating but based on #12034 (comment) (and the conversation before it) it sounded like we still need a third copy and that'll be the time to do the refactor for them to all share some level of implementation. cc @adamraine
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 can also do the follow up refactor if there's too much else going on right now.
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.
it sounded like we still need a third copy and that'll be the time to do the refactor for them to all share some level of implementation
Yeah, I think we should settle the duplication in a follow-up.
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 can also do the follow up refactor if there's too much else going on right now.
I started working on this locally, but I was waiting for this to land so I could use getLayoutShiftEvents
.
for (const event of layoutShiftEvents) { | ||
if (event.ts - prevTs > 5_000_000) clusterCount++; | ||
prevTs = event.ts; | ||
score += event.score; |
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.
are we ignoring framehole here for now?
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.
are we ignoring framehole here for now?
yes, see the // Main frame only for now
comment below. Since cumulativeLayoutShiftAllFrames
is currently in the data gathering stage it seemed like it made the most sense to make the direct comparison to main-frame-only CLS. We could add all frame versions of these variants if it made sense, though?
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'd like to include the allFrames variants as part of this yeah.
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'd like to include the allFrames variants as part of this yeah.
With the size this PR has grown, would you be ok with a follow-up?
(With #12023 it sounds like we can't do it right until m90, which won't be used by HTTP archive until April (?), so we could maybe wait for @adamraine's refactor as well if that's incoming)
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'd like to include the allFrames variants as part of this yeah.
With the size this PR has grown, would you be ok with a follow-up?
sure
it('evaluates LayoutShift variants correctly', async () => { | ||
const artifacts = { | ||
traces: { | ||
[MetricsAudit.DEFAULT_PASS]: clsAllFramesTrace, |
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.
do we have a real-world test for these where they aren't all identical? if we don't know of any test sites, something that layoutshifts in a set pattern on glitch or something might be just as quick to spin up
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.
do we have a real-world test for these where they aren't all identical? if we don't know of any test sites, something that layoutshifts in a set pattern on glitch or something might be just as quick to spin up
no, closest is frame-metrics-m89
/frame-metrics-m90
, but those both only have a single event in the main frame (the other 25 events in frame-metrics-m89
are in other frames). I didn't really want to add another trace to our menagerie but I guess it's for the best :)
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.
added a trace
No, pure--possibly not helpful--speculation :) |
also added tests for boundary conditions, because those are easy to mess up in refactors (e.g. inverting the condition but not correcting for inclusive/exclusive) and not notice. Verified the results with the Speed Metrics tool and that they fail if any of the conditionals are switched from |
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
* @param {LH.Artifacts.TraceOfTab} traceOfTab | ||
* @return {Array<LayoutShiftEvent>} | ||
*/ | ||
static getLayoutShiftEvents(traceOfTab) { |
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.
Was this settled? It seems like we're still just duplicating at the moment, and I'm a little unclear of how it will be resolved in a followup.
Adds the ("normalizing") Layout Shift variants to CLS described in "Feedback wanted: The road to a better layout shift metric for long-lived pages" to the hidden
metrics
audit.Landing soonish (and doing a release) would allow us to start collecting data over many sites using WPT throttling via HTTP Archive and using simulated throttling through local/cloud testing.
Areas of interest:
Implementation is almost entirely from https://github.com/mmocny/web-vitals/wiki/Snippets-for-LSN-using-PerformanceObserver. Further abstraction/generalization is probably not worth it since we won't be iterating on these in Lighthouse and we expect the multiple-variant implementation to be temporary.