[API Proposal]: Activity.HasRemoteParent (and problematic docs for Activity.Parent)

[Oberon00]

Background and motivation

In an exporter, we need to process the W3C tracestate in order to add certain information to the exported data. However, this information should only apply to the span that actually had that tracestate as its direct parent, i.e. that had a remote parent. In Java, we were able to implement that exporter based on SpanContext ReableSpan.getParentSpanContext() and SpanContext.isRemote().

ActivityContext.IsRemote is also available in .NET (even though it will often be wrong due to TryParse setting it to false #42575), but there is no way to get the parent span context's IsRemote property.

For now, we use Activity.Parent == null as replacement. The documentation of Activity.Parent makes this bold claim:

The parent of this Activity, if it is from the same process, or null if this instance has no parent (it is a root activity) or if the parent is from outside the process.

implying that it will always be != null if from the same process. This is obviously wrong though. In fact, the only way to explicitly pass a parent to ActivitySource.StartActivity is by passing an ActivityContext, and if using that overload, Parent will be null, regardless of whether the explicit parent context was from the same process or not (it's not even possible to know this in general).

CC @tarekgh @cijothomas

API Proposal

namespace System.Diagnostics
{
    public partial class Activity
    {
        public bool HasRemoteParent { get; }
    }
}

Adding a StartActivity overload that takes an Activity as parent instead of an ActivityContext and sets Activity.Parent to that instead of null would also be a nice-to-have.

Changing the default IsRemote value of ActivityContext.TryParse to true could also be extremely helpful, as the wrong default would become potentially more impactful when being retrievable via Activity.HasRemoteParent .

Additionally, the wrong documentation on Activity.Parent should be corrected (while non-null definitely means in-process, null-ness doesn't tell you much at all, other than that the implicit parent was either not used or was actually null).

API Usage

// Inside some OTel exporter
if (activity.HasRemoteParent)
  AddRemoteInfo(serializedActivity, activity.TraceStateString )

Alternative Designs

According to the OpenTelemetry specification of SpanProcessor.OnStart, OnStart should receive the full parent Context (context being a required concept that is missing from OTel-dotnet and the System.Diagnostics library). If the ActivityListener.ActivityStarted callback could receive at least the full parent ActivityContext, it could set a Tag on the span for IsRemote. Drawback being, that this tag is then visible to everybody, with exporters exporting it by default even though it may only be of interest for particular exporter implementations.

There is also ActivityListener.Sample which seems to get all the required information, however it is unclear how multiple Sample callbacks work together, and what one should do if one does not want to make any sample decision but just "misuse" the Sample callback to add SamplingTags

Risks

Exposing HasRemoteParent in combination with the default IsRemote=false in ActivityContext.TryParse may expose/trigger new bugs in code that starts using it.

[ghost]

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

Issue Details

---

Background and motivation

In an exporter, we need to process the W3C tracestate in order to add certain information to the exported data. However, this information should only apply to the span that actually had that tracestate as its direct parent, i.e. that had a remote parent. In Java, we were able to implement that exporter based on SpanContext ReableSpan.getParentSpanContext() and SpanContext.isRemote().

ActivityContext.IsRemote is also provided on .NET (even though it will often be wrong due to TryParse setting it to false #42575), but there is no way to get the parent span's IsRemote property.

For now, we use Activity.Parent != null as replacement. The documentation of Activity.Parent makes this bold claim:

The parent of this Activity, if it is from the same process, or null if this instance has no parent (it is a root activity) or if the parent is from outside the process.

implying that it will always be != null if from the same process. This is obviously wrong though. In fact, the only way to explicitly pass a parent to ActivitySource.StartActivity is by passing an [ActivityContext], and if using that overload, Parent will be null, regardless of whether the explicit parent context was from the same process or not (it's not even possible to know this in general).

CC @tarekgh @cijothomas

API Proposal

namespace System.Diagnostics
{
    public partial class Activity
    {
        public bool HasRemoteParent { get; } // Required to support OTel scenarios
    }
}

Adding a StartActivity overload that takes an Activity as parent instead of an ActivityContext and sets Activity.Parent to that instead of null would also be a nice-to-have.

Changing the default IsRemote value of ActivityContext.TryParse to true could also be extremely helpful, as the wrong default would become potentially more impactful when being retrievable via Activity.HasRemoteParent .

Additionally, the wrong documentation on Activity.Parent should be corrected (while non-null definitely means in-process, null-ness doesn't tell you much at all, other than that the implicit parent was either not used or was actually null).

API Usage

// Inside some OTel exporter
if (activity.HasRemoteParent)
  AddRemoteInfo(serializedActivity, activity.TraceStateString )

Alternative Designs

According to the OpenTelemetry specification of SpanProcessor.OnStart, OnStart should receive the full parent Context (context being a required concept that is missing from OTel-dotnet and the System.Diagnostics library). If the ActivityListener.ActivityStarted callback could receive at least the full parent ActivityContext, it could set a Tag on the span for IsRemote. Drawback being, that this tag is then visible to everybody, with exporters exporting it by default even though it may only be of interest for particular exporter implementations.

There is also ActivityListener.Sample which seems to get all the required information, however it is unclear how multiple Sample callbacks work together, and what one should do if one does not want to make any sample decision but just "misuse" the Sample callback to add SamplingTags

Risks

Exposing HasRemoteParent in combination with the default IsRemote=false in ActivityContext.TryParse may expose/trigger new bugs in code that starts using it.

Author:	Oberon00
Assignees:	-
Labels:	api-suggestion, area-System.Diagnostics.Tracing, untriaged
Milestone:	-

[Oberon00]

If open-telemetry/oteps#182 is merged, this would be required to properly implement OTLP export, "Export SpanContext.IsRemote in OTLP" I think.

[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

---

Background and motivation

In an exporter, we need to process the W3C tracestate in order to add certain information to the exported data. However, this information should only apply to the span that actually had that tracestate as its direct parent, i.e. that had a remote parent. In Java, we were able to implement that exporter based on SpanContext ReableSpan.getParentSpanContext() and SpanContext.isRemote().

ActivityContext.IsRemote is also available in .NET (even though it will often be wrong due to TryParse setting it to false #42575), but there is no way to get the parent span context's IsRemote property.

For now, we use Activity.Parent == null as replacement. The documentation of Activity.Parent makes this bold claim:

The parent of this Activity, if it is from the same process, or null if this instance has no parent (it is a root activity) or if the parent is from outside the process.

implying that it will always be != null if from the same process. This is obviously wrong though. In fact, the only way to explicitly pass a parent to ActivitySource.StartActivity is by passing an ActivityContext, and if using that overload, Parent will be null, regardless of whether the explicit parent context was from the same process or not (it's not even possible to know this in general).

CC @tarekgh @cijothomas

API Proposal

namespace System.Diagnostics
{
    public partial class Activity
    {
        public bool HasRemoteParent { get; }
    }
}

Adding a StartActivity overload that takes an Activity as parent instead of an ActivityContext and sets Activity.Parent to that instead of null would also be a nice-to-have.

Changing the default IsRemote value of ActivityContext.TryParse to true could also be extremely helpful, as the wrong default would become potentially more impactful when being retrievable via Activity.HasRemoteParent .

Additionally, the wrong documentation on Activity.Parent should be corrected (while non-null definitely means in-process, null-ness doesn't tell you much at all, other than that the implicit parent was either not used or was actually null).

API Usage

// Inside some OTel exporter
if (activity.HasRemoteParent)
  AddRemoteInfo(serializedActivity, activity.TraceStateString )

Alternative Designs

According to the OpenTelemetry specification of SpanProcessor.OnStart, OnStart should receive the full parent Context (context being a required concept that is missing from OTel-dotnet and the System.Diagnostics library). If the ActivityListener.ActivityStarted callback could receive at least the full parent ActivityContext, it could set a Tag on the span for IsRemote. Drawback being, that this tag is then visible to everybody, with exporters exporting it by default even though it may only be of interest for particular exporter implementations.

There is also ActivityListener.Sample which seems to get all the required information, however it is unclear how multiple Sample callbacks work together, and what one should do if one does not want to make any sample decision but just "misuse" the Sample callback to add SamplingTags

Risks

Exposing HasRemoteParent in combination with the default IsRemote=false in ActivityContext.TryParse may expose/trigger new bugs in code that starts using it.

Author:	Oberon00
Assignees:	-
Labels:	api-suggestion, untriaged, area-System.Diagnostics.Activity
Milestone:	-

[tarekgh]

(even though it will often be wrong due to TryParse setting it to false #42575),

I believe we have already addressed that and now you can parse with setting up the remote option if needed.

To double check the expectation here, the only time the parent would be remote is when creating the Activity object with the parent ActivityContext containing remote flag set to true. right? or you expect some other situations to make a remote parent?

CC @noahfalk @reyang

[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.

[Oberon00]

To double check the expectation here, the only time the parent would be remote is when creating the Activity object with the parent ActivityContext containing remote flag set to true. right?

Yes, I think so.

In terms of the OTel spec, I was thinking if a edge case with sampling is possible, where an unsamped span with a remote parent would itself count as "remote" so that sampled grandchildren would have HasRemoteParent == true, but that is not the case https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#sdk-span-creation

So I think the following could be used to describe the behavior:

• If creating an activity with a parent ActivityContext: Set HasRemoteParent to the IsRemote property of the parent context
• Otherwise (even if no parent, or with a parent activity that does have a remote parent, regardless of sampling): Set HasRemoteParent to false.

[tarekgh]

@noahfalk @reyang @cijothomas this looks reasonable proposal. any thoughts?

[noahfalk]

@noahfalk @reyang @cijothomas this looks reasonable proposal. any thoughts?

Looks reasonable to me.

it is unclear how multiple Sample callbacks work together, and what one should do if one does not want to make any sample decision but just "misuse" the Sample callback to add SamplingTags

The final sampling decision is the maximum level returned by each sampling callback. If you want to get the sampling notification but have no impact on the sampling you can return ActivitySamplingResult.None.

[terrajobst]

Video

• Looks good as proposed

namespace System.Diagnostics
{
    public partial class Activity
    {
        public bool HasRemoteParent { get; }
    }
}