Commit 2de84b47 authored by Heinrich Lee Yu's avatar Heinrich Lee Yu

Prevent unnecessary sticking to the DB primary

Experiments can be run on high-traffic read-only pages and when a
transaction is initiated, our load balancing code will direct all
queries to the primary.

This update is a no-op most of the time so we skip the update when
we detect that the attributes haven't changed.

ActiveRecord actually already does this under the hood but a
`#transaction` call is still made in the process causing the
undesired behavior with our load balancer.
parent 65a0a021
...@@ -21,7 +21,13 @@ class Experiment < ApplicationRecord ...@@ -21,7 +21,13 @@ class Experiment < ApplicationRecord
# Create or update the recorded experiment_user row for the user in this experiment. # Create or update the recorded experiment_user row for the user in this experiment.
def record_user_and_group(user, group_type, context = {}) def record_user_and_group(user, group_type, context = {})
experiment_user = experiment_users.find_or_initialize_by(user: user) 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 end
def record_conversion_event_for_user(user, context = {}) def record_conversion_event_for_user(user, context = {})
...@@ -32,7 +38,14 @@ class Experiment < ApplicationRecord ...@@ -32,7 +38,14 @@ class Experiment < ApplicationRecord
end end
def record_group_and_variant!(group, variant) 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 end
private private
......
---
title: Prevent sticking to DB primary when experiments are tracked
merge_request: 56852
author:
type: performance
...@@ -244,18 +244,27 @@ RSpec.describe Experiment do ...@@ -244,18 +244,27 @@ RSpec.describe Experiment do
context 'when no existing experiment_subject record exists for the given group' do context 'when no existing experiment_subject record exists for the given group' do
it 'creates an experiment_subject record' 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 { record_group_and_variant! }.to change(ExperimentSubject, :count).by(1)
expect(ExperimentSubject.last.variant).to eq(variant.to_s)
end end
end end
context 'when an existing experiment_subject exists for the given group' do context 'when an existing experiment_subject exists for the given group' do
context 'but it belonged to a different variant' do let_it_be(:experiment_subject) do
let!(:experiment_subject) do create(:experiment_subject, experiment: experiment, group: group, user: nil, variant: :experimental)
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
end
context 'but it belonged to a different variant' do
it 'updates the variant value' do it 'updates the variant value' do
expect { record_group_and_variant! }.to change { experiment_subject.reload.variant }.to('control') expect { record_group_and_variant! }.to change { experiment_subject.reload.variant }.to('control')
end end
...@@ -299,6 +308,16 @@ RSpec.describe Experiment do ...@@ -299,6 +308,16 @@ RSpec.describe Experiment do
expect { subject }.not_to change(ExperimentUser, :count) expect { subject }.not_to change(ExperimentUser, :count)
end 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 context 'but the group_type and context has changed' do
let(:group) { :experimental } let(:group) { :experimental }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment