Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Add ActivitySource support to DiagnosticsHandler#54437

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
MihaZupan merged 6 commits intodotnet:mainfromMihaZupan:diag-handler
Jun 24, 2021

Conversation

@MihaZupan
Copy link
Member

Today,DiagnosticsHandler will create a newActivity if either is true:

  • AnActivity was already set (Activity.Current is not null)
  • ADiagnosticListener requested one (was enabled for the activity name)

This PR preserves the same behaviour when noActivityListener is present.

The behaviour changesiff all of the following hold:

  • AnActivity was already set (Activity.Current was not null)
  • NoDiagnosticListener was enabled for the activity name
  • ActivitySource had listeners present
  • Listeners decided not to sample the request

In this case, noActivity is created (and so we also won't inject context headers).
If aDiagnosticListener is enabled for the activity, one is always created (regardless of sampling decisions).

The PR moves the decision of whether to log events/create an activity out of theSendAsync call into

// The interesting part of the PR - this method decides on the actual behaviourstaticboolShouldLogDiagnostics(HttpRequestMessagerequest,outActivity?activity)

I also removed the internalSettings andLoggingStrings classes - am I missing something and they actually provide value?

Note that the PR is only a slight refactor + addingActivitySource. It does not address other knownDiagnosticsHandler issues.
This is runtime's equivalent of whatdotnet/aspnetcore#30089 was for AspNet.

Opening as a draft so the diagnostics team can confirm whether the above is the desired behaviour withActivitySource.
cc:@shirhatti@tarekgh@noahfalk

@ghost
Copy link

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

Issue Details

Today,DiagnosticsHandler will create a newActivity if either is true:

  • AnActivity was already set (Activity.Current is not null)
  • ADiagnosticListener requested one (was enabled for the activity name)

This PR preserves the same behaviour when noActivityListener is present.

The behaviour changesiff all of the following hold:

  • AnActivity was already set (Activity.Current was not null)
  • NoDiagnosticListener was enabled for the activity name
  • ActivitySource had listeners present
  • Listeners decided not to sample the request

In this case, noActivity is created (and so we also won't inject context headers).
If aDiagnosticListener is enabled for the activity, one is always created (regardless of sampling decisions).

The PR moves the decision of whether to log events/create an activity out of theSendAsync call into

// The interesting part of the PR - this method decides on the actual behaviourstaticboolShouldLogDiagnostics(HttpRequestMessagerequest,outActivity?activity)

I also removed the internalSettings andLoggingStrings classes - am I missing something and they actually provide value?

Note that the PR is only a slight refactor + addingActivitySource. It does not address other knownDiagnosticsHandler issues.
This is runtime's equivalent of whatdotnet/aspnetcore#30089 was for AspNet.

Opening as a draft so the diagnostics team can confirm whether the above is the desired behaviour withActivitySource.
cc:@shirhatti@tarekgh@noahfalk

Author:MihaZupan
Assignees:-
Labels:

area-System.Net.Http

Milestone:6.0.0

Copy link
Member

@tarekghtarekgh left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

MihaZupan reacted with hooray emoji
@MihaZupanMihaZupan marked this pull request as ready for reviewJune 21, 2021 10:05
@MihaZupanMihaZupan requested a review froma teamJune 21, 2021 10:06
@MihaZupan
Copy link
MemberAuthor

@dotnet/ncl please take a look at this PR - the behaviour should be unchanged, but there is a considerable refactor to the underlying class.

privatestaticreadonlyDiagnosticListeners_diagnosticListener=new("HttpHandlerDiagnosticListener");
privatestaticreadonlyActivitySources_activitySource=new(Namespace);

publicstaticreadonlyboolIsGloballyEnabled=GetEnableActivityPropagationValue();
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be static property or it will regress feature switch setting and increases size duo to static ctor dependency

MihaZupan reacted with thumbs up emoji
Copy link
MemberAuthor

@MihaZupanMihaZupanJun 23, 2021•
edited
Loading

Choose a reason for hiding this comment

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

Changed to

publicstaticboolIsGloballyEnabled{get;}= GetEnableActivityPropagationValue();

@marek-safar Is this the pattern you had in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better but it will still all the unnecessary dependencies like DiagnosticListener, ActivitySource and others where were not there before. You can check the size regression to see how much it adds

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Is there a linker switch forDiagnosticsHandler?
Or was it capable of removing the code before (it would have had to removeActivity andDiagnosticListener)?

You can check the size regression to see how much it adds

Do you mean just the size of theSystem.Net.Http assembly? Or some other way of looking at trimmed regressions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a linker switch for DiagnosticsHandler?

Yes, see HttpActivityPropagationSupport athttps://github.com/dotnet/runtime/blob/main/docs/workflow/trimming/feature-switches.md

Do you mean just the size of the System.Net.Http assembly? Or some other way of looking at trimmed regressions?

System.Net.Http will probably show the biggest increase if there is some. You can publish trimmed release build with HttpActivityPropagationSupport property set to false to compare.

MihaZupan reacted with thumbs up emoji
Copy link
MemberAuthor

@MihaZupanMihaZupanJun 23, 2021•
edited
Loading

Choose a reason for hiding this comment

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

I updated theILLink.Substitutions.xml toget_IsGloballyEnabled().

Looking at.\build.cmd -projects .\src\libraries\System.Net.Http\src\System.Net.Http.csproj -os Browser -c Release
the size ofSystem.Net.Http.dll inbrowser-wasm\lib\net6.0 dropped from 247.296 to 246.272 bytes after this PR.

I don't know how to test the runtime change with trimming and the feature switch.

Copy link
Member

@noahfalknoahfalk left a comment

Choose a reason for hiding this comment

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

I had a few questions around what our desired behavior should be inline. Other than that all looked good : )

@karelzkarelz requested a review fromgeoffkizerJune 22, 2021 19:25
Copy link
Contributor

@geoffkizergeoffkizer left a comment

Choose a reason for hiding this comment

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

These changes look reasonable to me. I'd suggest changing the "when" usage (as I commented above), but it's not a huge deal.

MihaZupan reacted with thumbs up emoji
Copy link
Member

@noahfalknoahfalk left a comment

Choose a reason for hiding this comment

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

LGTM!

@MihaZupanMihaZupan merged commitc88da29 intodotnet:mainJun 24, 2021
antonfirsov added a commit that referenced this pull requestJun 25, 2021
MihaZupan added a commit to MihaZupan/runtime that referenced this pull requestJun 30, 2021
@MihaZupanMihaZupan mentioned this pull requestJun 30, 2021
MihaZupan added a commit to MihaZupan/runtime that referenced this pull requestJul 1, 2021
antonfirsov added a commit to antonfirsov/runtime that referenced this pull requestJul 1, 2021
MihaZupan added a commit to MihaZupan/runtime that referenced this pull requestJul 8, 2021
dotnet#54437 but without the ActivitySource support
@ghostghost locked asresolvedand limited conversation to collaboratorsJul 24, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@tarekghtarekghtarekgh approved these changes

@marek-safarmarek-safarmarek-safar approved these changes

@noahfalknoahfalknoahfalk approved these changes

@shirhattishirhattiAwaiting requested review from shirhatti

+1 more reviewer

@geoffkizergeoffkizergeoffkizer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@MihaZupanMihaZupan

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@MihaZupan@marek-safar@noahfalk@tarekgh@geoffkizer
[8]ページ先頭
©2009-2025 Movatter.jp