ref(aci): Remove a try/except block for more detailed errors#111439
Conversation
Backend Test FailuresFailures on
|
…the ValiditonError type and the re-raises were used as flow control.
0e179b7 to
b72343e
Compare
ceorourke
left a comment
There was a problem hiding this comment.
lgtm assuming we're ok with the information exposure risk
|
@ceorourke if you have any context on things that might be getting exposed outside of those expected cases, that'd be helpful! It seemed limited to me: https://github.com/getsentry/sentry/blob/master/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py#L118-L125 -- but not sure if i'm missing anything else? I guess it could potentially throw an exception on any line, and we probably wouldn't want to expose it? (I had also tried to let them bubble up from the original, but they're different validation errors, so the API won't create them as proper validation errors. 😭) |
…cted seer problems
| except (TimeoutError, MaxRetryError, ParseError, ValidationError) as e: | ||
| # don't update the snuba query if we failed to send data to Seer | ||
| raise serializers.ValidationError( | ||
| "Failed to send data to Seer, cannot update detector" | ||
| ) | ||
| raise serializers.ValidationError(str(e)) |
There was a problem hiding this comment.
Bug: The narrowed exception handling in update_data_source and update_anomaly_detection no longer catches DetectorException, which will cause an unhandled exception and a 500 error.
Severity: CRITICAL
Suggested Fix
Add DetectorException to the list of caught exceptions in the try...except blocks within update_data_source() and update_anomaly_detection() to ensure it is handled correctly and converted to a serializers.ValidationError.
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#L331-L333
Potential issue: The exception handling in `update_data_source()` and
`update_anomaly_detection()` was narrowed to catch a specific list of exceptions, but
this list omits `DetectorException`. This exception is raised by the
`_fetch_related_models()` function when related data for a detector is missing or
inconsistent. Because `DetectorException` is no longer caught, it will propagate up as
an unhandled exception, causing the API endpoint to return a 500 error instead of a
graceful `serializers.ValidationError` as it did previously. This can occur during
normal API operations if detector data is in an inconsistent state.
Description
When investigating an issue, it was difficult to determine the source of the issue because we hit the exception block and lost which of the ValidationError was bubbling up.
This will just allow the more specific validation error to bubble up and reformat them to all be ValidationErrors that can be handled in the API.
The code still catches the exception and retriggers it to ensure the control flow is consistent and the validation error types are correct.