core(optimized-images): log errors #9782
Conversation
Co-Authored-By: Patrick Hulce <[email protected]>
Co-Authored-By: Patrick Hulce <[email protected]>
connorjclark
requested review from
brendankenny,
patrickhulce and
paulirish
as code owners
| @@ -8,6 +8,7 @@ | |||
| // Example: | |||
| // node lighthouse-core/scripts/compare-timings.js --name my-collection --collect -n 3 --lh-flags='--only-audits=unminified-javascript' --urls https://www.example.com https://www.nyt.com | |||
| // node lighthouse-core/scripts/compare-timings.js --name my-collection --summarize --measure-filter 'loadPage|connect' | |||
| // node lighthouse-core/scripts/compare-timings.js --name base --name pr --compare | |||
There was a problem hiding this comment.
I think you've got some timings PR sneaking in here :)
| @@ -80,7 +80,7 @@ class UsesOptimizedImages extends ByteEfficiencyAudit { | |||
| const warnings = []; | |||
| for (const image of images) { | |||
| if (image.failed) { | |||
| warnings.push(`Unable to decode ${URL.getURLDisplayName(image.url)}`); | |||
| warnings.push(`Unable to decode ${URL.getURLDisplayName(image.url)}: ${image.errMsg}`); | |||
There was a problem hiding this comment.
I believe these messages used to be included but were removed because they typically had nothing to do with the user.
is the goal for us to know why they failed? because we should already be tracking those in Sentry via the gatherer :)
There was a problem hiding this comment.
is the goal for us to know why they failed? because we should already be tracking those in Sentry via the gatherer :)
yeah. but what about when i run LH? I am forced to print my woes in devtools, since I can't access the artifacts (and we don't log these errors anywhere).
but it's also nice to be able to see from a user report (which most often comes in the form of a PNG) what the error was
There was a problem hiding this comment.
but what about when i run LH?
you're not really the target user/reason we would hide the error :) I mean other than you, who is going to be able to do anything with this additional string?
but it's also nice to be able to see from a user report (which most often comes in the form of a PNG) what the error was
true! but when has this last happened where we've gotten a screenshot with warnings filed we needed to investigate? :) we already know what errors can happen here and we don't really do much about them (existing set seems just intentionally ignored target, response was evicted from memory, or wasn't a real image).
I think my overall rationale here is the same reason we designed the error drawer, outside of the base warning that something was up, detailed error messages are generally useless information to the user and exist purely for facilitating bug reports. In this case, we don't seem to learn anything new and until the drawer is implemented I'm not super excited to increase the verbosity of that messaging.
We've already got other walls of text warnings like you've filed, so it's not like we're doing great in this arena already, but I'd like to avoid heading in the other direction if we can.
That's my spiel, but happy to move this forward too if you're unconvinced and would like to hunt these down anyway :)
There was a problem hiding this comment.
this all sounds good. I'll just add a log.error in the gathering when it happens.
connorjclark
changed the title
| @@ -124,6 +125,8 @@ class OptimizedImages extends Gatherer { | |||
| const image = {failed: false, ...stats, ...record}; | |||
| results.push(image); | |||
| } catch (err) { | |||
| log.warn('optimized-images', err.message); | |||
There was a problem hiding this comment.
log.verbose seems like it might be better here. These happen all the time and I think it's going to get really old seeing them :)
Maybe we need to invest more in devtools debugging? It seems like this wouldn't be an issue if it were easier to get breakpoints in DT workers
There was a problem hiding this comment.
Why would they happen all the time? I only know about the case I mentioned in the OP.
We have error messages here but we don't use them.
Only error message I've seen here has been from requesting a response body from the wrong target.
Not localized because none of these warnings are yet.