feat(ACI): Send historical data to seer on update #102934
Conversation
github-actions
bot
added
the
Scope: Backend
| "id": self.data_condition_group.id, | ||
| "organizationId": self.organization.id, |
There was a problem hiding this comment.
These aren't sent by the front end so I wanted this to be the same. Especially for the ids, it doesn't make sense that we'd be sending these on creation.
| @@ -553,6 +551,379 @@ def test_transaction_dataset_deprecation_multiple_data_sources(self) -> None: | |||
| ): | |||
| validator.save() | |||
|
|
|||
|
|
|||
| class TestMetricAlertsUpdateDetectorValidator(TestMetricAlertsDetectorValidator): | |||
| def test_update_with_valid_data(self) -> None: | |||
There was a problem hiding this comment.
We didn't have a simple update test case so I added one
| raise DetectorException( | ||
| f"Could not create detector, data condition {dcg_id} not found or too many found." | ||
| ) | ||
| # use setattr to avoid saving the models until the Seer call has successfully finished, |
There was a problem hiding this comment.
This is the same as we do it for alert rules https://github.com/getsentry/sentry/blob/master/src/sentry/seer/anomaly_detection/store_data.py#L122
ceorourke
force-pushed
the
ceorourke/send-historical-data-to-seer-on-update
branch
from
c4fdd12 to
2adcc69
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #102934 +/- ##
===========================================
+ Coverage 79.08% 80.70% +1.61%
===========================================
Files 9224 9226 +2
Lines 393906 394080 +174
Branches 25109 25109
===========================================
+ Hits 311527 318032 +6505
+ Misses 81931 75600 -6331
Partials 448 448 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| resolution=timedelta(seconds=data_source.get("resolution", snuba_query.resolution)), | ||
| environment=data_source.get("environment", snuba_query.environment), | ||
| event_types=data_source.get("event_types", [event_type for event_type in event_types]), | ||
| ) |
There was a problem hiding this comment.
Bug: update_detector_data receives a single data source dict instead of a {"data_sources": [...]} structure, preventing snuba_query updates.
Severity: CRITICAL | Confidence: 0.95
🔍 Detailed Analysis
When update_detector_data is invoked at src/sentry/incidents/metric_issue_detector.py:249, it receives validated_data_source, which is a single dictionary. However, the update_detector_data function expects a dictionary containing a "data_sources" key with a list of data sources. This mismatch causes the internal logic to skip updating the snuba_query object's fields. Consequently, when a dynamic detector's snuba query is updated, the old query, aggregate, and event types are sent to Seer instead of the new values, leading to anomaly detection operating on incorrect metrics.
💡 Suggested Fix
Modify the call to update_detector_data at src/sentry/incidents/metric_issue_detector.py:249 to pass {"data_sources": [validated_data_source]} instead of validated_data_source directly, aligning with the expected input structure.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry/incidents/metric_issue_detector.py#L249
Potential issue: When `update_detector_data` is invoked at
`src/sentry/incidents/metric_issue_detector.py:249`, it receives
`validated_data_source`, which is a single dictionary. However, the
`update_detector_data` function expects a dictionary containing a `"data_sources"` key
with a list of data sources. This mismatch causes the internal logic to skip updating
the `snuba_query` object's fields. Consequently, when a dynamic detector's snuba query
is updated, the old query, aggregate, and event types are sent to Seer instead of the
new values, leading to anomaly detection operating on incorrect metrics.
Did we get this right? 👍 / 👎 to inform future reviews.
mifu67
left a comment
mifu67
left a comment
There was a problem hiding this comment.
The update logic itself looks good; just a question about when we should be updating.
src/sentry/seer/anomaly_detection/store_data_workflow_engine.py
Outdated
Show resolved
Hide resolved
|
|
||
| # Handle a dynamic detector's snuba query changing | ||
| if instance.config.get("detection_type") == AlertRuleDetectionType.DYNAMIC: | ||
| if snuba_query.query != data_source.get( |
There was a problem hiding this comment.
Do we need to check other snuba query fields as well (like timeWindow)? Should we just resend the data every time we update a dynamic detector?
There was a problem hiding this comment.
Good question, I will look into this for a follow up. I think every time we change it isn't necessary (e.g. it could just be changing the name of the detector) but we might be missing some cases we should be updating.
| event_types, | ||
| ) | ||
| except (TimeoutError, MaxRetryError, ParseError, ValidationError): | ||
| raise ValidationError("Couldn't send data to Seer, unable to update detector") |
There was a problem hiding this comment.
Wouldn't it be better to track timeout, retry failure and parse errors separately as opposed to converting them to ValidationError?
There was a problem hiding this comment.
I converted them to a ValidationError because this is in the update detector method and this is how we can surface the error to the user - I can break it out into separate ones to be more informative though:
except TimeoutError:
raise ValidationError("Timed out sending data to Seer, unable to update detector
89441c2 to
7a87a33
Compare
| if instance.config.get("detection_type") == AlertRuleDetectionType.DYNAMIC: | ||
| if snuba_query.query != data_source.get( | ||
| "query" | ||
| ) or snuba_query.aggregate != data_source.get("aggregate"): |
There was a problem hiding this comment.
Bug: Prevent Unnecessary Seer API Calls
The condition checking if a dynamic detector's query or aggregate changed compares against data_source.get("query") and data_source.get("aggregate") which return None when these fields aren't being updated. This causes the comparison snuba_query.query != None to be True even when the query hasn't changed, triggering unnecessary Seer API calls. The defaults should be the existing values: data_source.get("query", snuba_query.query) and data_source.get("aggregate", snuba_query.aggregate).
ceorourke
force-pushed
the
ceorourke/send-historical-data-to-seer-on-update
branch
from
0236524 to
2d5dec1
Compare
ceorourke
deleted the
ceorourke/send-historical-data-to-seer-on-update
branch
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues: |
) 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.
When a static or percent based detector is changed to become a dynamic detector we need to send Seer historical data for that detector so it can detect anomalies. Also when a dynamic detector's snuba query query or aggregate changes we need to update the data Seer has so it's detecting anomalies on the correct data. This PR also handles not updating the existing data if the call to Seer fails for any reason.