Reduce copying for StartSpan

open-telemetry · Jan 5, 2023 · 25aeac8 · 25aeac8
1 parent f62eb78
commit 25aeac8
Show file tree

Hide file tree

Showing 5 changed files with 124 additions and 61 deletions.
diff --git a/opentracing-shim/include/shim_utils.h b/opentracing-shim/include/shim_utils.h
@@ -10,9 +10,11 @@
 #include "opentelemetry/baggage/baggage.h"
 #include "opentelemetry/baggage/baggage_context.h"
 #include "opentelemetry/common/attribute_value.h"
+#include "opentelemetry/common/key_value_iterable.h"
 #include "opentelemetry/context/propagation/text_map_propagator.h"
 #include "opentelemetry/nostd/type_traits.h"
 #include "opentelemetry/trace/semantic_conventions.h"
+#include "opentelemetry/trace/span_context_kv_iterable.h"
 #include "opentelemetry/trace/tracer.h"
 #include "opentracing/propagation.h"
 #include "opentracing/tracer.h"
@@ -106,41 +108,80 @@ static opentelemetry::trace::StartSpanOptions makeOptionsShim(const opentracing:
   return options_shim;
 }
 
-using RefsList = std::vector<std::pair<nostd::string_view, common::AttributeValue>>;
-using LinksList = std::vector<std::pair<opentelemetry::trace::SpanContext, RefsList>>;
-
-static LinksList makeReferenceLinks(const opentracing::StartSpanOptions& options) noexcept
+class LinksIterable final : public opentelemetry::trace::SpanContextKeyValueIterable
 {
-  using opentracing::SpanReferenceType;
-  using namespace opentelemetry::trace::SemanticConventions;
-
-  LinksList links;
-  links.reserve(options.references.size());
-  // All values in the list MUST be added as Links with the reference type value
-  // as a Link attribute, i.e. opentracing.ref_type set to follows_from or child_of
-  for (const auto& entry : options.references)
+public:
+  using RefsList = std::vector<std::pair<opentracing::SpanReferenceType, const opentracing::SpanContext*>>;
+  explicit LinksIterable(const RefsList& refs) noexcept : refs_(refs) {}
+
+  bool ForEachKeyValue(nostd::function_ref<bool(opentelemetry::trace::SpanContext, 
+                                                const opentelemetry::common::KeyValueIterable&)>
+                                                callback) const noexcept override
   {
-    auto context_shim = dynamic_cast<const SpanContextShim*>(entry.second);
-    nostd::string_view span_kind;
-
-    if (entry.first == SpanReferenceType::ChildOfRef)
-    {
-      span_kind = OpentracingRefTypeValues::kChildOf;
-    }
-    else if (entry.first == SpanReferenceType::FollowsFromRef)
+    using opentracing::SpanReferenceType;
+    using namespace opentelemetry::trace::SemanticConventions;
+    using LinksList = std::initializer_list<std::pair<nostd::string_view, common::AttributeValue>>;
+
+    for (const auto& entry : refs_)
     {
-      span_kind = OpentracingRefTypeValues::kFollowsFrom;
+      auto context_shim = dynamic_cast<const SpanContextShim*>(entry.second);
+      nostd::string_view span_kind;
+
+      if (entry.first == SpanReferenceType::ChildOfRef)
+      {
+        span_kind = OpentracingRefTypeValues::kChildOf;
+      }
+      else if (entry.first == SpanReferenceType::FollowsFromRef)
+      {
+        span_kind = OpentracingRefTypeValues::kFollowsFrom;
+      }
+
+      if (context_shim && !span_kind.empty())
+      {
+        if (!callback(context_shim->context(), 
+                      opentelemetry::common::KeyValueIterableView<LinksList>(
+                        {{ kOpentracingRefType, span_kind }})))
+          return false;
+      }
     }
 
-    if (context_shim && !span_kind.empty())
+    return true;
+  }
+
+  size_t size() const noexcept { return refs_.size(); }
+
+private:
+  const RefsList& refs_;
+};
+
+static const LinksIterable makeIterableLinks(const opentracing::StartSpanOptions& options) noexcept
+{
+  return LinksIterable(options.references);
+}
+
+class TagsIterable final : public opentelemetry::common::KeyValueIterable
+{
+public:
+  explicit TagsIterable(const std::vector<std::pair<std::string, opentracing::Value>>& tags) noexcept : tags_(tags) {}
+
+  bool ForEachKeyValue(nostd::function_ref<bool(nostd::string_view, common::AttributeValue)> callback) const noexcept override
+  {
+    for (const auto& entry : tags_)
     {
-      links.emplace_back(std::piecewise_construct,
-                         std::forward_as_tuple(context_shim->context()),
-                         std::forward_as_tuple(std::forward<RefsList>({{ kOpentracingRefType, span_kind }})));
+      if (!callback(entry.first, utils::attributeFromValue(entry.second))) return false;
     }
+    return true;
   }
 
-  return links;
+  size_t size() const noexcept override { return tags_.size(); }
+
+private:
+  const std::vector<std::pair<std::string, opentracing::Value>>& tags_;
+};
+
+static const TagsIterable makeIterableTags(const opentracing::StartSpanOptions& options) noexcept
+{
+  return TagsIterable(options.tags);
 }
 
 static nostd::shared_ptr<opentelemetry::baggage::Baggage> makeBaggage(const opentracing::StartSpanOptions& options) noexcept
@@ -171,20 +212,5 @@ static nostd::shared_ptr<opentelemetry::baggage::Baggage> makeBaggage(const open
     : nostd::shared_ptr<Baggage>(new Baggage(baggage_items));
 }
 
-static std::vector<std::pair<std::string, common::AttributeValue>> makeTags(const opentracing::StartSpanOptions& options) noexcept
-{
-  std::vector<std::pair<std::string, common::AttributeValue>> tags;
-  tags.reserve(options.tags.size());
-
-  // If an initial set of tags is specified, the values MUST
-  // be set at the creation time of the OpenTelemetry Span.
-  for (const auto& entry : options.tags)
-  {
-    tags.emplace_back(entry.first, utils::attributeFromValue(entry.second));
-  }
-
-  return tags;
-}
-
 } // namespace opentracingshim::utils
 OPENTELEMETRY_END_NAMESPACE
diff --git a/opentracing-shim/src/tracer_shim.cc b/opentracing-shim/src/tracer_shim.cc
@@ -22,9 +22,9 @@ std::unique_ptr<opentracing::Span> TracerShim::StartSpanWithOptions(opentracing:
   if (is_closed_) return nullptr;
 
   const auto& opts = utils::makeOptionsShim(options);
-  const auto& links = utils::makeReferenceLinks(options);
+  const auto& links = utils::makeIterableLinks(options);
+  const auto& attributes = utils::makeIterableTags(options);
   const auto& baggage = utils::makeBaggage(options);
-  const auto& attributes = utils::makeTags(options);
   auto span = tracer_->StartSpan(operation_name.data(), attributes, links, opts);
   auto span_shim = new SpanShim(*this, span, baggage);
 

diff --git a/opentracing-shim/test/shim_utils_test.cc b/opentracing-shim/test/shim_utils_test.cc
@@ -183,7 +183,7 @@ TEST_F(ShimUtilsTest, MakeOptionsShim_FirstInList)
   ASSERT_EQ(nostd::get<trace_api::SpanContext>(options_shim.parent), span_context_shim->context());
 }
 
-TEST_F(ShimUtilsTest, MakeReferenceLinks)
+TEST_F(ShimUtilsTest, MakeIterableLinks)
 {
   auto span_context_shim1 = nostd::shared_ptr<shim::SpanContextShim>(new shim::SpanContextShim(
     trace_api::SpanContext::GetInvalid(), baggage::Baggage::GetDefault()));
@@ -193,24 +193,38 @@ TEST_F(ShimUtilsTest, MakeReferenceLinks)
   auto span_context2 = static_cast<opentracing::SpanContext*>(span_context_shim2.get());
 
   opentracing::StartSpanOptions options;
-  auto links = shim::utils::makeReferenceLinks(options);
-  ASSERT_TRUE(links.empty());
+  auto empty = shim::utils::makeIterableLinks(options);
+  ASSERT_EQ(empty.size(), 0);
 
   options.references = {
     { opentracing::SpanReferenceType::FollowsFromRef, nullptr },
     { opentracing::SpanReferenceType::FollowsFromRef, span_context1 },
     { opentracing::SpanReferenceType::ChildOfRef, span_context2 }
   };
-
-  links = shim::utils::makeReferenceLinks(options);
+  auto full = shim::utils::makeIterableLinks(options);
+  ASSERT_EQ(full.size(), 3);
+
+  std::vector<std::tuple<trace_api::SpanContext, nostd::string_view, common::AttributeValue>> links;
+  full.ForEachKeyValue([&links](trace_api::SpanContext ctx, const common::KeyValueIterable& it){
+    it.ForEachKeyValue([&links, &ctx](nostd::string_view key, common::AttributeValue value){
+      links.emplace_back(ctx, key, value);
+      return false;
+    });
+    return true;
+  });
   ASSERT_EQ(links.size(), 2);
-  ASSERT_EQ(links[0].first, span_context_shim1->context());
-  ASSERT_FALSE(links[0].second.empty());
-  ASSERT_EQ(links[0].second[0].first, "opentracing.ref_type");
-  ASSERT_EQ(nostd::get<nostd::string_view>(links[0].second[0].second), "follows_from");
-  ASSERT_EQ(links[1].first, span_context_shim2->context());
-  ASSERT_EQ(links[1].second[0].first, "opentracing.ref_type");
-  ASSERT_EQ(nostd::get<nostd::string_view>(links[1].second[0].second), "child_of");
+
+  trace_api::SpanContext ctx = trace_api::SpanContext::GetInvalid();
+  nostd::string_view key;
+  common::AttributeValue value;
+  std::tie(ctx, key, value) = links[0];
+  ASSERT_EQ(ctx, span_context_shim1->context());
+  ASSERT_EQ(key, "opentracing.ref_type");
+  ASSERT_EQ(nostd::get<nostd::string_view>(value), "follows_from");
+  std::tie(ctx, key, value) = links[1];
+  ASSERT_EQ(ctx, span_context_shim2->context());
+  ASSERT_EQ(key, "opentracing.ref_type");
+  ASSERT_EQ(nostd::get<nostd::string_view>(value), "child_of");
 }
 
 TEST_F(ShimUtilsTest, MakeBaggage_EmptyRefs)
