Consume DistributedContextPropagator APIs in DiagnosticsHandler #55392

MihaZupan · 2021-07-09T10:09:14Z

Replace InjectHeaders logic in DiagnosticsHandler with a call to DistributedContextPropagator.Inject which adds a lot of configurability to the user
- Using PassThroughPropagator or a custom propagator addresses Add an option to HttpClient's default DiagnosticHandler so that it does not create an Activity #41072
- Using NoOutputPropagator addresses HttpClient automatically adds Request-Id HTTP header #35337
Insert DiagnosticsHandler into the SocketsHttpHandler handler chain
- This lights up distributed tracing for customized SocketsHttpHandler instances Customized SocketsHttpHandler does not support diagnostics tracking #31261
- Placing the DiagnosticsHandler before RedirectHandler lights up propagation for redirect requests HttpHandlerDiagnosticListener does not send second chain of events if the request was autoredirected on netfx in W3C mode #40777

Side effects:

By moving DiagnosticsHandler into SocketsHttpHandler, any custom instance lights up for distributed tracing when moving to 6.0 by default
- Some people don't want that, but there is now an easier off switch available (NoOutputPropagator or the ActivityHeadersPropagator property on the handler)
- The behaviour is now the same regardless of whether a SocketsHttpHandler instance was created on its own / within HttpClientHandler.

Fixes #35337
Fixes #41072
Fixes #40777
Fixes #31261
Fixes #55556

dotnet-issue-labeler · 2021-07-09T10:09:18Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-07-09T10:09:24Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Opening the PR early to avoid blocking on the official implementation of #50658.

Replace InjectHeaders logic in DiagnosticsHandler with a call to TextMapPropagator.Inject which adds a lot of configurability to the user
- Using PassThroughPropagator or a custom propagator addresses Add an option to HttpClient's default DiagnosticHandler so that it does not create an Activity #41072
- Using NoOutputPropagator addresses HttpClient automatically adds Request-Id HTTP header #35337
Insert DiagnosticsHandler into the SocketsHttpHandler handler chain
- This lights up distributed tracing for customized SocketsHttpHandler instances Customized SocketsHttpHandler does not support diagnostics tracking #31261
- Placing the DiagnosticsHandler before RedirectHandler lights up propagation for redirect requests HttpHandlerDiagnosticListener does not send second chain of events if the request was autoredirected on netfx in W3C mode #40777

Side effects:

By moving DiagnosticsHandler into SocketsHttpHandler, any custom instance lights up for distributed tracing when moving to 6.0 by default
- Some people don't want that, but there is now an easier off switch available (NoOutputPropagator or the TextMapPropagator property on the handler)
- The behaviour is now the same regardless of whether a SocketsHttpHandler instance was created on its own / within HttpClientHandler.

No-merge:
Includes a minimal copy of APIs approved in #50658 and the unapproved TextMapPropagator property on SocketsHttpHandler.

Fixes #35337
Fixes #41072
Fixes #40777
Fixes #31261

cc: @tarekgh @noahfalk @shirhatti

Author:	MihaZupan
Assignees:	MihaZupan
Labels:	`* NO MERGE *`, `area-System.Net.Http`
Milestone:	6.0.0

ManickaP · 2021-07-09T12:57:29Z

Just FYI, #55384 is another PR touching ClientHttpHandler ATM. There might or might not be some clashes.

tarekgh · 2021-07-09T18:35:18Z

Opening the PR early to avoid blocking on the official implementation of #50658.

We have now the PR #55419 but it is a good idea to unblock yourself till you get the official changes.

MihaZupan · 2021-07-11T00:31:52Z

Updated to use the actual implementation now that #55419 is merged.

src/libraries/System.Diagnostics.DiagnosticSource/tests/PropagatorTests.cs

src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs

src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpHandler/SocketsHttpHandler.cs

src/libraries/System.Net.Http/tests/FunctionalTests/DiagnosticsTests.cs

src/libraries/System.Net.Http/ref/System.Net.Http.cs

src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs

src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs

noahfalk

LGTM

noahfalk · 2021-07-12T22:54:49Z

src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs

@@ -58,6 +86,16 @@ internal static bool IsEnabled()
                throw new ArgumentNullException(nameof(request), SR.net_http_handler_norequest);
            }

+            // Since we are reusing the request message instance on redirects, clear any existing headers
+            // Do so before writing DiagnosticListener events as instrumentations use those to inject headers
+            if (request.WasRedirected() && _propagatorFields is HeaderDescriptor[] fields)


As I recall OpenTelemetry defines Fields as a hint about which fields would be inspected but not a guarantee. In particular they left the door open that some protocol would have field names dynamically determined so it would be impossible to provide an exhaustive list. Of course in practice I am not aware of any propagator implementation that does this.

If we want to do this we should probably clarify in our API comments that our definition of Fields is strict and we don't support the dynamic shenanigans that OT left the door open to.

If we want to do this we should probably clarify in our API comments that our definition of Fields is strict and we don't support the dynamic shenanigans that OT left the door open to.

That would be my preference.

I can't think of a different approach that wouldn't break some scenarios. For example doing the check inside the setter callback and removing the header there instead is too late since instrumentations like AI/Otel may try to add headers before us - we really have to clear the headers in advance.

@tarekgh - making sure you see this : )

src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs

ManickaP

LGTM!

ManickaP · 2021-07-13T18:05:06Z

src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs

@@ -9,6 +9,7 @@
 using System.Security.Cryptography.X509Certificates;
 using System.Threading;
 using System.Threading.Tasks;
+using System.Diagnostics;


Suggested change

using System.Diagnostics;

using System.Diagnostics;

MihaZupan · 2021-07-13T23:41:48Z

QuicStreamTests_MsQuicProvider.ReadOutstanding_ReadAborted_Throws is the only test that failed and it also failed on other PRs.

wfurt · 2021-07-13T23:54:07Z

QuicStreamTests_MsQuicProvider.ReadOutstanding_ReadAborted_Throws is the only test that failed and it also failed on other PRs.

cc: @CarnaViire

…debugger_custom_views * 'main' of github.com:thaystg/runtime: (125 commits) [wasm] [debugger] Support method calls (dotnet#55458) [debugger] Fix debugging after hot reloading (dotnet#55599) Inliner: Extend IL limit for profiled call-sites, allow inlining for switches. (dotnet#55478) DiagnosticSourceEventSource supports base class properties (dotnet#55613) [mono] Fix race during mono_image_storage_open (dotnet#55201) [mono] Add wrapper info for native func wrappers. (dotnet#55602) H/3 and Quic AppContext switch (dotnet#55332) Compression.ZipFile support for Unix Permissions (dotnet#55531) [mono] Fix skipping of static methods during IMT table construction. (dotnet#55610) Combine System.Private.Xml TrimmingTests projects (dotnet#55606) fix name conflict with Configuration class (dotnet#55597) Finish migrating RSAOpenSsl from RSA* to EVP_PKEY* Disable generic math (dotnet#55540) Obsolete CryptoConfig.EncodeOID (dotnet#55592) Address System.Net.Http.WinHttpHandler's nullable warnings targeting .NETCoreApp (dotnet#54995) Enable Http2_MultipleConnectionsEnabled_ConnectionLimitNotReached_ConcurrentRequestsSuccessfullyHandled (dotnet#55572) Fix Task.WhenAny failure mode when passed ICollection of zero tasks (dotnet#55580) Consume DistributedContextPropagator in DiagnosticsHandler (dotnet#55392) Add property ordering feature (dotnet#55586) Reduce subtest count in Reflection (dotnet#55537) ...

MihaZupan added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-System.Net.Http labels Jul 9, 2021

MihaZupan added this to the 6.0.0 milestone Jul 9, 2021

MihaZupan requested a review from a team July 9, 2021 10:09

MihaZupan self-assigned this Jul 9, 2021

dotnet-issue-labeler bot added the new-api-needs-documentation label Jul 9, 2021

MihaZupan force-pushed the diag-handler-propagators branch 2 times, most recently from 3d2d361 to f893939 Compare July 11, 2021 00:22