Skip to content
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: round metric savings to remove false precision #15721

Merged
merged 8 commits into from
Jan 4, 2024

Conversation

adamraine
Copy link
Member

@adamraine adamraine commented Jan 2, 2024

This PR prevents us from delivering too much precision in metric savings by rounding the result. It also creates a minimum threshold for metric savings to be nonzero which is important because that is the difference between a score of 1 and 0.5.

With both of these goals in mind, I decided to just use Math.floor instead of Math.round but open to other approaches.

@adamraine adamraine requested a review from a team as a code owner January 2, 2024 18:39
@adamraine adamraine requested review from connorjclark and removed request for a team January 2, 2024 18:39
Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I think @brendankenny should review all things numbers :)

@connorjclark connorjclark changed the title core: normalize metric savings core: round metric savings to remove unwarranted precision Jan 2, 2024
@connorjclark
Copy link
Collaborator

I changed PR title (mentioning for you to review)

@adamraine adamraine changed the title core: round metric savings to remove unwarranted precision core: round metric savings to remove false precision Jan 2, 2024
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just a couple of possible nits

core/audits/audit.js Outdated Show resolved Hide resolved
if (value === undefined) continue;

const precision = METRIC_SAVINGS_PRECISION[key];
if (precision === undefined) continue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: is this check needed (since key has been cast)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, because the properties are all optional. You could make METRIC_SAVINGS_PRECISION Record<keyof LH.Audit.MetricSavings, number> or Required<LH.Audit.MetricSavings> (that would require a precision for INP, but maybe that's good anyways)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point about INP, I forgot that and will add. With recent type changes we would need to also provide precision for stuff like SI and FMP which will probably never have metric savings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of a side point, but looking at #15540 now, maybe it's not worth lhr.d.ts knowing about the metric acronyms? There's benefits to the report having to deal with the generic acronym string matching (free backwards/forwards compatibility), while probably still enforcing specific acronyms in core/.

It seems like the renderer implementation wouldn't have to change, just the types, but overconstraining the types now may require more work down the line. I guess the main weird thing might be AuditResult.MetricSavings would then need to be Record<string, number> while we'd still want Audit.ProductBase.metricSavings to be the enumerated set of acronyms.

core/audits/audit.js Outdated Show resolved Hide resolved
core/audits/audit.js Outdated Show resolved Hide resolved
@@ -5195,7 +5195,7 @@
"numericUnit": "element",
"displayValue": "153 elements",
"metricSavings": {
"TBT": 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol 1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, one of the reasons this PR exists

@@ -18550,7 +18550,7 @@
"numericUnit": "millisecond",
"displayValue": "0.2 s",
"metricSavings": {
"TBT": 25.85177364864864
"TBT": 50
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gah, I can see the appeal of using Math.floor() in examples like this. Because it's potential savings we don't want to overestimate the value. Agreed with @connorjclark that rounding 199 to 100 isn't great, but so is overpromising the savings by 2x here.

OTOH using a precision of 50 and Math.round() means the rounded value is only ever a max of 25ms away from the original value. To do the same with Math.floor() we'd have to drop the granularity to 25, which may be getting too precise again.

IMO in terms of purity (don't overpromise savings you won't be able to get) Math.floor() makes the most sense. However in practice, considering the chosen granularity levels and the lack of actual accuracy in the initial estimate, I think it probably doesn't matter, and so Math.round() works fine and arguably better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think the case I'm more worried about is this audit score goes from 0.5 to 1 just because of 25.x ms of savings. Perhaps a follow-up PR could introduce some minimum threshold for that condition as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants