Runtime and threadlocal context #209

satac2 · 2020-07-23T19:53:41Z

This is an implementation of the RuntimeContext and ThreadLocalContext which is used to provide access to a context object globally. @reyang

Co-authored-by: Reiley Yang <reyang@microsoft.com>

…emetry-cpp into context_api_dummy_methods

Context api actual methods

…ocal_context

…d an overloaded comparison to context API

…context will use

…d ContextConfig has been removed

codecov · 2020-07-23T19:56:42Z

Codecov Report

Merging #209 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #209      +/-   ##
==========================================
+ Coverage   92.31%   92.34%   +0.03%     
==========================================
  Files         102      102              
  Lines        3174     3188      +14     
==========================================
+ Hits         2930     2944      +14     
  Misses        244      244

Impacted Files	Coverage Δ
api/include/opentelemetry/context/context.h	`98.14% <100.00%> (+0.03%)`	⬆️
api/test/context/context_test.cc	`100.00% <100.00%> (ø)`

api/include/opentelemetry/context/threadlocal_context.h

reyang · 2020-07-23T23:46:12Z

api/include/opentelemetry/context/runtime_context.h

+class RuntimeContext
+{
+public:
+  class Token


Would it make sense to have a dtor for Token which calls Detach?

That make sense, seems like it will add an element of safety so the context can't get stuck after a token has gone out of scope. I will add it.

…pentelemetry-cpp into runtime_and_threadlocal_context

reyang

LGTM.

pyohannes · 2020-07-24T04:57:30Z

api/test/context/runtime_context_test.cc

+{
+  std::map<std::string, context::ContextValue> map_test = {{"test_key", (int64_t)123}};
+  context::Context test_context                         = context::Context(map_test);
+  context::RuntimeContext::Token old_context = context::RuntimeContext::Attach(&test_context);


What is the memory management strategy when managing context? Will contexts always be stored on the stack?

I'm afraid we might run into issues when having contexts stored on the stack, while managing all the context state via raw pointers.

Another thing I wonder about: whenever a new span is started, we have to set the current span. I think we can only do this by adding a new context. So we'll add a new context when a span is started and we end the context when the span is ended. The user though has the option to end a child span before a parent span. This shouldn't screw up our context stack. Maybe I'm also not completely understanding the workflow here, please correct me if I'm wrong.

Perhaps RuntimeContext::Attach should take a context object instead of a ptr and that should prevent issues if the original context object goes out of scope.

What is the memory management strategy when managing context? Will contexts always be stored on the stack?

I think the context object has to be always allocated on the heap. Context can be propagated between threads and threads can get killed by the underlying OS (which means the stack will be reclaimed and repurposed).

The user though has the option to end a child span before a parent span. This shouldn't screw up our context stack.

That's a very interesting topic, @pyohannes what would you expect for the following scenario?

// current span = EMPTY span("A") begin // current span = A span("B") begin // current span = B span("C") begin // current span = C span("B") end // current span = ??? span("A") end // current span = ??? span("C") end // current span = ???

Changed to RuntimeContext accepting a Context object instead of Context* so we should not be ref counting it.

I was originally thinking to unblock this PR first and then revisit

That's ok for me, as long as we keep track of open issues (memory management and asynchronous spans), preferably via GitHub issues.

Are we good to merge then?

#219 created for tracking.

api/include/opentelemetry/context/threadlocal_context.h

pyohannes · 2020-07-24T05:31:55Z

api/include/opentelemetry/context/runtime_context.h

+  virtual Token InternalAttach(Context *context) { return Token(); }
+
+  // Dummy method to be overriden by derived class implementation
+  virtual bool InternalDetach(Token &token) { return false; }


Let's make those methods pure virtual. Then the comment is not needed.

Changed to pure virtual.

pyohannes · 2020-07-24T05:33:34Z

api/include/opentelemetry/context/threadlocal_context.h

+    ~Stack() { delete[] base_; }
+
+    int size_;
+    int capacity_;


Those should be size_t.

Changed to size_t.

api/include/opentelemetry/context/threadlocal_context.h

api/test/context/runtime_context_test.cc

pyohannes

I have some smaller nits.

Can you please submit an issue regarding the support of asynchronous segments (parent segments that end before their children). It seems the memory issue is accounted for by having copies of Context on the stack.

pyohannes · 2020-07-27T19:07:34Z

api/include/opentelemetry/context/runtime_context.h

+
+  virtual Token InternalAttach(Context context) = 0;
+
+  virtual bool InternalDetach(Token &token) = 0;


Please mark all methods as noexcept.

Marked as noexcept.

pyohannes · 2020-07-27T19:08:25Z

api/include/opentelemetry/context/threadlocal_context.h

+
+private:
+  // A nested class to store the attached contexts in a stack.
+  class Stack


Let's mark all Stack methods as noexcept. Top can be const too.

Marked as noexcept and const.

pyohannes · 2020-07-27T19:10:02Z

api/include/opentelemetry/context/runtime_context.h

+
+    // A constructor that sets the token's Context object to the
+    // one that was passed in.
+    Token(Context context) { context_ = context; }


Please use a member initializer list here.

Switched to initializer list.

pyohannes · 2020-07-27T19:10:56Z

api/include/opentelemetry/context/runtime_context.h

+    Context context_;
+
+    // Returns the stored context object.
+    Context GetContext() { return context_; }


This method isn't used anywhere. What would it be needed for?

It was initially used to get the context for comparison, but now that the comparison operator compares directly with contexts it is unnecessary, I will remove it.

pyohannes · 2020-07-27T19:12:51Z

api/include/opentelemetry/context/runtime_context.h

+  class Token
+  {
+  public:
+    bool operator==(const Context &other) { return context_ == other; }


This makes it possible to compare a Token to a Context. I wonder if the argument should be of type Token?

Yes, that was on purpose. There has not been a use case for token to token comparison.

pyohannes · 2020-07-27T19:23:19Z

api/include/opentelemetry/context/threadlocal_context.h

+    {
+      size_     = 0;
+      capacity_ = 0;
+      base_     = nullptr;


Please use a member initializer list for that.

Changed to initializer list.

pyohannes · 2020-07-27T19:26:19Z

api/include/opentelemetry/context/threadlocal_context.h

+        for (auto i = 0; i <= old_size; i++)
+        {
+          temp[i] = base_[i];
+        }


I think you can use std::copy(base_, base_ + old_size, temp); instead of the loop.

satac2 and others added 30 commits July 6, 2020 19:35

added context header with dummy methods

4128a69

Build and CMake files

cba6d5f

auto formatted

b2e876e

changed variable name from attributes to values

a7480c8

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

96cd32f

Co-authored-by: Reiley Yang <reyang@microsoft.com>

changed to follow the spec and added context_value type

5f0634b

Merge branch 'context_api_dummy_methods' of github.com:satac2/opentel…

5d77daf

…emetry-cpp into context_api_dummy_methods

removed tests for this PR

68a1a70

removed line

5e68574

removed unnecessary include

16a492e

changed a function to pass by reference

497eb67

formatted

479dead

Added tests.

2a78c48

avoiding ABI compatibility issues

f51748f

Merge branch 'master' into context_api_dummy_methods

7d73219

added throw capture

3f85aa5

Merge branch 'context_api_dummy_methods' of github.com:satac2/opentel…

0ec83e8

…emetry-cpp into context_api_dummy_methods

minor exception syntax error

21ea872

Merge pull request #13 from satac2/context_api_dummy_methods

f016689

Context api actual methods

added runtime and threadlocal contexts

1f5e5fd

add comparison operator to context api

bbf9559

added stack for local storage of context objects

c8b7ed6

removed old context.h

8d8b91f

changed runtime context interface to match

4b78ac8

Merge remote-tracking branch 'origin/master' into runtime_and_threadl…

d3cea59

…ocal_context

Merge remote-tracking branch 'origin/master' into runtime_and_threadl…

25bec5e

…ocal_context

formatted and added some size checks to prevent seg faults, also adde…

d77e445

…d an overloaded comparison to context API

added tests

0583e3c

add a context_config file to choose which implementation the runtime …

e02fadb

…context will use

readding string_view_test

c06edc5

satac2 added 3 commits July 23, 2020 19:30

threadlocalcontext is once again a derived class of RuntimeContext an…

fc42d91

…d ContextConfig has been removed

protected virtual members

ce78585

reset string_view_test

02dbbdf

satac2 requested a review from a team July 23, 2020 19:53

reyang reviewed Jul 23, 2020

View reviewed changes

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

Merge branch 'master' into runtime_and_threadlocal_context

c1ebc16

reyang reviewed Jul 23, 2020

View reviewed changes

satac2 added 2 commits July 24, 2020 01:26

added destructor that detaches the context to the Token

5a5df6e

Merge branch 'runtime_and_threadlocal_context' of github.com:satac2/o…

a2c81b3

…pentelemetry-cpp into runtime_and_threadlocal_context

reyang approved these changes Jul 24, 2020

View reviewed changes

reyang added the pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.) label Jul 24, 2020

pyohannes reviewed Jul 24, 2020

View reviewed changes

minor changes and switch from RuntimeContext accepting a ptr

1d7549f

pyohannes reviewed Jul 27, 2020

View reviewed changes

satac2 added 2 commits July 28, 2020 00:46

fixed nits

090fbe7

added const to Top

2ba7e02

pyohannes approved these changes Jul 28, 2020

View reviewed changes

reyang added 2 commits July 27, 2020 19:02

Merge branch 'master' into runtime_and_threadlocal_context

2fe8935

Merge branch 'master' into runtime_and_threadlocal_context

6b2ff4c

reyang mentioned this pull request Jul 28, 2020

Memory management and asynchronous span handling for Context API #219

Closed

reyang merged commit 283114d into open-telemetry:master Jul 28, 2020

ziqizh pushed a commit to ziqizh/opentelemetry-cpp that referenced this pull request Jul 31, 2020

Runtime and threadlocal context (open-telemetry#209)

653619f

ziqizh pushed a commit to ziqizh/opentelemetry-cpp that referenced this pull request Jul 31, 2020

Runtime and threadlocal context (open-telemetry#209)

5c86342

reyang mentioned this pull request Aug 4, 2020

Make SuppressInstrumentation an IDisposable open-telemetry/opentelemetry-dotnet#988

Merged

reyang mentioned this pull request Aug 25, 2020

Add context API and implementation #73

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Runtime and threadlocal context #209

Runtime and threadlocal context #209

satac2 commented Jul 23, 2020

codecov bot commented Jul 23, 2020 •

edited

Loading

reyang Jul 23, 2020

satac2 Jul 24, 2020

satac2 Jul 24, 2020

reyang left a comment

pyohannes Jul 24, 2020

pyohannes Jul 24, 2020

satac2 Jul 24, 2020

reyang Jul 25, 2020

reyang Jul 25, 2020

satac2 Jul 25, 2020

satac2 Jul 25, 2020

pyohannes Jul 27, 2020

satac2 Jul 27, 2020

reyang Jul 28, 2020

pyohannes Jul 24, 2020

satac2 Jul 25, 2020

pyohannes Jul 24, 2020

satac2 Jul 25, 2020

pyohannes left a comment

pyohannes Jul 27, 2020

satac2 Jul 28, 2020

pyohannes Jul 27, 2020

satac2 Jul 28, 2020

pyohannes Jul 27, 2020

satac2 Jul 28, 2020

pyohannes Jul 27, 2020

satac2 Jul 28, 2020

pyohannes Jul 27, 2020

satac2 Jul 27, 2020

pyohannes Jul 27, 2020

satac2 Jul 28, 2020

pyohannes Jul 27, 2020

satac2 Jul 28, 2020


		virtual Token InternalAttach(Context context) = 0;

		virtual bool InternalDetach(Token &token) = 0;

Runtime and threadlocal context #209

Runtime and threadlocal context #209

Conversation

satac2 commented Jul 23, 2020

codecov bot commented Jul 23, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyang left a 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

pyohannes left a 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

codecov bot commented Jul 23, 2020 •

edited

Loading