Baggage API : refactor trace_state, and add Baggage implementation. #610
Conversation
lalitb
changed the title
Codecov Report
@@ Coverage Diff @@
## main #610 +/- ##
==========================================
- Coverage 94.45% 94.45% -0.01%
==========================================
Files 197 198 +1
Lines 9183 9195 +12
==========================================
+ Hits 8674 8685 +11
- Misses 509 510 +1
|
| nostd::shared_ptr<Baggage> Set(const nostd::string_view &key, const nostd::string_view &value) | ||
| { | ||
| nostd::shared_ptr<Baggage> baggage(new Baggage(kv_properties_->Size() + 1)); | ||
| baggage->kv_properties_->AddEntry(key, value); |
There was a problem hiding this comment.
I have a question here about the key_value pairs in the baggage. With this implementation, we are adding duplicate keys in our baggage (eg. if key already existed in the baggage, we still prepend a new entry with the same key). Do we want to keep this behaviour or we should avoid adding old entry in our returned baggage.
Standard says to keep only one value for each baggage name ref: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/baggage/api.md#overview
We are always returning fresh value in getter so in a way it does not seem to be a big issue. If we keep this behaviour same, I think we need to change couple of things in Delete function as size could be less than kv_properties_->Size() - 1 (line 37)
Let me know what do you think about this. Please correct me if I am missing anything.
There was a problem hiding this comment.
Good point, we should first add the k-v pair from current baggage except for key passed as argument ( if it exist). And in the end, add the new k-v. It should be fine to store one less element than actual allocation as long as we track number of elements internally. What do you think ?
There was a problem hiding this comment.
Yes, I think that should be the way. One more thing, I am planning to use linked list (deep clone everytime) in kv_properties for optimum memory usage (as I don't see usecase of any random access). Currently, context also uses linked list but its not deep clone as context does not have delete key api. So, context code will be unchanged in this refactoring.
Do you agree with the approach ? Wanted to give you heads up before making change to make sure that I am not missing anything.
There was a problem hiding this comment.
What benefit do you see with linked list as compared to fixed size array ( which we use now )? Is it because of extra processing we may have to do initially to find exact allocation size in case of array?
The idea is to minimize multiple dynamic memory allocation in api. And iterating a linked list would be slow as compared to array due to non-contiguous memory allocation of it's nodes.
There was a problem hiding this comment.
@pyohannes - would like to hear your thoughts too.
There was a problem hiding this comment.
Nikhil- That's true. just wondering if you have any further questions on this design ?
There was a problem hiding this comment.
Should I keep the array structure only and not the linked list ? (and not worry about extra memory usage for some corner cases)
There was a problem hiding this comment.
I think, I can also clarify thing about baggage metadata. We have not incorporated baggage metadata here. In my solution, I had kept baggage metadata along with key, value. Its an opaque structure with no semantic meaning for now so languages like golang are not keeping it separately (keeping it as a part of value). How should we proceed ? I am kind of stuck there. We might want to make kv_properties more generic if we decide to keep it separately.
There was a problem hiding this comment.
Should I keep the array structure only and not the linked list ? (and not worry about extra memory usage for some corner cases)
Yes that's correct.
There was a problem hiding this comment.
I think, I can also clarify thing about baggage metadata. We have not incorporated baggage metadata here. In my solution, I had kept baggage metadata along with key, value. Its an opaque structure with no semantic meaning for now so languages like golang are not keeping it separately (keeping it as a part of value). How should we proceed ? I am kind of stuck there. We might want to make kv_properties more generic if we decide to keep it separately.
I won't be much worried about incorporating metadata as of now, and probably revisit once it's semantics/usage is properly defined by W3C. Don't see languages like python, dotnet supporting it too.
| @@ -18,13 +18,16 @@ | |||
| #include <cstring> | |||
| #include <string> | |||
|
|
|||
| #include <iostream> | |||
| { | ||
|
|
||
| // Class to store fixed size array of key-value pairs of string type | ||
| class KeyValueProperties |
There was a problem hiding this comment.
Is there reasoning to not use std library containers, e.g. unordered_map over KeyValueProperties?
There was a problem hiding this comment.
This is to guarantee ABI stability in api. We don't use std library
- in api interface as arguments to virtual member functions (https://github.com/open-telemetry/opentelemetry-cpp/blob/main/docs/abi-policy.md)
- as member variables inside api classes.
|
closing for #676 |
Fixes #79
Changes
Please provide a brief description of the changes here.
Converting to draft for some more refactoring, and add more tests.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes