Skip to content

Restructure analytics error_details as hash#7454

Closed
aduth wants to merge 5 commits intomainfrom
aduth-form-response-error-details-hash
Closed

Restructure analytics error_details as hash#7454
aduth wants to merge 5 commits intomainfrom
aduth-form-response-error-details-hash

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Dec 8, 2022

🛠 Summary of changes

Revises the structure of FormResponse error_details handling to serialize a FormResponse error details as a hash, in order to improve querying supports for analytics logging, since CloudWatch does not support querying within an array field value.

Related Slack thread: https://gsa-tts.slack.com/archives/C0NGESUN5/p1670357411732859

# Before (Does not work!)
filter 'user_otp_expired' in properties.event_properties.error_details.code

# After
filter properties.event_properties.error_details.code.user_otp_expired = true
# Or: filter ispresent(properties.event_properties.error_details.code.user_otp_expired)

Based on a search of dashboard Terraform code, we do not currently reference this field, so a breaking change should be alright.

Draft while I work through any lingering specs which need to be updated to new format.

📜 Testing Plan

  • Specs pass

aduth added 5 commits December 8, 2022 11:27
changelog: Internal, Analytics, Adjust format of analytics logging to improve querying support
Revert to pluck + index_with static value
@aduth
Copy link
Contributor Author

aduth commented Dec 13, 2022

To check: IRS attempts API uses error_details as the result of its parse_failure_reason:

def parse_failure_reason(result)
return result.to_h[:error_details] || result.errors.presence
end

Unclear if we can make breaking changes to the structure of this result, or if we need to flatten it back out for Attempts API.

@aduth
Copy link
Contributor Author

aduth commented Dec 13, 2022

To check: IRS attempts API uses error_details as the result of its parse_failure_reason:

A cursory review of related documents suggests that it probably is something we've shared as being the structure of the hash, so we may need to keep it as a flattened array here (cc @18F/identity-agnes).

@aduth
Copy link
Contributor Author

aduth commented Dec 20, 2022

I'm still interested in the idea here, though not feeling quite as impassioned / suffering from the status quo to see it through now, nor to sort out the challenge concerning Attempts API backwards-compatibility.

Could be resumed later if there's a renewed desire for it.

@aduth aduth closed this Dec 20, 2022
@aduth aduth deleted the aduth-form-response-error-details-hash branch December 20, 2022 18:10
@zachmargolis
Copy link
Contributor

I'm still interested in the idea here, though not feeling quite as impassioned / suffering from the status quo to see it through now, nor to sort out the challenge concerning Attempts API backwards-compatibility.

Could be resumed later if there's a renewed desire for it.

I'm a fan of this! We could maybe have a limited error_details_hash_old_style for the Attempts API?

@aduth aduth restored the aduth-form-response-error-details-hash branch November 9, 2023 15:42
@aduth
Copy link
Contributor Author

aduth commented Nov 9, 2023

Resumed at #9572

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants