Merge branch '293927-package-event-counts-are-not-aggregated-correctly' into 'master'

Fix package event metrics aggregation

See merge request gitlab-org/gitlab!50108

app/models/packages/event.rb

@@ -6,6 +6,8 @@ class Packages::Event < ApplicationRecord
   UNIQUE_EVENTS_ALLOWED = %i[push_package delete_package pull_package].freeze
   EVENT_SCOPES = ::Packages::Package.package_types.merge(container: 1000, tag: 1001).freeze
 
+  EVENT_PREFIX = "i_package"
+
   enum event_scope: EVENT_SCOPES
 
   enum event_type: {
@@ -24,13 +26,6 @@ class Packages::Event < ApplicationRecord
 
   enum originator_type: { user: 0, deploy_token: 1, guest: 2 }
 
-  def self.allowed_event_name(event_scope, event_type, originator)
-    return unless event_allowed?(event_type)
-
-    # remove `package` from the event name to avoid issues with HLLRedisCounter class parsing
-    "i_package_#{event_scope}_#{originator}_#{event_type.gsub(/_packages?/, "")}"
-  end
-
   # Remove some of the events, for now, so we don't hammer Redis too hard.
   # See: https://gitlab.com/gitlab-org/gitlab/-/issues/280770
   def self.event_allowed?(event_type)
@@ -38,4 +33,23 @@ class Packages::Event < ApplicationRecord
 
     false
   end
+
+  # counter names for unique user tracking (for MAU)
+  def self.unique_counters_for(event_scope, event_type, originator_type)
+    return [] unless event_allowed?(event_type)
+    return [] if originator_type.to_s == 'guest'
+
+    ["#{EVENT_PREFIX}_#{event_scope}_#{originator_type}"]
+  end
+
+  # total counter names for tracking number of events
+  def self.counters_for(event_scope, event_type, originator_type)
+    return [] unless event_allowed?(event_type)
+
+    [
+      "#{EVENT_PREFIX}_#{event_type}",
+      "#{EVENT_PREFIX}_#{event_type}_by_#{originator_type}",
+      "#{EVENT_PREFIX}_#{event_scope}_#{event_type}"
+    ]
+  end
 end

app/services/packages/create_event_service.rb

@@ -3,11 +3,13 @@
 module Packages
   class CreateEventService < BaseService
     def execute
-      if Feature.enabled?(:collect_package_events_redis) && redis_event_name
-        if guest?
-          ::Gitlab::UsageDataCounters::GuestPackageEventCounter.count(redis_event_name)
-        else
-          ::Gitlab::UsageDataCounters::HLLRedisCounter.track_event(current_user.id, redis_event_name)
+      if Feature.enabled?(:collect_package_events_redis)
+        ::Packages::Event.unique_counters_for(event_scope, event_name, originator_type).each do |event_name|
+          ::Gitlab::UsageDataCounters::HLLRedisCounter.track_event(current_user.id, event_name)
+        end
+
+        ::Packages::Event.counters_for(event_scope, event_name, originator_type).each do |event_name|
+          ::Gitlab::UsageDataCounters::PackageEventCounter.count(event_name)
         end
       end
 
@@ -23,10 +25,6 @@ module Packages
 
     private
 
-    def redis_event_name
-      @redis_event_name ||= ::Packages::Event.allowed_event_name(event_scope, event_name, originator_type)
-    end
-
     def event_scope
       @event_scope ||= scope.is_a?(::Packages::Package) ? scope.package_type : scope
     end

changelogs/unreleased/293927-package-event-counts-are-not-aggregated-correctly.yml

+---
+title: Fix package event metrics aggregation
+merge_request: 50108
+author:
+type: changed

lib/gitlab/usage_data_counters.rb

@@ -3,7 +3,7 @@
 module Gitlab
   module UsageDataCounters
     COUNTERS = [
-      GuestPackageEventCounter,
+      PackageEventCounter,
       WikiPageCounter,
       WebIdeCounter,
       NoteCounter,

lib/gitlab/usage_data_counters/counter_events/guest_package_events.yml

----
-- i_package_composer_guest_delete
-- i_package_composer_guest_pull
-- i_package_composer_guest_push
-- i_package_conan_guest_delete
-- i_package_conan_guest_pull
-- i_package_conan_guest_push
-- i_package_container_guest_delete
-- i_package_container_guest_pull
-- i_package_container_guest_push
-- i_package_debian_guest_delete
-- i_package_debian_guest_pull
-- i_package_debian_guest_push
-- i_package_generic_guest_delete
-- i_package_generic_guest_pull
-- i_package_generic_guest_push
-- i_package_golang_guest_delete
-- i_package_golang_guest_pull
-- i_package_golang_guest_push
-- i_package_maven_guest_delete
-- i_package_maven_guest_pull
-- i_package_maven_guest_push
-- i_package_npm_guest_delete
-- i_package_npm_guest_pull
-- i_package_npm_guest_push
-- i_package_nuget_guest_delete
-- i_package_nuget_guest_pull
-- i_package_nuget_guest_push
-- i_package_pypi_guest_delete
-- i_package_pypi_guest_pull
-- i_package_pypi_guest_push
-- i_package_tag_guest_delete
-- i_package_tag_guest_pull
-- i_package_tag_guest_push

lib/gitlab/usage_data_counters/counter_events/package_events.yml

+---
+- i_package_composer_delete_package
+- i_package_composer_pull_package
+- i_package_composer_push_package
+- i_package_conan_delete_package
+- i_package_conan_pull_package
+- i_package_conan_push_package
+- i_package_container_delete_package
+- i_package_container_pull_package
+- i_package_container_push_package
+- i_package_debian_delete_package
+- i_package_debian_pull_package
+- i_package_debian_push_package
+- i_package_delete_package
+- i_package_delete_package_by_deploy_token
+- i_package_delete_package_by_guest
+- i_package_delete_package_by_user
+- i_package_generic_delete_package
+- i_package_generic_pull_package
+- i_package_generic_push_package
+- i_package_golang_delete_package
+- i_package_golang_pull_package
+- i_package_golang_push_package
+- i_package_maven_delete_package
+- i_package_maven_pull_package
+- i_package_maven_push_package
+- i_package_npm_delete_package
+- i_package_npm_pull_package
+- i_package_npm_push_package
+- i_package_nuget_delete_package
+- i_package_nuget_pull_package
+- i_package_nuget_push_package
+- i_package_pull_package
+- i_package_pull_package_by_deploy_token
+- i_package_pull_package_by_guest
+- i_package_pull_package_by_user
+- i_package_push_package
+- i_package_push_package_by_deploy_token
+- i_package_push_package_by_guest
+- i_package_push_package_by_user
+- i_package_pypi_delete_package
+- i_package_pypi_pull_package
+- i_package_pypi_push_package
+- i_package_tag_delete_package
+- i_package_tag_pull_package
+- i_package_tag_push_package

lib/gitlab/usage_data_counters/guest_package_event_counter.rb → lib/gitlab/usage_data_counters/package_event_counter.rb

@@ -2,10 +2,10 @@
 
 module Gitlab
   module UsageDataCounters
-    class GuestPackageEventCounter < BaseCounter
-      KNOWN_EVENTS_PATH = File.expand_path('counter_events/guest_package_events.yml', __dir__)
+    class PackageEventCounter < BaseCounter
+      KNOWN_EVENTS_PATH = File.expand_path('counter_events/package_events.yml', __dir__)
       KNOWN_EVENTS = YAML.safe_load(File.read(KNOWN_EVENTS_PATH)).freeze
-      PREFIX = 'package_guest'
+      PREFIX = 'package_events'
     end
   end
 end

lib/tasks/gitlab/packages/events.rake

@@ -5,18 +5,18 @@ namespace :gitlab do
   namespace :packages do
     namespace :events do
       task generate: :environment do
-        Rake::Task["gitlab:packages:events:generate_guest"].invoke
+        Rake::Task["gitlab:packages:events:generate_counts"].invoke
         Rake::Task["gitlab:packages:events:generate_unique"].invoke
       rescue => e
         logger.error("Error building events list: #{e}")
       end
 
-      task generate_guest: :environment do
+      task generate_counts: :environment do
         logger = Logger.new(STDOUT)
         logger.info('Building list of package events...')
 
-        path = Gitlab::UsageDataCounters::GuestPackageEventCounter::KNOWN_EVENTS_PATH
-        File.open(path, "w") { |file| file << guest_events_list.to_yaml }
+        path = Gitlab::UsageDataCounters::PackageEventCounter::KNOWN_EVENTS_PATH
+        File.open(path, "w") { |file| file << counter_events_list.to_yaml }
 
         logger.info("Events file `#{path}` generated successfully")
       rescue => e
@@ -43,26 +43,32 @@ namespace :gitlab do
 
       def generate_unique_events_list
         events = event_pairs.each_with_object([]) do |(event_type, event_scope), events|
-          Packages::Event.originator_types.keys.excluding('guest').each do |originator|
-            if name = Packages::Event.allowed_event_name(event_scope, event_type, originator)
-              events << {
-                "name" => name,
-                "category" => "#{event_scope}_packages",
+          Packages::Event.originator_types.keys.excluding('guest').each do |originator_type|
+            events_definition = Packages::Event.unique_counters_for(event_scope, event_type, originator_type).map do |event_name|
+              {
+                "name" => event_name,
+                "category" => "#{originator_type}_packages",
                 "aggregation" => "weekly",
                 "redis_slot" => "package",
                 "feature_flag" => "collect_package_events_redis"
               }
             end
+
+            events.concat(events_definition)
           end
         end
 
-        events.sort_by { |event| event["name"] }
+        events.sort_by { |event| event["name"] }.uniq
       end
 
-      def guest_events_list
-        event_pairs.map do |event_type, event_scope|
-          Packages::Event.allowed_event_name(event_scope, event_type, "guest")
-        end.compact.sort
+      def counter_events_list
+        counters = event_pairs.flat_map do |event_type, event_scope|
+          Packages::Event.originator_types.keys.flat_map do |originator_type|
+            Packages::Event.counters_for(event_scope, event_type, originator_type)
+          end
+        end
+
+        counters.compact.sort.uniq
       end
     end
   end

spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb

@@ -24,6 +24,8 @@ RSpec.describe Gitlab::UsageDataCounters::HLLRedisCounter, :clean_gitlab_redis_s
   describe '.categories' do
     it 'gets all unique category names' do
       expect(described_class.categories).to contain_exactly(
+        'deploy_token_packages',
+        'user_packages',
         'compliance',
         'analytics',
         'ide_edit',
@@ -34,17 +36,6 @@ RSpec.describe Gitlab::UsageDataCounters::HLLRedisCounter, :clean_gitlab_redis_s
         'testing',
         'issues_edit',
         'ci_secrets_management',
-        'maven_packages',
-        'npm_packages',
-        'conan_packages',
-        'nuget_packages',
-        'pypi_packages',
-        'composer_packages',
-        'generic_packages',
-        'golang_packages',
-        'debian_packages',
-        'container_packages',
-        'tag_packages',
         'snippets',
         'code_review',
         'terraform'

spec/lib/gitlab/usage_data_counters/guest_package_event_counter_spec.rb → spec/lib/gitlab/usage_data_counters/package_event_counter_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::UsageDataCounters::GuestPackageEventCounter, :clean_gitlab_redis_shared_state do
+RSpec.describe Gitlab::UsageDataCounters::PackageEventCounter, :clean_gitlab_redis_shared_state do
   shared_examples_for 'usage counter with totals' do |counter|
     it 'increments counter and returns total count' do
       expect(described_class.read(counter)).to eq(0)
@@ -14,7 +14,7 @@ RSpec.describe Gitlab::UsageDataCounters::GuestPackageEventCounter, :clean_gitla
   end
 
   it 'includes the right events' do
-    expect(described_class::KNOWN_EVENTS.size).to eq 33
+    expect(described_class::KNOWN_EVENTS.size).to eq 45
   end
 
   described_class::KNOWN_EVENTS.each do |event|
@@ -24,8 +24,8 @@ RSpec.describe Gitlab::UsageDataCounters::GuestPackageEventCounter, :clean_gitla
   describe '.fetch_supported_event' do
     subject { described_class.fetch_supported_event(event_name) }
 
-    let(:event_name) { 'package_guest_i_package_composer_guest_push' }
+    let(:event_name) { 'package_events_i_package_composer_push_package' }
 
-    it { is_expected.to eq 'i_package_composer_guest_push' }
+    it { is_expected.to eq 'i_package_composer_push_package' }
   end
 end

spec/lib/gitlab/usage_data_spec.rb

@@ -680,7 +680,9 @@ RSpec.describe Gitlab::UsageData, :aggregate_failures do
 
     it { is_expected.to include(:kubernetes_agent_gitops_sync) }
     it { is_expected.to include(:static_site_editor_views) }
-    it { is_expected.to include(:package_guest_i_package_composer_guest_pull) }
+    it { is_expected.to include(:package_events_i_package_pull_package) }
+    it { is_expected.to include(:package_events_i_package_delete_package_by_user) }
+    it { is_expected.to include(:package_events_i_package_conan_push_package) }
   end
 
   describe '.usage_data_counters' do

spec/services/packages/create_event_service_spec.rb

@@ -56,7 +56,7 @@ RSpec.describe Packages::CreateEventService do
       end
     end
 
-    shared_examples 'redis package event creation' do |originator_type, expected_scope|
+    shared_examples 'redis package unique event creation' do |originator_type, expected_scope|
       context 'with feature flag disable' do
         before do
           stub_feature_flags(collect_package_events_redis: false)
@@ -70,29 +70,27 @@ RSpec.describe Packages::CreateEventService do
       end
 
       it 'tracks the event' do
-        expect(::Gitlab::UsageDataCounters::GuestPackageEventCounter).not_to receive(:count)
-        expect(::Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event).with(user.id, Packages::Event.allowed_event_name(expected_scope, event_name, originator_type))
+        expect(::Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event).with(user.id, /package/)
 
         subject
       end
     end
 
-    shared_examples 'redis package guest event creation' do |originator_type, expected_scope|
+    shared_examples 'redis package count event creation' do |originator_type, expected_scope|
       context 'with feature flag disabled' do
         before do
           stub_feature_flags(collect_package_events_redis: false)
         end
 
         it 'does not track the event' do
-          expect(::Gitlab::UsageDataCounters::GuestPackageEventCounter).not_to receive(:count)
+          expect(::Gitlab::UsageDataCounters::PackageEventCounter).not_to receive(:count)
 
           subject
         end
       end
 
       it 'tracks the event' do
-        expect(::Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event)
-        expect(::Gitlab::UsageDataCounters::GuestPackageEventCounter).to receive(:count).with(Packages::Event.allowed_event_name(expected_scope, event_name, originator_type))
+        expect(::Gitlab::UsageDataCounters::PackageEventCounter).to receive(:count).at_least(:once)
 
         subject
       end
@@ -102,21 +100,23 @@ RSpec.describe Packages::CreateEventService do
       let(:user) { create(:user) }
 
       it_behaves_like 'db package event creation', 'user', 'container'
-      it_behaves_like 'redis package event creation', 'user', 'container'
+      it_behaves_like 'redis package unique event creation', 'user', 'container'
+      it_behaves_like 'redis package count event creation', 'user', 'container'
     end
 
     context 'with a deploy token' do
       let(:user) { create(:deploy_token) }
 
       it_behaves_like 'db package event creation', 'deploy_token', 'container'
-      it_behaves_like 'redis package event creation', 'deploy_token', 'container'
+      it_behaves_like 'redis package unique event creation', 'deploy_token', 'container'
+      it_behaves_like 'redis package count event creation', 'deploy_token', 'container'
     end
 
     context 'with no user' do
       let(:user) { nil }
 
       it_behaves_like 'db package event creation', 'guest', 'container'
-      it_behaves_like 'redis package guest event creation', 'guest', 'container'
+      it_behaves_like 'redis package count event creation', 'guest', 'container'
     end
 
     context 'with a package as scope' do
@@ -126,14 +126,15 @@ RSpec.describe Packages::CreateEventService do
         let(:user) { nil }
 
         it_behaves_like 'db package event creation', 'guest', 'npm'
-        it_behaves_like 'redis package guest event creation', 'guest', 'npm'
+        it_behaves_like 'redis package count event creation', 'guest', 'npm'
       end
 
       context 'with user' do
         let(:user) { create(:user) }
 
         it_behaves_like 'db package event creation', 'user', 'npm'
-        it_behaves_like 'redis package event creation', 'user', 'npm'
+        it_behaves_like 'redis package unique event creation', 'user', 'npm'
+        it_behaves_like 'redis package count event creation', 'user', 'npm'
       end
     end
   end

spec/tasks/gitlab/packages/events_rake_spec.rb

@@ -38,8 +38,8 @@ RSpec.describe 'gitlab:packages:events namespace rake task' do
     end
   end
 
-  describe 'generate_guest' do
-    let(:task) { 'generate_guest' }
+  describe 'generate_counts' do
+    let(:task) { 'generate_counts' }
 
     Packages::Event::EVENT_SCOPES.keys.each do |event_scope|
       it "includes `#{event_scope}` scope" do