Remove default parameters from Recordable interface (open-telemetry#208)

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<std::map<std::string, int>>({})) noexcept override;
-
-  void AddLink(
-      opentelemetry::trace::SpanContext span_context,
-      const trace::KeyValueIterable &attributes =
-          trace::KeyValueIterableView<std::map<std::string, int>>({})) 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;
 

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<std::chrono::nanoseconds>(event_time.time_since_epoch()).count();

sdk/include/opentelemetry/sdk/common/empty_attributes.h

@@ -0,0 +1,24 @@
+#include "opentelemetry/trace/key_value_iterable_view.h"
+
+#include <map>
+
+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<std::array<std::pair<std::string, int>, 0>>
+    &GetEmptyAttributes() noexcept
+{
+  static const std::array<std::pair<std::string, int>, 0> array{};
+  static const opentelemetry::trace::KeyValueIterableView<
+      std::array<std::pair<std::string, int>, 0>>
+      kEmptyAttributes(array);
+
+  return kEmptyAttributes;
+}
+}  // namespace sdk
+OPENTELEMETRY_END_NAMESPACE

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 <map>
 
@@ -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<std::map<std::string, int>>({})) 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<std::map<std::string, int>>({})) 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.

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<std::map<std::string, int>>({})) 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<std::map<std::string, int>>({})) noexcept override
+  void AddLink(opentelemetry::trace::SpanContext span_context,
+               const trace_api::KeyValueIterable &attributes) noexcept override
   {
     (void)span_context;
     (void)attributes;

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",
+    ],
+)

sdk/test/common/empty_attributes_test.cc

@@ -0,0 +1,16 @@
+#include "opentelemetry/sdk/common/empty_attributes.h"
+
+#include <gtest/gtest.h>
+
+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);
+}

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);