Skip to content

Conversation

@ceorourke
Copy link
Member

@ceorourke ceorourke commented Apr 22, 2024

The apply_delayed rule processor needs to call get_rate with bulk Snuba queries (see this code block), so this PR refactors get_rate to handle that.

Copy link
Contributor

@saponifi3d saponifi3d left a comment

Choose a reason for hiding this comment

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

generally lgtm, curious about the target branch change and your thoughts about get_start_end_from_duration though.

@ceorourke
Copy link
Member Author

@saponifi3d idk why I can't reply to your comment about the branch, I'd prefer to find out now rather than later when we have a lot more going on if I somehow fucked this up

@codecov
Copy link

codecov bot commented Apr 23, 2024

Codecov Report

Attention: Patch coverage is 72.41379% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 79.64%. Comparing base (6c0b883) to head (b3304fb).
Report is 1 commits behind head on master.

❗ Current head b3304fb differs from pull request most recent head 56120ee. Consider uploading reports for the commit 56120ee to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #69446       +/-   ##
===========================================
- Coverage   86.21%   79.64%    -6.58%     
===========================================
  Files        2848     6469     +3621     
  Lines      176602   287223   +110621     
  Branches    31517    49495    +17978     
===========================================
+ Hits       152261   228761    +76500     
- Misses      24338    58040    +33702     
- Partials        3      422      +419     
Files Coverage Δ
src/sentry/rules/conditions/event_frequency.py 90.40% <72.41%> (+8.12%) ⬆️

... and 4282 files with indirect coverage changes

Copy link
Contributor

@nhsiehgit nhsiehgit left a comment

Choose a reason for hiding this comment

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

So we've now added comparison type and interval to the data fixtures to determine our rates?

a few nit comments

@ceorourke
Copy link
Member Author

So we've now added comparison type and interval to the data fixtures to determine our rates?

a few nit comments

no, nothing is added. I just moved stuff around

@ceorourke ceorourke merged commit 781586e into master Apr 23, 2024
@ceorourke ceorourke deleted the ceorourke/refactor-get-rate branch April 23, 2024 19:06
@sentry
Copy link

sentry bot commented Apr 23, 2024

Suspect Issues

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

  • ‼️ KeyError: '' 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
  • ‼️ 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
  • ‼️ 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
  • ‼️ 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

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

@sentry
Copy link

sentry bot commented Apr 23, 2024

Suspect Issues

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

  • ‼️ SnubaError: Failed to parse snuba error response sentry.tasks.post_process.post_process_group View Issue

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

armenzg pushed a commit that referenced this pull request Apr 24, 2024
The `apply_delayed` rule processor needs to call `get_rate` with bulk
Snuba queries (see [this code
block](https://github.com/getsentry/sentry/blob/088b9c73b604743864f76bd39a81a3238699d306/src/sentry/rules/processing/delayed_processing.py#L136-L171)),
so this PR refactors `get_rate` to handle that.
MichaelSun48 pushed a commit that referenced this pull request Apr 25, 2024
The `apply_delayed` rule processor needs to call `get_rate` with bulk
Snuba queries (see [this code
block](https://github.com/getsentry/sentry/blob/088b9c73b604743864f76bd39a81a3238699d306/src/sentry/rules/processing/delayed_processing.py#L136-L171)),
so this PR refactors `get_rate` to handle that.
@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.

4 participants