From 4cfdfd1f30771afff29f939f8bf6ad499977c545 Mon Sep 17 00:00:00 2001
From: Niko Belokolodov <nbelokolodov@gitlab.com>
Date: Wed, 12 Jul 2023 08:05:39 +0000
Subject: [PATCH] Remove usage of event list files
Add FF which enables retrieving event names from
metric definitions instead of event files
which will be removed in following issues
---
.../quick_actions/interpret_service.rb | 3 +-
...use_metric_definitions_for_events_list.yml | 8 +++
..._redis_hll_events_to_weekly_aggregation.rb | 6 ++
doc/development/cicd/templates.md | 21 +-----
.../hll_redis_counter_spec.rb | 64 +++++++++--------
.../usage_data_counters/hll_redis_counter.rb | 21 +++---
lib/tasks/gitlab/packages/events.rake | 69 -------------------
lib/tasks/gitlab/usage_data.rake | 15 ----
.../hll_redis_counter_spec.rb | 63 ++++++++++++++---
.../tasks/gitlab/packages/events_rake_spec.rb | 55 ---------------
spec/tasks/gitlab/usage_data_rake_spec.rb | 17 -----
11 files changed, 116 insertions(+), 226 deletions(-)
create mode 100644 config/feature_flags/development/use_metric_definitions_for_events_list.yml
delete mode 100644 lib/tasks/gitlab/packages/events.rake
delete mode 100644 spec/tasks/gitlab/packages/events_rake_spec.rb
diff --git a/app/services/quick_actions/interpret_service.rb b/app/services/quick_actions/interpret_service.rb
index b5f6bff756b36..d1798ce6fc022 100644
--- a/app/services/quick_actions/interpret_service.rb
+++ b/app/services/quick_actions/interpret_service.rb
@@ -188,7 +188,8 @@ def extract_updates(commands)
next unless definition
definition.execute(self, arg)
- usage_ping_tracking(definition.name, arg)
+ # summarize_diff will be removed https://gitlab.com/gitlab-org/gitlab/-/issues/407258#note_1385269274
+ usage_ping_tracking(definition.name, arg) unless definition.name == :summarize_diff
end
end
diff --git a/config/feature_flags/development/use_metric_definitions_for_events_list.yml b/config/feature_flags/development/use_metric_definitions_for_events_list.yml
new file mode 100644
index 0000000000000..fee196085020a
--- /dev/null
+++ b/config/feature_flags/development/use_metric_definitions_for_events_list.yml
@@ -0,0 +1,8 @@
+---
+name: use_metric_definitions_for_events_list
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/122840
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/416893
+milestone: '16.2'
+type: development
+group: group::analytics instrumentation
+default_enabled: false
diff --git a/db/post_migrate/20230317004428_migrate_daily_redis_hll_events_to_weekly_aggregation.rb b/db/post_migrate/20230317004428_migrate_daily_redis_hll_events_to_weekly_aggregation.rb
index 59bff26f964dc..168354cd94683 100644
--- a/db/post_migrate/20230317004428_migrate_daily_redis_hll_events_to_weekly_aggregation.rb
+++ b/db/post_migrate/20230317004428_migrate_daily_redis_hll_events_to_weekly_aggregation.rb
@@ -3,6 +3,12 @@
class MigrateDailyRedisHllEventsToWeeklyAggregation < Gitlab::Database::Migration[2.1]
disable_ddl_transaction!
+ # Due to the feature flag `use_metric_definitions_for_events_list`, this makes a `SELECT` call
+ # so this migration is now considered a DML (data manipulation) migration.
+ # For the time being, we need to specify only `main` database because it is now technically retriving DB data.
+ # Should be removed with <issue link>
+ restrict_gitlab_migration gitlab_schema: :gitlab_main
+
DAILY_EVENTS =
%w[g_edit_by_web_ide
g_edit_by_sfe
diff --git a/doc/development/cicd/templates.md b/doc/development/cicd/templates.md
index 77e529867af31..bf392a3f446a1 100644
--- a/doc/development/cicd/templates.md
+++ b/doc/development/cicd/templates.md
@@ -424,27 +424,11 @@ To add a metric definition for a new template:
1. Install and start the [GitLab GDK](https://gitlab.com/gitlab-org/gitlab-development-kit#installation).
1. In the `gitlab` directory in your GDK, check out the branch that contains the new template.
-1. [Add the template inclusion event](../internal_analytics/service_ping/implement.md#add-new-events)
- with this Rake task:
-
- ```shell
- bin/rake gitlab:usage_data:generate_ci_template_events
- ```
-
- The task adds a section like the following to [`lib/gitlab/usage_data_counters/known_events/ci_templates.yml`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/usage_data_counters/known_events/ci_templates.yml):
-
- ```yaml
- - name: p_ci_templates_my_template_name
- category: ci_templates
- aggregation: weekly
- ```
-
-1. Copy the value of `name` from the new YAML section, and add it to the weekly
- and monthly CI/CD template total count metrics:
+1. Add the new template event name to the weekly and monthly CI/CD template total count metrics:
- [`config/metrics/counts_7d/20210216184557_ci_templates_total_unique_counts_weekly.yml`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/config/metrics/counts_7d/20210216184557_ci_templates_total_unique_counts_weekly.yml)
- [`config/metrics/counts_28d/20210216184559_ci_templates_total_unique_counts_monthly.yml`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/config/metrics/counts_28d/20210216184559_ci_templates_total_unique_counts_monthly.yml)
-1. Use the same `name` as above as the last argument in the following command to
+1. Use the same event name as above as the last argument in the following command to
[add new metric definitions](../internal_analytics/service_ping/metrics_dictionary.md#metrics-added-dynamic-to-service-ping-payload):
```shell
@@ -481,7 +465,6 @@ To add a metric definition for a new template:
For example, these are the metrics configuration files for the
[5 Minute Production App template](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/5-Minute-Production-App.gitlab-ci.yml):
-- The template inclusion event: [`lib/gitlab/usage_data_counters/known_events/ci_templates.yml#L438-L441`](https://gitlab.com/gitlab-org/gitlab/-/blob/dcddbf83c29d1ad0839d55362c1b43888304f453/lib/gitlab/usage_data_counters/known_events/ci_templates.yml#L438-L441)
- The weekly and monthly metrics definitions:
- [`config/metrics/counts_7d/20210901223501_p_ci_templates_5_minute_production_app_weekly.yml`](https://gitlab.com/gitlab-org/gitlab/-/blob/1a6eceff3914f240864b2ca15ae2dc076ea67bf6/config/metrics/counts_7d/20210216184515_p_ci_templates_5_min_production_app_weekly.yml)
- [`config/metrics/counts_28d/20210901223505_p_ci_templates_5_minute_production_app_monthly.yml`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/config/metrics/counts_28d/20210216184517_p_ci_templates_5_min_production_app_monthly.yml)
diff --git a/ee/spec/lib/ee/gitlab/usage_data_counters/hll_redis_counter_spec.rb b/ee/spec/lib/ee/gitlab/usage_data_counters/hll_redis_counter_spec.rb
index 8dca2b1efcba3..b2af52ebd21eb 100644
--- a/ee/spec/lib/ee/gitlab/usage_data_counters/hll_redis_counter_spec.rb
+++ b/ee/spec/lib/ee/gitlab/usage_data_counters/hll_redis_counter_spec.rb
@@ -37,40 +37,44 @@
end
describe '.known_events' do
- let(:ce_temp_dir) { Dir.mktmpdir }
- let(:ee_temp_dir) { Dir.mktmpdir }
- let(:ce_temp_file) { Tempfile.new(%w[common .yml], ce_temp_dir) }
- let(:ee_temp_file) { Tempfile.new(%w[common .yml], ee_temp_dir) }
- let(:ce_event) do
- {
- "name" => "ce_event",
- "expiry" => 84
- }
- end
+ context 'when use_metric_definitions_for_events_list is disabled' do
+ let(:ce_temp_dir) { Dir.mktmpdir }
+ let(:ee_temp_dir) { Dir.mktmpdir }
+ let(:ce_temp_file) { Tempfile.new(%w[common .yml], ce_temp_dir) }
+ let(:ee_temp_file) { Tempfile.new(%w[common .yml], ee_temp_dir) }
+ let(:ce_event) do
+ {
+ "name" => "ce_event",
+ "expiry" => 84
+ }
+ end
- let(:ee_event) do
- {
- "name" => "ee_event",
- "expiry" => 84
- }
- end
+ let(:ee_event) do
+ {
+ "name" => "ee_event",
+ "expiry" => 84
+ }
+ end
- before do
- stub_const("#{described_class}::KNOWN_EVENTS_PATH", File.expand_path('*.yml', ce_temp_dir))
- stub_const("EE::#{described_class}::EE_KNOWN_EVENTS_PATH", File.expand_path('*.yml', ee_temp_dir))
- File.open(ce_temp_file.path, "w+b") { |f| f.write [ce_event].to_yaml }
- File.open(ee_temp_file.path, "w+b") { |f| f.write [ee_event].to_yaml }
- end
+ before do
+ stub_feature_flags(use_metric_definitions_for_events_list: false)
- it 'returns both ee and ce events' do
- expect(described_class.known_events).to match_array [ce_event, ee_event]
- end
+ stub_const("#{described_class}::KNOWN_EVENTS_PATH", File.expand_path('*.yml', ce_temp_dir))
+ stub_const("EE::#{described_class}::EE_KNOWN_EVENTS_PATH", File.expand_path('*.yml', ee_temp_dir))
+ File.open(ce_temp_file.path, "w+b") { |f| f.write [ce_event].to_yaml }
+ File.open(ee_temp_file.path, "w+b") { |f| f.write [ee_event].to_yaml }
+ end
- after do
- ce_temp_file.unlink
- ee_temp_file.unlink
- FileUtils.remove_entry(ce_temp_dir) if Dir.exist?(ce_temp_dir)
- FileUtils.remove_entry(ee_temp_dir) if Dir.exist?(ee_temp_dir)
+ it 'returns both ee and ce events' do
+ expect(described_class.known_events).to match_array [ce_event, ee_event]
+ end
+
+ after do
+ ce_temp_file.unlink
+ ee_temp_file.unlink
+ FileUtils.remove_entry(ce_temp_dir) if Dir.exist?(ce_temp_dir)
+ FileUtils.remove_entry(ee_temp_dir) if Dir.exist?(ee_temp_dir)
+ end
end
end
diff --git a/lib/gitlab/usage_data_counters/hll_redis_counter.rb b/lib/gitlab/usage_data_counters/hll_redis_counter.rb
index eaa4bf15fe1d0..c77a809ade415 100644
--- a/lib/gitlab/usage_data_counters/hll_redis_counter.rb
+++ b/lib/gitlab/usage_data_counters/hll_redis_counter.rb
@@ -15,14 +15,7 @@ module HLLRedisCounter
# Track event on entity_id
# Increment a Redis HLL counter for unique event_name and entity_id
#
- # All events should be added to known_events yml files lib/gitlab/usage_data_counters/known_events/
- #
- # Event example:
- #
- # - name: g_compliance_dashboard # Unique event name
- #
# Usage:
- #
# * Track event: Gitlab::UsageDataCounters::HLLRedisCounter.track_event('g_compliance_dashboard', values: user_id)
# * Get unique counts per user: Gitlab::UsageDataCounters::HLLRedisCounter.unique_events(event_names: 'g_compliance_dashboard', start_date: 28.days.ago, end_date: Date.current)
class << self
@@ -119,8 +112,18 @@ def keys_for_aggregation(events:, start_date:, end_date:, context: '')
end
def load_events(wildcard)
- Dir[wildcard].each_with_object([]) do |path, events|
- events.push(*load_yaml_from_path(path))
+ if Feature.enabled?(:use_metric_definitions_for_events_list)
+ events = Gitlab::Usage::MetricDefinition.not_removed.values.map do |d|
+ d.attributes[:options] && d.attributes[:options][:events]
+ end.flatten.compact
+
+ events.map do |e|
+ { name: e }.with_indifferent_access
+ end
+ else
+ Dir[wildcard].each_with_object([]) do |path, events|
+ events.push(*load_yaml_from_path(path))
+ end
end
end
diff --git a/lib/tasks/gitlab/packages/events.rake b/lib/tasks/gitlab/packages/events.rake
deleted file mode 100644
index b5dfd163dba96..0000000000000
--- a/lib/tasks/gitlab/packages/events.rake
+++ /dev/null
@@ -1,69 +0,0 @@
-# frozen_string_literal: true
-
-desc "GitLab | Packages | Events | Generate hll counter events file for packages"
-namespace :gitlab do
- namespace :packages do
- namespace :events do
- task generate: :environment do
- Rake::Task["gitlab:packages:events:generate_counts"].invoke
- Rake::Task["gitlab:packages:events:generate_unique"].invoke
- rescue StandardError => e
- logger.error("Error building events list: #{e}")
- end
-
- task generate_counts: :environment do
- logger = Logger.new($stdout)
- logger.info('Building list of package events...')
-
- 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 StandardError => e
- logger.error("Error building events list: #{e}")
- end
-
- task generate_unique: :environment do
- logger = Logger.new($stdout)
- logger.info('Building list of package events...')
-
- path = File.join(File.dirname(Gitlab::UsageDataCounters::HLLRedisCounter::KNOWN_EVENTS_PATH), 'package_events.yml')
- File.open(path, "w") { |file| file << generate_unique_events_list.to_yaml }
-
- logger.info("Events file `#{path}` generated successfully")
- rescue StandardError => e
- logger.error("Error building events list: #{e}")
- end
-
- private
-
- def event_pairs
- Packages::Event::EVENT_TYPES.product(Packages::Event::EVENT_SCOPES.keys)
- end
-
- def generate_unique_events_list
- events = event_pairs.each_with_object([]) do |(event_type, event_scope), events|
- Packages::Event::ORIGINATOR_TYPES.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 }
- end
-
- events.concat(events_definition)
- end
- end
-
- events.sort_by { |event| event["name"] }.uniq
- end
-
- def counter_events_list
- counters = event_pairs.flat_map do |event_type, event_scope|
- Packages::Event::ORIGINATOR_TYPES.flat_map do |originator_type|
- Packages::Event.counters_for(event_scope, event_type, originator_type)
- end
- end
-
- counters.compact.sort.uniq
- end
- end
- end
-end
diff --git a/lib/tasks/gitlab/usage_data.rake b/lib/tasks/gitlab/usage_data.rake
index f5bf1a266e563..1cd72ee6a1be1 100644
--- a/lib/tasks/gitlab/usage_data.rake
+++ b/lib/tasks/gitlab/usage_data.rake
@@ -34,21 +34,6 @@ namespace :gitlab do
puts Gitlab::Json.pretty_generate(Gitlab::UsageDataMetrics.uncached_data)
end
- desc 'GitLab | UsageDataMetrics | Generate known_events/ci_templates.yml based on template definitions'
- task generate_ci_template_events: :environment do
- banner = <<~BANNER
- # This file is generated automatically by
- # bin/rake gitlab:usage_data:generate_ci_template_events
- #
- # Do not edit it manually!
- BANNER
-
- all_includes = explicit_template_includes + implicit_auto_devops_includes
- yaml = banner + YAML.dump(all_includes).gsub(/ *$/m, '')
-
- File.write(Gitlab::UsageDataCounters::CiTemplateUniqueCounter::KNOWN_EVENTS_FILE_PATH, yaml)
- end
-
desc 'GitLab | UsageDataMetrics | Generate raw SQL metrics queries for RSpec'
task generate_sql_metrics_queries: :environment do
require 'active_support/testing/time_helpers'
diff --git a/spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb b/spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb
index 50fb9f9df6e82..62798c1871734 100644
--- a/spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb
+++ b/spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb
@@ -24,22 +24,63 @@
end
describe '.known_events' do
- let(:ce_temp_dir) { Dir.mktmpdir }
- let(:ce_temp_file) { Tempfile.new(%w[common .yml], ce_temp_dir) }
let(:ce_event) { { "name" => "ce_event" } }
- before do
- stub_const("#{described_class}::KNOWN_EVENTS_PATH", File.expand_path('*.yml', ce_temp_dir))
- File.open(ce_temp_file.path, "w+b") { |f| f.write [ce_event].to_yaml }
- end
+ context 'with use_metric_definitions_for_events_list disabled' do
+ let(:ce_temp_dir) { Dir.mktmpdir }
+ let(:ce_temp_file) { Tempfile.new(%w[common .yml], ce_temp_dir) }
+
+ before do
+ stub_feature_flags(use_metric_definitions_for_events_list: false)
+ stub_const("#{described_class}::KNOWN_EVENTS_PATH", File.expand_path('*.yml', ce_temp_dir))
+ File.open(ce_temp_file.path, "w+b") { |f| f.write [ce_event].to_yaml }
+ end
- after do
- ce_temp_file.unlink
- FileUtils.remove_entry(ce_temp_dir) if Dir.exist?(ce_temp_dir)
+ after do
+ ce_temp_file.unlink
+ FileUtils.remove_entry(ce_temp_dir) if Dir.exist?(ce_temp_dir)
+ end
+
+ it 'returns ce events' do
+ expect(described_class.known_events).to include(ce_event)
+ end
end
- it 'returns ce events' do
- expect(described_class.known_events).to include(ce_event)
+ context 'with use_metric_definitions_for_events_list enabled' do
+ let(:removed_ce_event) { { "name" => "removed_ce_event" } }
+ let(:metric_definition) do
+ Gitlab::Usage::MetricDefinition.new('ce_metric',
+ {
+ key_path: 'ce_metric_weekly',
+ status: 'active',
+ options: {
+ events: [ce_event['name']]
+ }
+ })
+ end
+
+ let(:removed_metric_definition) do
+ Gitlab::Usage::MetricDefinition.new('removed_ce_metric',
+ {
+ key_path: 'removed_ce_metric_weekly',
+ status: 'removed',
+ options: {
+ events: [removed_ce_event['name']]
+ }
+ })
+ end
+
+ before do
+ allow(Gitlab::Usage::MetricDefinition).to receive(:all).and_return([metric_definition, removed_metric_definition])
+ end
+
+ it 'returns ce events' do
+ expect(described_class.known_events).to include(ce_event)
+ end
+
+ it 'does not return removed events' do
+ expect(described_class.known_events).not_to include(removed_ce_event)
+ end
end
end
diff --git a/spec/tasks/gitlab/packages/events_rake_spec.rb b/spec/tasks/gitlab/packages/events_rake_spec.rb
deleted file mode 100644
index 87f4db360ca4c..0000000000000
--- a/spec/tasks/gitlab/packages/events_rake_spec.rb
+++ /dev/null
@@ -1,55 +0,0 @@
-# frozen_string_literal: true
-
-require 'rake_helper'
-
-RSpec.describe 'gitlab:packages:events namespace rake task', :silence_stdout do
- before :all do
- Rake.application.rake_require 'tasks/gitlab/packages/events'
- end
-
- subject do
- file = double('file')
- yml_file = nil
-
- allow(file).to receive(:<<) { |contents| yml_file = contents }
- allow(File).to receive(:open).and_yield(file)
-
- run_rake_task("gitlab:packages:events:#{task}")
-
- YAML.safe_load(yml_file)
- end
-
- describe 'generate_unique' do
- let(:task) { 'generate_unique' }
-
- it 'excludes guest events' do
- expect(subject.find { |event| event['name'].include?("guest") }).to be_nil
- end
-
- Packages::Event::EVENT_SCOPES.keys.each do |event_scope|
- it "includes `#{event_scope}` scope" do
- expect(subject.find { |event| event['name'].include?(event_scope) }).not_to be_nil
- end
- end
-
- it 'excludes some event types' do
- expect(subject.grep(/search_package/)).to be_empty
- expect(subject.grep(/list_package/)).to be_empty
- end
- end
-
- describe 'generate_counts' do
- let(:task) { 'generate_counts' }
-
- Packages::Event::EVENT_SCOPES.keys.each do |event_scope|
- it "includes `#{event_scope}` scope" do
- expect(subject.find { |event| event.include?(event_scope) }).not_to be_nil
- end
- end
-
- it 'excludes some event types' do
- expect(subject.find { |event| event.include?("search_package") }).to be_nil
- expect(subject.find { |event| event.include?("list_package") }).to be_nil
- end
- end
-end
diff --git a/spec/tasks/gitlab/usage_data_rake_spec.rb b/spec/tasks/gitlab/usage_data_rake_spec.rb
index 11aab1b1b423b..170b131915474 100644
--- a/spec/tasks/gitlab/usage_data_rake_spec.rb
+++ b/spec/tasks/gitlab/usage_data_rake_spec.rb
@@ -69,23 +69,6 @@
expect { run_rake_task('gitlab:usage_data:generate_and_send') }.to output(/.*201.*/).to_stdout
end
- describe 'generate_ci_template_events' do
- around do |example|
- FileUtils.rm_rf(Gitlab::UsageDataCounters::CiTemplateUniqueCounter::KNOWN_EVENTS_FILE_PATH)
-
- example.run
-
- `git checkout -- #{Gitlab::UsageDataCounters::CiTemplateUniqueCounter::KNOWN_EVENTS_FILE_PATH}`
- end
-
- it "generates #{Gitlab::UsageDataCounters::CiTemplateUniqueCounter::KNOWN_EVENTS_FILE_PATH}",
- quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/386191' do
- run_rake_task('gitlab:usage_data:generate_ci_template_events')
-
- expect(File.exist?(Gitlab::UsageDataCounters::CiTemplateUniqueCounter::KNOWN_EVENTS_FILE_PATH)).to be true
- end
- end
-
private
def stub_response(body:, url: service_ping_payload_url, status: 201)
--
GitLab