Remove static global context_handler_ variable by pyohannes · Pull Request #322 · open-telemetry/opentelemetry-cpp

pyohannes · 2020-09-04T16:40:14Z

The main intention of this PR is to remove the static global context_handler_ variable, which caused linking problems in several instances, as this would imply a need to have the context_handler_ symbol present in the translation units linked, which conflicts with the header-only API approach.

To fix this, I added a singleton approach similar to what we have in place with the TracerProvider.

While working on that, I always cleaned things up a bit and I separated the RuntimeContext into RuntimeContext and a RuntimeContextStorage. The RuntimeContextStorage is the part that needs to be implemented for providing a custom context management strategy. I think having this separation in place reduces complexity.

Closes #321.

codecov · 2020-09-04T17:03:43Z

Codecov Report

Merging #322 into master will increase coverage by 0.08%.
The diff coverage is 96.07%.

@@            Coverage Diff             @@
##           master     #322      +/-   ##
==========================================
+ Coverage   94.62%   94.70%   +0.08%     
==========================================
  Files         148      148              
  Lines        6655     6729      +74     
==========================================
+ Hits         6297     6373      +76     
+ Misses        358      356       -2

Impacted Files	Coverage Δ
api/test/context/runtime_context_test.cc	`100.00% <ø> (ø)`
api/test/plugin/dynamic_load_test.cc	`100.00% <ø> (ø)`
api/test/trace/scope_test.cc	`100.00% <ø> (ø)`
exporters/ostream/test/ostream_span_test.cc	`100.00% <ø> (ø)`
ext/test/zpages/threadsafe_span_data_test.cc	`100.00% <ø> (ø)`
ext/test/zpages/tracez_data_aggregator_test.cc	`97.34% <ø> (ø)`
ext/test/zpages/tracez_processor_test.cc	`98.70% <ø> (ø)`
sdk/test/trace/always_off_sampler_test.cc	`100.00% <ø> (ø)`
sdk/test/trace/always_on_sampler_test.cc	`100.00% <ø> (ø)`
sdk/test/trace/attribute_utils_test.cc	`100.00% <ø> (ø)`
... and 10 more

maxgolov · 2020-09-04T18:56:22Z

+   *
+   * @param storage a custom runtime context storage
+   */
+  static void SetRuntimeContextStorage(nostd::shared_ptr<RuntimeContextStorage> storage) noexcept


Is this method typically gonna be the very first method called before any telemetry is emitted by the process?

Hard to say. I think it is, but I'm not sure about all the different scenarios.

Is this method typically gonna be the very first method called before any telemetry is emitted by the process?

I documented that this method has to be called before any spans are created.

maxgolov · 2020-09-04T19:06:21Z

+private:
+  static nostd::shared_ptr<RuntimeContextStorage> GetRuntimeContextStorage() noexcept
+  {
+    while (GetLock().test_and_set(std::memory_order_acquire))


I'm trying to understand the purpose of the lock here:

line 178 - &GetStorage() is already atomic, since static at line 180 is magic static aka function-local static. Since C++11, the initialization of magic statics is guaranteed to be thread safe...

whereas storage at line 172 is on stack...

Could you please clarify - what is the race we are trying to address? Is this a race between getting storage for the first time vs. SetRuntimeContextStorage potentially being called at the same time?

I was thinking about slightly less elegant but shorter solution.. It won't require a lock if we declare that the context storage customization MUST be done before any other telemetry API call:
https://github.com/maxgolov/opentelemetry-cpp/blob/4004c2d1ebc6bd9b1e55eaf9310726fc939f9de5/api/include/opentelemetry/context/runtime_context.h#L28

(sorry, I didn't refactor it much.. it's somewhat similar to previous implementation - same method names as before... I did this in my fork to alleviate the issue).

I like your solution overall. Just have doubts if we really need the locking here.

The race is between GetRuntimeContextStorage and SetRuntimeContextStorage. Without the lock, we might partially overwrite the shared_ptr, thus scrambling up the counter and causing memory corruption.

I went the safer way here. But I'm fine with adding a requirement that context storage customization MUST be done before any other telemetry API call. Then we can remove the lock and win a tiny bit of performance.

Agree. there is race condition and possible corruption if GetRuntimeContextStorage and SetRuntimeContextStorage is called simultaneously as line 163 is not atomic operation. Although this is tiny bit of performance overhead, it can become significant if GetRuntimeContextStorage get's called for each span processing. Adding a requirement for context storage initialization before telemetry api calls would be good in that case.

I removed the locking around GetRuntimeContextStorage and SetRuntimeContextStorage. I added a note to SetRuntimeContextStorage that the behavior is undefined when it it's called after any spans have been created.

ThomsonTan · 2020-09-05T00:14:46Z

+   */
+  static void SetRuntimeContextStorage(nostd::shared_ptr<RuntimeContextStorage> storage) noexcept
+  {
+    while (GetLock().test_and_set(std::memory_order_acquire))


Wrap GetLock().test_and_set() and GetLock().clear() into an object ctor and dtor to make sure clear() is always called here?

Good point. I'll wait for the result of the discussion whether we'll need this lock at all. If so, I can wrap the calls in some kind of lock guard.

I removed the locks altogether.

ThomsonTan · 2020-09-05T01:45:39Z

+    // Pops the top Context off the stack and returns it.
+    Context Pop() noexcept
+    {
+      if (size_ <= 0)


size_t is unsigned, so just check size_ == 0 here?

ThomsonTan · 2020-09-05T01:47:00Z

+      }
+      int index = size_ - 1;
+      size_--;
+      return base_[index];


Alternate to return base_[--size_];?

If size_ is of size_t type, then should we check before we decrement? (or is it going to be too much of a paranoid check?)

I simplified the code. The check is already there (a few lines above).

ThomsonTan · 2020-09-05T01:53:48Z

+
+static RuntimeContextStorage *GetDefaultStorage() noexcept
+{
+  return new ThreadLocalContextStorage();


Wondering whether it is necessary to new ThreadLocalContextStorage() at every call, or just keep one global instance?

That call is used to initialize the one global instance. It's only called once to initialize a static variable.

pyohannes · 2020-09-17T21:30:04Z

I removed all locks around the RuntimeContextStorage getter and setter, and I documented that setting a RuntimeContextStorage after spans have already been created may result in undefined behavior.

This is ready for another reviewround @open-telemetry/cpp-approvers.

…what_you_use-0.x Update dependency depend_on_what_you_use to v0.9.0

pyohannes requested a review from a team September 4, 2020 16:40

maxgolov reviewed Sep 4, 2020

View reviewed changes

ThomsonTan reviewed Sep 5, 2020

View reviewed changes

Comment thread api/include/opentelemetry/context/runtime_context.h

ThomsonTan reviewed Sep 5, 2020

View reviewed changes

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

pyohannes force-pushed the global-context branch from f1024a0 to 00cecbb Compare September 15, 2020 16:08

lalitb approved these changes Sep 18, 2020

View reviewed changes

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

Johannes Tax added 10 commits September 18, 2020 15:52

Remove static global context_handler_ variable

a911027

Remove obsolete include

f526460

Fix build

767ef52

Add missing include

c913502

PR comments

f7f2fbd

PR comments

121cc3e

Merge

130ff0a

Remove locking around GetRuntimeContextStorage

06d799c

Format

5ef902e

Remove obsolete header

934322b

pyohannes force-pushed the global-context branch from 5448bd8 to 934322b Compare September 18, 2020 22:54

reyang and others added 2 commits September 21, 2020 09:02

Merge branch 'master' into global-context

5a4b416

Add valgrind suppressions for grpc flakiness

97e7d42

maxgolov approved these changes Sep 21, 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 Sep 21, 2020

Merge branch 'master' into global-context

8bbc92b

reyang merged commit a39b9b3 into open-telemetry:master Sep 21, 2020

ThomsonTan mentioned this pull request Sep 30, 2020

REQUEST: New membership for ThomsonTan open-telemetry/community#512

Closed

6 tasks

reyang mentioned this pull request Oct 7, 2020

Add lalitb and ThomsonTan as approvers #355

Merged

GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Jun 17, 2025

Merge pull request open-telemetry#322 from malkia/renovate/depend_on_…

ce4f93c

…what_you_use-0.x Update dependency depend_on_what_you_use to v0.9.0

Conversation

pyohannes commented Sep 4, 2020

Uh oh!

codecov Bot commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxgolov Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pyohannes commented Sep 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Sep 4, 2020 •

edited

Loading

maxgolov Sep 4, 2020 •

edited

Loading