Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Developers will be able to control how distributed tracing information is propagated #50658

Closed
Tracked by #5929
shirhatti opened this issue Apr 2, 2021 · 10 comments · Fixed by #55419
Closed
Tracked by #5929
Labels
api-approved API was approved in API review, it can be implemented area-System.Diagnostics.Activity Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Milestone

Comments

@shirhatti
Copy link
Contributor

shirhatti commented Apr 2, 2021

Description edited by @tarekgh to include the API proposal

Background and Motivation

When .NET applications need to send their distributed tracing state to another process, they currently rely on functionality baked into HttpClient to transmit this information via HTTP headers (as described in W3C tracecontext spec and baggage spec). Similarly, when ASP.NET application receive distributed tracing information, they rely on the ASP.NET libraries to populate distributed tracing state based on incoming headers.

Unfortunately, the current implementation requires both libraries (HttpClient and ASP.NET) have prior knowledge of the specification. This makes it hard to react to changes in the specification or support other specifications (e.g., B3, Jaeger).

Propagation is a crossing-cutting concern should be solved in System.Diagnostics.DiagnosticsSource rather than require every library to have special knowledge of the wire protocol involved. Solving this in a generic way will also allow other libraries in the .NET ecosystem to participate in distributed tracing without requiring knowledge of the wire protocols involved.

The propagators are part of OpenTelemetry specification too. Having them in .NET is a good step to support it for libraries not taking direct dependencies on the OpenTelemetry.

This proposal also addressing solving customer scenarios that is listed below in the Customer issues addressed section.

Proposed API

namespace System.Diagnostics
{
    public abstract class TextMapPropagator
    {
      public delegate void PropagatorGetterCallback(object carrier, string fieldName, out string? value, out IEnumerable<string>? values);
      public abstract IReadOnlyCollection<string> Fields { get; }
      public abstract void Inject(Activity activity, object carrier, Action<object, string, string> setter);
      public abstract void Extract(object carrier, PropagatorGetterCallback getter, out string? id, out string? state);
      public abstract void Extract(object carrier, PropagatorGetterCallback getter, out IEnumerable<KeyValuePair<string, string?>>? baggage);
      public static TextMapPropagator Current { get; set; }
      public static TextMapPropagator CreateLegacyPropagator() { throw null; }
      public static TextMapPropagator CreatePassThroughPropagator() { throw null; }
      public static TextMapPropagator CreateNoOutputPropagator() { throw null; }
    }
}

Usage Examples

TextMapPropagator propagator = TextMapPropagator.Current;

propagator.Inject(Activity.Current, httpRequest, (carrier1, name, value1) => Console.WriteLine($"{name}: {value1}"));

propagator.Extract(httpRequest, (carrier2, name, out value, out values) => { Console.WriteLine($"{name}"); value =  "Mapped Key Value"; values = null; } );

Customer issues addressed:

Pass-thru mode

@a-elsheikh, @rynowak

There are certain environments (service meshes, dapr) where you are trying to avoid creating a intermediate span. Creating intermediate spans that aren't exported result in the causal chain being broken. (dotnet/aspnetcore#30392)

Proposed solution:

  • Create Activity with predetermined SpanId (which is equal to the parent's span id). This will require a new API
  • Call ActivityListeners, i.e., no special heuristics to suppress Activity Created callbacks when in pass-thru mode. Though we expect the use case here is that there are no listeners in the process.
  • Open question: How does the library (ASP.NET) know that is in this special pass-thru mode?
    • Is it a new static property on Activity?
    • Or is it a library specific setting?
    • Add a bit to ActivityContext to indicate we're in pass-thru mode
  • This scenario will be broken by uncooperative libraries who create child spans, but we will do nothing special to mitigate this

React to spec changes

@karelz, @MihaZupan, @Tratcher

As an example, the W3C baggage specification changed the name of the header from Correlation-Context to Baggage. We'd need a way to change to version and change propagation behavior without requiring every library to react to spec changes

-#45496
-dotnet/aspnetcore#28319

Turn off propagation

@karelz, @MihaZupan

There are instances where application want to turn off propagation the currently isn't an easy way to do without requiring every library to add library-specific settings. This could be addressed by adding a no-op propagator.

TODO: Add links to HttpClient issues where customers want to disable header propagation

Potential work items:

  • Add Propagator (abstract base class) to System.Diagnostics.DiagnosticSource
  • Add implementation for W3C propagator W3C baggage specs still not finalized. we'll wait to expose this propagator till the specs are final.
  • Add implementation for no-op propagator
  • Add a static DefaultPropagator to Activity the static property will be on the propagator abstract class (i.e. TextMapPropagator) instead of Activity class.
  • Add API to create Activity with predetermined SpanId this will not be needed after we expose the propagator.
  • Result of the open question
  • Modify System.Net.Http.DiagnosticsHandler to use the Default Propagator
  • Modify Microsoft.AspNetCore.Hosting.HostingApplicationDiagnostics to use the Default Propagator
@shirhatti shirhatti added User Story A single user-facing feature. Can be grouped under an epic. Team:Libraries area-System.Diagnostics.Activity labels Apr 2, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 2, 2021
@ghost
Copy link

ghost commented Apr 2, 2021

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

Issue Details

When .NET applications need to send their distributed tracing state to another process, they currently rely on functionality baked into HttpClient to transmit this information via HTTP headers (as described in W3C tracecontext spec and baggage spec). Similarly, when ASP.NET application receive distributed tracing information, the rely on the ASP.NET libraries to populate distributed tracing state based on incoming headers.

Unfortunately, the current implementation requires both libraries (HttpClient and ASP.NET) have prior knowledge of the specification. This makes it hard to react to changes in the specification or support other specifications (e.g., B3, Jaeger).

Propagation is a crossing-cutting concern should be solved in System.Diagnostics.DiagnosticsSource rather than require every library to have special knowledge of the wire protocol involved. Solving this in a generic way will also allow other libraries in the .NET ecosystem to participate in distributed tracing requiring knowledge of the wire protocols involved.

The OpenTelemetry.NET project has a propagators API that can serve as prior art for a starting point.

namespace OpenTelemetry.Context.Propagation
{
    /// <summary>
    /// Defines an interface for a Propagator of TextMap type,
    /// which uses string key/value pairs to inject and extract
    /// propagation data.
    /// </summary>
    public abstract class TextMapPropagator
    {
        /// <summary>
        /// Gets the list of headers used by propagator. The use cases of this are:
        ///   * allow pre-allocation of fields, especially in systems like gRPC Metadata
        ///   * allow a single-pass over an iterator (ex OpenTracing has no getter in TextMap).
        /// </summary>
        public abstract ISet<string> Fields { get; }

        /// <summary>
        /// Injects the context into a carrier.
        /// </summary>
        /// <typeparam name="T">Type of an object to set context on. Typically HttpRequest or similar.</typeparam>
        /// <param name="context">The default context to transmit over the wire.</param>
        /// <param name="carrier">Object to set context on. Instance of this object will be passed to setter.</param>
        /// <param name="setter">Action that will set name and value pair on the object.</param>
        public abstract void Inject<T>(PropagationContext context, T carrier, Action<T, string, string> setter);

        /// <summary>
        /// Extracts the context from a carrier.
        /// </summary>
        /// <typeparam name="T">Type of object to extract context from. Typically HttpRequest or similar.</typeparam>
        /// <param name="context">The default context to be used if Extract fails.</param>
        /// <param name="carrier">Object to extract context from. Instance of this object will be passed to the getter.</param>
        /// <param name="getter">Function that will return string value of a key with the specified name.</param>
        /// <returns>Context from it's text representation.</returns>
        public abstract PropagationContext Extract<T>(PropagationContext context, T carrier, Func<T, string, IEnumerable<string>> getter);
    }
}
namespace OpenTelemetry.Context.Propagation
{
    /// <summary>
    /// Stores propagation data.
    /// </summary>
    public readonly struct PropagationContext : IEquatable<PropagationContext>
    {
        /// <summary>
        /// Initializes a new instance of the <see cref="PropagationContext"/> struct.
        /// </summary>
        /// <param name="activityContext"><see cref="System.Diagnostics.ActivityContext"/>.</param>
        /// <param name="baggage"><see cref="Baggage"/>.</param>
        public PropagationContext(ActivityContext activityContext, Baggage baggage)
        {
            this.ActivityContext = activityContext;
            this.Baggage = baggage;
        }

        /// <summary>
        /// Gets <see cref="System.Diagnostics.ActivityContext"/>.
        /// </summary>
        public ActivityContext ActivityContext { get; }

        /// <summary>
        /// Gets <see cref="Baggage"/>.
        /// </summary>
        public Baggage Baggage { get; }
    }
}

Customer issues addressed:

Pass-thru mode

@a-elsheikh, @rynowak

There are certain environments (service meshes, dapr) where you are trying to avoid creating a intermediate span. Creating intermediate spans that aren't exported result in the causal chain being broken. (dotnet/aspnetcore#30392)

Proposed solution:

  • Create Activity with predetermined SpanId (which is equal to the parent's span id). This will require a new API
  • Call ActivityListeners, i.e., no special heuristics to suppress Activity Created callbacks when in pass-thru mode. Though we expect the use case here is that there are no listeners in the process.
  • Open question: How does the library (ASP.NET) know that is in this special pass-thru mode?
    • Is it a new static property on Activity?
    • Or is it a library specific setting?
    • Add a bit to ActivityContext to indicate we're in pass-thru mode
  • This scenario will be broken by uncooperative libraries who create child spans, but we will nothing special to mitigate this

React to spec changes

@karelz, @MihaZupan, @Tratcher

As an example, the W3C baggage specification changed the name of the header from Correlation-Context to Baggage. We'd need a way to change to version and change propagation behavior without requiring every library to react to spec changes

-#45496
-dotnet/aspnetcore#28319

Turn off propagation

@karelz, @MihaZupan

There are instances where application want to turn off propagation the currently isn't an easy way to do without requiring every library to add library-specific settings. This could be addressed by adding a no-op propagator.

TODO: Add links to HttpClient issues where customers want to disable header propagation

Potential work items:

  • Add Propagator (abstract base class) to System.Diagnostics.DiagnosticSource
  • Add implementation for W3C propagator
  • Add implementation for no-op propagator
  • Add a static DefaultPropagator to Activity
  • Add API to create Activity with predetermined SpanId
  • Result of the open question
  • Modify System.Net.Http.DiagnosticsHandler to use the DefaultPropagator
  • Modify Microsoft.AspNetCore.Hosting.HostingApplicationDiagnostics to use the DefaultPropagator
Author: shirhatti
Assignees: -
Labels:

Team:Libraries, User Story, area-System.Diagnostics.Activity

Milestone: -

@shirhatti
Copy link
Contributor Author

cc @cijothomas @noahfalk @reyang

@noahfalk
Copy link
Member

noahfalk commented Apr 3, 2021

Add API to create Activity with predetermined SpanId

I think this one was also part of the Open Question? My recollection when we chatted is that we were leaning away from adding a new API that sets spanId and instead add a static property that causes the local child to mirror the remote parent spanId.

@shirhatti
Copy link
Contributor Author

I think this one was also part of the Open Question? My recollection when we chatted is that we were leaning away from adding a new API that sets spanId and instead add a static property that causes the local child to mirror the remote parent spanId.

You're right, I believe the three options were:

  • Add a static property to Activity
  • Use the CreateActivity overload that already takes in ActivityContext. It can be inferred from the ActivityContext
  • Add a new API to CreateActivity with pre-determined SpanId

@noahfalk
Copy link
Member

noahfalk commented Apr 7, 2021

@shirhatti and I just chatted about an alternative idea for how to do pass-through mode propagation. Rather than try to restrict which Activities start/make SpanIds mirror a remote parent, we could have a propagator that emits the original IDs regardless. To do this the propagator would start from Activity.Current, follow the Activity.Parent linked list up to the local root, then emit the root TraceId and ParentSpanId.

@tarekgh tarekgh removed the untriaged New issue has not been triaged by the area owner label Apr 30, 2021
@tarekgh tarekgh added this to the 6.0.0 milestone Apr 30, 2021
@MihaZupan
Copy link
Member

@tarekgh tarekgh added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Jun 7, 2021
@tarekgh tarekgh added api-ready-for-review API is ready for review, it is NOT ready for implementation blocked Issue/PR is blocked on something - see comments labels Jun 29, 2021
@terrajobst
Copy link
Member

Based on your email I assume you meant to mark it as blocking, not blocked

@terrajobst terrajobst added blocking Marks issues that we want to fast track in order to unblock other important work and removed blocked Issue/PR is blocked on something - see comments labels Jun 30, 2021
@tarekgh
Copy link
Member

tarekgh commented Jun 30, 2021

@terrajobst

Based on your email I assume you meant to mark it as blocking, not blocked

Yes, thanks for the catch. I hope we can use other tags to mark the issues ready for design review. It is easy to run into this mistake :-)

@terrajobst terrajobst removed the api-needs-work API needs work before it is approved, it is NOT ready for implementation label Jun 30, 2021
@bartonjs
Copy link
Member

bartonjs commented Jul 6, 2021

  • Since there's already a strong delegate type for the getter, let's use a strong delegate type for the setter to name the string parameters.
  • Extract(out id, out state) should use the "trace" prefix to help callers understand the alignment to (e.g.) Activity.TraceStateString
  • Instead of relying on the shape of the out parameters, the different Extract methods should use purpose names
  • We renamed CreateLegacyPropagator() to CreateDefaultPropagator()
namespace System.Diagnostics
{
    public abstract class TextMapPropagator
    {
      public delegate void PropagatorGetterCallback(object carrier, string fieldName, out string? fieldValue, out IEnumerable<string>? fieldValues);
      public delegate void PropagatorSetterCallback(object carrier, string fieldName, string fieldValue);

      public abstract IReadOnlyCollection<string> Fields { get; }
      public abstract void Inject(Activity activity, object carrier, PropagatorSetterCallback setter);
      public abstract void ExtractTraceIdAndState(object carrier, PropagatorGetterCallback getter, out string? traceId, out string? traceState);
      public abstract IEnumerable<KeyValuePair<string, string?>>? ExtractBaggage(object carrier, PropagatorGetterCallback getter);

      public static TextMapPropagator Current { get; set; }
      public static TextMapPropagator CreateDefaultPropagator() { throw null; }
      public static TextMapPropagator CreatePassThroughPropagator() { throw null; }
      public static TextMapPropagator CreateNoOutputPropagator() { throw null; }
    }
}

@bartonjs bartonjs removed api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Jul 6, 2021
@bartonjs bartonjs added the api-approved API was approved in API review, it can be implemented label Jul 6, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 9, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 10, 2021
@tarekgh
Copy link
Member

tarekgh commented Jul 13, 2021

It has been decided to rename TextMapPropagator to DistributedContextPropagator. The rest of the approved proposal still the same.

#55539

namespace System.Diagnostics
{
    public abstract class DistributedContextPropagator
    {
      public delegate void PropagatorGetterCallback(object? carrier, string fieldName, out string? fieldValue, out IEnumerable<string>? fieldValues);
      public delegate void PropagatorSetterCallback(object? carrier, string fieldName, string fieldValue);

      public abstract IReadOnlyCollection<string> Fields { get; }
      public abstract void Inject(Activity? activity, object? carrier, PropagatorSetterCallback setter);
      public abstract void ExtractTraceIdAndState(object? carrier, PropagatorGetterCallback getter, out string? traceId, out string? traceState);
      public abstract IEnumerable<KeyValuePair<string, string?>>? ExtractBaggage(object? carrier, PropagatorGetterCallback getter);

      public static DistributedContextPropagator Current { get; set; }
      public static DistributedContextPropagator CreateDefaultPropagator() { throw null; }
      public static DistributedContextPropagator CreatePassThroughPropagator() { throw null; }
      public static DistributedContextPropagator CreateNoOutputPropagator() { throw null; }
    }
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Diagnostics.Activity Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants