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

[alanwest]

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

[ghost]

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti, @safern See info in area-owners.md if you want to be subscribed.

Issue Details

---

Regarding [Span Creation](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#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`. ```C# 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](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#context-interaction) 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

Author:	alanwest
Assignees:	-
Labels:	`area-System.Diagnostics.Tracing`, `untriaged`
Milestone:	-

[ghost]

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

Issue Details

---

Regarding [Span Creation](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#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`. ```C# 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](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#context-interaction) 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

Author:	alanwest
Assignees:	-
Labels:	`untriaged`, `area-System.Diagnostics.Activity`
Milestone:	-

[tarekgh]

For creating a root Activity, why can't you do:

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

That will create the Activity acting like the root. Or you don't want to use your own generated trace id for it?

For creating Activity without setting it as the current activity, you can just use ActivitySource.CreateActivity

[ghost]

This issue has been marked needs-author-action since it may be missing important information. Please refer to our contribution guidelines for tips on how to report issues effectively.

[tarekgh]

CC @noahfalk @cijothomas @reyang

[ghost]

This issue has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

[NinoFloris]

We're all kind of wondering over here as well https://twitter.com/evntdrvn/status/1503898501507284993

[tarekgh]

@NinoFloris thanks for pointing at twitter issue. 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.

[NinoFloris]

I didn't get either of these to work

The parentId: null version stays parented to Activity.Current: https://dotnetfiddle.net/qezpMP

The version in this issue returns a null activity: https://dotnetfiddle.net/oyj9Q7

[noahfalk]

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.

[tarekgh]

Thanks @noahfalk for the advice and the clarification.

[ericsampson]

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.

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.

These both seem...suboptimal.

For example, the JS OTel API (and probably others too) use their equivalent of default(ActivityContext) to explicitly indicate that it should start a new span, and so people working across languages are going to get confused by the .NET equivalent.

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.

Best, Eric

[ericsampson]

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?

[tarekgh]

I imagine that CreateActivity(String, ActivityKind) and StartActivity(String, ActivityKind) use Activity.Current as well?

Yes, these are going to use default Activity context and null parent Id which will cause using Activity.Current set as the parent.

[alanwest]

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.

[ghost]

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

Issue Details

---

Regarding [Span Creation](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#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`. ```C# 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](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#context-interaction) 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

Author:	alanwest
Assignees:	-
Labels:	`area-System.Diagnostics.Activity`
Milestone:	Future

[CodeBlanch]

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, ...) {}
}

[tarekgh]

@CodeBlanch thanks for the update. Is it possible we can get the actual parameters proposal?

• do we need to support these with the old-style parents (the APIs that take parent as string)?
• The new APIs still need to allow passing the parent context if need to create the Activity with specific trace id?
• Regarding passing linkToCurrentTrace, does the new APIs will take the links list anyway? or you are adding this to optimize it (to avoid creating IEnumerable)?
• Would it make sense instead of having Start/CreateRootActivity, to have Create/StartActivity taking some flag telling need to be root? this flag will be false by default.
• We may look exposing the new APIs with start using TagList instead of IEnumerable<KVP> to support optimization.

Last, how urgent is this request? Is it a blocker or can wait to next release?

[CodeBlanch]

@tarekgh

How about...

API Proposal

public Activity? CreateRootActivity(string name, ActivityKind kind) {}

public Activity? CreateRootActivity(
   string name, 
   ActivityKind kind,
   ActivityTraceId traceId = default,
   in TagList tags = default,
   IEnumerable<ActivityLink>? links = null,
   bool addCurrentTraceActivityContextAsLink = false) {}

public Activity? StartRootActivity(string name, ActivityKind kind) {}

public Activity? StartRootActivity(
   string name, 
   ActivityKind kind,
   ActivityTraceId traceId = default,
   in TagList tags = default,
   IEnumerable<ActivityLink>? links = null,
   bool addCurrentTraceActivityContextAsLink = false,
   DateTimeOffset startTime = default) {}

do we need to support these with the old-style parents (the APIs that take parent as string)?

I don't think so! Given it is a root we should be able to ignore parent.

The new APIs still need to allow passing the parent context if need to create the Activity with specific trace id?

I hadn't thought of that, but I added ActivityTraceId to the proposal so users can control the traceId if needed. I didn't add spanId I feel like that is always random in the current API?

Regarding passing linkToCurrentTrace, does the new APIs will take the links list anyway? or you are adding this to optimize it (to avoid creating IEnumerable)?

Ya it is a convenience thing to make (what I anticipate being) a common scenario easy/cheap. The case @martinjt has is some user sends a request to their service with traceparent. That trace is the customer's trace though they want to create their own root and essentially ignore it. But linking it to the customer one seems useful.

Would it make sense instead of having Start/CreateRootActivity, to have Create/StartActivity taking some flag telling need to be root? this flag will be false by default.

I'm not opposed to that. I think having dedicated APIs would be more obvious/cleaner but a flag could work.

We may look exposing the new APIs with start using TagList instead of IEnumerable to support optimization.

Used TagList on the proposal.

Last, how urgent is this request? Is it a blocker or can wait to next release?

I don't think it is urgent at all. Just a nice-to-have.

[ericsampson]

@cartermp @andrewlock might have some thoughts on this as well.

[ramonsmits]

Having the ability to create a new parent and (optionally) auto link these without requiring the .Current = null hack would be very useful.

[ericsampson]

Any chance of getting this to API review and moving along? Cheers!