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

Update HttpClient logging APIs per Noah's comments #4469

Merged
merged 1 commit into from
Sep 27, 2023
Merged

Update HttpClient logging APIs per Noah's comments #4469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 6 additions & 6 deletions bench/Libraries/Microsoft.Extensions.Http.Diagnostics.PerformanceTests/HttpClientFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public static System.Net.Http.HttpClient CreateWithLoggingLogRequest(string file
.AddSingleton(_ => NoRemoteCallHandler.Create(fileName))
.AddHttpClientLogEnricher<BenchEnricher>()
.AddHttpClient(nameof(fileName))
.AddHttpClientLogging(options =>
.AddExtendedHttpClientLogging(options =>
{
options.BodySizeLimit = readLimit;
options.RequestBodyContentTypes.Add(new("application/json"));
Expand All @@ -45,7 +45,7 @@ public static System.Net.Http.HttpClient CreateWithLoggingLogResponse(string fil
.AddFakeRedaction()
.AddSingleton(_ => NoRemoteCallHandler.Create(fileName))
.AddHttpClient(nameof(fileName))
.AddHttpClientLogging(options =>
.AddExtendedHttpClientLogging(options =>
{
options.BodySizeLimit = readLimit;
options.ResponseBodyContentTypes.Add(new("application/json"));
Expand All @@ -68,7 +68,7 @@ public static System.Net.Http.HttpClient CreateWithLoggingLogAll(string fileName
.AddFakeRedaction()
.AddSingleton(_ => NoRemoteCallHandler.Create(fileName))
.AddHttpClient(nameof(fileName))
.AddHttpClientLogging(options =>
.AddExtendedHttpClientLogging(options =>
{
options.BodySizeLimit = readLimit;

Expand All @@ -95,7 +95,7 @@ public static System.Net.Http.HttpClient CreateWithLoggingLogRequest_ChunkedEnco
.AddFakeRedaction()
.AddSingleton(_ => NoRemoteCallNotSeekableHandler.Create(fileName))
.AddHttpClient(nameof(fileName))
.AddHttpClientLogging(options =>
.AddExtendedHttpClientLogging(options =>
{
options.BodySizeLimit = readLimit;
options.RequestBodyContentTypes.Add("application/json");
Expand All @@ -118,7 +118,7 @@ public static System.Net.Http.HttpClient CreateWithLoggingLogResponse_ChunkedEnc
.AddFakeRedaction()
.AddSingleton(_ => NoRemoteCallNotSeekableHandler.Create(fileName))
.AddHttpClient(nameof(fileName))
.AddHttpClientLogging(options =>
.AddExtendedHttpClientLogging(options =>
{
options.BodySizeLimit = readLimit;
options.ResponseBodyContentTypes.Add("application/json");
Expand All @@ -141,7 +141,7 @@ public static System.Net.Http.HttpClient CreateWithLoggingLogAll_ChunkedEncoding
.AddFakeRedaction()
.AddSingleton(_ => NoRemoteCallNotSeekableHandler.Create(fileName))
.AddHttpClient(nameof(fileName))
.AddHttpClientLogging(options =>
.AddExtendedHttpClientLogging(options =>
{
options.BodySizeLimit = readLimit;

Expand Down
10 changes: 5 additions & 5 deletions ...ies/Microsoft.Extensions.Http.Diagnostics/Latency/HttpClientLatencyTelemetryExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public static class HttpClientLatencyTelemetryExtensions
/// </remarks>
/// <param name="services">The <see cref="IServiceCollection" />.</param>
/// <returns>The value of <paramref name="services"/>.</returns>
public static IServiceCollection AddDefaultHttpClientLatencyTelemetry(this IServiceCollection services)
public static IServiceCollection AddHttpClientLatencyTelemetry(this IServiceCollection services)
{
_ = Throw.IfNull(services);

Expand Down Expand Up @@ -62,14 +62,14 @@ public static IServiceCollection AddDefaultHttpClientLatencyTelemetry(this IServ
[UnconditionalSuppressMessage("Trimming",
"IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code",
Justification = "Addressed with [DynamicDependency]")]
public static IServiceCollection AddDefaultHttpClientLatencyTelemetry(this IServiceCollection services, IConfigurationSection section)
public static IServiceCollection AddHttpClientLatencyTelemetry(this IServiceCollection services, IConfigurationSection section)
{
_ = Throw.IfNull(section);

_ = services
.Configure<HttpClientLatencyTelemetryOptions>(section);

return services.AddDefaultHttpClientLatencyTelemetry();
return services.AddHttpClientLatencyTelemetry();
}

/// <summary>
Expand All @@ -81,13 +81,13 @@ public static IServiceCollection AddDefaultHttpClientLatencyTelemetry(this IServ
/// <param name="services">The <see cref="IServiceCollection" />.</param>
/// <param name="configure">The delegate to configure <see cref="HttpClientLatencyTelemetryOptions"/> with.</param>
/// <returns>The value of <paramref name="services"/>.</returns>
public static IServiceCollection AddDefaultHttpClientLatencyTelemetry(this IServiceCollection services, Action<HttpClientLatencyTelemetryOptions> configure)
public static IServiceCollection AddHttpClientLatencyTelemetry(this IServiceCollection services, Action<HttpClientLatencyTelemetryOptions> configure)
{
_ = Throw.IfNull(configure);

_ = services
.Configure(configure);

return services.AddDefaultHttpClientLatencyTelemetry();
return services.AddHttpClientLatencyTelemetry();
}
}
28 changes: 11 additions & 17 deletions ...osoft.Extensions.Http.Diagnostics/Logging/HttpClientLoggingHttpClientBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,36 +16,32 @@
namespace Microsoft.Extensions.DependencyInjection;

/// <summary>
/// Extension methods to register HTTP client logging feature.
/// Extensions to register extended HTTP client logging features.
/// </summary>
public static class HttpClientLoggingHttpClientBuilderExtensions
{
/// <summary>
/// Adds an <see cref="IHttpClientAsyncLogger" /> to emit logs for outgoing requests for a named <see cref="HttpClient"/>.
/// </summary>
/// <param name="builder">The <see cref="IHttpClientBuilder" />.</param>
/// <returns>
/// An <see cref="IHttpClientBuilder" /> that can be used to configure the client.
/// </returns>
/// <returns>The value of <paramref name="builder"/>.</returns>
/// <remarks>
/// All other loggers are removed - including the default one, registered via <see cref="HttpClientBuilderExtensions.AddDefaultLogger(IHttpClientBuilder)"/>.
/// </remarks>
/// <exception cref="ArgumentNullException">Argument <paramref name="builder"/> is <see langword="null"/>.</exception>
public static IHttpClientBuilder AddHttpClientLogging(this IHttpClientBuilder builder)
public static IHttpClientBuilder AddExtendedHttpClientLogging(this IHttpClientBuilder builder)
{
_ = Throw.IfNull(builder);

return AddNamedClientLoggingInternal(builder);
return AddExtendedHttpClientLoggingInternal(builder);
}

/// <summary>
/// Adds an <see cref="IHttpClientAsyncLogger" /> to emit logs for outgoing requests for a named <see cref="HttpClient"/>.
/// </summary>
/// <param name="builder">The <see cref="IHttpClientBuilder" />.</param>
/// <param name="section">The <see cref="IConfigurationSection"/> to use for configuring <see cref="LoggingOptions"/>.</param>
/// <returns>
/// An <see cref="IHttpClientBuilder" /> that can be used to configure the client.
/// </returns>
/// <returns>The value of <paramref name="builder"/>.</returns>
/// <remarks>
/// All other loggers are removed - including the default one, registered via <see cref="HttpClientBuilderExtensions.AddDefaultLogger(IHttpClientBuilder)"/>.
/// </remarks>
Expand All @@ -54,22 +50,20 @@ public static IHttpClientBuilder AddHttpClientLogging(this IHttpClientBuilder bu
[UnconditionalSuppressMessage("Trimming",
"IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code",
Justification = "Addressed with [DynamicDependency]")]
public static IHttpClientBuilder AddHttpClientLogging(this IHttpClientBuilder builder, IConfigurationSection section)
public static IHttpClientBuilder AddExtendedHttpClientLogging(this IHttpClientBuilder builder, IConfigurationSection section)
{
_ = Throw.IfNull(builder);
_ = Throw.IfNull(section);

return AddNamedClientLoggingInternal(builder, options => options.Bind(section));
return AddExtendedHttpClientLoggingInternal(builder, options => options.Bind(section));
}

/// <summary>
/// Adds an <see cref="IHttpClientAsyncLogger" /> to emit logs for outgoing requests for a named <see cref="HttpClient"/>.
/// </summary>
/// <param name="builder">The <see cref="IHttpClientBuilder" />.</param>
/// <param name="configure">The delegate to configure <see cref="LoggingOptions"/> with.</param>
/// <returns>
/// An <see cref="IHttpClientBuilder" /> that can be used to configure the client.
/// </returns>
/// <returns>The value of <paramref name="builder"/>.</returns>
/// <remarks>
/// All other loggers are removed - including the default one, registered via <see cref="HttpClientBuilderExtensions.AddDefaultLogger(IHttpClientBuilder)"/>.
/// </remarks>
Expand All @@ -78,15 +72,15 @@ public static IHttpClientBuilder AddHttpClientLogging(this IHttpClientBuilder bu
[UnconditionalSuppressMessage("Trimming",
"IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code",
Justification = "Addressed with [DynamicDependency]")]
public static IHttpClientBuilder AddHttpClientLogging(this IHttpClientBuilder builder, Action<LoggingOptions> configure)
public static IHttpClientBuilder AddExtendedHttpClientLogging(this IHttpClientBuilder builder, Action<LoggingOptions> configure)
{
_ = Throw.IfNull(builder);
_ = Throw.IfNull(configure);

return AddNamedClientLoggingInternal(builder, options => options.Configure(configure));
return AddExtendedHttpClientLoggingInternal(builder, options => options.Configure(configure));
}

private static IHttpClientBuilder AddNamedClientLoggingInternal(IHttpClientBuilder builder, Action<OptionsBuilder<LoggingOptions>>? configureOptionsBuilder = null)
private static IHttpClientBuilder AddExtendedHttpClientLoggingInternal(IHttpClientBuilder builder, Action<OptionsBuilder<LoggingOptions>>? configureOptionsBuilder = null)
{
var optionsBuilder = builder.Services
.AddOptionsWithValidateOnStart<LoggingOptions, LoggingOptionsValidator>(builder.Name);
Expand Down
12 changes: 6 additions & 6 deletions ...osoft.Extensions.Http.Diagnostics/Logging/HttpClientLoggingServiceCollectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
namespace Microsoft.Extensions.DependencyInjection;

/// <summary>
/// Extension methods to register HTTP client logging features.
/// Extensions to register extended HTTP client logging features.
/// </summary>
public static class HttpClientLoggingServiceCollectionExtensions
{
Expand All @@ -28,7 +28,7 @@ public static class HttpClientLoggingServiceCollectionExtensions
/// All other loggers are removed - including the default one, registered via <see cref="HttpClientBuilderExtensions.AddDefaultLogger(IHttpClientBuilder)"/>.
/// </remarks>
/// <exception cref="ArgumentNullException">Argument <paramref name="services"/> is <see langword="null"/>.</exception>
public static IServiceCollection AddDefaultHttpClientLogging(this IServiceCollection services)
public static IServiceCollection AddExtendedHttpClientLogging(this IServiceCollection services)
Copy link
Contributor

Choose a reason for hiding this comment

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

So, now both names are the same?
IServiceCollection.AddExtendedHttpClientLogging and IHttpClientBuilder.AddExtendedHttpClientLogging
Wouldn't it confuse users?

Copy link
Member

Choose a reason for hiding this comment

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

IMO as long as serviceCollection.AddExtendedHttpClientLogging() has the same behavior as services.ConfigureHttpClientDefaults(httpBuilder => httpBuilder.AddExtendedHttpClientLogging()) then I wouldn't expect significant confusion. Worst case if a customer was aware of both and confused by it there is still no wrong answer, whichever one they try will work.

@davidfowl I think has stronger opinions that we should avoid offering IServiceCollection extension methods when a sub-builder extension method also exists. Personally I'm neutral about this pattern, but assuming I didn't misunderstand his position, he'd probably want us not to include the IServiceCollection variant.

{
_ = Throw.IfNull(services);

Expand Down Expand Up @@ -62,7 +62,7 @@ public static IServiceCollection AddDefaultHttpClientLogging(this IServiceCollec
[UnconditionalSuppressMessage("Trimming",
"IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code",
Justification = "Addressed with [DynamicDependency]")]
public static IServiceCollection AddDefaultHttpClientLogging(this IServiceCollection services, IConfigurationSection section)
public static IServiceCollection AddExtendedHttpClientLogging(this IServiceCollection services, IConfigurationSection section)
{
_ = Throw.IfNull(services);
_ = Throw.IfNull(section);
Expand All @@ -71,7 +71,7 @@ public static IServiceCollection AddDefaultHttpClientLogging(this IServiceCollec
.AddOptionsWithValidateOnStart<LoggingOptions, LoggingOptionsValidator>()
.Bind(section);

return services.AddDefaultHttpClientLogging();
return services.AddExtendedHttpClientLogging();
}

/// <summary>
Expand All @@ -84,7 +84,7 @@ public static IServiceCollection AddDefaultHttpClientLogging(this IServiceCollec
/// All other loggers are removed - including the default one, registered via <see cref="HttpClientBuilderExtensions.AddDefaultLogger(IHttpClientBuilder)"/>.
/// </remarks>
/// <exception cref="ArgumentNullException">Any of the arguments is <see langword="null"/>.</exception>
public static IServiceCollection AddDefaultHttpClientLogging(this IServiceCollection services, Action<LoggingOptions> configure)
public static IServiceCollection AddExtendedHttpClientLogging(this IServiceCollection services, Action<LoggingOptions> configure)
{
_ = Throw.IfNull(services);
_ = Throw.IfNull(configure);
Expand All @@ -93,7 +93,7 @@ public static IServiceCollection AddDefaultHttpClientLogging(this IServiceCollec
.AddOptionsWithValidateOnStart<LoggingOptions, LoggingOptionsValidator>()
.Configure(configure);

return services.AddDefaultHttpClientLogging();
return services.AddExtendedHttpClientLogging();
}

/// <summary>
Expand Down
18 changes: 9 additions & 9 deletions ...ibraries/Microsoft.Extensions.Http.Diagnostics/Microsoft.Extensions.Http.Diagnostics.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@
"Stage": "Stable",
"Methods": [
{
"Member": "static Microsoft.Extensions.DependencyInjection.IServiceCollection Microsoft.Extensions.DependencyInjection.HttpClientLatencyTelemetryExtensions.AddDefaultHttpClientLatencyTelemetry(this Microsoft.Extensions.DependencyInjection.IServiceCollection services);",
"Member": "static Microsoft.Extensions.DependencyInjection.IServiceCollection Microsoft.Extensions.DependencyInjection.HttpClientLatencyTelemetryExtensions.AddHttpClientLatencyTelemetry(this Microsoft.Extensions.DependencyInjection.IServiceCollection services);",
"Stage": "Stable"
},
{
"Member": "static Microsoft.Extensions.DependencyInjection.IServiceCollection Microsoft.Extensions.DependencyInjection.HttpClientLatencyTelemetryExtensions.AddDefaultHttpClientLatencyTelemetry(this Microsoft.Extensions.DependencyInjection.IServiceCollection services, Microsoft.Extensions.Configuration.IConfigurationSection section);",
"Member": "static Microsoft.Extensions.DependencyInjection.IServiceCollection Microsoft.Extensions.DependencyInjection.HttpClientLatencyTelemetryExtensions.AddHttpClientLatencyTelemetry(this Microsoft.Extensions.DependencyInjection.IServiceCollection services, Microsoft.Extensions.Configuration.IConfigurationSection section);",
"Stage": "Stable"
},
{
"Member": "static Microsoft.Extensions.DependencyInjection.IServiceCollection Microsoft.Extensions.DependencyInjection.HttpClientLatencyTelemetryExtensions.AddDefaultHttpClientLatencyTelemetry(this Microsoft.Extensions.DependencyInjection.IServiceCollection services, System.Action<Microsoft.Extensions.Http.Latency.HttpClientLatencyTelemetryOptions> configure);",
"Member": "static Microsoft.Extensions.DependencyInjection.IServiceCollection Microsoft.Extensions.DependencyInjection.HttpClientLatencyTelemetryExtensions.AddHttpClientLatencyTelemetry(this Microsoft.Extensions.DependencyInjection.IServiceCollection services, System.Action<Microsoft.Extensions.Http.Latency.HttpClientLatencyTelemetryOptions> configure);",
"Stage": "Stable"
}
]
Expand All @@ -40,15 +40,15 @@
"Stage": "Stable",
"Methods": [
{
"Member": "static Microsoft.Extensions.DependencyInjection.IHttpClientBuilder Microsoft.Extensions.DependencyInjection.HttpClientLoggingHttpClientBuilderExtensions.AddHttpClientLogging(this Microsoft.Extensions.DependencyInjection.IHttpClientBuilder builder);",
"Member": "static Microsoft.Extensions.DependencyInjection.IHttpClientBuilder Microsoft.Extensions.DependencyInjection.HttpClientLoggingHttpClientBuilderExtensions.AddExtendedHttpClientLogging(this Microsoft.Extensions.DependencyInjection.IHttpClientBuilder builder);",
"Stage": "Stable"
},
{
"Member": "static Microsoft.Extensions.DependencyInjection.IHttpClientBuilder Microsoft.Extensions.DependencyInjection.HttpClientLoggingHttpClientBuilderExtensions.AddHttpClientLogging(this Microsoft.Extensions.DependencyInjection.IHttpClientBuilder builder, Microsoft.Extensions.Configuration.IConfigurationSection section);",
"Member": "static Microsoft.Extensions.DependencyInjection.IHttpClientBuilder Microsoft.Extensions.DependencyInjection.HttpClientLoggingHttpClientBuilderExtensions.AddExtendedHttpClientLogging(this Microsoft.Extensions.DependencyInjection.IHttpClientBuilder builder, Microsoft.Extensions.Configuration.IConfigurationSection section);",
"Stage": "Stable"
},
{
"Member": "static Microsoft.Extensions.DependencyInjection.IHttpClientBuilder Microsoft.Extensions.DependencyInjection.HttpClientLoggingHttpClientBuilderExtensions.AddHttpClientLogging(this Microsoft.Extensions.DependencyInjection.IHttpClientBuilder builder, System.Action<Microsoft.Extensions.Http.Logging.LoggingOptions> configure);",
"Member": "static Microsoft.Extensions.DependencyInjection.IHttpClientBuilder Microsoft.Extensions.DependencyInjection.HttpClientLoggingHttpClientBuilderExtensions.AddExtendedHttpClientLogging(this Microsoft.Extensions.DependencyInjection.IHttpClientBuilder builder, System.Action<Microsoft.Extensions.Http.Logging.LoggingOptions> configure);",
"Stage": "Stable"
}
]
Expand All @@ -58,15 +58,15 @@
"Stage": "Stable",
"Methods": [
{
"Member": "static Microsoft.Extensions.DependencyInjection.IServiceCollection Microsoft.Extensions.DependencyInjection.HttpClientLoggingServiceCollectionExtensions.AddDefaultHttpClientLogging(this Microsoft.Extensions.DependencyInjection.IServiceCollection services);",
"Member": "static Microsoft.Extensions.DependencyInjection.IServiceCollection Microsoft.Extensions.DependencyInjection.HttpClientLoggingServiceCollectionExtensions.AddExtendedHttpClientLogging(this Microsoft.Extensions.DependencyInjection.IServiceCollection services);",
"Stage": "Stable"
},
{
"Member": "static Microsoft.Extensions.DependencyInjection.IServiceCollection Microsoft.Extensions.DependencyInjection.HttpClientLoggingServiceCollectionExtensions.AddDefaultHttpClientLogging(this Microsoft.Extensions.DependencyInjection.IServiceCollection services, Microsoft.Extensions.Configuration.IConfigurationSection section);",
"Member": "static Microsoft.Extensions.DependencyInjection.IServiceCollection Microsoft.Extensions.DependencyInjection.HttpClientLoggingServiceCollectionExtensions.AddExtendedHttpClientLogging(this Microsoft.Extensions.DependencyInjection.IServiceCollection services, Microsoft.Extensions.Configuration.IConfigurationSection section);",
"Stage": "Stable"
},
{
"Member": "static Microsoft.Extensions.DependencyInjection.IServiceCollection Microsoft.Extensions.DependencyInjection.HttpClientLoggingServiceCollectionExtensions.AddDefaultHttpClientLogging(this Microsoft.Extensions.DependencyInjection.IServiceCollection services, System.Action<Microsoft.Extensions.Http.Logging.LoggingOptions> configure);",
"Member": "static Microsoft.Extensions.DependencyInjection.IServiceCollection Microsoft.Extensions.DependencyInjection.HttpClientLoggingServiceCollectionExtensions.AddExtendedHttpClientLogging(this Microsoft.Extensions.DependencyInjection.IServiceCollection services, System.Action<Microsoft.Extensions.Http.Logging.LoggingOptions> configure);",
"Stage": "Stable"
},
{
Expand Down
Loading