feat(ACI): Send updated data to Seer on all snuba query changes #103332
Conversation
github-actions
bot
added
the
Scope: Backend
| if updated_data_source_data: | ||
| data_source_data = updated_data_source_data[0] | ||
| event_types = data_source_data.get("eventTypes") | ||
| event_types = data_source_data.get("event_types") |
There was a problem hiding this comment.
this was just incorrect before, oops
There was a problem hiding this comment.
Mind adding a test for this to ensure everything is working as expected with this change?
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| ) or snuba_query.aggregate != data_source.get("aggregate"): | ||
| try: | ||
| validated_data_source = cast(dict[str, Any], data_source) | ||
| try: |
There was a problem hiding this comment.
Bug: Incorrect Enum Comparison Always Fails
The comparison checks if instance.config.get("detection_type") equals AlertRuleDetectionType.DYNAMIC, but the config stores string values while the enum comparison uses the enum member itself. This comparison will always be False since it's comparing a string ("dynamic") against an enum member. The check should use AlertRuleDetectionType.DYNAMIC.value to compare with the stored string value, as done elsewhere in the codebase at line 357.
There was a problem hiding this comment.
I don't think cursor understands string enums. My tests would fail if this were true.
| event=audit_log.get_event_id("DETECTOR_EDIT"), | ||
| data=dynamic_detector.get_audit_log_data(), | ||
| ) | ||
| mock_audit.reset_mock() |
There was a problem hiding this comment.
i'd recommend splitting this out into a new test case or breaking down this larger test.
saponifi3d
left a comment
saponifi3d
left a comment
There was a problem hiding this comment.
thanks for the tests!
| if updated_data_source_data: | ||
| data_source_data = updated_data_source_data[0] | ||
| event_types = data_source_data.get("eventTypes") | ||
| event_types = data_source_data.get("event_types") |
There was a problem hiding this comment.
Bug: Inconsistent Data: Mixing Stale and Current Values
When data_source_data.get("event_types") returns None (field not being updated), event_types is set to None and later falls back to snuba_query.event_types at line 162. However, snuba_query.event_types is a property that queries the database and returns the OLD values, while other fields like dataset, query, and aggregate may have been modified in-memory via setattr at lines 105-113. This creates an inconsistency where Seer receives a mix of new and old values - the updated fields reflect the changes, but event_types reflects the stale database state rather than what's actually being saved.
|
|
||
| if data_source is not None: | ||
| self.update_data_source(instance, data_source) | ||
| self.update_data_source(instance, data_source, seer_updated) |
There was a problem hiding this comment.
Bug: Transactional Flaw Leaves Detector Inconsistent
The detector update lacks transactional consistency. The parent's update at line 325 commits detector config changes (like detection type) in a transaction, but update_data_source at line 345 runs after that transaction completes. If update_data_source raises a ValidationError (which can happen at multiple points including Seer communication failures), the detector config will already be persisted while the snuba query changes won't be applied. This leaves the detector in an inconsistent state where its config (e.g., detection type) doesn't match its underlying query configuration.
Follow up to #102934 (comment) to update Seer when anything on the snuba query changes - we were missing some instances where the data changed in a way Seer would want to know about but we weren't sending the updates.