From e6a6b3b1b9660a5f57524daa50f90144ede6300f Mon Sep 17 00:00:00 2001 From: Nadia Ciobanu Date: Mon, 27 Jul 2020 19:04:38 -0700 Subject: [PATCH] Remove default parameters from Recordable interface (#208) --- .../opentelemetry/exporters/otlp/recordable.h | 16 +++---- exporters/otlp/test/recordable_test.cc | 2 +- .../sdk/common/empty_attributes.h | 24 ++++++++++ .../opentelemetry/sdk/trace/recordable.h | 45 ++++++++++++++----- .../opentelemetry/sdk/trace/span_data.h | 14 +++--- sdk/test/common/BUILD | 12 +++++ sdk/test/common/empty_attributes_test.cc | 16 +++++++ sdk/test/trace/span_data_test.cc | 2 +- 8 files changed, 100 insertions(+), 31 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/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 5a6eec8c3a..62ced4ac6b 100644 --- a/exporters/otlp/test/recordable_test.cc +++ b/exporters/otlp/test/recordable_test.cc @@ -86,7 +86,7 @@ TEST(Recordable, AddEventDefault) 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/common/empty_attributes.h b/sdk/include/opentelemetry/sdk/common/empty_attributes.h new file mode 100644 index 0000000000..b4c69abe7d --- /dev/null +++ b/sdk/include/opentelemetry/sdk/common/empty_attributes.h @@ -0,0 +1,24 @@ +#include "opentelemetry/trace/key_value_iterable_view.h" + +#include + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +/** + * 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, 0>> + &GetEmptyAttributes() noexcept +{ + static const std::array, 0> array{}; + static const opentelemetry::trace::KeyValueIterableView< + std::array, 0>> + kEmptyAttributes(array); + + 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 489bfd5a03..dcf9a50c7a 100644 --- a/sdk/include/opentelemetry/sdk/trace/recordable.h +++ b/sdk/include/opentelemetry/sdk/trace/recordable.h @@ -5,11 +5,11 @@ #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 @@ -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()), + opentelemetry::sdk::GetEmptyAttributes()); + } + + /** + * Add an event to a span with default (empty) attributes. + * @param name the name of the event + * @param timestamp the timestamp of the event + */ + void AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) + { + AddEvent(name, timestamp, opentelemetry::sdk::GetEmptyAttributes()); + } /** * 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, opentelemetry::sdk::GetEmptyAttributes()); + } /** * 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/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..8fdae04053 --- /dev/null +++ b/sdk/test/common/empty_attributes_test.cc @@ -0,0 +1,16 @@ +#include "opentelemetry/sdk/common/empty_attributes.h" + +#include + +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); +} 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);