chore(eco): Silences errors from 400 responses when creating Jira issues #100078
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #100078 +/- ##
===========================================
+ Coverage 79.86% 81.19% +1.32%
===========================================
Files 8576 8582 +6
Lines 380778 381037 +259
Branches 23963 23963
===========================================
+ Hits 304127 309391 +5264
+ Misses 76297 71292 -5005
Partials 354 354 |
| if isinstance(exc, ApiInvalidRequestError): | ||
| if exc.json: | ||
| error_fields = self.error_fields_from_json(exc.json) | ||
| if error_fields is not None: | ||
| raise IntegrationFormError(error_fields).with_traceback(sys.exc_info()[2]) | ||
| raise IntegrationConfigurationError(exc.text) from exc | ||
| super().raise_error(exc) |
There was a problem hiding this comment.
Are we trying to target errors like this - Sentry Issue ? In that case, I don't think our ApiError logic is knowledgeable enough to translate to ApiInvalidRequest error yet?
Also also, for error_fields_from_json, I think we need to get rid of the regex matching or add some more(?) since it only matches on the Team Field currently. Or we could use error_message_from_json 🤔
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| # We need to reraise ProcessingDeadlineExceeded for workflow engine to | ||
| # monitor and potentially retry this action. | ||
| raise | ||
| except RETRIABLE_EXCEPTIONS as e: |
There was a problem hiding this comment.
nit
| except RETRIABLE_EXCEPTIONS as e: | |
| except RETRYABLE_EXCEPTIONS as e: |
There was a problem hiding this comment.
Also, should these simply be reraised? I thought taskbroker would automatically retry unless the exception is in an exclude list
There was a problem hiding this comment.
Yeah that's fair. I'll change this. Remembering our office hours discussion on this: isn't this part of our contract with workflow engine? Depending on how this task gets executed, we may need to raise as this specific exception to make it obvious to top-level handling that we want an explicit retry.
| configuration problems, like required fields missing. | ||
| """ | ||
| if isinstance(exc, ApiInvalidRequestError): | ||
| if exc.json: |
There was a problem hiding this comment.
Is it reasonable to do more checking of the message like checking if "is required" is in the response from Jira (like adding to CUSTOM_ERROR_MESSAGE_MATCHERS)? I slightly worry about overreporting halts when they should be failures
There was a problem hiding this comment.
Yeah, this seems overly permissive for now. 🤔 I can start by logging out the invalid response bodies, then creating a list to check against. The problem is, I've seen so many distinct error messages, including some that have HTML bodies, that it'll be impossible to string match against all of them.
GabeVillalobos
force-pushed
the
gv/silence-jira-invalid-request-errors
branch
from
b91f554 to
7f58d2d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| logger.warning( | ||
| "sentry.jira.raise_error.api_invalid_request_error", | ||
| extra={ |
There was a problem hiding this comment.
We could have exc.json but it didn't match the existing error fields. Should that be logged too?
There was a problem hiding this comment.
Oh this is a really good point. This could help us cut down on the XML/HTML junk we get back from Jira. I'll do that instead, ty!
| responses.POST, | ||
| "https://example.atlassian.net/rest/api/2/issue", | ||
| status=400, | ||
| body=json.dumps({"error": "Jira had an oopsie"}), |
| invoke_future_with_error_handling(self.event_data, self.mock_callback, self.mock_futures) | ||
|
|
||
| self.mock_callback.assert_called_once_with(self.group_event, self.mock_futures) |
There was a problem hiding this comment.
TIL you can pass a Mock into a function and assert it got called 🤯
| # We need to reraise ProcessingDeadlineExceeded for workflow engine to | ||
| # monitor and potentially retry this action. | ||
| raise | ||
| except RETRIABLE_EXCEPTIONS as e: |
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues:
|
Our current ticket creation errors are noisy. The majority of them are the result of strict configuration on the Jira site rejecting the ticket creation. These messages tend to vary greatly in content, and are generally unhelpful in triaging failing ticket creations.
This PR overrides the default error handling, and reraises the issues as
IntegrationConfigurationErrors, which records them as halts instead of failures.