diff options
39 files changed, 289 insertions, 72 deletions
@@ -37,7 +37,7 @@ gem 'view_component', '~> 2.74.1' gem 'default_value_for', '~> 3.4.0' # Supported DBs -gem 'pg', '~> 1.4.3' +gem 'pg', '~> 1.4.5' gem 'rugged', '~> 1.2' gem 'grape-path-helpers', '~> 1.7.1' diff --git a/Gemfile.checksum b/Gemfile.checksum index ac4290d6352..a4e09727a06 100644 --- a/Gemfile.checksum +++ b/Gemfile.checksum @@ -415,10 +415,10 @@ {"name":"parslet","version":"1.8.2","platform":"ruby","checksum":"08d1ab3721cd3f175bfbee8788b2ddff71f92038f2d69bd65454c22bb9fbd98a"}, {"name":"pastel","version":"0.8.0","platform":"ruby","checksum":"481da9fb7d2f6e6b1a08faf11fa10363172dc40fd47848f096ae21209f805a75"}, {"name":"peek","version":"1.1.0","platform":"ruby","checksum":"d6501ead8cde46d8d8ed0d59eb6f0ba713d0a41c11a2c4a81447b2dce37b3ecc"}, -{"name":"pg","version":"1.4.3","platform":"ruby","checksum":"ab0219cdd9e5750abb04b8bca5a5a490f60abdf37503027fd2f90d0c2d31f2fa"}, -{"name":"pg","version":"1.4.3","platform":"x64-mingw-ucrt","checksum":"9f4d1d39af5ae5eea9f3c6b1e3092cbd5d26b716ff0e1283cf71c0690c69b36c"}, -{"name":"pg","version":"1.4.3","platform":"x64-mingw32","checksum":"3265afd0e00331c7c70e50d4a13eea9083e5b683ebcd808bd671af70d92b189e"}, -{"name":"pg","version":"1.4.3","platform":"x86-mingw32","checksum":"08a6ef4c702e313c1a04ad6b088b1843361ca8606843c7cd607e181e0d4e5508"}, +{"name":"pg","version":"1.4.5","platform":"ruby","checksum":"c139bd34907e7bbe3291a9b5e651bcf00de1f8a99a3148c064bc2d1b10b5a6f1"}, +{"name":"pg","version":"1.4.5","platform":"x64-mingw-ucrt","checksum":"614814a4597fed5c4a85e107a96ef6c8ee01b3e7dbc6529912249b7d778e5651"}, +{"name":"pg","version":"1.4.5","platform":"x64-mingw32","checksum":"d9a15cb4ee478bf719fee6ecd6c8b41d5569515ee0d968e561fe120aed862cb1"}, +{"name":"pg","version":"1.4.5","platform":"x86-mingw32","checksum":"255764ff8ac89203cc9dcc7188a4205e760fa7b884d75c94007b79897ee8613d"}, {"name":"pg_query","version":"2.2.0","platform":"ruby","checksum":"84a37548412f540061bcc52ee2915352297832816bca60e4524c716e03f1e950"}, {"name":"plist","version":"3.6.0","platform":"ruby","checksum":"f468bcf6b72ec6d1585ed6744eb4817c1932a5bf91895ed056e69b7f12ca10f2"}, {"name":"png_quantizator","version":"0.2.1","platform":"ruby","checksum":"6023d4d064125c3a7e02929c95b7320ed6ac0d7341f9e8de0c9ea6576ef3106b"}, diff --git a/Gemfile.lock b/Gemfile.lock index b7a587c33dc..c60500be0ee 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1068,7 +1068,7 @@ GEM tty-color (~> 0.5) peek (1.1.0) railties (>= 4.0.0) - pg (1.4.3) + pg (1.4.5) pg_query (2.2.0) google-protobuf (>= 3.19.2) plist (3.6.0) @@ -1767,7 +1767,7 @@ DEPENDENCIES parallel (~> 1.19) parslet (~> 1.8) peek (~> 1.1) - pg (~> 1.4.3) + pg (~> 1.4.5) pg_query (~> 2.2) png_quantizator (~> 0.2.1) premailer-rails (~> 1.10.3) diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb index 83377f67723..7ea15c830f3 100644 --- a/app/controllers/projects/merge_requests/diffs_controller.rb +++ b/app/controllers/projects/merge_requests/diffs_controller.rb @@ -36,7 +36,10 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic diff_options_hash[:paths] = params[:paths] if params[:paths] diffs = @compare.diffs_in_batch(params[:page], params[:per_page], diff_options: diff_options_hash) - unfoldable_positions = @merge_request.note_positions_for_paths(diffs.diff_file_paths, current_user).unfoldable + + unfoldable_positions = Gitlab::Metrics.measure(:diffs_unfoldable_positions) do + @merge_request.note_positions_for_paths(diffs.diff_file_paths, current_user).unfoldable + end options = { merge_request: @merge_request, @@ -62,10 +65,17 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic return unless stale?(etag: [cache_context + diff_options_hash.fetch(:paths, []), diffs]) - diffs.unfold_diff_files(unfoldable_positions) - diffs.write_cache + Gitlab::Metrics.measure(:diffs_unfold) do + diffs.unfold_diff_files(unfoldable_positions) + end - render json: PaginatedDiffSerializer.new(current_user: current_user).represent(diffs, options) + Gitlab::Metrics.measure(:diffs_write_cache) do + diffs.write_cache + end + + Gitlab::Metrics.measure(:diffs_render) do + render json: PaginatedDiffSerializer.new(current_user: current_user).represent(diffs, options) + end end # rubocop: enable Metrics/AbcSize diff --git a/app/controllers/projects/ml/experiments_controller.rb b/app/controllers/projects/ml/experiments_controller.rb index c82a959d612..c0e4783602d 100644 --- a/app/controllers/projects/ml/experiments_controller.rb +++ b/app/controllers/projects/ml/experiments_controller.rb @@ -19,6 +19,7 @@ module Projects return redirect_to project_ml_experiments_path(@project) unless @experiment.present? @candidates = @experiment.candidates&.including_metrics_and_params + @candidates.each(&:artifact_lazy) end private diff --git a/app/controllers/registrations/welcome_controller.rb b/app/controllers/registrations/welcome_controller.rb index 4a42632a980..69cf1c42f34 100644 --- a/app/controllers/registrations/welcome_controller.rb +++ b/app/controllers/registrations/welcome_controller.rb @@ -92,7 +92,7 @@ module Registrations end # overridden in EE - def track_event(category) + def track_event(action) end end end diff --git a/app/helpers/projects/ml/experiments_helper.rb b/app/helpers/projects/ml/experiments_helper.rb index a67484e3d2f..93b7ff6590b 100644 --- a/app/helpers/projects/ml/experiments_helper.rb +++ b/app/helpers/projects/ml/experiments_helper.rb @@ -45,11 +45,11 @@ module Projects return unless artifact.present? - project_package_path(candidate.experiment.project, artifact) + project_package_path(candidate.project, artifact) end def link_to_details(candidate) - project_ml_candidate_path(candidate.experiment.project, candidate.iid) + project_ml_candidate_path(candidate.project, candidate.iid) end def link_to_experiment(candidate) diff --git a/app/models/chat_name.rb b/app/models/chat_name.rb index 60370c525d5..5996e15df4e 100644 --- a/app/models/chat_name.rb +++ b/app/models/chat_name.rb @@ -11,8 +11,7 @@ class ChatName < ApplicationRecord validates :team_id, presence: true validates :chat_id, presence: true - validates :user_id, uniqueness: { scope: [:integration_id] } - validates :chat_id, uniqueness: { scope: [:integration_id, :team_id] } + validates :chat_id, uniqueness: { scope: :team_id } # Updates the "last_used_timestamp" but only if it wasn't already updated # recently. diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index cff8911d84b..1395b8ff162 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -392,8 +392,13 @@ class MergeRequestDiff < ApplicationRecord def diffs_in_batch(batch_page, batch_size, diff_options:) fetching_repository_diffs(diff_options) do |comparison| - reorder_diff_files! - diffs_batch = diffs_in_batch_collection(batch_page, batch_size, diff_options: diff_options) + Gitlab::Metrics.measure(:diffs_reorder) do + reorder_diff_files! + end + + diffs_batch = Gitlab::Metrics.measure(:diffs_collection) do + diffs_in_batch_collection(batch_page, batch_size, diff_options: diff_options) + end if comparison if diff_options[:paths].blank? && !without_files? @@ -406,7 +411,9 @@ class MergeRequestDiff < ApplicationRecord ) end - comparison.diffs(diff_options) + Gitlab::Metrics.measure(:diffs_comparison) do + comparison.diffs(diff_options) + end else diffs_batch end diff --git a/app/models/ml/candidate.rb b/app/models/ml/candidate.rb index f24161d598f..b65037c6e2e 100644 --- a/app/models/ml/candidate.rb +++ b/app/models/ml/candidate.rb @@ -2,6 +2,8 @@ module Ml class Candidate < ApplicationRecord + PACKAGE_PREFIX = 'ml_candidate_' + enum status: { running: 0, scheduled: 1, finished: 2, failed: 3, killed: 4 } validates :iid, :experiment, presence: true @@ -18,18 +20,29 @@ module Ml scope :including_metrics_and_params, -> { includes(:latest_metrics, :params) } + delegate :project_id, :project, to: :experiment + def artifact_root "/#{package_name}/#{package_version}/" end def artifact - ::Packages::Generic::PackageFinder.new(experiment.project).execute!(package_name, package_version) - rescue ActiveRecord::RecordNotFound - nil + artifact_lazy&.itself + end + + def artifact_lazy + BatchLoader.for(id).batch do |candidate_ids, loader| + Packages::Package + .joins("INNER JOIN ml_candidates ON packages_packages.name=(concat('#{PACKAGE_PREFIX}', ml_candidates.id))") + .where(ml_candidates: { id: candidate_ids }) + .find_each do |package| + loader.call(package.name.delete_prefix(PACKAGE_PREFIX).to_i, package) + end + end end def package_name - "ml_candidate_#{iid}" + "#{PACKAGE_PREFIX}#{id}" end def package_version diff --git a/app/views/shared/admin/_admin_note.html.haml b/app/views/shared/admin/_admin_note.html.haml index 9dcf181a118..2bf6baaf608 100644 --- a/app/views/shared/admin/_admin_note.html.haml +++ b/app/views/shared/admin/_admin_note.html.haml @@ -1,4 +1,4 @@ -- if @group.admin_note.present? +- if @group.admin_note&.note? - text = @group.admin_note.note = render Pajamas::CardComponent.new(card_options: { class: 'gl-border-blue-500 gl-mb-5' }, header_options: { class: 'gl-bg-blue-500 gl-text-white' }) do |c| - c.header do diff --git a/db/post_migrate/20221226153240_remove_chat_names_integration_id_foreign_key.rb b/db/post_migrate/20221226153240_remove_chat_names_integration_id_foreign_key.rb new file mode 100644 index 00000000000..51baa9c9f57 --- /dev/null +++ b/db/post_migrate/20221226153240_remove_chat_names_integration_id_foreign_key.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class RemoveChatNamesIntegrationIdForeignKey < Gitlab::Database::Migration[2.1] + disable_ddl_transaction! + + SOURCE_TABLE_NAME = :chat_names + TARGET_TABLE_NAME = :integrations + COLUMN = :integration_id + TARGET_COLUMN = :id + FK_NAME = :fk_99a1348daf + + def up + with_lock_retries do + remove_foreign_key_if_exists(SOURCE_TABLE_NAME, name: FK_NAME) + end + end + + def down + add_concurrent_foreign_key( + SOURCE_TABLE_NAME, + TARGET_TABLE_NAME, + column: COLUMN, + name: FK_NAME, + on_delete: :cascade + ) + end +end diff --git a/db/post_migrate/20221226154458_drop_index_on_chat_names_on_integration_id_and_team_id_and_chat_id.rb b/db/post_migrate/20221226154458_drop_index_on_chat_names_on_integration_id_and_team_id_and_chat_id.rb new file mode 100644 index 00000000000..d5aeb704983 --- /dev/null +++ b/db/post_migrate/20221226154458_drop_index_on_chat_names_on_integration_id_and_team_id_and_chat_id.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class DropIndexOnChatNamesOnIntegrationIdAndTeamIdAndChatId < Gitlab::Database::Migration[2.1] + disable_ddl_transaction! + + INDEX_NAME = 'index_chat_names_on_integration_id_and_team_id_and_chat_id' + + def up + remove_concurrent_index_by_name(:chat_names, INDEX_NAME) + end + + def down + add_concurrent_index(:chat_names, [:integration_id, :team_id, :chat_id], name: INDEX_NAME, unique: true) + end +end diff --git a/db/post_migrate/20221227100751_add_user_index_to_chat_names.rb b/db/post_migrate/20221227100751_add_user_index_to_chat_names.rb new file mode 100644 index 00000000000..7be671b2cbd --- /dev/null +++ b/db/post_migrate/20221227100751_add_user_index_to_chat_names.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class AddUserIndexToChatNames < Gitlab::Database::Migration[2.1] + disable_ddl_transaction! + + INDEX_NAME = 'index_chat_names_on_user_id' + + def up + add_concurrent_index(:chat_names, :user_id, name: INDEX_NAME) + end + + def down + remove_concurrent_index_by_name(:chat_names, name: INDEX_NAME) + end +end diff --git a/db/post_migrate/20221227101436_drop_index_on_chat_names_on_user_id_and_integration_id.rb b/db/post_migrate/20221227101436_drop_index_on_chat_names_on_user_id_and_integration_id.rb new file mode 100644 index 00000000000..ab2842ea775 --- /dev/null +++ b/db/post_migrate/20221227101436_drop_index_on_chat_names_on_user_id_and_integration_id.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class DropIndexOnChatNamesOnUserIdAndIntegrationId < Gitlab::Database::Migration[2.1] + disable_ddl_transaction! + + INDEX_NAME = 'index_chat_names_on_user_id_and_integration_id' + + def up + remove_concurrent_index_by_name(:chat_names, INDEX_NAME) + end + + def down + add_concurrent_index(:chat_names, [:user_id, :integration_id], name: INDEX_NAME, unique: true) + end +end diff --git a/db/schema_migrations/20221226153240 b/db/schema_migrations/20221226153240 new file mode 100644 index 00000000000..3f2eb6afe64 --- /dev/null +++ b/db/schema_migrations/20221226153240 @@ -0,0 +1 @@ +0ed51d0f733ec6c94cc951e1e0b56c2095bf685dfa55d55cca63554e97ef509e
\ No newline at end of file diff --git a/db/schema_migrations/20221226154458 b/db/schema_migrations/20221226154458 new file mode 100644 index 00000000000..8aefafb2fc9 --- /dev/null +++ b/db/schema_migrations/20221226154458 @@ -0,0 +1 @@ +55910df54a9f6260145aac231af36dfbb5018899eebccfc7f80b51b9d48a7c67
\ No newline at end of file diff --git a/db/schema_migrations/20221227100751 b/db/schema_migrations/20221227100751 new file mode 100644 index 00000000000..62606a7b6d5 --- /dev/null +++ b/db/schema_migrations/20221227100751 @@ -0,0 +1 @@ +cc745f68d3719de09f5c0943711a013e84dcd229faaa59f5157e4f1bbcc6d736
\ No newline at end of file diff --git a/db/schema_migrations/20221227101436 b/db/schema_migrations/20221227101436 new file mode 100644 index 00000000000..04b8bd3a2e1 --- /dev/null +++ b/db/schema_migrations/20221227101436 @@ -0,0 +1 @@ +06a999af8600837d5cb29efdcb56a14eb456d89ea00b6f8974a57a34f4c4008d
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 82248f0cf5f..2f9c0f127aa 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -28664,11 +28664,9 @@ CREATE INDEX index_bulk_import_failures_on_correlation_id_value ON bulk_import_f CREATE INDEX index_bulk_imports_on_user_id ON bulk_imports USING btree (user_id); -CREATE UNIQUE INDEX index_chat_names_on_integration_id_and_team_id_and_chat_id ON chat_names USING btree (integration_id, team_id, chat_id); - CREATE INDEX index_chat_names_on_team_id_and_chat_id ON chat_names USING btree (team_id, chat_id); -CREATE UNIQUE INDEX index_chat_names_on_user_id_and_integration_id ON chat_names USING btree (user_id, integration_id); +CREATE INDEX index_chat_names_on_user_id ON chat_names USING btree (user_id); CREATE UNIQUE INDEX index_chat_teams_on_namespace_id ON chat_teams USING btree (namespace_id); @@ -33630,9 +33628,6 @@ ALTER TABLE ONLY vulnerability_occurrences ALTER TABLE ONLY protected_branch_merge_access_levels ADD CONSTRAINT fk_98f3d044fe FOREIGN KEY (group_id) REFERENCES namespaces(id) ON DELETE CASCADE; -ALTER TABLE ONLY chat_names - ADD CONSTRAINT fk_99a1348daf FOREIGN KEY (integration_id) REFERENCES integrations(id) ON DELETE CASCADE; - ALTER TABLE ONLY notes ADD CONSTRAINT fk_99e097b079 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; diff --git a/doc/administration/monitoring/prometheus/gitlab_metrics.md b/doc/administration/monitoring/prometheus/gitlab_metrics.md index 53ee028bc32..29182077d66 100644 --- a/doc/administration/monitoring/prometheus/gitlab_metrics.md +++ b/doc/administration/monitoring/prometheus/gitlab_metrics.md @@ -153,6 +153,14 @@ The following metrics are available: | `cached_object_operations_total` | Counter | 15.3 | Total number of objects cached for specific web requests | `controller`, `action` | | `redis_hit_miss_operations_total` | Counter | 15.6 | Total number of Redis cache hits and misses | `cache_hit`, `caller_id`, `cache_identifier`, `feature_category`, `backing_resource` | | `redis_cache_generation_duration_seconds` | Histogram | 15.6 | Time to generate Redis cache | `cache_hit`, `caller_id`, `cache_identifier`, `feature_category`, `backing_resource` | +| `gitlab_diffs_reorder_real_duration_seconds` | Histogram | 15.8 | Duration in seconds spend on reordering of diff files on diffs batch request | `controller`, `action` | +| `gitlab_diffs_collection_real_duration_seconds` | Histogram | 15.8 | Duration in seconds spent on querying merge request diff files on diffs batch request | `controller`, `action` | +| `gitlab_diffs_comparison_real_duration_seconds` | Histogram | 15.8 | Duration in seconds spent on getting comparison data on diffs batch request | `controller`, `action` | +| `gitlab_diffs_unfoldable_positions_real_duration_seconds` | Histogram | 15.8 | Duration in seconds spent on getting unfoldable note positions on diffs batch request | `controller`, `action` | +| `gitlab_diffs_unfold_real_duration_seconds` | Histogram | 15.8 | Duration in seconds spent on unfolding positions on diffs batch request | `controller`, `action` | +| `gitlab_diffs_write_cache_real_duration_seconds` | Histogram | 15.8 | Duration in seconds spent on caching highlighted lines and stats on diffs batch request | `controller`, `action` | +| `gitlab_diffs_highlight_cache_decorate_real_duration_seconds` | Histogram | 15.8 | Duration in seconds spent on setting highlighted lines from cache on diffs batch request | `controller`, `action` | +| `gitlab_diffs_render_real_duration_seconds` | Histogram | 15.8 | Duration in seconds spent on serializing and rendering diffs on diffs batch request | `controller`, `action` | ## Metrics controlled by a feature flag diff --git a/doc/development/deprecation_guidelines/img/deprecation_removal_process.png b/doc/development/deprecation_guidelines/img/deprecation_removal_process.png Binary files differindex 594e15861b0..e9e7933ae32 100644 --- a/doc/development/deprecation_guidelines/img/deprecation_removal_process.png +++ b/doc/development/deprecation_guidelines/img/deprecation_removal_process.png diff --git a/doc/development/deprecation_guidelines/index.md b/doc/development/deprecation_guidelines/index.md index be4a3369dcb..5fc77038b08 100644 --- a/doc/development/deprecation_guidelines/index.md +++ b/doc/development/deprecation_guidelines/index.md @@ -46,8 +46,6 @@ Most features should be deprecated and then removed. [semantic versioning policy](../../policy/maintenance.md). - Begins after removal date has passed. - - **Breaking change**: A "breaking change" is any change that requires users to make a corresponding change to their code, settings, or workflow. "Users" might be humans, API clients, or even code classes that "use" another class. Examples of breaking changes include: @@ -58,6 +56,8 @@ A "breaking change" is any change that requires users to make a corresponding ch A breaking change can be considered major if it affects many users, or represents a significant change in behavior. + + ## When can a feature be deprecated? Deprecations should be announced on the [Deprecated feature removal schedule](../../update/deprecations.md). diff --git a/lib/gitlab/diff/file_collection/merge_request_diff_base.rb b/lib/gitlab/diff/file_collection/merge_request_diff_base.rb index c6ab56e783a..801c1967e0a 100644 --- a/lib/gitlab/diff/file_collection/merge_request_diff_base.rb +++ b/lib/gitlab/diff/file_collection/merge_request_diff_base.rb @@ -23,7 +23,9 @@ module Gitlab strong_memoize(:diff_files) do diff_files = super - diff_files.each { |diff_file| highlight_cache.decorate(diff_file) } + Gitlab::Metrics.measure(:diffs_highlight_cache_decorate) do + diff_files.each { |diff_file| highlight_cache.decorate(diff_file) } + end diff_files end diff --git a/scripts/verify-tff-mapping b/scripts/verify-tff-mapping index 302e50bf34f..08d9d7a33fd 100755 --- a/scripts/verify-tff-mapping +++ b/scripts/verify-tff-mapping @@ -188,6 +188,20 @@ tests = [ explanation: 'GLFM spec and config files for CE and EE should map to respective markdown snapshot specs', source: 'glfm_specification/foo', expected: ['spec/requests/api/markdown_snapshot_spec.rb', 'ee/spec/requests/api/markdown_snapshot_spec.rb'] + }, + + { + explanation: 'https://gitlab.com/gitlab-org/quality/engineering-productivity/master-broken-incidents/-/issues/287#note_1192008962', + # Note: The metrics seem to be changed every year or so, so this test will fail once a year or so. + # You will need to change the metric below for another metric present in the project. + source: 'ee/config/metrics/counts_all/20221114065035_delete_merge_request.yml', + expected: ['ee/spec/config/metrics/every_metric_definition_spec.rb'] + }, + + { + explanation: 'https://gitlab.com/gitlab-org/quality/engineering-productivity/master-broken-incidents/-/issues/287#note_1192008962', + source: 'ee/lib/ee/gitlab/usage_data_counters/known_events/common.yml', + expected: ['ee/spec/config/metrics/every_metric_definition_spec.rb'] } ] diff --git a/spec/controllers/projects/merge_requests/diffs_controller_spec.rb b/spec/controllers/projects/merge_requests/diffs_controller_spec.rb index 613d82efd06..f4fd78730b6 100644 --- a/spec/controllers/projects/merge_requests/diffs_controller_spec.rb +++ b/spec/controllers/projects/merge_requests/diffs_controller_spec.rb @@ -430,6 +430,16 @@ RSpec.describe Projects::MergeRequests::DiffsController do expect(response).to have_gitlab_http_status(:ok) end + it 'measures certain parts of the request' do + allow(Gitlab::Metrics).to receive(:measure).and_call_original + expect(Gitlab::Metrics).to receive(:measure).with(:diffs_unfoldable_positions).and_call_original + expect(Gitlab::Metrics).to receive(:measure).with(:diffs_unfold).and_call_original + expect(Gitlab::Metrics).to receive(:measure).with(:diffs_write_cache).and_call_original + expect(Gitlab::Metrics).to receive(:measure).with(:diffs_render).and_call_original + + subject + end + it 'tracks mr_diffs event' do expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) .to receive(:track_mr_diffs_action) diff --git a/spec/db/schema_spec.rb b/spec/db/schema_spec.rb index 65d5da00087..e65f01a6bfb 100644 --- a/spec/db/schema_spec.rb +++ b/spec/db/schema_spec.rb @@ -30,7 +30,7 @@ RSpec.describe 'Database schema' do award_emoji: %w[awardable_id user_id], aws_roles: %w[role_external_id], boards: %w[milestone_id iteration_id], - chat_names: %w[chat_id team_id user_id], + chat_names: %w[chat_id team_id user_id integration_id], chat_teams: %w[team_id], ci_build_needs: %w[partition_id], ci_build_pending_states: %w[partition_id], diff --git a/spec/factories/ml/candidates.rb b/spec/factories/ml/candidates.rb index 2daed36d777..1b41e39d711 100644 --- a/spec/factories/ml/candidates.rb +++ b/spec/factories/ml/candidates.rb @@ -16,5 +16,14 @@ FactoryBot.define do candidate.metadata = FactoryBot.create_list(:ml_candidate_metadata, 2, candidate: candidate ) end end + + trait :with_artifact do + after(:create) do |candidate| + FactoryBot.create(:generic_package, + name: candidate.package_name, + version: candidate.package_version, + project: candidate.project) + end + end end end diff --git a/spec/features/admin/admin_groups_spec.rb b/spec/features/admin/admin_groups_spec.rb index c36a742af6b..c49ba995ca4 100644 --- a/spec/features/admin/admin_groups_spec.rb +++ b/spec/features/admin/admin_groups_spec.rb @@ -204,6 +204,17 @@ RSpec.describe 'Admin Groups', feature_category: :subgroups do expect(page).to have_content(new_admin_note_text) end + + it 'hides removed note' do + group = create(:group, :private) + group.create_admin_note(note: 'A note by an administrator') + + visit admin_group_edit_path(group) + fill_in 'group_admin_note_attributes_note', with: '' + click_button 'Save changes' + + expect(page).not_to have_content(s_('Admin|Admin notes')) + end end describe 'add user into a group', :js do diff --git a/spec/helpers/projects/ml/experiments_helper_spec.rb b/spec/helpers/projects/ml/experiments_helper_spec.rb index e6959a03c4a..9fafa43f99f 100644 --- a/spec/helpers/projects/ml/experiments_helper_spec.rb +++ b/spec/helpers/projects/ml/experiments_helper_spec.rb @@ -9,7 +9,7 @@ RSpec.describe Projects::Ml::ExperimentsHelper, feature_category: :mlops do let_it_be(:project) { create(:project, :private) } let_it_be(:experiment) { create(:ml_experiments, user_id: project.creator, project: project) } let_it_be(:candidate0) do - create(:ml_candidates, experiment: experiment, user: project.creator).tap do |c| + create(:ml_candidates, :with_artifact, experiment: experiment, user: project.creator).tap do |c| c.params.build([{ name: 'param1', value: 'p1' }, { name: 'param2', value: 'p2' }]) c.metrics.create!( [{ name: 'metric1', value: 0.1 }, { name: 'metric2', value: 0.2 }, { name: 'metric3', value: 0.3 }] @@ -32,7 +32,8 @@ RSpec.describe Projects::Ml::ExperimentsHelper, feature_category: :mlops do it 'creates the correct model for the table' do expected_value = [ { 'param1' => 'p1', 'param2' => 'p2', 'metric1' => '0.1000', 'metric2' => '0.2000', 'metric3' => '0.3000', - 'artifact' => nil, 'details' => "/#{project.full_path}/-/ml/candidates/#{candidate0.iid}" }, + 'artifact' => "/#{project.full_path}/-/packages/#{candidate0.artifact.id}", + 'details' => "/#{project.full_path}/-/ml/candidates/#{candidate0.iid}" }, { 'param2' => 'p3', 'param3' => 'p4', 'metric3' => '0.4000', 'artifact' => nil, 'details' => "/#{project.full_path}/-/ml/candidates/#{candidate1.iid}" } ] @@ -57,9 +58,6 @@ RSpec.describe Projects::Ml::ExperimentsHelper, feature_category: :mlops do describe '#candidate_as_data' do let(:candidate) { candidate0 } - let(:package) do - create(:generic_package, name: candidate.package_name, version: candidate.package_version, project: project) - end subject { Gitlab::Json.parse(helper.candidate_as_data(candidate)) } @@ -81,7 +79,7 @@ RSpec.describe Projects::Ml::ExperimentsHelper, feature_category: :mlops do it 'generates the correct info' do expected_info = { 'iid' => candidate.iid, - 'path_to_artifact' => "/#{project.full_path}/-/packages/#{package.id}", + 'path_to_artifact' => "/#{project.full_path}/-/packages/#{candidate.artifact.id}", 'experiment_name' => candidate.experiment.name, 'path_to_experiment' => "/#{project.full_path}/-/ml/experiments/#{experiment.iid}", 'status' => 'running' diff --git a/spec/lib/gitlab/diff/file_collection/merge_request_diff_base_spec.rb b/spec/lib/gitlab/diff/file_collection/merge_request_diff_base_spec.rb index 51bee6d45e4..861852d8f0b 100644 --- a/spec/lib/gitlab/diff/file_collection/merge_request_diff_base_spec.rb +++ b/spec/lib/gitlab/diff/file_collection/merge_request_diff_base_spec.rb @@ -26,6 +26,17 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBase do end end + describe '#diff_files' do + subject(:diff_files) { described_class.new(diffable, diff_options: nil).diff_files } + + it 'measures diffs_highlight_cache_decorate' do + allow(Gitlab::Metrics).to receive(:measure).and_call_original + expect(Gitlab::Metrics).to receive(:measure).with(:diffs_highlight_cache_decorate).and_call_original + + diff_files + end + end + describe '#cache_key' do subject(:cache_key) { described_class.new(diffable, diff_options: nil).cache_key } diff --git a/spec/models/chat_name_spec.rb b/spec/models/chat_name_spec.rb index 02c38479d1a..3bb5ee5870b 100644 --- a/spec/models/chat_name_spec.rb +++ b/spec/models/chat_name_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ChatName do +RSpec.describe ChatName, feature_category: :integrations do let_it_be(:chat_name) { create(:chat_name) } subject { chat_name } @@ -15,13 +15,12 @@ RSpec.describe ChatName do it { is_expected.to validate_presence_of(:team_id) } it { is_expected.to validate_presence_of(:chat_id) } - it { is_expected.to validate_uniqueness_of(:user_id).scoped_to(:integration_id) } - it { is_expected.to validate_uniqueness_of(:chat_id).scoped_to(:integration_id, :team_id) } + it { is_expected.to validate_uniqueness_of(:chat_id).scoped_to(:team_id) } - it 'is removed when the project is deleted' do - expect { subject.reload.integration.project.delete }.to change { ChatName.count }.by(-1) + it 'is not removed when the project is deleted' do + expect { subject.reload.integration.project.delete }.not_to change { ChatName.count } - expect(ChatName.where(id: subject.id)).not_to exist + expect(ChatName.where(id: subject.id)).to exist end describe '#update_last_used_at', :clean_gitlab_redis_shared_state do diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index a17b90930f0..e5c5d683c76 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -412,7 +412,19 @@ RSpec.describe MergeRequestDiff do describe '#diffs_in_batch' do let(:diff_options) { {} } + shared_examples_for 'measuring diffs metrics' do + specify do + allow(Gitlab::Metrics).to receive(:measure).and_call_original + expect(Gitlab::Metrics).to receive(:measure).with(:diffs_reorder).and_call_original + expect(Gitlab::Metrics).to receive(:measure).with(:diffs_collection).and_call_original + + diff_with_commits.diffs_in_batch(0, 10, diff_options: diff_options) + end + end + shared_examples_for 'fetching full diffs' do + it_behaves_like 'measuring diffs metrics' + it 'returns diffs from repository comparison' do expect_next_instance_of(Compare) do |comparison| expect(comparison).to receive(:diffs) @@ -435,6 +447,13 @@ RSpec.describe MergeRequestDiff do expect(diffs.pagination_data).to eq(total_pages: nil) end + + it 'measures diffs_comparison' do + allow(Gitlab::Metrics).to receive(:measure).and_call_original + expect(Gitlab::Metrics).to receive(:measure).with(:diffs_comparison).and_call_original + + diff_with_commits.diffs_in_batch(1, 10, diff_options: diff_options) + end end context 'when no persisted files available' do @@ -454,6 +473,8 @@ RSpec.describe MergeRequestDiff do end context 'when persisted files available' do + it_behaves_like 'measuring diffs metrics' + it 'returns paginated diffs' do diffs = diff_with_commits.diffs_in_batch(0, 10, diff_options: diff_options) diff --git a/spec/models/ml/candidate_spec.rb b/spec/models/ml/candidate_spec.rb index 9ce411191f0..85f2440f2d7 100644 --- a/spec/models/ml/candidate_spec.rb +++ b/spec/models/ml/candidate_spec.rb @@ -4,6 +4,14 @@ require 'spec_helper' RSpec.describe Ml::Candidate, factory_default: :keep do let_it_be(:candidate) { create(:ml_candidates, :with_metrics_and_params) } + let_it_be(:candidate2) { create(:ml_candidates, experiment: candidate.experiment) } + + let_it_be(:candidate_artifact) do + FactoryBot.create(:generic_package, + name: candidate.package_name, + version: candidate.package_version, + project: candidate.project) + end let(:project) { candidate.experiment.project } @@ -22,13 +30,13 @@ RSpec.describe Ml::Candidate, factory_default: :keep do describe '.artifact_root' do subject { candidate.artifact_root } - it { is_expected.to eq("/ml_candidate_#{candidate.iid}/-/") } + it { is_expected.to eq("/ml_candidate_#{candidate.id}/-/") } end describe '.package_name' do subject { candidate.package_name } - it { is_expected.to eq("ml_candidate_#{candidate.iid}") } + it { is_expected.to eq("ml_candidate_#{candidate.id}") } end describe '.package_version' do @@ -38,27 +46,45 @@ RSpec.describe Ml::Candidate, factory_default: :keep do end describe '.artifact' do - subject { candidate.artifact } + let(:tested_candidate) { candidate } - context 'when has logged artifacts' do - let(:package) do - create(:generic_package, name: candidate.package_name, version: candidate.package_version, project: project) - end + subject { tested_candidate.artifact } - it 'returns the package' do - package + before do + candidate_artifact + end - is_expected.to eq(package) + context 'when has logged artifacts' do + it 'returns the package' do + expect(subject.name).to eq(tested_candidate.package_name) end end context 'when does not have logged artifacts' do - let(:tested_candidate) { create(:ml_candidates, :with_metrics_and_params) } + let(:tested_candidate) { candidate2 } it { is_expected.to be_nil } end end + describe '.artifact_lazy' do + context 'when candidates have same the same iid' do + before do + BatchLoader::Executor.clear_current + end + + it 'loads the correct artifacts', :aggregate_failures do + candidate.artifact_lazy + candidate2.artifact_lazy + + expect(Packages::Package).to receive(:joins).once.and_call_original # Only one database call + + expect(candidate.artifact.name).to eq(candidate.package_name) + expect(candidate2.artifact).to be_nil + end + end + end + describe '#by_project_id_and_iid' do let(:project_id) { candidate.experiment.project_id } let(:iid) { candidate.iid } diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 0c513078783..c54123ce9f7 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -7022,8 +7022,8 @@ RSpec.describe Project, factory_default: :keep do create_list(:chat_name, 5, integration: integration) end - it 'removes chat names on removal' do - expect { subject.destroy! }.to change { ChatName.count }.by(-5) + it 'does not remove chat names on removal' do + expect { subject.destroy! }.not_to change { ChatName.count } end end diff --git a/spec/requests/api/ml/mlflow_spec.rb b/spec/requests/api/ml/mlflow_spec.rb index f9aac7149e0..fdf115f7e92 100644 --- a/spec/requests/api/ml/mlflow_spec.rb +++ b/spec/requests/api/ml/mlflow_spec.rb @@ -409,7 +409,7 @@ RSpec.describe API::Ml::Mlflow, feature_category: :mlops do 'experiment_id' => candidate.experiment.iid.to_s, 'user_id' => candidate.user.id.to_s, 'start_time' => candidate.start_time, - 'artifact_uri' => "http://www.example.com/api/v4/projects/#{project_id}/packages/generic/ml_candidate_#{candidate.iid}/-/", + 'artifact_uri' => "http://www.example.com/api/v4/projects/#{project_id}/packages/generic/ml_candidate_#{candidate.id}/-/", 'status' => "RUNNING", 'lifecycle_stage' => "active" } @@ -428,8 +428,8 @@ RSpec.describe API::Ml::Mlflow, feature_category: :mlops do { 'key' => candidate.params[1].name, 'value' => candidate.params[1].value } ], 'tags' => [ - { 'key' => 'metadata_1', 'value' => 'value1' }, - { 'key' => 'metadata_2', 'value' => 'value2' } + { 'key' => candidate.metadata[0].name, 'value' => candidate.metadata[0].value }, + { 'key' => candidate.metadata[1].name, 'value' => candidate.metadata[1].value } ] }) end @@ -452,7 +452,7 @@ RSpec.describe API::Ml::Mlflow, feature_category: :mlops do 'user_id' => candidate.user.id.to_s, 'start_time' => candidate.start_time, 'end_time' => params[:end_time], - 'artifact_uri' => "http://www.example.com/api/v4/projects/#{project_id}/packages/generic/ml_candidate_#{candidate.iid}/-/", + 'artifact_uri' => "http://www.example.com/api/v4/projects/#{project_id}/packages/generic/ml_candidate_#{candidate.id}/-/", 'status' => 'FAILED', 'lifecycle_stage' => 'active' } diff --git a/spec/requests/projects/ml/candidates_controller_spec.rb b/spec/requests/projects/ml/candidates_controller_spec.rb index 4a0fd1ce4f5..d3f9d92bc44 100644 --- a/spec/requests/projects/ml/candidates_controller_spec.rb +++ b/spec/requests/projects/ml/candidates_controller_spec.rb @@ -9,7 +9,6 @@ RSpec.describe Projects::Ml::CandidatesController, feature_category: :mlops do let_it_be(:candidate) { create(:ml_candidates, experiment: experiment, user: user) } let(:ff_value) { true } - let(:threshold) { 4 } let(:candidate_iid) { candidate.iid } before do @@ -40,14 +39,13 @@ RSpec.describe Projects::Ml::CandidatesController, feature_category: :mlops do expect(response).to render_template('projects/ml/candidates/show') end - # MR removing this xit https://gitlab.com/gitlab-org/gitlab/-/merge_requests/104166 - xit 'does not perform N+1 sql queries' do - control_count = ActiveRecord::QueryRecorder.new { show_candidate } + it 'does not perform N+1 sql queries' do + control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { show_candidate } create_list(:ml_candidate_params, 3, candidate: candidate) create_list(:ml_candidate_metrics, 3, candidate: candidate) - expect { show_candidate }.not_to exceed_all_query_limit(control_count).with_threshold(threshold) + expect { show_candidate }.not_to exceed_all_query_limit(control_count) end context 'when candidate does not exist' do diff --git a/spec/requests/projects/ml/experiments_controller_spec.rb b/spec/requests/projects/ml/experiments_controller_spec.rb index f35f93b1e6c..b336ec60249 100644 --- a/spec/requests/projects/ml/experiments_controller_spec.rb +++ b/spec/requests/projects/ml/experiments_controller_spec.rb @@ -76,13 +76,12 @@ RSpec.describe Projects::Ml::ExperimentsController, feature_category: :mlops do expect(response).to render_template('projects/ml/experiments/show') end - # MR removing this xit https://gitlab.com/gitlab-org/gitlab/-/merge_requests/104166 - xit 'does not perform N+1 sql queries' do - control_count = ActiveRecord::QueryRecorder.new { show_experiment } + it 'does not perform N+1 sql queries' do + control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { show_experiment } create_list(:ml_candidates, 2, :with_metrics_and_params, experiment: experiment) - expect { show_experiment }.not_to exceed_all_query_limit(control_count).with_threshold(threshold) + expect { show_experiment }.not_to exceed_all_query_limit(control_count) end it_behaves_like '404 if feature flag disabled' diff --git a/tests.yml b/tests.yml index be6cb2c84c7..4526c9cf1f5 100644 --- a/tests.yml +++ b/tests.yml @@ -92,3 +92,13 @@ mapping: test: spec/requests/api/markdown_snapshot_spec.rb - source: glfm_specification/.+ test: ee/spec/requests/api/markdown_snapshot_spec.rb + + # Any change to metrics definition should trigger the specs in the ee/spec/config/metrics/ folder. + # + # Note: We only have those tests for ee, even though we have non-ee metrics. + # + # See https://gitlab.com/gitlab-org/quality/engineering-productivity/master-broken-incidents/-/issues/287#note_1192008962 + - source: ee/config/metrics/.*.yml + test: ee/spec/config/metrics/every_metric_definition_spec.rb + - source: ee/lib/ee/gitlab/usage_data_counters/known_events/.*.yml + test: ee/spec/config/metrics/every_metric_definition_spec.rb |
