System.Diagnostics.ActivitySource API Addition

[CodeBlanch]

Background and Motivation

I'm working on implementing the new System.Diagnostics.ActivitySource API using the OpenTelemetry-dotnet beta that was just released on top of it. I've run into a couple of pain points that I think an API tweak or two would help alleviate.

I'm consuming messages off a queue. These messages have W3C TraceParent and TraceState values stored as message headers. If I wanted to create an Activity using these values as a parent the API provides:

public sealed class ActivitySource
{
  public Activity? StartActivity(string name, ActivityKind kind, string parentId, IEnumerable<KeyValuePair<string, string?>>? tags = null, IEnumerable<ActivityLink>? links = null, DateTimeOffset startTime = default);
}

That would work fine but I don't want to create a parented Activity, I want to add this TraceContext as a link on that Activity.

ActivityLink only accepts ActivityContext in its ctor. There's actually no way I can find to parse a string TraceParent + TraceState into an ActivityContext. That functionality is available internally.

Proposed API

namespace System.Diagnostics
{
    public readonly partial struct ActivityContext : IEquatable<ActivityContext>
    {
+        public static bool TryParse(string w3cId, string? traceState, out ActivityContext context);
    }
}

What I'm looking for is a way to expose to users the internal parsing so I can turn my strings into a valid ActivityContext without a lot of additional work.

Workaround

What I'm doing right now is just implementing some of the internal logic myself...

		public static ActivityContext CreateActivityContextFromW3CTraceContext(string traceParent, string? traceState = null)
		{
			if (string.IsNullOrEmpty(traceParent))
				throw new ArgumentNullException(nameof(traceParent));

			if (traceParent.Length != 55)
				throw new FormatException($"TraceParent [{traceParent}] format is not supported.");

			ReadOnlySpan<char> traceIdSpan = traceParent.AsSpan(3, 32);
			ReadOnlySpan<char> spanIdSpan = traceParent.AsSpan(36, 16);

			return new ActivityContext(
				ActivityTraceId.CreateFromString(traceIdSpan),
				ActivitySpanId.CreateFromString(spanIdSpan),
				(ActivityTraceFlags)ConversionExtensions.HexByteFromChars(traceParent[53], traceParent[54]),
				traceState);
		}

/cc @tarekgh @noahfalk @cijothomas @eddynaka @alanwest

[ghost]

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

[cijothomas]

Similar ask: open-telemetry/opentelemetry-dotnet#934

[tarekgh]

This kind of nice to have but not really blocking as the workaround is really easy and simple.

The other workaround it to use reflection to call

runtime/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs

Line 842 in 5d1af65

internal static bool TryConvertIdToContext(string id, out ActivityContext context)

. or even you may copy the code from there.

This is pushed to next release we may look more if we need to expose it.

[CodeBlanch]

You and I have very different views on what is easy and simple 🤣 The goal is to call the ctor on ActivityLink. Not something out of the norm or unexpected, right? Calling Activity.TryParseW3CTraceContext and passing the result would be easy and simple. The workaround means first understanding how Id/TraceParent is composed. Then going through the source to figure out the internal logic. Then stitching up a reflection call or reproducing the logic, which is calling other internal things like HexByteFromChars. I think anyone that has to do this will agree it's a lot of effort just to call the ctor that exists.

The proposal makes the API more usable with an easy and simple tweak 😉 Isn't that why we do the previews? Anyway please reconsider including this.

[tarekgh]

You and I have very different views on what is easy and simple

fair enough. I said simple because the work around is a couple of lines I am pasing below.

The proposal makes the API more usable with an easy and simple tweak 😉 Isn't that why we do the previews?

We are in the stage of considering the blocking issues or the issues that very difficult to achieve. I don't think this one is either of them.

The goal is to call the ctor on ActivityLink. Not something out of the norm or unexpected, right? Calling Activity.TryParseW3CTraceContext and passing the result would be easy and simple. The workaround means first understanding how Id/TraceParent is composed. Then going through the source to figure out the internal logic. Then stitching up a reflection call or reproducing the logic, which is calling other internal things like HexByteFromChars. I think anyone that has to do this will agree it's a lot of effort just to call the ctor that exists.

Here is the workaround I am mentioning which really simple and also creates a wrapper method to create ActivityContext from the Id.

        delegate bool TryConvertIdToContext(string w3cId, out ActivityContext context);
        public static bool CreateActivityContext(string w3cId, out ActivityContext context) =>
            return ((TryConvertIdToContext)Delegate.CreateDelegate(typeof(TryConvertIdToContext), null, typeof(Activity).GetMethod("TryConvertIdToContext", BindingFlags.NonPublic | BindingFlags.Static)))(w3cId, out context);
        // we can optimize the allocation by caching the delegate too :-)

