From 8e9d21694c8feb20f2af0bf070e9c94e77ecd455 Mon Sep 17 00:00:00 2001 From: Mindaugas Vinkelis Date: Wed, 13 Nov 2024 07:23:15 +0200 Subject: [PATCH] Use DefaultHasher --- opentelemetry-sdk/Cargo.toml | 1 - .../src/metrics/internal/hashed.rs | 21 ++----- opentelemetry-sdk/src/metrics/internal/mod.rs | 58 +------------------ opentelemetry-sdk/src/metrics/mod.rs | 35 ++++++----- 4 files changed, 26 insertions(+), 89 deletions(-) diff --git a/opentelemetry-sdk/Cargo.toml b/opentelemetry-sdk/Cargo.toml index 8a7d648afd..39928fecb3 100644 --- a/opentelemetry-sdk/Cargo.toml +++ b/opentelemetry-sdk/Cargo.toml @@ -29,7 +29,6 @@ tokio = { workspace = true, features = ["rt", "time"], optional = true } tokio-stream = { workspace = true, optional = true } http = { workspace = true, optional = true } tracing = {workspace = true, optional = true} -rustc-hash = "2.0.0" [package.metadata.docs.rs] all-features = true diff --git a/opentelemetry-sdk/src/metrics/internal/hashed.rs b/opentelemetry-sdk/src/metrics/internal/hashed.rs index 387cc7444a..d66d8f898e 100644 --- a/opentelemetry-sdk/src/metrics/internal/hashed.rs +++ b/opentelemetry-sdk/src/metrics/internal/hashed.rs @@ -1,11 +1,9 @@ use std::{ borrow::{Borrow, Cow}, - hash::{BuildHasher, Hash, Hasher}, + hash::{BuildHasher, DefaultHasher, Hash, Hasher}, ops::Deref, }; -use rustc_hash::*; - /// Hash value only once, works with references and owned types. pub(crate) struct Hashed<'a, T> where @@ -20,11 +18,10 @@ where T: ToOwned + Hash + ?Sized, { pub(crate) fn from_borrowed(value: &'a T) -> Self { - let mut hasher = FxHasher::default(); - value.hash(&mut hasher); + let hash = calc_hash(&value); Self { value: Cow::Borrowed(value), - hash: hasher.finish(), + hash, } } @@ -36,16 +33,6 @@ where } } - pub(crate) fn mutate(self, f: impl FnOnce(&mut ::Owned)) -> Hashed<'static, T> { - let mut value = self.value.into_owned(); - f(&mut value); - let hash = calc_hash(value.borrow()); - Hashed { - value: Cow::Owned(value), - hash, - } - } - pub(crate) fn into_owned(self) -> Hashed<'static, T> { let value = self.value.into_owned(); Hashed { @@ -63,7 +50,7 @@ fn calc_hash(value: T) -> u64 where T: Hash, { - let mut hasher = FxHasher::default(); + let mut hasher = DefaultHasher::default(); value.hash(&mut hasher); hasher.finish() } diff --git a/opentelemetry-sdk/src/metrics/internal/mod.rs b/opentelemetry-sdk/src/metrics/internal/mod.rs index e20c615fb6..6f5ac6acae 100644 --- a/opentelemetry-sdk/src/metrics/internal/mod.rs +++ b/opentelemetry-sdk/src/metrics/internal/mod.rs @@ -20,6 +20,8 @@ use hashed::{Hashed, HashedNoOpBuilder}; use once_cell::sync::Lazy; use opentelemetry::{otel_warn, KeyValue}; +use super::sort_and_dedup; + pub(crate) static STREAM_OVERFLOW_ATTRIBUTES: Lazy> = Lazy::new(|| Hashed::from_owned(vec![KeyValue::new("otel.metric.overflow", "true")])); @@ -97,11 +99,7 @@ where } // Try to retrieve and update the tracker with the attributes sorted. - let sorted_attrs = attributes.clone().mutate(|list| { - // use stable sort - list.sort_by(|a, b| a.key.cmp(&b.key)); - dedup_remove_first(list, |a, b| a.key == b.key); - }); + let sorted_attrs = Hashed::from_owned(sort_and_dedup(&attributes)); if let Some(tracker) = trackers.get(&sorted_attrs) { tracker.update(value); return; @@ -198,20 +196,6 @@ where } } -fn dedup_remove_first(values: &mut Vec, is_eq: impl Fn(&T, &T) -> bool) { - // we cannot use vec.dedup_by because it will remove last duplicate not first - if values.len() > 1 { - let mut i = values.len() - 1; - while i != 0 { - let is_same = unsafe { is_eq(values.get_unchecked(i - 1), values.get_unchecked(i)) }; - if is_same { - values.remove(i - 1); - } - i -= 1; - } - } -} - /// Clear and allocate exactly required amount of space for all attribute-sets fn prepare_data(data: &mut Vec, list_len: usize) { data.clear(); @@ -415,45 +399,9 @@ impl AtomicallyUpdate for f64 { #[cfg(test)] mod tests { - use std::usize; use super::*; - fn assert_deduped( - input: [(i32, bool); N], - expect: [(i32, bool); M], - ) { - let mut list: Vec<(i32, bool)> = Vec::from(input); - dedup_remove_first(&mut list, |a, b| a.0 == b.0); - assert_eq!(list, expect); - } - - #[test] - fn deduplicate_by_removing_first_element_from_sorted_array() { - assert_deduped([], []); - assert_deduped([(1, true)], [(1, true)]); - assert_deduped([(1, false), (1, false), (1, true)], [(1, true)]); - assert_deduped( - [(1, true), (2, false), (2, false), (2, true)], - [(1, true), (2, true)], - ); - assert_deduped( - [(1, true), (1, false), (1, true), (2, true)], - [(1, true), (2, true)], - ); - assert_deduped( - [ - (1, false), - (1, true), - (2, false), - (2, true), - (3, false), - (3, true), - ], - [(1, true), (2, true), (3, true)], - ); - } - #[test] fn can_store_u64_atomic_value() { let atomic = u64::new_atomic_tracker(0); diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs index ca70ccfe24..54a606243b 100644 --- a/opentelemetry-sdk/src/metrics/mod.rs +++ b/opentelemetry-sdk/src/metrics/mod.rs @@ -115,23 +115,27 @@ pub(crate) struct AttributeSet(Vec, u64); impl From<&[KeyValue]> for AttributeSet { fn from(values: &[KeyValue]) -> Self { - let mut seen_keys = HashSet::with_capacity(values.len()); - let vec = values - .iter() - .rev() - .filter_map(|kv| { - if seen_keys.insert(kv.key.clone()) { - Some(kv.clone()) - } else { - None - } - }) - .collect::>(); - - AttributeSet::new(vec) + AttributeSet::new(sort_and_dedup(values)) } } +pub(crate) fn sort_and_dedup(values: &[KeyValue]) -> Vec { + let mut seen_keys = HashSet::with_capacity(values.len()); + let mut vec = values + .iter() + .rev() + .filter_map(|kv| { + if seen_keys.insert(kv.key.clone()) { + Some(kv.clone()) + } else { + None + } + }) + .collect::>(); + vec.sort_unstable_by(|a, b| a.key.cmp(&b.key)); + vec +} + fn calculate_hash(values: &[KeyValue]) -> u64 { let mut hasher = DefaultHasher::new(); values.iter().fold(&mut hasher, |mut hasher, item| { @@ -142,8 +146,7 @@ fn calculate_hash(values: &[KeyValue]) -> u64 { } impl AttributeSet { - fn new(mut values: Vec) -> Self { - values.sort_unstable_by(|a, b| a.key.cmp(&b.key)); + fn new(values: Vec) -> Self { let hash = calculate_hash(&values); AttributeSet(values, hash) }