Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SDK] Fix include instrumentation scope attributes in equal method #3214

3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ Increment the:

## [Unreleased]

* [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
Expand Down
82 changes: 82 additions & 0 deletions sdk/include/opentelemetry/sdk/common/attribute_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 arithmatic types
dbarker marked this conversation as resolved.
Show resolved Hide resolved
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 arithmatic types
dbarker marked this conversation as resolved.
Show resolved Hide resolved
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.
*/
Expand Down Expand Up @@ -162,8 +216,36 @@ class AttributeMap : public std::unordered_map<std::string, OwnedAttributeValue>
(*this)[std::string(key)] = nostd::visit(converter_, value);
}

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we can use this->find(kv.first) here for better performance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we have to start the search from the KeyValueIterable::ForEachKeyValue method and it provides a nostd::string_view key while the key type of the map is std::string. So we either have to iterate directly like this or create a temp string every time as in auto iter = this->find(std::string(key)). I chose the first option since it I've heard it could be faster than find for small maps :) and I didn't feel like string allocation (and its potential to throw) was ideal. I could profile both options if it seems worth it.

C++20 gives the "Heterogeneous comparison lookup" version of the std::unordered_map::find method of unordered_map which is really what we need here. Here's a neat example https://godbolt.org/z/Yqsx4rsY1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the details. Looks ok to me as is.

{
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_;
marcalff marked this conversation as resolved.
Show resolved Hide resolved
};

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,25 @@ class InstrumentationScope
*/
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_; }
Expand Down
2 changes: 1 addition & 1 deletion sdk/src/logs/logger_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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};
}
Expand Down
2 changes: 1 addition & 1 deletion sdk/src/metrics/meter_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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};
}
Expand Down
2 changes: 1 addition & 1 deletion sdk/src/trace/tracer_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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};
}
Expand Down
125 changes: 125 additions & 0 deletions sdk/test/common/attribute_utils_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -55,3 +58,125 @@ 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(0)}));
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"}};
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);

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));
}
52 changes: 52 additions & 0 deletions sdk/test/instrumentationscope/instrumentationscope_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -208,3 +208,55 @@ TEST(InstrumentationScope, LegacyInstrumentationLibrary)
EXPECT_EQ(instrumentation_library->GetVersion(), library_version);
EXPECT_EQ(instrumentation_library->GetSchemaURL(), schema_url);
}

TEST(InstrumentationScope, Equal)
{
using Attributes = std::initializer_list<
std::pair<opentelemetry::nostd::string_view, opentelemetry::common::AttributeValue>>;
Attributes attributes_empty = {};
Attributes attributes_match = {
{"key0", "some value"}, {"key1", 1}, {"key2", 2.0}, {"key3", true}};
Attributes attributes_different = {{"key42", "some other"}};

auto kv_iterable_empty =
opentelemetry::common::MakeKeyValueIterableView<Attributes>(attributes_empty);
auto kv_iterable_match =
opentelemetry::common::MakeKeyValueIterableView<Attributes>(attributes_match);
auto kv_iterable_different =
opentelemetry::common::MakeKeyValueIterableView<Attributes>(attributes_different);

// try with no attributes added to the instrumentation scope
auto instrumentation_scope =
opentelemetry::sdk::instrumentationscope::InstrumentationScope::Create(
"library_name", "library_version", "schema_url");

// the instrumentation scope is equal if all parameters are equal (must handle nullptr attributes
// or empty attributes)
EXPECT_TRUE(instrumentation_scope->equal("library_name", "library_version", "schema_url"));
EXPECT_TRUE(
instrumentation_scope->equal("library_name", "library_version", "schema_url", nullptr));
EXPECT_TRUE(instrumentation_scope->equal("library_name", "library_version", "schema_url",
&kv_iterable_empty));

// the instrumentation scope is not equal if any parameter is different
EXPECT_FALSE(instrumentation_scope->equal("library_name", ""));
EXPECT_FALSE(instrumentation_scope->equal("library_name", "library_version", ""));
EXPECT_FALSE(instrumentation_scope->equal("library_name", "library_version", "schema_url",
&kv_iterable_different));

// try with attributes added to the instrumentation scope
auto instrumentation_scope_w_attributes =
opentelemetry::sdk::instrumentationscope::InstrumentationScope::Create(
"library_name", "library_version", "schema_url", attributes_match);

// the instrumentation scope is equal if all parameters including all attribute keys, types, and
// values are equal
EXPECT_TRUE(instrumentation_scope_w_attributes->equal("library_name", "library_version",
"schema_url", &kv_iterable_match));
EXPECT_FALSE(instrumentation_scope_w_attributes->equal("library_name", "library_version",
"schema_url", nullptr));
EXPECT_FALSE(instrumentation_scope_w_attributes->equal("library_name", "library_version",
"schema_url", &kv_iterable_empty));
EXPECT_FALSE(instrumentation_scope_w_attributes->equal("library_name", "library_version",
"schema_url", &kv_iterable_different));
}
Loading
Loading