-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat(errors): create new error types to carry more info #199
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good.
Here are some things to consider:
- Perhaps we could consolidate a few of the "discriminator" values. If the discriminator value is only one of multiple values that are used to create a "hash" then it might not be important to have super granular discriminator values. For example, in jwt_utils.go perhaps all three calls to coreSDKErrorf() could use "invalid-token" or "invalid-jwt" for the discriminator.
- We might want to scrutinize the various discriminator values that we use (I have my info developer hat on while making this statement 😂). Perhaps "invalid-input" instead of "bad-char", etc.
- We might also want to include as part of this work changes to our error message constants so that the actual messages are not capitalized, which I think is idiomatic for Go (e.g. "Error while turning the light on" should be "error while turning the light on" 😄)
We discussed this in person but I'll leave responses to your feedback here so we have it in writing
The purpose of the discriminator is to differentiate between errors that would otherwise have the same hash. For example, in
Something I should have documented is that the discriminator is not ever meant to be seen by the user. It's a private field, so it won't be accessible and it won't appear in the YAML-ified log messages. It's just there as a convenience field for developers to "discriminate" between errors. So, the user-friendliness shouldn't be too relevant. That said, I agree it's worth taking a look to see if any of them detract from the readability of the code itself.
Good idea - I hadn't been aware of that but I agree, this would be an appropriate time to make such a change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks pretty good, I really like the logic. Left some comments to consider, but none of them is game changer.
Before I forget: I know this is just a draft, but please run the goimports tool on the project or at least on the new errors.go
file to fix the order of the imports (3rd party to the bottom, separated by a new line) and the indentation (tab instead of spaces 😛 ).
As potential consumer of this PR - i would like to extend HTTPError with
so i could do something like
vast majority of errors we have to handle is coming from: assuming we do this call in some handler in our microservice and we want e.g. to fail request with proper array of errors from downstream services, that would help a lot (we can do all this using DetailedResponse, but if we had this in error that would be very convenient) PS. please don not change current value returned by err.Error() |
Thank you for posting the feedback! The more eyes we can get on this change, the better. I'll discuss this proposed change with the team. That is helpful to be aware of.
Great call. It has been my goal to keep the existing error messages intact so that Thanks again for looking this over and providing feedback. Feel free to share any other thoughts you have. |
07ab5b4
to
7d46550
Compare
Signed-off-by: Dustin Popp <[email protected]>
Signed-off-by: Dustin Popp <[email protected]>
Signed-off-by: Dustin Popp <[email protected]>
Signed-off-by: Dustin Popp <[email protected]>
- Treat APIs as separate components by making HTTP errors the "causedBy" of core errors - Refactor and expose certain functions to enable utilizing the new error logic in Terraform - Require "causedBy" errors to be our custom "Problem" type, rather than allow generic errors - Outfit MCSP authenticator with new error creation functions - Add lower snake case JSON tags to all error structs, as well as the DetailedResponse struct Signed-off-by: Dustin Popp <[email protected]>
Also: try to add back in the error code but exclude it from the hash computation. Signed-off-by: Dustin Popp <[email protected]>
Now, responses from authentication requests are processed the same way responses from normal requests are. Signed-off-by: Dustin Popp <[email protected]>
Also: - Remove system from function name to avoid redundancy - Make Problem a superset of error interface - Document OrderableProblem stuff Signed-off-by: Dustin Popp <[email protected]>
Signed-off-by: Dustin Popp <[email protected]>
* Add "Severity" as a field for all Problem types * Rename "System" to be "Component" * Rename "Error" to be "Problem" Signed-off-by: Dustin Popp <[email protected]>
20ed1c0
to
5790cdf
Compare
Signed-off-by: Dustin Popp <[email protected]>
@padamstx @pyrooka @hudlow this is now ready for review. I need to add some tests and finalize a couple of details but the bulk of the logic should remain as it is now. I also might split some of the new logic into separate files because the |
@bartlomiej-malecki unfortunately, this was a bit too complicated to make it into the initial MVP scope but I still think it's a great idea and I hope to deliver it as a follow-on soon. It should be doable in a compatible fashion. |
Signed-off-by: Dustin Popp <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. I added a few ticky-tack comments to consider.
core/errors.go
Outdated
|
||
// IBMProblem holds the base set of fields that all problem types | ||
// should include. It is geared towards embedding in other | ||
// structs and it should not be used on its own (so it is not exported). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// structs and it should not be used on its own (so it is not exported). | |
// structs and it should not be used on its own. |
Just wanted to point out that since the struct name starts with an upper-case letter, it is in fact public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! That comment was leftover from when it was private, but it needs to be public to use it in Terraform. I'll update the comment.
core/errors.go
Outdated
|
||
// Summary is the informative, user-friendly message that describes | ||
// the problem and what caused it. | ||
Summary string `json:"summary" validate:"required"` // required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering... do we ever serialize/marshal these structs as JSON? If not, then we probably don't need these json
struct annotations.
Also, if we don't really ever use the validate framework on these structs (like we do with API-defined models, etc.) then we also probably don't need the validate
annotation here either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't, so you're right. These are also leftover from an earlier prototype. I'll remove them everywhere.
core/errors.go
Outdated
|
||
// Summary is the informative, user-friendly message that describes | ||
// the problem and what caused it. | ||
Summary string `json:"summary" validate:"required"` // required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fear that these various // required
comments are somewhat superfluous and might in the future create more harm than good.
I'm not exactly sure what that comment implies for a struct like this, but whatever it is conveying should probably just be incorporated into the comment above the field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I was adding these for my own edification while planning out the schemas but they are unnecessary and unhelpful - I'm removing them.
core/errors.go
Outdated
GetID() string | ||
|
||
// Error returns the message associated with a given problem and guarantees | ||
// every instance of Problem also implements the native `error` interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be in the minority, but I'm not a big fan of using backticks in comments as an alternative to single or double quotes. It just seems too much like markdown to me 😂. I double-checked and there appear to be very few uses of backticks in comments in the go core other than this new file.
I acknowledge that this is a really picky comment 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also use it (a lot?) to emphasize that I'm referring to a variable/code element. Can't decide if it's good or not, just my habit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha I think that's fair! I'd rather be consistent with the rest of the core, so I'll replace them with double quotes.
core/error_utils.go
Outdated
// sdkStackFrame is a convenience struct for formatting | ||
// frame data to be printed as YAML. | ||
type sdkStackFrame struct { | ||
Function string `json:"function,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have json
tags on these fields or yaml
tags if we're going to be doing yaml serialization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. Actually, with the YAML package I ended up using, the keys are "lowercased" automatically when marshalling so we don't need them at all. I will remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well...it's impressive and all looks really good! I left a few small comments to consider, but that's all.
Actually, I have a few questions:
- Wouldn't be good (in long term) to create and use the discriminator strings as constants? E.g. having a
discriminator.go
so we can use them asdiscriminator.ObjIsNil
. - Wouldn't it worth to pass the
err
objects to the "constructors" likeSDKErrorf
every time, even if they are not used? I am thinking of future scenarios where we might need them... 😄 - I know this is the only initial work and the change is already huge, but I am pretty sure we want to have unit tests for the new functions in the future.
core/errors.go
Outdated
GetID() string | ||
|
||
// Error returns the message associated with a given problem and guarantees | ||
// every instance of Problem also implements the native `error` interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also use it (a lot?) to emphasize that I'm referring to a variable/code element. Can't decide if it's good or not, just my habit.
core/error_utils.go
Outdated
// information about the function the problem was created in, and | ||
// returns the name of the function. | ||
func computeFunctionName(component string) string { | ||
if pc, _, _, ok := runtime.Caller(2); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad you can make use of this runtime.Caller()
function! :)
core/errors.go
Outdated
// function. Should only be used in public (exported) functions. | ||
func RepurposeSDKProblem(err error, discriminator string) error { | ||
if err == nil { | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hate to being this picky, but I'd return this way (you also use this pattern in the rest of the changes - I checked :))
return err | |
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough - I'll make that change!
core/iam_authenticator.go
Outdated
} | ||
|
||
// newIamAuthenticatorFromMap constructs a new IamAuthenticator instance from a map. | ||
func newIamAuthenticatorFromMap(properties map[string]string) (authenticator *IamAuthenticator, err error) { | ||
if properties == nil { | ||
return nil, fmt.Errorf(ERRORMSG_PROPS_MAP_NIL) | ||
return nil, SDKErrorf(nil, ERRORMSG_PROPS_MAP_NIL, "missing_props", getComponentInfo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo?
return nil, SDKErrorf(nil, ERRORMSG_PROPS_MAP_NIL, "missing_props", getComponentInfo) | |
return nil, SDKErrorf(nil, ERRORMSG_PROPS_MAP_NIL, "missing-props", getComponentInfo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you!
core/vpc_instance_authenticator.go
Outdated
@@ -148,7 +147,7 @@ func (authenticator *VpcInstanceAuthenticator) url() string { | |||
// configuration properties. | |||
func newVpcInstanceAuthenticatorFromMap(properties map[string]string) (authenticator *VpcInstanceAuthenticator, err error) { | |||
if properties == nil { | |||
return nil, fmt.Errorf(ERRORMSG_PROPS_MAP_NIL) | |||
return nil, SDKErrorf(nil, ERRORMSG_PROPS_MAP_NIL, "missing_props", getComponentInfo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo?
return nil, SDKErrorf(nil, ERRORMSG_PROPS_MAP_NIL, "missing_props", getComponentInfo) | |
return nil, SDKErrorf(nil, ERRORMSG_PROPS_MAP_NIL, "missing-props", getComponentInfo) |
core/bearer_token_authenticator.go
Outdated
@@ -43,7 +44,7 @@ func NewBearerTokenAuthenticator(bearerToken string) (*BearerTokenAuthenticator, | |||
// newBearerTokenAuthenticator : Constructs a new BearerTokenAuthenticator instance from a map. | |||
func newBearerTokenAuthenticatorFromMap(properties map[string]string) (*BearerTokenAuthenticator, error) { | |||
if properties == nil { | |||
return nil, fmt.Errorf(ERRORMSG_PROPS_MAP_NIL) | |||
return nil, SDKErrorf(nil, ERRORMSG_PROPS_MAP_NIL, "missing_props", getComponentInfo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo?
return nil, SDKErrorf(nil, ERRORMSG_PROPS_MAP_NIL, "missing_props", getComponentInfo) | |
return nil, SDKErrorf(nil, ERRORMSG_PROPS_MAP_NIL, "missing-props", getComponentInfo) |
core/container_authenticator.go
Outdated
@@ -204,7 +203,7 @@ func (authenticator *ContainerAuthenticator) client() *http.Client { | |||
// configuration properties. | |||
func newContainerAuthenticatorFromMap(properties map[string]string) (authenticator *ContainerAuthenticator, err error) { | |||
if properties == nil { | |||
return nil, fmt.Errorf(ERRORMSG_PROPS_MAP_NIL) | |||
return nil, SDKErrorf(nil, ERRORMSG_PROPS_MAP_NIL, "missing_props", getComponentInfo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo?
return nil, SDKErrorf(nil, ERRORMSG_PROPS_MAP_NIL, "missing_props", getComponentInfo) | |
return nil, SDKErrorf(nil, ERRORMSG_PROPS_MAP_NIL, "missing-props", getComponentInfo) |
core/base_service.go
Outdated
@@ -612,6 +649,39 @@ func getErrorMessage(responseMap map[string]interface{}, statusCode int) string | |||
return http.StatusText(statusCode) | |||
} | |||
|
|||
// getErrorCode: try to retrieve an error code from the decoded response body (map). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is more godoc compliant, but feel free to leave it as-is.
// getErrorCode: try to retrieve an error code from the decoded response body (map). | |
// getErrorCode tries to retrieve an error code from the decoded response body (map). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right, good catch!
- Component now follows the graph fragment pattern - Adjust system/version provider functions to use new struct - Adjust prefixes to use hyphens instead of underscores - Add status code/request id/correlation id to http message output Signed-off-by: Dustin Popp <[email protected]>
Signed-off-by: Dustin Popp <[email protected]>
Signed-off-by: Dustin Popp <[email protected]>
@pyrooka good questions! I'll answer here:
It's a good idea but I still want to avoid using constants for a couple of reasons. The biggest one is, we need to keep in mind how we are going to enumerate the possible errors within this package. Having the string value of each discriminator right in the error constructor makes statically computing the hash value for that created error much easier. I know that's not the best reason in the world to make code decisions but it's a very real problem before us. That, and the fact that many of the discriminators only have one usage and I'm not sure it's worth writing a whole new file for keeping track of them. Some values are used more than once, like in authenticators, but that doesn't actually matter that much. They shouldn't ever change once I commit them but even if one did or if I messed one up, consistency doesn't add any value beyond aesthetic.
The errors are only meant to be passed to the constructors if they are coming from a separate IBM Cloud component, like an HTTP service or another SDK. They are used as the "caused by" error to chain them together. Even though we have to accept an
100% - not just for the future, but now 🙂 I am working on them for this PR. |
Thanks for the answer @dpopp07! Re: the discriminator constants, it makes sense and I think you have already explained to me. I just wanted to make sure I understand your approach. Re: passing the errors - okay now I see.
Sorry, I might missed that but agree, we should have them before merging. I just didn't want to be pushy and say "LGTM, but can't approve without proper unit tests...". :) |
Signed-off-by: Dustin Popp <[email protected]>
Signed-off-by: Dustin Popp <[email protected]>
Signed-off-by: Dustin Popp <[email protected]>
# [5.16.0](v5.15.3...v5.16.0) (2024-03-13) ### Features * **errors:** augment errors with new "Problem" types ([#199](#199)) ([6ec86dd](6ec86dd))
🎉 This PR is included in version 5.16.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This is a commit with the prototype code for the new error message formats. This isn't final but it's ready for a more thorough review and using a PR seems like the best way to do it. I'll mark this as a draft and tests are still failing as I clean things up but please begin to leave feedback and comments!The PR is now complete and ready for review.