From d590a6a6afbb84dd8ba01b358ee79f20c1992af1 Mon Sep 17 00:00:00 2001 From: Evgeny Yakimov Date: Wed, 25 Dec 2019 22:58:42 +0000 Subject: [PATCH 01/15] ScopeManager interface Includes only the suggested ScopeManager interface and related classes. No implementation is provided but headers were included in a compilation unit for testing. Clang-format was run --- CMakeLists.txt | 1 + include/opentracing/scope_manager.h | 41 +++++++++++++++++++++++++++++ src/scope_manager.cpp | 1 + 3 files changed, 43 insertions(+) create mode 100644 include/opentracing/scope_manager.h create mode 100644 src/scope_manager.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index a451844..aab0002 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -136,6 +136,7 @@ include_directories(SYSTEM 3rd_party/include) set(SRCS src/propagation.cpp src/dynamic_load.cpp src/noop.cpp + src/scope_manager.cpp src/tracer.cpp src/tracer_factory.cpp src/ext/tags.cpp) diff --git a/include/opentracing/scope_manager.h b/include/opentracing/scope_manager.h new file mode 100644 index 0000000..0e8f1c2 --- /dev/null +++ b/include/opentracing/scope_manager.h @@ -0,0 +1,41 @@ +#ifndef OPENTRACING_SCOPE_MANAGER_H +#define OPENTRACING_SCOPE_MANAGER_H + +#include + +namespace opentracing { +BEGIN_OPENTRACING_ABI_NAMESPACE + +class Span; + +// Scope is returned by the ScopeManager when activating a span +// +// The lifetime of the Scope instance represends the duration of the +// activation. A Scope cannot be created and can only be returned by +// the ScopeManager. +class Scope { + private: + // Create an activation Scope for the given Span. + Scope(Span& span) noexcept; +}; + +// ScopeManager allows a Span to be activated for a specific scope. +// +// Once a span has been activated, it can then be accessed by any code +// executed within the lifetime of the scope and the same thread only. +class ScopeManager { + public: + // Activate the given Span, returning a Scope to track its duration. + static Scope Activate(Span& span) noexcept; + + // Return a reference to the current active Span. + // + // A span is always guaranteed to be returned. If there is no span + // active, then a default noop span instance will be returned. + static Span& ActiveSpan() noexcept; +}; + +END_OPENTRACING_ABI_NAMESPACE +} // namespace opentracing + +#endif // OPENTRACING_SCOPE_MANAGER_H diff --git a/src/scope_manager.cpp b/src/scope_manager.cpp new file mode 100644 index 0000000..e7ba8a8 --- /dev/null +++ b/src/scope_manager.cpp @@ -0,0 +1 @@ +#include From edc5547854daef970424f589f5285c7300940744 Mon Sep 17 00:00:00 2001 From: Evgeny Yakimov Date: Thu, 26 Dec 2019 12:55:32 +0000 Subject: [PATCH 02/15] ScopeManager stubs and tests All interfaces have been implemented with stub implementations, but do compile. Tests have been setup but not written for ScopeManager. --- include/opentracing/scope_manager.h | 8 ++++++++ src/scope_manager.cpp | 20 ++++++++++++++++++++ test/BUILD | 1 + test/CMakeLists.txt | 4 ++++ test/scope_manager_test.cpp | 9 +++++++++ 5 files changed, 42 insertions(+) create mode 100644 test/scope_manager_test.cpp diff --git a/include/opentracing/scope_manager.h b/include/opentracing/scope_manager.h index 0e8f1c2..ed23976 100644 --- a/include/opentracing/scope_manager.h +++ b/include/opentracing/scope_manager.h @@ -17,6 +17,14 @@ class Scope { private: // Create an activation Scope for the given Span. Scope(Span& span) noexcept; + ~Scope() noexcept; + + Scope(const Scope&) = delete; + Scope(Scope&&) = delete; + Scope& operator=(const Scope&) = delete; + Scope& operator=(Scope&&) = delete; + + friend class ScopeManager; }; // ScopeManager allows a Span to be activated for a specific scope. diff --git a/src/scope_manager.cpp b/src/scope_manager.cpp index e7ba8a8..6157769 100644 --- a/src/scope_manager.cpp +++ b/src/scope_manager.cpp @@ -1 +1,21 @@ +#include #include + +namespace opentracing { +BEGIN_OPENTRACING_ABI_NAMESPACE +namespace { + +thread_local Span* active = nullptr; + +} // anonymous namespace + +Scope::Scope(Span& span) noexcept {} + +Scope::~Scope() noexcept {} + +Scope ScopeManager::Activate(Span& span) noexcept { return {span}; } + +Span& ScopeManager::ActiveSpan() noexcept { return *active; } + +END_OPENTRACING_ABI_NAMESPACE +} // namespace opentracing diff --git a/test/BUILD b/test/BUILD index ce9077b..779e1ce 100644 --- a/test/BUILD +++ b/test/BUILD @@ -1,4 +1,5 @@ TEST_NAMES = [ + "scope_manager_test", "string_view_test", "tracer_test", "util_test", diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index fa2b0ca..aae6f27 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -17,6 +17,10 @@ add_test(NAME value_test COMMAND value_test) add_executable(util_test util_test.cpp) add_test(NAME util_test COMMAND util_test) +add_executable(scope_manager_test scope_manager_test.cpp) +target_link_libraries(scope_manager_test ${OPENTRACING_LIBRARY}) +add_test(NAME scope_manager_test COMMAND scope_manager_test) + if (BUILD_SHARED_LIBS AND BUILD_MOCKTRACER AND BUILD_DYNAMIC_LOADING) add_executable(dynamic_load_test dynamic_load_test.cpp) target_link_libraries(dynamic_load_test ${OPENTRACING_LIBRARY}) diff --git a/test/scope_manager_test.cpp b/test/scope_manager_test.cpp new file mode 100644 index 0000000..e5f7728 --- /dev/null +++ b/test/scope_manager_test.cpp @@ -0,0 +1,9 @@ +#include +#include + +using namespace opentracing; + +#define CATCH_CONFIG_MAIN +#include + +TEST_CASE("scope_manager") { CHECK(true); } From 6a4419d14bd942307bf370a5a51d207ddb5548e8 Mon Sep 17 00:00:00 2001 From: Evgeny Yakimov Date: Thu, 26 Dec 2019 19:34:31 +0000 Subject: [PATCH 03/15] Convert ScopeManager to interface, add ownership semantics The previous idea was to have ScopeManager as a Utility like class with the thread_local implementation. After reviewing the Java implementation, I have made the ScopeManager an interface and a seperate ThreadLocalScopeManager class will provide the thread_local implementation. The reference based interface has been replaced with shared_ptr. This is to allow the Span to be consumed from the ScopeManager and have its lifetime managed by the application/library code as appropriate. E.g. consider an application that keeps the Span alive until an Async signal is received. --- include/opentracing/scope_manager.h | 33 ++++++++++++++++++++--------- src/scope_manager.cpp | 13 +++--------- 2 files changed, 26 insertions(+), 20 deletions(-) diff --git a/include/opentracing/scope_manager.h b/include/opentracing/scope_manager.h index ed23976..101af85 100644 --- a/include/opentracing/scope_manager.h +++ b/include/opentracing/scope_manager.h @@ -7,20 +7,21 @@ namespace opentracing { BEGIN_OPENTRACING_ABI_NAMESPACE class Span; +class ScopeManager; // Scope is returned by the ScopeManager when activating a span // // The lifetime of the Scope instance represends the duration of the -// activation. A Scope cannot be created and can only be returned by -// the ScopeManager. +// activation. A Scope will be returned when Activate is called on the +// the ScopeManager. Its lifetime can not exist beyond that of the +// ScopeManager. class Scope { - private: - // Create an activation Scope for the given Span. - Scope(Span& span) noexcept; - ~Scope() noexcept; + Scope(ScopeManager& manager, std::shared_ptr span); + Scope(Scope&& scope) noexcept; + ~Scope(); - Scope(const Scope&) = delete; - Scope(Scope&&) = delete; + private: + Scope(const Scope& scope) = delete; Scope& operator=(const Scope&) = delete; Scope& operator=(Scope&&) = delete; @@ -33,14 +34,26 @@ class Scope { // executed within the lifetime of the scope and the same thread only. class ScopeManager { public: + virtual ~ScopeManager() = default; + // Activate the given Span, returning a Scope to track its duration. - static Scope Activate(Span& span) noexcept; + // + // A Span MUST be upgraded to a shared_ptr as consumers of the span + // via the ScopeManager may take ownership over it beyond the + // duration of the Scope. + virtual Scope Activate(std::shared_ptr span) noexcept = 0; // Return a reference to the current active Span. // // A span is always guaranteed to be returned. If there is no span // active, then a default noop span instance will be returned. - static Span& ActiveSpan() noexcept; + virtual std::shared_ptr ActiveSpan() noexcept = 0; + + private: + // Set the active Span, used by the Scope class. + virtual void SetActiveSpan(std::shared_ptr span) noexcept = 0; + + friend class Scope; }; END_OPENTRACING_ABI_NAMESPACE diff --git a/src/scope_manager.cpp b/src/scope_manager.cpp index 6157769..ef4af52 100644 --- a/src/scope_manager.cpp +++ b/src/scope_manager.cpp @@ -3,19 +3,12 @@ namespace opentracing { BEGIN_OPENTRACING_ABI_NAMESPACE -namespace { -thread_local Span* active = nullptr; +Scope::Scope(ScopeManager& manager, std::shared_ptr span) {} -} // anonymous namespace +Scope::Scope(Scope&& scope) noexcept {} -Scope::Scope(Span& span) noexcept {} - -Scope::~Scope() noexcept {} - -Scope ScopeManager::Activate(Span& span) noexcept { return {span}; } - -Span& ScopeManager::ActiveSpan() noexcept { return *active; } +Scope::~Scope() {} END_OPENTRACING_ABI_NAMESPACE } // namespace opentracing From 929e5ffdeb6095eaea2bd4845be4ef3883841760 Mon Sep 17 00:00:00 2001 From: Evgeny Yakimov Date: Thu, 26 Dec 2019 20:21:53 +0000 Subject: [PATCH 04/15] TDD Tests for Scope object --- include/opentracing/scope_manager.h | 1 + test/scope_manager_test.cpp | 38 ++++++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/include/opentracing/scope_manager.h b/include/opentracing/scope_manager.h index 101af85..c3313be 100644 --- a/include/opentracing/scope_manager.h +++ b/include/opentracing/scope_manager.h @@ -16,6 +16,7 @@ class ScopeManager; // the ScopeManager. Its lifetime can not exist beyond that of the // ScopeManager. class Scope { + public: Scope(ScopeManager& manager, std::shared_ptr span); Scope(Scope&& scope) noexcept; ~Scope(); diff --git a/test/scope_manager_test.cpp b/test/scope_manager_test.cpp index e5f7728..004049a 100644 --- a/test/scope_manager_test.cpp +++ b/test/scope_manager_test.cpp @@ -6,4 +6,40 @@ using namespace opentracing; #define CATCH_CONFIG_MAIN #include -TEST_CASE("scope_manager") { CHECK(true); } +class MockScopeManager : public ScopeManager { + public: + MockScopeManager() + : default_span_(MakeNoopTracer()->StartSpan("")), set_span_() {} + + Scope Activate(std::shared_ptr span) noexcept override { + return Scope(*this, span); + } + + std::shared_ptr ActiveSpan() noexcept override { return default_span_; } + + std::shared_ptr default_span_; + std::shared_ptr set_span_; + + private: + void SetActiveSpan(std::shared_ptr span) noexcept override { + set_span_ = span; + } +}; + +TEST_CASE("scope") { + MockScopeManager manager; + auto tracer = MakeNoopTracer(); + auto span = std::shared_ptr{tracer->StartSpan("a")}; + + // Validate that the test mock is sane + CHECK(manager.ActiveSpan() == manager.default_span_); + CHECK(manager.set_span_ == nullptr); + + SECTION("Check that Scope calls SetActiveSpan correctly.") { + { + auto Scope = manager.Activate(span); + CHECK(manager.set_span_ == span); + } + CHECK(manager.set_span_ == manager.default_span_); + } +} From 43f0892744a41b2c98f9ee1673c5db6ae6d83bd6 Mon Sep 17 00:00:00 2001 From: Evgeny Yakimov Date: Fri, 27 Dec 2019 03:08:36 +0000 Subject: [PATCH 05/15] Simpler Scope/ScopeManager interface - Change the Scope to simply take a callback and not require friendship with ScopeManager. - ScopeManager can now define implementation when instantiating Scope. - Updated tests/stubs to reflect new Scope interface. - Minor documentation fixes --- include/opentracing/scope_manager.h | 26 ++++++++-------- src/scope_manager.cpp | 2 +- test/scope_manager_test.cpp | 46 +++++++++-------------------- 3 files changed, 27 insertions(+), 47 deletions(-) diff --git a/include/opentracing/scope_manager.h b/include/opentracing/scope_manager.h index c3313be..c803201 100644 --- a/include/opentracing/scope_manager.h +++ b/include/opentracing/scope_manager.h @@ -2,6 +2,7 @@ #define OPENTRACING_SCOPE_MANAGER_H #include +#include namespace opentracing { BEGIN_OPENTRACING_ABI_NAMESPACE @@ -17,7 +18,9 @@ class ScopeManager; // ScopeManager. class Scope { public: - Scope(ScopeManager& manager, std::shared_ptr span); + using Callback = std::function; + + Scope(Callback cb) noexcept; Scope(Scope&& scope) noexcept; ~Scope(); @@ -25,14 +28,14 @@ class Scope { Scope(const Scope& scope) = delete; Scope& operator=(const Scope&) = delete; Scope& operator=(Scope&&) = delete; - - friend class ScopeManager; }; // ScopeManager allows a Span to be activated for a specific scope. // -// Once a span has been activated, it can then be accessed by any code -// executed within the lifetime of the scope and the same thread only. +// Once a Span has been activated, it can then be accessed via the +// ScopeManager. This interface can be implemented to provide +// different characteristics of Span propagation such as passing +// only within the same thread. class ScopeManager { public: virtual ~ScopeManager() = default; @@ -41,20 +44,15 @@ class ScopeManager { // // A Span MUST be upgraded to a shared_ptr as consumers of the span // via the ScopeManager may take ownership over it beyond the - // duration of the Scope. + // duration of the Scope. Implementations are expected to define the + // logic of Scope destrucion. virtual Scope Activate(std::shared_ptr span) noexcept = 0; - // Return a reference to the current active Span. + // Return the current active Span. // // A span is always guaranteed to be returned. If there is no span - // active, then a default noop span instance will be returned. + // active, then a default noop span instance should be returned. virtual std::shared_ptr ActiveSpan() noexcept = 0; - - private: - // Set the active Span, used by the Scope class. - virtual void SetActiveSpan(std::shared_ptr span) noexcept = 0; - - friend class Scope; }; END_OPENTRACING_ABI_NAMESPACE diff --git a/src/scope_manager.cpp b/src/scope_manager.cpp index ef4af52..c2a1ffa 100644 --- a/src/scope_manager.cpp +++ b/src/scope_manager.cpp @@ -4,7 +4,7 @@ namespace opentracing { BEGIN_OPENTRACING_ABI_NAMESPACE -Scope::Scope(ScopeManager& manager, std::shared_ptr span) {} +Scope::Scope(Callback cb) noexcept {} Scope::Scope(Scope&& scope) noexcept {} diff --git a/test/scope_manager_test.cpp b/test/scope_manager_test.cpp index 004049a..02c5a12 100644 --- a/test/scope_manager_test.cpp +++ b/test/scope_manager_test.cpp @@ -1,4 +1,3 @@ -#include #include using namespace opentracing; @@ -6,40 +5,23 @@ using namespace opentracing; #define CATCH_CONFIG_MAIN #include -class MockScopeManager : public ScopeManager { - public: - MockScopeManager() - : default_span_(MakeNoopTracer()->StartSpan("")), set_span_() {} - - Scope Activate(std::shared_ptr span) noexcept override { - return Scope(*this, span); - } - - std::shared_ptr ActiveSpan() noexcept override { return default_span_; } - - std::shared_ptr default_span_; - std::shared_ptr set_span_; - - private: - void SetActiveSpan(std::shared_ptr span) noexcept override { - set_span_ = span; - } -}; - TEST_CASE("scope") { - MockScopeManager manager; - auto tracer = MakeNoopTracer(); - auto span = std::shared_ptr{tracer->StartSpan("a")}; - - // Validate that the test mock is sane - CHECK(manager.ActiveSpan() == manager.default_span_); - CHECK(manager.set_span_ == nullptr); + SECTION("Scope invokes callback on destruction") { + int called = 0; + { + Scope scope{[&called]() { ++called; }}; + CHECK(called == 0); + } + CHECK(called == 1); + } - SECTION("Check that Scope calls SetActiveSpan correctly.") { + SECTION("Scope can be moved") { + int called = 0; { - auto Scope = manager.Activate(span); - CHECK(manager.set_span_ == span); + Scope scope{[&called]() { ++called; }}; + { Scope scope2{std::move(scope)}; } + CHECK(called == 1); } - CHECK(manager.set_span_ == manager.default_span_); + CHECK(called == 1); // check for double calls } } From 568272e165ea3b76eef18ec5440f45155484fbb2 Mon Sep 17 00:00:00 2001 From: Evgeny Yakimov Date: Fri, 27 Dec 2019 03:20:26 +0000 Subject: [PATCH 06/15] Scope implementation --- include/opentracing/scope_manager.h | 13 ++++++++----- src/scope_manager.cpp | 10 +++++++--- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/include/opentracing/scope_manager.h b/include/opentracing/scope_manager.h index c803201..61ef487 100644 --- a/include/opentracing/scope_manager.h +++ b/include/opentracing/scope_manager.h @@ -10,17 +10,18 @@ BEGIN_OPENTRACING_ABI_NAMESPACE class Span; class ScopeManager; -// Scope is returned by the ScopeManager when activating a span +// Scope is returned by the ScopeManager when activating a span. // -// The lifetime of the Scope instance represends the duration of the -// activation. A Scope will be returned when Activate is called on the -// the ScopeManager. Its lifetime can not exist beyond that of the +// The lifetime of the Scope instance represents the duration of the +// activation. Its lifetime can not exist beyond that of the // ScopeManager. class Scope { public: using Callback = std::function; - Scope(Callback cb) noexcept; + // Create a Scope that will invoke callback on destruction. + Scope(Callback callback) noexcept; + Scope(Scope&& scope) noexcept; ~Scope(); @@ -28,6 +29,8 @@ class Scope { Scope(const Scope& scope) = delete; Scope& operator=(const Scope&) = delete; Scope& operator=(Scope&&) = delete; + + Callback callback_; }; // ScopeManager allows a Span to be activated for a specific scope. diff --git a/src/scope_manager.cpp b/src/scope_manager.cpp index c2a1ffa..6e5e442 100644 --- a/src/scope_manager.cpp +++ b/src/scope_manager.cpp @@ -4,11 +4,15 @@ namespace opentracing { BEGIN_OPENTRACING_ABI_NAMESPACE -Scope::Scope(Callback cb) noexcept {} +Scope::Scope(Callback callback) noexcept : callback_(callback) {} -Scope::Scope(Scope&& scope) noexcept {} +Scope::Scope(Scope&& scope) noexcept { std::swap(callback_, scope.callback_); } -Scope::~Scope() {} +Scope::~Scope() { + if (callback_) { + callback_(); + } +} END_OPENTRACING_ABI_NAMESPACE } // namespace opentracing From f1d93a87b33957419778ff09744fdcbf3e11bf4e Mon Sep 17 00:00:00 2001 From: Evgeny Yakimov Date: Fri, 27 Dec 2019 03:28:31 +0000 Subject: [PATCH 07/15] Fix missing include --- include/opentracing/scope_manager.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/opentracing/scope_manager.h b/include/opentracing/scope_manager.h index 61ef487..39702f1 100644 --- a/include/opentracing/scope_manager.h +++ b/include/opentracing/scope_manager.h @@ -3,6 +3,7 @@ #include #include +#include namespace opentracing { BEGIN_OPENTRACING_ABI_NAMESPACE From db98d7a9126f6a2937647a2fd49f7d16cc2bae37 Mon Sep 17 00:00:00 2001 From: Evgeny Yakimov Date: Fri, 27 Dec 2019 13:27:18 +0000 Subject: [PATCH 08/15] ThreadLocalScopeManager header and stubs/tests - Created a ThreadLocalScopeManager which implements ScopeManager - Headers with declarations - Empty stubs and tests - Updated ScopeManager docs - Updated build files for new component --- CMakeLists.txt | 1 + include/opentracing/scope_manager.h | 4 +- .../opentracing/thread_local_scope_manager.h | 43 +++++++++++++++++++ src/thread_local_scope_manager.cpp | 12 ++++++ test/BUILD | 1 + test/CMakeLists.txt | 4 ++ test/thread_local_scope_manager_test.cpp | 8 ++++ 7 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 include/opentracing/thread_local_scope_manager.h create mode 100644 src/thread_local_scope_manager.cpp create mode 100644 test/thread_local_scope_manager_test.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index aab0002..ef15d32 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -137,6 +137,7 @@ set(SRCS src/propagation.cpp src/dynamic_load.cpp src/noop.cpp src/scope_manager.cpp + src/thread_local_scope_manager.cpp src/tracer.cpp src/tracer_factory.cpp src/ext/tags.cpp) diff --git a/include/opentracing/scope_manager.h b/include/opentracing/scope_manager.h index 39702f1..e85c605 100644 --- a/include/opentracing/scope_manager.h +++ b/include/opentracing/scope_manager.h @@ -15,7 +15,9 @@ class ScopeManager; // // The lifetime of the Scope instance represents the duration of the // activation. Its lifetime can not exist beyond that of the -// ScopeManager. +// ScopeManager. The specific implementation of ScopeManager may +// enforce additional constraints for the Scope object, please defer +// to the documentation of the ScopeManager for further information. class Scope { public: using Callback = std::function; diff --git a/include/opentracing/thread_local_scope_manager.h b/include/opentracing/thread_local_scope_manager.h new file mode 100644 index 0000000..2202400 --- /dev/null +++ b/include/opentracing/thread_local_scope_manager.h @@ -0,0 +1,43 @@ +#ifndef OPENTRACING_THREAD_LOCAL_SCOPE_MANAGER_H +#define OPENTRACING_THREAD_LOCAL_SCOPE_MANAGER_H + +#include +#include + +#include + +namespace opentracing { +BEGIN_OPENTRACING_ABI_NAMESPACE + +class Span; + +// A ScopeManager for propagating Spans within the same thread. +// +// Once activated, during the lifetime of the Scope, the Span can be +// accessed only within the same thread. This behaviour is best for +// propagating Spans down the execution stack without requiring each +// component to forward it explicitly. This is achieved using +// thread_local variables. +class ThreadLocalScopeManager : public ScopeManager { + public: + // Activate the given Span, returning a Scope to track its duration. + // + // An activated Span is only accessible from the same thread as the + // Activation for the lifetime of the Scope. The Scope MUST be stored + // on the Stack and MUST NOT be moved beyond the time of + // instantiation, otherwise behaviour is undefined. + Scope Activate(std::shared_ptr span) noexcept override; + + // Return the current active Span. + // + // A span is always guaranteed to be returned. If there is no span + // active, then a default noop span instance should be returned. + // Only active Spans from the same thread as the activation are + // returned. + std::shared_ptr ActiveSpan() noexcept override; +}; + +END_OPENTRACING_ABI_NAMESPACE +} // namespace opentracing + +#endif // OPENTRACING_THREAD_LOCAL_SCOPE_MANAGER_H diff --git a/src/thread_local_scope_manager.cpp b/src/thread_local_scope_manager.cpp new file mode 100644 index 0000000..0926f0a --- /dev/null +++ b/src/thread_local_scope_manager.cpp @@ -0,0 +1,12 @@ +#include +#include + +namespace opentracing { +BEGIN_OPENTRACING_ABI_NAMESPACE + +Scope ThreadLocalScopeManager::Activate(std::shared_ptr span) noexcept {} + +std::shared_ptr ThreadLocalScopeManager::ActiveSpan() noexcept {} + +END_OPENTRACING_ABI_NAMESPACE +} // namespace opentracing diff --git a/test/BUILD b/test/BUILD index 779e1ce..6e79e4c 100644 --- a/test/BUILD +++ b/test/BUILD @@ -1,6 +1,7 @@ TEST_NAMES = [ "scope_manager_test", "string_view_test", + "thread_local_scope_manager_test", "tracer_test", "util_test", "value_test", diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index aae6f27..4b4b4ab 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -21,6 +21,10 @@ add_executable(scope_manager_test scope_manager_test.cpp) target_link_libraries(scope_manager_test ${OPENTRACING_LIBRARY}) add_test(NAME scope_manager_test COMMAND scope_manager_test) +add_executable(thread_local_scope_manager_test thread_local_scope_manager_test.cpp) +target_link_libraries(thread_local_scope_manager_test ${OPENTRACING_LIBRARY}) +add_test(NAME thread_local_scope_manager_test COMMAND thread_local_scope_manager_test) + if (BUILD_SHARED_LIBS AND BUILD_MOCKTRACER AND BUILD_DYNAMIC_LOADING) add_executable(dynamic_load_test dynamic_load_test.cpp) target_link_libraries(dynamic_load_test ${OPENTRACING_LIBRARY}) diff --git a/test/thread_local_scope_manager_test.cpp b/test/thread_local_scope_manager_test.cpp new file mode 100644 index 0000000..01ca7ce --- /dev/null +++ b/test/thread_local_scope_manager_test.cpp @@ -0,0 +1,8 @@ +#include + +using namespace opentracing; + +#define CATCH_CONFIG_MAIN +#include + +TEST_CASE("thread_local_scope_manager") { CHECK(true); } From 369637bb2db9f03c4fa79ac11a3689ffba20dbc9 Mon Sep 17 00:00:00 2001 From: Evgeny Yakimov Date: Fri, 27 Dec 2019 14:39:52 +0000 Subject: [PATCH 09/15] TDD tests for ThreadLocalScopeManager --- test/thread_local_scope_manager_test.cpp | 54 +++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/test/thread_local_scope_manager_test.cpp b/test/thread_local_scope_manager_test.cpp index 01ca7ce..ddcb174 100644 --- a/test/thread_local_scope_manager_test.cpp +++ b/test/thread_local_scope_manager_test.cpp @@ -1,8 +1,60 @@ +#include #include +#include +#include using namespace opentracing; #define CATCH_CONFIG_MAIN #include -TEST_CASE("thread_local_scope_manager") { CHECK(true); } +TEST_CASE("thread_local_scope_manager") { + ThreadLocalScopeManager sm; + std::shared_ptr default_span; + + SECTION("Returns noop span with no activations") { + default_span = sm.ActiveSpan(); + CHECK(default_span); + } + + auto tracer = MakeNoopTracer(); + + SECTION("Basic span activation/deactivation") { + std::shared_ptr span{tracer->StartSpan("a")}; + CHECK(sm.ActiveSpan() == default_span); + { + auto scope = sm.Activate(span); + CHECK(sm.ActiveSpan() == span); + } + CHECK(sm.ActiveSpan() == default_span); + } + + SECTION("Nested span activation/deactivation") { + std::shared_ptr span1{tracer->StartSpan("1")}; + std::shared_ptr span2{tracer->StartSpan("2")}; + CHECK(sm.ActiveSpan() == default_span); + { + auto scope1 = sm.Activate(span1); + CHECK(sm.ActiveSpan() == span1); + { + auto scope2 = sm.Activate(span2); + CHECK(sm.ActiveSpan() == span2); + } + CHECK(sm.ActiveSpan() == span1); + } + CHECK(sm.ActiveSpan() == default_span); + } + + SECTION("Validate span activation is local to thread") { + std::shared_ptr thread_span; + std::shared_ptr span{tracer->StartSpan("a")}; + auto scope = sm.Activate(span); + + // Start thread which captures active span + std::thread{[&thread_span, &sm]() { + thread_span = sm.ActiveSpan(); + }}.join(); + + CHECK(thread_span != span); + } +} From 98b16cfd74a73a184059321fab08b0865295a33d Mon Sep 17 00:00:00 2001 From: Evgeny Yakimov Date: Fri, 27 Dec 2019 15:25:10 +0000 Subject: [PATCH 10/15] ThreadLocalScopeManager implementation --- src/thread_local_scope_manager.cpp | 30 ++++++++++++++++++++++-- test/thread_local_scope_manager_test.cpp | 7 ++---- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/src/thread_local_scope_manager.cpp b/src/thread_local_scope_manager.cpp index 0926f0a..9870832 100644 --- a/src/thread_local_scope_manager.cpp +++ b/src/thread_local_scope_manager.cpp @@ -1,12 +1,38 @@ #include #include +#include +#include + namespace opentracing { BEGIN_OPENTRACING_ABI_NAMESPACE -Scope ThreadLocalScopeManager::Activate(std::shared_ptr span) noexcept {} +namespace { + +const std::shared_ptr noopspan{MakeNoopTracer()->StartSpan("")}; +using ThreadLocalSpanMap = + std::map>; +thread_local ThreadLocalSpanMap thread_local_span_map; + +} // anonymous namespace + +Scope ThreadLocalScopeManager::Activate(std::shared_ptr span) noexcept { + auto it_bool = thread_local_span_map.insert(std::make_pair(this, span)); + if (it_bool.second) { + return Scope{[this]() { thread_local_span_map.erase(this); }}; + } else { + std::swap(it_bool.first->second, span); + return Scope{[this, span]() { thread_local_span_map[this] = span; }}; + } +} -std::shared_ptr ThreadLocalScopeManager::ActiveSpan() noexcept {} +std::shared_ptr ThreadLocalScopeManager::ActiveSpan() noexcept { + auto active_it = thread_local_span_map.find(this); + if (active_it != thread_local_span_map.end()) { + return active_it->second; + } + return noopspan; +} END_OPENTRACING_ABI_NAMESPACE } // namespace opentracing diff --git a/test/thread_local_scope_manager_test.cpp b/test/thread_local_scope_manager_test.cpp index ddcb174..2ea8a93 100644 --- a/test/thread_local_scope_manager_test.cpp +++ b/test/thread_local_scope_manager_test.cpp @@ -10,12 +10,9 @@ using namespace opentracing; TEST_CASE("thread_local_scope_manager") { ThreadLocalScopeManager sm; - std::shared_ptr default_span; + std::shared_ptr default_span = sm.ActiveSpan(); - SECTION("Returns noop span with no activations") { - default_span = sm.ActiveSpan(); - CHECK(default_span); - } + SECTION("Returns noop span with no activations") { CHECK(default_span); } auto tracer = MakeNoopTracer(); From 94ba98f5beaba628d29c4801870bd3fdf881da7c Mon Sep 17 00:00:00 2001 From: Evgeny Yakimov Date: Fri, 27 Dec 2019 15:50:59 +0000 Subject: [PATCH 11/15] Extend Tracer interface with ScopeManager --- include/opentracing/tracer.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/include/opentracing/tracer.h b/include/opentracing/tracer.h index a4a125a..6f751d4 100644 --- a/include/opentracing/tracer.h +++ b/include/opentracing/tracer.h @@ -2,6 +2,7 @@ #define OPENTRACING_TRACER_H #include +#include #include #include #include @@ -147,6 +148,13 @@ class OPENTRACING_API Tracer { return reader.Extract(*this); } + // Return a referene to the tracer's ScopeManager + // + // If not overriden, the default ThreadLocalScopeManager will be returned. + // The ScopeManager is merged with the Tracer for convenience so as not to + // require users to manage ownership themselves. + virtual ScopeManager& ScopeManager() const; + // Close is called when a tracer is finished processing spans. It is not // required to be called and its effect is unspecified. For example, an // implementation might use this function to flush buffered spans to its From f552ce8bd92f90dde98b4e076da5f7c7858087ce Mon Sep 17 00:00:00 2001 From: Evgeny Yakimov Date: Fri, 27 Dec 2019 15:54:04 +0000 Subject: [PATCH 12/15] TDD tests for Tracer ScopeManager --- test/tracer_test.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/tracer_test.cpp b/test/tracer_test.cpp index eca3a36..9648a02 100644 --- a/test/tracer_test.cpp +++ b/test/tracer_test.cpp @@ -33,6 +33,10 @@ TEST_CASE("tracer") { ChildOf(nullptr).Apply(options); CHECK(options.references.size() == 0); } + + SECTION("Tracer provides a valid ScopeManager") { + tracer.ScopeManager().Activate(span1); + } } TEST_CASE("A tracer can be globally registered") { From b28efdb65a90409cef924f83554d5015046a81ee Mon Sep 17 00:00:00 2001 From: Evgeny Yakimov Date: Fri, 27 Dec 2019 16:02:53 +0000 Subject: [PATCH 13/15] Implemented Tracer to provide ScopeManager - Tracer to instantiate its own ScopeManager (for backwards compatibility) - Implementation provided but can be overriden by Tracer implementations - Fixed minor errors in original tests --- include/opentracing/tracer.h | 7 +++++++ src/tracer.cpp | 3 +++ test/tracer_test.cpp | 4 +++- 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/include/opentracing/tracer.h b/include/opentracing/tracer.h index 6f751d4..4aa9b03 100644 --- a/include/opentracing/tracer.h +++ b/include/opentracing/tracer.h @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -171,6 +172,12 @@ class OPENTRACING_API Tracer { std::shared_ptr tracer) noexcept; static bool IsGlobalTracerRegistered() noexcept; + + private: + // The default ScopeManager to use if the Tracer doesn't provide their own. + // + // This is also required for backwards compatiblity. + mutable ThreadLocalScopeManager scope_manager_; }; // StartTimestamp is a StartSpanOption that sets an explicit start timestamp for diff --git a/src/tracer.cpp b/src/tracer.cpp index 52f2f46..e8cf9a7 100644 --- a/src/tracer.cpp +++ b/src/tracer.cpp @@ -56,5 +56,8 @@ std::shared_ptr Tracer::InitGlobal( bool Tracer::IsGlobalTracerRegistered() noexcept { return TracerRegistry::instance().is_registered(); } + +ScopeManager& Tracer::ScopeManager() const { return scope_manager_; } + END_OPENTRACING_ABI_NAMESPACE } // namespace opentracing diff --git a/test/tracer_test.cpp b/test/tracer_test.cpp index 9648a02..223c2f7 100644 --- a/test/tracer_test.cpp +++ b/test/tracer_test.cpp @@ -1,6 +1,7 @@ #include #include #include +#include using namespace opentracing; #define CATCH_CONFIG_MAIN @@ -35,7 +36,8 @@ TEST_CASE("tracer") { } SECTION("Tracer provides a valid ScopeManager") { - tracer.ScopeManager().Activate(span1); + std::shared_ptr span{tracer->StartSpan("s")}; + tracer->ScopeManager().Activate(span); } } From d9370d54d9461401e9e041e53659068cd4a0c8e0 Mon Sep 17 00:00:00 2001 From: Evgeny Yakimov Date: Sun, 5 Jan 2020 17:47:00 +0000 Subject: [PATCH 14/15] Minor build and typo fixes - GCC complained about function name/type of ScopeManager - Typo in docs - On ubuntu, tests required pthreads --- include/opentracing/tracer.h | 4 ++-- src/tracer.cpp | 2 +- test/CMakeLists.txt | 4 +++- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/include/opentracing/tracer.h b/include/opentracing/tracer.h index 4aa9b03..88cf6c2 100644 --- a/include/opentracing/tracer.h +++ b/include/opentracing/tracer.h @@ -149,12 +149,12 @@ class OPENTRACING_API Tracer { return reader.Extract(*this); } - // Return a referene to the tracer's ScopeManager + // Return a reference to the tracer's ScopeManager. // // If not overriden, the default ThreadLocalScopeManager will be returned. // The ScopeManager is merged with the Tracer for convenience so as not to // require users to manage ownership themselves. - virtual ScopeManager& ScopeManager() const; + virtual opentracing::ScopeManager& ScopeManager() const; // Close is called when a tracer is finished processing spans. It is not // required to be called and its effect is unspecified. For example, an diff --git a/src/tracer.cpp b/src/tracer.cpp index e8cf9a7..1a51f15 100644 --- a/src/tracer.cpp +++ b/src/tracer.cpp @@ -57,7 +57,7 @@ bool Tracer::IsGlobalTracerRegistered() noexcept { return TracerRegistry::instance().is_registered(); } -ScopeManager& Tracer::ScopeManager() const { return scope_manager_; } +opentracing::ScopeManager& Tracer::ScopeManager() const { return scope_manager_; } END_OPENTRACING_ABI_NAMESPACE } // namespace opentracing diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 4b4b4ab..9afdc0c 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -4,6 +4,8 @@ else() set(OPENTRACING_LIBRARY opentracing-static) endif() +find_package(Threads) + add_executable(tracer_test tracer_test.cpp) target_link_libraries(tracer_test ${OPENTRACING_LIBRARY}) add_test(NAME tracer_test COMMAND tracer_test) @@ -22,7 +24,7 @@ target_link_libraries(scope_manager_test ${OPENTRACING_LIBRARY}) add_test(NAME scope_manager_test COMMAND scope_manager_test) add_executable(thread_local_scope_manager_test thread_local_scope_manager_test.cpp) -target_link_libraries(thread_local_scope_manager_test ${OPENTRACING_LIBRARY}) +target_link_libraries(thread_local_scope_manager_test ${OPENTRACING_LIBRARY} ${CMAKE_THREAD_LIBS_INIT}) add_test(NAME thread_local_scope_manager_test COMMAND thread_local_scope_manager_test) if (BUILD_SHARED_LIBS AND BUILD_MOCKTRACER AND BUILD_DYNAMIC_LOADING) From 882a9cafa4f855e138f81d324be53ef338daca05 Mon Sep 17 00:00:00 2001 From: Evgeny Yakimov Date: Sun, 5 Jan 2020 20:48:26 +0000 Subject: [PATCH 15/15] Bazel to match CMakeLists.txt --- test/BUILD | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/test/BUILD b/test/BUILD index 6e79e4c..a8c351f 100644 --- a/test/BUILD +++ b/test/BUILD @@ -1,7 +1,6 @@ TEST_NAMES = [ "scope_manager_test", "string_view_test", - "thread_local_scope_manager_test", "tracer_test", "util_test", "value_test", @@ -16,6 +15,20 @@ TEST_NAMES = [ ], ) for test_name in TEST_NAMES] +cc_test( + name = "thread_local_scope_manager_test", + srcs = [ + "thread_local_scope_manager_test.cpp" + ], + deps = [ + "//:opentracing", + "//3rd_party:catch2", + ], + linkopts = [ + "-lpthread", + ] +) + cc_test( name = "mutiple_tracer_link_test", srcs = [