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

Rubocop linter for "enhanced" analytics events #10264

Conversation

matthinz
Copy link
Member

🛠 Summary of changes

I wrote this linter originally as part of #10230, but that PR was getting very complicated so I pulled it out here.

Background

Idv::AnalyticsEventsEnhancer will "enhance" certain analytics logging methods by automatically filling in more arguments. Currently, that is limited to proofing_components, but that will be expanding in the future (e.g. LG-12617).

We want these "enhanced" arguments to be present in method signatures so that:

  • We aren't over-relying on **extra
  • Our analytics methods are documented properly

What this PR does

This PR adds a Rubocop Cop that looks at analytics_events.rb and verifies all of these "enhanced" arguments are:

  1. Included in the signatures of the relevant methods
  2. Passed through to track_event properly
  3. Have a corresponding @param documentation line

There is also some autocorrecting built in, since I was originally thinking I'd be making lots of changes to analytics_events.rb.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Remove a method from Idv::AnalyticsEventsEnhancer::IGNORED_METHODS
  • Run rubocop: rubocop --cache false app/services/analytics_events.rb
  • Verify you get some errors about proofing_components not being present in method signature
  • Fix the errors: rubocop --cache false -a app/services/analytics_events.rb
  • Verify that all the issues are fixed

@matthinz matthinz requested a review from a team March 18, 2024 21:40
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

very nice! big fan of leveraging automation for this

lib/linters/enhanced_idv_events_linter.rb Outdated Show resolved Hide resolved
spec/lib/linters/enhanced_idv_events_linter_spec.rb Outdated Show resolved Hide resolved
spec/lib/linters/enhanced_idv_events_linter_spec.rb Outdated Show resolved Hide resolved
@aduth
Copy link
Member

aduth commented Mar 19, 2024

Is this something that #9946 could cover? We currently hide most errors from that with the allowed_extra_analytics, but I'd assume if we remove that from IdV specs we could uncover this?

The main downside I see to #9946 is that it requires that something actually calls the analytics methods, but I'd expect that's just a basic test coverage problem?

@matthinz
Copy link
Member Author

@aduth Yeah, that is a good point. I originally had in mind making large-scale changes to analytics_events.rb as part of another PR. In my mind the Rubocop stuff is nice because of the autocorrect functionality--you can use it to automate those kind of code modifications.

I'm ok not actually landing this right now, and bringing it back if we want to investigate strategies for managing analytics_events.rb (like eventually getting rid of **extra or something).

Base automatically changed from matthinz/12657-update-analytics-events-enchanter to main March 20, 2024 17:00
@matthinz matthinz force-pushed the matthinz/analytics-events-enhancer-linter branch from a330ea7 to 42935e7 Compare March 20, 2024 23:13
@matthinz matthinz changed the base branch from main to matthinz/12617-profile-logging March 20, 2024 23:13
@matthinz matthinz force-pushed the matthinz/analytics-events-enhancer-linter branch 2 times, most recently from 47b1b31 to 286d294 Compare March 20, 2024 23:52
@matthinz matthinz force-pushed the matthinz/12617-profile-logging branch from 22b984a to 68996f8 Compare March 22, 2024 20:09
Summarize the history of a user's profiles in IdV analytics events

changelog: Internal, Identity verification, Include profile metadata in analytics logs
Just return the levels, and use .presence to ensure falsey values are nil so they're compacted properly
It's a lot of data and we want to keep average event payload size low.
@matthinz matthinz force-pushed the matthinz/12617-profile-logging branch from 68996f8 to a773b46 Compare March 22, 2024 20:12
@matthinz matthinz force-pushed the matthinz/analytics-events-enhancer-linter branch from 286d294 to 51370b7 Compare March 22, 2024 20:28
Adding the following to covered methods:

- active_profile_idv_level
- pending_profile_idv_level
- profile_history

Also making sure proofing_components is documented appropriately.
@matthinz matthinz force-pushed the matthinz/12617-profile-logging branch from 0f82087 to a0a6db3 Compare March 22, 2024 21:06
@matthinz matthinz force-pushed the matthinz/analytics-events-enhancer-linter branch from 51370b7 to 20b215e Compare March 22, 2024 21:08
Sprinkle all these new args around a little.
…nalyticsEvents

Check each method on AnalyticsEvents that is "enhanced" by AnalyticsEventsEnhancer:

1. Includes the enhancements in its args list
2. Passes those to track_event
Look for "@param" documentation for e.g. proofing_components.

Autocorrect to a "TODO"
Delegate entirely Idv::AnalyticsEventsEnhancer rather than duplicating arg lists
@matthinz matthinz force-pushed the matthinz/analytics-events-enhancer-linter branch from 20b215e to dabec7b Compare March 22, 2024 23:52
@matthinz matthinz force-pushed the matthinz/12617-profile-logging branch 2 times, most recently from 6b22eee to 98b0c2b Compare March 28, 2024 21:31
@matthinz
Copy link
Member Author

Closing this for now. I did find some of this useful for working on #10270 but may not be worth landing on main and committing long-term

@matthinz matthinz closed this Mar 28, 2024
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.

3 participants