-
-
Notifications
You must be signed in to change notification settings - Fork 450
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
Add option to whitelist specific paths as 'in app' #909
Add option to whitelist specific paths as 'in app' #909
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time of preparing this contribution. I'll list some suggestion to further it:
- since this SDK follows Sentry's Unified APIs specification, this should become the implementation of the missing
in-app-include
option - you should add tests to verify your new feature
- you should add a new entry in the changelog
a29b524
to
a59af72
Compare
Since this is a new feature it must target the |
a59af72
to
585966a
Compare
585966a
to
898c185
Compare
898c185
to
4df49da
Compare
4df49da
to
717e0b9
Compare
6370823
to
64ce9b8
Compare
64ce9b8
to
1351b0d
Compare
1351b0d
to
222e93c
Compare
e2009f7
to
9e7aad8
Compare
@mfrischbutter I did some changes to your PR but it seems that I cannot push them because I don't have permissions for the branch of this PR. Would you be so kind to check if the "Allow edits from maintainers" flag is checked? |
@ste93cry |
I can edit the files from the web interface, so the permissions are ok. |
9e7aad8
to
00658f5
Compare
I just rebased onto develop. |
It should be enough, maybe the fact that the PR is not a branch owned by you but by another user (@mogic-le) is the problem as I get 403 as soon as I try to push
I just tried and yes, it opens the web editor although I didn't try to save any change
There are my changes that I would like to merge with this PR if everything is ok for you. I pushed the commit to my own fork, if you can cherry-pick it here and give your opinion it would be cool |
@mfrischbutter are you willing to rebase and merge the changes from my fork so that we can proceed with a new (and hopefully final) review of the code? |
00658f5
to
47596c9
Compare
Sorry it took a while, thanks for your patience. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it looks good, thank you for the work. LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢 thank you for your work and patience!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the hard work!
To bypass the default value for in_app_exclude from getsentry/sentry-laravel, I added the in_app_whitelist option.
The problem is that our own vendor/packages do not appear in the 'in app' stackstrace.
With this option it is possible to whitelist subfolders if you don't want to exclude all folders individually.
To use this option just add in_app_whitelist to your sentry configuration and define some relative paths as an array.
Resolves #595