Distributed tracing - implement traceparent from w3c Trace Context #168
Conversation
Also small imrovements for the DT sample.
Adding WebApiSample project.
|
To verify compliance, you could run the test-suite that we provided here: https://github.com/w3c/trace-context/tree/master/test There is a sample app for C#: https://github.com/distributed-tracing/samples/tree/master/csharp |
Comments are not part of JSON, so removed those.
Woo, haven't seen that yet. I'll test with it! Thanks! |
|
@gregkalapos When you say
Do you mean all Elastic APM agents don't implement it or is it just .NET Agent? |
That applies to all APM Agents, none of them implement |
Codecov Report
@@ Coverage Diff @@
## master #168 +/- ##
==========================================
+ Coverage 84.72% 85.21% +0.49%
==========================================
Files 47 47
Lines 1460 1468 +8
Branches 263 276 +13
==========================================
+ Hits 1237 1251 +14
+ Misses 134 133 -1
+ Partials 89 84 -5
Continue to review full report at Codecov.
|
Found this during testing with DT - ids should be lowercase (see trace context spec)
|
Adapted the validation logic based on the Obviously I wanna do some perf work on this, and I'll also go through @SergeyKleyman review. Once that's done I think it's ready to merge. |
|
Got rid of the Perf before: Perf after: Perf. test scenario: Probably we could still gain a lot by using |
Add more asserts on traceflags, to also cover other things than 00 and 01.
| @@ -112,7 +106,10 @@ internal static bool TryExtractTraceparent(string traceParentValue, out string t | |||
|
|
|||
| try | |||
| { | |||
| traceOptions = StringToByteArray(traceParentValue, VersionAndTraceIdAndSpanIdLength, OptionsLength); | |||
| var fields = StringToByteArray(traceParentValue, VersionAndTraceIdAndSpanIdLength, OptionsLength); | |||
There was a problem hiding this comment.
You don't really need to allocate fields array, right? You can just have (Hex)StringToByte which reads 2 chars from start index in src string.
There was a problem hiding this comment.
I did this, but the perf. difference is almost nothing - I measured it. This got it to 383.2 ns... and reference less. I leave it like that, since it's still a plus.
There was a problem hiding this comment.
Could you please clarify what do you mean by?
since it's still a plus
If you would like to keep (Hex)StringToByteArray for future use you can have it call (Hex)StringToByte to parse each byte.
It's not a big issue - I am just curious what pros you see in leaving the array.
I personally see only cons - it makes the code less readable since the code does something that the spec doesn't require and it has performance impact.
There was a problem hiding this comment.
By "it's a plus" I meant that it's positive for performance, so it's better to keep it that way. So I guess we are on the same page :)
I agree, there is no reason to keep the array, and the current version already got rid of that, so I guess we are ok here. In case we need it later we can add it, but atm we don't need it, so I also think it's better to avoid that unnecessary allocation.
Also added IDisposable to the DistributedTracingAspNetCoreTests, because the DiagnosticListeners did not unsubscribe.
traceparentheader iselastic-apm-traceparentand nottraceparent. This is what other agents do. Reason: the w3c document is a draft, and it can change. To avoid breaking changes and unwanted behaviour we go with a different header name. Once the draft becomes a standard we can rename the header.tracestateheader yet.SampleAspNetCoreAppvia Http.