Commit dcd37964 authored by Heinrich Lee Yu's avatar Heinrich Lee Yu

Record issue metrics without subtransaction

Use UPSERT to create or update issue metrics so we do not need a
subtransaction and multiple queries
parent 93d73652
...@@ -584,9 +584,10 @@ class Issue < ApplicationRecord ...@@ -584,9 +584,10 @@ class Issue < ApplicationRecord
confidential_changed?(from: true, to: false) confidential_changed?(from: true, to: false)
end end
# Ensure that the metrics association is safely created and respecting the unique constraint on issue_id
override :ensure_metrics override :ensure_metrics
def ensure_metrics def ensure_metrics
return Issue::Metrics.record!(self) if Feature.enabled?(:upsert_issue_metrics, default_enabled: :yaml)
if !association(:metrics).loaded? || metrics.blank? if !association(:metrics).loaded? || metrics.blank?
metrics_record = Issue::Metrics.safe_find_or_create_by(issue: self) metrics_record = Issue::Metrics.safe_find_or_create_by(issue: self)
self.metrics = metrics_record self.metrics = metrics_record
......
...@@ -9,6 +9,33 @@ class Issue::Metrics < ApplicationRecord ...@@ -9,6 +9,33 @@ class Issue::Metrics < ApplicationRecord
.or(where(arel_table['first_mentioned_in_commit_at'].gteq(timestamp))) .or(where(arel_table['first_mentioned_in_commit_at'].gteq(timestamp)))
} }
class << self
def record!(issue)
now = connection.quote(Time.current)
first_associated_with_milestone_at = issue.milestone_id.present? ? now : 'NULL'
first_added_to_board_at = issue_assigned_to_list_label?(issue) ? now : 'NULL'
sql = <<~SQL
INSERT INTO #{self.table_name} (issue_id, first_associated_with_milestone_at, first_added_to_board_at, created_at, updated_at)
VALUES (#{issue.id}, #{first_associated_with_milestone_at}, #{first_added_to_board_at}, NOW(), NOW())
ON CONFLICT (issue_id)
DO UPDATE SET
first_associated_with_milestone_at = LEAST(#{self.table_name}.first_associated_with_milestone_at, EXCLUDED.first_associated_with_milestone_at),
first_added_to_board_at = LEAST(#{self.table_name}.first_added_to_board_at, EXCLUDED.first_added_to_board_at),
updated_at = NOW()
RETURNING id
SQL
connection.execute(sql)
end
private
def issue_assigned_to_list_label?(issue)
issue.labels.joins(:lists).exists?
end
end
def record! def record!
if issue.milestone_id.present? && self.first_associated_with_milestone_at.blank? if issue.milestone_id.present? && self.first_associated_with_milestone_at.blank?
self.first_associated_with_milestone_at = Time.current self.first_associated_with_milestone_at = Time.current
...@@ -24,10 +51,6 @@ class Issue::Metrics < ApplicationRecord ...@@ -24,10 +51,6 @@ class Issue::Metrics < ApplicationRecord
private private
def issue_assigned_to_list_label? def issue_assigned_to_list_label?
# Avoid another DB lookup when issue.labels are empty by adding a guard clause here issue.labels.joins(:lists).exists?
# We can't use issue.labels.empty? because that will cause a `Label Exists?` DB lookup
return false if issue.labels.length == 0 # rubocop:disable Style/ZeroLengthPredicate
issue.labels.includes(:lists).any? { |label| label.lists.present? }
end end
end end
---
name: upsert_issue_metrics
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68509
rollout_issue_url:
milestone: '14.3'
type: development
group: group::project management
default_enabled: false
...@@ -34,7 +34,7 @@ RSpec.describe Issue::Metrics do ...@@ -34,7 +34,7 @@ RSpec.describe Issue::Metrics do
end end
end end
describe "when recording the default set of issue metrics on issue save" do shared_examples "when recording the default set of issue metrics on issue save" do
context "milestones" do context "milestones" do
it "records the first time an issue is associated with a milestone" do it "records the first time an issue is associated with a milestone" do
time = Time.current time = Time.current
...@@ -80,20 +80,21 @@ RSpec.describe Issue::Metrics do ...@@ -80,20 +80,21 @@ RSpec.describe Issue::Metrics do
expect(metrics.first_added_to_board_at).to be_like_time(time) expect(metrics.first_added_to_board_at).to be_like_time(time)
end end
end end
end
describe "#record!" do context 'when upsert_issue_metrics is enabled' do
it "does not cause an N+1 query" do before do
label = create(:label) stub_feature_flags(upsert_issue_metrics: true)
subject.update!(label_ids: [label.id]) end
control_count = ActiveRecord::QueryRecorder.new { Issue::Metrics.find_by(issue: subject).record! }.count
additional_labels = create_list(:label, 4)
subject.update!(label_ids: additional_labels.map(&:id))
expect { Issue::Metrics.find_by(issue: subject).record! }.not_to exceed_query_limit(control_count) it_behaves_like 'when recording the default set of issue metrics on issue save'
end end
context 'when upsert_issue_metrics is disabled' do
before do
stub_feature_flags(upsert_issue_metrics: false)
end end
it_behaves_like 'when recording the default set of issue metrics on issue save'
end end
end end
...@@ -102,7 +102,7 @@ RSpec.describe Issue do ...@@ -102,7 +102,7 @@ RSpec.describe Issue do
end end
it 'records current metrics' do it 'records current metrics' do
expect_any_instance_of(Issue::Metrics).to receive(:record!) expect(Issue::Metrics).to receive(:record!)
create(:issue, project: reusable_project) create(:issue, project: reusable_project)
end end
...@@ -111,7 +111,6 @@ RSpec.describe Issue do ...@@ -111,7 +111,6 @@ RSpec.describe Issue do
before do before do
subject.metrics.delete subject.metrics.delete
subject.reload subject.reload
subject.metrics # make sure metrics association is cached (currently nil)
end end
it 'creates the metrics record' do it 'creates the metrics record' do
......
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