Skip to content
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

[WIP] Context API #99

Closed
wants to merge 37 commits into from
Closed
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
7867296
demo context header
satac2 Jun 8, 2020
d5a360a
demo context header
satac2 Jun 8, 2020
5d3b713
added file threadlocal_context.h
satac2 Jun 8, 2020
d047ff7
added fixes
satac2 Jun 8, 2020
f283cc6
Fixed syntatical errors, moved token class definition to context.h an…
satac2 Jun 8, 2020
e1de13f
Update api/include/opentelemetry/context/context.h
satac2 Jun 8, 2020
d5b155d
changed define guards and minor syntactical changes
satac2 Jun 8, 2020
80b2332
removing context vars class
satac2 Jun 8, 2020
64cd1c2
Merge branch 'context_api' of github.com:satac2/opentelemetry-cpp int…
satac2 Jun 8, 2020
4d24115
removed threadlocal context and made the runtime context methods static
satac2 Jun 8, 2020
a17c028
removed threadlocal context and made the runtime context methods static
satac2 Jun 8, 2020
d2df2ac
added a constructor
satac2 Jun 8, 2020
c316636
fixed convention issue and removed unnecessary code
satac2 Jun 8, 2020
7b36945
Update context.h
satac2 Jun 8, 2020
8e02c07
Update context.h
satac2 Jun 8, 2020
115cd68
context implementation
satac2 Jun 10, 2020
0d9fea1
Adding tests
satac2 Jun 10, 2020
565c40e
Adding tests
satac2 Jun 10, 2020
2d4a411
renamed some variables, added tests
satac2 Jun 10, 2020
9874c4c
renamed some variables, added tests
satac2 Jun 10, 2020
7bceaab
Merge branch 'context_api' into context_api_content
satac2 Jun 10, 2020
4b9fc2b
Merge pull request #5 from satac2/context_api_content
satac2 Jun 10, 2020
061e564
reversed public/private order
satac2 Jun 10, 2020
b5c81ad
Merge branch 'context_api_content' of github.com:satac2/opentelemetry…
satac2 Jun 10, 2020
b2717b3
Added detach test
satac2 Jun 10, 2020
ea687a9
Merge pull request #6 from satac2/context_api_content
satac2 Jun 10, 2020
b7259cd
avoiding standard datastructures
satac2 Jun 11, 2020
34eacf2
replaced std::map and created a thread_local context
satac2 Jun 17, 2020
a086993
replaced std::map and created a thread_local context
satac2 Jun 17, 2020
e44e926
cleaned up
satac2 Jun 17, 2020
6edce29
cleaned up
satac2 Jun 17, 2020
dff6f8f
fixed undefined behavior in context.h
satac2 Jun 20, 2020
e660ab7
minor style
satac2 Jun 21, 2020
aec8ca8
style
satac2 Jun 21, 2020
605cb75
added tests and comparison operator to the context api
satac2 Jun 22, 2020
e4d3069
auto formatted
satac2 Jun 22, 2020
4ad6395
Merge pull request #7 from satac2/context_api_content
satac2 Jun 22, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
233 changes: 233 additions & 0 deletions api/include/opentelemetry/context/context.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,233 @@
#pragma once

#include <map>
#include <string>
#include <mutex>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We refrained from using <mutex> in the API, due to concerns with ABI stability. Please try to replace the use atomic operations instead of the mutex. Usage of <atomic> should be fine.

Copy link
Contributor

@maxgolov maxgolov Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact we do not require map nor string either.. We cannot use string on API surface since it does not provide ABI stability guaranteed. Please use nostd::string_view instead. Using STL mutex and map types in Context API would also make it not ABI stable.


#include "opentelemetry/version.h"

OPENTELEMETRY_BEGIN_NAMESPACE
namespace context
{

std::mutex context_id_mutex;

/*The context class provides a context identifier */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use // style comments and add a full stop. :)

class Context{

public:

/*The ContextKey class is used to obscure access from the
* user to the context map. The identifier is used as a key
* to the context map.
*/
class ContextKey{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to use this class as a foundation for key-value iterable:
https://github.com/open-telemetry/opentelemetry-cpp/blob/master/api/include/opentelemetry/trace/key_value_iterable.h

You can entirely avoid using std::map in API. We do not require std::map

private:
friend class Context;

std::string key_name_;

int identifier_;


/* GetIdentifier: returns the identifier */
int GetIdentifier(){
return identifier_;
}

/* Constructs a new ContextKey with the passed in name and
* identifier.
*/
ContextKey(std::string key_name, int identifier){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use std::string , only nostd::string_view in API

key_name_ = key_name;
identifier_ = identifier;
}

public:

/* Consructs a new ContextKey with the passed in name and increments
* the identifier then assigns it to be the key's identifier.
*/
ContextKey(std::string key_name){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use nostd::string_view

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please mark this constructor explicit.

key_name_ = key_name;

context_id_mutex.lock();

Context::last_key_identifier_++;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of maintaining a counter and introducing a contention, consider using the address of the underlying object to uniquely identify the key.


identifier_ = Context::last_key_identifier_;

context_id_mutex.unlock();
}

};


/* Creates a context object with no key/value pairs */
Context(){
ctx_map_ = std::map<int,int> {};

}

/* Contructor, creates a context object from a map
* of keys and identifiers
*/
Context(ContextKey key, int value){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ctor would only take 1 key-value pair?

ctx_map_[key.GetIdentifier()] = value;
}


/* Accepts a new key/value pair and then returns a new
* context that contains both the original pairs and the new pair.
*/
Context WriteValue(ContextKey key, int value){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the value should a nostd::variant type instead of an int?

std::map<int, int> temp_map = ctx_map_;

temp_map[key.GetIdentifier()] = value;

return Context(temp_map);
}

/* Class comparator to see if the context maps are the same. */
bool operator == (const Context &context){
if(context.ctx_map_ == ctx_map_){
return true;
}
else{
return false;
}
}

/* Returns the value associated with the passed in key */
int GetValue(ContextKey key){
return ctx_map_[key.GetIdentifier()];
}

/* Returns a ContextKey that has the passed in name and the
* next available identifier.*/
ContextKey CreateKey(std::string key_name){
int id;

context_id_mutex.lock();

last_key_identifier_++;

id = last_key_identifier_;

context_id_mutex.unlock();

return ContextKey(key_name,id);
}

private:

/* The identifier itself */
std::map<int, int> ctx_map_;

/*Used to track that last ContextKey identifier and create the next one */
static int last_key_identifier_;

/* A constructor that accepts a key/value map */
Context(std::map<int,int> ctx_map){
ctx_map_ = ctx_map;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to format the code.


};



/* The token class provides an identifier that is used by
* the attach and detach methods to keep track of context
* objects.
*/

class Token{

public:

/* A constructor that sets the token's Context object to the
* one that was passed in.
*/
Token(Context &ctx){
ctx_ = ctx;
}

/* Returns the stored context object */
Context GetContext(){
return ctx_;
}

private:

Context ctx_;
};


/* The RuntimeContext class provides a wrapper for
* propogating context through cpp. */
class RuntimeContext {

public:


/* A default constructor that will set the context to
* an empty context object.
*/
RuntimeContext(){
context_ = Context();
}

/* A constructor that will set the context as the passed in context. */
RuntimeContext(Context &context){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the spec these will be global/static function so no ctor needed here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My current thinking:

  • Provide RuntimeContext as an interface, providing static methods for Attach/Detach/GetCurrent.
  • Implement ThreadLocalContext, providing biding for TLS (thread local storage).
  • By default the static methods from RuntimeContext will use ThreadLocalContext, and there is a way for the user to implement/specify their own context binding.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused what you mean by this, should the ThreadLocalContext be a derived class of RuntimeContext?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused what you mean by this, should the ThreadLocalContext be a derived class of RuntimeContext?

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would it be different than RuntimeContext, should the RuntimeContext just have a static context and the ThreadLocal have a static thread_local context?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think RuntimeContext is the abstraction layer (which doesn't have understanding about TLS).

context_ = context;
}

/* Sets the current 'Context' object. Returns a token
* that can be used to reset to the previous Context.
*/
Token Attach(Context &context){

Token old_context_token = Token(context_);

context_ = context;

return old_context_token;
}


/* Return the current context. */
static Context GetCurrent(){
Context context = context_;
return context_;
}


/* Resets the context to a previous value stored in the
* passed in token. Returns zero if successful, -1 otherwise
*/
int Detach(Token &token){

if(token.GetContext() == context_){

return -1;
}

context_ = token.GetContext();

return 0;
}


private:

static thread_local Context context_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure this static would always work in certain DLL loading scenarios, where two different DLLs consume OT API surface, esp. lazy loading / delay-loading. May want to spell out the limitations somewhere in the class documentation..

https://devblogs.microsoft.com/oldnewthing/20101122-00/?p=12233

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Rather use this singleton style (adding thread_local). That's what we use here.


};

thread_local Context RuntimeContext::context_ = Context();
int Context::last_key_identifier_ = 0;


}
OPENTELEMETRY_END_NAMESPACE
1 change: 1 addition & 0 deletions api/test/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
add_subdirectory(core)
add_subdirectory(context)
add_subdirectory(plugin)
add_subdirectory(nostd)
add_subdirectory(trace)
25 changes: 25 additions & 0 deletions api/test/context/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
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",
],
)

cc_test(
name = "runtimeContext_test",
srcs = [
"runtimeContext_test.cc",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use lowercase + underscores, i.e. runtime_context_test (also on L15) to be consistent with the rest of the files.

],
copts = ["-Wall","-std=c++17"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these copts.

deps = [
"//api",
"@com_google_googletest//:gtest_main",
],
)

45 changes: 45 additions & 0 deletions api/test/context/context_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
#include "opentelemetry/context/context.h"

#include <gtest/gtest.h>

using namespace opentelemetry::context;

/* Tests whether the original context objects changes
* when you write to it.
*/
TEST(Context_test, is_context_immutable)
{

Context test_context = Context();

Context::ContextKey test_key = test_context.CreateKey("test_key");

EXPECT_EQ(test_context.GetValue(test_key), NULL);

Context new_test_context = test_context.WriteValue(test_key,7);

EXPECT_EQ(new_test_context.GetValue(test_key), 7);

EXPECT_EQ(test_context.GetValue(test_key), NULL);
}

/* Tests whether the new Context Objects inherits the keys and values
* of the original context object
*/
TEST(Context_test, context_write_new_object)
{


Context::ContextKey test_key = Context::ContextKey("test_key");

Context test_context = Context(test_key, 7);

Context::ContextKey foo_key = test_context.CreateKey("foo_key");

Context new_test_context = test_context.WriteValue(foo_key,1);

EXPECT_EQ(new_test_context.GetValue(test_key), 7);

EXPECT_EQ(new_test_context.GetValue(foo_key), 1);
}

61 changes: 61 additions & 0 deletions api/test/context/runtimeContext_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
#include "opentelemetry/context/context.h"

#include <gtest/gtest.h>

using namespace opentelemetry::context;


/* Tests whether the runtimeContext object properly returns the current context
*/
TEST(runtimeContext_test, get_current_context)
{

Context::ContextKey test_key = Context::ContextKey("test_key");
Context test_context = Context(test_key, 7);

RuntimeContext test_runtime = RuntimeContext(test_context);

EXPECT_EQ(test_runtime.GetCurrent().GetValue(test_key),
test_context.GetValue(test_key));

}




/* Tests whether the runtimeContext object properly attaches and detaches
* the context object.
*/
TEST(runtimeContext_test, attach_detach_context)
{

Context::ContextKey test_key = Context::ContextKey("test_key");
Context test_context = Context(test_key, 7);

RuntimeContext test_runtime = RuntimeContext(test_context);

Context::ContextKey foo_key = Context::ContextKey("foo_key");
Context foo_context = Context(foo_key, 5);

EXPECT_EQ(test_runtime.GetCurrent().GetValue(test_key),
test_context.GetValue(test_key));
EXPECT_NE(test_runtime.GetCurrent().GetValue(foo_key),
foo_context.GetValue(foo_key));

Token test_token = test_runtime.Attach(foo_context);

EXPECT_NE(test_runtime.GetCurrent().GetValue(test_key),
test_context.GetValue(test_key));
EXPECT_EQ(test_runtime.GetCurrent().GetValue(foo_key),
foo_context.GetValue(foo_key));

int detach_result = test_runtime.Detach(test_token);

EXPECT_EQ(detach_result, 0);

EXPECT_EQ(test_runtime.GetCurrent().GetValue(test_key),
test_context.GetValue(test_key));
EXPECT_NE(test_runtime.GetCurrent().GetValue(foo_key),
foo_context.GetValue(foo_key));
}