diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb index b5288aad6f0a4492844eeb403e78be1afd8cbdb1..4979af6dfe1d12360113d6c30c68680ddea12d20 100644 --- a/app/services/projects/import_service.rb +++ b/app/services/projects/import_service.rb @@ -16,6 +16,8 @@ def async? end def execute + track_start_import + add_repository_to_project download_lfs_objects @@ -25,16 +27,17 @@ def execute after_execute_hook success - rescue Gitlab::UrlBlocker::BlockedUrlError => e - Gitlab::ErrorTracking.track_exception(e, project_path: project.full_path, importer: project.import_type) + rescue Gitlab::UrlBlocker::BlockedUrlError, StandardError => e + Gitlab::Import::ImportFailureService.track( + project_id: project.id, + error_source: self.class.name, + exception: e, + metrics: true + ) - error(s_("ImportProjects|Error importing repository %{project_safe_import_url} into %{project_full_path} - %{message}") % { project_safe_import_url: project.safe_import_url, project_full_path: project.full_path, message: e.message }) - rescue StandardError => e message = Projects::ImportErrorFilter.filter_message(e.message) - - Gitlab::ErrorTracking.track_exception(e, project_path: project.full_path, importer: project.import_type) - - error(s_("ImportProjects|Error importing repository %{project_safe_import_url} into %{project_full_path} - %{message}") % { project_safe_import_url: project.safe_import_url, project_full_path: project.full_path, message: message }) + error(s_("ImportProjects|Error importing repository %{project_safe_import_url} into %{project_full_path} - %{message}") % + { project_safe_import_url: project.safe_import_url, project_full_path: project.full_path, message: message }) end protected @@ -54,6 +57,10 @@ def after_execute_hook # Defined in EE::Projects::ImportService end + def track_start_import + has_importer? && importer_class.try(:track_start_import, project) + end + def add_repository_to_project if project.external_import? && !unknown_url? begin diff --git a/app/workers/gitlab/github_import/stage/finish_import_worker.rb b/app/workers/gitlab/github_import/stage/finish_import_worker.rb index 006b79dbff47480ce93c0d7fa723705c6e21b4c4..5197c1e1e881eb4f2ef400e849823b3767b598f4 100644 --- a/app/workers/gitlab/github_import/stage/finish_import_worker.rb +++ b/app/workers/gitlab/github_import/stage/finish_import_worker.rb @@ -18,36 +18,28 @@ class FinishImportWorker # rubocop:disable Scalability/IdempotentWorker # project - An instance of Project. def import(_, project) + @project = project project.after_import - report_import_time(project) + report_import_time end - def report_import_time(project) - duration = Time.zone.now - project.created_at + private - histogram.observe({ project: project.full_path }, duration) - counter.increment + attr_reader :project + + def report_import_time + metrics.track_finished_import info( project.id, message: "GitHub project import finished", - duration_s: duration.round(2), + duration_s: metrics.duration.round(2), object_counts: ::Gitlab::GithubImport::ObjectCounter.summary(project) ) end - def histogram - @histogram ||= Gitlab::Metrics.histogram( - :github_importer_total_duration_seconds, - 'Total time spent importing GitHub projects, in seconds' - ) - end - - def counter - @counter ||= Gitlab::Metrics.counter( - :github_importer_imported_projects, - 'The number of imported GitHub projects' - ) + def metrics + @metrics ||= Gitlab::Import::Metrics.new(:github_importer, project) end end end diff --git a/app/workers/gitlab/github_import/stage/import_base_data_worker.rb b/app/workers/gitlab/github_import/stage/import_base_data_worker.rb index 715c39caf42568cd5ebc26eb52b2a63f415a485f..cc6a2255160b7c332f468850b1f26b05ffdae3aa 100644 --- a/app/workers/gitlab/github_import/stage/import_base_data_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_base_data_worker.rb @@ -31,6 +31,22 @@ def import(client, project) project.import_state.refresh_jid_expiration ImportPullRequestsWorker.perform_async(project.id) + rescue StandardError => e + Gitlab::Import::ImportFailureService.track( + project_id: project.id, + error_source: self.class.name, + exception: e, + fail_import: abort_on_failure, + metrics: true + ) + + raise(e) + end + + private + + def abort_on_failure + true end end end diff --git a/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb b/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb index d76d36531d1ea295015ceb4c1d9f4e4a82aa113a..71d0247bae0c0bdc841be22ddadf574a841cc397 100644 --- a/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb @@ -27,6 +27,22 @@ def import(client, project) { waiter.key => waiter.jobs_remaining }, :pull_requests_merged_by ) + rescue StandardError => e + Gitlab::Import::ImportFailureService.track( + project_id: project.id, + error_source: self.class.name, + exception: e, + fail_import: abort_on_failure, + metrics: true + ) + + raise(e) + end + + private + + def abort_on_failure + true end end end diff --git a/app/workers/gitlab/github_import/stage/import_repository_worker.rb b/app/workers/gitlab/github_import/stage/import_repository_worker.rb index 227b7c304b05d9275637000b541ea8a325c186b7..3e914cc75907c1589a0d553f2a7b5898f6305e5b 100644 --- a/app/workers/gitlab/github_import/stage/import_repository_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_repository_worker.rb @@ -33,6 +33,17 @@ def import(client, project) counter.increment ImportBaseDataWorker.perform_async(project.id) + + rescue StandardError => e + Gitlab::Import::ImportFailureService.track( + project_id: project.id, + error_source: self.class.name, + exception: e, + fail_import: abort_on_failure, + metrics: true + ) + + raise(e) end def counter diff --git a/config/feature_flags/development/track_importer_activity.yml b/config/feature_flags/development/track_importer_activity.yml new file mode 100644 index 0000000000000000000000000000000000000000..9f20a14790edf83d1a8509992d93b8377c319915 --- /dev/null +++ b/config/feature_flags/development/track_importer_activity.yml @@ -0,0 +1,8 @@ +--- +name: track_importer_activity +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/70012 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/339392 +milestone: '14.4' +type: development +group: group::import +default_enabled: false diff --git a/config/metrics/counts_28d/20210930125418_github_import_project_start_monthly.yml b/config/metrics/counts_28d/20210930125418_github_import_project_start_monthly.yml new file mode 100644 index 0000000000000000000000000000000000000000..2812aa73cad69721a8472fce4080aacab537f2a1 --- /dev/null +++ b/config/metrics/counts_28d/20210930125418_github_import_project_start_monthly.yml @@ -0,0 +1,26 @@ +--- +key_path: redis_hll_counters.importer.github_import_project_start_monthly +description: The number of github projects that were enqueued to start monthy +product_section: dev +product_stage: devops +product_group: group::import +product_category: +value_type: number +status: active +milestone: "14.4" +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/70012 +time_frame: 28d +data_source: redis_hll +data_category: optional +instrumentation_class: RedisHLLMetric +options: + events: + - github_import_project_start +performance_indicator_type: [] +distribution: +- ce +- ee +tier: +- free +- premium +- ultimate diff --git a/config/metrics/counts_28d/20210930130531_github_import_project_success_monthly.yml b/config/metrics/counts_28d/20210930130531_github_import_project_success_monthly.yml new file mode 100644 index 0000000000000000000000000000000000000000..ab599c6737693b1d8c984d8d015fb5dc35a8738d --- /dev/null +++ b/config/metrics/counts_28d/20210930130531_github_import_project_success_monthly.yml @@ -0,0 +1,25 @@ +--- +key_path: redis_hll_counters.importer.github_import_project_success_monthly +description: The number of github projects that were successful monthly +product_section: dev +product_stage: devops +product_group: group::import +product_category: +value_type: number +status: active +milestone: "14.4" +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/70012 +time_frame: 28d +data_source: redis_hll +data_category: optional +instrumentation_class: RedisHLLMetric +options: + events: + - github_import_project_success +distribution: +- ce +- ee +tier: +- free +- premium +- ultimate diff --git a/config/metrics/counts_28d/20210930163813_github_import_project_failure_monthly.yml b/config/metrics/counts_28d/20210930163813_github_import_project_failure_monthly.yml new file mode 100644 index 0000000000000000000000000000000000000000..6651a770920204847d4952bffc7be476a1486455 --- /dev/null +++ b/config/metrics/counts_28d/20210930163813_github_import_project_failure_monthly.yml @@ -0,0 +1,25 @@ +--- +key_path: redis_hll_counters.importer.github_import_project_failure_monthly +description: The number of github projects that failed monthly +product_section: dev +product_stage: devops +product_group: group::import +product_category: +value_type: number +status: active +milestone: "14.4" +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/70012 +time_frame: 28d +data_source: redis_hll +data_category: optional +instrumentation_class: RedisHLLMetric +options: + events: + - github_import_project_failure +distribution: +- ce +- ee +tier: +- free +- premium +- ultimate diff --git a/config/metrics/counts_7d/20210930125411_github_import_project_start_weekly.yml b/config/metrics/counts_7d/20210930125411_github_import_project_start_weekly.yml new file mode 100644 index 0000000000000000000000000000000000000000..2c5b7d46e1ae72bdb6180457f50fe7ad7a855678 --- /dev/null +++ b/config/metrics/counts_7d/20210930125411_github_import_project_start_weekly.yml @@ -0,0 +1,26 @@ +--- +key_path: redis_hll_counters.importer.github_import_project_start_weekly +description: The number of github projects that were enqueued to start weekly +product_section: dev +product_stage: devops +product_group: group::import +product_category: +value_type: number +status: active +milestone: "14.4" +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/70012 +time_frame: 7d +data_source: redis_hll +data_category: optional +instrumentation_class: RedisHLLMetric +options: + events: + - github_import_project_start +performance_indicator_type: [] +distribution: +- ce +- ee +tier: +- free +- premium +- ultimate diff --git a/config/metrics/counts_7d/20210930130525_github_import_project_success_weekly.yml b/config/metrics/counts_7d/20210930130525_github_import_project_success_weekly.yml new file mode 100644 index 0000000000000000000000000000000000000000..10147658ddc2b5a3ca690e9c40fdcdc6a032bb22 --- /dev/null +++ b/config/metrics/counts_7d/20210930130525_github_import_project_success_weekly.yml @@ -0,0 +1,26 @@ +--- +key_path: redis_hll_counters.importer.github_import_project_success_weekly +description: The number of github projects that were successful weekly +product_section: dev +product_stage: devops +product_group: group::import +product_category: +value_type: number +status: active +milestone: "14.4" +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/70012 +time_frame: 7d +data_source: redis_hll +data_category: optional +instrumentation_class: RedisHLLMetric +options: + events: + - github_import_project_success +performance_indicator_type: [] +distribution: +- ce +- ee +tier: +- free +- premium +- ultimate diff --git a/config/metrics/counts_7d/20210930163807_github_import_project_failure_weekly.yml b/config/metrics/counts_7d/20210930163807_github_import_project_failure_weekly.yml new file mode 100644 index 0000000000000000000000000000000000000000..33a1902504fbf5cb47c916ca108befa3baa93a50 --- /dev/null +++ b/config/metrics/counts_7d/20210930163807_github_import_project_failure_weekly.yml @@ -0,0 +1,25 @@ +--- +key_path: redis_hll_counters.importer.github_import_project_failure_weekly +description: The number of github projects that failed weekly +product_section: dev +product_stage: devops +product_group: group::import +product_category: +value_type: number +status: active +milestone: "14.4" +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/70012 +time_frame: 7d +data_source: redis_hll +data_category: optional +instrumentation_class: RedisHLLMetric +options: + events: + - github_import_project_failure +distribution: +- ce +- ee +tier: +- free +- premium +- ultimate diff --git a/ee/spec/config/metrics/every_metric_definition_spec.rb b/ee/spec/config/metrics/every_metric_definition_spec.rb index 50fa7ec3c74b40ad84a601504c35e9e0056bfdd3..bc6c2ac4f2f93a3cffec81d4ae316483927bd373 100644 --- a/ee/spec/config/metrics/every_metric_definition_spec.rb +++ b/ee/spec/config/metrics/every_metric_definition_spec.rb @@ -75,8 +75,10 @@ def object_with_schema?(key_path) stub_usage_data_connections end - it 'is included in the Usage Ping hash structure' do + it 'is included in the Usage Ping hash structure', :aggregate_failures do + msg = "see https://docs.gitlab.com/ee/development/service_ping/metrics_dictionary.html#metrics-added-dynamic-to-service-ping-payload" expect(metric_files_key_paths).to match_array(usage_ping_key_paths) + expect(metric_files_key_paths).to match_array(usage_ping_key_paths), msg end context 'with value json schema' do diff --git a/lib/gitlab/github_import/parallel_importer.rb b/lib/gitlab/github_import/parallel_importer.rb index 2429fa4de1d380090e081e9ff82164a0aa4b8f0f..f72e595e8e913204b0f7a9417f1559bbc1c65636 100644 --- a/lib/gitlab/github_import/parallel_importer.rb +++ b/lib/gitlab/github_import/parallel_importer.rb @@ -15,6 +15,10 @@ def self.imports_repository? true end + def self.track_start_import(project) + Gitlab::Import::Metrics.new(:github_importer, project).track_start_import + end + # This is a workaround for a Ruby 2.3.7 bug. rspec-mocks cannot restore # the visibility of prepended modules. See # https://github.com/rspec/rspec-mocks/issues/1231 for more details. diff --git a/lib/gitlab/github_import/parallel_scheduling.rb b/lib/gitlab/github_import/parallel_scheduling.rb index 4d0074e43d722831e808ef8287c76c6a973260ab..a8e006ea082974f51d4625edf805445e8f34d864 100644 --- a/lib/gitlab/github_import/parallel_scheduling.rb +++ b/lib/gitlab/github_import/parallel_scheduling.rb @@ -53,7 +53,8 @@ def execute project_id: project.id, error_source: self.class.name, exception: e, - fail_import: abort_on_failure + fail_import: abort_on_failure, + metrics: true ) raise(e) diff --git a/lib/gitlab/github_import/sequential_importer.rb b/lib/gitlab/github_import/sequential_importer.rb index cb6b20172084c2b7d6bcc655ccc7237a4e62c027..6bc373377995d599014811abec7e423ea71ea517 100644 --- a/lib/gitlab/github_import/sequential_importer.rb +++ b/lib/gitlab/github_import/sequential_importer.rb @@ -33,18 +33,41 @@ def initialize(project, token: nil, host: nil) end def execute - Importer::RepositoryImporter.new(project, client).execute + metrics.track_start_import - SEQUENTIAL_IMPORTERS.each do |klass| - klass.new(project, client).execute + begin + Importer::RepositoryImporter.new(project, client).execute + + SEQUENTIAL_IMPORTERS.each do |klass| + klass.new(project, client).execute + end + + rescue StandardError => e + Gitlab::Import::ImportFailureService.track( + project_id: project.id, + error_source: self.class.name, + exception: e, + fail_import: true, + metrics: true + ) + + raise(e) end PARALLEL_IMPORTERS.each do |klass| klass.new(project, client, parallel: false).execute end + metrics.track_finished_import + true end + + private + + def metrics + @metrics ||= Gitlab::Import::Metrics.new(:github_importer, project) + end end end end diff --git a/lib/gitlab/import/import_failure_service.rb b/lib/gitlab/import/import_failure_service.rb index f808ed1b6e20a7bdc13d8b544b02a69ec64b7307..142c00f7a6b8373449c869bf9071f44be7f3c6f4 100644 --- a/lib/gitlab/import/import_failure_service.rb +++ b/lib/gitlab/import/import_failure_service.rb @@ -8,14 +8,15 @@ def self.track( import_state: nil, project_id: nil, error_source: nil, - fail_import: false + fail_import: false, + metrics: false ) new( exception: exception, import_state: import_state, project_id: project_id, error_source: error_source - ).execute(fail_import: fail_import) + ).execute(fail_import: fail_import, metrics: metrics) end def initialize(exception:, import_state: nil, project_id: nil, error_source: nil) @@ -35,10 +36,11 @@ def initialize(exception:, import_state: nil, project_id: nil, error_source: nil @error_source = error_source end - def execute(fail_import:) + def execute(fail_import:, metrics:) track_exception persist_failure + track_metrics if metrics import_state.mark_as_failed(exception.message) if fail_import end @@ -71,6 +73,10 @@ def persist_failure correlation_id_value: Labkit::Correlation::CorrelationId.current_or_new_id ) end + + def track_metrics + Gitlab::Import::Metrics.new("#{project.import_type}_importer", project).track_failed_import + end end end end diff --git a/lib/gitlab/import/metrics.rb b/lib/gitlab/import/metrics.rb index 45a061b45db0d2fcbe4cf65fb38a487edf7415ab..5f27d0ab9659aecbfbe6bebee10c148c7891ac4f 100644 --- a/lib/gitlab/import/metrics.rb +++ b/lib/gitlab/import/metrics.rb @@ -3,20 +3,35 @@ module Gitlab module Import class Metrics + include Gitlab::Utils::UsageData + IMPORT_DURATION_BUCKETS = [0.5, 1, 3, 5, 10, 60, 120, 240, 360, 720, 1440].freeze - attr_reader :importer + attr_reader :importer, :duration def initialize(importer, project) @importer = importer @project = project end + def track_start_import + return unless project.github_import? + + track_usage_event(:github_import_project_start, project.id) + end + def track_finished_import - duration = Time.zone.now - @project.created_at + @duration = Time.zone.now - project.created_at - duration_histogram.observe({ importer: importer }, duration) + observe_histogram projects_counter.increment + track_finish_metric + end + + def track_failed_import + return unless project.github_import? + + track_usage_event(:github_import_project_failure, project.id) end def issues_counter @@ -35,6 +50,8 @@ def merge_requests_counter private + attr_reader :project + def duration_histogram @duration_histogram ||= Gitlab::Metrics.histogram( :"#{importer}_total_duration_seconds", @@ -50,6 +67,20 @@ def projects_counter 'The number of imported projects' ) end + + def observe_histogram + if project.github_import? + duration_histogram.observe({ project: project.full_path }, duration) + else + duration_histogram.observe({ importer: importer }, duration) + end + end + + def track_finish_metric + return unless project.github_import? + + track_usage_event(:github_import_project_success, project.id) + end 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 new file mode 100644 index 0000000000000000000000000000000000000000..79bbac229bc86dfac0617d82c8bd14ef2dafaedf --- /dev/null +++ b/lib/gitlab/usage_data_counters/known_events/importer_events.yml @@ -0,0 +1,17 @@ +--- +# Importer events +- name: github_import_project_start + category: importer + redis_slot: import + aggregation: weekly + feature_flag: track_importer_activity +- name: github_import_project_success + category: importer + redis_slot: import + aggregation: weekly + feature_flag: track_importer_activity +- name: github_import_project_failure + category: importer + redis_slot: import + aggregation: weekly + feature_flag: track_importer_activity diff --git a/spec/lib/gitlab/github_import/parallel_importer_spec.rb b/spec/lib/gitlab/github_import/parallel_importer_spec.rb index 06304bf84ca9c761e7e5940968f13c98e1affd13..c7b300ff043d080278a4b8b722eac9fdd17e8f94 100644 --- a/spec/lib/gitlab/github_import/parallel_importer_spec.rb +++ b/spec/lib/gitlab/github_import/parallel_importer_spec.rb @@ -9,6 +9,18 @@ end end + describe '.track_start_import' do + it 'tracks the start of import' do + project = double(:project) + metrics = double(:metrics) + + expect(Gitlab::Import::Metrics).to receive(:new).with(:github_importer, project).and_return(metrics) + expect(metrics).to receive(:track_start_import) + + described_class.track_start_import(project) + end + end + describe '#execute', :clean_gitlab_redis_shared_state do let(:project) { create(:project) } let(:importer) { described_class.new(project) } diff --git a/spec/lib/gitlab/github_import/parallel_scheduling_spec.rb b/spec/lib/gitlab/github_import/parallel_scheduling_spec.rb index 1fc7d3c887faf251fe3e1cf4cd32ebf75947d78f..f375e84e0fd09d9cd263d0afec9316e0aa6be7db 100644 --- a/spec/lib/gitlab/github_import/parallel_scheduling_spec.rb +++ b/spec/lib/gitlab/github_import/parallel_scheduling_spec.rb @@ -130,7 +130,8 @@ def collection_method project_id: project.id, exception: exception, error_source: 'MyImporter', - fail_import: false + fail_import: false, + metrics: true ).and_call_original expect { importer.execute } @@ -195,7 +196,8 @@ def abort_on_failure project_id: project.id, exception: exception, error_source: 'MyImporter', - fail_import: true + fail_import: true, + metrics: true ).and_call_original expect { importer.execute } diff --git a/spec/lib/gitlab/github_import/sequential_importer_spec.rb b/spec/lib/gitlab/github_import/sequential_importer_spec.rb index 3c3f8ff59d00710192f94fe36607edffa01fa243..2b76f0e27c922f1c5b8da6c4ab82e6e3c8781295 100644 --- a/spec/lib/gitlab/github_import/sequential_importer_spec.rb +++ b/spec/lib/gitlab/github_import/sequential_importer_spec.rb @@ -4,10 +4,17 @@ RSpec.describe Gitlab::GithubImport::SequentialImporter do describe '#execute' do + let_it_be(:project) do + create(:project, import_url: 'http://t0ken@github.another-domain.com/repo-org/repo.git', import_type: 'github') + end + + subject(:importer) { described_class.new(project, token: 'foo') } + it 'imports a project in sequence' do - repository = double(:repository) - project = double(:project, id: 1, repository: repository, import_url: 'http://t0ken@github.another-domain.com/repo-org/repo.git', group: nil) - importer = described_class.new(project, token: 'foo') + expect_next_instance_of(Gitlab::Import::Metrics) do |instance| + expect(instance).to receive(:track_start_import) + expect(instance).to receive(:track_finished_import) + end expect_next_instance_of(Gitlab::GithubImport::Importer::RepositoryImporter) do |instance| expect(instance).to receive(:execute) @@ -35,5 +42,23 @@ expect(importer.execute).to eq(true) end + + it 'raises an error' do + exception = StandardError.new('_some_error_') + + expect_next_instance_of(Gitlab::GithubImport::Importer::RepositoryImporter) do |importer| + expect(importer).to receive(:execute).and_raise(exception) + end + expect(Gitlab::Import::ImportFailureService).to receive(:track) + .with( + project_id: project.id, + exception: exception, + error_source: described_class.name, + fail_import: true, + metrics: true + ).and_call_original + + expect { importer.execute }.to raise_error(StandardError) + end end end diff --git a/spec/lib/gitlab/import/import_failure_service_spec.rb b/spec/lib/gitlab/import/import_failure_service_spec.rb index 50b32d634ad78e1c9bbff034daef0d3712015308..c16d4a7c80450ee126ae2ae3eb3e1abeb571929f 100644 --- a/spec/lib/gitlab/import/import_failure_service_spec.rb +++ b/spec/lib/gitlab/import/import_failure_service_spec.rb @@ -2,135 +2,171 @@ require 'spec_helper' -RSpec.describe Gitlab::Import::ImportFailureService do +RSpec.describe Gitlab::Import::ImportFailureService, :aggregate_failures do let_it_be(:import_type) { 'import_type' } + let_it_be(:project) { create(:project, :import_started, import_type: import_type) } - let_it_be(:project) do - create( - :project, - :import_started, - import_type: import_type - ) - end - - let(:import_state) { project.import_state } let(:exception) { StandardError.new('some error') } + let(:arguments) { { project_id: project.id } } + let(:base_arguments) { { error_source: 'SomeImporter', exception: exception }.merge(arguments) } + let(:exe_arguments) { { fail_import: false, metrics: false } } + + describe '.track' do + context 'with all arguments provided' do + let(:instance) { double(:failure_service) } + let(:instance_arguments) do + { + exception: exception, + import_state: '_import_state_', + project_id: '_project_id_', + error_source: '_error_source_' + } + end - shared_examples 'logs the exception and fails the import' do - it 'when the failure does not abort the import' do - expect(Gitlab::ErrorTracking) - .to receive(:track_exception) - .with( - exception, - project_id: project.id, - import_type: import_type, - source: 'SomeImporter' - ) - - expect(Gitlab::Import::Logger) - .to receive(:error) - .with( - message: 'importer failed', - 'error.message': 'some error', - project_id: project.id, - import_type: import_type, - source: 'SomeImporter' - ) - - described_class.track(**arguments) - - expect(project.import_state.reload.status).to eq('failed') - - expect(project.import_failures).not_to be_empty - expect(project.import_failures.last.exception_class).to eq('StandardError') - expect(project.import_failures.last.exception_message).to eq('some error') - end - end + let(:exe_arguments) do + { + fail_import: '_fail_import_', + metrics: '_metrics_' + } + end - shared_examples 'logs the exception and does not fail the import' do - it 'when the failure does not abort the import' do - expect(Gitlab::ErrorTracking) - .to receive(:track_exception) - .with( - exception, - project_id: project.id, - import_type: import_type, - source: 'SomeImporter' - ) - - expect(Gitlab::Import::Logger) - .to receive(:error) - .with( - message: 'importer failed', - 'error.message': 'some error', - project_id: project.id, - import_type: import_type, - source: 'SomeImporter' - ) - - described_class.track(**arguments) - - expect(project.import_state.reload.status).to eq('started') - - expect(project.import_failures).not_to be_empty - expect(project.import_failures.last.exception_class).to eq('StandardError') - expect(project.import_failures.last.exception_message).to eq('some error') + it 'invokes a new instance and executes' do + expect(described_class).to receive(:new).with(**instance_arguments).and_return(instance) + expect(instance).to receive(:execute).with(**exe_arguments) + + described_class.track(**instance_arguments.merge(exe_arguments)) + end end - end - context 'when using the project as reference' do - context 'when it fails the import' do - let(:arguments) do + context 'with only necessary arguments utilizing defaults' do + let(:instance) { double(:failure_service) } + let(:instance_arguments) do { - project_id: project.id, exception: exception, - error_source: 'SomeImporter', - fail_import: true + import_state: nil, + project_id: nil, + error_source: nil } end - it_behaves_like 'logs the exception and fails the import' - end - - context 'when it does not fail the import' do - let(:arguments) do + let(:exe_arguments) do { - project_id: project.id, - exception: exception, - error_source: 'SomeImporter', - fail_import: false + fail_import: false, + metrics: false } end - it_behaves_like 'logs the exception and does not fail the import' + it 'invokes a new instance and executes' do + expect(described_class).to receive(:new).with(**instance_arguments).and_return(instance) + expect(instance).to receive(:execute).with(**exe_arguments) + + described_class.track(exception: exception) + end end end - context 'when using the import_state as reference' do - context 'when it fails the import' do - let(:arguments) do - { - import_state: import_state, - exception: exception, - error_source: 'SomeImporter', - fail_import: true - } + describe '#execute' do + subject(:service) { described_class.new(**base_arguments) } + + shared_examples 'logs the exception and fails the import' do + it 'when the failure does not abort the import' do + expect(Gitlab::ErrorTracking) + .to receive(:track_exception) + .with( + exception, + project_id: project.id, + import_type: import_type, + source: 'SomeImporter' + ) + + expect(Gitlab::Import::Logger) + .to receive(:error) + .with( + message: 'importer failed', + 'error.message': 'some error', + project_id: project.id, + import_type: import_type, + source: 'SomeImporter' + ) + + service.execute(**exe_arguments) + + expect(project.import_state.reload.status).to eq('failed') + + expect(project.import_failures).not_to be_empty + expect(project.import_failures.last.exception_class).to eq('StandardError') + expect(project.import_failures.last.exception_message).to eq('some error') end + end - it_behaves_like 'logs the exception and fails the import' + shared_examples 'logs the exception and does not fail the import' do + it 'when the failure does not abort the import' do + expect(Gitlab::ErrorTracking) + .to receive(:track_exception) + .with( + exception, + project_id: project.id, + import_type: import_type, + source: 'SomeImporter' + ) + + expect(Gitlab::Import::Logger) + .to receive(:error) + .with( + message: 'importer failed', + 'error.message': 'some error', + project_id: project.id, + import_type: import_type, + source: 'SomeImporter' + ) + + service.execute(**exe_arguments) + + expect(project.import_state.reload.status).to eq('started') + + expect(project.import_failures).not_to be_empty + expect(project.import_failures.last.exception_class).to eq('StandardError') + expect(project.import_failures.last.exception_message).to eq('some error') + end end - context 'when it does not fail the import' do - let(:arguments) do - { - import_state: import_state, - exception: exception, - error_source: 'SomeImporter', - fail_import: false - } + context 'when tracking metrics' do + let(:exe_arguments) { { fail_import: false, metrics: true } } + + it 'tracks the failed import' do + metrics = double(:metrics) + + expect(Gitlab::Import::Metrics).to receive(:new).with("#{project.import_type}_importer", project).and_return(metrics) + expect(metrics).to receive(:track_failed_import) + + service.execute(**exe_arguments) end + end + + context 'when using the project as reference' do + context 'when it fails the import' do + let(:exe_arguments) { { fail_import: true, metrics: false } } - it_behaves_like 'logs the exception and does not fail the import' + it_behaves_like 'logs the exception and fails the import' + end + + context 'when it does not fail the import' do + it_behaves_like 'logs the exception and does not fail the import' + end + end + + context 'when using the import_state as reference' do + let(:arguments) { { import_state: project.import_state } } + + context 'when it fails the import' do + let(:exe_arguments) { { fail_import: true, metrics: false } } + + it_behaves_like 'logs the exception and fails the import' + end + + context 'when it does not fail the import' do + it_behaves_like 'logs the exception and does not fail the import' + end end end end diff --git a/spec/lib/gitlab/import/metrics_spec.rb b/spec/lib/gitlab/import/metrics_spec.rb index c79f26494337cf9ce48e8704b9badb42bd6dba59..035294a620f756cd9333bb3c48ea9123a7a0bee4 100644 --- a/spec/lib/gitlab/import/metrics_spec.rb +++ b/spec/lib/gitlab/import/metrics_spec.rb @@ -4,7 +4,7 @@ RSpec.describe Gitlab::Import::Metrics, :aggregate_failures do let(:importer) { :test_importer } - let(:project) { double(:project, created_at: Time.current) } + let(:project) { build(:project, id: non_existing_record_id, created_at: Time.current) } let(:histogram) { double(:histogram) } let(:counter) { double(:counter) } @@ -13,6 +13,51 @@ before do allow(Gitlab::Metrics).to receive(:counter) { counter } allow(counter).to receive(:increment) + allow(histogram).to receive(:observe) + 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) + + subject.track_start_import + end + 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) + + subject.track_failed_import + end + end end describe '#track_finished_import' do @@ -34,9 +79,34 @@ ) expect(counter).to receive(:increment) - expect(histogram).to receive(:observe).with({ importer: :test_importer }, anything) 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 end end 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 427dd4a205ee32d5eead2687b0206de8a88538b1..0ec805714e33ad3b7f3cd25336eec1235107fe88 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 @@ -47,6 +47,7 @@ 'epics_usage', 'epic_boards_usage', 'secure', + 'importer', 'network_policies' ) end diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb index 92e18b6cb46f09a158d70e293299947b61c03b8c..1d63f72ec38f6ff80ac12322dc206286396d1c4b 100644 --- a/spec/services/projects/import_service_spec.rb +++ b/spec/services/projects/import_service_spec.rb @@ -86,6 +86,12 @@ end context 'with a Github repository' do + it 'tracks the start of import' do + expect(Gitlab::GithubImport::ParallelImporter).to receive(:track_start_import) + + subject.execute + end + it 'succeeds if repository import was scheduled' do expect_any_instance_of(Gitlab::GithubImport::ParallelImporter) .to receive(:execute) diff --git a/spec/workers/gitlab/github_import/stage/finish_import_worker_spec.rb b/spec/workers/gitlab/github_import/stage/finish_import_worker_spec.rb index 132fe1dc618bec693020d47bd669803bcb6e7c4e..dd976eef28bfec1ff9d932f0ef6d3c5d57614389 100644 --- a/spec/workers/gitlab/github_import/stage/finish_import_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/finish_import_worker_spec.rb @@ -7,39 +7,27 @@ let(:worker) { described_class.new } describe '#perform' do - it 'marks the import as finished' do + it 'marks the import as finished and reports import statistics' do expect(project).to receive(:after_import) - expect(worker).to receive(:report_import_time).with(project) - - worker.import(double(:client), project) - end - end - - describe '#report_import_time' do - it 'reports the total import time' do - expect(worker.histogram) - .to receive(:observe) - .with({ project: project.path_with_namespace }, a_kind_of(Numeric)) - .and_call_original - - expect(worker.counter) - .to receive(:increment) - .and_call_original + expect_next_instance_of(Gitlab::Import::Metrics) do |instance| + expect(instance).to receive(:track_finished_import) + expect(instance).to receive(:duration).and_return(3.005) + end expect(Gitlab::GithubImport::Logger) .to receive(:info) - .with( - message: 'GitHub project import finished', - import_stage: 'Gitlab::GithubImport::Stage::FinishImportWorker', - object_counts: { - 'fetched' => {}, - 'imported' => {} - }, - project_id: project.id, - duration_s: a_kind_of(Numeric) - ) + .with( + message: 'GitHub project import finished', + import_stage: 'Gitlab::GithubImport::Stage::FinishImportWorker', + object_counts: { + 'fetched' => {}, + 'imported' => {} + }, + project_id: project.id, + duration_s: 3.01 + ) - worker.report_import_time(project) + worker.import(double(:client), project) end end end diff --git a/spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb b/spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb index f68d08385018ec8cdf2835422c6553e7ffaa654c..7b2218b1725a86bea4e456eaa6a7be0849ee1b7f 100644 --- a/spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb @@ -3,15 +3,15 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Stage::ImportBaseDataWorker do - let(:project) { create(:project) } - let(:import_state) { create(:import_state, project: project) } + let_it_be(:project) { create(:project) } + let_it_be(:import_state) { create(:import_state, project: project) } + let(:worker) { described_class.new } + let(:importer) { double(:importer) } + let(:client) { double(:client) } describe '#import' do it 'imports the base data of a project' do - importer = double(:importer) - client = double(:client) - described_class::IMPORTERS.each do |klass| expect(klass) .to receive(:new) @@ -29,5 +29,23 @@ worker.import(client, project) end + + it 'raises an error' do + exception = StandardError.new('_some_error_') + + expect_next_instance_of(Gitlab::GithubImport::Importer::LabelsImporter) do |importer| + expect(importer).to receive(:execute).and_raise(exception) + end + expect(Gitlab::Import::ImportFailureService).to receive(:track) + .with( + project_id: project.id, + exception: exception, + error_source: described_class.name, + fail_import: true, + metrics: true + ).and_call_original + + expect { worker.import(client, project) }.to raise_error(StandardError) + end end end diff --git a/spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb b/spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb index 29578f9bf37706f5bda94820e027f418fb30d083..b18b5ce64d16e79e7e1dbf938b4ab50426cf6ad0 100644 --- a/spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb @@ -3,14 +3,15 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Stage::ImportPullRequestsWorker do - let(:project) { create(:project) } - let(:import_state) { create(:import_state, project: project) } + let_it_be(:project) { create(:project) } + let_it_be(:import_state) { create(:import_state, project: project) } + let(:worker) { described_class.new } + let(:importer) { double(:importer) } + let(:client) { double(:client) } describe '#import' do it 'imports all the pull requests' do - importer = double(:importer) - client = double(:client) waiter = Gitlab::JobWaiter.new(2, '123') expect(Gitlab::GithubImport::Importer::PullRequestsImporter) @@ -32,4 +33,22 @@ worker.import(client, project) end end + + it 'raises an error' do + exception = StandardError.new('_some_error_') + + expect_next_instance_of(Gitlab::GithubImport::Importer::PullRequestsImporter) do |importer| + expect(importer).to receive(:execute).and_raise(exception) + end + expect(Gitlab::Import::ImportFailureService).to receive(:track) + .with( + project_id: project.id, + exception: exception, + error_source: described_class.name, + fail_import: true, + metrics: true + ).and_call_original + + expect { worker.import(client, project) }.to raise_error(StandardError) + end end diff --git a/spec/workers/gitlab/github_import/stage/import_repository_worker_spec.rb b/spec/workers/gitlab/github_import/stage/import_repository_worker_spec.rb index 875fc082975899f014a8d5de26f3b485a406f304..582cb76a6cd93c7b10334755f1c9a46768cc9e5e 100644 --- a/spec/workers/gitlab/github_import/stage/import_repository_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/import_repository_worker_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Stage::ImportRepositoryWorker do - let(:project) { double(:project, id: 4) } + let_it_be(:project) { create(:project, :import_started) } let(:worker) { described_class.new } @@ -43,6 +43,15 @@ expect(instance).to receive(:execute).and_raise(exception_class) end + expect(Gitlab::Import::ImportFailureService).to receive(:track) + .with( + project_id: project.id, + exception: exception_class, + error_source: described_class.name, + fail_import: true, + metrics: true + ).and_call_original + expect(Gitlab::GithubImport::Stage::ImportBaseDataWorker) .not_to receive(:perform_async)