Add ActivitySource support to DiagnosticsHandler

[MihaZupan]

Today, DiagnosticsHandler will create a new Activity if either is true:

• An Activity was already set (Activity.Current is not null)
• A DiagnosticListener requested one (was enabled for the activity name)

This PR preserves the same behaviour when no ActivityListener is present.

The behaviour changes iff all of the following hold:

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

In this case, no Activity is created (and so we also won't inject context headers).
If a DiagnosticListener 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 the SendAsync call into

// The interesting part of the PR - this method decides on the actual behaviour
static bool ShouldLogDiagnostics(HttpRequestMessage request, out Activity? activity)

I also removed the internal Settings and LoggingStrings classes - am I missing something and they actually provide value?

Note that the PR is only a slight refactor + adding ActivitySource. It does not address other known DiagnosticsHandler issues.
This is runtime's equivalent of what dotnet/aspnetcore#30089 was for AspNet.

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

[ghost]

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

Issue Details

---

Today, DiagnosticsHandler will create a new Activity if either is true:

• An Activity was already set (Activity.Current is not null)
• A DiagnosticListener requested one (was enabled for the activity name)

This PR preserves the same behaviour when no ActivityListener is present.

The behaviour changes iff all of the following hold:

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

In this case, no Activity is created (and so we also won't inject context headers).
If a DiagnosticListener 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 the SendAsync call into

// The interesting part of the PR - this method decides on the actual behaviour
static bool ShouldLogDiagnostics(HttpRequestMessage request, out Activity? activity)

I also removed the internal Settings and LoggingStrings classes - am I missing something and they actually provide value?

Note that the PR is only a slight refactor + adding ActivitySource. It does not address other known DiagnosticsHandler issues.
This is runtime's equivalent of what dotnet/aspnetcore#30089 was for AspNet.

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

Author:	MihaZupan
Assignees:	-
Labels:	area-System.Net.Http
Milestone:	6.0.0

[tarekgh]

LGTM. Thanks!

[MihaZupan]

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

[marek-safar]

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

[MihaZupan]

Changed to

public static bool IsGloballyEnabled { get; } = GetEnableActivityPropagationValue();

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

[marek-safar]

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

[MihaZupan]

Is there a linker switch for DiagnosticsHandler?
Or was it capable of removing the code before (it would have had to remove Activity and DiagnosticListener)?

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

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

[marek-safar]

Is there a linker switch for DiagnosticsHandler?

Yes, see HttpActivityPropagationSupport at https://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]

I updated the ILLink.Substitutions.xml to get_IsGloballyEnabled().

Looking at .\build.cmd -projects .\src\libraries\System.Net.Http\src\System.Net.Http.csproj -os Browser -c Release
the size of System.Net.Http.dll in browser-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.

[noahfalk]

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

[geoffkizer]

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

[noahfalk]

LGTM!