Rename of Propagators #1427
Rename of Propagators #1427
Conversation
| /// </summary> | ||
| public sealed class B3Propagator : IPropagator | ||
| public sealed class B3Propagator : ITextMapPropagator |
There was a problem hiding this comment.
Do we want an interface ITextMapPropagator or an abstract class TextMapPropagator?
Interface gives benefit of multi-inheritance, which I don't see a useful scenario here (an extreme case would be having TraceContextPropagator implementing both the text and binary interface).
On the other side interface forbids us from adding optional methods in the future (will have to introduce ITextMapPropagator2, ITextMapPropagator3, ...).
There was a problem hiding this comment.
yes i was thinking.. and agree :)
There was a problem hiding this comment.
Base class seems like a good idea.
Just FYI as of C#8 you can add default implementation on an interface. Check out: https://devblogs.microsoft.com/dotnet/default-implementations-in-interfaces/
I haven't ever used it, but it's there 😄
There was a problem hiding this comment.
Makes me wondering - if we have class Foobar: IFoo, IBar:
- Both
IFooandIBardecided to add a new default implementation and they share the same name 😂 IFooused to haveIFoo.SayHelloand nowIBardecided to add the same thing 😢
There was a problem hiding this comment.
Moved to abstract base class now. We follow same approach for others part in SDK, like base processor, exporter etc.
Codecov Report
@@ Coverage Diff @@
## master #1427 +/- ##
==========================================
+ Coverage 81.31% 81.89% +0.57%
==========================================
Files 227 227
Lines 6117 6086 -31
==========================================
+ Hits 4974 4984 +10
+ Misses 1143 1102 -41
|
| private const string TraceParent = "traceparent"; | ||
| private const string TraceState = "tracestate"; | ||
|
|
||
| private static readonly int VersionPrefixIdLength = "00-".Length; |
There was a problem hiding this comment.
not blocking this PR, I wonder if we could use const instead of readonly.
| private const string TraceState = "tracestate"; | ||
|
|
||
| private static readonly int VersionPrefixIdLength = "00-".Length; | ||
| private static readonly int TraceIdLength = "0af7651916cd43dd8448eb211c80319c".Length; |
There was a problem hiding this comment.
not blocking this PR, the official name (according to the TraceContext spec) should be ParentId instead of TraceId.
There was a problem hiding this comment.
This TraceId is TraceId itself in TraceContext spec. ParentId is what we call SpanId. But spec says "Span-id" is alternate name for parentid.
There was a problem hiding this comment.
Sorry I was looking at a wrong place, meant to say SpanIdLength should be ParentIdLength.
| private static readonly int SpanIdLength = "00f067aa0ba902b7".Length; | ||
| private static readonly int VersionAndTraceIdAndSpanIdLength = "00-0af7651916cd43dd8448eb211c80319c-00f067aa0ba902b7-".Length; | ||
| private static readonly int OptionsLength = "00".Length; | ||
| private static readonly int TraceparentLengthV0 = "00-0af7651916cd43dd8448eb211c80319c-00f067aa0ba902b7-00".Length; |
There was a problem hiding this comment.
This one probably can be TraceParent instead of Traceparent (we've finished the long time debate).
Renamed TextMapPropagator to TraceContextPropagator, CompositePropapagor
to CompositeTextMapPropagator. IPropagator is renamed to TextMapPropagator
and changed from interface to abstract class.
In prep for #581.
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes