Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Root Activity creation is not supported (in presence of active parent) #984

Open
lmolkova opened this issue Aug 3, 2020 · 7 comments
Open
Labels
enhancement New feature or request needs-runtime-change Issues which likely require changes from dotnet runtime - typically DiagnosticSource package

Comments

@lmolkova
Copy link

lmolkova commented Aug 3, 2020

Describe your environment. Describe any aspect of your environment relevant to the problem:

  • SDK version: (nuget versions of the all the relevant packages) 0.4.0-beta
  • .NET runtime version (.NET or .NET Core, TargetFramework in the .csproj file): any
  • Platform and OS version: any

Steps to reproduce.
OTel API spec requires a method that create a root span Implementations MUST provide an option to create a Span as a root span

There is no option to force .NET Activity to be root in the presence of Activity.Current
It seems to be an edge-case scenario: we fork current execution and link Activity.Current rather than creating a child of it.

Below is a workaround for this issue.

using (var parent = mySource.StartActivity("parent"))
{
    // dirty hack to force creating new Activity without parent
    Activity.Current = null;
    using (var newRoot = mySource.StartActivity(
        "new root",
        ActivityKind.Internal,
        parentContext: default,
        tags: null,
        links: new[] { new ActivityLink(parent.Context) }))
    { 
        // do stuff
    }

    // Current is null and we need another dirty hack to bring it back
    // there are other implicit dirty hacks (create newRoot in async Task
    // so that Current changes are not propagated back to parent execution context )
    Activity.Current = parent;
}

What is the expected behavior?
There should be a way to force root Activity creation without hacks.
There should be a way to restore original context after forced root is gone.

What is the actual behavior?
There is no API to force root creation - you have to provide the fake new context to force it. After new root is gone, Current needs to be manually restored to parent


I'm creating this issue to discuss whether OTel or .NET should support this scenario.
On the .NET side, it should be fine to ship it in the future release (the change would be controversial and not critical enough).
This scenario is a real-world case from one of the MS internal teams.

Can we support this on OTel side in the meantime?

  • one option is to support it through Span API: implement Tracer extension method StartRootSpan and make sure Current is captured/restored. This will force instrumentation to use Span API on top of Activity.

  • another options to support it thought ActivitySource extension method, but then there is no straightforward way to restore Current after newRoot ends and only possible if Activity would capture active context (but not parent) before is starts and restore it on dispose.

@lmolkova lmolkova added the bug Something isn't working label Aug 3, 2020
@cijothomas
Copy link
Member

cc: @rajkumar-rangaraj
We can handle this in OTel API while figuring out if .NET can support it in next release. We already have an issue about StartSpan vs StartActiveSpan which also need to do similar messing with .Current. @rajkumar-rangaraj is working to get that, and I think this can be clubbed into that.

@cijothomas
Copy link
Member

@rajkumar-rangaraj Can you confirm if this is addressed?

@rajkumar-rangaraj
Copy link
Contributor

@rajkumar-rangaraj Can you confirm if this is addressed?

Yes @cijothomas, this part is fixed in tracer shim. PR #994

@cijothomas cijothomas added enhancement New feature or request release:after-ga and removed bug Something isn't working labels Nov 6, 2020
@arakis
Copy link

arakis commented Mar 11, 2021

I've created a sample implementation for that workaround:
https://gist.github.com/arakis/6063fc4c263ef797651c7066d6a47870

@chandramouleswaran
Copy link

+1 on this request. Just a simple helper or an override to indicate “forceNew” will help. We are trying to stitch async workflows using trace links in our services

@cijothomas
Copy link
Member

Tagging with need-runtime-change as this likely needs support from DS.

@kalyanaj
Copy link
Contributor

+1 on this request as I am having to do a similar workaround here in this sample:
#4957

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-runtime-change Issues which likely require changes from dotnet runtime - typically DiagnosticSource package
Projects
None yet
Development

No branches or pull requests

7 participants