Ease interpretation of errors by HTTP status code

[seh]

Addressing #45, define two exported predicate functions to discern common reasons for instance-related operation failures.

Also, correct the expected HTTP status code in successful responses to deregistration requests.

Compare this branch against the earlier #46, which this one supersedes. In #46, I had used separate error types for each of the attempted HTTP operations. Here, instead, I used a single error type with an enumeration for the attempted HTTP operation, which I found made for more compact initializer expressions.

[damtur]

Hey @seh
Thank you very much for your PR. I have a few comments.
First I've double checked the return code for deregistration and indeed it looks like a bug to me, so thank you for noticing and fixing that. I checked current version of eureka code and it indeed returns 200. I'm a little bit concerned about how was that implemented in previous versions of eureka thus I would probably leave more defensive if 200 || 204 in case there was a bug in eureka and we want to keep backwards compatibility.

In terms of error handling I think your solution is really clever, but to be honest I don't think it should be in fargo. Frankly speaking I like your workaround more then implementing this on a fargo side. We want to keep fargo as small and simple as possible. I would leave interpretation of error codes to the client app.
I agree with you that exposing new types of errors is not great either, and parsing error string is not ideal.
Here are some ideas I would like to discuss with you:

1. Reformat error strings so they are unified and easy to parse with confidence e.g. [returnCode=%s] Error msg
2. Instead of checking if instance Was missing by checking the error add a new API call like IfInstanceIsMissing and then handle all logic on fargo side. This will allow you to check for missing instance even without registration. Why I think this is better then error handling you proposed? Well, it might not be, but it will not interfere with current API functions we have right now and also will not change errors we are returning right now. #backwardscompatibility

Please let me know what you think. (Also if you want to change Deregistration return code only feel free to open another pull request with that change so we can merge it).

Thanks!
Damian

[seh]

I appreciate your thoughtful reply. First, let me submit a separate PR for just the deregistration status code, and then I'll come back to this discussion.

[seh]

Please see #48 for the deregistration status code portion of the proposal.

[seh]

For your second idea (IfInstanceWasMissing), I'm not sure what you're proposing. Are you suggesting something like "register this instance if it's not currently known to Eureka"? If so, that's what fargo.RegisterInstance does today.

Would you be amenable to function like the following?
(Sketching)

// HTTPResponseStatusCode extracts the HTTP status code for the response from Eureka
// that motivated the supplied error, if any. If the returned present value is true, the returned
// code is an HTTP status code.
func HTTPResponseStatusCode(err error) (code int, present bool)

With that, a caller could try something like fargo.HeartBeatInstance and, if it fails, take the returned error and write the following:

if err := e.HeartBeatInstance(instance); err != nil {
  if code, ok := fargo.HTTPResponseStatusCode(err); ok {
    switch code {
      case http.StatusNotFound:
        // React to the instance being gone.
      default:
       // React to other codes.
    }
  }
  // Handle errors that preceded receiving an HTTP response.
}

I'd implement that function similarly to how I implemented the first pass at this proposal: return a private struct that implements error, but to which we can downcast and retrieve the status code.

I suspect that this solution would be more to your liking, as it keeps some of the situational interpretation out of fargo—even though fargo's method do contain some crude interpretation of which responses indicate success versus failure.

[damtur]

For my idea: IfInstanceISMissing would just check if the instance is currently missing without any changes. That would be safe to call as it just would return the state and not change anything.
I've not pursued if that's even possible, but that would be quite neat. As you could act in your code even without calls to trigger heartbeat.

With your soulution, I like that approach more. As long as you can make it backwards compatible, so if someone were parsing error message as it is now then it should still work. I feel that overriding Error() accordingly should do the trick.

Please let me know what you think. Thank you!

[seh]

It's possible to implement IfInstanceIsMissing by requesting the specific instance ID from Eureka via a GET request, and returning true if the response has status code 404, false if it's 200 (and maybe any other code, depending on how you want to express uncertainty). However, I'd rather first pursue the proposed HTTPResponseStatusCode function.

I've adapted this proposal, whittling it down, but I still need to revise the tests. I'll aim to post it as a fresh pull request tomorrow.

[seh]

I'll abandon this proposal in favor of #52.