diff --git a/app/jobs/reports/ab_tests_report.rb b/app/jobs/reports/ab_tests_report.rb index e96618e72cf..21adb6d6109 100644 --- a/app/jobs/reports/ab_tests_report.rb +++ b/app/jobs/reports/ab_tests_report.rb @@ -15,40 +15,39 @@ def initialize(report_date = nil, *args, **kwargs) def perform(report_date) @report_date = report_date - report_configs.each do |config| - tables_report(config).deliver_now + reported_ab_tests.each do |ab_test| + tables_report(ab_test).deliver_now end end - def tables_report(config) - experiment_name = config.experiment_name + def tables_report(ab_test) + experiment_name = ab_test.experiment_name subject = "A/B Tests Report - #{experiment_name} - #{report_date}" - reports = ab_tests_report(config).as_emailable_reports + report = ab_tests_report(ab_test) ReportMailer.tables_report( - email: config.email, + email: ab_test.report.email, subject:, - message: subject, - reports:, + message: [subject, report.participants_message].compact, + reports: report.as_emailable_reports, attachment_format: :csv, ) end - def ab_tests_report(config) + def ab_tests_report(ab_test) Reporting::AbTestsReport.new( - queries: config.queries, + ab_test:, time_range: report_date.yesterday..report_date, ) end private - def report_configs + def reported_ab_tests AbTests .all .values .select { |ab_test| ab_test.report&.email&.present? && ab_test.active? } - .map(&:report) end end end diff --git a/app/mailers/report_mailer.rb b/app/mailers/report_mailer.rb index 89bc2306e8f..4706555fc41 100644 --- a/app/mailers/report_mailer.rb +++ b/app/mailers/report_mailer.rb @@ -26,6 +26,7 @@ def warn_error(email:, error:, env: Rails.env) # @param [String] email # @param [String] subject # @param [String] env name of current deploy environment + # @param [String, String[]] message Message(s) to use as email prelude # @param [:csv,:xlsx] attachment_format # @param [Array] reports # an array of tables (which are arrays of rows (arrays of strings)) diff --git a/app/models/ab_test_assignment.rb b/app/models/ab_test_assignment.rb new file mode 100644 index 00000000000..52fcda0aa55 --- /dev/null +++ b/app/models/ab_test_assignment.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AbTestAssignment < ApplicationRecord + class << self + def bucket(**) + find_by(**)&.bucket&.to_sym + end + end +end diff --git a/app/services/analytics.rb b/app/services/analytics.rb index 214e467df0a..e62bfa0b574 100644 --- a/app/services/analytics.rb +++ b/app/services/analytics.rb @@ -88,6 +88,7 @@ def ab_test_attributes(event) session:, user:, user_session:, + persisted_read_only: true, ) if !bucket.blank? obj[test_id.downcase] = { diff --git a/app/views/report_mailer/tables_report.html.erb b/app/views/report_mailer/tables_report.html.erb index 8bb9fc0df7f..5aeaaa39422 100644 --- a/app/views/report_mailer/tables_report.html.erb +++ b/app/views/report_mailer/tables_report.html.erb @@ -11,7 +11,9 @@ ) %>
-<%= @message %>
+<% [*@message].each do |message| %> +

<%= message %>

+<% end %> <% @reports.each do |report| %> <% header, *rows = report.table %> <%# Allow nil title if a subtitle is set %> diff --git a/db/primary_migrate/20250207144037_create_ab_test_assignments.rb b/db/primary_migrate/20250207144037_create_ab_test_assignments.rb new file mode 100644 index 00000000000..adb6f78a698 --- /dev/null +++ b/db/primary_migrate/20250207144037_create_ab_test_assignments.rb @@ -0,0 +1,10 @@ +class CreateAbTestAssignments < ActiveRecord::Migration[8.0] + def change + create_table :ab_test_assignments do |t| + t.string :experiment, null: false, comment: 'sensitive=false' + t.string :discriminator, null: false, comment: 'sensitive=false' + t.string :bucket, null: false, comment: 'sensitive=false' + t.index [:experiment, :discriminator], unique: true, using: :btree + end + end +end diff --git a/db/schema.rb b/db/schema.rb index a5f6df18193..f9da1bb4248 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,12 +10,19 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2025_01_06_232958) do +ActiveRecord::Schema[8.0].define(version: 2025_02_07_144037) do # These are extensions that must be enabled in order to support this database enable_extension "citext" enable_extension "pg_catalog.plpgsql" enable_extension "pg_stat_statements" + create_table "ab_test_assignments", force: :cascade do |t| + t.string "experiment", null: false, comment: "sensitive=false" + t.string "discriminator", null: false, comment: "sensitive=false" + t.string "bucket", null: false, comment: "sensitive=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, comment: "sensitive=false" t.datetime "requested_at", precision: nil, comment: "sensitive=false" diff --git a/lib/ab_test.rb b/lib/ab_test.rb index abee04e0a13..e8aaa7d0c39 100644 --- a/lib/ab_test.rb +++ b/lib/ab_test.rb @@ -3,13 +3,22 @@ class AbTest include ::NewRelic::Agent::MethodTracer - attr_reader :buckets, :experiment_name, :default_bucket, :should_log, :report + attr_reader :buckets, + :experiment_name, + :default_bucket, + :should_log, + :report, + :persist, + :max_participants + + alias_method :experiment, :experiment_name + alias_method :persist?, :persist MAX_SHA = (16 ** 64) - 1 ReportQueryConfig = Struct.new(:title, :query, :row_labels, keyword_init: true).freeze - ReportConfig = Struct.new(:experiment_name, :email, :queries, keyword_init: true) do + ReportConfig = Struct.new(:email, :queries, keyword_init: true) do def initialize(queries: [], **) super self.queries.map!(&ReportQueryConfig.method(:new)) @@ -19,6 +28,9 @@ def initialize(queries: [], **) # @param [Regexp,#include?,nil] should_log A list of analytics event names for which the A/B test # bucket assignment should be logged, or a regular expression pattern which is tested against an # analytics event name when an event is being logged. + # @param [Hash] report Report mailer configuration. + # @param [Boolean] persist Whether the test assignment should be persisted to the database. + # @param [Integer] max_participants The maximum number of participants allowed in the test. # @yieldparam [ActionDispatch::Request] request # @yieldparam [String,nil] service_provider Issuer string for the service provider associated with # the current session. @@ -30,6 +42,8 @@ def initialize( should_log: nil, default_bucket: :default, report: nil, + persist: false, + max_participants: Float::INFINITY, &discriminator ) @buckets = buckets @@ -37,7 +51,10 @@ def initialize( @experiment_name = experiment_name @default_bucket = default_bucket @should_log = should_log - @report = ReportConfig.new(experiment_name:, **report.to_h) if report + @report = ReportConfig.new(**report.to_h) if report + @persist = persist + raise 'max_participants requires persist to be true' if max_participants.finite? && !persist? + @max_participants = max_participants raise 'invalid bucket data structure' unless valid_bucket_data_structure? ensure_numeric_percentages raise 'bucket percentages exceed 100' unless within_100_percent? @@ -50,7 +67,16 @@ def initialize( # @param [Hash] session # @param [User] user # @param [Hash] user_session - def bucket(request:, service_provider:, session:, user:, user_session:) + # @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 + ) return nil if !active? discriminator = resolve_discriminator( @@ -59,16 +85,28 @@ def bucket(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 maxed? + 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) @@ -87,8 +125,18 @@ def active? @active end + def participants_count + return if !persist? + AbTestAssignment.where(experiment:).count + end + private + def maxed? + return false if !persist? || !max_participants.finite? + participants_count >= max_participants + end + def resolve_discriminator(user:, **) if @discriminator @discriminator.call(user:, **) diff --git a/lib/reporting/ab_tests_report.rb b/lib/reporting/ab_tests_report.rb index 8dd45408c69..2a6144eb7ad 100644 --- a/lib/reporting/ab_tests_report.rb +++ b/lib/reporting/ab_tests_report.rb @@ -5,15 +5,15 @@ module Reporting class AbTestsReport - attr_reader :queries, :time_range + attr_reader :ab_test, :time_range - # @param [Array] queries + # @param [AbTest] ab_test # @param [Range