Removes ordered-float dependency, and make own implementation for f64…

… hashing. (open-telemetry#1806)
lalitb · May 22, 2024 · 35f9a60 · 35f9a60
1 parent f82dd14
commit 35f9a60
Show file tree

Hide file tree

Showing 9 changed files with 211 additions and 113 deletions.
diff --git a/opentelemetry-sdk/CHANGELOG.md b/opentelemetry-sdk/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 - Add "metrics", "logs" to default features. With this, default feature list is
   "trace", "metrics" and "logs".
+- Removed dependency on `ordered-float`.
 
 ## v0.23.0
 

diff --git a/opentelemetry-sdk/Cargo.toml b/opentelemetry-sdk/Cargo.toml
@@ -18,7 +18,6 @@ futures-channel = "0.3"
 futures-executor = { workspace = true }
 futures-util = { workspace = true, features = ["std", "sink", "async-await-macro"] }
 once_cell = { workspace = true }
-ordered-float = { workspace = true }
 percent-encoding = { version = "2.0", optional = true }
 rand = { workspace = true, features = ["std", "std_rng","small_rng"], optional = true }
 glob = { version = "0.3.1", optional =true}

diff --git a/opentelemetry-sdk/benches/metric_counter.rs b/opentelemetry-sdk/benches/metric_counter.rs
@@ -54,7 +54,15 @@ fn counter_add(c: &mut Criterion) {
     c.bench_function("Counter_Add_Sorted", |b| {
         b.iter(|| {
             // 4*4*10*10 = 1600 time series.
-            let rands = CURRENT_RNG.with_borrow_mut(|rng| [rng.gen_range(0..4), rng.gen_range(0..4), rng.gen_range(0..10), rng.gen_range(0..10)]);
+            let rands = CURRENT_RNG.with(|rng| {
+                let mut rng = rng.borrow_mut();
+                [
+                    rng.gen_range(0..4),
+                    rng.gen_range(0..4),
+                    rng.gen_range(0..10),
+                    rng.gen_range(0..10),
+                ]
+            });
             let index_first_attribute = rands[0];
             let index_second_attribute = rands[1];
             let index_third_attribute = rands[2];
@@ -74,7 +82,15 @@ fn counter_add(c: &mut Criterion) {
     c.bench_function("Counter_Add_Unsorted", |b| {
         b.iter(|| {
             // 4*4*10*10 = 1600 time series.
-            let rands = CURRENT_RNG.with_borrow_mut(|rng| [rng.gen_range(0..4), rng.gen_range(0..4), rng.gen_range(0..10), rng.gen_range(0..10)]);
+            let rands = CURRENT_RNG.with(|rng| {
+                let mut rng = rng.borrow_mut();
+                [
+                    rng.gen_range(0..4),
+                    rng.gen_range(0..4),
+                    rng.gen_range(0..10),
+                    rng.gen_range(0..10),
+                ]
+            });
             let index_first_attribute = rands[0];
             let index_second_attribute = rands[1];
             let index_third_attribute = rands[2];
@@ -104,7 +120,16 @@ fn counter_add(c: &mut Criterion) {
 
     c.bench_function("ThreadLocal_Random_Generator_5", |b| {
         b.iter(|| {
-            let _i1 = CURRENT_RNG.with_borrow_mut(|rng| [rng.gen_range(0..4),rng.gen_range(0..4),rng.gen_range(0..10), rng.gen_range(0..10), rng.gen_range(0..10)]);
+            let __i1 = CURRENT_RNG.with(|rng| {
+                let mut rng = rng.borrow_mut();
+                [
+                    rng.gen_range(0..4),
+                    rng.gen_range(0..4),
+                    rng.gen_range(0..10),
+                    rng.gen_range(0..10),
+                    rng.gen_range(0..10),
+                ]
+            });
         });
     });
 }

diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs
@@ -66,71 +66,16 @@ pub use view::*;
 
 use std::collections::hash_map::DefaultHasher;
 use std::collections::HashSet;
-use std::{
-    cmp::Ordering,
-    hash::{Hash, Hasher},
-};
+use std::hash::{Hash, Hasher};
 
-use opentelemetry::{Array, Key, KeyValue, Value};
-use ordered_float::OrderedFloat;
-
-#[derive(Clone, Debug)]
-struct HashKeyValue(KeyValue);
-
-impl Hash for HashKeyValue {
-    fn hash<H: Hasher>(&self, state: &mut H) {
-        self.0.key.hash(state);
-        match &self.0.value {
-            Value::F64(f) => OrderedFloat(*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| OrderedFloat(*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 PartialOrd for HashKeyValue {
-    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
-        Some(self.cmp(other))
-    }
-}
-
-impl Ord for HashKeyValue {
-    fn cmp(&self, other: &Self) -> Ordering {
-        self.0.key.cmp(&other.0.key)
-    }
-}
-
-impl PartialEq for HashKeyValue {
-    fn eq(&self, other: &Self) -> bool {
-        self.0.key == other.0.key
-            && match (&self.0.value, &other.0.value) {
-                (Value::F64(f), Value::F64(of)) => OrderedFloat(*f).eq(&OrderedFloat(*of)),
-                (Value::Array(Array::F64(f)), Value::Array(Array::F64(of))) => {
-                    f.len() == of.len()
-                        && f.iter()
-                            .zip(of.iter())
-                            .all(|(f, of)| OrderedFloat(*f).eq(&OrderedFloat(*of)))
-                }
-                (non_float, other_non_float) => non_float.eq(other_non_float),
-            }
-    }
-}
-
-impl Eq for HashKeyValue {}
+use opentelemetry::{Key, KeyValue, Value};
 
 /// A unique set of attributes that can be used as instrument identifiers.
 ///
 /// This must implement [Hash], [PartialEq], and [Eq] so it may be used as
 /// HashMap keys and other de-duplication methods.
 #[derive(Clone, Default, Debug, PartialEq, Eq)]
-pub struct AttributeSet(Vec<HashKeyValue>, u64);
+pub struct AttributeSet(Vec<KeyValue>, u64);
 
 impl From<&[KeyValue]> for AttributeSet {
     fn from(values: &[KeyValue]) -> Self {
@@ -140,7 +85,7 @@ impl From<&[KeyValue]> for AttributeSet {
             .rev()
             .filter_map(|kv| {
                 if seen_keys.insert(kv.key.clone()) {
-                    Some(HashKeyValue(kv.clone()))
+                    Some(kv.clone())
                 } else {
                     None
                 }
@@ -151,7 +96,7 @@ impl From<&[KeyValue]> for AttributeSet {
     }
 }
 
-fn calculate_hash(values: &[HashKeyValue]) -> u64 {
+fn calculate_hash(values: &[KeyValue]) -> u64 {
     let mut hasher = DefaultHasher::new();
     values.iter().fold(&mut hasher, |mut hasher, item| {
         item.hash(&mut hasher);
@@ -161,7 +106,7 @@ fn calculate_hash(values: &[HashKeyValue]) -> u64 {
 }
 
 impl AttributeSet {
-    fn new(mut values: Vec<HashKeyValue>) -> Self {
+    fn new(mut values: Vec<KeyValue>) -> Self {
         values.sort_unstable();
         let hash = calculate_hash(&values);
         AttributeSet(values, hash)
@@ -177,15 +122,15 @@ impl AttributeSet {
     where
         F: Fn(&KeyValue) -> bool,
     {
-        self.0.retain(|kv| f(&kv.0));
+        self.0.retain(|kv| f(kv));
 
         // Recalculate the hash as elements are changed.
         self.1 = calculate_hash(&self.0);
     }
 
     /// Iterate over key value pairs in the set
     pub fn iter(&self) -> impl Iterator<Item = (&Key, &Value)> {
-        self.0.iter().map(|kv| (&kv.0.key, &kv.0.value))
+        self.0.iter().map(|kv| (&kv.key, &kv.value))
     }
 }
 
@@ -209,52 +154,9 @@ mod tests {
         KeyValue,
     };
     use std::borrow::Cow;
-    use std::hash::DefaultHasher;
-    use std::hash::{Hash, Hasher};
 
     // Run all tests in this mod
     // cargo test metrics::tests --features=metrics,testing
-
-    #[test]
-    fn equality_kv_float() {
-        let kv1 = HashKeyValue(KeyValue::new("key", 1.0));
-        let kv2 = HashKeyValue(KeyValue::new("key", 1.0));
-        assert_eq!(kv1, kv2);
-
-        let kv1 = HashKeyValue(KeyValue::new("key", 1.0));
-        let kv2 = HashKeyValue(KeyValue::new("key", 1.01));
-        assert_ne!(kv1, kv2);
-
-        let kv1 = HashKeyValue(KeyValue::new("key", std::f64::NAN));
-        let kv2 = HashKeyValue(KeyValue::new("key", std::f64::NAN));
-        assert_eq!(kv1, kv2);
-
-        let kv1 = HashKeyValue(KeyValue::new("key", std::f64::INFINITY));
-        let kv2 = HashKeyValue(KeyValue::new("key", std::f64::INFINITY));
-        assert_eq!(kv1, kv2);
-    }
-
-    #[test]
-    fn hash_kv_float() {
-        let kv1 = HashKeyValue(KeyValue::new("key", 1.0));
-        let kv2 = HashKeyValue(KeyValue::new("key", 1.0));
-        assert_eq!(hash_helper(&kv1), hash_helper(&kv2));
-
-        let kv1 = HashKeyValue(KeyValue::new("key", std::f64::NAN));
-        let kv2 = HashKeyValue(KeyValue::new("key", std::f64::NAN));
-        assert_eq!(hash_helper(&kv1), hash_helper(&kv2));
-
-        let kv1 = HashKeyValue(KeyValue::new("key", std::f64::INFINITY));
-        let kv2 = HashKeyValue(KeyValue::new("key", std::f64::INFINITY));
-        assert_eq!(hash_helper(&kv1), hash_helper(&kv2));
-    }
-
-    fn hash_helper<T: Hash>(item: &T) -> u64 {
-        let mut hasher = DefaultHasher::new();
-        item.hash(&mut hasher);
-        hasher.finish()
-    }
-
     // Note for all tests from this point onwards in this mod:
     // "multi_thread" tokio flavor must be used else flush won't
     // be able to make progress!

diff --git a/opentelemetry/CHANGELOG.md b/opentelemetry/CHANGELOG.md
@@ -4,6 +4,9 @@
 
 - Add "metrics", "logs" to default features. With this, default feature list is
   "trace", "metrics" and "logs".
+- When "metrics" feature is enabled, `KeyValue` implements `PartialEq`, `Eq`,
+  `PartialOrder`, `Order`, `Hash`. This is meant to be used for metrics
+  aggregation purposes only.
 
 ## v0.23.0
 

diff --git a/opentelemetry/Cargo.toml b/opentelemetry/Cargo.toml
@@ -42,6 +42,7 @@ otel_unstable = []
 [dev-dependencies]
 opentelemetry_sdk = { path = "../opentelemetry-sdk", features = ["logs_level_enabled"]} # for documentation tests
 criterion = { version = "0.3" }
+rand = { workspace = true }
 
 [[bench]]
 name = "metrics"