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

Insights: enable Django cache by default #3300

Open
bcoe opened this issue Jul 17, 2024 · 4 comments
Open

Insights: enable Django cache by default #3300

bcoe opened this issue Jul 17, 2024 · 4 comments
Assignees

Comments

@bcoe
Copy link
Member

bcoe commented Jul 17, 2024

Problem Statement

Cache spans are turned off by default for Django cache due to performance issues described in #2122

Recent load testing showed that cache spans did not introduce significant overhead, granted in a clean room environment, #3058

Proposal

We should turn on cache spans by default, as it will allow more sentry users to get value out of cache insights.

However, before doing so, we should find a suitably high traffic deployment to validate the findings in #3058.

Solution Brainstorm

Are there any other ways we could detect that emitting cache spans is creating too much load on a system and disable cache spans, rather than defaulting cache_spans=False, i.e., disable cache spans if a certain volume of traffic is hit, rather than by default:

high_volume_cache_spans=False
@BYK BYK self-assigned this Nov 4, 2024
BYK added a commit that referenced this issue Dec 5, 2024
I was testing Spotlight with Sentry and realized things started to get slow and crashy. It looks like sometimes `args` is just an empty array on cache's `_instruments_call` causing lots of exceptions being thrown. This patch fixes that with explicit length checks and also adds a note for the missing instrumentation for `get_or_set` method. This might be related to #2122 and #3300.
BYK added a commit that referenced this issue Dec 5, 2024
I was testing Spotlight with Sentry and realized things started to get slow and crashy. It looks like sometimes `args` is just an empty array on cache's `_instruments_call` causing lots of exceptions being thrown. This patch fixes that with explicit length checks and also adds a note for the missing instrumentation for `get_or_set` method. This might be related to #2122 and #3300.
@antonpirker
Copy link
Member

We can dogfood cache spans on our sentry installation and see how much overview this creates. If we are comfortable turning cache spans on, than I guess setting it to true by default is OK.

@BYK
Copy link
Member

BYK commented Dec 11, 2024

@antonpirker you wanna enable it over at getsentry/sentry then? I'll approve it :)

@BYK
Copy link
Member

BYK commented Dec 30, 2024

getsentry/sentry#82645 is up. If all goes well, we can enable this for everyone by default.

@antonpirker
Copy link
Member

@BYK I have done some more local load testing (see #3907) to see how much overhead is added using the newest Python SDK.

BYK added a commit to getsentry/sentry that referenced this issue Jan 8, 2025
This is a follow up to
[getsentry/sentry-python#3300](getsentry/sentry-python#3300)
where we think we have addressed the performance issues regarding
`cache_spans` in `DjangoIntegration`. This is a test and if we can
confirm the lack of perf regression, we will enable this option by
default in Sentry SDK as the next step.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants