Add ActivitySource support to DiagnosticsHandler

[ManickaP]

This is an attempt to bring the logic from #54437 in main.

Description from the original PR:

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 for Activity creation if an ActivityListener is present:

• ActivitySource is given chance to create an activity
• if none was created, the logic falls back to original behaviour: either Activity.Current is set or DiagnosticListener is enabled for the activity name
  - 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 Activity? CreateActivity(HttpRequestMessage request)

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.

---

@MihaZupan could you please give it a look if I'm not missing anything? I had to adapt the PR and I tried to not to do much unnecessary churn.

Contributes to #63868

cc @denisivan0v @cijothomas

[ghost]

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

Issue Details

---

This is an attempt to bring the logic from #54437 in main.

Description from the original PR:

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.

---

@MihaZupan could you please give it a look if I'm not missing anything? I had to adapt the PR and I tried to not to do much unnecessary churn.

Author:	ManickaP
Assignees:	ManickaP
Labels:	area-System.Net.Http
Milestone:	-

[MihaZupan]

The behaviour changes iff all of the following hold:

Looks like I didn't update the description on the PR as things changed 😄
From what I can tell, this PR is functionally equivalent to #54437 and the difference is this:

• ActivitySource is given the chance to create the Activity first, but if it chooses not to, one will still be created manually.

I.e. ActivitySource's sampling decision is ignored if there is already an Activity present / a diagnostic listener said otherwise. This is the same thing AspNetCore does on their side. Some more details here: #54437 (comment)

[CarnaViire]

LGTM (I'm no expert though 😆)

[CarnaViire]

Would it be better to have a generated MemberData instead? 😅

[ManickaP]

I tried to preserve some of the wisdom from the comments so it's not much shorter than the original, but it's moved in member data.

[ManickaP]

No failing tests, pipelines are timing out on Mac/iOS/..., other runs have similar problems ==> merging.