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