core(metrics): support LCP in all frames for devtools throttling #11701

adamraine · 2020-11-23T21:46:43Z

Part 1a of the Framehole implementation plan for LCP events.

lighthouse-core/test/computed/metrics/timing-summary-test.js

patrickhulce

thanks @adamraine :)

lighthouse-core/computed/metrics/largest-contentful-paint-all-frames.js

lighthouse-core/computed/metrics/timing-summary.js

lighthouse-core/test/computed/metrics/timing-summary-test.js

lighthouse-core/test/lib/tracehouse/trace-processor-test.js

adamraine · 2020-11-24T22:19:06Z

@patrickhulce @connorjclark Just a heads up, I'm gonna bring up another PR to merge into this one which supports CLS from all frames.

connorjclark · 2020-11-25T01:59:31Z

can we keep that as a separate PR (and not merge into this branch)?

adamraine · 2020-11-25T02:09:19Z

can we keep that as a separate PR (and not merge into this branch)?

Sure

connorjclark

LGTM. Patrick, wdyt?

lighthouse-core/lib/tracehouse/trace-processor.js

patrickhulce · 2020-11-25T20:40:29Z

lighthouse-core/test/fixtures/traces/lcp-all-frames-m89.devtools.log.json

@@ -0,0 +1,55 @@
+[


muuuuuch better :) thanks!

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

adamraine · 2020-11-25T21:10:04Z

@patrickhulce @connorjclark Do we need to wait for v7 to merge this?

patrickhulce · 2020-11-25T21:22:32Z

I think the opposite @adamraine we really want to get this in for the last 6.x

paulirish

noticed a few things that might matter to the calculation accuracy under certain conditions. @adamraine lemme know if you've already spotted this stuff.

paulirish · 2020-11-30T21:41:14Z

lighthouse-core/lib/tracehouse/trace-processor.js

+    const lcpAllFramesEvt = this.computeValidLCP({
+      candidateEventName: 'NavStartToLargestContentfulPaint::Candidate::AllFrames::UKM',
+      invalidateEventName: 'NavStartToLargestContentfulPaint::Invalidate::AllFrames::UKM',
+      events: keyEvents,


I think there's an edge case this may not account for.

The situation would be theverge loading in one tab while nytimes is loading in another tab. keyEvents is for all processes and so I think the resulting LCP-AF would merge together both tabs.

I'm not sure this is the case, but it looks like it...

the trace event lives in browser/ so my guess is it's emitted on the browser process. it does contain a mainframeid however. and we have findMainFrameIds, so I think we can filter the trace events to be just the mainframeid ones we care about.

The situation would be theverge loading in one tab while nytimes is loading in another tab. keyEvents is for all processes and so I think the resulting LCP-AF would merge together both tabs.

I jumped to this as well, but I tried on half a dozen pages unsuccessfully 😅 Is the "looks like it" from looking at the code in Chromium or a case that you observed?

What would a trace like this look like?

Using the performance panel, I am able to produce a trace with LCP-AF events with two different values for main_frame_tree_node_id.

hmm i just got one.

trace: https://gist.github.com/paulirish/ebfeeac8d2a243042041ab4e1bc87c7f

repro:

chrome-debug

open devtools

open another chrome window, and make sure both windows are visible (not occluding the other)

put example.com in window 1's omnibox and paulirish.com in window 2's.

start perf panel recording

hit enter on both omniboxes

wait a bit and stop recording

and make sure both windows are visible (not occluding the other)

🤦 this is almost definitely what I forgot

@paulirish findMainFrameIds doesn't return a node id, is there another way to get the main frame node id?

@paulirish findMainFrameIds doesn't return a node id, is there another way to get the main frame node id?

good question.... doing this would certainly be easier if main_frame_tree_node_id was instead main_frame_tree_frame_id but alas. Hmmmm.. we'll have to do some hunting.

@npm1 do you currently make use of main_frame_tree_frame_id to filter things? as of now, we don't have any mapping between frames/frametrees to node ids, so before we invent one, I'm curious if something already exists.

paulirish · 2020-11-30T21:43:42Z

lighthouse-core/lib/tracehouse/trace-processor.js

@@ -543,6 +586,7 @@ class TraceProcessor {
        firstContentfulPaint: frameTimings.timings.firstContentfulPaint,
        firstMeaningfulPaint: frameTimings.timings.firstMeaningfulPaint,
        largestContentfulPaint: frameTimings.timings.largestContentfulPaint,
+        largestContentfulPaintAllFrames: maybeGetTiming(lcpAllFramesEvt && lcpAllFramesEvt.ts),


something i spotted while looking at the trace event.....

https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/page_load_metrics/observers/core/ukm_page_load_metrics_observer.cc;l=1091-1095;drc=2b934611e4e480b7121488288f97ef27e0950e2a

It seems like sometimes the trace event carries a specific timestamp.. so i think we'd want to use that rather than the event's ts.

I can't seem to find the timestamp you're referring to. There is only a durationInMilliseconds property.

my silly project agrees with @adamraine https://github.com/connorjclark/chrome-trace-events-tsc/blob/master/dist/lh-trace-events.d.ts#L487 (altho I only used one recent trace from cnn.com)

@connorjclark thanks for checking. yeah you can see in the .cc source i linked that there are two ways the LCP trace event are emitted. I haven't really tracked back the conditions that determine which of the two are used.. but.. we'd need enough traces to get both of em for yr project to catch it, i guess. no big deal though.

@adamraine ah yeah.. actually the durationInMilliseconds prop is exactly what i was talking about. (I hadn't clicked through on DataAsTraceValue() till now).

But yes it looks vaguely like: if durationInMilliseconds is being provided, it should be used rather than the ts of the event. @npm1 is that what's going on?

if durationInMilliseconds is being provided, it should be used rather than the ts of the event.

👀 at our time origin complexities, oh boy 😅

Correct, durationInMilliseconds is the LCP value. Sorry for the confusion, maybe there was a way to have the trace event's timestamp be the LCP itself? It's probably a bit late to fix.

adamraine added 4 commits November 23, 2020 16:14

first pass

49794b3

update trace of tab test

ab8b7e6

update metrics test

412e401

update sample

dfc76f5

adamraine requested a review from a team as a code owner November 23, 2020 21:46

adamraine requested review from connorjclark and removed request for a team November 23, 2020 21:46

google-cla bot added the cla: yes label Nov 23, 2020

vercel bot deployed to Preview November 23, 2020 21:47 View deployment

adamraine commented Nov 23, 2020

View reviewed changes

lighthouse-core/test/computed/metrics/timing-summary-test.js Show resolved Hide resolved

Merge branch 'master' into lcp-all-frames-metrics

bf7b221

vercel bot deployed to Preview November 23, 2020 22:01 View deployment

patrickhulce reviewed Nov 24, 2020

View reviewed changes

devtools-bot assigned connorjclark Nov 24, 2020

devtools-bot added the waiting4reviewer label Nov 24, 2020

don't fall back

9a563b0

vercel bot deployed to Preview November 24, 2020 17:55 View deployment

smaller trace

976d3bd

vercel bot deployed to Preview November 24, 2020 19:26 View deployment

updates sample

582452e

vercel bot deployed to Preview November 24, 2020 19:49 View deployment

remove all frames invalidate

d7cba0a

vercel bot deployed to Preview November 25, 2020 17:03 View deployment

update sample and tests

241cb3d

vercel bot deployed to Preview November 25, 2020 17:14 View deployment

adamraine mentioned this pull request Nov 25, 2020

core(metrics): support CLS for all frames #11713

Merged

connorjclark approved these changes Nov 25, 2020

View reviewed changes

patrickhulce approved these changes Nov 25, 2020

View reviewed changes

= -> undefined

63a25b9

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

vercel bot deployed to Preview November 25, 2020 20:50 View deployment

adamraine merged commit bca4e21 into master Nov 26, 2020

adamraine deleted the lcp-all-frames-metrics branch November 26, 2020 01:26

paulirish reviewed Nov 30, 2020

View reviewed changes

adamraine mentioned this pull request Dec 3, 2020

core(trace): compute trace for main frame and any child frames #11760

Merged

adamraine mentioned this pull request Dec 22, 2020

core(metrics): support FCP for all frames with devtools throttling #11874

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(metrics): support LCP in all frames for devtools throttling #11701

core(metrics): support LCP in all frames for devtools throttling #11701

adamraine commented Nov 23, 2020

patrickhulce left a comment

adamraine commented Nov 24, 2020

connorjclark commented Nov 25, 2020 •

edited

Loading

adamraine commented Nov 25, 2020

connorjclark left a comment

patrickhulce Nov 25, 2020

adamraine commented Nov 25, 2020

patrickhulce commented Nov 25, 2020

paulirish left a comment

paulirish Nov 30, 2020

patrickhulce Nov 30, 2020 •

edited

Loading

adamraine Nov 30, 2020

paulirish Nov 30, 2020

patrickhulce Nov 30, 2020

adamraine Nov 30, 2020

paulirish Dec 2, 2020

paulirish Nov 30, 2020

adamraine Nov 30, 2020

connorjclark Dec 1, 2020

paulirish Dec 2, 2020

patrickhulce Dec 2, 2020

npm1 Dec 3, 2020

core(metrics): support LCP in all frames for devtools throttling #11701

core(metrics): support LCP in all frames for devtools throttling #11701

Conversation

adamraine commented Nov 23, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

adamraine commented Nov 24, 2020

connorjclark commented Nov 25, 2020 • edited Loading

adamraine commented Nov 25, 2020

connorjclark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamraine commented Nov 25, 2020

patrickhulce commented Nov 25, 2020

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Nov 25, 2020 •

edited

Loading

patrickhulce Nov 30, 2020 •

edited

Loading