[crashtracker] Continue symbolicating after errors (#991)

Author: danielsn

bin_tests/tests/crashtracker_bin_test.rs

@@ -329,7 +329,7 @@ fn assert_telemetry_message(crash_telemetry: &[u8], crash_typ: &str) {
 
     let base_expected_tags: std::collections::HashSet<&str> =
         std::collections::HashSet::from_iter([
-            "data_schema_version:1.2",
+            "data_schema_version:1.3",
             "incomplete:false",
             "is_crash:true",
             "profiler_collecting_sample:1",

crashtracker/src/crash_info/error_data.rs

@@ -19,27 +19,47 @@ pub struct ErrorData {
 #[cfg(unix)]
 impl ErrorData {
     pub fn normalize_ips(&mut self, pid: u32) -> anyhow::Result<()> {
+        let mut errors = 0;
         let normalizer = blazesym::normalize::Normalizer::new();
         let pid = pid.into();
-        // TODO, should we continue after error or just exit?
-        self.stack.normalize_ips(&normalizer, pid)?;
+        self.stack
+            .normalize_ips(&normalizer, pid)
+            .unwrap_or_else(|_| errors += 1);
+
         for thread in &mut self.threads {
-            thread.stack.normalize_ips(&normalizer, pid)?;
+            thread
+                .stack
+                .normalize_ips(&normalizer, pid)
+                .unwrap_or_else(|_| errors += 1);
         }
+        anyhow::ensure!(
+            errors == 0,
+            "Failed to normalize ips, see frame comments for details"
+        );
         Ok(())
     }
 
     pub fn resolve_names(&mut self, pid: u32) -> anyhow::Result<()> {
+        let mut errors = 0;
         let mut process = blazesym::symbolize::Process::new(pid.into());
         // https://github.com/libbpf/blazesym/issues/518
         process.map_files = false;
         let src = blazesym::symbolize::Source::Process(process);
         let symbolizer = blazesym::symbolize::Symbolizer::new();
-        self.stack.resolve_names(&src, &symbolizer)?;
+        self.stack
+            .resolve_names(&src, &symbolizer)
+            .unwrap_or_else(|_| errors += 1);
 
         for thread in &mut self.threads {
-            thread.stack.resolve_names(&src, &symbolizer)?;
+            thread
+                .stack
+                .resolve_names(&src, &symbolizer)
+                .unwrap_or_else(|_| errors += 1);
         }
+        anyhow::ensure!(
+            errors == 0,
+            "Failed to resolve names, see frame comments for details"
+        );
         Ok(())
     }
 }

crashtracker/src/crash_info/mod.rs

@@ -62,7 +62,7 @@ pub struct CrashInfo {
 
 impl CrashInfo {
     pub fn current_schema_version() -> String {
-        "1.2".to_string()
+        "1.3".to_string()
     }
 }
 
@@ -123,13 +123,23 @@ impl CrashInfo {
 
 #[cfg(test)]
 mod tests {
+    use schemars::schema::RootSchema;
+    use std::fs;
+
     use super::*;
-    #[ignore]
     #[test]
-    /// Utility function to print the schema.
-    fn print_schema() {
+    fn test_schema_matches_rfc() {
+        let rfc_schema_filename = concat!(
+            env!("CARGO_MANIFEST_DIR"),
+            "/../docs/RFCs/artifacts/0008-crashtracker-schema.json"
+        );
+        let rfc_schema_json = fs::read_to_string(rfc_schema_filename).expect("File to exist");
+        let rfc_schema: RootSchema = serde_json::from_str(&rfc_schema_json).expect("Valid json");
         let schema = schemars::schema_for!(CrashInfo);
-        println!("{}", serde_json::to_string_pretty(&schema).unwrap());
+
+        assert_eq!(rfc_schema, schema);
+        // If it doesn't match, you can use this command to generate a new schema json
+        // println!("{}", serde_json::to_string_pretty(&schema).unwrap());
     }
 
     impl test_utils::TestInstance for CrashInfo {

crashtracker/src/crash_info/stacktrace.rs

@@ -74,18 +74,26 @@ impl StackTrace {
 #[cfg(unix)]
 impl StackTrace {
     pub fn normalize_ips(&mut self, normalizer: &Normalizer, pid: Pid) -> anyhow::Result<()> {
+        let mut errors = 0;
         for frame in &mut self.frames {
-            // TODO: Should this keep going on failure, and report at the end?
-            frame.normalize_ip(normalizer, pid)?;
+            frame.normalize_ip(normalizer, pid).unwrap_or_else(|e| {
+                frame.comments.push(e.to_string());
+                errors += 1;
+            });
         }
+        anyhow::ensure!(errors == 0);
         Ok(())
     }
 
     pub fn resolve_names(&mut self, src: &Source, symbolizer: &Symbolizer) -> anyhow::Result<()> {
+        let mut errors = 0;
         for frame in &mut self.frames {
-            // TODO: Should this keep going on failure, and report at the end?
-            frame.resolve_names(src, symbolizer)?;
+            frame.resolve_names(src, symbolizer).unwrap_or_else(|e| {
+                frame.comments.push(e.to_string());
+                errors += 1;
+            });
         }
+        anyhow::ensure!(errors == 0);
         Ok(())
     }
 }
@@ -129,6 +137,10 @@ pub struct StackFrame {
     pub function: Option<String>,
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub line: Option<u32>,
+
+    // Additional Info
+    #[serde(default, skip_serializing_if = "Vec::is_empty")]
+    pub comments: Vec<String>,
 }
 
 impl StackFrame {
@@ -242,6 +254,8 @@ impl super::test_utils::TestInstance for StackFrame {
         let file = Some(format!("banana{seed}.rs"));
         let function = Some(format!("Bar::baz{seed}"));
         let line = Some((2 * seed + 1) as u32);
+
+        let comments = vec![format!("This is a comment on frame {seed}")];
         Self {
             ip,
             module_base_address,
@@ -256,6 +270,7 @@ impl super::test_utils::TestInstance for StackFrame {
             file,
             function,
             line,
+            comments,
         }
     }
 }

crashtracker/src/crash_info/telemetry.rs

@@ -278,7 +278,7 @@ mod tests {
         assert_eq!(
             HashSet::from_iter([
                 "collecting_sample:1",
-                "data_schema_version:1.2",
+                "data_schema_version:1.3",
                 "incomplete:true",
                 "is_crash:true",
                 "not_profiling:0",

crashtracker/src/receiver/entry_points.rs

@@ -133,8 +133,12 @@ fn resolve_frames(
             .as_ref()
             .context("Unable to resolve frames: No PID specified")?
             .pid;
-        crash_info.resolve_names(pid)?;
-        crash_info.normalize_ips(pid)?;
+        let rval1 = crash_info.resolve_names(pid);
+        let rval2 = crash_info.normalize_ips(pid);
+        anyhow::ensure!(
+            rval1.is_ok() && rval2.is_ok(),
+            "resolve_names: {rval1:?}\tnormalize_ips: {rval2:?}"
+        );
     }
     Ok(())
 }

docs/RFCs/0008-crashtracker-stackframe-comments.md

@@ -0,0 +1,37 @@
+# RFC 0008: Crashtracker Structured Log Format (Version 1.3). Adds stackframe comments.
+
+The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in [IETF RFC 2119](https://datatracker.ietf.org/doc/html/rfc2119).
+
+## Summary
+
+This document describes version 1.3 of the crashinfo data format.
+
+## Motivation
+
+The `libdatadog` crashtracker detects program crashes.
+It automatically collects information relevant to the characterizing and debugging the crash, including stack-traces, the crash-type (e.g. SIGSIGV, SIGBUS, etc) crash, the library version, etc.
+In some cases, a crashtracker collector may have information related to stackframes that does not fit in the current schema.
+For example, if a stackframe failed to symbolicate, the crashtracker implementation may wish to record the reason for the failure to allow debugging.
+
+## Proposed format
+
+The format is an extension of the [1.0 json schema](0005-crashtracker-structured-log-format.md), with the following changes.
+The updated schema is given in Appendix A.
+Any field not listed as "Required" is optional.
+Consumers MUST accept json with elided optional fields.
+
+### Fields
+
+- `data_schema_version`: **[required]** \*\*[UPDATED]
+  A string containing the semver ID of the crashtracker data schema ("1.3" for the current version).
+
+### Stackframes
+
+A stackframe consists of all the fields specified in [1.0 json schema](0005-crashtracker-structured-log-format.md), with the additional
+
+- `comments`: **[optional]** **[NEW]**
+  An array of Strings, containing comments about the given stackframe.
+
+## Appendix A: Json Schema
+
+[Available here](artifacts/0006-crashtracker-schema.json)