@@ -258,15 +272,21 @@ TEST_F(ShimUtilsTest, MakeBaggage_NonEmptyRefs)
   ASSERT_EQ(value, "world");
 }
 
-TEST_F(ShimUtilsTest, MakeTags)
+TEST_F(ShimUtilsTest, MakeIterableTags)
 {
   opentracing::StartSpanOptions options;
-  auto attributes = shim::utils::makeTags(options);
-  ASSERT_TRUE(attributes.empty());
+  auto empty = shim::utils::makeIterableTags(options);
+  ASSERT_EQ(empty.size(), 0);
 
   options.tags = {{ "foo", 42.0 }, { "bar", true }, { "baz", "test" }};
-  attributes = shim::utils::makeTags(options);
-  ASSERT_EQ(attributes.size(), 3);
+  auto full = shim::utils::makeIterableTags(options);
+  ASSERT_EQ(full.size(), 3);
+
+  std::vector<std::pair<nostd::string_view, common::AttributeValue>> attributes;
+  full.ForEachKeyValue([&attributes](nostd::string_view key, common::AttributeValue value){
+    attributes.push_back({key, value});
+    return true;
+  });
   ASSERT_EQ(attributes[0].first, "foo");
   ASSERT_EQ(nostd::get<double>(attributes[0].second), 42.0);
   ASSERT_EQ(attributes[1].first, "bar");

diff --git a/opentracing-shim/test/span_shim_test.cc b/opentracing-shim/test/span_shim_test.cc
@@ -181,4 +181,4 @@ TEST_F(SpanShimTest, Log_Error)
   ASSERT_EQ(nostd::get<int64_t>(attributes["exception.type"]), 42);
   ASSERT_STREQ(nostd::get<const char *>(attributes["exception.message"]), "hello");
   ASSERT_STREQ(nostd::get<const char *>(attributes["exception.stacktrace"]), "overflow");
-}
+}
diff --git a/opentracing-shim/test/tracer_shim_test.cc b/opentracing-shim/test/tracer_shim_test.cc
@@ -53,6 +53,23 @@ TEST_F(TracerShimTest, SpanReferenceToCreatingTracer)
   ASSERT_EQ(&span_shim->tracer(), tracer_shim.get());
 }
 
+TEST_F(TracerShimTest, SpanParentChildRelationship)
+{
+  auto span_shim1 = tracer_shim->StartSpan("a");
+  auto span_shim2 = tracer_shim->StartSpan("b", { opentracing::ChildOf(&span_shim1->context()) });
+  ASSERT_NE(span_shim1, nullptr);
+  ASSERT_NE(span_shim2, nullptr);
+  ASSERT_NE(span_shim1, span_shim2);
+  ASSERT_EQ(span_shim1->context().ToSpanID(), span_shim2->context().ToSpanID());
+  ASSERT_EQ(span_shim1->context().ToTraceID(), span_shim2->context().ToTraceID());
+
+  auto span_context_shim1 = dynamic_cast<const shim::SpanContextShim*>(&span_shim1->context());
+  auto span_context_shim2 = dynamic_cast<const shim::SpanContextShim*>(&span_shim2->context());
+  ASSERT_TRUE(span_context_shim1 != nullptr);
+  ASSERT_TRUE(span_context_shim2 != nullptr);
+  ASSERT_EQ(span_context_shim1->context(), span_context_shim2->context());
+}
+
 TEST_F(TracerShimTest, TracerGloballyRegistered)
 {
   ASSERT_FALSE(opentracing::Tracer::IsGlobalTracerRegistered());