Merge branch '338815-use-upsert-when-recording-issue-metrics' into 'master'

Record issue metrics without subtransaction

See merge request gitlab-org/gitlab!68509

app/models/issue.rb

@@ -584,9 +584,10 @@ class Issue < ApplicationRecord
       confidential_changed?(from: true, to: false)
   end
 
-  # Ensure that the metrics association is safely created and respecting the unique constraint on issue_id
   override :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?
       metrics_record = Issue::Metrics.safe_find_or_create_by(issue: self)
       self.metrics = metrics_record

app/models/issue/metrics.rb

@@ -9,6 +9,33 @@ class Issue::Metrics < ApplicationRecord
       .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!
     if issue.milestone_id.present? && self.first_associated_with_milestone_at.blank?
       self.first_associated_with_milestone_at = Time.current
@@ -24,10 +51,6 @@ class Issue::Metrics < ApplicationRecord
   private
 
   def issue_assigned_to_list_label?
-    # Avoid another DB lookup when issue.labels are empty by adding a guard clause here
-    # 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? }
+    issue.labels.joins(:lists).exists?
   end
 end

config/feature_flags/development/upsert_issue_metrics.yml

+---
+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

spec/models/issue/metrics_spec.rb

@@ -34,7 +34,7 @@ RSpec.describe Issue::Metrics do
     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
       it "records the first time an issue is associated with a milestone" do
         time = Time.current
@@ -80,20 +80,21 @@ RSpec.describe Issue::Metrics do
         expect(metrics.first_added_to_board_at).to be_like_time(time)
       end
     end
+  end
 
-    describe "#record!" do
-      it "does not cause an N+1 query" do
-        label = create(:label)
-        subject.update!(label_ids: [label.id])
-
-        control_count = ActiveRecord::QueryRecorder.new { Issue::Metrics.find_by(issue: subject).record! }.count
-
-        additional_labels = create_list(:label, 4)
+  context 'when upsert_issue_metrics is enabled' do
+    before do
+      stub_feature_flags(upsert_issue_metrics: true)
+    end
 
-        subject.update!(label_ids: additional_labels.map(&:id))
+    it_behaves_like 'when recording the default set of issue metrics on issue save'
+  end
 
-        expect { Issue::Metrics.find_by(issue: subject).record! }.not_to exceed_query_limit(control_count)
-      end
+  context 'when upsert_issue_metrics is disabled' do
+    before do
+      stub_feature_flags(upsert_issue_metrics: false)
     end
+
+    it_behaves_like 'when recording the default set of issue metrics on issue save'
   end
 end

spec/models/issue_spec.rb

@@ -102,7 +102,7 @@ RSpec.describe Issue do
       end
 
       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)
       end
@@ -111,7 +111,6 @@ RSpec.describe Issue do
         before do
           subject.metrics.delete
           subject.reload
-          subject.metrics # make sure metrics association is cached (currently nil)
         end
 
         it 'creates the metrics record' do