Merge branch '338734-remove-experiment-backwards-compatible-index-capability' into 'master'

Remove experiment backwards compatible index capability

See merge request gitlab-org/gitlab!68489

lib/gitlab/experimentation.rb

@@ -5,12 +5,8 @@
 # Utility module for A/B testing experimental features. Define your experiments in the `EXPERIMENTS` constant.
 # 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.
-#
 # 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.
 #
@@ -118,11 +114,7 @@ module Gitlab
       private
 
       def index_for_subject(experiment, subject)
-        index = if experiment.use_backwards_compatible_subject_index
-                  Digest::SHA1.hexdigest(subject_id(subject)).hex
-                else
-                  Zlib.crc32("#{experiment.key}#{subject_id(subject)}")
-                end
+        index = Zlib.crc32("#{experiment.key}#{subject_id(subject)}")
 
         index % 100
       end

lib/gitlab/experimentation/controller_concern.rb

@@ -48,7 +48,7 @@ module Gitlab
 
         Experimentation.log_invalid_rollout(experiment_key, subject)
 
-        subject ||= fallback_experimentation_subject_index(experiment_key)
+        subject ||= experimentation_subject_id
 
         Experimentation.in_experiment_group?(experiment_key, subject: subject)
       end
@@ -106,16 +106,6 @@ module Gitlab
         cookies.signed[:experimentation_subject_id]
       end
 
-      def fallback_experimentation_subject_index(experiment_key)
-        return if experimentation_subject_id.blank?
-
-        if Experimentation.get_experiment(experiment_key).use_backwards_compatible_subject_index
-          experimentation_subject_id.delete('-')
-        else
-          experimentation_subject_id
-        end
-      end
-
       def track_experiment_event_for(experiment_key, action, value, subject: nil)
         return unless Experimentation.active?(experiment_key)
 
@@ -139,7 +129,7 @@ module Gitlab
       def tracking_group(experiment_key, suffix = nil, subject: nil)
         return unless Experimentation.active?(experiment_key)
 
-        subject ||= fallback_experimentation_subject_index(experiment_key)
+        subject ||= experimentation_subject_id
         group = experiment_enabled?(experiment_key, subject: subject) ? GROUP_EXPERIMENTAL : GROUP_CONTROL
 
         suffix ? "#{group}#{suffix}" : group

lib/gitlab/experimentation/experiment.rb

@@ -5,12 +5,11 @@ module Gitlab
     class Experiment
       FEATURE_FLAG_SUFFIX = "_experiment_percentage"
 
-      attr_reader :key, :tracking_category, :use_backwards_compatible_subject_index, :rollout_strategy
+      attr_reader :key, :tracking_category, :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
 

spec/lib/gitlab/experimentation/controller_concern_spec.rb

@@ -7,10 +7,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
 
   before do
     stub_const('Gitlab::Experimentation::EXPERIMENTS', {
-        backwards_compatible_test_experiment: {
-          tracking_category: 'Team',
-          use_backwards_compatible_subject_index: true
-        },
         test_experiment: {
           tracking_category: 'Team',
           rollout_strategy: rollout_strategy
@@ -23,7 +19,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
 
     allow(Gitlab).to receive(:dev_env_or_com?).and_return(is_gitlab_com)
 
-    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
 
@@ -124,24 +119,15 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
     end
 
     context 'cookie is present' do
-      using RSpec::Parameterized::TableSyntax
-
       before do
         cookies.permanent.signed[:experimentation_subject_id] = 'abcd-1234'
         get :index
       end
 
-      where(:experiment_key, :index_value) do
-        :test_experiment | 'abcd-1234'
-        :backwards_compatible_test_experiment | 'abcd1234'
-      end
-
-      with_them do
-        it 'calls Gitlab::Experimentation.in_experiment_group?? with the name of the experiment and the calculated experimentation_subject_index based on the uuid' do
-          expect(Gitlab::Experimentation).to receive(:in_experiment_group?).with(experiment_key, subject: index_value)
+      it 'calls Gitlab::Experimentation.in_experiment_group? with the name of the experiment and the calculated experimentation_subject_index based on the uuid' do
+        expect(Gitlab::Experimentation).to receive(:in_experiment_group?).with(:test_experiment, subject: 'abcd-1234')
 
-          check_experiment(experiment_key)
-        end
+        check_experiment(:test_experiment)
       end
 
       context 'when subject is given' do

spec/lib/gitlab/experimentation/experiment_spec.rb

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

spec/lib/gitlab/experimentation_spec.rb

@@ -7,10 +7,6 @@ RSpec.describe Gitlab::Experimentation do
 
   before do
     stub_const('Gitlab::Experimentation::EXPERIMENTS', {
-      backwards_compatible_test_experiment: {
-        tracking_category: 'Team',
-        use_backwards_compatible_subject_index: true
-      },
       test_experiment: {
         tracking_category: 'Team'
       },
@@ -22,7 +18,6 @@ RSpec.describe Gitlab::Experimentation do
 
     skip_feature_flags_yaml_validation
     skip_default_enabled_yaml_check
-    Feature.enable_percentage_of_time(:backwards_compatible_test_experiment_experiment_percentage, enabled_percentage)
     Feature.enable_percentage_of_time(:test_experiment_experiment_percentage, enabled_percentage)
     allow(Gitlab).to receive(:com?).and_return(true)
   end
@@ -65,97 +60,47 @@ RSpec.describe Gitlab::Experimentation do
   end
 
   describe '.in_experiment_group?' do
-    context 'with new index calculation' do
-      let(:enabled_percentage) { 50 }
-      let(:experiment_subject) { 'z' } # Zlib.crc32('test_experimentz') % 100 = 33
-
-      subject { described_class.in_experiment_group?(:test_experiment, subject: experiment_subject) }
-
-      context 'when experiment is active' do
-        context 'when subject is part of the experiment' do
-          it { is_expected.to eq(true) }
-        end
+    let(:enabled_percentage) { 50 }
+    let(:experiment_subject) { 'z' } # Zlib.crc32('test_experimentz') % 100 = 33
 
-        context 'when subject is not part of the experiment' do
-          let(:experiment_subject) { 'a' } # Zlib.crc32('test_experimenta') % 100 = 61
+    subject { described_class.in_experiment_group?(:test_experiment, subject: experiment_subject) }
 
-          it { is_expected.to eq(false) }
-        end
+    context 'when experiment is active' do
+      context 'when subject is part of the experiment' do
+        it { is_expected.to eq(true) }
+      end
 
-        context 'when subject has a global_id' do
-          let(:experiment_subject) { double(:subject, to_global_id: 'z') }
+      context 'when subject is not part of the experiment' do
+        let(:experiment_subject) { 'a' } # Zlib.crc32('test_experimenta') % 100 = 61
 
-          it { is_expected.to eq(true) }
-        end
+        it { is_expected.to eq(false) }
+      end
 
-        context 'when subject is nil' do
-          let(:experiment_subject) { nil }
+      context 'when subject has a global_id' do
+        let(:experiment_subject) { double(:subject, to_global_id: 'z') }
 
-          it { is_expected.to eq(false) }
-        end
+        it { is_expected.to eq(true) }
+      end
 
-        context 'when subject is an empty string' do
-          let(:experiment_subject) { '' }
+      context 'when subject is nil' do
+        let(:experiment_subject) { nil }
 
-          it { is_expected.to eq(false) }
-        end
+        it { is_expected.to eq(false) }
       end
 
-      context 'when experiment is not active' do
-        before do
-          allow(described_class).to receive(:active?).and_return(false)
-        end
+      context 'when subject is an empty string' do
+        let(:experiment_subject) { '' }
 
         it { is_expected.to eq(false) }
       end
     end
 
-    context 'with backwards compatible index calculation' do
-      let(:experiment_subject) { 'abcd' } # Digest::SHA1.hexdigest('abcd').hex % 100 = 7
-
-      subject { described_class.in_experiment_group?(:backwards_compatible_test_experiment, subject: experiment_subject) }
-
-      context 'when experiment is active' do
-        before do
-          allow(described_class).to receive(:active?).and_return(true)
-        end
-
-        context 'when subject is part of the experiment' do
-          it { is_expected.to eq(true) }
-        end
-
-        context 'when subject is not part of the experiment' do
-          let(:experiment_subject) { 'abc' } # Digest::SHA1.hexdigest('abc').hex % 100 = 17
-
-          it { is_expected.to eq(false) }
-        end
-
-        context 'when subject has a global_id' do
-          let(:experiment_subject) { double(:subject, to_global_id: 'abcd') }
-
-          it { is_expected.to eq(true) }
-        end
-
-        context 'when subject is nil' do
-          let(:experiment_subject) { nil }
-
-          it { is_expected.to eq(false) }
-        end
-
-        context 'when subject is an empty string' do
-          let(:experiment_subject) { '' }
-
-          it { is_expected.to eq(false) }
-        end
+    context 'when experiment is not active' do
+      before do
+        allow(described_class).to receive(:active?).and_return(false)
       end
 
-      context 'when experiment is not active' do
-        before do
-          allow(described_class).to receive(:active?).and_return(false)
-        end
-
-        it { is_expected.to eq(false) }
-      end
+      it { is_expected.to eq(false) }
     end
   end