Skip to content

Commit

Permalink
fix(js): Recognize more abs_paths as in-app (#1505)
Browse files Browse the repository at this point in the history
The original logic for deciding whether a JS frame should be in-app
(seen at 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.
  • Loading branch information
loewenheim authored Aug 1, 2024
1 parent 4533192 commit 9ce2ad1
Showing 1 changed file with 23 additions and 1 deletion.
24 changes: 23 additions & 1 deletion crates/symbolicator-js/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,17 @@ 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<bool> {
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/") {
Expand Down Expand Up @@ -718,6 +726,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:///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";

Expand Down

0 comments on commit 9ce2ad1

Please sign in to comment.