Skip to content

Conversation

@everettbu
Copy link
Contributor

Test [new PR number opened]

… 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. -->
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This PR implements a significant architectural enhancement to Sentry's workflow engine by adding the ability for stateful detectors to produce issue occurrences. The changes span across four main files:

Core Model Enhancement (detector.py): A new group_type property is added to the Detector model, providing typed access to the GroupType instance associated with each detector. This refactors the existing detector_handler property to eliminate code duplication and improve type safety by centralizing the group type lookup logic.

Handler Migration (grouptype.py): The MetricAlertDetectorHandler is refactored to inherit from StatefulDetectorHandler instead of DetectorHandler, removing its concrete evaluate() method implementation. This aligns with the broader migration towards stateful detection capabilities.

Processor Core Logic (detector.py): The most substantial changes occur in the processor, where the detector evaluation pipeline is enhanced to support occurrence production. Key modifications include:

  • Changing return types from lists to dictionaries keyed by DetectorGroupKey to eliminate duplicate handling
  • Adding a new abstract method build_occurrence_and_event_data that subclasses must implement
  • Integrating Kafka occurrence production when detectors transition to non-OK status
  • Removing redundant duplicate detection logic in favor of dictionary-based deduplication

Test Infrastructure (test_detector.py): Comprehensive test updates validate the new occurrence production functionality, including mock implementations of the abstract method and verification of Kafka integration.

These changes enable the workflow engine to create and emit proper IssueOccurrence objects to Kafka when detectors identify problems, completing the integration between the detection system and Sentry's issue tracking infrastructure. The architecture now supports both issue creation (via occurrences) and resolution (via status change messages).

Confidence score: 2/5

  • This PR introduces significant architectural changes but has several implementation gaps that make it risky to merge
  • The score reflects incomplete abstract method implementations and potential runtime failures in the MetricAlertDetectorHandler
  • Files needing attention: src/sentry/incidents/grouptype.py (incomplete implementation), src/sentry/workflow_engine/processors/detector.py (verify abstract method contracts)

4 files reviewed, 3 comments

Edit Code Review Bot Settings | Greptile

Comment on lines +11 to +12
class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]):
pass
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: StatefulDetectorHandler is an abstract base class that requires implementing several abstract methods: counter_names, get_dedupe_value, get_group_key_values, and build_occurrence_and_event_data. This class will not be instantiable without these implementations.

Comment on lines +190 to +191
occurrence_2, event_data_2 = build_mock_occurrence_and_event(
detector.detector_handler, "group_2", 6, PriorityLevel.HIGH
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: The hardcoded value 6 for group_2 seems inconsistent - the test data shows group_2 has value 10, but the expected occurrence is built with value 6

Suggested change
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

Comment on lines +536 to +537
occurrence, event_data = build_mock_occurrence_and_event(
handler, "val1", 6, PriorityLevel.HIGH
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: The test builds an expected result using build_mock_occurrence_and_event with 'val1' and value 6, but this doesn't match the test's group_key parameter 'group_key' - potential test logic error

@GitHoobar
Copy link

Review Summary

🏷️ Draft Comments (1)

Skipped posting 1 draft comments that were valid but scored below your review threshold (>=13/15). Feel free to update them here.

src/sentry/workflow_engine/models/detector.py (1)

59-61: group_type property calls grouptype.registry.get_by_slug(self.type) on every access, causing repeated registry lookups for the same detector and wasting CPU if accessed frequently.

📊 Impact Scores:

  • Production Impact: 2/5
  • Fix Specificity: 4/5
  • Urgency Impact: 2/5
  • Total Score: 8/15

🤖 AI Agent Prompt (Copy & Paste Ready):

In src/sentry/workflow_engine/models/detector.py, lines 59-61, the `group_type` property performs a registry lookup on every access, which is inefficient if accessed frequently. Decorate the property with `@functools.lru_cache(maxsize=1)` to cache the result per instance and avoid repeated lookups. Ensure you import `functools` at the top of the file if not already present.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants