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.

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

Add top-level error property on LHR #5881

Closed
paulirish opened this issue Aug 21, 2018 · 5 comments
Closed

Add top-level error property on LHR #5881

paulirish opened this issue Aug 21, 2018 · 5 comments
Assignees
Labels

Comments

@paulirish
Copy link
Member

paulirish commented Aug 21, 2018

To communicate if there were any errors during the run. We have to decide which errors should be considered for this. At the very least, a boolean that captures if there were any auditErrors.

See PSI's error enum: https://goto.google.com/zhian

Property name is available for bikeshedding. ;) errorState?

any takers?

@brendankenny
Copy link
Member

What kind of error state is it trying to communicate?

  • Numbers may not be trustworthy
  • Something weird happened, you should try rerunning
  • Something happened that rerunning won't fix, re-check how you've configured lighthouse

?

@paulirish
Copy link
Member Author

as mentioned in the thread. i think we have 3 classes of errors.

  1. the page couldnt be loaded correctly (bad TLS, 500 status, etc). aka getPageLoadError returned truthy.
  2. page was loaded but we couldn't collect necessary diagnostics (NO_TRACING_STARTED, NO_NAVSTART).
    • see screenshot. If we're gonna mark ALL perf stuff as 'Error' then we should consider it an audit failure. The two above error codes definitely end up in this state. there may be more.
  3. Individual audits had errors
    • some, but not all

Looks like we're about to have agreement the 1 single audit error shouldn't tank the run (and so we shouldnt report a toplevel failure). But classes 1 and 2 should both tank the run.

Relatedly, we need a catch in runLighthouseForLR() so we dont get unhandled rejections. And I guess we'll have to return something in this errorState to LR?

@brendankenny can you put together a PR for this while I'm out?

@paulirish
Copy link
Member Author

toplevel LHR prop called runtimeError.

These are handled as "fatal" runtimeErrors

  1. defaultPass pageload error = runtimeError
    • 500'd. 400'd. certificate error. network disconnected / offline.
    • FAILED_DOCUMENT_REQUEST, NO_DOCUMENT_REQUEST
  2. tracing result failures: NO_tracing_start, no_navstart, NO_FCP, no_dcl = FATAL
  3. tracing recording failures: TRACING_ALREADY_STARTED, PARSING_problem, read_failed, (timeout_during_trace_retreival)
  4. Screenshot/speedline errors = all fatal. (unless its a pain. no big deal, really)

these are Not fatal runtimeErrors:

  1. no_fmp = not fatal, we need to tell them the page was too busy.
  2. TTI calculation failures = not fatal, does not apply to top-level error enum, because we want to tell them that the page is too busy for us.
  3. REQUEST_CONTENT_TIMEOUT = not fatal
  4. chrome-launcher/couldn't find a chrome = deal with later.

todo: figure out plan with caught exceptions.

catch in runner/index and always return LHR or catch outside (cli/ext/background) and construct an LHR there?

@paulirish
Copy link
Member Author

paulirish commented Sep 18, 2018

Remaining:

  • 500s/pageLoadError case isn't reported as runtimeError
  • paul confirming the shallow LHR is parseable in LR

@paulirish
Copy link
Member Author

Fixed. Some small followups done in #6071

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants