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

Fix experiment user recording and "group type" mismatch

See merge request gitlab-org/gitlab!51468

lib/gitlab/experimentation.rb

@@ -6,6 +6,7 @@
 # Experiment options:
 # - 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.)
+# - 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):
 # 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.
@@ -92,16 +93,19 @@ module Gitlab
         tracking_category: 'Growth::Conversion::Experiment::TrialDuringSignup'
       },
       ci_syntax_templates: {
-        tracking_category: 'Growth::Activation::Experiment::CiSyntaxTemplates'
+        tracking_category: 'Growth::Activation::Experiment::CiSyntaxTemplates',
+        rollout_strategy: :user
       },
       pipelines_empty_state: {
-        tracking_category: 'Growth::Activation::Experiment::PipelinesEmptyState'
+        tracking_category: 'Growth::Activation::Experiment::PipelinesEmptyState',
+        rollout_strategy: :user
       },
       invite_members_new_dropdown: {
         tracking_category: 'Growth::Expansion::Experiment::InviteMembersNewDropdown'
       },
       show_trial_status_in_sidebar: {
-        tracking_category: 'Growth::Conversion::Experiment::ShowTrialStatusInSidebar'
+        tracking_category: 'Growth::Conversion::Experiment::ShowTrialStatusInSidebar',
+        rollout_strategy: :group
       },
       trial_onboarding_issues: {
         tracking_category: 'Growth::Conversion::Experiment::TrialOnboardingIssues'
@@ -125,6 +129,7 @@ module Gitlab
       def in_experiment_group?(experiment_key, subject:)
         return false if subject.blank?
         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)
         return false unless experiment
@@ -132,6 +137,26 @@ module Gitlab
         experiment.enabled_for_index?(index_for_subject(experiment, subject))
       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
 
       def index_for_subject(experiment, subject)

lib/gitlab/experimentation/controller_concern.rb

@@ -39,6 +39,7 @@ module Gitlab
       def experiment_enabled?(experiment_key, subject: nil)
         return true if forced_enabled?(experiment_key)
         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)
 
@@ -65,7 +66,9 @@ module Gitlab
         return if dnt_enabled?
         return unless Experimentation.active?(experiment_key) && current_user
 
-        ::Experiment.add_user(experiment_key, tracking_group(experiment_key, nil, subject: current_user), current_user, context)
+        subject = Experimentation.rollout_strategy(experiment_key) == :cookie ? nil : current_user
+
+        ::Experiment.add_user(experiment_key, tracking_group(experiment_key, nil, subject: subject), current_user, context)
       end
 
       def record_experiment_conversion_event(experiment_key)

lib/gitlab/experimentation/experiment.rb

@@ -5,12 +5,13 @@ module Gitlab
     class Experiment
       FEATURE_FLAG_SUFFIX = "_experiment_percentage"
 
-      attr_reader :key, :tracking_category, :use_backwards_compatible_subject_index
+      attr_reader :key, :tracking_category, :use_backwards_compatible_subject_index, :rollout_strategy
 
       def initialize(key, **params)
         @key = key
         @tracking_category = params[:tracking_category]
         @use_backwards_compatible_subject_index = params[:use_backwards_compatible_subject_index]
+        @rollout_strategy = params[:rollout_strategy] || :cookie
       end
 
       def active?

spec/lib/gitlab/experimentation/controller_concern_spec.rb

@@ -10,6 +10,10 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
           use_backwards_compatible_subject_index: true
         },
         test_experiment: {
+          tracking_category: 'Team',
+          rollout_strategy: rollout_strategy
+        },
+        my_experiment: {
           tracking_category: 'Team'
         }
       }
@@ -20,6 +24,7 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
   end
 
   let(:enabled_percentage) { 10 }
+  let(:rollout_strategy) { nil }
 
   controller(ApplicationController) do
     include Gitlab::Experimentation::ControllerConcern
@@ -117,6 +122,7 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
       end
 
       context 'when subject is given' do
+        let(:rollout_strategy) { :user }
         let(:user) { build(:user) }
 
         it 'uses the subject' do
@@ -244,6 +250,7 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
 
         it "provides the subject's hashed global_id as label" do
           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)
 
@@ -420,6 +427,26 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
 
           controller.record_experiment_user(:test_experiment, context)
         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
 
       context 'the user is part of the control group' do

spec/lib/gitlab/experimentation/experiment_spec.rb

@@ -9,7 +9,8 @@ RSpec.describe Gitlab::Experimentation::Experiment do
   let(:params) do
     {
       tracking_category: 'Category1',
-      use_backwards_compatible_subject_index: true
+      use_backwards_compatible_subject_index: true,
+      rollout_strategy: nil
     }
   end
 

spec/lib/gitlab/experimentation_spec.rb

@@ -102,7 +102,11 @@ RSpec.describe Gitlab::Experimentation do
         context 'when subject has a global_id' do
           let(:experiment_subject) { double(:subject, to_global_id: 'z') }
 
-          it { is_expected.to eq(true) }
+          it do
+            allow(Gitlab::Experimentation).to receive(:valid_subject_for_rollout_strategy?).and_return(true)
+
+            is_expected.to eq(true)
+          end
         end
 
         context 'when subject is nil' do
@@ -150,7 +154,11 @@ RSpec.describe Gitlab::Experimentation do
         context 'when subject has a global_id' do
           let(:experiment_subject) { double(:subject, to_global_id: 'abcd') }
 
-          it { is_expected.to eq(true) }
+          it do
+            allow(Gitlab::Experimentation).to receive(:valid_subject_for_rollout_strategy?).and_return(true)
+
+            is_expected.to eq(true)
+          end
         end
 
         context 'when subject is nil' do