From 28c82c6e579e52739f6b3d1c13772054c43ca143 Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Thu, 10 Jan 2019 15:36:20 +0100 Subject: [PATCH] feat: Port slim_frame_data --- general/src/store/mod.rs | 8 ++ general/src/store/normalize.rs | 8 +- general/src/store/normalize/stacktrace.rs | 119 ++++++++++++++++++++++ 3 files changed, 133 insertions(+), 2 deletions(-) diff --git a/general/src/store/mod.rs b/general/src/store/mod.rs index 8824d3dcad..a169fec441 100644 --- a/general/src/store/mod.rs +++ b/general/src/store/mod.rs @@ -27,7 +27,15 @@ pub struct StoreConfig { pub is_public_auth: bool, pub key_id: Option, pub protocol_version: Option, + + /// Hard limit for stacktrace frames + /// Corresponds to SENTRY_STACKTRACE_FRAMES_HARD_LIMIT pub stacktrace_frames_hard_limit: Option, + + /// Soft limit for stacktrace frames + /// Corresponds to SENTRY_MAX_STACKTRACE_FRAMES + pub max_stacktrace_frames: Option, + pub valid_platforms: BTreeSet, pub max_secs_in_future: Option, pub max_secs_in_past: Option, diff --git a/general/src/store/normalize.rs b/general/src/store/normalize.rs index 5749db29fd..1de8f634c8 100644 --- a/general/src/store/normalize.rs +++ b/general/src/store/normalize.rs @@ -449,8 +449,12 @@ impl<'a> Processor for NormalizeProcessor<'a> { stacktrace.process_child_values(self, state); - // TODO: port slim_frame_data and call it here (needs to run after process_frame because of - // `in_app`) + // needs to run after process_frame because of `in_app` + if let Some(limit) = self.config.max_stacktrace_frames { + stacktrace + .frames + .apply(|frames, _meta| stacktrace::slim_frame_data(frames, limit)); + } ValueAction::Keep } diff --git a/general/src/store/normalize/stacktrace.rs b/general/src/store/normalize/stacktrace.rs index 0cfb6c9e2d..c946cb3723 100644 --- a/general/src/store/normalize/stacktrace.rs +++ b/general/src/store/normalize/stacktrace.rs @@ -58,6 +58,54 @@ pub fn enforce_frame_hard_limit(frames: &mut Array, meta: &mut Meta, limi } } +/// Remove excess metadata for middle frames which go beyond `frame_allowance`. +/// +/// This is supposed to be equivalent to `slim_frame_data` in Sentry. +pub fn slim_frame_data(frames: &mut Array, frame_allowance: usize) { + let frames_len = frames.len(); + + if frames_len <= frame_allowance { + return; + } + + // Avoid ownership issues by only storing indices + let mut app_frame_indices = Vec::with_capacity(frames_len); + let mut system_frame_indices = Vec::with_capacity(frames_len); + + for (i, frame) in frames.iter().enumerate() { + if let Some(frame) = frame.value() { + match frame.in_app.value() { + Some(true) => app_frame_indices.push(i), + _ => system_frame_indices.push(i), + } + } + } + + let app_count = app_frame_indices.len(); + let system_allowance_half = frame_allowance.saturating_sub(app_count) / 2; + let system_frames_to_remove = &system_frame_indices + [system_allowance_half..system_frame_indices.len() - system_allowance_half]; + + let remaining = frames_len + .saturating_sub(frame_allowance) + .saturating_sub(system_frames_to_remove.len()); + let app_allowance_half = app_count.saturating_sub(remaining) / 2; + let app_frames_to_remove = + &app_frame_indices[app_allowance_half..app_frame_indices.len() - app_allowance_half]; + + // TODO: Which annotation to set? + + for i in system_frames_to_remove.iter().chain(app_frames_to_remove) { + if let Some(frame) = frames.get_mut(*i) { + if let Some(ref mut frame) = frame.value_mut().as_mut() { + frame.vars = Annotated::empty(); + frame.pre_context = Annotated::empty(); + frame.post_context = Annotated::empty(); + } + } + } +} + #[test] fn test_coerces_url_filenames() { let mut frame = Annotated::new(Frame { @@ -152,3 +200,74 @@ fn test_is_url() { assert!(!is_url("data:,")); assert!(!is_url("blob:\x00")); } + +#[test] +fn test_slim_frame_data_under_max() { + let mut frames = vec![Annotated::new(Frame { + filename: Annotated::new("foo".to_string()), + pre_context: Annotated::new(vec![Annotated::new("a".to_string())]), + context_line: Annotated::new("b".to_string()), + post_context: Annotated::new(vec![Annotated::new("c".to_string())]), + ..Default::default() + })]; + + let old_frames = frames.clone(); + slim_frame_data(&mut frames, 4); + + assert_eq_dbg!(frames, old_frames); +} + +#[test] +fn test_slim_frame_data_over_max() { + let mut frames = vec![]; + + for n in 0..5 { + frames.push(Annotated::new(Frame { + filename: Annotated::new(format!("foo {}", n)), + pre_context: Annotated::new(vec![Annotated::new("a".to_string())]), + context_line: Annotated::new("b".to_string()), + post_context: Annotated::new(vec![Annotated::new("c".to_string())]), + ..Default::default() + })); + } + + slim_frame_data(&mut frames, 4); + + let expected = vec![ + Annotated::new(Frame { + filename: Annotated::new("foo 0".to_string()), + pre_context: Annotated::new(vec![Annotated::new("a".to_string())]), + context_line: Annotated::new("b".to_string()), + post_context: Annotated::new(vec![Annotated::new("c".to_string())]), + ..Default::default() + }), + Annotated::new(Frame { + filename: Annotated::new("foo 1".to_string()), + pre_context: Annotated::new(vec![Annotated::new("a".to_string())]), + context_line: Annotated::new("b".to_string()), + post_context: Annotated::new(vec![Annotated::new("c".to_string())]), + ..Default::default() + }), + Annotated::new(Frame { + filename: Annotated::new("foo 2".to_string()), + context_line: Annotated::new("b".to_string()), + ..Default::default() + }), + Annotated::new(Frame { + filename: Annotated::new("foo 3".to_string()), + pre_context: Annotated::new(vec![Annotated::new("a".to_string())]), + context_line: Annotated::new("b".to_string()), + post_context: Annotated::new(vec![Annotated::new("c".to_string())]), + ..Default::default() + }), + Annotated::new(Frame { + filename: Annotated::new("foo 4".to_string()), + pre_context: Annotated::new(vec![Annotated::new("a".to_string())]), + context_line: Annotated::new("b".to_string()), + post_context: Annotated::new(vec![Annotated::new("c".to_string())]), + ..Default::default() + }), + ]; + + assert_eq_dbg!(frames, expected); +}