ActivityContext.TryParse should support IsRemote

[cg110]

Edited and added the proposal by @tarekgh

Description

When using ActivitySource I wanted to support IsRemote (for opentelemetry), as the majority of the time any activity started with a w3c trace context will be remote.

To start an activity with IsRemote set needs an ActivityContext, the best option appears to be use:
ActivityContext.TryParse(traceParent, traceState, out var context)
However, a context then needs creating to copy the parsed context, and set IsRemote.

It would be useful if TryParse could provide a version that can pass IsRemote into the ActivityContext. I'd expect the majority of new activities that use TryParse are providing a traceparent and state from a remote caller.

Proposal

public readonly struct ActivityContext
{
   public static bool TryParse(string? traceParent, string? traceState, out System.Diagnostics.ActivityContext context)
+  public static bool TryParse(string? traceParent, string? traceState, bool isRemote, out System.Diagnostics.ActivityContext context)
}

Example

    const string w3cId = "00-99d43cb30a4cdb4fbeee3a19c29201b0-e82825765f051b47-01";
    if (TryParseActivityContext(w3cId, "k=v", isRemote: true, out ActivityContext context)) // Parse the context with setting isRemote to true
    {
        // do something with the context
    }

Alternatives

Users can manually add the following helper method but that means every user need this functionality will need to add this method.

    public static bool TryParseActivityContext(string? traceParent, string? traceState, bool isRemote, out ActivityContext context)
    {
        if (ActivityContext.TryParse(traceParent, traceState, out  context))
        {
            context = new ActivityContext(context.TraceId, context.SpanId, context.TraceFlags, context.TraceState, isRemote);
            return true;
        }
        return false;
    }

[ghost]

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

[tarekgh]

However, a context then needs creating to copy the parsed context, and set IsRemote.

Why this is not enough for you? ActivityContext is a struct and doing that wouldn't cause any real perf issue as it doesn't allocate and the struct size is really small. I don't think it is a good idea to expose a new API just to support this.

CC @noahfalk

[cg110]

It's more that it's error prone, and easy to forget

Really I'd like to do Activity.StartActivity with trace parent, trace source, and isremote, but using activityContext seemed the next easiest alternative, but it needs a dance to get IsRemote set.

[tarekgh]

It's more that it's error prone, and easy to forget

I am not sure I understand this. How this is error prone? You can create your own helper method doing the functionality, it is simple and I wouldn't expect anyone can get it wrong. Easy to forget would be same as if someone manually creating ActivityContext object and forget to pass isRemote as it is optional parameters. Who cares about isRemote would never forget about it.

[CodeBlanch]

API proposal:

public readonly struct ActivityContext
{
   public static bool TryParse(
      string? traceParent,
      string? traceState,
      out System.Diagnostics.ActivityContext context,
      bool isRemote = false) // New parameter with default set to current behavior.
   {
   }

For me it isn't a question of whether the workaround is easy or not, it is a question of how do you know you need the workaround? You don't get immediate feedback that you made a misake. You might not even notice until in production when your sampling is off. If you do notice your sampling is off, it is natural to look at the sampler for issues. Triaging that all the way back to this API is difficult. I think adding the parameter would be a nice hint to users that they should consider whether or not the context belongs to a remote parent when it is created.

[tarekgh]

@CodeBlanch thanks for the proposal. We cannot add an extra parameter to the existing method as it will be a breaking change. It is like removing a method. I suggest we add extra overload. Something like:

public readonly struct ActivityContext
{
   public static bool TryParse(string? traceParent, string? traceState, out System.Diagnostics.ActivityContext context)
+  public static bool TryParse(string? traceParent, string? traceState, bool isRemote, out System.Diagnostics.ActivityContext context)
}

Note, isRomte is not defaulted parameter as I want to disnguish this overload from the already existing one. The other reason is I want to stick with the TryParse pattern which having the out parameter be last parameter. What do you think?

CC @noahfalk @cijothomas @reyang

[CodeBlanch]

@tarekgh New method looks good to me. I suspected adding the parameter would be breaking but wasn't sure 😄

PS: How did you do that diff comment thing? That is neat.

[tarekgh]

PS: How did you do that diff comment thing? That is neat.

```diff

and having + in front of the line means added and will show up in green. having - in front of the line means deleted and will show up in red

[bartonjs]

Video

Looks good as proposed.

namespace System.Diagnostics
{
    public readonly partial struct ActivityContext
    {
        // existing
        // public static bool TryParse(string? traceParent, string? traceState, out System.Diagnostics.ActivityContext context);

        // new
        public static bool TryParse(
            string? traceParent,
            string? traceState,
            bool isRemote,
            out System.Diagnostics.ActivityContext context);
    }
}