Expose wrapHandlersPipeline parameter in AddExtendedHttpClientLogging API by rainsxng · Pull Request #7231 · dotnet/extensions

rainsxng · 2026-01-26T15:47:00Z

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

Copilot

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 AddExtendedHttpClientLogging that accept a bool wrapHandlersPipeline parameter, matching the design of the runtime's AddLogger method
Added comprehensive tests verifying that latency information is correctly populated with both wrapHandlersPipeline: true and false values
Updated the internal implementation to pass the wrapHandlersPipeline parameter through to the AddLogger call

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

Copilot · 2026-01-27T13:14:49Z

...icrosoft.Extensions.Http.Diagnostics/Logging/HttpClientLoggingHttpClientBuilderExtensions.cs

+    [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);
    }


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.

@rainsxng Can you, please, test whether this comment is correct or not? If it is correct than introducing API with a default parameter wrapHandlersPipeline = true might be a breaking change, meaning it could break pipelines of our customers. They'll unintentionally switch from a stable method without the bool flag to the experimental methods with the flag. The issue can be easily fixed by suppressing the warning, but it will break our customers first.

It seems that Copilot's comment is invalid. I did this simple test, and the IL code shows that if you call a method without default parameter and there is an overload with default parameter the compiler prefers the method with default parameter:

Also, according to C# spec a method without a default parameter is considered a better choice by the compiler:

@iliar-turdushev that API is approved and we can get it merged without Experimental attribute

and the IL code shows that if you call a method without default parameter and there is an overload with default parameter the compiler prefers the method with default parameter:

in this case how can I call the method without default parameter? is it possible at all?

But original API always had a default parameter, so we should be able to call it with new overloads

I mean, if we have methods

public static IHttpClientBuilder AddExtendedHttpClientLogging(this IHttpClientBuilder builder, bool wrapHandlersPipeline = true); public static IHttpClientBuilder AddExtendedHttpClientLogging(this IHttpClientBuilder builder);

then any call like builder.AddExtendedHttpClientLogging() will go to the first overload. Is there a way to call the second overload?

It is possible. We can use OverloadResolutionPriorityAttribute, see usage sample here. It is available in C# 13.

But what is the point to call the first overload instead of the second?

But what is the point to call the first overload instead of the second?

The point is that it is public API and must be callable. I would also think of deprecating it

talked to @rainsxng offline, I have no more question from my side in this thread

Copilot · 2026-01-27T13:14:50Z

...icrosoft.Extensions.Http.Diagnostics/Logging/HttpClientLoggingHttpClientBuilderExtensions.cs

+    [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);
    }


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.

Copilot · 2026-01-27T13:14:50Z

...icrosoft.Extensions.Http.Diagnostics/Logging/HttpClientLoggingHttpClientBuilderExtensions.cs

+    [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);
    }


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

…ency/HttpClientLatencyTelemetryExtensionsTest.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

...icrosoft.Extensions.Http.Diagnostics/Logging/HttpClientLoggingHttpClientBuilderExtensions.cs

iliar-turdushev · 2026-02-04T11:09:36Z

...icrosoft.Extensions.Http.Diagnostics/Logging/HttpClientLoggingHttpClientBuilderExtensions.cs

+    /// </summary>
+    /// <param name="builder">The <see cref="IHttpClientBuilder" />.</param>
+    /// <param name="wrapHandlersPipeline">
+    /// Determines the logging behavior when resilience strategies (like retries or hedging) are configured.


While "resilience strategies+HttpClient logging" is an important use case, it is not the only use case for putting HttpClient logging into different places in the request pipeline. I think we should document wrapHandlersPipeline parameter in more generic way:

when true - the logger is at the beginning of the pipeline, meaning it "wraps" all the handlers in the request pipeline

when false - the logger is put at the end of the pipeline right before the primary request handler.

And then we can elaborate what effect this parameter has when using with such a resilience strategy as retry attempts.

What do you think?

+1, we don't need to mention any resilience stuff here

Agree, will change that

iliar-turdushev · 2026-02-04T13:20:22Z

...rosoft.Extensions.Http.Diagnostics.Tests/Latency/HttpClientLatencyTelemetryExtensionsTest.cs

+    }
+
+    [Fact]
+    public async Task LatencyInfo_IsNotPresent_WhenLatencyTelemetryNotAdded()


I think we don't need this test, because it doesn't test HttpClient Latency. What about removing it?

iliar-turdushev · 2026-02-04T13:25:32Z

...rosoft.Extensions.Http.Diagnostics.Tests/Latency/HttpClientLatencyTelemetryExtensionsTest.cs

+    }
+
+    [Fact]
+    public async Task LatencyInfo_IsPopulated_WithConfigurationSection_AndWrapHandlersPipeline()


In my opinion, we don't need LatencyInfo_IsPopulated_WithConfigurationSection_AndWrapHandlersPipeline and LatencyInfo_IsPopulated_WithActionConfiguration_WhenLoggerDoesNotWrapHandlersPipeline tests here, because the current tests set is focused on HttpClient Latency.

Removed both of them

iliar-turdushev · 2026-02-04T13:28:54Z

...rosoft.Extensions.Http.Diagnostics.Tests/Latency/HttpClientLatencyTelemetryExtensionsTest.cs

 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.

 using System;


This test set is for HttpClient Latency, but we are adding new methods to the HttpClientLoggingHttpClientBuilderExtensions. We need to add tests for new methods to the https://github.com/dotnet/extensions/blob/main/test/Libraries/Microsoft.Extensions.Http.Diagnostics.Tests/Logging/HttpClientLoggingExtensionsTest.cs.

iliar-turdushev · 2026-02-04T13:35:59Z

...rosoft.Extensions.Http.Diagnostics.Tests/Latency/HttpClientLatencyTelemetryExtensionsTest.cs

    }

+    [Fact]
+    public async Task LatencyInfo_IsPopulated_WhenLoggerWrapsHandlersPipeline()


As I understand, this test ensures that the argument wrapsHandlersPipeline helps to fix this issue #5744. Is my understanding correct? Then I would expect that the test sets wrapHandlersPipeline=false. I'm wondering why the latency info is correctly emitted when when wrapHandlersPipeline=true.

Yeah, it's strange that it passes, but it's likely related to the pipeline order. Original bug was failing with

.AddLatencyContext() .AddExtendedHttpClientLogging() .AddHttpClientLatencyTelemetry();

And this test has it a bit different:

.AddLatencyContext() .AddHttpClientLatencyTelemetry() .AddHttpClient("test") .ConfigurePrimaryHttpMessageHandler(() => new ServerNameStubHandler("TestServer")) .AddExtendedHttpClientLogging(wrapHandlersPipeline: true)

So this tests checks a bit different pipeline order, will update test name to reflect it better

Updated test names, should be better now

evgenyfedorov2 · 2026-02-05T13:06:55Z

...icrosoft.Extensions.Http.Diagnostics/Logging/HttpClientLoggingHttpClientBuilderExtensions.cs

+    /// </summary>
+    /// <param name="builder">The <see cref="IHttpClientBuilder" />.</param>
+    /// <param name="wrapHandlersPipeline">
+    /// Determines the logging behavior when resilience strategies (like retries or hedging) are configured.


+1, we don't need to mention any resilience stuff here

evgenyfedorov2 · 2026-02-05T13:08:42Z

...icrosoft.Extensions.Http.Diagnostics/Logging/HttpClientLoggingHttpClientBuilderExtensions.cs

+    /// you have a way of viewing structured logs in order to view this extra information.
+    /// </remarks>
+    /// <exception cref="ArgumentNullException">Any of the arguments is <see langword="null"/>.</exception>
+    [Experimental(diagnosticId: DiagnosticIds.Experiments.Telemetry, UrlFormat = DiagnosticIds.UrlFormat)]


is our plan to release new overloads as Experimental and then remove that attribute in a release after?

evgenyfedorov2 · 2026-02-05T13:14:37Z

...aries/Microsoft.Extensions.Http.Diagnostics.Tests/Logging/HttpClientLoggingExtensionsTest.cs

we need to have a test for wrapHandlersPipeline (without latency, latency tests should live in their own test file). Probably it is not mockable, one way to test it is end to end similar to this https://github.com/dotnet/runtime/blob/f6b4b6ce93a8dce234a93e461060c648865dae3f/src/libraries/Microsoft.Extensions.Http/tests/Microsoft.Extensions.Http.Tests/Logging/HttpClientLoggerTest.cs#L329

rainsxng added 2 commits January 26, 2026 16:40

new api surface, first test iteration

19a2a6c

fix tests

930c40d

rainsxng self-assigned this Jan 26, 2026

github-actions bot added the area-telemetry label Jan 26, 2026

rainsxng and others added 2 commits January 26, 2026 17:17

add experimental attributes

3da8f98

Merge branch 'main' into dbohdanov/diagnostics-latecy-api-update

4373f93

rainsxng changed the title ~~latency api updates~~ Expose wrapHandlersPipeline parameter in AddExtendedHttpClientLogging API Jan 27, 2026

rainsxng marked this pull request as ready for review January 27, 2026 13:04

rainsxng requested a review from a team as a code owner January 27, 2026 13:04

Copilot AI review requested due to automatic review settings January 27, 2026 13:04

Copilot started reviewing on behalf of rainsxng January 27, 2026 13:05 View session

rainsxng mentioned this pull request Jan 27, 2026

How do I use AddHttpClientLatencyTelemetry? #5744

Open

rainsxng requested a review from evgenyfedorov2 January 27, 2026 13:11

Copilot AI reviewed Jan 27, 2026

View reviewed changes

Update test/Libraries/Microsoft.Extensions.Http.Diagnostics.Tests/Lat…

4018e1a

…ency/HttpClientLatencyTelemetryExtensionsTest.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

iliar-turdushev self-requested a review January 28, 2026 13:51

Merge branch 'main' into dbohdanov/diagnostics-latecy-api-update

e3134ec

evgenyfedorov2 reviewed Feb 3, 2026

View reviewed changes

...icrosoft.Extensions.Http.Diagnostics/Logging/HttpClientLoggingHttpClientBuilderExtensions.cs Show resolved Hide resolved

...icrosoft.Extensions.Http.Diagnostics/Logging/HttpClientLoggingHttpClientBuilderExtensions.cs Show resolved Hide resolved

iliar-turdushev reviewed Feb 4, 2026

View reviewed changes

rainsxng added 2 commits February 4, 2026 15:53

move and remove tests

e669fef

fix warnings

25674fb

evgenyfedorov2 reviewed Feb 5, 2026

View reviewed changes

Conversation

rainsxng commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iliar-turdushev Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rainsxng commented Jan 26, 2026 •

edited

Loading

iliar-turdushev Feb 5, 2026 •

edited

Loading