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

fix: change value type of Baggage to StringValue #2729

Merged
merged 2 commits into from
Mar 1, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions opentelemetry-sdk/src/propagation/baggage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,34 +166,34 @@ mod tests {
use std::collections::HashMap;

#[rustfmt::skip]
fn valid_extract_data() -> Vec<(&'static str, HashMap<Key, Value>)> {
fn valid_extract_data() -> Vec<(&'static str, HashMap<Key, StringValue>)> {
vec![
// "valid w3cHeader"
("key1=val1,key2=val2", vec![(Key::new("key1"), Value::from("val1")), (Key::new("key2"), Value::from("val2"))].into_iter().collect()),
("key1=val1,key2=val2", vec![(Key::new("key1"), StringValue::from("val1")), (Key::new("key2"), StringValue::from("val2"))].into_iter().collect()),
// "valid w3cHeader with spaces"
("key1 = val1, key2 =val2 ", vec![(Key::new("key1"), Value::from("val1")), (Key::new("key2"), Value::from("val2"))].into_iter().collect()),
("key1 = val1, key2 =val2 ", vec![(Key::new("key1"), StringValue::from("val1")), (Key::new("key2"), StringValue::from("val2"))].into_iter().collect()),
// "valid header with url-escaped comma"
("key1=val1,key2=val2%2Cval3", vec![(Key::new("key1"), Value::from("val1")), (Key::new("key2"), Value::from("val2,val3"))].into_iter().collect()),
("key1=val1,key2=val2%2Cval3", vec![(Key::new("key1"), StringValue::from("val1")), (Key::new("key2"), StringValue::from("val2,val3"))].into_iter().collect()),
// "valid header with an invalid header"
("key1=val1,key2=val2,a,val3", vec![(Key::new("key1"), Value::from("val1")), (Key::new("key2"), Value::from("val2"))].into_iter().collect()),
("key1=val1,key2=val2,a,val3", vec![(Key::new("key1"), StringValue::from("val1")), (Key::new("key2"), StringValue::from("val2"))].into_iter().collect()),
// "valid header with no value"
("key1=,key2=val2", vec![(Key::new("key1"), Value::from("")), (Key::new("key2"), Value::from("val2"))].into_iter().collect()),
("key1=,key2=val2", vec![(Key::new("key1"), StringValue::from("")), (Key::new("key2"), StringValue::from("val2"))].into_iter().collect()),
]
}

#[rustfmt::skip]
#[allow(clippy::type_complexity)]
fn valid_extract_data_with_metadata() -> Vec<(&'static str, HashMap<Key, (Value, BaggageMetadata)>)> {
fn valid_extract_data_with_metadata() -> Vec<(&'static str, HashMap<Key, (StringValue, BaggageMetadata)>)> {
vec![
// "valid w3cHeader with properties"
("key1=val1,key2=val2;prop=1", vec![(Key::new("key1"), (Value::from("val1"), BaggageMetadata::default())), (Key::new("key2"), (Value::from("val2"), BaggageMetadata::from("prop=1")))].into_iter().collect()),
("key1=val1,key2=val2;prop=1", vec![(Key::new("key1"), (StringValue::from("val1"), BaggageMetadata::default())), (Key::new("key2"), (StringValue::from("val2"), BaggageMetadata::from("prop=1")))].into_iter().collect()),
// prop can don't need to be key value pair
("key1=val1,key2=val2;prop1", vec![(Key::new("key1"), (Value::from("val1"), BaggageMetadata::default())), (Key::new("key2"), (Value::from("val2"), BaggageMetadata::from("prop1")))].into_iter().collect()),
("key1=val1,key2=val2;prop1", vec![(Key::new("key1"), (StringValue::from("val1"), BaggageMetadata::default())), (Key::new("key2"), (StringValue::from("val2"), BaggageMetadata::from("prop1")))].into_iter().collect()),
("key1=value1;property1;property2, key2 = value2, key3=value3; propertyKey=propertyValue",
vec![
(Key::new("key1"), (Value::from("value1"), BaggageMetadata::from("property1;property2"))),
(Key::new("key2"), (Value::from("value2"), BaggageMetadata::default())),
(Key::new("key3"), (Value::from("value3"), BaggageMetadata::from("propertyKey=propertyValue"))),
(Key::new("key1"), (StringValue::from("value1"), BaggageMetadata::from("property1;property2"))),
(Key::new("key2"), (StringValue::from("value2"), BaggageMetadata::default())),
(Key::new("key3"), (StringValue::from("value3"), BaggageMetadata::from("propertyKey=propertyValue"))),
].into_iter().collect()),
]
}
Expand Down
1 change: 1 addition & 0 deletions opentelemetry/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- *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.
- *Breaking* Changed value type of `Baggage` from `Value` to `StringValue`

## 0.28.0

Expand Down
71 changes: 37 additions & 34 deletions opentelemetry/src/baggage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
//! accordance with the [W3C Baggage] specification.
//!
//! [W3C Baggage]: https://w3c.github.io/baggage
use crate::{Context, Key, KeyValue, Value};
use crate::{Context, Key, KeyValue, StringValue};
use std::collections::{hash_map, HashMap};
use std::fmt;
use std::sync::OnceLock;
Expand Down Expand Up @@ -56,7 +56,7 @@
/// [RFC2616, Section 2.2]: https://tools.ietf.org/html/rfc2616#section-2.2
#[derive(Debug, Default)]
pub struct Baggage {
inner: HashMap<Key, (Value, BaggageMetadata)>,
inner: HashMap<Key, (StringValue, BaggageMetadata)>,
kv_content_len: usize, // the length of key-value-metadata string in `inner`
}

Expand All @@ -74,30 +74,33 @@
/// # Examples
///
/// ```
/// use opentelemetry::{baggage::Baggage, Value};
/// use opentelemetry::{baggage::Baggage, StringValue};
///
/// let mut cc = Baggage::new();
/// let _ = cc.insert("my-name", "my-value");
///
/// assert_eq!(cc.get("my-name"), Some(&Value::from("my-value")))
/// assert_eq!(cc.get("my-name"), Some(&StringValue::from("my-value")))
/// ```
pub fn get<K: AsRef<str>>(&self, key: K) -> Option<&Value> {
pub fn get<K: AsRef<str>>(&self, key: K) -> Option<&StringValue> {
self.inner.get(key.as_ref()).map(|(value, _metadata)| value)
}

/// Returns a reference to the value and metadata associated with a given name
///
/// # Examples
/// ```
/// use opentelemetry::{baggage::{Baggage, BaggageMetadata}, Value};
/// use opentelemetry::{baggage::{Baggage, BaggageMetadata}, StringValue};
///
/// let mut cc = Baggage::new();
/// let _ = cc.insert("my-name", "my-value");
///
/// // By default, the metadata is empty
/// assert_eq!(cc.get_with_metadata("my-name"), Some(&(Value::from("my-value"), BaggageMetadata::from(""))))
/// assert_eq!(cc.get_with_metadata("my-name"), Some(&(StringValue::from("my-value"), BaggageMetadata::from(""))))
/// ```
pub fn get_with_metadata<K: AsRef<str>>(&self, key: K) -> Option<&(Value, BaggageMetadata)> {
pub fn get_with_metadata<K: AsRef<str>>(
&self,
key: K,
) -> Option<&(StringValue, BaggageMetadata)> {

Check warning on line 103 in opentelemetry/src/baggage.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry/src/baggage.rs#L100-L103

Added lines #L100 - L103 were not covered by tests
self.inner.get(key.as_ref())
}

Expand All @@ -109,17 +112,17 @@
/// # Examples
///
/// ```
/// use opentelemetry::{baggage::Baggage, Value};
/// use opentelemetry::{baggage::Baggage, StringValue};
///
/// let mut cc = Baggage::new();
/// let _ = cc.insert("my-name", "my-value");
///
/// assert_eq!(cc.get("my-name"), Some(&Value::from("my-value")))
/// assert_eq!(cc.get("my-name"), Some(&StringValue::from("my-value")))
/// ```
pub fn insert<K, V>(&mut self, key: K, value: V) -> Option<Value>
pub fn insert<K, V>(&mut self, key: K, value: V) -> Option<StringValue>
where
K: Into<Key>,
V: Into<Value>,
V: Into<StringValue>,
{
self.insert_with_metadata(key, value, BaggageMetadata::default())
.map(|pair| pair.0)
Expand All @@ -133,22 +136,22 @@
/// # Examples
///
/// ```
/// use opentelemetry::{baggage::{Baggage, BaggageMetadata}, Value};
/// use opentelemetry::{baggage::{Baggage, BaggageMetadata}, StringValue};
///
/// let mut cc = Baggage::new();
/// let _ = cc.insert_with_metadata("my-name", "my-value", "test");
///
/// assert_eq!(cc.get_with_metadata("my-name"), Some(&(Value::from("my-value"), BaggageMetadata::from("test"))))
/// assert_eq!(cc.get_with_metadata("my-name"), Some(&(StringValue::from("my-value"), BaggageMetadata::from("test"))))
/// ```
pub fn insert_with_metadata<K, V, S>(
&mut self,
key: K,
value: V,
metadata: S,
) -> Option<(Value, BaggageMetadata)>
) -> Option<(StringValue, BaggageMetadata)>
where
K: Into<Key>,
V: Into<Value>,
V: Into<StringValue>,
S: Into<BaggageMetadata>,
{
let (key, value, metadata) = (key.into(), value.into(), metadata.into());
Expand All @@ -161,7 +164,7 @@

/// Removes a name from the baggage, returning the value
/// corresponding to the name if the pair was previously in the map.
pub fn remove<K: Into<Key>>(&mut self, key: K) -> Option<(Value, BaggageMetadata)> {
pub fn remove<K: Into<Key>>(&mut self, key: K) -> Option<(StringValue, BaggageMetadata)> {

Check warning on line 167 in opentelemetry/src/baggage.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry/src/baggage.rs#L167

Added line #L167 was not covered by tests
self.inner.remove(&key.into())
}

Expand All @@ -182,12 +185,12 @@

/// 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: &Value, metadata: &BaggageMetadata) -> bool {
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.as_ref(), metadata.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) {
Expand Down Expand Up @@ -237,27 +240,27 @@

/// An iterator over the entries of a [`Baggage`].
#[derive(Debug)]
pub struct Iter<'a>(hash_map::Iter<'a, Key, (Value, BaggageMetadata)>);
pub struct Iter<'a>(hash_map::Iter<'a, Key, (StringValue, BaggageMetadata)>);

impl<'a> Iterator for Iter<'a> {
type Item = (&'a Key, &'a (Value, BaggageMetadata));
type Item = (&'a Key, &'a (StringValue, BaggageMetadata));

fn next(&mut self) -> Option<Self::Item> {
self.0.next()
}
}

impl<'a> IntoIterator for &'a Baggage {
type Item = (&'a Key, &'a (Value, BaggageMetadata));
type Item = (&'a Key, &'a (StringValue, BaggageMetadata));
type IntoIter = Iter<'a>;

fn into_iter(self) -> Self::IntoIter {
Iter(self.inner.iter())
}
}

impl FromIterator<(Key, (Value, BaggageMetadata))> for Baggage {
fn from_iter<I: IntoIterator<Item = (Key, (Value, BaggageMetadata))>>(iter: I) -> Self {
impl FromIterator<(Key, (StringValue, BaggageMetadata))> for Baggage {
fn from_iter<I: IntoIterator<Item = (Key, (StringValue, BaggageMetadata))>>(iter: I) -> Self {

Check warning on line 263 in opentelemetry/src/baggage.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry/src/baggage.rs#L263

Added line #L263 was not covered by tests
let mut baggage = Baggage::default();
for (key, (value, metadata)) in iter.into_iter() {
baggage.insert_with_metadata(key, value, metadata);
Expand Down Expand Up @@ -304,7 +307,7 @@
impl fmt::Display for Baggage {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
for (i, (k, v)) in self.into_iter().enumerate() {
write!(f, "{}={}", k, encode(v.0.as_str().as_ref()))?;
write!(f, "{}={}", k, encode(v.0.as_str()))?;
if !v.1.as_str().is_empty() {
write!(f, ";{}", v.1)?;
}
Expand All @@ -325,15 +328,15 @@
/// # Examples
///
/// ```
/// use opentelemetry::{baggage::BaggageExt, Context, KeyValue, Value};
/// use opentelemetry::{baggage::BaggageExt, Context, KeyValue, StringValue};
///
/// let cx = Context::map_current(|cx| {
/// cx.with_baggage(vec![KeyValue::new("my-name", "my-value")])
/// });
///
/// assert_eq!(
/// cx.baggage().get("my-name"),
/// Some(&Value::from("my-value")),
/// Some(&StringValue::from("my-value")),
/// )
/// ```
fn with_baggage<T: IntoIterator<Item = I>, I: Into<KeyValueMetadata>>(
Expand All @@ -346,13 +349,13 @@
/// # Examples
///
/// ```
/// use opentelemetry::{baggage::BaggageExt, Context, KeyValue, Value};
/// use opentelemetry::{baggage::BaggageExt, Context, KeyValue, StringValue};
///
/// let cx = Context::current_with_baggage(vec![KeyValue::new("my-name", "my-value")]);
///
/// assert_eq!(
/// cx.baggage().get("my-name"),
/// Some(&Value::from("my-value")),
/// Some(&StringValue::from("my-value")),
/// )
/// ```
fn current_with_baggage<T: IntoIterator<Item = I>, I: Into<KeyValueMetadata>>(
Expand All @@ -364,7 +367,7 @@
/// # Examples
///
/// ```
/// use opentelemetry::{baggage::BaggageExt, Context, KeyValue, Value};
/// use opentelemetry::{baggage::BaggageExt, Context};
///
/// let cx = Context::map_current(|cx| cx.with_cleared_baggage());
///
Expand Down Expand Up @@ -448,7 +451,7 @@
/// Dimension or event key
pub key: Key,
/// Dimension or event value
pub value: Value,
pub value: StringValue,
/// Metadata associate with this key value pair
pub metadata: BaggageMetadata,
}
Expand All @@ -458,7 +461,7 @@
pub fn new<K, V, S>(key: K, value: V, metadata: S) -> Self
where
K: Into<Key>,
V: Into<Value>,
V: Into<StringValue>,
S: Into<BaggageMetadata>,
{
KeyValueMetadata {
Expand All @@ -473,7 +476,7 @@
fn from(kv: KeyValue) -> Self {
KeyValueMetadata {
key: kv.key,
value: kv.value,
value: kv.value.into(),
metadata: BaggageMetadata::default(),
}
}
Expand Down Expand Up @@ -546,7 +549,7 @@
baggage.insert(pair.key.clone(), pair.value);
assert_eq!(
baggage.get(pair.key),
Some(&Value::from("value")),
Some(&StringValue::from("value")),
"If the input pair is too long, then don't replace entry with same key"
)
}
Expand Down
14 changes: 13 additions & 1 deletion opentelemetry/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -302,10 +302,22 @@ impl From<Cow<'static, str>> for StringValue {
}
}

impl From<Value> for StringValue {
fn from(s: Value) -> Self {
match s {
Value::Bool(v) => format!("{}", v).into(),
Value::I64(v) => format!("{}", v).into(),
Value::F64(v) => format!("{}", v).into(),
Value::String(v) => v,
Value::Array(v) => format!("{}", v).into(),
}
}
}

impl Value {
/// String representation of the `Value`
///
/// This will allocate iff the underlying value is not a `String`.
/// This will allocate if the underlying value is not a `String`.
pub fn as_str(&self) -> Cow<'_, str> {
match self {
Value::Bool(v) => format!("{}", v).into(),
Expand Down