-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
chore(aci milestone 3): remove processing feature flags #102456
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just the nit / ask for a jira ticket around the the AlertRuleDetector table.
| # temporarily fetch the alert rule ID from the detector ID | ||
| alert_rule_detector = AlertRuleDetector.objects.filter( | ||
| detector_id=open_period_identifier, alert_rule_id__isnull=False | ||
| ).first() | ||
| if alert_rule_detector is not None: | ||
| # open_period_identifier is a metric detector ID -> get the alert rule ID | ||
| open_period_identifier = alert_rule_detector.alert_rule_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way we could update this to no longer rely on the AlertRuleDetector table (or the alert_rule_id?) as we're removing these processing flags, my hope is that as we remove the feature flags we're left with the desired end state implementation.
if we have to have the alert_rule_id for like charting things, then it'd help to have that listed out somewhere as they'll be important blockers to fix before we can cleanup too much code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is coming in my next PR 😅 doing a refactor so the charts use the open period serializer instead.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #102456 +/- ##
===========================================
+ Coverage 80.71% 80.80% +0.08%
===========================================
Files 8826 8836 +10
Lines 390071 390791 +720
Branches 24799 24799
===========================================
+ Hits 314863 315784 +921
+ Misses 74857 74656 -201
Partials 351 351 |
|
Ugh, I think this is going to be tied up with removing the legacy actions code 🫠 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me.
Trying again now that subscription processor code has removed references to AlertRule. Flags removed: - `organizations:workflow-engine-process-metric-issue-workflows` - `organizations:workflow-engine-metric-alert-processing` - `organizations:workflow-engine-single-process-metric-issues`
Trying again now that subscription processor code has removed references to AlertRule. Flags removed: - `organizations:workflow-engine-process-metric-issue-workflows` - `organizations:workflow-engine-metric-alert-processing` - `organizations:workflow-engine-single-process-metric-issues`
Trying again now that subscription processor code has removed references to AlertRule. Flags removed:
organizations:workflow-engine-process-metric-issue-workflowsorganizations:workflow-engine-metric-alert-processingorganizations:workflow-engine-single-process-metric-issues