Skip to content

Commit

Permalink
Refactor removal of built-in HttpClient logging handler (#4060)
Browse files Browse the repository at this point in the history
  • Loading branch information
@JamesNK
JamesNK authored Jun 14, 2023
1 parent c0155a5 commit 21ba7a0
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 9 deletions.
81 changes: 72 additions & 9 deletions src/Libraries/Microsoft.Extensions.Http.Telemetry/Logging/HttpClientLoggingExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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>
Expand All @@ -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>();
Expand Down Expand Up @@ -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>();
Expand Down Expand Up @@ -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>();
Expand Down Expand Up @@ -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>();
Expand Down Expand Up @@ -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);
}
}
}
};
}
}
}
29 changes: 29 additions & 0 deletions ...ries/Microsoft.Extensions.Http.Telemetry.Tests/Logging/HttpClientLoggingExtensionsTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down

0 comments on commit 21ba7a0

Please sign in to comment.