Adding baggage api #3
Conversation
| /* Returns shared_ptr of new baggage object which contains new key-value pair. If key or value is | ||
| invalid, copy of current baggage is returned | ||
| */ | ||
| nostd::shared_ptr<Baggage> Set(const nostd::string_view &key, const nostd::string_view &value) |
There was a problem hiding this comment.
Wouldn't it be better to check validity of kv pair first? If not valid, then this copying can be avoided or may be i am missing something from ot specs here.
There was a problem hiding this comment.
Let me think through this. Ideally, we return copy everytime but we might return same copy in case keys do not change.
There was a problem hiding this comment.
I am planning to keep it as it is for now. We are copying in all other cases anyway and so I am not keen on handling this edge case separately. Lets see what other think in main repo.
| : kv_properties_(new opentelemetry::common::KeyValueProperties(keys_and_values)) | ||
| {} | ||
|
|
||
| static nostd::shared_ptr<Baggage> GetDefault() |
There was a problem hiding this comment.
Does spec talk about baggage having a singleton interface for default cases?
There was a problem hiding this comment.
No it doesnt talk about it. We had similar thing in trace_state and so I went ahead with it.
| nostd::shared_ptr<Baggage> Delete(const nostd::string_view &key) | ||
| { | ||
| // keeping size of baggage same as key might not be found in it | ||
| nostd::shared_ptr<Baggage> baggage(new Baggage(kv_properties_->Size())); |
There was a problem hiding this comment.
If key to be deleted is found then size of baggage is one more than the no of keys. isn`t that a bug?
There was a problem hiding this comment.
Its just an upper bound on the number of pairs. Not much of worry. We must make sure that size is not less than the actual size.
Fixes
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes