Added Http Trace Context#143
Conversation
Codecov Report
@@ Coverage Diff @@
## master #143 +/- ##
==========================================
+ Coverage 94.43% 94.55% +0.11%
==========================================
Files 146 146
Lines 6597 6610 +13
==========================================
+ Hits 6230 6250 +20
+ Misses 367 360 -7
|
|
|
||
| # Bazel files | ||
| /bazel-* | ||
| /.idea/ |
There was a problem hiding this comment.
This seems to be IDE specific files, would you put them in a separate section (and put a comment)?
There was a problem hiding this comment.
Yes, I'll do that right away. Actually, do you mind to see the other PR I've made sometime before?
It contains the header I'd like to merge into while this PR is more about consulting about some of the coding details.
There was a problem hiding this comment.
Why are the lines below needed? Isn't the whole .idea subdir ignored?
Also I'm not sure this belongs in the .gitignore file at all. I'd like to avoid e.g. every single IDE/editor being covered here.
There was a problem hiding this comment.
They should have been removed by now.
There was a problem hiding this comment.
Do you still plan to keep this line?
There was a problem hiding this comment.
I've commited those changes to remove it.
|
|
||
| # Bazel files | ||
| /bazel-* | ||
| /.idea/ |
There was a problem hiding this comment.
Why are the lines below needed? Isn't the whole .idea subdir ignored?
Also I'm not sure this belongs in the .gitignore file at all. I'd like to avoid e.g. every single IDE/editor being covered here.
|
Okay, so from today's meeting, I learned that I have to separate the code I made now into api and sdk folders. Could anyone specify more which should be in api and which should be in sdks? |
Origin/propagators
…propagators # Conflicts: # api/include/opentelemetry/trace/propagation/http_trace_context.h
Origin/propagators
| } | ||
| } | ||
|
|
||
| static void GenerateBuffer(nostd::string_view string, int bytes, uint8_t *buf) |
There was a problem hiding this comment.
The name GenerateBuffer is misleading.
There was a problem hiding this comment.
Changed name to GenerateHexFromString
| mutable std::mutex mu_; | ||
| std::unique_ptr<Recordable> recordable_; | ||
| opentelemetry::core::SteadyTimestamp start_steady_time; | ||
| std::shared_ptr<trace_api::SpanContext> span_context_; |
There was a problem hiding this comment.
Not sure if I am missing something, but how is SpanContext going to be initialized. Don't see this passed in constructor.
There was a problem hiding this comment.
Also, see whether it need to be unique_ptr - ideally Span should fully own it's context. <shared_ptr> have performance impact so should be avoided unless we can't.
| namespace trace | ||
| { | ||
| class DefaultSpan : public Span | ||
| { |
There was a problem hiding this comment.
nitpik: Can we use override keyword for the function which are virtually defined in Span base class. Will make this class more readable.
| */ | ||
| nostd::unique_ptr<Span> StartSpan(nostd::string_view name, | ||
| const KeyValueIterable &attributes, | ||
| const StartSpanOptions &options = {}) noexcept |
There was a problem hiding this comment.
nitpik: override this, and below two functions.
|
|
||
| private: | ||
| std::shared_ptr<Tracer> tracer_; | ||
| SpanContext span_context_; |
There was a problem hiding this comment.
how is this being initialized? should it be in constructor ?
There was a problem hiding this comment.
This is just a placeholder to conform to its parent class Span, I don't think it's being used.
| return context.SetValue(span_key, sp); | ||
| } | ||
|
|
||
| static void GetCurrentSpan(const context::Context &context, SpanContext &span_context) |
There was a problem hiding this comment.
should this have public access ?
There was a problem hiding this comment.
Yes because it can be used elsewhere and so it is public in Python version I believe
| setter(carrier, kTraceParent, hex_string); | ||
| } | ||
|
|
||
| static void InjectImpl(Setter setter, T &carrier, const SpanContext &span_context) |
There was a problem hiding this comment.
should this have public access ? please check this for all the member functions.
There was a problem hiding this comment.
I think all is cool except for the GenerateHexFromString, the rest are useful because they are useful functions outside of this class as well and they also need tests to make sure they are correct, although I have changed the GenerateHexFromString to private.
| static const nostd::string_view kTraceParent = "traceparent"; | ||
| static const nostd::string_view kTraceState = "tracestate"; | ||
| static const int kTraceDelimiterBytes = 3; | ||
| static const int kHeaderElementLengths[4] = {2, 32, 16, 2}; |
There was a problem hiding this comment.
This is confusing now. It's important to know what these numbers indicate - which is getting missed now. Better to have constants as defined earlier, and use them within kHeaderElementLengths array. Also, size of trace_id is 16 bytes, and span_id is 8 bytes. Why are we using 32 bytes and 16 bytes here ?
There was a problem hiding this comment.
I just followed a recommendation to remove the definitions and convert them into this form though. I could provide comments on the size to bolster understanding.
There was a problem hiding this comment.
Assuming you're referring to #143 (comment), there wasn't a recommendation to remove the definitions. reyang pointed out that the numbers are repeated, but didn't say "get rid of the constants".
I agree with lalitb: the constants give useful mnemonics to the values.
Regarding why the values are {2,32,16,2} - these are characters in a hex string, which encode nibbles not bytes. The count is 2:1 with the byte size.
There was a problem hiding this comment.
So I just added those constants back but this time with those constants using the values stored in kHeadeElementLengths as definition.
| }; | ||
| } // namespace propagation | ||
| } // namespace trace | ||
| OPENTELEMETRY_END_NAMESPACE No newline at end of file |
There was a problem hiding this comment.
nit: put a blank line before EOF (and change the other files).
| { | ||
| static const nostd::string_view kTraceParent = "traceparent"; | ||
| static const nostd::string_view kTraceState = "tracestate"; | ||
| static const int kTraceDelimiterBytes = 3; |
There was a problem hiding this comment.
Why do we need this? Should this be the number of components - 1?
There was a problem hiding this comment.
Yes right now it is the number of componets - 1, but this variable is used to calculate the length of a trace parent hence I think it might make more sense to refer to it as a length unit.
reyang
left a comment
reyang
left a comment
There was a problem hiding this comment.
LGTM, left some minor suggestions.
reyang
added
the
pr:please-merge
Update dependency rules_go to v0.52.0
9 participants
This PR is has the full implemented version of Http Text Format i.e. Http Trace Context without the Trace State. All Trace State related code are commented out for the sake of compilation.
Modification is made to span.h to remove tracer field from class Span because otherwise Span and Tracer would define upon each other.