new_audit: add Max Potential FID to JSON #5842

patrickhulce · 2018-08-16T00:23:42Z

Summary

Putting this up for discussion. Over email we settled on "Maximum Potential FID (mFID)" as a good lab FID proxy that's easy to understand. In our case, it is the longest task post-FCP and this is its PR... :)

Open questions to answer along with my thoughts

Should LH surface this metric?
Yes, mFID is easily explainable and has a clear tie to our RUM push with FID.
What should we call this metric?
Max Potential FID is what was somewhat settled on in the thread, but Estimated Input Latency is a lot prettier and legit sounding of a name to compete with, so I'm all ears to suggestions. I'd be so bold as to suggest the title is just "Expected First Input Delay" with the explanation of "maximum potential blah blah" in the description tooltip, but I'm certain others will disagree :)
What do we do with Estimated Input Latency?
This PR moves it down to LHR only but we can do other things. Keeping it in right now makes the metrics section a bit weird with 3-4. EIL's implementation with thresholds and boundaries has always been somewhat tenuous. Until we have a clear motivating example for devs, I'd prefer a FID stand-in. We're just going to land this in the JSON and see how it compares in HTTPArchive for now.

Key Change Introduced By This PR

~~This PR also adjusts the EIL estimate multipliers to fully represent the Lantern prediction. Previously the raw EIL estimate was reduced by 20% before being surfaced in the UI.~~ This will be split out into a separate PR.

cc @tdresser @philipwalton

tdresser · 2018-08-16T12:52:41Z

@stubbornella

I think this is reasonable. It does make the story around page load interactivity a bit more confusing though.

Do we have any way of evaluating the success of a change like this?

@addyosmani - should we have some documentation on how mFID / FID / TTI all relate before shipping this in lighthouse?

philipwalton · 2018-08-16T17:58:39Z

I'm OK with calling it "Estimated First Input Delay" (note, I wouldn't use the acronym since we don't use it for other metrics).

I'm also OK with removing estimated input latency entirely since it's not super clear how it's calculated (and thus not super clear how to fix it if you're getting a value higher than you expect).

Overall, I'm happy to see this as part of Lighthouse. If nothing else, it'll help make developers aware of long tasks in their script initialization that they should break up.

brendankenny

Well, technically it is the current EIL queried at the 100th percentile :D

The name is admittedly kind of awkward, but I like it because it really is the max potential FID, and "estimated"/"expected" each bring their own nomenclature baggage that I think will end up confusing things.

Current EIL I agree isn't super helpful and I'd personally like to see it come back in moving-window graph form (when we resurrect the graph) as a diagnostic for thread busyness. That makes issues with arbitrary window size and percentiles go away and doesn't discard the rich amount of data available to LH in the trace for a single summary statistic.

paulirish · 2018-08-16T21:44:12Z

lighthouse-core/gather/computed/metrics/max-potential-fid.js

