From efd6ee4597f34a1b969daa74f0aac3e9c75527dc Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Fri, 21 Feb 2025 10:55:09 -0800 Subject: [PATCH 1/2] InstrumentationScope to include attributes in hash and eq check --- .../src/metrics/meter_provider.rs | 19 +++++ opentelemetry-sdk/src/metrics/mod.rs | 4 +- opentelemetry/CHANGELOG.md | 1 + opentelemetry/src/common.rs | 83 ++++++++++++++++++- 4 files changed, 102 insertions(+), 5 deletions(-) diff --git a/opentelemetry-sdk/src/metrics/meter_provider.rs b/opentelemetry-sdk/src/metrics/meter_provider.rs index 5474316235..2c76dba3a6 100644 --- a/opentelemetry-sdk/src/metrics/meter_provider.rs +++ b/opentelemetry-sdk/src/metrics/meter_provider.rs @@ -562,6 +562,25 @@ mod tests { assert_eq!(provider.inner.meters.lock().unwrap().len(), 5); } + #[test] + fn same_meter_reused_same_scope_attributes() { + let meter_provider = super::SdkMeterProvider::builder().build(); + let make_scope = |attributes| { + InstrumentationScope::builder("test.meter") + .with_version("v0.1.0") + .with_schema_url("http://example.com") + .with_attributes(attributes) + .build() + }; + + let _meter1 = + meter_provider.meter_with_scope(make_scope(vec![KeyValue::new("key", "value1")])); + let _meter2 = + meter_provider.meter_with_scope(make_scope(vec![KeyValue::new("key", "value1")])); + + assert_eq!(meter_provider.inner.meters.lock().unwrap().len(), 1); + } + #[test] fn with_resource_multiple_calls_ensure_additive() { let builder = SdkMeterProvider::builder() diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs index a3f690a606..25a7389d92 100644 --- a/opentelemetry-sdk/src/metrics/mod.rs +++ b/opentelemetry-sdk/src/metrics/mod.rs @@ -782,7 +782,7 @@ mod tests { .build(); // Act - // Meters are identical except for scope attributes, but scope attributes are not an identifying property. + // Meters are identical. // Hence there should be a single metric stream output for this test. let make_scope = |attributes| { InstrumentationScope::builder("test.meter") @@ -795,7 +795,7 @@ mod tests { let meter1 = meter_provider.meter_with_scope(make_scope(vec![KeyValue::new("key", "value1")])); let meter2 = - meter_provider.meter_with_scope(make_scope(vec![KeyValue::new("key", "value2")])); + meter_provider.meter_with_scope(make_scope(vec![KeyValue::new("key", "value1")])); let counter1 = meter1 .u64_counter("my_counter") diff --git a/opentelemetry/CHANGELOG.md b/opentelemetry/CHANGELOG.md index 91b3089982..73b8932ace 100644 --- a/opentelemetry/CHANGELOG.md +++ b/opentelemetry/CHANGELOG.md @@ -6,6 +6,7 @@ - *Breaking* Moved `TraceError` enum from `opentelemetry::trace::TraceError` to `opentelemetry_sdk::trace::TraceError` - *Breaking* Moved `TraceResult` type alias from `opentelemetry::trace::TraceResult` to `opentelemetry_sdk::trace::TraceResult` - {PLACEHOLDER} - Remove the above completely. // TODO fill this when changes are actually in. +- Bug Fix: `InstrumentationScope` implementation for `PartialEq` and `Hash` fixed to include Attributes also. ## 0.28.0 diff --git a/opentelemetry/src/common.rs b/opentelemetry/src/common.rs index 85fa6e7f9d..72f9846fc6 100644 --- a/opentelemetry/src/common.rs +++ b/opentelemetry/src/common.rs @@ -427,22 +427,33 @@ pub struct InstrumentationScope { attributes: Vec, } -// Uniqueness for InstrumentationScope does not depend on attributes -impl Eq for InstrumentationScope {} - impl PartialEq for InstrumentationScope { fn eq(&self, other: &Self) -> bool { self.name == other.name && self.version == other.version && self.schema_url == other.schema_url + && { + let mut self_attrs = self.attributes.clone(); + let mut other_attrs = other.attributes.clone(); + self_attrs.sort_unstable_by(|a, b| a.key.cmp(&b.key)); + other_attrs.sort_unstable_by(|a, b| a.key.cmp(&b.key)); + self_attrs == other_attrs + } } } +impl Eq for InstrumentationScope {} + impl hash::Hash for InstrumentationScope { fn hash(&self, state: &mut H) { self.name.hash(state); self.version.hash(state); self.schema_url.hash(state); + let mut sorted_attrs = self.attributes.clone(); + sorted_attrs.sort_unstable_by(|a, b| a.key.cmp(&b.key)); + for attribute in sorted_attrs { + attribute.hash(state); + } } } @@ -561,3 +572,69 @@ impl InstrumentationScopeBuilder { } } } + +#[cfg(test)] +mod tests { + use std::hash::{Hash, Hasher}; + + use crate::{InstrumentationScope, KeyValue}; + + #[test] + fn instrumentation_scope_equality() { + let scope1 = InstrumentationScope::builder("my-crate") + .with_version("v0.1.0") + .with_schema_url("https://opentelemetry.io/schemas/1.17.0") + .with_attributes([KeyValue::new("k", "v")]) + .build(); + let scope2 = InstrumentationScope::builder("my-crate") + .with_version("v0.1.0") + .with_schema_url("https://opentelemetry.io/schemas/1.17.0") + .with_attributes([KeyValue::new("k", "v")]) + .build(); + assert_eq!(scope1, scope2); + } + + #[test] + fn instrumentation_scope_equality_attributes_diff_order() { + let scope1 = InstrumentationScope::builder("my-crate") + .with_version("v0.1.0") + .with_schema_url("https://opentelemetry.io/schemas/1.17.0") + .with_attributes([KeyValue::new("k1", "v1"), KeyValue::new("k2", "v2")]) + .build(); + let scope2 = InstrumentationScope::builder("my-crate") + .with_version("v0.1.0") + .with_schema_url("https://opentelemetry.io/schemas/1.17.0") + .with_attributes([KeyValue::new("k2", "v2"), KeyValue::new("k1", "v1")]) + .build(); + assert_eq!(scope1, scope2); + + // assert hash are same for both scopes + let mut hasher1 = std::collections::hash_map::DefaultHasher::new(); + scope1.hash(&mut hasher1); + let mut hasher2 = std::collections::hash_map::DefaultHasher::new(); + scope2.hash(&mut hasher2); + assert_eq!(hasher1.finish(), hasher2.finish()); + } + + #[test] + fn instrumentation_scope_equality_different_attributes() { + let scope1 = InstrumentationScope::builder("my-crate") + .with_version("v0.1.0") + .with_schema_url("https://opentelemetry.io/schemas/1.17.0") + .with_attributes([KeyValue::new("k1", "v1"), KeyValue::new("k2", "v2")]) + .build(); + let scope2 = InstrumentationScope::builder("my-crate") + .with_version("v0.1.0") + .with_schema_url("https://opentelemetry.io/schemas/1.17.0") + .with_attributes([KeyValue::new("k2", "v3"), KeyValue::new("k4", "v5")]) + .build(); + assert_ne!(scope1, scope2); + + // assert hash are same for both scopes + let mut hasher1 = std::collections::hash_map::DefaultHasher::new(); + scope1.hash(&mut hasher1); + let mut hasher2 = std::collections::hash_map::DefaultHasher::new(); + scope2.hash(&mut hasher2); + assert_ne!(hasher1.finish(), hasher2.finish()); + } +} From 9c107d2eba165fed6d108ea96c4449caa700f588 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Fri, 21 Feb 2025 11:17:46 -0800 Subject: [PATCH 2/2] move keyvalue eq and hash to common --- opentelemetry/src/common.rs | 109 +++++++++++++++++++++++++++++ opentelemetry/src/metrics/mod.rs | 116 ------------------------------- 2 files changed, 109 insertions(+), 116 deletions(-) diff --git a/opentelemetry/src/common.rs b/opentelemetry/src/common.rs index 72f9846fc6..37622ed5b7 100644 --- a/opentelemetry/src/common.rs +++ b/opentelemetry/src/common.rs @@ -2,6 +2,8 @@ use std::borrow::{Borrow, Cow}; use std::sync::Arc; use std::{fmt, hash}; +use std::hash::{Hash, Hasher}; + /// The key part of attribute [KeyValue] pairs. /// /// See the [attribute naming] spec for guidelines. @@ -399,6 +401,42 @@ impl KeyValue { } } +struct F64Hashable(f64); + +impl PartialEq for F64Hashable { + fn eq(&self, other: &Self) -> bool { + self.0.to_bits() == other.0.to_bits() + } +} + +impl Eq for F64Hashable {} + +impl Hash for F64Hashable { + fn hash(&self, state: &mut H) { + self.0.to_bits().hash(state); + } +} + +impl Hash for KeyValue { + fn hash(&self, state: &mut H) { + self.key.hash(state); + match &self.value { + Value::F64(f) => F64Hashable(*f).hash(state), + Value::Array(a) => match a { + Array::Bool(b) => b.hash(state), + Array::I64(i) => i.hash(state), + Array::F64(f) => f.iter().for_each(|f| F64Hashable(*f).hash(state)), + Array::String(s) => s.hash(state), + }, + Value::Bool(b) => b.hash(state), + Value::I64(i) => i.hash(state), + Value::String(s) => s.hash(state), + }; + } +} + +impl Eq for KeyValue {} + /// Information about a library or crate providing instrumentation. /// /// An instrumentation scope should be named to follow any naming conventions @@ -579,6 +617,77 @@ mod tests { use crate::{InstrumentationScope, KeyValue}; + use rand::random; + use std::collections::hash_map::DefaultHasher; + use std::f64; + + #[test] + fn kv_float_equality() { + let kv1 = KeyValue::new("key", 1.0); + let kv2 = KeyValue::new("key", 1.0); + assert_eq!(kv1, kv2); + + let kv1 = KeyValue::new("key", 1.0); + let kv2 = KeyValue::new("key", 1.01); + assert_ne!(kv1, kv2); + + let kv1 = KeyValue::new("key", f64::NAN); + let kv2 = KeyValue::new("key", f64::NAN); + assert_ne!(kv1, kv2, "NAN is not equal to itself"); + + for float_val in [ + f64::INFINITY, + f64::NEG_INFINITY, + f64::MAX, + f64::MIN, + f64::MIN_POSITIVE, + ] + .iter() + { + let kv1 = KeyValue::new("key", *float_val); + let kv2 = KeyValue::new("key", *float_val); + assert_eq!(kv1, kv2); + } + + for _ in 0..100 { + let random_value = random::(); + let kv1 = KeyValue::new("key", random_value); + let kv2 = KeyValue::new("key", random_value); + assert_eq!(kv1, kv2); + } + } + + #[test] + fn kv_float_hash() { + for float_val in [ + f64::NAN, + f64::INFINITY, + f64::NEG_INFINITY, + f64::MAX, + f64::MIN, + f64::MIN_POSITIVE, + ] + .iter() + { + let kv1 = KeyValue::new("key", *float_val); + let kv2 = KeyValue::new("key", *float_val); + assert_eq!(hash_helper(&kv1), hash_helper(&kv2)); + } + + for _ in 0..100 { + let random_value = random::(); + let kv1 = KeyValue::new("key", random_value); + let kv2 = KeyValue::new("key", random_value); + assert_eq!(hash_helper(&kv1), hash_helper(&kv2)); + } + } + + fn hash_helper(item: &T) -> u64 { + let mut hasher = DefaultHasher::new(); + item.hash(&mut hasher); + hasher.finish() + } + #[test] fn instrumentation_scope_equality() { let scope1 = InstrumentationScope::builder("my-crate") diff --git a/opentelemetry/src/metrics/mod.rs b/opentelemetry/src/metrics/mod.rs index e971f34e42..385e3fbd9b 100644 --- a/opentelemetry/src/metrics/mod.rs +++ b/opentelemetry/src/metrics/mod.rs @@ -1,13 +1,10 @@ //! # OpenTelemetry Metrics API -use std::hash::{Hash, Hasher}; use std::sync::Arc; mod instruments; mod meter; pub(crate) mod noop; - -use crate::{Array, KeyValue, Value}; pub use instruments::{ counter::{Counter, ObservableCounter}, gauge::{Gauge, ObservableGauge}, @@ -18,42 +15,6 @@ pub use instruments::{ }; pub use meter::{Meter, MeterProvider}; -struct F64Hashable(f64); - -impl PartialEq for F64Hashable { - fn eq(&self, other: &Self) -> bool { - self.0.to_bits() == other.0.to_bits() - } -} - -impl Eq for F64Hashable {} - -impl Hash for F64Hashable { - fn hash(&self, state: &mut H) { - self.0.to_bits().hash(state); - } -} - -impl Hash for KeyValue { - fn hash(&self, state: &mut H) { - self.key.hash(state); - match &self.value { - Value::F64(f) => F64Hashable(*f).hash(state), - Value::Array(a) => match a { - Array::Bool(b) => b.hash(state), - Array::I64(i) => i.hash(state), - Array::F64(f) => f.iter().for_each(|f| F64Hashable(*f).hash(state)), - Array::String(s) => s.hash(state), - }, - Value::Bool(b) => b.hash(state), - Value::I64(i) => i.hash(state), - Value::String(s) => s.hash(state), - }; - } -} - -impl Eq for KeyValue {} - /// SDK implemented trait for creating instruments pub trait InstrumentProvider { /// creates an instrument for recording increasing values. @@ -163,80 +124,3 @@ pub trait InstrumentProvider { Histogram::new(Arc::new(noop::NoopSyncInstrument::new())) } } - -#[cfg(test)] -mod tests { - use rand::random; - - use crate::KeyValue; - use std::collections::hash_map::DefaultHasher; - use std::f64; - use std::hash::{Hash, Hasher}; - - #[test] - fn kv_float_equality() { - let kv1 = KeyValue::new("key", 1.0); - let kv2 = KeyValue::new("key", 1.0); - assert_eq!(kv1, kv2); - - let kv1 = KeyValue::new("key", 1.0); - let kv2 = KeyValue::new("key", 1.01); - assert_ne!(kv1, kv2); - - let kv1 = KeyValue::new("key", f64::NAN); - let kv2 = KeyValue::new("key", f64::NAN); - assert_ne!(kv1, kv2, "NAN is not equal to itself"); - - for float_val in [ - f64::INFINITY, - f64::NEG_INFINITY, - f64::MAX, - f64::MIN, - f64::MIN_POSITIVE, - ] - .iter() - { - let kv1 = KeyValue::new("key", *float_val); - let kv2 = KeyValue::new("key", *float_val); - assert_eq!(kv1, kv2); - } - - for _ in 0..100 { - let random_value = random::(); - let kv1 = KeyValue::new("key", random_value); - let kv2 = KeyValue::new("key", random_value); - assert_eq!(kv1, kv2); - } - } - - #[test] - fn kv_float_hash() { - for float_val in [ - f64::NAN, - f64::INFINITY, - f64::NEG_INFINITY, - f64::MAX, - f64::MIN, - f64::MIN_POSITIVE, - ] - .iter() - { - let kv1 = KeyValue::new("key", *float_val); - let kv2 = KeyValue::new("key", *float_val); - assert_eq!(hash_helper(&kv1), hash_helper(&kv2)); - } - - for _ in 0..100 { - let random_value = random::(); - let kv1 = KeyValue::new("key", random_value); - let kv2 = KeyValue::new("key", random_value); - assert_eq!(hash_helper(&kv1), hash_helper(&kv2)); - } - } - - fn hash_helper(item: &T) -> u64 { - let mut hasher = DefaultHasher::new(); - item.hash(&mut hasher); - hasher.finish() - } -}