-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[LH Doc Upload Migration] Fix Lighthouse Upload Failure Metrics Logging #19466
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -207,55 +207,50 @@ | |
end | ||
end | ||
|
||
context 'when we get a non-200 response from Lighthouse' do | ||
let(:error_response_body) do | ||
# From vcr_cassettes/lighthouse/benefits_claims/documents/lighthouse_form_526_document_upload_400.yml | ||
{ | ||
'errors' => [ | ||
{ | ||
'detail' => 'Something broke', | ||
'status' => 400, | ||
'title' => 'Bad Request', | ||
'instance' => Faker::Internet.uuid | ||
} | ||
] | ||
} | ||
end | ||
|
||
# See lib/lighthouse/service_exception.rb | ||
context 'when a Lighthouse::ServiceException error is raised' do | ||
before do | ||
# Skip upload attempt logging | ||
allow(provider).to receive(:log_upload_attempt) | ||
end | ||
|
||
allow(BenefitsDocuments::Form526::UploadSupplementalDocumentService).to receive(:call) | ||
.with(file_body, lighthouse_document) | ||
.and_return(faraday_response) | ||
RSpec.shared_examples 'log Lighthouse response exception' do |exception_class| | ||
it 'increments a StatsD failure metric, logs the error metadata and re-raises the error' do | ||
error_info = { title: 'error title', detail: 'error message' } | ||
exception = exception_class.new(error_info) | ||
|
||
allow(faraday_response).to receive(:body).and_return(error_response_body) | ||
end | ||
allow(BenefitsDocuments::Form526::UploadSupplementalDocumentService).to receive(:call) | ||
.and_raise(exception) | ||
|
||
it 'logs to the Rails logger' do | ||
expect(Rails.logger).to receive(:error).with( | ||
'LighthouseSupplementalDocumentUploadProvider upload failed', | ||
{ | ||
class: 'LighthouseSupplementalDocumentUploadProvider', | ||
submitted_claim_id: submission.submitted_claim_id, | ||
submission_id: submission.id, | ||
user_uuid: submission.user_uuid, | ||
va_document_type_code: va_document_type, | ||
primary_form: 'Form526', | ||
lighthouse_error_response: error_response_body | ||
} | ||
) | ||
expect(StatsD).to receive(:increment).with( | ||
'my_stats_metric_prefix.lighthouse_supplemental_document_upload_provider.upload_failure' | ||
) | ||
expect(Rails.logger).to receive(:error).with( | ||
'LighthouseSupplementalDocumentUploadProvider upload failed', | ||
{ | ||
class: 'LighthouseSupplementalDocumentUploadProvider', | ||
submitted_claim_id: submission.submitted_claim_id, | ||
submission_id: submission.id, | ||
user_uuid: submission.user_uuid, | ||
va_document_type_code: va_document_type, | ||
primary_form: 'Form526', | ||
error_info: error_info.to_s | ||
} | ||
) | ||
|
||
provider.submit_upload_document(lighthouse_document, file_body) | ||
expect { provider.submit_upload_document(lighthouse_document, file_body) }.to raise_error(exception) | ||
end | ||
end | ||
|
||
it 'increments a StatsD metric' do | ||
expect(StatsD).to receive(:increment).with( | ||
'my_stats_metric_prefix.lighthouse_supplemental_document_upload_provider.upload_failure' | ||
) | ||
describe 'service exceptions' do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't yet been able to get all of these tests passing with the Rails logger logging, some work and some do not which is confusing since they are all just custom exception classes. The metrics increment and re-raising the error behavior does seem to work for all of them. We may not want to use the Rails logger for capturing exceptions, as this information is already logged elsewhere (you can see the custom exceptions showing up in the "catch all" widget on our migration dashboard such as this one) The purpose of logging the exception here would just be to have a unified system of logging events in the upload providers, as this logging matches how we log attempts and successes, in addition to the metrics, which are more helpful for aggregating data There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One option to just get this working is to just increment the metric and not worry about a redundant call to the rails logger |
||
error_values = Lighthouse::ServiceException::ERROR_MAP.values | ||
|
||
provider.submit_upload_document(lighthouse_document, file_body) | ||
error_values.each do |exception| | ||
it_behaves_like 'log Lighthouse response exception', exception | ||
end | ||
|
||
it_behaves_like 'log Lighthouse response exception', Common::Exceptions::Timeout | ||
it_behaves_like 'log Lighthouse response exception', Common::Exceptions::ServiceError | ||
end | ||
end | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename hand success response