Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 15 additions & 0 deletions app/jobs/outdated_ab_test_assignment_cleanup_job.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# frozen_string_literal: true

class OutdatedAbTestAssignmentCleanupJob < ApplicationJob
queue_as :low

def perform
AbTestAssignment.where.not(experiment: active_experiments).destroy_all
end

private

def active_experiments
AbTests.all.values.map(&:experiment_name)
end
end
9 changes: 9 additions & 0 deletions app/models/ab_test_assignment.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# frozen_string_literal: true

class AbTestAssignment < ApplicationRecord
class << self
def bucket(**)
find_by(**)&.bucket&.to_sym
end
end
end
1 change: 1 addition & 0 deletions app/services/analytics.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ def ab_test_attributes(event)
session:,
user:,
user_session:,
persisted_read_only: true,
)
if !bucket.blank?
obj[test_id.downcase] = {
Expand Down
9 changes: 2 additions & 7 deletions config/initializers/ab_tests.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,18 +65,13 @@ def self.all

RECAPTCHA_SIGN_IN = AbTest.new(
experiment_name: 'reCAPTCHA at Sign-In',
persist: true,
should_log: [
'Email and Password Authentication',
'IdV: doc auth verify proofing results',
'reCAPTCHA verify result received',
:idv_enter_password_submitted,
].to_set,
buckets: { sign_in_recaptcha: IdentityConfig.store.sign_in_recaptcha_percent_tested },
) do |user:, user_session:, **|
if user_session&.[](:captcha_validation_performed_at_sign_in) == false
nil
else
user&.uuid
end
end.freeze
).freeze
end
5 changes: 5 additions & 0 deletions config/initializers/job_configurations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,11 @@
cron: cron_every_monday_2am,
args: -> { [Time.zone.yesterday.end_of_day] },
},
# Outdated A/B test assignment clean-up
outdated_ab_test_assignment_cleanup: {
class: 'OutdatedAbTestAssignmentCleanupJob',
cron: cron_every_monday,
},
}.compact
end
# rubocop:enable Metrics/BlockLength
Expand Down
10 changes: 10 additions & 0 deletions db/primary_migrate/20240905125027_create_ab_test_assignments.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
class CreateAbTestAssignments < ActiveRecord::Migration[7.1]
def change
create_table :ab_test_assignments do |t|
t.string :experiment, null: false
t.string :discriminator, null: false
t.string :bucket, null: false
t.index [:experiment, :discriminator], unique: true
end
end
end
12 changes: 10 additions & 2 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,20 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.1].define(version: 2024_08_28_182041) do
ActiveRecord::Schema[7.1].define(version: 2024_09_05_125027) do
# These are extensions that must be enabled in order to support this database
enable_extension "citext"
enable_extension "pg_stat_statements"
enable_extension "pgcrypto"
enable_extension "plpgsql"

create_table "ab_test_assignments", force: :cascade do |t|
t.string "experiment", null: false
t.string "discriminator", null: false
t.string "bucket", null: false
t.index ["experiment", "discriminator"], name: "index_ab_test_assignments_on_experiment_and_discriminator", unique: true
end

create_table "account_reset_requests", force: :cascade do |t|
t.integer "user_id", null: false
t.datetime "requested_at", precision: nil
Expand Down Expand Up @@ -664,7 +672,7 @@
add_foreign_key "iaa_gtcs", "partner_accounts"
add_foreign_key "iaa_orders", "iaa_gtcs"
add_foreign_key "in_person_enrollments", "profiles"
add_foreign_key "in_person_enrollments", "service_providers", column: "issuer", primary_key: "issuer", validate: false
add_foreign_key "in_person_enrollments", "service_providers", column: "issuer", primary_key: "issuer"
add_foreign_key "in_person_enrollments", "users"
add_foreign_key "integration_usages", "iaa_orders"
add_foreign_key "integration_usages", "integrations"
Expand Down
34 changes: 28 additions & 6 deletions lib/ab_test.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
# frozen_string_literal: true

class AbTest
attr_reader :buckets, :experiment_name, :default_bucket, :should_log
attr_reader :buckets, :experiment_name, :default_bucket, :should_log, :persist
alias_method :experiment, :experiment_name

MAX_SHA = (16 ** 64) - 1

Expand All @@ -18,13 +19,15 @@ def initialize(
buckets: {},
should_log: nil,
default_bucket: :default,
persist: false,
&discriminator
)
@buckets = buckets
@discriminator = discriminator
@experiment_name = experiment_name
@default_bucket = default_bucket
@should_log = should_log
@persist = persist
raise 'invalid bucket data structure' unless valid_bucket_data_structure?
ensure_numeric_percentages
raise 'bucket percentages exceed 100' unless within_100_percent?
Expand All @@ -36,25 +39,44 @@ def initialize(
# @params [Hash] session
# @param [User] user
# @param [Hash] user_session
def bucket(request:, service_provider:, session:, user:, user_session:)
return nil if no_percentages?

# @param [Boolean] persisted_read_only Avoid new bucket assignment if test is configured to be
# persisted but there is no persisted value.
def bucket(
request:,
service_provider:,
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

)
discriminator = resolve_discriminator(
request:, service_provider:, session:, user:,
user_session:
)
return nil if discriminator.blank?

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?


user_value = percent(discriminator)

bucket = @default_bucket

min = 0
buckets.keys.each do |key|
max = min + buckets[key]
return key if user_value > min && user_value <= max
if user_value > min && user_value <= max
bucket = key
break
end
min = max
end

@default_bucket
AbTestAssignment.create(experiment:, discriminator:, bucket:) if persist

bucket
end

def include_in_analytics_event?(event_name)
Expand Down