Expose wrapHandlersPipeline parameter in AddExtendedHttpClientLogging API #7231
Conversation
rainsxng
changed the title
There was a problem hiding this comment.
Pull request overview
This PR exposes the wrapHandlersPipeline parameter in the AddExtendedHttpClientLogging API, allowing callers to control whether the HTTP logging handler wraps the entire handler pipeline or is placed at the point of invocation. This addresses issue #5744 where latency context was being lost when retrieved outside the handlers pipeline, and implements the API proposal from issue #5924.
Changes:
- Added three new overloads to
AddExtendedHttpClientLoggingthat accept abool wrapHandlersPipelineparameter, matching the design of the runtime'sAddLoggermethod - Added comprehensive tests verifying that latency information is correctly populated with both
wrapHandlersPipeline: trueandfalsevalues - Updated the internal implementation to pass the
wrapHandlersPipelineparameter through to theAddLoggercall
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Libraries/Microsoft.Extensions.Http.Diagnostics/Logging/HttpClientLoggingHttpClientBuilderExtensions.cs | Adds three new public API overloads with wrapHandlersPipeline parameter and updates internal implementation to accept and pass through this parameter |
| test/Libraries/Microsoft.Extensions.Http.Diagnostics.Tests/Latency/HttpClientLatencyTelemetryExtensionsTest.cs | Adds four new tests verifying latency info population with different parameter combinations and when latency telemetry is not configured |
| [Experimental(diagnosticId: DiagnosticIds.Experiments.Telemetry, UrlFormat = DiagnosticIds.UrlFormat)] | ||
| public static IHttpClientBuilder AddExtendedHttpClientLogging(this IHttpClientBuilder builder, bool wrapHandlersPipeline) | ||
| { | ||
| _ = Throw.IfNull(builder); | ||
|
|
||
| return AddExtendedHttpClientLoggingInternal(builder, configureOptionsBuilder: null, wrapHandlersPipeline); | ||
| } |
There was a problem hiding this comment.
The new overload with wrapHandlersPipeline parameter is marked with [Experimental] attribute, but the existing overload without this parameter (line 35) is not, even though both can result in the same behavior (wrapHandlersPipeline: true). This creates an inconsistency where users calling AddExtendedHttpClientLogging(builder, true) get an experimental warning, but users calling AddExtendedHttpClientLogging(builder) (which also uses wrapHandlersPipeline: true internally) do not. Consider either: (1) marking all overloads as experimental for consistency, (2) removing the experimental attribute from the new overloads if the underlying functionality is stable, or (3) providing clear documentation explaining why only the new overloads are experimental.
| [Experimental(diagnosticId: DiagnosticIds.Experiments.Telemetry, UrlFormat = DiagnosticIds.UrlFormat)] | ||
| public static IHttpClientBuilder AddExtendedHttpClientLogging(this IHttpClientBuilder builder, IConfigurationSection section, bool wrapHandlersPipeline) | ||
| { | ||
| _ = Throw.IfNull(builder); | ||
| _ = Throw.IfNull(section); | ||
|
|
||
| return AddExtendedHttpClientLoggingInternal(builder, options => options.Bind(section), wrapHandlersPipeline); | ||
| } |
There was a problem hiding this comment.
Same inconsistency as the previous overload: the new overload with wrapHandlersPipeline is marked [Experimental], but the existing overload at line 78 (which calls the same internal method with wrapHandlersPipeline: true) is not. This means users will get different experimental warnings depending on which overload they call, even when the behavior is identical.
| [Experimental(diagnosticId: DiagnosticIds.Experiments.Telemetry, UrlFormat = DiagnosticIds.UrlFormat)] | ||
| public static IHttpClientBuilder AddExtendedHttpClientLogging(this IHttpClientBuilder builder, Action<LoggingOptions> configure, bool wrapHandlersPipeline) | ||
| { | ||
| _ = Throw.IfNull(builder); | ||
| _ = Throw.IfNull(configure); | ||
|
|
||
| return AddExtendedHttpClientLoggingInternal(builder, options => options.Configure(configure), wrapHandlersPipeline); | ||
| } |
There was a problem hiding this comment.
Same inconsistency as previous overloads: the new overload with wrapHandlersPipeline is marked [Experimental], but the existing overload at line 124 (which calls the same internal method with wrapHandlersPipeline: true) is not. This means users will get different experimental warnings depending on which overload they call, even when the behavior is identical.
...rosoft.Extensions.Http.Diagnostics.Tests/Latency/HttpClientLatencyTelemetryExtensionsTest.cs
Outdated
Show resolved
Hide resolved
…ency/HttpClientLatencyTelemetryExtensionsTest.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
As per following API review, these changes allow you to control the wrapHandlersPipeline variable in the AddExtendedHttpClientLogging API. This should fix a bug in which latency context is getting lost when retrieved outside a handlers pipeline
Microsoft Reviewers: Open in CodeFlow