-
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: expose error stack on errored audits #14491
Conversation
const evaluationError = new Error(`Runtime.evaluate exception: ${message}`); | ||
if (ex.exception?.description && ex.stackTrace) { | ||
// The description contains the stack trace formatted as expected, if present. | ||
evaluationError.stack = ex.exception.description; |
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.
Could we combine the error stacks so the error stack also tells us where in LH code it occured?
Ex: https://github.com/GoogleChrome/lighthouse/blob/main/core/scripts/pptr-run-devtools.js#L87
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.
If we want that, we should just recurse the error.cause
chain and join all the stacks for errorStack
.
EDIT: read the code, don't see what that has to do with stacks but ya adding maybe the first few characters of the expression could be good for context here. We don't have the name/Function at this layer, and the entire thing would be way too long (axe...)
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 that's what @adamraine meant, otherwise you lose the evaluate()
call stack on the LH side. In theory not a huge deal: if you're getting an error this early, you were probably just editing the js and know where the call was coming from, but it can be an issue for evaluate calls originating in many places
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 decided that modifying stack
is probably a bad idea. Just added the extra details to the error message instead, and then was pleasantly surprised to see that v8 will parse the message for additional stack frames and add it to stacks
itself!
Here's what printing evaluationError
looks like now:
Error: Runtime.evaluate exception
Expression: (() => { return (function computeBenchmarkIndex() { /** * The GC-heavy benchmark that creates a stri
---- (elided)
ReferenceError: Prommmise is not defined
at wrapInNativePromise (_lighthouse-eval.js:8:9)
at _lighthouse-eval.js:83:8
at ExecutionContext._evaluateInContext (file:///Users/cjamcl/src/lighthouse/core/gather/driver/execution-context.js:139:31)
at processTicksAndRejections (node:internal/process/task_queues:96:5)
at async ExecutionContext.evaluateAsync (file:///Users/cjamcl/src/lighthouse/core/gather/driver/execution-context.js:173:14)
at async getBenchmarkIndex (file:///Users/cjamcl/src/lighthouse/core/gather/driver/environment.js:54:20)
at async getBaseArtifacts (file:///Users/cjamcl/src/lighthouse/core/gather/base-artifacts.js:21:26)
at async _setup (file:///Users/cjamcl/src/lighthouse/core/gather/navigation-runner.js:56:25)
at async file:///Users/cjamcl/src/lighthouse/core/gather/navigation-runner.js:336:31
at async Function.gather (file:///Users/cjamcl/src/lighthouse/core/runner.js:172:21)
at async navigationGather (file:///Users/cjamcl/src/lighthouse/core/gather/navigation-runner.js:318:21)
at async navigation (file:///Users/cjamcl/src/lighthouse/core/index.js:94:24)
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.
did something change? Here's the error stack I get on hreflang
if I make an error in LinkElements
:
errorMessage: Required LinkElements gatherer encountered an error: getElementsInDocumentz is not defined
errorStack: Error: LighthouseError: ERRORED_REQUIRED_ARTIFACT
at Runner._runAudit (~/lighthouse/core/runner.js:431:25)
at Runner._runAudits (~/lighthouse/core/runner.js:370:40)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async Runner.audit (~/lighthouse/core/runner.js:62:32)
at async runLighthouse (~/lighthouse/cli/run.js:250:8)
at async ~/lighthouse/cli/index.js:10:1
at <anonymous>:1: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.
ah, to get a populated response.exceptionDetails
you actually need invalid JS or the like, not just an error. Adding some random characters, I now get
errorMessage:
Required LinkElements gatherer encountered an error: Runtime.evaluate exception
Expression: (() => { function getNodeDetails(element) { function truncate(string, characterLimit) { const Util = ---- (elided)
Parse error at: 294:6
SyntaxError: Unexpected token '*'
at <anonymous>:1:1
errorStack:
LighthouseError: ERRORED_REQUIRED_ARTIFACT
at Runner._runAudit (~/lighthouse/core/runner.js:431:25)
at Runner._runAudits (~/lighthouse/core/runner.js:370:40)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async Runner.audit (~/lighthouse/core/runner.js:62:32)
at async runLighthouse (~/lighthouse/cli/run.js:250:8)
at async ~/lighthouse/cli/index.js:10:1
at <anonymous>:1:1
errorMessage
seems to have some of the useful new bits, but formatted poorly for an error message.
Breaking in _evaluateInContext
I see the expanded evaluationError
Error: Runtime.evaluate exception
Expression: (() => { function getNodeDetails(element) { function truncate(string, characterLimit) { const Util =
---- (elided)
Parse error at: 294:6
SyntaxError: Unexpected token '*'
at ExecutionContext._evaluateInContext (~/lighthouse/core/gather/driver/execution-context.js:140:31)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async ExecutionContext.evaluateAsync (~/lighthouse/core/gather/driver/execution-context.js:173:14)
at async LinkElements._getArtifact (~/lighthouse/core/gather/gatherers/link-elements.js:183:17)
at async ~/lighthouse/core/gather/runner-helpers.js:101:24
at async collectPhaseArtifacts (~/lighthouse/core/gather/runner-helpers.js:118:5)
at async _computeNavigationResult (~/lighthouse/core/gather/navigation-runner.js:200:5)
at async _navigations (~/lighthouse/core/gather/navigation-runner.js:297:30)
at async ~/lighthouse/core/gather/navigation-runner.js:370:27
at async Runner.gather (~/lighthouse/core/runner.js:218:21) {stack: 'Error: Runtime.evaluate exception
Expression:…~/lighthouse/core/runner.js:218:21)', message: 'Runtime.evaluate exception
Expression: (() =… at: 294:6
SyntaxError: Unexpected token '*''}
Looking at just evaluationError.stack
:
'Error: Runtime.evaluate exception
Expression: (() => { function getNodeDetails(element) { function truncate(string, characterLimit) { const Util =
---- (elided)
Parse error at: 294:6
SyntaxError: Unexpected token '*'
at ExecutionContext._evaluateInContext (~/lighthouse/core/gather/driver/execution-context.js:140:31)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async ExecutionContext.evaluateAsync (~/lighthouse/core/gather/driver/execution-context.js:172:14)
at async LinkElements._getArtifact (~/lighthouse/core/gather/gatherers/link-elements.js:183:17)
at async ~/lighthouse/core/gather/runner-helpers.js:101:24
at async collectPhaseArtifacts (~/lighthouse/core/gather/runner-helpers.js:118:5)
at async _computeNavigationResult (~/lighthouse/core/gather/navigation-runner.js:200:5)
at async _navigations (~/lighthouse/core/gather/navigation-runner.js:297:30)
at async ~/lighthouse/core/gather/navigation-runner.js:370:27
at async Runner.gather (~/lighthouse/core/runner.js:218:21)'
vs the old new Error(`Evaluation exception: ${errorMessage}`)
's stack:
Error: Evaluation exception: SyntaxError: Unexpected token '*'
at ExecutionContext._evaluateInContext (~/lighthouse/core/gather/driver/execution-context.js:140:31)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async ExecutionContext.evaluateAsync (~/lighthouse/core/gather/driver/execution-context.js:172:14)
at async LinkElements._getArtifact (~/lighthouse/core/gather/gatherers/link-elements.js:183:17)
at async ~/lighthouse/core/gather/runner-helpers.js:101:24
at async collectPhaseArtifacts (~/lighthouse/core/gather/runner-helpers.js:118:5)
at async _computeNavigationResult (~/lighthouse/core/gather/navigation-runner.js:200:5)
at async _navigations (~/lighthouse/core/gather/navigation-runner.js:297:30)
at async ~/lighthouse/core/gather/navigation-runner.js:370:27
at async Runner.gather (~/lighthouse/core/runner.js:218:21)
looks exactly the same except the new version has more non-stack stuff at the top
const evaluationError = new Error(`Runtime.evaluate exception: ${message}`); | ||
if (ex.exception?.description && ex.stackTrace) { | ||
// The description contains the stack trace formatted as expected, if present. | ||
evaluationError.stack = ex.exception.description; |
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 that's what @adamraine meant, otherwise you lose the evaluate()
call stack on the LH side. In theory not a huge deal: if you're getting an error this early, you were probably just editing the js and know where the call was coming from, but it can be an issue for evaluate calls originating in many places
core/lib/lh-error.js
Outdated
*/ | ||
constructor(errorDefinition, properties) { | ||
super(errorDefinition.code); | ||
constructor(errorDefinition, properties, cause) { |
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.
copying the Error
options param might be better instead of just having a cause
param, both to match the parent's style and so it's a named argument at call sites
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 - but now need to bump tsc to get ErrorOptions to be unknown instead of Error.
core/scripts/cleanup-LHR-for-diff.js
Outdated
auditResult.description = '**Excluded from diff**'; | ||
} | ||
if (auditResult.errorStack) { | ||
auditResult.errorStack = auditResult.errorStack.replaceAll(baseCallFrameUrl.href, ''); |
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.
It's still pretty of annoying to update sample LHRs for line number changes in any of the files in an error stack. Could do like errorStack.replaceAll(/\([^)]+\)/g, '()')
but maybe we should snip it completely
5a3adcf
to
1210b54
Compare
1210b54
to
aa0c628
Compare
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
aa0c628
to
d041926
Compare
const evaluationError = new Error(`Runtime.evaluate exception: ${message}`); | ||
if (ex.exception?.description && ex.stackTrace) { | ||
// The description contains the stack trace formatted as expected, if present. | ||
evaluationError.stack = ex.exception.description; |
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.
did something change? Here's the error stack I get on hreflang
if I make an error in LinkElements
:
errorMessage: Required LinkElements gatherer encountered an error: getElementsInDocumentz is not defined
errorStack: Error: LighthouseError: ERRORED_REQUIRED_ARTIFACT
at Runner._runAudit (~/lighthouse/core/runner.js:431:25)
at Runner._runAudits (~/lighthouse/core/runner.js:370:40)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async Runner.audit (~/lighthouse/core/runner.js:62:32)
at async runLighthouse (~/lighthouse/cli/run.js:250:8)
at async ~/lighthouse/cli/index.js:10:1
at <anonymous>:1:1
This adds an optional
errorStack
to an error'd audit product.Also included is some better handling for our
.evaluate
function for when there is a syntax error.example: