Improve track_redis_hll_event helper

- Support custom conditions with `if:`, so we can use it in
  `EE::SearchController`

- Move format and DNT checks from shared examples into specs for the
  concern, since some controllers only support HTML requests, and the
  DNT check is hard-coded so it doesn't need to be verified again on
  every use.

app/controllers/concerns/redis_tracking.rb

@@ -11,12 +11,17 @@
 #
 # if the feature flag is enabled by default you should use
 # track_redis_hll_event :index, :show, name: 'i_analytics_dev_ops_score', feature: :my_feature, feature_default_enabled: true
+#
+# You can also pass custom conditions using `if:`, using the same format as with Rails callbacks.
 module RedisTracking
   extend ActiveSupport::Concern
 
   class_methods do
-    def track_redis_hll_event(*controller_actions, name:, feature:, feature_default_enabled: false)
-      after_action only: controller_actions, if: -> { request.format.html? && request.headers['DNT'] != '1' } do
+    def track_redis_hll_event(*controller_actions, name:, feature:, feature_default_enabled: false, if: nil)
+      custom_conditions = Array.wrap(binding.local_variable_get('if'))
+      conditions = [:trackable_request?, *custom_conditions]
+
+      after_action only: controller_actions, if: conditions do
         track_unique_redis_hll_event(name, feature, feature_default_enabled)
       end
     end
@@ -31,6 +36,10 @@ module RedisTracking
     Gitlab::UsageDataCounters::HLLRedisCounter.track_event(visitor_id, event_name)
   end
 
+  def trackable_request?
+    request.format.html? && request.headers['DNT'] != '1'
+  end
+
   def metric_feature_enabled?(feature, default_enabled)
     Feature.enabled?(feature, default_enabled: default_enabled)
   end

app/controllers/concerns/wiki_actions.rb

@@ -5,6 +5,7 @@ module WikiActions
   include PreviewMarkdown
   include SendsBlob
   include Gitlab::Utils::StrongMemoize
+  include RedisTracking
   extend ActiveSupport::Concern
 
   included do
@@ -31,6 +32,11 @@ module WikiActions
       end
     end
 
+    # NOTE: We want to include wiki page views in the same counter as the other
+    # Event-based wiki actions tracked through TrackUniqueEvents, so we use the same event name.
+    track_redis_hll_event :show, name: Gitlab::UsageDataCounters::TrackUniqueEvents::WIKI_ACTION.to_s,
+      feature: :track_unique_wiki_page_views, feature_default_enabled: true
+
     helper_method :view_file_button, :diff_file_html_data
   end
 

changelogs/unreleased/262686-track-unique-wiki-page-views.yml

+---
+title: Track unique wiki page views in Usage Ping
+merge_request: 44622
+author:
+type: changed

config/feature_flags/development/track_unique_wiki_page_views.yml

+---
+name: track_unique_wiki_page_views
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/44622
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/267162
+type: development
+group: group::knowledge
+default_enabled: true

ee/app/controllers/ee/search_controller.rb

@@ -5,19 +5,29 @@ module EE
     extend ActiveSupport::Concern
 
     prepended do
-      before_action :track_advanced_search, only: :show, if: -> { ::Feature.enabled?(:search_track_unique_users, default_enabled: true) && request.format.html? && request.headers['DNT'] != '1' }
-    end
-
-    private
-
-    def track_advanced_search
       # track unique users of advanced global search
-      track_unique_redis_hll_event('i_search_advanced', :search_track_unique_users, true) if search_service.use_elasticsearch?
+      track_redis_hll_event :show, name: 'i_search_advanced', feature: :search_track_unique_users, feature_default_enabled: true,
+        if: :track_search_advanced?
 
       # track unique paid users (users who already use elasticsearch and users who could use it if they enable elasticsearch integration)
       # for gitlab.com we check if the search uses elasticsearch
       # for self-managed we check if the licensed feature available
