Skip to content
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

Missing documentation about standard HTTP resilience values #5078

Open
joegoldman2 opened this issue Apr 3, 2024 · 4 comments
Open

Missing documentation about standard HTTP resilience values #5078

joegoldman2 opened this issue Apr 3, 2024 · 4 comments
Assignees

Comments

@joegoldman2
Copy link
Contributor

The current documentation is not saying anything about the default values for the total request timeout, attempt timeout, number of retries, circuit breaker break duration, failure ratio, etc when using httpClientBuilder.AddStandardResilienceHandler();

@ghost
Copy link

ghost commented Apr 10, 2024

I was also looking for HttpStandardResilienceOptions defaults so logged it as JSON in AddStandardResilienceHandler and found the following. Note have filtered out "ShouldHandle", "DelayGenerator", "Randomizer" from this as they are too large to attach here.

{
  "RateLimiter": {
    "RateLimiter": null,
    "DefaultRateLimiterOptions": {
      "PermitLimit": 1000,
      "QueueProcessingOrder": "OldestFirst",
      "QueueLimit": 0
    },
    "OnRejected": null,
    "Name": "Standard-RateLimiter"
  },
  "TotalRequestTimeout": {
    "Timeout": "00:00:30",
    "TimeoutGenerator": null,
    "OnTimeout": null,
    "Name": "Standard-TotalRequestTimeout"
  },
  "Retry": {
    "ShouldRetryAfterHeader": true,
    "MaxRetryAttempts": 3,
    "BackoffType": "Exponential",
    "UseJitter": true,
    "Delay": "00:00:02",
    "MaxDelay": null,
    "OnRetry": null,
    "Name": "Standard-Retry"
  },
  "CircuitBreaker": {
    "FailureRatio": 0.1,
    "MinimumThroughput": 100,
    "SamplingDuration": "00:00:30",
    "BreakDuration": "00:00:05",
    "BreakDurationGenerator": null,
    "OnClosed": null,
    "OnOpened": null,
    "OnHalfOpened": null,
    "ManualControl": null,
    "StateProvider": null,
    "Name": "Standard-CircuitBreaker"
  },
  "AttemptTimeout": {
    "Timeout": "00:00:10",
    "TimeoutGenerator": null,
    "OnTimeout": null,
    "Name": "Standard-AttemptTimeout"
  }
}
dotnet --version
8.0.203

@abatishchev
Copy link

+1
I tried to follow the readme file and convert the existing code which uses Microsoft.Extensions.Http.Polly

.AddPolicyHandler(
    static _ => HttpPolicyExtensions.HandleTransientHttpError()
        .OrResult(static r => r.StatusCode == System.Net.HttpStatusCode.NotFound)
        .WaitAndRetryAsync(
            10,
            static (_, result, _) => result.Result?.Headers.RetryAfter?.Delta.GetValueOrDefault() ?? TimeSpan.FromSeconds(5),
            static (_, _, _, _) => Task.CompletedTask));

to use new Microsoft.Extensions.Http.Resilience, but the documentation is lacking. Please create a migration guide.

@tyler-boyd
Copy link

Related to this, I would love to see an explanation for why the policies are there in the order they are in.

As an example, why is the total request timeout policy inside/after the rate limit policy, when the rate limit policy can cause large delays if you enable queueing? For example...if you have a queue size of 10 and parallelism 1 and total request timeout = 30s, in the worst case scenario it could take that 10th queued request up to 300s, which is 10x higher than the total request timeout.

This is either bad or I'm misunderstanding something, and it would be much easier to tell which if the motivation behind the standard policies were shared.

@martintmk
Copy link
Contributor

As an example, why is the total request timeout policy inside/after the rate limit policy, when the rate limit policy can cause large delays if you enable queueing?

This is a very good point and you are correct. Total timeout should cancel executions queued by rate limiter.

We should change the order of strategies, total timeout should be on top. This is behavioral change, however, it's varanted.

@tyler-boyd Can you create an improvement or bug fix ticket please?

Cc @iliar-turdushev, @geeknoid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants