From 31b83a4511aa4b565e75b5f982b3a17fc4c263fe Mon Sep 17 00:00:00 2001 From: Ben Halpern Date: Mon, 7 Dec 2020 13:52:54 -0500 Subject: [PATCH] Remove concept of "ongoing" field tests and clarify a/b test instructions/expectations. (#11734) * Remove concept of 'ongoing' field tests * Fix specs * Feeds spec * Fix spec * Fix style issues * More details on field tests --- app/controllers/stories/feeds_controller.rb | 4 -- app/models/comment.rb | 3 +- app/models/page_view.rb | 6 ++- app/models/reaction.rb | 3 +- .../prune_old_experiments_worker.rb | 13 ----- config/field_test.yml | 10 ---- config/schedule.yml | 3 -- docs/technical-overview/ab_testing.md | 52 ++++++++++++++++++- spec/factories/field_test_memberships.rb | 2 +- spec/requests/comments_spec.rb | 2 +- spec/requests/page_views_spec.rb | 2 +- spec/requests/reactions_spec.rb | 2 +- spec/requests/stories/feeds_spec.rb | 31 ----------- .../prune_old_experiments_worker_spec.rb | 25 --------- .../record_field_test_event_worker_spec.rb | 20 +++---- 15 files changed, 72 insertions(+), 106 deletions(-) delete mode 100644 app/workers/field_tests/prune_old_experiments_worker.rb delete mode 100644 spec/workers/field_tests/prune_old_experiments_worker_spec.rb diff --git a/app/controllers/stories/feeds_controller.rb b/app/controllers/stories/feeds_controller.rb index 6235c8825..eb620229a 100644 --- a/app/controllers/stories/feeds_controller.rb +++ b/app/controllers/stories/feeds_controller.rb @@ -50,10 +50,6 @@ module Stories def optimized_signed_in_feed feed = Articles::Feeds::LargeForemExperimental.new(user: current_user, page: @page, tag: params[:tag]) - # continue to track conversions even in the absence of an experiment so we - # can develop a baseline to compare to - field_test(:user_home_feed, participant: current_user) - feed.more_comments_minimal_weight_randomized_at_end end end diff --git a/app/models/comment.rb b/app/models/comment.rb index ae597f8d9..801e445f9 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -304,7 +304,8 @@ class Comment < ApplicationRecord end def record_field_test_event - Users::RecordFieldTestEventWorker.perform_async(user_id, :user_home_feed, "user_creates_comment") + Users::RecordFieldTestEventWorker + .perform_async(user_id, :follow_implicit_points, "user_creates_comment") end def notify_slack_channel_about_warned_users diff --git a/app/models/page_view.rb b/app/models/page_view.rb index 75dec7904..897970af2 100644 --- a/app/models/page_view.rb +++ b/app/models/page_view.rb @@ -30,7 +30,9 @@ class PageView < ApplicationRecord def record_field_test_event return unless user_id - Users::RecordFieldTestEventWorker.perform_async(user_id, :user_home_feed, "user_views_article_four_days_in_week") - Users::RecordFieldTestEventWorker.perform_async(user_id, :user_home_feed, "user_views_article_four_hours_in_day") + Users::RecordFieldTestEventWorker + .perform_async(user_id, :follow_implicit_points, "user_views_article_four_days_in_week") + Users::RecordFieldTestEventWorker + .perform_async(user_id, :follow_implicit_points, "user_views_article_four_hours_in_day") end end diff --git a/app/models/reaction.rb b/app/models/reaction.rb index aad360c81..c2a42b6bf 100644 --- a/app/models/reaction.rb +++ b/app/models/reaction.rb @@ -139,7 +139,8 @@ class Reaction < ApplicationRecord end def record_field_test_event - Users::RecordFieldTestEventWorker.perform_async(user_id, :user_home_feed, "user_creates_reaction") + Users::RecordFieldTestEventWorker + .perform_async(user_id, :follow_implicit_points, "user_creates_reaction") end def notify_slack_channel_about_vomit_reaction diff --git a/app/workers/field_tests/prune_old_experiments_worker.rb b/app/workers/field_tests/prune_old_experiments_worker.rb deleted file mode 100644 index 11405a4fd..000000000 --- a/app/workers/field_tests/prune_old_experiments_worker.rb +++ /dev/null @@ -1,13 +0,0 @@ -module FieldTests - class PruneOldExperimentsWorker - include Sidekiq::Worker - sidekiq_options queue: :low_priority, retry: 10 - - def perform - five_precent_membership_count = FieldTest::Membership.count / 20 - memberships = FieldTest::Membership.first(five_precent_membership_count) - FieldTest::Event.where(field_test_membership_id: memberships.map(&:id)).delete_all - memberships.map(&:delete) - end - end -end diff --git a/config/field_test.yml b/config/field_test.yml index a3accfff3..00a8b7eb1 100644 --- a/config/field_test.yml +++ b/config/field_test.yml @@ -1,14 +1,4 @@ experiments: - user_home_feed: # Home feed collection for logged in user - variants: - - more_comments_minimal_weight_randomized_at_end - weights: - - 100 - goals: - - user_creates_comment - - user_creates_reaction - - user_views_article_four_days_in_week - - user_views_article_four_hours_in_day follow_implicit_points: # Points given for implicit tag weights # Implemented here: app/workers/follows/update_points_worker.rb # Hypotheses: Weighting tags based on implied preference will lead diff --git a/config/schedule.yml b/config/schedule.yml index 3ef74b361..462fac49c 100644 --- a/config/schedule.yml +++ b/config/schedule.yml @@ -4,9 +4,6 @@ fetch_all_rss: log_worker_queue_stats: cron: "*/10 * * * *" # every 10 minutes class: "Metrics::RecordBackgroundQueueStatsWorker" -prune_old_field_tests: - cron: "0 13 * * *" # daily at 1 pm UTC - class: "FieldTests::PruneOldExperimentsWorker" record_daily_usage: cron: "0 11 * * *" # daily at 11:00 UTC class: "Metrics::RecordDailyUsageWorker" diff --git a/docs/technical-overview/ab_testing.md b/docs/technical-overview/ab_testing.md index b998e5222..84db147e9 100644 --- a/docs/technical-overview/ab_testing.md +++ b/docs/technical-overview/ab_testing.md @@ -2,6 +2,54 @@ We use the [Field Test](https://github.com/ankane/field_test) gem for conducting simple A/B tests. -If you want to propose an A/B test of a feature, you may make a pull request which defines the hypotheses and what admins should look for to declare a winner. As A/B tests are going to have results that may differ from Forem to Forem, the process is relatively immature. In the future we may have more studies that can return anonymous ecosystem-wide results. +If you want to propose an A/B test of a feature, you may make a pull request +which defines the hypotheses and what admins should look for to declare a winner. +As A/B tests are going to have results that may differ from Forem to Forem, the +process is relatively immature. In the future we may have more studies that can return anonymous ecosystem-wide results. -A/B tests are inherently the most useful in _large_ Forems, where qualitative feedback may be more useful on small Forems. As such, [DEV](https://dev.to) is our largest Forem and therefore can provide us with the most feedback for our existing A/B tests. However, we must keep in mind that DEV results may not apply well to future large Forems. We should seek to re-run useful experiments within the ecosystem after time has passed. +A/B tests are inherently the most useful in _large_ Forems, where qualitative +feedback may be more useful on small Forems. As such, [DEV](https://dev.to) +is our largest Forem and therefore can provide us with the most feedback for +our existing A/B tests. However, we must keep in mind that DEV results may +not apply well to future large Forems. We should seek to re-run useful experiments within the ecosystem after time has passed. + +## Creating a new A/B test + +Follow the guidelines of the field test gem and add the test info to [config/field_test.yml](https://github.com/forem/forem/blob/master/config/field_test.yml). + +Then where you want to trigger the variant, you'll add some code like this: + +```ruby + test_variant = field_test(:follow_implicit_points, participant: user) + case test_variant + when "no_implicit_score" + 0 + when "half_weight_after_log" + Math.log(occurrences + bonus + 1) * 0.5 + when "double_weight_after_log" + Math.log(occurrences + bonus + 1) * 2.0 + when "double_bonus_before_log" + Math.log(occurrences + (bonus * 2) + 1) + when "without_weighting_bonus" + Math.log(occurrences + 1) + else # base - Our current "default" implementation + Math.log(occurrences + bonus + 1) # + 1 in all cases is to avoid log(0) => -infinity + end +``` + +Which would find or create the test variant for that user in particular. If +this code is not called in the controller or view, you'll need to first +include the gem helpers at the top of the file... + +``` +include FieldTest::Helpers +``` + +To record a successful field test outcome, you should call something like this + +```ruby + Users::RecordFieldTestEventWorker + .perform_async(user_id, :follow_implicit_points, "user_creates_reaction") +``` + +And modify that class as needed to determine whether to record the successful trial. \ No newline at end of file diff --git a/spec/factories/field_test_memberships.rb b/spec/factories/field_test_memberships.rb index a3536443d..2c4e42bf9 100644 --- a/spec/factories/field_test_memberships.rb +++ b/spec/factories/field_test_memberships.rb @@ -1,7 +1,7 @@ FactoryBot.define do factory :field_test_membership, class: "FieldTest::Membership" do converted { false } - experiment { :user_home_feed } + experiment { :follow_implicit_points } participant_type { "User" } variant { "base" } end diff --git a/spec/requests/comments_spec.rb b/spec/requests/comments_spec.rb index 53c6f415e..24b88577b 100644 --- a/spec/requests/comments_spec.rb +++ b/spec/requests/comments_spec.rb @@ -318,7 +318,7 @@ RSpec.describe "Comments", type: :request do it "converts field test" do post "/comments", params: base_comment_params - expected_args = [user.id, :user_home_feed, "user_creates_comment"] + expected_args = [user.id, :follow_implicit_points, "user_creates_comment"] expect(Users::RecordFieldTestEventWorker).to have_received(:perform_async).with(*expected_args) end end diff --git a/spec/requests/page_views_spec.rb b/spec/requests/page_views_spec.rb index 8f3396d52..ec7fffbd1 100644 --- a/spec/requests/page_views_spec.rb +++ b/spec/requests/page_views_spec.rb @@ -56,7 +56,7 @@ RSpec.describe "PageViews", type: :request do referrer: "test" } expect(Users::RecordFieldTestEventWorker).to have_received(:perform_async) - .with(user.id, :user_home_feed, "user_views_article_four_days_in_week") + .with(user.id, :follow_implicit_points, "user_views_article_four_days_in_week") end end diff --git a/spec/requests/reactions_spec.rb b/spec/requests/reactions_spec.rb index e1fa2b08c..b2a5d9a4f 100644 --- a/spec/requests/reactions_spec.rb +++ b/spec/requests/reactions_spec.rb @@ -324,7 +324,7 @@ RSpec.describe "Reactions", type: :request do it "converts field test" do post "/reactions", params: article_params - expect(Users::RecordFieldTestEventWorker).to have_received(:perform_async).with(user.id, :user_home_feed, + expect(Users::RecordFieldTestEventWorker).to have_received(:perform_async).with(user.id, :follow_implicit_points, "user_creates_reaction") end end diff --git a/spec/requests/stories/feeds_spec.rb b/spec/requests/stories/feeds_spec.rb index b48e77cf4..c1c1e6f6b 100644 --- a/spec/requests/stories/feeds_spec.rb +++ b/spec/requests/stories/feeds_spec.rb @@ -127,23 +127,6 @@ RSpec.describe "Stories::Feeds", type: :request do sign_in user end - it "sets a field test when feed_strategy is optimized" do - allow(SiteConfig).to receive(:feed_strategy).and_return("optimized") - expect do - get "/stories/feed" - end.to change(user.field_test_memberships, :count).by(1) - - ftm = user.field_test_memberships.last - expect(ftm.experiment).to eq("user_home_feed") - end - - it "does not set a field test when feed_strategy is basic" do - allow(SiteConfig).to receive(:feed_strategy).and_return("basic") - expect do - get "/stories/feed" - end.not_to change(user.field_test_memberships, :count) - end - it "returns feed when feed_strategy is basic" do allow(SiteConfig).to receive(:feed_strategy).and_return("basic") get "/stories/feed" @@ -192,19 +175,5 @@ RSpec.describe "Stories::Feeds", type: :request do expect(response_article["top_comments"].first["username"]).not_to be_nil end end - - context "when user is signed in but there's no field_test" do - before do - sign_in user - end - - it "does not set a field test" do - allow_any_instance_of(Stories::FeedsController).to receive(:field_test) # rubocop:disable RSpec/AnyInstance - - expect do - get "/stories/feed" - end.not_to change(user.field_test_memberships, :count) - end - end end end diff --git a/spec/workers/field_tests/prune_old_experiments_worker_spec.rb b/spec/workers/field_tests/prune_old_experiments_worker_spec.rb deleted file mode 100644 index 3c341729c..000000000 --- a/spec/workers/field_tests/prune_old_experiments_worker_spec.rb +++ /dev/null @@ -1,25 +0,0 @@ -require "rails_helper" - -RSpec.describe FieldTests::PruneOldExperimentsWorker, type: :worker do - include_examples "#enqueues_on_correct_queue", "low_priority", 1 - include FieldTest::Helpers - - describe "#perform" do - let(:worker) { subject } - - it "prunes first 5% of memberships and events" do - users = create_list(:user, 40) - - users.each do |user| - create(:field_test_membership, participant_id: user.id.to_s) - field_test_converted(:user_home_feed, participant: user, goal: "user_creates_comment") - end - - worker.perform - - expect(FieldTest::Membership.count).to eq(38) - expect(FieldTest::Event.count).to eq(38) - expect(FieldTest::Event.pluck(:field_test_membership_id).sort).to eq(FieldTest::Membership.ids.sort) - end - end -end diff --git a/spec/workers/users/record_field_test_event_worker_spec.rb b/spec/workers/users/record_field_test_event_worker_spec.rb index ad5316b6f..0af88c429 100644 --- a/spec/workers/users/record_field_test_event_worker_spec.rb +++ b/spec/workers/users/record_field_test_event_worker_spec.rb @@ -11,17 +11,17 @@ RSpec.describe Users::RecordFieldTestEventWorker, type: :worker do context "with user who is part of field test" do before do - field_test(:user_home_feed, participant: user) + field_test(:follow_implicit_points, participant: user) end it "records user_creates_reaction field test conversion" do - worker.perform(user.id, "user_home_feed", "user_creates_reaction") + worker.perform(user.id, "follow_implicit_points", "user_creates_reaction") expect(FieldTest::Event.last.field_test_membership.participant_id).to eq(user.id.to_s) expect(FieldTest::Event.last.name).to eq("user_creates_reaction") end it "records user_creates_comment field test conversion" do - worker.perform(user.id, "user_home_feed", "user_creates_comment") + worker.perform(user.id, "follow_implicit_points", "user_creates_comment") expect(FieldTest::Event.last.field_test_membership.participant_id).to eq(user.id.to_s) expect(FieldTest::Event.last.name).to eq("user_creates_comment") end @@ -30,7 +30,7 @@ RSpec.describe Users::RecordFieldTestEventWorker, type: :worker do 7.times do |n| create(:page_view, user_id: user.id, created_at: n.days.ago) end - worker.perform(user.id, "user_home_feed", "user_views_article_four_days_in_week") + worker.perform(user.id, "follow_implicit_points", "user_views_article_four_days_in_week") expect(FieldTest::Event.last.field_test_membership.participant_id).to eq(user.id.to_s) expect(FieldTest::Event.last.name).to eq("user_views_article_four_days_in_week") end @@ -39,7 +39,7 @@ RSpec.describe Users::RecordFieldTestEventWorker, type: :worker do 2.times do |n| create(:page_view, user_id: user.id, created_at: n.days.ago) end - worker.perform(user.id, "user_home_feed", "user_views_article_four_days_in_week") + worker.perform(user.id, "follow_implicit_points", "user_views_article_four_days_in_week") expect(FieldTest::Event.all.size).to be(0) end @@ -47,7 +47,7 @@ RSpec.describe Users::RecordFieldTestEventWorker, type: :worker do 7.times do |n| create(:page_view, user_id: user.id, created_at: n.hours.ago) end - worker.perform(user.id, "user_home_feed", "user_views_article_four_hours_in_day") + worker.perform(user.id, "follow_implicit_points", "user_views_article_four_hours_in_day") expect(FieldTest::Event.last.field_test_membership.participant_id).to eq(user.id.to_s) expect(FieldTest::Event.last.name).to eq("user_views_article_four_hours_in_day") end @@ -56,19 +56,19 @@ RSpec.describe Users::RecordFieldTestEventWorker, type: :worker do 2.times do |n| create(:page_view, user_id: user.id, created_at: n.hours.ago) end - worker.perform(user.id, "user_home_feed", "user_views_article_four_hours_in_day") + worker.perform(user.id, "follow_implicit_points", "user_views_article_four_hours_in_day") expect(FieldTest::Event.all.size).to be(0) end end context "with user who is not part of field test" do it "records user_creates_reaction field test conversion" do - worker.perform(user.id, "user_home_feed", "user_creates_reaction") + worker.perform(user.id, "follow_implicit_points", "user_creates_reaction") expect(FieldTest::Event.all.size).to be(0) end it "records user_creates_comment field test conversion" do - worker.perform(user.id, "user_home_feed", "user_creates_comment") + worker.perform(user.id, "follow_implicit_points", "user_creates_comment") expect(FieldTest::Event.all.size).to be(0) end @@ -83,7 +83,7 @@ RSpec.describe Users::RecordFieldTestEventWorker, type: :worker do context "without a user" do it "does not raise an error" do expect do - worker.perform(user.id + 1000, "user_home_feed", "user_creates_reaction") + worker.perform(user.id + 1000, "follow_implicit_points", "user_creates_reaction") end.not_to raise_error end end