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.

Sign up for GitHub

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

Jump to bottom

Refactor removal of built-in HttpClient logging handler #4060

Merged
merged 5 commits into from
Jun 14, 2023
Merged

Refactor removal of built-in HttpClient logging handler #4060

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4eab5a4
Refactor removal of built-in HttpClient logging handler
JamesNK Jun 9, 2023
edf0c6f
Fix build
JamesNK Jun 9, 2023
1e244de
Support removing logging from selected clients
JamesNK Jun 13, 2023
fdc57b1
Fix test
JamesNK Jun 14, 2023
9f14af0
Improve test
JamesNK Jun 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 49 additions & 9 deletions src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/HttpClientLoggingExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
using System.Net.Http;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Extensions;
using Microsoft.Extensions.Http.Logging;
using Microsoft.Extensions.Http.Telemetry.Logging.Internal;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
Expand All @@ -25,6 +25,8 @@ public static class HttpClientLoggingExtensions
internal static readonly string HandlerAddedTwiceExceptionMessage =
$"{typeof(HttpLoggingHandler)} was already added either to all HttpClientBuilder's or to the current instance of {typeof(IHttpClientBuilder)}.";

private static readonly ServiceDescriptor _removeDefaultLoggingFilterDescriptor = ServiceDescriptor.Singleton<IHttpMessageHandlerBuilderFilter, BuiltInLoggerRemoverFilter>();

