-
Notifications
You must be signed in to change notification settings - Fork 240
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
Provide a method for ferrying API errors back to the caller #265
Conversation
@echlebek ping, since you raised the linked issue. |
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 good to me, and is a big improvement over the current state of affairs in my opinion!
I do wonder if it's also worth including the response headers in the APIError type
? I would consider these to be useful for debugging, especially for situations where something is brokering the communication and adding its own headers.
I haven't tested this yet, but if the PR is still open on Monday, I'll try integrating it into the code I was working with when I filed the original issue.
@@ -40,3 +41,174 @@ func testEqual(t *testing.T, expected interface{}, actual interface{}) { | |||
t.Errorf("returned %#v; want %#v", expected, actual) | |||
} | |||
} | |||
|
|||
func TestAPIError_Error(t *testing.T) { | |||
const jsonBody = `{"error":{"code": 420, "message": "Enhance Your Calm", "errors":["Enhance Your Calm", "Slow Your Roll"]}}` |
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.
👌 ✌️
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'm still a little upset that didn't become the official status code in the RFC.
client.go
Outdated
if !strings.HasPrefix(resp.Header.Get("Content-Type"), "application/json") { | ||
return resp, fmt.Errorf("HTTP response with status code %d does not contain Content-Type: application/json", resp.StatusCode) | ||
} |
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.
Wondering if there are versions of the API that respond with JSON but don't bother setting this header? Or, are there versions of the API that respond with a 2xx status, but have a JSON-encoded error message?
If yes, is it worth caring about?
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.
In a past life I worked at PagerDuty, and at least when I was there and helped design a version of the API it was important that the Content-Type
was set and correct. I think we should feel comfortable that will be set when they return JSON from the API (whether success or an error)
I'm expecting their Nginx load balancers may not provide a JSON response if they fail, and so this would not try to parse any sort of junk that may spit back but provide the status code. In that case I'd expect it to be 5XX
, which most consumers would interpret to mean that PagerDuty experienced some sort of failure that resulted in a malformed response. So I think this is okay.
Their documentation indicates that 2XX codes will not use an error JSON document:
If an API request is successful, PagerDuty will respond to it with HTTP status code 2XX and the resources that match the query.
If the API request is unsuccessful, PagerDuty will respond with a corresponding HTTP error status code and the response body will contain PagerDuty error details.
@echlebek Regarding the headers, is that something your environment has in-place and if so could you describe it a bit more? I'm genuinely curious. Sharing this thought so I don't forget it, recognizing this may seem like overkill, but since you can provide your own HTTPClient to the PagerDuty client ( But if that's what you wanted, you could have your |
I'm now realizing I should make |
@theckman perhaps providing a custom HTTPClient does make the most sense for my use case. I can't remember exactly, but I think at the time, I didn't realize the client made that possible. The software it's been used in on my side is https://github.com/sensu/sensu-pagerduty-handler. It's a small and somewhat roughly-hewn program that's responsible for sending alerts to pagerduty from peoples' Sensu deployments. Typically I don't have visibility into or control over the environment that it's running in. Something you can probably commiserate with: failure to send a pagerduty alert is a pretty unfortunate circumstance, and people experiencing failures are often looking to diagnose and repair the problem as quickly as possible. So for this use case, more information is almost always better. When the pagerduty client fails, sensu-pagerduty-handler back-propagates a failure event to Sensu (likely ultimately destined for whatever the fallback is when pagerduty is not working). These events are usually quite data-rich and it's here that I'd be wanting to capture as much information as possible. |
Today the Go PagerDuty API client parses the JSON error response provided by the API, but does not return it in a way that the caller can use it to make an informed decision on how to handle that error. For example, being able to identify if the API request failed or if that resource just wasn't found. This change adds a new `pagerduty.APIError` type that includes the error object that gets returned from the API as well as the HTTP Status Code. This should allow callers to identify if they've ben rate limited, if the resource they were looking for wasn't found, etc. This new type also includes some helper methods to further ease the barrier in trying to understand the failure, such as .RateLimited(). This also adds a short blurb to the `README.md` file to explain how to use this error type / value. Fixes #222
7f50a48
to
31ab2fd
Compare
@echlebek I definitely can, and some of this work is related to me building a tool to make it so I can page an entire team if we are having issues getting someone off-hours. 😄 I also just force-pushed my commit here to update it a little bit. That I think I like where this is at now, especially after refining its behaviors in the case where we have an error without a JSON response. |
Thanks so much for your work on this! |
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.
Thanks for adding this @theckman! Looks awesome. I just have one question that I asked inline.
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.
👍 Thank you for improving the Error Responses! 🎉
Today the Go PagerDuty API client parses the JSON error response provided by the
API, but does not return it in a way that the caller can use it to make an
informed decision on how to handle that error. For example, being able to
identify if the API request failed or if that resource just wasn't found.
This change adds a new
pagerduty.APIError
type that includes the error objectthat gets returned from the API as well as the HTTP Status Code. This should
allow callers to identify if they've ben rate limited, if the resource they were
looking for wasn't found, etc. This new type also includes some helper methods
to further ease the barrier in trying to understand the failure, such as
.RateLimited().
This also adds a short blurb to the
README.md
file to explain how to use thiserror type / value.
Fixes #222