diff --git a/notifier/migration.rb b/notifier/migration.rb index 13e9b62353176e0db0a415abd0490b9aac60f87f..1f7577fe3c43c9d4f6f32cd1deb332c9801abe6a 100644 --- a/notifier/migration.rb +++ b/notifier/migration.rb @@ -3,6 +3,7 @@ class Migration REGULAR_MIGRATION_GUIDANCE_SECONDS = 3.minutes.freeze POST_DEPLOY_MIGRATION_GUIDANCE_SECONDS = 10.minutes.freeze + POST_DEPLOY_CONCURRENT_MIGRATION_GUIDANCE_SECONDS = 20.minutes.freeze SRE_NOTIFICATION_GUIDANCE = 20.minutes.freeze TYPE_REGULAR = 'regular' TYPE_POST_DEPLOY = 'post_deploy' @@ -95,9 +96,18 @@ class Migration type == TYPE_BACKGROUND_BATCH end + def concurrent_only? + important_queries.all?(&:concurrent?) + end + def time_guidance return REGULAR_MIGRATION_GUIDANCE_SECONDS if regular? - return POST_DEPLOY_MIGRATION_GUIDANCE_SECONDS if post_deploy? + + if post_deploy? + return POST_DEPLOY_CONCURRENT_MIGRATION_GUIDANCE_SECONDS if concurrent_only? + + return POST_DEPLOY_MIGRATION_GUIDANCE_SECONDS + end raise 'Unknown migration type' end @@ -144,7 +154,7 @@ class Migration warnings << "#{name_formatted} did not complete successfully, check the job log for details" unless success? if sre_should_be_informed? - warnings << "#{name_formatted} took #{walltime_minutes}. Please add a comment that mentions Release "\ + warnings << "#{name_formatted} took #{walltime_minutes}. Please add a comment that mentions Release " \ "Managers (`@gitlab-org/release/managers`) so they are informed." end @@ -155,13 +165,13 @@ class Migration def time_remedy if regular? && walltime < POST_DEPLOY_MIGRATION_GUIDANCE_SECONDS - "#{name_formatted} may need a [post-deploy migration](#{POST_DEPLOY_MIGRATION_GUIDE}) "\ - "to comply with [timing guidelines](#{TIMING_GUIDELINES}). It took #{walltime_minutes}, "\ - "but should not exceed #{time_guidance_minutes}" + "#{name_formatted} may need a [post-deploy migration](#{POST_DEPLOY_MIGRATION_GUIDE}) " \ + "to comply with [timing guidelines](#{TIMING_GUIDELINES}). It took #{walltime_minutes}, " \ + "but should not exceed #{time_guidance_minutes}" else - "#{name_formatted} may need a [background migration](#{BACKGROUND_MIGRATION_GUIDE}) to "\ - "comply with [timing guidelines](#{TIMING_GUIDELINES}). It took #{walltime_minutes}, "\ - "but should not exceed #{time_guidance_minutes}" + "#{name_formatted} may need a [background migration](#{BACKGROUND_MIGRATION_GUIDE}) to " \ + "comply with [timing guidelines](#{TIMING_GUIDELINES}). It took #{walltime_minutes}, " \ + "but should not exceed #{time_guidance_minutes}" end end @@ -185,7 +195,7 @@ class Migration def init_queries(stats) @queries = stats['query_statistics'].map do |query| - Query.new(query) + Query.new(query, execution_context: self) end end diff --git a/notifier/query.rb b/notifier/query.rb index df1f616037c77708d18e36c39abccbdfc6b457c9..b6e0473c42c28fc7657eadd822eb7302595ca960 100644 --- a/notifier/query.rb +++ b/notifier/query.rb @@ -7,18 +7,20 @@ require_relative 'query_exclusion' class Query QUERY_GUIDANCE_MILLISECONDS = 100 CONCURRENT_QUERY_GUIDANCE_MILLISECONDS = 5.minutes.in_milliseconds + CONCURRENT_POSTMIGRATE_QUERY_GUIDANCE_MILLISECONDS = 20.minutes.in_milliseconds TIMING_GUIDELINES = 'https://docs.gitlab.com/ee/development/query_performance.html#timing-guidelines-for-queries' CREATE_TABLE_STATMENT = 'create table' - attr_accessor :query, :calls, :total_time, :max_time, :mean_time, :rows, :executions + attr_accessor :query, :calls, :total_time, :max_time, :mean_time, :rows, :executions, :execution_context - def initialize(pgss_row) + def initialize(pgss_row, execution_context: nil) @query = PgQuery.normalize(pgss_row['query']) @calls = pgss_row['calls'] @total_time = pgss_row['total_time'] @max_time = pgss_row['max_time'] @mean_time = pgss_row['mean_time'] @rows = pgss_row['rows'] + @execution_context = execution_context end def formatted_query @@ -33,7 +35,9 @@ class Query end def concurrent? - query.downcase.include?('create index concurrently') + downcased = query.downcase + + downcased.include?('create index concurrently') || downcased.include?('drop index concurrently') end def new_table_fields_and_types @@ -56,7 +60,11 @@ class Query end def time_guidance - return CONCURRENT_QUERY_GUIDANCE_MILLISECONDS if concurrent? + if concurrent? + return CONCURRENT_POSTMIGRATE_QUERY_GUIDANCE_MILLISECONDS if execution_context&.post_deploy? + + return CONCURRENT_QUERY_GUIDANCE_MILLISECONDS + end QUERY_GUIDANCE_MILLISECONDS end @@ -74,9 +82,9 @@ class Query end def warning(migration_name) - "#{migration_name} had a query that [exceeded timing guidelines](#{TIMING_GUIDELINES}). Run time "\ - "should not exceed #{time_guidance}ms, but #{timing_phrase}. Please consider possible options to "\ - "improve the query performance.
#{formatted_query}
" + "#{migration_name} had a query that [exceeded timing guidelines](#{TIMING_GUIDELINES}). Run time " \ + "should not exceed #{time_guidance}ms, but #{timing_phrase}. Please consider possible options to " \ + "improve the query performance.
#{formatted_query}
" end def excluded? diff --git a/notifier/spec/migration_spec.rb b/notifier/spec/migration_spec.rb index 4697f29924a0414a3d698e3efbee99cbe29b1c19..b7772473b4e81c700029414eb0f7a90acfbe0aea 100644 --- a/notifier/spec/migration_spec.rb +++ b/notifier/spec/migration_spec.rb @@ -35,6 +35,17 @@ RSpec.describe Migration do } end + let(:concurrent_query) do + { + "query" => "create index concurrently tmp_index ON tmp_table (id)", + "calls" => 1, + "total_time" => 87.621755, + "max_time" => 87.621755, + "mean_time" => 87.621755, + "rows" => 1 + } + end + let(:queries) { good_queries << bad_query } let(:stats) do @@ -108,40 +119,76 @@ RSpec.describe Migration do end end - describe '#exceeds_time_guidance?' do - context 'with a regular migration' do + describe '#concurrent_only?' do + context 'when all important commands are concurrent' do + let(:queries) { [concurrent_query] } + + it { is_expected.to be_concurrent_only } + end + + context 'when at least one important commands is not concurrent' do + let(:queries) { [concurrent_query, bad_query] } + + it { is_expected.not_to be_concurrent_only } + end + end + + describe '#time_guidance' do + context 'with regular migration' do + it 'is 3 minutes' do + expect(subject.time_guidance).to eq 3.minutes + end + end + + context 'with post_deploy migration' do before do - migration_hash['type'] = described_class::TYPE_REGULAR + migration_hash['type'] = 'post_deploy' + end + + it 'is 10 minutes' do + expect(subject.time_guidance).to eq 10.minutes + end + + context 'with concurrent commands only' do + let(:queries) { [concurrent_query] } + + it 'is 20 minutes' do + expect(subject.time_guidance).to eq 20.minutes + end end + end + end - it 'returns false if under REGULAR_MIGRATION_GUIDANCE_SECONDS' do - stats['walltime'] = described_class::REGULAR_MIGRATION_GUIDANCE_SECONDS / 2 + describe '#exceeds_time_guidance?' do + shared_examples 'has time_guidance limit' do |limit_in_seconds| + it "is false if <= #{limit_in_seconds}" do + stats['walltime'] = limit_in_seconds - 1 expect(subject.exceeds_time_guidance?).to be false end - it 'returns true if over REGULAR_MIGRATION_GUIDANCE_SECONDS' do - stats['walltime'] = described_class::REGULAR_MIGRATION_GUIDANCE_SECONDS + 30 + it "is true if > #{limit_in_seconds}" do + stats['walltime'] = limit_in_seconds + 1 expect(subject.exceeds_time_guidance?).to be true end end + context 'with a regular migration' do + include_examples 'has time_guidance limit', described_class::REGULAR_MIGRATION_GUIDANCE_SECONDS + end + context 'with a post_deploy migration' do before do migration_hash['type'] = described_class::TYPE_POST_DEPLOY end - it 'returns false if under POST_DEPLOY_MIGRATION_GUIDANCE_SECONDS' do - stats['walltime'] = described_class::POST_DEPLOY_MIGRATION_GUIDANCE_SECONDS / 2 + include_examples 'has time_guidance limit', described_class::POST_DEPLOY_MIGRATION_GUIDANCE_SECONDS - expect(subject.exceeds_time_guidance?).to be false - end + context 'with concurrent commands only' do + let(:queries) { [concurrent_query] } - it 'returns true if over POST_DEPLOY_MIGRATION_GUIDANCE_SECONDS' do - stats['walltime'] = described_class::POST_DEPLOY_MIGRATION_GUIDANCE_SECONDS + 30 - - expect(subject.exceeds_time_guidance?).to be true + include_examples 'has time_guidance limit', described_class::POST_DEPLOY_CONCURRENT_MIGRATION_GUIDANCE_SECONDS end end end diff --git a/notifier/spec/query_spec.rb b/notifier/spec/query_spec.rb index 6efca2bf3a9bfb2f3a1b149159ff5aca7bde6afb..36f42db3d55d1448c53828fa3b5500468200c419 100644 --- a/notifier/spec/query_spec.rb +++ b/notifier/spec/query_spec.rb @@ -124,33 +124,86 @@ RSpec.describe Query do end end - describe '#exceeds_time_guidance?' do - it 'returns true if max time greater than QUERY_GUIDANCE_MILLISECONDS' do - subject.max_time = described_class::QUERY_GUIDANCE_MILLISECONDS * 2 + describe '#time_guidance' do + context 'with regular query' do + it 'is 100ms' do + expect(subject.time_guidance).to eq 100 + end + end + + context 'with concurrent query' do + subject(:query) { described_class.new(pgss, execution_context: migration) } + + before do + pgss['query'] = 'CREATE INDEX CONCURRENTLY index_ci_runners_on_token_lower ' \ + 'ON ci_runners (LOWER(token)) /*application:test*/' + end + + context 'with regular migration' do + let(:migration) { Migration.new({ 'type' => Migration::TYPE_REGULAR }, nil, []) } + + it 'is 5 minutes' do + expect(subject.time_guidance).to eq 5.minutes.in_milliseconds + end + end + + context 'with post_deploy migration' do + let(:migration) { Migration.new({ 'type' => Migration::TYPE_POST_DEPLOY }, nil, []) } - expect(subject.exceeds_time_guidance?).to be true + it 'is 20 minutes' do + expect(subject.time_guidance).to eq 20.minutes.in_milliseconds + end + end end + end + + describe '#exceeds_time_guidance?' do + shared_examples 'has time_guidance limit' do |limit_in_milliseconds| + it "is true if max time > #{limit_in_milliseconds}" do + subject.max_time = limit_in_milliseconds + 1 + + expect(subject.exceeds_time_guidance?).to be true + end - it 'returns false if concurrent operations are below 5 minutes' do - pgss['query'] = 'CREATE INDEX CONCURRENTLY index_ci_runners_on_token_lower '\ - 'ON ci_runners (LOWER(token)) /*application:test*/' + it "is false if max time <= #{limit_in_milliseconds}" do + subject.max_time = limit_in_milliseconds - 1 - expect(subject.exceeds_time_guidance?).to be false + expect(subject.exceeds_time_guidance?).to be false + end end - it 'returns false if max time less than QUERY_GUIDANCE_MILLISECONDS' do - subject.max_time = described_class::QUERY_GUIDANCE_MILLISECONDS / 2 + context 'with regular query' do + include_examples 'has time_guidance limit', described_class::QUERY_GUIDANCE_MILLISECONDS + end - expect(subject.exceeds_time_guidance?).to be false + context 'with concurrent query' do + subject(:query) { described_class.new(pgss, execution_context: migration) } + + before do + pgss['query'] = 'CREATE INDEX CONCURRENTLY index_ci_runners_on_token_lower ' \ + 'ON ci_runners (LOWER(token)) /*application:test*/' + end + + context 'with regular migration' do + let(:migration) { Migration.new({ 'type' => Migration::TYPE_REGULAR }, nil, []) } + + include_examples 'has time_guidance limit', described_class::CONCURRENT_QUERY_GUIDANCE_MILLISECONDS + end + + context 'with post_deploy migration' do + let(:migration) { Migration.new({ 'type' => Migration::TYPE_POST_DEPLOY }, nil, []) } + + include_examples 'has time_guidance limit', described_class::CONCURRENT_POSTMIGRATE_QUERY_GUIDANCE_MILLISECONDS + end end end context 'when query has special characters' do let(:pgss) do { - "query" => "CREATE INDEX CONCURRENTLY \"tmp_index_merge_requests_draft_and_status\""\ - " ON \"merge_requests\" (\"id\") WHERE draft = false AND state_id = 1 AND "\ - "((title)::text ~* '^\\[draft\\]|\\(draft\\)|draft:|draft|\\[WIP\\]|WIP:|WIP'::text) "\ + "query" => "CREATE INDEX CONCURRENTLY \"tmp_index_merge_requests_draft_and_status\"" \ + " ON \"merge_requests\" (\"id\") WHERE draft = false AND state_id = 1 AND " \ + "((title)::text ~* '^\\[draft\\]|\\(draft\\)|draft:|draft|\\[WIP\\]|WIP:|WIP'::text) " \ "/*application:test*/", "calls" => 1, "total_time" => 1824825.496259, @@ -164,4 +217,30 @@ RSpec.describe Query do expect(subject.formatted_query).not_to include('|') end end + + describe '#concurrent?' do + it 'is false for select query' do + pgss['query'] = 'SELECT * FROM my_table' + + expect(subject).not_to be_concurrent + end + + it 'is false for create index query' do + pgss['query'] = 'CREATE INDEX my_tmp ON my_table (id)' + + expect(subject).not_to be_concurrent + end + + it 'is true for concurrent create index query' do + pgss['query'] = 'CREATE INDEX CONCURRENTLY my_tmp ON my_table (id)' + + expect(subject).to be_concurrent + end + + it 'is true for concurrent drop index query' do + pgss['query'] = 'DROP INDEX CONCURRENTLY my_tmp' + + expect(subject).to be_concurrent + end + end end