Merge branch '270858-make-experimentation-subject-index-unique-per-experiment' into 'master'

Make experimentation subject index unique per experiment

See merge request gitlab-org/gitlab!45733

lib/gitlab/experimentation.rb

 # frozen_string_literal: true
 
+require 'zlib'
+
 # == Experimentation
 #
 # Utility module for A/B testing experimental features. Define your experiments in the `EXPERIMENTS` constant.
 # Experiment options:
 # - environment (optional, defaults to enabled for development and GitLab.com)
 # - 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)
 #
 # The experiment is controlled by a Feature Flag (https://docs.gitlab.com/ee/development/feature_flags/controls.html),
 # which is named "#{experiment_key}_experiment_percentage" and *must* be set with a percentage and not be used for other purposes.
@@ -31,46 +34,60 @@ module Gitlab
   module Experimentation
     EXPERIMENTS = {
       signup_flow: {
-        tracking_category: 'Growth::Acquisition::Experiment::SignUpFlow'
+        tracking_category: 'Growth::Acquisition::Experiment::SignUpFlow',
+        use_backwards_compatible_subject_index: true
       },
       onboarding_issues: {
-        tracking_category: 'Growth::Conversion::Experiment::OnboardingIssues'
+        tracking_category: 'Growth::Conversion::Experiment::OnboardingIssues',
+        use_backwards_compatible_subject_index: true
       },
       ci_notification_dot: {
-        tracking_category: 'Growth::Expansion::Experiment::CiNotificationDot'
+        tracking_category: 'Growth::Expansion::Experiment::CiNotificationDot',
+        use_backwards_compatible_subject_index: true
       },
       upgrade_link_in_user_menu_a: {
-        tracking_category: 'Growth::Expansion::Experiment::UpgradeLinkInUserMenuA'
+        tracking_category: 'Growth::Expansion::Experiment::UpgradeLinkInUserMenuA',
+        use_backwards_compatible_subject_index: true
       },
       invite_members_version_a: {
-        tracking_category: 'Growth::Expansion::Experiment::InviteMembersVersionA'
+        tracking_category: 'Growth::Expansion::Experiment::InviteMembersVersionA',
+        use_backwards_compatible_subject_index: true
       },
       invite_members_version_b: {
-        tracking_category: 'Growth::Expansion::Experiment::InviteMembersVersionB'
+        tracking_category: 'Growth::Expansion::Experiment::InviteMembersVersionB',
+        use_backwards_compatible_subject_index: true
       },
       invite_members_empty_group_version_a: {
-        tracking_category: 'Growth::Expansion::Experiment::InviteMembersEmptyGroupVersionA'
+        tracking_category: 'Growth::Expansion::Experiment::InviteMembersEmptyGroupVersionA',
+        use_backwards_compatible_subject_index: true
       },
       new_create_project_ui: {
-        tracking_category: 'Manage::Import::Experiment::NewCreateProjectUi'
+        tracking_category: 'Manage::Import::Experiment::NewCreateProjectUi',
+        use_backwards_compatible_subject_index: true
       },
       contact_sales_btn_in_app: {
-        tracking_category: 'Growth::Conversion::Experiment::ContactSalesInApp'
+        tracking_category: 'Growth::Conversion::Experiment::ContactSalesInApp',
+        use_backwards_compatible_subject_index: true
       },
       customize_homepage: {
-        tracking_category: 'Growth::Expansion::Experiment::CustomizeHomepage'
+        tracking_category: 'Growth::Expansion::Experiment::CustomizeHomepage',
+        use_backwards_compatible_subject_index: true
       },
       invite_email: {
-        tracking_category: 'Growth::Acquisition::Experiment::InviteEmail'
+        tracking_category: 'Growth::Acquisition::Experiment::InviteEmail',
+        use_backwards_compatible_subject_index: true
       },
       invitation_reminders: {
-        tracking_category: 'Growth::Acquisition::Experiment::InvitationReminders'
+        tracking_category: 'Growth::Acquisition::Experiment::InvitationReminders',
+        use_backwards_compatible_subject_index: true
       },
       group_only_trials: {
-        tracking_category: 'Growth::Conversion::Experiment::GroupOnlyTrials'
+        tracking_category: 'Growth::Conversion::Experiment::GroupOnlyTrials',
+        use_backwards_compatible_subject_index: true
       },
       default_to_issues_board: {
-        tracking_category: 'Growth::Conversion::Experiment::DefaultToIssuesBoard'
+        tracking_category: 'Growth::Conversion::Experiment::DefaultToIssuesBoard',
+        use_backwards_compatible_subject_index: true
       }
     }.freeze
 
@@ -110,7 +127,7 @@ module Gitlab
       def experiment_enabled?(experiment_key)
         return false if dnt_enabled?
 
-        return true if Experimentation.enabled_for_value?(experiment_key, experimentation_subject_index)
+        return true if Experimentation.enabled_for_value?(experiment_key, experimentation_subject_index(experiment_key))
         return true if forced_enabled?(experiment_key)
 
         false
