Skip to content

core: add DevtoolsLogError and TraceError artifacts #15311

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.

Sign up for GitHub

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

Merged
adamraine merged 5 commits into from
Aug 2, 2023
Merged

core: add DevtoolsLogError and TraceError artifacts #15311

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a9ae6a4
core: remove error pass id
adamraine Jul 26, 2023
9621822
ope
adamraine Jul 26, 2023
7e394a4
test
adamraine Jul 27, 2023
0b8fa26
new error versions
adamraine Jul 27, 2023
66f7f5d
Merge branch 'main' into rm-error-trace-name
adamraine Jul 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
core: remove error pass id
  • Loading branch information
@adamraine
adamraine committed Jul 26, 2023
commit a9ae6a40fd10c08a2dfaca40a3374a879ca03eaf
10 changes: 3 additions & 7 deletions cli/test/smokehouse/test-definitions/errors-expired-ssl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,14 @@ const expectations = {
audits: {
'first-contentful-paint': {
scoreDisplayMode: 'error',
errorMessage: 'Required traces gatherer did not run.',
errorMessage: 'The URL you have provided does not have a valid security certificate. net::ERR_CERT_DATE_INVALID',
},
},
},
artifacts: {
PageLoadError: {code: 'INSECURE_DOCUMENT_REQUEST'},
devtoolsLogs: {
'pageLoadError-default': {...NONEMPTY_ARRAY},
},
traces: {
'pageLoadError-default': {traceEvents: NONEMPTY_ARRAY},
},
DevtoolsLog: NONEMPTY_ARRAY,
Trace: {traceEvents: NONEMPTY_ARRAY},
},
};

Expand Down
8 changes: 2 additions & 6 deletions cli/test/smokehouse/test-definitions/errors-iframe-expired-ssl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,8 @@ const expectations = {
},
},
artifacts: {
devtoolsLogs: {
defaultPass: NONEMPTY_ARRAY,
},
traces: {
defaultPass: {traceEvents: NONEMPTY_ARRAY},
},
DevtoolsLog: NONEMPTY_ARRAY,
Trace: {traceEvents: NONEMPTY_ARRAY},
},
};

Expand Down
10 changes: 3 additions & 7 deletions cli/test/smokehouse/test-definitions/errors-infinite-loop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,14 @@ const expectations = {
audits: {
'first-contentful-paint': {
scoreDisplayMode: 'error',
errorMessage: 'Required traces gatherer did not run.',
errorMessage: 'Lighthouse was unable to reliably load the URL you requested because the page stopped responding.',
},
},
},
artifacts: {
PageLoadError: {code: 'PAGE_HUNG'},
devtoolsLogs: {
'pageLoadError-default': {...NONEMPTY_ARRAY},
},
traces: {
'pageLoadError-default': {traceEvents: NONEMPTY_ARRAY},
},
DevtoolsLog: NONEMPTY_ARRAY,
Trace: {traceEvents: NONEMPTY_ARRAY},
},
};

Expand Down
2 changes: 1 addition & 1 deletion core/config/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ const defaultSettings = {

/** @type {Required<LH.Config.NavigationJson>} */
const defaultNavigationConfig = {
id: 'default',
id: 'defaultPass',
loadFailureMode: 'fatal',
disableThrottling: false,
disableStorageReset: false,
Expand Down
8 changes: 4 additions & 4 deletions core/config/default-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,7 +593,7 @@ const defaultConfig = {
supportedModes: ['navigation', 'timespan', 'snapshot'],
auditRefs: [
// Trust & Safety
{id: 'is-on-https', weight: 1, group: 'best-practices-trust-safety'},
{id: 'is-on-https', weight: 5, group: 'best-practices-trust-safety'},
{id: 'geolocation-on-start', weight: 1, group: 'best-practices-trust-safety'},
{id: 'notification-on-start', weight: 1, group: 'best-practices-trust-safety'},
{id: 'csp-xss', weight: 0, group: 'best-practices-trust-safety'},
Expand All @@ -608,10 +608,10 @@ const defaultConfig = {
// General Group
{id: 'no-unload-listeners', weight: 1, group: 'best-practices-general'},
{id: 'js-libraries', weight: 0, group: 'best-practices-general'},
{id: 'deprecations', weight: 1, group: 'best-practices-general'},
{id: 'errors-in-console', weight: 1, group: 'best-practices-general'},
{id: 'deprecations', weight: 5, group: 'best-practices-general'},
{id: 'errors-in-console', weight: 3, group: 'best-practices-general'},
{id: 'valid-source-maps', weight: 0, group: 'best-practices-general'},
{id: 'inspector-issues', weight: 1, group: 'best-practices-general'},
{id: 'inspector-issues', weight: 3, group: 'best-practices-general'},
],
},
'seo': {
Expand Down
12 changes: 9 additions & 3 deletions core/gather/navigation-runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,9 +187,15 @@ async function _computeNavigationResult(

/** @type {Partial<LH.GathererArtifacts>} */
const artifacts = {};
const pageLoadErrorId = `pageLoadError-${navigationContext.navigation.id}`;
if (debugData.devtoolsLog) artifacts.devtoolsLogs = {[pageLoadErrorId]: debugData.devtoolsLog};
if (debugData.trace) artifacts.traces = {[pageLoadErrorId]: debugData.trace};
const navigationId = navigationContext.navigation.id;
if (debugData.devtoolsLog) {
artifacts.DevtoolsLog = debugData.devtoolsLog;
artifacts.devtoolsLogs = {[navigationId]: debugData.devtoolsLog};
}
if (debugData.trace) {
artifacts.Trace = debugData.trace;
artifacts.traces = {[navigationId]: debugData.trace};
}

return {
pageLoadError,
Expand Down
2 changes: 2 additions & 0 deletions core/runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,8 @@ class Runner {

let auditResult;
try {
if (artifacts.PageLoadError) throw artifacts.PageLoadError;
Copy link
Contributor

@brendankenny brendankenny Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@adamraine adamraine Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@connorjclark connorjclark Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@adamraine adamraine Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Copy link
Collaborator

@connorjclark connorjclark Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Contributor Author

@adamraine adamraine Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

Copy link
Collaborator

@connorjclark connorjclark Jul 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@adamraine adamraine Jul 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@brendankenny brendankenny Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@connorjclark connorjclark Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@brendankenny brendankenny Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@connorjclark connorjclark Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@adamraine adamraine Jul 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second PR sounds good

// Return an early error if an artifact required for the audit is missing or an error.
for (const artifactName of audit.meta.requiredArtifacts) {
const noArtifact = artifacts[artifactName] === undefined;
Expand Down
6 changes: 4 additions & 2 deletions core/test/gather/navigation-runner-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,8 +472,10 @@ describe('NavigationRunner', () => {
const {artifacts, pageLoadError} = await run(navigation);
expect(pageLoadError).toBeInstanceOf(LighthouseError);
expect(artifacts).toEqual({
devtoolsLogs: {'pageLoadError-default': expect.any(Array)},
traces: {'pageLoadError-default': {traceEvents: []}},
DevtoolsLog: expect.any(Array),
Trace: {traceEvents: []},
devtoolsLogs: {defaultPass: expect.any(Array)},
traces: {defaultPass: {traceEvents: []}},
});
});

Expand Down