-
Notifications
You must be signed in to change notification settings - Fork 559
Context api alternative implementation #180
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
Changes from all commits
7867296
d5a360a
5d3b713
d047ff7
f283cc6
e1de13f
d5b155d
80b2332
64cd1c2
4d24115
a17c028
d2df2ac
c316636
7b36945
8e02c07
115cd68
0d9fea1
565c40e
2d4a411
9874c4c
7bceaab
061e564
b5c81ad
b2717b3
b7259cd
34eacf2
a086993
e44e926
6edce29
dff6f8f
e660ab7
aec8ca8
605cb75
e4d3069
786f8a6
62b0c88
9c39536
92f8bb7
4f39935
821bfb4
25813c8
ec12e39
7ac58e2
cf403b7
09b0d50
6979e81
6b75dec
e91e0fe
0f870e0
3593653
67a7978
cab9bc9
6bf0821
ada7c68
134d6b4
00aee4c
9a42949
d8432dd
00ed02a
d3d6b30
5c59f5c
ac8503f
9125477
80bf695
9d3d643
4781d74
7d0a145
a4d9b73
ea288a5
0f1d485
3b922a5
39941d3
e1f2cfa
f4e2718
f1f2480
ce6d9fa
c7cbb21
f2542fd
ba9da8e
a9c4eea
a7fbbd0
dd3f297
8292012
7534cbc
c925962
b2e53b2
067e7dd
8396792
86b032c
f78ab74
77c4c9a
f63d595
5f25cf1
86eacca
7a2ea4c
961f7ed
4358d35
c5f6c08
80cbb96
896bc45
feb3f17
acffb61
bf94af8
5eddd2f
78bd8a6
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,166 @@ | ||
| #pragma once | ||
|
|
||
| #include "opentelemetry/context/context_value.h" | ||
| #include "opentelemetry/nostd/shared_ptr.h" | ||
| #include "opentelemetry/nostd/string_view.h" | ||
|
|
||
| OPENTELEMETRY_BEGIN_NAMESPACE | ||
| namespace context | ||
| { | ||
|
|
||
| // The context class provides a context identifier. Is built as a linked list | ||
| // of DataList nodes and each context holds a shared_ptr to a place within | ||
| // the list that determines which keys and values it has access to. All that | ||
| // come before and none that come after. | ||
| class Context | ||
| { | ||
|
|
||
| public: | ||
| // Creates a context object from a map of keys and identifiers, this will | ||
| // hold a shared_ptr to the head of the DataList linked list | ||
| template <class T> | ||
| Context(const T &keys_and_values) | ||
| { | ||
| head_ = nostd::shared_ptr<DataList>{new DataList(keys_and_values)}; | ||
| } | ||
|
|
||
| // Creates a context object from a key and value, this will | ||
| // hold a shared_ptr to the head of the DataList linked list | ||
| Context(nostd::string_view key, ContextValue value) | ||
| { | ||
| head_ = nostd::shared_ptr<DataList>{new DataList(key, value)}; | ||
| } | ||
|
|
||
| // Accepts a new iterable and then returns a new context that | ||
| // contains the new key and value data. It attaches the | ||
| // exisiting list to the end of the new list. | ||
| template <class T> | ||
| Context SetValues(T &values) noexcept | ||
| { | ||
| Context context = Context(values); | ||
| nostd::shared_ptr<DataList> &last = context.head_; | ||
| while (last->next_ != nullptr) | ||
| { | ||
| last = last->next_; | ||
| } | ||
| last->next_ = head_; | ||
| return context; | ||
| } | ||
|
|
||
| // Accepts a new iterable and then returns a new context that | ||
| // contains the new key and value data. It attaches the | ||
| // exisiting list to the end of the new list. | ||
| Context SetValue(nostd::string_view key, ContextValue value) noexcept | ||
| { | ||
| Context context = Context(key, value); | ||
| context.head_->next_ = head_; | ||
| return context; | ||
| } | ||
|
|
||
| // Returns the value associated with the passed in key. | ||
| context::ContextValue GetValue(const nostd::string_view key) noexcept | ||
| { | ||
| for (DataList *data = head_.get(); data != nullptr; data = data->next_.get()) | ||
| { | ||
| if (key.size() == data->key_length_) | ||
| { | ||
| if (memcmp(key.data(), data->key_, data->key_length_) == 0) | ||
| { | ||
| return data->value_; | ||
| } | ||
| } | ||
| } | ||
| return (int64_t)0; | ||
|
Member
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. Not blocking this PR, do we need a nil type?
Contributor
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. Boost has a boost::blank type for this purpose. The more C++ idiomatic way would be top wrap the return value in a std::optional, but if feel skeptical about extending I also think this shouldn't block this PR. |
||
| } | ||
|
|
||
| // Checks for key and returns true if found | ||
| bool HasKey(const nostd::string_view key) noexcept | ||
| { | ||
| for (DataList *data = head_.get(); data != nullptr; data = data->next_.get()) | ||
| { | ||
| if (key.size() == data->key_length_) | ||
| { | ||
| if (memcmp(key.data(), data->key_, data->key_length_) == 0) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| private: | ||
| Context() = default; | ||
|
|
||
| // A linked list to contain the keys and values of this context node | ||
| class DataList | ||
| { | ||
| public: | ||
| nostd::shared_ptr<DataList> next_; | ||
|
|
||
| char *key_; | ||
|
|
||
| size_t key_length_; | ||
|
|
||
| ContextValue value_; | ||
|
|
||
| DataList() { next_ = nullptr; } | ||
|
|
||
| // Builds a data list off of a key and value iterable and returns the head | ||
| template <class T> | ||
| DataList(const T &keys_and_vals) : key_{nullptr} | ||
| { | ||
| bool first = true; | ||
| auto *node = this; | ||
| for (auto &iter : keys_and_vals) | ||
| { | ||
| if (first) | ||
| { | ||
| *node = std::move(DataList(iter.first, iter.second)); | ||
| first = false; | ||
| } | ||
| else | ||
| { | ||
| node->next_ = nostd::shared_ptr<DataList>(new DataList(iter.first, iter.second)); | ||
| node = node->next_.get(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Builds a data list with just a key and value, so it will just be the head | ||
| // and returns that head. | ||
| DataList(nostd::string_view key, ContextValue value) | ||
| { | ||
| key_ = new char[key.size()]; | ||
| key_length_ = key.size(); | ||
| memcpy(key_, key.data(), key.size() * sizeof(char)); | ||
| value_ = value; | ||
| next_ = nostd::shared_ptr<DataList>{nullptr}; | ||
| } | ||
|
|
||
| DataList &operator=(DataList &&other) | ||
| { | ||
| key_length_ = other.key_length_; | ||
| value_ = std::move(other.value_); | ||
| next_ = std::move(other.next_); | ||
|
|
||
| key_ = other.key_; | ||
| other.key_ = nullptr; | ||
|
|
||
| return *this; | ||
| } | ||
|
|
||
| ~DataList() | ||
| { | ||
| if (key_ != nullptr) | ||
| { | ||
| delete[] key_; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| // Head of the list which holds the keys and values of this context | ||
| nostd::shared_ptr<DataList> head_; | ||
| }; | ||
| } // namespace context | ||
| OPENTELEMETRY_END_NAMESPACE | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| #pragma once | ||
|
|
||
| #include <cstdint> | ||
|
|
||
| #include "opentelemetry/nostd/shared_ptr.h" | ||
| #include "opentelemetry/nostd/span.h" | ||
| #include "opentelemetry/nostd/unique_ptr.h" | ||
| #include "opentelemetry/nostd/variant.h" | ||
| #include "opentelemetry/trace/span_context.h" | ||
| #include "opentelemetry/version.h" | ||
|
|
||
| OPENTELEMETRY_BEGIN_NAMESPACE | ||
| namespace context | ||
| { | ||
| using ContextValue = | ||
| nostd::variant<bool, int64_t, uint64_t, double, nostd::shared_ptr<trace::SpanContext>>; | ||
| } // namespace context | ||
| OPENTELEMETRY_END_NAMESPACE |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| load("//bazel:otel_cc_benchmark.bzl", "otel_cc_benchmark") | ||
|
|
||
| cc_test( | ||
| name = "context_test", | ||
| srcs = [ | ||
| "context_test.cc", | ||
| ], | ||
| deps = [ | ||
| "//api", | ||
| "@com_google_googletest//:gtest_main", | ||
| ], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| include(GoogleTest) | ||
|
|
||
| foreach(testname context_test) | ||
| add_executable(${testname} "${testname}.cc") | ||
| target_link_libraries(${testname} ${GTEST_BOTH_LIBRARIES} | ||
| ${CMAKE_THREAD_LIBS_INIT} opentelemetry_api) | ||
| gtest_add_tests(TARGET ${testname} TEST_PREFIX context. TEST_LIST ${testname}) | ||
| endforeach() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| #include "opentelemetry/context/context.h" | ||
|
|
||
| #include <map> | ||
|
|
||
| #include <gtest/gtest.h> | ||
|
|
||
| using namespace opentelemetry; | ||
|
|
||
| // Tests that the context constructor accepts an std::map. | ||
| TEST(ContextTest, ContextIterableAcceptsMap) | ||
| { | ||
| std::map<std::string, context::ContextValue> map_test = {{"test_key", (int64_t)123}}; | ||
| context::Context test_context = context::Context(map_test); | ||
| } | ||
|
|
||
| // Tests that the GetValue method returns the expected value. | ||
| TEST(ContextTest, ContextGetValueReturnsExpectedValue) | ||
| { | ||
| std::map<std::string, context::ContextValue> map_test = {{"test_key", (int64_t)123}, | ||
| {"foo_key", (int64_t)456}}; | ||
| context::Context test_context = context::Context(map_test); | ||
| EXPECT_EQ(nostd::get<int64_t>(test_context.GetValue("test_key")), 123); | ||
| EXPECT_EQ(nostd::get<int64_t>(test_context.GetValue("foo_key")), 456); | ||
| } | ||
|
|
||
| // Tests that the SetValues method accepts an std::map. | ||
| TEST(ContextTest, ContextSetValuesAcceptsMap) | ||
| { | ||
| std::map<std::string, context::ContextValue> map_test = {{"test_key", (int64_t)123}}; | ||
| std::map<std::string, context::ContextValue> map_test_write = {{"foo_key", (int64_t)456}}; | ||
|
|
||
| context::Context test_context = context::Context(map_test); | ||
| context::Context foo_context = test_context.SetValues(map_test_write); | ||
|
|
||
| EXPECT_EQ(nostd::get<int64_t>(foo_context.GetValue("test_key")), 123); | ||
| EXPECT_EQ(nostd::get<int64_t>(foo_context.GetValue("foo_key")), 456); | ||
| } | ||
|
|
||
| // Tests that the SetValues method accepts a nostd::string_view and | ||
| // context::ContextValue. | ||
| TEST(ContextTest, ContextSetValuesAcceptsStringViewContextValue) | ||
| { | ||
| nostd::string_view string_view_test = "string_view"; | ||
| context::ContextValue context_value_test = (int64_t)123; | ||
|
|
||
| context::Context test_context = context::Context(string_view_test, context_value_test); | ||
| context::Context foo_context = test_context.SetValue(string_view_test, context_value_test); | ||
|
|
||
| EXPECT_EQ(nostd::get<int64_t>(foo_context.GetValue(string_view_test)), 123); | ||
| } | ||
|
|
||
| // Tests that the original context does not change when a value is | ||
| // written to it. | ||
| TEST(ContextTest, ContextImmutability) | ||
| { | ||
| std::map<std::string, context::ContextValue> map_test = {{"test_key", (int64_t)123}}; | ||
|
|
||
| context::Context context_test = context::Context(map_test); | ||
| context::Context context_foo = context_test.SetValue("foo_key", (int64_t)456); | ||
|
|
||
| EXPECT_NE(nostd::get<int64_t>(context_test.GetValue("foo_key")), 456); | ||
| } | ||
|
|
||
| // Tests that writing the same to a context overwrites the original value. | ||
| TEST(ContextTest, ContextKeyOverwrite) | ||
| { | ||
| std::map<std::string, context::ContextValue> map_test = {{"test_key", (int64_t)123}}; | ||
|
|
||
| context::Context context_test = context::Context(map_test); | ||
| context::Context context_foo = context_test.SetValue("test_key", (int64_t)456); | ||
|
|
||
| EXPECT_EQ(nostd::get<int64_t>(context_foo.GetValue("test_key")), 456); | ||
| } | ||
|
|
||
| // Tests that the new Context Objects inherits the keys and values | ||
| // of the original context object. | ||
| TEST(ContextTest, ContextInheritance) | ||
| { | ||
| using M = std::map<std::string, context::ContextValue>; | ||
|
|
||
| M m1 = {{"test_key", (int64_t)123}, {"foo_key", (int64_t)456}}; | ||
| M m2 = {{"other_key", (int64_t)789}}; | ||
|
|
||
| context::Context test_context = context::Context(m1); | ||
| context::Context foo_context = test_context.SetValues(m2); | ||
|
|
||
| EXPECT_EQ(nostd::get<int64_t>(foo_context.GetValue("test_key")), 123); | ||
| EXPECT_EQ(nostd::get<int64_t>(foo_context.GetValue("foo_key")), 456); | ||
| } | ||
|
|
||
| // Tests that copying a context copies the key value pairs as expected. | ||
|
reyang marked this conversation as resolved.
|
||
| TEST(ContextTest, ContextCopyOperator) | ||
| { | ||
| std::map<std::string, context::ContextValue> test_map = { | ||
| {"test_key", (int64_t)123}, {"foo_key", (int64_t)456}, {"other_key", (int64_t)789}}; | ||
|
|
||
| context::Context test_context = context::Context(test_map); | ||
| context::Context copied_context = test_context; | ||
|
|
||
| EXPECT_EQ(nostd::get<int64_t>(copied_context.GetValue("test_key")), 123); | ||
| EXPECT_EQ(nostd::get<int64_t>(copied_context.GetValue("foo_key")), 456); | ||
| EXPECT_EQ(nostd::get<int64_t>(copied_context.GetValue("other_key")), 789); | ||
| } | ||
|
|
||
| // Tests that the Context accepts an empty map. | ||
| TEST(ContextTest, ContextEmptyMap) | ||
| { | ||
| std::map<std::string, context::ContextValue> map_test = {}; | ||
| context::Context test_context = context::Context(map_test); | ||
| } | ||
|
|
||
| // Tests that if a key exists within a context has key will return true | ||
| // false if not. | ||
| TEST(ContextTest, ContextHasKey) | ||
| { | ||
| std::map<std::string, context::ContextValue> map_test = {{"test_key", (int64_t)123}}; | ||
| context::Context context_test = context::Context(map_test); | ||
| EXPECT_TRUE(context_test.HasKey("test_key")); | ||
| EXPECT_FALSE(context_test.HasKey("foo_key")); | ||
| } | ||
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.
Will we need the functionality to retrieve a parent context for a context? Will this be needed for modifying the current context at some point?
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 can't think of an instance, can you?
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'm not 100% sure, maybe I'm missing some part of the high-level view.
I think we'll use
Contextto obtain the currently active span: we will have a method to obtain the current context, the current context will hold theSpanContextof the active span. I'm trying to figure out necessary steps to make this possible:SetValue("current_span", span_context). Then the new span will be stored in the new current context.I think we'll need some kind of parenting of contexts, but I'm not sure if it will be managed by the
Contextitself or by some other class.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 believe that will be handled within the threadlocal context class
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.
The implementation of which is described by the context spec
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.
Or rather, interface of