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

Performance Audit Prioritization Cleanup #15522

Closed
3 tasks done
adamraine opened this issue Oct 6, 2023 · 2 comments · Fixed by #15902
Closed
3 tasks done

Performance Audit Prioritization Cleanup #15522

adamraine opened this issue Oct 6, 2023 · 2 comments · Fixed by #15902
Assignees
Milestone

Comments

@adamraine
Copy link
Member

adamraine commented Oct 6, 2023

There are several breaking changes related to the new performance audit prioritization that we should consider to clean up our JSON results:

  • Create an explicit group to contain all performance diagnostics. All performance diagnostics have no group right now because the group is technically auto selected by the report renderer.
    • Consider renaming "Diagnostics" to "Insights"? Too much backporting/churn to be worth it
  • Deprecate, but keep the overallSavingsMs (as metricSavings exists now)
  • Remove metrics-to-audits.js
  • Probably remove the opportunity details type
@adamraine adamraine added this to the v12.0 milestone Oct 6, 2023
@adamraine adamraine self-assigned this Oct 6, 2023
@adamraine
Copy link
Member Author

It's a big change to remove the old overallSavingsMs and individual item wastedMs when we could easily just keep it. We should at least do a soft deprecation with JSDOC deprecation warnings etc.

Some usages of overallSavingsMs: https://github.com/search?q=overallSavingsMs&type=code

It appears to be useful to have a single JSON ranking for some users. This brings up a lot of the concerns I've had with computing the overall impact on the JSON. I'm willing to consider this for the future though. We could do this through an implicit sort performance audits, or an overallImpact number on each audit (Similar to Brendan's suggestion in #15445)

OpportunityItem includes wastedMs number for each item, we should look into a metric savings for each item.

@paulirish
Copy link
Member

Probably remove the opportunity details type

seems like a bunch of work only to break users. report-wise i guess its not useful, but...

Types wise.. overallWastedX isn't an opportunity-specific thing, so no blocker there. But url is a required OpportunityItem member that's not part of base TableItem. Don't really have a solution there unless we're okay with optional url field.

Lots of words to say.. does dropping this type help us or users out? :)

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

Successfully merging a pull request may close this issue.

2 participants