Skip to content

Consume DistributedContextPropagator APIs in DiagnosticsHandler #55392

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

Jump to bottom
Merged
merged 1 commit into from
Jul 13, 2021
Merged

Consume DistributedContextPropagator APIs in DiagnosticsHandler #55392

merged 1 commit into from
Jul 13, 2021

Conversation

@MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Jul 9, 2021 •
edited
Loading

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

cc: @tarekgh @noahfalk @shirhatti

@MihaZupan 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 MihaZupan added this to the 6.0.0 milestone Jul 9, 2021
@MihaZupan MihaZupan requested a review from a team July 9, 2021 10:09
@MihaZupan MihaZupan self-assigned this Jul 9, 2021
@ghost
Copy link

ghost commented Jul 9, 2021

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
Copy link

ghost commented Jul 9, 2021

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.

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
Copy link
Member

ManickaP commented Jul 9, 2021

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

@tarekgh
Copy link
Member

tarekgh commented Jul 9, 2021

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 MihaZupan force-pushed the diag-handler-propagators branch 2 times, most recently from 3d2d361 to f893939 Compare July 11, 2021 00:22
@MihaZupan
Copy link
Member Author

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

stephentoub
stephentoub reviewed Jul 12, 2021
View reviewed changes
noahfalk
noahfalk approved these changes Jul 12, 2021
View reviewed changes
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

LGTM

src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@tarekgh - making sure you see this : )

@MihaZupan MihaZupan force-pushed the diag-handler-propagators branch from 09e7902 to aa21a9a Compare July 12, 2021 23:38
@runfoapp runfoapp bot mentioned this pull request Jul 13, 2021
Closed
@MihaZupan MihaZupan changed the title Consume TextMapPropagator APIs in DiagnosticsHandler Consume DistributedContextPropagator APIs in DiagnosticsHandler Jul 13, 2021
@MihaZupan MihaZupan force-pushed the diag-handler-propagators branch from aa21a9a to 0828c85 Compare July 13, 2021 17:07
@MihaZupan MihaZupan removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 13, 2021
@MihaZupan MihaZupan force-pushed the diag-handler-propagators branch from 0828c85 to c6bd75e Compare July 13, 2021 17:58
ManickaP
ManickaP approved these changes Jul 13, 2021
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

LGTM!

src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs
using System.Security.Cryptography.X509Certificates;
using System.Threading;
using System.Threading.Tasks;
using System.Diagnostics;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
using System.Diagnostics;
using System.Diagnostics;

This was referenced Jul 13, 2021
Closed
Closed
@MihaZupan
Copy link
Member Author

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

@MihaZupan MihaZupan merged commit ccfe218 into dotnet:main Jul 13, 2021
@wfurt
Copy link
Member

wfurt commented Jul 13, 2021

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

cc: @CarnaViire

thaystg added a commit to thaystg/runtime that referenced this pull request Jul 14, 2021
@thaystg
Merge branch 'main' of github.com:thaystg/runtime into thays_support_…
a8fb8cf 
…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)
  ...
@ManickaP ManickaP mentioned this pull request Jul 20, 2021
Merged
@MihaZupan MihaZupan mentioned this pull request Aug 3, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub left review comments

@noahfalk noahfalk noahfalk approved these changes

@tarekgh tarekgh tarekgh left review comments

@ManickaP ManickaP ManickaP approved these changes

Assignees

@MihaZupan MihaZupan

Labels

area-System.Net.Http new-api-needs-documentation

Projects

None yet

Milestone

6.0.0

6 participants

@MihaZupan @ManickaP @tarekgh @wfurt @stephentoub @noahfalk