Merge branch '197312_store_lines_added_and_removed_in_mr_metrics' into 'master'

Store added and removed lines in MR metrics

See merge request gitlab-org/gitlab!28658

changelogs/unreleased/197312_store_lines_added_and_removed_in_mr_metrics.yml

+---
+title: Add added_lines and removed_lines columns to merge_request_metrics table
+merge_request: 28658
+author:
+type: added

db/migrate/20200402123926_add_line_metrics_to_mr_metrics.rb

+# frozen_string_literal: true
+
+class AddLineMetricsToMrMetrics < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_column :merge_request_metrics, :added_lines, :integer
+      add_column :merge_request_metrics, :removed_lines, :integer
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :merge_request_metrics, :added_lines, :integer
+      remove_column :merge_request_metrics, :removed_lines, :integer
+    end
+  end
+end

db/structure.sql

@@ -3749,7 +3749,9 @@ CREATE TABLE public.merge_request_metrics (
     modified_paths_size integer,
     commits_count integer,
     first_approved_at timestamp with time zone,
-    first_reassigned_at timestamp with time zone
+    first_reassigned_at timestamp with time zone,
+    added_lines integer,
+    removed_lines integer
 );
 
 CREATE SEQUENCE public.merge_request_metrics_id_seq
@@ -12936,6 +12938,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200330123739
 20200330132913
 20200331220930
+20200402123926
 20200402135250
 20200403184110
 20200403185127

ee/app/services/ee/merge_request_metrics_service.rb

@@ -11,7 +11,7 @@ module EE
       data = {
         merged_by_id: event.author_id,
         merged_at: event.created_at
-      }.merge(metrics_calculator.productivity_data)
+      }.merge(metrics_calculator.productivity_data, metrics_calculator.line_counts_data)
 
       update!(data)
     end

ee/lib/analytics/merge_request_metrics_calculator.rb

@@ -17,6 +17,17 @@ module Analytics
       }
     end
 
+    # rubocop: disable CodeReuse/ActiveRecord
+    def line_counts_data
+      return {} if Feature.disabled?(:store_merge_request_line_metrics, merge_request.target_project)
+
+      {
+        added_lines: raw_diff_files.sum(&:added_lines),
+        removed_lines: raw_diff_files.sum(&:removed_lines)
+      }
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+
     # rubocop: disable CodeReuse/ActiveRecord
     def first_comment_at
       merge_request.related_notes.by_humans
@@ -61,5 +72,9 @@ module Analytics
     def modified_paths_size
       merge_request.modified_paths.size
     end
+
+    def raw_diff_files
+      @raw_diff_files ||= merge_request_diff.diffs.raw_diff_files
+    end
   end
 end

ee/spec/services/ee/merge_request_metrics_service_spec.rb

@@ -3,19 +3,49 @@
 require 'spec_helper'
 
 describe EE::MergeRequestMetricsService do
-  subject { MergeRequestMetricsService.new(merge_request.metrics) }
+  subject do
+    service = MergeRequestMetricsService.new(merge_request.metrics)
+    service.merge(event)
+    service.merge_request.metrics.reload
+
+    service
+  end
 
   describe '#merge' do
     let(:merge_request) { create(:merge_request, :merged) }
     let(:expected_commit_count) { 21 }
+    let(:event) { instance_double('Event', author_id: merge_request.author.id, created_at: Time.now) }
 
     it 'saves metrics with productivity_data' do
       allow(merge_request).to receive(:commits_count).and_return(expected_commit_count)
 
-      expect do
-        subject.merge(instance_double('Event', author_id: merge_request.author.id, created_at: Time.now))
-        merge_request.metrics.reload
-      end.to change { merge_request.metrics.commits_count }.to(expected_commit_count)
+      expect { subject }.to change { merge_request.metrics.commits_count }.to(expected_commit_count)
+    end
+
+    describe 'storing line counts' do
+      let(:expected_added_lines) { 118 }
+      let(:expected_removed_lines) { 9 }
+
+      it 'updates `added_lines`' do
+        expect { subject }.to change { merge_request.metrics.added_lines }.from(nil).to(expected_added_lines)
+      end
+
+      it 'updates `removed_lines`' do
+        expect { subject }.to change { merge_request.metrics.removed_lines }.from(nil).to(expected_removed_lines)
+      end
+
+      context 'when `store_merge_request_line_metrics` feature flag is disabled' do
+        before do
+          stub_feature_flags(store_merge_request_line_metrics: { enabled: false, thing: merge_request.target_project })
+        end
+
+        it 'does not update line counts' do
+          subject
+
+          expect(merge_request.metrics.added_lines).to be_nil
+          expect(merge_request.metrics.removed_lines).to be_nil
+        end
+      end
     end
   end
 end

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -283,6 +283,8 @@ MergeRequest::Metrics:
 - commits_count
 - first_approved_at
 - first_reassigned_at
+- added_lines
+- removed_lines
 Ci::Pipeline:
 - id
 - project_id