-
Notifications
You must be signed in to change notification settings - Fork 122
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
base: main
Are you sure you want to change the base?
Conversation
26e6419
to
0ae15be
Compare
app/forms/webauthn_visit_form.rb
Outdated
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) |
There was a problem hiding this comment.
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 } }
There was a problem hiding this comment.
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.
f3da80a
to
39f0030
Compare
There was a problem hiding this 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
changelog: Internal, Analytics, Omit errors from analytics when error_details available
39f0030
to
88d6b20
Compare
🛠 Summary of changes
Updates
FormResponse
to assignerrors
tonil
whenerror_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, anderror_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:
serialize_error_details_only
option altogether (make baseline behavior)errors
altogether, as opposed to assigning asnil
FormResponse
with non-ActiveRecord::Errors
errors
type📜 Testing Plan
Verify build passes.
Validate using
make watch_events
that event logs which would previously includeerrors
no longer include the property, but still include equivalenterror_details
.