@@ -153,10 +170,14 @@ module Gitlab
         cookies.signed[:experimentation_subject_id]
       end
 
-      def experimentation_subject_index
+      def experimentation_subject_index(experiment_key)
         return if experimentation_subject_id.blank?
 
+        if Experimentation.experiment(experiment_key).use_backwards_compatible_subject_index
           experimentation_subject_id.delete('-').hex % 100
+        else
+          Zlib.crc32("#{experiment_key}#{experimentation_subject_id}") % 100
+        end
       end
 
       def track_experiment_event_for(experiment_key, action, value)
@@ -209,13 +230,18 @@ module Gitlab
         enabled_for_value?(experiment_key, index)
       end
 
-      def enabled_for_value?(experiment_key, experimentation_subject_index)
-        enabled?(experiment_key) &&
-          experiment(experiment_key).enabled_for_index?(experimentation_subject_index)
+      def enabled_for_value?(experiment_key, value)
+        enabled?(experiment_key) && experiment(experiment_key).enabled_for_index?(value)
       end
     end
 
-    Experiment = Struct.new(:key, :environment, :tracking_category, keyword_init: true) do
+    Experiment = Struct.new(
+      :key,
+      :environment,
+      :tracking_category,
+      :use_backwards_compatible_subject_index,
+      keyword_init: true
+    ) do
       def enabled?
         experiment_percentage > 0
       end

spec/lib/gitlab/experimentation_spec.rb

@@ -2,15 +2,46 @@
 
 require 'spec_helper'
 
+# This is temporary while https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45733 gets rolled out.
+# TODO: clean this up once the MR has been merged.
+RSpec.describe Gitlab::Experimentation::EXPERIMENTS do
+  it 'temporarily ensures we know what experiments exist for backwards compatibility' do
+    known_experiments = [
+      :signup_flow,
+      :onboarding_issues,
+      :ci_notification_dot,
+      :upgrade_link_in_user_menu_a,
+      :invite_members_version_a,
+      :invite_members_version_b,
+      :invite_members_empty_group_version_a,
+      :new_create_project_ui,
+      :contact_sales_btn_in_app,
+      :customize_homepage,
+      :invite_email,
+      :invitation_reminders,
+      :group_only_trials,
+      :default_to_issues_board
+    ]
+
+    expect(described_class.keys).to match(known_experiments)
+  end
+end
+
 RSpec.describe Gitlab::Experimentation, :snowplow do
   before do
     stub_const('Gitlab::Experimentation::EXPERIMENTS', {
+      backwards_compatible_test_experiment: {
+        environment: environment,
+        tracking_category: 'Team',
+        use_backwards_compatible_subject_index: true
+      },
       test_experiment: {
         environment: environment,
         tracking_category: 'Team'
       }
     })
 
+    Feature.enable_percentage_of_time(:backwards_compatible_test_experiment_experiment_percentage, enabled_percentage)
     Feature.enable_percentage_of_time(:test_experiment_experiment_percentage, enabled_percentage)
   end
 
@@ -84,25 +115,37 @@ RSpec.describe Gitlab::Experimentation, :snowplow do
     end
 
     describe '#experiment_enabled?' do
-      subject { controller.experiment_enabled?(:test_experiment) }
+      def check_experiment(exp_key = :test_experiment)
+        controller.experiment_enabled?(exp_key)
+      end
+
+      subject { check_experiment }
 
       context 'cookie is not present' do
         it 'calls Gitlab::Experimentation.enabled_for_value? with the name of the experiment and an experimentation_subject_index of nil' do
           expect(Gitlab::Experimentation).to receive(:enabled_for_value?).with(:test_experiment, nil)
-          controller.experiment_enabled?(:test_experiment)
+          check_experiment
         end
       end
 
       context 'cookie is present' do
+        using RSpec::Parameterized::TableSyntax
+
         before do
           cookies.permanent.signed[:experimentation_subject_id] = 'abcd-1234'
           get :index
         end
 
-        it 'calls Gitlab::Experimentation.enabled_for_value? with the name of the experiment and an experimentation_subject_index of the modulo 100 of the hex value of the uuid' do
-          # 'abcd1234'.hex % 100 = 76
-          expect(Gitlab::Experimentation).to receive(:enabled_for_value?).with(:test_experiment, 76)
-          controller.experiment_enabled?(:test_experiment)
+        where(:experiment_key, :index_value) do
+          :test_experiment | 40 # Zlib.crc32('test_experimentabcd-1234') % 100 = 40
+          :backwards_compatible_test_experiment | 76 # 'abcd1234'.hex % 100 = 76
+        end
+
+        with_them do
+          it 'calls Gitlab::Experimentation.enabled_for_value? with the name of the experiment and the calculated experimentation_subject_index based on the uuid' do
+            expect(Gitlab::Experimentation).to receive(:enabled_for_value?).with(experiment_key, index_value)
+            check_experiment(experiment_key)
+          end
         end
       end