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.

Sign up for GitHub

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

Jump to bottom

core: round metric savings to remove false precision #15721

Merged
merged 8 commits into from
Jan 4, 2024
Merged

core: round metric savings to remove false precision #15721

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1dca7bf
core: normalize metric savings
adamraine Jan 2, 2024
d6cea5d
test
adamraine Jan 2, 2024
f632bb0
sample
adamraine Jan 2, 2024
68eb7ce
pr
adamraine Jan 2, 2024
c7c49aa
Merge branch 'main' into normalize-metric-savings
adamraine Jan 4, 2024
9757c3a
inp
adamraine Jan 4, 2024
34b4779
nits
adamraine Jan 4, 2024
b83a126
nit logic
adamraine Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions core/audits/audit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,14 @@ import {Util} from '../../shared/util.js';

const DEFAULT_PASS = 'defaultPass';

/** @type {LH.Audit.MetricSavings} */
const METRIC_SAVINGS_PRECISION = {
FCP: 50,
LCP: 50,
TBT: 50,
CLS: 0.001,
};
adamraine marked this conversation as resolved.
Show resolved Hide resolved

/**
* @typedef TableOptions
* @property {number=} wastedMs
Expand Down Expand Up @@ -339,6 +347,30 @@ class Audit {
return score;
}

/**
* @param {LH.Audit.MetricSavings|undefined} metricSavings
* @return {LH.Audit.MetricSavings|undefined}
*/
static _normalizeMetricSavings(metricSavings) {
adamraine marked this conversation as resolved.
Show resolved Hide resolved
if (!metricSavings) return;

/** @type {LH.Audit.MetricSavings} */
const normalizedMetricSavings = {...metricSavings};

// eslint-disable-next-line max-len
for (const key of /** @type {Array<keyof LH.Audit.MetricSavings>} */ (Object.keys(metricSavings))) {
const value = metricSavings[key];
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.


normalizedMetricSavings[key] = Math.round(value / precision) * precision;
adamraine marked this conversation as resolved.
Show resolved Hide resolved
}

return normalizedMetricSavings;
}

