dotnet / extensions

This repository contains a suite of libraries that provide facilities commonly needed when creating production-ready applications.
MIT License
2.57k stars 737 forks source link

Currently impossible to override the AddStandardREsillienceHandler() call for a specific client which means that you either use it or can't. #4814

Open JohnGalt1717 opened 8 months ago

JohnGalt1717 commented 8 months ago

Description

I debated if this was a bug or a feature request, but since Aspire sets this by the default (which it should) and then every single HttpClient you create then is forced into those defaults with apparently no way to override them, this becomes a bug because it's both not discoverable with what's happening and the documentation says nothing about this, while simultaneously adding a new resilience handler to a specific client implementation doesn't override it.

Hence, at best, it's a bug in documentation, but more, this is a major blocker, because it's impossible to set anything other than defaults as soon as you opt into this, which I can't imagine was the design intent.

Reproduction Steps

If this is in your project.

        builder.Services.ConfigureHttpClientDefaults(http => {
            //Turn on resilience by default
            http.AddStandardResilienceHandler();

            // Turn on service discovery by default
            http.UseServiceDiscovery();
        });

The timeout on any client that you try and override will be ignored.

This doesn't work.

services.AddHttpClient<IKaiLLamaClient, LLamaKaiLLM>()
.AddStandardResilienceHandler(options => {
    options.AttemptTimeout = new HttpTimeoutStrategyOptions {
        Timeout = TimeSpan.FromMinutes(5)
    };
    options.TotalRequestTimeout = new HttpTimeoutStrategyOptions {
        Timeout = TimeSpan.FromMinutes(15)
    };
    options.CircuitBreaker.SamplingDuration = TimeSpan.FromMinutes(10);
});

This doesn't work: (where timeoutPolicy is a polly timeout policy that sets the time out)

builder.Services.AddHttpClient<IOrca2LLamaClient, LLamaOrca2LLM>(client => {
    client.Timeout = TimeSpan.FromMinutes(5);
})
.AddPolicyHandler(timeoutPolicy);

I've tried every permutation and combination I can think of, and in all cases, it is ignored.

Expected behavior

You should be able to override the defaults on a specific client and how to do so should be documented clearly.

This should just work:

            services.AddHttpClient<IKaiLLamaClient, LLamaKaiLLM>()
                .AddStandardResilienceHandler(options => {
                    options.AttemptTimeout = new HttpTimeoutStrategyOptions {
                        Timeout = TimeSpan.FromMinutes(5)
                    };
                    options.TotalRequestTimeout = new HttpTimeoutStrategyOptions {
                        Timeout = TimeSpan.FromMinutes(15)
                    };
                    options.CircuitBreaker.SamplingDuration = TimeSpan.FromMinutes(10);
                });

Or there should be an OverrideResilienceHandler that replaces it for this client or something.

Actual behavior

It appears to just add a new resiliance handler that is suplimental to the other one, so whatever one times out first, kills the client.

Regression?

No response

Known Workarounds

None other than removing all default resiliance handling for http.

Configuration

.NET SDK: Version: 8.0.100 Commit: 57efcf1350 Workload version: 8.0.100-manifests.71b9f198

Runtime Environment: OS Name: Windows OS Version: 10.0.22631 OS Platform: Windows RID: win-x64 Base Path: C:\Program Files\dotnet\sdk\8.0.100\

.NET workloads installed: Workload version: 8.0.100-manifests.71b9f198 [aspire] Installation Source: SDK 8.0.100, VS 17.9.34321.82 Manifest Version: 8.0.0-preview.1.23557.2/8.0.100 Manifest Path: C:\Program Files\dotnet\sdk-manifests\8.0.100\microsoft.net.sdk.aspire\8.0.0-preview.1.23557.2\WorkloadManifest.json Install Type: Msi

[maui-windows] Installation Source: VS 17.8.34330.188 Manifest Version: 8.0.3/8.0.100 Manifest Path: C:\Program Files\dotnet\sdk-manifests\8.0.100\microsoft.net.sdk.maui\8.0.3\WorkloadManifest.json Install Type: Msi

[maccatalyst] Installation Source: VS 17.8.34330.188 Manifest Version: 17.0.8478/8.0.100 Manifest Path: C:\Program Files\dotnet\sdk-manifests\8.0.100\microsoft.net.sdk.maccatalyst\17.0.8478\WorkloadManifest.json Install Type: Msi

[ios] Installation Source: VS 17.8.34330.188 Manifest Version: 17.0.8478/8.0.100 Manifest Path: C:\Program Files\dotnet\sdk-manifests\8.0.100\microsoft.net.sdk.ios\17.0.8478\WorkloadManifest.json Install Type: Msi

[android] Installation Source: VS 17.8.34330.188 Manifest Version: 34.0.43/8.0.100 Manifest Path: C:\Program Files\dotnet\sdk-manifests\8.0.100\microsoft.net.sdk.android\34.0.43\WorkloadManifest.json
Install Type: Msi

Host: Version: 8.0.0 Architecture: x64 Commit: 5535e31a71

.NET SDKs installed: 6.0.417 [C:\Program Files\dotnet\sdk] 7.0.404 [C:\Program Files\dotnet\sdk] 8.0.100 [C:\Program Files\dotnet\sdk]

.NET runtimes installed: Microsoft.AspNetCore.App 6.0.25 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 7.0.14 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 8.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.NETCore.App 6.0.25 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 7.0.14 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 8.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.WindowsDesktop.App 6.0.25 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 7.0.14 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 8.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Other architectures found: x86 [C:\Program Files (x86)\dotnet] registered at [HKLM\SOFTWARE\dotnet\Setup\InstalledVersions\x86\InstallLocation]

Environment variables: Not set

global.json file: E:\Repos\Atteria\Pltfrmd\services\global.json

Learn more: https://aka.ms/dotnet/info

Download .NET: https://aka.ms/dotnet/download

Other information

No response

meinsiedler commented 5 months ago

Additionally to what @JohnGalt1717 suggests: It would also help us if we could override the resiliency settings for one single endpoint of a typed http client.

The use case for this is that we have a typed http client with some endpoints where we want to use the standard resiliency settings. But we have one single endpoint in that typed http client where we know that this endpoint takes longer than the configured AttemptTimeout (which is totally fine). We have no possibility - as I'm aware of - to override the default behavior for this single endpoint.

silkfire commented 5 months ago

For specific endpoints I'd probably create a separate client registration. You can still reuse a lot of logic by creating an extension method.

meinsiedler commented 5 months ago

@silkfire I forgot to mention that our typed http client is generated via NSwag and represents a whole API. The typed http client even comes as separate NuGet package, which we cannot influence.

It would be quite cumbersome to split up the client registrations just for having different resiliency settings and it also feels like a leaky abstraction to have two typed http clients just to differeniate between those two resiliency settings. The calling code, which then uses one of the two typed clients, would then need to be aware of which client to use for which endpoint.

I would rather prefer to have specific configuration possiblities per endpoint when registering the typed http client and re-use the same typed http client everywhere.

silkfire commented 5 months ago

@meinsiedler I just realized you can add a strategy conditionally by configuring the Retry.ShouldHandle predicate:

httpClientBuilder.AddStandardResilienceHandler(o =>
                                              {
                                                  o.Retry.ShouldHandle = args => args.Outcome switch
                                                  {
                                                     { Result.RequestMessage: not null } outcome when outcome.Result.RequestMessage.RequestUri != null && outcome.Result.RequestMessage.RequestUri.AbsolutePath.EndsWith("/exclude-endpoint-from-retry") => PredicateResult.False(),
                                                     var outcome when HttpClientResiliencePredicates.IsTransient(outcome) => PredicateResult.True(),
                                                     { Exception: InvalidOperationException } => PredicateResult.True(),
                                                     _ => PredicateResult.False()
                                                  };
                                              });
joperezr commented 4 months ago

@martintmk could you take a look into this issue? Is this a case were we do have a way to configure this but it is not discoverable, or is there no good way of overriding the defaults? If the latter, could it be possible for us to add a feature to support this? As pointed out in the original post, this particularly a problem in Aspire as it adds these by default to both the ServiceDefaults, as well as the scenario tests project.

iliar-turdushev commented 3 months ago

The issue is caused by the following behavior #101719 of ConfigureHttpClientDefaults method. Because of the behavior the method calls ConfigureHttpClientDefaults(x => x.AddStandardResiliencePipeline()) and AddHttpClient<T>().AddStandardResiliencePipline() register two distinct resilience pipelines in the HttpClient request pipeline. We need to figure out whether the issue is with ConfigureHttpClientDefaults. If not, and ConfigureHttpClientDefaults is not going to be updated/fixed to support our scenario, then we'll need to find out what to do with AddStandardResiliencePipeline.

CarnaViire commented 2 months ago

we'll need to find out what to do with AddStandardResiliencePipeline.

@iliar-turdushev I've put some ideas in a comment here https://github.com/dotnet/runtime/issues/101719#issuecomment-2163609832