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 3e8c119b695abf18088acddb417cbc6837613fa0..48dc53bfefc7fa8beb1e73f0eeb64cce79041895 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 9d536617030ec2ef404ddcb649f9ce2e48144be8..630c8dbe0a773856802d95eaee02a4a9ee32cf29 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 d3bbb3ee02cada0fcf1d24e78e0e53f6f8364ef1..743c80d0588abe4459f27ac9d6fdb497ff70d439 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 52b2fc8148c1a21ad48754ae7662c224079182cc..21df6d9fc5c9402e2e84d525d3d72ecf4947a1fe 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 780d9d3092a17080c5ed4e0fa9bf10b40c35ad60..c177ae39f9a45940717546688f3fac6bddc55733 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 33868d365a515f06f74c57fd95e0b01c531d1e6b..7f0b2ea7c99c31ffce927ce46436b920cc1aa217 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 9b039ef1c344018f24c3fdb7d3a44d2e71e80971..b787bf3baccd1e43ab75b967a316b8413e70adb5 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 2c2bdbeb3e6c48347b4128b6bc3a123d21234e89..ef68978b52236f3dd634deb5fc82809072d29ecc 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 79274599b995e64830d8073f76f2ae4a89cfa47c..8c94d17d7cd66c7f3c259b5e783b1afe48e2bdd9 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 a46a1438d1864757b3446184d7bc60152a71b62d..5517cafe1cda3280dc1e201678cd6090c5566b76 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 33c973a2431a2646189dfe7223ad16792904c967..d17a6a331a945724b710c1942b27f4aab82b3b1d 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 908f348c68bb542743ea5964dffd67d4063ac85a..28ab98cd59261dc810071538a12c971c0449435a 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 4249b90bf6629e0109825857fed798139e5549b3..367924ab76833b87beed8a1d2586837f7dd46760 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 }