Skip to content
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

93214: Fix error logging for DR SavedClaim status updater jobs #18642

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

dfong-adh
Copy link
Contributor

@dfong-adh dfong-adh commented Sep 26, 2024

Summary

This fixes an erroneous log message for non-error evidence statuses in the DR SavedClaim status updater jobs.

Related issue(s)

department-of-veterans-affairs/va.gov-team#93214
department-of-veterans-affairs/va.gov-team#90103
#18579

Testing done

Tested locally and spec tests

  • New code is covered by unit tests

What areas of the site does it impact?

This impacts Rails logs and the SavedClaim status updater jobs.

Acceptance criteria

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • No error nor warning in the console.
  • Events are being sent to the appropriate logging solution
  • Documentation has been updated (link to documentation)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
  • Feature/bug has a monitor built into Datadog (if applicable)
  • If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
  • I added a screenshot of the developed feature

# Increment StatsD and log only for new errors
unless old_uploads_metadata.dig(upload_id, 'status') == ERROR_STATUS
StatsD.increment("#{STATSD_KEY_PREFIX}_upload.status", tags: ["status:#{status}"])
if status == ERROR_STATUS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the conditional is different here where logging is happening upon error status whereas before in the unless condition, it was happening when the status was not ERROR_STATUS. Is this the intent, just want to check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct - we want it to log only if the status changed to ERROR_STATUS but not if it was already ERROR_STATUS.

Copy link
Contributor

@anniebtran anniebtran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes look good 👍 one question just for clarification — are we still incrementing when non-error statuses are unchanged? 🤔 i might have missed it in the code, but seems like we might also want to more broadly check that we only increment when statuses change?

@dfong-adh
Copy link
Contributor Author

changes look good 👍 one question just for clarification — are we still incrementing when non-error statuses are unchanged? 🤔 i might have missed it in the code, but seems like we might also want to more broadly check that we only increment when statuses change?

For now we are incrementing for every non-error status check. I think it is a good idea to change it so we are incrementing statsd values only when the status values change, but I'd prefer to make that a separate ticket + PR.

Copy link

github-actions bot commented Oct 1, 2024

Backend-review-group approval confirmed.

@dfong-adh dfong-adh merged commit e262b9c into master Oct 1, 2024
35 of 38 checks passed
@dfong-adh dfong-adh deleted the 93214-fix-dr-saved-claim-error-logging branch October 1, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants