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

Better in app module allow list default #854

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

solnic
Copy link
Collaborator

@solnic solnic commented Jan 29, 2025

This adds otp_apps config option which is then used to auto-discover module names for the in_app_module_allow_list option.

Closes #800

@solnic solnic linked an issue Jan 29, 2025 that may be closed by this pull request
@solnic solnic force-pushed the solnic/800-better-in_app_module_allow_list-default branch from eeede8c to aa7273a Compare January 29, 2025 14:33
@solnic solnic force-pushed the solnic/800-better-in_app_module_allow_list-default branch from aa7273a to 7c2ce0e Compare January 30, 2025 13:06
@solnic solnic force-pushed the solnic/800-better-in_app_module_allow_list-default branch from 7c2ce0e to 901d184 Compare January 30, 2025 13:09
@solnic solnic marked this pull request as ready for review January 30, 2025 13:27
@solnic solnic requested a review from whatyouhide January 30, 2025 13:27
@sl0thentr0py
Copy link
Member

sl0thentr0py commented Feb 3, 2025

so if I understand correctly, the user will still have to supply these values?
Is there no way to just detect a project root and just use that so that it works ootb?
We really want a good ootb experience here, that's kinda important.

Copy link
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

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

we can merge this but let's also please make the ootb experience possible

@solnic
Copy link
Collaborator Author

solnic commented Feb 3, 2025

so if I understand correctly, the user will still have to supply these values?

No, the user would only have to provide names of applications that should be considered when auto-discovering the modules. For example if you're building an app called Blog and there's a corresponding :blog OTP app, then you just configure it in otp_apps: [:blog] and every module from this app will be added to in_app_module_allow_list setting.

@sl0thentr0py
Copy link
Member

yes but that's still manual, why cannot we just detect a project root like other SDKs? we want good default ootb stacktraces

@solnic
Copy link
Collaborator Author

solnic commented Feb 3, 2025

@sl0thentr0py due to Elixir limitations, it cannot provide us with this information in a released Elixir app, so it does work in dev env, but not released prod.

@sl0thentr0py
Copy link
Member

sl0thentr0py commented Feb 3, 2025

okay but from here
#800 (comment)

If yes, then we get all the apps from the umbrella and do what's described below, otherwise just do it for the root app.

all the apps are already in the mix config, why do we ask for them again in a separate config?

@solnic
Copy link
Collaborator Author

solnic commented Feb 3, 2025

okay but from here #800 (comment)

If yes, then we get all the apps from the umbrella and do what's described below, otherwise just do it for the root app.

all the apps are already in the mix config, why do we ask for them again in a separate config?

It's explained here #854 (comment)

lib/sentry/application.ex Outdated Show resolved Hide resolved
lib/sentry/config.ex Outdated Show resolved Hide resolved
@solnic solnic requested a review from whatyouhide February 6, 2025 11:29
Copy link
Collaborator

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

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

We need to mention the new option in :in_app_module_allow_list too

lib/sentry/config.ex Outdated Show resolved Hide resolved
@solnic solnic requested a review from whatyouhide February 6, 2025 15:57
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.

Better :in_app_module_allow_list default
3 participants