From cc7cd67d520006f994b964807a6e369f77b279ca Mon Sep 17 00:00:00 2001 From: Mindaugas Vinkelis Date: Sat, 1 Mar 2025 22:34:53 +0200 Subject: [PATCH 1/2] perf: better Baggage insert and BaggageExt::with_baggage performance & updated constants to the latest standard --- opentelemetry/CHANGELOG.md | 1 + opentelemetry/src/baggage.rs | 133 ++++++++++++----------------------- 2 files changed, 45 insertions(+), 89 deletions(-) diff --git a/opentelemetry/CHANGELOG.md b/opentelemetry/CHANGELOG.md index 58b21d09e2..5ea43ff87a 100644 --- a/opentelemetry/CHANGELOG.md +++ b/opentelemetry/CHANGELOG.md @@ -8,6 +8,7 @@ - {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. - *Breaking* Changed value type of `Baggage` from `Value` to `StringValue` +- Updated `Baggage` constants to reflect latest standard (`MAX_KEY_VALUE_PAIRS` - 180 -> 64, `MAX_BYTES_FOR_ONE_PAIR` - removed) and increased insert performance see #[2284](https://github.com/open-telemetry/opentelemetry-rust/pull/2284). ## 0.28.0 diff --git a/opentelemetry/src/baggage.rs b/opentelemetry/src/baggage.rs index bb96b711f5..37785e8962 100644 --- a/opentelemetry/src/baggage.rs +++ b/opentelemetry/src/baggage.rs @@ -15,14 +15,14 @@ //! //! [W3C Baggage]: https://w3c.github.io/baggage use crate::{Context, Key, KeyValue, StringValue}; +use std::collections::hash_map::Entry; use std::collections::{hash_map, HashMap}; use std::fmt; use std::sync::OnceLock; static DEFAULT_BAGGAGE: OnceLock = OnceLock::new(); -const MAX_KEY_VALUE_PAIRS: usize = 180; -const MAX_BYTES_FOR_ONE_PAIR: usize = 4096; +const MAX_KEY_VALUE_PAIRS: usize = 64; const MAX_LEN_OF_ALL_PAIRS: usize = 8192; /// Returns the default baggage, ensuring it is initialized only once. @@ -49,11 +49,10 @@ fn get_default_baggage() -> &'static Baggage { /// /// ### Limits /// -/// * Maximum number of name/value pairs: `180`. -/// * Maximum number of bytes per a single name/value pair: `4096`. +/// * Maximum number of name/value pairs: `64`. /// * Maximum total length of all name/value pairs: `8192`. /// -/// [RFC2616, Section 2.2]: https://tools.ietf.org/html/rfc2616#section-2.2 +/// #[derive(Debug, Default)] pub struct Baggage { inner: HashMap, @@ -133,6 +132,8 @@ impl Baggage { /// Same with `insert`, if the name was not present, [`None`] will be returned. /// If the name is present, the old value and metadata will be returned. /// + /// Also checks for [limits](https://w3c.github.io/baggage/#limits). + /// /// # Examples /// /// ``` @@ -155,10 +156,38 @@ impl Baggage { S: Into, { let (key, value, metadata) = (key.into(), value.into(), metadata.into()); - if self.insertable(&key, &value, &metadata) { - self.inner.insert(key, (value, metadata)) - } else { - None + if !key.as_str().is_ascii() { + return None; + } + let entry_content_len = + key_value_metadata_bytes_size(key.as_str(), value.as_str(), metadata.as_str()); + let entries_count = self.inner.len(); + match self.inner.entry(key) { + Entry::Occupied(mut occupied_entry) => { + let prev_content_len = key_value_metadata_bytes_size( + occupied_entry.key().as_str(), + occupied_entry.get().0.as_str(), + occupied_entry.get().1.as_str(), + ); + let new_content_len = self.kv_content_len + entry_content_len - prev_content_len; + if new_content_len > MAX_LEN_OF_ALL_PAIRS { + return None; + } + self.kv_content_len = new_content_len; + Some(occupied_entry.insert((value, metadata))) + } + Entry::Vacant(vacant_entry) => { + if entries_count == MAX_KEY_VALUE_PAIRS { + return None; + } + let new_content_len = self.kv_content_len + entry_content_len; + if new_content_len > MAX_LEN_OF_ALL_PAIRS { + return None; + } + self.kv_content_len = new_content_len; + vacant_entry.insert((value, metadata)); + None + } } } @@ -178,59 +207,10 @@ impl Baggage { self.inner.is_empty() } - /// Gets an iterator over the baggage items, sorted by name. + /// Gets an iterator over the baggage items, in any order. pub fn iter(&self) -> Iter<'_> { self.into_iter() } - - /// Determine whether the key value pair exceed one of the [limits](https://w3c.github.io/baggage/#limits). - /// If not, update the total length of key values - fn insertable(&mut self, key: &Key, value: &StringValue, metadata: &BaggageMetadata) -> bool { - if !key.as_str().is_ascii() { - return false; - } - let value = value.as_str(); - if key_value_metadata_bytes_size(key.as_str(), value, metadata.as_str()) - < MAX_BYTES_FOR_ONE_PAIR - { - match self.inner.get(key) { - None => { - // check total length - if self.kv_content_len - + metadata.as_str().len() - + value.len() - + key.as_str().len() - > MAX_LEN_OF_ALL_PAIRS - { - return false; - } - // check number of pairs - if self.inner.len() + 1 > MAX_KEY_VALUE_PAIRS { - return false; - } - self.kv_content_len += - metadata.as_str().len() + value.len() + key.as_str().len() - } - Some((old_value, old_metadata)) => { - let old_value = old_value.as_str(); - if self.kv_content_len - old_metadata.as_str().len() - old_value.len() - + metadata.as_str().len() - + value.len() - > MAX_LEN_OF_ALL_PAIRS - { - return false; - } - self.kv_content_len = - self.kv_content_len - old_metadata.as_str().len() - old_value.len() - + metadata.as_str().len() - + value.len() - } - } - true - } else { - false - } - } } /// Get the number of bytes for one key-value pair @@ -385,13 +365,11 @@ impl BaggageExt for Context { &self, baggage: T, ) -> Self { - let mut merged: Baggage = self - .baggage() - .iter() - .map(|(key, (value, metadata))| { - KeyValueMetadata::new(key.clone(), value.clone(), metadata.clone()) - }) - .collect(); + let old = self.baggage(); + let mut merged = Baggage { + inner: old.inner.clone(), + kv_content_len: old.kv_content_len, + }; for kvm in baggage.into_iter().map(|kv| kv.into()) { merged.insert_with_metadata(kvm.key, kvm.value, kvm.metadata); } @@ -531,29 +509,6 @@ mod tests { assert_eq!(baggage.len(), MAX_KEY_VALUE_PAIRS) } - #[test] - fn insert_too_long_pair() { - let pair = KeyValue::new( - "test", - String::from_utf8_lossy(vec![12u8; MAX_BYTES_FOR_ONE_PAIR].as_slice()).to_string(), - ); - let mut baggage = Baggage::default(); - baggage.insert(pair.key.clone(), pair.value.clone()); - assert_eq!( - baggage.len(), - 0, - "The input pair is too long to insert into baggage" - ); - - baggage.insert("test", "value"); - baggage.insert(pair.key.clone(), pair.value); - assert_eq!( - baggage.get(pair.key), - Some(&StringValue::from("value")), - "If the input pair is too long, then don't replace entry with same key" - ) - } - #[test] fn insert_pairs_length_exceed() { let mut data = vec![]; From 520154377ccc1a0b91143f953ccb937f42572753 Mon Sep 17 00:00:00 2001 From: Mindaugas Vinkelis Date: Sat, 1 Mar 2025 23:55:42 +0200 Subject: [PATCH 2/2] added test --- opentelemetry/src/baggage.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/opentelemetry/src/baggage.rs b/opentelemetry/src/baggage.rs index 37785e8962..9521a53898 100644 --- a/opentelemetry/src/baggage.rs +++ b/opentelemetry/src/baggage.rs @@ -566,4 +566,22 @@ mod tests { assert!(b.to_string().contains("bar=2;yellow")); assert!(b.to_string().contains("foo=1;red;state=on")); } + + #[test] + fn replace_existing_key() { + let half_minus2: StringValue = (0..MAX_LEN_OF_ALL_PAIRS / 2 - 2) + .map(|_| 'x') + .collect::() + .into(); + + let mut b = Baggage::default(); + b.insert("a", half_minus2.clone()); // +1 for key + b.insert("b", half_minus2); // +1 for key + b.insert("c", StringValue::from(".")); // total of 2 bytes + assert!(b.get("a").is_some()); + assert!(b.get("b").is_some()); + assert!(b.get("c").is_some()); + assert!(b.insert("c", StringValue::from("..")).is_none()); // exceeds MAX_LEN_OF_ALL_PAIRS + assert_eq!(b.insert("c", StringValue::from("!")).unwrap(), ".".into()); // replaces existing + } }