API-27098: Improved Error Handling and Observability of Duplicate UUID Response from Central Mail (Appeals and Benefits Intake) #12906
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
kristen-brown
added
Lighthouse
kristen-brown
commented
Jun 6, 2023
| @@ -30,6 +30,8 @@ class PdfSubmitJob | |||
| # 503 Database Offline || SOLR Service Offline || Intake API is undergoing maintenance | |||
| RETRYABLE_EMMS_RESP_STATUS_CODES = [401, 429, 500, 503].freeze | |||
|
|
|||
| STATSD_DUPLICATE_UUID_KEY = 'api.appeals.document_upload.duplicate_uuid' | |||
There was a problem hiding this comment.
The STATSD_UPLOAD_FAIL_KEY for Appeals is set to api.appeals.document_upload.fail, so I was trying to keep things consistent here.
| elsif response.status == 400 && response.body.match?(DUPLICATE_UUID_REGEX) | ||
| StatsD.increment(STATSD_DUPLICATE_UUID_KEY) | ||
| Rails.logger.warn("#{appeal.class.to_s.gsub('::', ' ')}: Duplicate UUID submitted to Central Mail", | ||
| 'uuid' => appeal.id) |
There was a problem hiding this comment.
Here I tried to keep to the logging pattern started in log_upload_error (in the same class).
| @@ -13,6 +13,8 @@ class UploadProcessor | |||
| include Sidekiq::Worker | |||
| include VBADocuments::UploadValidations | |||
|
|
|||
| STATSD_DUPLICATE_UUID_KEY = 'api.vba.document_upload.duplicate_uuid' | |||
There was a problem hiding this comment.
The STATSD_UPLOAD_FAIL_KEY for Benefits Intake is set to api.vba.document_upload.fail, so I was trying to keep things consistent here.
va-vfs-bot
temporarily deployed
to
API-27098-handle-duplicate-uuid-response/main/main
va-vsp-bot
requested a deployment
to
API-27098-handle-duplicate-uuid-response/main/main
mathisto
reviewed
Jun 7, 2023
| @@ -258,6 +258,36 @@ | |||
| end | |||
| end | |||
|
|
|||
| context 'with a duplicate UUID response from Central Mail' do | |||
| before do | |||
There was a problem hiding this comment.
I'm legit intimidated by your rspec-fu
kristen-brown
changed the title
kristen-brown
changed the title
ryan-mcneil
pushed a commit
that referenced
this pull request
Dec 11, 2023
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.
Summary
Some recent observations after changing our error handling slightly in the Appeals API have led us to the understanding that a status code of
400with a response body containing "Document already uploaded with uuid," when returned from the Central Mail/uploadendpoint, indicates that there was an earlier submission (with that same UUID) that was successful, and so the submission record itself can be marked as successful.This new understanding has led to the changes in this PR, which counts "duplicate UUID" responses as always successful in both the Appeals API and Benefits Intake API and adds a StatsD counter and Rails logs for the "duplicate UUID" scenario so that we can more easily monitor how often it occurs. The StatsD counter will be added to the Team Banana Peels Datadog dashboard.
Related issue(s)
API-27098
Testing done
The
AppealsApi::PdfSubmitJobandVBADocuments::UploadProcessorare well-covered by existing unit tests. I have also added unit tests specifically for the "duplicate UUID" scenario for both classes.Screenshots
None
What areas of the site does it impact?
This PR impacts the Appeals API and the Benefits Intake API, both of which my team own.
Acceptance criteria
Requested Feedback
No specific feedback requested on this PR.