-
Notifications
You must be signed in to change notification settings - Fork 709
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
feature: add OnError hook #398
Conversation
This indirectly allows #356 since you can use OnAfterResponse to return an custom error that includes the response object, and therefore react to it from OnResponseError: type ResponseError struct {
Response *resty.Response
}
func (r *ResponseError) Error() string {
return r.Response.Status()
}
/// ...
client.OnAfterResponse(func(client *resty.Client, response *resty.Response) error {
if response.IsError() {
return &ResponseError{Response: response}
}
return nil
})
client.OnResponseError(func(err error) {
var re *ResponseError
if errors.As(err, &re) {
// Can use re.Response to get response body, status, etc...
}
}) This PR could be modified to change the Another approach might be to add this ResponseError type directly to I'm open to comments about either approach, or any other ideas. |
Codecov Report
@@ Coverage Diff @@
## master #398 +/- ##
==========================================
+ Coverage 95.51% 95.94% +0.43%
==========================================
Files 10 10
Lines 1003 1012 +9
==========================================
+ Hits 958 971 +13
+ Misses 25 23 -2
+ Partials 20 18 -2
Continue to review full report at Codecov.
|
One other thing I'm thinking about is if |
33cc56e
to
6e4f1bd
Compare
In the absence of any feedback, I've implemented the above enhancements:
Note: I'm going the route of wrapping the error rather than changing the function signature so that it is more difficult for users of the library to unintentionally nil-dereference the response. Although the type assertion is arguably uglier to look at, it is more difficult to misuse, which I think is a good trade-off. type assertionclient.OnError(func(err error) {
if v, ok := err.(*resty.ResponseError); ok {
resp := v.Response()
// Do something with v.Response
}
// Log the error, increment a metric, etc...
}) errors.Asclient.OnError(func(err error) {
var v *resty.ResponseError
if errors.As(err, &v) {
// Do something with v.Response
}
// Log the error, increment a metric, etc...
}) |
Update: I added this as another commit ErrorHook func(*Request, error) |
Adds a new hook that is called when a rest request returns an error. This is called when the err returned by client.execute is non-nil, and it is only called after all retry attempts have been exhausted. This feature is intended to support logging, metrics, tracing on request errors that otherwise cannot be captured with OnAfterResponse, such as TLS Connection errors. It also provides a way to report response errors only once after retries have been attempted, to avoid emitting error logs when a request ultimately succeeds.
If a request results in an error which also has a response from the server, it will be wrapped with responseError so that the response is available to the OnError hook. The hook can use a type assertion or errors.As to get the response. client.OnError(func(err error) { if v, ok := err.(interface{ Response() *resty.Response }); ok { resp := v.Response() } // Log the error, increment a metric, etc... })
6e4f1bd
to
932ec09
Compare
932ec09
to
569a226
Compare
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.
Requesting a few changes.
One question: why allow multiple OnError
hooks to be defined? What's the use case?
Don't have a particular use-case in mind for multiple on-error hooks, but using a slice/range makes it at least possible for one to arise without a code change to accommodate it. It doesn't make the implementation any less clean. (ranging over nil is the same as ranging over an empty list, so we don't have to do any nil checks) |
5b4bee3
to
0702d72
Compare
0702d72
to
e94de3b
Compare
Thanks for all the hard work and iterating/improving your idea. I approved this change, but that doesn't mean it will be merged. We'll wait for @jeevatkm to chime in. I consider myself a "junior" member of this project, so I defer to @jeevatkm for all merges. Also, if @jeevatkm approves of this PR, we'll want to update the README where appropriate. |
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.
@justenwalker Thanks for your PR and iterative improvements while working on the PR. Please go ahead and update the README then we merge it.
@moorereason Thanks for your review and help.
README updated. Also noticed that it did not call the hook on SRV lookup failure, so I added that. |
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.
@justenwalker Thanks for the README update.
While looking at the SRV error update, I felt OnError
could be improved with all errors in the Resty. That can be done later on.
Thanks, again 😄
@jeevatkm, do you mean including the validation error? To be frank, I think that validation check might be more trouble than it's worth. There are a lot of so-called "REST" APIs out there that do not follow standards. For example, they accept a Body in a DELETE request, etc... If you want go-resty to work with them too, I'd suggest it removing that validation check entirely. But, that's outside the scope of this PR. Anyway, Thanks for Merging! |
@justenwalker I see, it makes sense. Thanks!
@justenwalker I thought about it since the latest RFC getting an update for |
Adds a new hook that is called when a rest request returns an error.
This is called when the err returned by client.execute is non-nil, and
it is only called after all retry attempts have been exhausted.
This feature is intended to support logging, metrics, tracing on request
errors that otherwise cannot be captured with OnAfterResponse, such as
TLS Connection errors. It also provides a way to report response errors
only once after retries have been attempted, to avoid emitting error
logs when a request ultimately succeeds.