Skip to content

Proof-of-Concept: Add persistence option for A/B tests#11202

Closed
aduth wants to merge 3 commits intomainfrom
aduth-persist-ab-test
Closed

Proof-of-Concept: Add persistence option for A/B tests#11202
aduth wants to merge 3 commits intomainfrom
aduth-persist-ab-test

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Sep 5, 2024

🛠 Summary of changes

Adds support for an A/B test to persist its bucket assignment to the database, useful in scenarios where relevant metrics of a test may be logged at a different point in time than the enrollment itself.

Discussed in 2024-09-04 Engineering Huddle

General use-cases:

  • Avoid consider a user as part of a bucket if they did not complete some earlier action while the test was active (e.g. account creation)
  • Allow logging to continue even after the test itself has ended (percentages reset to 0)

Specific examples:

  • If a user is subjected to reCAPTCHA assessment at sign-in, we want to log this test assignment if the user is later suspended for fraudulent activity
  • If we allow some users to register Face or Touch Unlock as an MFA method on desktop computers, we want to know how successful these users are when they later authenticate with that method. We also want the ability to stop enrolling new users into the test while continuing to monitor success rates for those who had been previously enrolled.

📜 Testing Plan

Verify that bucket assignment for reCAPTCHA at sign-in sticks, even after the test is configured to not enroll new users.

  1. In a Private Browsing tab, go to http://localhost:3000
  2. Sign in
  3. Edit config/application.yml to effectively disable test:
development:
  sign_in_recaptcha_percent_tested: 0
  1. Restart server
  2. Run make watch_events in a separate terminal process
  3. Repeat Steps 1 & 2
  4. Observe "Email and Password Authentication" event includes ab_tests.recaptcha_sign_in.bucket value (sign_in_recaptcha)

session:,
user:,
user_session:,
persisted_read_only: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe clearer name?

Suggested change
persisted_read_only: false
assign_if_unpersisted: true

Copy link
Contributor

Choose a reason for hiding this comment

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

some more ideas

skip_assignment_for_unpersisted: false
skip_assignment_for_new: false

persisted_value = AbTestAssignment.bucket(experiment:, discriminator:) if persist
return persisted_value if persisted_value || (persist && persisted_read_only)

return nil if no_percentages?
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't we want to return @default_bucket here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting question. This specific code existed previously, I had just rearranged it. But I'm not sure if nil is meant to be some meaningful value separate from @default_bucket. @matthinz Any thoughts here, since you recently worked on this?

@aduth
Copy link
Contributor Author

aduth commented Sep 10, 2024

I'm going to close this proof-of-concept for now, since we don't have a present need for this at the moment, and #11210 created incompatibilities with persisting the discriminator for reCAPTCHA at sign-in. I still think this'll have value with future A/B tests, but we can reopen once there's an actual need for it.

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.

2 participants