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

Centralized way to configure custom instrumentation #1960

Merged
merged 15 commits into from Mar 28, 2023

Conversation

antonpirker
Copy link
Member

@antonpirker antonpirker commented Mar 16, 2023

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.

functions_to_trace = [
    {"qualified_name": "tests.test_basics._hello_world_counter"},
    {"qualified_name": "time.sleep"},
    {"qualified_name": "collections.Counter.most_common"},
]

sentry_sdk.init(
    dsn="...",
    traces_sample_rate=1.0,
    functions_to_trace=functions_to_trace,
)

Fixes #1963
Docs: getsentry/sentry-docs#6520
Fixes getsentry/team-webplatform-meta#19

@antonpirker antonpirker marked this pull request as draft March 16, 2023 12:22
@antonpirker antonpirker marked this pull request as ready for review March 17, 2023 10:20
# ex: "mymodule.submodule.MyClassName.member_function"

module_name, class_name = module_name.rsplit(".", 1)
module_obj = import_module(module_name)
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member Author

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)?

Copy link
Member

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.

Copy link
Member Author

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 ?

Copy link
Member Author

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).)

@antonpirker antonpirker self-assigned this Mar 21, 2023
@antonpirker
Copy link
Member Author

Documentation for this feature is here: getsentry/sentry-docs#6520

@antonpirker antonpirker enabled auto-merge (squash) March 28, 2023 06:20
@antonpirker antonpirker merged commit dc730ed into master Mar 28, 2023
229 checks passed
@antonpirker antonpirker deleted the antonpirker/instrumentation-list branch March 28, 2023 06:33
@davidgrohmann
Copy link

@antonpirker FYI the docs in getsentry/sentry-docs#6520 do not match your implementation in this PR
docs state that functions_to_trace is a list of strings, but you are expecting a list of dictionaries each with a "qualified_name" field

https://docs.sentry.io/platforms/python/performance/instrumentation/custom-instrumentation/?original_referrer=https%3A%2F%2Fwww.google.com%2F#define-span-creation-in-a-central-place

@sentrivana
Copy link
Contributor

Hey @davidgrohmann, thanks for pointing that out. I'll fix the docs.

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.

Central place to configure custom instrumentation QoL SDK API Improvments
6 participants