Propagators Support #54465

tarekgh · 2021-06-20T20:10:48Z

No description provided.

dotnet-issue-labeler · 2021-06-20T20:10:54Z

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 · 2021-06-20T20:10:56Z

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

Issue Details

Author:	tarekgh
Assignees:	-
Labels:	`area-System.Diagnostics.Tracing`, `new-api-needs-documentation`
Milestone:	-

ghost · 2021-06-20T20:11:23Z

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

Issue Details

Author:	tarekgh
Assignees:	-
Labels:	`* NO MERGE *`, `area-System.Diagnostics.Activity`, `new-api-needs-documentation`
Milestone:	-

MihaZupan

Just a few questions/suggestions around the API shape, thanks!

...aries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs

MihaZupan · 2021-06-21T09:48:37Z

...aries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs

+        public abstract bool Inject(System.Diagnostics.Activity activity, object carrier, Action<object, string, string> setter);
+        public abstract bool Inject(System.Diagnostics.ActivityContext context, object carrier, Action<object, string, string> setter);
+        public abstract bool Inject(System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, string?>> baggage, object carrier, Action<object, string, string> setter);
+        public abstract bool Extract(object carrier, string fieldName, PropagatorGetterCallback getter, out string? value);
+        public abstract bool Extract(object carrier, PropagatorGetterCallback getter, out System.Diagnostics.ActivityContext context);
+        public abstract bool Extract(object carrier, PropagatorGetterCallback getter, out System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, string?>>? baggage);


Overriding 6 methods seems like a lot if a simple use case might be

public sealed class SkipOneActivityLayerPropagator : TextMapPropagator { public override bool Inject(Activity activity, object carrier, Action<object, string, string> setter) => TextMapPropagator.CreateW3CPropagator().Inject(activity.Parent, carrier, setter); }

Maybe this isn't a problem if it won't be common for users to implement these?

As a side note, such a propagator is how one could solve #41072.

I am not sure if we need to force the default implementation here as whoever subclass TextMapPropagator should be aware about this implementation to decide if need to override it or not.

MihaZupan · 2021-06-21T09:51:07Z

...aries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs

+        public abstract bool Inject(System.Diagnostics.Activity activity, object carrier, Action<object, string, string> setter);
+        public abstract bool Inject(System.Diagnostics.ActivityContext context, object carrier, Action<object, string, string> setter);
+        public abstract bool Inject(System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, string?>> baggage, object carrier, Action<object, string, string> setter);
+        public abstract bool Extract(object carrier, string fieldName, PropagatorGetterCallback getter, out string? value);


When would this overload be used?

This is needed for asp.net when reading hierarchical ids with the header name Request-Id. The other Extract overloads will not be able to read such Id.

Thanks. I assume it's not needed for the inject since it can be represented by the Activity?

That is right. I am trying to discourage the usage of the hierarchical ids so I wouldn't expose Inject overload for that. I expose the overload that takes Activity object only to support the usage in http client so you don't need to carry any logic there and can just use the propagator as a black box.

MihaZupan · 2021-06-21T09:54:55Z

...aries/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs

+        public static TextMapPropagator DefaultPropagator { get; set; }
+        public static TextMapPropagator CreateLegacyPropagator() { throw null; }
+        public static TextMapPropagator CreatePassThroughPropagator() { throw null; }
+        public static TextMapPropagator CreateOutputSuppressionPropagator() { throw null; }
+        public static TextMapPropagator CreateW3CPropagator() { throw null; }


