Skip to content

LG-15560: Enhance A/B tests to support configurable max participants#11853

Merged
aduth merged 8 commits intomainfrom
aduth-lg-15560-ab-test-user-count
Feb 10, 2025
Merged

LG-15560: Enhance A/B tests to support configurable max participants#11853
aduth merged 8 commits intomainfrom
aduth-lg-15560-ab-test-user-count

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Feb 7, 2025

🎫 Ticket

LG-15560

🛠 Summary of changes

Enhances the A/B test tooling to support the ability to disable the test automatically after a configurable number of users have been opted-in to the test.

As part of this, it adds the ability to persist A/B test assignments to the database, building upon an earlier proof-of-concept explored in #11202.

📜 Testing Plan

None of the current tests are planned to use this new feature, but we can validate with manual edits to an existing test in your local copy of the repository:

diff --git a/config/initializers/ab_tests.rb b/config/initializers/ab_tests.rb
index 7b6cc53223..63df620a9a 100644
--- a/config/initializers/ab_tests.rb
+++ b/config/initializers/ab_tests.rb
@@ -74,2 +74,4 @@ module AbTests
     buckets: { sign_in_recaptcha: IdentityConfig.store.sign_in_recaptcha_percent_tested },
+    persist: true,
+    max_participants: 1,
   ) do |user:, user_session:, **|

The reCAPTCHA test is already configured to capture 100% of attempts in A/B test in local development (sign_in_recaptcha_percent_tested).

Verify that a failed reCAPTCHA simulated result will only trigger the "Security check failed" screen until max participants (1) is reached:

  1. In a private browsing window, go to http://localhost:3000
  2. Enter an email address and password for an account (whether it exists or not), e.g. nobody@example.com
  3. Change simulated "reCAPTCHA score" to a failing score (e.g. 0.1)
  4. Click "Sign in"
  5. Observe "Security check failed" screen
  6. Click "Back" link
  7. Enter an email address and password for another account (whether it exists or not), e.g. nobody2@example.com
  8. Change simulated "reCAPTCHA score" to a failing score (e.g. 0.1)
  9. Click "Sign in"
  10. Observe that you either see the MFA screen or the "email or password you've entered is wrong" screen, depending if you entered valid credentials. In other words, reCAPTCHA testing was not applied and you are not redirected to the security check failed screen.

You can also check that there's a single database entry for the one assigned participant in rails console:

> AbTestAssignment.all
=> [#<AbTestAssignment id: 1, experiment: "reCAPTCHA at Sign-In", discriminator: "...", bucket: "sign_in_recaptcha">]

@aduth aduth requested a review from a team February 7, 2025 16:18
@aduth
Copy link
Contributor Author

aduth commented Feb 7, 2025

Reverting to draft to look into enhancing A/B tests reports with participant count status, per discussion at #11853 (comment).

@aduth aduth marked this pull request as draft February 7, 2025 17:47
Copy link
Contributor

Choose a reason for hiding this comment

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

just thinking out loud, is there like a vacuum or something we should run if this deletes a significant number of records? that also might be a risky thing to automate (and run a big locking job at an arbitrary time) so probs better to leave out for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, those are good points. I do think it's "safer" to leave out for now, but this feels like the sort of thing that if we defer to our future selves is at high risk of never being addressed.

One thought could be to make the job only delete up to X records, maybe run more frequently than once a week, so it's more of an incremental cleanup?

Copy link
Contributor

@zachmargolis zachmargolis Feb 7, 2025

Choose a reason for hiding this comment

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

  • Yeah a .limit(1000) would help keep this manageable too
  • Or doing a big load + many slices of deletes, like:
  • Also switching to delete_all does it all in one SQL statement, destroy will load the AR callback for each object and is generally slower
Suggested change
AbTestAssignment.where.not(experiment: configured_experiments).destroy_all
ids_to_delete = AbTestAssignment.where.not(experiment: configured_experiments).pluck(:id)
ids_to_delete.each_slice(1000) do |batch|
AbTestAssignment.where(id: batch).delete_all
end

Copy link
Contributor Author

@aduth aduth Feb 7, 2025

Choose a reason for hiding this comment

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

Good call on destroy_all vs. delete_all. Sort-of on the fence about doing it in batches, vs. just deleting X amount and then relying on the next job run to delete the next X amount.

Happened to stumble across #in_batches, which defaults to 1000 and could be nice for simplifying the code a bit.

Suggested change
AbTestAssignment.where.not(experiment: configured_experiments).destroy_all
AbTestAssignment.where.not(experiment: configured_experiments).in_batches.delete_all

Copy link
Contributor

Choose a reason for hiding this comment

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

oooo I didn't know about chaining to the batch enumerator, that is nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revisiting the initial concern of "risky thing to automate", I opted to take this a different direction in 3846448 and create a Rake task that we can manually run. Let me know if that feels safer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the switch to small batches makes it safe enough to run automatically, but it would be extra safe to manually run it the first few times to confirm, before scheduling it

@aduth aduth marked this pull request as ready for review February 7, 2025 19:29
aduth and others added 8 commits February 10, 2025 12:02
changelog: Internal, A/B Tests, Add persistence option for A/B tests
changelog: Internal, A/B Tests, Enhance A/B tests to support configurable max participants
Future support for pulling participant count, having access to configured test max_participants
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
See: #11853 (comment)

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@aduth aduth force-pushed the aduth-lg-15560-ab-test-user-count branch from 3846448 to 6181ebe Compare February 10, 2025 17:04
@aduth aduth merged commit 4a29ca2 into main Feb 10, 2025
2 checks passed
@aduth aduth deleted the aduth-lg-15560-ab-test-user-count branch February 10, 2025 18:21
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