core: warn when extensions affected perf #5666
Conversation
|
Oof. This'll hit basically every user. I feel like we should do this only above some threshold of main thread usage (and/or something else) |
|
Alright @paulirish how bout now only when chrome extensions had 100ms of bootup time? |
There was a problem hiding this comment.
LGTM! This will be a really nice warning to have in the report (I have no opinion on the specific threshold, but having one seems like a good idea).
The only shame is to have it only when running the bootup time audit. Maybe add a TODO to consider moving it into some core diagnostic (runner? no idea)? But that would require a lot of work (e.g. bootup breakdowns to computed artifacts), and in the extension and devtools (and CLI if using --only-categories) you'll always get the audit if you're running the perf category, so it should usually be fine
| @@ -84,6 +84,7 @@ class BootupTime extends Audit { | |||
| * @return {Promise<LH.Audit.Product>} | |||
| */ | |||
| static async audit(artifacts, context) { | |||
| const runWarnings = artifacts.LighthouseRunWarnings || []; | |||
There was a problem hiding this comment.
shouldn't this always be defined (since part of the base artifacts provided by gatherrunner, but also because specified in requiredArtifacts)
There was a problem hiding this comment.
yeah was just thinking about older fixtures or whatnot, I'll remove
|
|
||
| if (hadExcessiveChromeExtension) { | ||
| runWarnings.push('Chrome extensions impacted the performance of this page. ' + | ||
| 'Try running Lighthouse in a clean profile to avoid being impacted by extensions.'); |
There was a problem hiding this comment.
"clean Chrome profile", maybe? and should we suggest the CLI or not worth it for the average extension/devtools user?
There was a problem hiding this comment.
done. I think CLI isn't worth it, might just confuse folks :)
|
todo added too! |
| // TODO: consider moving this to core gathering so you don't need to run the audit for warning | ||
| if (hadExcessiveChromeExtension) { | ||
| runWarnings.push('Chrome extensions impacted the performance of this page. ' + | ||
| 'Try running Lighthouse in a clean Chrome profile to avoid being impacted by extensions.'); |
There was a problem hiding this comment.
@kaycebasques wdyt about this wording? "impacted" feels a little awkward to me, but I could roll with it.
I think we should add ", or incognito mode, " to the second sentence though.
There was a problem hiding this comment.
"Impacted" doesn't jump out to me, but since you summoned me:
- Interfered
- Negatively affected
- Distorted
Chrome extensions negatively affected this page's load performance. Try auditing the page in incognito mode or from a clean Chrome profile.
(I think you can drop "to avoid being impacted by extensions"... it's clear enough why you'd use a clean profile or incognito)
There was a problem hiding this comment.
good feedback @kaycebasques I like it! done
|
(hold off on merging). I want to add this right after we tag 3.0.3 |
Related Issues/PRs
closes #5543
closes #4878