Refactoring trace_state to reuse common functionality in baggage #638
Conversation
nikhil1511
changed the title
Codecov Report
@@ Coverage Diff @@
## main #638 +/- ##
==========================================
+ Coverage 94.51% 94.66% +0.15%
==========================================
Files 199 203 +4
Lines 9136 9322 +186
==========================================
+ Hits 8635 8825 +190
+ Misses 501 497 -4
|
| if (list_member.size() == 0) | ||
| { | ||
| // empty list member. Move to next entry. For both baggage and trace_state this is valid | ||
| // behaviour. |
There was a problem hiding this comment.
nit - as this tokenizer class is not just for baggage and trace_state, we can remove comment specific to them. Or perhaps add flag whether to skip empty entries ( with default as true ) ?
There was a problem hiding this comment.
Makes sense, I will update the comment. About the flag, I am not sure if we will need this parsing at many places, so thought of not making it too generic.
If we go ahead with the flag approach, do you think we should have that as part of tokenizer's constructor (I feel flag will be same for all next calls) ? In that case we can probably create TokenizerConfigOption struct to be passed to the constructor.
There was a problem hiding this comment.
Yeah, I actually meant this flag for constructor.
| { | ||
|
|
||
| // Iterator for key-value headers | ||
| class KeyValueStringIterator |
There was a problem hiding this comment.
change class name to KeyValueStringTokenizer taking inspiration from strtok function in C ?
| @@ -5,3 +5,25 @@ otel_cc_benchmark( | |||
| srcs = ["spinlock_benchmark.cc"], | |||
| deps = ["//api"], | |||
| ) | |||
|
|
|||
| cc_test( | |||
There was a problem hiding this comment.
Adding the tests to CMake build script as well?
| { | ||
| first = false; | ||
| } | ||
| header_s.append(std::string(key)); |
There was a problem hiding this comment.
This is probably my mistake - but I don't think this conversion (nostd::string_view to std::string) will work properly always (eg if key contains end of line character). As C++11 std::string constructor won't accept std::string_view type. Better to use proper conversion : https://stackoverflow.com/questions/59424390/how-to-correctly-create-stdstring-from-a-stdstring-view
| } | ||
| header_s.append(std::string(key)); | ||
| header_s.append(1, kKeyValueSeparator); | ||
| header_s.append(std::string(value)); |
There was a problem hiding this comment.
Need to check this conversion in other places too
| @@ -7,58 +21,14 @@ namespace | |||
| { | |||
|
|
|||
| using opentelemetry::trace::TraceState; | |||
| namespace nostd = opentelemetry::nostd; | |||
There was a problem hiding this comment.
Its used in the newly added tests (check line 200). I see that there is some inconsistency, at some places opentelemetry::nostd is used and at some nostd is directly used.
There was a problem hiding this comment.
I'd suggest keeping its usage consistent.
| @@ -7,58 +21,14 @@ namespace | |||
| { | |||
|
|
|||
| using opentelemetry::trace::TraceState; | |||
| namespace nostd = opentelemetry::nostd; | |||
There was a problem hiding this comment.
I'd suggest keeping its usage consistent.
Related to #79
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes