dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.16k stars 4.72k forks source link

[API Proposal]: Expose Id on System.Diagnostics.ActivityContext #97899

Open CodeBlanch opened 8 months ago

CodeBlanch commented 8 months ago

Background and motivation

OpenTelemetry .NET's propagation API is centered around ActivityContext. Anytime we need to propagate a span/Activity (for example outgoing http call) we generate a W3C traceparent:

https://github.com/open-telemetry/opentelemetry-dotnet/blob/ef942a11ac264d7b0f754a78dd3b4dd7d4d2c6f3/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs#L116-L121

This is a string allocation and takes some CPU cycles.

If a trace is NOT sampled, we create a single root span/Activity. That root span/Activity may be propagated many times servicing a request (if it calls a lot of downstream services). In that case we're going to be generating the same string each time.

The idea here is basically a performance optimization.

Activity has this Id property which is the same W3C traceparent as what OTel is building, but it is cached so subsequent calls don't incur the perf hit to generate it.

The goal here is to expose that same Id on ActivityContext and utilize the cache mechanism where we can.

Proof of concept diff: https://github.com/dotnet/runtime/compare/main...CodeBlanch:activitycontext-id?expand=1

API Proposal

namespace System.Diagnostics;

public readonly partial struct ActivityContext
{
+    public string Id { get; }
}

API Usage

// OTel W3C propagation implementation
public class TraceContextPropagator : TextMapPropagator
{
    public override void Inject<T>(PropagationContext context, T carrier, Action<T, string, string> setter)
    {
         // ...
+        var traceparent = context.ActivityContext.Id;
-#if NET6_0_OR_GREATER
-        var traceparent = string.Create(55, context.ActivityContext, WriteTraceParentIntoSpan);
-#else
-        var traceparent = string.Concat("00-", context.ActivityContext.TraceId.ToHexString(), "-", -context.ActivityContext.SpanId.ToHexString());
-        traceparent = string.Concat(traceparent, (context.ActivityContext.TraceFlags & ActivityTraceFlags.Recorded) != 0 ? "-01" : "-00");
-#endif
         // ...
    }
}

Alternative Designs

No response

Risks

No response

ghost commented 8 months ago

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

Issue Details
### Background and motivation OpenTelemetry .NET's propagation API is centered around `ActivityContext`. Anytime we need to propagate a span/`Activity` (for example outgoing http call) we generate a W3C `traceparent`: https://github.com/open-telemetry/opentelemetry-dotnet/blob/ef942a11ac264d7b0f754a78dd3b4dd7d4d2c6f3/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs#L116-L121 This is a string allocation and takes some CPU cycles. If a trace is NOT sampled, we create a single root span/`Activity`. That root span/`Activity` may be propagated many times servicing a request (if it calls a lot of downstream services). In that case we're going to be generating the same string each time. The idea here is basically a performance optimization. `Activity` has this [Id](https://github.com/dotnet/runtime/blob/0d8c1e298bb6a896e9b9796832073b8fa9f73ecd/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L217) property which is the same W3C `traceparent` as what OTel is building, but it is cached so subsequent calls don't incur the perf hit to generate it. The goal here is to expose that same `Id` on `ActivityContext` and utilize the cache mechanism where we can. Proof of concept diff: https://github.com/dotnet/runtime/compare/main...CodeBlanch:activitycontext-id?expand=1 ### API Proposal ```diff namespace System.Diagnostics; public readonly partial struct ActivityContext { + public string Id { get; } } ``` ### API Usage ```diff // OTel W3C propagation implementation public class TraceContextPropagator : TextMapPropagator { public override void Inject(PropagationContext context, T carrier, Action setter) { // ... + var traceparent = context.ActivityContext.Id; -#if NET6_0_OR_GREATER - var traceparent = string.Create(55, context.ActivityContext, WriteTraceParentIntoSpan); -#else - var traceparent = string.Concat("00-", context.ActivityContext.TraceId.ToHexString(), "-", -context.ActivityContext.SpanId.ToHexString()); - traceparent = string.Concat(traceparent, (context.ActivityContext.TraceFlags & ActivityTraceFlags.Recorded) != 0 ? "-01" : "-00"); -#endif // ... } } ``` ### Alternative Designs _No response_ ### Risks _No response_
Author: CodeBlanch
Assignees: -
Labels: `api-suggestion`, `area-System.Diagnostics.Tracing`, `untriaged`
Milestone: -
tarekgh commented 8 months ago

Note that, this proposal will increase the ActivityContext size which I guess it is used a lot especially ActivitContext is a struct which is possible get copied in some scenarios. You are not concerned about this?

I marked this to future and we can get it back to current release if we see a demand need it soon.

ghost commented 8 months ago

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 OpenTelemetry .NET's propagation API is centered around `ActivityContext`. Anytime we need to propagate a span/`Activity` (for example outgoing http call) we generate a W3C `traceparent`: https://github.com/open-telemetry/opentelemetry-dotnet/blob/ef942a11ac264d7b0f754a78dd3b4dd7d4d2c6f3/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs#L116-L121 This is a string allocation and takes some CPU cycles. If a trace is NOT sampled, we create a single root span/`Activity`. That root span/`Activity` may be propagated many times servicing a request (if it calls a lot of downstream services). In that case we're going to be generating the same string each time. The idea here is basically a performance optimization. `Activity` has this [Id](https://github.com/dotnet/runtime/blob/0d8c1e298bb6a896e9b9796832073b8fa9f73ecd/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L217) property which is the same W3C `traceparent` as what OTel is building, but it is cached so subsequent calls don't incur the perf hit to generate it. The goal here is to expose that same `Id` on `ActivityContext` and utilize the cache mechanism where we can. Proof of concept diff: https://github.com/dotnet/runtime/compare/main...CodeBlanch:activitycontext-id?expand=1 ### API Proposal ```diff namespace System.Diagnostics; public readonly partial struct ActivityContext { + public string Id { get; } } ``` ### API Usage ```diff // OTel W3C propagation implementation public class TraceContextPropagator : TextMapPropagator { public override void Inject(PropagationContext context, T carrier, Action setter) { // ... + var traceparent = context.ActivityContext.Id; -#if NET6_0_OR_GREATER - var traceparent = string.Create(55, context.ActivityContext, WriteTraceParentIntoSpan); -#else - var traceparent = string.Concat("00-", context.ActivityContext.TraceId.ToHexString(), "-", -context.ActivityContext.SpanId.ToHexString()); - traceparent = string.Concat(traceparent, (context.ActivityContext.TraceFlags & ActivityTraceFlags.Recorded) != 0 ? "-01" : "-00"); -#endif // ... } } ``` ### Alternative Designs _No response_ ### Risks _No response_
Author: CodeBlanch
Assignees: -
Labels: `api-suggestion`, `untriaged`, `area-System.Diagnostics.Activity`
Milestone: Future