Add hook for producing occurrences from the stateful detector #10
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. -->
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||
|
/agentic_review |
1 similar comment
|
/agentic_review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request refactors detector result collection from lists to dicts keyed by DetectorGroupKey, updating process_detectors and evaluate method signatures across DetectorHandler and StatefulDetectorHandler. MetricAlertDetectorHandler now inherits from StatefulDetectorHandler, and a new abstract hook build_occurrence_and_event_data was introduced for generating occurrence and event data during non-OK status scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller / Pipeline
participant ProcDet as process_detectors
participant DetHandler as DetectorHandler.evaluate
participant StateDetHandler as StatefulDetectorHandler.evaluate
participant Hook as build_occurrence_and_event_data
Caller->>ProcDet: process_detectors(data_packet, detectors)
loop for each detector
ProcDet->>DetHandler: evaluate(data_packet)
alt StatefulDetectorHandler subclass
DetHandler->>StateDetHandler: evaluate(data_packet)
loop for each group_key
StateDetHandler->>StateDetHandler: evaluate_group_key_value()
alt status != OK/RESOLVED
StateDetHandler->>Hook: build_occurrence_and_event_data(group_key, value, status)
Hook-->>StateDetHandler: (occurrence, event_data)
StateDetHandler->>StateDetHandler: create DetectorEvaluationResult(occurrence, event_data)
else status == OK/RESOLVED
StateDetHandler->>StateDetHandler: create DetectorEvaluationResult(StatusChangeMessage)
end
end
StateDetHandler-->>DetHandler: dict[DetectorGroupKey, DetectorEvaluationResult]
end
DetHandler-->>ProcDet: dict[DetectorGroupKey, DetectorEvaluationResult]
ProcDet->>ProcDet: collect as (Detector, dict[...])
end
ProcDet-->>Caller: list[tuple[Detector, dict[DetectorGroupKey, DetectorEvaluationResult]]]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
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: MetricAlertDetectorHandler is now abstract but still registered as detector_handler
MetricAlertDetectorHandlernow subclassesStatefulDetectorHandlerbut doesn’t implement any of its abstract methods (counter_names,get_dedupe_value,get_group_key_values,build_occurrence_and_event_data). WithMetricAlertFire.detector_handler = MetricAlertDetectorHandler, any attempt to resolvedetector.detector_handlerfor this group type will raise aTypeErrorwhen instantiating the handler.If this handler truly isn’t used yet, this won’t surface immediately, but it’s a sharp edge for anyone wiring metric alerts later. Consider one of:
- Implementing minimal concrete overrides that raise
NotImplementedErrorfrom within the methods, so instantiation works and failures occur only if the path is actually exercised; or- Keeping
MetricAlertDetectorHandleras aDetectorHandler[...]stub until you’re ready to implement the stateful version; or- Removing it from
MetricAlertFire.detector_handleruntil an implementation exists.
🧹 Nitpick comments (3)
tests/sentry/workflow_engine/processors/test_detector.py (3)
27-35: counter_names implementation vs abstract property and Ruff warningHere
counter_namesoverrides an abstract@propertywith a mutable class attribute list. That works at runtime, but it both hides the property and triggers Ruff’s RUF012 about mutable class attributes.If you want to appease tooling and keep intent clear, consider either:
- Making it a proper property:
-class MockDetectorStateHandler(StatefulDetectorHandler[dict]): - counter_names = ["test1", "test2"] +class MockDetectorStateHandler(StatefulDetectorHandler[dict]): + @property + def counter_names(self) -> list[str]: + return ["test1", "test2"]or
- Annotating as a
ClassVar[list[str]]if you prefer the constant-attribute style.
58-63: Unused data_packet parameter in MockDetectorHandler.evaluate
MockDetectorHandler.evaluateignores itsdata_packetargument, which is what Ruff’s ARG002 is flagging. Since this is a test stub, it’s harmless, but you can quiet the warning and make intent clearer by renaming it to_data_packet:- def evaluate( - self, data_packet: DataPacket[dict] - ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: + def evaluate( + self, _data_packet: DataPacket[dict] + ) -> dict[DetectorGroupKey, DetectorEvaluationResult]:
244-275: Unusedvalueparameter in build_mock_occurrence_and_eventThe
valueargument isn’t used insidebuild_mock_occurrence_and_event, which is what ARG001 is reporting. Either drop it from the signature or incorporate it into the constructedIssueOccurrence(e.g., inevidence_dataorsubtitle) so the parameter meaning is explicit:-def build_mock_occurrence_and_event( - handler: StatefulDetectorHandler, - group_key: DetectorGroupKey, - value: int, - new_status: PriorityLevel, -) -> tuple[IssueOccurrence, dict[str, Any]]: +def build_mock_occurrence_and_event( + handler: StatefulDetectorHandler, + group_key: DetectorGroupKey, + new_status: PriorityLevel, +) -> tuple[IssueOccurrence, dict[str, Any]]:(and update call sites), or start actually using
valueinside the helper.
📜 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
🧬 Code graph analysis (3)
src/sentry/workflow_engine/models/detector.py (1)
src/sentry/issues/grouptype.py (2)
GroupType(141-223)get_by_slug(104-107)
src/sentry/incidents/grouptype.py (1)
src/sentry/workflow_engine/processors/detector.py (1)
StatefulDetectorHandler(135-407)
src/sentry/workflow_engine/processors/detector.py (3)
src/sentry/types/group.py (1)
PriorityLevel(63-80)src/sentry/workflow_engine/models/detector.py (1)
Detector(23-86)src/sentry/workflow_engine/models/data_source.py (1)
DataPacket(19-21)
🪛 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-65: Detector.group_type helper and handler resolution look goodCentralizing the slug lookup in
group_typeand reusing it indetector_handlerkeeps behavior the same while making the group type reusable (e.g. in tests and occurrence construction). No issues from this change.tests/sentry/workflow_engine/processors/test_detector.py (1)
123-222: New tests thoroughly exercise dict-based results and occurrence emissionThe updated tests around
process_detectors,StatefulDetectorHandler.evaluate, andevaluate_group_key_value(including thefreeze_timeusage and Kafka producer mocks) give good coverage of:
- dict-structured
DetectorEvaluationResults keyed byDetectorGroupKey- both IssueOccurrence and StatusChangeMessage paths
- dedupe behavior and state transitions
This looks like solid protection for the new API and behavior.
Also applies to: 422-580
src/sentry/workflow_engine/processors/detector.py (1)
34-45: Dict-based DetectorEvaluationResult API and process_detectors loop look consistentSwitching
evaluateandprocess_detectorsto usedict[DetectorGroupKey, DetectorEvaluationResult]and then iteratingdetector_results.values()to produce Kafka messages is internally consistent:
- Callers now get a per-detector mapping keyed by
DetectorGroupKey.- Only entries with a non-
Noneresulttriggercreate_issue_occurrence_from_result, so “no-op” evaluations don’t accidentally emit.- State commits remain per-detector via
handler.commit_state_updates().Assuming all existing handlers have been updated to return dicts (tests suggest so), this refactor looks sound.
Also applies to: 47-70, 114-133
| @abc.abstractmethod | ||
| def build_occurrence_and_event_data( | ||
| self, group_key: DetectorGroupKey, value: int, new_status: PriorityLevel | ||
| ) -> tuple[IssueOccurrence, dict[str, Any]]: | ||
| pass | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
New build_occurrence_and_event_data hook and PriorityLevel cast — verify enum alignment and subclasses
The new build_occurrence_and_event_data abstract method plus the updated evaluate/evaluate_group_key_value flow give a clear extension point for detectors to construct IssueOccurrence + event payloads, which is a nice separation of concerns.
Two follow-ups are worth tightening:
- Enum cast correctness
result, event_data = self.build_occurrence_and_event_data(
group_key, value, PriorityLevel(new_status)
)Here new_status is a DetectorPriorityLevel. This relies on the underlying integer values lining up with PriorityLevel so the cast is valid. If they ever diverge, you’ll get ValueError or incorrect priorities on the occurrences.
It might be safer long term to provide an explicit converter (e.g. a helper that maps DetectorPriorityLevel → PriorityLevel) rather than relying on raw IntEnum value equality.
- Subclass completeness
Adding build_occurrence_and_event_data as an abstract method means every concrete StatefulDetectorHandler subclass must now implement it; otherwise those classes remain abstract and can’t be instantiated. Tests cover MockDetectorStateHandler, but it’s worth checking for any production subclasses that might now be missing an implementation.
You can scan for subclasses to verify they all override this hook:
Also applies to: 226-245, 285-308
🏁 Script executed:
#!/bin/bash
# Find classes that subclass StatefulDetectorHandler and show their definitions
ast-grep --pattern $'class $C(StatefulDetectorHandler[$T]) {\n $$$\n}' -n -C3Length of output: 177
I'll correct the script and verify the concerns from the review comment.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find classes that subclass StatefulDetectorHandler (corrected ast-grep syntax)
ast-grep --pattern 'class $C(StatefulDetectorHandler[$T]) { $$$ }'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find all classes that inherit from StatefulDetectorHandler using rg
rg --type-add 'py:*.py' -n 'class\s+\w+.*StatefulDetectorHandler' -A 2Length of output: 490
🏁 Script executed:
#!/bin/bash
# Search for DetectorPriorityLevel and PriorityLevel definitions and usage
rg --type-add 'py:*.py' -n 'DetectorPriorityLevel|PriorityLevel' src/sentry/workflow_engine/ -B 2 -A 2Length of output: 10376
🏁 Script executed:
#!/bin/bash
# Examine the detector.py file around specified lines
cat src/sentry/workflow_engine/processors/detector.py | head -320 | tail -160Length of output: 6948
🏁 Script executed:
#!/bin/bash
# Search for build_occurrence_and_event_data implementations
rg --type-add 'py:*.py' -n 'build_occurrence_and_event_data' -B 2 -A 5Length of output: 2241
I need to check the full implementation of MetricAlertDetectorHandler and verify the PriorityLevel enum values.
🏁 Script executed:
#!/bin/bash
# Get full MetricAlertDetectorHandler class definition
cat src/sentry/incidents/grouptype.pyLength of output: 1073
🏁 Script executed:
#!/bin/bash
# Find PriorityLevel enum definition
rg --type-add 'py:*.py' -n 'class PriorityLevel' -A 10Length of output: 2316
🏁 Script executed:
#!/bin/bash
# Check if MetricAlertDetectorHandler inherits any abstract methods or if it's instantiated anywhere
rg --type-add 'py:*.py' -n 'MetricAlertDetectorHandler' -B 2 -A 5Length of output: 865
Enum cast will fail on DetectorPriorityLevel.OK; MetricAlertDetectorHandler missing required implementation
The PriorityLevel(new_status) cast at line 300 has a real alignment problem. PriorityLevel enum only defines LOW (25), MEDIUM (50), and HIGH (75), but DetectorPriorityLevel.OK has value 0. When new_status == DetectorPriorityLevel.OK, the cast will raise ValueError.
Additionally, MetricAlertDetectorHandler in src/sentry/incidents/grouptype.py:11 is a concrete subclass but only contains pass—it never implements the required abstract method build_occurrence_and_event_data. Although marked with a TODO comment suggesting future work, it's currently referenced as detector_handler in MetricAlertFire and cannot be instantiated.
Recommended fixes:
- Add an explicit converter from
DetectorPriorityLevel→PriorityLevelthat handlesOK(e.g., map it to a safe default or raise a clearer error). - Either implement
build_occurrence_and_event_datainMetricAlertDetectorHandleror make it explicitly abstract (or stub it withNotImplementedError).
🤖 Prompt for AI Agents
In src/sentry/workflow_engine/processors/detector.py around lines 166–171 (and
where PriorityLevel(new_status) is used around line 300), stop directly casting
DetectorPriorityLevel into PriorityLevel; add a small converter function that
maps DetectorPriorityLevel.OK (0) to a safe PriorityLevel (e.g.,
PriorityLevel.LOW) or returns None/raises a clear ValueError with context, and
use that converter instead of direct enum construction to avoid ValueError on
OK. In src/sentry/incidents/grouptype.py (MetricAlertDetectorHandler) implement
the required abstract method build_occurrence_and_event_data (or explicitly make
the class abstract / raise NotImplementedError) so MetricAlertDetectorHandler
cannot be instantiated without a concrete implementation; if you choose a stub,
raise NotImplementedError with a clear message referencing required behavior.
User description
PR #10
PR Type
Enhancement
Description
Change
evaluate()return type from list to dict keyed by group keyAdd abstract hook
build_occurrence_and_event_data()for detector-specific occurrence generationSimplify duplicate detection by using dict structure instead of manual tracking
Update tests to use new dict-based return format and mock occurrence generation
Diagram Walkthrough
File Walkthrough
grouptype.py
Simplify MetricAlertDetectorHandler to use StatefulDetectorHandlersrc/sentry/incidents/grouptype.py
MetricAlertDetectorHandlerto inherit fromStatefulDetectorHandlerinstead ofDetectorHandlerevaluate()method implementationStatefulDetectorHandlerdetector.py
Add group_type property to Detector modelsrc/sentry/workflow_engine/models/detector.py
builtinsimport for type hintsGroupTypeimport fromsentry.issues.grouptypegroup_typeproperty that retrieves the GroupType fromregistry
detector_handlerproperty to use the newgroup_typeproperty
detector.py
Change evaluate return type to dict and add occurrence hooksrc/sentry/workflow_engine/processors/detector.py
evaluate()return type fromlist[DetectorEvaluationResult]todict[DetectorGroupKey, DetectorEvaluationResult]build_occurrence_and_event_data()for subclassesto implement occurrence generation
keys
process_detectors()to iterate over dict values instead oflist
evaluate_group_key_value()to callbuild_occurrence_and_event_data()when creating new issuesPriorityLevelimporttest_detector.py
Update tests for dict-based evaluate return and occurrence generationtests/sentry/workflow_engine/processors/test_detector.py
BaseDetectorHandlerTestbase class with common test setup andhelper methods
MockDetectorStateHandlerto top of file and addedbuild_occurrence_and_event_data()implementationbuild_mock_occurrence_and_event()helper function for creatingtest occurrences
lists
produce_occurrence_to_kafkain relevant teststest_state_results_multi_group_dupe()test as duplicatedetection is now handled by dict structure
@freeze_time()decorator to tests that check timestampsTestProcessDetectors,TestGetStateData,TestCommitStateUpdateData,TestEvaluate, andTestEvaluateGroupKeyValueclasses
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.