+    ).filter(evt => evt.duration >= 1);
+
+    return Promise.resolve({
+      timing: Math.max(...events.map(evt => evt.duration), 16),


timing and not timeInMs?

yeppers

lighthouse/typings/artifacts.d.ts

Lines 345 to 348 in c062cf3

export interface Metric {

timing: number;

timestamp?: number;

}

paulirish · 2018-08-16T21:44:39Z

lighthouse-core/audits/metrics/max-potential-fid.js

+
+const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);
+
+class MaxPotentialFID extends Audit {


can we get some overview comment to explain the basics of how this metric is calculated?

paulirish · 2018-08-16T21:50:42Z

lighthouse-core/gather/computed/metrics/lantern-max-potential-fid.js

+    return {
+      intercept: 0,
+      optimistic: 0.4,
+      pessimistic: 0.4,


whats this mean? the two graphs are the same. Both are adjusted?

similarly, lantern-speed-index has two identical graphics but then has different coefficients.

The graphs aren't the differentiating part of the optimistic/pessimistic equation here, it's actually whether we use optimistic/pessimistic FCP as the start point :)

The 0.4 was to mirror EIL, but honestly the justification for it (that we'll overestimate EIL a tad) hasn't proved to be true in the data, so I'll stick to 0.5 (simple average of optimistic + pessimistic estimates)

For speed index case the "optimistic"/"pessimistic" labels are a real stretch. The "optimistic" one is just normal speed index on the fast trace, the graph is essentially ignored completely here. The "pessimistic" one is our lantern psuedo-speed index based on the TTI graph.

paulirish · 2018-08-16T22:29:06Z

Should LH surface this metric?

yup

What should we call this metric?

Def think it should tie into "First Input Delay" closely.
Right now I'm most fond of First Input Delay Impact

What do we do with Estimated Input Latency?

Comment it out until its glorious return as a rolling EQT diagnostic.

addyosmani · 2018-08-17T18:31:45Z

should we have some documentation on how mFID / FID / TTI all relate before shipping this in lighthouse?

Our interactivity narrative over the last quarter has focused on TTI in the lab and FID in the field; I'd love to make sure First Input Delay Impact or Estimated First Input Delay (whichever name we decide on) is well covered in our metrics guidance so it's clear:

TTI still matters (eFID compliments TTI without diminishing it)
How to think about our two lab metrics for interactivity (eFID is a clear lab corollary for FID, how it differs).

@philipwalton Will you have bandwidth to update our metrics docs to reflect the above?
@kaycebasques would be great to similarly see if we can get a Lighthouse doc up about eFID.

patrickhulce · 2018-12-10T22:38:31Z

The new plan here is to land this without exposing for now, so we can take a look at the HTTPArchive results and how it compares to EIL/TTI/etc

patrickhulce · 2019-01-14T16:46:50Z

bump @brendankenny @paulirish :)

paulirish

LGTM.

paulirish · 2019-01-14T17:47:22Z

lighthouse-core/computed/metrics/lantern-max-potential-fid.js

+      if (node.type !== BaseNode.TYPES.CPU) continue;
+      if (timing.endTime < fcpTimeInMs) continue;
+
+      events.push({


you could just filter nodeTimings instead of constructing these new objects...

good point! done

new_audit(mfid): add Max Potential FID metric

6372106

patrickhulce requested review from brendankenny and paulirish as code owners August 16, 2018 00:23

patrickhulce changed the title ~~new_audit(mfid): add Max Potential FID metric~~ new_audit(mFID): add Max Potential FID metric Aug 16, 2018

patrickhulce changed the title ~~new_audit(mFID): add Max Potential FID metric~~ new_audit: add Max Potential FID metric Aug 16, 2018

brendankenny reviewed Aug 16, 2018

View reviewed changes

paulirish reviewed Aug 16, 2018

View reviewed changes

feedback

d54ad37

patrickhulce changed the title ~~new_audit: add Max Potential FID metric~~ new_audit: add First Input Delay metric Aug 22, 2018

patrickhulce added 3 commits December 10, 2018 13:21

Merge branch 'master' into max_fid

09e4791

fix after merge

a330763

test fixes

bce2822

patrickhulce requested a review from exterkamp as a code owner December 10, 2018 19:50

patrickhulce changed the title ~~new_audit: add First Input Delay metric~~ new_audit: add Max Potential FID to JSON Dec 10, 2018

undo EIL change

a69895f

add test

e2b6999

patrickhulce force-pushed the max_fid branch from 4386d40 to e2b6999 Compare December 10, 2018 22:45

paulirish approved these changes Jan 14, 2019

View reviewed changes

patrickhulce added 2 commits January 14, 2019 13:56

just use filter instead

835967c

Merge branch 'master' into max_fid

6554fa1

patrickhulce merged commit c4d8983 into master Jan 30, 2019

patrickhulce deleted the max_fid branch January 30, 2019 02:28

patrickhulce mentioned this pull request Apr 30, 2019

core: replace EIL with MP-FID #8729

Merged

patrickhulce mentioned this pull request May 16, 2019

new_audit: add TTI Companion Metric to JSON #8975

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new_audit: add Max Potential FID to JSON #5842

new_audit: add Max Potential FID to JSON #5842

patrickhulce commented Aug 16, 2018 •

edited

Loading

tdresser commented Aug 16, 2018

philipwalton commented Aug 16, 2018

brendankenny left a comment

paulirish Aug 16, 2018

patrickhulce Aug 16, 2018

paulirish Aug 16, 2018

patrickhulce Aug 16, 2018

paulirish Aug 16, 2018

patrickhulce Aug 16, 2018

paulirish commented Aug 16, 2018

addyosmani commented Aug 17, 2018

patrickhulce commented Dec 10, 2018

patrickhulce commented Jan 14, 2019

paulirish left a comment

paulirish Jan 14, 2019

patrickhulce Jan 14, 2019

	export interface Metric {
	timing: number;
	timestamp?: number;
	}


		const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);

		class MaxPotentialFID extends Audit {

new_audit: add Max Potential FID to JSON #5842

new_audit: add Max Potential FID to JSON #5842

Conversation

patrickhulce commented Aug 16, 2018 • edited Loading

Summary

Open questions to answer along with my thoughts

Key Change Introduced By This PR

tdresser commented Aug 16, 2018

philipwalton commented Aug 16, 2018

brendankenny 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Aug 16, 2018

addyosmani commented Aug 17, 2018

patrickhulce commented Dec 10, 2018

patrickhulce commented Jan 14, 2019

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

patrickhulce commented Aug 16, 2018 •

edited

Loading