Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(js): Recognize more abs_paths as in-app #1505

Merged
merged 5 commits into from
Aug 1, 2024
Merged

Conversation

loewenheim
Copy link
Contributor

The original logic for deciding whether a JS frame should be in-app (seen here) 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.

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.
@loewenheim loewenheim self-assigned this Aug 1, 2024
Copy link
Member

@armenzg armenzg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing this so quickly! Just some minor requests.

crates/symbolicator-js/src/utils.rs Outdated Show resolved Hide resolved
crates/symbolicator-js/src/utils.rs Show resolved Hide resolved
@loewenheim loewenheim requested a review from armenzg August 1, 2024 14:39
Copy link
Member

@armenzg armenzg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃🏻

@loewenheim loewenheim enabled auto-merge (squash) August 1, 2024 15:55
@loewenheim loewenheim merged commit 9ce2ad1 into master Aug 1, 2024
13 checks passed
@loewenheim loewenheim deleted the fix/js-in-app branch August 1, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants