From 4bf58599383441a473bd146e4793157d25033239 Mon Sep 17 00:00:00 2001 From: Nadia Ciobanu Date: Wed, 22 Jul 2020 22:34:46 +0000 Subject: [PATCH 1/9] Handle defaults in base class only --- .../opentelemetry/exporters/otlp/recordable.h | 16 +++---- exporters/otlp/test/recordable_test.cc | 2 +- .../opentelemetry/sdk/trace/recordable.h | 43 +++++++++++++++---- .../opentelemetry/sdk/trace/span_data.h | 14 +++--- sdk/test/trace/span_data_test.cc | 2 +- 5 files changed, 47 insertions(+), 30 deletions(-) diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h b/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h index 57519bad01..5dc4de02f0 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/recordable.h @@ -21,16 +21,12 @@ class Recordable final : public sdk::trace::Recordable void SetAttribute(nostd::string_view key, const opentelemetry::common::AttributeValue &value) noexcept override; - void AddEvent( - nostd::string_view name, - core::SystemTimestamp timestamp = core::SystemTimestamp(std::chrono::system_clock::now()), - const trace::KeyValueIterable &attributes = - trace::KeyValueIterableView>({})) noexcept override; - - void AddLink( - opentelemetry::trace::SpanContext span_context, - const trace::KeyValueIterable &attributes = - trace::KeyValueIterableView>({})) noexcept override; + void AddEvent(nostd::string_view name, + core::SystemTimestamp timestamp, + const trace::KeyValueIterable &attributes) noexcept override; + + void AddLink(opentelemetry::trace::SpanContext span_context, + const trace::KeyValueIterable &attributes) noexcept override; void SetStatus(trace::CanonicalCode code, nostd::string_view description) noexcept override; diff --git a/exporters/otlp/test/recordable_test.cc b/exporters/otlp/test/recordable_test.cc index ec1662662d..fe54491791 100644 --- a/exporters/otlp/test/recordable_test.cc +++ b/exporters/otlp/test/recordable_test.cc @@ -86,7 +86,7 @@ TEST(Recordable, AddEvents) std::chrono::system_clock::time_point event_time = std::chrono::system_clock::now(); core::SystemTimestamp event_timestamp(event_time); - rec.AddEvent(name, event_timestamp); + rec.sdk::trace::Recordable::AddEvent(name, event_timestamp); uint64_t unix_event_time = std::chrono::duration_cast(event_time.time_since_epoch()).count(); diff --git a/sdk/include/opentelemetry/sdk/trace/recordable.h b/sdk/include/opentelemetry/sdk/trace/recordable.h index 489bfd5a03..9e1cab751d 100644 --- a/sdk/include/opentelemetry/sdk/trace/recordable.h +++ b/sdk/include/opentelemetry/sdk/trace/recordable.h @@ -53,21 +53,46 @@ class Recordable * @param timestamp the timestamp of the event * @param attributes the attributes associated with the event */ - virtual 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 = 0; + virtual void AddEvent(nostd::string_view name, + core::SystemTimestamp timestamp, + const trace_api::KeyValueIterable &attributes) noexcept = 0; + + /** + * Add an event to a span with default timestamp and attributes. + * @param name the name of the event + */ + void AddEvent(nostd::string_view name) + { + AddEvent(name, core::SystemTimestamp(std::chrono::system_clock::now()), + trace_api::KeyValueIterableView>({})); + } + + /** + * Add an event to a span with default (empty) attributes. + * @param name the name of the event + * @param name the timestamp of the event + */ + void AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) + { + AddEvent(name, timestamp, trace_api::KeyValueIterableView>({})); + } /** * Add a link to a span. * @param span_context the span context of the linked span * @param attributes the attributes associated with the link */ - virtual void AddLink( - opentelemetry::trace::SpanContext span_context, - const trace_api::KeyValueIterable &attributes = - trace_api::KeyValueIterableView>({})) noexcept = 0; + virtual void AddLink(opentelemetry::trace::SpanContext span_context, + const trace_api::KeyValueIterable &attributes) noexcept = 0; + + /** + * Add a link to a span with default (empty) attributes. + * @param span_context the span context of the linked span + */ + void AddLink(opentelemetry::trace::SpanContext span_context) + { + AddLink(span_context, trace_api::KeyValueIterableView>({})); + } /** * Set the status of the span. diff --git a/sdk/include/opentelemetry/sdk/trace/span_data.h b/sdk/include/opentelemetry/sdk/trace/span_data.h index 47477fcaf7..99a33f957b 100644 --- a/sdk/include/opentelemetry/sdk/trace/span_data.h +++ b/sdk/include/opentelemetry/sdk/trace/span_data.h @@ -194,20 +194,16 @@ class SpanData final : public Recordable attributes_[std::string(key)] = nostd::visit(converter_, value); } - 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 + void AddEvent(nostd::string_view name, + core::SystemTimestamp timestamp, + const trace_api::KeyValueIterable &attributes) noexcept override { events_.push_back(SpanDataEvent(std::string(name), timestamp)); // TODO: handle attributes } - void AddLink( - opentelemetry::trace::SpanContext span_context, - const trace_api::KeyValueIterable &attributes = - trace_api::KeyValueIterableView>({})) noexcept override + void AddLink(opentelemetry::trace::SpanContext span_context, + const trace_api::KeyValueIterable &attributes) noexcept override { (void)span_context; (void)attributes; diff --git a/sdk/test/trace/span_data_test.cc b/sdk/test/trace/span_data_test.cc index 41d7b2c670..81189991a0 100644 --- a/sdk/test/trace/span_data_test.cc +++ b/sdk/test/trace/span_data_test.cc @@ -39,7 +39,7 @@ TEST(SpanData, Set) data.SetStartTime(now); data.SetDuration(std::chrono::nanoseconds(1000000)); data.SetAttribute("attr1", 314159); - data.AddEvent("event1", now); + data.opentelemetry::sdk::trace::Recordable::AddEvent("event1", now); ASSERT_EQ(data.GetTraceId(), trace_id); ASSERT_EQ(data.GetSpanId(), span_id); From eff601c9da8304b7d60c5d8fc1104896ade3bfe5 Mon Sep 17 00:00:00 2001 From: Nadia Ciobanu Date: Thu, 23 Jul 2020 00:10:20 +0000 Subject: [PATCH 2/9] Fix typo --- sdk/include/opentelemetry/sdk/trace/recordable.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/include/opentelemetry/sdk/trace/recordable.h b/sdk/include/opentelemetry/sdk/trace/recordable.h index 9e1cab751d..8a393579bc 100644 --- a/sdk/include/opentelemetry/sdk/trace/recordable.h +++ b/sdk/include/opentelemetry/sdk/trace/recordable.h @@ -70,7 +70,7 @@ class Recordable /** * Add an event to a span with default (empty) attributes. * @param name the name of the event - * @param name the timestamp of the event + * @param timestamp the timestamp of the event */ void AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) { From e4dc39d31bc121e9504d43747c8122926e1da913 Mon Sep 17 00:00:00 2001 From: Nadia Ciobanu Date: Thu, 23 Jul 2020 18:20:07 +0000 Subject: [PATCH 3/9] Add default attributes map --- exporters/otlp/test/recordable_test.cc | 2 ++ .../opentelemetry/sdk/trace/recordable.h | 20 ++++++++++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/exporters/otlp/test/recordable_test.cc b/exporters/otlp/test/recordable_test.cc index fe54491791..7c679cec2e 100644 --- a/exporters/otlp/test/recordable_test.cc +++ b/exporters/otlp/test/recordable_test.cc @@ -93,6 +93,8 @@ TEST(Recordable, AddEvents) EXPECT_EQ(rec.span().events(0).name(), name); EXPECT_EQ(rec.span().events(0).time_unix_nano(), unix_event_time); + // Test default attributes + EXPECT_EQ(rec.span().events(0).attributes_size(), 0); } // Test non-int single types. Int single types are tested using templates (see IntAttributeTest) diff --git a/sdk/include/opentelemetry/sdk/trace/recordable.h b/sdk/include/opentelemetry/sdk/trace/recordable.h index 8a393579bc..3b55896b9f 100644 --- a/sdk/include/opentelemetry/sdk/trace/recordable.h +++ b/sdk/include/opentelemetry/sdk/trace/recordable.h @@ -2,6 +2,7 @@ #include "opentelemetry/common/attribute_value.h" #include "opentelemetry/core/timestamp.h" +#include "opentelemetry/nostd/shared_ptr.h" #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/trace/canonical_code.h" #include "opentelemetry/trace/key_value_iterable.h" @@ -64,7 +65,7 @@ class Recordable void AddEvent(nostd::string_view name) { AddEvent(name, core::SystemTimestamp(std::chrono::system_clock::now()), - trace_api::KeyValueIterableView>({})); + *GetDefaultAttributes()); } /** @@ -74,7 +75,7 @@ class Recordable */ void AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) { - AddEvent(name, timestamp, trace_api::KeyValueIterableView>({})); + AddEvent(name, timestamp, *GetDefaultAttributes()); } /** @@ -91,7 +92,7 @@ class Recordable */ void AddLink(opentelemetry::trace::SpanContext span_context) { - AddLink(span_context, trace_api::KeyValueIterableView>({})); + AddLink(span_context, *GetDefaultAttributes()); } /** @@ -119,6 +120,19 @@ class Recordable * @param duration the duration to set */ virtual void SetDuration(std::chrono::nanoseconds duration) noexcept = 0; + +private: + /** + * Maintain a static empty map that represents default attributes. + * Avoid constructing a new empty map everytime a call is made with default attributes. + */ + static const nostd::shared_ptr>> + &GetDefaultAttributes() noexcept + { + static const nostd::shared_ptr>> + kDefaultAttributes({}); + return kDefaultAttributes; + } }; } // namespace trace } // namespace sdk From b5a1aba5f2c6af9567df55cb52d3763b153453a1 Mon Sep 17 00:00:00 2001 From: Nadia Ciobanu Date: Thu, 23 Jul 2020 18:26:03 +0000 Subject: [PATCH 4/9] Remove unnecessary check --- exporters/otlp/test/recordable_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/exporters/otlp/test/recordable_test.cc b/exporters/otlp/test/recordable_test.cc index 7c679cec2e..fe54491791 100644 --- a/exporters/otlp/test/recordable_test.cc +++ b/exporters/otlp/test/recordable_test.cc @@ -93,8 +93,6 @@ TEST(Recordable, AddEvents) EXPECT_EQ(rec.span().events(0).name(), name); EXPECT_EQ(rec.span().events(0).time_unix_nano(), unix_event_time); - // Test default attributes - EXPECT_EQ(rec.span().events(0).attributes_size(), 0); } // Test non-int single types. Int single types are tested using templates (see IntAttributeTest) From 720a3eee98b3a5aa3a4933657d0f9242dbbdabeb Mon Sep 17 00:00:00 2001 From: Nadia Ciobanu Date: Thu, 23 Jul 2020 22:06:15 +0000 Subject: [PATCH 5/9] Address review comments, add common GetEmptyAttributes --- exporters/otlp/test/recordable_test.cc | 1 + .../sdk/common/empty_attributes.h | 21 ++++++++++++++++++ .../opentelemetry/sdk/trace/recordable.h | 22 ++++--------------- sdk/test/common/BUILD | 12 ++++++++++ sdk/test/common/empty_attributes_test.cc | 8 +++++++ 5 files changed, 46 insertions(+), 18 deletions(-) create mode 100644 sdk/include/opentelemetry/sdk/common/empty_attributes.h create mode 100644 sdk/test/common/empty_attributes_test.cc diff --git a/exporters/otlp/test/recordable_test.cc b/exporters/otlp/test/recordable_test.cc index fe54491791..bb067222b3 100644 --- a/exporters/otlp/test/recordable_test.cc +++ b/exporters/otlp/test/recordable_test.cc @@ -93,6 +93,7 @@ TEST(Recordable, AddEvents) EXPECT_EQ(rec.span().events(0).name(), name); EXPECT_EQ(rec.span().events(0).time_unix_nano(), unix_event_time); + EXPECT_EQ(rec.span().events(0).attributes().size(), 0); } // Test non-int single types. Int single types are tested using templates (see IntAttributeTest) diff --git a/sdk/include/opentelemetry/sdk/common/empty_attributes.h b/sdk/include/opentelemetry/sdk/common/empty_attributes.h new file mode 100644 index 0000000000..06005f3231 --- /dev/null +++ b/sdk/include/opentelemetry/sdk/common/empty_attributes.h @@ -0,0 +1,21 @@ +#include "opentelemetry/trace/key_value_iterable_view.h" + +#include + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +/** + * Maintain a static empty map that represents empty (default) attributes. + * Helps to avoid constructing a new empty map everytime a call is made with default attributes. + */ +static const opentelemetry::trace::KeyValueIterableView> + &GetEmptyAttributes() noexcept +{ + static const std::map* map = new std::map; + static const opentelemetry::trace::KeyValueIterableView> + kEmptyAttributes(*map); + return kEmptyAttributes; +} +} // namespace sdk +OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/include/opentelemetry/sdk/trace/recordable.h b/sdk/include/opentelemetry/sdk/trace/recordable.h index 3b55896b9f..dcf9a50c7a 100644 --- a/sdk/include/opentelemetry/sdk/trace/recordable.h +++ b/sdk/include/opentelemetry/sdk/trace/recordable.h @@ -2,15 +2,14 @@ #include "opentelemetry/common/attribute_value.h" #include "opentelemetry/core/timestamp.h" -#include "opentelemetry/nostd/shared_ptr.h" #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/trace/canonical_code.h" #include "opentelemetry/trace/key_value_iterable.h" -#include "opentelemetry/trace/key_value_iterable_view.h" #include "opentelemetry/trace/span_context.h" #include "opentelemetry/trace/span_id.h" #include "opentelemetry/trace/trace_id.h" #include "opentelemetry/version.h" +#include "opentelemetry/sdk/common/empty_attributes.h" #include @@ -65,7 +64,7 @@ class Recordable void AddEvent(nostd::string_view name) { AddEvent(name, core::SystemTimestamp(std::chrono::system_clock::now()), - *GetDefaultAttributes()); + opentelemetry::sdk::GetEmptyAttributes()); } /** @@ -75,7 +74,7 @@ class Recordable */ void AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) { - AddEvent(name, timestamp, *GetDefaultAttributes()); + AddEvent(name, timestamp, opentelemetry::sdk::GetEmptyAttributes()); } /** @@ -92,7 +91,7 @@ class Recordable */ void AddLink(opentelemetry::trace::SpanContext span_context) { - AddLink(span_context, *GetDefaultAttributes()); + AddLink(span_context, opentelemetry::sdk::GetEmptyAttributes()); } /** @@ -120,19 +119,6 @@ class Recordable * @param duration the duration to set */ virtual void SetDuration(std::chrono::nanoseconds duration) noexcept = 0; - -private: - /** - * Maintain a static empty map that represents default attributes. - * Avoid constructing a new empty map everytime a call is made with default attributes. - */ - static const nostd::shared_ptr>> - &GetDefaultAttributes() noexcept - { - static const nostd::shared_ptr>> - kDefaultAttributes({}); - return kDefaultAttributes; - } }; } // namespace trace } // namespace sdk diff --git a/sdk/test/common/BUILD b/sdk/test/common/BUILD index a4b778f388..fc7f668948 100644 --- a/sdk/test/common/BUILD +++ b/sdk/test/common/BUILD @@ -79,3 +79,15 @@ otel_cc_benchmark( "//sdk/src/common:circular_buffer", ], ) + +cc_test( + name = "empty_attributes_test", + srcs = [ + "empty_attributes_test.cc", + ], + deps = [ + "//api", + "//sdk:headers", + "@com_google_googletest//:gtest_main", + ], +) diff --git a/sdk/test/common/empty_attributes_test.cc b/sdk/test/common/empty_attributes_test.cc new file mode 100644 index 0000000000..2ca0ec3bac --- /dev/null +++ b/sdk/test/common/empty_attributes_test.cc @@ -0,0 +1,8 @@ +#include "opentelemetry/sdk/common/empty_attributes.h" + +#include + +TEST(EmptyAttributesTest, EmptyTest) +{ + EXPECT_EQ((opentelemetry::sdk::GetEmptyAttributes()).size(), 0); +} From ba14ba66eee7c178a84681626fc9b9bffdf09ca9 Mon Sep 17 00:00:00 2001 From: Nadia Ciobanu Date: Thu, 23 Jul 2020 22:24:31 +0000 Subject: [PATCH 6/9] Remove extra parentheses --- sdk/test/common/empty_attributes_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/test/common/empty_attributes_test.cc b/sdk/test/common/empty_attributes_test.cc index 2ca0ec3bac..1406d8b7a4 100644 --- a/sdk/test/common/empty_attributes_test.cc +++ b/sdk/test/common/empty_attributes_test.cc @@ -4,5 +4,5 @@ TEST(EmptyAttributesTest, EmptyTest) { - EXPECT_EQ((opentelemetry::sdk::GetEmptyAttributes()).size(), 0); + EXPECT_EQ(opentelemetry::sdk::GetEmptyAttributes().size(), 0); } From 153545deb70347e613cdadb1a6d8ffa342cff14f Mon Sep 17 00:00:00 2001 From: Nadia Ciobanu Date: Thu, 23 Jul 2020 23:38:28 +0000 Subject: [PATCH 7/9] Add test, fix bug --- .../opentelemetry/sdk/common/empty_attributes.h | 4 ++-- sdk/test/common/empty_attributes_test.cc | 10 +++++++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/common/empty_attributes.h b/sdk/include/opentelemetry/sdk/common/empty_attributes.h index 06005f3231..11ba05417d 100644 --- a/sdk/include/opentelemetry/sdk/common/empty_attributes.h +++ b/sdk/include/opentelemetry/sdk/common/empty_attributes.h @@ -12,9 +12,9 @@ namespace sdk static const opentelemetry::trace::KeyValueIterableView> &GetEmptyAttributes() noexcept { - static const std::map* map = new std::map; + static const std::map map; static const opentelemetry::trace::KeyValueIterableView> - kEmptyAttributes(*map); + kEmptyAttributes(map); return kEmptyAttributes; } } // namespace sdk diff --git a/sdk/test/common/empty_attributes_test.cc b/sdk/test/common/empty_attributes_test.cc index 1406d8b7a4..8fdae04053 100644 --- a/sdk/test/common/empty_attributes_test.cc +++ b/sdk/test/common/empty_attributes_test.cc @@ -2,7 +2,15 @@ #include -TEST(EmptyAttributesTest, EmptyTest) +TEST(EmptyAttributesTest, TestSize) { EXPECT_EQ(opentelemetry::sdk::GetEmptyAttributes().size(), 0); } + +// Test that GetEmptyAttributes() always returns the same KeyValueIterableView +TEST(EmptyAttributesTest, TestMemory) +{ + auto attributes1 = opentelemetry::sdk::GetEmptyAttributes(); + auto attributes2 = opentelemetry::sdk::GetEmptyAttributes(); + EXPECT_EQ(memcmp(&attributes1, &attributes2, sizeof(attributes1)), 0); +} From 9e4e62c2d38fffd4df8c8d9b56576d338b60ef8d Mon Sep 17 00:00:00 2001 From: Nadia Ciobanu Date: Mon, 27 Jul 2020 19:04:35 +0000 Subject: [PATCH 8/9] Use array instead of map --- .../opentelemetry/sdk/common/empty_attributes.h | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/common/empty_attributes.h b/sdk/include/opentelemetry/sdk/common/empty_attributes.h index 11ba05417d..aaf8386444 100644 --- a/sdk/include/opentelemetry/sdk/common/empty_attributes.h +++ b/sdk/include/opentelemetry/sdk/common/empty_attributes.h @@ -6,15 +6,18 @@ OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk { /** - * Maintain a static empty map that represents empty (default) attributes. - * Helps to avoid constructing a new empty map everytime a call is made with default attributes. + * Maintain a static empty array of pairs that represents empty (default) attributes. + * This helps to avoid constructing a new empty container every time a call is made + * with default attributes. */ -static const opentelemetry::trace::KeyValueIterableView> +static const opentelemetry::trace::KeyValueIterableView, 0>> &GetEmptyAttributes() noexcept { - static const std::map map; - static const opentelemetry::trace::KeyValueIterableView> - kEmptyAttributes(map); + static constexpr std::array, 0> array; + static const opentelemetry::trace::KeyValueIterableView< + std::array, 0>> + kEmptyAttributes(array); + return kEmptyAttributes; } } // namespace sdk From 31e9d1b8d6de10f918269c7b8515351503045d7c Mon Sep 17 00:00:00 2001 From: Nadia Ciobanu Date: Mon, 27 Jul 2020 19:13:18 +0000 Subject: [PATCH 9/9] Change constexpr to const --- sdk/include/opentelemetry/sdk/common/empty_attributes.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/include/opentelemetry/sdk/common/empty_attributes.h b/sdk/include/opentelemetry/sdk/common/empty_attributes.h index aaf8386444..dd8db493ff 100644 --- a/sdk/include/opentelemetry/sdk/common/empty_attributes.h +++ b/sdk/include/opentelemetry/sdk/common/empty_attributes.h @@ -13,7 +13,7 @@ namespace sdk static const opentelemetry::trace::KeyValueIterableView, 0>> &GetEmptyAttributes() noexcept { - static constexpr std::array, 0> array; + static const std::array, 0> array; static const opentelemetry::trace::KeyValueIterableView< std::array, 0>> kEmptyAttributes(array);