/**
* @param {typeof Audit} audit
* @param {string | LH.IcuMessage} errorMessage
Expand Down Expand Up @@ -378,10 +410,12 @@ class Audit {
scoreDisplayMode = product.scoreDisplayMode;
}

const metricSavings = Audit._normalizeMetricSavings(product.metricSavings);

if (scoreDisplayMode === Audit.SCORING_MODES.METRIC_SAVINGS) {
if (score && score >= Util.PASS_THRESHOLD) {
score = 1;
} else if (Object.values(product.metricSavings || {}).some(v => v)) {
} else if (Object.values(metricSavings || {}).some(v => v)) {
adamraine marked this conversation as resolved.
Show resolved Hide resolved
score = 0;
} else {
score = 0.5;
Expand Down Expand Up @@ -419,7 +453,7 @@ class Audit {
errorStack: product.errorStack,
warnings: product.warnings,
scoringOptions: product.scoringOptions,
metricSavings: product.metricSavings,
metricSavings,

details: product.details,
guidanceLevel: audit.meta.guidanceLevel,
Expand Down
8 changes: 8 additions & 0 deletions core/test/audits/audit-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,14 @@ describe('Audit', () => {
assert.strictEqual(auditResult.score, 0.3);
});

it('normalizes metric savings', () => {
const auditResult = Audit.generateAuditResult(PassOrFailAudit, {
score: 0,
metricSavings: {LCP: 0.1, FCP: 149},
});
assert.deepStrictEqual(auditResult.metricSavings, {LCP: 0, FCP: 150});
});
adamraine marked this conversation as resolved.
Show resolved Hide resolved

it('chooses the title if score is passing', () => {
const auditResult = Audit.generateAuditResult(PassOrFailAudit, {score: 1});
assert.strictEqual(auditResult.score, 1);
Expand Down
24 changes: 12 additions & 12 deletions core/test/fixtures/user-flows/reports/sample-flow-result.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@
"numericUnit": "millisecond",
"displayValue": "1.0 s",
"metricSavings": {
"TBT": 143
"TBT": 150
},
"details": {
"type": "table",
Expand Down Expand Up @@ -594,7 +594,7 @@
"numericUnit": "millisecond",
"displayValue": "0.3 s",
"metricSavings": {
"TBT": 142.50196940171634
"TBT": 150
},
"details": {
"type": "table",
Expand Down Expand Up @@ -1744,7 +1744,7 @@
"scoreDisplayMode": "informative",
"displayValue": "5 elements found",
"metricSavings": {
"CLS": 0.002631263732910156
"CLS": 0.003
},
"details": {
"type": "table",
Expand Down Expand Up @@ -1869,7 +1869,7 @@
"scoreDisplayMode": "informative",
"displayValue": "3 long tasks found",
"metricSavings": {
"TBT": 143
"TBT": 150
},
"details": {
"type": "table",
Expand Down Expand Up @@ -8996,7 +8996,7 @@
"numericUnit": "millisecond",
"displayValue": "0.9 s",
"metricSavings": {
"TBT": 122.83299999999986
"TBT": 100
},
"details": {
"type": "table",
Expand Down Expand Up @@ -9066,7 +9066,7 @@
"numericUnit": "millisecond",
"displayValue": "0.4 s",
"metricSavings": {
"TBT": 106.76186411159928
"TBT": 100
},
"details": {
"type": "table",
Expand Down Expand Up @@ -9874,7 +9874,7 @@
"scoreDisplayMode": "metricSavings",
"displayValue": "1 element found",
"metricSavings": {
"CLS": 0.13125
"CLS": 0.131
},
"details": {
"type": "table",
Expand Down Expand Up @@ -9923,7 +9923,7 @@
"scoreDisplayMode": "informative",
"displayValue": "1 long task found",
"metricSavings": {
"TBT": 122.83299999999986
"TBT": 100
},
"details": {
"type": "table",
Expand Down Expand Up @@ -18485,7 +18485,7 @@
"numericUnit": "millisecond",
"displayValue": "0.5 s",
"metricSavings": {
"TBT": 13
"TBT": 0
},
"details": {
"type": "table",
Expand Down Expand Up @@ -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.

},
"details": {
"type": "table",
Expand Down Expand Up @@ -19676,7 +19676,7 @@
"scoreDisplayMode": "informative",
"displayValue": "2 long tasks found",
"metricSavings": {
"TBT": 13
"TBT": 0
},
"details": {
"type": "table",
Expand Down Expand Up @@ -20988,7 +20988,7 @@
"warnings": [],
"metricSavings": {
"FCP": 0,
"LCP": 330
"LCP": 350
},
"details": {
"type": "opportunity",
Expand Down
22 changes: 11 additions & 11 deletions core/test/results/sample_v2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,8 +390,8 @@
"numericUnit": "millisecond",
"displayValue": "Root document took 570 ms",
"metricSavings": {
"FCP": 468.46799999999996,
"LCP": 468.46799999999996
"FCP": 450,
"LCP": 450
},
"details": {
"type": "opportunity",
Expand Down Expand Up @@ -1106,7 +1106,7 @@
"numericUnit": "millisecond",
"displayValue": "2.2 s",
"metricSavings": {
"TBT": 1220.691999999999
"TBT": 1200
},
"details": {
"type": "table",
Expand Down Expand Up @@ -1171,7 +1171,7 @@
"numericUnit": "millisecond",
"displayValue": "1.3 s",
"metricSavings": {
"TBT": 1209.4020894768312
"TBT": 1200
},
"details": {
"type": "table",
Expand Down Expand Up @@ -2349,7 +2349,7 @@
"scoreDisplayMode": "informative",
"displayValue": "Third-party code blocked the main thread for 0 ms",
"metricSavings": {
"TBT": 23.424291876693196
"TBT": 0
},
"details": {
"type": "table",
Expand Down Expand Up @@ -2430,7 +2430,7 @@
"scoreDisplayMode": "metricSavings",
"displayValue": "13,320 ms",
"metricSavings": {
"LCP": 10819.961
"LCP": 10800
},
"details": {
"type": "list",
Expand Down Expand Up @@ -2530,7 +2530,7 @@
"scoreDisplayMode": "metricSavings",
"displayValue": "5 elements found",
"metricSavings": {
"CLS": 0.13570762803819444
"CLS": 0.136
},
"details": {
"type": "table",
Expand Down Expand Up @@ -2655,7 +2655,7 @@
"scoreDisplayMode": "informative",
"displayValue": "2 long tasks found",
"metricSavings": {
"TBT": 1220.691999999999
"TBT": 1200
},
"details": {
"type": "table",
Expand Down Expand Up @@ -4609,7 +4609,7 @@
"warnings": [],
"metricSavings": {
"FCP": 0,
"LCP": 610
"LCP": 600
},
"details": {
"type": "opportunity",
Expand Down Expand Up @@ -4919,7 +4919,7 @@
"displayValue": "Potential savings of 143 KiB",
"metricSavings": {
"FCP": 150,
"LCP": 1060
"LCP": 1050
},
"details": {
"type": "opportunity",
Expand Down Expand Up @@ -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

"TBT": 0
},
"details": {
"type": "table",
Expand Down
Loading