fix: custom error wrapped around error and commitment meta #134
Conversation
hopeyen
force-pushed
the
hope/metrics-and-status-code-fix
branch
from
1b63a97 to
89078cd
Compare
server/server.go
Outdated
| if err.Err != nil { | ||
| if err.Meta != nil { | ||
| recordDur(w.Header().Get("status"), string(err.Meta.Mode), string(err.Meta.CertVersion)) | ||
| } else { | ||
| recordDur(w.Header().Get("status"), string("NoCommitmentModeSet"), string("NoCertVersionSet")) | ||
| } | ||
| return err.Err | ||
| } |
There was a problem hiding this comment.
when can the meta not be set? Would be good to document this.
Actually thinking more not a big fan of this. We should only return a metaError if the meta fields are set. If they aren't then we can return a simple error. And then we can switch on the error type instead of doing "duck-typing" like this.
There was a problem hiding this comment.
I was thinking this would take care of the case when readCommitmentMode has returned error, but I realized that now that commitmentMeta {} won't match to nil:(
I've taken a different approach now by matching like if meta, ok := err.(MetaError); ok {...}. let me know if you think it could be improved further
There was a problem hiding this comment.
Yep this is almost there! Just want to use go's native error library's is function (https://pkg.go.dev/errors#Is) instead of type checking like this, which allows us to wrap a MetaError in another error if ever we need that, and will still work.
I recommend https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully if you want to learn more about golang's error handling philosophy. :)
There was a problem hiding this comment.
thank you for the resources! Isn't the current errors.As an native error function just like errors.Is? My interpretation between these two options were that errors.Is checks for exact values of the error while errors.As checks for the type of the error, and both options are recursive for wrapped types.
Using As made more sense to me, but per your suggestion, if I were to move from As to Is, it seems I will have to define a function like func (m MetaError) Is(target error) bool { return target.Err != nil && target.Meta != nil}. is this what you had in mind?
There was a problem hiding this comment.
Oh nvm errors.As is the right option here. errors.Is/As are the same, just that is only checks the type, whereas As unwraps it into a struct that is then usable. Since we want to use that struct, As is perfect here. I was probably still looking at the old code that was just type casting instead of using errors.As.
server/server.go
Outdated
| if err.Err != nil { | ||
| if err.Meta != nil { | ||
| recordDur(w.Header().Get("status"), string(err.Meta.Mode), string(err.Meta.CertVersion)) | ||
| } else { | ||
| recordDur(w.Header().Get("status"), string("NoCommitmentModeSet"), string("NoCertVersionSet")) | ||
| } | ||
| return err.Err | ||
| } |
There was a problem hiding this comment.
Yep this is almost there! Just want to use go's native error library's is function (https://pkg.go.dev/errors#Is) instead of type checking like this, which allows us to wrap a MetaError in another error if ever we need that, and will still work.
I recommend https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully if you want to learn more about golang's error handling philosophy. :)
Added a
HandlerErrortype to contain error and commitment meta (mode and version) for metrics