dotnet / dotnet/extensions

[API Proposal]: Allow HttpClientLoggerHandler in different places of the handler pipeline

Open
#5,924 4 comments 2 reactions 1 assignee Assigned to @rainsxng View on GitHub
api-approved api-suggestion area-telemetry
Dominant language
C#
Stars
3.2k
Forks
894
Avg merge
1d 12h
Merged PRs (30d)
23

Description

### Background and motivation

Currently the set of `AddExtendedHttpClientLogging` methods is eventually calling the `AddLogger` method with `wrapHandlersPipeline == true`, as visible [here](https://github.com/dotnet/extensions/blob/49f36066e4e9a244211e2135ceb0a5760bf60f2e/src/Libraries/Microsoft.Extensions.Http.Diagnostics/Logging/HttpClientLoggingHttpClientBuilderExtensions.cs#L96). This forces the `HttpClientLoggerHandler` to be added to the top of the handler chain, as visible [here](https://github.com/dotnet/runtime/blob/6e06aef86f90cc867ade4be48ed8cddea5a3905d/src/libraries/Microsoft.Extensions.Http/src/DependencyInjection/HttpClientBuilderExtensions.Logging.cs#L60).

This does not fit use cases where all the outgoing requests from the HttpClient pipeline should be tracked. If people for example would want to log Polly resilience handler retries. I would like to therefore propose new overloads for the extension methods which would surface the wrapHandlersPipeline bool parameter. This way any caller could select which behavior is desired.

### API Proposal

```diff
namespace Microsoft.Extensions.DependencyInjection;

public static class HttpClientLoggingHttpClientBuilderExtensions
{
// Existing methods.
// They register the HttpClientLoggingHandler as the top-most handler in the pipeline.
public static IHttpClientBuilder AddExtendedHttpClientLogging(this IHttpClientBuilder builder);
public static IHttpClientBuilder AddExtendedHttpClientLogging(this IHttpClientBuilder builder, IConfigurationSection section);
public static IHttpClientBuilder AddExtendedHttpClientLogging(this IHttpClientBuilder builder, Action configure);

// New methods have a new parameter "bool wrapHandlersPipeline".
// When wrapHandlersPipeline = true the HttpClientLoggingHandler is registered as the top-most handler.
// When wrapHandlersPipeline = true it is registered at the place of the method invocation.
+ public static IHttpClientBuilder AddExtendedHttpClientLogging(this IHttpClientBuilder builder, bool wrapHandlersPipeline);
+ public static IHttpClientBuilder AddExtendedHttpClientLogging(this IHttpClientBuilder builder, IConfigurationSection section, bool wrapHandlersPipeline);
+ public static IHttpClientBuilder AddExtendedHttpClientLogging(this IHttpClientBuilder builder, Action configure, bool wrapHandlersPipeline);
}
```

This design matches the approach used for registering HttpClientLogger in dotnet/runtime. See the [`AddLogger`](https://github.com/dotnet/runtime/blob/94b6d05ad5e6fffe1d213305a9bc938c2545e9a2/src/libraries/Microsoft.Extensions.Http/src/DependencyInjection/HttpClientBuilderExtensions.Logging.cs#L46) method.

There’s also a set of extension methods for `IServiceCollection` that registers `HttpClientLoggingHandler` for all `HttpClient` instances. These methods use the [ConfigureHttpClientDefaults](https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.dependencyinjection.httpclientfactoryservicecollectionextensions.configurehttpclientdefaults?view=net-9.0-pp) method under the hood, which causes the logging handler to be registered before any delegating handlers added for a named HttpClient. We won’t add overloads with the `wrapHandlersPipeline` parameter for these methods, as that would make their behavior confusing. It’s better to keep these methods as they are and register `HttpClientLoggingHandler` as the top-most handler in the pipeline. The code sample below shows scenarios where this could lead to confusing behavior. Adding the wrapHandlersPipeline parameter to these methods would also expose the implementation detail that they rely on ConfigureHttpClientDefaults internally.

```csharp
services.ConfigureHttpClientDefaults(builder => builder.AddSomeHandler("A"));

services.AddHttpClient("client-1").AddSomeHandler("C1");

// The logging handler will be registered as part of default HttpClient configuration. It means that it would be put between
// handlers A and B.
// For named clients client-1 and client-2, it will be put before the first handler of this clients. It doesn't matter where the
// AddExtendedHttpClientLogging(IServiceCollection) is invoked. The final handler configuration for client-1 and client-2 will be the following:
// client-1: A -- LoggingHandler -- B -- C1
// client-2: A -- LoggingHandler -- B -- C2
//
// The behavior that might be not obvious or confusing:
// - AddExtendedHttpClientLogging uses ConfigureHttpClientDefaults internally.
// As a result, the logging handler is inserted between handlers registered
// in earlier and later calls to ConfigureHttpClientDefaults.
// - For named HttpClient registrations it is different. The logging handler will always be added before the first handler of any named HttpClient configuraiton.
services.AddExtendedHttpClientLogging(wrapHandlersPipeline: false);

services.AddHttpClient("client-2").AddSomeHandler("C2");

services.ConfigureHttpClientDefaults(builder => builder.AddSomeHandler("B"));
```

However, if we decide that the `IServiceCollection` extension methods will always register it as the top-most handler, then this flag would have no effect in those methods. This is a downside we'll need to properly document.

### API Usage

```csharp
services
.AddHttpClient("client")
.AddExtendedHttpClientLogging(wrapHandlersPipeline: false);
```

### Risks

The new methods (with the `wrapHandlersPipeline` parameter) would either make the existing ones (without the parameter) unusable or they always be used with the `wrapHandlersPipeline` parameter set to `false`.

### Alternative Designs

```diff
namespace Microsoft.Extensions.Http.Logging;

public class LoggingOptions
{
+ ///
+ /// Gets or sets a value indicating whether to register the logging handler as the top-most handler in the pipeline.
+ ///
+ public bool WrapHandlersPipeline { get; set; } = true;
}
```

Instead of adding a set of overloads, we can introduce a flag that controls whether `HttpClientLoggingHandler` should be registered as the top-most handler in the pipeline.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.