Merge branch '212402-harden-usage-ping-redis-counters-2' into 'master'

Harden Usage Ping | Redis Counters

Closes #212402

See merge request gitlab-org/gitlab!29474

ee/lib/ee/gitlab/usage_data.rb

@@ -148,7 +148,7 @@ module EE
           super.tap do |usage_data|
             usage_data[:counts].merge!(
               {
-                dependency_list_usages_total: ::Gitlab::UsageCounters::DependencyList.usage_totals[:total],
+                dependency_list_usages_total: redis_usage_data { ::Gitlab::UsageCounters::DependencyList.usage_totals[:total] },
                 epics: count(::Epic),
                 feature_flags: count(Operations::FeatureFlag),
                 geo_nodes: count(::GeoNode),
@@ -156,7 +156,7 @@ module EE
                 issues_with_health_status: count(::Issue.with_health_status),
                 ldap_keys: count(::LDAPKey),
                 ldap_users: count(::User.ldap, 'users.id'),
-                pod_logs_usages_total: ::Gitlab::UsageCounters::PodLogs.usage_totals[:total],
+                pod_logs_usages_total: redis_usage_data { ::Gitlab::UsageCounters::PodLogs.usage_totals[:total] },
                 projects_enforcing_code_owner_approval: count(::Project.without_deleted.non_archived.requiring_code_owner_approval),
                 merge_requests_with_optional_codeowners: distinct_count(::ApprovalMergeRequestRule.code_owner_approval_optional, :merge_request_id),
                 merge_requests_with_required_codeowners: distinct_count(::ApprovalMergeRequestRule.code_owner_approval_required, :merge_request_id),

ee/lib/gitlab/usage_counters/designs_counter.rb

@@ -25,7 +25,17 @@ module Gitlab::UsageCounters
       end
 
       def totals
-        KNOWN_EVENTS.map { |e| ["design_management_designs_#{e}".to_sym, read(e)] }.to_h
+        KNOWN_EVENTS.map { |event| [counter_key(event), read(event)] }.to_h
+      end
+
+      def fallback_totals
+        KNOWN_EVENTS.map { |event| [counter_key(event), -1] }.to_h
+      end
+
+      private
+
+      def counter_key(event)
+        "design_management_designs_#{event}".to_sym
       end
     end
   end

lib/gitlab/usage_data.rb

 # frozen_string_literal: true
 
-# For hardening usage ping and make it easier to add measures there is in place alt_usage_data method
-# which handles StandardError and fallbacks into -1
-# this way not all measures fail if we encounter one exception
+# For hardening usage ping and make it easier to add measures there is in place
+#   * alt_usage_data method
+#     handles StandardError and fallbacks into -1 this way not all measures fail if we encounter one exception
 #
 #     Examples:
 #     alt_usage_data { Gitlab::VERSION }
 #     alt_usage_data { Gitlab::CurrentSettings.uuid }
+#
+#   * redis_usage_data method
+#     handles ::Redis::CommandError, Gitlab::UsageDataCounters::BaseCounter::UnknownEvent
+#     returns -1 when a block is sent or hash with all values -1 when a counter is sent
+#     different behaviour due to 2 different implementations of redis counter
+#
+#     Examples:
+#     redis_usage_data(Gitlab::UsageDataCounters::WikiPageCounter)
+#     redis_usage_data { ::Gitlab::UsageCounters::PodLogs.usage_totals[:total] }
 module Gitlab
   class UsageData
     BATCH_SIZE = 100
@@ -192,7 +201,7 @@ module Gitlab
 
       # @return [Hash<Symbol, Integer>]
       def usage_counters
-        usage_data_counters.map(&:totals).reduce({}) { |a, b| a.merge(b) }
+        usage_data_counters.map { |counter| redis_usage_data(counter) }.reduce({}, :merge)
       end
 
       # @return [Array<#totals>] An array of objects that respond to `#totals`
@@ -388,8 +397,28 @@ module Gitlab
         fallback
       end
 
+      def redis_usage_data(counter = nil, &block)
+        if block_given?
+          redis_usage_counter(&block)
+        elsif counter.present?
+          redis_usage_data_totals(counter)
+        end
+      end
+
       private
 
+      def redis_usage_counter
+        yield
+      rescue ::Redis::CommandError, Gitlab::UsageDataCounters::BaseCounter::UnknownEvent
+        -1
+      end
+
+      def redis_usage_data_totals(counter)
+        counter.totals
+      rescue ::Redis::CommandError, Gitlab::UsageDataCounters::BaseCounter::UnknownEvent
+        counter.fallback_totals
+      end
+
       def installation_type
         if Rails.env.production?
           Gitlab::INSTALLATION_TYPE

lib/gitlab/usage_data_counters/base_counter.rb

@@ -22,11 +22,19 @@ module Gitlab::UsageDataCounters
       end
 
       def totals
-        known_events.map { |e| ["#{prefix}_#{e}".to_sym, read(e)] }.to_h
+        known_events.map { |event| [counter_key(event), read(event)] }.to_h
+      end
+
+      def fallback_totals
+        known_events.map { |event| [counter_key(event), -1] }.to_h
       end
 
       private
 
+      def counter_key(event)
+        "#{prefix}_#{event}".to_sym
+      end
+
       def known_events
         self::KNOWN_EVENTS
       end

lib/gitlab/usage_data_counters/note_counter.rb

@@ -25,12 +25,20 @@ module Gitlab::UsageDataCounters
 
       def totals
         COUNTABLE_TYPES.map do |countable_type|
-          [:"#{countable_type.underscore}_comment", read(:create, countable_type)]
+          [counter_key(countable_type), read(:create, countable_type)]
         end.to_h
       end
 
+      def fallback_totals
+        COUNTABLE_TYPES.map { |counter_key| [counter_key(counter_key), -1] }.to_h
+      end
+
       private
 
+      def counter_key(countable_type)
+        "#{countable_type.underscore}_comment".to_sym
+      end
+
       def countable?(noteable_type)
         COUNTABLE_TYPES.include?(noteable_type.to_s)
       end

lib/gitlab/usage_data_counters/search_counter.rb

@@ -21,6 +21,10 @@ module Gitlab
             navbar_searches: total_navbar_searches_count
           }
         end
+
+        def fallback_totals
+          { navbar_searches: -1 }
+        end
       end
     end
   end

lib/gitlab/usage_data_counters/web_ide_counter.rb

@@ -4,54 +4,44 @@ module Gitlab
   module UsageDataCounters
     class WebIdeCounter
       extend RedisCounter
-
-      COMMITS_COUNT_KEY = 'WEB_IDE_COMMITS_COUNT'
-      MERGE_REQUEST_COUNT_KEY = 'WEB_IDE_MERGE_REQUESTS_COUNT'
-      VIEWS_COUNT_KEY = 'WEB_IDE_VIEWS_COUNT'
-      PREVIEW_COUNT_KEY = 'WEB_IDE_PREVIEWS_COUNT'
+      KNOWN_EVENTS = %i[commits views merge_requests previews].freeze
+      PREFIX = 'web_ide'
 
       class << self
         def increment_commits_count
-          increment(COMMITS_COUNT_KEY)
-        end
-
-        def total_commits_count
-          total_count(COMMITS_COUNT_KEY)
+          increment(redis_key('commits'))
         end
 
         def increment_merge_requests_count
-          increment(MERGE_REQUEST_COUNT_KEY)
-        end
-
-        def total_merge_requests_count
-          total_count(MERGE_REQUEST_COUNT_KEY)
+          increment(redis_key('merge_requests'))
         end
 
         def increment_views_count
-          increment(VIEWS_COUNT_KEY)
-        end
-
-        def total_views_count
-          total_count(VIEWS_COUNT_KEY)
+          increment(redis_key('views'))
         end
 
         def increment_previews_count
           return unless Gitlab::CurrentSettings.web_ide_clientside_preview_enabled?
 
-          increment(PREVIEW_COUNT_KEY)
+          increment(redis_key('previews'))
         end
 
-        def total_previews_count
-          total_count(PREVIEW_COUNT_KEY)
+        def totals
+          KNOWN_EVENTS.map { |event| [counter_key(event), total_count(redis_key(event))] }.to_h
         end
 
-        def totals
-          {
-            web_ide_commits: total_commits_count,
-            web_ide_views: total_views_count,
-            web_ide_merge_requests: total_merge_requests_count,
-            web_ide_previews: total_previews_count
-          }
+        def fallback_totals
+          KNOWN_EVENTS.map { |event| [counter_key(event), -1] }.to_h
+        end
+
+        private
+
+        def redis_key(event)
+          "#{PREFIX}_#{event}_count".upcase
+        end
+
+        def counter_key(event)
+          "#{PREFIX}_#{event}".to_sym
         end
       end
     end

spec/controllers/projects/usage_ping_controller_spec.rb

@@ -42,7 +42,7 @@ describe Projects::UsagePingController do
         it 'increments the counter' do
           expect do
             subject
-          end.to change { Gitlab::UsageDataCounters::WebIdeCounter.total_previews_count }.by(1)
+          end.to change { Gitlab::UsageDataCounters::WebIdeCounter.total_count('WEB_IDE_PREVIEWS_COUNT') }.by(1)
         end
       end
     end

spec/lib/gitlab/usage_data_counters/web_ide_counter_spec.rb

@@ -3,35 +3,27 @@
 require 'spec_helper'
 
 describe Gitlab::UsageDataCounters::WebIdeCounter, :clean_gitlab_redis_shared_state do
-  shared_examples 'counter examples' do
+  shared_examples 'counter examples' do |event|
     it 'increments counter and return the total count' do
-      expect(described_class.public_send(total_counter_method)).to eq(0)
+      expect(described_class.public_send(:total_count, event)).to eq(0)
 
-      2.times { described_class.public_send(increment_counter_method) }
+      2.times { described_class.public_send(:"increment_#{event}_count") }
 
-      expect(described_class.public_send(total_counter_method)).to eq(2)
+      redis_key = "web_ide_#{event}_count".upcase
+      expect(described_class.public_send(:total_count, redis_key)).to eq(2)
     end
   end
 
   describe 'commits counter' do
-    let(:increment_counter_method) { :increment_commits_count }
-    let(:total_counter_method) { :total_commits_count }
-
-    it_behaves_like 'counter examples'
+    it_behaves_like 'counter examples', 'commits'
   end
 
   describe 'merge requests counter' do
-    let(:increment_counter_method) { :increment_merge_requests_count }
-    let(:total_counter_method) { :total_merge_requests_count }
-
-    it_behaves_like 'counter examples'
+    it_behaves_like 'counter examples', 'merge_requests'
   end
 
   describe 'views counter' do
-    let(:increment_counter_method) { :increment_views_count }
-    let(:total_counter_method) { :total_views_count }
-
-    it_behaves_like 'counter examples'
+    it_behaves_like 'counter examples', 'views'
   end
 
   describe 'previews counter' do
@@ -42,21 +34,19 @@ describe Gitlab::UsageDataCounters::WebIdeCounter, :clean_gitlab_redis_shared_st
     end
 
     context 'when web ide clientside preview is enabled' do
-      let(:increment_counter_method) { :increment_previews_count }
-      let(:total_counter_method) { :total_previews_count }
-
-      it_behaves_like 'counter examples'
+      it_behaves_like 'counter examples', 'previews'
     end
 
     context 'when web ide clientside preview is not enabled' do
       let(:setting_enabled) { false }
 
       it 'does not increment the counter' do
-        expect(described_class.total_previews_count).to eq(0)
+        redis_key = 'WEB_IDE_PREVIEWS_COUNT'
+        expect(described_class.total_count(redis_key)).to eq(0)
 
         2.times { described_class.increment_previews_count }
 
-        expect(described_class.total_previews_count).to eq(0)
+        expect(described_class.total_count(redis_key)).to eq(0)
       end
     end
   end

spec/lib/gitlab/usage_data_spec.rb

@@ -116,6 +116,7 @@ describe Gitlab::UsageData, :aggregate_failures do
       subject { described_class.usage_data_counters }
 
       it { is_expected.to all(respond_to :totals) }
+      it { is_expected.to all(respond_to :fallback_totals) }
 
       describe 'the results of calling #totals on all objects in the array' do
         subject { described_class.usage_data_counters.map(&:totals) }
@@ -124,6 +125,13 @@ describe Gitlab::UsageData, :aggregate_failures do
         it { is_expected.to all(have_attributes(keys: all(be_a Symbol), values: all(be_a Integer))) }
       end
 
+      describe 'the results of calling #fallback_totals on all objects in the array' do
+        subject { described_class.usage_data_counters.map(&:fallback_totals) }
+
+        it { is_expected.to all(be_a Hash) }
+        it { is_expected.to all(have_attributes(keys: all(be_a Symbol), values: all(eq(-1)))) }
+      end
+
       it 'does not have any conflicts' do
         all_keys = subject.flat_map { |counter| counter.totals.keys }
 
@@ -576,4 +584,28 @@ describe Gitlab::UsageData, :aggregate_failures do
       expect(described_class.alt_usage_data(1)).to eq 1
     end
   end
+
+  describe '#redis_usage_data' do
+    context 'with block given' do
+      it 'returns the fallback when it gets an error' do
+        expect(described_class.redis_usage_data { raise ::Redis::CommandError } ).to eq(-1)
+      end
+
+      it 'returns the evaluated block when given' do
+        expect(described_class.redis_usage_data { 1 }).to eq(1)
+      end
+    end
+
+    context 'with counter given' do
+      it 'returns the falback values for all counter keys when it gets an error' do
+        allow(::Gitlab::UsageDataCounters::WikiPageCounter).to receive(:totals).and_raise(::Redis::CommandError)
+        expect(described_class.redis_usage_data(::Gitlab::UsageDataCounters::WikiPageCounter)).to eql(::Gitlab::UsageDataCounters::WikiPageCounter.fallback_totals)
+      end
+
+      it 'returns the totals when couter is given' do
+        allow(::Gitlab::UsageDataCounters::WikiPageCounter).to receive(:totals).and_return({ wiki_pages_create: 2 })
+        expect(described_class.redis_usage_data(::Gitlab::UsageDataCounters::WikiPageCounter)).to eql({ wiki_pages_create: 2 })
+      end
+    end
+  end
 end