Code Review: Tidy up specs and change redis hash key

Issue: gitlab.com/gitlab-org/gitlab/-/issues/219956
Merge Request: gitlab.com/gitlab-org/gitlab/-/merge_requests/35580

app/services/event_create_service.rb

@@ -176,14 +176,18 @@ class EventCreateService
       action = Event.actions[status]
       raise IllegalActionError, "#{status} is not a valid status" if action.nil?
 
-      Gitlab::UsageDataCounters::TrackUniqueActions.track_action(event_action: status, event_target: record.class, author_id: current_user.id)
-
       parent_attrs(record.resource_parent)
         .merge(base_attrs)
         .merge(action: action, target_id: record.id, target_type: record.class.name)
     end
 
-    Event.insert_all(attribute_sets, returning: %w[id])
+    result = Event.insert_all(attribute_sets, returning: %w[id])
+
+    pairs.each do |record, status|
+      Gitlab::UsageDataCounters::TrackUniqueActions.track_action(event_action: status, event_target: record.class, author_id: current_user.id)
+    end
+
+    result
   end
 
   def create_push_event(service_class, project, current_user, push_data)

lib/gitlab/usage_data_counters/track_unique_actions.rb

@@ -49,11 +49,9 @@ module Gitlab
         keys = (date_from.to_date..date_to.to_date).map { |date| key(event_action, date) }
 
         Gitlab::Redis::SharedState.with do |redis|
-          Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
           redis.pfcount(*keys)
         end
       end
-      end
 
       private
 
@@ -75,7 +73,7 @@ module Gitlab
 
       def key(event_action, date)
         year_day = date.strftime('%G-%j')
-        "#{event_action}-#{year_day}"
+        "{#{event_action}}-#{year_day}"
       end
 
       def add_event(event_action, author_id, date)

spec/lib/gitlab/usage_data_counters/track_unique_actions_spec.rb

@@ -5,6 +5,8 @@ require 'spec_helper'
 RSpec.describe Gitlab::UsageDataCounters::TrackUniqueActions, :clean_gitlab_redis_shared_state do
   subject(:track_unique_events) { described_class.new }
 
+  let(:time) { Time.zone.now }
+
   def track_action(params)
     track_unique_events.track_action(params)
   end
@@ -13,91 +15,67 @@ RSpec.describe Gitlab::UsageDataCounters::TrackUniqueActions, :clean_gitlab_redi
     track_unique_events.count_unique_events(params)
   end
 
-  describe '#tracking_event' do
-    context 'when usage pings are enabled' do
-      before do
-        Gitlab::CurrentSettings.update!(usage_ping_enabled: true)
-      end
-
-      context 'when the feature flag is enabled ' do
-        let(:time) { Time.zone.now }
-
+  context 'tracking an event' do
+    context 'when tracking successfully' do
+      context 'when the feature flag and the application setting is enabled' do
+        context 'when the target and the action is valid' do
           before do
             stub_feature_flags(cache_diff_stats_merge_request: true)
+            stub_application_setting(usage_ping_enabled: true)
           end
 
-        context 'when target is valid' do
-          context 'when action is valid' do
           it 'tracks and counts the events as expected' do
             project = Event::TARGET_TYPES[:project]
             design = Event::TARGET_TYPES[:design]
             wiki = Event::TARGET_TYPES[:wiki]
 
-              track_action(event_action: :pushed, event_target: project, author_id: 1)
-              track_action(event_action: :pushed, event_target: project, author_id: 1)
-              track_action(event_action: :pushed, event_target: project, author_id: 2)
-              track_action(event_action: :pushed, event_target: project, author_id: 3)
-              track_action(event_action: :pushed, event_target: project, author_id: 4, time: time - 3.days)
-              track_action(event_action: :created, event_target: project, author_id: 5, time: time - 3.days)
+            expect(track_action(event_action: :pushed, event_target: project, author_id: 1)).to be_truthy
+            expect(track_action(event_action: :pushed, event_target: project, author_id: 1)).to be_truthy
+            expect(track_action(event_action: :pushed, event_target: project, author_id: 2)).to be_truthy
+            expect(track_action(event_action: :pushed, event_target: project, author_id: 3)).to be_truthy
+            expect(track_action(event_action: :pushed, event_target: project, author_id: 4, time: time - 3.days)).to be_truthy
+            expect(track_action(event_action: :created, event_target: project, author_id: 5, time: time - 3.days)).to be_truthy
 
-              track_action(event_action: :destroyed, event_target: design, author_id: 3)
-              track_action(event_action: :created, event_target: design, author_id: 4)
-              track_action(event_action: :updated, event_target: design, author_id: 5)
-              track_action(event_action: :pushed, event_target: design, author_id: 6)
+            expect(track_action(event_action: :destroyed, event_target: design, author_id: 3)).to be_truthy
+            expect(track_action(event_action: :created, event_target: design, author_id: 4)).to be_truthy
+            expect(track_action(event_action: :updated, event_target: design, author_id: 5)).to be_truthy
+            expect(track_action(event_action: :pushed, event_target: design, author_id: 6)).to be_truthy
 
-              track_action(event_action: :destroyed, event_target: wiki, author_id: 5)
-              track_action(event_action: :created, event_target: wiki, author_id: 3)
-              track_action(event_action: :updated, event_target: wiki, author_id: 4)
-              track_action(event_action: :pushed, event_target: wiki, author_id: 6)
+            expect(track_action(event_action: :destroyed, event_target: wiki, author_id: 5)).to be_truthy
+            expect(track_action(event_action: :created, event_target: wiki, author_id: 3)).to be_truthy
+            expect(track_action(event_action: :updated, event_target: wiki, author_id: 4)).to be_truthy
+            expect(track_action(event_action: :pushed, event_target: wiki, author_id: 6)).to be_truthy
 
-              expect(count_unique_events(event_action: described_class::PUSH_ACTION, date_from: time, date_to: Date.tomorrow)).to eq(3)
+            expect(count_unique_events(event_action: described_class::PUSH_ACTION, date_from: time, date_to: Date.today)).to eq(3)
             expect(count_unique_events(event_action: described_class::PUSH_ACTION, date_from: time - 5.days, date_to: Date.tomorrow)).to eq(4)
-              expect(count_unique_events(event_action: described_class::DESIGN_ACTION, date_from: time - 5.days, date_to: Date.tomorrow)).to eq(3)
-              expect(count_unique_events(event_action: described_class::WIKI_ACTION, date_from: time - 5.days, date_to: Date.tomorrow)).to eq(3)
+            expect(count_unique_events(event_action: described_class::DESIGN_ACTION, date_from: time - 5.days, date_to: Date.today)).to eq(3)
+            expect(count_unique_events(event_action: described_class::WIKI_ACTION, date_from: time - 5.days, date_to: Date.today)).to eq(3)
             expect(count_unique_events(event_action: described_class::PUSH_ACTION, date_from: time - 5.days, date_to: time - 2.days)).to eq(1)
           end
         end
-
-          context 'when action is invalid' do
-            it 'does not add any event' do
-              expect(Gitlab::Redis::SharedState).not_to receive(:with)
-
-              track_action(event_action: :test, event_target: :wiki, author_id: 2)
-            end
       end
     end
 
-        context 'when target is invalid' do
-          it 'does not add any event' do
-            expect(Gitlab::Redis::SharedState).not_to receive(:with)
+    context 'when tracking unsuccessfully' do
+      using RSpec::Parameterized::TableSyntax
 
-            track_action(event_action: :pushed, event_target: :test, author_id: 2)
-          end
-        end
+      where(:feature_flag, :application_setting, :target, :action) do
+        true  | true  | :project        | :invalid_action
+        false | true  | :project        | :pushed
+        true  | false | :project        | :pushed
+        true  | true  | :invalid_target | :pushed
       end
 
-      context 'when the feature flag is disabled' do
+      with_them do
         before do
-          stub_feature_flags(cache_diff_stats_merge_request: true)
-        end
-
-        it 'does not add any event' do
-          expect(Gitlab::Redis::SharedState).not_to receive(:with)
-
-          track_action(event_action: :pushed, event_target: :project, author_id: 2)
-        end
-      end
+          stub_application_setting(usage_ping_enabled: application_setting)
+          stub_feature_flags(cache_diff_stats_merge_request: feature_flag)
         end
 
-    context 'when usage pings are disabled' do
-      before do
-        Gitlab::CurrentSettings.update!(usage_ping_enabled: false)
+        it 'returns the expected values' do
+          expect(track_action(event_action: action, event_target: target, author_id: 2)).to be_nil
+          expect(count_unique_events(event_action: described_class::PUSH_ACTION, date_from: time, date_to: Date.today)).to eq(0)
         end
-
-      it 'does not add any event' do
-        expect(Gitlab::Redis::SharedState).not_to receive(:with)
-
-        track_action(event_action: :pushed, event_target: :project, author_id: 2)
       end
     end
   end