feat(workflow_engine): Add in hook for producing occurrences from the stateful detector#6
Conversation
… stateful detector (#80168) This adds a hook that can be implemented to produce an occurrence specific to the detector that is subclassing the StatefulDetector. Also change the signature of evaluate to return a dict keyed by groupkey instead of a list. This helps avoid the chance of duplicate results for the same group key. <!-- Describe your PR here. -->
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe PR refactors detector evaluation results from list-based to dictionary-based structures keyed by group identifiers. MetricAlertDetectorHandler transitions to inherit from StatefulDetectorHandler, which gains a new abstract method for building occurrences and event data. The Detector model adds a group_type property for cleaner group type registry lookups. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant Detector
participant Handler as StatefulDetectorHandler
participant GroupRegistry as GroupType Registry
Caller->>Detector: process_detectors(data_packet, detectors)
Detector->>Handler: evaluate(data_packet)
Handler->>Handler: iterate groups
alt non-OK status
Handler->>Handler: build_occurrence_and_event_data(group_key, value, status)
Handler->>Handler: create DetectorEvaluationResult
else OK status
Handler->>Handler: create StatusChangeMessage
end
Handler-->>Detector: dict[DetectorGroupKey, DetectorEvaluationResult]
Detector-->>Caller: list[(Detector, dict[DetectorGroupKey, DetectorEvaluationResult])]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sentry/incidents/grouptype.py (1)
10-27:MetricAlertDetectorHandleris now abstract but still wired as a concrete handlerThe verification confirms the review comment is correct. StatefulDetectorHandler inherits from abc.ABC and declares four abstract methods that
MetricAlertDetectorHandlerdoes not implement:counter_names,get_dedupe_value,get_group_key_values, andbuild_occurrence_and_event_data. The instantiation path atsrc/sentry/workflow_engine/models/detector.py:86(return group_type.detector_handler(self)) will attempt to constructMetricAlertDetectorHandler, triggeringTypeError: Can't instantiate abstract classif a Detector is created with this GroupType.Either implement all required abstract methods or revert
MetricAlertDetectorHandlerto inherit from the non-statefulDetectorHandlerbase until the stateful behavior is ready.
🧹 Nitpick comments (5)
src/sentry/workflow_engine/processors/detector.py (2)
166-171: Newbuild_occurrence_and_event_datahook is solid, but enum conversion is a bit brittleThe abstract
build_occurrence_and_event_datahook is a good place to centralizeIssueOccurrence+event_dataconstruction for non‑OK statuses, and the wayevaluate_group_key_valuecalls it ensures every non‑OK transition has concrete payloads.The only concern is the enum conversion:
result, event_data = self.build_occurrence_and_event_data( group_key, value, PriorityLevel(new_status) )This assumes
DetectorPriorityLevelandPriorityLevelshare compatible underlying values. If they ever diverge,PriorityLevel(new_status)will raise or map to the wrong priority. Consider instead:
- Changing the hook signature to accept
DetectorPriorityLeveland letting implementations convert toPriorityLevelif/when they need it, or- Introducing an explicit mapping function from
DetectorPriorityLevel→PriorityLevelso the coupling is visible and testable.Either option would make this more robust against future enum changes.
Also applies to: 285-301
226-245: UpdateStatefulDetectorHandler.evaluatedocstring to mention dict resultsThe method now returns
dict[DetectorGroupKey, DetectorEvaluationResult], but the docstring still says “returns a list ofDetectorEvaluationResult.” Updating the wording to “returns a dict keyed by group_key” will avoid confusion for future implementers.tests/sentry/workflow_engine/processors/test_detector.py (3)
27-29: Annotatecounter_namesas aClassVarto satisfy Ruff and clarify intent
counter_namesis a mutable class attribute used as shared configuration, and Ruff flags this (RUF012). You can make the intent explicit and silence the warning by annotating it as aClassVar:-from typing import Any +from typing import Any, ClassVar @@ -class MockDetectorStateHandler(StatefulDetectorHandler[dict]): - counter_names = ["test1", "test2"] +class MockDetectorStateHandler(StatefulDetectorHandler[dict]): + counter_names: ClassVar[list[str]] = ["test1", "test2"]
58-63: Rename unuseddata_packetparameter inMockDetectorHandler.evaluateRuff’s ARG002 warning here is valid: the
data_packetargument isn’t used in this simple test double. Easiest fix is to rename it to_data_packetto signal intentional non‑use and satisfy the linter:- class MockDetectorHandler(DetectorHandler[dict]): - def evaluate( - self, data_packet: DataPacket[dict] - ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: - return {None: DetectorEvaluationResult(None, True, DetectorPriorityLevel.HIGH)} + class MockDetectorHandler(DetectorHandler[dict]): + def evaluate( + self, _data_packet: DataPacket[dict] + ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: + return {None: DetectorEvaluationResult(None, True, DetectorPriorityLevel.HIGH)}
244-275: Tidy up unusedvalueparameter inbuild_mock_occurrence_and_event(and align test usage)Ruff’s ARG001 warning is correct:
valueisn’t used insidebuild_mock_occurrence_and_event, and intest_state_results_multi_groupyou even passvalue=6for"group_2"while the underlyinggroup_valsis10, which could become confusing oncevaluestarts influencing the payload.Two minimal cleanups that keep behavior identical today:
- Mark the parameter as intentionally unused:
-def build_mock_occurrence_and_event( - handler: StatefulDetectorHandler, - group_key: DetectorGroupKey, - value: int, - new_status: PriorityLevel, +def build_mock_occurrence_and_event( + handler: StatefulDetectorHandler, + group_key: DetectorGroupKey, + _value: int, + new_status: PriorityLevel, ) -> tuple[IssueOccurrence, dict[str, Any]]:
- (Optional) In
test_state_results_multi_group, pass the actual group value for readability:- occurrence_2, event_data_2 = build_mock_occurrence_and_event( - detector.detector_handler, "group_2", 6, PriorityLevel.HIGH - ) + occurrence_2, event_data_2 = build_mock_occurrence_and_event( + detector.detector_handler, "group_2", 10, PriorityLevel.HIGH + )That keeps tests semantically aligned if you later decide to use
valuewhen building occurrences.Also applies to: 175-192
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/sentry/incidents/grouptype.py(1 hunks)src/sentry/workflow_engine/models/detector.py(3 hunks)src/sentry/workflow_engine/processors/detector.py(9 hunks)tests/sentry/workflow_engine/processors/test_detector.py(11 hunks)
🧰 Additional context used
🪛 Ruff (0.14.5)
tests/sentry/workflow_engine/processors/test_detector.py
28-28: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
60-60: Unused method argument: data_packet
(ARG002)
247-247: Unused function argument: value
(ARG001)
🔇 Additional comments (3)
src/sentry/workflow_engine/models/detector.py (1)
58-86:group_typeproperty and updateddetector_handlerlookup look goodCentralizing the grouptype registry lookup behind
Detector.group_typeand reusing it indetector_handlerkeeps the error handling in one place and makes the tests’handler.detector.group_type is not Noneassertions clearer. No functional issues spotted here.tests/sentry/workflow_engine/processors/test_detector.py (1)
123-223: New stateful-detector tests give strong coverage of the dict-based and Kafka flowsThe added tests around
MockDetectorStateHandler(state data fetching, commit behavior, dict-shapedevaluateresults, per-group evaluation, and Kafka emission viaproduce_occurrence_to_kafka) exercise the newStatefulDetectorHandlercontract thoroughly. They validate:
- Dict-based results from both
process_detectorsandevaluate.- Correct interaction with Redis and
DetectorStatepersistence.- Emission behavior for single and multi-group cases, including
any_orderverification.This is a solid safety net for the new hook and stateful logic.
Also applies to: 327-419, 422-619
src/sentry/workflow_engine/processors/detector.py (1)
47-70: Verification complete: no double-emission risk with current detector implementationsThe concern about existing detectors calling
produce_occurrence_to_kafkadirectly is not an issue. The onlyStatefulDetectorHandlerimplementation isMetricAlertDetectorHandler, which is a stub (empty class with no methods). The workflow engine detector pipeline is not yet in production, and when new detectors are added, the architecture correctly centralizes Kafka emission throughprocess_detectors→create_issue_occurrence_from_result. The code change is sound.
2 participants
Test 10
Summary by CodeRabbit
Refactor
Tests
Replicated from ai-code-review-evaluation/sentry-coderabbit#10