core: add `DevtoolsLogError` and `TraceError` artifacts #15311

adamraine · 2023-07-26T21:37:15Z

Before: https://googlechrome.github.io/lighthouse/viewer/?gist=1c712c30d8e871163cadb014df6125c8
After: https://googlechrome.github.io/lighthouse/viewer/?gist=765026ff99e2f51e1e5b82675e26ef46

If there was a page load error in navigation mode, we still hold on to the DT log and trace for debugging purposes. This currently requires us to use the devtoolsLogs/traces compat artifacts because we assign them a different "pass name" for being errors (i.e. pageLoadError-default instead of defaultPass).

This PR will hold onto the DT log and trace in the default artifacts.DevtoolsLog and artifacts.Trace locations. This means that audits which used to throw for a missing DT log or trace could now produce a valid result if we ran them. This PR also throws an error for all audits if it detects a page load error so we are still not running these audits.

It kinda contradicts the goals of #9236 though but using the page load error as the audit error message makes more sense than saying "Traces gatherer did not run" IMO.

Some alternatives include:

[Edit: changed this PR to this alt] Adding artifacts.DevtoolsLogError and artifacts.TraceError to keep erroneous DT logs/traces separate from regular ones in the new artifacts structure
Remove our support for keeping the DT log/trace when there was a page load error
Allow audits to use the DT log/trace normally even if there was a page load error (i.e. remove the change in core/runner.js)

connorjclark · 2023-07-26T22:29:58Z

core/runner.js

@@ -367,6 +367,8 @@ class Runner {

    let auditResult;
    try {
+      if (artifacts.PageLoadError) throw artifacts.PageLoadError;
+


I'm unsure about this change.

My understanding from discussing this earlier was that the most non-breaking / simple change here would be for L380 conditional to include artifacts.PageLoadError, to keep the behavior of "don't run any audits if we can't trust the trace/logs to be good".

"don't run any audits if we can't trust the trace/logs to be good"

That's what this behavior is. We throw an error early if we can't trust the DT log or trace (i.e. there was a page load error).

If we added artifacts.PageLoadError to the conditional on L380 we would always be throwing a missing artifact error for whichever artifact happened to be first in the list. So we are throwing an error in both situations, the difference is what the error message is. I think it makes more sense to surface the page load error rather than say "Missing required artifact devtoolsLogs" especially when the devtools log is there.

The situation is definitely confusing in both situations, so I won't die on this hill. We could also consider one of the alternatives.

for whichever artifact happened to be first in the list

Ah, my actual working suggestion would have to be adding to L376/377, then.

Does throwing the artifact PageLoadError here have desired results? as in, now instead of throwing a MISSING_REQUIRED_ARTIFACT error, we'd get the page load error instead in some cases.

Also, when would this actually occur - do we not throw on a page load error earlier than this function?

Does throwing the artifact PageLoadError here have desired results? as in, now instead of throwing a MISSING_REQUIRED_ARTIFACT error, we'd get the page load error instead in some cases.

Yeah https://googlechrome.github.io/lighthouse/viewer/?gist=765026ff99e2f51e1e5b82675e26ef46

Also, when would this actually occur - do we not throw on a page load error earlier than this function?

We will always surface the page load error before mentioning any missing artifacts now. We still might have a missing artifact error if we did lighthouse -A on some bad artifact data or something.

We don't throw on a page load error but it is listed as a top level warning.

We don't throw on a page load error but it is listed as a top level warning.

Yes, see the discussion at #8865 (comment) linked from #9236

My only concern here is the code we would run in catch for every single audit in this case.

Can we instead pull this up into where Runner._runAudit is called - if PageLoadError is present, we construct the err'd audit results at that level? This would skip us reporting to Sentry or logging a warning for each audit.

This could be a good idea, the sentry logging is completely unnecessary, but FWIW this is unchanged from current behavior, it's just using the PageLoadError instead of a MISSING_REQUIRED_ARTIFACT error per audit.

Agreed it's same behavior as today - let's do in a second PR since this one is big enough already.

Second PR sounds good

brendankenny

This seems reasonable. As I recall the primary reason for keeping the artifacts.traces.pageLoadError-whatever was for debugging with -A in the rare cases where somehow you ended up with a page load error and you didn't know how. Is there any other use of them? Having the devtoolslog/trace under their normal names makes it slightly more likely they could be mistakenly used when there was a pageLoadError, but I don't think we want to get rid of the -A use case.

It kinda contradicts the goals of #9236 though but using the page load error as the audit error message makes more sense than saying "Traces gatherer did not run" IMO.

I think this solution feels well within the intention of what #9236 was fixing (the changed error message was just a side effect, and this is arguably an improvement on those).

core/gather/navigation-runner.js

brendankenny · 2023-07-26T23:23:35Z

core/runner.js

@@ -367,6 +367,8 @@ class Runner {

    let auditResult;
    try {
+      if (artifacts.PageLoadError) throw artifacts.PageLoadError;


Looking at the smoke test as examples, maybe this should throw a simple new LH error rather than repeating the page load error a million times? The metric section in particular could end up pretty gnarly with the longer network errors.

The top-level pageLoadError would be the main explanation, the individual audit errors can simply say they were unable to run due to a page load error or whatever?

Not an outrageous idea, although I think repeating the same error message everywhere has benefits.

I've responded to a few bugs where users ask about the "traces gatherer did not run" error message but completely miss the more detailed top level run warning that explains the problem better. I believe users (myself included sometimes) skip over the warning section and go straight to the giant wall of red text for the failure information.

If we have a generic "Page load failed error" I think we're missing an opportunity to explain the error in the first place the user's attention in drawn to.

If we have a generic "Page load failed error"

The message could be "An error during page load prevented this audit from running. See the error message given at the top of the report" or something.

with the longer network errors.

How long are we talking here? 10x/100x than what I just wrote above?

The message could be "An error during page load prevented this audit from running. See the error message given at the top of the report" or something.

Yeah but from a JSON consumer perspective this makes less sense.

How long are we talking here? 10x/100x than what I just wrote above?

It vary depending on what error details are provided. FWIW the error will be in a tooltip most of the time. If we're really worried about blowing up the metric section though, I would prefer to elide the displayed text rather than choose a more indirect albeit shorter error message.

core/config/constants.js

brendankenny · 2023-07-26T23:54:22Z

core/runner.js

@@ -367,6 +367,8 @@ class Runner {

    let auditResult;
    try {
+      if (artifacts.PageLoadError) throw artifacts.PageLoadError;
+


We don't throw on a page load error but it is listed as a top level warning.

Yes, see the discussion at #8865 (comment) linked from #9236

brendankenny · 2023-07-27T00:10:36Z

Adding artifacts.DevtoolsLogError and artifacts.TraceError to keep erroneous DT logs/traces separate from regular ones in the new artifacts structure

This plus teaching asset-saver to save these (with some kind of error-ful name) seems like a good solution too. Cover the current use cases and ensure no code would accidentally use a trace or devtoolslog without handling the pageLoadError case.

adamraine added 2 commits July 26, 2023 13:24

core: remove error pass id

a9ae6a4

ope

9621822

connorjclark reviewed Jul 26, 2023

View reviewed changes

brendankenny reviewed Jul 27, 2023

View reviewed changes

test

7e394a4

vercel bot deployed to Preview July 27, 2023 03:04 View deployment

new error versions

0b8fa26

vercel bot deployed to Preview July 27, 2023 20:04 View deployment

adamraine changed the title ~~WIP: remove error pass id~~ core: add DevtoolsLogError and TraceError artifacts Jul 31, 2023

adamraine marked this pull request as ready for review July 31, 2023 18:31

adamraine requested a review from a team as a code owner July 31, 2023 18:31

adamraine requested review from brendankenny and removed request for a team July 31, 2023 18:31

Merge branch 'main' into rm-error-trace-name

66f7f5d

vercel bot deployed to Preview July 31, 2023 18:32 View deployment

devtools-bot assigned brendankenny Jul 31, 2023

devtools-bot added the waiting4reviewer label Jul 31, 2023

connorjclark approved these changes Aug 2, 2023

View reviewed changes

adamraine merged commit 67699c5 into main Aug 2, 2023
25 of 26 checks passed

adamraine deleted the rm-error-trace-name branch August 2, 2023 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: add `DevtoolsLogError` and `TraceError` artifacts #15311

core: add `DevtoolsLogError` and `TraceError` artifacts #15311

core: add DevtoolsLogError and TraceError artifacts #15311

core: add DevtoolsLogError and TraceError artifacts #15311

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

core: add `DevtoolsLogError` and `TraceError` artifacts #15311

core: add `DevtoolsLogError` and `TraceError` artifacts #15311