ref(metrics): Refactor JVM & JS symbolication stats (#1565)

This counts both symbolicated and unsymbolicated frames by platform. Also, for JVM, it properly counts symbolicated and unsymbolicated exceptions and classes.
getsentry · Dec 5, 2024 · 2f2f70e · 2f2f70e
1 parent 4b9ab28
commit 2f2f70e
Show file tree

Hide file tree

Showing 4 changed files with 133 additions and 94 deletions.
diff --git a/crates/symbolicator-js/src/metrics.rs b/crates/symbolicator-js/src/metrics.rs
@@ -31,7 +31,7 @@ use std::collections::HashMap;
 use symbolic::debuginfo::sourcebundle::SourceFileType;
 use symbolicator_service::{metric, metrics, types::Platform};
 
-use crate::interface::{JsStacktrace, ResolvedWith};
+use crate::interface::ResolvedWith;
 
 /// Various metrics we want to capture *per-event* for JS events.
 #[derive(Debug, Default)]
@@ -223,38 +223,39 @@ impl JsMetrics {
 }
 
 /// Record metrics about stacktraces and frames.
-pub fn record_stacktrace_metrics(
-    event_platform: Option<Platform>,
-    stacktraces: &[JsStacktrace],
-    unsymbolicated_frames: u64,
-    missing_sourcescontent: u64,
-) {
+pub fn record_stacktrace_metrics(event_platform: Option<Platform>, stats: SymbolicationStats) {
     let event_platform = event_platform
         .as_ref()
         .map(|p| p.as_ref())
         .unwrap_or("none");
 
-    metric!(time_raw("symbolication.num_stacktraces") = stacktraces.len() as u64);
+    metric!(time_raw("symbolication.num_stacktraces") = stats.num_stacktraces);
 
-    // Count number of frames by platform (including no platform)
-    let frames_by_platform = stacktraces.iter().flat_map(|st| st.frames.iter()).fold(
-        HashMap::new(),
-        |mut map, frame| {
-            let platform = frame.platform.as_ref();
-            let count: &mut usize = map.entry(platform).or_default();
-            *count += 1;
-            map
-        },
-    );
-
-    for (p, count) in &frames_by_platform {
-        let frame_platform = p.map(|p| p.as_ref()).unwrap_or("none");
+    for (p, count) in stats.symbolicated_frames {
+        let frame_platform = p.as_ref().map(|p| p.as_ref()).unwrap_or("none");
         metric!(
             time_raw("symbolication.num_frames") =
                 count,
             "frame_platform" => frame_platform, "event_platform" => event_platform
         );
     }
-    metric!(time_raw("symbolication.unsymbolicated_frames") = unsymbolicated_frames);
-    metric!(time_raw("js.missing_sourcescontent") = missing_sourcescontent);
+
+    for (p, count) in stats.unsymbolicated_frames {
+        let frame_platform = p.as_ref().map(|p| p.as_ref()).unwrap_or("none");
+        metric!(
+            time_raw("symbolication.unsymbolicated_frames") =
+                count,
+            "frame_platform" => frame_platform, "event_platform" => event_platform
+        );
+    }
+
+    metric!(time_raw("js.missing_sourcescontent") = stats.missing_sourcescontent);
+}
+
+#[derive(Debug, Clone, Default)]
+pub(crate) struct SymbolicationStats {
+    pub(crate) symbolicated_frames: HashMap<Option<Platform>, u64>,
+    pub(crate) unsymbolicated_frames: HashMap<Option<Platform>, u64>,
+    pub(crate) num_stacktraces: u64,
+    pub(crate) missing_sourcescontent: u64,
 }
diff --git a/crates/symbolicator-js/src/symbolication.rs b/crates/symbolicator-js/src/symbolication.rs
@@ -9,7 +9,7 @@ use crate::interface::{
     SymbolicateJsStacktraces,
 };
 use crate::lookup::SourceMapLookup;
-use crate::metrics::record_stacktrace_metrics;
+use crate::metrics::{record_stacktrace_metrics, SymbolicationStats};
 use crate::utils::{
     fixup_webpack_filename, fold_function_name, generate_module, get_function_for_token, is_in_app,
     join_paths,
@@ -28,8 +28,7 @@ impl SourceMapService {
         let mut lookup = SourceMapLookup::new(self.clone(), request).await;
         lookup.prepare_modules(&mut raw_stacktraces[..]);
 
-        let mut unsymbolicated_frames = 0;
-        let mut missing_sourcescontent = 0;
+        let mut stats = SymbolicationStats::default();
 
         let num_stacktraces = raw_stacktraces.len();
         let mut stacktraces = Vec::with_capacity(num_stacktraces);
@@ -47,16 +46,23 @@ impl SourceMapService {
                     &mut errors,
                     std::mem::take(&mut callsite_fn_name),
                     apply_source_context,
-                    &mut missing_sourcescontent,
+                    &mut stats,
                 )
                 .await
                 {
                     Ok(mut frame) => {
+                        *stats
+                            .symbolicated_frames
+                            .entry(raw_frame.platform.clone())
+                            .or_default() += 1;
                         std::mem::swap(&mut callsite_fn_name, &mut frame.token_name);
                         symbolicated_frames.push(frame);
                     }
                     Err(err) => {
-                        unsymbolicated_frames += 1;
+                        *stats
+                            .unsymbolicated_frames
+                            .entry(raw_frame.platform.clone())
+                            .or_default() += 1;
                         errors.insert(JsModuleError {
                             abs_path: raw_frame.abs_path.clone(),
                             kind: err,
@@ -71,13 +77,10 @@ impl SourceMapService {
             });
         }
 
+        stats.num_stacktraces = stacktraces.len() as u64;
+
         lookup.record_metrics();
-        record_stacktrace_metrics(
-            platform,
-            &stacktraces,
-            unsymbolicated_frames,
-            missing_sourcescontent,
-        );
+        record_stacktrace_metrics(platform, stats);
 
         let (used_artifact_bundles, scraping_attempts) = lookup.into_records();
 
@@ -97,7 +100,7 @@ async fn symbolicate_js_frame(
     errors: &mut BTreeSet<JsModuleError>,
     callsite_fn_name: Option<String>,
     should_apply_source_context: bool,
-    missing_sourcescontent: &mut u64,
+    stats: &mut SymbolicationStats,
 ) -> Result<JsFrame, JsModuleErrorKind> {
     // we check for a valid line (i.e. >= 1) first, as we want to avoid resolving / scraping the minified
     // file in that case. we frequently saw 0 line/col values in combination with non-js files,
@@ -266,7 +269,7 @@ async fn symbolicate_js_frame(
                 });
             }
         } else {
-            *missing_sourcescontent += 1;
+            stats.missing_sourcescontent += 1;
 
             // If we have no source context from within the `SourceMapCache`,
             // fall back to applying the source context from a raw artifact file

diff --git a/crates/symbolicator-proguard/src/metrics.rs b/crates/symbolicator-proguard/src/metrics.rs
@@ -1,44 +1,51 @@
-use std::{collections::HashMap, sync::Arc};
+use std::collections::HashMap;
 
 use symbolicator_service::{metric, types::Platform};
 
-use crate::interface::{JvmException, JvmStacktrace};
-
 /// Record metrics about exceptions, stacktraces, frames, and remapped classes.
-pub fn record_symbolication_metrics(
+pub(crate) fn record_symbolication_metrics(
     event_platform: Option<Platform>,
-    exceptions: &[JvmException],
-    stacktraces: &[JvmStacktrace],
-    classes: &HashMap<Arc<str>, Arc<str>>,
-    unsymbolicated_frames: u64,
+    stats: SymbolicationStats,
 ) {
     let event_platform = event_platform
         .as_ref()
         .map(|p| p.as_ref())
         .unwrap_or("none");
 
-    metric!(time_raw("symbolication.num_exceptions") = exceptions.len() as u64, "event_platform" => event_platform);
-    metric!(time_raw("symbolication.num_stacktraces") = stacktraces.len() as u64);
-
-    // Count number of frames by platform (including no platform)
-    let frames_by_platform = stacktraces.iter().flat_map(|st| st.frames.iter()).fold(
-        HashMap::new(),
-        |mut map, frame| {
-            let platform = frame.platform.as_ref();
-            let count: &mut usize = map.entry(platform).or_default();
-            *count += 1;
-            map
-        },
-    );
-
-    for (p, count) in &frames_by_platform {
-        let frame_platform = p.map(|p| p.as_ref()).unwrap_or("none");
+    metric!(time_raw("symbolication.num_exceptions") = stats.symbolicated_exceptions, "event_platform" => event_platform);
+    metric!(time_raw("symbolication.unsymbolicated_exceptions") = stats.unsymbolicated_exceptions, "event_platform" => event_platform);
+
+    metric!(time_raw("symbolication.num_stacktraces") = stats.num_stacktraces);
+
+    for (p, count) in stats.symbolicated_frames {
+        let frame_platform = p.as_ref().map(|p| p.as_ref()).unwrap_or("none");
         metric!(
             time_raw("symbolication.num_frames") =
                 count,
             "frame_platform" => frame_platform, "event_platform" => event_platform
         );
     }
-    metric!(time_raw("symbolication.num_classes") = classes.len() as u64, "event_platform" => event_platform);
-    metric!(time_raw("symbolication.unsymbolicated_frames") = unsymbolicated_frames);
+
+    for (p, count) in stats.unsymbolicated_frames {
+        let frame_platform = p.as_ref().map(|p| p.as_ref()).unwrap_or("none");
+        metric!(
+            time_raw("symbolication.unsymbolicated_frames") =
+                count,
+            "frame_platform" => frame_platform, "event_platform" => event_platform
+        );
+    }
+
+    metric!(time_raw("symbolication.num_classes") = stats.symbolicated_classes, "event_platform" => event_platform);
+    metric!(time_raw("symbolication.unsymbolicated_classes") = stats.unsymbolicated_classes, "event_platform" => event_platform);
+}
+
+#[derive(Debug, Clone, Default)]
+pub(crate) struct SymbolicationStats {
+    pub(crate) symbolicated_exceptions: u64,
+    pub(crate) unsymbolicated_exceptions: u64,
+    pub(crate) symbolicated_classes: u64,
+    pub(crate) unsymbolicated_classes: u64,
+    pub(crate) symbolicated_frames: HashMap<Option<Platform>, u64>,
+    pub(crate) unsymbolicated_frames: HashMap<Option<Platform>, u64>,
+    pub(crate) num_stacktraces: u64,
 }