IN 1035 - log unhandled exceptions #41
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Why these changes are being introduced: Sentry recently reported, 'Unexpected exception, aborting DSpace Submission Service processing', which is a prepared logging statement as part of Submission.submit(). This suggests that a totally unhandled exception was encountered during submit work. While somewhat helpful, this does not give us insight into the underlying exception that caused it in Sentry or Cloudwatch. How this addresses that need: * Changes logger.error to logger.exception to log the underlying Exception to Sentry with a full traceback * Though we may still need work to rollback actions taken, or manage the SQS queue based on an unhandled exception, this change will provide more information in these situations Side effects of this change: * Unhandled exceptions will be fully logged to Sentry Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1035
jonavellecuerdo
approved these changes
Aug 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose and background context
A recent Sentry event indicates that an unhandled exception was encountered during
Submission.submit(), but does not provide any details about the underlying exception encountered.The error handling in
submit()is pretty intricate, where sub-methods called will handle exceptions and raise custom exceptions likeItemCreateError,ItemPostError, etc. When these are encountered, there is logic in place to handle them.But when an exception occurs that is not gracefully handled by this method, formerly only the following message was logged, but without any traceback or information about the actual exception encountered:
This PR changes this from
logger.errortologger.exceptionwhich will also log the associated, underlying exception to Cloudwatch and Sentry. In this way, we can understand what happened in this situation.As noted in the associated Jira ticket, this is in support of some future work to better rollback any operations performed in Dspace and/or manage SQS queues. Work will continue for this, but this intermediate PR at least begins to log unhandled exceptions in a way that supports retroactive debugging and understanding.
How can a reviewer manually see the effects of these changes?
The newly created test
test_submit_dspace_unknown_api_error_logs_exception_and_raises_errordemonstrates this behavior.Instead of a request to DSpace returning an HTTP response that itself indicates any errors, the request to collection handle
"0000/collection04"is mocked to throw arequests.exceptions.RequestExceptionimmediately. Because there is not response, there is nothing to parse and report.This test demonstrates that while we still don't have entirely graceful handling of unhandled exceptions during
Submission.submit()(e.g. managing the SQS queue depending on what happened), we at least have logged the actual, underlying exception in a way that allows us to better react to the error.Includes new or updated dependencies?
NO
Changes expectations for external applications?
NO
What are the relevant tickets?
Developer
Code Reviewer
(not just this pull request message)