Additional tests and cleanup

open-telemetry · Jan 9, 2023 · 32ba915 · 32ba915
1 parent 5d15422
commit 32ba915
Show file tree

Hide file tree

Showing 6 changed files with 73 additions and 44 deletions.
diff --git a/opentracing-shim/src/span_shim.cc b/opentracing-shim/src/span_shim.cc
@@ -24,13 +24,13 @@ void SpanShim::handleError(const opentracing::Value& value) noexcept
   // - false maps to Ok
   // - no value being set maps to Unset.
   auto code = StatusCode::kUnset;
-  const auto& str_value = utils::stringFromValue(value);
+  const auto& error_tag = utils::stringFromValue(value);
 
-  if (str_value == "true")
+  if (error_tag == "true")
   {
     code = StatusCode::kError;
   }
-  else if (str_value == "false")
+  else if (error_tag == "false")
   {
     code = StatusCode::kOk;
   }
@@ -68,7 +68,7 @@ void SpanShim::SetBaggageItem(opentracing::string_view restricted_key, opentraci
   // Creates a new SpanContext Shim with a new OpenTelemetry Baggage containing the specified
   // Baggage key/value pair, and sets it as the current instance for this Span Shim.
   if (restricted_key.empty() || value.empty()) return;
-
+  // This operation MUST be safe to be called concurrently.
   const std::lock_guard<decltype(context_lock_)> guard(context_lock_);
   context_ = context_.newWithKeyValue(restricted_key.data(), value.data());
 }
@@ -78,7 +78,7 @@ std::string SpanShim::BaggageItem(opentracing::string_view restricted_key) const
   // Returns the value for the specified key in the OpenTelemetry Baggage
   // of the current SpanContext Shim, or null if none exists.
   if (restricted_key.empty()) return "";
-
+  // This operation MUST be safe to be called concurrently.
   const std::lock_guard<decltype(context_lock_)> guard(context_lock_);
   std::string value;
   return context_.BaggageItem(restricted_key.data(), value) ? value : "";

diff --git a/opentracing-shim/test/propagation_test.cc b/opentracing-shim/test/propagation_test.cc
@@ -14,19 +14,7 @@ namespace common  = opentelemetry::common;
 namespace nostd   = opentelemetry::nostd;
 namespace shim    = opentelemetry::opentracingshim;
 
-class PropagationTest : public testing::Test
-{
-protected:
-  virtual void SetUp()
-  {
-  }
-
-  virtual void TearDown()
-  {
-  }
-};
-
-TEST_F(PropagationTest, TextMapReader_Get_LookupKey_Unsupported)
+TEST(PropagationTest, TextMapReader_Get_LookupKey_Unsupported)
 {
   std::unordered_map<std::string, std::string> text_map;
   TextMapCarrier testee{text_map};
@@ -48,7 +36,7 @@ TEST_F(PropagationTest, TextMapReader_Get_LookupKey_Unsupported)
   ASSERT_EQ(testee.foreach_key_call_count, 2);
 }
 
-TEST_F(PropagationTest, TextMapReader_Get_LookupKey_Supported)
+TEST(PropagationTest, TextMapReader_Get_LookupKey_Supported)
 {
   std::unordered_map<std::string, std::string> text_map;
   TextMapCarrier testee{text_map};
@@ -71,7 +59,7 @@ TEST_F(PropagationTest, TextMapReader_Get_LookupKey_Supported)
   ASSERT_EQ(testee.foreach_key_call_count, 1);
 }
 
-TEST_F(PropagationTest, TextMapReader_Keys)
+TEST(PropagationTest, TextMapReader_Keys)
 {
   std::unordered_map<std::string, std::string> text_map;
   TextMapCarrier testee{text_map};
@@ -98,7 +86,7 @@ TEST_F(PropagationTest, TextMapReader_Keys)
   ASSERT_EQ(testee.foreach_key_call_count, 2);
 }
 
-TEST_F(PropagationTest, TextMapWriter_Set)
+TEST(PropagationTest, TextMapWriter_Set)
 {
   std::unordered_map<std::string, std::string> text_map;
   TextMapCarrier testee{text_map};

diff --git a/opentracing-shim/test/shim_mocks.h b/opentracing-shim/test/shim_mocks.h
@@ -11,6 +11,7 @@
 #include "opentelemetry/trace/span_context.h"
 #include "opentelemetry/trace/span_metadata.h"
 #include "opentelemetry/trace/tracer.h"
+#include "opentelemetry/trace/tracer_provider.h"
 #include "opentracing/propagation.h"
 
 #include <unordered_map>
@@ -22,6 +23,19 @@ namespace common    = opentelemetry::common;
 namespace context   = opentelemetry::context;
 namespace nostd     = opentelemetry::nostd;
 
+struct MockTracerProvider : public trace_api::TracerProvider
+{
+  nostd::shared_ptr<trace_api::Tracer> GetTracer(nostd::string_view library_name,
+                                                 nostd::string_view,
+                                                 nostd::string_view) noexcept override
+  {
+    library_name_ = std::string{library_name};
+    return nostd::shared_ptr<opentelemetry::trace::Tracer>();
+  }
+
+  std::string library_name_;
+};
+
 struct MockSpan final : public trace_api::Span
 {
   void SetAttribute(nostd::string_view key,

diff --git a/opentracing-shim/test/shim_utils_test.cc b/opentracing-shim/test/shim_utils_test.cc
@@ -17,19 +17,7 @@ namespace common    = opentelemetry::common;
 namespace nostd     = opentelemetry::nostd;
 namespace shim      = opentelemetry::opentracingshim;
 
-class ShimUtilsTest : public testing::Test
-{
-protected:
-  virtual void SetUp()
-  {
-  }
-
-  virtual void TearDown()
-  {
-  }
-};
-
-TEST_F(ShimUtilsTest, IsBaggageEmpty)
+TEST(ShimUtilsTest, IsBaggageEmpty)
 {
   auto none = nostd::shared_ptr<baggage::Baggage>(nullptr);
   ASSERT_TRUE(shim::utils::isBaggageEmpty(none));
@@ -42,7 +30,7 @@ TEST_F(ShimUtilsTest, IsBaggageEmpty)
   ASSERT_FALSE(shim::utils::isBaggageEmpty(non_empty));
 }
 
-TEST_F(ShimUtilsTest, StringFromValue)
+TEST(ShimUtilsTest, StringFromValue)
 {
   ASSERT_EQ(shim::utils::stringFromValue(true), "true");
   ASSERT_EQ(shim::utils::stringFromValue(false), "false");
@@ -61,7 +49,7 @@ TEST_F(ShimUtilsTest, StringFromValue)
   ASSERT_EQ(shim::utils::stringFromValue(dict.get()), "");
 }
 
-TEST_F(ShimUtilsTest, AttributeFromValue)
+TEST(ShimUtilsTest, AttributeFromValue)
 {
   auto value = shim::utils::attributeFromValue(true);
   ASSERT_EQ(value.index(), common::AttributeType::kTypeBool);
@@ -110,7 +98,7 @@ TEST_F(ShimUtilsTest, AttributeFromValue)
   ASSERT_EQ(nostd::get<nostd::string_view>(value), nostd::string_view{});
 }
 
-TEST_F(ShimUtilsTest, MakeOptionsShim_EmptyRefs)
+TEST(ShimUtilsTest, MakeOptionsShim_EmptyRefs)
 {
   auto span_context_shim = nostd::shared_ptr<shim::SpanContextShim>(new shim::SpanContextShim(
     trace_api::SpanContext::GetInvalid(), baggage::Baggage::GetDefault()));
@@ -126,7 +114,7 @@ TEST_F(ShimUtilsTest, MakeOptionsShim_EmptyRefs)
   ASSERT_EQ(nostd::get<trace_api::SpanContext>(options_shim.parent), trace_api::SpanContext::GetInvalid());
 }
 
-TEST_F(ShimUtilsTest, MakeOptionsShim_InvalidSpanContext)
+TEST(ShimUtilsTest, MakeOptionsShim_InvalidSpanContext)
 {
   auto span_context_shim = nostd::shared_ptr<shim::SpanContextShim>(new shim::SpanContextShim(
     trace_api::SpanContext::GetInvalid(), baggage::Baggage::GetDefault()));
@@ -143,7 +131,7 @@ TEST_F(ShimUtilsTest, MakeOptionsShim_InvalidSpanContext)
   ASSERT_EQ(nostd::get<trace_api::SpanContext>(options_shim.parent), trace_api::SpanContext::GetInvalid());
 }
 
-TEST_F(ShimUtilsTest, MakeOptionsShim_FirstChildOf)
+TEST(ShimUtilsTest, MakeOptionsShim_FirstChildOf)
 {
   auto span_context_shim = nostd::shared_ptr<shim::SpanContextShim>(new shim::SpanContextShim(
     trace_api::SpanContext::GetInvalid(), baggage::Baggage::GetDefault()));
@@ -164,7 +152,7 @@ TEST_F(ShimUtilsTest, MakeOptionsShim_FirstChildOf)
   ASSERT_EQ(nostd::get<trace_api::SpanContext>(options_shim.parent), span_context_shim->context());
 }
 
-TEST_F(ShimUtilsTest, MakeOptionsShim_FirstInList)
+TEST(ShimUtilsTest, MakeOptionsShim_FirstInList)
 {
   auto span_context_shim = nostd::shared_ptr<shim::SpanContextShim>(new shim::SpanContextShim(
     trace_api::SpanContext::GetInvalid(), baggage::Baggage::GetDefault()));
@@ -184,7 +172,7 @@ TEST_F(ShimUtilsTest, MakeOptionsShim_FirstInList)
   ASSERT_EQ(nostd::get<trace_api::SpanContext>(options_shim.parent), span_context_shim->context());
 }
 
-TEST_F(ShimUtilsTest, MakeIterableLinks)
+TEST(ShimUtilsTest, MakeIterableLinks)
 {
   auto span_context_shim1 = nostd::shared_ptr<shim::SpanContextShim>(new shim::SpanContextShim(
     trace_api::SpanContext::GetInvalid(), baggage::Baggage::GetDefault()));
@@ -228,7 +216,7 @@ TEST_F(ShimUtilsTest, MakeIterableLinks)
   ASSERT_EQ(nostd::get<nostd::string_view>(value), "child_of");
 }
 
-TEST_F(ShimUtilsTest, MakeBaggage_EmptyRefs)
+TEST(ShimUtilsTest, MakeBaggage_EmptyRefs)
 {
   auto baggage = baggage::Baggage::GetDefault()->Set("foo", "bar");
   std::string value;
@@ -246,7 +234,7 @@ TEST_F(ShimUtilsTest, MakeBaggage_EmptyRefs)
   ASSERT_EQ(value, "bar");
 }
 
-TEST_F(ShimUtilsTest, MakeBaggage_NonEmptyRefs)
+TEST(ShimUtilsTest, MakeBaggage_NonEmptyRefs)
 {
   auto span_context_shim1 = nostd::shared_ptr<shim::SpanContextShim>(new shim::SpanContextShim(
     trace_api::SpanContext::GetInvalid(), 
@@ -273,7 +261,7 @@ TEST_F(ShimUtilsTest, MakeBaggage_NonEmptyRefs)
   ASSERT_EQ(value, "world");
 }
 
-TEST_F(ShimUtilsTest, MakeIterableTags)
+TEST(ShimUtilsTest, MakeIterableTags)
 {
   opentracing::StartSpanOptions options;
   auto empty = shim::utils::makeIterableTags(options);

diff --git a/opentracing-shim/test/span_shim_test.cc b/opentracing-shim/test/span_shim_test.cc
@@ -107,6 +107,37 @@ TEST_F(SpanShimTest, SetBaggageItem)
   ASSERT_EQ(span_shim->BaggageItem("no value"), "");
 }
 
+TEST_F(SpanShimTest, SetBaggageItem_MultiThreaded)
+{
+  auto span = nostd::shared_ptr<trace_api::Span>(new MockSpan());
+  auto tracer = shim::TracerShim::createTracerShim();
+  auto tracer_shim = dynamic_cast<shim::TracerShim*>(tracer.get());
+  auto baggage = baggage::Baggage::GetDefault();
+  shim::SpanShim span_shim(*tracer_shim, span, baggage);
+
+  std::vector<std::thread> threads;
+  std::vector<std::string> keys;
+  std::vector<std::string> values;
+  int thread_count = 100;
+
+  for (int index = 0; index < thread_count; ++index) 
+  {
+    keys.emplace_back("key-" + std::to_string(index));
+    values.emplace_back("value-" + std::to_string(index));
+    threads.emplace_back(std::bind(&shim::SpanShim::SetBaggageItem, &span_shim, keys[index], values[index]));
+  }
+
+  for (auto& thread : threads)
+  {
+    thread.join();
+  }
+
+  for (int index = 0; index < thread_count; ++index) 
+  {
+    ASSERT_EQ(span_shim.BaggageItem(keys[index]), values[index]);
+  }
+}
+
 TEST_F(SpanShimTest, Log_NoEvent)
 {
   std::string name;

diff --git a/opentracing-shim/test/tracer_shim_test.cc b/opentracing-shim/test/tracer_shim_test.cc
@@ -47,6 +47,14 @@ class TracerShimTest : public testing::Test
   }
 };
 
+TEST_F(TracerShimTest, TracerProviderName)
+{
+  auto mock_provider_ptr = new MockTracerProvider();
+  nostd::shared_ptr<trace_api::TracerProvider> provider(mock_provider_ptr);
+  ASSERT_NE(shim::TracerShim::createTracerShim(provider), nullptr);
+  ASSERT_EQ(mock_provider_ptr->library_name_, "opentracing-shim");
+}
+
 TEST_F(TracerShimTest, SpanReferenceToCreatingTracer)
 {
   auto span_shim = tracer_shim->StartSpan("a");