Skip to content

Add thread safety to SimpleSpanProcessor#358

Merged
reyang merged 2 commits intoopen-telemetry:masterfrom
jsuereth:wip-thread-safe-simple-exporter
Oct 12, 2020
Merged

Add thread safety to SimpleSpanProcessor#358
reyang merged 2 commits intoopen-telemetry:masterfrom
jsuereth:wip-thread-safe-simple-exporter

Conversation

@jsuereth
Copy link
Copy Markdown
Contributor

@jsuereth jsuereth commented Oct 8, 2020

Changes:

  • Ensure calls to the configured Exporter are locked behind an atomic_flag.
  • We use a spin-lock for now, under the assumption that Exporter calls are fast.

Notes:

  1. I tried really really hard to write a test that would fail if the locking wasn't in place, and was unable to make anything consistent/worthwhile, so that's dropped. If you have a good idea to how to write a test to ensure locking is done properly and failure to lock causes an issue, more than happy to add it.
  2. I thought spin-lock usage was "ok" given two assumptions: (a) Exporter calls will be 'fast' and (b) "simple" processor is not likely to be used over the buffering processor. If either of these are wrong, please let me know.
  3. The spin-lock itself could easily be improved w/ C++20x mechanisms or by pulling in some helpful utilities to better do exponential-back-off and provide better processor instructions instead of just looping. I think that this code would also benefit the *Provider classes that do similar spin-locking.

@jsuereth jsuereth requested a review from a team October 8, 2020 18:20
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Oct 8, 2020

CLA Check
The committers are authorized under a signed CLA.

@jsuereth
Copy link
Copy Markdown
Contributor Author

jsuereth commented Oct 8, 2020

@pyohannes let me know if this is in line with what you were thinking!

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 8, 2020

Codecov Report

Merging #358 into master will decrease coverage by 0.04%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #358      +/-   ##
==========================================
- Coverage   94.95%   94.90%   -0.05%     
==========================================
  Files         154      155       +1     
  Lines        6854     6874      +20     
==========================================
+ Hits         6508     6524      +16     
- Misses        346      350       +4     
Impacted Files Coverage Δ
sdk/include/opentelemetry/sdk/trace/exporter.h 100.00% <ø> (ø)
sdk/include/opentelemetry/sdk/trace/processor.h 100.00% <ø> (ø)
sdk/test/trace/simple_processor_test.cc 86.20% <78.94%> (-13.80%) ⬇️
api/include/opentelemetry/common/spin_lock_mutex.h 100.00% <100.00%> (ø)
api/include/opentelemetry/metrics/provider.h 100.00% <100.00%> (ø)
api/include/opentelemetry/trace/provider.h 100.00% <100.00%> (ø)
...include/opentelemetry/sdk/trace/simple_processor.h 87.50% <100.00%> (+1.78%) ⬆️

Comment thread sdk/include/opentelemetry/sdk/trace/simple_processor.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/simple_processor.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/simple_processor.h Outdated
Comment thread api/include/opentelemetry/common/spin_lock_mutex.h
Comment thread sdk/test/trace/simple_processor_test.cc Outdated
Comment thread api/include/opentelemetry/common/spin_lock_mutex.h
Copy link
Copy Markdown
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

Looks good, and improves the quality of our code base. Thanks!

I only have one nit, but this is ready to merge.

Comment thread sdk/include/opentelemetry/sdk/trace/simple_processor.h Outdated
…exporter usage in SimpleProcessor.

- Add generic SpinLockMutex and modify code using spin-locks to make use of it and std::lock_guard
- Update existing spin-lock usage
- add thread safe usage of Exporter in SimpleSpanProcessor.
- Ensure shutdown is only called once.
@jsuereth jsuereth force-pushed the wip-thread-safe-simple-exporter branch from 1a071af to 7512d6c Compare October 12, 2020 19:16
Copy link
Copy Markdown
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

}
/** Releases the lock held by the execution agent. Throws no exceptions. */
void unlock() noexcept { flag_.clear(std::memory_order_release); }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: We can also add a try_lock() method (as provided by std::mutex ) which returns immediately with status. It will provide flexibility to caller whether try acquiring in tight loop, or lazy way . This mayn't be usable through lock_guard, but still useful if caller is not concerned about RAII. Something like:
bool try_lock() { return !flag_.test_and_set(std::memory_order_acquire)) }

@reyang 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 Oct 12, 2020
@reyang reyang merged commit 0f3a5c8 into open-telemetry:master Oct 12, 2020
@jsuereth jsuereth deleted the wip-thread-safe-simple-exporter branch December 12, 2020 18:11
GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Aug 31, 2025
…wnload-artifact-digest

Update actions/download-artifact digest to 448e3f8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants