core(lightwallet): remove tolerance from timing budgets API

[khempenius]

Summary
This updates the existing, un-implemented API for timing budgets.

// Summary of Timing Budget API proposed in this PR

Budget.TimingBudget {
  metric: TimingMetric;
  budget: number;
}

TimingMetric = 'first-contentful-paint' | 'first-cpu-idle' | 'interactive' | 'first-meaningful-paint' | 'max-potential-fid' | 'estimated-input-latency' | 'total-blocking-time';

Comments

• I think that measurementStrategy and runs is a better way to deal with variance than setting a tolerance, hence its removal.

Related Issues
(same issue, filed two different places)

• [Lightwallet] Support timings in budgets UI #8917
• Support timing-based budgets budget.json#3

Related PRs
I will update this with the PR that implements this.

[patrickhulce]

Thanks @khempenius!

measurementStrategy?: MeasurementStrategy;

In LHCI we currently call this mergeMethod, I'm not necessarily tied to that name but measurementStrategy feels a little more like it's trying to describe how to measure each metric as opposed to how to combine multiple measurements.

WDYT of any of these alternatives? (and potentially moving to 'min'/'max' in place of 'optimistic'/'pessimistic' if we replace with statistical*)

• statisticalMethod
• statisticalStrategy
• statisticalApproach
• combinationMethod
• combinationStrategy

I think that measurementStrategy and runs is a better way to deal with variance than setting a tolerance, hence its removal.

Strongly agree with dropping tolerance 👍 SGTM!

Adding runs to budget.json complicates interactions with LighthouseCI a bit: if someone sets multiple runs while running Lighthouse CI (e.g. —numberOfRuns=5) as well as in budget.json (e.g. runs: 3),

I think runs is a field that is only ever going to apply in LHCI anyhow (i.e. Lighthouse core won't be running multiple times), so IMO we just make LHCI override it's default numberOfRuns with the budget.json runs if it is set.

[patrickhulce]

Oh for measurementStrategy it's also worth noting that for CDS we plan on introducing 'median-run' which bases the assertion on the value of the median-run according to TTI rather than the median of all observed values. This is the method that will be consistent with the LHCI UI.

[khempenius]

statisticalApproach sounds good to me. I will switch to that.

I think I prefer statistical* over combination or merge because I think the latter could be misinterpreted as relating to budget/property merging once budget.json supports extends.

[khempenius]

Interesting. Would median and median-run be two separate strategies then? Or would only median-run be supported?

[patrickhulce]

Would median and median-run be two separate strategies then? Or would only median-run be supported?

Two separate strategies. median is useful on it's own but for the UI we're only going to be comparing the median run reports so we want something that reflects that strategy as well if you want to be consistent between the two.

[patrickhulce]

statisticalApproach sounds good to me. I will switch to that.
I think I prefer statistical* over combination or merge because I think the latter could be misinterpreted as relating to budget/property merging once budget.json supports extends.

Makes sense! Should we add anything more verbose to ensure they're connected to the statistics of multiple run measurements? Nothing jumps out that isn't super super long to me though statisticalRunCombinationApproach, etc haha

Also thoughts on moving to min/max then if we're reframing as a statistical approach instead of a looser "merge method"? I'm somewhat split between them. I think min/max requires more thinking on which one you should be using but is will never require explanation of how it works compared to optimistic/pessimistic.

[khempenius]

I think I'm probably very slightly opposed to moving to median/min/max because it locks in the assumption that all performance metrics are good if low; bad if high. OTOH, there’s little precedent for a formal metric working opposite to this; a LH score is probably the only counterexample I can think of.

[patrickhulce]

I think I'm probably very slightly opposed to moving to median/min/max because it locks in the assumption that all performance metrics are good if low; bad if high. OTOH, there’s little precedent for a formal metric working opposite to this; a LH score is probably the only counterexample I can think of.

Ah, for budgets it's done only at the file level rather than individual metric/assertion level I suppose, hmmmm. This is a tricky one then 🤔

[patrickhulce]

While writing some docs I also came up with

• aggregationMethod
• aggregationStrategy
• aggregationApproach

which appeal to me to cover methods that don't have to be directly related to statistics (like in LHCI how we have the median-run which is like a hybrid median of multiple metrics to determine the "most representative" one.

[khempenius]

I really really like aggregation*. I think it is a much closer approximation than "combination". aggregationMethod is probably my favorite then.

[patrickhulce]

Sounds good I'll align on aggregationMethod as well! :D

[patrickhulce]

LGTM % potential leftovers

[brendankenny]

we should probably call this out as a breaking change somewhere in the 6.0 docs (not sure if anyone was using this yet, though)

LGTM!

[paulirish]

we should probably call this out as a breaking change somewhere in the 6.0 docs (not sure if anyone was using this yet, though)

afaik it was undocumented

i'm cool with not calling it breaking.

btw i love this approach. 👍