From d560d54c37664778a604afc9b5e0b83febc1dbed Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Micha=C5=82=20Wielich?= <mwielich@gitlab.com>
Date: Tue, 13 Feb 2024 15:50:23 +0000
Subject: [PATCH] Add redisHLL unique values reading for RedisHLL metric
Add redisHLL unique values reading for RedisHLL metric
---
...1126090001_p_analytics_ci_cd_pipelines.yml | 6 ++-
...1126090003_p_analytics_ci_cd_lead_time.yml | 6 ++-
.../instrumentations/redis_hll_metric.rb | 13 +++++-
.../usage_data_counters/hll_redis_counter.rb | 3 +-
.../hll_redis_legacy_events.yml | 20 ++++------
.../instrumentations/redis_hll_metric_spec.rb | 40 +++++++++++++++++++
.../hll_redis_counter_spec.rb | 36 ++++++++---------
.../issue_activity_unique_counter_spec.rb | 8 +++-
.../delete_designs_service_spec.rb | 2 +-
spec/services/notes/create_service_spec.rb | 2 +-
spec/services/notes/destroy_service_spec.rb | 2 +-
spec/services/notes/update_service_spec.rb | 2 +-
...metrics_instrumentation_shared_examples.rb | 4 +-
13 files changed, 96 insertions(+), 48 deletions(-)
diff --git a/config/metrics/counts_7d/20211126090001_p_analytics_ci_cd_pipelines.yml b/config/metrics/counts_7d/20211126090001_p_analytics_ci_cd_pipelines.yml
index 3e8c119b695ab..48dc53bfefc7f 100644
--- a/config/metrics/counts_7d/20211126090001_p_analytics_ci_cd_pipelines.yml
+++ b/config/metrics/counts_7d/20211126090001_p_analytics_ci_cd_pipelines.yml
@@ -10,8 +10,10 @@ status: active
milestone: '14.6'
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/75187
time_frame: 7d
-data_source: redis_hll
-instrumentation_class: RedisHLLMetric
+data_source: internal_events
+events:
+ - name: p_analytics_ci_cd_pipelines
+ unique: user.id
options:
events:
- p_analytics_ci_cd_pipelines
diff --git a/config/metrics/counts_7d/20211126090003_p_analytics_ci_cd_lead_time.yml b/config/metrics/counts_7d/20211126090003_p_analytics_ci_cd_lead_time.yml
index 9d536617030ec..630c8dbe0a773 100644
--- a/config/metrics/counts_7d/20211126090003_p_analytics_ci_cd_lead_time.yml
+++ b/config/metrics/counts_7d/20211126090003_p_analytics_ci_cd_lead_time.yml
@@ -10,8 +10,10 @@ status: active
milestone: '14.6'
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/75187
time_frame: 7d
-data_source: redis_hll
-instrumentation_class: RedisHLLMetric
+data_source: internal_events
+events:
+ - name: p_analytics_ci_cd_lead_time
+ unique: user.id
options:
events:
- p_analytics_ci_cd_lead_time
diff --git a/lib/gitlab/usage/metrics/instrumentations/redis_hll_metric.rb b/lib/gitlab/usage/metrics/instrumentations/redis_hll_metric.rb
index d3bbb3ee02cad..743c80d0588ab 100644
--- a/lib/gitlab/usage/metrics/instrumentations/redis_hll_metric.rb
+++ b/lib/gitlab/usage/metrics/instrumentations/redis_hll_metric.rb
@@ -24,7 +24,7 @@ def metric_events
def value
redis_usage_data do
- event_params = time_constraints.merge(event_names: metric_events)
+ event_params = time_constraints.merge(event_names: metric_events, property_name: property_name)
Gitlab::UsageDataCounters::HLLRedisCounter.unique_events(**event_params)
end
@@ -32,6 +32,17 @@ def value
private
+ def property_name
+ return if events.empty?
+
+ uniques = events.pluck(:unique).uniq
+
+ return uniques.first if uniques.count == 1
+
+ message = "RedisHLLMetric for events #{events}, options #{options} has multiple unique_by values"
+ raise Gitlab::Usage::MetricDefinition::InvalidError, message
+ end
+
def time_constraints
case time_frame
when '28d'
diff --git a/lib/gitlab/usage_data_counters/hll_redis_counter.rb b/lib/gitlab/usage_data_counters/hll_redis_counter.rb
index 52b2fc8148c1a..21df6d9fc5c94 100644
--- a/lib/gitlab/usage_data_counters/hll_redis_counter.rb
+++ b/lib/gitlab/usage_data_counters/hll_redis_counter.rb
@@ -56,8 +56,7 @@ def track_event(event_name, values:, property_name: nil, time: Time.current)
# start_date - The start date of the time range.
# end_date - The end date of the time range.
def unique_events(event_names:, start_date:, end_date:, property_name: nil)
- # :skip_file_list_validation can be removed when we add property_name passing to RedisHLLMetric
- count_unique_events(event_names: event_names, property_name: property_name, start_date: start_date, end_date: end_date, skip_file_list_validation: true)
+ count_unique_events(event_names: event_names, property_name: property_name, start_date: start_date, end_date: end_date)
end
def known_event?(event_name)
diff --git a/lib/gitlab/usage_data_counters/hll_redis_legacy_events.yml b/lib/gitlab/usage_data_counters/hll_redis_legacy_events.yml
index 780d9d3092a17..c177ae39f9a45 100644
--- a/lib/gitlab/usage_data_counters/hll_redis_legacy_events.yml
+++ b/lib/gitlab/usage_data_counters/hll_redis_legacy_events.yml
@@ -9,15 +9,10 @@
- ci_users_executing_deployment_job
- ci_users_executing_verify_environment_job
- cicd_component_usage
-- code_suggestions_authenticate
-- code_suggestions_rate_limit_exceeded
- commit_note_created
-- dependency_proxy_packages_maven_file_pulled_from_cache
-- dependency_proxy_packages_maven_file_pulled_from_external
- design_action
- error_tracking_view_details
- error_tracking_view_list
-- export_runner_usage_by_project_as_csv
- flux_git_push_notified_unique_projects
- g_analytics_ci_cd_change_failure_rate
- g_analytics_ci_cd_deployment_frequency
@@ -44,10 +39,10 @@
- github_import_project_success
- i_analytics_cohorts
- i_analytics_instance_statistics
+- i_ci_secrets_management_azure_key_vault_build_created
+- i_ci_secrets_management_gcp_secret_manager_build_created
- i_ci_secrets_management_id_tokens_build_created
- i_ci_secrets_management_vault_build_created
-- i_ci_secrets_management_gcp_secret_manager_build_created
-- i_ci_secrets_management_azure_key_vault_build_created
- i_code_review_click_diff_view_setting
- i_code_review_click_file_browser_setting
- i_code_review_click_single_file_mode_setting
@@ -174,10 +169,15 @@
- i_code_review_widget_nothing_merge_click_new_file
- i_compliance_audit_events
- i_compliance_credential_inventory
+- i_container_registry_create_repository_deploy_token
- i_container_registry_create_repository_user
+- i_container_registry_delete_repository_deploy_token
- i_container_registry_delete_repository_user
+- i_container_registry_delete_tag_deploy_token
- i_container_registry_delete_tag_user
+- i_container_registry_push_repository_deploy_token
- i_container_registry_push_repository_user
+- i_container_registry_push_tag_deploy_token
- i_container_registry_push_tag_user
- i_ecosystem_jira_service_close_issue
- i_ecosystem_jira_service_create_issue
@@ -352,12 +352,6 @@
- k8s_api_proxy_requests_unique_agents_via_pat_access
- k8s_api_proxy_requests_unique_agents_via_user_access
- merge_request_action
-- merge_request_click_add_to_review_on_changes_tab
-- merge_request_click_add_to_review_on_overview_tab
-- merge_request_click_start_review_on_changes_tab
-- merge_request_click_start_review_on_overview_tab
-- model_registry_ml_model_created
-- model_registry_ml_model_version_created
- o_pipeline_authoring_unique_users_committing_ciconfigfile
- o_pipeline_authoring_unique_users_pushing_mr_ciconfigfile
- p_analytics_ci_cd_change_failure_rate
diff --git a/spec/lib/gitlab/usage/metrics/instrumentations/redis_hll_metric_spec.rb b/spec/lib/gitlab/usage/metrics/instrumentations/redis_hll_metric_spec.rb
index 33868d365a515..7f0b2ea7c99c3 100644
--- a/spec/lib/gitlab/usage/metrics/instrumentations/redis_hll_metric_spec.rb
+++ b/spec/lib/gitlab/usage/metrics/instrumentations/redis_hll_metric_spec.rb
@@ -27,6 +27,46 @@
expect { described_class.new(time_frame: '28d') }.to raise_error(ArgumentError)
end
+ context "with events attribute defined" do
+ let(:expected_value) { 2 }
+ let(:flag_enabled) { true }
+
+ before do
+ stub_feature_flags(redis_hll_property_name_tracking: flag_enabled)
+
+ Gitlab::UsageDataCounters::HLLRedisCounter.track_event(:g_project_management_issue_iteration_changed, values: 1, time: 1.week.ago, property_name: 'user')
+ Gitlab::UsageDataCounters::HLLRedisCounter.track_event(:g_project_management_issue_iteration_changed, values: 2, time: 2.weeks.ago, property_name: 'user')
+ Gitlab::UsageDataCounters::HLLRedisCounter.track_event(:g_project_management_issue_iteration_changed, values: 1, time: 2.weeks.ago, property_name: 'user')
+ Gitlab::UsageDataCounters::HLLRedisCounter.track_event(:g_project_management_issue_iteration_changed, values: 3, time: 2.weeks.ago, property_name: 'project')
+ end
+
+ it_behaves_like 'a correct instrumented metric value', { time_frame: '28d', options: { events: ['g_project_management_issue_iteration_changed'] }, events: [name: 'g_project_management_issue_iteration_changed', unique: 'user.id'] }
+
+ context "with feature flag disabled" do
+ let(:expected_value) { 3 }
+ let(:flag_enabled) { false }
+
+ it_behaves_like 'a correct instrumented metric value', { time_frame: '28d', options: { events: ['g_project_management_issue_iteration_changed'] }, events: [name: 'g_project_management_issue_iteration_changed', unique: 'user.id'] }
+ end
+
+ context "with events having different `unique` values" do
+ let(:expected_value) { 3 }
+ let(:flag_enabled) { false }
+ let(:events) do
+ [
+ { name: 'g_project_management_issue_iteration_changed', unique: 'user.id' },
+ { name: 'g_project_management_issue_label_changed', unique: 'project.id' }
+ ]
+ end
+
+ it 'raises an exception' do
+ expect do
+ described_class.new(time_frame: '28d', options: { events: %w[g_project_management_issue_iteration_changed g_project_management_issue_label_changed] }, events: events).value
+ end.to raise_error(Gitlab::Usage::MetricDefinition::InvalidError)
+ end
+ end
+ end
+
describe 'children classes' do
let(:options) { { events: ['i_quickactions_approve'] } }
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 9b039ef1c3440..b787bf3baccd1 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
@@ -427,29 +427,26 @@
end
context "with a property_name for a legacy event" do
- it "tracks the events using a Redis key with the property_name" do
- expected_key = "{hll_counters}_g_analytics_productivity-project-2020-22"
- expect(Gitlab::Redis::HLL).to receive(:count).with(keys: [expected_key])
-
- described_class.unique_events(event_names: 'g_analytics_productivity', property_name: 'project', start_date: 7.days.ago, end_date: Date.current)
+ it "raises an error with an instructive message" do
+ expect do
+ described_class.unique_events(event_names: 'g_analytics_productivity', property_name: 'project', start_date: 7.days.ago, end_date: Date.current)
+ end.to raise_error(described_class::UnfinishedEventMigrationError, /migration\.html/)
end
end
context "with no property_name for a overridden event" do
- it "tracks the events using a Redis key with no property_name" do
- expected_key = "{hll_counters}_#{event_overridden_for_user}-2020-22"
- expect(Gitlab::Redis::HLL).to receive(:count).with(keys: [expected_key])
-
- described_class.unique_events(event_names: [event_overridden_for_user], start_date: 7.days.ago, end_date: Date.current)
+ it "raises an error with an instructive message" do
+ expect do
+ described_class.unique_events(event_names: [event_overridden_for_user], start_date: 7.days.ago, end_date: Date.current)
+ end.to raise_error(described_class::UnknownLegacyEventError, /hll_redis_legacy_events.yml/)
end
end
context "with no property_name for a new event" do
- it "tracks the events using a Redis key with no property_name" do
- expected_key = "{hll_counters}_#{no_slot}-2020-22"
- expect(Gitlab::Redis::HLL).to receive(:count).with(keys: [expected_key])
-
- described_class.unique_events(event_names: [no_slot], start_date: 7.days.ago, end_date: Date.current)
+ it "raises an error with an instructive message" do
+ expect do
+ described_class.unique_events(event_names: [no_slot], start_date: 7.days.ago, end_date: Date.current)
+ end.to raise_error(described_class::UnknownLegacyEventError, /hll_redis_legacy_events.yml/)
end
end
end
@@ -471,11 +468,10 @@
described_class.unique_events(event_names: [no_slot], property_name: 'project', start_date: 7.days.ago, end_date: Date.current)
end
- it "uses old Redis key for new events when no property name sent" do
- expected_key = "{hll_counters}_#{no_slot}-2020-22"
- expect(Gitlab::Redis::HLL).to receive(:count).with(keys: [expected_key])
-
- described_class.unique_events(event_names: [no_slot], start_date: 7.days.ago, end_date: Date.current)
+ it "raises an error with an instructive message" do
+ expect do
+ described_class.unique_events(event_names: [no_slot], start_date: 7.days.ago, end_date: Date.current)
+ end.to raise_error(described_class::UnknownLegacyEventError, /hll_redis_legacy_events.yml/)
end
end
end
diff --git a/spec/lib/gitlab/usage_data_counters/issue_activity_unique_counter_spec.rb b/spec/lib/gitlab/usage_data_counters/issue_activity_unique_counter_spec.rb
index 2c2bdbeb3e6c4..ef68978b52236 100644
--- a/spec/lib/gitlab/usage_data_counters/issue_activity_unique_counter_spec.rb
+++ b/spec/lib/gitlab/usage_data_counters/issue_activity_unique_counter_spec.rb
@@ -251,8 +251,12 @@
end
events = [described_class::ISSUE_TITLE_CHANGED, described_class::ISSUE_DESCRIPTION_CHANGED, described_class::ISSUE_ASSIGNEE_CHANGED]
- week_count = Gitlab::UsageDataCounters::HLLRedisCounter.unique_events(event_names: events, start_date: time.beginning_of_week,
- end_date: time + 1.week)
+ week_count = Gitlab::UsageDataCounters::HLLRedisCounter.unique_events(
+ event_names: events,
+ property_name: :user,
+ start_date: time.beginning_of_week,
+ end_date: time + 1.week
+ )
expect(week_count).to eq(3)
end
diff --git a/spec/services/design_management/delete_designs_service_spec.rb b/spec/services/design_management/delete_designs_service_spec.rb
index 79274599b995e..8c94d17d7cd66 100644
--- a/spec/services/design_management/delete_designs_service_spec.rb
+++ b/spec/services/design_management/delete_designs_service_spec.rb
@@ -99,7 +99,7 @@ def run_service(delenda = nil)
rescue StandardError
nil
end
- .not_to change { redis_hll.unique_events(event_names: event, start_date: Date.today, end_date: 1.week.from_now) }
+ .not_to change { redis_hll.unique_events(event_names: event, property_name: :user, start_date: Date.today, end_date: 1.week.from_now) }
begin
run_service
diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb
index a46a1438d1864..5517cafe1cda3 100644
--- a/spec/services/notes/create_service_spec.rb
+++ b/spec/services/notes/create_service_spec.rb
@@ -203,7 +203,7 @@
expect do
execute_create_service
end.to change {
- counter.unique_events(event_names: event, start_date: Date.today.beginning_of_week, end_date: 1.week.from_now)
+ counter.unique_events(event_names: event, property_name: :user, start_date: Date.today.beginning_of_week, end_date: 1.week.from_now)
}.by(1)
end
diff --git a/spec/services/notes/destroy_service_spec.rb b/spec/services/notes/destroy_service_spec.rb
index 33c973a2431a2..d17a6a331a945 100644
--- a/spec/services/notes/destroy_service_spec.rb
+++ b/spec/services/notes/destroy_service_spec.rb
@@ -39,7 +39,7 @@
expect do
service_action
end.to change {
- counter.unique_events(event_names: event, start_date: Date.today.beginning_of_week, end_date: 1.week.from_now)
+ counter.unique_events(event_names: event, property_name: :user, start_date: Date.today.beginning_of_week, end_date: 1.week.from_now)
}.by(1)
end
diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb
index 908f348c68bb5..28ab98cd59261 100644
--- a/spec/services/notes/update_service_spec.rb
+++ b/spec/services/notes/update_service_spec.rb
@@ -90,7 +90,7 @@ def update_note(opts)
.and_call_original
expect do
update_note(note: 'new text')
- end.to change { counter.unique_events(event_names: event, start_date: Date.today.beginning_of_week, end_date: 1.week.from_now) }.by(1)
+ end.to change { counter.unique_events(event_names: event, property_name: :user, start_date: Date.today.beginning_of_week, end_date: 1.week.from_now) }.by(1)
end
end
diff --git a/spec/support/shared_examples/metrics_instrumentation_shared_examples.rb b/spec/support/shared_examples/metrics_instrumentation_shared_examples.rb
index 4249b90bf6629..367924ab76833 100644
--- a/spec/support/shared_examples/metrics_instrumentation_shared_examples.rb
+++ b/spec/support/shared_examples/metrics_instrumentation_shared_examples.rb
@@ -3,8 +3,8 @@
RSpec.shared_examples 'a correct instrumented metric value' do |params|
let(:time_frame) { params[:time_frame] }
let(:options) { params[:options] }
- let(:events) { params[:events] }
- let(:metric) { described_class.new(time_frame: time_frame, options: options, events: events) }
+ let(:events) { params.slice(:events) }
+ let(:metric) { described_class.new({ time_frame: time_frame, options: options }.merge(events)) }
around do |example|
freeze_time { example.run }
--
GitLab