-      track_unique_redis_hll_event('i_search_paid', :search_track_unique_users, true) if (::Gitlab.com? && search_service.use_elasticsearch?) || (!::Gitlab.com? && License.feature_available?(:elastic_search))
+      track_redis_hll_event :show, name: 'i_search_paid', feature: :search_track_unique_users, feature_default_enabled: true,
+        if: :track_search_paid?
+    end
+
+    private
+
+    def track_search_advanced?
+      search_service.use_elasticsearch?
+    end
+
+    def track_search_paid?
+      if ::Gitlab.com?
+        search_service.use_elasticsearch?
+      else
+        License.feature_available?(:elastic_search)
+      end
     end
   end
 end

ee/spec/controllers/ee/search_controller_spec.rb

@@ -18,7 +18,7 @@ RSpec.describe SearchController do
 
       context 'i_search_advanced' do
         it_behaves_like 'tracking unique hll events', :search_track_unique_users do
-          subject { get :show, params: { scope: 'projects', search: 'term' }, format: format }
+          subject(:request) { get :show, params: { scope: 'projects', search: 'term' } }
 
           let(:target_id) { 'i_search_advanced' }
           let(:expected_type) { instance_of(String) }
@@ -38,7 +38,7 @@ RSpec.describe SearchController do
           end
 
           it_behaves_like 'tracking unique hll events', :search_track_unique_users do
-            subject { get :show, params: request_params, format: format }
+            subject(:request) { get :show, params: request_params }
 
             let(:expected_type) { instance_of(String) }
           end
@@ -55,7 +55,7 @@ RSpec.describe SearchController do
             end
 
             it_behaves_like 'tracking unique hll events', :search_track_unique_users do
-              subject { get :show, params: request_params, format: format }
+              subject(:request) { get :show, params: request_params }
 
               let(:expected_type) { instance_of(String) }
             end

spec/controllers/concerns/redis_tracking_spec.rb

@@ -10,7 +10,8 @@ RSpec.describe RedisTracking do
     include RedisTracking
 
     skip_before_action :authenticate_user!, only: :show
-    track_redis_hll_event :index, :show, name: 'g_compliance_approval_rules', feature: :approval_rule, feature_default_enabled: true
+    track_redis_hll_event :index, :show, name: 'g_compliance_approval_rules', feature: :approval_rule, feature_default_enabled: true,
+      if: [:custom_condition_one?, :custom_condition_two?]
 
     def index
       render html: 'index'
@@ -23,13 +24,32 @@ RSpec.describe RedisTracking do
     def show
       render html: 'show'
     end
+
+    private
+
+    def custom_condition_one?
+      true
+    end
+
+    def custom_condition_two?
+      true
+    end
+  end
+
+  def expect_tracking
+    expect(Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event)
+      .with(instance_of(String), 'g_compliance_approval_rules')
+  end
+
+  def expect_no_tracking
+    expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event)
   end
 
   context 'with feature disabled' do
     it 'does not track the event' do
       stub_feature_flags(feature => false)
 
-      expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event)
+      expect_no_tracking
 
       get :index
     end
@@ -41,21 +61,57 @@ RSpec.describe RedisTracking do
     end
 
     context 'when user is logged in' do
-      it 'tracks the event' do
+      before do
         sign_in(user)
+      end
 
-        expect(Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event)
+      it 'tracks the event' do
+        expect_tracking
 
         get :index
       end
 
       it 'passes default_enabled flag' do
-        sign_in(user)
-
         expect(controller).to receive(:metric_feature_enabled?).with(feature.to_sym, true)
 
         get :index
       end
+
+      it 'tracks the event if DNT is not enabled' do
+        request.headers['DNT'] = '0'
+
+        expect_tracking
+
+        get :index
+      end
+
+      it 'does not track the event if DNT is enabled' do
+        request.headers['DNT'] = '1'
+
+        expect_no_tracking
+
+        get :index
+      end
+
+      it 'does not track the event if the format is not HTML' do
+        expect_no_tracking
+
+        get :index, format: :json
+      end
+
+      it 'does not track the event if a custom condition returns false' do
+        expect(controller).to receive(:custom_condition_two?).and_return(false)
+
+        expect_no_tracking
+
+        get :index
+      end
+
+      it 'does not track the event for untracked actions' do
+        expect_no_tracking
+
+        get :new
+      end
     end
 
     context 'when user is not logged in and there is a visitor_id' do
@@ -68,26 +124,18 @@ RSpec.describe RedisTracking do
       it 'tracks the event' do
         cookies[:visitor_id] = { value: visitor_id, expires: 24.months }
 
-        expect(Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event)
+        expect_tracking
 
         get :show
       end
     end
 
     context 'when user is not logged in and there is no visitor_id' do
-      it 'does not tracks the event' do
-        expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event)
+      it 'does not track the event' do
+        expect_no_tracking
 
         get :index
       end
     end
-
-    context 'for untracked action' do
-      it 'does not tracks the event' do
-        expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event)
-
-        get :new
-      end
-    end
   end
 end

spec/controllers/projects/blob_controller_spec.rb

@@ -349,7 +349,7 @@ RSpec.describe Projects::BlobController do
     end
 
     it_behaves_like 'tracking unique hll events', :track_editor_edit_actions do
-      subject { put :update, params: default_params, format: format }
+      subject(:request) { put :update, params: default_params }
 
       let(:target_id) { 'g_edit_by_sfe' }
       let(:expected_type) { instance_of(Integer) }
@@ -465,7 +465,7 @@ RSpec.describe Projects::BlobController do
     end
 
     it_behaves_like 'tracking unique hll events', :track_editor_edit_actions do
-      subject { post :create, params: default_params, format: format }
+      subject(:request) { post :create, params: default_params }
 
       let(:target_id) { 'g_edit_by_sfe' }
       let(:expected_type) { instance_of(Integer) }

spec/controllers/search_controller_spec.rb

@@ -183,7 +183,7 @@ RSpec.describe SearchController do
       end
 
       it_behaves_like 'tracking unique hll events', :search_track_unique_users do
-        subject { get :show, params: { scope: 'projects', search: 'term' }, format: format }
+        subject(:request) { get :show, params: { scope: 'projects', search: 'term' } }
 
         let(:target_id) { 'i_search_total' }
         let(:expected_type) { instance_of(String) }

spec/support/shared_examples/controllers/unique_hll_events_examples.rb

 # frozen_string_literal: true
+#
+# Requires a context containing:
+# - request
+# - expected_type
+# - target_id
 
 RSpec.shared_examples 'tracking unique hll events' do |feature_flag|
-  context 'when format is HTML' do
-    let(:format) { :html }
+  it 'tracks unique event' do
+    expect(Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event).with(expected_type, target_id)
 
-    it 'tracks unique event' do
-      expect(Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event).with(expected_type, target_id)
-
-      subject
-    end
-
-    it 'tracks unique event if DNT is not enabled' do
-      expect(Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event).with(expected_type, target_id)
-      request.headers['DNT'] = '0'
-
-      subject
-    end
-
-    it 'does not track unique event if DNT is enabled' do
-      expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event).with(expected_type, target_id)
-      request.headers['DNT'] = '1'
-
-      subject
-    end
-
-    context 'when feature flag is disabled' do
-      it 'does not track unique event' do
-        stub_feature_flags(feature_flag => false)
-
-        expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event).with(expected_type, target_id)
-
-        subject
-      end
-    end
+    request
   end
 
-  context 'when format is JSON' do
-    let(:format) { :json }
+  context 'when feature flag is disabled' do
+    it 'does not track unique event' do
+      stub_feature_flags(feature_flag => false)
 
-    it 'does not track unique event if the format is JSON' do
-      expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event).with(expected_type, target_id)
+      expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event)
 
-      subject
+      request
     end
   end
 end

spec/support/shared_examples/controllers/wiki_actions_shared_examples.rb

@@ -15,10 +15,10 @@ RSpec.shared_examples 'wiki controller actions' do
   end
 
   describe 'GET #new' do
