refactor(prof): Labels have strs directly, not Option<str>

morrisonlevi · morrisonlevi · commit ec26aa7b079f · 2025-04-15T14:05:31.000-06:00
diff --git a/profiling-ffi/src/profiles/datatypes.rs b/profiling-ffi/src/profiles/datatypes.rs
@@ -319,13 +319,7 @@ impl<'a> TryFrom<&'a Label<'a>> for api::Label<'a> {
     fn try_from(label: &'a Label<'a>) -> Result<Self, Self::Error> {
         let key = label.key.try_to_utf8()?;
         let str = label.str.try_to_utf8()?;
-        let str = if str.is_empty() { None } else { Some(str) };
         let num_unit = label.num_unit.try_to_utf8()?;
-        let num_unit = if num_unit.is_empty() {
-            None
-        } else {
-            Some(num_unit)
-        };
 
         Ok(Self {
             key,
@@ -340,13 +334,7 @@ impl<'a> From<&'a Label<'a>> for api::StringIdLabel {
     fn from(label: &'a Label<'a>) -> Self {
         let key = label.key_id;
         let str = label.str_id;
-        let str = if str.value == 0 { None } else { Some(str) };
         let num_unit = label.num_unit_id;
-        let num_unit = if num_unit.value == 0 {
-            None
-        } else {
-            Some(num_unit)
-        };
 
         Self {
             key,
diff --git a/profiling-ffi/src/profiles/interning_api.rs b/profiling-ffi/src/profiles/interning_api.rs
@@ -70,7 +70,13 @@ pub unsafe extern "C" fn ddog_prof_Profile_intern_label_num(
     key: GenerationalId<StringId>,
     val: i64,
 ) -> Result<GenerationalId<LabelId>> {
-    wrap_with_ffi_result!({ profile_ptr_to_inner(profile)?.intern_label_num(key, val, None) })
+    wrap_with_ffi_result!({
+        profile_ptr_to_inner(profile)?.intern_label_num(
+            key,
+            val,
+            GenerationalId::new_immortal(StringId::ZERO),
+        )
+    })
 }
 
 /// This function interns its argument into the profiler.
@@ -93,7 +99,7 @@ pub unsafe extern "C" fn ddog_prof_Profile_intern_label_num_with_unit(
     val: i64,
     unit: GenerationalId<StringId>,
 ) -> Result<GenerationalId<LabelId>> {
-    wrap_with_ffi_result!({ profile_ptr_to_inner(profile)?.intern_label_num(key, val, Some(unit)) })
+    wrap_with_ffi_result!({ profile_ptr_to_inner(profile)?.intern_label_num(key, val, unit) })
 }
 
 /// This function interns its argument into the profiler.
diff --git a/profiling-replayer/src/replayer.rs b/profiling-replayer/src/replayer.rs
@@ -95,17 +95,9 @@ impl<'pprof> Replayer<'pprof> {
             .map(|label| {
                 Ok(api::Label {
                     key: profile_index.get_string(label.key)?,
-                    str: if label.str == 0 {
-                        None
-                    } else {
-                        Some(profile_index.get_string(label.str)?)
-                    },
+                    str: profile_index.get_string(label.str)?,
                     num: label.num,
-                    num_unit: if label.num_unit == 0 {
-                        None
-                    } else {
-                        Some(profile_index.get_string(label.num_unit)?)
-                    },
+                    num_unit: profile_index.get_string(label.num_unit)?,
                 })
             })
             .collect();
@@ -126,9 +118,9 @@ impl<'pprof> Replayer<'pprof> {
                 "local root span ids of zero do not make sense"
             );
 
-            let endpoint_value = match endpoint_label.str {
-                Some(v) => v,
-                None => anyhow::bail!("expected trace endpoint label value to have a string"),
+            let endpoint_value = endpoint_label.str;
+            if endpoint_value.is_empty() {
+                anyhow::bail!("expected trace endpoint label value to have a string")
             };
 
             endpoint_info.replace((local_root_span_id, endpoint_value));
diff --git a/profiling/src/api.rs b/profiling/src/api.rs
@@ -31,11 +31,11 @@ pub struct ManagedStringId {
 }
 
 impl ManagedStringId {
-    pub fn empty() -> Self {
+    pub const fn empty() -> Self {
         Self::new(0)
     }
 
-    pub fn new(value: u32) -> Self {
+    pub const fn new(value: u32) -> Self {
         ManagedStringId { value }
     }
 }
@@ -130,7 +130,7 @@ pub struct Label<'a> {
     pub key: &'a str,
 
     /// At most one of the following must be present
-    pub str: Option<&'a str>,
+    pub str: &'a str,
     pub num: i64,
 
     /// Should only be present when num is present.
@@ -140,7 +140,7 @@ pub struct Label<'a> {
     /// Consumers may also  interpret units like "bytes" and "kilobytes" as memory
     /// units and units like "seconds" and "nanoseconds" as time units,
     /// and apply appropriate unit conversions to these.
-    pub num_unit: Option<&'a str>,
+    pub num_unit: &'a str,
 }
 
 #[derive(Copy, Clone, Debug, Default, Eq, PartialEq)]
@@ -149,16 +149,16 @@ pub struct StringIdLabel {
     pub key: ManagedStringId,
 
     /// At most one of the following must be present
-    pub str: Option<ManagedStringId>,
+    pub str: ManagedStringId,
     pub num: i64,
 
     /// Should only be present when num is present.
-    pub num_unit: Option<ManagedStringId>,
+    pub num_unit: ManagedStringId,
 }
 
 impl Label<'_> {
     pub fn uses_at_most_one_of_str_and_num(&self) -> bool {
-        self.str.is_none() || (self.num == 0 && self.num_unit.is_none())
+        self.str.is_empty() || (self.num == 0 && self.num_unit.is_empty())
     }
 }
 
@@ -400,17 +400,9 @@ impl<'a> TryFrom<&'a pprof::Profile> for Profile<'a> {
             for label in sample.labels.iter() {
                 labels.push(Label {
                     key: string_table_fetch(pprof, label.key)?,
-                    str: if label.str == 0 {
-                        None
-                    } else {
-                        Some(string_table_fetch(pprof, label.str)?)
-                    },
+                    str: string_table_fetch(pprof, label.str)?,
                     num: label.num,
-                    num_unit: if label.num_unit == 0 {
-                        None
-                    } else {
-                        Some(string_table_fetch(pprof, label.num_unit)?)
-                    },
+                    num_unit: string_table_fetch(pprof, label.num_unit)?,
                 })
             }
             let sample = Sample {
@@ -439,49 +431,49 @@ mod tests {
     fn label_uses_at_most_one_of_str_and_num() {
         let label = Label {
             key: "name",
-            str: Some("levi"),
+            str: "levi",
             num: 0,
-            num_unit: Some("name"), // can't use num_unit with str
+            num_unit: "name", // can't use num_unit with str
         };
         assert!(!label.uses_at_most_one_of_str_and_num());
 
         let label = Label {
             key: "name",
-            str: Some("levi"),
+            str: "levi",
             num: 10, // can't use num with str
-            num_unit: None,
+            num_unit: "",
         };
         assert!(!label.uses_at_most_one_of_str_and_num());
 
         let label = Label {
             key: "name",
-            str: Some("levi"),
+            str: "levi",
             num: 0,
-            num_unit: None,
+            num_unit: "",
         };
         assert!(label.uses_at_most_one_of_str_and_num());
 
         let label = Label {
             key: "process_id",
-            str: None,
+            str: "",
             num: 0,
-            num_unit: None,
+            num_unit: "",
         };
         assert!(label.uses_at_most_one_of_str_and_num());
 
         let label = Label {
             key: "local root span id",
-            str: None,
+            str: "",
             num: 10901,
-            num_unit: None,
+            num_unit: "",
         };
         assert!(label.uses_at_most_one_of_str_and_num());
 
         let label = Label {
             key: "duration",
-            str: None,
+            str: "",
             num: 12345,
-            num_unit: Some("nanoseconds"),
+            num_unit: "nanoseconds",
         };
         assert!(label.uses_at_most_one_of_str_and_num());
     }
diff --git a/profiling/src/internal/label.rs b/profiling/src/internal/label.rs
@@ -6,10 +6,7 @@ use super::*;
 #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, PartialOrd, Ord)]
 pub enum LabelValue {
     Str(StringId),
-    Num {
-        num: i64,
-        num_unit: Option<StringId>,
-    },
+    Num { num: i64, num_unit: StringId },
 }
 
 #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, PartialOrd, Ord)]
@@ -35,7 +32,7 @@ impl Label {
         &self.value
     }
 
-    pub fn num(key: StringId, num: i64, num_unit: Option<StringId>) -> Self {
+    pub fn num(key: StringId, num: i64, num_unit: StringId) -> Self {
         Self {
             key,
             value: LabelValue::Num { num, num_unit },
@@ -70,7 +67,7 @@ impl From<&Label> for pprof::Label {
                 key,
                 str: 0,
                 num,
-                num_unit: num_unit.map(StringId::into_raw_id).unwrap_or_default(),
+                num_unit: num_unit.into_raw_id(),
             },
         }
     }
diff --git a/profiling/src/internal/profile/fuzz_tests.rs b/profiling/src/internal/profile/fuzz_tests.rs
@@ -43,7 +43,7 @@ pub struct Label {
     pub key: Box<str>,
 
     /// At most one of the following must be present
-    pub str: Option<Box<str>>,
+    pub str: Box<str>,
     pub num: i64,
 
     /// Should only be present when num is present.
@@ -53,16 +53,16 @@ pub struct Label {
     /// Consumers may also  interpret units like "bytes" and "kilobytes" as memory
     /// units and units like "seconds" and "nanoseconds" as time units,
     /// and apply appropriate unit conversions to these.
-    pub num_unit: Option<Box<str>>,
+    pub num_unit: Box<str>,
 }
 
 impl<'a> From<&'a Label> for api::Label<'a> {
     fn from(value: &'a Label) -> Self {
         Self {
             key: &value.key,
-            str: value.str.as_deref(),
+            str: &value.str,
             num: value.num,
-            num_unit: value.num_unit.as_deref(),
+            num_unit: &value.num_unit,
         }
     }
 }
@@ -193,10 +193,10 @@ pub struct Sample {
 
 #[cfg(test)]
 impl Sample {
-    /// Checks if the sample is well formed.  Useful in testing.
+    /// Checks if the sample is well-formed.  Useful in testing.
     pub fn is_well_formed(&self) -> bool {
         let labels_are_unique = {
-            let mut uniq = std::collections::HashSet::new();
+            let mut uniq = HashSet::new();
             self.labels.iter().map(|l| &l.key).all(|x| uniq.insert(x))
         };
         labels_are_unique
@@ -280,7 +280,7 @@ fn assert_samples_eq(
                 Default::default()
             };
             // internal::Location::to_pprof() always creates a single line.
-            assert!(location.lines.len() == 1);
+            assert_eq!(1, location.lines.len());
             let line = location.lines[0];
             let function = profile.functions[line.function_id as usize - 1];
             assert!(!location.is_folded);
@@ -333,20 +333,16 @@ fn assert_samples_eq(
                 let str = Box::from(profile.string_table_fetch(label.str).as_str());
                 owned_labels.push(Label {
                     key,
-                    str: Some(str),
+                    str,
                     num: 0,
-                    num_unit: None,
+                    num_unit: Box::from(""),
                 });
             } else {
                 let num = label.num;
-                let num_unit = if label.num_unit != 0 {
-                    Some(profile.string_table_fetch_owned(label.num_unit))
-                } else {
-                    None
-                };
+                let num_unit = profile.string_table_fetch_owned(label.num_unit);
                 owned_labels.push(Label {
                     key,
-                    str: None,
+                    str: Box::from(""),
                     num,
                     num_unit,
                 });
@@ -406,15 +402,15 @@ fn fuzz_failure_001() {
         labels: vec![
             Label {
                 key: Box::from(""),
-                str: Some(Box::from("")),
+                str: Box::from(""),
                 num: 0,
-                num_unit: Some(Box::from("")),
+                num_unit: Box::from(""),
             },
             Label {
                 key: Box::from("local root span id"),
-                str: None,
+                str: Box::from(""),
                 num: 281474976710656,
-                num_unit: None,
+                num_unit: Box::from(""),
             },
         ],
     };
diff --git a/profiling/src/internal/profile/interning_api/mod.rs b/profiling/src/internal/profile/interning_api/mod.rs
@@ -32,10 +32,10 @@ impl Profile {
         &mut self,
         key: GenerationalId<StringId>,
         val: i64,
-        unit: Option<GenerationalId<StringId>>,
+        unit: GenerationalId<StringId>,
     ) -> anyhow::Result<GenerationalId<LabelId>> {
         let key = key.get(self.generation)?;
-        let unit = unit.map(|u| u.get(self.generation)).transpose()?;
+        let unit = unit.get(self.generation)?;
         let id = self.labels.dedup(Label::num(key, val, unit));
         Ok(GenerationalId::new(id, self.generation))
     }
diff --git a/profiling/src/internal/profile/mod.rs b/profiling/src/internal/profile/mod.rs
