Omit errors from analytics when error_details available#11810
Conversation
26e6419 to
0ae15be
Compare
app/forms/webauthn_visit_form.rb
Outdated
There was a problem hiding this comment.
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.
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
zachmargolis
left a comment
There was a problem hiding this comment.
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
39f0030 to
88d6b20
Compare
changelog: Internal, Analytics, Omit errors from analytics when error_details available
88d6b20 to
be41f96
Compare
🛠 Summary of changes
Updates
FormResponseto assignerrorstonilwhenerror_detailsis 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
errorsis not valuable for log querying, anderror_detailsshould be preferred.The changes here leave
FormResponseis a somewhat fragmented state, but it does so intentionally to lessen the amount of changes.Future pull requests will:
serialize_error_details_onlyoption altogether (make baseline behavior)errorsaltogether, as opposed to assigning asnilFormResponsewith non-ActiveRecord::Errorserrorstype📜 Testing Plan
Verify build passes.
Validate using
make watch_eventsthat event logs which would previously includeerrorsno longer include the property, but still include equivalenterror_details.