From 2f2f70e770ebd74af9320610991c30d1a66ed874 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Thu, 5 Dec 2024 17:50:39 +0100 Subject: [PATCH] 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. --- crates/symbolicator-js/src/metrics.rs | 47 +++++----- crates/symbolicator-js/src/symbolication.rs | 29 +++--- crates/symbolicator-proguard/src/metrics.rs | 59 ++++++------ .../src/symbolication.rs | 92 ++++++++++++------- 4 files changed, 133 insertions(+), 94 deletions(-) diff --git a/crates/symbolicator-js/src/metrics.rs b/crates/symbolicator-js/src/metrics.rs index b31911d72..996b2798c 100644 --- 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, - stacktraces: &[JsStacktrace], - unsymbolicated_frames: u64, - missing_sourcescontent: u64, -) { +pub fn record_stacktrace_metrics(event_platform: Option, 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, u64>, + pub(crate) unsymbolicated_frames: HashMap, 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 index fd1482674..16f716bc6 100644 --- 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, callsite_fn_name: Option, should_apply_source_context: bool, - missing_sourcescontent: &mut u64, + stats: &mut SymbolicationStats, ) -> Result { // 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 index e6e6441fa..581e8a2ac 100644 --- 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, - exceptions: &[JvmException], - stacktraces: &[JvmStacktrace], - classes: &HashMap, Arc>, - 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, u64>, + pub(crate) unsymbolicated_frames: HashMap, u64>, + pub(crate) num_stacktraces: u64, } diff --git a/crates/symbolicator-proguard/src/symbolication.rs b/crates/symbolicator-proguard/src/symbolication.rs index f62efe47b..49c43dee1 100644 --- a/crates/symbolicator-proguard/src/symbolication.rs +++ b/crates/symbolicator-proguard/src/symbolication.rs @@ -4,7 +4,7 @@ use crate::interface::{ CompletedJvmSymbolicationResponse, JvmException, JvmFrame, JvmModuleType, JvmStacktrace, ProguardError, ProguardErrorKind, SymbolicateJvmStacktraces, }; -use crate::metrics::record_symbolication_metrics; +use crate::metrics::{record_symbolication_metrics, SymbolicationStats}; use crate::ProguardService; use futures::future; @@ -40,7 +40,7 @@ impl ProguardService { classes, } = request; - let mut unsymbolicated_frames = 0; + let mut stats = SymbolicationStats::default(); let maybe_mappers = future::join_all( modules @@ -118,9 +118,18 @@ impl ProguardService { let remapped_exceptions: Vec<_> = exceptions .into_iter() - .map(|raw_exception| { - Self::map_exception(&mappers, &raw_exception).unwrap_or(raw_exception) - }) + .map( + |raw_exception| match Self::map_exception(&mappers, &raw_exception) { + Some(exc) => { + stats.symbolicated_exceptions += 1; + exc + } + None => { + stats.unsymbolicated_exceptions += 1; + raw_exception + } + }, + ) .collect(); let mut remapped_stacktraces: Vec<_> = stacktraces @@ -130,13 +139,8 @@ impl ProguardService { .frames .iter() .flat_map(|frame| { - Self::map_frame( - &mappers, - frame, - release_package.as_deref(), - &mut unsymbolicated_frames, - ) - .into_iter() + Self::map_frame(&mappers, frame, release_package.as_deref(), &mut stats) + .into_iter() }) .collect(); JvmStacktrace { @@ -157,20 +161,22 @@ impl ProguardService { let remapped_classes = classes .into_iter() .filter_map(|class| { - let remapped = mappers - .iter() - .find_map(|mapper| mapper.remap_class(&class))?; - Some((class, Arc::from(remapped))) + match mappers.iter().find_map(|mapper| mapper.remap_class(&class)) { + Some(remapped) => { + stats.symbolicated_classes += 1; + Some((class, Arc::from(remapped))) + } + None => { + stats.unsymbolicated_classes += 1; + None + } + } }) .collect(); - record_symbolication_metrics( - platform, - &remapped_exceptions, - &remapped_stacktraces, - &remapped_classes, - unsymbolicated_frames, - ); + stats.num_stacktraces = remapped_stacktraces.len() as u64; + + record_symbolication_metrics(platform, stats); CompletedJvmSymbolicationResponse { exceptions: remapped_exceptions, @@ -225,7 +231,7 @@ impl ProguardService { mappers: &[&proguard::ProguardCache], frame: &JvmFrame, release_package: Option<&str>, - unsymbolicated_frames: &mut u64, + stats: &mut SymbolicationStats, ) -> Vec { let deobfuscated_signature = frame.signature.as_ref().and_then(|signature| { mappers @@ -290,7 +296,7 @@ impl ProguardService { // Fix up the frames' in-app fields only if they were actually mapped if let Some(frames) = frames.as_mut() { - for frame in frames { + for frame in frames.iter_mut() { // mark the frame as in_app after deobfuscation based on the release package name // only if it's not present if let Some(package) = release_package { @@ -299,11 +305,22 @@ impl ProguardService { } } } + + // Also count the frames as symbolicated at this point + for frame in frames { + *stats + .symbolicated_frames + .entry(frame.platform.clone()) + .or_default() += 1; + } } // If all else fails, just return the original frame. let mut frames = frames.unwrap_or_else(|| { - *unsymbolicated_frames += 1; + *stats + .unsymbolicated_frames + .entry(frame.platform.clone()) + .or_default() += 1; vec![frame.clone()] }); @@ -581,7 +598,8 @@ io.sentry.sample.MainActivity -> io.sentry.sample.MainActivity: let mapped_frames: Vec<_> = frames .iter() .flat_map(|frame| { - ProguardService::map_frame(&[&cache], frame, None, &mut 0).into_iter() + ProguardService::map_frame(&[&cache], frame, None, &mut Default::default()) + .into_iter() }) .collect(); @@ -692,7 +710,13 @@ org.slf4j.helpers.Util$ClassContext -> org.a.b.g$b: let mapped_frames: Vec<_> = frames .iter() .flat_map(|frame| { - ProguardService::map_frame(&[&cache], frame, Some("org.slf4j"), &mut 0).into_iter() + ProguardService::map_frame( + &[&cache], + frame, + Some("org.slf4j"), + &mut Default::default(), + ) + .into_iter() }) .collect(); @@ -716,7 +740,8 @@ org.slf4j.helpers.Util$ClassContext -> org.a.b.g$b: ..Default::default() }; - let remapped = ProguardService::map_frame(&[], &frame, Some("android"), &mut 0); + let remapped = + ProguardService::map_frame(&[], &frame, Some("android"), &mut Default::default()); assert_eq!(remapped.len(), 1); // The frame didn't get mapped, so we shouldn't set `in_app` even though @@ -763,7 +788,8 @@ org.slf4j.helpers.Util$ClassContext -> org.a.b.g$b: ..Default::default() }; - let mapped_frames = ProguardService::map_frame(&[&cache], &frame, None, &mut 0); + let mapped_frames = + ProguardService::map_frame(&[&cache], &frame, None, &mut Default::default()); assert_eq!(mapped_frames.len(), 2); @@ -833,7 +859,8 @@ y.b -> y.b: ..Default::default() }; - let mapped_frames = ProguardService::map_frame(&[&cache], &frame, None, &mut 0); + let mapped_frames = + ProguardService::map_frame(&[&cache], &frame, None, &mut Default::default()); assert_eq!(mapped_frames.len(), 1); @@ -973,7 +1000,8 @@ io.wzieba.r8fullmoderenamessources.R -> a.d: let (remapped_filenames, remapped_abs_paths): (Vec<_>, Vec<_>) = frames .iter() .flat_map(|frame| { - ProguardService::map_frame(&[&cache], frame, None, &mut 0).into_iter() + ProguardService::map_frame(&[&cache], frame, None, &mut Default::default()) + .into_iter() }) .map(|frame| (frame.filename.unwrap(), frame.abs_path.unwrap())) .unzip();