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

add Sentry logging to narrow down failure mode #18336

Merged
merged 3 commits into from
Sep 10, 2024

Conversation

Mitch-A6
Copy link
Contributor

@Mitch-A6 Mitch-A6 commented Sep 5, 2024

Note: Delete the description statements, complete each step. None are optional, but can be justified as to why they cannot be completed as written. Provide known gaps to testing that may raise the risk of merging to production.

Summary

  • This work is behind a feature toggle (flipper): NO
  • Add exception-catching blocks around each of the steps of this action, logging any errors to Sentry
  • It's not clear why these 403 errors are coming up, and I hope a more specific understanding of their origin will help.
  • Health Enrollment 10-10EZ/CG team

Related issue(s)

Ticket: department-of-veterans-affairs/va.gov-team#92139

Testing done

I ran the app locally and confirmed that the upload process works; since this is a logging change only, there's no testing to add.

What areas of the site does it impact?

  • This only affects Sentry logging for File Upload failures

Acceptance criteria

  • No error nor warning in the console.
  • Events are being sent to the appropriate logging solution
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs

@Mitch-A6 Mitch-A6 requested review from a team as code owners September 5, 2024 19:55
@va-vfs-bot va-vfs-bot temporarily deployed to 92139-file-upload-403-investigation/main/main September 5, 2024 20:01 Inactive
@Mitch-A6 Mitch-A6 force-pushed the 92139-file-upload-403-investigation branch 2 times, most recently from 7b15cd2 to 19479d5 Compare September 5, 2024 20:06
@va-vfs-bot va-vfs-bot temporarily deployed to 92139-file-upload-403-investigation/main/main September 5, 2024 20:17 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 92139-file-upload-403-investigation/main/main September 6, 2024 14:03 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 92139-file-upload-403-investigation/main/main September 6, 2024 20:43 Inactive
@Mitch-A6 Mitch-A6 force-pushed the 92139-file-upload-403-investigation branch from abb0ed6 to 2c10348 Compare September 9, 2024 15:44
@va-vfs-bot va-vfs-bot temporarily deployed to 92139-file-upload-403-investigation/main/main September 9, 2024 16:10 Inactive
@Mitch-A6 Mitch-A6 force-pushed the 92139-file-upload-403-investigation branch from 2c10348 to 3b37549 Compare September 9, 2024 16:48
@va-vfs-bot va-vfs-bot temporarily deployed to 92139-file-upload-403-investigation/main/main September 9, 2024 18:07 Inactive
Copy link
Contributor

@JoshingYou1 JoshingYou1 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

Backend-review-group approval confirmed.

@Mitch-A6
Copy link
Contributor Author

Mitch-A6 commented Sep 10, 2024

deleted resolved comment on app/controllers/concerns/form_attachment_create.rb:22 to enable merging:

coope93 4 days ago
Looks like you don't need begin here as it is redundant. Once these are cleaned up it looks good to go 👍

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.

6 participants