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.

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

core: add DevtoolsLogError and TraceError artifacts #15311

Merged
merged 5 commits into from
Aug 2, 2023
Merged

core: add DevtoolsLogError and TraceError artifacts #15311

Show file tree
Hide file tree
Changes from all commits
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
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},
},
DevtoolsLogError: NONEMPTY_ARRAY,
TraceError: {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},
},
DevtoolsLogError: NONEMPTY_ARRAY,
TraceError: {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',
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
loadFailureMode: 'fatal',
disableThrottling: false,
disableStorageReset: false,
Expand Down
10 changes: 8 additions & 2 deletions core/gather/navigation-runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,14 @@ 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};
if (debugData.devtoolsLog) {
artifacts.DevtoolsLogError = debugData.devtoolsLog;
artifacts.devtoolsLogs = {[pageLoadErrorId]: debugData.devtoolsLog};
}
if (debugData.trace) {
artifacts.TraceError = debugData.trace;
artifacts.traces = {[pageLoadErrorId]: debugData.trace};
}

return {
pageLoadError,
Expand Down
6 changes: 6 additions & 0 deletions core/lib/asset-saver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ function loadArtifacts(basePath) {
if (passName === 'defaultPass') {
artifacts.DevtoolsLog = devtoolsLog;
}
if (passName === 'pageLoadError-defaultPass') {
artifacts.DevtoolsLogError = devtoolsLog;
}
});

// load traces
Expand All @@ -74,6 +77,9 @@ function loadArtifacts(basePath) {
if (passName === 'defaultPass') {
artifacts.Trace = artifacts.traces[passName];
}
if (passName === 'pageLoadError-defaultPass') {
artifacts.TraceError = artifacts.traces[passName];
}
});

if (Array.isArray(artifacts.Timing)) {
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
Member

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
Member 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

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
Member Author

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

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
Member Author

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
Member 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
Member

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

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
Member

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

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
Member Author

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
11 changes: 6 additions & 5 deletions core/test/config/config-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,9 +253,10 @@ describe('Config', () => {

expect(resolvedConfig).toMatchObject({
artifacts: [{id: 'Accessibility', gatherer: {path: 'accessibility'}}],
navigations: [
{id: 'default', artifacts: [{id: 'Accessibility', gatherer: {path: 'accessibility'}}]},
],
navigations: [{
id: 'defaultPass',
artifacts: [{id: 'Accessibility', gatherer: {path: 'accessibility'}}],
}],
});
});

Expand All @@ -269,7 +270,7 @@ describe('Config', () => {
expect(resolvedConfig).toMatchObject({
navigations: [
{
id: 'default',
id: 'defaultPass',
blankPage: 'about:blank',
artifacts: [{id: 'Accessibility', gatherer: {path: 'accessibility'}}],
loadFailureMode: 'fatal',
Expand Down Expand Up @@ -366,7 +367,7 @@ describe('Config', () => {
{id: 'Accessibility'},
],
navigations: [
{id: 'default', artifacts: [{id: 'Accessibility'}]},
{id: 'defaultPass', artifacts: [{id: 'Accessibility'}]},
],
});
});
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: []}},
DevtoolsLogError: expect.any(Array),
TraceError: {traceEvents: []},
devtoolsLogs: {'pageLoadError-defaultPass': expect.any(Array)},
traces: {'pageLoadError-defaultPass': {traceEvents: []}},
});
});

Expand Down
4 changes: 4 additions & 0 deletions types/artifacts.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ export interface GathererArtifacts extends PublicGathererArtifacts {
CSSUsage: {rules: Crdp.CSS.RuleUsage[], stylesheets: Artifacts.CSSStyleSheetInfo[]};
/** The primary log of devtools protocol activity. */
DevtoolsLog: DevtoolsLog;
/** The log of devtools protocol activity if there was a page load error and Chrome navigated to a `chrome-error://` page. */
DevtoolsLogError: DevtoolsLog;
/** Information on the document's doctype(or null if not present), specifically the name, publicId, and systemId.
All properties default to an empty string if not present */
Doctype: Artifacts.Doctype | null;
Expand Down Expand Up @@ -152,6 +154,8 @@ export interface GathererArtifacts extends PublicGathererArtifacts {
TapTargets: Artifacts.TapTarget[];
/** The primary trace taken over the entire run. */
Trace: Trace;
/** The trace if there was a page load error and Chrome navigated to a `chrome-error://` page. */
TraceError: Trace;
/** Elements associated with metrics (ie: Largest Contentful Paint element). */
TraceElements: Artifacts.TraceElement[];
/** Parsed version of the page's Web App Manifest, or null if none found. */
Expand Down