fix: change value type of Baggage to StringValue (open-telemetry#2729)

Co-authored-by: Cijo Thomas <[email protected]>

Author: gruebel

opentelemetry-sdk/src/propagation/baggage.rs

@@ -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()),
         ]
     }

opentelemetry/CHANGELOG.md

@@ -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
 

opentelemetry/src/baggage.rs

@@ -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;
@@ -56,7 +56,7 @@ fn get_default_baggage() -> &'static Baggage {
 /// [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`
 }
 
@@ -74,30 +74,33 @@ impl Baggage {
     /// # 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)> {
         self.inner.get(key.as_ref())
     }
 
@@ -109,17 +112,17 @@ impl Baggage {
     /// # 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)
@@ -133,22 +136,22 @@ impl Baggage {
     /// # 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());
@@ -161,7 +164,7 @@ impl Baggage {
 
     /// 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)> {
         self.inner.remove(&key.into())
     }
 
@@ -182,12 +185,12 @@ impl Baggage {
 
     /// 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) {
@@ -237,27 +240,27 @@ fn key_value_metadata_bytes_size(key: &str, value: &str, metadata: &str) -> usiz
 
 /// 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 {
         let mut baggage = Baggage::default();
         for (key, (value, metadata)) in iter.into_iter() {
             baggage.insert_with_metadata(key, value, metadata);
@@ -304,7 +307,7 @@ fn encode(s: &str) -> String {
 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)?;
             }
@@ -325,15 +328,15 @@ pub trait BaggageExt {
     /// # 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>>(
@@ -346,13 +349,13 @@ pub trait BaggageExt {
     /// # 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>>(
@@ -364,7 +367,7 @@ pub trait BaggageExt {
     /// # Examples
     ///
     /// ```
-    /// use opentelemetry::{baggage::BaggageExt, Context, KeyValue, Value};
+    /// use opentelemetry::{baggage::BaggageExt, Context};
     ///
     /// let cx = Context::map_current(|cx| cx.with_cleared_baggage());
     ///
@@ -448,7 +451,7 @@ pub struct KeyValueMetadata {
     /// 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,
 }
@@ -458,7 +461,7 @@ impl KeyValueMetadata {
     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 {
@@ -473,7 +476,7 @@ impl From<KeyValue> for KeyValueMetadata {
     fn from(kv: KeyValue) -> Self {
         KeyValueMetadata {
             key: kv.key,
-            value: kv.value,
+            value: kv.value.into(),
             metadata: BaggageMetadata::default(),
         }
     }
@@ -546,7 +549,7 @@ mod tests {
         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"
         )
     }

opentelemetry/src/common.rs

@@ -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(),