Context api#4
Conversation
…d added the contextvars_context.h file
Co-authored-by: Kirk Kelsey <kirkmkelsey@gmail.com>
|
|
||
| /* The RuntimeContext class provides a wrapper for | ||
| * propogating context through cpp*/ | ||
| class RuntimeContext { |
There was a problem hiding this comment.
Do you have any thoughts on having two separate classes, versus moving the RuntimeContext methods into context itself?
There was a problem hiding this comment.
I don't really have thoughts either way, I am happy to merge them
Context api content
…-cpp into context_api_content
Context api content
|
|
||
| #include <gtest/gtest.h> | ||
|
|
||
| using namespace opentelemetry::context; |
There was a problem hiding this comment.
Consider putting the tests in the namespace rather than "using" it.
There was a problem hiding this comment.
That seems to be contrary to the tests that already exist, should I do it anyway?
There was a problem hiding this comment.
No, stick with the pattern they have then.
| * user to the context map. The identifier is used as a key | ||
| * to the context map. | ||
| */ | ||
| class ContextKey{ |
There was a problem hiding this comment.
Consider naming this "Key" since it's already scoped to the Context. In places like the test it's currently called Context::ContextKey, which is redundant.
| class ContextKey{ | |
| class Key{ |
| /* Tests whether the original context objects changes | ||
| * when you write to it. | ||
| */ | ||
| TEST(Context_test, is_context_immutable) |
There was a problem hiding this comment.
Avoid underscores in the test name (I'm having a hard time finding public documentation on it, but maybe https://chromium.googlesource.com/external/github.com/google/googletest/+/refs/tags/release-1.8.0/googletest/docs/FAQ.md#why-should-not-test-case-names-and-test-names-contain-underscore)
| TEST(Context_test, is_context_immutable) | |
| TEST(ContextTest, ContextIsImmutable) |
FWIW, some of the existing files (e.g. https://github.com/satac2/opentelemetry-cpp/blob/master/api/test/trace/key_value_iterable_view_test.cc) do have a mix.
|
|
||
| EXPECT_EQ(new_test_context.GetValue(foo_key), 1); | ||
| } | ||
|
|
There was a problem hiding this comment.
Two other tests I can think of are
- you can override a key (maybe in the same context and in a derived context)
- that two keys created with the same name set / get different values
There was a problem hiding this comment.
What is the expected result when you attempt to override a key?
There was a problem hiding this comment.
The spec doesn't seem to specify.
There was a problem hiding this comment.
If you call SetValue / WriteValue with the same key I would expect it the context to have only the new value, like a map's key/value pairs.
| namespace context | ||
| { | ||
|
|
||
| std::mutex context_id_mutex; |
There was a problem hiding this comment.
It feels like the mutex should have the same storage class as the thing(s) it's guarding, so in this case it should also be a static class field like last_key_identifier_.
| * and increments the identifier then assigns it to be the key's | ||
| * identifier. | ||
| */ | ||
| ContextKey(std::string key_name){ |
There was a problem hiding this comment.
This is a public method, right? (ContextKey is a public class and this is a public method)
Is there a use case for calling this anywhere other than within the Context implementation?
There was a problem hiding this comment.
Well, that is an interesting question, I implemented it because I wanted to test the use case for constructing a context with a Key, so I needed the key before I had the context object.
| identifier_ = identifier; | ||
| } | ||
|
|
||
| public: |
There was a problem hiding this comment.
Do you want public copy constructor and assignment operator?
https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types
| Context(){ | ||
| ctx_map_ = std::map<int,int> {}; | ||
|
|
||
| } |
There was a problem hiding this comment.
Since ctx_map_ is concrete (not e.g. a pointer) this assignment is equivalent to the state of the map when Context is default constructed.
| Context(){ | |
| ctx_map_ = std::map<int,int> {}; | |
| } | |
| Context() = default; |
| int id; | ||
|
|
||
| context_id_mutex.lock(); | ||
|
|
||
| last_key_identifier_++; | ||
|
|
||
| id = last_key_identifier_; |
There was a problem hiding this comment.
This is a minor style point, but declare variables as close to their use as possible
| int id; | |
| context_id_mutex.lock(); | |
| last_key_identifier_++; | |
| id = last_key_identifier_; | |
| context_id_mutex.lock(); | |
| last_key_identifier_++; | |
| int id = last_key_identifier_; |
|
|
||
| Context ctx_; | ||
|
|
||
| public: |
There was a problem hiding this comment.
The public methods looks like things that a user shouldn't call (they make the Token transparent rather than opqaue as the specification demands).
Context api content
2 participants
Just the class declarations.