-
Notifications
You must be signed in to change notification settings - Fork 438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Baggage API : refactor trace_state, and add Baggage implementation. #610
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
#include "opentelemetry/common/kv_properties.h" | ||
#include "opentelemetry/nostd/shared_ptr.h" | ||
#include "opentelemetry/nostd/string_view.h" | ||
#include "opentelemetry/version.h" | ||
|
||
OPENTELEMETRY_BEGIN_NAMESPACE | ||
namespace baggage | ||
{ | ||
|
||
class Baggage | ||
{ | ||
|
||
public: | ||
Baggage() : kv_properties_(new opentelemetry::common::KeyValueProperties()) {} | ||
|
||
template <class T> | ||
Baggage(const T &keys_and_values) | ||
: kv_properties_(new opentelemetry::common::KeyValueProperties(T)) | ||
{} | ||
|
||
bool GetValue(const nostd::string_view &key, std::string &value) | ||
{ | ||
return kv_properties_->GetValue(key, value); | ||
} | ||
|
||
// @return all key-values entris by repeatedly invoking the function reference passed as argument | ||
// for each entry | ||
bool GetAllEntries( | ||
nostd::function_ref<bool(nostd::string_view, nostd::string_view)> callback) const noexcept | ||
{ | ||
return kv_properties_->GetAllEntries( | ||
callback); // [&callback](nostd::string_view key, nostd::string_view value) { | ||
} | ||
|
||
nostd::shared_ptr<Baggage> Delete(const nostd::string_view &key) | ||
{ | ||
nostd::shared_ptr<Baggage> baggage(new Baggage(kv_properties_->Size() - 1)); | ||
kv_properties_->GetAllEntries( | ||
[&baggage, &key](nostd::string_view e_key, nostd::string_view e_value) { | ||
if (key != e_key) | ||
baggage->kv_properties_->AddEntry(e_key, e_value); | ||
return true; | ||
}); | ||
return baggage; | ||
} | ||
|
||
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); | ||
// add rest of the fields. | ||
kv_properties_->GetAllEntries([&baggage](nostd::string_view key, nostd::string_view value) { | ||
baggage->kv_properties_->AddEntry(key, value); | ||
return true; | ||
}); | ||
return baggage; | ||
} | ||
|
||
private: | ||
// Store entries in a C-style array to avoid using std::array or std::vector. | ||
|
||
nostd::unique_ptr<opentelemetry::common::KeyValueProperties> kv_properties_; | ||
Baggage(size_t size) : kv_properties_(new opentelemetry::common::KeyValueProperties(size)){}; | ||
}; | ||
} // namespace baggage | ||
OPENTELEMETRY_END_NAMESPACE |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,148 @@ | ||
#include "opentelemetry/nostd/function_ref.h" | ||
#include "opentelemetry/nostd/shared_ptr.h" | ||
#include "opentelemetry/nostd/string_view.h" | ||
#include "opentelemetry/nostd/unique_ptr.h" | ||
#include "opentelemetry/version.h" | ||
|
||
#include <string> | ||
|
||
OPENTELEMETRY_BEGIN_NAMESPACE | ||
namespace common | ||
{ | ||
|
||
// Class to store fixed size array of key-value pairs of string type | ||
class KeyValueProperties | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there reasoning to not use std library containers, e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is to guarantee ABI stability in api. We don't use std library
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, good to know! 🙂 |
||
{ | ||
// Class to store key-value pairs of string types | ||
public: | ||
class Entry | ||
{ | ||
public: | ||
Entry() : key_(nullptr), value_(nullptr){}; | ||
|
||
// Copy constructor | ||
Entry(const Entry ©) | ||
{ | ||
key_ = CopyStringToPointer(copy.key_.get()); | ||
value_ = CopyStringToPointer(copy.value_.get()); | ||
} | ||
|
||
// Copy assignment operator | ||
Entry &operator=(Entry &other) | ||
{ | ||
key_ = CopyStringToPointer(other.key_.get()); | ||
value_ = CopyStringToPointer(other.value_.get()); | ||
return *this; | ||
} | ||
|
||
// Move contructor and assignment operator | ||
Entry(Entry &&other) = default; | ||
Entry &operator=(Entry &&other) = default; | ||
|
||
// Creates an Entry for a given key-value pair. | ||
Entry(nostd::string_view key, nostd::string_view value) noexcept | ||
{ | ||
key_ = CopyStringToPointer(key); | ||
value_ = CopyStringToPointer(value); | ||
} | ||
|
||
// Gets the key associated with this entry. | ||
nostd::string_view GetKey() const { return key_.get(); } | ||
|
||
// Gets the value associated with this entry. | ||
nostd::string_view GetValue() const { return value_.get(); } | ||
|
||
// Sets the value for this entry. This overrides the previous value. | ||
void SetValue(nostd::string_view value) { value_ = CopyStringToPointer(value); } | ||
|
||
private: | ||
// Store key and value as raw char pointers to avoid using std::string. | ||
nostd::unique_ptr<const char[]> key_; | ||
nostd::unique_ptr<const char[]> value_; | ||
|
||
// Copies string into a buffer and returns a unique_ptr to the buffer. | ||
// This is a workaround for the fact that memcpy doesn't accept a const destination. | ||
nostd::unique_ptr<const char[]> CopyStringToPointer(nostd::string_view str) | ||
{ | ||
char *temp = new char[str.size() + 1]; | ||
memcpy(temp, str.data(), str.size()); | ||
temp[str.size()] = '\0'; | ||
return nostd::unique_ptr<const char[]>(temp); | ||
} | ||
}; | ||
|
||
// Maintain the number of entries in entries_. | ||
size_t num_entries_; | ||
|
||
// Max size of allocated array | ||
size_t max_num_entries_; | ||
// Store entries in a C-style array to avoid using std::array or std::vector. | ||
nostd::unique_ptr<Entry[]> entries_; | ||
|
||
public: | ||
// Create Key-value list of given size | ||
// @param size : Size of list. | ||
KeyValueProperties(size_t size) | ||
: num_entries_(0), max_num_entries_(size), entries_(new Entry[size]) | ||
{} | ||
|
||
// Create Empty Key-Value list | ||
KeyValueProperties() : num_entries_(0), max_num_entries_(0), entries_(nullptr) {} | ||
|
||
/** | ||
* TBD - Check if Key-Value iterable, and has size() method | ||
*/ | ||
|
||
template <class T> | ||
KeyValueProperties(const T &keys_and_values) | ||
: max_num_entries_(keys_and_values.size()), entries_(new Entry[max_num_entries_]) | ||
{ | ||
size_t index = 0; | ||
for (auto &e : keys_and_values) | ||
{ | ||
Entry entry(keys_and_values.first, keys_and_values.second); | ||
(entries_.get())[num_entries_++] = entry; | ||
} | ||
} | ||
|
||
void AddEntry(const nostd::string_view &key, const nostd::string_view &value) | ||
{ | ||
if (num_entries_ < max_num_entries_) | ||
{ | ||
Entry entry(key, value); | ||
(entries_.get())[num_entries_++] = entry; | ||
} | ||
} | ||
|
||
bool GetAllEntries( | ||
nostd::function_ref<bool(nostd::string_view, nostd::string_view)> callback) const noexcept | ||
{ | ||
for (size_t i = 0; i < num_entries_; i++) | ||
{ | ||
auto entry = (entries_.get())[i]; | ||
if (!callback(entry.GetKey(), entry.GetValue())) | ||
{ | ||
return false; | ||
} | ||
} | ||
return true; | ||
} | ||
|
||
bool GetValue(const nostd::string_view key, std::string &value) | ||
{ | ||
for (size_t i = 0; i < num_entries_; i++) | ||
{ | ||
auto entry = (entries_.get())[i]; | ||
if (entry.GetKey() == key) | ||
{ | ||
value = std::string(entry.GetValue()); | ||
return true; | ||
} | ||
} | ||
return false; | ||
} | ||
|
||
size_t Size() { return num_entries_; } | ||
}; | ||
} // namespace common | ||
OPENTELEMETRY_END_NAMESPACE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pyohannes - would like to hear your thoughts too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nikhil- That's true. just wondering if you have any further questions on this design ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.