Merge branch '224094-analytics-usage-past-month' into 'master'

Add monthly usage data for analytics targets

Closes #224094

See merge request gitlab-org/gitlab!37417

app/controllers/dashboard/todos_controller.rb

@@ -9,8 +9,6 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   before_action :authorize_read_group!, only: :index
   before_action :find_todos, only: [:index, :destroy_all]
 
-  track_unique_visits :index, target_id: 'u_todos'
-
   def index
     @sort = params[:sort]
     @todos = @todos.page(params[:page])

changelogs/unreleased/224094-analytics-usage-past-month.yml

+---
+title: Add monthly usage ping data for analytics
+merge_request: 37417
+author:
+type: added

doc/development/redis.md

@@ -41,8 +41,16 @@ moment, but may wish to in the future: [#118820](https://gitlab.com/gitlab-org/g
 This imposes an additional constraint on naming: where GitLab is performing
 operations that require several keys to be held on the same Redis server - for
 instance, diffing two sets held in Redis - the keys should ensure that by
-enclosing the changeable parts in curly braces, such as, `project:{1}:set_a` and
-`project:{1}:set_b`.
+enclosing the changeable parts in curly braces.
+For example:
+
+```plaintext
+project:{1}:set_a
+project:{1}:set_b
+project:{2}:set_c
+```
+
+`set_a` and `set_b` are guaranteed to be held on the same Redis server, while `set_c` is not.
 
 Currently, we validate this in the development and test environments
 with the [`RedisClusterValidator`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/instrumentation/redis_cluster_validator.rb),

doc/development/telemetry/usage_ping.md

@@ -618,7 +618,6 @@ appear to be associated to any of the services running, since they all appear to
 | `sd`                                                      | `avg_cycle_analytics - production`   |               |                  |         |                                                                            |
 | `missing`                                                 | `avg_cycle_analytics - production`   |               |                  |         |                                                                            |
 | `total`                                                   | `avg_cycle_analytics`                |               |                  |         |                                                                            |
-| `u_todos`                                                 |                                      | `manage`      |                  |         | Visits to /dashboard/todos                                                 |
 | `g_analytics_contribution`                                | `analytics_unique_visits`            | `manage`      |                  |         | Visits to /groups/:group/-/contribution_analytics                          |
 | `g_analytics_insights`                                    | `analytics_unique_visits`            | `manage`      |                  |         | Visits to /groups/:group/-/insights                                        |
 | `g_analytics_issues`                                      | `analytics_unique_visits`            | `manage`      |                  |         | Visits to /groups/:group/-/issues_analytics                                |
@@ -632,7 +631,8 @@ appear to be associated to any of the services running, since they all appear to
 | `p_analytics_repo`                                        | `analytics_unique_visits`            | `manage`      |                  |         | Visits to /:group/:project/-/graphs/master/charts                          |
 | `i_analytics_cohorts`                                     | `analytics_unique_visits`            | `manage`      |                  |         | Visits to /-/instance_statistics/cohorts                                   |
 | `i_analytics_dev_ops_score`                               | `analytics_unique_visits`            | `manage`      |                  |         | Visits to /-/instance_statistics/dev_ops_score                             |
-| `analytics_unique_visits_for_any_target`                  | `analytics_unique_visits`            | `manage`      |                  |         | Visits to any of the pages listed above                                    |
+| `analytics_unique_visits_for_any_target`                  | `analytics_unique_visits`            | `manage`      |                  |         | Visits to any of the pages listed above per week                           |
+| `analytics_unique_visits_for_any_target_monthly`          | `analytics_unique_visits`            | `manage`      |                  |         | Visits to any of the pages listed above per 28 days                        |
 | `clusters_applications_cert_managers`                     | `usage_activity_by_stage`            | `configure`   |                  | CE+EE   | Unique clusters with certificate managers enabled                          |
 | `clusters_applications_helm`                              | `usage_activity_by_stage`            | `configure`   |                  | CE+EE   | Unique clusters with Helm enabled                                          |
 | `clusters_applications_ingress`                           | `usage_activity_by_stage`            | `configure`   |                  | CE+EE   | Unique clusters with Ingress enabled                                       |

lib/gitlab/analytics/unique_visits.rb

@@ -15,12 +15,11 @@ module Gitlab
         'p_analytics_insights',
         'p_analytics_issues',
         'p_analytics_repo',
-        'u_todos',
         'i_analytics_cohorts',
         'i_analytics_dev_ops_score'
       ].freeze
 
-      KEY_EXPIRY_LENGTH = 28.days
+      KEY_EXPIRY_LENGTH = 12.weeks
 
       def track_visit(visitor_id, target_id, time = Time.zone.now)
         target_key = key(target_id, time)
@@ -28,16 +27,24 @@ module Gitlab
         Gitlab::Redis::HLL.add(key: target_key, value: visitor_id, expiry: KEY_EXPIRY_LENGTH)
       end
 
-      def weekly_unique_visits_for_target(target_id, week_of: 7.days.ago)
-        target_key = key(target_id, week_of)
+      # 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
+      # @param [ActiveSupport::TimeWithZone] start_week start of time frame
+      # @param [Integer] weeks time frame length in weeks
+      # @return [Integer] number of unique visitors
+      def unique_visits_for(targets:, start_week: 7.days.ago, weeks: 1)
+        target_ids = if targets == :any
+                       TARGET_IDS
+                     else
+                       Array(targets)
+                     end
 
-        Gitlab::Redis::HLL.count(keys: [target_key])
-      end
+        timeframe_start = [start_week, weeks.weeks.ago].min
 
-      def weekly_unique_visits_for_any_target(week_of: 7.days.ago)
-        keys = TARGET_IDS.select { |id| id =~ /_analytics_/ }.map { |target_id| key(target_id, week_of) }
+        redis_keys = keys(targets: target_ids, timeframe_start: timeframe_start, weeks: weeks)
 
-        Gitlab::Redis::HLL.count(keys: keys)
+        Gitlab::Redis::HLL.count(keys: redis_keys)
       end
 
       private
@@ -45,8 +52,16 @@ module Gitlab
       def key(target_id, time)
         raise "Invalid target id #{target_id}" unless TARGET_IDS.include?(target_id.to_s)
 
+        target_key = target_id.to_s.gsub('analytics', '{analytics}')
         year_week = time.strftime('%G-%V')
-        "#{target_id}-{#{year_week}}"
+
+        "#{target_key}-#{year_week}"
+      end
+
+      def keys(targets:, timeframe_start:, weeks:)
+        (0..(weeks - 1)).map do |week_increment|
+          targets.map { |target_id| key(target_id, timeframe_start + week_increment * 7.days) }
+        end.flatten
       end
     end
   end

lib/gitlab/usage_data.rb

@@ -573,9 +573,10 @@ module Gitlab
 
       def analytics_unique_visits_data
         results = ::Gitlab::Analytics::UniqueVisits::TARGET_IDS.each_with_object({}) do |target_id, hash|
-          hash[target_id] = redis_usage_data { unique_visit_service.weekly_unique_visits_for_target(target_id) }
+          hash[target_id] = redis_usage_data { unique_visit_service.unique_visits_for(targets: target_id) }
         end
-        results['analytics_unique_visits_for_any_target'] = redis_usage_data { unique_visit_service.weekly_unique_visits_for_any_target }
+        results['analytics_unique_visits_for_any_target'] = redis_usage_data { unique_visit_service.unique_visits_for(targets: :any) }
+        results['analytics_unique_visits_for_any_target_monthly'] = redis_usage_data { unique_visit_service.unique_visits_for(targets: :any, weeks: 4) }
 
         { analytics_unique_visits: results }
       end

spec/controllers/dashboard/todos_controller_spec.rb

@@ -42,15 +42,6 @@ RSpec.describe Dashboard::TodosController do
 
         expect(response).to have_gitlab_http_status(:ok)
       end
-
-      context 'tracking visits' do
-        let_it_be(:authorized_project) { create(:project, :public) }
-
-        it_behaves_like 'tracking unique visits', :index do
-          let(:request_params) { { project_id: authorized_project.id } }
-          let(:target_id) { 'u_todos' }
-        end
-      end
     end
 
     context "with render_views" do

