[SDK] Fix include instrumentation scope attributes in equal method (o…

…pen-telemetry#3214)
lalitb · Dec 18, 2024 · 92bf8da · 92bf8da
1 parent 0b94d71
commit 92bf8da
Show file tree

Hide file tree

Showing 11 changed files with 528 additions and 7 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -18,6 +18,9 @@ Increment the:
 * [SDK] Do not frequently create and destroy http client threads
   [#3198](https://github.com/open-telemetry/opentelemetry-cpp/pull/3198)
 
+* [SDK] Fix instrumentation scope attributes evaluated in equal method
+  [#3214](https://github.com/open-telemetry/opentelemetry-cpp/pull/3214)
+
 ## [1.18 2024-11-25]
 
 * [EXPORTER] Fix crash in ElasticsearchLogRecordExporter

diff --git a/sdk/include/opentelemetry/sdk/common/attribute_utils.h b/sdk/include/opentelemetry/sdk/common/attribute_utils.h
@@ -105,6 +105,60 @@ struct AttributeConverter
   }
 };
 
+/**
+ * Evaluates if an owned value (from an OwnedAttributeValue) is equal to another value (from a
+ * non-owning AttributeValue). This only supports the checking equality with
+ * nostd::visit(AttributeEqualToVisitor, OwnedAttributeValue, AttributeValue).
+ */
+struct AttributeEqualToVisitor
+{
+  // Different types are not equal including containers of different element types
+  template <typename T, typename U>
+  bool operator()(const T &, const U &) const noexcept
+  {
+    return false;
+  }
+
+  // Compare the same arithmetic types
+  template <typename T>
+  bool operator()(const T &owned_value, const T &value) const noexcept
+  {
+    return owned_value == value;
+  }
+
+  // Compare std::string and const char*
+  bool operator()(const std::string &owned_value, const char *value) const noexcept
+  {
+    return owned_value == value;
+  }
+
+  // Compare std::string and nostd::string_view
+  bool operator()(const std::string &owned_value, nostd::string_view value) const noexcept
+  {
+    return owned_value == value;
+  }
+
+  // Compare std::vector<std::string> and nostd::span<const nostd::string_view>
+  bool operator()(const std::vector<std::string> &owned_value,
+                  const nostd::span<const nostd::string_view> &value) const noexcept
+  {
+    return owned_value.size() == value.size() &&
+           std::equal(owned_value.begin(), owned_value.end(), value.begin(),
+                      [](const std::string &owned_element, nostd::string_view element) {
+                        return owned_element == element;
+                      });
+  }
+
+  // Compare nostd::span<const T> and std::vector<T> for arithmetic types
+  template <typename T>
+  bool operator()(const std::vector<T> &owned_value,
+                  const nostd::span<const T> &value) const noexcept
+  {
+    return owned_value.size() == value.size() &&
+           std::equal(owned_value.begin(), owned_value.end(), value.begin());
+  }
+};
+
 /**
  * Class for storing attributes.
  */
@@ -162,8 +216,37 @@ class AttributeMap : public std::unordered_map<std::string, OwnedAttributeValue>
     (*this)[std::string(key)] = nostd::visit(converter_, value);
   }
 
+  // Compare the attributes of this map with another KeyValueIterable
+  bool EqualTo(const opentelemetry::common::KeyValueIterable &attributes) const noexcept
+  {
+    if (attributes.size() != this->size())
+    {
+      return false;
+    }
+
+    const bool is_equal = attributes.ForEachKeyValue(
+        [this](nostd::string_view key,
+               const opentelemetry::common::AttributeValue &value) noexcept {
+          // Perform a linear search to find the key assuming the map is small
+          // This avoids temporary string creation from this->find(std::string(key))
+          for (const auto &kv : *this)
+          {
+            if (kv.first == key)
+            {
+              // Order of arguments is important here. OwnedAttributeValue is first then
+              // AttributeValue AttributeEqualToVisitor does not support the reverse order
+              return nostd::visit(equal_to_visitor_, kv.second, value);
+            }
+          }
+          return false;
+        });
+
+    return is_equal;
+  }
+
 private:
   AttributeConverter converter_;
