Skip to content

ref(alerts): fetch group_to_groupevent once per delayed processing task #91831

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

Merged
merged 5 commits into from
May 19, 2025

Conversation

cathteng
Copy link
Member

Because we fetch group_to_groupevent for every rule that we need to fire groups for, this quickly takes up space in the span tree when we are trying to debug why fire_rules is taking so long. Fetching it once should also improve performance a little.

Rules can also fire for the same groups so this reduces some redundancy.

@cathteng cathteng requested a review from a team as a code owner May 16, 2025 21:05
@cathteng cathteng requested a review from kcons May 16, 2025 21:05
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label May 16, 2025
@cathteng cathteng force-pushed the cathy/alerts/fetch-group-to-groupevent-once branch from 267e04b to 583eaa4 Compare May 16, 2025 21:37
group = group_id_to_group.get(group_id)
if not group:
logger.info(
"delayed_processing.group_not_found",
Copy link
Member

Choose a reason for hiding this comment

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

We should make it easier to understand what this situation means.
group_id_to_group is derived from rules_to_fire, but I think the subset of groups from rulegroup_to_event_data that were in rules_to_fire and we were able to find in the DB.
group_ids is just the raw group ids from rules_to_fire. rules_to_fire groups are themselves a subset of the groups present in rulegroup_to_event_data (i think).
So, my read is that not finding the group suggests something went wrong in fetching, which seems unexpected and probably worth escalating beyond logs.

If that's wrong, I think a comment clarifying what someone who sees this log message should be thinking is worthwhile.

Copy link
Member Author

Choose a reason for hiding this comment

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

i can logger.error here, it will throw a sentry error

Copy link

codecov bot commented May 16, 2025

Codecov Report

Attention: Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.

⚠️ Parser warning

The parser emitted a warning. Please review your JUnit XML file:

Warning while parsing testcase attributes: Limit of string is 1000 chars, for name, we got 2083 at 1:156898 in /home/runner/work/sentry/sentry/.artifacts/pytest.junit.xml
Files with missing lines Patch % Lines
src/sentry/rules/processing/delayed_processing.py 63.63% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #91831      +/-   ##
==========================================
- Coverage   82.60%   82.60%   -0.01%     
==========================================
  Files       10350    10350              
  Lines      586633   586639       +6     
  Branches    22574    22574              
==========================================
- Hits       484580   484579       -1     
- Misses     100821   100828       +7     
  Partials     1232     1232              

@cathteng cathteng requested a review from kcons May 16, 2025 22:35
@cathteng cathteng merged commit f431944 into master May 19, 2025
59 checks passed
@cathteng cathteng deleted the cathy/alerts/fetch-group-to-groupevent-once branch May 19, 2025 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants