From 1525b4c00437ebc689fab1a08bab67160a5ef38c Mon Sep 17 00:00:00 2001 From: Douglas Barker Date: Tue, 16 Jun 2026 13:44:42 -0400 Subject: [PATCH 1/3] ensure unique keys for otlp attributes, always allow bytes values in otlp attribute AnyValue types. Use a variant vistor instead of sequential if/else conditions to set otlp attributes. Populate otlp attribute and AnyValue tests. --- exporters/otlp/BUILD | 14 + exporters/otlp/CMakeLists.txt | 15 + .../otlp/otlp_populate_attribute_utils.h | 20 +- exporters/otlp/src/otlp_log_recordable.cc | 12 +- exporters/otlp/src/otlp_metric_utils.cc | 8 +- .../otlp/src/otlp_populate_attribute_utils.cc | 409 +++++++++-------- exporters/otlp/src/otlp_recordable.cc | 17 +- .../otlp/test/otlp_log_recordable_test.cc | 26 ++ .../otlp_populate_attribute_utils_test.cc | 420 ++++++++++++++++++ exporters/otlp/test/otlp_recordable_test.cc | 69 ++- 10 files changed, 765 insertions(+), 245 deletions(-) create mode 100644 exporters/otlp/test/otlp_populate_attribute_utils_test.cc diff --git a/exporters/otlp/BUILD b/exporters/otlp/BUILD index 60311dc949..d6c369b9e9 100644 --- a/exporters/otlp/BUILD +++ b/exporters/otlp/BUILD @@ -633,6 +633,20 @@ cc_library( ], ) +cc_test( + name = "otlp_populate_attribute_utils_test", + srcs = ["test/otlp_populate_attribute_utils_test.cc"], + tags = [ + "otlp", + "test", + ], + deps = [ + ":otlp_recordable", + "//sdk/src/metrics", + "@com_google_googletest//:gtest_main", + ], +) + cc_test( name = "otlp_recordable_test", srcs = ["test/otlp_recordable_test.cc"], diff --git a/exporters/otlp/CMakeLists.txt b/exporters/otlp/CMakeLists.txt index 1d2852dc73..aac92e075c 100644 --- a/exporters/otlp/CMakeLists.txt +++ b/exporters/otlp/CMakeLists.txt @@ -871,6 +871,21 @@ if(WITH_OTLP_FILE) endif() if(BUILD_TESTING) + add_executable(otlp_populate_attribute_utils_test + test/otlp_populate_attribute_utils_test.cc) + target_link_libraries( + otlp_populate_attribute_utils_test ${GTEST_BOTH_LIBRARIES} + ${CMAKE_THREAD_LIBS_INIT} opentelemetry_otlp_recordable + protobuf::libprotobuf) + gtest_add_tests( + TARGET otlp_populate_attribute_utils_test + TEST_PREFIX exporter.otlp. + TEST_LIST otlp_populate_attribute_utils_test) + if(WITH_OTLP_UTF8_VALIDITY AND TARGET utf8_range::utf8_validity) + target_compile_definitions(otlp_populate_attribute_utils_test + PRIVATE ENABLE_OTLP_UTF8_VALIDITY) + endif() + add_executable(otlp_recordable_test test/otlp_recordable_test.cc) target_link_libraries( otlp_recordable_test ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT} diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_populate_attribute_utils.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_populate_attribute_utils.h index 02273938bf..441d1d3c58 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_populate_attribute_utils.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_populate_attribute_utils.h @@ -56,22 +56,20 @@ class OtlpPopulateAttributeUtils &instrumentation_scope) noexcept; static void PopulateAnyValue(opentelemetry::proto::common::v1::AnyValue *proto_value, - const opentelemetry::common::AttributeValue &value, - bool allow_bytes) noexcept; + const opentelemetry::common::AttributeValue &value) noexcept; - static void PopulateAnyValue(opentelemetry::proto::common::v1::AnyValue *proto_value, - const opentelemetry::sdk::common::OwnedAttributeValue &value, - bool allow_bytes) noexcept; + static void PopulateAnyValue( + opentelemetry::proto::common::v1::AnyValue *proto_value, + const opentelemetry::sdk::common::OwnedAttributeValue &value) noexcept; static void PopulateAttribute(opentelemetry::proto::common::v1::KeyValue *attribute, nostd::string_view key, - const opentelemetry::common::AttributeValue &value, - bool allow_bytes) noexcept; + const opentelemetry::common::AttributeValue &value) noexcept; - static void PopulateAttribute(opentelemetry::proto::common::v1::KeyValue *attribute, - nostd::string_view key, - const opentelemetry::sdk::common::OwnedAttributeValue &value, - bool allow_bytes) noexcept; + static void PopulateAttribute( + opentelemetry::proto::common::v1::KeyValue *attribute, + nostd::string_view key, + const opentelemetry::sdk::common::OwnedAttributeValue &value) noexcept; }; } // namespace otlp diff --git a/exporters/otlp/src/otlp_log_recordable.cc b/exporters/otlp/src/otlp_log_recordable.cc index 4f1877bb41..a9b77d1a33 100644 --- a/exporters/otlp/src/otlp_log_recordable.cc +++ b/exporters/otlp/src/otlp_log_recordable.cc @@ -194,7 +194,7 @@ void OtlpLogRecordable::SetSeverity(opentelemetry::logs::Severity severity) noex void OtlpLogRecordable::SetBody(const opentelemetry::common::AttributeValue &message) noexcept { - OtlpPopulateAttributeUtils::PopulateAnyValue(proto_record_.mutable_body(), message, true); + OtlpPopulateAttributeUtils::PopulateAnyValue(proto_record_.mutable_body(), message); } void OtlpLogRecordable::SetEventId(int64_t /* id */, nostd::string_view event_name) noexcept @@ -236,7 +236,15 @@ void OtlpLogRecordable::SetTraceFlags(const opentelemetry::trace::TraceFlags &tr void OtlpLogRecordable::SetAttribute(opentelemetry::nostd::string_view key, const opentelemetry::common::AttributeValue &value) noexcept { - OtlpPopulateAttributeUtils::PopulateAttribute(proto_record_.add_attributes(), key, value, true); + for (auto &attr : *proto_record_.mutable_attributes()) + { + if (attr.key() == key) + { + OtlpPopulateAttributeUtils::PopulateAttribute(&attr, key, value); + return; + } + } + OtlpPopulateAttributeUtils::PopulateAttribute(proto_record_.add_attributes(), key, value); } void OtlpLogRecordable::SetResource(const opentelemetry::sdk::resource::Resource &resource) noexcept diff --git a/exporters/otlp/src/otlp_metric_utils.cc b/exporters/otlp/src/otlp_metric_utils.cc index e57359ee48..d52ba2cf92 100644 --- a/exporters/otlp/src/otlp_metric_utils.cc +++ b/exporters/otlp/src/otlp_metric_utils.cc @@ -106,7 +106,7 @@ void OtlpMetricUtils::ConvertSumMetric(const metric_sdk::MetricData &metric_data for (auto &kv_attr : point_data_with_attributes.attributes) { OtlpPopulateAttributeUtils::PopulateAttribute(proto_sum_point_data->add_attributes(), - kv_attr.first, kv_attr.second, false); + kv_attr.first, kv_attr.second); } } } @@ -178,7 +178,7 @@ void OtlpMetricUtils::ConvertHistogramMetric( for (auto &kv_attr : point_data_with_attributes.attributes) { OtlpPopulateAttributeUtils::PopulateAttribute(proto_histogram_point_data->add_attributes(), - kv_attr.first, kv_attr.second, false); + kv_attr.first, kv_attr.second); } } } @@ -242,7 +242,7 @@ void OtlpMetricUtils::ConvertExponentialHistogramMetric( for (auto &kv_attr : point_data_with_attributes.attributes) { OtlpPopulateAttributeUtils::PopulateAttribute(proto_histogram_point_data->add_attributes(), - kv_attr.first, kv_attr.second, false); + kv_attr.first, kv_attr.second); } } } @@ -272,7 +272,7 @@ void OtlpMetricUtils::ConvertGaugeMetric(const opentelemetry::sdk::metrics::Metr for (auto &kv_attr : point_data_with_attributes.attributes) { OtlpPopulateAttributeUtils::PopulateAttribute(proto_gauge_point_data->add_attributes(), - kv_attr.first, kv_attr.second, false); + kv_attr.first, kv_attr.second); } } } diff --git a/exporters/otlp/src/otlp_populate_attribute_utils.cc b/exporters/otlp/src/otlp_populate_attribute_utils.cc index 14259773db..c0ea4c199d 100644 --- a/exporters/otlp/src/otlp_populate_attribute_utils.cc +++ b/exporters/otlp/src/otlp_populate_attribute_utils.cc @@ -5,25 +5,32 @@ # include #endif -#include -#include +#include +#include #include +#include +#include #include +#include #include #include +#include + #include "opentelemetry/common/attribute_value.h" #include "opentelemetry/exporters/otlp/otlp_populate_attribute_utils.h" #include "opentelemetry/nostd/span.h" #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/nostd/variant.h" #include "opentelemetry/sdk/common/attribute_utils.h" +#include "opentelemetry/sdk/common/global_log_handler.h" #include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h" #include "opentelemetry/sdk/resource/resource.h" #include "opentelemetry/version.h" // clang-format off #include "opentelemetry/exporters/otlp/protobuf_include_prefix.h" // IWYU pragma: keep +// IWYU pragma: no_include "net/proto2/public/repeated_field.h" #include "opentelemetry/proto/common/v1/common.pb.h" #include "opentelemetry/proto/resource/v1/resource.pb.h" #include "opentelemetry/exporters/otlp/protobuf_include_suffix.h" // IWYU pragma: keep @@ -35,14 +42,15 @@ namespace exporter namespace otlp { +namespace +{ + // // See `attribute_value.h` for details. // const int kAttributeValueSize = 16; const int kOwnedAttributeValueSize = 15; -namespace -{ // Per OpenTelemetry spec, uint64_t attribute values exceeding INT64_MAX must be // encoded as a decimal string rather than wrapping to a negative int64 via narrowing. // https://opentelemetry.io/docs/specs/otel/common/attribute-type-mapping/#integer-values @@ -57,149 +65,110 @@ inline void SetUint64Value(opentelemetry::proto::common::v1::AnyValue *proto_val proto_value->set_string_value(std::to_string(val)); } } -} // namespace -void OtlpPopulateAttributeUtils::PopulateAnyValue( - opentelemetry::proto::common::v1::AnyValue *proto_value, - const opentelemetry::common::AttributeValue &value, - bool allow_bytes) noexcept -{ - if (nullptr == proto_value) - { - return; - } +template +struct IsSupportedAttributeValue : std::false_type +{}; - // Assert size of variant to ensure that this method gets updated if the variant - // definition changes - static_assert( - nostd::variant_size::value == kAttributeValueSize, - "AttributeValue contains unknown type"); +struct AttributeValueVisitor +{ + opentelemetry::proto::common::v1::AnyValue *proto_value_; - if (nostd::holds_alternative(value)) + template + void operator()(T &&) const { - proto_value->set_bool_value(nostd::get(value)); + static_assert( + IsSupportedAttributeValue::value, + "AttributeValueVisitor: Value type in opentelemetry::common::AttributeValue does not have " + "an overload operator implemented in this visitor OR implicit conversion attempted!"); } - else if (nostd::holds_alternative(value)) - { - proto_value->set_int_value(nostd::get(value)); - } - else if (nostd::holds_alternative(value)) - { - proto_value->set_int_value(nostd::get(value)); - } - else if (nostd::holds_alternative(value)) - { - proto_value->set_int_value(nostd::get(value)); - } - else if (nostd::holds_alternative(value)) - { - SetUint64Value(proto_value, nostd::get(value)); - } - else if (nostd::holds_alternative(value)) - { - proto_value->set_double_value(nostd::get(value)); - } - else if (nostd::holds_alternative(value)) + + void operator()(bool value) const { proto_value_->set_bool_value(value); } + void operator()(int32_t value) const { proto_value_->set_int_value(value); } + void operator()(int64_t value) const { proto_value_->set_int_value(value); } + void operator()(uint32_t value) const { proto_value_->set_int_value(value); } + void operator()(uint64_t value) const { SetUint64Value(proto_value_, value); } + void operator()(double value) const { proto_value_->set_double_value(value); } + void operator()(const char *value) const { - const char *str_value = nostd::get(value); #if defined(ENABLE_OTLP_UTF8_VALIDITY) - if (utf8_range::IsStructurallyValid(str_value)) + if (utf8_range::IsStructurallyValid(value)) { - proto_value->set_string_value(str_value); + proto_value_->set_string_value(value); } else { - proto_value->set_bytes_value(str_value, strlen(str_value)); + proto_value_->set_bytes_value(value, std::strlen(value)); } #else - proto_value->set_string_value(str_value); + proto_value_->set_string_value(value); #endif } - else if (nostd::holds_alternative(value)) + void operator()(nostd::string_view value) const { - nostd::string_view str_value = nostd::get(value); #if defined(ENABLE_OTLP_UTF8_VALIDITY) - if (utf8_range::IsStructurallyValid({str_value.data(), str_value.size()})) + if (utf8_range::IsStructurallyValid({value.data(), value.size()})) { - proto_value->set_string_value(str_value.data(), str_value.size()); + proto_value_->set_string_value(value.data(), value.size()); } else { - proto_value->set_bytes_value(str_value.data(), str_value.size()); + proto_value_->set_bytes_value(value.data(), value.size()); } #else - proto_value->set_string_value(str_value.data(), str_value.size()); + proto_value_->set_string_value(value.data(), value.size()); #endif } - else if (nostd::holds_alternative>(value)) + void operator()(nostd::span values) const { - if (allow_bytes) - { - proto_value->set_bytes_value( - reinterpret_cast(nostd::get>(value).data()), - nostd::get>(value).size()); - } - else - { - auto array_value = proto_value->mutable_array_value(); - for (const auto &val : nostd::get>(value)) - { - array_value->add_values()->set_int_value(val); - } - } - } - else if (nostd::holds_alternative>(value)) - { - auto array_value = proto_value->mutable_array_value(); - for (const auto &val : nostd::get>(value)) + opentelemetry::proto::common::v1::ArrayValue *array_value = proto_value_->mutable_array_value(); + array_value->mutable_values()->Reserve(static_cast(values.size())); + for (const auto &val : values) { array_value->add_values()->set_bool_value(val); } } - else if (nostd::holds_alternative>(value)) + void operator()(nostd::span values) const { - auto array_value = proto_value->mutable_array_value(); - for (const auto &val : nostd::get>(value)) + opentelemetry::proto::common::v1::ArrayValue *array_value = proto_value_->mutable_array_value(); + array_value->mutable_values()->Reserve(static_cast(values.size())); + for (const auto &val : values) { array_value->add_values()->set_int_value(val); } } - else if (nostd::holds_alternative>(value)) + void operator()(nostd::span values) const { - auto array_value = proto_value->mutable_array_value(); - for (const auto &val : nostd::get>(value)) + opentelemetry::proto::common::v1::ArrayValue *array_value = proto_value_->mutable_array_value(); + array_value->mutable_values()->Reserve(static_cast(values.size())); + for (const auto &val : values) { array_value->add_values()->set_int_value(val); } } - else if (nostd::holds_alternative>(value)) + void operator()(nostd::span values) const { - auto array_value = proto_value->mutable_array_value(); - for (const auto &val : nostd::get>(value)) + opentelemetry::proto::common::v1::ArrayValue *array_value = proto_value_->mutable_array_value(); + array_value->mutable_values()->Reserve(static_cast(values.size())); + for (const auto &val : values) { array_value->add_values()->set_int_value(val); } } - else if (nostd::holds_alternative>(value)) + void operator()(nostd::span values) const { - auto array_value = proto_value->mutable_array_value(); - for (const auto &val : nostd::get>(value)) - { - SetUint64Value(array_value->add_values(), val); - } - } - else if (nostd::holds_alternative>(value)) - { - auto array_value = proto_value->mutable_array_value(); - for (const auto &val : nostd::get>(value)) + opentelemetry::proto::common::v1::ArrayValue *array_value = proto_value_->mutable_array_value(); + array_value->mutable_values()->Reserve(static_cast(values.size())); + for (const auto &val : values) { array_value->add_values()->set_double_value(val); } } - else if (nostd::holds_alternative>(value)) + void operator()(nostd::span values) const { - auto array_value = proto_value->mutable_array_value(); - for (const auto &val : nostd::get>(value)) + opentelemetry::proto::common::v1::ArrayValue *array_value = proto_value_->mutable_array_value(); + array_value->mutable_values()->Reserve(static_cast(values.size())); + for (const auto &val : values) { #if defined(ENABLE_OTLP_UTF8_VALIDITY) if (utf8_range::IsStructurallyValid({val.data(), val.size()})) @@ -215,156 +184,185 @@ void OtlpPopulateAttributeUtils::PopulateAnyValue( #endif } } -} - -void OtlpPopulateAttributeUtils::PopulateAnyValue( - opentelemetry::proto::common::v1::AnyValue *proto_value, - const opentelemetry::sdk::common::OwnedAttributeValue &value, - bool allow_bytes) noexcept -{ - if (nullptr == proto_value) - { - return; - } - - // Assert size of variant to ensure that this method gets updated if the variant - // definition changes - static_assert(nostd::variant_size::value == - kOwnedAttributeValueSize, - "OwnedAttributeValue contains unknown type"); - - if (nostd::holds_alternative(value)) - { - proto_value->set_bool_value(nostd::get(value)); - } - else if (nostd::holds_alternative(value)) - { - proto_value->set_int_value(nostd::get(value)); - } - else if (nostd::holds_alternative(value)) - { - proto_value->set_int_value(nostd::get(value)); - } - else if (nostd::holds_alternative(value)) - { - proto_value->set_int_value(nostd::get(value)); - } - else if (nostd::holds_alternative(value)) + void operator()(nostd::span values) const { - SetUint64Value(proto_value, nostd::get(value)); - } - else if (nostd::holds_alternative(value)) - { - proto_value->set_double_value(nostd::get(value)); - } - else if (nostd::holds_alternative>(value)) - { - if (allow_bytes) + opentelemetry::proto::common::v1::ArrayValue *array_value = proto_value_->mutable_array_value(); + array_value->mutable_values()->Reserve(static_cast(values.size())); + for (const auto &val : values) { - const std::vector &byte_array = nostd::get>(value); - proto_value->set_bytes_value(reinterpret_cast(byte_array.data()), - byte_array.size()); - } - else - { - auto array_value = proto_value->mutable_array_value(); - for (const auto &val : nostd::get>(value)) - { - array_value->add_values()->set_int_value(val); - } + SetUint64Value(array_value->add_values(), val); } } - else if (nostd::holds_alternative(value)) + void operator()(nostd::span values) const { - const std::string &str_value = nostd::get(value); -#if defined(ENABLE_OTLP_UTF8_VALIDITY) - if (utf8_range::IsStructurallyValid(str_value)) - { - proto_value->set_string_value(str_value); - } - else - { - proto_value->set_bytes_value(str_value); - } -#else - proto_value->set_string_value(str_value); -#endif + proto_value_->set_bytes_value(reinterpret_cast(values.data()), values.size()); } - else if (nostd::holds_alternative>(value)) +}; + +struct OwnedAttributeValueVisitor +{ + opentelemetry::proto::common::v1::AnyValue *proto_value_; + + template + void operator()(T &&) const { - auto array_value = proto_value->mutable_array_value(); - for (const auto val : nostd::get>(value)) + static_assert(IsSupportedAttributeValue::value, + "OwnedAttributeValueVisitor: Value type in " + "opentelemetry::sdk::common::OwnedAttributeValue does not have an overload " + "operator implemented in this visitor OR implicit conversion attempted!"); + } + + void operator()(bool value) const { proto_value_->set_bool_value(value); } + void operator()(int32_t value) const { proto_value_->set_int_value(value); } + void operator()(uint32_t value) const { proto_value_->set_int_value(value); } + void operator()(int64_t value) const { proto_value_->set_int_value(value); } + void operator()(uint64_t value) const { SetUint64Value(proto_value_, value); } + void operator()(double value) const { proto_value_->set_double_value(value); } + void operator()(const std::string &value) const { proto_value_->set_string_value(value); } + void operator()(const std::vector &values) const + { + opentelemetry::proto::common::v1::ArrayValue *array_value = proto_value_->mutable_array_value(); + array_value->mutable_values()->Reserve(static_cast(values.size())); + for (const auto val : values) { array_value->add_values()->set_bool_value(val); } } - else if (nostd::holds_alternative>(value)) + void operator()(const std::vector &values) const { - auto array_value = proto_value->mutable_array_value(); - for (const auto &val : nostd::get>(value)) + opentelemetry::proto::common::v1::ArrayValue *array_value = proto_value_->mutable_array_value(); + array_value->mutable_values()->Reserve(static_cast(values.size())); + for (const auto &val : values) { array_value->add_values()->set_int_value(val); } } - else if (nostd::holds_alternative>(value)) + void operator()(const std::vector &values) const { - auto array_value = proto_value->mutable_array_value(); - for (const auto &val : nostd::get>(value)) + opentelemetry::proto::common::v1::ArrayValue *array_value = proto_value_->mutable_array_value(); + array_value->mutable_values()->Reserve(static_cast(values.size())); + for (const auto &val : values) { - array_value->add_values()->set_int_value( - val); // NOLINT(cppcoreguidelines-narrowing-conversions) + array_value->add_values()->set_int_value(static_cast(val)); } } - else if (nostd::holds_alternative>(value)) + void operator()(const std::vector &values) const { - auto array_value = proto_value->mutable_array_value(); - for (const auto &val : nostd::get>(value)) + opentelemetry::proto::common::v1::ArrayValue *array_value = proto_value_->mutable_array_value(); + array_value->mutable_values()->Reserve(static_cast(values.size())); + for (const auto &val : values) { array_value->add_values()->set_int_value(val); } } - else if (nostd::holds_alternative>(value)) - { - auto array_value = proto_value->mutable_array_value(); - for (const auto &val : nostd::get>(value)) - { - SetUint64Value(array_value->add_values(), val); - } - } - else if (nostd::holds_alternative>(value)) + void operator()(const std::vector &values) const { - auto array_value = proto_value->mutable_array_value(); - for (const auto &val : nostd::get>(value)) + opentelemetry::proto::common::v1::ArrayValue *array_value = proto_value_->mutable_array_value(); + array_value->mutable_values()->Reserve(static_cast(values.size())); + for (const auto &val : values) { array_value->add_values()->set_double_value(val); } } - else if (nostd::holds_alternative>(value)) + void operator()(const std::vector &values) const { - auto array_value = proto_value->mutable_array_value(); - for (const auto &val : nostd::get>(value)) + opentelemetry::proto::common::v1::ArrayValue *array_value = proto_value_->mutable_array_value(); + array_value->mutable_values()->Reserve(static_cast(values.size())); + for (const auto &val : values) { #if defined(ENABLE_OTLP_UTF8_VALIDITY) - if (utf8_range::IsStructurallyValid(val)) + if (utf8_range::IsStructurallyValid({val.data(), val.size()})) { - array_value->add_values()->set_string_value(val); + array_value->add_values()->set_string_value(val.data(), val.size()); } else { - array_value->add_values()->set_bytes_value(val); + array_value->add_values()->set_bytes_value(val.data(), val.size()); } #else - array_value->add_values()->set_string_value(val); + array_value->add_values()->set_string_value(val.data(), val.size()); #endif } } + void operator()(const std::vector &values) const + { + opentelemetry::proto::common::v1::ArrayValue *array_value = proto_value_->mutable_array_value(); + array_value->mutable_values()->Reserve(static_cast(values.size())); + for (const auto &val : values) + { + SetUint64Value(array_value->add_values(), val); + } + } + void operator()(const std::vector &values) const + { + proto_value_->set_bytes_value(reinterpret_cast(values.data()), values.size()); + } +}; + +} // namespace + +void OtlpPopulateAttributeUtils::PopulateAnyValue( + opentelemetry::proto::common::v1::AnyValue *proto_value, + const opentelemetry::common::AttributeValue &value) noexcept +{ +#if OPENTELEMETRY_HAVE_EXCEPTIONS + try + { +#endif + nostd::visit(AttributeValueVisitor{proto_value}, value); +#if OPENTELEMETRY_HAVE_EXCEPTIONS + } +# if defined(OPENTELEMETRY_HAVE_STD_VARIANT) + catch (const std::bad_variant_access &e) +# else + catch (const opentelemetry::nostd::bad_variant_access &e) +# endif + { + OTEL_INTERNAL_LOG_ERROR( + "[OTLP Populate Attribute] bad_variant_access in PopulateAnyValue: " << e.what()); + } + catch (const std::bad_alloc &e) + { + // TODO: Log an error once the logger doesn't require dynamic memory allocation. + // OTEL_INTERNAL_LOG_ERROR( + // "[OTLP Populate Attribute] std::bad_alloc in PopulateAnyValue: " << e.what()); + } +#endif +} + +void OtlpPopulateAttributeUtils::PopulateAnyValue( + opentelemetry::proto::common::v1::AnyValue *proto_value, + const opentelemetry::sdk::common::OwnedAttributeValue &value) noexcept +{ +#if OPENTELEMETRY_HAVE_EXCEPTIONS + try + { +#endif + nostd::visit(OwnedAttributeValueVisitor{proto_value}, value); +#if OPENTELEMETRY_HAVE_EXCEPTIONS + } +# if defined(OPENTELEMETRY_HAVE_STD_VARIANT) + catch (const std::bad_variant_access &e) +# else + catch (const opentelemetry::nostd::bad_variant_access &e) +# endif + { + OTEL_INTERNAL_LOG_ERROR( + "[OTLP Populate Attribute] bad_variant_access in PopulateAnyValue: " << e.what()); + } + catch (const std::bad_alloc &e) + { + // TODO: Log an error once the logger doesn't require dynamic memory allocation. + // OTEL_INTERNAL_LOG_ERROR( + // "[OTLP Populate Attribute] std::bad_alloc in PopulateAnyValue: " << e.what()); + } +#endif } void OtlpPopulateAttributeUtils::PopulateAttribute( opentelemetry::proto::common::v1::KeyValue *attribute, nostd::string_view key, - const opentelemetry::common::AttributeValue &value, - bool allow_bytes) noexcept + const opentelemetry::common::AttributeValue &value) noexcept { if (nullptr == attribute) { @@ -378,15 +376,14 @@ void OtlpPopulateAttributeUtils::PopulateAttribute( "AttributeValue contains unknown type"); attribute->set_key(key.data(), key.size()); - PopulateAnyValue(attribute->mutable_value(), value, allow_bytes); + PopulateAnyValue(attribute->mutable_value(), value); } /** Maps from C++ attribute into OTLP proto attribute. */ void OtlpPopulateAttributeUtils::PopulateAttribute( opentelemetry::proto::common::v1::KeyValue *attribute, nostd::string_view key, - const opentelemetry::sdk::common::OwnedAttributeValue &value, - bool allow_bytes) noexcept + const opentelemetry::sdk::common::OwnedAttributeValue &value) noexcept { if (nullptr == attribute) { @@ -400,7 +397,7 @@ void OtlpPopulateAttributeUtils::PopulateAttribute( "OwnedAttributeValue contains unknown type"); attribute->set_key(key.data(), key.size()); - PopulateAnyValue(attribute->mutable_value(), value, allow_bytes); + PopulateAnyValue(attribute->mutable_value(), value); } void OtlpPopulateAttributeUtils::PopulateAttribute( @@ -414,8 +411,7 @@ void OtlpPopulateAttributeUtils::PopulateAttribute( for (const auto &kv : resource.GetAttributes()) { - OtlpPopulateAttributeUtils::PopulateAttribute(proto->add_attributes(), kv.first, kv.second, - false); + OtlpPopulateAttributeUtils::PopulateAttribute(proto->add_attributes(), kv.first, kv.second); } } @@ -426,8 +422,7 @@ void OtlpPopulateAttributeUtils::PopulateAttribute( { for (const auto &kv : instrumentation_scope.GetAttributes()) { - OtlpPopulateAttributeUtils::PopulateAttribute(proto->add_attributes(), kv.first, kv.second, - false); + OtlpPopulateAttributeUtils::PopulateAttribute(proto->add_attributes(), kv.first, kv.second); } } diff --git a/exporters/otlp/src/otlp_recordable.cc b/exporters/otlp/src/otlp_recordable.cc index 9270e17383..d90dd661c6 100644 --- a/exporters/otlp/src/otlp_recordable.cc +++ b/exporters/otlp/src/otlp_recordable.cc @@ -117,14 +117,23 @@ void OtlpRecordable::SetResource(const sdk::resource::Resource &resource) noexce void OtlpRecordable::SetAttribute(nostd::string_view key, const common::AttributeValue &value) noexcept { + // Check for an existing attribute with the same key and overwrite it (spec requires unique keys). + for (auto &attr : *span_.mutable_attributes()) + { + if (attr.key() == key) + { + OtlpPopulateAttributeUtils::PopulateAttribute(&attr, key, value); + return; + } + } + if (static_cast(span_.attributes_size()) >= max_attributes_) { span_.set_dropped_attributes_count(span_.dropped_attributes_count() + 1); return; } - auto *attribute = span_.add_attributes(); - OtlpPopulateAttributeUtils::PopulateAttribute(attribute, key, value, false); + OtlpPopulateAttributeUtils::PopulateAttribute(span_.add_attributes(), key, value); } void OtlpRecordable::AddEvent(nostd::string_view name, @@ -147,7 +156,7 @@ void OtlpRecordable::AddEvent(nostd::string_view name, event->set_dropped_attributes_count(event->dropped_attributes_count() + 1); return true; } - OtlpPopulateAttributeUtils::PopulateAttribute(event->add_attributes(), key, value, false); + OtlpPopulateAttributeUtils::PopulateAttribute(event->add_attributes(), key, value); return true; }); } @@ -172,7 +181,7 @@ void OtlpRecordable::AddLink(const trace::SpanContext &span_context, link->set_dropped_attributes_count(link->dropped_attributes_count() + 1); return true; } - OtlpPopulateAttributeUtils::PopulateAttribute(link->add_attributes(), key, value, false); + OtlpPopulateAttributeUtils::PopulateAttribute(link->add_attributes(), key, value); return true; }); } diff --git a/exporters/otlp/test/otlp_log_recordable_test.cc b/exporters/otlp/test/otlp_log_recordable_test.cc index 93bb67dca8..ca0b62390a 100644 --- a/exporters/otlp/test/otlp_log_recordable_test.cc +++ b/exporters/otlp/test/otlp_log_recordable_test.cc @@ -419,6 +419,32 @@ TEST(OtlpLogRecordable, PopulateRequestSameScope) EXPECT_EQ(req.resource_logs(0).scope_logs(0).log_records_size(), 2); EXPECT_EQ(req.resource_logs(0).scope_logs(0).scope().name(), "lib"); } +// Test that setting an attribute with an existing key overwrites the value in place +// without creating a duplicate entry (spec: attribute keys MUST be unique). +TEST(OtlpLogRecordable, SetAttributeDeduplicatesKey) +{ + OtlpLogRecordable rec; + rec.SetAttribute("key", opentelemetry::common::AttributeValue{int64_t(1)}); + rec.SetAttribute("key", opentelemetry::common::AttributeValue{int64_t(2)}); + + // Only one attribute; the second call must overwrite, not append. + ASSERT_EQ(rec.log_record().attributes_size(), 1); + EXPECT_EQ(rec.log_record().attributes(0).key(), "key"); + EXPECT_EQ(rec.log_record().attributes(0).value().int_value(), 2); +} + +// Test that updating a duplicate key changes the type as well as the value. +TEST(OtlpLogRecordable, SetAttributeDeduplicateChangesType) +{ + OtlpLogRecordable rec; + rec.SetAttribute("type_change_attribute", opentelemetry::common::AttributeValue{int64_t(42)}); + rec.SetAttribute("type_change_attribute", + opentelemetry::common::AttributeValue{nostd::string_view("hello")}); + + ASSERT_EQ(rec.log_record().attributes_size(), 1); + EXPECT_EQ(rec.log_record().attributes(0).value().string_value(), "hello"); +} + } // namespace otlp } // namespace exporter OPENTELEMETRY_END_NAMESPACE diff --git a/exporters/otlp/test/otlp_populate_attribute_utils_test.cc b/exporters/otlp/test/otlp_populate_attribute_utils_test.cc new file mode 100644 index 0000000000..ef32f17e95 --- /dev/null +++ b/exporters/otlp/test/otlp_populate_attribute_utils_test.cc @@ -0,0 +1,420 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#include +#include +#include +#include +#include + +#include "opentelemetry/common/attribute_value.h" +#include "opentelemetry/exporters/otlp/otlp_populate_attribute_utils.h" +#include "opentelemetry/nostd/span.h" +#include "opentelemetry/nostd/string_view.h" +#include "opentelemetry/sdk/common/attribute_utils.h" +#include "opentelemetry/version.h" + +// clang-format off +#include "opentelemetry/exporters/otlp/protobuf_include_prefix.h" // IWYU pragma: keep +#include "opentelemetry/proto/common/v1/common.pb.h" +#include "opentelemetry/exporters/otlp/protobuf_include_suffix.h" // IWYU pragma: keep +// clang-format on + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace exporter +{ +namespace otlp +{ + +class PopulateAnyValueTest : public ::testing::Test +{ +protected: + opentelemetry::proto::common::v1::AnyValue proto_anyvalue_; +}; + +class PopulateAttributeTest : public ::testing::Test +{ +protected: + opentelemetry::proto::common::v1::KeyValue proto_keyvalue_; +}; + +// ============================================================ +// PopulateAnyValue with common::AttributeValue Tests +// ============================================================ + +TEST_F(PopulateAnyValueTest, AnyValueBool) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue(&proto_anyvalue_, common::AttributeValue{true}); + EXPECT_TRUE(proto_anyvalue_.bool_value()); +} + +TEST_F(PopulateAnyValueTest, AnyValueInt32) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue(&proto_anyvalue_, + common::AttributeValue{int32_t{42}}); + EXPECT_EQ(proto_anyvalue_.int_value(), 42); +} + +TEST_F(PopulateAnyValueTest, AnyValueInt64) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue(&proto_anyvalue_, + common::AttributeValue{int64_t{-1}}); + EXPECT_EQ(proto_anyvalue_.int_value(), -1); +} + +TEST_F(PopulateAnyValueTest, AnyValueUint32) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue(&proto_anyvalue_, + common::AttributeValue{uint32_t{100}}); + EXPECT_EQ(proto_anyvalue_.int_value(), 100); +} + +TEST_F(PopulateAnyValueTest, AnyValueDouble) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue(&proto_anyvalue_, common::AttributeValue{3.14}); + EXPECT_DOUBLE_EQ(proto_anyvalue_.double_value(), 3.14); +} + +TEST_F(PopulateAnyValueTest, AnyValueCString) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue(&proto_anyvalue_, common::AttributeValue{"test123"}); + EXPECT_EQ(proto_anyvalue_.string_value(), "test123"); +} + +TEST_F(PopulateAnyValueTest, AnyValueStringView) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, common::AttributeValue{nostd::string_view{"test123"}}); + EXPECT_EQ(proto_anyvalue_.string_value(), "test123"); +} + +#if defined(ENABLE_OTLP_UTF8_VALIDITY) +TEST_F(PopulateAnyValueTest, AnyValueCStringInvalidUtf8) +{ + // Bare continuation bytes are structurally invalid UTF-8. + const char invalid_utf8[] = {'\x80', '\x81', '\x82', '\x83', '\0'}; + OtlpPopulateAttributeUtils::PopulateAnyValue(&proto_anyvalue_, + common::AttributeValue{invalid_utf8}); + EXPECT_EQ(proto_anyvalue_.bytes_value(), std::string("\x80\x81\x82\x83", 4)); +} + +TEST_F(PopulateAnyValueTest, AnyValueStringViewInvalidUtf8) +{ + // Bare continuation bytes are structurally invalid UTF-8. + const char invalid_utf8[] = {'\x80', '\x81', '\x82', '\x83'}; + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, common::AttributeValue{nostd::string_view{invalid_utf8, 4}}); + EXPECT_EQ(proto_anyvalue_.bytes_value(), std::string("\x80\x81\x82\x83", 4)); +} +#endif // defined(ENABLE_OTLP_UTF8_VALIDITY) + +// uint64_t values within int64_t range map to int_value per the `Mapping Arbitrary Data to OTLP +// AnyValue` specification. +TEST_F(PopulateAnyValueTest, AnyValueUint64InRange) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, + common::AttributeValue{static_cast(std::numeric_limits::max())}); + EXPECT_EQ(proto_anyvalue_.int_value(), std::numeric_limits::max()); +} + +// uint64_t values above INT64_MAX map to string_value per the `Mapping Arbitrary Data to OTLP +// AnyValue` specification. +TEST_F(PopulateAnyValueTest, AnyValueUint64OutOfRange) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, common::AttributeValue{std::numeric_limits::max()}); + EXPECT_EQ(proto_anyvalue_.string_value(), std::to_string(std::numeric_limits::max())); +} + +TEST_F(PopulateAnyValueTest, AnyValueSpanUint8) +{ + const uint8_t data[] = {0x01, 0x02, 0x03}; + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, common::AttributeValue{nostd::span{data}}); + EXPECT_EQ(proto_anyvalue_.bytes_value(), std::string("\x01\x02\x03", 3)); +} + +TEST_F(PopulateAnyValueTest, AnyValueSpanBool) +{ + const bool data[] = {true, false, true}; + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, common::AttributeValue{nostd::span{data}}); + ASSERT_EQ(proto_anyvalue_.array_value().values_size(), 3); + EXPECT_TRUE(proto_anyvalue_.array_value().values(0).bool_value()); + EXPECT_FALSE(proto_anyvalue_.array_value().values(1).bool_value()); + EXPECT_TRUE(proto_anyvalue_.array_value().values(2).bool_value()); +} + +TEST_F(PopulateAnyValueTest, AnyValueSpanInt32) +{ + const int32_t data[] = {10, -20, 30}; + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, common::AttributeValue{nostd::span{data}}); + ASSERT_EQ(proto_anyvalue_.array_value().values_size(), 3); + EXPECT_EQ(proto_anyvalue_.array_value().values(0).int_value(), 10); + EXPECT_EQ(proto_anyvalue_.array_value().values(1).int_value(), -20); + EXPECT_EQ(proto_anyvalue_.array_value().values(2).int_value(), 30); +} + +TEST_F(PopulateAnyValueTest, AnyValueSpanInt64) +{ + const int64_t data[] = {-1000, 2000}; + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, common::AttributeValue{nostd::span{data}}); + ASSERT_EQ(proto_anyvalue_.array_value().values_size(), 2); + EXPECT_EQ(proto_anyvalue_.array_value().values(0).int_value(), -1000); + EXPECT_EQ(proto_anyvalue_.array_value().values(1).int_value(), 2000); +} + +TEST_F(PopulateAnyValueTest, AnyValueSpanUint32) +{ + const uint32_t data[] = {100, 200}; + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, common::AttributeValue{nostd::span{data}}); + ASSERT_EQ(proto_anyvalue_.array_value().values_size(), 2); + EXPECT_EQ(proto_anyvalue_.array_value().values(0).int_value(), 100); + EXPECT_EQ(proto_anyvalue_.array_value().values(1).int_value(), 200); +} + +TEST_F(PopulateAnyValueTest, AnyValueSpanUint64) +{ + const uint64_t data[] = {100, 200, 300}; + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, common::AttributeValue{nostd::span{data}}); + ASSERT_EQ(proto_anyvalue_.array_value().values_size(), 3); + EXPECT_EQ(proto_anyvalue_.array_value().values(0).int_value(), 100); + EXPECT_EQ(proto_anyvalue_.array_value().values(1).int_value(), 200); + EXPECT_EQ(proto_anyvalue_.array_value().values(2).int_value(), 300); +} + +// uint64_t values above INT64_MAX in a span map to string_value per the `Mapping Arbitrary Data +// to OTLP AnyValue` specification. +TEST_F(PopulateAnyValueTest, AnyValueSpanUint64Overflow) +{ + const uint64_t data[] = {static_cast(std::numeric_limits::max()), + std::numeric_limits::max()}; + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, common::AttributeValue{nostd::span{data}}); + ASSERT_EQ(proto_anyvalue_.array_value().values_size(), 2); + EXPECT_EQ(proto_anyvalue_.array_value().values(0).int_value(), + std::numeric_limits::max()); + EXPECT_EQ(proto_anyvalue_.array_value().values(1).string_value(), + std::to_string(std::numeric_limits::max())); +} + +TEST_F(PopulateAnyValueTest, AnyValueSpanDouble) +{ + const double data[] = {1.5, 2.5}; + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, common::AttributeValue{nostd::span{data}}); + ASSERT_EQ(proto_anyvalue_.array_value().values_size(), 2); + EXPECT_DOUBLE_EQ(proto_anyvalue_.array_value().values(0).double_value(), 1.5); + EXPECT_DOUBLE_EQ(proto_anyvalue_.array_value().values(1).double_value(), 2.5); +} + +TEST_F(PopulateAnyValueTest, AnyValueSpanStringView) +{ + const nostd::string_view data[] = {"foo", "bar"}; + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, common::AttributeValue{nostd::span{data}}); + ASSERT_EQ(proto_anyvalue_.array_value().values_size(), 2); + EXPECT_EQ(proto_anyvalue_.array_value().values(0).string_value(), "foo"); + EXPECT_EQ(proto_anyvalue_.array_value().values(1).string_value(), "bar"); +} + +#if defined(ENABLE_OTLP_UTF8_VALIDITY) +TEST_F(PopulateAnyValueTest, AnyValueSpanStringViewInvalidUtf8) +{ + // Mix of valid and invalid UTF-8 entries. + const char invalid_utf8[] = {'\x80', '\x81', '\x82', '\x83'}; + const nostd::string_view data[] = {"valid", nostd::string_view{invalid_utf8, 4}}; + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, common::AttributeValue{nostd::span{data}}); + ASSERT_EQ(proto_anyvalue_.array_value().values_size(), 2); + EXPECT_EQ(proto_anyvalue_.array_value().values(0).string_value(), "valid"); + EXPECT_EQ(proto_anyvalue_.array_value().values(1).bytes_value(), + std::string("\x80\x81\x82\x83", 4)); +} +#endif // defined(ENABLE_OTLP_UTF8_VALIDITY) + +// ============================================================ +// PopulateAnyValue with sdk::common::OwnedAttributeValue Tests +// ============================================================ + +TEST_F(PopulateAnyValueTest, OwnedAnyValueBool) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue(&proto_anyvalue_, + sdk::common::OwnedAttributeValue{false}); + EXPECT_FALSE(proto_anyvalue_.bool_value()); +} + +TEST_F(PopulateAnyValueTest, OwnedAnyValueInt32) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue(&proto_anyvalue_, + sdk::common::OwnedAttributeValue{int32_t{7}}); + EXPECT_EQ(proto_anyvalue_.int_value(), 7); +} + +TEST_F(PopulateAnyValueTest, OwnedAnyValueUint32) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue(&proto_anyvalue_, + sdk::common::OwnedAttributeValue{uint32_t{200}}); + EXPECT_EQ(proto_anyvalue_.int_value(), 200); +} + +TEST_F(PopulateAnyValueTest, OwnedAnyValueInt64) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue(&proto_anyvalue_, + sdk::common::OwnedAttributeValue{int64_t{-99}}); + EXPECT_EQ(proto_anyvalue_.int_value(), -99); +} + +TEST_F(PopulateAnyValueTest, OwnedAnyValueDouble) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue(&proto_anyvalue_, + sdk::common::OwnedAttributeValue{2.71}); + EXPECT_DOUBLE_EQ(proto_anyvalue_.double_value(), 2.71); +} + +TEST_F(PopulateAnyValueTest, OwnedAnyValueString) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, sdk::common::OwnedAttributeValue{std::string{"owned"}}); + EXPECT_EQ(proto_anyvalue_.string_value(), "owned"); +} + +TEST_F(PopulateAnyValueTest, OwnedAnyValueVectorUint8) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, sdk::common::OwnedAttributeValue{std::vector{0xAA, 0xBB}}); + EXPECT_EQ(proto_anyvalue_.bytes_value(), std::string("\xAA\xBB", 2)); +} + +TEST_F(PopulateAnyValueTest, OwnedAnyValueVectorBool) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, sdk::common::OwnedAttributeValue{std::vector{true, false, true}}); + ASSERT_EQ(proto_anyvalue_.array_value().values_size(), 3); + EXPECT_TRUE(proto_anyvalue_.array_value().values(0).bool_value()); + EXPECT_FALSE(proto_anyvalue_.array_value().values(1).bool_value()); + EXPECT_TRUE(proto_anyvalue_.array_value().values(2).bool_value()); +} + +TEST_F(PopulateAnyValueTest, OwnedAnyValueVectorInt32) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, sdk::common::OwnedAttributeValue{std::vector{10, -20, 30}}); + ASSERT_EQ(proto_anyvalue_.array_value().values_size(), 3); + EXPECT_EQ(proto_anyvalue_.array_value().values(0).int_value(), 10); + EXPECT_EQ(proto_anyvalue_.array_value().values(1).int_value(), -20); + EXPECT_EQ(proto_anyvalue_.array_value().values(2).int_value(), 30); +} + +TEST_F(PopulateAnyValueTest, OwnedAnyValueVectorUint32) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, sdk::common::OwnedAttributeValue{std::vector{100, 200}}); + ASSERT_EQ(proto_anyvalue_.array_value().values_size(), 2); + EXPECT_EQ(proto_anyvalue_.array_value().values(0).int_value(), 100); + EXPECT_EQ(proto_anyvalue_.array_value().values(1).int_value(), 200); +} + +TEST_F(PopulateAnyValueTest, OwnedAnyValueVectorInt64) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, sdk::common::OwnedAttributeValue{std::vector{-1000, 2000}}); + ASSERT_EQ(proto_anyvalue_.array_value().values_size(), 2); + EXPECT_EQ(proto_anyvalue_.array_value().values(0).int_value(), -1000); + EXPECT_EQ(proto_anyvalue_.array_value().values(1).int_value(), 2000); +} + +TEST_F(PopulateAnyValueTest, OwnedAnyValueVectorDouble) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, sdk::common::OwnedAttributeValue{std::vector{1.1, 2.2, 3.3}}); + ASSERT_EQ(proto_anyvalue_.array_value().values_size(), 3); + EXPECT_DOUBLE_EQ(proto_anyvalue_.array_value().values(0).double_value(), 1.1); + EXPECT_DOUBLE_EQ(proto_anyvalue_.array_value().values(1).double_value(), 2.2); + EXPECT_DOUBLE_EQ(proto_anyvalue_.array_value().values(2).double_value(), 3.3); +} + +TEST_F(PopulateAnyValueTest, OwnedAnyValueVectorString) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, + sdk::common::OwnedAttributeValue{std::vector{"one", "two", "three"}}); + ASSERT_EQ(proto_anyvalue_.array_value().values_size(), 3); + EXPECT_EQ(proto_anyvalue_.array_value().values(0).string_value(), "one"); + EXPECT_EQ(proto_anyvalue_.array_value().values(1).string_value(), "two"); + EXPECT_EQ(proto_anyvalue_.array_value().values(2).string_value(), "three"); +} + +#if defined(ENABLE_OTLP_UTF8_VALIDITY) +TEST_F(PopulateAnyValueTest, OwnedAnyValueVectorStringInvalidUtf8) +{ + // Mix of valid and invalid UTF-8 entries. + const std::string invalid_utf8("\x80\x81\x82\x83", 4); + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, + sdk::common::OwnedAttributeValue{std::vector{"valid", invalid_utf8}}); + ASSERT_EQ(proto_anyvalue_.array_value().values_size(), 2); + EXPECT_EQ(proto_anyvalue_.array_value().values(0).string_value(), "valid"); + EXPECT_EQ(proto_anyvalue_.array_value().values(1).bytes_value(), + std::string("\x80\x81\x82\x83", 4)); +} +#endif // defined(ENABLE_OTLP_UTF8_VALIDITY) + +// uint64_t values within int64_t range map to int_value per the `Mapping Arbitrary Data to OTLP +// AnyValue` specification. +TEST_F(PopulateAnyValueTest, OwnedAnyValueUint64InRange) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, + sdk::common::OwnedAttributeValue{static_cast(std::numeric_limits::max())}); + EXPECT_EQ(proto_anyvalue_.int_value(), std::numeric_limits::max()); +} + +// uint64_t values above INT64_MAX map to string_value per the `Mapping Arbitrary Data to OTLP +// AnyValue` specification. +TEST_F(PopulateAnyValueTest, OwnedAnyValueUint64OutOfRange) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, sdk::common::OwnedAttributeValue{std::numeric_limits::max()}); + EXPECT_EQ(proto_anyvalue_.string_value(), std::to_string(std::numeric_limits::max())); +} + +TEST_F(PopulateAnyValueTest, OwnedAnyValueVectorUint64) +{ + OtlpPopulateAttributeUtils::PopulateAnyValue( + &proto_anyvalue_, sdk::common::OwnedAttributeValue{std::vector{100, 200, 300}}); + ASSERT_EQ(proto_anyvalue_.array_value().values_size(), 3); + EXPECT_EQ(proto_anyvalue_.array_value().values(0).int_value(), 100); + EXPECT_EQ(proto_anyvalue_.array_value().values(1).int_value(), 200); + EXPECT_EQ(proto_anyvalue_.array_value().values(2).int_value(), 300); +} + +// ============================================================ +// PopulateAttribute Tests +// ============================================================ + +TEST_F(PopulateAttributeTest, PopulateAttributeKeyValue) +{ + OtlpPopulateAttributeUtils::PopulateAttribute(&proto_keyvalue_, nostd::string_view{"my-key"}, + common::AttributeValue{int64_t{123}}); + EXPECT_EQ(proto_keyvalue_.key(), "my-key"); + EXPECT_EQ(proto_keyvalue_.value().int_value(), 123); +} + +TEST_F(PopulateAttributeTest, PopulateAttributeKeyValueOwned) +{ + OtlpPopulateAttributeUtils::PopulateAttribute( + &proto_keyvalue_, nostd::string_view{"my-key"}, + sdk::common::OwnedAttributeValue{std::string{"my-value"}}); + EXPECT_EQ(proto_keyvalue_.key(), "my-key"); + EXPECT_EQ(proto_keyvalue_.value().string_value(), "my-value"); +} + +} // namespace otlp +} // namespace exporter +OPENTELEMETRY_END_NAMESPACE diff --git a/exporters/otlp/test/otlp_recordable_test.cc b/exporters/otlp/test/otlp_recordable_test.cc index 7f3dd00180..93f1db408d 100644 --- a/exporters/otlp/test/otlp_recordable_test.cc +++ b/exporters/otlp/test/otlp_recordable_test.cc @@ -320,13 +320,13 @@ TEST(OtlpRecordable, SetResource) } else if (attr.key() == "bytes_value") { - EXPECT_EQ(attr.value().array_value().values_size(), 6); - EXPECT_EQ(attr.value().array_value().values(0).int_value(), 1); - EXPECT_EQ(attr.value().array_value().values(1).int_value(), 0); - EXPECT_EQ(attr.value().array_value().values(2).int_value(), 3); - EXPECT_EQ(attr.value().array_value().values(3).int_value(), static_cast('a')); - EXPECT_EQ(attr.value().array_value().values(4).int_value(), static_cast('b')); - EXPECT_EQ(attr.value().array_value().values(5).int_value(), static_cast('c')); + EXPECT_EQ(attr.value().bytes_value().size(), 6u); + EXPECT_EQ(static_cast(attr.value().bytes_value()[0]), 1); + EXPECT_EQ(static_cast(attr.value().bytes_value()[1]), 0); + EXPECT_EQ(static_cast(attr.value().bytes_value()[2]), 3); + EXPECT_EQ(attr.value().bytes_value()[3], 'a'); + EXPECT_EQ(attr.value().bytes_value()[4], 'b'); + EXPECT_EQ(attr.value().bytes_value()[5], 'c'); ++found_attribute_count; } else if (attr.key() == "bool_array") @@ -411,15 +411,11 @@ TEST(OtlpRecordable, SetSingleAttribute) nostd::get(str_val).data()); EXPECT_EQ(rec.span().attributes(3).key(), byte_key); - EXPECT_EQ(rec.span().attributes(3).value().array_value().values_size(), 4); - EXPECT_EQ(rec.span().attributes(3).value().array_value().values(0).int_value(), - static_cast('T')); - EXPECT_EQ(rec.span().attributes(3).value().array_value().values(1).int_value(), - static_cast('e')); - EXPECT_EQ(rec.span().attributes(3).value().array_value().values(2).int_value(), - static_cast('s')); - EXPECT_EQ(rec.span().attributes(3).value().array_value().values(3).int_value(), - static_cast('t')); + EXPECT_EQ(rec.span().attributes(3).value().bytes_value().size(), 4u); + EXPECT_EQ(rec.span().attributes(3).value().bytes_value()[0], 'T'); + EXPECT_EQ(rec.span().attributes(3).value().bytes_value()[1], 'e'); + EXPECT_EQ(rec.span().attributes(3).value().bytes_value()[2], 's'); + EXPECT_EQ(rec.span().attributes(3).value().bytes_value()[3], 't'); } // Test non-int array types. Int array types are tested using templates (see IntAttributeTest) @@ -555,7 +551,7 @@ struct EmptyArrayAttributeTest : public testing::Test }; using ArrayElementTypes = - testing::Types; + testing::Types; TYPED_TEST_SUITE(EmptyArrayAttributeTest, ArrayElementTypes); // Test empty arrays. @@ -779,6 +775,45 @@ TEST(OtlpRecordable, PopulateRequestSameScope) EXPECT_EQ(req.resource_spans(0).scope_spans(0).spans_size(), 2); EXPECT_EQ(req.resource_spans(0).scope_spans(0).scope().name(), "lib"); } +// Test that setting an attribute with an existing key overwrites the value in place +// without creating a duplicate entry (spec: attribute keys MUST be unique). +TEST(OtlpRecordable, SetAttributeDeduplicatesKey) +{ + OtlpRecordable rec; + rec.SetAttribute("key", common::AttributeValue{int64_t(1)}); + rec.SetAttribute("key", common::AttributeValue{int64_t(2)}); + + // Only one attribute; the second call must overwrite, not append. + ASSERT_EQ(rec.span().attributes_size(), 1); + EXPECT_EQ(rec.span().attributes(0).key(), "key"); + EXPECT_EQ(rec.span().attributes(0).value().int_value(), 2); +} + +// Test that updating a duplicate key does not consume an attribute slot or increment +// the dropped-attributes counter, even when the attribute limit is already reached. +TEST(OtlpRecordable, SetAttributeDeduplicateDoesNotIncrementDropped) +{ + // Limit to exactly one attribute so the slot is full after the first call. + OtlpRecordable rec(/*max_attributes=*/1); + rec.SetAttribute("duplicate_attribute", common::AttributeValue{int64_t(1)}); + rec.SetAttribute("duplicate_attribute", common::AttributeValue{int64_t(2)}); + + ASSERT_EQ(rec.span().attributes_size(), 1); + EXPECT_EQ(rec.span().attributes(0).value().int_value(), 2); + EXPECT_EQ(rec.span().dropped_attributes_count(), 0u); +} + +// Test that updating a duplicate key changes the type as well as the value. +TEST(OtlpRecordable, SetAttributeDeduplicateChangesType) +{ + OtlpRecordable rec; + rec.SetAttribute("type_change_attribute", common::AttributeValue{int64_t(42)}); + rec.SetAttribute("type_change_attribute", common::AttributeValue{nostd::string_view("hello")}); + + ASSERT_EQ(rec.span().attributes_size(), 1); + EXPECT_EQ(rec.span().attributes(0).value().string_value(), "hello"); +} + } // namespace otlp } // namespace exporter OPENTELEMETRY_END_NAMESPACE From de16c15fbe51ef188e3de052c8216856218addb3 Mon Sep 17 00:00:00 2001 From: Douglas Barker Date: Tue, 16 Jun 2026 19:21:18 -0400 Subject: [PATCH 2/3] fix ci issues --- exporters/otlp/src/otlp_log_recordable.cc | 7 ++++++- exporters/otlp/src/otlp_populate_attribute_utils.cc | 7 +++---- exporters/otlp/test/otlp_populate_attribute_utils_test.cc | 4 +++- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/exporters/otlp/src/otlp_log_recordable.cc b/exporters/otlp/src/otlp_log_recordable.cc index a9b77d1a33..adb2113e39 100644 --- a/exporters/otlp/src/otlp_log_recordable.cc +++ b/exporters/otlp/src/otlp_log_recordable.cc @@ -1,9 +1,12 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 -#include "opentelemetry/exporters/otlp/otlp_log_recordable.h" +#include +#include + #include "opentelemetry/common/attribute_value.h" #include "opentelemetry/common/timestamp.h" +#include "opentelemetry/exporters/otlp/otlp_log_recordable.h" #include "opentelemetry/exporters/otlp/otlp_populate_attribute_utils.h" #include "opentelemetry/logs/severity.h" #include "opentelemetry/nostd/span.h" @@ -18,6 +21,8 @@ // clang-format off #include "opentelemetry/exporters/otlp/protobuf_include_prefix.h" // IWYU pragma: keep +#include +#include "opentelemetry/proto/common/v1/common.pb.h" #include "opentelemetry/proto/logs/v1/logs.pb.h" #include "opentelemetry/exporters/otlp/protobuf_include_suffix.h" // IWYU pragma: keep // clang-format on diff --git a/exporters/otlp/src/otlp_populate_attribute_utils.cc b/exporters/otlp/src/otlp_populate_attribute_utils.cc index c0ea4c199d..15cda481a7 100644 --- a/exporters/otlp/src/otlp_populate_attribute_utils.cc +++ b/exporters/otlp/src/otlp_populate_attribute_utils.cc @@ -15,8 +15,6 @@ #include #include -#include - #include "opentelemetry/common/attribute_value.h" #include "opentelemetry/exporters/otlp/otlp_populate_attribute_utils.h" #include "opentelemetry/nostd/span.h" @@ -30,6 +28,7 @@ // clang-format off #include "opentelemetry/exporters/otlp/protobuf_include_prefix.h" // IWYU pragma: keep +#include // IWYU pragma: no_include "net/proto2/public/repeated_field.h" #include "opentelemetry/proto/common/v1/common.pb.h" #include "opentelemetry/proto/resource/v1/resource.pb.h" @@ -321,7 +320,7 @@ void OtlpPopulateAttributeUtils::PopulateAnyValue( OTEL_INTERNAL_LOG_ERROR( "[OTLP Populate Attribute] bad_variant_access in PopulateAnyValue: " << e.what()); } - catch (const std::bad_alloc &e) + catch (const std::bad_alloc &) // NOLINT(bugprone-empty-catch) { // TODO: Log an error once the logger doesn't require dynamic memory allocation. // OTEL_INTERNAL_LOG_ERROR( @@ -350,7 +349,7 @@ void OtlpPopulateAttributeUtils::PopulateAnyValue( OTEL_INTERNAL_LOG_ERROR( "[OTLP Populate Attribute] bad_variant_access in PopulateAnyValue: " << e.what()); } - catch (const std::bad_alloc &e) + catch (const std::bad_alloc &) // NOLINT(bugprone-empty-catch) { // TODO: Log an error once the logger doesn't require dynamic memory allocation. // OTEL_INTERNAL_LOG_ERROR( diff --git a/exporters/otlp/test/otlp_populate_attribute_utils_test.cc b/exporters/otlp/test/otlp_populate_attribute_utils_test.cc index ef32f17e95..9b9b57f671 100644 --- a/exporters/otlp/test/otlp_populate_attribute_utils_test.cc +++ b/exporters/otlp/test/otlp_populate_attribute_utils_test.cc @@ -26,6 +26,8 @@ namespace exporter namespace otlp { +namespace +{ class PopulateAnyValueTest : public ::testing::Test { protected: @@ -37,7 +39,7 @@ class PopulateAttributeTest : public ::testing::Test protected: opentelemetry::proto::common::v1::KeyValue proto_keyvalue_; }; - +} // namespace // ============================================================ // PopulateAnyValue with common::AttributeValue Tests // ============================================================ From 88e47f670fc3d60fc6210c5fb1e4d0a67d18a9b9 Mon Sep 17 00:00:00 2001 From: Douglas Barker Date: Thu, 18 Jun 2026 13:22:30 -0400 Subject: [PATCH 3/3] use unordered map to track otlp recordable attributes. check for null/empty keys and add test. --- .../exporters/otlp/otlp_log_recordable.h | 3 +++ .../exporters/otlp/otlp_recordable.h | 2 ++ exporters/otlp/src/otlp_log_recordable.cc | 22 ++++++++++----- exporters/otlp/src/otlp_recordable.cc | 27 +++++++++++-------- .../otlp/test/otlp_log_recordable_test.cc | 9 +++++++ exporters/otlp/test/otlp_recordable_test.cc | 10 +++++++ sdk/src/logs/multi_recordable.cc | 4 +++ sdk/src/trace/span.cc | 4 +++ 8 files changed, 63 insertions(+), 18 deletions(-) diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_log_recordable.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_log_recordable.h index efcb0d846b..b5a45b4da3 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_log_recordable.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_log_recordable.h @@ -4,6 +4,7 @@ #pragma once #include +#include #include "opentelemetry/common/attribute_value.h" #include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h" @@ -13,6 +14,7 @@ // clang-format off #include "opentelemetry/exporters/otlp/protobuf_include_prefix.h" // IWYU pragma: keep +#include "opentelemetry/proto/common/v1/common.pb.h" #include "opentelemetry/proto/logs/v1/logs.pb.h" #include "opentelemetry/exporters/otlp/protobuf_include_suffix.h" // IWYU pragma: keep // clang-format on @@ -111,6 +113,7 @@ class OtlpLogRecordable final : public opentelemetry::sdk::logs::Recordable private: proto::logs::v1::LogRecord proto_record_; + std::unordered_map attributes_map_; const opentelemetry::sdk::resource::Resource *resource_ = nullptr; const opentelemetry::sdk::instrumentationscope::InstrumentationScope *instrumentation_scope_ = nullptr; diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_recordable.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_recordable.h index b63bc5ff7d..a60a183f22 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_recordable.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_recordable.h @@ -7,6 +7,7 @@ #include // For std::uint32_t #include // For std::numeric_limits #include +#include // clang-format off #include "opentelemetry/exporters/otlp/protobuf_include_prefix.h" // IWYU pragma: keep @@ -97,6 +98,7 @@ class OtlpRecordable final : public opentelemetry::sdk::trace::Recordable private: proto::trace::v1::Span span_; + std::unordered_map attributes_map_; const opentelemetry::sdk::resource::Resource *resource_ = nullptr; const opentelemetry::sdk::instrumentationscope::InstrumentationScope *instrumentation_scope_ = nullptr; diff --git a/exporters/otlp/src/otlp_log_recordable.cc b/exporters/otlp/src/otlp_log_recordable.cc index adb2113e39..697a2d6c9d 100644 --- a/exporters/otlp/src/otlp_log_recordable.cc +++ b/exporters/otlp/src/otlp_log_recordable.cc @@ -241,15 +241,23 @@ void OtlpLogRecordable::SetTraceFlags(const opentelemetry::trace::TraceFlags &tr void OtlpLogRecordable::SetAttribute(opentelemetry::nostd::string_view key, const opentelemetry::common::AttributeValue &value) noexcept { - for (auto &attr : *proto_record_.mutable_attributes()) + if (key.empty()) { - if (attr.key() == key) - { - OtlpPopulateAttributeUtils::PopulateAttribute(&attr, key, value); - return; - } + return; + } + const std::string key_str(key); + auto it = attributes_map_.find(key_str); + if (it != attributes_map_.end()) + { + // Update an existing attribute value + OtlpPopulateAttributeUtils::PopulateAnyValue(it->second, value); + return; } - OtlpPopulateAttributeUtils::PopulateAttribute(proto_record_.add_attributes(), key, value); + auto *kv = proto_record_.add_attributes(); + kv->set_key(key_str); + auto *any = kv->mutable_value(); + OtlpPopulateAttributeUtils::PopulateAnyValue(any, value); + attributes_map_.emplace(std::move(key_str), any); } void OtlpLogRecordable::SetResource(const opentelemetry::sdk::resource::Resource &resource) noexcept diff --git a/exporters/otlp/src/otlp_recordable.cc b/exporters/otlp/src/otlp_recordable.cc index d90dd661c6..b05b9f379a 100644 --- a/exporters/otlp/src/otlp_recordable.cc +++ b/exporters/otlp/src/otlp_recordable.cc @@ -117,23 +117,28 @@ void OtlpRecordable::SetResource(const sdk::resource::Resource &resource) noexce void OtlpRecordable::SetAttribute(nostd::string_view key, const common::AttributeValue &value) noexcept { - // Check for an existing attribute with the same key and overwrite it (spec requires unique keys). - for (auto &attr : *span_.mutable_attributes()) + if (key.empty()) { - if (attr.key() == key) - { - OtlpPopulateAttributeUtils::PopulateAttribute(&attr, key, value); - return; - } + return; } - - if (static_cast(span_.attributes_size()) >= max_attributes_) + const std::string key_str(key); + auto it = attributes_map_.find(key_str); + if (it != attributes_map_.end()) + { + // Update an existing attribute value + OtlpPopulateAttributeUtils::PopulateAnyValue(it->second, value); + return; + } + if (static_cast(attributes_map_.size()) >= max_attributes_) { span_.set_dropped_attributes_count(span_.dropped_attributes_count() + 1); return; } - - OtlpPopulateAttributeUtils::PopulateAttribute(span_.add_attributes(), key, value); + auto *attribute = span_.add_attributes(); + attribute->set_key(key_str); + auto *any_value = attribute->mutable_value(); + OtlpPopulateAttributeUtils::PopulateAnyValue(any_value, value); + attributes_map_.emplace(std::move(key_str), any_value); } void OtlpRecordable::AddEvent(nostd::string_view name, diff --git a/exporters/otlp/test/otlp_log_recordable_test.cc b/exporters/otlp/test/otlp_log_recordable_test.cc index ca0b62390a..7afdb08d17 100644 --- a/exporters/otlp/test/otlp_log_recordable_test.cc +++ b/exporters/otlp/test/otlp_log_recordable_test.cc @@ -445,6 +445,15 @@ TEST(OtlpLogRecordable, SetAttributeDeduplicateChangesType) EXPECT_EQ(rec.log_record().attributes(0).value().string_value(), "hello"); } +// Test that an empty key is rejected and not stored. +TEST(OtlpLogRecordable, SetAttributeEmptyKeyIsRejected) +{ + OtlpLogRecordable rec; + rec.SetAttribute("", opentelemetry::common::AttributeValue{int64_t(1)}); + + EXPECT_EQ(rec.log_record().attributes_size(), 0); +} + } // namespace otlp } // namespace exporter OPENTELEMETRY_END_NAMESPACE diff --git a/exporters/otlp/test/otlp_recordable_test.cc b/exporters/otlp/test/otlp_recordable_test.cc index 93f1db408d..9c9e8b4a72 100644 --- a/exporters/otlp/test/otlp_recordable_test.cc +++ b/exporters/otlp/test/otlp_recordable_test.cc @@ -814,6 +814,16 @@ TEST(OtlpRecordable, SetAttributeDeduplicateChangesType) EXPECT_EQ(rec.span().attributes(0).value().string_value(), "hello"); } +// Test that an empty key is rejected and not stored. +TEST(OtlpRecordable, SetAttributeEmptyKeyIsRejected) +{ + OtlpRecordable rec; + rec.SetAttribute("", common::AttributeValue{int64_t(1)}); + + EXPECT_EQ(rec.span().attributes_size(), 0); + EXPECT_EQ(rec.span().dropped_attributes_count(), 0u); +} + } // namespace otlp } // namespace exporter OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/src/logs/multi_recordable.cc b/sdk/src/logs/multi_recordable.cc index 1f4f1b8311..6ff34de645 100644 --- a/sdk/src/logs/multi_recordable.cc +++ b/sdk/src/logs/multi_recordable.cc @@ -151,6 +151,10 @@ void MultiRecordable::SetTraceFlags(const opentelemetry::trace::TraceFlags &trac void MultiRecordable::SetAttribute(nostd::string_view key, const opentelemetry::common::AttributeValue &value) noexcept { + if (key.empty()) + { + return; + } for (auto &recordable : recordables_) { if (recordable.second) diff --git a/sdk/src/trace/span.cc b/sdk/src/trace/span.cc index d764540875..5829cf48b4 100644 --- a/sdk/src/trace/span.cc +++ b/sdk/src/trace/span.cc @@ -99,6 +99,10 @@ Span::~Span() void Span::SetAttribute(nostd::string_view key, const common::AttributeValue &value) noexcept { + if (key.empty()) + { + return; + } std::lock_guard lock_guard{mu_}; if (recordable_ == nullptr) {