HttpClientSample icon indicating copy to clipboard operation
HttpClientSample copied to clipboard

Suggestions

Open dustinmoris opened this issue 6 years ago • 1 comments

Hi Rehan,

I came to this repository from your blog post on how to optimally configure an ASP.NET Core HttpClientFactory.

First I'd like to say thanks for writing this up, as it is always very helpful to see real world examples of how people configure this stuff in their projects. Secondly I knew it's going to be good as soon as I read

"I don’t know why the ASP.NET team bothered to provide three ways to register a client, the typed client is the one to use."

;)

Now here's some questions/suggestions:

Another possibility I’ve not tried is to combine these two scenarios, so you have two circuit breakers. The curcuit breaker with the lower limit would kick in first but only break the circuit for a short time, if exceptions are no longer thrown, then things go back to normal quickly. If exceptions continue to be thrown, then the other circuit breaker with a longer duration of break would kick in and the circuit would be broken for a longer period of time.

Is there currently no way in Polly to achieve this with a single circuit breaker? I think this would be an optimal implementation, because the longer a service is broken, the less it makes sense to periodically keep hitting it. I think there must be a way to configure the circuit breaker to either have an exponential break like the retry policy can have, or we should raise this as a feature request in the Polly repository.

..., it looks like they consider any of the below as transient errors: Any HttpRequestException thrown. This can happen when the server is down. A response with a status code of 408 Request Timeout. A response with a status code of 500 or above.

I think this is a good start, but when speaking about optimally configuring a HttpClient I think a default policy (which really every http client in .NET Core should always use - without exceptions) would be to respect a 429 Too many requests response with an optional Retry-After http header which tells the client for how long to back off. This should be IMHO a standard circuit breaker policy, because there is no point for a client to even retry or hit a second time the server from another request if the API is rate limiting the client. At this point the only sensible option is to back off via a circuit break for at least the given time in Retry-After. Would love to see that in your blog post being covered, because a lot of folks will probably use that as a template for optimal configuration.

Another topic which I personally am interested and I don't find much useful information in the official Polly documentation is logging. For debugging purposes I think it would be useful to have access to an ILogger<T> object inside the policy which would allow us to log when a circuit is broken for a long time for example. I'd love to have somewhere a _logger.LogDebug(...) call so when I want to turn the log level up in a local environment I can follow in my logs when certain events are being triggered.

There were my main questions/suggestions which I think would add huge value when talking about optimally configuring this stuff, so it can be useful in a real world scenario. Would love to hear your thoughts on this!

dustinmoris avatar Sep 29 '18 08:09 dustinmoris

Thank you for your kind words.

Double Circuit Breaker

This is a pretty advanced pattern that would probably only be useful for large applications under a lot of load. It should be sufficient to simply add an additional named circuit breaker policy with different settings but I do like your idea for an exponential backoff. The retry policy already does this, so the design could be modelled on that. Would make a nice issue/PR in the Polly repo.

429 Too Many Requests

I think thats a great idea. If implemented, it could potentially be a one liner that you could add as the first policy in your configuration, something like:

HttpPolicyExtensions
    .HandleTooManyRequestsHttpError()
    .HandleTransientHttpError()

The second part to this is of course to catch BrokenCircuitException and return the 429 Too Many Requests response on the server. You'd need to decide the length of time to return in the Retry-After HTTP header as the BrokenCircuitException does not contain that information and ideally, it should. So potentially, there are two PR's to Polly here.

Logging

I can't remember if I covered it in my blog post but there is some default logging in HttpClientFactory. Did you have a look at that?

RehanSaeed avatar Oct 01 '18 08:10 RehanSaeed