Commit 37bad949 authored by Doug Stull's avatar Doug Stull

Revert "Merge branch...

Revert "Merge branch '297379-experiment-recording-and-control-experiment-bug-investigation' into 'master'"

This reverts merge request !51468
parent 82325168
...@@ -6,7 +6,6 @@ ...@@ -6,7 +6,6 @@
# Experiment options: # Experiment options:
# - tracking_category (optional, used to set the category when tracking an experiment event) # - tracking_category (optional, used to set the category when tracking an experiment event)
# - use_backwards_compatible_subject_index (optional, set this to true if you need backwards compatibility -- you likely do not need this, see note in the next paragraph.) # - use_backwards_compatible_subject_index (optional, set this to true if you need backwards compatibility -- you likely do not need this, see note in the next paragraph.)
# - rollout_strategy: default is `:cookie` based rollout. We may also set it to `:user` based rollout
# #
# Using the backwards-compatible subject index (use_backwards_compatible_subject_index option): # Using the backwards-compatible subject index (use_backwards_compatible_subject_index option):
# This option was added when [the calculation of experimentation_subject_index was changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45733/diffs#41af4a6fa5a10c7068559ce21c5188483751d934_157_173). It is not intended to be used by new experiments, it exists merely for the segmentation integrity of in-flight experiments at the time the change was deployed. That is, we want users who were assigned to the "experimental" group or the "control" group before the change to still be in those same groups after the change. See [the original issue](https://gitlab.com/gitlab-org/gitlab/-/issues/270858) and [this related comment](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48110#note_458223745) for more information. # This option was added when [the calculation of experimentation_subject_index was changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45733/diffs#41af4a6fa5a10c7068559ce21c5188483751d934_157_173). It is not intended to be used by new experiments, it exists merely for the segmentation integrity of in-flight experiments at the time the change was deployed. That is, we want users who were assigned to the "experimental" group or the "control" group before the change to still be in those same groups after the change. See [the original issue](https://gitlab.com/gitlab-org/gitlab/-/issues/270858) and [this related comment](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48110#note_458223745) for more information.
...@@ -93,19 +92,16 @@ module Gitlab ...@@ -93,19 +92,16 @@ module Gitlab
tracking_category: 'Growth::Conversion::Experiment::TrialDuringSignup' tracking_category: 'Growth::Conversion::Experiment::TrialDuringSignup'
}, },
ci_syntax_templates: { ci_syntax_templates: {
tracking_category: 'Growth::Activation::Experiment::CiSyntaxTemplates', tracking_category: 'Growth::Activation::Experiment::CiSyntaxTemplates'
rollout_strategy: :user
}, },
pipelines_empty_state: { pipelines_empty_state: {
tracking_category: 'Growth::Activation::Experiment::PipelinesEmptyState', tracking_category: 'Growth::Activation::Experiment::PipelinesEmptyState'
rollout_strategy: :user
}, },
invite_members_new_dropdown: { invite_members_new_dropdown: {
tracking_category: 'Growth::Expansion::Experiment::InviteMembersNewDropdown' tracking_category: 'Growth::Expansion::Experiment::InviteMembersNewDropdown'
}, },
show_trial_status_in_sidebar: { show_trial_status_in_sidebar: {
tracking_category: 'Growth::Conversion::Experiment::ShowTrialStatusInSidebar', tracking_category: 'Growth::Conversion::Experiment::ShowTrialStatusInSidebar'
rollout_strategy: :group
}, },
trial_onboarding_issues: { trial_onboarding_issues: {
tracking_category: 'Growth::Conversion::Experiment::TrialOnboardingIssues' tracking_category: 'Growth::Conversion::Experiment::TrialOnboardingIssues'
...@@ -129,7 +125,6 @@ module Gitlab ...@@ -129,7 +125,6 @@ module Gitlab
def in_experiment_group?(experiment_key, subject:) def in_experiment_group?(experiment_key, subject:)
return false if subject.blank? return false if subject.blank?
return false unless active?(experiment_key) return false unless active?(experiment_key)
raise 'Subject must conform to the rollout strategy' unless valid_subject_for_rollout_strategy?(experiment_key, subject)
experiment = get_experiment(experiment_key) experiment = get_experiment(experiment_key)
return false unless experiment return false unless experiment
...@@ -137,26 +132,6 @@ module Gitlab ...@@ -137,26 +132,6 @@ module Gitlab
experiment.enabled_for_index?(index_for_subject(experiment, subject)) experiment.enabled_for_index?(index_for_subject(experiment, subject))
end end
def rollout_strategy(experiment_key)
experiment = get_experiment(experiment_key)
return unless experiment
experiment.rollout_strategy
end
def valid_subject_for_rollout_strategy?(experiment_key, subject)
case rollout_strategy(experiment_key)
when :user
subject.is_a?(User)
when :group
subject.is_a?(Group)
when :cookie
subject.nil? || subject.is_a?(String)
else
false
end
end
private private
def index_for_subject(experiment, subject) def index_for_subject(experiment, subject)
......
...@@ -39,7 +39,6 @@ module Gitlab ...@@ -39,7 +39,6 @@ module Gitlab
def experiment_enabled?(experiment_key, subject: nil) def experiment_enabled?(experiment_key, subject: nil)
return true if forced_enabled?(experiment_key) return true if forced_enabled?(experiment_key)
return false if dnt_enabled? return false if dnt_enabled?
raise "Subject must conform to the rollout strategy for #{experiment_key}" unless Experimentation.valid_subject_for_rollout_strategy?(experiment_key, subject)
subject ||= fallback_experimentation_subject_index(experiment_key) subject ||= fallback_experimentation_subject_index(experiment_key)
...@@ -66,9 +65,7 @@ module Gitlab ...@@ -66,9 +65,7 @@ module Gitlab
return if dnt_enabled? return if dnt_enabled?
return unless Experimentation.active?(experiment_key) && current_user return unless Experimentation.active?(experiment_key) && current_user
subject = Experimentation.rollout_strategy(experiment_key) == :cookie ? nil : current_user ::Experiment.add_user(experiment_key, tracking_group(experiment_key, nil, subject: current_user), current_user, context)
::Experiment.add_user(experiment_key, tracking_group(experiment_key, nil, subject: subject), current_user, context)
end end
def record_experiment_conversion_event(experiment_key) def record_experiment_conversion_event(experiment_key)
......
...@@ -5,13 +5,12 @@ module Gitlab ...@@ -5,13 +5,12 @@ module Gitlab
class Experiment class Experiment
FEATURE_FLAG_SUFFIX = "_experiment_percentage" FEATURE_FLAG_SUFFIX = "_experiment_percentage"
attr_reader :key, :tracking_category, :use_backwards_compatible_subject_index, :rollout_strategy attr_reader :key, :tracking_category, :use_backwards_compatible_subject_index
def initialize(key, **params) def initialize(key, **params)
@key = key @key = key
@tracking_category = params[:tracking_category] @tracking_category = params[:tracking_category]
@use_backwards_compatible_subject_index = params[:use_backwards_compatible_subject_index] @use_backwards_compatible_subject_index = params[:use_backwards_compatible_subject_index]
@rollout_strategy = params[:rollout_strategy] || :cookie
end end
def active? def active?
......
...@@ -10,10 +10,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do ...@@ -10,10 +10,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
use_backwards_compatible_subject_index: true use_backwards_compatible_subject_index: true
}, },
test_experiment: { test_experiment: {
tracking_category: 'Team',
rollout_strategy: rollout_strategy
},
my_experiment: {
tracking_category: 'Team' tracking_category: 'Team'
} }
} }
...@@ -24,7 +20,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do ...@@ -24,7 +20,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
end end
let(:enabled_percentage) { 10 } let(:enabled_percentage) { 10 }
let(:rollout_strategy) { nil }
controller(ApplicationController) do controller(ApplicationController) do
include Gitlab::Experimentation::ControllerConcern include Gitlab::Experimentation::ControllerConcern
...@@ -122,7 +117,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do ...@@ -122,7 +117,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
end end
context 'when subject is given' do context 'when subject is given' do
let(:rollout_strategy) { :user }
let(:user) { build(:user) } let(:user) { build(:user) }
it 'uses the subject' do it 'uses the subject' do
...@@ -250,7 +244,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do ...@@ -250,7 +244,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
it "provides the subject's hashed global_id as label" do it "provides the subject's hashed global_id as label" do
experiment_subject = double(:subject, to_global_id: 'abc') experiment_subject = double(:subject, to_global_id: 'abc')
allow(Gitlab::Experimentation).to receive(:valid_subject_for_rollout_strategy?).and_return(true)
controller.track_experiment_event(:test_experiment, 'start', 1, subject: experiment_subject) controller.track_experiment_event(:test_experiment, 'start', 1, subject: experiment_subject)
...@@ -427,26 +420,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do ...@@ -427,26 +420,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
controller.record_experiment_user(:test_experiment, context) controller.record_experiment_user(:test_experiment, context)
end end
context 'with a cookie based rollout strategy' do
it 'calls tracking_group with a nil subject' do
expect(controller).to receive(:tracking_group).with(:test_experiment, nil, subject: nil).and_return(:experimental)
allow(::Experiment).to receive(:add_user).with(:test_experiment, :experimental, user, context)
controller.record_experiment_user(:test_experiment, context)
end
end
context 'with a user based rollout strategy' do
let(:rollout_strategy) { :user }
it 'calls tracking_group with a user subject' do
expect(controller).to receive(:tracking_group).with(:test_experiment, nil, subject: user).and_return(:experimental)
allow(::Experiment).to receive(:add_user).with(:test_experiment, :experimental, user, context)
controller.record_experiment_user(:test_experiment, context)
end
end
end end
context 'the user is part of the control group' do context 'the user is part of the control group' do
......
...@@ -9,8 +9,7 @@ RSpec.describe Gitlab::Experimentation::Experiment do ...@@ -9,8 +9,7 @@ RSpec.describe Gitlab::Experimentation::Experiment do
let(:params) do let(:params) do
{ {
tracking_category: 'Category1', tracking_category: 'Category1',
use_backwards_compatible_subject_index: true, use_backwards_compatible_subject_index: true
rollout_strategy: nil
} }
end end
......
...@@ -102,11 +102,7 @@ RSpec.describe Gitlab::Experimentation do ...@@ -102,11 +102,7 @@ RSpec.describe Gitlab::Experimentation do
context 'when subject has a global_id' do context 'when subject has a global_id' do
let(:experiment_subject) { double(:subject, to_global_id: 'z') } let(:experiment_subject) { double(:subject, to_global_id: 'z') }
it do it { is_expected.to eq(true) }
allow(Gitlab::Experimentation).to receive(:valid_subject_for_rollout_strategy?).and_return(true)
is_expected.to eq(true)
end
end end
context 'when subject is nil' do context 'when subject is nil' do
...@@ -154,11 +150,7 @@ RSpec.describe Gitlab::Experimentation do ...@@ -154,11 +150,7 @@ RSpec.describe Gitlab::Experimentation do
context 'when subject has a global_id' do context 'when subject has a global_id' do
let(:experiment_subject) { double(:subject, to_global_id: 'abcd') } let(:experiment_subject) { double(:subject, to_global_id: 'abcd') }
it do it { is_expected.to eq(true) }
allow(Gitlab::Experimentation).to receive(:valid_subject_for_rollout_strategy?).and_return(true)
is_expected.to eq(true)
end
end end
context 'when subject is nil' do context 'when subject is nil' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment