Merge branch '325517-replace-usage-of-unique-visits-helper' into 'master'

Replace UniqueVisitsHelper with RedisTracking

See merge request gitlab-org/gitlab!65594

app/controllers/admin/cohorts_controller.rb

 # frozen_string_literal: true
 
 class Admin::CohortsController < Admin::ApplicationController
-  include Analytics::UniqueVisitsHelper
+  include RedisTracking
 
   feature_category :devops_reports
 
@@ -21,6 +21,6 @@ class Admin::CohortsController < Admin::ApplicationController
   end
 
   def track_cohorts_visit
-    track_visit('i_analytics_cohorts') if trackable_html_request?
+    track_unique_redis_hll_event('i_analytics_cohorts') if trackable_html_request?
   end
 end

app/controllers/admin/usage_trends_controller.rb

 # frozen_string_literal: true
 
 class Admin::UsageTrendsController < Admin::ApplicationController
-  include Analytics::UniqueVisitsHelper
+  include RedisTracking
 
-  track_unique_visits :index, target_id: 'i_analytics_instance_statistics'
+  track_redis_hll_event :index, name: 'i_analytics_instance_statistics'
 
   feature_category :devops_reports
 

app/controllers/dashboard/todos_controller.rb

@@ -3,7 +3,6 @@
 class Dashboard::TodosController < Dashboard::ApplicationController
   include ActionView::Helpers::NumberHelper
   include PaginatedCollection
-  include Analytics::UniqueVisitsHelper
 
   before_action :authorize_read_project!, only: :index
   before_action :authorize_read_group!, only: :index

app/controllers/projects/cycle_analytics_controller.rb

@@ -4,12 +4,12 @@ class Projects::CycleAnalyticsController < Projects::ApplicationController
   include ActionView::Helpers::DateHelper
   include ActionView::Helpers::TextHelper
   include CycleAnalyticsParams
-  include Analytics::UniqueVisitsHelper
   include GracefulTimeoutHandling
+  include RedisTracking
 
   before_action :authorize_read_cycle_analytics!
 
-  track_unique_visits :show, target_id: 'p_analytics_valuestream'
+  track_redis_hll_event :show, name: 'p_analytics_valuestream'
 
   feature_category :planning_analytics
 

app/controllers/projects/graphs_controller.rb

@@ -2,14 +2,14 @@
 
 class Projects::GraphsController < Projects::ApplicationController
   include ExtractsPath
-  include Analytics::UniqueVisitsHelper
+  include RedisTracking
 
   # Authorize
   before_action :require_non_empty_project
   before_action :assign_ref_vars
   before_action :authorize_read_repository_graphs!
 
-  track_unique_visits :charts, target_id: 'p_analytics_repo'
+  track_redis_hll_event :charts, name: 'p_analytics_repo'
 
   feature_category :source_code_management
 

app/controllers/projects/pipelines_controller.rb

@@ -2,7 +2,7 @@
 
 class Projects::PipelinesController < Projects::ApplicationController
   include ::Gitlab::Utils::StrongMemoize
-  include Analytics::UniqueVisitsHelper
+  include RedisTracking
 
   before_action :disable_query_limiting, only: [:create, :retry]
   before_action :pipeline, except: [:index, :new, :create, :charts, :config_variables]
@@ -24,7 +24,7 @@ class Projects::PipelinesController < Projects::ApplicationController
 
   around_action :allow_gitaly_ref_name_caching, only: [:index, :show]
 
-  track_unique_visits :charts, target_id: 'p_analytics_pipelines'
+  track_redis_hll_event :charts, name: 'p_analytics_pipelines'
 
   wrap_parameters Ci::Pipeline
 

app/helpers/analytics/unique_visits_helper.rb

-# frozen_string_literal: true
-
-module Analytics
-  module UniqueVisitsHelper
-    include Gitlab::Tracking::Helpers
-    extend ActiveSupport::Concern
-
-    def visitor_id
-      return cookies[:visitor_id] if cookies[:visitor_id].present?
-      return unless current_user
-
-      uuid = SecureRandom.uuid
-      cookies[:visitor_id] = { value: uuid, expires: 24.months }
-      uuid
-    end
-
-    def track_visit(target_id)
-      return unless visitor_id
-
-      Gitlab::Analytics::UniqueVisits.new.track_visit(target_id, values: visitor_id)
-    end
-
-    class_methods do
-      def track_unique_visits(controller_actions, target_id:)
-        after_action only: controller_actions, if: -> { trackable_html_request? } do
-          track_visit(target_id)
-        end
-      end
-    end
-  end
-end

ee/app/controllers/admin/audit_logs_controller.rb

@@ -6,12 +6,12 @@ class Admin::AuditLogsController < Admin::ApplicationController
   include AuditEvents::AuditLogsParams
   include AuditEvents::Sortable
   include AuditEvents::DateRange
-  include Analytics::UniqueVisitsHelper
   include Gitlab::Tracking
+  include RedisTracking
 
   before_action :check_license_admin_audit_log_available!
 
-  track_unique_visits :index, target_id: 'i_compliance_audit_events'
+  track_redis_hll_event :index, name: 'i_compliance_audit_events'
 
   feature_category :audit_events
 

ee/app/controllers/admin/credentials_controller.rb

@@ -3,14 +3,14 @@
 class Admin::CredentialsController < Admin::ApplicationController
   extend ::Gitlab::Utils::Override
   include CredentialsInventoryActions
-  include Analytics::UniqueVisitsHelper
+  include RedisTracking
 
   helper_method :credentials_inventory_path, :user_detail_path, :personal_access_token_revoke_path,
                 :ssh_key_delete_path, :gpg_keys_available?
 
   before_action :check_license_credentials_inventory_available!, only: [:index, :revoke, :destroy]
 
-  track_unique_visits :index, target_id: 'i_compliance_credential_inventory'
+  track_redis_hll_event :index, name: 'i_compliance_credential_inventory'
 
   feature_category :compliance_management
 

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

 # frozen_string_literal: true
 
 class Groups::Analytics::CycleAnalyticsController < Groups::Analytics::ApplicationController
-  include Analytics::UniqueVisitsHelper
   include CycleAnalyticsParams
+  include RedisTracking
   extend ::Gitlab::Utils::Override
 
   increment_usage_counter Gitlab::UsageDataCounters::CycleAnalyticsCounter, :views, only: :show
@@ -18,7 +18,7 @@ class Groups::Analytics::CycleAnalyticsController < Groups::Analytics::Applicati
 
   layout 'group'
 
-  track_unique_visits :show, target_id: 'g_analytics_valuestream'
+  track_redis_hll_event :show, name: 'g_analytics_valuestream'
 
   private
 

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

@@ -20,9 +20,9 @@ class Groups::Analytics::ProductivityAnalyticsController < Groups::Analytics::Ap
   before_action :validate_params, only: :show, if: -> { request.format.json? }
 
   include IssuableCollections
-  include Analytics::UniqueVisitsHelper
+  include RedisTracking
 
-  track_unique_visits :show, target_id: 'g_analytics_productivity'
+  track_redis_hll_event :show, name: 'g_analytics_productivity'
 
   def show
     respond_to do |format|

ee/app/controllers/groups/audit_events_controller.rb

@@ -6,11 +6,11 @@ class Groups::AuditEventsController < Groups::ApplicationController
   include AuditEvents::AuditLogsParams
   include AuditEvents::Sortable
   include AuditEvents::DateRange
-  include Analytics::UniqueVisitsHelper
+  include RedisTracking
 
   before_action :check_audit_events_available!
 
-  track_unique_visits :index, target_id: 'g_compliance_audit_events'
+  track_redis_hll_event :index, name: 'g_compliance_audit_events'
 
   feature_category :audit_events
 

ee/app/controllers/groups/contribution_analytics_controller.rb

 # frozen_string_literal: true
 
 class Groups::ContributionAnalyticsController < Groups::ApplicationController
-  include Analytics::UniqueVisitsHelper
+  include RedisTracking
 
   before_action :group
   before_action :check_contribution_analytics_available!
@@ -9,7 +9,7 @@ class Groups::ContributionAnalyticsController < Groups::ApplicationController
 
   layout 'group'
 
-  track_unique_visits :show, target_id: 'g_analytics_contribution'
+  track_redis_hll_event :show, name: 'g_analytics_contribution'
 
   feature_category :planning_analytics
 

ee/app/controllers/groups/insights_controller.rb

@@ -2,12 +2,12 @@
 
 class Groups::InsightsController < Groups::ApplicationController
   include InsightsActions
-  include Analytics::UniqueVisitsHelper
+  include RedisTracking
 
   before_action :authorize_read_group!
   before_action :authorize_read_insights_config_project!
 
-  track_unique_visits :show, target_id: 'g_analytics_insights'
+  track_redis_hll_event :show, name: 'g_analytics_insights'
 
   feature_category :insights
 

ee/app/controllers/groups/issues_analytics_controller.rb

@@ -2,12 +2,12 @@
 
 class Groups::IssuesAnalyticsController < Groups::ApplicationController
   include IssuableCollections
-  include Analytics::UniqueVisitsHelper
+  include RedisTracking
 
   before_action :authorize_read_group!
   before_action :authorize_read_issue_analytics!
 
-  track_unique_visits :show, target_id: 'g_analytics_issues'
+  track_redis_hll_event :show, name: 'g_analytics_issues'
 
   feature_category :planning_analytics
 

ee/app/controllers/groups/security/compliance_dashboards_controller.rb

 # frozen_string_literal: true
 class Groups::Security::ComplianceDashboardsController < Groups::ApplicationController
   include Groups::SecurityFeaturesHelper
-  include Analytics::UniqueVisitsHelper
+  include RedisTracking
 
   layout 'group'
 
   before_action :authorize_compliance_dashboard!
 
-  track_unique_visits :show, target_id: 'g_compliance_dashboard'
+  track_redis_hll_event :show, name: 'g_compliance_dashboard'
 
   feature_category :compliance_management
 

ee/app/controllers/projects/analytics/code_reviews_controller.rb

@@ -3,11 +3,11 @@
 module Projects
   module Analytics
     class CodeReviewsController < Projects::ApplicationController
-      include ::Analytics::UniqueVisitsHelper
+      include RedisTracking
 
       before_action :authorize_read_code_review_analytics!
 
-      track_unique_visits :index, target_id: 'p_analytics_code_reviews'
+      track_redis_hll_event :index, name: 'p_analytics_code_reviews'
 
       feature_category :planning_analytics
 

ee/app/controllers/projects/analytics/issues_analytics_controller.rb

@@ -2,11 +2,11 @@
 
 class Projects::Analytics::IssuesAnalyticsController < Projects::ApplicationController
   include IssuableCollections
-  include ::Analytics::UniqueVisitsHelper
+  include RedisTracking
 
   before_action :authorize_read_issue_analytics!
 
-  track_unique_visits :show, target_id: 'p_analytics_issues'
+  track_redis_hll_event :show, name: 'p_analytics_issues'
 
   feature_category :planning_analytics
 

ee/app/controllers/projects/analytics/merge_request_analytics_controller.rb

 # frozen_string_literal: true
 
 class Projects::Analytics::MergeRequestAnalyticsController < Projects::ApplicationController
-  include Analytics::UniqueVisitsHelper
+  include RedisTracking
 
   before_action :authorize_read_project_merge_request_analytics!
 
-  track_unique_visits :show, target_id: 'p_analytics_merge_request'
+  track_redis_hll_event :show, name: 'p_analytics_merge_request'
 
   feature_category :planning_analytics
 

ee/app/controllers/projects/insights_controller.rb

@@ -2,14 +2,14 @@
 
 class Projects::InsightsController < Projects::ApplicationController
   include InsightsActions
-  include Analytics::UniqueVisitsHelper
+  include RedisTracking
 
   helper_method :project_insights_config
 
   before_action :authorize_read_project!
   before_action :authorize_read_insights!
 
-  track_unique_visits :show, target_id: 'p_analytics_insights'
+  track_redis_hll_event :show, name: 'p_analytics_insights'
 
   feature_category :insights
 

lib/gitlab/analytics/unique_visits.rb

@@ -3,10 +3,6 @@
 module Gitlab
   module Analytics
     class UniqueVisits
-      def track_visit(*args, **kwargs)
-        Gitlab::UsageDataCounters::HLLRedisCounter.track_event(*args, **kwargs)
-      end
-
       # Returns number of unique visitors for given targets in given time frame
       #
       # @param [String, Array[<String>]] targets ids of targets to count visits on. Special case for :any

spec/helpers/analytics/unique_visits_helper_spec.rb

-# frozen_string_literal: true
-
-require "spec_helper"
-
-RSpec.describe Analytics::UniqueVisitsHelper do
-  include Devise::Test::ControllerHelpers
-
-  describe '#track_visit' do
-    let(:target_id) { 'p_analytics_valuestream' }
-    let(:current_user) { create(:user) }
-
-    it 'does not track visit if user is not logged in' do
-      expect_any_instance_of(Gitlab::Analytics::UniqueVisits).not_to receive(:track_visit)
-
-      helper.track_visit(target_id)
-    end
-
-    it 'tracks visit if user is logged in' do
-      sign_in(current_user)
-
-      expect_any_instance_of(Gitlab::Analytics::UniqueVisits).to receive(:track_visit)
-
-      helper.track_visit(target_id)
-    end
-
-    it 'tracks visit if user is not logged in, but has the cookie already' do
-      helper.request.cookies[:visitor_id] = { value: SecureRandom.uuid, expires: 24.months }
-
-      expect_any_instance_of(Gitlab::Analytics::UniqueVisits).to receive(:track_visit)
-
-      helper.track_visit(target_id)
-    end
-  end
-end

spec/lib/gitlab/analytics/unique_visits_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Gitlab::Analytics::UniqueVisits, :clean_gitlab_redis_shared_state do
-  let(:unique_visits) { Gitlab::Analytics::UniqueVisits.new }
-  let(:target1_id) { 'g_analytics_contribution' }
-  let(:target2_id) { 'g_analytics_insights' }
-  let(:target3_id) { 'g_analytics_issues' }
-  let(:target4_id) { 'g_compliance_dashboard' }
-  let(:target5_id) { 'i_compliance_credential_inventory' }
-  let(:visitor1_id) { 'dfb9d2d2-f56c-4c77-8aeb-6cddc4a1f857' }
-  let(:visitor2_id) { '1dd9afb2-a3ee-4de1-8ae3-a405579c8584' }
-  let(:visitor3_id) { '34rfjuuy-ce56-sa35-ds34-dfer567dfrf2' }
-
-  around do |example|
-    # We need to freeze to a reference time
-    # because visits are grouped by the week number in the year
-    # Without freezing the time, the test may behave inconsistently
-    # depending on which day of the week test is run.
-    reference_time = Time.utc(2020, 6, 1)
-    travel_to(reference_time) { example.run }
-  end
-
-  describe '#track_visit' do
-    it 'tracks the unique weekly visits for targets' do
-      unique_visits.track_visit(target1_id, values: visitor1_id, time: 7.days.ago)
-      unique_visits.track_visit(target1_id, values: visitor1_id, time: 7.days.ago)
-      unique_visits.track_visit(target1_id, values: visitor2_id, time: 7.days.ago)
-
-      unique_visits.track_visit(target2_id, values: visitor2_id, time: 7.days.ago)
-      unique_visits.track_visit(target2_id, values: visitor1_id, time: 8.days.ago)
-      unique_visits.track_visit(target2_id, values: visitor1_id, time: 15.days.ago)
-
-      unique_visits.track_visit(target4_id, values: visitor3_id, time: 7.days.ago)
-
-      unique_visits.track_visit(target5_id, values: visitor3_id, time: 15.days.ago)
-      unique_visits.track_visit(target5_id, values: visitor2_id, time: 15.days.ago)
-
-      expect(unique_visits.unique_visits_for(targets: target1_id)).to eq(2)
-      expect(unique_visits.unique_visits_for(targets: target2_id)).to eq(1)
-      expect(unique_visits.unique_visits_for(targets: target4_id)).to eq(1)
-
-      expect(unique_visits.unique_visits_for(targets: target2_id, start_date: 15.days.ago)).to eq(1)
-
-      expect(unique_visits.unique_visits_for(targets: target3_id)).to eq(0)
-
-      expect(unique_visits.unique_visits_for(targets: target5_id, start_date: 15.days.ago)).to eq(2)
-
-      expect(unique_visits.unique_visits_for(targets: :analytics)).to eq(2)
-      expect(unique_visits.unique_visits_for(targets: :analytics, start_date: 15.days.ago)).to eq(1)
-      expect(unique_visits.unique_visits_for(targets: :analytics, start_date: 30.days.ago)).to eq(0)
-
-      expect(unique_visits.unique_visits_for(targets: :analytics, start_date: 4.weeks.ago, end_date: Date.current)).to eq(2)
-
-      expect(unique_visits.unique_visits_for(targets: :compliance)).to eq(1)
-      expect(unique_visits.unique_visits_for(targets: :compliance, start_date: 15.days.ago)).to eq(2)
-      expect(unique_visits.unique_visits_for(targets: :compliance, start_date: 30.days.ago)).to eq(0)
-
-      expect(unique_visits.unique_visits_for(targets: :compliance, start_date: 4.weeks.ago, end_date: Date.current)).to eq(2)
-    end
-
-    it 'sets the keys in Redis to expire automatically after 12 weeks' do
-      unique_visits.track_visit(target1_id, values: visitor1_id)
-
-      Gitlab::Redis::SharedState.with do |redis|
-        redis.scan_each(match: "{#{target1_id}}-*").each do |key|
-          expect(redis.ttl(key)).to be_within(5.seconds).of(12.weeks)
-        end
-      end
-    end
-
-    it 'raises an error if an invalid target id is given' do
-      invalid_target_id = "x_invalid"
-
-      expect do
-        unique_visits.track_visit(invalid_target_id, values: visitor1_id)
-      end.to raise_error(Gitlab::UsageDataCounters::HLLRedisCounter::UnknownEvent)
-    end
-  end
-end