Handle null span tag values (#1394)

add failing tests

convert null to empty string

handle null value consistently

fixes

clippy

fmt

update comment

Co-authored-by: lucaspimentel <[email protected]>
Co-authored-by: shreya.malpani <[email protected]>

Author: shreyamalpani

libdd-trace-protobuf/build.rs

@@ -93,7 +93,7 @@ fn generate_protobuf() {
     );
     config.field_attribute(
         ".pb.Span.meta",
-        "#[serde(default)] #[serde(deserialize_with = \"crate::deserializers::deserialize_null_into_default\")]",
+        "#[serde(default)] #[serde(deserialize_with = \"crate::deserializers::deserialize_map_with_nullable_values\")]",
     );
     config.field_attribute(
         ".pb.Span.metrics",

libdd-trace-protobuf/src/deserializers.rs

@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use serde::{Deserialize, Deserializer};
+use std::collections::HashMap;
 
 pub fn deserialize_null_into_default<'de, D, T>(deserializer: D) -> Result<T, D::Error>
 where
@@ -12,6 +13,23 @@ where
     Ok(opt.unwrap_or_default())
 }
 
+/// Deserialize a HashMap<String, String> where null values are skipped.
+pub fn deserialize_map_with_nullable_values<'de, D>(
+    deserializer: D,
+) -> Result<HashMap<String, String>, D::Error>
+where
+    D: Deserializer<'de>,
+{
+    let opt: Option<HashMap<String, Option<String>>> = Option::deserialize(deserializer)?;
+    match opt {
+        None => Ok(HashMap::new()),
+        Some(map) => Ok(map
+            .into_iter()
+            .filter_map(|(k, v)| v.map(|val| (k, val)))
+            .collect()),
+    }
+}
+
 pub fn is_default<T: Default + PartialEq>(t: &T) -> bool {
     t == &T::default()
 }

libdd-trace-protobuf/src/pb.rs

@@ -301,7 +301,9 @@ pub struct Span {
     /// @gotags: json:"meta,omitempty" msg:"meta,omitempty"
     #[prost(map = "string, string", tag = "10")]
     #[serde(default)]
-    #[serde(deserialize_with = "crate::deserializers::deserialize_null_into_default")]
+    #[serde(
+        deserialize_with = "crate::deserializers::deserialize_map_with_nullable_values"
+    )]
     pub meta: ::std::collections::HashMap<
         ::prost::alloc::string::String,
         ::prost::alloc::string::String,

libdd-trace-utils/src/msgpack_decoder/decode/string.rs

@@ -56,6 +56,7 @@ pub fn read_nullable_string<'a>(buf: &mut &'a [u8]) -> Result<&'a str, DecodeErr
 /// # Errors
 /// Fails if the buffer does not contain a valid map length prefix,
 /// or if any key or value is not a valid utf8 msgpack string.
+/// Null values are skipped (key not inserted into map).
 #[inline]
 pub fn read_str_map_to_strings<'a>(
     buf: &mut &'a [u8],
@@ -67,8 +68,11 @@ pub fn read_str_map_to_strings<'a>(
     let mut map = HashMap::with_capacity(len.try_into().expect("Unable to cast map len to usize"));
     for _ in 0..len {
         let key = read_string_ref(buf)?;
-        let value = read_string_ref(buf)?;
-        map.insert(key, value);
+        // Only insert if value is not null
+        if !handle_null_marker(buf) {
+            let value = read_string_ref(buf)?;
+            map.insert(key, value);
+        }
     }
     Ok(map)
 }
@@ -78,6 +82,7 @@ pub fn read_str_map_to_strings<'a>(
 /// # Errors
 /// Fails if the buffer does not contain a valid map length prefix,
 /// or if any key or value is not a valid utf8 msgpack string.
+/// Null values are skipped (key not inserted into map).
 #[inline]
 pub fn read_nullable_str_map_to_strings<'a>(
     buf: &mut &'a [u8],

libdd-trace-utils/src/msgpack_decoder/v04/mod.rs

@@ -492,6 +492,39 @@ mod tests {
         assert!(decoded_span.span_links.is_empty());
     }
 
+    #[test]
+    fn test_decoder_meta_with_null_values() {
+        let mut span = create_test_json_span(1, 2, 0, 0, false);
+        span["meta"] = json!({
+            "key1": "value1",
+            "key2": null,  // This null value should be skipped
+            "key3": "value3"
+        });
+
+        let encoded_data = rmp_serde::to_vec_named(&vec![vec![span]]).unwrap();
+        let (decoded_traces, _) =
+            from_bytes(libdd_tinybytes::Bytes::from(encoded_data)).expect("Decoding failed");
+
+        assert_eq!(1, decoded_traces.len());
+        assert_eq!(1, decoded_traces[0].len());
+        let decoded_span = &decoded_traces[0][0];
+
+        assert_eq!(
+            "value1",
+            decoded_span.meta[&BytesString::from_slice("key1".as_ref()).unwrap()].as_str()
+        );
+        assert!(
+            !decoded_span
+                .meta
+                .contains_key(&BytesString::from_slice("key2".as_ref()).unwrap()),
+            "Null value should be skipped, but key was present"
+        );
+        assert_eq!(
+            "value3",
+            decoded_span.meta[&BytesString::from_slice("key3".as_ref()).unwrap()].as_str()
+        );
+    }
+
     #[test]
     fn test_decoder_read_string_wrong_format() {
         let span = SpanBytes {

libdd-trace-utils/src/trace_utils.rs

@@ -1197,4 +1197,55 @@ mod tests {
             1.0
         );
     }
+
+    #[test]
+    fn test_rmp_serde_deserialize_meta_with_null_values() {
+        // Create a JSON representation with null value in meta
+        let span_json = json!({
+            "service": "test-service",
+            "name": "test_name",
+            "resource": "test-resource",
+            "trace_id": 1_u64,
+            "span_id": 2_u64,
+            "parent_id": 0_u64,
+            "start": 0_i64,
+            "duration": 5_i64,
+            "error": 0_i32,
+            "meta": {
+                "service": "test-service",
+                "env": "test-env",
+                "runtime-id": "test-runtime-id-value",
+                "problematic_key": null  // Ensure this null value does not cause an error
+            },
+            "metrics": {},
+            "type": "",
+            "meta_struct": {},
+            "span_links": [],
+            "span_events": []
+        });
+
+        let traces_json = vec![vec![span_json]];
+        let encoded_data = rmp_serde::to_vec(&traces_json).unwrap();
+        let traces: Vec<Vec<pb::Span>> = rmp_serde::from_read(&encoded_data[..])
+            .expect("Failed to deserialize traces with null values in meta");
+
+        assert_eq!(1, traces.len());
+        assert_eq!(1, traces[0].len());
+        let decoded_span = &traces[0][0];
+
+        assert_eq!("test-service", decoded_span.service);
+        assert_eq!("test_name", decoded_span.name);
+        assert_eq!("test-resource", decoded_span.resource);
+        assert_eq!("test-service", decoded_span.meta.get("service").unwrap());
+        assert_eq!("test-env", decoded_span.meta.get("env").unwrap());
+        assert_eq!(
+            "test-runtime-id-value",
+            decoded_span.meta.get("runtime-id").unwrap()
+        );
+        // Assert that the null value was filtered out (key not present in map)
+        assert!(
+            !decoded_span.meta.contains_key("problematic_key"),
+            "Null value should be skipped, but key was present"
+        );
+    }
 }