report(flow): top opportunities #13082

adamraine · 2021-09-17T20:58:09Z

Mostly interested in any thoughts on computeTopOpportunities. Once we agree on the implementation, I will start working on the visuals for the top opportunities.

I'm imagining the final structure looks something like:

Performance --------------------------------------- ▼

Images do not have explicit width and height
	Navigation report 1 -> #index=0&anchor=performance
	Snapshot report 1 -> #index=1&anchor=performance
	Navigation report 2 -> #index=2&anchor=performance

Serve images in next-gen formats
	Navigation report 1 -> #index=0&anchor=performance
	Navigation report 2 -> #index=2&anchor=performance

Accessibility ------------------------------------- ◀
Best Practices ------------------------------------ ◀
SEO ----------------------------------------------- ◀

There are some other minor visual updates to the summary page (pretty much everything outside opportunities.tsx), I'm happy to split those into a separate PR if it will make this review easier.

patrickhulce

Awesome start here 🌮

I'm happy to split those into a separate PR if it will make this review easier.

There's enough going on just in opportunities I think that might be worth it to land the summary changes quicker 👍

patrickhulce · 2021-09-20T14:18:03Z

flow-report/src/summary/opportunities.tsx

+import {Util} from '../../../report/renderer/util';
+import {useFlowResult} from '../util';
+
+const MAX_TOP_OPPORTUNITIES = 5;


WDYT about calling this something other than "opportunities" since that term already has a specific meaning in the performance category? (at least for now).

Highest Impact Audits (I think this is what we tested with users)

Top Audits

Top Improvements

Top Issues

I'm also thinking we shift the intent here and tone down our promise on them being "Top" if we don't have a lot of confidence yet that we're actually surfacing the highest impact things at first. Examples...

Common Issues

Issues At-a-Glance

Quick Fixes

Highest Impact Audits (I think this is what we tested with users)
Common Issues

These two are my top picks. "Highest Impact Audits" because we tested it with users, "Common Issues" because it best describes the current algo. If I had to pick one it would be "Common Issues" because it's shorted 🙃

I'd prefer common issues as well as I think it's more accurate of our early impl here.

patrickhulce · 2021-09-20T14:59:16Z

flow-report/src/summary/opportunities.tsx

+
+const MAX_TOP_OPPORTUNITIES = 5;
+
+function computeTopOpportunities(reportResults: LH.ReportResult[], categoryId: string) {


I think we have a few possible goals here:

Surface the absolute most important things to fix across the entire flow

Surface issues that are actionable and/or easy to fix

Surface issues that are very common/high ROI

My initial take is that the first is rather difficult to accomplish for us, and possibly not actionable for the developer either (i.e. if you got a report that just listed the "Top Audits" as the 6 metrics, that wouldn't be very interesting). I'm inclined to favor the "very common"/"high ROI" approach for non-perf categories and the "actionable"+"impact" approach for perf category. What this means in practice for a proposed algorithm...

Non-perf

Group all failures across reports by audit ID

Map each group to compute the sum of number of details items

Return the top N audits when sorted by # of total details items

Perf

Group all opportunities across reports by audit ID

Map each group to compute the sum of overallSavingsMs.

Return the top N audits when sorted by # of total ms savings.

If no items are returned, fallback to non-perf algorithm.

It looks like what you have here is pretty close to non-perf algo, just using the # of reports it appears in rather than number of details items, and using weight as a tiebreaker. Is that right? WDYT about the above?

It looks like what you have here is pretty close to non-perf algo, just using the # of reports it appears in rather than number of details items, and using weight as a tiebreaker. Is that right? WDYT about the above?

Yup that's the algo

if you got a report that just listed the "Top Audits" as the 6 metrics, that wouldn't be very interesting

WDYT about filtering out any audits under group "metrics"?

Non-perf

Group all failures across reports by audit ID
Map each group to compute the sum of number of details items
Return the top N audits when sorted by # of total details items

I'm worried about using the details items because audits like viewport don't include any details items. I understand that using # of relevant reports isn't perfect either, but it will be easier to rank all the audits including ones without items. Maybe we could sort by # reports, then # details items, then weight?

Perf

Group all opportunities across reports by audit ID
Map each group to compute the sum of overallSavingsMs.
Return the top N audits when sorted by # of total ms savings.
If no items are returned, fallback to non-perf algorithm.

Ranking first by savings SGTM, but I would still want to include some diagnostics with 0 savings if there is available space.

WDYT about filtering out any audits under group "metrics"?

SGTM

I'm worried about using the details items because audits like viewport don't include any details items. I understand that using # of relevant reports isn't perfect either, but it will be easier to rank all the audits including ones without items. Maybe we could sort by # reports, then # details items, then weight?

WFM

Ranking first by savings SGTM, but I would still want to include some diagnostics with 0 savings if there is available space.

Yes agreed.

To solve multiple of these concerns, what about # of reports weighted by score? i.e. we wouldn't want to surface a yellow opportunity above a red diagnostic.

To solve multiple of these concerns, what about # of reports weighted by score?

SGTM

adamraine · 2021-09-20T16:03:03Z

There's enough going on just in opportunities I think that might be worth it to land the summary changes quicker 👍

Aight gonna split these

adamraine · 2021-09-20T20:36:34Z

flow-report/src/summary/highest-impact.tsx

+      };
+      if (!Util.showAsPassed(auditRef.result)) {
+        reportListForAudit.reports.push(i);
+        reportListForAudit.remainingScore += (1 - Number(auditRef.result.score)) * auditRef.weight;


This is a slightly than #13082 (comment). I figured auditRef.weight makes more sense as a weight than a tiebreaker. # of reports is now the tiebreaker. WDYT @patrickhulce?

I like the concept, I think we'll also want a version of this for perf though where everything has weight 0 but we still want to consider the score.

I'm thinking...

interface AuditEntry { occurrences: Array<{ref: AuditResultRef, reportIndex: number}> totalCategoryScoreHeadroom: number // this one is weighted by `.weight` totalAuditScoreHeadroom: number // this one is not totalOverallSavingsMs?: number }

patrickhulce · 2021-09-20T20:59:35Z

flow-report/src/summary/highest-impact.tsx

+    .filter(audit =>
+      audit.reports.length &&
+      audit.ref.group !== 'metrics' &&
+      audit.ref.result.scoreDisplayMode !== 'informative' &&


seems like we should skip these per ref above rather than based on a single ref in the report?

patrickhulce · 2021-09-20T21:01:24Z

flow-report/src/summary/highest-impact.tsx

+      };
+      if (!Util.showAsPassed(auditRef.result)) {
+        reportListForAudit.reports.push(i);
+        reportListForAudit.remainingScore += (1 - Number(auditRef.result.score)) * auditRef.weight;


I like the concept, I think we'll also want a version of this for perf though where everything has weight 0 but we still want to consider the score.

I'm thinking...

interface AuditEntry { occurrences: Array<{ref: AuditResultRef, reportIndex: number}> totalCategoryScoreHeadroom: number // this one is weighted by `.weight` totalAuditScoreHeadroom: number // this one is not totalOverallSavingsMs?: number }

adamraine · 2021-09-20T22:03:57Z

Punting until we can understand real use cases better

adamraine added 8 commits September 15, 2021 12:15

summary style

9dc52d6

ope

f2b14b6

maybe

a2eae29

Merge branch 'master' into flow-highest-impact

f50f406

rm audit

49c14a0

layout

eefe3df

section header

a217a42

compute top opportunities

d1476ec

adamraine requested a review from patrickhulce September 17, 2021 20:58

google-cla bot added the cla: yes label Sep 17, 2021

devtools-bot assigned patrickhulce Sep 17, 2021

devtools-bot added the waiting4reviewer label Sep 17, 2021

patrickhulce reviewed Sep 20, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Sep 20, 2021

adamraine mentioned this pull request Sep 20, 2021

report(flow): summary sections #13086

Merged

adamraine changed the base branch from master to fr-summary-sections September 20, 2021 16:08

adamraine added 3 commits September 20, 2021 15:08

ignore metrics

3e42ec6

remaining score

78eee46

Merge branch 'master' into flow-highest-impact

373a6e3

Base automatically changed from fr-summary-sections to master September 20, 2021 20:16

adamraine added 2 commits September 20, 2021 16:18

Merge branch 'master' into flow-highest-impact

d2c80f4

rn

9bca01c

vercel bot deployed to Preview September 20, 2021 20:33 View deployment

adamraine commented Sep 20, 2021

View reviewed changes

adamraine added waiting4reviewer and removed waiting4committer labels Sep 20, 2021

patrickhulce reviewed Sep 20, 2021

View reviewed changes

adamraine closed this Sep 20, 2021

adamraine mentioned this pull request Sep 20, 2021

Flow Support (Fraggle Rock) #11313

Closed

adamraine mentioned this pull request Jul 11, 2023

Flow Report UI Improvements #15254

Open

6 tasks


		const MAX_TOP_OPPORTUNITIES = 5;

		function computeTopOpportunities(reportResults: LH.ReportResult[], categoryId: string) {

report(flow): top opportunities #13082

report(flow): top opportunities #13082

Uh oh!

Conversation

adamraine commented Sep 17, 2021

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamraine commented Sep 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamraine commented Sep 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants