From 21ba7a0678869eaefec7ede1d7aee057bb1b813f Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Wed, 14 Jun 2023 09:34:26 +0800 Subject: [PATCH] Refactor removal of built-in HttpClient logging handler (#4060) --- .../Logging/HttpClientLoggingExtensions.cs | 81 ++++++++++++++++--- .../HttpClientLoggingExtensionsTest.cs | 29 +++++++ 2 files changed, 101 insertions(+), 9 deletions(-) diff --git a/src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/HttpClientLoggingExtensions.cs b/src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/HttpClientLoggingExtensions.cs index 64ad5ec45ea..8e783670cf5 100644 --- a/src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/HttpClientLoggingExtensions.cs +++ b/src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/HttpClientLoggingExtensions.cs @@ -2,12 +2,13 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; using System.Linq; 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; @@ -25,6 +26,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(); + /// /// Adds a to collect and emit logs for outgoing requests for all http clients. /// @@ -42,8 +45,9 @@ public static IServiceCollection AddDefaultHttpClientLogging(this IServiceCollec _ = services .AddHttpRouteProcessor() .AddHttpHeadersRedactor() - .AddOutgoingRequestContext() - .RemoveAll(); + .AddOutgoingRequestContext(); + + AddBuiltInLoggingRemoverFilter(services, name: null); services.TryAddActivatedSingleton(); services.TryAddActivatedSingleton(); @@ -135,8 +139,9 @@ public static IHttpClientBuilder AddHttpClientLogging(this IHttpClientBuilder bu _ = builder.Services .AddHttpRouteProcessor() .AddHttpHeadersRedactor() - .AddOutgoingRequestContext() - .RemoveAll(); + .AddOutgoingRequestContext(); + + AddBuiltInLoggingRemoverFilter(builder.Services, builder.Name); builder.Services.TryAddActivatedSingleton(); builder.Services.TryAddActivatedSingleton(); @@ -177,8 +182,9 @@ public static IHttpClientBuilder AddHttpClientLogging(this IHttpClientBuilder bu _ = builder.Services .AddHttpRouteProcessor() .AddHttpHeadersRedactor() - .AddOutgoingRequestContext() - .RemoveAll(); + .AddOutgoingRequestContext(); + + AddBuiltInLoggingRemoverFilter(builder.Services, builder.Name); builder.Services.TryAddActivatedSingleton(); builder.Services.TryAddActivatedSingleton(); @@ -211,8 +217,9 @@ public static IHttpClientBuilder AddHttpClientLogging(this IHttpClientBuilder bu _ = builder.Services .AddHttpRouteProcessor() .AddHttpHeadersRedactor() - .AddOutgoingRequestContext() - .RemoveAll(); + .AddOutgoingRequestContext(); + + AddBuiltInLoggingRemoverFilter(builder.Services, builder.Name); builder.Services.TryAddActivatedSingleton(); builder.Services.TryAddActivatedSingleton(); @@ -261,4 +268,60 @@ private static Func ConfigureHandler(IHttpC loggingOptions); }; } + + private static void AddBuiltInLoggingRemoverFilter(IServiceCollection services, string? name) + { + // 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); + } + + _ = services.Configure(o => o.ClientNames.Add(name)); + } + + private sealed class BuiltInLoggerRemoverFilterOptions + { + // Names of clients to remove built-in logging from. + // A null value means built-in logging is removed globally from clients. + public HashSet ClientNames { get; } = new HashSet(); + } + + private sealed class BuiltInLoggingRemoverFilter : IHttpMessageHandlerBuilderFilter + { + private readonly BuiltInLoggerRemoverFilterOptions _options; + private readonly bool _global; + + [SuppressMessage("Major Code Smell", "S1144:Unused private types or members should be removed", Justification = "This constructor is used by dependency injection.")] + public BuiltInLoggingRemoverFilter(IOptions options) + { + _options = options.Value; + _global = _options.ClientNames.Contains(null); + } + + public Action Configure(Action next) + { + return (builder) => + { + // Run other configuration first, we want to decorate. + next(builder); + + if (_global || _options.ClientNames.Contains(builder.Name)) + { + // 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); + } + } + } + }; + } + } } diff --git a/test/Libraries/Microsoft.Extensions.Http.Telemetry.Tests/Logging/HttpClientLoggingExtensionsTest.cs b/test/Libraries/Microsoft.Extensions.Http.Telemetry.Tests/Logging/HttpClientLoggingExtensionsTest.cs index ca62bef4c5c..83774b3075c 100644 --- a/test/Libraries/Microsoft.Extensions.Http.Telemetry.Tests/Logging/HttpClientLoggingExtensionsTest.cs +++ b/test/Libraries/Microsoft.Extensions.Http.Telemetry.Tests/Logging/HttpClientLoggingExtensionsTest.cs @@ -834,6 +834,35 @@ public async Task AddHttpClientLogging_DisablesNetScope() logRecord.Scopes.Should().HaveCount(0); } + [Fact] + public async Task AddHttpClientLogging_CallFromOtherClient_HasBuiltInLogging() + { + const string RequestPath = "https://we.wont.hit.this.dd22anyway.com"; + await using var provider = new ServiceCollection() + .AddFakeLogging() + .AddFakeRedaction() + .AddHttpClient("test") + .AddHttpClientLogging() + .Services + .AddHttpClient("normal") + .Services + .BlockRemoteCall() + .BuildServiceProvider(); + + // The test client has AddHttpClientLogging. The normal client doesn't. + // The normal client should still log via the builtin HTTP logging. + var client = provider.GetRequiredService().CreateClient("normal"); + using var httpRequestMessage = new HttpRequestMessage(HttpMethod.Get, new Uri(RequestPath)); + + _ = await client.SendAsync(httpRequestMessage, HttpCompletionOption.ResponseHeadersRead).ConfigureAwait(false); + var collector = provider.GetFakeLogCollector(); + var logRecords = collector.GetSnapshot().Where(l => l.Category == "System.Net.Http.HttpClient.normal.LogicalHandler").ToList(); + + Assert.Collection(logRecords, + r => Assert.Equal("RequestPipelineStart", r.Id.Name), + r => Assert.Equal("RequestPipelineEnd", r.Id.Name)); + } + [Fact] public async Task AddDefaultHttpClientLogging_DisablesNetScope() {