+  AttributeEqualToVisitor equal_to_visitor_;
 };
 
 /**

diff --git a/sdk/include/opentelemetry/sdk/instrumentationscope/instrumentation_scope.h b/sdk/include/opentelemetry/sdk/instrumentationscope/instrumentation_scope.h
@@ -103,7 +103,8 @@ class InstrumentationScope
    */
   bool operator==(const InstrumentationScope &other) const noexcept
   {
-    return equal(other.name_, other.version_, other.schema_url_);
+    return this->name_ == other.name_ && this->version_ == other.version_ &&
+           this->schema_url_ == other.schema_url_ && this->attributes_ == other.attributes_;
   }
 
   /**
@@ -112,14 +113,31 @@ class InstrumentationScope
    * @param name name of the instrumentation scope to compare.
    * @param version version of the instrumentation scope to compare.
    * @param schema_url schema url of the telemetry emitted by the scope.
+   * @param attributes attributes of the instrumentation scope to compare.
    * @returns true if name and version in this instrumentation scope are equal with the given name
    * and version.
    */
   bool equal(const nostd::string_view name,
              const nostd::string_view version,
-             const nostd::string_view schema_url = "") const noexcept
+             const nostd::string_view schema_url                       = "",
+             const opentelemetry::common::KeyValueIterable *attributes = nullptr) const noexcept
   {
-    return this->name_ == name && this->version_ == version && this->schema_url_ == schema_url;
+
+    if (this->name_ != name || this->version_ != version || this->schema_url_ != schema_url)
+    {
+      return false;
+    }
+
+    if (attributes == nullptr)
+    {
+      if (attributes_.empty())
+      {
+        return true;
+      }
+      return false;
+    }
+
+    return attributes_.EqualTo(*attributes);
   }
 
   const std::string &GetName() const noexcept { return name_; }

diff --git a/sdk/src/logs/logger_provider.cc b/sdk/src/logs/logger_provider.cc
@@ -80,7 +80,7 @@ opentelemetry::nostd::shared_ptr<opentelemetry::logs::Logger> LoggerProvider::Ge
   {
     auto &logger_lib = logger->GetInstrumentationScope();
     if (logger->GetName() == logger_name &&
-        logger_lib.equal(library_name, library_version, schema_url))
+        logger_lib.equal(library_name, library_version, schema_url, &attributes))
     {
       return opentelemetry::nostd::shared_ptr<opentelemetry::logs::Logger>{logger};
     }

diff --git a/sdk/src/metrics/meter_provider.cc b/sdk/src/metrics/meter_provider.cc
@@ -71,7 +71,7 @@ nostd::shared_ptr<metrics_api::Meter> MeterProvider::GetMeter(
   for (auto &meter : context_->GetMeters())
   {
     auto meter_lib = meter->GetInstrumentationScope();
-    if (meter_lib->equal(name, version, schema_url))
+    if (meter_lib->equal(name, version, schema_url, attributes))
     {
       return nostd::shared_ptr<metrics_api::Meter>{meter};
     }

diff --git a/sdk/src/trace/tracer_provider.cc b/sdk/src/trace/tracer_provider.cc
@@ -101,7 +101,7 @@ nostd::shared_ptr<trace_api::Tracer> TracerProvider::GetTracer(
   for (auto &tracer : tracers_)
   {
     auto &tracer_scope = tracer->GetInstrumentationScope();
-    if (tracer_scope.equal(name, version, schema_url))
+    if (tracer_scope.equal(name, version, schema_url, attributes))
     {
       return nostd::shared_ptr<trace_api::Tracer>{tracer};
     }

diff --git a/sdk/test/common/attribute_utils_test.cc b/sdk/test/common/attribute_utils_test.cc
@@ -2,11 +2,14 @@
 // SPDX-License-Identifier: Apache-2.0
 
 #include <gtest/gtest.h>
+#include <array>
 #include <map>
 #include <string>
 #include <unordered_map>
 
 #include "opentelemetry/common/key_value_iterable_view.h"
+#include "opentelemetry/nostd/span.h"
+#include "opentelemetry/nostd/string_view.h"
 #include "opentelemetry/nostd/variant.h"
 #include "opentelemetry/sdk/common/attribute_utils.h"
 
@@ -55,3 +58,132 @@ TEST(OrderedAttributeMapTest, AttributesConstruction)
     EXPECT_EQ(opentelemetry::nostd::get<int>(attribute_map.GetAttributes().at(keys[i])), values[i]);
   }
 }
+
+TEST(AttributeEqualToVisitorTest, AttributeValueEqualTo)
+{
+  namespace sdk   = opentelemetry::sdk::common;
+  namespace api   = opentelemetry::common;
+  namespace nostd = opentelemetry::nostd;
+
+  using AV = api::AttributeValue;
+  using OV = sdk::OwnedAttributeValue;
+
+  sdk::AttributeEqualToVisitor equal_to_visitor;
+
+  // arithmetic types
+  EXPECT_TRUE(opentelemetry::nostd::visit(equal_to_visitor, OV{bool(true)}, AV{bool(true)}));
+  EXPECT_TRUE(opentelemetry::nostd::visit(equal_to_visitor, OV{int32_t(22)}, AV{int32_t(22)}));
+  EXPECT_TRUE(opentelemetry::nostd::visit(equal_to_visitor, OV{int64_t(22)}, AV{int64_t(22)}));
+  EXPECT_TRUE(opentelemetry::nostd::visit(equal_to_visitor, OV{uint32_t(22)}, AV{uint32_t(22)}));
+  EXPECT_TRUE(opentelemetry::nostd::visit(equal_to_visitor, OV{uint64_t(22)}, AV{uint64_t(22)}));
+  EXPECT_TRUE(opentelemetry::nostd::visit(equal_to_visitor, OV{double(22.0)}, AV{double(22.0)}));
+
+  // string types
+  EXPECT_TRUE(opentelemetry::nostd::visit(
+      equal_to_visitor, OV{std::string("string to const char*")}, AV{"string to const char*"}));
+  EXPECT_TRUE(opentelemetry::nostd::visit(equal_to_visitor,
+                                          OV{std::string("string to string_view")},
+                                          AV{nostd::string_view("string to string_view")}));
+
+  // container types
+  EXPECT_TRUE(opentelemetry::nostd::visit(equal_to_visitor, OV{std::vector<bool>{true, false}},
+                                          AV{std::array<const bool, 2>{true, false}}));
+  EXPECT_TRUE(opentelemetry::nostd::visit(equal_to_visitor, OV{std::vector<uint8_t>{33, 44}},
+                                          AV{std::array<const uint8_t, 2>{33, 44}}));
+  EXPECT_TRUE(opentelemetry::nostd::visit(equal_to_visitor, OV{std::vector<int32_t>{33, 44}},
+                                          AV{std::array<const int32_t, 2>{33, 44}}));
+  EXPECT_TRUE(opentelemetry::nostd::visit(equal_to_visitor, OV{std::vector<int64_t>{33, 44}},
+                                          AV{std::array<const int64_t, 2>{33, 44}}));
+  EXPECT_TRUE(opentelemetry::nostd::visit(equal_to_visitor, OV{std::vector<uint32_t>{33, 44}},
+                                          AV{std::array<const uint32_t, 2>{33, 44}}));
+  EXPECT_TRUE(opentelemetry::nostd::visit(equal_to_visitor, OV{std::vector<uint64_t>{33, 44}},
+                                          AV{std::array<const uint64_t, 2>{33, 44}}));
+  EXPECT_TRUE(opentelemetry::nostd::visit(equal_to_visitor, OV{std::vector<double>{33.0, 44.0}},
+                                          AV{std::array<const double, 2>{33.0, 44.0}}));
+  EXPECT_TRUE(opentelemetry::nostd::visit(
+      equal_to_visitor, OV{std::vector<std::string>{"a string", "another string"}},
+      AV{std::array<const nostd::string_view, 2>{"a string", "another string"}}));
+}
+
+TEST(AttributeEqualToVisitorTest, AttributeValueNotEqualTo)
+{
+  namespace sdk   = opentelemetry::sdk::common;
+  namespace api   = opentelemetry::common;
+  namespace nostd = opentelemetry::nostd;
+
+  using AV = api::AttributeValue;
+  using OV = sdk::OwnedAttributeValue;
+
+  sdk::AttributeEqualToVisitor equal_to_visitor;
+
+  // check different values of the same type
+  EXPECT_FALSE(opentelemetry::nostd::visit(equal_to_visitor, OV{bool(true)}, AV{bool(false)}));
+  EXPECT_FALSE(opentelemetry::nostd::visit(equal_to_visitor, OV{int32_t(22)}, AV{int32_t(33)}));
+  EXPECT_FALSE(opentelemetry::nostd::visit(equal_to_visitor, OV{int64_t(22)}, AV{int64_t(33)}));
+  EXPECT_FALSE(opentelemetry::nostd::visit(equal_to_visitor, OV{uint32_t(22)}, AV{uint32_t(33)}));
+  EXPECT_FALSE(opentelemetry::nostd::visit(equal_to_visitor, OV{double(22.2)}, AV{double(33.3)}));
+  EXPECT_FALSE(opentelemetry::nostd::visit(equal_to_visitor, OV{std::string("string one")},
+                                           AV{"another string"}));
+  EXPECT_FALSE(opentelemetry::nostd::visit(equal_to_visitor, OV{std::string("string one")},
+                                           AV{nostd::string_view("another string")}));
+
+  // check different value types
+  EXPECT_FALSE(opentelemetry::nostd::visit(equal_to_visitor, OV{bool(true)}, AV{uint32_t(1)}));
+  EXPECT_FALSE(opentelemetry::nostd::visit(equal_to_visitor, OV{int32_t(22)}, AV{uint32_t(22)}));
+
+  // check containers of different element values
+  EXPECT_FALSE(opentelemetry::nostd::visit(equal_to_visitor, OV{std::vector<bool>{true, false}},
+                                           AV{std::array<const bool, 2>{false, true}}));
+  EXPECT_FALSE(opentelemetry::nostd::visit(equal_to_visitor, OV{std::vector<int32_t>{22, 33}},
+                                           AV{std::array<const int32_t, 2>{33, 44}}));
+  EXPECT_FALSE(opentelemetry::nostd::visit(
+      equal_to_visitor, OV{std::vector<std::string>{"a string", "another string"}},
+      AV{std::array<const nostd::string_view, 2>{"a string", "a really different string"}}));
+
+  // check containers of different element types
+  EXPECT_FALSE(opentelemetry::nostd::visit(equal_to_visitor, OV{std::vector<int32_t>{22, 33}},
+                                           AV{std::array<const uint32_t, 2>{22, 33}}));
+}
+
+TEST(AttributeMapTest, EqualTo)
+{
+  using Attributes = std::initializer_list<
+      std::pair<opentelemetry::nostd::string_view, opentelemetry::common::AttributeValue>>;
+
+  // check for case where both are empty
+  Attributes attributes_empty = {};
+  auto kv_iterable_empty =
+      opentelemetry::common::MakeKeyValueIterableView<Attributes>(attributes_empty);
+  opentelemetry::sdk::common::AttributeMap attribute_map_empty(kv_iterable_empty);
+  EXPECT_TRUE(attribute_map_empty.EqualTo(kv_iterable_empty));
+
+  // check for equality with a range of attributes and types
+  Attributes attributes  = {{"key0", "some value"}, {"key1", 1}, {"key2", 2.0}, {"key3", true}};
+  auto kv_iterable_match = opentelemetry::common::MakeKeyValueIterableView<Attributes>(attributes);
+  opentelemetry::sdk::common::AttributeMap attribute_map(attributes);
+  EXPECT_TRUE(attribute_map.EqualTo(kv_iterable_match));
+
+  // check for several cases where the attributes are different
+  Attributes attributes_different_value = {
+      {"key0", "some value"}, {"key1", 1}, {"key2", 2.0}, {"key3", false}};
+  Attributes attributes_different_type = {
+      {"key0", "some value"}, {"key1", 1.0}, {"key2", 2.0}, {"key3", true}};
+  Attributes attributes_different_size = {{"key0", "some value"}};
+
+  // check for the case where the number of attributes is the same but all keys are different
+  Attributes attributes_different_all = {{"a", "b"}, {"c", "d"}, {"e", 4.0}, {"f", uint8_t(5)}};
+
+  auto kv_iterable_different_value =
+      opentelemetry::common::MakeKeyValueIterableView<Attributes>(attributes_different_value);
+  auto kv_iterable_different_type =
+      opentelemetry::common::MakeKeyValueIterableView<Attributes>(attributes_different_type);
+  auto kv_iterable_different_size =
+      opentelemetry::common::MakeKeyValueIterableView<Attributes>(attributes_different_size);
+  auto kv_iterable_different_all =
+      opentelemetry::common::MakeKeyValueIterableView<Attributes>(attributes_different_all);
+
+  EXPECT_FALSE(attribute_map.EqualTo(kv_iterable_different_value));
+  EXPECT_FALSE(attribute_map.EqualTo(kv_iterable_different_type));
+  EXPECT_FALSE(attribute_map.EqualTo(kv_iterable_different_size));
+  EXPECT_FALSE(attribute_map.EqualTo(kv_iterable_different_all));
+}