From d82daadaa86e4b502aea5ba2196fb6b49d89849a Mon Sep 17 00:00:00 2001 From: lma-git Date: Fri, 6 Jan 2023 09:53:38 -0800 Subject: [PATCH 1/2] Log pipelines that exceed includes maximum Log pipelines whose includes count exceeds the maximum number allowed. This is to gauge how many customers would be affected by fixing the includes limit. --- app/services/ci/create_pipeline_service.rb | 7 ++ lib/gitlab/ci/pipeline/chain/command.rb | 9 +++ lib/gitlab/ci/pipeline/chain/sequence.rb | 1 + .../ci/create_pipeline_service/logger_spec.rb | 75 +++++++++++++++++++ 4 files changed, 92 insertions(+) diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index 9c3cc803587ce4..07b3705cad7399 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -157,6 +157,13 @@ def build_logger duration >= LOG_MAX_CREATION_THRESHOLD end + + l.log_when do |observations| + pipeline_includes = observations['pipeline_includes'] + next false unless pipeline_includes + + pipeline_includes[:count].to_i > pipeline_includes[:max] + end end end end diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb index 31b130b5ab73da..accdd6ca91bd25 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -127,6 +127,15 @@ def observe_jobs_count_in_alive_pipelines .observe({ plan: project.actual_plan_name }, jobs_count) end + def observe_pipeline_includes(pipeline) + logger.observe( + :pipeline_includes, { + count: pipeline.config_metadata&.[](:includes)&.count, + max: Gitlab::Ci::Config::External::Context::MAX_INCLUDES + }, once: true + ) + end + def increment_pipeline_failure_reason_counter(reason) metrics.pipeline_failure_reason_counter .increment(reason: (reason || :unknown_failure).to_s) diff --git a/lib/gitlab/ci/pipeline/chain/sequence.rb b/lib/gitlab/ci/pipeline/chain/sequence.rb index de14791485052e..ed7d7cbbd04a5b 100644 --- a/lib/gitlab/ci/pipeline/chain/sequence.rb +++ b/lib/gitlab/ci/pipeline/chain/sequence.rb @@ -30,6 +30,7 @@ def build! @command.observe_creation_duration(current_monotonic_time - @start) @command.observe_pipeline_size(@pipeline) @command.observe_jobs_count_in_alive_pipelines + @command.observe_pipeline_includes(@pipeline) @pipeline end diff --git a/spec/services/ci/create_pipeline_service/logger_spec.rb b/spec/services/ci/create_pipeline_service/logger_spec.rb index ccb15bfa684837..5e17e0701612db 100644 --- a/spec/services/ci/create_pipeline_service/logger_spec.rb +++ b/spec/services/ci/create_pipeline_service/logger_spec.rb @@ -139,5 +139,80 @@ expect(pipeline).to be_created_successfully end end + + describe 'pipeline includes count' do + before do + stub_const('Gitlab::Ci::Config::External::Context::MAX_INCLUDES', 2) + end + + context 'when the includes count exceeds the maximum' do + before do + allow_next_instance_of(Ci::Pipeline) do |pipeline| + allow(pipeline).to receive(:config_metadata) + .and_return({ includes: [{ file: 1 }, { file: 2 }, { file: 3 }] }) + end + end + + it 'creates a log entry' do + expect(Gitlab::AppJsonLogger) + .to receive(:info) + .with(a_hash_including({ 'pipeline_includes' => { 'count': 3, 'max': 2 } })) + .and_call_original + + expect(pipeline).to be_created_successfully + end + end + + context 'when the includes count does not exceed the maximum' do + before do + allow_next_instance_of(Ci::Pipeline) do |pipeline| + allow(pipeline).to receive(:config_metadata) + .and_return({ includes: [{ file: 1 }, { file: 2 }] }) + end + end + + it 'does not create a log entry but it collects the data' do + expect(Gitlab::AppJsonLogger).not_to receive(:info) + expect(pipeline).to be_created_successfully + + expect(service.logger.observations_hash) + .to match(a_hash_including({ 'pipeline_includes' => { 'count': 2, 'max': 2 } })) + end + end + + context 'when the includes data is nil' do + before do + allow_next_instance_of(Ci::Pipeline) do |pipeline| + allow(pipeline).to receive(:config_metadata) + .and_return({}) + end + end + + it 'does not create a log entry but it collects the data' do + expect(Gitlab::AppJsonLogger).not_to receive(:info) + expect(pipeline).to be_created_successfully + + expect(service.logger.observations_hash) + .to match(a_hash_including({ 'pipeline_includes' => { 'count': nil, 'max': 2 } })) + end + end + + context 'when the pipeline config_metadata is nil' do + before do + allow_next_instance_of(Ci::Pipeline) do |pipeline| + allow(pipeline).to receive(:config_metadata) + .and_return(nil) + end + end + + it 'does not create a log entry but it collects the data' do + expect(Gitlab::AppJsonLogger).not_to receive(:info) + expect(pipeline).to be_created_successfully + + expect(service.logger.observations_hash) + .to match(a_hash_including({ 'pipeline_includes' => { 'count': nil, 'max': 2 } })) + end + end + end end end -- GitLab From ca4d32fd3733f3bc39b48c1317447159d2ffab8f Mon Sep 17 00:00:00 2001 From: lma-git Date: Tue, 10 Jan 2023 09:12:53 -0800 Subject: [PATCH 2/2] Remove logging the includes maximum Is isn't necessary to log the includes maximum value. So we can remove it here to simplify the logs. --- app/services/ci/create_pipeline_service.rb | 6 +++--- lib/gitlab/ci/pipeline/chain/command.rb | 9 ++------- lib/gitlab/ci/pipeline/chain/sequence.rb | 2 +- .../ci/create_pipeline_service/logger_spec.rb | 12 +++--------- 4 files changed, 9 insertions(+), 20 deletions(-) diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index 07b3705cad7399..eb25aeaf5a5d2b 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -159,10 +159,10 @@ def build_logger end l.log_when do |observations| - pipeline_includes = observations['pipeline_includes'] - next false unless pipeline_includes + pipeline_includes_count = observations['pipeline_includes_count'] + next false unless pipeline_includes_count - pipeline_includes[:count].to_i > pipeline_includes[:max] + pipeline_includes_count.to_i > Gitlab::Ci::Config::External::Context::MAX_INCLUDES end end end diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb index accdd6ca91bd25..d2dc712e366a6b 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -127,13 +127,8 @@ def observe_jobs_count_in_alive_pipelines .observe({ plan: project.actual_plan_name }, jobs_count) end - def observe_pipeline_includes(pipeline) - logger.observe( - :pipeline_includes, { - count: pipeline.config_metadata&.[](:includes)&.count, - max: Gitlab::Ci::Config::External::Context::MAX_INCLUDES - }, once: true - ) + def observe_pipeline_includes_count(pipeline) + logger.observe(:pipeline_includes_count, pipeline.config_metadata&.[](:includes)&.count, once: true) end def increment_pipeline_failure_reason_counter(reason) diff --git a/lib/gitlab/ci/pipeline/chain/sequence.rb b/lib/gitlab/ci/pipeline/chain/sequence.rb index ed7d7cbbd04a5b..dd097187955ca4 100644 --- a/lib/gitlab/ci/pipeline/chain/sequence.rb +++ b/lib/gitlab/ci/pipeline/chain/sequence.rb @@ -30,7 +30,7 @@ def build! @command.observe_creation_duration(current_monotonic_time - @start) @command.observe_pipeline_size(@pipeline) @command.observe_jobs_count_in_alive_pipelines - @command.observe_pipeline_includes(@pipeline) + @command.observe_pipeline_includes_count(@pipeline) @pipeline end diff --git a/spec/services/ci/create_pipeline_service/logger_spec.rb b/spec/services/ci/create_pipeline_service/logger_spec.rb index 5e17e0701612db..ecb24a610752f2 100644 --- a/spec/services/ci/create_pipeline_service/logger_spec.rb +++ b/spec/services/ci/create_pipeline_service/logger_spec.rb @@ -156,7 +156,7 @@ it 'creates a log entry' do expect(Gitlab::AppJsonLogger) .to receive(:info) - .with(a_hash_including({ 'pipeline_includes' => { 'count': 3, 'max': 2 } })) + .with(a_hash_including({ 'pipeline_includes_count' => 3 })) .and_call_original expect(pipeline).to be_created_successfully @@ -176,7 +176,7 @@ expect(pipeline).to be_created_successfully expect(service.logger.observations_hash) - .to match(a_hash_including({ 'pipeline_includes' => { 'count': 2, 'max': 2 } })) + .to match(a_hash_including({ 'pipeline_includes_count' => 2 })) end end @@ -188,12 +188,9 @@ end end - it 'does not create a log entry but it collects the data' do + it 'does not create a log entry' do expect(Gitlab::AppJsonLogger).not_to receive(:info) expect(pipeline).to be_created_successfully - - expect(service.logger.observations_hash) - .to match(a_hash_including({ 'pipeline_includes' => { 'count': nil, 'max': 2 } })) end end @@ -208,9 +205,6 @@ it 'does not create a log entry but it collects the data' do expect(Gitlab::AppJsonLogger).not_to receive(:info) expect(pipeline).to be_created_successfully - - expect(service.logger.observations_hash) - .to match(a_hash_including({ 'pipeline_includes' => { 'count': nil, 'max': 2 } })) end end end -- GitLab