Add W3C propagator #102
Add W3C propagator #102
Conversation
| { | ||
| return getter(traceIdConvention); | ||
| // W3C propagator requires Otel TraceId convention as it's specification clearly states lengths of traceId and spanId values in the header. | ||
| return IsW3CButNotOtelTraceId(propagator, traceIdConvention) |
There was a problem hiding this comment.
I would throw an unsupported exception here. Cause if config is asking for W3C and gets default, it might cause more misunderstanding.
There was a problem hiding this comment.
Well, that actually sounds reasonable 👍
| if (context == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(context)); | ||
| } | ||
|
|
||
| if (carrier == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(carrier)); | ||
| } | ||
|
|
||
| if (setter == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(setter)); | ||
| } |
There was a problem hiding this comment.
can you "collate" it into 3 lines, please 😄
There was a problem hiding this comment.
I know this is just a matter of styling, but I hate one line ifs. I would rather leave it as it is if we don't have strict rules on such things.
There was a problem hiding this comment.
TBH I see no value of null checks for internals. It just adds noise :-)
There was a problem hiding this comment.
Yep, I did it to be consistent with other propagators. TBH if nobody opposes I would love to get rid of that :)
| } | ||
|
|
||
| var spanIdString = traceParentHeader.Substring(VersionAndTraceIdLength, SpanIdLength); | ||
| if (ulong.TryParse(spanIdString, NumberStyles.HexNumber, CultureInfo.InvariantCulture, out var spanId)) |
There was a problem hiding this comment.
I propose to invert the if statement so that in "main" flow we have the "positive" scenario
| /// <summary> | ||
| /// Gets the trace state for W3C propagation | ||
| /// </summary> | ||
| internal string TraceState { get; } |
There was a problem hiding this comment.
this is not following the OTEL model: https://github.com/open-telemetry/opentelemetry-specification/blob/cf59c0b868744b764ca8b9175668abc6755fecca/specification/trace/api.md#spancontext
See an example implementation here: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Api/Trace/SpanContext.cs
There was a problem hiding this comment.
I have redone how this is parsed.
| @@ -100,6 +116,11 @@ private SpanContext(TraceId? traceId, string serviceName) | |||
| /// </summary> | |||
| internal string Origin { get; } | |||
|
|
|||
| /// <summary> | |||
| /// Gets the trace state for W3C propagation | |||
There was a problem hiding this comment.
Maybe something like:
TraceState carries vendor-specific trace identification data, represented as a list of key-value pairs. TraceState allows multiple tracing systems to participate in the same trace. It is fully described in the W3C Trace Context specification: https://www.w3.org/TR/trace-context/#tracestate-header.
| // W3C propagator requires Otel TraceId convention as it's specification clearly states lengths of traceId and spanId values in the header. | ||
| if (propagator == PropagatorType.W3C && traceIdConvention is not OtelTraceIdConvention) | ||
| { | ||
| throw new NotSupportedException($"'{PropagatorType.W3C}' propagator requires '{ConventionType.OpenTelemetry}' convention to be set"); |
There was a problem hiding this comment.
@pjanotti @zacharycmontoya @nrcventura
I am not sure if we should throw an exception and crash the instrumented application in case of improper configuration. As far as I see in all other places there is a fallback to defaults in case of improper value.
Besides, I think we should have some docs to describe the strategies for error handling e.g. when we should log vs crash etc.
There was a problem hiding this comment.
For me it seems very confusing if the application does not respect the values and starts behaving differently.
- Either fail fast (+ log the reason of the failure)
- Settings should be consistent for the whole system. So instead of creating default propagator, the setting should be reverting back to the default value (+ log the reason). Otherwise other parts of the system will still see the invalid value but don't know about the actual behavior. Which means it could lead to weird bugs.
There was a problem hiding this comment.
I would go with the 1. here, but after giving it a second thought that would make any .NET app crash on the machine which can be disturbing.
There was a problem hiding this comment.
In general, I am also in favour of 1.
However, maybe there are reasons that we should not go that way. Especially that currently it does not work that way if I follow the code correctly.
There was a problem hiding this comment.
@dszmigielski I think it's very important point - already forgot that the default behavior is to instruct every dotnet application.
Wondering why the internals of tracer are not protected? If there is a fatal failure it should put the tracer to offline mode (+ log the reason) but leave the original application alive.
There was a problem hiding this comment.
This time it seems ok
TBH now I am not sure. We would need to check what happens if we attach the profiler e.g. to an app that only prints Hello world to the console.
There was a problem hiding this comment.
In this case, it makes sense to bubble up an exception through the Tracer constructor because some combination of configuration was invalid. However, I don't think we account for this currently. We currently use a lazy initializer and get the default Tracer instance:
Inside Instrumentation.Initialize would be a good place to
- Catch the exception from the constructor
- Log the exception from the constructor
- Maybe set
Tracer.Instance = new Tracer(new TracerSettings { TraceEnabled = false } );so it can't do anymore harm, even though the IL rewriting will still occur?
There was a problem hiding this comment.
ad. 3
I think it would be better to additionally "log" to the console (stdout) and then close the application. Otherwise, the user may not be aware that the auto-instrumentation is not working.
There was a problem hiding this comment.
At the moment this will not fail until the first instrumentation attempt is made, so it's no near perfect. Even though I would close this PR and create an issue for fixing this behaviour separately.
There was a problem hiding this comment.
Yes this issue can be handled separately, and we can discuss what the side effect of bad configuration is. At Datadog, we have a strong opinion that we should absolutely minimize changes to the application's behavior when attaching the tracer, so in our tracer we would not choose to stop the application because of bad configuration.
|
|
||
| public SpanContext Extract<T>(T carrier, Func<T, string, IEnumerable<string>> getter) | ||
| { | ||
| var traceStateCollection = getter(carrier, W3CHeaderNames.TraceState); |
There was a problem hiding this comment.
It maybe slightly more efficient to read the tracestate right before returning the final SpanContext so you don't spend time reading headers you may not use.
| var traceStateCollection = getter(carrier, W3CHeaderNames.TraceState); | ||
| var traceState = ExtractTraceState(traceStateCollection); | ||
|
|
||
| var traceParentValues = getter(carrier, W3CHeaderNames.TraceParent).ToList(); |
There was a problem hiding this comment.
In the SDK, they get the Count() of the resulting enumerable and then access the traceparent by getting the First() of the resulting enumerable. That seems slightly more efficient so we don't allocate a new List when we don't really need to
There was a problem hiding this comment.
Hmm, this could cause multiple enumeration, so I would rather not do that.
There was a problem hiding this comment.
I think Zach's point is valid. string.Join(",", traceParentValues) could be just traceParentHeader, cause traceParentValues.Count != 1 will not let you past anyway. So you are left with Count() and First() operations - which means full list will be never needed.
There was a problem hiding this comment.
... or even better, FirstOrDefault() single enumerable operation.
There was a problem hiding this comment.
The FirstOrDefault would be cool but then we have no way of checking if there was more than one value, and for now in this case we return null SpanContext.
There was a problem hiding this comment.
sorry, I meant SingleOrDefault(), my bad.
|
Just make GH issue for leftovers, please. |
This PR introduces W3C propagator. One thing to have in mind is that this feature will work only for app configured to use OpenTelemetry TraceId convention. Because of that there is a need for an iffy if in ContextPropagatorBuilder. Please have a closer look at that when reviewing, maybe someone will come up with a better idea how to handle that.