Merge branch 'jejacks0n/maintenance/add-feature-rollout-strategy' into 'master'

Provide an experiment rollout strategy based on Feature

See merge request gitlab-org/gitlab!79887

app/experiments/application_experiment.rb

 # frozen_string_literal: true
 
 class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/NamespacedClass
-  def enabled?
-    return false if Feature::Definition.get(feature_flag_name).nil? # there has to be a feature flag yaml file
-    return false unless Gitlab.dev_env_or_com? # we have to be in an environment that allows experiments
-
-    # the feature flag has to be rolled out
-    Feature.get(feature_flag_name).state != :off # rubocop:disable Gitlab/AvoidFeatureGet
-  end
-
   def publish(_result = nil)
     super
 
@@ -72,12 +64,4 @@ class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/Namesp
     actor = context.try(:actor)
     actor.respond_to?(:id) ? actor : context.try(:user)
   end
-
-  def feature_flag_name
-    name.tr('/', '_')
-  end
-
-  def experiment_group?
-    Feature.enabled?(feature_flag_name, self, type: :experiment, default_enabled: :yaml)
-  end
 end

config/initializers/gitlab_experiment.rb

@@ -23,9 +23,7 @@ Gitlab::Experiment.configure do |config|
   # Customize the logic of our default rollout, which shouldn't include
   # assigning the control yet -- we specifically set it to false for now.
   #
-  config.default_rollout = Gitlab::Experiment::Rollout::Percent.new(
-    include_control: false
-  )
+  config.default_rollout = Gitlab::Experiment::Rollout::Feature.new
 
   # Mount the engine and middleware at a gitlab friendly style path.
   #

lib/gitlab/experiment/rollout/feature.rb

+# frozen_string_literal: true
+
+module Gitlab
+  class Experiment
+    module Rollout
+      class Feature < Percent
+        # For this rollout strategy to consider an experiment as enabled, we
+        # must:
+        #
+        # - have a feature flag yaml file that declares it.
+        # - be in an environment that permits it.
+        # - not have rolled out the feature flag at all (no percent of actors,
+        #   no inclusions, etc.)
+        def enabled?
+          return false if ::Feature::Definition.get(feature_flag_name).nil?
+          return false unless Gitlab.dev_env_or_com?
+
+          ::Feature.get(feature_flag_name).state != :off # rubocop:disable Gitlab/AvoidFeatureGet
+        end
+
+        # For assignment we first check to see if our feature flag is enabled
+        # for "self". This is done by calling `#flipper_id` (used behind the
+        # scenes by `Feature`). By default this is our `experiment.id` (or more
+        # specifically, the context key, which is an anonymous SHA generated
+        # using the details of an experiment.
+        #
+        # If the `Feature.enabled?` check is false, we return nil implicitly,
+        # which will assign the control. Otherwise we call super, which will
+        # assign a variant evenly, or based on our provided distribution rules.
+        def execute_assigment
+          super if ::Feature.enabled?(feature_flag_name, self, type: :experiment, default_enabled: :yaml)
+        end
+
+        # NOTE: There's a typo in the name of this method that we'll fix up.
+        alias_method :execute_assignment, :execute_assigment
+
+        # This is what's provided to the `Feature.enabled?` call that will be
+        # used to determine experiment inclusion. An experiment may provide an
+        # override for this method to make the experiment work on user, group,
+        # or projects.
+        #
+        # For example, when running an experiment on a project, you could make
+        # the experiment assignable by project (using chatops) by implementing
+        # a `flipper_id` method in the experiment:
+        #
+        # def flipper_id
+        #   context.project.flipper_id
+        # end
+        #
+        # Or even cleaner, simply delegate it:
+        #
+        # delegate :flipper_id, to: -> { context.project }
+        def flipper_id
+          return experiment.flipper_id if experiment.respond_to?(:flipper_id)
+
+          "Experiment;#{id}"
+        end
+
+        private
+
+        def feature_flag_name
+          experiment.name.tr('/', '_')
+        end
+      end
+    end
+  end
+end

spec/experiments/application_experiment_spec.rb

@@ -24,38 +24,6 @@ RSpec.describe ApplicationExperiment, :experiment do
     expect { experiment('namespaced/stub') { } }.not_to raise_error
   end
 
-  describe "#enabled?" do
-    before do
-      allow(application_experiment).to receive(:enabled?).and_call_original
-
-      allow(Feature::Definition).to receive(:get).and_return('_instance_')
-      allow(Gitlab).to receive(:dev_env_or_com?).and_return(true)
-      allow(Feature).to receive(:get).and_return(double(state: :on))
-    end
-
-    it "is enabled when all criteria are met" do
-      expect(application_experiment).to be_enabled
-    end
-
-    it "isn't enabled if the feature definition doesn't exist" do
-      expect(Feature::Definition).to receive(:get).with('namespaced_stub').and_return(nil)
-
-      expect(application_experiment).not_to be_enabled
-    end
-
-    it "isn't enabled if we're not in dev or dotcom environments" do
-      expect(Gitlab).to receive(:dev_env_or_com?).and_return(false)
-
-      expect(application_experiment).not_to be_enabled
-    end
-
-    it "isn't enabled if the feature flag state is :off" do
-      expect(Feature).to receive(:get).with('namespaced_stub').and_return(double(state: :off))
-
-      expect(application_experiment).not_to be_enabled
-    end
-  end
-
   describe "#publish" do
     let(:should_track) { true }
 
@@ -214,26 +182,6 @@ RSpec.describe ApplicationExperiment, :experiment do
       )
     end
 
-    it "tracks the event correctly even when using the base class" do
-      subject = Gitlab::Experiment.new(:unnamed)
-      subject.track(:action, context: [fake_context])
-
-      expect_snowplow_event(
-        category: 'unnamed',
-        action: 'action',
-        context: [
-          {
-            schema: 'iglu:com.gitlab/fake/jsonschema/0-0-0',
-            data: { data: '_data_' }
-          },
-          {
-            schema: 'iglu:com.gitlab/gitlab_experiment/jsonschema/1-0-0',
-            data: { experiment: 'unnamed', key: subject.context.key, variant: 'control' }
-          }
-        ]
-      )
-    end
-
     context "when using known context resources" do
       let(:user) { build(:user, id: non_existing_record_id) }
       let(:project) { build(:project, id: non_existing_record_id) }
@@ -347,13 +295,6 @@ RSpec.describe ApplicationExperiment, :experiment do
   end
 
   context "when resolving variants" do
-    it "uses the default value as specified in the yaml" do
-      expect(Feature).to receive(:enabled?).with('namespaced_stub', application_experiment, type: :experiment, default_enabled: :yaml)
-
-      expect(application_experiment.variant.name).to eq('control')
-    end
-
-    context "when rolled out to 100%" do
     before do
       stub_feature_flags(namespaced_stub: true)
     end
@@ -362,8 +303,7 @@ RSpec.describe ApplicationExperiment, :experiment do
       application_experiment.variant(:variant1) {}
       application_experiment.variant(:variant2) {}
 
-        expect(application_experiment.variant.name).to eq('variant2')
-      end
+      expect(application_experiment.assigned.name).to eq('variant2')
     end
   end
 

spec/lib/gitlab/experiment/rollout/feature_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Experiment::Rollout::Feature, :experiment do
+  subject { described_class.new.for(subject_experiment) }
+
+  let(:subject_experiment) { experiment('namespaced/stub') }
+
+  describe "#enabled?" do
+    before do
+      allow(Feature::Definition).to receive(:get).and_return('_instance_')
+      allow(Gitlab).to receive(:dev_env_or_com?).and_return(true)
+      allow(Feature).to receive(:get).and_return(double(state: :on))
+    end
+
+    it "is enabled when all criteria are met" do
+      expect(subject).to be_enabled
+    end
+
+    it "isn't enabled if the feature definition doesn't exist" do
+      expect(Feature::Definition).to receive(:get).with('namespaced_stub').and_return(nil)
+
+      expect(subject).not_to be_enabled
+    end
+
+    it "isn't enabled if we're not in dev or dotcom environments" do
+      expect(Gitlab).to receive(:dev_env_or_com?).and_return(false)
+
+      expect(subject).not_to be_enabled
+    end
+
+    it "isn't enabled if the feature flag state is :off" do
+      expect(Feature).to receive(:get).with('namespaced_stub').and_return(double(state: :off))
+
+      expect(subject).not_to be_enabled
+    end
+  end
+
+  describe "#execute_assignment" do
+    before do
+      allow(Feature).to receive(:enabled?).with('namespaced_stub', any_args).and_return(true)
+    end
+
+    it "uses the default value as specified in the yaml" do
+      expect(Feature).to receive(:enabled?).with(
+        'namespaced_stub',
+        subject,
+        type: :experiment,
+        default_enabled: :yaml
+      ).and_return(false)
+
+      expect(subject.execute_assignment).to be_nil
+    end
+
+    it "returns an assigned name" do
+      allow(subject).to receive(:behavior_names).and_return([:variant1, :variant2])
+
+      expect(subject.execute_assignment).to eq(:variant2)
+    end
+  end
+
+  describe "#flipper_id" do
+    it "returns the expected flipper id if the experiment doesn't provide one" do
+      subject.instance_variable_set(:@experiment, double(id: '__id__'))
+      expect(subject.flipper_id).to eq('Experiment;__id__')
+    end
+
+    it "lets the experiment provide a flipper id so it can override the default" do
+      allow(subject_experiment).to receive(:flipper_id).and_return('_my_overridden_id_')
+
+      expect(subject.flipper_id).to eq('_my_overridden_id_')
+    end
+  end
+end