As an implementation detail, these should be caching a single instance IMO (each is allocating a HashSet internally so it's not just an empty object)

Yes, this is implementation details. We can cache the instances we return if needed. I didn't bother in the protype to go to that level.

noahfalk · 2021-06-22T03:31:04Z

src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/TextMapPropagator.cs

+        public static TextMapPropagator CreateLegacyPropagator() => new LegacyTextMapPropagator();
+
+        // Suppress context propagation.
+        public static TextMapPropagator CreateOutputSuppressionPropagator() => new OutputSuppressionPropagator();


I was chatting a bit with @shirhatti about locking down what kind of customization is in scope and what isn't. For example this propagator has legacy behavior for ingress and no egress. If someone wanted to only support TraceParent on ingress + no egress presumably they would need to write their own propagator. Likewise if someone wanted to egress TraceParent but not baggage they would also need their own propagator. I am fine with those customizations being out of scope, I just want to make sure everyone is in agreement and it aligns with what we've heard from customers.

I agree there will be some scenarios users may need to write their own customized propagators. I would like to see first what these scenarios would be. If such scenarios truly little tweaks from what we are exposing, we still have the option to introduce more overloads to the static methods which can easily achieve such scenarios. something like CreateOutputSuppressionPropagator(bool forceW3C).

noahfalk · 2021-06-22T03:37:40Z

src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/TextMapPropagator.cs

+        public static TextMapPropagator CreateOutputSuppressionPropagator() => new OutputSuppressionPropagator();
+
+        // propagate only root parent context and ignore any intermediate created context.
+        public static TextMapPropagator CreatePassThroughPropagator() => new PassThroughPropagator();


Similar to the above, this PassThroughPropagator is implemented to always emit baggage using the Correlation-Context header (even if it was received via the W3C baggage header). If someone wanted a different behavior such as always emitting with the W3C baggage header or using different baggage headers in different circumstances they would need to author their own propagator. No objection from me, but I want to highlight which customizations are supported and which aren't to make sure everyone is in agreement.

noahfalk · 2021-06-22T03:45:21Z

src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/TextMapPropagator.cs

+        public abstract bool Extract(object carrier, PropagatorGetterCallback getter, out string? id, out string? state);
+        public abstract bool Extract(object carrier, PropagatorGetterCallback getter, out ActivityContext context);
+        public abstract bool Extract(object carrier, PropagatorGetterCallback getter, out IEnumerable<KeyValuePair<string, string?>>? baggage);
+


I'm trying to avoid digging into the details of the API as long as it appears to satisfy the goals we wanted to achieve. I think this API can achieve it, but I have more faith in the practical demonstration than in my mental model : )

We already trying the prototype with http client and asp.net to ensure no gaps or issues with that design.

tarekgh · 2021-07-10T02:27:10Z

Closing the as we already have official PR #55419.

Propagators Support

7c5d554

dotnet-issue-labeler bot added area-System.Diagnostics.Tracing new-api-needs-documentation labels Jun 20, 2021

tarekgh added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-System.Diagnostics.Activity and removed area-System.Diagnostics.Tracing labels Jun 20, 2021

MihaZupan reviewed Jun 21, 2021

View reviewed changes

tarekgh added 2 commits June 21, 2021 10:52

Address Feedback

cf75376

Update Extract method.

f3b285a

noahfalk reviewed Jun 22, 2021

View reviewed changes

tarekgh added 3 commits June 22, 2021 19:47

Fix space trimming and baggage order

7f4ebc5

Tests

fa7d0d9

More changes

ec81f5a

tarekgh closed this Jul 10, 2021

karelz mentioned this pull request Jul 28, 2021

[WinHttpHandler] Long running test: ResponseHeadersRead_SynchronizationContextNotUsedByHandler on Win7/Win8 #54034

Closed

ghost locked as resolved and limited conversation to collaborators Aug 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagators Support #54465

Propagators Support #54465

tarekgh commented Jun 20, 2021

dotnet-issue-labeler bot commented Jun 20, 2021

ghost commented Jun 20, 2021

ghost commented Jun 20, 2021

MihaZupan left a comment

MihaZupan Jun 21, 2021 •

edited

Loading

tarekgh Jun 21, 2021

MihaZupan Jun 21, 2021

tarekgh Jun 21, 2021

MihaZupan Jun 21, 2021

tarekgh Jun 21, 2021

MihaZupan Jun 21, 2021

tarekgh Jun 21, 2021

noahfalk Jun 22, 2021

tarekgh Jun 22, 2021

noahfalk Jun 22, 2021

noahfalk Jun 22, 2021

tarekgh Jun 22, 2021

tarekgh commented Jul 10, 2021

Propagators Support #54465

Propagators Support #54465

Conversation

tarekgh commented Jun 20, 2021

dotnet-issue-labeler bot commented Jun 20, 2021

ghost commented Jun 20, 2021

ghost commented Jun 20, 2021

MihaZupan left a comment

Choose a reason for hiding this comment

MihaZupan Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh commented Jul 10, 2021

MihaZupan Jun 21, 2021 •

edited

Loading