feat(workflow_engine): Add state tracking for priority levels for stateful detectors #91791
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is
|
| Files with missing lines | Patch % | Lines |
|---|---|---|
| ...ntry/workflow_engine/handlers/detector/stateful.py | 83.33% | 11 Missing |
Additional details and impacted files
@@ Coverage Diff @@
## master #91791 +/- ##
========================================
Coverage 87.63% 87.63%
========================================
Files 10356 10357 +1
Lines 587192 587355 +163
Branches 22585 22585
========================================
+ Hits 514558 514712 +154
- Misses 72206 72215 +9
Partials 428 428 
| storing counter values. | ||
| """ | ||
| pass | ||
| # Merge all kinds of counters together for the state manager |
There was a problem hiding this comment.
Nit: These are already merged in self._thresholds
There was a problem hiding this comment.
The way i was thinking about this is that there might be additional counters that the implementer wants to track in their detector handler. this is meant to give the space to allow for that override fairly easily -- but yeah, i feel like this is probs an over optimization, i can simplify to just having threshold counters on the stateful detector for now.
| **self._thresholds, | ||
| } | ||
|
|
||
| self.state_manager = DetectorStateManager(detector, list(counters.keys())) |
There was a problem hiding this comment.
Should we just use list(self._thresholds.keys()) here?
| updated_status_count = (state.counter_updates.get(new_priority) or 0) + 1 | ||
| self.state_manager.enqueue_counter_update(None, {new_priority: updated_status_count}) | ||
|
|
||
| if self._thresholds[new_priority] > updated_status_count: | ||
| # We haven't met the threshold yet, so don't trigger | ||
| return None |
There was a problem hiding this comment.
One thing to keep in mind - probably, higher priorities should also increment counts for all lower priorities.
For metric alerts, if we had a threshold of 3, and then we got crit, crit, warn, the warning should fire.
There was a problem hiding this comment.
🤔 makes sense. will update.
saponifi3d
force-pushed
the
jcallender/aci/ref-stateful-priorities
branch
from
16408ae to
9b9e176
Compare
| def _get_configured_detector_levels(self) -> list[DetectorPriorityLevel]: | ||
| # TODO - Is this something that should be provided by the detector itself rather | ||
| # than having to query the db for each level? | ||
| priority_levels: list[DetectorPriorityLevel] = [level for level in DetectorPriorityLevel] | ||
|
|
||
| if self.detector.workflow_condition_group is None: | ||
| # TODO - Should this default to _all_ levels or no levels? | ||
| return priority_levels | ||
|
|
||
| condition_result_levels = self.detector.workflow_condition_group.conditions.filter( | ||
| condition_result__in=priority_levels | ||
| ).values_list("condition_result", flat=True) | ||
|
|
||
| return list(DetectorPriorityLevel(level) for level in condition_result_levels) |
There was a problem hiding this comment.
@wedamija this is how i was thinking we could filter to just the levels configure -- any ways we can easily cache this data on the model or should i make a field in the model that looks up all these values and caches it there? 🤔
saponifi3d
force-pushed
the
jcallender/aci/ref-stateful-priorities
branch
from
58fe124 to
3a9cf30
Compare
…an stay generic and stateful handlers are explicit
…e conditions were met. make that clearer too
…now. can make more generic when cases arise
…rcuit the if statement putting the faster check first
…han the triggered priority as well. handle OK separately
…d to think about if this should be filtered to only configured thresholds or not.
saponifi3d
force-pushed
the
jcallender/aci/ref-stateful-priorities
branch
from
3a9cf30 to
2611032
Compare
Description
Implement state tracking for threshold changes on detectors.
This is pt 4 of the great stateful detector refactoring. Taking a moment to also document these changes. For now, adding these as inline markdown. (Not sure if we have a way to do docgen from comment headers or not)
Up next, - the final refactoring - make the
Statefulhandler easier to reckon with grouping.