Improve error handling #116
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Go coverage report: Click to expand.
Go lint report: No issues found. 😎 |
|
This will be great, thank you for working on this. Two thoughts: I wonder how cautious we should be about not breaking the public interface in terms of errors returned. The current defined errors (like Any reason why you decided to not go with carbonapi/zipper/types/errors.go Lines 13 to 34 in 02613ff Extra info could be added like this: var BadType = merry.New("bad type")
err := BadType.Append("expected string - got int")
fmt.Println(err.Error()) // bad type: expected string - got intI see that the errors in the parser are defined with carbonapi/pkg/parser/interface.go Lines 32 to 49 in 02613ff but it looks like that's ~5 years old, and merry was introduced later. So I think it's OK to migrate those errors to merry |
I believe most of the errors I have added were were defined with fmt.Errorf() or error.New(), so they weren't originally defined as merry errors. One thing we could do is define any errors that were originally created with merry.New() as merry errors. What we could also do is merry.Wrap() with these new error types, which will make them into merry errors. |
Comment on lines
+15
to
+18
| func (e ErrMissingExpr) HTTPStatusCode() int { | ||
| return http.StatusBadRequest | ||
| } |
There was a problem hiding this comment.
I can't think of any errors we would define here that are not bad requests, plus I don't see we are using this method anywhere yet. So I believe we can don't define these for now to simplify the code if you agree
Comment on lines
+58
to
+71
| Exp string | ||
| Got string |
There was a problem hiding this comment.
Idea: maybe these could be of type ExprType and the func (e ErrBadType) Error() could take care of formatting them? (i.e. EtName -> "series name", EtBool -> "bool", etc)
There was a problem hiding this comment.
In some cases, more than one type is acceptable, i.e. EtConst or EtString. What should we do in that case?
There was a problem hiding this comment.
Maybe I can make Expected a list of type ExprType, then join them?
pkg/errors/errors.go
Outdated
| type ErrInvalidArgument string | ||
|
|
||
| func (e ErrInvalidArgument) Error() string { | ||
| return fmt.Sprintf("\"too many arguments\" %q", string(e)) |
This commit adds the files that are specific to Grafana's fork that are not intended to be included in the upstream. This files are already present in the commit history (they were deleted), but this is an attempt of a fresh start on top of the upstream's `main`. At the moment, this should the only commit we expect to skip when sending our changes.
npazosmendez
force-pushed
the
cedwards/improve-error-handling
branch
from
27e9a61
to
e1f83cd
Compare
* Fix exponentialMovingAverage panic The panic was happening when the window size was greater than the length of the values. Now if that happens no panics occur and the average is returned. This matches graphiteweb. * Add support for Graphite web toLowerCase function * Add tests for toLowerCase function * Update glue.go * Update COMPATIBILITY.md to show support of toLowerCase * Add support for Graphite web toUpperCase function * Add tests for toUpperCase function * Update glue.go * Update COMPATIBILITY.md to include toUpperCase function * Add support for Graphite web sumSeriesLists function * Add tests for sumSeriesLists * Update glue.go * Update COMPATIBILITY.md to include sumSeriesLists * Fix problems that I accidentally introduced while trying to resolve merge conflict * Another attempt to fix previous problem Apparantly there is a directory that gen.go wasn't expecting and `go generate` in expr/functions produced `glue.go` file that didn't compile fix both generator and glue * Adds Drone and Github workflow for Grafana's fork This commit adds the files that are specific to Grafana's fork that are not intended to be included in the upstream. This files are already present in the commit history (they were deleted), but this is an attempt of a fresh start on top of the upstream's `main`. At the moment, this should the only commit we expect to skip when sending our changes. Co-authored-by: Nicolás Pazos <npazosmendez@gmail.com> Co-authored-by: Vladimir Smirnov <civiloid@google.com>
carrieedwards
force-pushed
the
cedwards/improve-error-handling
branch
from
e683b99
to
f81cb69
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR creates an error package that can be reused throughout the code and will provide better detail regarding the origin of the error. This is useful for testing errors.