runtime: Allow creation of a root Activity when Activity.Current is not null

Regarding Span Creation the OpenTelemetry specification states that the API must accept:

The parent Context or an indication that the new Span should be a root Span.

The current ActivitySource.StartActivity API does not allow for this if Activity.Current != null. The following code creates a new activity as a child of Activity.Current.

var rootSpan = activitySource.StartActivity(
        "RootSpan",
        ActivityKind.Internal,
        parentContext: default)

One solution may be to set Activity.Current = null when StartActivity receives parentContext = default, but then it may be debatable whether Activity.Current should then be set to:

  • the newly created root activity, or
  • be restored to the previous value of Activity.Current prior to returning from StartActivity.

The OpenTelemetry specification also states:

In languages with implicit Context propagation, Span creation MUST NOT set the newly created Span as the active Span in the current Context by default, but this functionality MAY be offered additionally as a separate operation.

This implies that StartActivity should not affect the value of Activity.Current, but since it already does today it may be reasonable to set Activity.Current to the newly created root activity.

Another option could be to introduce a new API StartRootActivity that starts a new activity but does not affect Activity.Current. It would be up to the user to manage Activity.Current.


This need was originally outlined in https://github.com/open-telemetry/opentelemetry-dotnet/issues/984.

@cijothomas

About this issue

  • Original URL
  • State: open
  • Created 2 years ago
  • Reactions: 3
  • Comments: 20 (17 by maintainers)

Most upvoted comments

I think we might want to consider doing something here. Sure there are workarounds, but I don’t think the API makes it obvious that they are needed. Users have to fail, realize the failure, and then figure out what to do about it. This is a somewhat common scenario/question we see on the OpenTelemetry .NET slack.

API Proposal

namespace System.Diagnostics;

public class ActivitySource
{
    public Activity? CreateRootActivity(...) {}
    public Activity? StartRootActivity(...) {}
}

If we add this API it might be useful to supply a parameter which would automatically add the current parent context (if it exists) as a link.

namespace System.Diagnostics;

public class ActivitySource
{
    public Activity? CreateRootActivity(..., bool linkToCurrentTrace = false, ...) {}
    public Activity? StartRootActivity(..., bool linkToCurrentTrace = false, ...) {}
}

Thanks to everyone for taking a close review this. Apologies, somehow I missed @tarekgh initial suggestion, but alas it sounds like it’s a no go.

It would be great to have a more concise API @alanwest, whether that means introducing new sentinel values for parentContext or parentSpanId to indicate that the parent not be made Activity.Current, or another approach.

Yes, agreed. I think something like StartRootActivity could work well to clearly indicate the intent to create a root activity. Maybe StartRootActivity could take a bool makeCurrent parameter indicating to set Activity.Current to the new root activity.

I’m going to submit a doc request so that the current behavior is at least documented. I imagine that CreateActivity(String, ActivityKind) and StartActivity(String, ActivityKind) use Activity.Current as well?

If you wanted to create a new root using the APIs that currently exist, you can do this:

Activity previous = Activity.Current;
// by default StartActivity parents to the current span, we don't want it to have a parent
Activity.Current = null;
Activity newRoot = source.StartActivity("Item2");
// by default StartActivity() makes the new span current, but if wanted to keep the old span as current then we need this...
Activity.Current = previous; 

@NinoFloris based on your latest fiddle (https://dotnetfiddle.net/1gkh9Q) it appears you discovered the same result. Writing 4 lines of code doesn’t look as elegant as one line of course, so it still seems perfectly legitimate to explore having a more concise API as @alanwest was originally asking about.

I have replied there but you can do also what I have mentioned in my comment https://github.com/dotnet/runtime/issues/65528#issuecomment-1045019455. what mentioned in https://github.com/open-telemetry/opentelemetry.io/issues/1073 is ok too.

This isn’t correct unfortunately, neither of these options create new root Activities.

using Activity? activity = aSource.StartActivity(“A1”, ActivityKind.Client, new ActivityContext(Activity.TraceIdGenerator is null ? ActivityTraceId.CreateRandom() : Activity.TraceIdGenerator(), default, default, default));

@tarekgh - I don’t know if we’ve done a good job defining what the behavior on that input should be, but the current behavior on this input does weird things. The easiest option for us to do would be to define that as bad input (it is an invalid parent ActivityContext after all), or if we define the behavior some other way then we’ll need to change the implementation to match. In the sample callback we pass it as the ParentContext and OpenTelemetry treats any non-default ParentContext to mean that the Activity has a parent and isn’t a root (incidentally classifying it as a parented Activity is also what caused OT to not to sample it in which is why @NinoFloris saw that null result). In Start() we use _parentSpanId == null to infer that the Activity doesn’t have a parent, but then we fallback to making Activity.Current be the parent . So instead of a new root Activity we get a new parented Activity where the sampling callback and Activity.Parent disagree about what the identity of the parent is. This path also opens the door to have an Activity whose TraceId doesn’t match its parent’s id which we’ve attempted to never allow.

what mentioned in https://github.com/open-telemetry/opentelemetry.io/issues/1073 is ok too

This one doesn’t work either I’m afraid. The code treats default(ActivityContext) as the sentinel value indicating that it should parent the new Activity to Activity.Current.