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

Improve error reporting #9499

Merged
merged 13 commits into from May 19, 2021
Merged

Improve error reporting #9499

merged 13 commits into from May 19, 2021

Conversation

medikoo
Copy link
Contributor

@medikoo medikoo commented May 19, 2021

With goal to improve specificity of error codes in telemetry reports

  • Bug fixes:
    • Ensure user errors are thrown with ServerlessError constructor (otherwise they're assumed as programmer error)
    • In local invocation report invalid (non function) handler meaningfully (bug noticed in telemetry reports)
    • Ensure to not dirty crash on misconfigured resources when normalizing resources for check for changes check (user may inject incomplete resource configuration to resources and crashing here on that, prevented failing with meaningful error message)
  • Telemetry enhancements
    • If we deal with user error, which has error code, but it's format is not familiar, report also error location (it's to pick eventual omissions in setting meaningful error code, where e.g. external error code is passed, in telemetry such case can be observed with ValidationError which was not perfectly clear where it comes from)
    • Introduce specific error codes for AWS request error. now error code will include information about service and method that failed
    • Introduce specific error codes for Stack update failures. Property validation is detected specifically, otherwise we report service, method name and error type within error code
  • Maintenance improvements
    • Move local invocation test fixtures to test folder
    • Remove obsolete (dead path) error handling in implementation of sls stats command

@medikoo medikoo force-pushed the 0511-fix-user-error branch 2 times, most recently from f0a13c6 to 1cfbc43 Compare May 19, 2021 10:22
@codecov
Copy link

codecov bot commented May 19, 2021

Codecov Report

Merging #9499 (8057cca) into master (5601025) will decrease coverage by 0.07%.
The diff coverage is 69.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9499      +/-   ##
==========================================
- Coverage   86.83%   86.76%   -0.08%     
==========================================
  Files         328      322       -6     
  Lines       12050    12033      -17     
==========================================
- Hits        10464    10440      -24     
- Misses       1586     1593       +7     
Impacted Files Coverage Δ
lib/classes/Service.js 86.40% <0.00%> (ø)
lib/plugins/aws/deployFunction.js 96.62% <0.00%> (ø)
lib/plugins/aws/provider.js 93.75% <0.00%> (ø)
lib/plugins/aws/invokeLocal/index.js 68.76% <22.22%> (-0.11%) ⬇️
lib/configuration/read.js 83.56% <44.44%> (-4.85%) ⬇️
lib/classes/PluginManager.js 95.13% <50.00%> (ø)
lib/configuration/variables/index.js 90.00% <50.00%> (+0.52%) ⬆️
lib/plugins/plugin/install.js 98.63% <50.00%> (+0.01%) ⬆️
lib/plugins/aws/utils/credentials.js 90.62% <66.66%> (-1.32%) ⬇️
lib/plugins/aws/lib/monitorStack.js 98.66% <91.66%> (-1.34%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5601025...8057cca. Read the comment docs.

pgrzesik
pgrzesik previously approved these changes May 19, 2021
Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome job 👏 I've managed to spot only a minor typo in one of the error codes, otherwise it looks great

throw new ServerlessError(
`Custom variable resolver for ${variablePrefix} defined by ${pluginName} ` +
"specifies isDisabledAtPrepopulation but doesn't provide a string for a name",
'USTOM_LEGACY_VARIABLES_RESOLVER_INVALID_CONFIGURATION'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's C missing at the beginning of error code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find! Fixed

@@ -167,7 +169,7 @@ module.exports = async (exception, options = {}) => {
code: exception.code,
};

if (!isUserError || !exception.code) {
if (!isUserError || !exception.code || !isErrorCodeNormative(exception.code)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great call with including location for such errors as well

Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done 👏

@medikoo medikoo merged commit a6f4dc3 into master May 19, 2021
@medikoo medikoo deleted the 0511-fix-user-error branch May 19, 2021 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants