From 364150112f5c114298aba5d186ad33e51c32971d Mon Sep 17 00:00:00 2001 From: Keshav Manghat Date: Mon, 20 Jul 2020 20:27:33 +0000 Subject: [PATCH 1/7] Threadsafe recordable for tracez zpages --- .../ext/zpages/threadsafe_span_data.h | 172 ++++++++++++++++++ ext/test/zpages/BUILD | 12 ++ ext/test/zpages/CMakeLists.txt | 2 +- ext/test/zpages/threadsafe_span_data_test.cc | 67 +++++++ 4 files changed, 252 insertions(+), 1 deletion(-) create mode 100644 ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h create mode 100644 ext/test/zpages/threadsafe_span_data_test.cc diff --git a/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h b/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h new file mode 100644 index 0000000000..f2db0c15d9 --- /dev/null +++ b/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h @@ -0,0 +1,172 @@ +#pragma once + +#include +#include +#include +#include + +#include "opentelemetry/core/timestamp.h" +#include "opentelemetry/nostd/string_view.h" +#include "opentelemetry/sdk/trace/recordable.h" +#include "opentelemetry/trace/canonical_code.h" +#include "opentelemetry/trace/span.h" +#include "opentelemetry/sdk/trace/span_data.h" +#include "opentelemetry/trace/span_id.h" +#include "opentelemetry/trace/trace_id.h" + +using opentelemetry::sdk::trace::AttributeConverter; +using opentelemetry::sdk::trace::SpanDataAttributeValue; +namespace trace_api = opentelemetry::trace; + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace ext { +namespace zpages { + +/** + * This class is a threadsafe version of span data used for zpages in OT + */ +class ThreadsafeSpanData final : public opentelemetry::sdk::trace::Recordable { + public: + /** + * Get the trace id for this span + * @return the trace id for this span + */ + opentelemetry::trace::TraceId GetTraceId() const noexcept { + std::lock_guard lock(mutex_); + return trace_id_; + } + + /** + * Get the span id for this span + * @return the span id for this span + */ + opentelemetry::trace::SpanId GetSpanId() const noexcept { + std::lock_guard lock(mutex_); + return span_id_; + } + + /** + * Get the parent span id for this span + * @return the span id for this span's parent + */ + opentelemetry::trace::SpanId GetParentSpanId() const noexcept { + std::lock_guard lock(mutex_); + return parent_span_id_; + } + + /** + * Get the name for this span + * @return the name for this span + */ + opentelemetry::nostd::string_view GetName() const noexcept { + std::lock_guard lock(mutex_); + return name_; + } + + /** + * Get the status for this span + * @return the status for this span + */ + opentelemetry::trace::CanonicalCode GetStatus() const noexcept { + std::lock_guard lock(mutex_); + return status_code_; + } + + /** + * Get the status description for this span + * @return the description of the the status of this span + */ + opentelemetry::nostd::string_view GetDescription() const noexcept { + std::lock_guard lock(mutex_); + return status_desc_; + } + + /** + * Get the start time for this span + * @return the start time for this span + */ + opentelemetry::core::SystemTimestamp GetStartTime() const noexcept { + std::lock_guard lock(mutex_); + return start_time_; + } + + /** + * Get the duration for this span + * @return the duration for this span + */ + std::chrono::nanoseconds GetDuration() const noexcept { + std::lock_guard lock(mutex_); + return duration_; + } + + /** + * Get the attributes for this span + * @return the attributes for this span + */ + const std::unordered_map &GetAttributes() + const noexcept { + std::lock_guard lock(mutex_); + return attributes_; + } + + void SetIds(opentelemetry::trace::TraceId trace_id, + opentelemetry::trace::SpanId span_id, + opentelemetry::trace::SpanId parent_span_id) noexcept override { + std::lock_guard lock(mutex_); + trace_id_ = trace_id; + span_id_ = span_id; + parent_span_id_ = parent_span_id; + } + + void SetAttribute(nostd::string_view key, + const common::AttributeValue &value) noexcept override { + std::lock_guard lock(mutex_); + attributes_[std::string(key)] = nostd::visit(converter_, value); + } + + void AddEvent(nostd::string_view name, + core::SystemTimestamp timestamp) noexcept override { + std::lock_guard lock(mutex_); + (void)name; + (void)timestamp; + } + + void SetStatus(trace_api::CanonicalCode code, + nostd::string_view description) noexcept override { + std::lock_guard lock(mutex_); + status_code_ = code; + status_desc_ = std::string(description); + } + + void SetName(nostd::string_view name) noexcept override { + std::lock_guard lock(mutex_); + name_ = std::string(name); + } + + void SetStartTime( + opentelemetry::core::SystemTimestamp start_time) noexcept override { + std::lock_guard lock(mutex_); + start_time_ = start_time; + } + + void SetDuration(std::chrono::nanoseconds duration) noexcept override { + std::lock_guard lock(mutex_); + duration_ = duration; + } + + private: + mutable std::mutex mutex_; + opentelemetry::trace::TraceId trace_id_; + opentelemetry::trace::SpanId span_id_; + opentelemetry::trace::SpanId parent_span_id_; + core::SystemTimestamp start_time_; + std::chrono::nanoseconds duration_{0}; + std::string name_; + opentelemetry::trace::CanonicalCode status_code_{ + opentelemetry::trace::CanonicalCode::OK}; + std::string status_desc_; + std::unordered_map attributes_; + AttributeConverter converter_; +}; +} // namespace zpages +} // namespace ext diff --git a/ext/test/zpages/BUILD b/ext/test/zpages/BUILD index 50d027dbbd..3d6dfbc69a 100644 --- a/ext/test/zpages/BUILD +++ b/ext/test/zpages/BUILD @@ -1,3 +1,15 @@ +cc_test( + name = "threadsafe_span_data_tests", + srcs = [ + "threadsafe_span_data_test.cc", + ], + deps = [ + "//sdk/src/trace", + "//ext/src/zpages", + "@com_google_googletest//:gtest_main", + ], +) + cc_test( name = "tracez_processor_tests", srcs = [ diff --git a/ext/test/zpages/CMakeLists.txt b/ext/test/zpages/CMakeLists.txt index c0e1ddc15b..2fb33cde27 100644 --- a/ext/test/zpages/CMakeLists.txt +++ b/ext/test/zpages/CMakeLists.txt @@ -1,5 +1,5 @@ foreach(testname - tracez_processor_test) + tracez_processor_test threadsafe_span_data_test) add_executable(${testname} "${testname}.cc") target_link_libraries( ${testname} ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT} diff --git a/ext/test/zpages/threadsafe_span_data_test.cc b/ext/test/zpages/threadsafe_span_data_test.cc new file mode 100644 index 0000000000..8cd3e223dd --- /dev/null +++ b/ext/test/zpages/threadsafe_span_data_test.cc @@ -0,0 +1,67 @@ +#include "opentelemetry/ext/zpages/threadsafe_span_data.h" +#include "opentelemetry/nostd/variant.h" +#include "opentelemetry/trace/span_id.h" +#include "opentelemetry/trace/trace_id.h" + +#include +#include + +using opentelemetry::sdk::trace::AttributeConverter; +using opentelemetry::sdk::trace::SpanDataAttributeValue; +using opentelemetry::ext::zpages::ThreadsafeSpanData; + +TEST(ThreadsafeSpanData, DefaultValues) +{ + opentelemetry::trace::TraceId zero_trace_id; + opentelemetry::trace::SpanId zero_span_id; + ThreadsafeSpanData data; + + ASSERT_EQ(data.GetTraceId(), zero_trace_id); + ASSERT_EQ(data.GetSpanId(), zero_span_id); + ASSERT_EQ(data.GetParentSpanId(), zero_span_id); + ASSERT_EQ(data.GetName(), ""); + ASSERT_EQ(data.GetStatus(), opentelemetry::trace::CanonicalCode::OK); + ASSERT_EQ(data.GetDescription(), ""); + ASSERT_EQ(data.GetStartTime().time_since_epoch(), std::chrono::nanoseconds(0)); + ASSERT_EQ(data.GetDuration(), std::chrono::nanoseconds(0)); + ASSERT_EQ(data.GetAttributes().size(), 0); +} + +TEST(ThreadsafeSpanData, Set) +{ + opentelemetry::trace::TraceId trace_id; + opentelemetry::trace::SpanId span_id; + opentelemetry::trace::SpanId parent_span_id; + opentelemetry::core::SystemTimestamp now(std::chrono::system_clock::now()); + + ThreadsafeSpanData data; + data.SetIds(trace_id, span_id, parent_span_id); + data.SetName("span name"); + data.SetStatus(opentelemetry::trace::CanonicalCode::UNKNOWN, "description"); + data.SetStartTime(now); + data.SetDuration(std::chrono::nanoseconds(1000000)); + data.SetAttribute("attr1", 314159); + data.AddEvent("event1", now); + + ASSERT_EQ(data.GetTraceId(), trace_id); + ASSERT_EQ(data.GetSpanId(), span_id); + ASSERT_EQ(data.GetParentSpanId(), parent_span_id); + ASSERT_EQ(data.GetName(), "span name"); + ASSERT_EQ(data.GetStatus(), opentelemetry::trace::CanonicalCode::UNKNOWN); + ASSERT_EQ(data.GetDescription(), "description"); + ASSERT_EQ(data.GetStartTime().time_since_epoch(), now.time_since_epoch()); + ASSERT_EQ(data.GetDuration(), std::chrono::nanoseconds(1000000)); + ASSERT_EQ(opentelemetry::nostd::get(data.GetAttributes().at("attr1")), 314159); +} + +TEST(ThreadsafeSpanData, ThreadSafety) +{ + opentelemetry::trace::TraceId trace_id; + opentelemetry::trace::SpanId span_id; + opentelemetry::trace::SpanId parent_span_id; + + ThreadsafeSpanData data; + + std::thread set_ids(&ThreadsafeSpanData::SetIds, &data, trace_id, span_id, parent_span_id); + set_ids.join(); +} From ee267f5490810415a7cae24c27f4c6991d3bf5ad Mon Sep 17 00:00:00 2001 From: Keshav Manghat Date: Mon, 20 Jul 2020 20:34:14 +0000 Subject: [PATCH 2/7] Updated span data tests --- ext/test/zpages/threadsafe_span_data_test.cc | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/ext/test/zpages/threadsafe_span_data_test.cc b/ext/test/zpages/threadsafe_span_data_test.cc index 8cd3e223dd..f9686ee326 100644 --- a/ext/test/zpages/threadsafe_span_data_test.cc +++ b/ext/test/zpages/threadsafe_span_data_test.cc @@ -54,14 +54,3 @@ TEST(ThreadsafeSpanData, Set) ASSERT_EQ(opentelemetry::nostd::get(data.GetAttributes().at("attr1")), 314159); } -TEST(ThreadsafeSpanData, ThreadSafety) -{ - opentelemetry::trace::TraceId trace_id; - opentelemetry::trace::SpanId span_id; - opentelemetry::trace::SpanId parent_span_id; - - ThreadsafeSpanData data; - - std::thread set_ids(&ThreadsafeSpanData::SetIds, &data, trace_id, span_id, parent_span_id); - set_ids.join(); -} From d0a549e901673aacc3520f112b933d1aab1108e7 Mon Sep 17 00:00:00 2001 From: Keshav Manghat Date: Mon, 20 Jul 2020 20:39:59 +0000 Subject: [PATCH 3/7] updated tests --- ext/test/zpages/threadsafe_span_data_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ext/test/zpages/threadsafe_span_data_test.cc b/ext/test/zpages/threadsafe_span_data_test.cc index f9686ee326..09b44b8e9f 100644 --- a/ext/test/zpages/threadsafe_span_data_test.cc +++ b/ext/test/zpages/threadsafe_span_data_test.cc @@ -3,8 +3,9 @@ #include "opentelemetry/trace/span_id.h" #include "opentelemetry/trace/trace_id.h" -#include #include +#include + using opentelemetry::sdk::trace::AttributeConverter; using opentelemetry::sdk::trace::SpanDataAttributeValue; From 117384300183398254c82eef43f24fca2006169a Mon Sep 17 00:00:00 2001 From: Keshav Manghat Date: Mon, 20 Jul 2020 21:11:00 +0000 Subject: [PATCH 4/7] Added threadsafe span data for zpages --- .../opentelemetry/ext/zpages/threadsafe_span_data.h | 1 + ext/test/zpages/BUILD | 12 ------------ ext/test/zpages/CMakeLists.txt | 2 +- ext/test/zpages/threadsafe_span_data_test.cc | 4 +--- 4 files changed, 3 insertions(+), 16 deletions(-) diff --git a/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h b/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h index f2db0c15d9..dfe94347b0 100644 --- a/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h +++ b/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h @@ -170,3 +170,4 @@ class ThreadsafeSpanData final : public opentelemetry::sdk::trace::Recordable { }; } // namespace zpages } // namespace ext +OPENTELEMETRY_END_NAMESPACE diff --git a/ext/test/zpages/BUILD b/ext/test/zpages/BUILD index 3d6dfbc69a..50d027dbbd 100644 --- a/ext/test/zpages/BUILD +++ b/ext/test/zpages/BUILD @@ -1,15 +1,3 @@ -cc_test( - name = "threadsafe_span_data_tests", - srcs = [ - "threadsafe_span_data_test.cc", - ], - deps = [ - "//sdk/src/trace", - "//ext/src/zpages", - "@com_google_googletest//:gtest_main", - ], -) - cc_test( name = "tracez_processor_tests", srcs = [ diff --git a/ext/test/zpages/CMakeLists.txt b/ext/test/zpages/CMakeLists.txt index 2fb33cde27..c0e1ddc15b 100644 --- a/ext/test/zpages/CMakeLists.txt +++ b/ext/test/zpages/CMakeLists.txt @@ -1,5 +1,5 @@ foreach(testname - tracez_processor_test threadsafe_span_data_test) + tracez_processor_test) add_executable(${testname} "${testname}.cc") target_link_libraries( ${testname} ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT} diff --git a/ext/test/zpages/threadsafe_span_data_test.cc b/ext/test/zpages/threadsafe_span_data_test.cc index 09b44b8e9f..51f6765eb0 100644 --- a/ext/test/zpages/threadsafe_span_data_test.cc +++ b/ext/test/zpages/threadsafe_span_data_test.cc @@ -3,9 +3,8 @@ #include "opentelemetry/trace/span_id.h" #include "opentelemetry/trace/trace_id.h" -#include #include - +#include using opentelemetry::sdk::trace::AttributeConverter; using opentelemetry::sdk::trace::SpanDataAttributeValue; @@ -54,4 +53,3 @@ TEST(ThreadsafeSpanData, Set) ASSERT_EQ(data.GetDuration(), std::chrono::nanoseconds(1000000)); ASSERT_EQ(opentelemetry::nostd::get(data.GetAttributes().at("attr1")), 314159); } - From 042be9fda73a4b4bd6b943a46f48015b53ca4b07 Mon Sep 17 00:00:00 2001 From: Keshav Manghat Date: Mon, 20 Jul 2020 21:15:21 +0000 Subject: [PATCH 5/7] Added tests to build and cmake file --- ext/test/zpages/BUILD | 12 ++++++++++++ ext/test/zpages/CMakeLists.txt | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/ext/test/zpages/BUILD b/ext/test/zpages/BUILD index 50d027dbbd..3d6dfbc69a 100644 --- a/ext/test/zpages/BUILD +++ b/ext/test/zpages/BUILD @@ -1,3 +1,15 @@ +cc_test( + name = "threadsafe_span_data_tests", + srcs = [ + "threadsafe_span_data_test.cc", + ], + deps = [ + "//sdk/src/trace", + "//ext/src/zpages", + "@com_google_googletest//:gtest_main", + ], +) + cc_test( name = "tracez_processor_tests", srcs = [ diff --git a/ext/test/zpages/CMakeLists.txt b/ext/test/zpages/CMakeLists.txt index c0e1ddc15b..2fb33cde27 100644 --- a/ext/test/zpages/CMakeLists.txt +++ b/ext/test/zpages/CMakeLists.txt @@ -1,5 +1,5 @@ foreach(testname - tracez_processor_test) + tracez_processor_test threadsafe_span_data_test) add_executable(${testname} "${testname}.cc") target_link_libraries( ${testname} ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT} From 636bdf493d0837d740778c58a9203be601ddbc97 Mon Sep 17 00:00:00 2001 From: Keshav Manghat Date: Thu, 23 Jul 2020 13:23:41 +0000 Subject: [PATCH 6/7] Return copy of map rather than reference --- ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h b/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h index dfe94347b0..88660ffd80 100644 --- a/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h +++ b/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h @@ -103,7 +103,7 @@ class ThreadsafeSpanData final : public opentelemetry::sdk::trace::Recordable { * Get the attributes for this span * @return the attributes for this span */ - const std::unordered_map &GetAttributes() + const std::unordered_map GetAttributes() const noexcept { std::lock_guard lock(mutex_); return attributes_; From c4c3848f4c35c42f2f24f6a040be77095ed19da2 Mon Sep 17 00:00:00 2001 From: Keshav Manghat Date: Thu, 23 Jul 2020 18:39:10 +0000 Subject: [PATCH 7/7] Implementing new functions from recordable interface --- .../ext/zpages/threadsafe_span_data.h | 30 ++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h b/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h index 88660ffd80..a8f596696c 100644 --- a/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h +++ b/ext/include/opentelemetry/ext/zpages/threadsafe_span_data.h @@ -16,6 +16,7 @@ using opentelemetry::sdk::trace::AttributeConverter; using opentelemetry::sdk::trace::SpanDataAttributeValue; +using opentelemetry::sdk::trace::SpanDataEvent; namespace trace_api = opentelemetry::trace; OPENTELEMETRY_BEGIN_NAMESPACE @@ -124,13 +125,6 @@ class ThreadsafeSpanData final : public opentelemetry::sdk::trace::Recordable { attributes_[std::string(key)] = nostd::visit(converter_, value); } - void AddEvent(nostd::string_view name, - core::SystemTimestamp timestamp) noexcept override { - std::lock_guard lock(mutex_); - (void)name; - (void)timestamp; - } - void SetStatus(trace_api::CanonicalCode code, nostd::string_view description) noexcept override { std::lock_guard lock(mutex_); @@ -153,6 +147,27 @@ class ThreadsafeSpanData final : public opentelemetry::sdk::trace::Recordable { std::lock_guard lock(mutex_); duration_ = duration; } + + void AddLink( + opentelemetry::trace::SpanContext span_context, + const trace_api::KeyValueIterable &attributes = + trace_api::KeyValueIterableView>({})) noexcept override + { + std::lock_guard lock(mutex_); + (void)span_context; + (void)attributes; + } + + void AddEvent( + nostd::string_view name, + core::SystemTimestamp timestamp = core::SystemTimestamp(std::chrono::system_clock::now()), + const trace_api::KeyValueIterable &attributes = + trace_api::KeyValueIterableView>({})) noexcept override + { + std::lock_guard lock(mutex_); + events_.push_back(SpanDataEvent(std::string(name), timestamp)); + // TODO: handle attributes + } private: mutable std::mutex mutex_; @@ -166,6 +181,7 @@ class ThreadsafeSpanData final : public opentelemetry::sdk::trace::Recordable { opentelemetry::trace::CanonicalCode::OK}; std::string status_desc_; std::unordered_map attributes_; + std::vector events_; AttributeConverter converter_; }; } // namespace zpages