Merge branch '324649-prevent-experiments-sticking-to-primary' into 'master'

Prevent unnecessary sticking to the DB primary when recording experiments

See merge request gitlab-org/gitlab!56852

app/models/experiment.rb

@@ -21,7 +21,13 @@ class Experiment < ApplicationRecord
   # Create or update the recorded experiment_user row for the user in this experiment.
   def record_user_and_group(user, group_type, context = {})
     experiment_user = experiment_users.find_or_initialize_by(user: user)
-    experiment_user.update!(group_type: group_type, context: merged_context(experiment_user, context))
+    experiment_user.assign_attributes(group_type: group_type, context: merged_context(experiment_user, context))
+    # 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
+    # https://gitlab.com/gitlab-org/gitlab/-/issues/324649
+    experiment_user.save! if experiment_user.changed?
+
+    experiment_user
   end
 
   def record_conversion_event_for_user(user, context = {})
@@ -32,7 +38,14 @@ class Experiment < ApplicationRecord
   end
 
   def record_group_and_variant!(group, variant)
-    experiment_subjects.find_or_initialize_by(group: group).update!(variant: variant)
+    experiment_subject = experiment_subjects.find_or_initialize_by(group: group)
+    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
+    # https://gitlab.com/gitlab-org/gitlab/-/issues/324649
+    experiment_subject.save! if experiment_subject.changed?
+
+    experiment_subject
   end
 
   private

changelogs/unreleased/324649-prevent-experiments-sticking-to-primary.yml

+---
+title: Prevent sticking to DB primary when experiments are tracked
+merge_request: 56852
+author:
+type: performance

spec/models/experiment_spec.rb

@@ -244,18 +244,27 @@ RSpec.describe Experiment do
 
     context 'when no existing experiment_subject record exists for the given group' do
       it 'creates an experiment_subject record' do
-        expect_next(ExperimentSubject).to receive(:update!).with(variant: variant).and_call_original
-
         expect { record_group_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 'but it belonged to a different variant' do
-        let!(:experiment_subject) do
-          create(:experiment_subject, experiment: experiment, group: group, user: nil, variant: :experimental)
+      let_it_be(:experiment_subject) do
+        create(:experiment_subject, experiment: experiment, group: group, user: nil, variant: :experimental)
+      end
+
+      context 'when it belongs to the same variant' do
+        let(:variant) { :experimental }
+
+        it 'does not initiate a transaction' do
+          expect(ActiveRecord::Base.connection).not_to receive(:transaction)
+
+          subject
         end
+      end
 
+      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')
         end
@@ -299,6 +308,16 @@ RSpec.describe Experiment do
         expect { subject }.not_to change(ExperimentUser, :count)
       end
 
+      context 'when group type or context did not change' do
+        let(:context) { {} }
+
+        it 'does not initiate a transaction' do
+          expect(ActiveRecord::Base.connection).not_to receive(:transaction)
+
+          subject
+        end
+      end
+
       context 'but the group_type and context has changed' do
         let(:group) { :experimental }