From c3135d888488e80c7e3dad99312ee6b1890a369d Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Thu, 1 Aug 2024 11:34:01 +0200 Subject: [PATCH 1/5] fix(js): Recognize more abs_paths as in-app The original logic for deciding whether a JS frame should be in-app (seen [here](https://github.com/getsentry/sentry/blob/69ee8d0fcbff3494f2d2a6fb9fb59195fc49b575/src/sentry/lang/javascript/processor.py#L1573-L1603)) is quite convoluted and difficult to understand. The `is_in_app` function reproduces it faithfully, but with some refactoring. In any case, for webpack, only file names beginning with `./` would be considered as in-app, but judging from examples that's too strict. Therefore we change the logic for webpack as follows: A file name is considered _not_ in-app if either of these is true: * It starts with `~/` (according to the documentation in the original Python version, this means it's "coming from node_modules"). * It contains `/node_modules/` as a substring. Otherwise it's considered in-app. --- crates/symbolicator-js/src/utils.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/crates/symbolicator-js/src/utils.rs b/crates/symbolicator-js/src/utils.rs index 4e0845b73..58f6c1767 100644 --- a/crates/symbolicator-js/src/utils.rs +++ b/crates/symbolicator-js/src/utils.rs @@ -111,7 +111,9 @@ pub fn fixup_webpack_filename(filename: &str) -> String { pub fn is_in_app(abs_path: &str, filename: &str) -> Option { if abs_path.starts_with("webpack:") { - Some(filename.starts_with("./") && !filename.contains("/node_modules/")) + // This diverges from the original logic. Previously we would only consider + // a filename starting with `./` as in-app, but that seems to be overly strict. + Some(!filename.starts_with("~/") && !filename.contains("/node_modules/")) } else if abs_path.starts_with("app:") { Some(!NODE_MODULES_RE.is_match(filename)) } else if abs_path.contains("/node_modules/") { @@ -713,6 +715,12 @@ mod tests { assert_eq!(is_in_app(abs_path, filename), Some(true)); assert_eq!(is_in_app_faithful(abs_path, filename), Some(true)); + let abs_path = "webpack:///@sentry/browser/esm/helpers.js"; + let filename = "@sentry/browser/esm/helpers.js"; + + assert_eq!(is_in_app(abs_path, filename), Some(true)); + assert_eq!(is_in_app_faithful(abs_path, filename), Some(false)); + let abs_path = "webpack:///./node_modules/@sentry/browser/esm/helpers.js"; let filename = "./node_modules/@sentry/browser/esm/helpers.js"; From cfdfc488f667cab01d7cf2ac07a734864b07cf7d Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Thu, 1 Aug 2024 16:33:02 +0200 Subject: [PATCH 2/5] Add link --- crates/symbolicator-js/src/utils.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/symbolicator-js/src/utils.rs b/crates/symbolicator-js/src/utils.rs index 58f6c1767..20a249a0e 100644 --- a/crates/symbolicator-js/src/utils.rs +++ b/crates/symbolicator-js/src/utils.rs @@ -109,6 +109,10 @@ pub fn fixup_webpack_filename(filename: &str) -> String { } } +/// Returns whether the given abs_path and filename should be considered in-app. +/// +/// This function was originally a simplified (but semantically faithful) version of +/// https://github.com/getsentry/sentry/blob/69ee8d0fcbff3494f2d2a6fb9fb59195fc49b575/src/sentry/lang/javascript/processor.py#L1573-L1603. pub fn is_in_app(abs_path: &str, filename: &str) -> Option { if abs_path.starts_with("webpack:") { // This diverges from the original logic. Previously we would only consider From 42137e4d813bb5ddd5a0a190a541d01e95bb47ab Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Thu, 1 Aug 2024 16:38:28 +0200 Subject: [PATCH 3/5] Adjust test case and add another --- crates/symbolicator-js/src/utils.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/crates/symbolicator-js/src/utils.rs b/crates/symbolicator-js/src/utils.rs index 20a249a0e..c0ab0a206 100644 --- a/crates/symbolicator-js/src/utils.rs +++ b/crates/symbolicator-js/src/utils.rs @@ -719,12 +719,20 @@ mod tests { assert_eq!(is_in_app(abs_path, filename), Some(true)); assert_eq!(is_in_app_faithful(abs_path, filename), Some(true)); - let abs_path = "webpack:///@sentry/browser/esm/helpers.js"; - let filename = "@sentry/browser/esm/helpers.js"; + let abs_path = "webpack:///foo/bar/src/App.jsx"; + let filename = "foo/bar/src/App.jsx"; + // Here we have a discrepancy because the behavior of `is_in_app` + // longer aligns with that of the original Python version. assert_eq!(is_in_app(abs_path, filename), Some(true)); assert_eq!(is_in_app_faithful(abs_path, filename), Some(false)); + let abs_path = "webpack:///./foo/bar/App.tsx"; + let filename = "./foo/bar/src/App.jsx"; + + assert_eq!(is_in_app(abs_path, filename), Some(true)); + assert_eq!(is_in_app_faithful(abs_path, filename), Some(true)); + let abs_path = "webpack:///./node_modules/@sentry/browser/esm/helpers.js"; let filename = "./node_modules/@sentry/browser/esm/helpers.js"; From 528dea5af9e11894edb21bd5f9b98d1c4fd03fa0 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Thu, 1 Aug 2024 16:38:46 +0200 Subject: [PATCH 4/5] Expand comment --- crates/symbolicator-js/src/utils.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/symbolicator-js/src/utils.rs b/crates/symbolicator-js/src/utils.rs index c0ab0a206..8e86d91e6 100644 --- a/crates/symbolicator-js/src/utils.rs +++ b/crates/symbolicator-js/src/utils.rs @@ -113,6 +113,8 @@ pub fn fixup_webpack_filename(filename: &str) -> String { /// /// This function was originally a simplified (but semantically faithful) version of /// https://github.com/getsentry/sentry/blob/69ee8d0fcbff3494f2d2a6fb9fb59195fc49b575/src/sentry/lang/javascript/processor.py#L1573-L1603. +/// For a version that preserves the original exactly (modulo Python -> Rust translation), see +/// `is_in_app_faithful`. pub fn is_in_app(abs_path: &str, filename: &str) -> Option { if abs_path.starts_with("webpack:") { // This diverges from the original logic. Previously we would only consider From 6924cd8fcbf10247eb9a49c0bef80dc1ce65a7be Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Thu, 1 Aug 2024 17:54:18 +0200 Subject: [PATCH 5/5] Fix link --- crates/symbolicator-js/src/utils.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/symbolicator-js/src/utils.rs b/crates/symbolicator-js/src/utils.rs index 8e86d91e6..d4a45226f 100644 --- a/crates/symbolicator-js/src/utils.rs +++ b/crates/symbolicator-js/src/utils.rs @@ -112,7 +112,7 @@ pub fn fixup_webpack_filename(filename: &str) -> String { /// Returns whether the given abs_path and filename should be considered in-app. /// /// This function was originally a simplified (but semantically faithful) version of -/// https://github.com/getsentry/sentry/blob/69ee8d0fcbff3494f2d2a6fb9fb59195fc49b575/src/sentry/lang/javascript/processor.py#L1573-L1603. +/// . /// For a version that preserves the original exactly (modulo Python -> Rust translation), see /// `is_in_app_faithful`. pub fn is_in_app(abs_path: &str, filename: &str) -> Option {