-
Notifications
You must be signed in to change notification settings - Fork 0
feat(workflow_engine): Add in hook for producing occurrences from the stateful detector #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: workflow-engine-stateful-detector-before
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,17 +4,12 @@ | |
| from sentry.issues.grouptype import GroupCategory, GroupType | ||
| from sentry.ratelimits.sliding_windows import Quota | ||
| from sentry.types.group import PriorityLevel | ||
| from sentry.workflow_engine.models import DataPacket | ||
| from sentry.workflow_engine.processors.detector import DetectorEvaluationResult, DetectorHandler | ||
| from sentry.workflow_engine.processors.detector import StatefulDetectorHandler | ||
|
|
||
|
|
||
| # TODO: This will be a stateful detector when we build that abstraction | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 CRITICAL — Business Logic Correctness (confidence: 100%) MetricAlertDetectorHandler inherits from StatefulDetectorHandler but does not implement the abstract methods Evidence:
Agent: logic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 INFO — Code organization (confidence: 100%)
Evidence:
Agent: style There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡
--- a/src/sentry/incidents/grouptype.py
+++ b/src/sentry/incidents/grouptype.py
@@ -4,10 +4,13 @@
from sentry.issues.grouptype import GroupCategory, GroupType
from sentry.ratelimits.sliding_windows import Quota
from sentry.types.group import PriorityLevel
-from sentry.workflow_engine.processors.detector import StatefulDetectorHandler
+from sentry.workflow_engine.models import DataPacket
+from sentry.workflow_engine.processors.detector import DetectorEvaluationResult, DetectorHandler
# TODO: This will be a stateful detector when we build that abstraction
-class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]):
- pass
+class MetricAlertDetectorHandler(DetectorHandler[QuerySubscriptionUpdate]):
+ def evaluate(
+ self, data_packet: DataPacket[QuerySubscriptionUpdate]
+ ) -> list[DetectorEvaluationResult]:
+ # TODO: Implement when StatefulDetectorHandler abstract methods are ready
+ return []🤖 Grapple PR auto-fix • critical • Review this diff before applying There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 CRITICAL — Business logic correctness (confidence: 100%) MetricAlertDetectorHandler now extends StatefulDetectorHandler which has an abstract method Evidence:
Agent: logic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡
--- a/src/sentry/incidents/grouptype.py
+++ b/src/sentry/incidents/grouptype.py
@@ -4,10 +4,11 @@
from sentry.issues.grouptype import GroupCategory, GroupType
from sentry.ratelimits.sliding_windows import Quota
from sentry.types.group import PriorityLevel
-from sentry.workflow_engine.processors.detector import StatefulDetectorHandler
+from sentry.workflow_engine.models import DataPacket
+from sentry.workflow_engine.processors.detector import DetectorEvaluationResult, DetectorHandler
# TODO: This will be a stateful detector when we build that abstraction
-class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]):
- pass
+class MetricAlertDetectorHandler(DetectorHandler[QuerySubscriptionUpdate]):
+ def evaluate(
+ self, data_packet: DataPacket[QuerySubscriptionUpdate]
+ ) -> list[DetectorEvaluationResult]:
+ # TODO: Implement - will migrate to StatefulDetectorHandler once
+ # build_occurrence_and_event_data, get_dedupe_value, and get_group_key_values
+ # are implemented for metric alert detectors
+ return []🤖 Grapple PR auto-fix • critical • Review this diff before applying |
||
| class MetricAlertDetectorHandler(DetectorHandler[QuerySubscriptionUpdate]): | ||
| def evaluate( | ||
| self, data_packet: DataPacket[QuerySubscriptionUpdate] | ||
| ) -> list[DetectorEvaluationResult]: | ||
| # TODO: Implement | ||
| return [] | ||
| class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 CRITICAL — API contract violation (confidence: 100%) MetricAlertDetectorHandler now inherits from StatefulDetectorHandler but does not implement the required abstract method Evidence:
Agent: architecture There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡
--- a/src/sentry/incidents/grouptype.py
+++ b/src/sentry/incidents/grouptype.py
@@ -1,12 +1,14 @@
from dataclasses import dataclass
+from typing import Any
from sentry.incidents.utils.types import QuerySubscriptionUpdate
from sentry.issues.grouptype import GroupCategory, GroupType
from sentry.ratelimits.sliding_windows import Quota
from sentry.types.group import PriorityLevel
-from sentry.workflow_engine.processors.detector import StatefulDetectorHandler
+from sentry.workflow_engine.models import DataPacket
+from sentry.workflow_engine.processors.detector import DetectorGroupKey, StatefulDetectorHandler
# TODO: This will be a stateful detector when we build that abstraction
class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]):
- pass
+ def build_occurrence_and_event_data(
+ self, group_key: DetectorGroupKey, new_status: Any, is_new_group: bool
+ ) -> Any:
+ # TODO: Implement — produce IssueOccurrence + event data for metric alert fires
+ return None
+
+ def get_dedupe_value(self, data_packet: DataPacket[QuerySubscriptionUpdate]) -> int:
+ # TODO: Implement — return a deduplication value derived from the subscription update
+ return 0
+
+ def get_group_key_values(
+ self, data_packet: DataPacket[QuerySubscriptionUpdate]
+ ) -> dict[DetectorGroupKey, int]:
+ # TODO: Implement — return keyed numeric values driving detector state transitions
+ return {}🤖 Grapple PR auto-fix • critical • Review this diff before applying |
||
| pass | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 MAJOR — Authentication/Authorization (confidence: 100%) MetricAlertDetectorHandler now inherits from StatefulDetectorHandler but does not implement the abstract method build_occurrence_and_event_data. This means any attempt to instantiate MetricAlertDetectorHandler in production will raise a TypeError, effectively breaking metric alert detection. Until implemented, no metric alert occurrences can be produced. Evidence:
Agent: security There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡
--- a/src/sentry/incidents/grouptype.py
+++ b/src/sentry/incidents/grouptype.py
@@ -1,12 +1,23 @@
from dataclasses import dataclass
+from typing import Any
+
from sentry.incidents.utils.types import QuerySubscriptionUpdate
from sentry.issues.grouptype import GroupCategory, GroupType
from sentry.ratelimits.sliding_windows import Quota
from sentry.types.group import PriorityLevel
from sentry.workflow_engine.processors.detector import StatefulDetectorHandler
# TODO: This will be a stateful detector when we build that abstraction
class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]):
- pass
+ def build_occurrence_and_event_data(
+ self, *args: Any, **kwargs: Any
+ ) -> Any:
+ # TODO: Implement build_occurrence_and_event_data for metric alert detection.
+ # This must be completed before MetricAlertDetectorHandler can be used in production.
+ # See StatefulDetectorHandler.build_occurrence_and_event_data for the expected signature
+ # and return type.
+ raise NotImplementedError(
+ "MetricAlertDetectorHandler.build_occurrence_and_event_data is not yet implemented. "
+ "This must be implemented before metric alert occurrences can be produced."
+ )🤖 Grapple PR auto-fix • major • Review this diff before applying |
||
|
|
||
|
|
||
| # Example GroupType and detector handler for metric alerts. We don't create these issues yet, but we'll use something | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||
| from __future__ import annotations | ||||||
|
|
||||||
| import builtins | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MINOR — Dependency drift (confidence: 93%)
Evidence:
Agent: architecture There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡
--- a/src/sentry/workflow_engine/models/detector.py
+++ b/src/sentry/workflow_engine/models/detector.py
@@ -1,7 +1,6 @@
from __future__ import annotations
-import builtins
import logging
from typing import TYPE_CHECKING
@@ -55,7 +54,7 @@ def project_id(self):
return 1
@property
- def group_type(self) -> builtins.type[GroupType] | None:
+ def group_type(self) -> type[GroupType] | None:
return grouptype.registry.get_by_slug(self.type)
@property🤖 Grapple PR auto-fix • minor • Review this diff before applying |
||||||
| import logging | ||||||
| from typing import TYPE_CHECKING | ||||||
|
|
||||||
|
|
@@ -9,6 +10,7 @@ | |||||
| from sentry.backup.scopes import RelocationScope | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 INFO — Naming conventions (confidence: 89%) Import statement Evidence:
Agent: style |
||||||
| from sentry.db.models import DefaultFieldsModel, FlexibleForeignKey, region_silo_model | ||||||
| from sentry.issues import grouptype | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 INFO — Dependency risks (confidence: 99%) import builtins is used solely to reference builtins.type[GroupType] in the type annotation. While not a vulnerability, this is an unusual pattern that could cause confusion and is unnecessary — Python's built-in Evidence:
Agent: security There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MINOR — naming conventions (confidence: 83%) Import of Evidence:
Agent: style There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡
--- a/src/sentry/workflow_engine/models/detector.py
+++ b/src/sentry/workflow_engine/models/detector.py
@@ -1,7 +1,6 @@
from __future__ import annotations
-import builtins
import logging
from typing import TYPE_CHECKING
@@ -55,7 +55,7 @@ def project_id(self):
return 1
@property
- def group_type(self) -> builtins.type[GroupType] | None:
+ def group_type(self) -> type[GroupType] | None:
return grouptype.registry.get_by_slug(self.type)
@property🤖 Grapple PR auto-fix • minor • Review this diff before applying |
||||||
| from sentry.issues.grouptype import GroupType | ||||||
| from sentry.models.owner_base import OwnerModel | ||||||
|
|
||||||
| if TYPE_CHECKING: | ||||||
|
|
@@ -53,9 +55,13 @@ def project_id(self): | |||||
| # XXX: Temporary property until we add `project_id` to the model. | ||||||
| return 1 | ||||||
|
|
||||||
| @property | ||||||
| def group_type(self) -> builtins.type[GroupType] | None: | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MINOR — Scalability concerns (confidence: 93%) The new Evidence:
Agent: architecture There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡
--- a/src/sentry/workflow_engine/models/detector.py
+++ b/src/sentry/workflow_engine/models/detector.py
@@ -1,6 +1,7 @@
from __future__ import annotations
import builtins
+from functools import cached_property
import logging
from typing import TYPE_CHECKING
@@ -53,7 +54,7 @@ def project_id(self):
# XXX: Temporary property until we add `project_id` to the model.
return 1
- @property
+ @cached_property
def group_type(self) -> builtins.type[GroupType] | None:
+ # Registry lookup is memoized per instance to avoid repeated lookups in hot paths
return grouptype.registry.get_by_slug(self.type)
@property🤖 Grapple PR auto-fix • minor • Review this diff before applying |
||||||
| return grouptype.registry.get_by_slug(self.type) | ||||||
|
|
||||||
| @property | ||||||
| def detector_handler(self) -> DetectorHandler | None: | ||||||
| group_type = grouptype.registry.get_by_slug(self.type) | ||||||
| group_type = self.group_type | ||||||
| if not group_type: | ||||||
| logger.error( | ||||||
| "No registered grouptype for detector", | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MINOR — Unnecessary work (confidence: 95%) Detector.group_type introduces a second registry lookup when detector_handler is called, since detector_handler now calls self.group_type instead of a local variable. Previously there was one registry lookup per detector_handler access; now any caller that uses both group_type and detector_handler will hit the registry twice. The registry lookup (get_by_slug) likely involves a dict scan or similar O(n) operation over registered group types. Evidence:
Agent: performance There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅
Suggested change
🤖 Grapple PR auto-fix • minor • confidence: 95% |
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |||||||||||||||||||||
| from sentry.issues.producer import PayloadType, produce_occurrence_to_kafka | ||||||||||||||||||||||
| from sentry.issues.status_change_message import StatusChangeMessage | ||||||||||||||||||||||
| from sentry.models.group import GroupStatus | ||||||||||||||||||||||
| from sentry.types.group import PriorityLevel | ||||||||||||||||||||||
| from sentry.utils import metrics, redis | ||||||||||||||||||||||
| from sentry.utils.function_cache import cache_func_for_models | ||||||||||||||||||||||
| from sentry.utils.iterators import chunked | ||||||||||||||||||||||
|
|
@@ -45,7 +46,7 @@ class DetectorEvaluationResult: | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| def process_detectors( | ||||||||||||||||||||||
| data_packet: DataPacket, detectors: list[Detector] | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 MAJOR — Cross-service impact / API contract violation (confidence: 99%) The return type of Evidence:
Agent: architecture There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -46,7 +46,14 @@ class DetectorEvaluationResult:
def process_detectors(
data_packet: DataPacket, detectors: list[Detector]
) -> list[tuple[Detector, dict[DetectorGroupKey, DetectorEvaluationResult]]]:
+ """
+ Evaluates each detector against the given data packet.
+
+ Returns a list of (Detector, results) tuples where results is a dict
+ keyed by DetectorGroupKey. NOTE: Callers must iterate over
+ ``results.values()`` to access DetectorEvaluationResult objects —
+ iterating over the dict directly yields keys, not results.
+ """
results = []
for detector in detectors:
@@ -58,6 +65,16 @@ def process_detectors(
detector_results = handler.evaluate(data_packet)
for result in detector_results.values():
+ if result.group_key in detector_results and list(detector_results.keys()).count(result.group_key) > 1:
+ # This shouldn't happen - log an error and continue on, but we should investigate this.
+ logger.error(
+ "Duplicate detector state group keys found",
+ extra={
+ "detector_id": detector.id,
+ "group_key": result.group_key,
+ },
+ )
+ continue
+
if result.result is not None:
create_issue_occurrence_from_result(result)
@@ -234,10 +251,16 @@ def evaluate(
dedupe_value = self.get_dedupe_value(data_packet)
group_values = self.get_group_key_values(data_packet)
all_state_data = self.get_state_data(list(group_values.keys()))
results = {}
for group_key, group_value in group_values.items():
result = self.evaluate_group_key_value(
group_key, group_value, all_state_data[group_key], dedupe_value
)
if result:
+ if result.group_key in results:
+ logger.error(
+ "Duplicate detector state group keys found",
+ extra={
+ "detector_id": self.detector.id,
+ "group_key": result.group_key,
+ },
+ )
+ continue
results[result.group_key] = result
return results🤖 Grapple PR auto-fix • major • Review this diff before applying |
||||||||||||||||||||||
| ) -> list[tuple[Detector, list[DetectorEvaluationResult]]]: | ||||||||||||||||||||||
| ) -> list[tuple[Detector, dict[DetectorGroupKey, DetectorEvaluationResult]]]: | ||||||||||||||||||||||
| results = [] | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| for detector in detectors: | ||||||||||||||||||||||
|
|
@@ -55,25 +56,11 @@ def process_detectors( | |||||||||||||||||||||
| continue | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| detector_results = handler.evaluate(data_packet) | ||||||||||||||||||||||
| detector_group_keys = set() | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| for result in detector_results: | ||||||||||||||||||||||
| if result.group_key in detector_group_keys: | ||||||||||||||||||||||
| # This shouldn't happen - log an error and continue on, but we should investigate this. | ||||||||||||||||||||||
| logger.error( | ||||||||||||||||||||||
| "Duplicate detector state group keys found", | ||||||||||||||||||||||
| extra={ | ||||||||||||||||||||||
| "detector_id": detector.id, | ||||||||||||||||||||||
| "group_key": result.group_key, | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| continue | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| for result in detector_results.values(): | ||||||||||||||||||||||
| if result.result is not None: | ||||||||||||||||||||||
| create_issue_occurrence_from_result(result) | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 MAJOR — Edge Cases (confidence: 95%) Empty dict returned from evaluate() is falsy in Python ( Evidence:
Agent: logic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅
Suggested change
🤖 Grapple PR auto-fix • major • confidence: 95% |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| detector_group_keys.add(result.group_key) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if detector_results: | ||||||||||||||||||||||
| results.append((detector, detector_results)) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -136,7 +123,9 @@ def __init__(self, detector: Detector): | |||||||||||||||||||||
| self.conditions = [] | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @abc.abstractmethod | ||||||||||||||||||||||
| def evaluate(self, data_packet: DataPacket[T]) -> list[DetectorEvaluationResult]: | ||||||||||||||||||||||
| def evaluate( | ||||||||||||||||||||||
| self, data_packet: DataPacket[T] | ||||||||||||||||||||||
| ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: | ||||||||||||||||||||||
| pass | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def commit_state_updates(self): | ||||||||||||||||||||||
|
|
@@ -174,6 +163,12 @@ def get_group_key_values(self, data_packet: DataPacket[T]) -> dict[str, int]: | |||||||||||||||||||||
| """ | ||||||||||||||||||||||
| pass | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @abc.abstractmethod | ||||||||||||||||||||||
| def build_occurrence_and_event_data( | ||||||||||||||||||||||
| self, group_key: DetectorGroupKey, value: int, new_status: PriorityLevel | ||||||||||||||||||||||
| ) -> tuple[IssueOccurrence, dict[str, Any]]: | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MINOR — Input Validation (confidence: 93%) build_occurrence_and_event_data receives Evidence:
Agent: security There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -290,8 +290,22 @@ def evaluate_group_key_value(
else:
- result, event_data = self.build_occurrence_and_event_data(
- group_key, value, PriorityLevel(new_status)
- )
+ try:
+ priority_level = PriorityLevel(new_status)
+ except ValueError:
+ logger.exception(
+ "Failed to convert DetectorPriorityLevel to PriorityLevel",
+ extra={
+ "detector_id": self.detector.id,
+ "group_key": group_key,
+ "new_status": new_status,
+ },
+ )
+ return None
+ try:
+ result, event_data = self.build_occurrence_and_event_data(
+ group_key, value, priority_level
+ )
+ except Exception:
+ logger.exception(
+ "Failed to build occurrence and event data",
+ extra={
+ "detector_id": self.detector.id,
+ "group_key": group_key,
+ "value": value,
+ "new_status": new_status,
+ },
+ )
+ return None🤖 Grapple PR auto-fix • minor • Review this diff before applying |
||||||||||||||||||||||
| pass | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def build_fingerprint(self, group_key) -> list[str]: | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| Builds a fingerprint to uniquely identify a detected issue | ||||||||||||||||||||||
|
|
@@ -228,7 +223,9 @@ def get_state_data( | |||||||||||||||||||||
| ) | ||||||||||||||||||||||
| return results | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def evaluate(self, data_packet: DataPacket[T]) -> list[DetectorEvaluationResult]: | ||||||||||||||||||||||
| def evaluate( | ||||||||||||||||||||||
| self, data_packet: DataPacket[T] | ||||||||||||||||||||||
| ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| Evaluates a given data packet and returns a list of `DetectorEvaluationResult`. | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 INFO — code patterns (confidence: 84%) The Evidence:
Agent: style |
||||||||||||||||||||||
| There will be one result for each group key result in the packet, unless the | ||||||||||||||||||||||
|
|
@@ -237,13 +234,13 @@ def evaluate(self, data_packet: DataPacket[T]) -> list[DetectorEvaluationResult] | |||||||||||||||||||||
| dedupe_value = self.get_dedupe_value(data_packet) | ||||||||||||||||||||||
| group_values = self.get_group_key_values(data_packet) | ||||||||||||||||||||||
| all_state_data = self.get_state_data(list(group_values.keys())) | ||||||||||||||||||||||
| results = [] | ||||||||||||||||||||||
| results = {} | ||||||||||||||||||||||
| for group_key, group_value in group_values.items(): | ||||||||||||||||||||||
| result = self.evaluate_group_key_value( | ||||||||||||||||||||||
| group_key, group_value, all_state_data[group_key], dedupe_value | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 INFO — Documentation (confidence: 84%) The docstring for Evidence:
Agent: style |
||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| if result: | ||||||||||||||||||||||
| results.append(result) | ||||||||||||||||||||||
| results[result.group_key] = result | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 MAJOR — Edge case - silent duplicate suppression (confidence: 100%) The evaluate() method iterates over group_values.items() and stores results in a dict keyed by result.group_key. If get_group_key_values returns keys that map to different values but evaluate_group_key_value produces results with the same group_key (e.g., via some transformation), the dict will silently keep only the last result. The previously explicit duplicate-key error log has been removed, so this scenario now has zero observability. While the intent notes this is by design, the removal of the error log means any future bug in get_group_key_values that produces duplicate keys will be completely silent. Evidence:
Agent: logic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -234,10 +234,20 @@ def evaluate(
dedupe_value = self.get_dedupe_value(data_packet)
group_values = self.get_group_key_values(data_packet)
all_state_data = self.get_state_data(list(group_values.keys()))
results = {}
for group_key, group_value in group_values.items():
result = self.evaluate_group_key_value(
group_key, group_value, all_state_data[group_key], dedupe_value
)
if result:
+ if result.group_key in results:
+ # This can happen if get_group_key_values returns keys that
+ # map to the same result.group_key after transformation in
+ # evaluate_group_key_value. Last-write-wins is intentional
+ # per the dict-keyed design, but log a warning so any future
+ # bug in get_group_key_values producing duplicate keys is
+ # observable. See: Silent removal of duplicate group key
+ # error logging in process_detectors.
+ logger.warning(
+ "Duplicate detector state group keys found",
+ extra={
+ "detector_id": self.detector.id,
+ "group_key": result.group_key,
+ "input_group_key": group_key,
+ },
+ )
results[result.group_key] = result
return results🤖 Grapple PR auto-fix • major • Review this diff before applying |
||||||||||||||||||||||
| return results | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 MAJOR — Intent Alignment - Silent Overwrite (confidence: 90%) The previous code logged an error when duplicate group keys were detected. The dict structure now silently overwrites earlier results for the same key. While the intent spec acknowledges removing the duplicate detection, this is identified as a risk area: 'any bugs in evaluate() returning conflicting keys will now silently overwrite earlier results rather than logging an error'. Since Evidence:
Agent: logic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅
Suggested change
🤖 Grapple PR auto-fix • major • confidence: 90% |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 MAJOR — Pattern violation / Silent data loss (confidence: 84%) The dict-based return from Evidence:
Agent: architecture There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -234,11 +234,27 @@ def evaluate(
dedupe_value = self.get_dedupe_value(data_packet)
group_values = self.get_group_key_values(data_packet)
all_state_data = self.get_state_data(list(group_values.keys()))
results = {}
for group_key, group_value in group_values.items():
result = self.evaluate_group_key_value(
group_key, group_value, all_state_data[group_key], dedupe_value
)
if result:
- results[result.group_key] = result
+ if result.group_key != group_key:
+ # A subclass returned a result whose group_key differs from the
+ # iteration key. This indicates a bug in the override — log an
+ # error and use the iteration key so we don't silently corrupt
+ # the results dict with an unexpected key.
+ logger.error(
+ "Detector evaluate_group_key_value returned mismatched group_key",
+ extra={
+ "detector_id": self.detector.id,
+ "expected_group_key": group_key,
+ "returned_group_key": result.group_key,
+ },
+ )
+ if result.group_key in results:
+ # This should not happen — log an error to match the behaviour
+ # of the old list-based duplicate detection (see removed lines
+ # that logged "Duplicate detector state group keys found").
+ logger.error(
+ "Duplicate detector state group keys found",
+ extra={
+ "detector_id": self.detector.id,
+ "group_key": result.group_key,
+ },
+ )
+ continue
+ results[result.group_key] = result
return results🤖 Grapple PR auto-fix • major • Review this diff before applying There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MINOR — Documentation (confidence: 100%) The docstring for StatefulDetectorHandler.evaluate() still refers to returning 'a list of DetectorEvaluationResult' but the method now returns a dict. The docstring should be updated to reflect the new return type. Evidence:
Agent: style There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅
Suggested change
🤖 Grapple PR auto-fix • minor • confidence: 100% |
||||||||||||||||||||||
| def evaluate_group_key_value( | ||||||||||||||||||||||
|
|
@@ -289,7 +286,7 @@ def evaluate_group_key_value( | |||||||||||||||||||||
| is_active = new_status != DetectorPriorityLevel.OK | ||||||||||||||||||||||
| self.enqueue_state_update(group_key, is_active, new_status) | ||||||||||||||||||||||
| event_data = None | ||||||||||||||||||||||
| result = None | ||||||||||||||||||||||
| result: StatusChangeMessage | IssueOccurrence | ||||||||||||||||||||||
| if new_status == DetectorPriorityLevel.OK: | ||||||||||||||||||||||
| # If we've determined that we're now ok, we just want to resolve the issue | ||||||||||||||||||||||
| result = StatusChangeMessage( | ||||||||||||||||||||||
|
|
@@ -298,8 +295,10 @@ def evaluate_group_key_value( | |||||||||||||||||||||
| new_status=GroupStatus.RESOLVED, | ||||||||||||||||||||||
| new_substatus=None, | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # TODO: Add hook here for generating occurrence | ||||||||||||||||||||||
| else: | ||||||||||||||||||||||
| result, event_data = self.build_occurrence_and_event_data( | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 MAJOR — Error Handling (confidence: 100%) When Evidence:
Agent: logic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MINOR — Data Flow (confidence: 100%) The Evidence:
Agent: logic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -293,7 +293,18 @@ def evaluate_group_key_value(
else:
- result, event_data = self.build_occurrence_and_event_data(
- group_key, value, PriorityLevel(new_status)
- )
+ try:
+ result, event_data = self.build_occurrence_and_event_data(
+ group_key, value, PriorityLevel(new_status)
+ )
+ except Exception:
+ logger.exception(
+ "Failed to build occurrence and event data for detector",
+ extra={
+ "detector_id": self.detector.id,
+ "group_key": group_key,
+ "new_status": new_status,
+ },
+ )
+ return None
return DetectorEvaluationResult(
group_key=group_key,
is_active=is_active,🤖 Grapple PR auto-fix • major • Review this diff before applying There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -263,7 +263,7 @@ class StatefulDetectorHandler(DetectorHandler[T], abc.ABC):
def evaluate_group_key_value(
- self, group_key, value, state_data, dedupe_value
+ self, group_key, group_value, state_data, dedupe_value
) -> DetectorEvaluationResult | None:
"""
Evaluates a single group key value against its state data and returns
a DetectorEvaluationResult if any state changes are detected.
"""
- if not self.conditions or not any(
- condition.is_triggered(value) for condition in self.conditions
- ):
+ if not self.conditions or not any(
+ condition.is_triggered(group_value) for condition in self.conditions
+ ):
return None
state_data = state_data or DetectorStateData(
group_key=group_key,
is_active=False,
status=DetectorPriorityLevel.OK,
dedupe_value=None,
counter_updates={},
)
- new_status = self.get_new_status(state_data, value)
+ new_status = self.get_new_status(state_data, group_value)
if new_status is None or new_status == state_data.status:
return None
if dedupe_value <= (state_data.dedupe_value or 0):
return None
is_active = new_status != DetectorPriorityLevel.OK
self.enqueue_state_update(group_key, is_active, new_status)
event_data = None
result: StatusChangeMessage | IssueOccurrence
if new_status == DetectorPriorityLevel.OK:
result = StatusChangeMessage(
fingerprint=self.build_fingerprint(group_key),
project_id=self.detector.project_id,
new_status=GroupStatus.RESOLVED,
new_substatus=None,
)
else:
result, event_data = self.build_occurrence_and_event_data(
- group_key, value, PriorityLevel(new_status)
+ group_key, group_value, PriorityLevel(new_status)
)
return DetectorEvaluationResult(
group_key=group_key,
is_active=is_active,
priority=new_status,
result=result,
event_data=event_data,
)
return None🤖 Grapple PR auto-fix • minor • Review this diff before applying |
||||||||||||||||||||||
| group_key, value, PriorityLevel(new_status) | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 MAJOR — Data flow - enum type mismatch (confidence: 99%) build_occurrence_and_event_data is called with Evidence:
Agent: logic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -14,6 +14,7 @@
from sentry.issues.producer import PayloadType, produce_occurrence_to_kafka
from sentry.issues.status_change_message import StatusChangeMessage
from sentry.models.group import GroupStatus
+from sentry.types.group import PriorityLevel
from sentry.utils import metrics, redis
from sentry.utils.function_cache import cache_func_for_models
from sentry.utils.iterators import chunked
@@ -20,6 +21,14 @@ from sentry.workflow_engine.types import (
DetectorGroupKey,
DetectorPriorityLevel,
)
+
+# Explicit mapping from DetectorPriorityLevel to PriorityLevel.
+# Do NOT replace this with PriorityLevel(detector_priority_level) — the two enums
+# may not share identical integer values, and implicit coercion would silently
+# assign the wrong priority if they ever diverge.
+DETECTOR_PRIORITY_TO_ISSUE_PRIORITY: dict[DetectorPriorityLevel, PriorityLevel] = {
+ DetectorPriorityLevel.LOW: PriorityLevel.LOW,
+ DetectorPriorityLevel.MEDIUM: PriorityLevel.MEDIUM,
+ DetectorPriorityLevel.HIGH: PriorityLevel.HIGH,
+ DetectorPriorityLevel.CRITICAL: PriorityLevel.CRITICAL,
+}
@@ -295,7 +304,12 @@ def evaluate_group_key_value(
new_substatus=None,
)
else:
+ # Use the explicit mapping — never rely on int coercion between
+ # DetectorPriorityLevel and PriorityLevel, as their integer values
+ # are not guaranteed to be identical. A KeyError here means a new
+ # DetectorPriorityLevel was added without updating the mapping above.
+ issue_priority = DETECTOR_PRIORITY_TO_ISSUE_PRIORITY[new_status]
result, event_data = self.build_occurrence_and_event_data(
- group_key, value, PriorityLevel(new_status)
+ group_key, value, issue_priority
)🤖 Grapple PR auto-fix • major • Review this diff before applying |
||||||||||||||||||||||
| return DetectorEvaluationResult( | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MINOR — Code organization (confidence: 95%) Variable Evidence:
Agent: style There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -286,16 +286,17 @@ def evaluate_group_key_value(
is_active = new_status != DetectorPriorityLevel.OK
self.enqueue_state_update(group_key, is_active, new_status)
- event_data = None
- result: StatusChangeMessage | IssueOccurrence
- if new_status == DetectorPriorityLevel.OK:
+ result: StatusChangeMessage | IssueOccurrence
+ if new_status == DetectorPriorityLevel.OK:
# If we've determined that we're now ok, we just want to resolve the issue
result = StatusChangeMessage(
fingerprint=self.build_fingerprint(group_key),
project_id=self.detector.project_id,
new_status=GroupStatus.RESOLVED,
new_substatus=None,
)
-
- # TODO: Add hook here for generating occurrence
+ event_data = None
+ else:
+ result, event_data = self.build_occurrence_and_event_data(
+ group_key, value, PriorityLevel(new_status)
+ )
return DetectorEvaluationResult(
group_key=group_key,
is_active=is_active,🤖 Grapple PR auto-fix • minor • Review this diff before applying There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 MAJOR — Module boundaries / Type safety (confidence: 94%) The code converts Evidence:
Agent: architecture There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡
--- a/src/sentry/workflow_engine/processors/detector.py
+++ b/src/sentry/workflow_engine/processors/detector.py
@@ -1,6 +1,7 @@
from __future__ import annotations
import abc
+import dataclasses
import logging
from datetime import timedelta
from typing import Any
@@ -14,6 +15,7 @@
from sentry.issues.producer import PayloadType, produce_occurrence_to_kafka
from sentry.issues.status_change_message import StatusChangeMessage
from sentry.models.group import GroupStatus
+from sentry.types.group import PriorityLevel
from sentry.utils import metrics, redis
from sentry.utils.function_cache import cache_func_for_models
from sentry.utils.iterators import chunked
@@ -20,6 +22,28 @@
from sentry.workflow_engine.models import DataPacket, Detector
from sentry.workflow_engine.types import DetectorGroupKey, DetectorPriorityLevel
+# Explicit mapping from DetectorPriorityLevel to PriorityLevel.
+#
+# IMPORTANT: Do NOT replace this with PriorityLevel(new_status) — an implicit
+# integer cast. DetectorPriorityLevel.OK == 0 has no corresponding PriorityLevel
+# value and would raise a ValueError. Even for non-OK values the integer
+# coincidence is accidental; this mapping makes the relationship auditable and
+# will surface a clear error if the enums ever diverge.
+_DETECTOR_PRIORITY_TO_PRIORITY_LEVEL: dict[DetectorPriorityLevel, PriorityLevel] = {
+ DetectorPriorityLevel.LOW: PriorityLevel.LOW,
+ DetectorPriorityLevel.MEDIUM: PriorityLevel.MEDIUM,
+ DetectorPriorityLevel.HIGH: PriorityLevel.HIGH,
+ DetectorPriorityLevel.CRITICAL: PriorityLevel.CRITICAL,
+ # DetectorPriorityLevel.OK is intentionally excluded: callers must handle
+ # the OK case before converting (it maps to GroupStatus.RESOLVED, not a
+ # PriorityLevel).
+}
+
+
+def detector_priority_to_priority_level(level: DetectorPriorityLevel) -> PriorityLevel:
+ """Convert a non-OK DetectorPriorityLevel to a PriorityLevel."""
+ try:
+ return _DETECTOR_PRIORITY_TO_PRIORITY_LEVEL[level]
+ except KeyError:
+ raise ValueError(
+ f"Cannot convert DetectorPriorityLevel.{level.name} to PriorityLevel. "
+ f"Only non-OK levels have a PriorityLevel equivalent."
+ )
+
+
logger = logging.getLogger(__name__)
@@ -286,9 +310,12 @@ def evaluate_group_key_value(
self.enqueue_state_update(group_key, is_active, new_status)
event_data = None
result: StatusChangeMessage | IssueOccurrence
if new_status == DetectorPriorityLevel.OK:
# If we've determined that we're now ok, we just want to resolve the issue
result = StatusChangeMessage(
fingerprint=self.build_fingerprint(group_key),
project_id=self.detector.project_id,
new_status=GroupStatus.RESOLVED,
new_substatus=None,
)
else:
+ # Use the explicit mapping function rather than PriorityLevel(new_status).
+ # The implicit integer cast would raise ValueError for OK=0 and is
+ # fragile if either enum's values change in future.
result, event_data = self.build_occurrence_and_event_data(
- group_key, value, PriorityLevel(new_status)
+ group_key, value, detector_priority_to_priority_level(new_status)
)
return DetectorEvaluationResult(
group_key=group_key,
is_active=is_active,
priority=new_status,
result=result,
event_data=event_data,
)
return None🤖 Grapple PR auto-fix • major • Review this diff before applying |
||||||||||||||||||||||
| group_key=group_key, | ||||||||||||||||||||||
| is_active=is_active, | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 INFO — documentation (confidence: 84%)
The
MetricAlertDetectorHandlerclass body is now empty (pass), but the TODO comment above it says 'This will be a stateful detector when we build that abstraction.' This comment is now outdated since the class IS now a stateful detector.Evidence:
StatefulDetectorHandlerinstead ofDetectorHandlerAgent: style