Merge branch '294154-use-experiments-tables-with-glex' into 'master'

Record experiment subjects in the database for GLEX experiments

See merge request gitlab-org/gitlab!61914

app/controllers/projects/pipelines_controller.rb

@@ -49,14 +49,14 @@ class Projects::PipelinesController < Projects::ApplicationController
 
     respond_to do |format|
       format.html do
-        experiment(:pipeline_empty_state_templates, actor: current_user) do |e|
+        experiment(:pipeline_empty_state_templates, namespace: project.root_ancestor) do |e|
           e.exclude! unless current_user
           e.exclude! if @pipelines_count.to_i > 0
           e.exclude! if helpers.has_gitlab_ci?(project)
 
           e.use {}
           e.try {}
-          e.track(:view, value: project.namespace_id)
+          e.record!
         end
         experiment(:code_quality_walkthrough, namespace: project.root_ancestor) do |e|
           e.exclude! unless current_user
@@ -67,7 +67,7 @@ class Projects::PipelinesController < Projects::ApplicationController
 
           e.use {}
           e.try {}
-          e.track(:view, property: project.root_ancestor.id.to_s)
+          e.record!
         end
       end
       format.json do

app/experiments/application_experiment.rb

@@ -12,6 +12,8 @@ class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/Namesp
   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
 
     begin
@@ -32,6 +34,10 @@ class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/Namesp
     ))
   end
 
+  def record!
+    @record = true
+  end
+
   def exclude!
     @excluded = true
   end
@@ -49,4 +55,13 @@ 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 subject.is_a?(Group) || subject.is_a?(User) || subject.is_a?(Project)
+
+    variant = :experimental if @variant_name != :control
+
+    Experiment.add_subject(name, variant: variant || :control, subject: subject)
+  end
 end

app/models/experiment.rb

@@ -11,7 +11,11 @@ class Experiment < ApplicationRecord
   end
 
   def self.add_group(name, variant:, group:)
-    find_or_create_by!(name: name).record_group_and_variant!(group, variant)
+    add_subject(name, variant: variant, subject: group)
+  end
+
+  def self.add_subject(name, variant:, subject:)
+    find_or_create_by!(name: name).record_subject_and_variant!(subject, variant)
   end
 
   def self.record_conversion_event(name, user, context = {})
@@ -37,8 +41,11 @@ class Experiment < ApplicationRecord
     experiment_user.update!(converted_at: Time.current, context: merged_context(experiment_user, context))
   end
 
-  def record_group_and_variant!(group, variant)
-    experiment_subject = experiment_subjects.find_or_initialize_by(group: group)
+  def record_subject_and_variant!(subject, variant)
+    subject = subject.owner if subject.is_a?(Namespace) && subject.user?
+    raise 'Incompatible subject provided!' unless subject.is_a?(Group) || subject.is_a?(User) || subject.is_a?(Project)
+
+    experiment_subject = experiment_subjects.find_or_initialize_by(subject.class.name.downcase => subject)
     experiment_subject.assign_attributes(variant: variant)
     # We only call save when necessary because this causes the request to stick to the primary DB
     # even when the save is a no-op

doc/development/experiment_guide/gitlab_experiment.md

@@ -394,6 +394,26 @@ You may be asked from time to time to track a specific record ID in experiments.
 The approach is largely up to the PM and engineer creating the implementation.
 No recommendations are provided here at this time.
 
+### Record experiment subjects
+
+Snowplow tracking of identifiable users or groups is prohibited, but you can still
+determine if an experiment is successful or not. We're allowed to record the ID of
+a group, project or user in our database. Therefore, we can tell the experiment
+to record their ID together with the assigned experiment variant in the
+`experiment_subjects` database table for later analysis.
+
+For the recording to work, the experiment's context must include a `namespace`,
+`group`, `project`, `user`, or `actor`.
+
+To record the experiment subject when you first assign a variant, call `record!` in
+the experiment's block:
+
+```ruby
+experiment(:pill_color, actor: current_user) do |e|
+  e.record!
+end
+```
+
 ## Test with RSpec
 
 This gem provides some RSpec helpers and custom matchers. These are in flux as of GitLab 13.10.

spec/controllers/projects/pipelines_controller_spec.rb

@@ -274,15 +274,15 @@ RSpec.describe Projects::PipelinesController do
   end
 
   describe 'GET #index' do
-    context 'pipeline_empty_state_templates experiment' do
-      before do
-        stub_application_setting(auto_devops_enabled: false)
-      end
+    before do
+      stub_application_setting(auto_devops_enabled: false)
+    end
 
-      it 'tracks the view', :experiment do
+    context 'pipeline_empty_state_templates experiment' do
+      it 'tracks the assignment', :experiment do
         expect(experiment(:pipeline_empty_state_templates))
-          .to track(:view, value: project.namespace_id)
-          .with_context(actor: user)
+          .to track(:assignment)
+          .with_context(namespace: project.root_ancestor)
           .on_next_instance
 
         get :index, params: { namespace_id: project.namespace, project_id: project }
@@ -290,9 +290,9 @@ RSpec.describe Projects::PipelinesController do
     end
 
     context 'code_quality_walkthrough experiment' do
-      it 'tracks the view', :experiment do
+      it 'tracks the assignment', :experiment do
         expect(experiment(:code_quality_walkthrough))
-          .to track(:view, property: project.root_ancestor.id.to_s)
+          .to track(:assignment)
           .with_context(namespace: project.root_ancestor)
           .on_next_instance
 

spec/experiments/application_experiment_spec.rb

@@ -64,8 +64,12 @@ RSpec.describe ApplicationExperiment, :experiment do
   end
 
   describe "publishing results" do
-    it "doesn't track or push data to the client if we shouldn't track", :snowplow do
+    it "doesn't record, track or push data to the client if we shouldn'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)
 
       subject.publish(:action)
@@ -73,6 +77,22 @@ RSpec.describe ApplicationExperiment, :experiment do
       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)
 
@@ -96,6 +116,49 @@ RSpec.describe ApplicationExperiment, :experiment do
     expect(described_class.new('namespaced/stub') { |e| e.exclude! }).to be_excluded
   end
 
+  describe 'recording the experiment subject' do
+    using RSpec::Parameterized::TableSyntax
+
+    subject { described_class.new('namespaced/stub', nil, **context) }
+
+    before do
+      subject.record!
+    end
+
+    context 'when providing a compatible context' do
+      where(:context_key, :object_type) do
+        :namespace | :group
+        :group     | :group
+        :project   | :project
+        :user      | :user
+        :actor     | :user
+      end
+
+      with_them do
+        let(:context) { { context_key => build(object_type) }}
+
+        it 'records the experiment and the experiment subject from the context' do
+          expect { subject.publish }.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])
+        end
+      end
+    end
+
+    context 'when providing an incompatible or no context' do
+      where(context_hash: [{ foo: :bar }, {}])
+
+      with_them do
+        let(:context) { context_hash }
+
+        it 'does not record the experiment' do
+          expect { subject.publish }.not_to change(Experiment, :count)
+        end
+      end
+    end
+  end
+
   describe "tracking events", :snowplow do
     it "doesn't track if we shouldn't track" do
       allow(subject).to receive(:should_track?).and_return(false)

spec/models/experiment_spec.rb

@@ -79,7 +79,7 @@ RSpec.describe Experiment do
     context 'when an experiment with the provided name does not exist' do
       it 'creates a new experiment record' do
         allow_next(described_class, name: :experiment_key)
-          .to receive(:record_group_and_variant!).with(group, variant)
+          .to receive(:record_subject_and_variant!).with(group, variant)
 
         expect { add_group }.to change(described_class, :count).by(1)
       end
@@ -235,23 +235,23 @@ RSpec.describe Experiment do
     end
   end
 
-  describe '#record_group_and_variant!' do
-    let_it_be(:group) { create(:group) }
+  describe '#record_subject_and_variant!' do
+    let_it_be(:subject_to_record) { create(:group) }
     let_it_be(:variant) { :control }
     let_it_be(:experiment) { create(:experiment) }
 
-    subject(:record_group_and_variant!) { experiment.record_group_and_variant!(group, variant) }
+    subject(:record_subject_and_variant!) { experiment.record_subject_and_variant!(subject_to_record, variant) }
 
-    context 'when no existing experiment_subject record exists for the given group' do
+    context 'when no existing experiment_subject record exists for the given subject' do
       it 'creates an experiment_subject record' do
-        expect { record_group_and_variant! }.to change(ExperimentSubject, :count).by(1)
+        expect { record_subject_and_variant! }.to change(ExperimentSubject, :count).by(1)
         expect(ExperimentSubject.last.variant).to eq(variant.to_s)
       end
     end
 
-    context 'when an existing experiment_subject exists for the given group' do
+    context 'when an existing experiment_subject exists for the given subject' do
       let_it_be(:experiment_subject) do
-        create(:experiment_subject, experiment: experiment, group: group, user: nil, variant: :experimental)
+        create(:experiment_subject, experiment: experiment, group: subject_to_record, user: nil, variant: :experimental)
       end
 
       context 'when it belongs to the same variant' do
@@ -266,7 +266,55 @@ RSpec.describe Experiment do
 
       context 'but it belonged to a different variant' do
         it 'updates the variant value' do
-          expect { record_group_and_variant! }.to change { experiment_subject.reload.variant }.to('control')
+          expect { record_subject_and_variant! }.to change { experiment_subject.reload.variant }.to('control')
+        end
+      end
+    end
+
+    describe 'providing a subject to record' do
+      context 'when given a group as subject' do
+        it 'saves the namespace owner as experiment_subject user' do
+          expect(record_subject_and_variant!.group).to eq(subject_to_record)
+        end
+      end
+
+      context 'when given a users namespace as subject' do
+        let_it_be(:subject_to_record) { build(:namespace) }
+
+        it 'saves the namespace owner as experiment_subject user' do
+          expect(record_subject_and_variant!.user).to eq(subject_to_record.owner)
+        end
+      end
+
+      context 'when given a user as subject' do
+        let_it_be(:subject_to_record) { build(:user) }
+
+        it 'saves the user as experiment_subject user' do
+          expect(record_subject_and_variant!.user).to eq(subject_to_record)
+        end
+      end
+
+      context 'when given a project as subject' do
+        let_it_be(:subject_to_record) { build(:project) }
+
+        it 'saves the project as experiment_subject user' do
+          expect(record_subject_and_variant!.project).to eq(subject_to_record)
+        end
+      end
+
+      context 'when given no subject' do
+        let_it_be(:subject_to_record) { nil }
+
+        it 'raises an error' do
+          expect { record_subject_and_variant! }.to raise_error('Incompatible subject provided!')
+        end
+      end
+
+      context 'when given an incompatible subject' do
+        let_it_be(:subject_to_record) { build(:ci_build) }
+
+        it 'raises an error' do
+          expect { record_subject_and_variant! }.to raise_error('Incompatible subject provided!')
         end
       end
     end