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

What's a more elegant way to handle errors? #90

Closed
luhring opened this issue Aug 5, 2020 · 6 comments
Closed

What's a more elegant way to handle errors? #90

luhring opened this issue Aug 5, 2020 · 6 comments
Assignees
Labels

Comments

@luhring
Copy link
Contributor

luhring commented Aug 5, 2020

There's a scenario where grype will show an error to the user, related to an issue parsing a package version, and then will exit 0, despite the apparent error.

This display of an error is jarring and confusing to users. We should talk through what the definition of the "correct way to handle this scenario" is.

In general, I'd suggest we not show errors and then exit cleanly. And for this specific Malformed version error, we should answer a few relevant questions:

  1. Should this Go error be occurring in the first place? If so,
  2. Is a malformed package version something that should be reported to the user? If so,
  3. If we're saying the analysis finished despite this apparent problem, should this be a WARN instead of an ERROR?

Screen Shot 2020-08-05 at 10 21 28 AM

Error text:

[0012] ERROR matcher failed for pkg=Pkg(type=wheel, name=toastedmarshmallow, version=2.15.2.post1): matcher failed to parse version pkg='toastedmarshmallow' ver='2.15.2.post1': unable to crate semver obj: Malformed version: 2.15.2.post1
@luhring luhring added the bug Something isn't working label Aug 5, 2020
@luhring luhring added this to the v0.1.0 milestone Aug 5, 2020
@alfredodeza
Copy link
Contributor

alfredodeza commented Aug 5, 2020

This is going to be a problem moving forwards with Python (specifically). PEP 404 does allow for these "post release" versions which are not compliant with semantic versioning.

The quick fix for this issue is to use "Unknown" for the versioning scheme which in turn allows to try semantic versioning first, and then fallback to fuzzy-match.

The longer (more correct) fix involves writing a Python versioning scheme parser that follows PEP 404 closely. Namely: [N!]N(.N)*[{a|b|rc}N][.postN][.devN]

@wagoodman
Copy link
Contributor

wagoodman commented Aug 10, 2020

This specific error is being handled in #91 , this issue should encompass how we want to deal with displaying errors and go error handling in general. Lets consider this a spike for now.

@wagoodman wagoodman removed this from the v0.1.0 milestone Aug 10, 2020
@alfredodeza
Copy link
Contributor

This is fixed with #124 and https://github.com/anchore/grype-db-builder/pull/53

@luhring
Copy link
Contributor Author

luhring commented Aug 13, 2020

Reopening — this is a spike for investigation of error reporting patterns across the application(s)

@luhring luhring reopened this Aug 13, 2020
@luhring luhring assigned luhring and unassigned alfredodeza Aug 13, 2020
@luhring luhring removed the bug Something isn't working label Aug 13, 2020
@luhring luhring changed the title Error shown to user for malformed package version What's a more elegant way to handle errors? Aug 14, 2020
@luhring
Copy link
Contributor Author

luhring commented Aug 14, 2020

Spike Conclusions

Consulted references

Error handling in Go by Uday Hiwarale
The Go Blog: Error handling and go by Andrew Gerrand
Handling Errors in Go by Gopher Guides
Concurrency in Go by Katherine Cox-Buday

Emergent philosophies

  1. Logging is not the ideal mechanism for displaying information to users. Though both log data and user interface components can be colocated in the same terminal, these should be thought of as two distinct concerns. Communication to the user should be optimized for human beings' understanding. When communicating errors to the user, the user should be able to understand what went wrong and have at least one suggestion for a next step. Log data should be optimized based on how log data is consumed, for example, by being easily searchable, indexable, debuggable, machine-readable, etc. The kind of error output that gets displayed by a logger can be difficult for humans to make sense of, and log output typically discourages providing "extra" information or suggestions that might be very useful to users.
  2. Developers should express in code which errors will occur during scenarios that can be anticipated. This forces developers to understand which code execution paths are knowably immune to surprise error scenarios. Having code that handles anticipated errors differently than unanticipated errors makes it easier to surface bugs to the user (and then request that the user open an issue).
  3. Custom error types should be used when added context is useful to consumers of the error. If any code that consumes an error could benefit from knowing unique context about the error, the developer should seriously consider implementing a custom error type. All custom error types should implement Go's built-in error interface. Consumers should use type assertions or type switches when determining how to respond to the error.

Suggested next steps

  1. Review and consider instances in the code where a function's return signature includes error. 1) What's the value in this function returning an error to its caller(s)? 2) Do you understand the scenarios in which this function would return an error? 3) Could a caller of this function benefit from having access to a data structure that provides more context? 4) Does the user need to be alerted that this error occurred?
  2. Wrap expected errors in custom error types, such that unwrapped (or differently wrapped) errors can be considered unexpected.
  3. Create a centralized function (e.g. in the cmd package) for handling the display of errors to the user. Handle unexpected errors by suggesting the user surface the issue to the project's maintainers (such as by including a link for opening an issue).
  4. Turn off the display of "ERROR" level log data to the user by default.
  5. Once the logger is unburdened of having to explain context to the CLI user, consider the possibility of improving the log data format (e.g. structured logging).

@luhring
Copy link
Contributor Author

luhring commented Aug 14, 2020

Discussed in a Zoom meeting with Alex and Alfredo, and there was general agreement on these opinions and next steps. 👍

@luhring luhring closed this as completed Aug 14, 2020
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

3 participants