/// <summary>
/// Adds a <see cref="DelegatingHandler" /> to collect and emit logs for outgoing requests for all http clients.
/// </summary>
Expand All @@ -42,8 +44,9 @@ public static IServiceCollection AddDefaultHttpClientLogging(this IServiceCollec
_ = services
.AddHttpRouteProcessor()
.AddHttpHeadersRedactor()
.AddOutgoingRequestContext()
.RemoveAll<IHttpMessageHandlerBuilderFilter>();
.AddOutgoingRequestContext();

AddBuiltInLoggerRemoverFilter(services);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is part of the plan to change the naming or behavior of dotnet/extension logging? Perhaps that is for another PR/prototype but I think we should either:

  • Change the naming to make it clear these methods are opting into an alternative strategy. For example UseMinimalHttpLogging()
  • Change the behavior of these methods so that prior to configuring any LoggingOptions they match the existing HttpClient logging output. For example there might be a LoggingOptions.LoggingMode property with enum options that are either Classic or Light. By default it would be Classic and have all the existing behavior, and Light has the new behavior that logs much less. Hopefully all (or most) of the other options can compose regardless whether Classic or Light has been chosen.
  • Do something like what ASP.NET Core logging is doing where the built-in logging gains the configurability and dotnet/extensions is layering specific scoped features on top like redaction and enrichment. The APIs would then be named based on the specific capability being added.

As-is the naming and user experience of these methods feels confusing to me. There is already default http logging behavior that comes with AddHttpClient() so if this API has any effect then it must be adding non-default behavior despite the "default" in the name.

Copy link
Member Author

@JamesNK JamesNK Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the Default in the extension method name AddDefaultHttpClientLogging refers to how this change is applied to all HTTP clients by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that was probably the intent, but I think it would be pretty easy for our developers not to understand that. Also even if they do interpret 'default' in that way it still leaves the question "I already have Http logging behavior before I called this API, so what behavior is this API adding?"

If our hands are tied and we have to have this API named as-is we can certainly try to mitigate with docs, but it doesn't feel nice to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @noahfalk, the current naming here is super confusing. And if e.g. later we in BCL will go forward with API similar to what I proposed in dotnet/runtime#85840, then we'd have both AddDefaultHttpClientLogging and AddHttpClientDefaults().ConfigureLogging(o => o.AddDefaultProviders()) and it would be really hard to tell the difference and which to use (and too many defaults 😄). (And BCL either won't be able to adopt AddHttpClientLogging name for our API or we'd have conflicting names). I also agree that "default" associates more strongly with the logging we have in BCL (behavior that comes with AddHttpClient()).

Do something like what ASP.NET Core logging is doing where the built-in logging gains the configurability and dotnet/extensions is layering specific scoped features on top like redaction and enrichment. The APIs would then be named based on the specific capability being added.

This would be the ideal solution IMO. I wonder if we'd have enough time and capacity to do that (in this release at least), but I would love to go with that way.

But regardless, I think it should be either clear from the API name that that's the alternative logging, or it all should be as one big layered logging solution.


services.TryAddActivatedSingleton<IHttpRequestReader, HttpRequestReader>();
services.TryAddActivatedSingleton<IHttpHeadersReader, HttpHeadersReader>();
Expand Down Expand Up @@ -135,8 +138,9 @@ public static IHttpClientBuilder AddHttpClientLogging(this IHttpClientBuilder bu
_ = builder.Services
.AddHttpRouteProcessor()
.AddHttpHeadersRedactor()
.AddOutgoingRequestContext()
.RemoveAll<IHttpMessageHandlerBuilderFilter>();
.AddOutgoingRequestContext();

AddBuiltInLoggerRemoverFilter(builder.Services);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this disable the logging for this specific builder, or for all builders? I think this fixes the removing other non-logging filters, but it still has an issue that if R9 registers logging for one HttpClientBuilder it disables logging for all of them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It disables the built-in logging for all builders.

The feedback at dotnet/runtime#85840 was that it should be disabled everywhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think dotnet/runtime#85840 has only talked about the global case when R9 has both global and per-clientbuilder scenarios. IMO if someone called AddHttpClientLogging(this IHttpClientBuilder builder, ...) to use the new logging for one client builder they are not expecting built-in logging for all other unrelated client builders to be disabled. Does that sound right or I am missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geeknoid @dpk83

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see what you mean. I added support for selective disable.


builder.Services.TryAddActivatedSingleton<IHttpRequestReader, HttpRequestReader>();
builder.Services.TryAddActivatedSingleton<IHttpHeadersReader, HttpHeadersReader>();
Expand Down Expand Up @@ -177,8 +181,9 @@ public static IHttpClientBuilder AddHttpClientLogging(this IHttpClientBuilder bu
_ = builder.Services
.AddHttpRouteProcessor()
.AddHttpHeadersRedactor()
.AddOutgoingRequestContext()
.RemoveAll<IHttpMessageHandlerBuilderFilter>();
.AddOutgoingRequestContext();

AddBuiltInLoggerRemoverFilter(builder.Services);

builder.Services.TryAddActivatedSingleton<IHttpRequestReader, HttpRequestReader>();
builder.Services.TryAddActivatedSingleton<IHttpHeadersReader, HttpHeadersReader>();
Expand Down Expand Up @@ -211,8 +216,9 @@ public static IHttpClientBuilder AddHttpClientLogging(this IHttpClientBuilder bu
_ = builder.Services
.AddHttpRouteProcessor()
.AddHttpHeadersRedactor()
.AddOutgoingRequestContext()
.RemoveAll<IHttpMessageHandlerBuilderFilter>();
.AddOutgoingRequestContext();

AddBuiltInLoggerRemoverFilter(builder.Services);

builder.Services.TryAddActivatedSingleton<IHttpRequestReader, HttpRequestReader>();
builder.Services.TryAddActivatedSingleton<IHttpHeadersReader, HttpHeadersReader>();
Expand Down Expand Up @@ -261,4 +267,38 @@ private static Func<IServiceProvider, DelegatingHandler> ConfigureHandler(IHttpC
loggingOptions);
};
}

private static void AddBuiltInLoggerRemoverFilter(IServiceCollection services)
{
// We want to remove default logging. To do that we need to modify the builder after the filter that adds logging runs.
// To do that we use another filter that runs after LoggingHttpMessageHandlerBuilderFilter. This is done by inserting
// our filter to the service collection as the first item. That ensures it is in the right position when resolving
// IHttpMessageHandlerBuilderFilter instances. It doesn't matter if AddHttpClient is called before or after.
if (!services.Contains(_removeDefaultLoggingFilterDescriptor))
{
services.Insert(0, _removeDefaultLoggingFilterDescriptor);
}
}

private sealed class BuiltInLoggerRemoverFilter : IHttpMessageHandlerBuilderFilter
{
public Action<HttpMessageHandlerBuilder> Configure(Action<HttpMessageHandlerBuilder> next)
{
return (builder) =>
{
// Run other configuration first, we want to decorate.
next(builder);

// Remove the logger handlers added by the filter. Fortunately, they're both public, so it is a simple test on the type.
for (var i = builder.AdditionalHandlers.Count - 1; i >= 0; i--)
{
var handlerType = builder.AdditionalHandlers[i].GetType();
if (handlerType == typeof(LoggingScopeHttpMessageHandler) || handlerType == typeof(LoggingHttpMessageHandler))
{
builder.AdditionalHandlers.RemoveAt(i);
}
}
};
}
}
}