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

Log session answers to rollbar #3478

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rjlynch
Copy link
Contributor

@rjlynch rjlynch commented Dec 30, 2024

Some of the errors in rollbar are tricky to debug as we don't know what
answers the claimant provided. This commit adds a mechanism for flagging
attributes as PII and updates the ClaimsController to include the non
pii attributes in the Rollbar scope such that they are available in
Rollbar.

Here are two errors in rollbar showing the additional params sent over https://app.rollbar.com/a/dfe-digital/fix/item/dfe-claims/1530#occurrences

We couldn't reuse the config/analytics_blocklist.yml list for scrubbing pi as journey sessions store all answers in the answers db column and we just block the whole column. I think tacking on pii: true to the attribute definition isn't too much overhead and may end up being useful in other contexts.

@rjlynch rjlynch added the deploy Deploy a review app for this PR label Dec 30, 2024
@rjlynch rjlynch force-pushed the CAPT-1861/log_session_answers branch from 98870de to c642bc0 Compare December 30, 2024 13:23
@rjlynch rjlynch requested a review from slorek December 30, 2024 13:26
@rjlynch rjlynch marked this pull request as ready for review December 30, 2024 13:27
Some of the errors in rollbar are tricky to debug as we don't know what
answers the claimant provided. This commit adds a mechanism for flagging
attributes as PII and updates the ClaimsController to include the non
pii attributes in the Rollbar scope such that they are available in
Rollbar.
@rjlynch rjlynch force-pushed the CAPT-1861/log_session_answers branch from c642bc0 to 4e32654 Compare December 30, 2024 13:30
end
end

def attributes_with_pii_redacted
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add some basic coverage for this method?

Copy link
Contributor

@slorek slorek left a comment

Choose a reason for hiding this comment

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

There is already some config here https://github.com/DFE-Digital/claim-additional-payments-for-teaching/blob/master/app/models/claim.rb#L20 from when the Claim object was built during the journey. It might be that this is now redundant or could be considered as part of this change. I think the list of filtered attributes should be managed in one place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy Deploy a review app for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants