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

Omit errors from analytics when error_details available #11810

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

aduth
Copy link
Member

@aduth aduth commented Jan 27, 2025

🛠 Summary of changes

Updates FormResponse to assign errors to nil when error_details is available, thereby resulting in them being omitted from analytics logs.

As with #11799, this serves to reduce the size (and cost) of our logging payloads. As discussed with the team previously and as outlined in the SBAR document "IdP logging approach for error details", the current usage of errors is not valuable for log querying, and error_details should be preferred.

The changes here leave FormResponse is a somewhat fragmented state, but it does so intentionally to lessen the amount of changes.

Future pull requests will:

  • Remove the serialize_error_details_only option altogether (make baseline behavior)
  • Avoid assigning errors altogether, as opposed to assigning as nil
  • Consider removing existing use of FormResponse with non-ActiveRecord::Errors errors type

📜 Testing Plan

Verify build passes.

Validate using make watch_events that event logs which would previously include errors no longer include the property, but still include equivalent error_details.

@aduth aduth changed the title Aduth form response errors nil Omit errors from analytics when error_details available Jan 27, 2025
@aduth aduth marked this pull request as draft January 27, 2025 22:31
@aduth aduth force-pushed the aduth-form-response-errors-nil branch from 26e6419 to 0ae15be Compare January 28, 2025 13:47
Comment on lines 44 to 46
errors.add error, translate_platform_authenticator_error(error),
type: :"#{translate_platform_authenticator_error(error).split('.').last}"
errors.add error, :invalid, message: translate_platform_authenticator_error(error)
else
errors.add error, translate_error(error), type: :"#{translate_error(error).split('.').last}"
errors.add error, :invalid, message: translate_error(error)
Copy link
Member Author

@aduth aduth Jan 28, 2025

Choose a reason for hiding this comment

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

Noting that this is a change in behavior, though the previous implementation would log error_details with string keys derived from the user-facing string, which would make querying much more difficult. As best I can tell, we don't have any existing dashboards or alarms which relied on the previous format.

Before:

error_details: { InvalidStateError: { :" Please try again or choose another authentication method" => true } }

After:

error_details: { InvalidStateError: { invalid: true } }

Copy link
Member Author

@aduth aduth Jan 28, 2025

Choose a reason for hiding this comment

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

Granting that this is still not a consistent format even after the changes, since keys of error_details should refer to the specific problematic field, not the error itself. This page is a bit unique in how it logs errors, but a better format might be something like...

error_details: { client_data_json: { invalid_state: true } }

client_data_json referring to one of the fields expected to be populated in the client-side form.

@aduth aduth force-pushed the aduth-form-response-errors-nil branch from f3da80a to 39f0030 Compare January 28, 2025 17:41
@aduth aduth marked this pull request as ready for review January 28, 2025 17:41
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM, this seems like a good change to make sure to communicate widely internally just in case people had been consuming these errors fields instead of error_details

app/services/form_response.rb Show resolved Hide resolved
@aduth aduth force-pushed the aduth-form-response-errors-nil branch from 39f0030 to 88d6b20 Compare January 31, 2025 16:11
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