Migrate to use ActivitySource API to create Activities in AspNetCore Hosting

[cijothomas]

Is your feature request related to a problem? Please describe.

AspNetCore uses Activity API along with DiagnosticSource/Listener API to emit telemetry/diagnostic events. https://github.com/dotnet/aspnetcore/blob/master/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs

Its currently using the new Activity() method to create an Activity. In .NET 5.0, a new mechanism was introduced to create Activity. The new API is based on ActivitySource. It offers following advantages:

1. ActivitySource allows setting proper Kind to be set on Activity, instead of the default Internal.

2. ActivitySource assigns correct ActivitySource to Activity. (instead of a default ActivitySource which has "" name)

3. ActivitySource runs through the Sampler before deciding to create the activity and creates Activity with correct values for IsAllDataRequested and Recorded. This allows connecting samplers to control Activity creation and is in alignment with OpenTelemetry specifications. Since AspNetCore does not use ActivitySource API, Activity is always created without the ability to configure Sampler.

Describe the solution you'd like

A clear and concise description of what you want to happen. Include any alternative solutions you've considered.

1. Migrating to ActivitySource API - replacing new Activity() with ActivitySource.StartActivity() is a breaking change, as Activity may or may not be created when using ActivitySource. To avoid this breaking change, it is proposed to create a dummy ActivityListener, which will set SamplingResult to PropagationData. This ensures that activity will always be created (thus maintaining backward compatibility).

OpenTelemetry (or other telemetry systems) can setup additional Listeners and achieve its needs.

2. Populate the necessary tags. (as per OpenTelemetry semantic conventions) - This is optional, if replace batch files with kvm #3 is continued, as a listener can listen to DiagnosticSource events, and populate Tags as needed.
3. The DiagnosticSource events may continue to be fired.

Once the above is achieved, OpenTelemetry instrumentations can avoid the current hacks it deploys to workaround the limitations. As of now, OpenTelemetry uses reflection to set Kind, Source on Activity, and runs samplers to correctly set IsAllDataRequested and Recorded fields.

Additional context

Related issue: #28642

[cijothomas]

@shirhatti @noahfalk @tarekgh @reyang @ank3it

[cijothomas]

@alanwest @CodeBlanch from OpenTelemetry .NET group

[shirhatti]

This is technically a behavior change in that the ActivityKind and ActivitySource that were previously not set will now be populated. While I don't think a mitigation like an AppContextSwitch is required, it's worth discussing it.

Also, as mentioned in the issue, we need to address the scenario where no activity is created. I'm not a fan of creating a dummy listener. I'd prefer if ActivitySource itself could expose new API to force that an Activity is always created.

[shirhatti]

👀 @JamesNK we might want to make a similar change in gRPC

[cijothomas]

👀 @JamesNK we might want to make a similar change in gRPC

And in HttpClient.. and Asp.Net (its separate package for aspnet) too.

[cijothomas]

I'd prefer if ActivitySource itself could expose new API to force that an Activity is always created.

@noahfalk @tarekgh Is the above something ActivitySource can consider adding?

[JamesNK]

👀 @JamesNK we might want to make a similar change in gRPC

Grpc.Net.Client multitargets .NET Core 3. Is it worth having the legacy method in .NET Core 3, and creating activities with the new method in .NET 5+?

[tarekgh]

All suggestions here are good. Only we need to be careful not breaking current working scenarios. e.g. if someone today creating DiagnosticsListener to listen for such cultures properly they will be broken if we didn't do something to make this work.

Is the above something ActivitySource can consider adding?

Why creating simple ActivityListener is not good here. it s a few lines:

        using ActivityListener listener = new ActivityListener();
        listener.ShouldListenTo = (activitySource) => object.ReferenceEquals(aSource, activitySource);
        listener.SampleUsingParentId = (ref ActivityCreationOptions<string> activityOptions) => ActivitySamplingResult.AllData;
        listener.Sample = (ref ActivityCreationOptions<ActivityContext> activityOptions) => ActivitySamplingResult.AllData;

but not a problem to explore the request and discuss it.

[noahfalk]

Couple thoughts:

1. I believe many of the code paths we are proposing touching are very performance sensitive. Starting an Activity always probably creates noticeable regressions in http (and grpc?) benchmarks and would be rejected. Creating an Activity is also side-effecting for out-bound http traffic (the messages will now have headers on them that didn't exist previously) and we've seen feedback from some .NET devs that don't appreciate these unexpected headers. So we probably need to operate within the constraint that if the current code path wouldn't have created an Activity, the new one doesn't either (assuming all else is equal).
2. I wouldn't worry about the API design as the high order bit, I'd first try to figure out how much flexibility, if any, do we have to adjust when the existing callbacks get dispatched. In particular the DiagnosticListener path sets some properties and invokes some callbacks (like ActivityImport() or AddBaggage()) prior to Activity.Start() and if we are using ActivitySource then there is no access to the Activity object at that stage in its lifecycle. We have to chose one of these options:

• defer that work until after the Activity is started
• don't use ActivitySource to start the activity when a DiagnosticListener is also present
• change the API in some way
  If we could get away with the first option that is probably the least work and provides predictable forward-looking behavior for ActivityListener, but it has the highest potential to break back-compat. Research into how existing code that subscribes to this DiagnosticListener behaves would probably tell if it is was a reasonable option, or if we needed a compat switch to mitigate some risk, or its just clearly broken and we need to pick one of the other options.

3. Recently I think there were some parallel requests to support pluggable propagation implementations? If so it might be good to consider the impact of that at the same time because it is probably touching the exact same code.

Grpc.Net.Client multitargets .NET Core 3. Is it worth having the legacy method in .NET Core 3, and creating activities with the new method in .NET 5+?

I am guessing Grpc.Net.Client is an out-of-band NuGet package? I'm not sure how this will play out yet by my hunch is that we would ship Grpc.Net.Client v.next with an assembly reference to Microsoft.Diagnostics.DiagnosticSource.dll v.next and both of these packages are capable of targetting .NET Core 3 if that is where developers wish to deploy them. It will probably be simpler maintaining only one code path rather than maintaining separate v3 and >v3 codepaths. I was trying to find where Grpc code created Activities for incoming messages but my github-foo was failing me. I found what looked like the Activity name but not where it was being used to create activities. I'm glad to take a closer look if you can point at the right spot?

[cijothomas]

Starting an Activity always probably creates noticeable regressions in http (and grpc?) benchmarks and would be rejected

I meant to say - we should fully retain existing behavior. If an Activity would have been created currently, we should still create it. If not, we shouldn't either.
I think this is now based on "if some logging is enabled, then create Activity". We can encapsulate this logic to the ShouldListenTo of the "dummy" Listener to achieve full b/w compat.

[ghost]

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[davidfowl]

I'm allergic to making this code more complex and less performant. I think we'll need to spend some dedicated time figuring out how to move this logic outs of the critical path so it's more amenable to changes

[JamesNK]

Grpc.Net.Client multitargets .NET Core 3. Is it worth having the legacy method in .NET Core 3, and creating activities with the new method in .NET 5+?

I am guessing Grpc.Net.Client is an out-of-band NuGet package? I'm not sure how this will play out yet by my hunch is that we would ship Grpc.Net.Client v.next with an assembly reference to Microsoft.Diagnostics.DiagnosticSource.dll v.next and both of these packages are capable of targetting .NET Core 3 if that is where developers wish to deploy them. It will probably be simpler maintaining only one code path rather than maintaining separate v3 and >v3 codepaths. I was trying to find where Grpc code created Activities for incoming messages but my github-foo was failing me. I found what looked like the Activity name but not where it was being used to create activities. I'm glad to take a closer look if you can point at the right spot?

Grpc.Net.Client creates an activity here - https://github.com/grpc/grpc-dotnet/blob/d8d982b29182e99a956f71963455c8cd5496e915/src/Grpc.Net.Client/Internal/GrpcCall.cs#L660-L750

[noahfalk]

how to move this logic outs of the critical path

What do you consider the critical path? If critical path means code that runs for:

• benchmarks and production scenarios where no telemetry is enabled - we've got super-fast IsEnabled() calls that let everything else sit in a cold conditional scope
• the normal production case where we expect telemetry to be enabled - The new code paths support early sampling so telemetry collection costs are pay-for-play.

I'm allergic to making this code ... less performant

I wouldn't assume that is the result : )

Grpc.Net.Client creates an activity here

Thanks! This usage looks simpler the Kestrel host. I'm optimistic that we could write something that wouldn't add too much complexity and it would handle both DiagnosticListener and ActivityListener regardless of which .NET Core version is installed.

[pakrym]

Did you consider keeping the code paths independent and producing activity for DiagnosticSource and one for AcitivitySource depending on what's enabled? Sometimes you might get two activities if two independent listeners subscribe to the same thing but at least it keeps the back-compat story simple.