Commit 9e81b932 authored by Patrick Bajao's avatar Patrick Bajao

Instrument viewing merge request diffs file by file

In order to understand how are users are using the view file by
file feature, we need to track:
- How many users viewed MRs file by file
- How many MRs are being viewed file by file
- How many MRs are being viewed

This is tracked on each request to `diffs_batch.json` since that's
when we show the file by file controls.
parent ef36f93d
...@@ -11,6 +11,8 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic ...@@ -11,6 +11,8 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
around_action :allow_gitaly_ref_name_caching around_action :allow_gitaly_ref_name_caching
after_action :track_viewed_diffs_events, only: [:diffs_batch]
def show def show
render_diffs render_diffs
end end
...@@ -188,4 +190,16 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic ...@@ -188,4 +190,16 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
Discussions::CaptureDiffNotePositionsService.new(@merge_request).execute Discussions::CaptureDiffNotePositionsService.new(@merge_request).execute
end 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 end
---
title: Instrument viewing merge request diffs file by file
merge_request: 48470
author:
type: added
---
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
---
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
---
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
...@@ -425,3 +425,18 @@ ...@@ -425,3 +425,18 @@
redis_slot: snippets redis_slot: snippets
aggregation: weekly aggregation: weekly
feature_flag: usage_data_i_snippets_show 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
# 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
...@@ -378,6 +378,57 @@ RSpec.describe Projects::MergeRequests::DiffsController do ...@@ -378,6 +378,57 @@ RSpec.describe Projects::MergeRequests::DiffsController do
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
end 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 end
def collection_arguments(pagination_data = {}) def collection_arguments(pagination_data = {})
......
...@@ -45,7 +45,8 @@ RSpec.describe Gitlab::UsageDataCounters::HLLRedisCounter, :clean_gitlab_redis_s ...@@ -45,7 +45,8 @@ RSpec.describe Gitlab::UsageDataCounters::HLLRedisCounter, :clean_gitlab_redis_s
'debian_packages', 'debian_packages',
'container_packages', 'container_packages',
'tag_packages', 'tag_packages',
'snippets' 'snippets',
'code_review'
) )
end end
end end
......
# 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
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