Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
77c4e9c
Proof of Concept: Document analytics events via YARD (LG-5590)
zachmargolis Mar 2, 2022
78a13b0
optional
zachmargolis Mar 2, 2022
1085051
Migrate ACCOUNT_DELETE_SUBMITTED
zachmargolis Mar 2, 2022
cbe37c2
Use Makefile to generate the JSON
zachmargolis Mar 2, 2022
dd75757
Migrate ACCOUNT_DELETE_VISITED
zachmargolis Mar 2, 2022
a28fc64
fixup ACCOUNT_DELETE_SUBMITTED
zachmargolis Mar 2, 2022
0b54508
Makefile help line
zachmargolis Mar 2, 2022
2d4f2c6
Add separate lint and output commands, factor into a class
zachmargolis Mar 2, 2022
b9b5740
rubocops
zachmargolis Mar 2, 2022
778eeaa
Update analytics lint command and fix analytics lint errors
zachmargolis Mar 2, 2022
5cfb746
Bundle exec stuff
zachmargolis Mar 3, 2022
75dd072
Add changelog
zachmargolis Mar 3, 2022
d562814
Add additional code coverage
zachmargolis Mar 3, 2022
cdeca15
Switch to dasherized /api/analytics-events endpoint, add to CORS allo…
zachmargolis Mar 3, 2022
7cb3951
Merge remote-tracking branch 'origin/main' into margolis-analytics-ev…
zachmargolis Mar 3, 2022
13da56d
gitignore
zachmargolis Mar 3, 2022
531b95d
eslint ignore doc dir
zachmargolis Mar 3, 2022
24ba2bc
build analytics_events as part of artifact compilation
zachmargolis Mar 3, 2022
1c8a239
Move JSON building to build-post-config
zachmargolis Mar 3, 2022
7377ed0
Serve the analytics events data from Rails so that the CORS middlewar…
zachmargolis Mar 3, 2022
e872498
Clean up documentation
zachmargolis Mar 4, 2022
fb3a914
Clean up documentation
zachmargolis Mar 4, 2022
698e2c7
Migrate ACCOUNT_DELETION event
zachmargolis Mar 4, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ node_modules
public
vendor
coverage
doc
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ Vagrantfile
/kitchen/cookbooks
/log/*
/postgres-data
/public/api
/public/system
/public/user_flows
/pwned_passwords/*
Expand All @@ -69,6 +70,7 @@ Vagrantfile
/.tmp/
/.vscode/
/vendor/bundle
.yardoc

package-lock.json

Expand Down
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ gem 'view_component', '~> 2.49.1', require: 'view_component/engine'
gem 'webauthn', '~> 2.1'
gem 'xmldsig', '~> 0.6'
gem 'xmlenc', '~> 0.7', '>= 0.7.1'
gem 'yard'

# This version of the zxcvbn gem matches the data and behavior of the zxcvbn NPM package.
# It should not be updated without verifying that the behavior still matches JS version 4.4.2.
Expand Down
1 change: 1 addition & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,7 @@ DEPENDENCIES
webmock
xmldsig (~> 0.6)
xmlenc (~> 0.7, >= 0.7.1)
yard
zonebie
zxcvbn (= 0.1.7)

Expand Down
40 changes: 39 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,31 @@ PORT ?= 3000
GZIP_COMMAND ?= gzip
ARTIFACT_DESTINATION_FILE ?= ./tmp/idp.tar.gz

.PHONY: brakeman check check_asset_strings docker_setup fast_setup fast_test help lint lint_country_dialing_codes lint_erb lint_optimized_assets lint_yaml lintfix normalize_yaml optimize_assets optimize_svg run run setup test update_pinpoint_supported_countries build_artifact
.PHONY: \
analytics_events \
brakeman \
build_artifact \
check \
check_asset_strings \
docker_setup \
fast_setup \
fast_test \
help \
lint \
lint_analytics_events \
lint_country_dialing_codes \
lint_erb \
lint_optimized_assets \
lint_yaml \
lintfix \
normalize_yaml \
optimize_assets \
optimize_svg \
run \
run \
setup \
test \
update_pinpoint_supported_countries

help: ## Show this help
@echo "--- Help ---"
Expand All @@ -35,6 +59,8 @@ lint: ## Runs all lint tests
make lint_erb
@echo "--- rubocop ---"
bundle exec rubocop --parallel
@echo "--- analytics_events ---"
make lint_analytics_events
@echo "--- brakeman ---"
bundle exec brakeman
@echo "--- zeitwerk check ---"
Expand Down Expand Up @@ -158,3 +184,15 @@ build_artifact $(ARTIFACT_DESTINATION_FILE): ## Builds zipped tar file artifact
--exclude='./vendor/ruby' \
--exclude='./config/application.yml' \
-cf - "." | "$(GZIP_COMMAND)" > $(ARTIFACT_DESTINATION_FILE)

analytics_events: public/api/_analytics-events.json ## Generates a JSON file that documents analytics events for events.log

lint_analytics_events: .yardoc # Checks that all methods on AnalyticsEvents are documented
bundle exec ruby lib/analytics_events_documenter.rb --check $<

public/api/_analytics-events.json: .yardoc .yardoc/objects/root.dat
mkdir -p public/api
bundle exec ruby lib/analytics_events_documenter.rb --json $< > $@

.yardoc .yardoc/objects/root.dat: app/services/analytics_events.rb
bundle exec yard doc --type-tag identity.idp.event_name:"Event Name" --db $@ -- $<
2 changes: 1 addition & 1 deletion app/controllers/account_reset/cancel_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def create
private

def track_event(result)
analytics.track_event(Analytics::ACCOUNT_RESET, result.to_h)
analytics.account_reset(**result.to_h)
end

def handle_valid_token
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/account_reset/delete_account_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ def show
render :show and return unless token

result = AccountReset::ValidateGrantedToken.new(token).call
analytics.track_event(Analytics::ACCOUNT_RESET, result.to_h)
analytics.account_reset(**result.to_h)

if result.success?
handle_valid_token
Expand All @@ -16,7 +16,7 @@ def show
def delete
granted_token = session.delete(:granted_token)
result = AccountReset::DeleteAccount.new(granted_token).call
analytics.track_event(Analytics::ACCOUNT_RESET, result.to_h.except(:email))
analytics.account_reset(**result.to_h.except(:email))

if result.success?
handle_successful_deletion(result)
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/account_reset/request_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def create

def create_account_reset_request
response = AccountReset::CreateRequest.new(current_user).call
analytics.track_event(Analytics::ACCOUNT_RESET, response.to_h.merge(analytics_attributes))
analytics.account_reset(**response.to_h.merge(analytics_attributes))
end

def confirm_two_factor_enabled
Expand Down
18 changes: 18 additions & 0 deletions app/controllers/analytics_events_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Serve a static file from Rails so that the CORS middleware can add the correct headers
class AnalyticsEventsController < ApplicationController
prepend_before_action :skip_session_load
prepend_before_action :skip_session_expiration
skip_before_action :disable_caching

JSON_FILE = Rails.root.join('public', 'api', '_analytics-events.json')

def index
if File.exist?(JSON_FILE)
expires_in 15.minutes, public: true

send_file JSON_FILE, type: 'application/json', disposition: 'inline'
else
render_not_found
end
end
end
6 changes: 3 additions & 3 deletions app/controllers/users/delete_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@ class DeleteController < ApplicationController
before_action :confirm_current_password, only: [:delete]

def show
analytics.track_event(Analytics::ACCOUNT_DELETE_VISITED)
analytics.account_delete_visited
end

def delete
send_push_notifications
delete_user
sign_out
flash[:success] = t('devise.registrations.destroyed')
analytics.track_event(Analytics::ACCOUNT_DELETE_SUBMITTED, success: true)
analytics.account_delete_submitted(success: true)
redirect_to root_url
end

Expand All @@ -29,7 +29,7 @@ def confirm_current_password
return if valid_password?

flash[:error] = t('idv.errors.incorrect_password')
analytics.track_event(Analytics::ACCOUNT_DELETE_SUBMITTED, success: false)
analytics.account_delete_submitted(success: false)
render :show
end

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def destroy

def track_account_deletion_event
properties = ParseControllerFromReferer.new(request.referer).call
analytics.track_event(Analytics::ACCOUNT_DELETION, properties)
analytics.account_deletion(**properties)
end

def destroy_user
Expand Down
3 changes: 1 addition & 2 deletions app/services/account_reset/grant_requests_and_send_emails.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ def perform(now)
notifications_sent += 1 if grant_request_and_send_email(arr)
end

analytics.track_event(
Analytics::ACCOUNT_RESET,
analytics.account_reset(
event: :notifications,
count: notifications_sent,
)
Expand Down
6 changes: 2 additions & 4 deletions app/services/analytics.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# frozen_string_literal: true

class Analytics
include AnalyticsEvents

def initialize(user:, request:, sp:, session:, ahoy: nil)
@user = user
@request = request
Expand Down Expand Up @@ -120,10 +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
Contributor

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

ACCOUNT_DELETE_SUBMITTED = 'Account Delete submitted'
ACCOUNT_DELETE_VISITED = 'Account Delete visited'
ACCOUNT_DELETION = 'Account Deletion Requested'
ACCOUNT_RESET_VISIT = 'Account deletion and reset visited'
ACCOUNT_VISIT = 'Account Page Visited'
ADD_EMAIL = 'Add Email: Email Submitted'
Expand Down
78 changes: 78 additions & 0 deletions app/services/analytics_events.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
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

# @param [Boolean] success
# @param ["cancel", "delete", "cancel token validation", "granted token validation",
# "notifications"] event
# @param [String] message_id from AWS Pinpoint API
# @param [String] request_id from AWS Pinpoint API
# @param [Boolean] sms_phone
# @param [Boolean] totp does the user have an authentication app as a 2FA option?
# @param [Boolean] piv_cac does the user have PIV/CAC as a 2FA option?
# @param [Integer] count number of notifications sent
# @param [Hash] errors
# @param [Hash] error_details
# @param [String] user_id
# @param [Integer] account_age_in_days
# @param [Hash] mfa_method_counts
# @param [Integer] email_addresses number of email addresses the user has
# Tracks events related to a user requesting to delete their account during the sign in process
# (because they have no other means to sign in).
def account_reset(
success: nil,
event: nil,
message_id: nil,
piv_cac: nil,
request_id: nil,
sms_phone: nil,
totp: nil,
count: nil,
errors: nil,
user_id: nil,
account_age_in_days: nil,
mfa_method_counts: nil,
pii_like_keypaths: nil,
error_details: nil,
email_addresses: nil
)
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

event: event,
message_id: message_id,
piv_cac: piv_cac,
request_id: request_id,
sms_phone: sms_phone,
totp: totp,
count: count,
errors: errors,
user_id: user_id,
account_age_in_days: account_age_in_days,
mfa_method_counts: mfa_method_counts,
pii_like_keypaths: pii_like_keypaths,
error_details: error_details,
email_addresses: email_addresses,
}.compact,
)
end

# @identity.idp.event_name Account Delete submitted
# @param [Boolean] success
# When a user submits a form to delete their account
def account_delete_submitted(success:)
track_event('Account Delete submitted', success: success)
end

# @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

track_event('Account Delete visited')
end

# @identity.idp.event_name Account Deletion Requested
# @param [String] request_came_from the controller/action the request came from
# When a user deletes their account
def account_deletion(request_came_from:)
track_event('Account Deletion Requested', request_came_from: request_came_from)
end
end
1 change: 1 addition & 0 deletions config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ class Application < Rails::Application
end

origins allowed_origins
resource '/api/analytics-events', headers: :any, methods: [:get]
resource '/api/country-support', headers: :any, methods: [:get]
end
end
Expand Down
1 change: 1 addition & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
Rails.application.routes.draw do
# Non i18n routes. Alphabetically sorted.
get '/api/analytics-events' => 'analytics_events#index'
get '/api/country-support' => 'country_support#index'
get '/api/health' => 'health/health#index'
get '/api/health/database' => 'health/database#index'
Expand Down
2 changes: 2 additions & 0 deletions deploy/build-post-config
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ bundle exec rake assets:precompile

bundle exec bin/copy_robots_file

make analytics_events

set +x

echo "deploy/build-post-config finished"
Loading