Return structured errors from functions that do HTTP requests

[echlebek]

This is a similar issue to #95, but somewhat distinct.

In ManageEvent, several different conditions can return an error. Depending on the type of error delivered, the caller might want to take a variety of actions. However, this is hard-to-impossible currently, since the function just returns a string.

One use case, for instance, is retrying a request if the HTTP status is 5xx, but not retrying the request if the HTTP status is 4xx. Or, truncating the request size if the service returns a 413 error, and trying again.

I'd suggest something like the following for a structured error type:

type ManageEventError struct {
  // StatusCode is the status code from the response
  StatusCode int

  // Body is the contents of the response body
  Body []byte

  // Header is the response header
  Header http.Header

  // Message is a human-readable error message
  Message string
}

func (e *ManageEventError) Error() string {
  return e.Message
}

Depending on the exact expectations of the submitter of #95, this should be enough information to satisfy their needs as well. The upside of doing this, is that the functionality can be implemented without breaking the API, and so can be done in a minor release. The caller can simply use a type assertion to check further error details.

[stmcallister]

Hello! Sorry for the delay on responding to this. I like your suggestion. Any chance you could submit a PR that implements this?

[theckman]

So I'm not confident the structure above makes sense for the error structure that the PagerDuty API commits to returning. It may make more sense to have the error type include the HTTP status code, as well as a structure aligned with what's here: https://developer.pagerduty.com/docs/rest-api-v2/errors/

I'm hitting an annoying case with this lack of error propagation, where I'm trying to enumerate all the active users who could be paged for an incident against a service. Unfortunately, our schedule is hella old and contains deactivated users in old layers... unfortunately, we get a 404 error from the API when trying to get the details of these users. The Go API client makes it nearly impossible to programmatically know what's what happened, since I could only check that by doing a substring match against the error.

I might try to whip-up a PR that takes some inspiration from the above structure, but instead provides the error structure instead of the unparsed response body.

[theckman]

Actually, that looks to mostly exist here. I'd want to see whether I could provide something more than an interface{} for those errors:

go-pagerduty/client.go

Lines 59 to 63 in eab6a10

	type errorObject struct {
	Code int `json:"code,omitempty"`
	Message string `json:"message,omitempty"`
	Errors interface{} `json:"errors,omitempty"`
	}

[echlebek]

Hi @stmcallister and @theckman.

Sorry I didn't respond before. I think at the time, I was probably busy with the holiday season and your message got lost in the noise. But, now I'm glad I didn't respond, because Tim made a great point about using the structured data from PagerDuty's API.

I think that including the details from the expected error response as suggested makes a great deal of sense. Happy to do some code review if someone ends up writing this.

[theckman]

@echlebek thank you for that offer. I've a branch that I think should accomplish what both you and I want. Let me just add a blurb to the README.md about how to use the new exported error type, and maybe the package documentation as well, and then I'll get the PR up and tag you on it for 👀 .