Skip to content

Conversation

@kcons
Copy link
Member

@kcons kcons commented Nov 15, 2025

We want DetectorGroups for all Groups that can associate with a Detector.
We already have path to add it for all new groups, but backfilling existing groups is a little tricky,
and by doing it incrementally be recurrence here, we can make DetectorGroup reliable for Detector lookup
for essentially any Group the workflow engine is asked to process.

We may still need a backfill for completeness if we can't wait until the Group retention window for completeness.

@kcons kcons requested review from a team as code owners November 15, 2025 01:29
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 15, 2025
@kcons kcons changed the title feat(aci): Ensure DetectorGroup for new error group events feat(aci): Ensure DetectorGroup for new events on existing error Groups Nov 15, 2025
@codecov
Copy link

codecov bot commented Nov 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #103419      +/-   ##
===========================================
- Coverage   80.67%    80.67%   -0.01%     
===========================================
  Files        9243      9243              
  Lines      394826    394936     +110     
  Branches    25152     25152              
===========================================
+ Hits       318538    318615      +77     
- Misses      75841     75874      +33     
  Partials      447       447              

@mifu67 mifu67 self-requested a review November 17, 2025 19:09
Copy link
Contributor

@mifu67 mifu67 left a comment

Choose a reason for hiding this comment

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

You're only touching error issues here—ensure_association_with_detector is never called with a detector ID. Did you want to handle other grouptypes in a separate PR?
In any case, I think that save_issue_occurrence is a good place to handle healing the relationship for metric issues. We can get the detector ID using occurrence_data.evidence_data. Let me know too if you want me to handle that.

@kcons
Copy link
Member Author

kcons commented Nov 17, 2025

You're only touching error issues here—ensure_association_with_detector is never called with a detector ID. Did you want to handle other grouptypes in a separate PR? In any case, I think that save_issue_occurrence is a good place to handle healing the relationship for metric issues. We can get the detector ID using occurrence_data.evidence_data. Let me know too if you want me to handle that.

I was initially just targeting errors here, since for metric issues there was already a migration PR out, but I intentionally left the helper method more general to allow us to extend it to other issue types. So, happy to do that here.

@kcons kcons marked this pull request as draft November 17, 2025 22:02
@kcons kcons marked this pull request as ready for review November 17, 2025 23:54
@kcons kcons requested a review from mifu67 November 18, 2025 20:01
@mifu67 mifu67 requested a review from a team November 18, 2025 22:08
Copy link
Contributor

@mifu67 mifu67 left a comment

Choose a reason for hiding this comment

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

Looks good to me from the metric issue perspective. Feel free to merge, unless you want a review from the errors perspective too.

@kcons kcons changed the title feat(aci): Ensure DetectorGroup for new events on existing error Groups feat(aci): Ensure DetectorGroup for recurring Groups Nov 18, 2025
@kcons kcons merged commit b95d638 into master Nov 18, 2025
68 checks passed
@kcons kcons deleted the kcons/remit branch November 18, 2025 22:58
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.

3 participants