core(largest-contentful-paint): add new metric #9706

connorjclark · 2019-09-20T03:38:41Z

https://web.dev/largest-contentful-paint

paulirish

great first pass

lighthouse-core/computed/metrics/lantern-largest-contentful-paint.js

lighthouse-core/computed/metrics/lantern-first-meaningful-paint.js

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

paulirish · 2019-09-20T07:50:12Z

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

+    for (let i = frameEvents.length - 1; i >= 0; i--) {
+      const e = frameEvents[i];
+      if (e.ts <= navigationStart.ts) break;
+      if (e.name === 'largestContentfulPaint::Invalidate') break;


can we add a test for this case?

i'd like to base the test from a real trace.

can we get a single comment above each if case space shuttle style? in my first read I missed that we were counting backwards and was very confused :)

+1 to test cases for these :)

hmmmm luckily i just made a huge corpus of new traces.

I ran it on a bunch of traces
(i also put a log at if (e.name === 'largestContentfulPaint::Invalidate')):

find dist/collect-lantern-traces -name '*trace.json' | xargs -I{} node -e "\ const path = '{}'; \ console.log(path); \ const trace = JSON.parse(require('fs').readFileSync(path, 'utf-8')); \ // wpt traces start with an empty event ... trace.traceEvents = trace.traceEvents.filter(e => Object.keys(e).length > 0); \ const traceOfTab = require('./lighthouse-core/lib/tracehouse/trace-processor.js').computeTraceOfTab(trace); \ console.log(traceOfTab.timings.largestContentfulPaint)";

and I never hit the log point.

In fact, only two of these collected traces have largestContentfulPaint::Invalidate.

https---www-huffpost-com--mobile-unthrottled-4-trace.json - it's not from the main frame, so we ignore it.

https---www-reddit-com--mobile-wpt-2-trace.json - after a short gap, it's followed by a largestContentfulPaint::Candidate.

according to https://chromium-review.googlesource.com/c/chromium/src/+/1745433/19#message-435ee4e2ea3f8d117b90bbc4f00c4f4486928eba

These pages have image animated carousel that makes LCP generates both candidate and invalidate events - http://www.jd.com/ , http://www.kakao.com/, http://www.dnevnik.ru/, http://www.united.com/

note that some basic test cases can also be constructed by e.g. create-test-trace.js (and then adding LCP events), or lack of LCP can be found with existing traces :) This should make testing at least some of this more lightweight.

It's obviously still good to find a real trace as you're doing to make sure the tests match what happens in reality (especially with cases like invalidation)

IMO a real trace test for this does not need to be blocking landing (create-test-trace.js is nice :)), though if we wanted to add a flag that indicates if we fell into the invalidate path so we can examine frequency in HTTPArchive it wouldn't be the worst idea :)

would still love some of those space shuttle comments though :D

paulirish · 2019-09-20T07:51:03Z

lighthouse-core/test/results/sample_v2.json

-        ]
-      }
+      "scoreDisplayMode": "error",
+      "errorMessage": "Something went wrong with recording the trace over your page load. Please run Lighthouse again. (NO_FMP)"


that isn't supposed to happen.. :)

I used the wrong error code, should have been NO_LCP here (since the sample artifacts have not been updated yet).

updated just the traces now ...

Co-Authored-By: Paul Irish <[email protected]>

connorjclark · 2019-09-23T17:03:56Z

lighthouse-core/test/computed/page-dependency-graph-test.js

@@ -9,8 +9,8 @@ const PageDependencyGraph = require('../../computed/page-dependency-graph.js');
 const BaseNode = require('../../lib/dependency-graph/base-node.js');
 const NetworkRequest = require('../../lib/network-request.js');

-const sampleTrace = require('../fixtures/traces/progressive-app-m60.json');
-const sampleDevtoolsLog = require('../fixtures/traces/progressive-app-m60.devtools.log.json');
+const sampleTrace = require('../fixtures/traces/progressive-app-m79.json');


should I have generate this trace/logs in a different way than just running on https://pwa.rocks/ w/ default options? A lot of the numbers that changed with -u seem too different.

the trace that was recorded before used devtools throttling, see my other thoughts on using a new trace though :)

Co-Authored-By: Brendan Kenny <[email protected]>

brendankenny

two last things :)

lighthouse-core/audits/metrics.js

lighthouse-core/test/computed/metrics/largest-contentful-paint-test.js

Co-Authored-By: Brendan Kenny <[email protected]>

…-test.js Co-Authored-By: Brendan Kenny <[email protected]>

brendankenny

LGTM! 🐘🖼 🚢

brendankenny · 2019-10-02T02:49:48Z

lighthouse-core/audits/metrics.js

@@ -115,7 +120,7 @@ class Metrics extends Audit {
    const details = {
      type: 'debugdata',
      // TODO: Consider not nesting metrics under `items`.
-      items: [metrics],
+      items: [metrics, {lcpInvalidated: traceOfTab.lcpInvalidated}],


maybe this should be observedLcpInvalidated or something but I can't imagine it ever mattering (and maybe like fmpFellBack it'll eventually just go unused in the artifacts)

connorjclark added 6 commits September 19, 2019 18:20

first pass

ec75de3

update

183b080

i18n

03f8ab6

typos

52a8d28

metrics

4e2e688

ts

f1836a0

connorjclark requested review from brendankenny, patrickhulce and paulirish as code owners September 20, 2019 03:38

googlebot added the cla: yes label Sep 20, 2019

fix

449c7c5

vercel bot deployed to staging September 20, 2019 03:39 View deployment

paulirish reviewed Sep 20, 2019

View reviewed changes

connorjclark and others added 2 commits September 20, 2019 13:51

Apply suggestions from code review

d821da7

Co-Authored-By: Paul Irish <[email protected]>

todo

9cc6b8c

vercel bot deployed to staging September 20, 2019 20:51 View deployment

fix lcp

211a90b

connorjclark requested a review from exterkamp as a code owner September 20, 2019 20:56

vercel bot deployed to staging September 20, 2019 20:56 View deployment

update sample trace

ee9f34e

vercel bot deployed to staging September 20, 2019 21:04 View deployment

tweaks

24db2ed

vercel bot deployed to staging September 20, 2019 21:47 View deployment

fix metrics-test

308f0fe

vercel bot deployed to staging September 20, 2019 21:53 View deployment

rename all those pwa trace requires

33fecc9

vercel bot deployed to staging September 20, 2019 21:54 View deployment

just update all the metric snapshots

2b87da0

vercel bot deployed to staging September 20, 2019 22:07 View deployment

connorjclark commented Sep 23, 2019

View reviewed changes

typo

697cf9b

vercel bot deployed to staging October 1, 2019 21:56 View deployment

timings

2bb6b26

vercel bot deployed to staging October 1, 2019 21:58 View deployment

timestamps

c80abb6

vercel bot deployed to staging October 1, 2019 21:59 View deployment

nolcp

5fa41eb

vercel bot temporarily deployed to staging October 1, 2019 22:02 Inactive

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

ca65238

Co-Authored-By: Brendan Kenny <[email protected]>

vercel bot deployed to staging October 1, 2019 22:02 View deployment

more tests

5fe7a48

vercel bot deployed to staging October 1, 2019 22:07 View deployment

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

7c35c8b

Co-Authored-By: Brendan Kenny <[email protected]>

vercel bot deployed to staging October 1, 2019 22:11 View deployment

connorjclark added 2 commits October 1, 2019 15:14

tst

9df0ea6

Merge branch 'lcp' of github.com:GoogleChrome/lighthouse into lcp

917f73a

vercel bot deployed to staging October 1, 2019 22:32 View deployment

brendankenny reviewed Oct 1, 2019

View reviewed changes

lighthouse-core/audits/metrics.js Outdated Show resolved Hide resolved

lighthouse-core/test/computed/metrics/largest-contentful-paint-test.js Outdated Show resolved Hide resolved

Update lighthouse-core/audits/metrics.js

0c0dc97

Co-Authored-By: Brendan Kenny <[email protected]>

vercel bot temporarily deployed to staging October 1, 2019 22:52 Inactive

Update lighthouse-core/test/computed/metrics/largest-contentful-paint…

6cc68e3

…-test.js Co-Authored-By: Brendan Kenny <[email protected]>

vercel bot deployed to staging October 1, 2019 22:52 View deployment

sample

793bfc6

vercel bot deployed to staging October 2, 2019 01:46 View deployment

brendankenny approved these changes Oct 2, 2019

View reviewed changes

brendankenny merged commit ac67b37 into master Oct 2, 2019

brendankenny deleted the lcp branch October 2, 2019 02:55

brendankenny mentioned this pull request Oct 2, 2019

[meta] Lighthouse 6.0 Burndown #9774

Closed

54 tasks

paulirish pushed a commit that referenced this pull request Nov 6, 2019

core: add largest-contentful-paint metric to JSON (#9706)

8c330d8

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(largest-contentful-paint): add new metric #9706

core(largest-contentful-paint): add new metric #9706

connorjclark commented Sep 20, 2019 •

edited by brendankenny

Loading

paulirish left a comment

paulirish Sep 20, 2019

patrickhulce Sep 23, 2019

patrickhulce Sep 23, 2019

connorjclark Sep 27, 2019

connorjclark Sep 27, 2019 •

edited

Loading

brendankenny Sep 30, 2019

brendankenny Sep 30, 2019

patrickhulce Oct 1, 2019

patrickhulce Oct 1, 2019

paulirish Sep 20, 2019

connorjclark Sep 20, 2019

connorjclark Sep 20, 2019

connorjclark Sep 23, 2019

patrickhulce Sep 23, 2019

brendankenny left a comment

brendankenny left a comment

brendankenny Oct 2, 2019

core(largest-contentful-paint): add new metric #9706

core(largest-contentful-paint): add new metric #9706

Conversation

connorjclark commented Sep 20, 2019 • edited by brendankenny Loading

paulirish left a 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 Sep 27, 2019 • 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

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Sep 20, 2019 •

edited by brendankenny

Loading

connorjclark Sep 27, 2019 •

edited

Loading