-
Notifications
You must be signed in to change notification settings - Fork 1
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
Update logs for alerting #172
base: main
Are you sure you want to change the base?
Conversation
@laurenkrugen-navapbc Would these changes still be useful? The new logged data fields arent secrets or anything right? Curious how often these logs are used as well as more generally the ssas logs. Would it be helpful to add a common request identifier that could be used to tie the entire request into one easy to parse set of logs? |
I believe these would! I need to re-review these changes. The logged fields don't contain any sensitive data. We have been working to introduce a common identifier for logs -- I will send you the docs on those! SSAS doesn't contain the identifier in as many places as the API and we still need to make some updates to clean things up in both. |
LGTM, good to merge? |
@laurenkrugen-navapbc @carlpartridge Would we like to merge this? |
🎫 Ticket
n/a
🛠 Changes
Added back in logs that we use for informational alerting when credentials are revoked, reset, or registered.
ℹ️ Context for reviewers
As part of this PR, the logging that is used for credentials are revoked, reset, or registered changed, which stopped the alerts from firing. They have been added back in along with an inline comment.
Previous log statements can be found in this file on from an earlier commit: https://github.com/CMSgov/bcda-ssas-app/blob/55915297a5f690b434a3c5a32f575bd460ab193a/ssas/systems.go
✅ Acceptance Validation
tbd
🔒 Security Implications
If any security implications apply, add Jason Ashbaugh (GitHub username: StewGoin) as a reviewer and do not merge this PR without his approval.