Merge branch '247184-instument-file-by-file-mrs' into 'master'

Instrument viewing merge request diffs file by file

See merge request gitlab-org/gitlab!48470

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -11,6 +11,8 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
 
   around_action :allow_gitaly_ref_name_caching
 
+  after_action :track_viewed_diffs_events, only: [:diffs_batch]
+
   def show
     render_diffs
   end
@@ -188,4 +190,16 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
 
     Discussions::CaptureDiffNotePositionsService.new(@merge_request).execute
   end
+
+  def track_viewed_diffs_events
+    return if request.headers['DNT'] == '1'
+
+    Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter
+      .track_mr_diffs_action(merge_request: @merge_request)
+
+    return unless current_user&.view_diffs_file_by_file
+
+    Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter
+      .track_mr_diffs_single_file_action(merge_request: @merge_request, user: current_user)
+  end
 end

changelogs/unreleased/247184-instument-file-by-file-mrs.yml

+---
+title: Instrument viewing merge request diffs file by file
+merge_request: 48470
+author:
+type: added

config/feature_flags/development/usage_data_i_code_review_mr_diffs.yml

+---
+name: usage_data_i_code_review_mr_diffs
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48470
+rollout_issue_url:
+milestone: '13.7'
+type: development
+group: group::code review
+default_enabled: true

config/feature_flags/development/usage_data_i_code_review_mr_single_file_diffs.yml

+---
+name: usage_data_i_code_review_mr_single_file_diffs
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48470
+rollout_issue_url:
+milestone: '13.7'
+type: development
+group: group::code review
+default_enabled: true

config/feature_flags/development/usage_data_i_code_review_user_single_file_diffs.yml

+---
+name: usage_data_i_code_review_user_single_file_diffs
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48470
+rollout_issue_url:
+milestone: '13.7'
+type: development
+group: group::code review
+default_enabled: true

lib/gitlab/usage_data_counters/known_events/common.yml

@@ -425,3 +425,18 @@
   redis_slot: snippets
   aggregation: weekly
   feature_flag: usage_data_i_snippets_show
+- name: i_code_review_mr_diffs
+  redis_slot: code_review
+  category: code_review
+  aggregation: weekly
+  feature_flag: usage_data_i_code_review_mr_diffs
+- name: i_code_review_user_single_file_diffs
+  redis_slot: code_review
+  category: code_review
+  aggregation: weekly
+  feature_flag: usage_data_i_code_review_user_single_file_diffs
+- name: i_code_review_mr_single_file_diffs
+  redis_slot: code_review
+  category: code_review
+  aggregation: weekly
+  feature_flag: usage_data_i_code_review_mr_single_file_diffs

lib/gitlab/usage_data_counters/merge_request_activity_unique_counter.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module UsageDataCounters
+    module MergeRequestActivityUniqueCounter
+      MR_DIFFS_ACTION = 'i_code_review_mr_diffs'
+      MR_DIFFS_SINGLE_FILE_ACTION = 'i_code_review_mr_single_file_diffs'
+      MR_DIFFS_USER_SINGLE_FILE_ACTION = 'i_code_review_user_single_file_diffs'
+
+      class << self
+        def track_mr_diffs_action(merge_request:)
+          track_unique_action_by_merge_request(MR_DIFFS_ACTION, merge_request)
+        end
+
+        def track_mr_diffs_single_file_action(merge_request:, user:)
+          track_unique_action_by_merge_request(MR_DIFFS_SINGLE_FILE_ACTION, merge_request)
+          track_unique_action_by_user(MR_DIFFS_USER_SINGLE_FILE_ACTION, user)
+        end
+
+        private
+
+        def track_unique_action_by_merge_request(action, merge_request)
+          track_unique_action(action, merge_request.id)
+        end
+
+        def track_unique_action_by_user(action, user)
+          return unless user
+
+          track_unique_action(action, user.id)
+        end
+
+        def track_unique_action(action, value)
+          Gitlab::UsageDataCounters::HLLRedisCounter.track_usage_event(action, value)
+        end
+      end
+    end
+  end
+end

spec/controllers/projects/merge_requests/diffs_controller_spec.rb

@@ -378,6 +378,57 @@ RSpec.describe Projects::MergeRequests::DiffsController do
 
         expect(response).to have_gitlab_http_status(:ok)
       end
+
+      it 'tracks mr_diffs event' do
+        expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
+          .to receive(:track_mr_diffs_action)
+          .with(merge_request: merge_request)
+
+        subject
+      end
+
+      context 'when DNT is enabled' do
+        before do
+          request.headers['DNT'] = '1'
+        end
+
+        it 'does not track any mr_diffs event' do
+          expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
+            .not_to receive(:track_mr_diffs_action)
+
+          expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
+            .not_to receive(:track_mr_diffs_single_file_action)
+
+          subject
+        end
+      end
+
+      context 'when user has view_diffs_file_by_file set to false' do
+        before do
+          user.update!(view_diffs_file_by_file: false)
+        end
+
+        it 'does not track single_file_diffs events' do
+          expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
+            .not_to receive(:track_mr_diffs_single_file_action)
+
+          subject
+        end
+      end
+
+      context 'when user has view_diffs_file_by_file set to true' do
+        before do
+          user.update!(view_diffs_file_by_file: true)
+        end
+
+        it 'tracks single_file_diffs events' do
+          expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
+            .to receive(:track_mr_diffs_single_file_action)
+            .with(merge_request: merge_request, user: user)
+
+          subject
+        end
+      end
     end
 
     def collection_arguments(pagination_data = {})

spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb

@@ -45,7 +45,8 @@ RSpec.describe Gitlab::UsageDataCounters::HLLRedisCounter, :clean_gitlab_redis_s
         'debian_packages',
         'container_packages',
         'tag_packages',
-        'snippets'
+        'snippets',
+        'code_review'
       )
     end
   end

spec/lib/gitlab/usage_data_counters/merge_request_activity_unique_counter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter, :clean_gitlab_redis_shared_state do
+  let(:merge_request) { build(:merge_request, id: 1) }
+  let(:user) { build(:user, id: 1) }
+
+  shared_examples_for 'a tracked merge request unique event' do
+    specify do
+      expect { 3.times { subject } }
+        .to change {
+          Gitlab::UsageDataCounters::HLLRedisCounter.unique_events(
+            event_names: action,
+            start_date: 2.weeks.ago,
+            end_date: 2.weeks.from_now
+          )
+        }
+        .by(1)
+    end
+  end
+
+  describe '.track_mr_diffs_action' do
+    subject { described_class.track_mr_diffs_action(merge_request: merge_request) }
+
+    it_behaves_like 'a tracked merge request unique event' do
+      let(:action) { described_class::MR_DIFFS_ACTION }
+    end
+  end
+
+  describe '.track_mr_diffs_single_file_action' do
+    subject { described_class.track_mr_diffs_single_file_action(merge_request: merge_request, user: user) }
+
+    it_behaves_like 'a tracked merge request unique event' do
+      let(:action) { described_class::MR_DIFFS_SINGLE_FILE_ACTION }
+    end
+
+    it_behaves_like 'a tracked merge request unique event' do
+      let(:action) { described_class::MR_DIFFS_USER_SINGLE_FILE_ACTION }
+    end
+  end
+end