Your calling code should be simple enough as if calling the ActivityContext constructor:

	CreateActivityContext("00-99d43cb30a4cdb4fbeee3a19c29201b0-e82825765f051b47-01", out ActivityContext context); // equivalent to the future constructor: ActivityContext context = new ActivityContext("00-99d43cb30a4cdb4fbeee3a19c29201b0-e82825765f051b47-01");
    // you can check the result if needed. In the future if we have ActivityContext constructor, this may throw and you'll try-catch that I guess.

You can do the same idea for ActivityLink if you like.

[CodeBlanch]

We are in the stage of considering the blocking issues or the issues that very difficult to achieve. I don't think this one is either of them.

My closing statements I promise 😄 Thanks for indulging me.

I think it will block the adoption. Once this ActivitySource API drops our mission in OTel will be to go out and get library authors to use it to instrument their code. That job is going to be a lot harder without a really usable API. Imagine how well it is going to go when we tell the Confluent Kafka client team they need to add Activity links on message consumption but they will need drills reflection to do it.

PS: The above solution doesn't work fully if you also want TraceState in the ActivityContext. TryConvertIdToContext only handles the TraceParent. The solution I have in the description works but up to the user to implement their own "HexToByte" extension.

[tarekgh]

My closing statements I promise 😄 Thanks for indulging me.

Your feedback is always valuable. we are discussing here the importance of this API. I am seeing the API is useful and helpful, only I am trying to know if it is urgent for this release or can wait. Anyway, please keep replying expressing your opinion. We'll discuss this issue and get back to you.

[alanwest]

Just to chiming in with my two-cents here...

I'm with @CodeBlanch on the importance of this API given that the concept of links between spans has become an integral part of the OpenTelemetry specification. At this point I can't say I'm sure which vendors have backend support for links yet, though it will naturally be on their roadmaps. I work for New Relic, and at the moment New Relic does not yet have a way for visualizing links, though discussions are definitely underway. I have to imagine this is true for other vendors. ApplicationInsights maybe?

I'm definitely excited about the fact that .NET has embraced OTel and is baking its concepts directly into native .NET APIs, but with OpenTelemetry being pretty cutting edge, it does mean that things are moving and changing rapidly. Given that 1) links seem pretty well established in the OpenTelemetry specification and 2) .NET has already made the effort to encompass the concept as an ActivityLink, it seems like this suggestion is lower risk. Maybe?

Thanks for all your hard work! It's really cool seeing all this come together.

[CodeBlanch]

I think where it will be really important is messaging clients. Adding a "follows-from" type of link to the original TraceParent. That's what I'm working on right now where I ran into the gap. The spec also talks about scatter/gather pattern and batching cases.

[tarekgh]

We have discussed internally and we'll look at getting this in 5.0. Here is the suggestion of the APIs

namespace System.Diagnostics
{
    public readonly partial struct ActivityContext : IEquatable<ActivityContext>
    {
        public bool TryParse(string w3cId, string? traceState, out ActivityContext context);
    }
}

The reason preferring this to avoid throwing in the constructor which make the caller code ugly.

Also, if we provided this, should be enough to parse and create ActivityLink. So, ActivityLink proposed API could be not needed at least for now.

Please let me know what you think as soon as you can. thanks for your feedback.

[CodeBlanch]

@tarekgh LGTM

[tarekgh]

Thanks @CodeBlanch for your quick reply. do you mind updating the the issue description with the proposal? we should be ok to schedule this for the design review after that.

[CodeBlanch]

@tarekgh Done. I made it static in the proposal. @cijothomas pointed that out to me, it seemed like an oversight. Let me know if you want me to switch to instance-based.

[tarekgh]

@CodeBlanch good catch. Thanks a lot. Looks good to me. We should be good to proceed with that.

[terrajobst]

Video

• We should rename w3cid to traceParent
• We considered offering a span-based overload but right now the most common case for the use of the API starts with a header value which is currently only ever going to be a string. If the networking layer will give access in a span-based fashion we can add it.
• We should have a corresponding Parse method

namespace System.Diagnostics
{
    public readonly partial struct ActivityContext : IEquatable<ActivityContext>
    {
        public static ActivityContext Parse(string traceParent, string? traceState);
        public static bool TryParse(string traceParent, string? traceState, out ActivityContext context);
    }
}

[CodeBlanch]

@tarekgh May I take a stab at a PR for this?

[tarekgh]

@CodeBlanch I am already finishing it. Sorry, if I know you are interested in doing it I would left it for you.

[CodeBlanch]

@tarekgh No prob! Thanks for getting this in.

[tarekgh]

#38419
#39087
dotnet/designs#134
dotnet/designs#98
#31373