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

Possible performance regression related to django signals instrumentation #1826

Closed
ron8mcr opened this issue Jan 8, 2023 · 3 comments · Fixed by #2758
Closed

Possible performance regression related to django signals instrumentation #1826

ron8mcr opened this issue Jan 8, 2023 · 3 comments · Fixed by #2758

Comments

@ron8mcr
Copy link

ron8mcr commented Jan 8, 2023

How do you use Sentry?

Self-hosted/on-premise

Version

1.12.1

Steps to Reproduce

Updated sentry-sdk from 1.9.8 to 1.12.1.

Expected Result

Overall project performance not changed

Actual Result

Performance decreased for about 25%.

Looks like instrumentation of some django signals (#1526) brought some overhead.
In my specific example, I've used https://github.com/matthewwithanm/django-imagekit for thumbnails generation. And this lib uses post_init signal to track changes of ImageField values.
For cases when there is about 1000 instances in response of DRF API method, post_init handler is called for a lot of times (even for models that actually does not have any ImageField). imagekit itself implements a fast check if handler should be called. However, looks like instrumentation of this slow down overall time of response.

See screenshots with example request. The first one is with commented out patch_signals call:

image

The second is with enabled signals instrumentation:
image

I think that sentry option that allow to disable instrumentation of some specific signals/receivers will allow to solve this problem. Or maybe option that will allow to disable specific instrumentation.

P.S. I used following monkey-patch to disable signals instrumentation and restore previous performance:

from sentry_sdk.integrations import django
django.patch_signals = lambda: None

sentry_sdk.init(
	"integrations": [django.DjangoIntegration()],
	"dsn": "dsn",
)
@kanu
Copy link

kanu commented Jan 20, 2023

I would also love to have this configurable. On signal or sender level.
What I additionally noticed was that I am not able to easily see the connected receivers of a signal by calling
e.g. post_save._live_receivers(AnyModel) as they are wrapped now. But this should be solvable by using functools.wraps to update the wrapper function
: signature was fixed in #1700

@antonpirker
Copy link
Member

Hey @kanu nice to see you here!

We have an issue for adding a configuration to disable signal instrumentation: #1739

@ron8mcr
Copy link
Author

ron8mcr commented Mar 23, 2023

@antonpirker I've checked performance with changes from #1929 (sentry-sdk==1.17.0) and it solved the problem. Thanks for fixing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants