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

Proof of Concept: Document analytics events via YARD (LG-5590) #6014

Merged
merged 23 commits into from
Mar 4, 2022

Conversation

zachmargolis
Copy link
Contributor

@zachmargolis zachmargolis commented Mar 2, 2022

  • 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

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:

  • Decide if we think this amount of extra typing is acceptable
  • Decide how we want to display this

Example JSON output:

> ./scripts/document-analytics-events       
{
  "events": [
    {
      "event_name": "Account Reset",
      "description": "Tracks events related to a user requesting to delete their account during the sign in process\n(because they have no other means to sign in).",
      "attributes": [
        {
          "name": "success",
          "types": [
            "Boolean"
          ],
          "description": null
        },
        {
          "name": "event",
          "types": [
            "\"cancel\"",
            "\"delete\"",
            "\"cancel token validation\"",
            "\"granted token validation\"",
            ":notifications"
          ],
          "description": null
        },
        {
          "name": "message_id",
          "types": [
            "String"
          ],
          "description": "Request ID from AWS Pinpoint API"
        },
...

- 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'
Copy link
Contributor Author

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

Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor Author

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:

  1. communicating to the rest of the team that new events should go to analytics_events.rb
  2. Writing tickets to help migrate all the old events

track_event(
'Account Reset',
{
success: success,
Copy link
Contributor Author

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
Copy link
Contributor Author

@zachmargolis zachmargolis Mar 2, 2022

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

@zachmargolis zachmargolis force-pushed the margolis-analytics-events branch from 2bd4744 to 1085051 Compare March 2, 2022 18:05
@zachmargolis zachmargolis marked this pull request as ready for review March 2, 2022 23:23
changelog: Internal, Documentation, Add documentation for analytics events in events.log
Comment on lines +100 to +104
{
event_name: method_object.tag(EVENT_NAME_TAG)&.text,
description: method_object.docstring.presence,
attributes: attributes,
}
Copy link
Contributor Author

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

Copy link
Contributor Author

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
Copy link
Contributor Author

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

Copy link
Contributor

@theabrad theabrad left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@gsa-manish gsa-manish left a comment

Choose a reason for hiding this comment

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

lgtm

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.

4 participants