-    subject { get :new, params: routing_params }
+    subject(:request) { get :new, params: routing_params }
 
     it 'redirects to #show and appends a `random_title` param' do
-      subject
+      request
 
       expect(response).to be_redirect
       expect(response.redirect_url).to match(%r{
@@ -35,7 +35,7 @@ RSpec.shared_examples 'wiki controller actions' do
       end
 
       it 'redirects to the wiki container and displays an error message' do
-        subject
+        request
 
         expect(response).to redirect_to(container)
         expect(flash[:notice]).to eq('Could not create Wiki Repository at this time. Please try again later.')
@@ -146,13 +146,13 @@ RSpec.shared_examples 'wiki controller actions' do
 
     let(:random_title) { nil }
 
-    subject { get :show, params: routing_params.merge(id: id, random_title: random_title) }
+    subject(:request) { get :show, params: routing_params.merge(id: id, random_title: random_title) }
 
     context 'when page exists' do
       let(:id) { wiki_title }
 
       it 'renders the page' do
-        subject
+        request
 
         expect(response).to have_gitlab_http_status(:ok)
         expect(response).to render_template('shared/wikis/show')
@@ -161,19 +161,26 @@ RSpec.shared_examples 'wiki controller actions' do
         expect(assigns(:sidebar_limited)).to be(false)
       end
 
-      it 'increases the page view counter' do
-        expect do
-          subject
+      context 'page view tracking' do
+        it_behaves_like 'tracking unique hll events', :track_unique_wiki_page_views do
+          let(:target_id) { 'wiki_action' }
+          let(:expected_type) { instance_of(String) }
+        end
 
-          expect(response).to have_gitlab_http_status(:ok)
-        end.to change { Gitlab::UsageDataCounters::WikiPageCounter.read(:view) }.by(1)
+        it 'increases the page view counter' do
+          expect do
+            request
+
+            expect(response).to have_gitlab_http_status(:ok)
+          end.to change { Gitlab::UsageDataCounters::WikiPageCounter.read(:view) }.by(1)
+        end
       end
 
       context 'when page content encoding is invalid' do
         it 'sets flash error' do
           allow(controller).to receive(:valid_encoding?).and_return(false)
 
-          subject
+          request
 
           expect(response).to have_gitlab_http_status(:ok)
           expect(response).to render_template('shared/wikis/show')
@@ -187,7 +194,7 @@ RSpec.shared_examples 'wiki controller actions' do
 
       context 'when the user can create pages' do
         before do
-          subject
+          request
 
           expect(response).to have_gitlab_http_status(:ok)
           expect(response).to render_template('shared/wikis/edit')
@@ -212,7 +219,7 @@ RSpec.shared_examples 'wiki controller actions' do
         end
 
         it 'shows the empty state' do
-          subject
+          request
 
           expect(response).to have_gitlab_http_status(:ok)
           expect(response).to render_template('shared/wikis/empty')
@@ -229,7 +236,7 @@ RSpec.shared_examples 'wiki controller actions' do
         let(:id) { upload_file_to_wiki(wiki, user, file_name) }
 
         it 'delivers the file with the correct headers' do
-          subject
+          request
 
           expect(response).to have_gitlab_http_status(:ok)
           expect(response.headers['Content-Disposition']).to match(/^inline/)
@@ -255,7 +262,7 @@ RSpec.shared_examples 'wiki controller actions' do
       let(:id_param) { 'invalid' }
 
       it 'redirects to show' do
-        subject
+        request
 
         expect(response).to redirect_to_wiki(wiki, 'invalid')
       end
@@ -265,7 +272,7 @@ RSpec.shared_examples 'wiki controller actions' do
       let(:id_param) { ' ' }
 
       it 'redirects to the home page' do
-        subject
+        request
 
         expect(response).to redirect_to_wiki(wiki, 'home')
       end
@@ -275,7 +282,7 @@ RSpec.shared_examples 'wiki controller actions' do
       it 'redirects to show' do
         allow(controller).to receive(:valid_encoding?).and_return(false)
 
-        subject
+        request
 
         expect(response).to redirect_to_wiki(wiki, wiki.list_pages.first)
       end
@@ -288,7 +295,7 @@ RSpec.shared_examples 'wiki controller actions' do
         allow(page).to receive(:content).and_return(nil)
         allow(controller).to receive(:page).and_return(page)
 
-        subject
+        request
 
         expect(response).to redirect_to_wiki(wiki, page)
       end
@@ -298,7 +305,7 @@ RSpec.shared_examples 'wiki controller actions' do
   describe 'GET #edit' do
     let(:id_param) { wiki_title }
 
-    subject { get(:edit, params: routing_params.merge(id: id_param)) }
+    subject(:request) { get(:edit, params: routing_params.merge(id: id_param)) }
 
     it_behaves_like 'edit action'
 
@@ -306,7 +313,7 @@ RSpec.shared_examples 'wiki controller actions' do
       render_views
 
       it 'shows the edit page' do
-        subject
+        request
 
         expect(response).to have_gitlab_http_status(:ok)
         expect(response.body).to include(s_('Wiki|Edit Page'))
@@ -319,7 +326,7 @@ RSpec.shared_examples 'wiki controller actions' do
     let(:new_content) { 'New content' }
     let(:id_param) { wiki_title }
 
-    subject do
+    subject(:request) do
       patch(:update,
             params: routing_params.merge(
               id: id_param,
@@ -333,7 +340,7 @@ RSpec.shared_examples 'wiki controller actions' do
       render_views
 
       it 'updates the page' do
-        subject
+        request
 
         wiki_page = wiki.list_pages(load_content: true).first
 
@@ -348,7 +355,7 @@ RSpec.shared_examples 'wiki controller actions' do
       end
 
       it 'renders the empty state' do
-        subject
+        request
 
         expect(response).to render_template('shared/wikis/empty')
       end
@@ -359,7 +366,7 @@ RSpec.shared_examples 'wiki controller actions' do
     let(:new_title) { 'New title' }
     let(:new_content) { 'New content' }
 
-    subject do
+    subject(:request) do
       post(:create,
             params: routing_params.merge(
               wiki: { title: new_title, content: new_content }
@@ -369,7 +376,7 @@ RSpec.shared_examples 'wiki controller actions' do
     context 'when page is valid' do
       it 'creates the page' do
         expect do
-          subject
+          request
         end.to change { wiki.list_pages.size }.by 1
 
         wiki_page = wiki.find_page(new_title)
@@ -384,7 +391,7 @@ RSpec.shared_examples 'wiki controller actions' do
 
       it 'renders the edit state' do
         expect do
-          subject
+          request
         end.not_to change { wiki.list_pages.size }
 
         expect(response).to render_template('shared/wikis/edit')
@@ -395,7 +402,7 @@ RSpec.shared_examples 'wiki controller actions' do
   describe 'DELETE #destroy' do
     let(:id_param) { wiki_title }
 
-    subject do
+    subject(:request) do
       delete(:destroy,
             params: routing_params.merge(
               id: id_param
@@ -405,7 +412,7 @@ RSpec.shared_examples 'wiki controller actions' do
     context 'when page exists' do
       it 'deletes the page' do
         expect do
-          subject
+          request
         end.to change { wiki.list_pages.size }.by(-1)
       end
 
@@ -418,7 +425,7 @@ RSpec.shared_examples 'wiki controller actions' do
 
         it 'renders the edit state' do
           expect do
-            subject
+            request
           end.not_to change { wiki.list_pages.size }
 
           expect(response).to render_template('shared/wikis/edit')
@@ -432,7 +439,7 @@ RSpec.shared_examples 'wiki controller actions' do
 
       it 'renders 404' do
         expect do
-          subject
+          request
         end.not_to change { wiki.list_pages.size }
 
         expect(response).to have_gitlab_http_status(:not_found)