Merge branch 'aa-fix-experiment-group-mismatch' into 'master'

RE-Fix experiment user recording and "group type" mismatch

See merge request gitlab-org/gitlab!51937

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'
@@ -126,12 +130,44 @@ module Gitlab
         return false if subject.blank?
         return false unless active?(experiment_key)
 
+        log_invalid_rollout(experiment_key, subject)
+
         experiment = get_experiment(experiment_key)
         return false unless experiment
 
         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 log_invalid_rollout(experiment_key, subject)
+        return if valid_subject_for_rollout_strategy?(experiment_key, subject)
+
+        logger = Gitlab::ExperimentationLogger.build
+        logger.warn message: 'Subject must conform to the rollout strategy',
+                     experiment_key: experiment_key,
+                     subject: subject.class.to_s,
+                     rollout_strategy: rollout_strategy(experiment_key)
+      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

@@ -40,6 +40,8 @@ module Gitlab
         return true if forced_enabled?(experiment_key)
         return false if dnt_enabled?
 
+        Experimentation.log_invalid_rollout(experiment_key, subject)
+
         subject ||= fallback_experimentation_subject_index(experiment_key)
 
         Experimentation.in_experiment_group?(experiment_key, subject: subject)
@@ -65,7 +67,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?

lib/gitlab/experimentation_logger.rb

+# frozen_string_literal: true
+
+module Gitlab
+  class ExperimentationLogger < ::Gitlab::JsonLogger
+    def self.file_name_noext
+      'experimentation_json'
+    end
+  end
+end

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

@@ -27,6 +27,8 @@ RSpec.describe Gitlab::Experimentation::EXPERIMENTS do
 end
 
 RSpec.describe Gitlab::Experimentation do
+  using RSpec::Parameterized::TableSyntax
+
   before do
     stub_const('Gitlab::Experimentation::EXPERIMENTS', {
       backwards_compatible_test_experiment: {
@@ -35,6 +37,10 @@ RSpec.describe Gitlab::Experimentation do
       },
       test_experiment: {
         tracking_category: 'Team'
+      },
+      tabular_experiment: {
+        tracking_category: 'Team',
+        rollout_strategy: rollout_strategy
       }
     })
 
@@ -46,6 +52,7 @@ RSpec.describe Gitlab::Experimentation do
   end
 
   let(:enabled_percentage) { 10 }
+  let(:rollout_strategy) { nil }
 
   describe '.get_experiment' do
     subject { described_class.get_experiment(:test_experiment) }
@@ -175,4 +182,59 @@ RSpec.describe Gitlab::Experimentation do
       end
     end
   end
+
+  describe '.log_invalid_rollout' do
+    subject { described_class.log_invalid_rollout(:test_experiment, 1) }
+
+    before do
+      allow(described_class).to receive(:valid_subject_for_rollout_strategy?).and_return(valid)
+    end
+
+    context 'subject is not valid for experiment' do
+      let(:valid) { false }
+
+      it 'logs a warning message' do
+        expect_next_instance_of(Gitlab::ExperimentationLogger) do |logger|
+          expect(logger)
+            .to receive(:warn)
+                  .with(
+                    message: 'Subject must conform to the rollout strategy',
+                    experiment_key: :test_experiment,
+                    subject: 'Integer',
+                    rollout_strategy: :cookie
+                  )
+        end
+
+        subject
+      end
+    end
+
+    context 'subject is valid for experiment' do
+      let(:valid) { true }
+
+      it 'does not log a warning message' do
+        expect(Gitlab::ExperimentationLogger).not_to receive(:build)
+
+        subject
+      end
+    end
+  end
+
+  describe '.valid_subject_for_rollout_strategy?' do
+    subject { described_class.valid_subject_for_rollout_strategy?(:tabular_experiment, experiment_subject) }
+
+    where(:rollout_strategy, :experiment_subject, :result) do
+      :cookie | nil       | true
+      nil     | nil       | true
+      :cookie | 'string'  | true
+      nil     | User.new  | false
+      :user   | User.new  | true
+      :group  | User.new  | false
+      :group  | Group.new | true
+    end
+
+    with_them do
+      it { is_expected.to be(result) }
+    end
+  end
 end