spec/lib/gitlab/analytics/unique_visits_spec.rb

@@ -29,24 +29,26 @@ RSpec.describe Gitlab::Analytics::UniqueVisits, :clean_gitlab_redis_shared_state
       unique_visits.track_visit(visitor1_id, target2_id, 8.days.ago)
       unique_visits.track_visit(visitor1_id, target2_id, 15.days.ago)
 
-      expect(unique_visits.weekly_unique_visits_for_target(target1_id)).to eq(2)
-      expect(unique_visits.weekly_unique_visits_for_target(target2_id)).to eq(1)
+      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.weekly_unique_visits_for_target(target2_id, week_of: 15.days.ago)).to eq(1)
+      expect(unique_visits.unique_visits_for(targets: target2_id, start_week: 15.days.ago)).to eq(1)
 
-      expect(unique_visits.weekly_unique_visits_for_target(target3_id)).to eq(0)
+      expect(unique_visits.unique_visits_for(targets: target3_id)).to eq(0)
 
-      expect(unique_visits.weekly_unique_visits_for_any_target).to eq(2)
-      expect(unique_visits.weekly_unique_visits_for_any_target(week_of: 15.days.ago)).to eq(1)
-      expect(unique_visits.weekly_unique_visits_for_any_target(week_of: 30.days.ago)).to eq(0)
+      expect(unique_visits.unique_visits_for(targets: :any)).to eq(2)
+      expect(unique_visits.unique_visits_for(targets: :any, start_week: 15.days.ago)).to eq(1)
+      expect(unique_visits.unique_visits_for(targets: :any, start_week: 30.days.ago)).to eq(0)
+
+      expect(unique_visits.unique_visits_for(targets: :any, weeks: 4)).to eq(2)
     end
 
-    it 'sets the keys in Redis to expire automatically after 28 days' do
+    it 'sets the keys in Redis to expire automatically after 12 weeks' do
       unique_visits.track_visit(visitor1_id, target1_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(28.days)
+        redis.scan_each(match: "{#{target1_id}}-*").each do |key|
+          expect(redis.ttl(key)).to be_within(5.seconds).of(12.weeks)
         end
       end
     end

spec/lib/gitlab/usage_data_spec.rb

@@ -953,10 +953,11 @@ RSpec.describe Gitlab::UsageData, :aggregate_failures do
 
     it 'returns the number of unique visits to pages with analytics features' do
       ::Gitlab::Analytics::UniqueVisits::TARGET_IDS.each do |target_id|
-        expect_any_instance_of(::Gitlab::Analytics::UniqueVisits).to receive(:weekly_unique_visits_for_target).with(target_id).and_return(123)
+        expect_any_instance_of(::Gitlab::Analytics::UniqueVisits).to receive(:unique_visits_for).with(targets: target_id).and_return(123)
       end
 
-      expect_any_instance_of(::Gitlab::Analytics::UniqueVisits).to receive(:weekly_unique_visits_for_any_target).and_return(543)
+      expect_any_instance_of(::Gitlab::Analytics::UniqueVisits).to receive(:unique_visits_for).with(targets: :any).and_return(543)
+      expect_any_instance_of(::Gitlab::Analytics::UniqueVisits).to receive(:unique_visits_for).with(targets: :any, weeks: 4).and_return(987)
 
       expect(subject).to eq({
         analytics_unique_visits: {
@@ -971,10 +972,10 @@ RSpec.describe Gitlab::UsageData, :aggregate_failures do
           'p_analytics_insights' => 123,
           'p_analytics_issues' => 123,
           'p_analytics_repo' => 123,
-          'u_todos' => 123,
           'i_analytics_cohorts' => 123,
           'i_analytics_dev_ops_score' => 123,
-          'analytics_unique_visits_for_any_target' => 543
+          'analytics_unique_visits_for_any_target' => 543,
+          'analytics_unique_visits_for_any_target_monthly' => 987
         }
       })
     end