Merge branch 'jejacks0n/update-gitlab-experiment-0.6.1' into 'master'

Update gitlab-experiment to 0.6.1

See merge request gitlab-org/gitlab!64551

Gemfile

@@ -496,7 +496,7 @@ gem 'flipper', '~> 0.21.0'
 gem 'flipper-active_record', '~> 0.21.0'
 gem 'flipper-active_support_cache_store', '~> 0.21.0'
 gem 'unleash', '~> 0.1.5'
-gem 'gitlab-experiment', '~> 0.5.4'
+gem 'gitlab-experiment', '~> 0.6.1'
 
 # Structured logging
 gem 'lograge', '~> 0.5'

Gemfile.lock

@@ -470,7 +470,7 @@ GEM
       numerizer (~> 0.2)
     gitlab-dangerfiles (2.1.2)
       danger-gitlab
-    gitlab-experiment (0.5.4)
+    gitlab-experiment (0.6.1)
       activesupport (>= 3.0)
       request_store (>= 1.0)
       scientist (~> 1.6, >= 1.6.0)
@@ -1487,7 +1487,7 @@ DEPENDENCIES
   github-markup (~> 1.7.0)
   gitlab-chronic (~> 0.10.5)
   gitlab-dangerfiles (~> 2.1.2)
-  gitlab-experiment (~> 0.5.4)
+  gitlab-experiment (~> 0.6.1)
   gitlab-fog-azure-rm (~> 1.1.1)
   gitlab-labkit (~> 0.18.0)
   gitlab-license (~> 1.5)

app/experiments/application_experiment.rb

@@ -10,22 +10,28 @@ class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/Namesp
   end
 
   def publish(_result = nil)
-    return unless should_track? # don't track events for excluded contexts
-
-    record_experiment if @record # record the subject in the database if the context contains a namespace, group, project, actor or user
-
-    track(:assignment) # track that we've assigned a variant for this context
+    super
 
-    push_to_client
+    publish_to_client if should_track? # publish the experiment data to the client
+    publish_to_database if @record # publish the experiment context to the database
   end
 
-  # push the experiment data to the client
-  def push_to_client
+  def publish_to_client
     Gon.push({ experiment: { name => signature } }, true)
   rescue NoMethodError
     # means we're not in the request cycle, and can't add to Gon. Log a warning maybe?
   end
 
+  def publish_to_database
+    # if the context contains a namespace, group, project, user, or actor
+    value = context.value
+    subject = value[:namespace] || value[:group] || value[:project] || value[:user] || value[:actor]
+    return unless ExperimentSubject.valid_subject?(subject)
+
+    variant = :experimental if @variant_name != :control
+    Experiment.add_subject(name, variant: variant || :control, subject: subject)
+  end
+
   def track(action, **event_args)
     return unless should_track? # don't track events for excluded contexts
 
@@ -41,14 +47,23 @@ class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/Namesp
     @record = true
   end
 
-  def exclude!
-    @excluded = true
-  end
-
   def control_behavior
     # define a default nil control behavior so we can omit it when not needed
   end
 
+  # TODO: remove
+  # This is deprecated logic as of v0.6.0 and should eventually be removed, but
+  # needs to stay intact for actively running experiments. The new strategy
+  # utilizes Digest::SHA2, a secret seed, and generates a 64-byte string.
+  def key_for(source, seed = name)
+    source = source.keys + source.values if source.is_a?(Hash)
+
+    ingredients = Array(source).map { |v| identify(v) }
+    ingredients.unshift(seed)
+
+    Digest::MD5.hexdigest(ingredients.join('|'))
+  end
+
   private
 
   def feature_flag_name
@@ -58,13 +73,4 @@ class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/Namesp
   def experiment_group?
     Feature.enabled?(feature_flag_name, self, type: :experiment, default_enabled: :yaml)
   end
-
-  def record_experiment
-    subject = context.value[:namespace] || context.value[:group] || context.value[:project] || context.value[:user] || context.value[:actor]
-    return unless ExperimentSubject.valid_subject?(subject)
-
-    variant = :experimental if @variant_name != :control
-
-    Experiment.add_subject(name, variant: variant || :control, subject: subject)
-  end
 end

config/feature_flags/development/gitlab_experiment_middleware.yml

+---
+name: gitlab_experiment_middleware
+introduced_by_url:
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/323643
+milestone: '14.1'
+type: development
+group: group::adoption
+default_enabled: false

config/initializers/gitlab_experiment.rb

@@ -2,16 +2,22 @@
 
 Gitlab::Experiment.configure do |config|
   config.base_class = 'ApplicationExperiment'
+  config.mount_at = '/-/experiment'
   config.cache = Gitlab::Experiment::Cache::RedisHashStore.new(
     pool: ->(&block) { Gitlab::Redis::SharedState.with { |redis| block.call(redis) } }
   )
+end
+
+# TODO: This shim should be removed after the feature flag is rolled out, as
+#   it only exists to facilitate the feature flag control of the behavior.
+module Gitlab::Experiment::MiddlewareWithFeatureFlags
+  attr_reader :app
 
-  # TODO: This will be deprecated as of v0.6.0, but needs to stay intact for
-  #  actively running experiments until a versioning concept is put in place to
-  #  enable migrating into the new SHA2 strategy.
-  config.context_hash_strategy = lambda do |source, seed|
-    source = source.keys + source.values if source.is_a?(Hash)
-    data = Array(source).map { |v| (v.respond_to?(:to_global_id) ? v.to_global_id : v).to_s }
-    Digest::MD5.hexdigest(data.unshift(seed).join('|'))
+  def call(env)
+    return app.call(env) unless Feature.enabled?(:gitlab_experiment_middleware)
+
+    super
   end
 end
+
+Gitlab::Experiment::Middleware.prepend(Gitlab::Experiment::MiddlewareWithFeatureFlags)

spec/experiments/application_experiment_spec.rb

@@ -3,11 +3,10 @@
 require 'spec_helper'
 
 RSpec.describe ApplicationExperiment, :experiment do
-  subject { described_class.new('namespaced/stub') }
+  subject { described_class.new('namespaced/stub', **context) }
 
-  let(:feature_definition) do
-    { name: 'namespaced_stub', type: 'experiment', group: 'group::adoption', default_enabled: false }
-  end
+  let(:context) { {} }
+  let(:feature_definition) { { name: 'namespaced_stub', type: 'experiment', default_enabled: false } }
 
   around do |example|
     Feature::Definition.definitions[:namespaced_stub] = Feature::Definition.new('namespaced_stub.yml', feature_definition)
@@ -25,7 +24,7 @@ RSpec.describe ApplicationExperiment, :experiment do
     expect { experiment('namespaced/stub') { } }.not_to raise_error
   end
 
-  describe "enabled" do
+  describe "#enabled?" do
     before do
       allow(subject).to receive(:enabled?).and_call_original
 
@@ -57,103 +56,100 @@ RSpec.describe ApplicationExperiment, :experiment do
     end
   end
 
-  describe "publishing results" do
-    it "doesn't record, track or push data to the client if we shouldn't track", :snowplow do
+  describe "#publish" do
+    it "doesn't track or publish to the client or database if we can't track", :snowplow do
       allow(subject).to receive(:should_track?).and_return(false)
-      subject.record!
 
-      expect(subject).not_to receive(:record_experiment)
-      expect(subject).not_to receive(:track)
-      expect(Gon).not_to receive(:push)
+      expect(subject).not_to receive(:publish_to_client)
+      expect(subject).not_to receive(:publish_to_database)
 
-      subject.publish(:action)
+      subject.publish
 
       expect_no_snowplow_event
     end
 
-    describe 'recording the experiment' do
-      it 'does not record the experiment if we do not tell it to' do
-        expect(subject).not_to receive(:record_experiment)
-
-        subject.publish
-      end
-
-      it 'records the experiment if we tell it to' do
-        subject.record!
-
-        expect(subject).to receive(:record_experiment)
-
-        subject.publish
-      end
-    end
-
     it "tracks the assignment" do
       expect(subject).to receive(:track).with(:assignment)
 
       subject.publish
     end
 
-    it "pushes the experiment knowledge into the client using Gon" do
-      expect(Gon).to receive(:push).with({ experiment: { 'namespaced/stub' => subject.signature } }, true)
+    it "publishes the to the client" do
+      expect(subject).to receive(:publish_to_client)
 
       subject.publish
     end
 
-    it "handles when Gon raises exceptions (like when it can't be pushed into)" do
-      expect(Gon).to receive(:push).and_raise(NoMethodError)
+    it "publishes to the database if we've opted for that" do
+      subject.record!
+
+      expect(subject).to receive(:publish_to_database)
 
-      expect { subject.publish }.not_to raise_error
+      subject.publish
     end
-  end
 
-  it "can exclude from within the block" do
-    expect(described_class.new('namespaced/stub') { |e| e.exclude! }).to be_excluded
-  end
+    describe "#publish_to_client" do
+      it "adds the data into Gon" do
+        signature = { key: '86208ac54ca798e11f127e8b23ec396a', variant: 'control' }
+        expect(Gon).to receive(:push).with({ experiment: { 'namespaced/stub' => hash_including(signature) } }, true)
 
-  describe 'recording the experiment subject' do
-    using RSpec::Parameterized::TableSyntax
+        subject.publish_to_client
+      end
 
-    subject { described_class.new('namespaced/stub', nil, **context) }
+      it "handles when Gon raises exceptions (like when it can't be pushed into)" do
+        expect(Gon).to receive(:push).and_raise(NoMethodError)
 
-    before do
-      subject.record!
+        expect { subject.publish_to_client }.not_to raise_error
+      end
     end
 
-    context 'when providing a compatible context' do
-      where(:context_key, :object_type) do
-        :namespace | :namespace
-        :group     | :namespace
-        :project   | :project
-        :user      | :user
-        :actor     | :user
+    describe "#publish_to_database" do
+      using RSpec::Parameterized::TableSyntax
+      let(:context) { { context_key => context_value }}
+
+      before do
+        subject.record!
       end
 
-      with_them do
-        let(:context) { { context_key => build(object_type) }}
+      context "when there's a usable subject" do
+        where(:context_key, :context_value, :object_type) do
+          :namespace | build(:namespace) | :namespace
+          :group     | build(:namespace) | :namespace
+          :project   | build(:project)   | :project
+          :user      | build(:user)      | :user
+          :actor     | build(:user)      | :user
+        end
 
-        it 'records the experiment and the experiment subject from the context' do
-          expect { subject.publish }.to change(Experiment, :count).by(1)
+        with_them do
+          it "creates an experiment and experiment subject record" do
+            expect { subject.publish_to_database }.to change(Experiment, :count).by(1)
 
-          expect(Experiment.last.name).to eq('namespaced/stub')
-          expect(ExperimentSubject.last.send(object_type)).to eq(context[context_key])
+            expect(Experiment.last.name).to eq('namespaced/stub')
+            expect(ExperimentSubject.last.send(object_type)).to eq(context[context_key])
+          end
         end
       end
-    end
 
-    context 'when providing an incompatible or no context' do
-      where(context_hash: [{ foo: :bar }, {}])
+      context "when there's not a usable subject" do
+        where(:context_key, :context_value) do
+          :namespace | nil
+          :foo       | :bar
+        end
 
-      with_them do
-        let(:context) { context_hash }
+        with_them do
+          it "doesn't create an experiment record" do
+            expect { subject.publish_to_database }.not_to change(Experiment, :count)
+          end
 
-        it 'does not record the experiment' do
-          expect { subject.publish }.not_to change(Experiment, :count)
+          it "doesn't create an experiment subject record" do
+            expect { subject.publish_to_database }.not_to change(ExperimentSubject, :count)
+          end
         end
       end
     end
   end
 
-  describe "tracking events", :snowplow do
+  describe "#track", :snowplow do
     it "doesn't track if we shouldn't track" do
       allow(subject).to receive(:should_track?).and_return(false)
 
@@ -185,7 +181,13 @@ RSpec.describe ApplicationExperiment, :experiment do
     end
   end
 
-  describe "variant resolution" do
+  describe "#key_for" do
+    it "generates MD5 hashes" do
+      expect(subject.key_for(foo: :bar)).to eq('6f9ac12afdb9b58c2f19a136d09f9153')
+    end
+  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', subject, type: :experiment, default_enabled: :yaml)