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 @@ -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
Expand Down
83 changes: 83 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 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.
*/
Expand Down Expand Up @@ -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)
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 @@ -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_;
}

/**
Expand All @@ -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_; }
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
132 changes: 132 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,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));
}
Loading
Loading