-
Notifications
You must be signed in to change notification settings - Fork 515
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
Centralized way to configure custom instrumentation #1960
Conversation
# ex: "mymodule.submodule.MyClassName.member_function" | ||
|
||
module_name, class_name = module_name.rsplit(".", 1) | ||
module_obj = import_module(module_name) |
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.
this import_module
is a lot of magic, because
- you might accidentally run user code that was not supposed to run just because sentry needs to know what's in that namespace
- is also a potential security issue / injection vector since you're letting users specify arbitrary modules and then actually performing IO on them. This is basically an
eval
call and we should be very careful of introducing such things in our codebase.
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.
Yes, true. If there is code executed on module load it could be have unintentional behavior.
But this optional and is only active when functions_to_trace
is set.
What if we point out this concerns in the documentation, so people know about them when activating this feature?
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.
@mitsuhiko can I get your opinion on this? Should we avoid putting import_module
in the SDK (although it needs to be enabled by default)?
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.
I’m okay with importing from strings. But I can’t say about the feature as such.
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.
Are you saying you can not say anything about the feature, or are you saying that you are not okay with the feature @mitsuhiko ?
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.
(On another note, @mdtro (Matthew T) said in slack: A note in the documentation is sufficient, imo. The list should be static or loaded from a secure source (no user or untrusted input).)
Documentation for this feature is here: getsentry/sentry-docs#6520 |
@antonpirker FYI the docs in getsentry/sentry-docs#6520 do not match your implementation in this PR |
Hey @davidgrohmann, thanks for pointing that out. I'll fix the docs. |
Have a list of functions that can be passed to "sentry_sdk.init()". When the SDK starts it goes through the list and instruments all the functions in the list.
Fixes #1963
Docs: getsentry/sentry-docs#6520
Fixes getsentry/team-webplatform-meta#19