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

perf(issues): add pseudo-cache to test_frequency_rates in GroupSnooze #69939

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

vartec
Copy link
Member

@vartec vartec commented Apr 30, 2024

Reducing unnecessary Snuba queries by only querying when Redis count reached.
Follows exactly same pattern as #69556

@vartec vartec requested a review from a team April 30, 2024 00:51
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 30, 2024
referrer_suffix="frequency_snoozes",
)[self.group_id]

# TTL is further into the future than it needs to be, but we'd rather over-estimate
Copy link
Member

Choose a reason for hiding this comment

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

TTL is further into the future than it needs to be

if we want the cacheing to be less aggressive shouldn't the ttl be less into the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

It short-circuits to say that the threshold is not reached when the number is less than threshold, by extending the caching window further into the future we guarantee to be overcounting, rather then undercounting, while keeping the cache as long as possible.

OTOH, if we'd like to have short lived cache, that would work too. On expired cache it falls back to +inf sentinel value, and triggers Snuba query

Copy link
Member

Choose a reason for hiding this comment

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

ah i see now 👍🏼

@vartec vartec merged commit 3a67f2b into master Apr 30, 2024
49 checks passed
@vartec vartec deleted the vartec/groupsnooze-frequency-cache branch April 30, 2024 01:24
Copy link

sentry-io bot commented Apr 30, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ RateLimitExceeded: Query on could not be run due to allocation policies, details: {'ConcurrentRateLimitAllocationPol... sentry.tasks.post_process.post_process_group View Issue
  • ‼️ SnubaError: HTTPConnectionPool(host='127.0.0.1', port=10006): Max retries exceeded with url: /events/snql (Ca... sentry.tasks.post_process.post_process_group View Issue
  • ‼️ RateLimitExceeded: Query on could not be run due to allocation policies, details: {'ConcurrentRateLimitAllocationPol... sentry.tasks.post_process.post_process_group View Issue
  • ‼️ SnubaError: HTTPConnectionPool(host='127.0.0.1', port=10006): Read timed out. (read timeout=30) sentry.tasks.post_process.post_process_group View Issue

Did you find this useful? React with a 👍 or 👎

vartec added a commit that referenced this pull request May 1, 2024
Cleaning up #69556 and
#69939

Also changing cached `value` to more meaningful names.
@github-actions github-actions bot locked and limited conversation to collaborators May 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants