[WIP] Context API #99

satac2 · 2020-06-08T20:48:07Z

If you could take a look at the class declarations and see if I am on the right track for the context api, that would be great, thanks.

…d added the contextvars_context.h file

Co-authored-by: Kirk Kelsey <kirkmkelsey@gmail.com>

…o context_api

linux-foundation-easycla · 2020-06-08T20:48:13Z

The committers are authorized under a signed CLA.

✅ Sam Atac (7867296, d5a360a, 5d3b713, d047ff7, f283cc6, e1de13f, d5b155d, 80b2332, 64cd1c2, 4d24115, a17c028, d2df2ac, c316636, 7b36945, 8e02c07, 115cd68, 0d9fea1, 565c40e, 2d4a411, 9874c4c, 7bceaab, 4b9fc2b, 061e564, b5c81ad, b2717b3, ea687a9)

api/include/opentelemetry/context/context.h

reyang · 2020-06-08T21:16:38Z

api/include/opentelemetry/context/context.h

+    /* Context: contructor, creates a context object from a map
+     * of keys and identifiers
+     */
+    Context(std::map<std::string, int> ctx);  


All the public APIs need to be ABI compatible - which means they are not supposed to expose dependency on STL traits/layout.

api/include/opentelemetry/context/context.h

reyang · 2020-06-08T21:25:21Z

Welcome to your first PR @satac2, please clear the CLA steps as required by the CI / CNCF.

Context api content

reyang · 2020-06-10T18:28:07Z

api/include/opentelemetry/context/context.h

+
+            context_id_mutex.lock();
+
+            Context::last_key_identifier_++;


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

reyang · 2020-06-10T18:31:25Z

api/include/opentelemetry/context/context.h

+      Context(std::map<int,int> ctx_map){
+        ctx_map_ = ctx_map;
+      } 
+


Need to format the code.

reyang · 2020-06-10T18:35:59Z

api/include/opentelemetry/context/context.h

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


This ctor would only take 1 key-value pair?

reyang · 2020-06-10T18:36:30Z

api/include/opentelemetry/context/context.h

+      /* 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){


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

reyang · 2020-06-10T18:41:40Z

api/include/opentelemetry/context/context.h

+      }
+
+      /* A constructor that will set the context as the passed in context. */
+      RuntimeContext(Context &context){ 


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

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.

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

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

Yes.

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

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

maxgolov · 2020-06-10T21:31:33Z

api/include/opentelemetry/context/context.h

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


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

maxgolov · 2020-06-10T21:31:50Z

api/include/opentelemetry/context/context.h

+          /* 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){


Please use nostd::string_view

maxgolov · 2020-06-10T21:34:36Z

api/include/opentelemetry/context/context.h

+       * user to the context map. The identifier is used as a key 
+       * to the context map.
+       */
+      class ContextKey{


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

maxgolov · 2020-06-11T00:49:01Z

api/include/opentelemetry/context/context.h

+
+    private:
+
+      static thread_local Context context_;


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

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

g-easy · 2020-06-11T08:20:55Z

api/include/opentelemetry/context/context.h

+
+  std::mutex context_id_mutex;
+
+  /*The context class provides a context identifier */


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

g-easy · 2020-06-11T08:21:33Z

api/include/opentelemetry/context/context.h

+          /* 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){


Please mark this constructor explicit.

g-easy · 2020-06-11T08:23:32Z

api/test/context/BUILD

+cc_test(
+    name = "runtimeContext_test",
+    srcs = [
+        "runtimeContext_test.cc",


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

g-easy · 2020-06-11T08:25:38Z

api/test/context/BUILD

+    srcs = [
+        "runtimeContext_test.cc",
+    ],
+    copts = ["-Wall","-std=c++17"],


Please remove these copts.

g-easy · 2020-06-11T08:27:58Z

More general context questions:

Should the actual TLS part be a pointer, and allocate the per-thread context when it's accessed by Attach/GetCurrent?
However we end up resolving the std::map question: how can we attach/detach faster than doing a full copy of the map?

reyang · 2020-06-11T08:33:55Z

However we end up resolving the std::map question: how can we attach/detach faster than doing a full copy of the map?

https://www.python.org/dev/peps/pep-0550/#implementation 😄

g-easy · 2020-06-11T08:36:50Z

And refcount the context objects that form chains?

g-easy · 2020-06-12T09:27:26Z

Can we have a no-op Context API and move the implementation into the SDK, so that other SDKs can make different performance tradeoffs in how this is implemented?

reyang · 2020-06-12T16:25:57Z

Can we have a no-op Context API and move the implementation into the SDK, so that other SDKs can make different performance trade-offs in how this is implemented?

Probably not, the OpenTelemetry API package has to give a default implementation that flows the incoming traceid/spanid/tracestate/flags to the outgoing request, thus the actual implementation cannot be in the SDK.
However we need to make sure that other SDKs / components can replace the context implementation.

Context api content

satac2 and others added 15 commits June 8, 2020 14:25

demo context header

7867296

demo context header

d5a360a

added file threadlocal_context.h

5d3b713

added fixes

d047ff7

Fixed syntatical errors, moved token class definition to context.h an…

f283cc6

…d added the contextvars_context.h file

Update api/include/opentelemetry/context/context.h

e1de13f

Co-authored-by: Kirk Kelsey <kirkmkelsey@gmail.com>

changed define guards and minor syntactical changes

d5b155d

removing context vars class

80b2332

Merge branch 'context_api' of github.com:satac2/opentelemetry-cpp int…

64cd1c2

…o context_api

removed threadlocal context and made the runtime context methods static

4d24115

removed threadlocal context and made the runtime context methods static

a17c028

added a constructor

d2df2ac

fixed convention issue and removed unnecessary code

c316636

Update context.h

7b36945

Update context.h

8e02c07

reyang reviewed Jun 8, 2020

View reviewed changes

api/include/opentelemetry/context/context.h Outdated Show resolved Hide resolved

reyang reviewed Jun 8, 2020

View reviewed changes

api/include/opentelemetry/context/context.h Outdated Show resolved Hide resolved

reyang reviewed Jun 8, 2020

View reviewed changes

api/include/opentelemetry/context/context.h Outdated Show resolved Hide resolved

reyang changed the title ~~Context api [WIP] @reyang~~ [WIP] Context API Jun 8, 2020

satac2 and others added 8 commits June 10, 2020 16:28

context implementation

115cd68

Adding tests

0d9fea1

Adding tests

565c40e

renamed some variables, added tests

2d4a411

renamed some variables, added tests

9874c4c

Merge branch 'context_api' into context_api_content

7bceaab

Merge pull request #5 from satac2/context_api_content

4b9fc2b

Context api content

reversed public/private order

061e564

satac2 and others added 2 commits June 10, 2020 18:16

Added detach test

b2717b3

Merge pull request #6 from satac2/context_api_content

ea687a9

Context api content

reyang reviewed Jun 10, 2020

View reviewed changes

api/include/opentelemetry/context/context.h

Context(std::map<int,int> ctx_map){

ctx_map_ = ctx_map;

}

Copy link

Member

reyang Jun 10, 2020

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.

reyang reviewed Jun 10, 2020

View reviewed changes

maxgolov reviewed Jun 10, 2020

View reviewed changes

maxgolov reviewed Jun 11, 2020

View reviewed changes

g-easy reviewed Jun 11, 2020

View reviewed changes

avoiding standard datastructures

b7259cd

satac2 and others added 10 commits June 17, 2020 18:36

replaced std::map and created a thread_local context

34eacf2

replaced std::map and created a thread_local context

a086993

cleaned up

e44e926

cleaned up

6edce29

fixed undefined behavior in context.h

dff6f8f

minor style

e660ab7

style

aec8ca8

added tests and comparison operator to the context api

605cb75

auto formatted

e4d3069

Merge pull request #7 from satac2/context_api_content

4ad6395

Context api content

0x4b mentioned this pull request Jun 24, 2020

List and pair satac2/opentelemetry-cpp#8

Merged

satac2 closed this Jul 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Context API #99

[WIP] Context API #99

satac2 commented Jun 8, 2020

linux-foundation-easycla bot commented Jun 8, 2020 •

edited

Loading

reyang Jun 8, 2020

reyang commented Jun 8, 2020

reyang Jun 10, 2020

reyang Jun 10, 2020

reyang Jun 10, 2020

reyang Jun 10, 2020

reyang Jun 10, 2020

reyang Jun 10, 2020

satac2 Jun 15, 2020

reyang Jun 15, 2020

satac2 Jun 15, 2020

reyang Jun 16, 2020

maxgolov Jun 10, 2020

maxgolov Jun 10, 2020

maxgolov Jun 10, 2020

maxgolov Jun 11, 2020

pyohannes Jun 11, 2020

g-easy Jun 11, 2020

g-easy Jun 11, 2020

g-easy Jun 11, 2020

g-easy Jun 11, 2020

g-easy commented Jun 11, 2020

reyang commented Jun 11, 2020

g-easy commented Jun 11, 2020

g-easy commented Jun 12, 2020

reyang commented Jun 12, 2020


		std::mutex context_id_mutex;

		/The context class provides a context identifier /

[WIP] Context API #99

[WIP] Context API #99

Conversation

satac2 commented Jun 8, 2020

linux-foundation-easycla bot commented Jun 8, 2020 • edited Loading

Choose a reason for hiding this comment

reyang commented Jun 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g-easy commented Jun 11, 2020

reyang commented Jun 11, 2020

g-easy commented Jun 11, 2020

g-easy commented Jun 12, 2020

reyang commented Jun 12, 2020

linux-foundation-easycla bot commented Jun 8, 2020 •

edited

Loading