System.Diagnostics.DiagnosticSource: ActivityContext.TryParse sampling issue

[CodeBlanch]

An OpenTelemetry user raised a bug that our ParentBasedSampler did not work as expected. I tracked it down to a gap in the runtime API.

User is doing this:

var traceparents = httpContext.Request.Headers[TraceParentRequestHeaderName];

ActivityContext context = default;

if (traceparents.Count > 0)
{
	ActivityContext.TryParse(traceparents.First(), httpContext.Request.Headers[TraceStateRequestHeaderName].FirstOrDefault(), out context);
}

using var activity = this.activitySource.StartActivity(name: ActivityName, ActivityKind.Server, context);

These spans were falling into this block:

https://github.com/open-telemetry/opentelemetry-dotnet/blob/116a101732e2e6738c38529f529a239f67ed38c7/src/OpenTelemetry/Trace/ParentBasedSampler.cs#L126

Which is defaulted to always NOT sample: https://github.com/open-telemetry/opentelemetry-dotnet/blob/116a101732e2e6738c38529f529a239f67ed38c7/src/OpenTelemetry/Trace/ParentBasedSampler.cs#L54

The reason for this is ActivityContext.TryParse defaults the IsRemote property to false so we treat the span as local. In order to make this work, I think we need to expose a parameter for setting IsRemote on the ActivityContext.TryParse method. If we do that, user can do this...

	ActivityContext.TryParse([traceParent], [traceState], out context, isRemote: true);

...and everything will work fine.

/cc @cijothomas @tarekgh @noahfalk

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

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

Issue Details

---

An OpenTelemetry user raised a bug that our ParentBasedSampler did not work as expected. I tracked it down to a gap in the runtime API.

User is doing this:

var traceparents = httpContext.Request.Headers[TraceParentRequestHeaderName];

ActivityContext context = default;

if (traceparents.Count > 0)
{
	ActivityContext.TryParse(traceparents.First(), httpContext.Request.Headers[TraceStateRequestHeaderName].FirstOrDefault(), out context);
}

using var activity = this.activitySource.StartActivity(name: ActivityName, ActivityKind.Server, context);

These spans were falling into this block:

https://github.com/open-telemetry/opentelemetry-dotnet/blob/116a101732e2e6738c38529f529a239f67ed38c7/src/OpenTelemetry/Trace/ParentBasedSampler.cs#L126

Which is defaulted to always NOT sample: https://github.com/open-telemetry/opentelemetry-dotnet/blob/116a101732e2e6738c38529f529a239f67ed38c7/src/OpenTelemetry/Trace/ParentBasedSampler.cs#L54

The reason for this is ActivityContext.TryParse defaults the IsRemote property to false so we treat the span as local. In order to make this work, I think we need to expose a parameter for setting IsRemote on the ActivityContext.TryParse method. If we do that, user can do this...

	ActivityContext.TryParse([traceParent], [traceState], out context, isRemote: true);

...and everything will work fine.

/cc @cijothomas @tarekgh @noahfalk

Author:	CodeBlanch
Assignees:	-
Labels:	area-System.Diagnostics.Tracing, new-api-needs-documentation, community-contribution
Milestone:	-

[tarekgh]

@CodeBlanch thanks for submitting the PR. any public changes must have a design proposal and then reviewed by the design reviewers. Even making a default parameter to the method still a change which potentially can be breaking. Also, there is an easy workaround which can parse the context and then create a new ActivityContext from the parsed context with setting the remote value. We already have issue #42575 which tracking that. Feel free to add the proposal there and we can go from there.

[tarekgh]

CC @noahfalk