Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve cost of creating AttributeSets #1379

Merged
merged 16 commits into from
Nov 22, 2023
24 changes: 7 additions & 17 deletions opentelemetry-sdk/src/attributes/set.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use std::{
cmp::Ordering,
collections::{BTreeSet, HashSet},
hash::{Hash, Hasher},
};

Expand Down Expand Up @@ -105,25 +104,16 @@ impl Eq for HashKeyValue {}
/// This must implement [Hash], [PartialEq], and [Eq] so it may be used as
/// HashMap keys and other de-duplication methods.
#[derive(Clone, Default, Debug, Hash, PartialEq, Eq)]
pub struct AttributeSet(BTreeSet<HashKeyValue>);
pub struct AttributeSet(Vec<HashKeyValue>);

impl From<&[KeyValue]> for AttributeSet {
fn from(values: &[KeyValue]) -> Self {
let mut seen = HashSet::with_capacity(values.len());
AttributeSet(
values
.iter()
.rev()
.filter_map(|kv| {
if seen.contains(&&kv.key) {
None
} else {
seen.insert(&kv.key);
Some(HashKeyValue(kv.clone()))
}
})
.collect(),
)
let mut vec = values
.iter()
.map(|k| HashKeyValue(k.clone()))
.collect::<Vec<_>>();
vec.sort_by(|a, b| a.0.key.cmp(&b.0.key));
AttributeSet(vec)
}
}

Expand Down
4 changes: 3 additions & 1 deletion stress/src/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,16 @@ fn test_counter() {
let index_first_attribute = rng.gen_range(0..len);
let index_second_attribute = rng.gen_range(0..len);
let index_third_attribute = rng.gen_range(0..len);
let index_fourth_attribute = rng.gen_range(0..len);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets not make change to the stress test itself in this PR. We can drive changes to stress test as a separate PR if we believe it should do unsorted order. (I do not believe it should, but want to avoid adding noise to this PR).


// each attribute has 10 possible values, so there are 1000 possible combinations (time-series)
COUNTER.add(
1,
&[
KeyValue::new("attribute1", ATTRIBUTE_VALUES[index_first_attribute]),
KeyValue::new("attribute4", ATTRIBUTE_VALUES[index_fourth_attribute]),
KeyValue::new("attribute2", ATTRIBUTE_VALUES[index_second_attribute]),
KeyValue::new("attribute3", ATTRIBUTE_VALUES[index_third_attribute]),
KeyValue::new("attribute1", ATTRIBUTE_VALUES[index_first_attribute]),
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
],
);
}
Loading