-
Notifications
You must be signed in to change notification settings - Fork 120
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
Proof of Concept: Document analytics events via YARD (LG-5590) #6014
Conversation
- Adds AnalyticsEvents module where we can put explicitly typed events - Example script that uses YARD to parse these and output JSON that we could use to power more accessible documentation
@@ -120,7 +122,6 @@ def browser_attributes | |||
end | |||
|
|||
# rubocop:disable Layout/LineLength | |||
ACCOUNT_RESET = 'Account Reset' |
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.
I picked this one because it was first on the list. It turned out to be super verbose to document because it is essentially 5 different events (it has its own event
key 🤦 )
I will take some time and try migrating other events to see how much typing they result in
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.
Maybe this is a good thing as it uncovers a smell where this should have separate event names for those events?
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.
Created a ticket to track splitting it up: https://cm-jira.usa.gov/browse/LG-5910
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.
Does splitting it up mean splitting up each event (like this one would be for Account Reset) into a separate document, or does this happen currently? I think with the number of events we have, the documentation can get rather large.
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.
Not 100% sure what you're asking? The goal is to have a lot of documentation, so we can better explain what the events in our logs mean, so when we query events, we query them confidently.
But yes, this analytics_events.rb
file would be expected to get pretty big
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.
So the file analytics_events.rb will have all of the events going forward not just account reset such as all the events in analytics.rb?
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.
Yeah next steps would be:
- communicating to the rest of the team that new events should go to
analytics_events.rb
- Writing tickets to help migrate all the old events
track_event( | ||
'Account Reset', | ||
{ | ||
success: success, |
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.
One of the cons of this approach is each key needs to be written 3 times in this file (YARD, param, and then when it's passed over to track_event
), which is very error-prone
Thanks to #6012, we can be sure that if we forget to pass a param, we will error because it's unused
@@ -0,0 +1,55 @@ | |||
module AnalyticsEvents | |||
# @identity.idp.event_name Account Reset |
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.
👀 custom tag! we can make a form of the script that errors when this is absent as a lint, maybe even try to make sure that the string is present in the method body
2bd4744
to
1085051
Compare
changelog: Internal, Documentation, Add documentation for analytics events in events.log
{ | ||
event_name: method_object.tag(EVENT_NAME_TAG)&.text, | ||
description: method_object.docstring.presence, | ||
attributes: attributes, | ||
} |
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.
💡 One idea is if we added line number + filename + commit SHA, the docs could link to the source in GitHub
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.
I gave this a shot, I don't think it added much to the docs, but we can always revisit and add it later if needed
|
||
# @identity.idp.event_name Account Delete visited | ||
# When a user visits the page to delete their account | ||
def account_delete_visited |
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.
Turns out there are a lot of these events with no params, which would make for a super easy migration if we wanted
git grep -E "analytics.track_event\(Analytics::[A-Z_]+\)"
Full list of events without params
git grep -E "analytics.track_event\(Analytics::[A-Z_]+\)" | cut -d'(' -f 2 | cut -d')' -f 1 | sort | uniq
Analytics::ACCOUNT_VISIT
Analytics::AUTHENTICATION_CONFIRMATION
Analytics::AUTHENTICATION_CONFIRMATION_CONTINUE
Analytics::AUTHENTICATION_CONFIRMATION_RESET
Analytics::BACKUP_CODE_CREATED
Analytics::BANNED_USER_REDIRECT
Analytics::BANNED_USER_VISITED
Analytics::BROKEN_PERSONAL_KEY_REGENERATED
Analytics::EMAIL_LANGUAGE_VISITED
Analytics::EVENTS_VISIT
Analytics::FORGET_ALL_BROWSERS_SUBMITTED
Analytics::FORGET_ALL_BROWSERS_VISITED
Analytics::IDV_ADDRESS_VISIT
Analytics::IDV_COME_BACK_LATER_VISIT
Analytics::IDV_FORGOT_PASSWORD
Analytics::IDV_FORGOT_PASSWORD_CONFIRMED
Analytics::IDV_GPO_ADDRESS_LETTER_REQUESTED
Analytics::IDV_GPO_VERIFICATION_VISITED
Analytics::IDV_INTRO_VISIT
Analytics::IDV_PERSONAL_KEY_SUBMITTED
Analytics::IDV_PERSONAL_KEY_VISITED
Analytics::IDV_PHONE_CONFIRMATION_OTP_RATE_LIMIT_ATTEMPTS
Analytics::IDV_PHONE_CONFIRMATION_OTP_RATE_LIMIT_LOCKED_OUT
Analytics::IDV_PHONE_CONFIRMATION_OTP_RATE_LIMIT_SENDS
Analytics::IDV_PHONE_CONFIRMATION_OTP_VISIT
Analytics::IDV_PHONE_OTP_DELIVERY_SELECTION_VISIT
Analytics::IDV_PHONE_RECORD_VISIT
Analytics::IDV_REVIEW_COMPLETE
Analytics::IDV_REVIEW_VISIT
Analytics::MULTI_FACTOR_AUTH_ENTER_TOTP_VISIT
Analytics::MULTI_FACTOR_AUTH_MAX_ATTEMPTS
Analytics::MULTI_FACTOR_AUTH_MAX_SENDS
Analytics::MULTI_FACTOR_AUTH_OPTION_LIST_VISIT
Analytics::PASSWORD_MAX_ATTEMPTS
Analytics::PASSWORD_RESET_VISIT
Analytics::PENDING_ACCOUNT_RESET_CANCELLED
Analytics::PENDING_ACCOUNT_RESET_VISITED
Analytics::PERSONAL_KEY_REACTIVATION
Analytics::PERSONAL_KEY_REACTIVATION_SIGN_IN
Analytics::PERSONAL_KEY_REACTIVATION_VISITED
Analytics::PHONE_CHANGE_VIEWED
Analytics::PROFILE_PERSONAL_KEY_CREATE
Analytics::PROFILE_PERSONAL_KEY_VISIT
Analytics::PROOFING_ADDRESS_RESULT_MISSING
Analytics::PROOFING_DOCUMENT_RESULT_MISSING
Analytics::PROOFING_RESOLUTION_RESULT_MISSING
Analytics::RULES_OF_USE_VISIT
Analytics::SESSION_KEPT_ALIVE
Analytics::SESSION_TIMED_OUT
Analytics::SESSION_TOTAL_DURATION_TIMEOUT
Analytics::SP_HANDOFF_BOUNCED_DETECTED
Analytics::SP_HANDOFF_BOUNCED_VISIT
Analytics::SP_INACTIVE_VISIT
Analytics::TOTP_USER_DISABLED
Analytics::USER_REGISTRATION_ENTER_EMAIL_VISIT
Analytics::USER_REGISTRATION_PHONE_SETUP_VISIT
Analytics::USER_REGISTRATION_PIV_CAC_DISABLED
Analytics::USER_REGISTRATION_PIV_CAC_SETUP_VISIT
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
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
that we could use to power more accessible documentation
Previous work: manual documentation https://github.com/18F/identity-analytics-etl/blob/b42d5ca75bce809e00dcd1aa1bbbedf96339aca7/docs/Managing-analytics-events-in-IDP.md#user-registration-agency-handoff-complete
Next Steps:
Example JSON output: