LG-6317 - Retire registration_logs#7107
Merged
zachmargolis merged 9 commits intomainfrom Oct 12, 2022
Merged
Conversation
zachmargolis
reviewed
Oct 7, 2022
This was referenced Oct 12, 2022
jskinne3
pushed a commit
that referenced
this pull request
Oct 12, 2022
* changelog: Internal, Analytics, Only populate registration_logs for final step
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎫 Ticket
LG-6317
🛠 Summary of changes
registration_logs was originally created as a way to track the registration funnel. (ie how long on average does it take for a user to complete registration?) With this addition #6194 the funnel can now be tracked completely in the analytics. However registration_logs also gives us a way to calculate fully registered users, that is users that have completed entering a password and successfully adding an mfa. Historically 2 mfas were required and that restriction was lifted and now the moment they successfully configure an mfa they are considered fully registered.
The full registered user counts are required by several reports including one that restricts the count to a certain range. Therefore an indexed registered_at time is still needed. However all the fields in registration_logs other than user_id can now be removed. We could put registered_at on the users table but this would not only be a drain on the table when migrating over but would also add more indexes to users which is not recommended. The decoupled 1:1 solution is superior and since the registered_at field will never be null (can be enforced in subsequent prs and data migration to remove rows not fully registered) a simple count of all the records can be done to get the fully registered count including fast semi-realtime methods of getting table counts/estimates can be utilized in addition to using just the index to calculate the size.
The registration_logs will look like this after a subsequent culling pr:
currently testing if the fully registered counts can be entirely calculated from the mfa configurations tables.
Update - we can't because the user can delete an mfa and we used destroy (rather than disable) so the new registered_at will be the new mfa's addition time.
📜 Testing Plan
Provide a checklist of steps to confirm the changes.
👀 Screenshots
If relevant, include a screenshot or screen capture of the changes.
Before:
After:
🚀 Notes for Deployment
Include any special instructions for deployment.