ref(test-alerts): Adds exception filtering for IntegrationErrors to be shown in the UI: #78653
Conversation
github-actions
bot
added
the
Scope: Backend
| @@ -110,7 +111,10 @@ def execute_future_on_test_event( | |||
| logger.warning( | |||
| "%s.test_alert.unexpected_exception", callback_name, exc_info=True | |||
| ) | |||
| break | |||
| error_id = sentry_sdk.capture_exception(exc) | |||
There was a problem hiding this comment.
I realize this will add a bit more noise to our sentry issues in the short term, so I may need to split this out into a separate exception type, or perhaps just an info level log? Either way, we probably want to add custom handling for these exceptions to reduce customer confusion when they see this response.
There was a problem hiding this comment.
TIL this function has a return value :O
There was a problem hiding this comment.
Yep! We use this in our base API exception handler and surface error IDs there as well, which is the only reason I know about this 😅
Line 334 in 4efffec
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #78653 +/- ##
===========================================
+ Coverage 64.67% 78.24% +13.56%
===========================================
Files 7100 7105 +5
Lines 312820 313067 +247
Branches 51092 51121 +29
===========================================
+ Hits 202331 244960 +42629
+ Misses 103837 61740 -42097
+ Partials 6652 6367 -285 |
| @@ -110,7 +111,10 @@ def execute_future_on_test_event( | |||
| logger.warning( | |||
| "%s.test_alert.unexpected_exception", callback_name, exc_info=True | |||
| ) | |||
| break | |||
| error_id = sentry_sdk.capture_exception(exc) | |||
There was a problem hiding this comment.
TIL this function has a return value :O
| @@ -110,7 +111,10 @@ def execute_future_on_test_event( | |||
| logger.warning( | |||
| "%s.test_alert.unexpected_exception", callback_name, exc_info=True | |||
| ) | |||
| break | |||
There was a problem hiding this comment.
should we break early so we don't have to stack error messages in the UI?
There was a problem hiding this comment.
Can do. When I was testing this with multiple integrations, it already breaks on the first error and halts execution, but it wouldn't hurt to do this just in case.
| break | ||
| error_id = sentry_sdk.capture_exception(exc) | ||
| action_exceptions.append( | ||
| f"An unexpected error occurred. Error ID: '{error_id}'" |
There was a problem hiding this comment.
We kind of know at this point that this error is related to integration. Can we say something like Jira returned an unexpected error? Basically sending people's attention to their Jira instance, and maybe they can figure out the issue without filing support.
There was a problem hiding this comment.
If we encounter an error here, it may not necessarily be integration related. I've also noticed that having multiple integrations in the same alert rule also makes things confusing, so I'll try to filter this down a bit more in a follow-up PR that includes better error messaging for both of these cases.
Uses this to narrow the criteria for an IntegrationFormErrors being raised for erorr propagation safety.
GabeVillalobos
force-pushed
the
gv/better-differentiate-between-api-error-types
branch
from
1464eaa to
9289202
Compare
GabeVillalobos
deleted the
gv/better-differentiate-between-api-error-types
branch
ApiInvalidRequestErrortype, to better differentiate between 400 and unhandled non-400 responses from downstream APIs.IntegrationFormErrorswhen a known 400 status response is received by requests to an integration's APIWhen an unhandled error type is provided, we will show it + the event ID associated with it for easier triage:
Note: This does not fix the brunt of the
IntegrationFormErrorsentry issues we're getting for Jira, which I'll address in a follow-up PR, but this does give me more confidence to GA the UI error reporting for test notifications.