Merge branch '276948-check-feature-flag-in-hll-redis-counter' into 'master'

Use feature from know_events yml files for checking the features [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!53481

app/controllers/concerns/redis_tracking.rb

@@ -7,30 +7,26 @@
 #
 # include RedisTracking
 #
-# track_redis_hll_event :index, :show, name: 'i_analytics_dev_ops_score', feature: :my_feature
-#
-# 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
+# track_redis_hll_event :index, :show, name: 'i_analytics_dev_ops_score'
 #
 # 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, if: nil)
+    def track_redis_hll_event(*controller_actions, name:, 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)
+        track_unique_redis_hll_event(name)
       end
     end
   end
 
   private
 
-  def track_unique_redis_hll_event(event_name, feature, feature_default_enabled)
-    return unless metric_feature_enabled?(feature, feature_default_enabled)
+  def track_unique_redis_hll_event(event_name)
     return unless visitor_id
 
     Gitlab::UsageDataCounters::HLLRedisCounter.track_event(event_name, values: visitor_id)
@@ -40,10 +36,6 @@ module RedisTracking
     request.format.html? && request.headers['DNT'] != '1'
   end
 
-  def metric_feature_enabled?(feature, default_enabled)
-    Feature.enabled?(feature, default_enabled: default_enabled)
-  end
-
   def visitor_id
     return cookies[:visitor_id] if cookies[:visitor_id].present?
     return unless current_user

app/controllers/concerns/snippets_actions.rb

@@ -15,7 +15,7 @@ module SnippetsActions
     skip_before_action :verify_authenticity_token,
       if: -> { action_name == 'show' && js_request? }
 
-    track_redis_hll_event :show, name: 'i_snippets_show', feature: :usage_data_i_snippets_show, feature_default_enabled: true
+    track_redis_hll_event :show, name: 'i_snippets_show'
 
     respond_to :html
   end

app/controllers/concerns/wiki_actions.rb

@@ -36,8 +36,7 @@ module WikiActions
 
     # 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
+    track_redis_hll_event :show, name: Gitlab::UsageDataCounters::TrackUniqueEvents::WIKI_ACTION.to_s
 
     helper_method :view_file_button, :diff_file_html_data
 

app/controllers/projects/blob_controller.rb

@@ -35,7 +35,7 @@ class Projects::BlobController < Projects::ApplicationController
     record_experiment_user(:ci_syntax_templates, namespace_id: @project.namespace_id) if params[:file_name] == @project.ci_config_path_or_default
   end
 
-  track_redis_hll_event :create, :update, name: 'g_edit_by_sfe', feature: :track_editor_edit_actions, feature_default_enabled: true
+  track_redis_hll_event :create, :update, name: 'g_edit_by_sfe'
 
   feature_category :source_code_management
 

app/controllers/search_controller.rb

@@ -5,7 +5,7 @@ class SearchController < ApplicationController
   include SearchHelper
   include RedisTracking
 
-  track_redis_hll_event :show, name: 'i_search_total', feature: :search_track_unique_users, feature_default_enabled: true
+  track_redis_hll_event :show, name: 'i_search_total'
 
   around_action :allow_gitaly_ref_name_caching
 

doc/development/usage_ping.md

@@ -495,18 +495,17 @@ Implemented using Redis methods [PFADD](https://redis.io/commands/pfadd) and [PF
      aggregation.
    - `aggregation`: may be set to a `:daily` or `:weekly` key. Defines how counting data is stored in Redis.
      Aggregation on a `daily` basis does not pull more fine grained data.
-   - `feature_flag`: optional. For details, see our [GitLab internal Feature flags](feature_flags/) documentation. The feature flags are owned by the group adding the event tracking.
+   - `feature_flag`: optional `default_enabled: :yaml`. If no feature flag is set then the tracking is enabled. For details, see our [GitLab internal Feature flags](feature_flags/) documentation. The feature flags are owned by the group adding the event tracking.
 
 Use one of the following methods to track events:
 
-1. Track event in controller using `RedisTracking` module with `track_redis_hll_event(*controller_actions, name:, feature:, feature_default_enabled: false)`.
+1. Track event in controller using `RedisTracking` module with `track_redis_hll_event(*controller_actions, name:, if: nil)`.
 
    Arguments:
 
    - `controller_actions`: controller actions we want to track.
    - `name`: event name.
-   - `feature`: feature name, all metrics we track should be under feature flag.
-   - `feature_default_enabled`: feature flag is disabled by default, set to `true` for it to be enabled by default.
+   - `if`: optional custom conditions, using the same format as with Rails callbacks.
 
    Example usage:
 
@@ -516,7 +515,7 @@ Use one of the following methods to track events:
      include RedisTracking
 
      skip_before_action :authenticate_user!, only: :show
-     track_redis_hll_event :index, :show, name: 'g_compliance_example_feature_visitors', feature: :compliance_example_feature, feature_default_enabled: true
+     track_redis_hll_event :index, :show, name: 'g_compliance_example_feature_visitors'
 
      def index
        render html: 'index'

ee/app/controllers/ee/search_controller.rb

@@ -7,13 +7,13 @@ module EE
 
     prepended do
       # track unique users of advanced global search
-      track_redis_hll_event :show, name: 'i_search_advanced', feature: :search_track_unique_users, feature_default_enabled: true,
+      track_redis_hll_event :show, name: 'i_search_advanced',
         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_redis_hll_event :show, name: 'i_search_paid', feature: :search_track_unique_users, feature_default_enabled: true,
+      track_redis_hll_event :show, name: 'i_search_paid',
         if: :track_search_paid?
     end
 

ee/app/controllers/groups/analytics/repository_analytics_controller.rb

@@ -10,7 +10,7 @@ class Groups::Analytics::RepositoryAnalyticsController < Groups::Analytics::Appl
   before_action only: [:show] do
     push_frontend_feature_flag(:usage_data_i_testing_group_code_coverage_project_click_total, @group, default_enabled: :yaml)
   end
-  track_redis_hll_event :show, name: 'i_testing_group_code_coverage_visit_total', feature: :usage_data_i_testing_group_code_coverage_visit_total, feature_default_enabled: true
+  track_redis_hll_event :show, name: 'i_testing_group_code_coverage_visit_total'
 
   def show
     track_event(**pageview_tracker_params)

ee/app/controllers/projects/integrations/jira/issues_controller.rb

@@ -10,8 +10,7 @@ module Projects
         include RedisTracking
 
         track_redis_hll_event :index,
-          name: 'i_ecosystem_jira_service_list_issues',
-          feature: :usage_data_track_ecosystem_jira_service
+          name: 'i_ecosystem_jira_service_list_issues'
 
         before_action :check_feature_enabled!
         before_action :check_issues_show_enabled!, only: :show

ee/spec/controllers/ee/search_controller_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe SearchController do
       end
 
       context 'i_search_advanced' do
-        it_behaves_like 'tracking unique hll events', :search_track_unique_users do
+        it_behaves_like 'tracking unique hll events' do
           subject(:request) { get :show, params: { scope: 'projects', search: 'term' } }
 
           let(:target_id) { 'i_search_advanced' }
@@ -37,7 +37,7 @@ RSpec.describe SearchController do
             stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
           end
 
-          it_behaves_like 'tracking unique hll events', :search_track_unique_users do
+          it_behaves_like 'tracking unique hll events' do
             subject(:request) { get :show, params: request_params }
 
             let(:expected_type) { instance_of(String) }
@@ -54,7 +54,7 @@ RSpec.describe SearchController do
               stub_licensed_features(elastic_search: true)
             end
 
-            it_behaves_like 'tracking unique hll events', :search_track_unique_users do
+            it_behaves_like 'tracking unique hll events' do
               subject(:request) { get :show, params: request_params }
 
               let(:expected_type) { instance_of(String) }

ee/spec/controllers/groups/analytics/repository_analytics_controller_spec.rb

@@ -38,7 +38,7 @@ RSpec.describe Groups::Analytics::RepositoryAnalyticsController do
         allow(Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event)
       end
 
-      it_behaves_like 'tracking unique hll events', :usage_data_i_testing_group_code_coverage_visit_total do
+      it_behaves_like 'tracking unique hll events' do
         subject(:request) { get :show, params: { group_id: group } }
 
         let(:target_id) { 'i_testing_group_code_coverage_visit_total' }

lib/gitlab/usage_data_counters/hll_redis_counter.rb

@@ -129,6 +129,8 @@ module Gitlab
           event = event_for(event_name)
           raise UnknownEvent, "Unknown event #{event_name}" unless event.present?
 
+          return unless feature_enabled?(event)
+
           Gitlab::Redis::HLL.add(key: redis_key(event, time, context), value: values, expiry: expiry(event))
         end
 
@@ -148,6 +150,12 @@ module Gitlab
           redis_usage_data { Gitlab::Redis::HLL.count(keys: keys) }
         end
 
+        def feature_enabled?(event)
+          return true if event[:feature_flag].blank?
+
+          Feature.enabled?(event[:feature_flag], default_enabled: :yaml)
+        end
+
         # Allow to add totals for events that are in the same redis slot, category and have the same aggregation level
         # and if there are more than 1 event
         def eligible_for_totals?(events_names)

spec/controllers/concerns/redis_tracking_spec.rb

@@ -3,18 +3,13 @@
 require "spec_helper"
 
 RSpec.describe RedisTracking do
-  let(:feature) { 'approval_rule' }
   let(:user) { create(:user) }
 
-  before do
-    skip_feature_flags_yaml_validation
-  end
-
   controller(ApplicationController) 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',
       if: [:custom_condition_one?, :custom_condition_two?]
 
     def index
@@ -49,97 +44,75 @@ RSpec.describe RedisTracking do
     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_no_tracking
-
-      get :index
-    end
-  end
-
-  context 'with feature enabled' do
+  context 'when user is logged in' do
     before do
-      stub_feature_flags(feature => true)
+      sign_in(user)
     end
 
-    context 'when user is logged in' do
-      before do
-        sign_in(user)
-      end
-
-      it 'tracks the event' do
-        expect_tracking
-
-        get :index
-      end
-
-      it 'passes default_enabled flag' do
-        expect(controller).to receive(:metric_feature_enabled?).with(feature.to_sym, true)
+    it 'tracks the event' do
+      expect_tracking
 
-        get :index
-      end
+      get :index
+    end
 
-      it 'tracks the event if DNT is not enabled' do
-        request.headers['DNT'] = '0'
+    it 'tracks the event if DNT is not enabled' do
+      request.headers['DNT'] = '0'
 
-        expect_tracking
+      expect_tracking
 
-        get :index
-      end
+      get :index
+    end
 
-      it 'does not track the event if DNT is enabled' do
-        request.headers['DNT'] = '1'
+    it 'does not track the event if DNT is enabled' do
+      request.headers['DNT'] = '1'
 
-        expect_no_tracking
+      expect_no_tracking
 
-        get :index
-      end
+      get :index
+    end
 
-      it 'does not track the event if the format is not HTML' do
-        expect_no_tracking
+    it 'does not track the event if the format is not HTML' do
+      expect_no_tracking
 
-        get :index, format: :json
-      end
+      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)
+    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
+      expect_no_tracking
 
-        get :index
-      end
+      get :index
+    end
 
-      it 'does not track the event for untracked actions' do
-        expect_no_tracking
+    it 'does not track the event for untracked actions' do
+      expect_no_tracking
 
-        get :new
-      end
+      get :new
     end
+  end
 
-    context 'when user is not logged in and there is a visitor_id' do
-      let(:visitor_id) { SecureRandom.uuid }
+  context 'when user is not logged in and there is a visitor_id' do
+    let(:visitor_id) { SecureRandom.uuid }
 
-      before do
-        routes.draw { get 'show' => 'anonymous#show' }
-      end
+    before do
+      routes.draw { get 'show' => 'anonymous#show' }
+    end
 
-      it 'tracks the event' do
-        cookies[:visitor_id] = { value: visitor_id, expires: 24.months }
+    it 'tracks the event' do
+      cookies[:visitor_id] = { value: visitor_id, expires: 24.months }
 
-        expect_tracking
+      expect_tracking
 
-        get :show
-      end
+      get :show
     end
+  end
 
-    context 'when user is not logged in and there is no visitor_id' do
-      it 'does not track the event' do
-        expect_no_tracking
+  context 'when user is not logged in and there is no visitor_id' do
+    it 'does not track the event' do
+      expect_no_tracking
 
-        get :index
-      end
+      get :index
     end
   end
 end

spec/controllers/projects/blob_controller_spec.rb

@@ -424,7 +424,7 @@ RSpec.describe Projects::BlobController do
       end
     end
 
-    it_behaves_like 'tracking unique hll events', :track_editor_edit_actions do
+    it_behaves_like 'tracking unique hll events' do
       subject(:request) { put :update, params: default_params }
 
       let(:target_id) { 'g_edit_by_sfe' }
@@ -540,7 +540,7 @@ RSpec.describe Projects::BlobController do
       sign_in(user)
     end
 
-    it_behaves_like 'tracking unique hll events', :track_editor_edit_actions do
+    it_behaves_like 'tracking unique hll events' do
       subject(:request) { post :create, params: default_params }
 
       let(:target_id) { 'g_edit_by_sfe' }

spec/controllers/search_controller_spec.rb

@@ -183,7 +183,7 @@ RSpec.describe SearchController do
           allow(Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event)
         end
 
-        it_behaves_like 'tracking unique hll events', :search_track_unique_users do
+        it_behaves_like 'tracking unique hll events' do
           subject(:request) { get :show, params: { scope: 'projects', search: 'term' } }
 
           let(:target_id) { 'i_search_total' }

spec/controllers/snippets_controller_spec.rb

@@ -173,7 +173,7 @@ RSpec.describe SnippetsController do
           expect(response).to have_gitlab_http_status(:ok)
         end
 
-        it_behaves_like 'tracking unique hll events', :usage_data_i_snippets_show do
+        it_behaves_like 'tracking unique hll events' do
           subject(:request) { get :show, params: { id: public_snippet.to_param } }
 
           let(:target_id) { 'i_snippets_show' }

spec/lib/gitlab/usage_data_counters/ci_template_unique_counter_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe Gitlab::UsageDataCounters::CiTemplateUniqueCounter do
   describe '.track_unique_project_event' do
     described_class::TEMPLATE_TO_EVENT.keys.each do |template|
       context "when given template #{template}" do
-        it_behaves_like 'tracking unique hll events', :usage_data_track_ci_templates_unique_projects do
+        it_behaves_like 'tracking unique hll events' do
           subject(:request) { described_class.track_unique_project_event(project_id: project_id, template: template) }
 
           let(:target_id) { "p_ci_templates_#{described_class::TEMPLATE_TO_EVENT[template]}" }

spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb

@@ -48,6 +48,8 @@ RSpec.describe Gitlab::UsageDataCounters::HLLRedisCounter, :clean_gitlab_redis_s
   end
 
   describe 'known_events' do
+    let(:feature) { 'test_hll_redis_counter_ff_check' }
+
     let(:weekly_event) { 'g_analytics_contribution' }
     let(:daily_event) { 'g_analytics_search' }
     let(:analytics_slot_event) { 'g_analytics_contribution' }
@@ -67,7 +69,7 @@ RSpec.describe Gitlab::UsageDataCounters::HLLRedisCounter, :clean_gitlab_redis_s
 
     let(:known_events) do
       [
-        { name: weekly_event, redis_slot: "analytics", category: analytics_category, expiry: 84, aggregation: "weekly" },
+        { name: weekly_event, redis_slot: "analytics", category: analytics_category, expiry: 84, aggregation: "weekly", feature_flag: feature },
         { name: daily_event, redis_slot: "analytics", category: analytics_category, expiry: 84, aggregation: "daily" },
         { name: category_productivity_event, redis_slot: "analytics", category: productivity_category, aggregation: "weekly" },
         { name: compliance_slot_event, redis_slot: "compliance", category: compliance_category, aggregation: "weekly" },
@@ -78,6 +80,8 @@ RSpec.describe Gitlab::UsageDataCounters::HLLRedisCounter, :clean_gitlab_redis_s
     end
 
     before do
+      skip_feature_flags_yaml_validation
+      skip_default_enabled_yaml_check
       allow(described_class).to receive(:known_events).and_return(known_events)
     end
 
@@ -88,6 +92,32 @@ RSpec.describe Gitlab::UsageDataCounters::HLLRedisCounter, :clean_gitlab_redis_s
     end
 
     describe '.track_event' do
+      context 'with feature flag set' do
+        it 'tracks the event when feature enabled' do
+          stub_feature_flags(feature => true)
+
+          expect(Gitlab::Redis::HLL).to receive(:add)
+
+          described_class.track_event(weekly_event, values: 1)
+        end
+
+        it 'does not track the event with feature flag disabled' do
+          stub_feature_flags(feature => false)
+
+          expect(Gitlab::Redis::HLL).not_to receive(:add)
+
+          described_class.track_event(weekly_event, values: 1)
+        end
+      end
+
+      context 'with no feature flag set' do
+        it 'tracks the event' do
+          expect(Gitlab::Redis::HLL).to receive(:add)
+
+          described_class.track_event(daily_event, values: 1)
+        end
+      end
+
       context 'when usage_ping is disabled' do
         it 'does not track the event' do
           stub_application_setting(usage_ping_enabled: false)

spec/requests/api/terraform/state_spec.rb

@@ -39,7 +39,7 @@ RSpec.describe API::Terraform::State do
     context 'with maintainer permissions' do
       let(:current_user) { maintainer }
 
-      it_behaves_like 'tracking unique hll events', :usage_data_p_terraform_state_api_unique_users do
+      it_behaves_like 'tracking unique hll events' do
         let(:target_id) { 'p_terraform_state_api_unique_users' }
         let(:expected_type) { instance_of(Integer) }
       end

spec/support/shared_examples/controllers/unique_hll_events_examples.rb

@@ -5,7 +5,7 @@
 # - expected_type
 # - target_id
 
-RSpec.shared_examples 'tracking unique hll events' do |feature_flag|
+RSpec.shared_examples 'tracking unique hll events' do
   it 'tracks unique event' do
     expect(Gitlab::UsageDataCounters::HLLRedisCounter).to(
       receive(:track_event)
@@ -15,14 +15,4 @@ RSpec.shared_examples 'tracking unique hll events' do |feature_flag|
 
     request
   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)
-
-      request
-    end
-  end
 end

spec/support/shared_examples/controllers/wiki_actions_shared_examples.rb

@@ -218,7 +218,7 @@ RSpec.shared_examples 'wiki controller actions' do
       end
 
       context 'page view tracking' do
-        it_behaves_like 'tracking unique hll events', :track_unique_wiki_page_views do
+        it_behaves_like 'tracking unique hll events' do
           let(:target_id) { 'wiki_action' }
           let(:expected_type) { instance_of(String) }
         end