diff --git a/notifier/migration.rb b/notifier/migration.rb index 32ae3fff874068a4be8ab54d45f4cd23cdbaa8c4..94ee04ce7ee59c061d140a81742212e8716a19e2 100644 --- a/notifier/migration.rb +++ b/notifier/migration.rb @@ -1,11 +1,14 @@ # frozen_string_literal: true class Migration - REGULAR_MIGRATION_GUIDANCE_SECONDS = (3 * 60) - POST_DEPLOY_MIGRATION_GUIDANCE_SECONDS = (10 * 60) + REGULAR_MIGRATION_GUIDANCE_SECONDS = 3.minutes.freeze + POST_DEPLOY_MIGRATION_GUIDANCE_SECONDS = 10.minutes.freeze + SRE_NOTIFICATION_GUIDANCE = 20.minutes.freeze TYPE_REGULAR = 'regular' TYPE_POST_DEPLOY = 'post_deploy' - TIMING_GUIDELINES = "https://docs.gitlab.com/ee/development/database_review.html#timing-guidelines-for-migrations" + TIMING_GUIDELINES = 'https://docs.gitlab.com/ee/development/database_review.html#timing-guidelines-for-migrations' + POST_DEPLOY_MIGRATION_GUIDE = 'https://docs.gitlab.com/ee/development/post_deployment_migrations.html' + BACKGROUND_MIGRATION_GUIDE = 'https://docs.gitlab.com/ee/development/background_migrations.html' attr_accessor :version, :path, :name, :statistics, :total_database_size_change, :queries, :type, :walltime, :intro_on_current_branch, :success, @@ -40,6 +43,10 @@ class Migration raise 'Unknown migration type' end + def sre_should_be_informed? + walltime > SRE_NOTIFICATION_GUIDANCE + end + def exceeds_time_guidance? walltime > time_guidance end @@ -60,19 +67,41 @@ class Migration "#{version} - #{name}" end + def walltime_minutes + "#{walltime.in_minutes.round(2)}min" + end + + def time_guidance_minutes + "#{time_guidance.in_minutes.round(2)}min" + end + def warnings warnings = queries_with_warnings.map { |q| q.warning(name_formatted) } warnings << "#{name_formatted} did not complete successfully, check the job log for details" unless success? - if exceeds_time_guidance? - warnings << "#{name_formatted} [exceeded timing guidelines](#{TIMING_GUIDELINES})."\ - "This migration should not exceed #{time_guidance}s, but was #{walltime}s" + if sre_should_be_informed? + warnings << "#{name_formatted} took #{walltime_minutes}. Please add a comment that mentions Release "\ + "Managers (`@gitlab-org/release/managers`) so they are informed." end + warnings << time_remedy if exceeds_time_guidance? + warnings end + def time_remedy + if type == TYPE_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}" + 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}" + end + end + def warning? !success? || exceeds_time_guidance? || has_queries_with_warnings? end @@ -85,7 +114,7 @@ class Migration @was_run = true @total_database_size_change = stats['total_database_size_change'] - @walltime = stats['walltime'] + @walltime = stats['walltime'].seconds @success = stats['success'] @statistics = stats init_queries(stats) diff --git a/notifier/query.rb b/notifier/query.rb index c9126a52040ea995ab9a259ab16c586a6f7f1731..0f12e5c3acffc79a36231d3904a56d34d252749f 100644 --- a/notifier/query.rb +++ b/notifier/query.rb @@ -5,7 +5,7 @@ require_relative 'query_execution' class Query QUERY_GUIDANCE_MILLISECONDS = 100 - CONCURRENT_QUERY_GUIDANCE_MILLISECONDS = 5 * 60 * 1000 + CONCURRENT_QUERY_GUIDANCE_MILLISECONDS = 5.minutes.in_milliseconds TIMING_GUIDELINES = 'https://docs.gitlab.com/ee/development/query_performance.html#timing-guidelines-for-queries' # rubocop:disable Layout/LineLength @@ -37,7 +37,6 @@ class Query def formatted_query Niceql::Prettifier.prettify_sql(query) - .gsub(' ', ' ') .gsub('/*', '/*') .gsub('*/', '*/') .gsub("\n", '
') @@ -62,15 +61,16 @@ class Query def timing if calls == 1 - "it was #{max_time}" + "it was #{max_time.truncate(2)}" else - "the longest was #{max_time}ms, and the average was #{mean_time}ms" + "the longest was #{max_time.truncate(2)}ms, and the average was #{mean_time.truncate(2)}ms" end 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} 
#{formatted_query}
" + "should not exceed #{time_guidance}ms, but was #{timing}ms. Please consider possible options to "\ + "improve the query performance. 
#{formatted_query}
" end def excluded? diff --git a/notifier/spec/fixtures/migration-testing/expected-comment.txt b/notifier/spec/fixtures/migration-testing/expected-comment.txt index 350c9912415a9cd94de46252972eb461dd1552fe..297daee3a77b2430eb8018e0e8adebdaf827afaf 100644 --- a/notifier/spec/fixtures/migration-testing/expected-comment.txt +++ b/notifier/spec/fixtures/migration-testing/expected-comment.txt @@ -1,15 +1,20 @@ ### Database migrations -| | 1 Warnings | +| | 4 Warnings | | --------- | -------------------- | -| :warning: | 20210604233157 - MigrationThrowsException did not complete successfully, check the job log for details | +| :warning: | 20200325104758 - AddTestMigrationThatExceedsTimingForRegularButNotPost may need a
[post-deploy migration](https://docs.gitlab.com/ee/development/post_deployment_migrations.html) to comply with [timing guidelines](https://docs.gitlab.com/ee/development/database_review.html#timing-guidelines-for-migrations). It took 5.08min, but should not exceed 3.0min | +| :warning: | 20201015154529 - AddMigrationThatRequiresBackground took 25.02min. Please add a comment that
mentions Release Managers (`@gitlab-org/release/managers`) so they are informed. | +| :warning: | 20201015154529 - AddMigrationThatRequiresBackground may need a [background migration](https://docs.gitlab.com/ee/development/background_migrations.html) to
comply with [timing guidelines](https://docs.gitlab.com/ee/development/database_review.html#timing-guidelines-for-migrations). It took 25.02min, but should not exceed 10.0min | +| :warning: | 20210604233157 - MigrationThrowsException did not complete successfully, check the job log
for details | Migrations included in this change have been executed on gitlab.com data for testing purposes. For details, please see the [migration testing pipeline](https://gitlab.com/gitlab-org/database-team/gitlab-com-database-testing/-/pipelines/4711) (limited access). | Migration | Type | Total runtime | Result | DB size change | | --------- | ---- | ------------- | ------ | -------------- | +| 20200325104758 - AddTestMigrationThatExceedsTimingForRegularButNotPost | Regular | 306.0 s | :warning: | +32.00 KiB | | 20210602144718 - CreateTestTable | Regular | 1.8 s | :white_check_mark: | +40.00 KiB | +| 20201015154529 - AddMigrationThatRequiresBackground | Post deploy | 1502.0 s | :warning: | +32.00 KiB | | 20210604232017 - DropTestTable | Post deploy | 1.2 s | :white_check_mark: | -24.00 KiB | | 20210604233157 - MigrationThrowsException | Post deploy | 0.9 s | :boom: | +0.00 B | @@ -27,6 +32,13 @@ Migrations included in this change have been executed on gitlab.com data for tes +#### :warning: Migration: 20200325104758 - AddTestMigrationThatExceedsTimingForRegularButNotPost + +* Type: Regular +* Duration: 306.0 s +* Database size change: +32.00 KiB + + #### Migration: 20210602144718 - CreateTestTable * Type: Regular @@ -35,9 +47,9 @@ Migrations included in this change have been executed on gitlab.com data for tes | Query | Calls | Total Time | Max Time | Mean Time | Rows | | ----- | ----- | ---------- | -------- | --------- | ---- | -| 
CREATE TABLE "test_tables" ("id" bigserial primary key, "stars" bigint DEFAULT 0 NOT NULL, "created_at" timestamp(6) NOT NULL, "updated_at" timestamp(6) NOT NULL, "title" text, "notes" text) /*application:test*/
| 1 | 71.1 ms | 71.1 ms | 71.1 ms | 0 | -| 
ALTER TABLE "test_tables" ADD CONSTRAINT "check_0770ba173a" CHECK (char_length("title") <= 128), ADD CONSTRAINT "check_9cfc473dbc" CHECK (char_length("notes") <= 1024)
/*application:test*/
| 1 | 6.4 ms | 6.4 ms | 6.4 ms | 0 | -| 
SELECT $1::regtype::oid
| 1 | 0.0 ms | 0.0 ms | 0.0 ms | 1 | +| 
CREATE TABLE "test_tables" ("id" bigserial primary key, "stars" bigint DEFAULT 0 NOT NULL, "created_at" timestamp(6) NOT NULL, "updated_at" timestamp(6) NOT NULL, "title" text, "notes" text) /*application:test*/
| 1 | 71.1 ms | 71.1 ms | 71.1 ms | 0 | +| 
ALTER TABLE "test_tables" ADD CONSTRAINT "check_0770ba173a" CHECK (char_length("title") <= 128), ADD CONSTRAINT "check_9cfc473dbc" CHECK (char_length("notes") <= 1024)
/*application:test*/
| 1 | 6.4 ms | 6.4 ms | 6.4 ms | 0 | +| 
SELECT $1::regtype::oid
| 1 | 0.0 ms | 0.0 ms | 0.0 ms | 1 |
Histogram for CreateTestTable @@ -52,6 +64,13 @@ Migrations included in this change have been executed on gitlab.com data for tes
+#### :warning: Migration: 20201015154529 - AddMigrationThatRequiresBackground + +* Type: Post deploy +* Duration: 1502.0 s +* Database size change: +32.00 KiB + + #### Migration: 20210604232017 - DropTestTable * Type: Post deploy @@ -60,7 +79,7 @@ Migrations included in this change have been executed on gitlab.com data for tes | Query | Calls | Total Time | Max Time | Mean Time | Rows | | ----- | ----- | ---------- | -------- | --------- | ---- | -| 
DROP TABLE "test_tables" /*application:test*/
| 1 | 5.7 ms | 5.7 ms | 5.7 ms | 0 | +| 
DROP TABLE "test_tables" /*application:test*/
| 1 | 5.7 ms | 5.7 ms | 5.7 ms | 0 |
Histogram for DropTestTable @@ -116,12 +135,17 @@ Migrations included in this change have been executed on gitlab.com data for tes --- Brought to you by [gitlab-org/database-team/gitlab-com-database-testing](https://gitlab.com/gitlab-org/database-team/gitlab-com-database-testing). [Epic](https://gitlab.com/groups/gitlab-org/database-team/-/epics/9) - \ No newline at end of file diff --git a/notifier/spec/fixtures/migration-testing/migration-stats.json b/notifier/spec/fixtures/migration-testing/migration-stats.json index ae821b4d43d4d7479907bec1820687841448a2a9..36d531a3e8ef13ea104f34cdb7f7b6b0c05d8f74 100644 --- a/notifier/spec/fixtures/migration-testing/migration-stats.json +++ b/notifier/spec/fixtures/migration-testing/migration-stats.json @@ -263,6 +263,20 @@ } ] }, + { + "migration": 20200325104758, + "walltime": 305.9509822260588408, + "success": true, + "total_database_size_change": 32768, + "query_statistics": [] + }, + { + "migration": 20201015154529, + "walltime": 1501.9509822260588408, + "success": true, + "total_database_size_change": 32768, + "query_statistics": [] + }, { "migration": 20210604232017, "walltime": 1.1652476582676172, diff --git a/notifier/spec/fixtures/migration-testing/migrations.json b/notifier/spec/fixtures/migration-testing/migrations.json index b04a25c2f17af1c04a66c9dba1bf78e2d161a9d8..a3bb79cca296adeddf58e1cdae2be16fe27982a4 100644 --- a/notifier/spec/fixtures/migration-testing/migrations.json +++ b/notifier/spec/fixtures/migration-testing/migrations.json @@ -1,4 +1,25 @@ { + "20200323011225": { + "version": 20200323011225, + "path": "db/post_migrate/20200323011225_complete_migrate_security_scans.rb", + "name": "CompleteMigrateSecurityScans", + "type": "post_deploy", + "intro_on_current_branch": false + }, + "20201015154527": { + "version": 20201015154527, + "path": "db/post_migrate/20201015154527_add_index_on_services_for_usage_data.rb", + "name": "AddIndexOnServicesForUsageData", + "type": "post_deploy", + "intro_on_current_branch": false + }, + "20201015154529": { + "version": 20201015154529, + "path": "db/post_migrate/20201015154529_add_migration_that_requires_background.rb", + "name": "AddMigrationThatRequiresBackground", + "type": "post_deploy", + "intro_on_current_branch": true + }, "20210604232017": { "version": 20210604232017, "path": "db/post_migrate/20210604232017_drop_test_table.rb", @@ -13,6 +34,34 @@ "type": "post_deploy", "intro_on_current_branch": true }, + "20200325104755": { + "version": 20200325104755, + "path": "db/migrate/20200325104755_add_push_rules_id_to_project_settings.rb", + "name": "AddPushRulesIdToProjectSettings", + "type": "regular", + "intro_on_current_branch": false + }, + "20200325104758": { + "version": 20200325104758, + "path": "db/migrate/20200325104758_add_migration_that_exceeds_timing_for_regular_but_not_post.rb", + "name": "AddTestMigrationThatExceedsTimingForRegularButNotPost", + "type": "regular", + "intro_on_current_branch": true + }, + "20200911120132": { + "version": 20200911120132, + "path": "db/migrate/20200911120132_create_pages_deployments.rb", + "name": "CreatePagesDeployments", + "type": "regular", + "intro_on_current_branch": false + }, + "20200415161206": { + "version": 20200415161206, + "path": "db/migrate/20200415161206_remove_not_null_uploads_constraint.rb", + "name": "RemoveNotNullUploadsConstraint", + "type": "regular", + "intro_on_current_branch": false + }, "20210602144718": { "version": 20210602144718, "path": "db/migrate/20210602144718_create_test_table.rb", diff --git a/notifier/spec/warnings_spec.rb b/notifier/spec/warnings_spec.rb index 4060077457270d2c0767af0223b4bb39b26bb2de..78606b3cf50e03405deb5a8fe41247ace497986e 100644 --- a/notifier/spec/warnings_spec.rb +++ b/notifier/spec/warnings_spec.rb @@ -14,8 +14,8 @@ RSpec.describe Warnings do describe '#render' do it 'renders a few things' do - allow(migration).to receive(:exceeds_time_guidance?).and_return(true) allow(migration).to receive(:success?).and_return(false) + allow(migration).to receive(:walltime).and_return(Migration::SRE_NOTIFICATION_GUIDANCE * 2) allow(migration).to receive(:queries_with_warnings).and_return( [Query.new({ "query" => "select * from users limit 1000 /*application:test*/", @@ -27,9 +27,10 @@ RSpec.describe Warnings do })]) expect(subject.render).to include('did not complete') - expect(subject.render).to include('This migration should not exceed') - expect(subject.render).to include('should not exceed 100ms') - expect(subject.render).to include('222.49203') + expect(subject.render).to include('[background migration]') + expect(subject.render).to include('[post-deploy migration]') + expect(subject.render).to include('(`@gitlab-org/release/managers`)') + expect(subject.render).to include('222.49') end it 'excludes migrations not introduced on current branch' do diff --git a/notifier/templates/warnings.erb b/notifier/templates/warnings.erb index a63b7012f7340a2a2a8a5928213c27647c8c8df6..b0d7004ebaa746ed5927b9197f68542f50ca0e09 100644 --- a/notifier/templates/warnings.erb +++ b/notifier/templates/warnings.erb @@ -1,5 +1,5 @@ | | <%= count %> Warnings | | --------- | -------------------- | -% all.each do |warning| +% with_line_breaks.each do |warning| | :warning: | <%= warning %> | % end diff --git a/notifier/warnings.rb b/notifier/warnings.rb index 8e2b9484f22ab980f518c9aa5b509c937c71419d..a44421e68a6a629f29e367886da82b711261d332 100644 --- a/notifier/warnings.rb +++ b/notifier/warnings.rb @@ -15,6 +15,18 @@ class Warnings @all.any? end + def with_line_breaks + all.map do |warning| + warning.split(' ').reduce('') do |final, word| + if word.sub(/]\(.*\)/, '').length + (final.split('
').last&.sub(/]\(.*\)/, '')&.length || 0) > 100 + final = "#{final}
" + end + + "#{final} #{word}" + end + end + end + def render return '' unless any?