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.

Sign up for GitHub

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

Jump to bottom

ref(rules): Only fetch necessary data #74229

Merged
merged 12 commits into from
Jul 16, 2024
Merged

ref(rules): Only fetch necessary data #74229

merged 12 commits into from
Jul 16, 2024

Conversation

ceorourke
Copy link
Member

Optimize queries by only fetching the strictly necessary data, and add a metric around a rough estimate of how many groups we're processing.

@ceorourke ceorourke requested a review from a team as a code owner July 12, 2024 20:40
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 12, 2024
wedamija
wedamija reviewed Jul 12, 2024
View reviewed changes
src/sentry/rules/conditions/event_frequency.py Outdated Show resolved Hide resolved
src/sentry/rules/conditions/event_frequency.py Outdated Show resolved Hide resolved
src/sentry/rules/conditions/event_frequency.py Outdated Show resolved Hide resolved
@ceorourke ceorourke force-pushed the ceorourke/only-fetch-necessary-data-inc-666 branch from 6e58b02 to e8ab3f5 Compare July 12, 2024 22:03
@ceorourke ceorourke requested a review from wedamija July 12, 2024 22:29
@codecov Codecov
Copy link

codecov bot commented Jul 12, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 92.85714% with 3 lines in your changes missing coverage. Please review.

Project coverage is 78.12%. Comparing base (e0977ac) to head (62024e4).
Report is 28 commits behind head on master.

Additional details and impacted files 
@@             Coverage Diff             @@
##           master   #74229       +/-   ##
===========================================
+ Coverage   56.96%   78.12%   +21.16%     
===========================================
  Files        6451     6672      +221     
  Lines      285728   298475    +12747     
  Branches    49139    51354     +2215     
===========================================
+ Hits       162775   233195    +70420     
+ Misses     118543    58996    -59547     
- Partials     4410     6284     +1874
Files Coverage Δ
src/sentry/rules/processing/delayed_processing.py 90.03% <100.00%> (ø)
src/sentry/rules/conditions/event_frequency.py 91.63% <90.90%> (+52.79%) ⬆️

... and 2111 files with indirect coverage changes

saponifi3d
saponifi3d reviewed Jul 12, 2024
View reviewed changes
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.

mind adding a few unit tests to this as well? (i think we need it to get past codecov too)

src/sentry/rules/processing/delayed_processing.py Outdated Show resolved Hide resolved
@ceorourke ceorourke force-pushed the ceorourke/only-fetch-necessary-data-inc-666 branch from 2826e8b to 1c50ce3 Compare July 15, 2024 19:59
saponifi3d
saponifi3d approved these changes Jul 15, 2024
View reviewed changes
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.

LGTM, hopefully helps! 🤞

src/sentry/rules/conditions/event_frequency.py Outdated Show resolved Hide resolved
@ceorourke ceorourke force-pushed the ceorourke/only-fetch-necessary-data-inc-666 branch from 278605c to 0be28cf Compare July 15, 2024 23:18
@ceorourke ceorourke merged commit c80d024 into master Jul 16, 2024
50 checks passed
@ceorourke ceorourke deleted the ceorourke/only-fetch-necessary-data-inc-666 branch July 16, 2024 18:15
@ceorourke ceorourke added the Trigger: Revert Add to a merged PR to revert it (skips CI) label Jul 16, 2024
@getsentry-bot
Copy link
Contributor

PR reverted: 1e1e996

getsentry-bot added a commit that referenced this pull request Jul 16, 2024
@getsentry-bot @ceorourke
Revert "ref(rules): Only fetch necessary data (#74229)"
1e1e996 
This reverts commit c80d024.

Co-authored-by: ceorourke <29959063+ceorourke@users.noreply.github.com>
This was referenced Jul 16, 2024
Closed
Merged
ceorourke added a commit that referenced this pull request Jul 17, 2024
@ceorourke
ref(rules): Optimize Group query (#74400)
b78aefe 
Redo of #74229 that fixes
https://sentry.sentry.io/issues/5612848734
priscilawebdev pushed a commit that referenced this pull request Jul 18, 2024
@ceorourke @priscilawebdev
ref(rules): Optimize Group query (#74400)
9fbcc30 
Redo of #74229 that fixes
https://sentry.sentry.io/issues/5612848734
@github-actions github-actions bot locked and limited conversation to collaborators Aug 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@saponifi3d saponifi3d saponifi3d approved these changes

@wedamija wedamija Awaiting requested review from wedamija

Assignees
No one assigned
Labels
Scope: Backend Automatically applied to PRs that change backend components Trigger: Revert Add to a merged PR to revert it (skips CI)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ceorourke @getsentry-bot @saponifi3d @wedamija