-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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.
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: centralize error strings #4280
Conversation
Maybe I missed this, but are we not adding in the error codes? |
@@ -28,7 +28,7 @@ describe('FirstInteractive computed artifact:', () => { | |||
return computedArtifacts.requestFirstInteractive({traceEvents: tooShortTrace}).then(() => { | |||
assert.ok(false, 'should have thrown for short trace'); | |||
}).catch(err => { | |||
assert.equal(err.message, 'trace not at least 5 seconds longer than FMP'); | |||
assert.ok(err.message, 'FMP_TOO_LATE_FOR_FCPUI'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was expecting FMP_TOO_LATE_FOR_FCPUI
to be the err.code, and the long string as the .message. What am i missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe some wires got crossed. I expected the long string to just be used for display and not the one used in the stack/Sentry/CLI/etc. The codes are fairly self explanatory for us and the display string gets used in the report.
Error: FMP_TOO_LATE_FOR_FCPUI
at repl:1:5
at ContextifyScript.Script.runInThisContext (vm.js:44:33)
at REPLServer.defaultEval (repl.js:239:29)
at bound (domain.js:301:14)
seems better in sentry than
Error: Your page took too long to load. Please follow the opportunities in the report to reduce your page load time, and then try re-running Lighthouse.
at repl:1:5
at ContextifyScript.Script.runInThisContext (vm.js:44:33)
at REPLServer.defaultEval (repl.js:239:29)
at bound (domain.js:301:14)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes sense. Agree that the friendlyMessage is too wordy to be what we see in stack traces. Hmmm...
How about a basic LHError: ${err.code}
? Just prefixing the string with our custom error type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I like it,
LHError: FMP_TOO_LATE_FOR_FCPUI
at repl:1:5
at ContextifyScript.Script.runInThisContext (vm.js:44:33)
at REPLServer.defaultEval (repl.js:239:29)
at bound (domain.js:301:14)
right?
or are you suggesting
Error: LHError: FMP_TOO_LATE_FOR_FCPUI
at repl:1:5
at ContextifyScript.Script.runInThisContext (vm.js:44:33)
at REPLServer.defaultEval (repl.js:239:29)
at bound (domain.js:301:14)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1st one: LHError: FMP_TOO_LATE_FOR_FCPUI
.
A la "TypeError" etc.
lighthouse-core/runner.js
Outdated
@@ -254,7 +254,8 @@ class Runner { | |||
|
|||
Sentry.captureException(err, {tags: {audit: audit.meta.name}, level: 'error'}); | |||
// Non-fatal error become error audit result. | |||
return Audit.generateErrorAuditResult(audit, 'Audit error: ' + err.message); | |||
const debugString = err.friendlyMessage || `Audit error: ${err.message}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add the err.code at the end of the debugString so we can differentiate the various errors with identical messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, fine with me
@vinamratasingal the codes are there, they are currently the way each audit accesses the error and the name that will show up in Sentry and (now after Paul's feedback) will be displayed in parens after the friendly message |
Cool, then SGTM from me. Once Paul LGTM'es I think we can #shipit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feedback!
I have a few questions around the gather-runner failed-to-load errors..
The rest of the feedback is fairly ordinary.
In addition to the notes below, we had CRI_TIMEOUT marked in our spreadsheet and that's omitted from this PR:
lighthouse/lighthouse-core/gather/connections/cri.js
Lines 118 to 120 in 2182513
const err = new Error('Timeout waiting for initial Debugger Protocol connection.'); | |
err.code = 'CRI_TIMEOUT'; | |
log.error('CriConnection', err.message); |
Aside from that, I've reviewed the spreadsheet and made sure it's all covered by this PR.
@@ -28,7 +28,7 @@ describe('FirstInteractive computed artifact:', () => { | |||
return computedArtifacts.requestFirstInteractive({traceEvents: tooShortTrace}).then(() => { | |||
assert.ok(false, 'should have thrown for short trace'); | |||
}).catch(err => { | |||
assert.equal(err.message, 'trace not at least 5 seconds longer than FMP'); | |||
assert.ok(err.message, 'FMP_TOO_LATE_FOR_FCPUI'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, separate note on this line.. seems like it should still be using equal()
;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha oops, done :)
log.error('GatherRunner', errorReason, url); | ||
const msg = 'Your page failed to load. Verify that the URL is valid and re-run Lighthouse.'; | ||
const error = new Error(msg); | ||
error.reason = errorReason; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where does this reason property go? can we just turn these two cases into two diff LHError codes?
back in the spreadsheet we agreed on two different codes for this. How about NO_DOC_REQUEST
and FAILED_DOC_REQUEST
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
if (errorReason) { | ||
log.error('GatherRunner', errorReason, url); | ||
const msg = 'Your page failed to load. Verify that the URL is valid and re-run Lighthouse.'; | ||
const error = new Error(msg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this use an LHError?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
lighthouse-core/runner.js
Outdated
@@ -254,7 +254,12 @@ class Runner { | |||
|
|||
Sentry.captureException(err, {tags: {audit: audit.meta.name}, level: 'error'}); | |||
// Non-fatal error become error audit result. | |||
return Audit.generateErrorAuditResult(audit, 'Audit error: ' + err.message); | |||
let debugString = `Audit error: ${err.message}`; | |||
if (err.friendlyMessage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nite: let's do a ternary for this const debugString
assignment instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
const error = new Error(`Unable to load page: ${errorMessage}`); | ||
if (errorReason) { | ||
log.error('GatherRunner', errorReason, url); | ||
const msg = 'Your page failed to load. Verify that the URL is valid and re-run Lighthouse.'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the new text i think we need to change this guy now:
lighthouse/lighthouse-core/lib/sentry.js
Line 24 in 550b0c4
{pattern: /Unable to load/, rate: 0.1}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
lighthouse-core/lib/errors.js
Outdated
constructor(errorDefinition) { | ||
super(errorDefinition.code); | ||
Error.captureStackTrace(this, LighthouseError); | ||
this.friendlyMessage = errorDefinition.message; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we can show your provided message in the CLI instead of
lighthouse/lighthouse-cli/run.js
Line 85 in b433a2a
console.error('Unable to load the page. Please verify the url you are trying to review.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, cleaned that area up a bit too
lighthouse-core/lib/errors.js
Outdated
|
||
class LighthouseError extends Error { | ||
constructor(errorDefinition) { | ||
super(errorDefinition.code); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels like we should also set this.code
to be the code. that clean string should will persist all the way to CLI and devtools for those comparisons.
We'll have to update the error handling in devtools: https://github.com/ChromeDevTools/devtools-frontend/blob/28cf1d1a910a19f48c9a5d9d9374006f24179010/front_end/audits2/Audits2Panel.js#L708-L715 Can you make a note of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah sg, done
lighthouse-core/lib/errors.js
Outdated
const errors = { | ||
NO_SPEEDLINE_FRAMES: {message: strings.didntCollectScreenshots}, | ||
SPEEDINDEX_OF_ZERO: {message: strings.didntCollectScreenshots}, | ||
NO_SCREENSHOTS: {message: strings.didntCollectScreenshots}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to note, this one isn't used yet. we created it for https://github.com/paulirish/speedline/blob/53b51d283fcbed69710eda58e8c3539f0fb9134d/src/frame.js#L105
do you want to catch the provided error and rethrow as LHError?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
13b52ce
to
fef29ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, though i had one small suggestion.
nice stuff. :D
lighthouse-core/lib/errors.js
Outdated
* @return {!Error|LighthouseError} | ||
*/ | ||
static fromProtocolMessage(method, protocolError) { | ||
const matchedErrorDefinition = protocolErrors.find(e => e.pattern.test(protocolError.message)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels like doing this find() and iterating through the overall errors
object (on L89) should be together.
this seems fine:
// extract all errors with a regex pattern to match against. findProtocolErrorDfn() then can find the matching one
const protocolErrors = Object.keys(errors).filter(k => errors[k].pattern).map(k => errors[k]);
const findProtocolErrorDfn = pattern => protocolErrors.find(e => e.pattern.test(protocolError.message));
...
const matchedErrorDefinition = findProtocolErrorDfn(protocolError.message);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sg, I also errors ERRORS to make it a bit more clear it's not just a local variable, wdyt? too ugly?
closes #3598
pulled the strings from the spreadsheet with some slight consistency modifications