Refactor removal of built-in HttpClient logging handler

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<IHttpMessageHandlerBuilderFilter, BuiltInLoggingRemoverFilter>();
+
     /// <summary>
     /// Adds a <see cref="DelegatingHandler" /> to collect and emit logs for outgoing requests for all http clients.
     /// </summary>
@@ -42,8 +45,9 @@ public static IServiceCollection AddDefaultHttpClientLogging(this IServiceCollec
         _ = services
             .AddHttpRouteProcessor()
             .AddHttpHeadersRedactor()
-            .AddOutgoingRequestContext()
-            .RemoveAll<IHttpMessageHandlerBuilderFilter>();
+            .AddOutgoingRequestContext();
+
+        AddBuiltInLoggingRemoverFilter(services, name: null);
 
         services.TryAddActivatedSingleton<IHttpRequestReader, HttpRequestReader>();
         services.TryAddActivatedSingleton<IHttpHeadersReader, HttpHeadersReader>();
@@ -135,8 +139,9 @@ public static IHttpClientBuilder AddHttpClientLogging(this IHttpClientBuilder bu
         _ = builder.Services
             .AddHttpRouteProcessor()
             .AddHttpHeadersRedactor()
-            .AddOutgoingRequestContext()
-            .RemoveAll<IHttpMessageHandlerBuilderFilter>();
+            .AddOutgoingRequestContext();
+
+        AddBuiltInLoggingRemoverFilter(builder.Services, builder.Name);
 
         builder.Services.TryAddActivatedSingleton<IHttpRequestReader, HttpRequestReader>();
         builder.Services.TryAddActivatedSingleton<IHttpHeadersReader, HttpHeadersReader>();
@@ -177,8 +182,9 @@ public static IHttpClientBuilder AddHttpClientLogging(this IHttpClientBuilder bu
         _ = builder.Services
             .AddHttpRouteProcessor()
             .AddHttpHeadersRedactor()
-            .AddOutgoingRequestContext()
-            .RemoveAll<IHttpMessageHandlerBuilderFilter>();
+            .AddOutgoingRequestContext();
+
+        AddBuiltInLoggingRemoverFilter(builder.Services, builder.Name);
 
         builder.Services.TryAddActivatedSingleton<IHttpRequestReader, HttpRequestReader>();
         builder.Services.TryAddActivatedSingleton<IHttpHeadersReader, HttpHeadersReader>();
@@ -211,8 +217,9 @@ public static IHttpClientBuilder AddHttpClientLogging(this IHttpClientBuilder bu
         _ = builder.Services
             .AddHttpRouteProcessor()
             .AddHttpHeadersRedactor()
-            .AddOutgoingRequestContext()
-            .RemoveAll<IHttpMessageHandlerBuilderFilter>();
+            .AddOutgoingRequestContext();
+
+        AddBuiltInLoggingRemoverFilter(builder.Services, builder.Name);
 
         builder.Services.TryAddActivatedSingleton<IHttpRequestReader, HttpRequestReader>();
         builder.Services.TryAddActivatedSingleton<IHttpHeadersReader, HttpHeadersReader>();
@@ -261,4 +268,60 @@ private static Func<IServiceProvider, DelegatingHandler> 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<BuiltInLoggerRemoverFilterOptions>(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<string?> ClientNames { get; } = new HashSet<string?>();
+    }
+
+    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<BuiltInLoggerRemoverFilterOptions> options)
+        {
+            _options = options.Value;
+            _global = _options.ClientNames.Contains(null);
+        }
+
+        public Action<HttpMessageHandlerBuilder> Configure(Action<HttpMessageHandlerBuilder> 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);
+                        }
+                    }
+                }
+            };
+        }
+    }
 }

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<IHttpClientFactory>().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()
     {