Improve error handling #6502

medikoo · 2019-08-05T13:27:04Z

Do not pass handled errors via unhandled rejections channel - it's a logic error, that's also indirect cause of Exit status for deploy #6441
Use default error logging for uncaught exceptions, but ensure process exits immediately after report is made (improves UX)
Propagate unhandled rejections as uncaught excpeptions but only if no other unhandled rejection handlers were registered (it's to resemble eventual future Node.js behavior, and to allow users to opt out from this handling - related issue: Server crashes on unhandled promise rejection dherault/serverless-offline#255)
Use default error logging for non error exceptions (improves UX)
(Note: here to distinguish error instances I decided to rely on type/error/is, which makes it a new dependency. It's the best way known to me to detect a native error, and dependency on it's own is light (has no other deps), and has many other useful type validation utils which we may want to rely on. It's also already used in @serverless/enterprise-plugin)
Do not show SLS_DEBUG=* suggestion when it's already used
Unconditionally expose stack trace for non user errors (so not ServerlessError's, and others we recognize) (If they happen they're definitely a result of bug, and it'll be good to have stack trace provided in issue reports)
Improve variable naming

Is it a breaking change?: NO

pmuens

Looks good so far 👍

I ran the code locally and apparently the recent changes introduce some hard to read error messages (see below). Note that I haven't set SLS_DEBUG in both cases.

Old (Master)

Y A M L Exception --------------------------------------
 
  can not read a block mapping entry; a multiline key may not be an implicit key in "/Users/philipp/Desktop/code.nosync/serverless/tmp/test/serverless.yml" at line 13, column 1:
    # -- POOLS
    ^
 
     For debugging logs, run again after setting the "SLS_DEBUG=*" environment variable.
 
  Get Support --------------------------------------------
     Docs:          docs.serverless.com
     Bugs:          github.com/serverless/serverless/issues
     Issues:        forum.serverless.com
 
  Your Environment Information ---------------------------
     Operating System:          darwin
     Node Version:              10.15.3
     Serverless Version:        1.49.0
     Enterprise Plugin Version: 1.3.1
     Platform SDK Version:      2.0.3

New (This PR)

 Exception -----------------------------------------------
 
  { YAMLException: can not read a block mapping entry; a multiline key may not be an implicit key in "/Users/philipp/Desktop/code.nosync/serverless/tmp/test/serverless.yml" at line 13, column 1:
    # -- POOLS
    ^
    at generateError (/Users/philipp/Desktop/code.nosync/serverless/node_modules/js-yaml/lib/js-yaml/loader.js:167:10)
    at throwError (/Users/philipp/Desktop/code.nosync/serverless/node_modules/js-yaml/lib/js-yaml/loader.js:173:9)
    at readBlockMapping (/Users/philipp/Desktop/code.nosync/serverless/node_modules/js-yaml/lib/js-yaml/loader.js:1073:9)
    at composeNode (/Users/philipp/Desktop/code.nosync/serverless/node_modules/js-yaml/lib/js-yaml/loader.js:1359:12)
    at readDocument (/Users/philipp/Desktop/code.nosync/serverless/node_modules/js-yaml/lib/js-yaml/loader.js:1519:3)
    at loadDocuments (/Users/philipp/Desktop/code.nosync/serverless/node_modules/js-yaml/lib/js-yaml/loader.js:1575:5)
    at Object.load (/Users/philipp/Desktop/code.nosync/serverless/node_modules/js-yaml/lib/js-yaml/loader.js:1596:19)
    at loadYaml (/Users/philipp/Desktop/code.nosync/serverless/lib/utils/fs/parse.js:12:17)
    at parse (/Users/philipp/Desktop/code.nosync/serverless/lib/utils/fs/parse.js:30:16)
    at fse.readFileAsync.then.contents (/Users/philipp/Desktop/code.nosync/serverless/lib/utils/fs/readFile.js:7:63)
    at tryCatcher (/Users/philipp/Desktop/code.nosync/serverless/node_modules/bluebird/js/release/util.js:16:23)
    at Promise._settlePromiseFromHandler (/Users/philipp/Desktop/code.nosync/serverless/node_modules/bluebird/js/release/promise.js:517:31)
    at Promise._settlePromise (/Users/philipp/Desktop/code.nosync/serverless/node_modules/bluebird/js/release/promise.js:574:18)
    at Promise._settlePromise0 (/Users/philipp/Desktop/code.nosync/serverless/node_modules/bluebird/js/release/promise.js:619:10)
    at Promise._settlePromises (/Users/philipp/Desktop/code.nosync/serverless/node_modules/bluebird/js/release/promise.js:699:18)
    at Promise._fulfill (/Users/philipp/Desktop/code.nosync/serverless/node_modules/bluebird/js/release/promise.js:643:18)
    at /Users/philipp/Desktop/code.nosync/serverless/node_modules/bluebird/js/release/nodeback.js:42:21
    at /Users/philipp/Desktop/code.nosync/serverless/node_modules/graceful-fs/graceful-fs.js:90:16
    at FSReqWrap.readFileAfterClose [as oncomplete] (internal/fs/read_file_context.js:53:3)
  name: 'YAMLException',
  reason:
   'can not read a block mapping entry; a multiline key may not be an implicit key',
  mark:
   Mark {
     name:
      '/Users/philipp/Desktop/code.nosync/serverless/tmp/test/serverless.yml',
     buffer:
      'service: philipp-test-${self:custom.idx}\n\nprovider:\n  name: aws\n  runtime: nodejs10.x\n  versionFunctions: false\n\ncustom:\n  idx: 0\n  bucket: serverless-existing-s3-bucket\n  pool: ExistingUserPool\ns\n# -- POOLS\n# functions:\n#   pre:\n#     handler: handler.hello\n#     events:\n#       - cognitoUserPool:\n#           pool: ${self:custom.pool}\n#           trigger: PreSignUp\n#           existing: true\n#       # - cognitoUserPool:\n#       #     pool: ${self:custom.pool}\n#       #     trigger: DefineAuthChallenge\n#       #     existing: true\n#   post:\n#     handler: handler.hello\n#     events:\n#       - cognitoUserPool:\n#           pool: ${self:custom.pool}\n#           trigger: CreateAuthChallenge\n#           existing: true\n#       # - cognitoUserPool:\n#       #     pool: ${self:custom.pool}\n#       #     trigger: VerifyAuthChallengeResponse\n#       #     existing: true\n# -- BUCKETS\n# functions:\n#   created:\n#     handler: handler.hello\n#     events:\n#       - s3:\n#           bucket: ${self:custom.bucket}\n#           events: s3:ObjectCreated:*\n#           rules:\n#             - suffix: .png\n#             - prefix: uploads\n#           existing: true\n#       - s3:\n#           bucket: ${self:custom.bucket}\n#           events: s3:ObjectCreated:*\n#           rules:\n#             - suffix: .jpg\n#             - prefix: uploads\n#           existing: true\n#   deleted:\n#     handler: handler.hello\n#     events:\n#       - s3:\n#           bucket: ${self:custom.bucket}\n#           event: s3:ObjectRemoved:*\n#           rules:\n#             - suffix: .png\n#             - prefix: uploads\n#           existing: true\n#       - s3:\n#           bucket: ${self:custom.bucket}\n#           event: s3:ObjectRemoved:*\n#           rules:\n#             - suffix: .jpg\n#             - prefix: uploads\n#           existing: true\n\u0000',
     position: 197,
     line: 12,
     column: 0 },
  message:
   'can not read a block mapping entry; a multiline key may not be an implicit key in "/Users/philipp/Desktop/code.nosync/serverless/tmp/test/serverless.yml" at line 13, column 1:\n    # -- POOLS\n    ^' }
 
     For debugging logs, run again after setting the "SLS_DEBUG=*" environment variable.
 
  Get Support --------------------------------------------
     Docs:          docs.serverless.com
     Bugs:          github.com/serverless/serverless/issues
     Issues:        forum.serverless.com
 
  Your Environment Information ---------------------------
     Operating System:          darwin
     Node Version:              10.15.3
     Serverless Version:        1.49.0
     Enterprise Plugin Version: 1.3.1
     Platform SDK Version:      2.0.3

pmuens · 2019-08-06T10:01:24Z

lib/classes/Error.js

+  consoleLog(chalk.yellow(`     Node Version:              ${nodeVersion}`));
+  consoleLog(chalk.yellow(`     Serverless Version:        ${slsVersion}`));
+  consoleLog(chalk.yellow(`     Enterprise Plugin Version: ${sfeVersion}`));
+  consoleLog(chalk.yellow(`     Platform SDK Version:      ${sdkVersion}`));


We just changed the name of those values via #6501 so this PR might need a rebase / update.

Actually it was not changed here (in master it's still Enterprise Plugin Version). I can rename it, still isn't Plugin name too vague?

Yes, most agreed that it's too vague. According to JIRA that's the whole idea (vague == more flexible)

pmuens · 2019-08-06T10:02:46Z

lib/classes/Error.js

@@ -2,6 +2,7 @@

 const chalk = require('chalk');
 const { inspect } = require('util');
+const isError = require('type/error/is');


Why don't we use https://lodash.com/docs/4.17.15#isError here?

My bad, I didn't actually check if Lodash offers something like that.

I've updated it to use _.isError (by looking into internals it looks good, and I agree in context of this single case it's not justified to introduce new dependency).

Still I think at some point it might be good if we derive from lodash, so we rely more on native constructs, and imo type stands as nice complement of runtime type validation utils (which is not really covered by lodash), through which we may improve DX.

medikoo · 2019-08-06T12:36:50Z

I ran the code locally and apparently the recent changes introduce some hard to read error messages (see below).

Good find, indeed it got worse for YAMLException.

First reason is that YAMLException is custom error constructed with ES5 means (no as class extension), so it was not detected as native error by type/error/is, and it was output through inspect(error).
I've improved error detection in type so ES5 custom errors are picked now, but I've also switched for now to _.isError which also recognizes it.

Other reason is that full stack trace was output as YAMLException was not recognized as user error. I've updated it so it's recognized now as such (like ServerlessError is)

pmuens

Thanks for updating the PR @medikoo 👍

I just tested it again and it looks good so far. It looks like the only thing we need to update as well is the naming we've also changed via #6501.

Not sure why we ended up having 2 different places for that information but it would be less confusing for users reporting errors / their version if we have 2 different naming systems.

pmuens

Thanks for updating @medikoo 👍

LGTM

medikoo · 2019-08-07T07:02:28Z

It looks like the only thing we need to update as well is the naming we've also changed via #6501.

Ok, updated

medikoo added the enhancement label Aug 5, 2019

medikoo added this to the 1.50.0 milestone Aug 5, 2019

medikoo self-assigned this Aug 5, 2019

medikoo force-pushed the improve-error-handling branch from d8830fe to aa67ee1 Compare August 5, 2019 14:15

medikoo added the pr/in-review label Aug 5, 2019

medikoo requested a review from pmuens August 5, 2019 14:33

pmuens suggested changes Aug 6, 2019

View reviewed changes

medikoo added 3 commits August 6, 2019 13:02

Improve variable name

11cb305

Improve variable name

77432a0

Improve variable name

5e57ad3

medikoo force-pushed the improve-error-handling branch 2 times, most recently from c9d6ccd to 3577270 Compare August 6, 2019 11:37

medikoo added 7 commits August 6, 2019 13:44

Improve non-error exception handling

18dfb69

Show SLS_DEBUG info only when it's not used

ee091fb

Recognize isUserError variable

80e6ad5

Expose stack trace for non user errors

406d4c7

Do not pass handled errors via unhandled channel

bb4af35

Log uncaught exceptions

7ce351e

Treat unhandled rejections as uncaught exceptions

918146e

medikoo force-pushed the improve-error-handling branch from 3577270 to fb96b33 Compare August 6, 2019 11:52

medikoo added 2 commits August 6, 2019 14:14

Do not duplicate error message output

b3f06b8

Recognize YAMLException as user error

ae77dbe

medikoo force-pushed the improve-error-handling branch from fb96b33 to ae77dbe Compare August 6, 2019 12:14

Merge branch 'master' into improve-error-handling

6c54275

medikoo requested a review from pmuens August 6, 2019 12:45

pmuens suggested changes Aug 6, 2019

View reviewed changes

Clear confusing wording

87faf71

medikoo force-pushed the improve-error-handling branch from 98d08d4 to 87faf71 Compare August 6, 2019 20:00

pmuens approved these changes Aug 7, 2019

View reviewed changes

medikoo requested a review from pmuens August 7, 2019 07:02

medikoo merged commit c69b054 into master Aug 7, 2019

medikoo deleted the improve-error-handling branch August 7, 2019 07:04

medikoo mentioned this pull request Aug 16, 2019

AWS API Gateway customize log level #6542

Merged

8 tasks

This was referenced Oct 16, 2020

DX: Cleaner stack traces #3577

Closed

"Cannot read property 'LogicalResourceId' of null" (export in use) #3589

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error handling #6502

Improve error handling #6502

medikoo commented Aug 5, 2019 •

edited

pmuens left a comment •

edited

pmuens Aug 6, 2019

medikoo Aug 6, 2019

pmuens Aug 6, 2019

pmuens Aug 6, 2019

medikoo Aug 6, 2019 •

edited

medikoo commented Aug 6, 2019

pmuens left a comment •

edited

pmuens left a comment

medikoo commented Aug 7, 2019

Improve error handling #6502

Improve error handling #6502

Conversation

medikoo commented Aug 5, 2019 • edited

pmuens left a comment • edited

Choose a reason for hiding this comment

pmuens Aug 6, 2019

Choose a reason for hiding this comment

medikoo Aug 6, 2019

Choose a reason for hiding this comment

pmuens Aug 6, 2019

Choose a reason for hiding this comment

pmuens Aug 6, 2019

Choose a reason for hiding this comment

medikoo Aug 6, 2019 • edited

Choose a reason for hiding this comment

medikoo commented Aug 6, 2019

pmuens left a comment • edited

Choose a reason for hiding this comment

pmuens left a comment

Choose a reason for hiding this comment

medikoo commented Aug 7, 2019

medikoo commented Aug 5, 2019 •

edited

pmuens left a comment •

edited

medikoo Aug 6, 2019 •

edited

pmuens left a comment •

edited