From 4cad83c04de081cace9924d2f3286ad96bcb385b Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Mon, 21 Oct 2024 13:53:02 +0100 Subject: [PATCH 01/21] feat(errors): Fingerprinting first pass --- rust/Cargo.lock | 1 + rust/cymbal/Cargo.toml | 1 + rust/cymbal/src/lib.rs | 1 + rust/cymbal/src/main.rs | 5 +++++ 4 files changed, 8 insertions(+) diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 3f3c36157e36b..dd2df6cc5b826 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -996,6 +996,7 @@ dependencies = [ "reqwest 0.12.3", "serde", "serde_json", + "sha2", "sourcemap", "sqlx", "thiserror", diff --git a/rust/cymbal/Cargo.toml b/rust/cymbal/Cargo.toml index 532730d12e36a..44ffa877642c0 100644 --- a/rust/cymbal/Cargo.toml +++ b/rust/cymbal/Cargo.toml @@ -23,6 +23,7 @@ serde_json = { workspace = true } serde = { workspace = true } sourcemap = "9.0.0" reqwest = { workspace = true } +sha2 = "0.10.8" [dev-dependencies] httpmock = { workspace = true } diff --git a/rust/cymbal/src/lib.rs b/rust/cymbal/src/lib.rs index d923f4d04b93c..8fdc5e3142904 100644 --- a/rust/cymbal/src/lib.rs +++ b/rust/cymbal/src/lib.rs @@ -6,3 +6,4 @@ pub mod metric_consts; pub mod resolver; pub mod symbol_store; pub mod types; +pub mod fingerprinting; \ No newline at end of file diff --git a/rust/cymbal/src/main.rs b/rust/cymbal/src/main.rs index 079aa5c92a6bf..7c2ec641fdfc9 100644 --- a/rust/cymbal/src/main.rs +++ b/rust/cymbal/src/main.rs @@ -124,6 +124,11 @@ async fn main() -> Result<(), Error> { resolved_frames.push(resolved); } + let Ok(fingerprint) = cymbal::fingerprinting::v1::generate_fingerprint(&properties.exception_list[0], resolved_frames) else { + metrics::counter!(ERRORS, "cause" => "fingerprint_generation_failed").increment(1); + continue; + }; + metrics::counter!(STACK_PROCESSED).increment(1); } } From c81d83b4c1434340d86e85e267fd7efc0b6f0a05 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Mon, 21 Oct 2024 13:57:43 +0100 Subject: [PATCH 02/21] add files --- rust/cymbal/src/fingerprinting/mod.rs | 1 + rust/cymbal/src/fingerprinting/v1.rs | 103 ++++++++++++++++++++++++++ rust/cymbal/src/lib.rs | 2 +- rust/cymbal/src/main.rs | 5 +- 4 files changed, 109 insertions(+), 2 deletions(-) create mode 100644 rust/cymbal/src/fingerprinting/mod.rs create mode 100644 rust/cymbal/src/fingerprinting/v1.rs diff --git a/rust/cymbal/src/fingerprinting/mod.rs b/rust/cymbal/src/fingerprinting/mod.rs new file mode 100644 index 0000000000000..a3a6d96c3f59d --- /dev/null +++ b/rust/cymbal/src/fingerprinting/mod.rs @@ -0,0 +1 @@ +pub mod v1; diff --git a/rust/cymbal/src/fingerprinting/v1.rs b/rust/cymbal/src/fingerprinting/v1.rs new file mode 100644 index 0000000000000..f9f90a0055b25 --- /dev/null +++ b/rust/cymbal/src/fingerprinting/v1.rs @@ -0,0 +1,103 @@ +use crate::{ + error::Error, + types::{frames::Frame, Exception}, +}; +use sha2::{Digest, Sha256}; + +// Given resolved Frames vector and the original Exception, we can now generate a fingerprint for it +pub fn generate_fingerprint( + exception: &Exception, + resolved_frames: Vec, +) -> Result { + let mut fingerprint = format!( + "{}-{}", + exception.exception_type, exception.exception_message + ); + for frame in resolved_frames { + if !frame.in_app { + // We only want to fingerprint in-app frames + // TODO: What happens if we don't have any in-app frames? The lowest level frame should at least be in app + // - this can only happen when there's some bug in our stack trace collection? + continue; + } + fingerprint.push_str("-"); + // TODO: if soure contains url as well, should probably strip it out. + fingerprint.push_str(&frame.source.unwrap_or("unknown".to_string())); + fingerprint.push_str(":"); + fingerprint.push_str(&frame.resolved_name.unwrap_or(frame.mangled_name)); + } + // TODO: Handle anonymous functions somehow? Not sure if these would have a resolved name at all. How would they show up + // as unresolved names? + + Ok(fingerprint) +} + +// Generate sha256 hash of the fingerprint to get a unique fingerprint identifier +pub fn hash_fingerprint(fingerprint: &str) -> String { + let mut hasher = Sha256::new(); + hasher.update(fingerprint.as_bytes()); + let result = hasher.finalize(); + format!("{:x}", result) +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn test_fingerprint_generation() { + let exception = Exception { + exception_type: "TypeError".to_string(), + exception_message: "Cannot read property 'foo' of undefined".to_string(), + mechanism: Default::default(), + module: Default::default(), + thread_id: None, + stacktrace: Default::default(), + }; + + let resolved_frames = vec![ + Frame { + mangled_name: "foo".to_string(), + line: Some(10), + column: Some(5), + source: Some("http://example.com/foo.js".to_string()), + in_app: true, + resolved_name: Some("bar".to_string()), + lang: "javascript".to_string(), + }, + Frame { + mangled_name: "bar".to_string(), + line: Some(20), + column: Some(15), + source: Some("http://example.com/bar.js".to_string()), + in_app: true, + resolved_name: Some("baz".to_string()), + lang: "javascript".to_string(), + }, + Frame { + mangled_name: "xyz".to_string(), + line: Some(30), + column: Some(25), + source: None, + in_app: true, + resolved_name: None, + lang: "javascript".to_string(), + }, + Frame { + mangled_name: "".to_string(), + line: None, + column: None, + source: None, + in_app: false, + resolved_name: None, + lang: "javascript".to_string(), + }, + ]; + + let fingerprint = super::generate_fingerprint(&exception, resolved_frames).unwrap(); + assert_eq!( + fingerprint, + "TypeError-Cannot read property 'foo' of undefined-http://example.com/foo.js:bar-http://example.com/bar.js:baz-unknown:xyz" + ); + } +} diff --git a/rust/cymbal/src/lib.rs b/rust/cymbal/src/lib.rs index 8fdc5e3142904..43c4d7955bfdb 100644 --- a/rust/cymbal/src/lib.rs +++ b/rust/cymbal/src/lib.rs @@ -1,9 +1,9 @@ pub mod app_context; pub mod config; pub mod error; +pub mod fingerprinting; pub mod langs; pub mod metric_consts; pub mod resolver; pub mod symbol_store; pub mod types; -pub mod fingerprinting; \ No newline at end of file diff --git a/rust/cymbal/src/main.rs b/rust/cymbal/src/main.rs index 7c2ec641fdfc9..68577503dfe13 100644 --- a/rust/cymbal/src/main.rs +++ b/rust/cymbal/src/main.rs @@ -124,7 +124,10 @@ async fn main() -> Result<(), Error> { resolved_frames.push(resolved); } - let Ok(fingerprint) = cymbal::fingerprinting::v1::generate_fingerprint(&properties.exception_list[0], resolved_frames) else { + let Ok(fingerprint) = cymbal::fingerprinting::v1::generate_fingerprint( + &properties.exception_list[0], + resolved_frames, + ) else { metrics::counter!(ERRORS, "cause" => "fingerprint_generation_failed").increment(1); continue; }; From d219d0ed5e3ee26059a246c7172625449548df1b Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Mon, 21 Oct 2024 14:08:48 +0100 Subject: [PATCH 03/21] clippy --- rust/cymbal/src/fingerprinting/v1.rs | 4 ++-- rust/cymbal/src/main.rs | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/rust/cymbal/src/fingerprinting/v1.rs b/rust/cymbal/src/fingerprinting/v1.rs index f9f90a0055b25..858a940225205 100644 --- a/rust/cymbal/src/fingerprinting/v1.rs +++ b/rust/cymbal/src/fingerprinting/v1.rs @@ -20,10 +20,10 @@ pub fn generate_fingerprint( // - this can only happen when there's some bug in our stack trace collection? continue; } - fingerprint.push_str("-"); + fingerprint.push('-'); // TODO: if soure contains url as well, should probably strip it out. fingerprint.push_str(&frame.source.unwrap_or("unknown".to_string())); - fingerprint.push_str(":"); + fingerprint.push(':'); fingerprint.push_str(&frame.resolved_name.unwrap_or(frame.mangled_name)); } // TODO: Handle anonymous functions somehow? Not sure if these would have a resolved name at all. How would they show up diff --git a/rust/cymbal/src/main.rs b/rust/cymbal/src/main.rs index 68577503dfe13..8a021572ace40 100644 --- a/rust/cymbal/src/main.rs +++ b/rust/cymbal/src/main.rs @@ -8,6 +8,7 @@ use cymbal::{ app_context::AppContext, config::Config, error::Error, + fingerprinting, metric_consts::{ERRORS, EVENT_RECEIVED, STACK_PROCESSED}, types::{frames::RawFrame, ErrProps}, }; @@ -124,7 +125,7 @@ async fn main() -> Result<(), Error> { resolved_frames.push(resolved); } - let Ok(fingerprint) = cymbal::fingerprinting::v1::generate_fingerprint( + let Ok(fingerprint) = fingerprinting::v1::generate_fingerprint( &properties.exception_list[0], resolved_frames, ) else { From 68bb5234b4d002d541a217fbe1059a9f9858a1f0 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Tue, 22 Oct 2024 13:53:09 +0100 Subject: [PATCH 04/21] strip url from fingerprint --- rust/cymbal/src/fingerprinting/v1.rs | 14 ++++++++++---- rust/cymbal/src/langs/js.rs | 2 +- rust/cymbal/src/main.rs | 2 +- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/rust/cymbal/src/fingerprinting/v1.rs b/rust/cymbal/src/fingerprinting/v1.rs index 858a940225205..70ca791385579 100644 --- a/rust/cymbal/src/fingerprinting/v1.rs +++ b/rust/cymbal/src/fingerprinting/v1.rs @@ -2,6 +2,7 @@ use crate::{ error::Error, types::{frames::Frame, Exception}, }; +use reqwest::Url; use sha2::{Digest, Sha256}; // Given resolved Frames vector and the original Exception, we can now generate a fingerprint for it @@ -20,9 +21,14 @@ pub fn generate_fingerprint( // - this can only happen when there's some bug in our stack trace collection? continue; } + + let source_fn = match Url::parse(&frame.source.unwrap_or("".to_string())) { + Ok(url) => url.path().to_string(), + Err(_) => "unknown".to_string(), + }; + fingerprint.push('-'); - // TODO: if soure contains url as well, should probably strip it out. - fingerprint.push_str(&frame.source.unwrap_or("unknown".to_string())); + fingerprint.push_str(&source_fn); fingerprint.push(':'); fingerprint.push_str(&frame.resolved_name.unwrap_or(frame.mangled_name)); } @@ -60,7 +66,7 @@ mod test { mangled_name: "foo".to_string(), line: Some(10), column: Some(5), - source: Some("http://example.com/foo.js".to_string()), + source: Some("http://example.com/alpha/foo.js".to_string()), in_app: true, resolved_name: Some("bar".to_string()), lang: "javascript".to_string(), @@ -97,7 +103,7 @@ mod test { let fingerprint = super::generate_fingerprint(&exception, resolved_frames).unwrap(); assert_eq!( fingerprint, - "TypeError-Cannot read property 'foo' of undefined-http://example.com/foo.js:bar-http://example.com/bar.js:baz-unknown:xyz" + "TypeError-Cannot read property 'foo' of undefined-/alpha/foo.js:bar-/bar.js:baz-unknown:xyz" ); } } diff --git a/rust/cymbal/src/langs/js.rs b/rust/cymbal/src/langs/js.rs index eb7a84ef60333..c01e795dbe6d0 100644 --- a/rust/cymbal/src/langs/js.rs +++ b/rust/cymbal/src/langs/js.rs @@ -103,7 +103,7 @@ impl RawJSFrame { }?) } - // Returns none if the frame is + // Returns none if the frame is minified fn try_assume_unminified(&self) -> Option { // TODO - we should include logic here that uses some kind of heuristic to determine // if this frame is minified or not. Right now, we simply assume it isn't if this is diff --git a/rust/cymbal/src/main.rs b/rust/cymbal/src/main.rs index 8a021572ace40..704324aba42df 100644 --- a/rust/cymbal/src/main.rs +++ b/rust/cymbal/src/main.rs @@ -125,7 +125,7 @@ async fn main() -> Result<(), Error> { resolved_frames.push(resolved); } - let Ok(fingerprint) = fingerprinting::v1::generate_fingerprint( + let Ok(_fingerprint) = fingerprinting::v1::generate_fingerprint( &properties.exception_list[0], resolved_frames, ) else { From 537b2534211f3c17dccf4c7f6b1e0ac08138a5a2 Mon Sep 17 00:00:00 2001 From: David Newell Date: Thu, 24 Oct 2024 10:17:40 +0100 Subject: [PATCH 05/21] add more conditions and tests --- rust/cymbal/src/fingerprinting/v1.rs | 171 +++++++++++++++++++++++++-- rust/cymbal/src/langs/js.rs | 2 + rust/cymbal/src/types/frames.rs | 5 +- 3 files changed, 168 insertions(+), 10 deletions(-) diff --git a/rust/cymbal/src/fingerprinting/v1.rs b/rust/cymbal/src/fingerprinting/v1.rs index 70ca791385579..723f096af9c58 100644 --- a/rust/cymbal/src/fingerprinting/v1.rs +++ b/rust/cymbal/src/fingerprinting/v1.rs @@ -1,5 +1,6 @@ use crate::{ error::Error, + metric_consts::ERRORS, types::{frames::Frame, Exception}, }; use reqwest::Url; @@ -8,20 +9,27 @@ use sha2::{Digest, Sha256}; // Given resolved Frames vector and the original Exception, we can now generate a fingerprint for it pub fn generate_fingerprint( exception: &Exception, - resolved_frames: Vec, + mut frames: Vec, ) -> Result { let mut fingerprint = format!( "{}-{}", exception.exception_type, exception.exception_message ); - for frame in resolved_frames { - if !frame.in_app { - // We only want to fingerprint in-app frames - // TODO: What happens if we don't have any in-app frames? The lowest level frame should at least be in app - // - this can only happen when there's some bug in our stack trace collection? - continue; - } + let has_resolved_frames: bool = frames.iter().any(|f| f.resolved); + if has_resolved_frames { + frames = frames.into_iter().filter(|f| f.resolved).collect(); + } + + let has_in_app_frames: bool = frames.iter().any(|f| f.in_app); + if has_in_app_frames { + frames = frames.into_iter().filter(|f| f.in_app).collect(); + } else { + metrics::counter!(ERRORS, "cause" => "no_in_app_frames").increment(1); + frames = frames.into_iter().take(1).collect() + } + + for frame in frames { let source_fn = match Url::parse(&frame.source.unwrap_or("".to_string())) { Ok(url) => url.path().to_string(), Err(_) => "unknown".to_string(), @@ -69,6 +77,7 @@ mod test { source: Some("http://example.com/alpha/foo.js".to_string()), in_app: true, resolved_name: Some("bar".to_string()), + resolved: true, lang: "javascript".to_string(), }, Frame { @@ -78,6 +87,7 @@ mod test { source: Some("http://example.com/bar.js".to_string()), in_app: true, resolved_name: Some("baz".to_string()), + resolved: true, lang: "javascript".to_string(), }, Frame { @@ -87,6 +97,7 @@ mod test { source: None, in_app: true, resolved_name: None, + resolved: true, lang: "javascript".to_string(), }, Frame { @@ -96,6 +107,109 @@ mod test { source: None, in_app: false, resolved_name: None, + resolved: true, + lang: "javascript".to_string(), + }, + ]; + + let fingerprint = super::generate_fingerprint(&exception, resolved_frames).unwrap(); + assert_eq!( + fingerprint, + "TypeError-Cannot read property 'foo' of undefined-/alpha/foo.js:bar-/bar.js:baz-unknown:xyz" + ); + } + + #[test] + fn test_some_resolved_frames() { + let exception = Exception { + exception_type: "TypeError".to_string(), + exception_message: "Cannot read property 'foo' of undefined".to_string(), + mechanism: Default::default(), + module: Default::default(), + thread_id: None, + stacktrace: Default::default(), + }; + + let resolved_frames = vec![ + Frame { + mangled_name: "foo".to_string(), + line: Some(10), + column: Some(5), + source: Some("http://example.com/alpha/foo.js".to_string()), + in_app: true, + resolved_name: Some("bar".to_string()), + resolved: true, + lang: "javascript".to_string(), + }, + Frame { + mangled_name: "bar".to_string(), + line: Some(20), + column: Some(15), + source: Some("http://example.com/bar.js".to_string()), + in_app: true, + resolved_name: Some("baz".to_string()), + resolved: true, + lang: "javascript".to_string(), + }, + Frame { + mangled_name: "xyz".to_string(), + line: Some(30), + column: Some(25), + source: None, + in_app: true, + resolved_name: None, + resolved: false, + lang: "javascript".to_string(), + }, + ]; + + let fingerprint = super::generate_fingerprint(&exception, resolved_frames).unwrap(); + assert_eq!( + fingerprint, + "TypeError-Cannot read property 'foo' of undefined-/alpha/foo.js:bar-/bar.js:baz" + ); + } + + #[test] + fn test_no_resolved_frames() { + let exception = Exception { + exception_type: "TypeError".to_string(), + exception_message: "Cannot read property 'foo' of undefined".to_string(), + mechanism: Default::default(), + module: Default::default(), + thread_id: None, + stacktrace: Default::default(), + }; + + let resolved_frames = vec![ + Frame { + mangled_name: "foo".to_string(), + line: Some(10), + column: Some(5), + source: Some("http://example.com/alpha/foo.js".to_string()), + in_app: true, + resolved_name: Some("bar".to_string()), + resolved: false, + lang: "javascript".to_string(), + }, + Frame { + mangled_name: "bar".to_string(), + line: Some(20), + column: Some(15), + source: Some("http://example.com/bar.js".to_string()), + in_app: true, + resolved_name: Some("baz".to_string()), + resolved: false, + lang: "javascript".to_string(), + }, + Frame { + mangled_name: "xyz".to_string(), + line: Some(30), + column: Some(25), + source: None, + in_app: true, + resolved_name: None, + resolved: false, lang: "javascript".to_string(), }, ]; @@ -106,4 +220,45 @@ mod test { "TypeError-Cannot read property 'foo' of undefined-/alpha/foo.js:bar-/bar.js:baz-unknown:xyz" ); } + + #[test] + fn test_no_in_app_frames() { + let exception = Exception { + exception_type: "TypeError".to_string(), + exception_message: "Cannot read property 'foo' of undefined".to_string(), + mechanism: Default::default(), + module: Default::default(), + thread_id: None, + stacktrace: Default::default(), + }; + + let resolved_frames = vec![ + Frame { + mangled_name: "foo".to_string(), + line: Some(10), + column: Some(5), + source: Some("http://example.com/alpha/foo.js".to_string()), + in_app: false, + resolved_name: Some("bar".to_string()), + resolved: false, + lang: "javascript".to_string(), + }, + Frame { + mangled_name: "bar".to_string(), + line: Some(20), + column: Some(15), + source: Some("http://example.com/bar.js".to_string()), + in_app: false, + resolved_name: Some("baz".to_string()), + resolved: false, + lang: "javascript".to_string(), + }, + ]; + + let fingerprint = super::generate_fingerprint(&exception, resolved_frames).unwrap(); + assert_eq!( + fingerprint, + "TypeError-Cannot read property 'foo' of undefined-/alpha/foo.js:bar" + ); + } } diff --git a/rust/cymbal/src/langs/js.rs b/rust/cymbal/src/langs/js.rs index c01e795dbe6d0..673cfad797bbb 100644 --- a/rust/cymbal/src/langs/js.rs +++ b/rust/cymbal/src/langs/js.rs @@ -115,6 +115,7 @@ impl RawJSFrame { source: self.source_url.clone(), // Maybe we have one? in_app: self.in_app, resolved_name: Some(self.fn_name.clone()), // This is the bit we'd want to check + resolved: false, lang: "javascript".to_string(), }) } @@ -131,6 +132,7 @@ impl From<(&RawJSFrame, Token<'_>)> for Frame { source: token.get_source().map(String::from), in_app: raw_frame.in_app, resolved_name: token.get_name().map(String::from), + resolved: true, lang: "javascript".to_string(), } } diff --git a/rust/cymbal/src/types/frames.rs b/rust/cymbal/src/types/frames.rs index 480fd3b38d4af..07487524aee87 100644 --- a/rust/cymbal/src/types/frames.rs +++ b/rust/cymbal/src/types/frames.rs @@ -45,7 +45,8 @@ pub struct Frame { pub line: Option, // Line the function is define on, if known pub column: Option, // Column the function is defined on, if known pub source: Option, // Generally, the file the function is defined in - pub in_app: bool, // We hard-require clients to tell us this? pub resolved_name: Option, // The name of the function, after symbolification - pub lang: String, // The language of the frame. Always known (I guess?) + pub lang: String, // The language of the frame. Always known + pub in_app: bool, // We hard-require clients to tell us this? + pub resolved: bool, // Whether the frame was successfully demangled (always true if it does not apply) } From 2495167ea212cd70e6237b9cde16ce14f9828bf2 Mon Sep 17 00:00:00 2001 From: David Newell Date: Thu, 24 Oct 2024 10:32:26 +0100 Subject: [PATCH 06/21] use retain --- rust/cymbal/src/fingerprinting/v1.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/cymbal/src/fingerprinting/v1.rs b/rust/cymbal/src/fingerprinting/v1.rs index 723f096af9c58..79f84d5e975cd 100644 --- a/rust/cymbal/src/fingerprinting/v1.rs +++ b/rust/cymbal/src/fingerprinting/v1.rs @@ -18,12 +18,12 @@ pub fn generate_fingerprint( let has_resolved_frames: bool = frames.iter().any(|f| f.resolved); if has_resolved_frames { - frames = frames.into_iter().filter(|f| f.resolved).collect(); + frames.retain(|f| f.resolved); } let has_in_app_frames: bool = frames.iter().any(|f| f.in_app); if has_in_app_frames { - frames = frames.into_iter().filter(|f| f.in_app).collect(); + frames.retain(|f| f.in_app); } else { metrics::counter!(ERRORS, "cause" => "no_in_app_frames").increment(1); frames = frames.into_iter().take(1).collect() From 40579d82da074f1a458a594afc96ddd74ab67e4f Mon Sep 17 00:00:00 2001 From: David Newell Date: Thu, 24 Oct 2024 17:57:56 +0100 Subject: [PATCH 07/21] appease linter --- rust/cymbal/src/main.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/rust/cymbal/src/main.rs b/rust/cymbal/src/main.rs index c6d3a832f27b9..3506a7c318aa5 100644 --- a/rust/cymbal/src/main.rs +++ b/rust/cymbal/src/main.rs @@ -130,10 +130,9 @@ async fn main() -> Result<(), Error> { resolved_frames.push(resolved); } - let Ok(_fingerprint) = fingerprinting::v1::generate_fingerprint( - &properties.exception_list[0], - resolved_frames, - ) else { + let Ok(_fingerprint) = + fingerprinting::v1::generate_fingerprint(&exception_list[0], resolved_frames) + else { metrics::counter!(ERRORS, "cause" => "fingerprint_generation_failed").increment(1); continue; }; From c5a53e57b7dd66838fa894884eb7b611021ff752 Mon Sep 17 00:00:00 2001 From: David Newell Date: Tue, 29 Oct 2024 11:04:31 +0000 Subject: [PATCH 08/21] fix resolved --- rust/cymbal/src/langs/js.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/rust/cymbal/src/langs/js.rs b/rust/cymbal/src/langs/js.rs index b371e99c20656..c7bcc823eb03e 100644 --- a/rust/cymbal/src/langs/js.rs +++ b/rust/cymbal/src/langs/js.rs @@ -116,7 +116,6 @@ impl From<(&RawJSFrame, Token<'_>)> for Frame { resolved_name: token.get_name().map(String::from), resolved: true, lang: "javascript".to_string(), - resolved: true, resolve_failure: None, } } From 9e74a7f04a987d3130013241a0b09868e76c35ef Mon Sep 17 00:00:00 2001 From: David Newell Date: Tue, 29 Oct 2024 11:05:25 +0000 Subject: [PATCH 09/21] fix --- rust/cymbal/src/langs/js.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/cymbal/src/langs/js.rs b/rust/cymbal/src/langs/js.rs index c7bcc823eb03e..89deafdc46479 100644 --- a/rust/cymbal/src/langs/js.rs +++ b/rust/cymbal/src/langs/js.rs @@ -114,8 +114,8 @@ impl From<(&RawJSFrame, Token<'_>)> for Frame { source: token.get_source().map(String::from), in_app: raw_frame.in_app, resolved_name: token.get_name().map(String::from), - resolved: true, lang: "javascript".to_string(), + resolved: true, resolve_failure: None, } } From 176d63d8e6a4797067901e1832ea9686cab19271 Mon Sep 17 00:00:00 2001 From: David Newell Date: Mon, 4 Nov 2024 11:20:57 +0100 Subject: [PATCH 10/21] simplify with truncate --- rust/cymbal/src/fingerprinting/v1.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/cymbal/src/fingerprinting/v1.rs b/rust/cymbal/src/fingerprinting/v1.rs index 79f84d5e975cd..e728349f6ef8b 100644 --- a/rust/cymbal/src/fingerprinting/v1.rs +++ b/rust/cymbal/src/fingerprinting/v1.rs @@ -26,7 +26,7 @@ pub fn generate_fingerprint( frames.retain(|f| f.in_app); } else { metrics::counter!(ERRORS, "cause" => "no_in_app_frames").increment(1); - frames = frames.into_iter().take(1).collect() + frames.truncate(1); } for frame in frames { From 30b67abaf1c48ceb672d0b11640ae79adb788f62 Mon Sep 17 00:00:00 2001 From: David Newell Date: Mon, 4 Nov 2024 11:36:25 +0100 Subject: [PATCH 11/21] update fingerprinting to hashing --- rust/cymbal/src/fingerprinting/v1.rs | 48 ++++++++++++++++------------ 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/rust/cymbal/src/fingerprinting/v1.rs b/rust/cymbal/src/fingerprinting/v1.rs index e728349f6ef8b..f075841bbd432 100644 --- a/rust/cymbal/src/fingerprinting/v1.rs +++ b/rust/cymbal/src/fingerprinting/v1.rs @@ -4,23 +4,25 @@ use crate::{ types::{frames::Frame, Exception}, }; use reqwest::Url; -use sha2::{Digest, Sha256}; +use sha2::{Digest, Sha512}; // Given resolved Frames vector and the original Exception, we can now generate a fingerprint for it pub fn generate_fingerprint( exception: &Exception, mut frames: Vec, ) -> Result { - let mut fingerprint = format!( - "{}-{}", - exception.exception_type, exception.exception_message - ); + let mut hasher = Sha512::new(); + hasher.update(&exception.exception_type); + hasher.update(&exception.exception_message); + + // only use resolved frames if any exist let has_resolved_frames: bool = frames.iter().any(|f| f.resolved); if has_resolved_frames { frames.retain(|f| f.resolved); } + // only use in app frames if any exist, otherwise only use the first frame let has_in_app_frames: bool = frames.iter().any(|f| f.in_app); if has_in_app_frames { frames.retain(|f| f.in_app); @@ -35,23 +37,15 @@ pub fn generate_fingerprint( Err(_) => "unknown".to_string(), }; - fingerprint.push('-'); - fingerprint.push_str(&source_fn); - fingerprint.push(':'); - fingerprint.push_str(&frame.resolved_name.unwrap_or(frame.mangled_name)); + hasher.update(&source_fn); + hasher.update(frame.resolved_name.unwrap_or(frame.mangled_name)); } // TODO: Handle anonymous functions somehow? Not sure if these would have a resolved name at all. How would they show up // as unresolved names? - Ok(fingerprint) -} - -// Generate sha256 hash of the fingerprint to get a unique fingerprint identifier -pub fn hash_fingerprint(fingerprint: &str) -> String { - let mut hasher = Sha256::new(); - hasher.update(fingerprint.as_bytes()); let result = hasher.finalize(); - format!("{:x}", result) + + Ok(format!("{:x}", result)) } #[cfg(test)] @@ -78,6 +72,7 @@ mod test { in_app: true, resolved_name: Some("bar".to_string()), resolved: true, + resolve_failure: None, lang: "javascript".to_string(), }, Frame { @@ -88,6 +83,7 @@ mod test { in_app: true, resolved_name: Some("baz".to_string()), resolved: true, + resolve_failure: None, lang: "javascript".to_string(), }, Frame { @@ -98,6 +94,7 @@ mod test { in_app: true, resolved_name: None, resolved: true, + resolve_failure: None, lang: "javascript".to_string(), }, Frame { @@ -108,6 +105,7 @@ mod test { in_app: false, resolved_name: None, resolved: true, + resolve_failure: None, lang: "javascript".to_string(), }, ]; @@ -115,7 +113,7 @@ mod test { let fingerprint = super::generate_fingerprint(&exception, resolved_frames).unwrap(); assert_eq!( fingerprint, - "TypeError-Cannot read property 'foo' of undefined-/alpha/foo.js:bar-/bar.js:baz-unknown:xyz" + "e2a134db8585bafb19fa6c15b92bd46c699e06df7f39ce74281227e7c915adf0c997ab61757820710d85817b4a11823e57f9bcae526432c085b53e28397f3007" ); } @@ -139,6 +137,7 @@ mod test { in_app: true, resolved_name: Some("bar".to_string()), resolved: true, + resolve_failure: None, lang: "javascript".to_string(), }, Frame { @@ -149,6 +148,7 @@ mod test { in_app: true, resolved_name: Some("baz".to_string()), resolved: true, + resolve_failure: None, lang: "javascript".to_string(), }, Frame { @@ -159,6 +159,7 @@ mod test { in_app: true, resolved_name: None, resolved: false, + resolve_failure: None, lang: "javascript".to_string(), }, ]; @@ -166,7 +167,7 @@ mod test { let fingerprint = super::generate_fingerprint(&exception, resolved_frames).unwrap(); assert_eq!( fingerprint, - "TypeError-Cannot read property 'foo' of undefined-/alpha/foo.js:bar-/bar.js:baz" + "528bf82706cdcf928b3382d6bfb717c9572aa6855ffe8b620132a813dd54f2ee5c13437d22bb557634c4ceec104ce9c8c5a87d5d7cf36239be052ace8b9962aa" ); } @@ -190,6 +191,7 @@ mod test { in_app: true, resolved_name: Some("bar".to_string()), resolved: false, + resolve_failure: None, lang: "javascript".to_string(), }, Frame { @@ -200,6 +202,7 @@ mod test { in_app: true, resolved_name: Some("baz".to_string()), resolved: false, + resolve_failure: None, lang: "javascript".to_string(), }, Frame { @@ -210,6 +213,7 @@ mod test { in_app: true, resolved_name: None, resolved: false, + resolve_failure: None, lang: "javascript".to_string(), }, ]; @@ -217,7 +221,7 @@ mod test { let fingerprint = super::generate_fingerprint(&exception, resolved_frames).unwrap(); assert_eq!( fingerprint, - "TypeError-Cannot read property 'foo' of undefined-/alpha/foo.js:bar-/bar.js:baz-unknown:xyz" + "e2a134db8585bafb19fa6c15b92bd46c699e06df7f39ce74281227e7c915adf0c997ab61757820710d85817b4a11823e57f9bcae526432c085b53e28397f3007" ); } @@ -241,6 +245,7 @@ mod test { in_app: false, resolved_name: Some("bar".to_string()), resolved: false, + resolve_failure: None, lang: "javascript".to_string(), }, Frame { @@ -251,6 +256,7 @@ mod test { in_app: false, resolved_name: Some("baz".to_string()), resolved: false, + resolve_failure: None, lang: "javascript".to_string(), }, ]; @@ -258,7 +264,7 @@ mod test { let fingerprint = super::generate_fingerprint(&exception, resolved_frames).unwrap(); assert_eq!( fingerprint, - "TypeError-Cannot read property 'foo' of undefined-/alpha/foo.js:bar" + "c0fea2676a395a11577fd7a7b3006bcc416f6ceeb13290f831c4b5dc4de443ff51c5866d5c378137307c25cc8837cca6c82737e9704c47ed8408ff3937033e70" ); } } From d4f386b6722c657ea6d2ef610f99337671ed57d5 Mon Sep 17 00:00:00 2001 From: David Newell Date: Mon, 4 Nov 2024 12:01:40 +0100 Subject: [PATCH 12/21] cleanup types --- rust/cymbal/src/main.rs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/rust/cymbal/src/main.rs b/rust/cymbal/src/main.rs index b2c89dedb42a4..12c45067fa19c 100644 --- a/rust/cymbal/src/main.rs +++ b/rust/cymbal/src/main.rs @@ -137,17 +137,9 @@ async fn main() -> Result<(), Error> { let mut results = Vec::with_capacity(stack_trace.len()); for (_, frames) in groups.into_iter() { context.worker_liveness.report_healthy().await; // TODO - we shouldn't need to do this, but we do for now. - let mut any_success = false; - let per_frame_group = common_metrics::timing_guard(PER_FRAME_GROUP_TIME, &[]); for frame in frames { - results.push(frame.resolve(team_id, &context.catalog).await); - if results.last().unwrap().is_ok() { - any_success = true; - } + results.push(frame.resolve(team_id, &context.catalog).await?); } - per_frame_group - .label("resolved_any", if any_success { "true" } else { "false" }) - .fin(); } per_stack From 018e3a1b0f15c9baec7e6af04897c31885687379 Mon Sep 17 00:00:00 2001 From: Oliver Browne Date: Mon, 4 Nov 2024 14:25:47 +0100 Subject: [PATCH 13/21] refactor --- .../v1.rs => fingerprinting.rs} | 84 ++++++++----------- rust/cymbal/src/fingerprinting/mod.rs | 1 - rust/cymbal/src/main.rs | 76 ++++++++--------- rust/cymbal/src/types/frames.rs | 27 +++++- rust/cymbal/src/types/mod.rs | 40 +++++++-- rust/cymbal/tests/resolve.rs | 16 ++-- 6 files changed, 136 insertions(+), 108 deletions(-) rename rust/cymbal/src/{fingerprinting/v1.rs => fingerprinting.rs} (79%) delete mode 100644 rust/cymbal/src/fingerprinting/mod.rs diff --git a/rust/cymbal/src/fingerprinting/v1.rs b/rust/cymbal/src/fingerprinting.rs similarity index 79% rename from rust/cymbal/src/fingerprinting/v1.rs rename to rust/cymbal/src/fingerprinting.rs index f075841bbd432..415e6af641e38 100644 --- a/rust/cymbal/src/fingerprinting/v1.rs +++ b/rust/cymbal/src/fingerprinting.rs @@ -1,66 +1,36 @@ -use crate::{ - error::Error, - metric_consts::ERRORS, - types::{frames::Frame, Exception}, -}; -use reqwest::Url; +use crate::types::Exception; use sha2::{Digest, Sha512}; // Given resolved Frames vector and the original Exception, we can now generate a fingerprint for it -pub fn generate_fingerprint( - exception: &Exception, - mut frames: Vec, -) -> Result { +pub fn generate_fingerprint(exception: &[Exception]) -> String { let mut hasher = Sha512::new(); - hasher.update(&exception.exception_type); - hasher.update(&exception.exception_message); - - // only use resolved frames if any exist - let has_resolved_frames: bool = frames.iter().any(|f| f.resolved); - if has_resolved_frames { - frames.retain(|f| f.resolved); - } - - // only use in app frames if any exist, otherwise only use the first frame - let has_in_app_frames: bool = frames.iter().any(|f| f.in_app); - if has_in_app_frames { - frames.retain(|f| f.in_app); - } else { - metrics::counter!(ERRORS, "cause" => "no_in_app_frames").increment(1); - frames.truncate(1); - } - - for frame in frames { - let source_fn = match Url::parse(&frame.source.unwrap_or("".to_string())) { - Ok(url) => url.path().to_string(), - Err(_) => "unknown".to_string(), - }; - - hasher.update(&source_fn); - hasher.update(frame.resolved_name.unwrap_or(frame.mangled_name)); + for exc in exception { + exc.include_in_fingerprint(&mut hasher); } // TODO: Handle anonymous functions somehow? Not sure if these would have a resolved name at all. How would they show up // as unresolved names? let result = hasher.finalize(); - Ok(format!("{:x}", result)) + format!("{:x}", result) } #[cfg(test)] mod test { + use crate::types::{frames::Frame, Stacktrace}; + use super::*; #[test] fn test_fingerprint_generation() { - let exception = Exception { + let mut exception = Exception { exception_type: "TypeError".to_string(), exception_message: "Cannot read property 'foo' of undefined".to_string(), mechanism: Default::default(), module: Default::default(), thread_id: None, - stacktrace: Default::default(), + stack: Default::default(), }; let resolved_frames = vec![ @@ -110,7 +80,11 @@ mod test { }, ]; - let fingerprint = super::generate_fingerprint(&exception, resolved_frames).unwrap(); + exception.stack = Some(Stacktrace::Resolved { + frames: resolved_frames, + }); + + let fingerprint = super::generate_fingerprint(&[exception]); assert_eq!( fingerprint, "e2a134db8585bafb19fa6c15b92bd46c699e06df7f39ce74281227e7c915adf0c997ab61757820710d85817b4a11823e57f9bcae526432c085b53e28397f3007" @@ -119,13 +93,13 @@ mod test { #[test] fn test_some_resolved_frames() { - let exception = Exception { + let mut exception = Exception { exception_type: "TypeError".to_string(), exception_message: "Cannot read property 'foo' of undefined".to_string(), mechanism: Default::default(), module: Default::default(), thread_id: None, - stacktrace: Default::default(), + stack: Default::default(), }; let resolved_frames = vec![ @@ -164,7 +138,11 @@ mod test { }, ]; - let fingerprint = super::generate_fingerprint(&exception, resolved_frames).unwrap(); + exception.stack = Some(Stacktrace::Resolved { + frames: resolved_frames, + }); + + let fingerprint = super::generate_fingerprint(&[exception]); assert_eq!( fingerprint, "528bf82706cdcf928b3382d6bfb717c9572aa6855ffe8b620132a813dd54f2ee5c13437d22bb557634c4ceec104ce9c8c5a87d5d7cf36239be052ace8b9962aa" @@ -173,13 +151,13 @@ mod test { #[test] fn test_no_resolved_frames() { - let exception = Exception { + let mut exception = Exception { exception_type: "TypeError".to_string(), exception_message: "Cannot read property 'foo' of undefined".to_string(), mechanism: Default::default(), module: Default::default(), thread_id: None, - stacktrace: Default::default(), + stack: Default::default(), }; let resolved_frames = vec![ @@ -218,7 +196,11 @@ mod test { }, ]; - let fingerprint = super::generate_fingerprint(&exception, resolved_frames).unwrap(); + exception.stack = Some(Stacktrace::Resolved { + frames: resolved_frames, + }); + + let fingerprint = super::generate_fingerprint(&[exception]); assert_eq!( fingerprint, "e2a134db8585bafb19fa6c15b92bd46c699e06df7f39ce74281227e7c915adf0c997ab61757820710d85817b4a11823e57f9bcae526432c085b53e28397f3007" @@ -227,13 +209,13 @@ mod test { #[test] fn test_no_in_app_frames() { - let exception = Exception { + let mut exception = Exception { exception_type: "TypeError".to_string(), exception_message: "Cannot read property 'foo' of undefined".to_string(), mechanism: Default::default(), module: Default::default(), thread_id: None, - stacktrace: Default::default(), + stack: Default::default(), }; let resolved_frames = vec![ @@ -261,7 +243,11 @@ mod test { }, ]; - let fingerprint = super::generate_fingerprint(&exception, resolved_frames).unwrap(); + exception.stack = Some(Stacktrace::Resolved { + frames: resolved_frames, + }); + + let fingerprint = super::generate_fingerprint(&[exception]); assert_eq!( fingerprint, "c0fea2676a395a11577fd7a7b3006bcc416f6ceeb13290f831c4b5dc4de443ff51c5866d5c378137307c25cc8837cca6c82737e9704c47ed8408ff3937033e70" diff --git a/rust/cymbal/src/fingerprinting/mod.rs b/rust/cymbal/src/fingerprinting/mod.rs deleted file mode 100644 index a3a6d96c3f59d..0000000000000 --- a/rust/cymbal/src/fingerprinting/mod.rs +++ /dev/null @@ -1 +0,0 @@ -pub mod v1; diff --git a/rust/cymbal/src/main.rs b/rust/cymbal/src/main.rs index 12c45067fa19c..e0f34ff77acc8 100644 --- a/rust/cymbal/src/main.rs +++ b/rust/cymbal/src/main.rs @@ -9,11 +9,8 @@ use cymbal::{ config::Config, error::Error, fingerprinting, - metric_consts::{ - ERRORS, EVENT_RECEIVED, MAIN_LOOP_TIME, PER_FRAME_GROUP_TIME, PER_STACK_TIME, - STACK_PROCESSED, - }, - types::{frames::RawFrame, ErrProps}, + metric_consts::{ERRORS, EVENT_RECEIVED, MAIN_LOOP_TIME, STACK_PROCESSED}, + types::{ErrProps, Stacktrace}, }; use envconfig::Envconfig; use tokio::task::JoinHandle; @@ -104,7 +101,7 @@ async fn main() -> Result<(), Error> { } }; - let Some(exception_list) = &properties.exception_list else { + let Some(mut exception_list) = properties.exception_list else { // Known issue that $exception_list didn't exist on old clients continue; }; @@ -114,49 +111,46 @@ async fn main() -> Result<(), Error> { continue; } - // TODO - we should resolve all traces - let Some(trace) = exception_list[0].stacktrace.as_ref() else { - metrics::counter!(ERRORS, "cause" => "no_stack_trace").increment(1); - continue; - }; + for exception in exception_list.iter_mut() { + let stack = std::mem::take(&mut exception.stack); + let Some(Stacktrace::Raw { frames }) = stack else { + continue; + }; - let stack_trace: &Vec = &trace.frames; + if frames.is_empty() { + metrics::counter!(ERRORS, "cause" => "no_frames").increment(1); + continue; + } - let per_stack = common_metrics::timing_guard(PER_STACK_TIME, &[]); + let team_id = event.team_id; + let mut results = Vec::with_capacity(frames.len()); - // Cluster the frames by symbol set - let mut groups = HashMap::new(); - for frame in stack_trace { - let group = groups - .entry(frame.symbol_set_group_key()) - .or_insert_with(Vec::new); - group.push(frame.clone()); - } + // Cluster the frames by symbol set + let mut groups = HashMap::new(); + for (i, frame) in frames.into_iter().enumerate() { + let group = groups + .entry(frame.symbol_set_group_key()) + .or_insert_with(Vec::new); + group.push((i, frame.clone())); + } - let team_id = event.team_id; - let mut results = Vec::with_capacity(stack_trace.len()); - for (_, frames) in groups.into_iter() { - context.worker_liveness.report_healthy().await; // TODO - we shouldn't need to do this, but we do for now. - for frame in frames { - results.push(frame.resolve(team_id, &context.catalog).await?); + for (_, frames) in groups.into_iter() { + context.worker_liveness.report_healthy().await; // TODO - we shouldn't need to do this, but we do for now. + for (i, frame) in frames { + results.push((i, frame.resolve(team_id, &context.catalog).await?)); + } } - } - per_stack - .label( - "resolved_any", - if results.is_empty() { "true" } else { "false" }, - ) - .fin(); - whole_loop.label("had_frame", "true").fin(); + results.sort_unstable_by_key(|(i, _)| *i); - let Ok(_fingerprint) = - fingerprinting::v1::generate_fingerprint(&exception_list[0], results) - else { - metrics::counter!(ERRORS, "cause" => "fingerprint_generation_failed").increment(1); - continue; - }; + exception.stack = Some(Stacktrace::Resolved { + frames: results.into_iter().map(|(_, frame)| frame).collect(), + }); + } + + let _fingerprint = fingerprinting::generate_fingerprint(&exception_list); metrics::counter!(STACK_PROCESSED).increment(1); + whole_loop.label("had_frame", "true").fin(); } } diff --git a/rust/cymbal/src/types/frames.rs b/rust/cymbal/src/types/frames.rs index f14840cfb0073..8f7943f97fecb 100644 --- a/rust/cymbal/src/types/frames.rs +++ b/rust/cymbal/src/types/frames.rs @@ -1,4 +1,5 @@ use serde::{Deserialize, Serialize}; +use sha2::{Digest, Sha512}; use crate::{ error::Error, langs::js::RawJSFrame, metric_consts::PER_FRAME_TIME, symbol_store::Catalog, @@ -35,7 +36,7 @@ impl RawFrame { } // We emit a single, unified representation of a frame, which is what we pass on to users. -#[derive(Debug, Clone, Serialize)] +#[derive(Debug, Clone, Serialize, Deserialize)] pub struct Frame { pub mangled_name: String, // Mangled name of the function pub line: Option, // Line the function is define on, if known @@ -47,3 +48,27 @@ pub struct Frame { pub resolved: bool, // Did we manage to resolve the frame? pub resolve_failure: Option, // If we failed to resolve the frame, why? } + +impl Frame { + pub fn include_in_fingerprint(&self, h: &mut Sha512) { + if let Some(resolved) = &self.resolved_name { + h.update(resolved.as_bytes()); + } else { + h.update(self.mangled_name.as_bytes()); + } + + if let Some(source) = &self.source { + h.update(source.as_bytes()); + } + + if let Some(line) = self.line { + h.update(line.to_string().as_bytes()); + } + + if let Some(column) = self.column { + h.update(column.to_string().as_bytes()); + } + + h.update(self.lang.as_bytes()); + } +} diff --git a/rust/cymbal/src/types/mod.rs b/rust/cymbal/src/types/mod.rs index 3a420e6410601..861157c4e264d 100644 --- a/rust/cymbal/src/types/mod.rs +++ b/rust/cymbal/src/types/mod.rs @@ -1,7 +1,9 @@ use std::collections::HashMap; +use frames::{Frame, RawFrame}; use serde::{Deserialize, Serialize}; use serde_json::Value; +use sha2::{digest::Update, Sha512}; pub mod frames; @@ -19,8 +21,10 @@ pub struct Mechanism { } #[derive(Debug, Deserialize, Serialize, Clone)] -pub struct Stacktrace { - pub frames: Vec, +#[serde(tag = "type", rename_all = "lowercase")] +pub enum Stacktrace { + Raw { frames: Vec }, + Resolved { frames: Vec }, } #[derive(Debug, Deserialize, Serialize, Clone)] @@ -35,7 +39,7 @@ pub struct Exception { #[serde(skip_serializing_if = "Option::is_none")] pub thread_id: Option, #[serde(skip_serializing_if = "Option::is_none")] - pub stacktrace: Option, + pub stack: Option, } // Given a Clickhouse Event's properties, we care about the contents @@ -60,12 +64,30 @@ pub struct ErrProps { pub other: HashMap, } +impl Exception { + pub fn include_in_fingerprint(&self, h: &mut Sha512) { + h.update(self.exception_type.as_bytes()); + h.update(self.exception_message.as_bytes()); + let Some(Stacktrace::Resolved { frames }) = &self.stack else { + return; + }; + + let has_no_resolved = !frames.iter().any(|f| f.resolved); + + for frame in frames { + if has_no_resolved || frame.resolved { + frame.include_in_fingerprint(h) + } + } + } +} + #[cfg(test)] mod test { use common_types::ClickHouseEvent; use serde_json::Error; - use crate::types::frames::RawFrame; + use crate::types::{frames::RawFrame, Stacktrace}; use super::ErrProps; @@ -93,9 +115,11 @@ mod test { assert_eq!(mechanism.source, None); assert_eq!(mechanism.synthetic, Some(false)); - let stacktrace = exception_list[0].stacktrace.as_ref().unwrap(); - assert_eq!(stacktrace.frames.len(), 2); - let RawFrame::JavaScript(frame) = &stacktrace.frames[0]; + let Stacktrace::Raw { frames } = exception_list[0].stack.as_ref().unwrap() else { + panic!("Expected a Raw stacktrace") + }; + assert_eq!(frames.len(), 2); + let RawFrame::JavaScript(frame) = &frames[0]; assert_eq!( frame.source_url, @@ -106,7 +130,7 @@ mod test { assert_eq!(frame.line, 64); assert_eq!(frame.column, 25112); - let RawFrame::JavaScript(frame) = &stacktrace.frames[1]; + let RawFrame::JavaScript(frame) = &frames[1]; assert_eq!( frame.source_url, Some("https://app-static.eu.posthog.com/static/chunk-PGUQKT6S.js".to_string()) diff --git a/rust/cymbal/tests/resolve.rs b/rust/cymbal/tests/resolve.rs index 1be56aeb90536..ec71862e55793 100644 --- a/rust/cymbal/tests/resolve.rs +++ b/rust/cymbal/tests/resolve.rs @@ -2,7 +2,7 @@ use common_types::ClickHouseEvent; use cymbal::{ config::Config, symbol_store::{sourcemap::SourcemapProvider, Catalog}, - types::{frames::RawFrame, ErrProps}, + types::{frames::RawFrame, ErrProps, Stacktrace}, }; use httpmock::MockServer; @@ -28,12 +28,12 @@ async fn end_to_end_resolver_test() { let exception: ClickHouseEvent = serde_json::from_str(EXAMPLE_EXCEPTION).unwrap(); let props: ErrProps = serde_json::from_str(&exception.properties.unwrap()).unwrap(); - let mut test_stack: Vec = props.exception_list.unwrap()[0] - .stacktrace - .as_ref() - .unwrap() - .frames - .clone(); + let Stacktrace::Raw { + frames: mut test_stack, + } = props.exception_list.unwrap().swap_remove(0).stack.unwrap() + else { + panic!("Expected a Raw stacktrace") + }; // We're going to pretend out stack consists exclusively of JS frames whose source // we have locally @@ -42,7 +42,7 @@ async fn end_to_end_resolver_test() { s.source_url.as_ref().unwrap().contains(CHUNK_PATH) }); - for frame in &mut test_stack { + for frame in test_stack.iter_mut() { let RawFrame::JavaScript(frame) = frame; // Our test data contains our /actual/ source urls - we need to swap that to localhost // When I first wrote this test, I forgot to do this, and it took me a while to figure out From 9577de8f359a26fb6a84fea74ae4fc4112d558ab Mon Sep 17 00:00:00 2001 From: Oliver Browne Date: Mon, 4 Nov 2024 14:37:37 +0100 Subject: [PATCH 14/21] tests, ignoring in some cases --- rust/cymbal/src/fingerprinting.rs | 118 +++++++++++++++++------------- rust/cymbal/src/types/frames.rs | 3 + 2 files changed, 69 insertions(+), 52 deletions(-) diff --git a/rust/cymbal/src/fingerprinting.rs b/rust/cymbal/src/fingerprinting.rs index 415e6af641e38..9b6f07041be8b 100644 --- a/rust/cymbal/src/fingerprinting.rs +++ b/rust/cymbal/src/fingerprinting.rs @@ -87,7 +87,7 @@ mod test { let fingerprint = super::generate_fingerprint(&[exception]); assert_eq!( fingerprint, - "e2a134db8585bafb19fa6c15b92bd46c699e06df7f39ce74281227e7c915adf0c997ab61757820710d85817b4a11823e57f9bcae526432c085b53e28397f3007" + "c31e87e59707d377bd211fe0b66af1bec9918ad7a750fee0cada2c68f95aa7e464c0230a92046096233285b303f825d2d398f659c903f3b14df7806b40b1d886" ); } @@ -102,7 +102,7 @@ mod test { stack: Default::default(), }; - let resolved_frames = vec![ + let mut resolved_frames = vec![ Frame { mangled_name: "foo".to_string(), line: Some(10), @@ -125,28 +125,36 @@ mod test { resolve_failure: None, lang: "javascript".to_string(), }, - Frame { - mangled_name: "xyz".to_string(), - line: Some(30), - column: Some(25), - source: None, - in_app: true, - resolved_name: None, - resolved: false, - resolve_failure: None, - lang: "javascript".to_string(), - }, ]; + let unresolved_frame = Frame { + mangled_name: "xyz".to_string(), + line: Some(30), + column: Some(25), + source: None, + in_app: true, + resolved_name: None, + resolved: false, + resolve_failure: None, + lang: "javascript".to_string(), + }; + + exception.stack = Some(Stacktrace::Resolved { + frames: resolved_frames.clone(), + }); + + let fingerprint_with_all_resolved = super::generate_fingerprint(&[exception.clone()]); + + resolved_frames.push(unresolved_frame); exception.stack = Some(Stacktrace::Resolved { frames: resolved_frames, }); - let fingerprint = super::generate_fingerprint(&[exception]); - assert_eq!( - fingerprint, - "528bf82706cdcf928b3382d6bfb717c9572aa6855ffe8b620132a813dd54f2ee5c13437d22bb557634c4ceec104ce9c8c5a87d5d7cf36239be052ace8b9962aa" - ); + let mixed_fingerprint = super::generate_fingerprint(&[exception]); + + // In cases where there are SOME resolved frames, the fingerprint should be identical + // to the case where all frames are resolved (unresolved frames should be ignored) + assert_eq!(fingerprint_with_all_resolved, mixed_fingerprint); } #[test] @@ -196,15 +204,16 @@ mod test { }, ]; + let no_stack_fingerprint = super::generate_fingerprint(&[exception.clone()]); + exception.stack = Some(Stacktrace::Resolved { frames: resolved_frames, }); - let fingerprint = super::generate_fingerprint(&[exception]); - assert_eq!( - fingerprint, - "e2a134db8585bafb19fa6c15b92bd46c699e06df7f39ce74281227e7c915adf0c997ab61757820710d85817b4a11823e57f9bcae526432c085b53e28397f3007" - ); + let with_stack_fingerprint = super::generate_fingerprint(&[exception]); + + // If there are NO resolved frames, fingerprinting should account for the unresolved frames + assert_ne!(no_stack_fingerprint, with_stack_fingerprint); } #[test] @@ -218,39 +227,44 @@ mod test { stack: Default::default(), }; - let resolved_frames = vec![ - Frame { - mangled_name: "foo".to_string(), - line: Some(10), - column: Some(5), - source: Some("http://example.com/alpha/foo.js".to_string()), - in_app: false, - resolved_name: Some("bar".to_string()), - resolved: false, - resolve_failure: None, - lang: "javascript".to_string(), - }, - Frame { - mangled_name: "bar".to_string(), - line: Some(20), - column: Some(15), - source: Some("http://example.com/bar.js".to_string()), - in_app: false, - resolved_name: Some("baz".to_string()), - resolved: false, - resolve_failure: None, - lang: "javascript".to_string(), - }, - ]; + let mut resolved_frames = vec![Frame { + mangled_name: "foo".to_string(), + line: Some(10), + column: Some(5), + source: Some("http://example.com/alpha/foo.js".to_string()), + in_app: true, + resolved_name: Some("bar".to_string()), + resolved: false, + resolve_failure: None, + lang: "javascript".to_string(), + }]; + + let non_app_frame = Frame { + mangled_name: "bar".to_string(), + line: Some(20), + column: Some(15), + source: Some("http://example.com/bar.js".to_string()), + in_app: false, + resolved_name: Some("baz".to_string()), + resolved: false, + resolve_failure: None, + lang: "javascript".to_string(), + }; + + exception.stack = Some(Stacktrace::Resolved { + frames: resolved_frames.clone(), + }); + let fingerprint_1 = super::generate_fingerprint(&[exception.clone()]); + + resolved_frames.push(non_app_frame); exception.stack = Some(Stacktrace::Resolved { frames: resolved_frames, }); - let fingerprint = super::generate_fingerprint(&[exception]); - assert_eq!( - fingerprint, - "c0fea2676a395a11577fd7a7b3006bcc416f6ceeb13290f831c4b5dc4de443ff51c5866d5c378137307c25cc8837cca6c82737e9704c47ed8408ff3937033e70" - ); + let fingerprint_2 = super::generate_fingerprint(&[exception]); + + // Fingerprinting should ignore non-in-app frames + assert_eq!(fingerprint_1, fingerprint_2); } } diff --git a/rust/cymbal/src/types/frames.rs b/rust/cymbal/src/types/frames.rs index 8f7943f97fecb..6040dd0692f29 100644 --- a/rust/cymbal/src/types/frames.rs +++ b/rust/cymbal/src/types/frames.rs @@ -51,6 +51,9 @@ pub struct Frame { impl Frame { pub fn include_in_fingerprint(&self, h: &mut Sha512) { + if !self.in_app { + return; + } if let Some(resolved) = &self.resolved_name { h.update(resolved.as_bytes()); } else { From 646e3f6bd70c24e8289fa12c8af932acecc06b52 Mon Sep 17 00:00:00 2001 From: Oliver Browne Date: Mon, 4 Nov 2024 14:42:18 +0100 Subject: [PATCH 15/21] rm comment --- rust/cymbal/src/fingerprinting.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/rust/cymbal/src/fingerprinting.rs b/rust/cymbal/src/fingerprinting.rs index 9b6f07041be8b..63affc6b6cfd5 100644 --- a/rust/cymbal/src/fingerprinting.rs +++ b/rust/cymbal/src/fingerprinting.rs @@ -8,8 +8,6 @@ pub fn generate_fingerprint(exception: &[Exception]) -> String { for exc in exception { exc.include_in_fingerprint(&mut hasher); } - // TODO: Handle anonymous functions somehow? Not sure if these would have a resolved name at all. How would they show up - // as unresolved names? let result = hasher.finalize(); From 6aec37e9bbb4365b5767ea7ff92f54951c766550 Mon Sep 17 00:00:00 2001 From: Oliver Browne Date: Tue, 5 Nov 2024 11:13:20 +0100 Subject: [PATCH 16/21] use first frame if no in app frames --- rust/cymbal/src/types/mod.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/rust/cymbal/src/types/mod.rs b/rust/cymbal/src/types/mod.rs index 861157c4e264d..9b006069cb6fb 100644 --- a/rust/cymbal/src/types/mod.rs +++ b/rust/cymbal/src/types/mod.rs @@ -73,6 +73,13 @@ impl Exception { }; let has_no_resolved = !frames.iter().any(|f| f.resolved); + let has_no_in_app = !frames.iter().any(|f| f.in_app); + + if has_no_in_app { + // TODO: this could be better. + frames.get(0).map(|f| f.include_in_fingerprint(h)); + return; + } for frame in frames { if has_no_resolved || frame.resolved { From d8e1dcc80f2f35cf1c35f77b5283844cabf66ca2 Mon Sep 17 00:00:00 2001 From: Oliver Browne Date: Tue, 5 Nov 2024 11:14:21 +0100 Subject: [PATCH 17/21] fix --- rust/cymbal/src/types/frames.rs | 3 --- rust/cymbal/src/types/mod.rs | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/rust/cymbal/src/types/frames.rs b/rust/cymbal/src/types/frames.rs index 6040dd0692f29..8f7943f97fecb 100644 --- a/rust/cymbal/src/types/frames.rs +++ b/rust/cymbal/src/types/frames.rs @@ -51,9 +51,6 @@ pub struct Frame { impl Frame { pub fn include_in_fingerprint(&self, h: &mut Sha512) { - if !self.in_app { - return; - } if let Some(resolved) = &self.resolved_name { h.update(resolved.as_bytes()); } else { diff --git a/rust/cymbal/src/types/mod.rs b/rust/cymbal/src/types/mod.rs index 9b006069cb6fb..059e2e0ed4d90 100644 --- a/rust/cymbal/src/types/mod.rs +++ b/rust/cymbal/src/types/mod.rs @@ -82,7 +82,7 @@ impl Exception { } for frame in frames { - if has_no_resolved || frame.resolved { + if (has_no_resolved || frame.resolved) && frame.in_app { frame.include_in_fingerprint(h) } } From aa65c639beedf56e4b66207d4b0c1594b8fb2756 Mon Sep 17 00:00:00 2001 From: Oliver Browne Date: Tue, 5 Nov 2024 11:19:20 +0100 Subject: [PATCH 18/21] fix test --- rust/cymbal/src/types/mod.rs | 2 +- rust/cymbal/tests/static/raw_ch_exception_list.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/cymbal/src/types/mod.rs b/rust/cymbal/src/types/mod.rs index 059e2e0ed4d90..691d8fcf66f6f 100644 --- a/rust/cymbal/src/types/mod.rs +++ b/rust/cymbal/src/types/mod.rs @@ -38,7 +38,7 @@ pub struct Exception { pub module: Option, #[serde(skip_serializing_if = "Option::is_none")] pub thread_id: Option, - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(skip_serializing_if = "Option::is_none", rename = "stacktrace")] pub stack: Option, } diff --git a/rust/cymbal/tests/static/raw_ch_exception_list.json b/rust/cymbal/tests/static/raw_ch_exception_list.json index 7e7080c8fcf12..1c8eebbdc97e8 100644 --- a/rust/cymbal/tests/static/raw_ch_exception_list.json +++ b/rust/cymbal/tests/static/raw_ch_exception_list.json @@ -1,7 +1,7 @@ { "uuid": "019295b1-519f-71a6-aacf-c97b5db73696", "event": "$exception", - "properties": "{\"$os\":\"Mac OS X\",\"$os_version\":\"10.15.7\",\"$browser\":\"Chrome\",\"$device_type\":\"Desktop\",\"$current_url\":\"https://eu.posthog.com/project/12557/feature_flags/31624\",\"$host\":\"eu.posthog.com\",\"$pathname\":\"/project/12557/feature_flags/31624\",\"$raw_user_agent\":\"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/129.0.0.0 Safari/537.36\",\"$browser_version\":129,\"$browser_language\":\"en-GB\",\"$screen_height\":1080,\"$screen_width\":1920,\"$viewport_height\":934,\"$viewport_width\":1920,\"$lib\":\"web\",\"$lib_version\":\"1.170.1\",\"$insert_id\":\"xjfjg606eo2x7n4x\",\"$time\":1729088278.943,\"distinct_id\":\"pQC9X9Fe7BPzJXVxpY0fx37UwFOCd1vXHzh8rjUPv1G\",\"$device_id\":\"018ccedb-d598-79bb-94e0-4751a3b956f4\",\"$console_log_recording_enabled_server_side\":true,\"$autocapture_disabled_server_side\":false,\"$web_vitals_enabled_server_side\":true,\"$exception_capture_enabled_server_side\":true,\"$exception_capture_endpoint\":\"/e/\",\"realm\":\"cloud\",\"email_service_available\":true,\"slack_service_available\":true,\"commit_sha\":\"bafa32953e\",\"$user_id\":\"pQC9X9Fe7BPzJXVxpY0fx37UwFOCd1vXHzh8rjUPv1G\",\"is_demo_project\":false,\"$groups\":{\"project\":\"018c1057-288d-0000-93bb-3bd44c845f22\",\"organization\":\"018afaa6-8b2e-0000-2311-d58d2df832ad\",\"customer\":\"cus_P5B9QmoUKLAUlx\",\"instance\":\"https://eu.posthog.com\"},\"has_billing_plan\":true,\"$referrer\":\"$direct\",\"$referring_domain\":\"$direct\",\"$session_recording_start_reason\":\"session_id_changed\",\"$exception_list\":[{\"type\":\"UnhandledRejection\",\"value\":\"Unexpected usage\",\"stacktrace\":{\"frames\":[{\"filename\":\"https://app-static.eu.posthog.com/static/chunk-PGUQKT6S.js\",\"function\":\"?\",\"in_app\":true,\"lineno\":64,\"colno\":25112},{\"filename\":\"https://app-static.eu.posthog.com/static/chunk-PGUQKT6S.js\",\"function\":\"n.loadForeignModule\",\"in_app\":true,\"lineno\":64,\"colno\":15003}]},\"mechanism\":{\"handled\":false,\"synthetic\":false}}],\"$exception_level\":\"error\",\"$exception_personURL\":\"https://us.posthog.com/project/sTMFPsFhdP1Ssg/person/pQC9X9Fe7BPzJXVxpY0fx37UwFOCd1vXHzh8rjUPv1G\",\"token\":\"sTMFPsFhdP1Ssg\",\"$session_id\":\"019295b0-db2b-7e02-8010-0a1c4db680df\",\"$window_id\":\"019295b0-db2b-7e02-8010-0a1dee88e5f5\",\"$lib_custom_api_host\":\"https://internal-t.posthog.com\",\"$is_identified\":true,\"$lib_rate_limit_remaining_tokens\":97.28999999999999,\"$sent_at\":\"2024-10-16T14:17:59.543000+00:00\",\"$geoip_city_name\":\"Lisbon\",\"$geoip_city_confidence\":null,\"$geoip_country_name\":\"Portugal\",\"$geoip_country_code\":\"PT\",\"$geoip_country_confidence\":null,\"$geoip_continent_name\":\"Europe\",\"$geoip_continent_code\":\"EU\",\"$geoip_postal_code\":\"1269-001\",\"$geoip_postal_code_confidence\":null,\"$geoip_latitude\":38.731,\"$geoip_longitude\":-9.1373,\"$geoip_accuracy_radius\":100,\"$geoip_time_zone\":\"Europe/Lisbon\",\"$geoip_subdivision_1_code\":\"11\",\"$geoip_subdivision_1_name\":\"Lisbon\",\"$geoip_subdivision_1_confidence\":null,\"$lib_version__major\":1,\"$lib_version__minor\":170,\"$lib_version__patch\":1,\"$group_2\":\"018c1057-288d-0000-93bb-3bd44c845f22\",\"$group_0\":\"018afaa6-8b2e-0000-2311-d58d2df832ad\",\"$group_3\":\"cus_P5B9QmoUKLAUlx\",\"$group_1\":\"https://eu.posthog.com\"}", + "properties": "{\"$os\":\"Mac OS X\",\"$os_version\":\"10.15.7\",\"$browser\":\"Chrome\",\"$device_type\":\"Desktop\",\"$current_url\":\"https://eu.posthog.com/project/12557/feature_flags/31624\",\"$host\":\"eu.posthog.com\",\"$pathname\":\"/project/12557/feature_flags/31624\",\"$raw_user_agent\":\"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/129.0.0.0 Safari/537.36\",\"$browser_version\":129,\"$browser_language\":\"en-GB\",\"$screen_height\":1080,\"$screen_width\":1920,\"$viewport_height\":934,\"$viewport_width\":1920,\"$lib\":\"web\",\"$lib_version\":\"1.170.1\",\"$insert_id\":\"xjfjg606eo2x7n4x\",\"$time\":1729088278.943,\"distinct_id\":\"pQC9X9Fe7BPzJXVxpY0fx37UwFOCd1vXHzh8rjUPv1G\",\"$device_id\":\"018ccedb-d598-79bb-94e0-4751a3b956f4\",\"$console_log_recording_enabled_server_side\":true,\"$autocapture_disabled_server_side\":false,\"$web_vitals_enabled_server_side\":true,\"$exception_capture_enabled_server_side\":true,\"$exception_capture_endpoint\":\"/e/\",\"realm\":\"cloud\",\"email_service_available\":true,\"slack_service_available\":true,\"commit_sha\":\"bafa32953e\",\"$user_id\":\"pQC9X9Fe7BPzJXVxpY0fx37UwFOCd1vXHzh8rjUPv1G\",\"is_demo_project\":false,\"$groups\":{\"project\":\"018c1057-288d-0000-93bb-3bd44c845f22\",\"organization\":\"018afaa6-8b2e-0000-2311-d58d2df832ad\",\"customer\":\"cus_P5B9QmoUKLAUlx\",\"instance\":\"https://eu.posthog.com\"},\"has_billing_plan\":true,\"$referrer\":\"$direct\",\"$referring_domain\":\"$direct\",\"$session_recording_start_reason\":\"session_id_changed\",\"$exception_list\":[{\"type\":\"UnhandledRejection\",\"value\":\"Unexpected usage\",\"stacktrace\":{\"type\": \"raw\", \"frames\":[{\"filename\":\"https://app-static.eu.posthog.com/static/chunk-PGUQKT6S.js\",\"function\":\"?\",\"in_app\":true,\"lineno\":64,\"colno\":25112},{\"filename\":\"https://app-static.eu.posthog.com/static/chunk-PGUQKT6S.js\",\"function\":\"n.loadForeignModule\",\"in_app\":true,\"lineno\":64,\"colno\":15003}]},\"mechanism\":{\"handled\":false,\"synthetic\":false}}],\"$exception_level\":\"error\",\"$exception_personURL\":\"https://us.posthog.com/project/sTMFPsFhdP1Ssg/person/pQC9X9Fe7BPzJXVxpY0fx37UwFOCd1vXHzh8rjUPv1G\",\"token\":\"sTMFPsFhdP1Ssg\",\"$session_id\":\"019295b0-db2b-7e02-8010-0a1c4db680df\",\"$window_id\":\"019295b0-db2b-7e02-8010-0a1dee88e5f5\",\"$lib_custom_api_host\":\"https://internal-t.posthog.com\",\"$is_identified\":true,\"$lib_rate_limit_remaining_tokens\":97.28999999999999,\"$sent_at\":\"2024-10-16T14:17:59.543000+00:00\",\"$geoip_city_name\":\"Lisbon\",\"$geoip_city_confidence\":null,\"$geoip_country_name\":\"Portugal\",\"$geoip_country_code\":\"PT\",\"$geoip_country_confidence\":null,\"$geoip_continent_name\":\"Europe\",\"$geoip_continent_code\":\"EU\",\"$geoip_postal_code\":\"1269-001\",\"$geoip_postal_code_confidence\":null,\"$geoip_latitude\":38.731,\"$geoip_longitude\":-9.1373,\"$geoip_accuracy_radius\":100,\"$geoip_time_zone\":\"Europe/Lisbon\",\"$geoip_subdivision_1_code\":\"11\",\"$geoip_subdivision_1_name\":\"Lisbon\",\"$geoip_subdivision_1_confidence\":null,\"$lib_version__major\":1,\"$lib_version__minor\":170,\"$lib_version__patch\":1,\"$group_2\":\"018c1057-288d-0000-93bb-3bd44c845f22\",\"$group_0\":\"018afaa6-8b2e-0000-2311-d58d2df832ad\",\"$group_3\":\"cus_P5B9QmoUKLAUlx\",\"$group_1\":\"https://eu.posthog.com\"}", "timestamp": "2024-10-16T07:17:59.088000-07:00", "team_id": 2, "distinct_id": "pQC9X9Fe7BPzJXVxpY0fx37UwFOCd1vXHzh8rjUPv1G", From d20e00f8b740d0990c741c8ebb4253defa649083 Mon Sep 17 00:00:00 2001 From: Oliver Browne Date: Tue, 5 Nov 2024 11:58:15 +0100 Subject: [PATCH 19/21] fingerprinting needs to be a bit clever --- rust/cymbal/src/langs/js.rs | 2 +- rust/cymbal/src/types/frames.rs | 6 ++++-- rust/cymbal/src/types/mod.rs | 3 ++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/rust/cymbal/src/langs/js.rs b/rust/cymbal/src/langs/js.rs index e8921d614518d..2b9d8fa00258d 100644 --- a/rust/cymbal/src/langs/js.rs +++ b/rust/cymbal/src/langs/js.rs @@ -127,7 +127,7 @@ impl From<(&RawJSFrame, JsResolveErr)> for Frame { mangled_name: raw_frame.fn_name.clone(), line: Some(raw_frame.line), column: Some(raw_frame.column), - source: raw_frame.source_url.clone(), + source: raw_frame.source_url().map(|u| u.path().to_string()).ok(), in_app: raw_frame.in_app, resolved_name: None, lang: "javascript".to_string(), diff --git a/rust/cymbal/src/types/frames.rs b/rust/cymbal/src/types/frames.rs index 8f7943f97fecb..12fec1b56e7b3 100644 --- a/rust/cymbal/src/types/frames.rs +++ b/rust/cymbal/src/types/frames.rs @@ -53,10 +53,12 @@ impl Frame { pub fn include_in_fingerprint(&self, h: &mut Sha512) { if let Some(resolved) = &self.resolved_name { h.update(resolved.as_bytes()); - } else { - h.update(self.mangled_name.as_bytes()); + self.source.as_ref().map(|s| h.update(s.as_bytes())); + return; } + h.update(self.mangled_name.as_bytes()); + if let Some(source) = &self.source { h.update(source.as_bytes()); } diff --git a/rust/cymbal/src/types/mod.rs b/rust/cymbal/src/types/mod.rs index 691d8fcf66f6f..999f583fa1f8f 100644 --- a/rust/cymbal/src/types/mod.rs +++ b/rust/cymbal/src/types/mod.rs @@ -76,7 +76,8 @@ impl Exception { let has_no_in_app = !frames.iter().any(|f| f.in_app); if has_no_in_app { - // TODO: this could be better. + // TODO: we should try to be smarter about handling the case when + // there are no in-app frames frames.get(0).map(|f| f.include_in_fingerprint(h)); return; } From db5d174343f6808475246a582171b005ffd34a12 Mon Sep 17 00:00:00 2001 From: Oliver Browne Date: Tue, 5 Nov 2024 11:59:37 +0100 Subject: [PATCH 20/21] test fix --- rust/cymbal/src/fingerprinting.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/cymbal/src/fingerprinting.rs b/rust/cymbal/src/fingerprinting.rs index 63affc6b6cfd5..6420b1677016b 100644 --- a/rust/cymbal/src/fingerprinting.rs +++ b/rust/cymbal/src/fingerprinting.rs @@ -85,7 +85,7 @@ mod test { let fingerprint = super::generate_fingerprint(&[exception]); assert_eq!( fingerprint, - "c31e87e59707d377bd211fe0b66af1bec9918ad7a750fee0cada2c68f95aa7e464c0230a92046096233285b303f825d2d398f659c903f3b14df7806b40b1d886" + "7f5c327cd3941f2da655d852eb4661b411440c080c7ff014feb920afde68beaffe663908d4ab5fb7b7f1e7ab7f1f7cd17949139e8f812b1c3ff0911fc5b68f37" ); } From 7e7bb6b41fa6d14ee19b441a9c45ac1fdfd727d8 Mon Sep 17 00:00:00 2001 From: Oliver Browne Date: Tue, 5 Nov 2024 12:11:15 +0100 Subject: [PATCH 21/21] clippy --- rust/cymbal/src/types/frames.rs | 4 +++- rust/cymbal/src/types/mod.rs | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/rust/cymbal/src/types/frames.rs b/rust/cymbal/src/types/frames.rs index 12fec1b56e7b3..5a1150c3c3842 100644 --- a/rust/cymbal/src/types/frames.rs +++ b/rust/cymbal/src/types/frames.rs @@ -53,7 +53,9 @@ impl Frame { pub fn include_in_fingerprint(&self, h: &mut Sha512) { if let Some(resolved) = &self.resolved_name { h.update(resolved.as_bytes()); - self.source.as_ref().map(|s| h.update(s.as_bytes())); + if let Some(s) = self.source.as_ref() { + h.update(s.as_bytes()) + } return; } diff --git a/rust/cymbal/src/types/mod.rs b/rust/cymbal/src/types/mod.rs index 999f583fa1f8f..57db29fb813cc 100644 --- a/rust/cymbal/src/types/mod.rs +++ b/rust/cymbal/src/types/mod.rs @@ -78,7 +78,9 @@ impl Exception { if has_no_in_app { // TODO: we should try to be smarter about handling the case when // there are no in-app frames - frames.get(0).map(|f| f.include_in_fingerprint(h)); + if let Some(f) = frames.first() { + f.include_in_fingerprint(h) + } return; }