Repository navigation
feat(workflow_engine): Add in hook for producing occurrences from the stateful detector - #1825
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. -->
… stateful detector
![unblocked-local-kaihao[bot]](https://avatars.githubusercontent.com/u/304571795?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found.
About Unblocked
Unblocked has been set up to automatically review your team's pull requests to identify genuine bugs and issues.
📖 Documentation — Learn more in our docs.
💬 Ask questions — Mention @unblocked-local-kaihao to request a review or summary, or ask follow-up questions.
👍 Give feedback — React to comments with 👍 or 👎 to help us improve.
⚙️ Customize — Adjust settings in your preferences.
| class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]): | ||
| pass |
There was a problem hiding this comment.
This class now inherits four unimplemented abstract members: counter_names, get_dedupe_value, get_group_key_values, and build_occurrence_and_event_data. MetricAlertFire still registers it as its handler, so accessing detector.detector_handler for metric_alert_fire raises TypeError, aborting process_detectors before its missing-handler guard. Implement those members, or retain a concrete no-op handler returning {} until the implementation is ready.
| result, event_data = self.build_occurrence_and_event_data( | ||
| group_key, value, PriorityLevel(new_status) |
There was a problem hiding this comment.
The new occurrence path can permanently lose an alert when publication fails. produce_occurrence_to_kafka catches KafkaException, logs it, and returns without signaling failure; process_detectors then commits the new priority and dedupe value anyway. Replaying the packet is skipped, and subsequent packets at the same priority produce no occurrence because the detector already records that status. Propagate publication failure and avoid committing the transition on failure, or persist the occurrence through a durable outbox.
| ) -> tuple[IssueOccurrence, dict[str, Any]]: | ||
| assert handler.detector.group_type is not None | ||
| occurrence = IssueOccurrence( | ||
| id="eb4b0acffadb4d098d48cb14165ab578", |
There was a problem hiding this comment.
IssueOccurrence.__eq__ compares only id, and this helper gives every actual and expected occurrence the same ID. Consequently, the new result assertions and Kafka mock-call assertions accept occurrences with incorrect fingerprints, project IDs, or initial priorities. Compare serialized occurrence fields or install a field-wise test comparator so these assertions validate the payload produced by the new hook.
| handler = self.build_handler() | ||
| with mock.patch("sentry.workflow_engine.processors.detector.metrics") as mock_metrics: | ||
| occurrence, event_data = build_mock_occurrence_and_event( | ||
| handler, "val1", 6, PriorityLevel.HIGH |
There was a problem hiding this comment.
The expected occurrence is built for "val1", but this test evaluates "group_key". Since the helper incorporates its group key into the fingerprint, the expected occurrence describes a different issue from the one being evaluated. ID-only occurrence equality currently masks this contradiction.
| handler, "val1", 6, PriorityLevel.HIGH | |
| handler, "group_key", 10, PriorityLevel.HIGH |
| self.sm_comp_patcher = mock.patch.object( | ||
| StatusChangeMessage, "__eq__", status_change_comparator | ||
| ) | ||
| self.sm_comp_patcher.__enter__() |
There was a problem hiding this comment.
The global StatusChangeMessage.__eq__ patch is restored only in tearDown. If setup raises after this point—for example, during group-type feature registration—unittest skips tearDown, leaving the comparator installed for subsequent tests. Although no setup failure was reproduced here, this cleanup path is not exception-safe. Register the patch's cleanup with addCleanup, or use enterContext, so restoration also runs after setup errors.
| ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: | ||
| """ | ||
| Evaluates a given data packet and returns a list of `DetectorEvaluationResult`. |
There was a problem hiding this comment.
The rewritten annotation and implementation return a dictionary keyed by detector group key, but the docstring still promises a list of DetectorEvaluationResult. Update the documented return shape to match the new contract.
2 participants
See title.