From 6f0165068d1466d80b6a41de85ba98ca8b649ad9 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Wed, 10 Nov 2021 15:35:24 +0000 Subject: [PATCH] Remove import-specific metris from Gitlab::Import::Metrics - remove project tag from github importer histogram, because it doesn't bring much value since this data is used as time vectors on prometheus/grafana. - Use the same metrics for all importers that uses this class (Bitbucket, Github), this will help to normalize the dashboards and reduce the cognitive load from the developers, since all dashboards can have the same format. Changelog: changed MR: https://gitlab.com/gitlab-com/runbooks/-/merge_requests/4082 --- lib/gitlab/import/metrics.rb | 18 ++--- .../known_events/importer_events.yml | 72 +++++++++++++++++-- .../github_import/parallel_scheduling_spec.rb | 9 ++- spec/lib/gitlab/import/metrics_spec.rb | 67 +++-------------- spec/services/projects/import_service_spec.rb | 2 +- 5 files changed, 90 insertions(+), 78 deletions(-) diff --git a/lib/gitlab/import/metrics.rb b/lib/gitlab/import/metrics.rb index 5f27d0ab9659ae..004382c7b5c98c 100644 --- a/lib/gitlab/import/metrics.rb +++ b/lib/gitlab/import/metrics.rb @@ -15,9 +15,7 @@ def initialize(importer, project) end def track_start_import - return unless project.github_import? - - track_usage_event(:github_import_project_start, project.id) + track_usage_event(:"#{importer}_project_start", project.id) end def track_finished_import @@ -29,9 +27,7 @@ def track_finished_import end def track_failed_import - return unless project.github_import? - - track_usage_event(:github_import_project_failure, project.id) + track_usage_event(:"#{importer}_project_failure", project.id) end def issues_counter @@ -69,17 +65,11 @@ def projects_counter end def observe_histogram - if project.github_import? - duration_histogram.observe({ project: project.full_path }, duration) - else - duration_histogram.observe({ importer: importer }, duration) - end + duration_histogram.observe({ importer: importer }, duration) end def track_finish_metric - return unless project.github_import? - - track_usage_event(:github_import_project_success, project.id) + track_usage_event(:"#{importer}_project_success", project.id) end end end diff --git a/lib/gitlab/usage_data_counters/known_events/importer_events.yml b/lib/gitlab/usage_data_counters/known_events/importer_events.yml index 79bbac229bc86d..c3c5ed1e9b825f 100644 --- a/lib/gitlab/usage_data_counters/known_events/importer_events.yml +++ b/lib/gitlab/usage_data_counters/known_events/importer_events.yml @@ -1,16 +1,80 @@ --- -# Importer events -- name: github_import_project_start +# Gitlab Importer +- name: gitlab_project_importer_project_start category: importer redis_slot: import aggregation: weekly feature_flag: track_importer_activity -- name: github_import_project_success +- name: gitlab_project_importer_project_success category: importer redis_slot: import aggregation: weekly feature_flag: track_importer_activity -- name: github_import_project_failure +- name: gitlab_project_importer_project_failure + category: importer + redis_slot: import + aggregation: weekly + feature_flag: track_importer_activity +# Git Importer +- name: git_importer_project_start + category: importer + redis_slot: import + aggregation: weekly + feature_flag: track_importer_activity +- name: git_importer_project_success + category: importer + redis_slot: import + aggregation: weekly + feature_flag: track_importer_activity +- name: git_importer_project_failure + category: importer + redis_slot: import + aggregation: weekly + feature_flag: track_importer_activity +# Github Importer +- name: github_importer_project_start + category: importer + redis_slot: import + aggregation: weekly + feature_flag: track_importer_activity +- name: github_importer_project_success + category: importer + redis_slot: import + aggregation: weekly + feature_flag: track_importer_activity +- name: github_importer_project_failure + category: importer + redis_slot: import + aggregation: weekly + feature_flag: track_importer_activity +# Bitbucket Cloud Importer +- name: bitbucket_importer_project_start + category: importer + redis_slot: import + aggregation: weekly + feature_flag: track_importer_activity +- name: bitbucket_importer_project_success + category: importer + redis_slot: import + aggregation: weekly + feature_flag: track_importer_activity +- name: bitbucket_importer_project_failure + category: importer + redis_slot: import + aggregation: weekly + feature_flag: track_importer_activity +# Bitbucket Server Importer +- name: bitbucket_server_importer_project_start + category: importer + redis_slot: import + aggregation: weekly + feature_flag: track_importer_activity +- name: bitbucket_server_importer_project_success + category: importer + redis_slot: import + aggregation: weekly + feature_flag: track_importer_activity +- name: bitbucket_server_importer_project_failure category: importer redis_slot: import aggregation: weekly diff --git a/spec/lib/gitlab/github_import/parallel_scheduling_spec.rb b/spec/lib/gitlab/github_import/parallel_scheduling_spec.rb index f375e84e0fd09d..69e395e7ce1ea4 100644 --- a/spec/lib/gitlab/github_import/parallel_scheduling_spec.rb +++ b/spec/lib/gitlab/github_import/parallel_scheduling_spec.rb @@ -25,7 +25,14 @@ def collection_method end end - let_it_be(:project) { create(:project, :import_started, import_source: 'foo/bar') } + let_it_be(:project) do + create( + :project, + :import_started, + import_type: :github, + import_source: 'foo/bar' + ) + end let(:client) { double(:client) } diff --git a/spec/lib/gitlab/import/metrics_spec.rb b/spec/lib/gitlab/import/metrics_spec.rb index 035294a620f756..b4215d90416dad 100644 --- a/spec/lib/gitlab/import/metrics_spec.rb +++ b/spec/lib/gitlab/import/metrics_spec.rb @@ -17,46 +17,18 @@ end describe '#track_start_import' do - context 'when project is not a github import' do - it 'does not emit importer metrics' do - expect(subject).not_to receive(:track_usage_event) - - subject.track_start_import - end - end - - context 'when project is a github import' do - before do - project.import_type = 'github' - end - - it 'emits importer metrics' do - expect(subject).to receive(:track_usage_event).with(:github_import_project_start, project.id) + it 'emits importer metrics' do + expect(subject).to receive(:track_usage_event).with(:test_importer_project_start, project.id) - subject.track_start_import - end + subject.track_start_import end end describe '#track_failed_import' do - context 'when project is not a github import' do - it 'does not emit importer metrics' do - expect(subject).not_to receive(:track_usage_event) - - subject.track_failed_import - end - end - - context 'when project is a github import' do - before do - project.import_type = 'github' - end - - it 'emits importer metrics' do - expect(subject).to receive(:track_usage_event).with(:github_import_project_failure, project.id) + it 'emits importer metrics' do + expect(subject).to receive(:track_usage_event).with(:test_importer_project_failure, project.id) - subject.track_failed_import - end + subject.track_failed_import end end @@ -66,6 +38,8 @@ end it 'emits importer metrics' do + expect(subject).to receive(:track_usage_event).with(:test_importer_project_success, project.id) + expect(Gitlab::Metrics).to receive(:counter).with( :test_importer_imported_projects_total, 'The number of imported projects' @@ -83,30 +57,7 @@ subject.track_finished_import expect(subject.duration).not_to be_nil - end - - context 'when project is not a github import' do - it 'does not emit importer metrics' do - expect(subject).not_to receive(:track_usage_event) - - subject.track_finished_import - - expect(histogram).to have_received(:observe).with({ importer: :test_importer }, anything) - end - end - - context 'when project is a github import' do - before do - project.import_type = 'github' - end - - it 'emits importer metrics' do - expect(subject).to receive(:track_usage_event).with(:github_import_project_success, project.id) - - subject.track_finished_import - - expect(histogram).to have_received(:observe).with({ project: project.full_path }, anything) - end + expect(histogram).to have_received(:observe).with({ importer: :test_importer }, anything) end end diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb index 1d63f72ec38f6f..3b5e57515bcc05 100644 --- a/spec/services/projects/import_service_spec.rb +++ b/spec/services/projects/import_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Projects::ImportService do - let!(:project) { create(:project) } + let!(:project) { create(:project, import_type: :git) } let(:user) { project.creator } subject { described_class.new(project, user) } -- GitLab