Remove ActivitySourceAdapter from AspNetCore

[eddynaka]

Refs #1585

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Design discussion issue #
• Changes in public API reviewed

[cijothomas]

rename to parentActivityContextOfOriginalActivity if you don't mind looong names.

[cijothomas]

Not for merging now. Just to demonstrate the concept. We can borrow this approach for other instrumentation and see if it can a good alternative, than exposing ActivitySourceAdapter.
Either way, the correct fix would to modify the libraries to switch to ActivitySource itself, to avoid any hacks elsewhere.

[ejsmith]

Not for merging now. Just to demonstrate the concept. We can borrow this approach for other instrumentation and see if it can a good alternative, than exposing ActivitySourceAdapter.
Either way, the correct fix would to modify the libraries to switch to ActivitySource itself, to avoid any hacks elsewhere.

1. we don't have control over a lot of these libraries that we would like to add instrumentation to that works with OTel.
2. even if we do, changing them to be ActivitySource probably means they won't work for older tools that were being used to monitor them. So that doesn't really seem like a good option.

[CodeBlanch]

It looks like AspNetCore hosting keeps a reference to the Activity it started and that's the one it will stop:

https://github.com/dotnet/aspnetcore/blob/4a7ed303fff26e4070e02e385fb22df484f6b81f/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L134-L139

So my question is: Does the Activity we create in this approach actually get stopped?

[eddynaka]

@CodeBlanch , when you say "this approach", that means the old approach or this new one? For the new one, we are replacing the activity, but, if aspnetcore has a reference there, we might be creating a new one that would never be stopped.
@cijothomas to comment as well :)

[CodeBlanch]

@eddynaka I meant the new approach sorry 😄

For the new one, we are replacing the activity, but, if aspnetcore has a reference there, we might be creating a new one that would never be stopped.

Ya, exactly. That would be a problem!

[cijothomas]

we are stopping our own activity. aspnet core stops their activity. check line 197 for our activity being stopped.

(This is a diff. issue in AspNet, as it relies on Activity.Current, so we need to restore it for AspNet, but for AspNetCore we dont. But agree - this is a bit flaky design, and will fall apart if aspnetcore changes their implementation. We'll need to revisit this in .NET6, when they are expected to do some changes.)

[CodeBlanch]

@eddynaka Can you verify the one we're creating is stopped correctly? I see the activity.Stop on LN197 but we're stopping the same activity passed in OnStopActivity so it should already be stopped? 🤷

[cijothomas]

OnStopActivity gets Activity.Current, which should be the one we created, right?

[CodeBlanch]

Ya you're right it works fine. Just took it for a spin to verify.

[cijothomas]

Not for merging now. Just to demonstrate the concept. We can borrow this approach for other instrumentation and see if it can a good alternative, than exposing ActivitySourceAdapter.
Either way, the correct fix would to modify the libraries to switch to ActivitySource itself, to avoid any hacks elsewhere.

1. we don't have control over a lot of these libraries that we would like to add instrumentation to that works with OTel.
2. even if we do, changing them to be ActivitySource probably means they won't work for older tools that were being used to monitor them. So that doesn't really seem like a good option.

This is already known. Even if .NET libraries (or any library) move to ActivitySource, it'll only be for new version only. For older version, use the approach demo-ed in this PR. After 1.0.0 is out, we'll revisit ActivitySourceAdapter approach.

[cijothomas]

Closing as this PR was meant to demo an concept only.