diff options
| -rw-r--r-- | changelogs/unreleased/bjk-48176_ruby_gc.yml | 5 | ||||
| -rw-r--r-- | doc/administration/monitoring/prometheus/gitlab_metrics.md | 14 | ||||
| -rw-r--r-- | doc/api/runners.md | 12 | ||||
| -rw-r--r-- | doc/development/utilities.md | 41 | ||||
| -rw-r--r-- | doc/topics/autodevops/index.md | 12 | ||||
| -rw-r--r-- | doc/user/project/merge_requests/squash_and_merge.md | 6 | ||||
| -rw-r--r-- | lib/gitlab/cache/request_cache.rb | 37 | ||||
| -rw-r--r-- | lib/gitlab/git/repository.rb | 51 | ||||
| -rw-r--r-- | lib/gitlab/metrics/samplers/ruby_sampler.rb | 26 | ||||
| -rw-r--r-- | rubocop/cop/migration/update_large_table.rb | 15 | ||||
| -rw-r--r-- | spec/features/issues/filtered_search/filter_issues_spec.rb | 36 | ||||
| -rw-r--r-- | spec/lib/gitlab/metrics/samplers/ruby_sampler_spec.rb | 2 | ||||
| -rw-r--r-- | spec/rubocop/cop/migration/update_large_table_spec.rb | 20 |
13 files changed, 166 insertions, 111 deletions
diff --git a/changelogs/unreleased/bjk-48176_ruby_gc.yml b/changelogs/unreleased/bjk-48176_ruby_gc.yml new file mode 100644 index 00000000000..45c6338df81 --- /dev/null +++ b/changelogs/unreleased/bjk-48176_ruby_gc.yml @@ -0,0 +1,5 @@ +--- +title: Cleanup Prometheus ruby metrics +merge_request: 20039 +author: Ben Kochie +type: fixed diff --git a/doc/administration/monitoring/prometheus/gitlab_metrics.md b/doc/administration/monitoring/prometheus/gitlab_metrics.md index 411a0fae93f..cea6764df41 100644 --- a/doc/administration/monitoring/prometheus/gitlab_metrics.md +++ b/doc/administration/monitoring/prometheus/gitlab_metrics.md @@ -49,6 +49,20 @@ The following metrics are available: | filesystem_circuitbreaker | Gauge | 9.5 | Whether or not the circuit for a certain shard is broken or not | | circuitbreaker_storage_check_duration_seconds | Histogram | 10.3 | Time a single storage probe took | +### Ruby metrics + +Some basic Ruby runtime metrics are available: + +| Metric | Type | Since | Description | +|:-------------------------------------- |:--------- |:----- |:----------- | +| ruby_gc_duration_seconds_total | Counter | 11.1 | Time spent by Ruby in GC | +| ruby_gc_stat_... | Gauge | 11.1 | Various metrics from [GC.stat] | +| ruby_file_descriptors | Gauge | 11.1 | File descriptors per process | +| ruby_memory_bytes | Gauge | 11.1 | Memory usage by process | +| ruby_sampler_duration_seconds_total | Counter | 11.1 | Time spent collecting stats | + +[GC.stat]: https://ruby-doc.org/core-2.3.0/GC.html#method-c-stat + ## Metrics shared directory GitLab's Prometheus client requires a directory to store metrics data shared between multi-process services. diff --git a/doc/api/runners.md b/doc/api/runners.md index 3ca07ce9795..ac814bbf19a 100644 --- a/doc/api/runners.md +++ b/doc/api/runners.md @@ -30,6 +30,7 @@ Example response: "description": "test-1-20150125", "id": 6, "is_shared": false, + "ip_address": "127.0.0.1", "name": null, "online": true, "status": "online" @@ -38,6 +39,7 @@ Example response: "active": true, "description": "test-2-20150125", "id": 8, + "ip_address": "127.0.0.1", "is_shared": false, "name": null, "online": false, @@ -72,6 +74,7 @@ Example response: "active": true, "description": "shared-runner-1", "id": 1, + "ip_address": "127.0.0.1", "is_shared": true, "name": null, "online": true, @@ -81,6 +84,7 @@ Example response: "active": true, "description": "shared-runner-2", "id": 3, + "ip_address": "127.0.0.1", "is_shared": true, "name": null, "online": false @@ -90,6 +94,7 @@ Example response: "active": true, "description": "test-1-20150125", "id": 6, + "ip_address": "127.0.0.1", "is_shared": false, "name": null, "online": true @@ -99,6 +104,7 @@ Example response: "active": true, "description": "test-2-20150125", "id": 8, + "ip_address": "127.0.0.1", "is_shared": false, "name": null, "online": false, @@ -131,6 +137,7 @@ Example response: "architecture": null, "description": "test-1-20150125", "id": 6, + "ip_address": "127.0.0.1", "is_shared": false, "contacted_at": "2016-01-25T16:39:48.066Z", "name": null, @@ -189,6 +196,7 @@ Example response: "architecture": null, "description": "test-1-20150125-test", "id": 6, + "ip_address": "127.0.0.1", "is_shared": false, "contacted_at": "2016-01-25T16:39:48.066Z", "name": null, @@ -257,6 +265,7 @@ Example response: [ { "id": 2, + "ip_address": "127.0.0.1", "status": "running", "stage": "test", "name": "test", @@ -345,6 +354,7 @@ Example response: "active": true, "description": "test-2-20150125", "id": 8, + "ip_address": "127.0.0.1", "is_shared": false, "name": null, "online": false, @@ -354,6 +364,7 @@ Example response: "active": true, "description": "development_runner", "id": 5, + "ip_address": "127.0.0.1", "is_shared": true, "name": null, "online": true @@ -386,6 +397,7 @@ Example response: "active": true, "description": "test-2016-02-01", "id": 9, + "ip_address": "127.0.0.1", "is_shared": false, "name": null, "online": true, diff --git a/doc/development/utilities.md b/doc/development/utilities.md index 8f9aff1a35f..0d074a3ef05 100644 --- a/doc/development/utilities.md +++ b/doc/development/utilities.md @@ -135,3 +135,44 @@ We developed a number of utilities to ease development. Find.new.clear_memoization(:result) ``` + +## [`RequestCache`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/gitlab/cache/request_cache.rb) + +This module provides a simple way to cache values in RequestStore, +and the cache key would be based on the class name, method name, +optionally customized instance level values, optionally customized +method level values, and optional method arguments. + +A simple example that only uses the instance level customised values: + +``` ruby +class UserAccess + extend Gitlab::Cache::RequestCache + + request_cache_key do + [user&.id, project&.id] + end + + request_cache def can_push_to_branch?(ref) + # ... + end +end +``` + +This way, the result of `can_push_to_branch?` would be cached in +`RequestStore.store` based on the cache key. If `RequestStore` is not +currently active, then it would be stored in a hash saved in an +instance variable, so the cache logic would be the same. + +We can also set different strategies for different methods: + +``` ruby +class Commit + extend Gitlab::Cache::RequestCache + + def author + User.find_by_any_email(author_email.downcase) + end + request_cache(:author) { author_email.downcase } +end +``` diff --git a/doc/topics/autodevops/index.md b/doc/topics/autodevops/index.md index 9b1acbac17f..1d26a743500 100644 --- a/doc/topics/autodevops/index.md +++ b/doc/topics/autodevops/index.md @@ -44,15 +44,15 @@ project in a simple and automatic way: 1. [Auto Build](#auto-build) 1. [Auto Test](#auto-test) -1. [Auto Code Quality](#auto-code-quality) -1. [Auto SAST (Static Application Security Testing)](#auto-sast) -1. [Auto Dependency Scanning](#auto-dependency-scanning) -1. [Auto License Management](#auto-license-management) +1. [Auto Code Quality](#auto-code-quality) **[STARTER]** +1. [Auto SAST (Static Application Security Testing)](#auto-sast) **[ULTIMATE]** +1. [Auto Dependency Scanning](#auto-dependency-scanning) **[ULTIMATE]** +1. [Auto License Management](#auto-license-management) **[ULTIMATE]** 1. [Auto Container Scanning](#auto-container-scanning) 1. [Auto Review Apps](#auto-review-apps) -1. [Auto DAST (Dynamic Application Security Testing)](#auto-dast) +1. [Auto DAST (Dynamic Application Security Testing)](#auto-dast) **[ULTIMATE]** 1. [Auto Deploy](#auto-deploy) -1. [Auto Browser Performance Testing](#auto-browser-performance-testing) +1. [Auto Browser Performance Testing](#auto-browser-performance-testing) **[PREMIUM]** 1. [Auto Monitoring](#auto-monitoring) As Auto DevOps relies on many different components, it's good to have a basic diff --git a/doc/user/project/merge_requests/squash_and_merge.md b/doc/user/project/merge_requests/squash_and_merge.md index a6efe893853..2ec423dcf70 100644 --- a/doc/user/project/merge_requests/squash_and_merge.md +++ b/doc/user/project/merge_requests/squash_and_merge.md @@ -1,6 +1,6 @@ # Squash and merge -> [Introduced][ee-1024] in [GitLab Starter][ee] 8.17, and in [GitLab CE][ce] [11.0][ce-18956]. +> [Introduced][ee-1024] in [GitLab Starter][ee] 8.17, and in [GitLab Core][ce] [11.0][ce-18956]. Combine all commits of your merge request into one and retain a clean history. @@ -75,6 +75,6 @@ squashing can itself be considered equivalent to rebasing. [squash-edit-form]: img/squash_edit_form.png [squash-mr-widget]: img/squash_mr_widget.png [ff-merge]: fast_forward_merge.md#enabling-fast-forward-merges -[ce]: https://about.gitlab.com/products/ -[ee]: https://about.gitlab.com/products/ +[ce]: https://about.gitlab.com/pricing/ +[ee]: https://about.gitlab.com/pricing/ [revert]: revert_changes.md diff --git a/lib/gitlab/cache/request_cache.rb b/lib/gitlab/cache/request_cache.rb index ecc85f847d4..671b8e7e1b1 100644 --- a/lib/gitlab/cache/request_cache.rb +++ b/lib/gitlab/cache/request_cache.rb @@ -1,41 +1,6 @@ module Gitlab module Cache - # This module provides a simple way to cache values in RequestStore, - # and the cache key would be based on the class name, method name, - # optionally customized instance level values, optionally customized - # method level values, and optional method arguments. - # - # A simple example: - # - # class UserAccess - # extend Gitlab::Cache::RequestCache - # - # request_cache_key do - # [user&.id, project&.id] - # end - # - # request_cache def can_push_to_branch?(ref) - # # ... - # end - # end - # - # This way, the result of `can_push_to_branch?` would be cached in - # `RequestStore.store` based on the cache key. If RequestStore is not - # currently active, then it would be stored in a hash saved in an - # instance variable, so the cache logic would be the same. - # Here's another example using customized method level values: - # - # class Commit - # extend Gitlab::Cache::RequestCache - # - # def author - # User.find_by_any_email(author_email.downcase) - # end - # request_cache(:author) { author_email.downcase } - # end - # - # So that we could have different strategies for different methods - # + # See https://docs.gitlab.com/ee/development/utilities.html#requestcache module RequestCache def self.extended(klass) return if klass < self diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index de1ecf18dd8..0904e1c2973 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -403,13 +403,7 @@ module Gitlab # Return repo size in megabytes def size - size = gitaly_migrate(:repository_size) do |is_enabled| - if is_enabled - size_by_gitaly - else - size_by_shelling_out - end - end + size = gitaly_repository_client.repository_size (size.to_f / 1024).round(2) end @@ -936,13 +930,7 @@ module Gitlab # # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/327 def ls_files(ref) - gitaly_migrate(:ls_files) do |is_enabled| - if is_enabled - gitaly_ls_files(ref) - else - git_ls_files(ref) - end - end + gitaly_commit_client.ls_files(ref) end # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/328 @@ -1818,41 +1806,6 @@ module Gitlab commit(sha) end - def size_by_shelling_out - popen(%w(du -sk), path).first.strip.to_i - end - - def size_by_gitaly - gitaly_repository_client.repository_size - end - - def gitaly_ls_files(ref) - gitaly_commit_client.ls_files(ref) - end - - def git_ls_files(ref) - actual_ref = ref || root_ref - - begin - sha_from_ref(actual_ref) - rescue Rugged::OdbError, Rugged::InvalidError, Rugged::ReferenceError - # Return an empty array if the ref wasn't found - return [] - end - - cmd = %W(ls-tree -r --full-tree --full-name -- #{actual_ref}) - raw_output, _status = run_git(cmd) - - lines = raw_output.split("\n").map do |f| - stuff, path = f.split("\t") - _mode, type, _sha = stuff.split(" ") - path if type == "blob" - # Contain only blob type - end - - lines.compact - end - # Returns true if the given ref name exists # # Ref names must start with `refs/`. diff --git a/lib/gitlab/metrics/samplers/ruby_sampler.rb b/lib/gitlab/metrics/samplers/ruby_sampler.rb index a39b3bc158c..7b2b3bedf04 100644 --- a/lib/gitlab/metrics/samplers/ruby_sampler.rb +++ b/lib/gitlab/metrics/samplers/ruby_sampler.rb @@ -22,27 +22,27 @@ module Gitlab def init_metrics metrics = {} - metrics[:sampler_duration] = Metrics.histogram(with_prefix(:sampler_duration, :seconds), 'Sampler time', { worker: nil }) - metrics[:total_time] = Metrics.gauge(with_prefix(:gc, :time_total), 'Total GC time', labels, :livesum) + metrics[:sampler_duration] = Metrics.counter(with_prefix(:sampler, :duration_seconds_total), 'Sampler time', labels) + metrics[:total_time] = Metrics.counter(with_prefix(:gc, :duration_seconds_total), 'Total GC time', labels) GC.stat.keys.each do |key| - metrics[key] = Metrics.gauge(with_prefix(:gc, key), to_doc_string(key), labels, :livesum) + metrics[key] = Metrics.gauge(with_prefix(:gc_stat, key), to_doc_string(key), labels, :livesum) end - metrics[:objects_total] = Metrics.gauge(with_prefix(:objects, :total), 'Objects total', labels.merge(class: nil), :livesum) - metrics[:memory_usage] = Metrics.gauge(with_prefix(:memory, :usage_total), 'Memory used total', labels, :livesum) - metrics[:file_descriptors] = Metrics.gauge(with_prefix(:file, :descriptors_total), 'File descriptors total', labels, :livesum) + metrics[:memory_usage] = Metrics.gauge(with_prefix(:memory, :bytes), 'Memory used', labels, :livesum) + metrics[:file_descriptors] = Metrics.gauge(with_prefix(:file, :descriptors), 'File descriptors used', labels, :livesum) metrics end def sample start_time = System.monotonic_time - sample_gc - metrics[:memory_usage].set(labels, System.memory_usage) - metrics[:file_descriptors].set(labels, System.file_descriptor_count) + metrics[:memory_usage].set(labels.merge(worker_label), System.memory_usage) + metrics[:file_descriptors].set(labels.merge(worker_label), System.file_descriptor_count) + + sample_gc - metrics[:sampler_duration].observe(labels.merge(worker_label), System.monotonic_time - start_time) + metrics[:sampler_duration].increment(labels, System.monotonic_time - start_time) ensure GC::Profiler.clear end @@ -50,11 +50,13 @@ module Gitlab private def sample_gc - metrics[:total_time].set(labels, GC::Profiler.total_time * 1000) - + # Collect generic GC stats. GC.stat.each do |key, value| metrics[key].set(labels, value) end + + # Collect the GC time since last sample in float seconds. + metrics[:total_time].increment(labels, GC::Profiler.total_time) end def worker_label diff --git a/rubocop/cop/migration/update_large_table.rb b/rubocop/cop/migration/update_large_table.rb index bb14d0f4f56..c15eec22d04 100644 --- a/rubocop/cop/migration/update_large_table.rb +++ b/rubocop/cop/migration/update_large_table.rb @@ -20,10 +20,14 @@ module RuboCop 'necessary'.freeze LARGE_TABLES = %i[ - ci_pipelines + ci_build_trace_sections ci_builds + ci_job_artifacts + ci_pipelines + ci_stages events issues + merge_request_diff_commits merge_request_diff_files merge_request_diffs merge_requests @@ -34,8 +38,15 @@ module RuboCop users ].freeze + BATCH_UPDATE_METHODS = %w[ + :add_column_with_default + :change_column_type_concurrently + :rename_column_concurrently + :update_column_in_batches + ].join(' ').freeze + def_node_matcher :batch_update?, <<~PATTERN - (send nil? ${:add_column_with_default :update_column_in_batches} $(sym ...) ...) + (send nil? ${#{BATCH_UPDATE_METHODS}} $(sym ...) ...) PATTERN def on_send(node) diff --git a/spec/features/issues/filtered_search/filter_issues_spec.rb b/spec/features/issues/filtered_search/filter_issues_spec.rb index bc42618306f..8dca81a8627 100644 --- a/spec/features/issues/filtered_search/filter_issues_spec.rb +++ b/spec/features/issues/filtered_search/filter_issues_spec.rb @@ -10,6 +10,7 @@ describe 'Filter issues', :js do # When the name is longer, the filtered search input can end up scrolling # horizontally, and PhantomJS can't handle it. let(:user) { create(:user, name: 'Ann') } + let(:user2) { create(:user, name: 'jane') } let!(:bug_label) { create(:label, project: project, title: 'bug') } let!(:caps_sensitive_label) { create(:label, project: project, title: 'CaPs') } @@ -25,8 +26,6 @@ describe 'Filter issues', :js do before do project.add_master(user) - user2 = create(:user) - create(:issue, project: project, author: user2, title: "Bug report 1") create(:issue, project: project, author: user2, title: "Bug report 2") @@ -113,6 +112,24 @@ describe 'Filter issues', :js do expect_issues_list_count(3) expect_filtered_search_input_empty end + + it 'filters issues by invalid assignee' do + skip('to be tested, issue #26546') + end + + it 'filters issues by multiple assignees' do + create(:issue, project: project, author: user, assignees: [user2, user]) + + input_filtered_search("assignee:@#{user.username} assignee:@#{user2.username}") + + expect_tokens([ + assignee_token(user.name), + assignee_token(user2.name) + ]) + + expect_issues_list_count(1) + expect_filtered_search_input_empty + end end end @@ -491,6 +508,21 @@ describe 'Filter issues', :js do it_behaves_like 'updates atom feed link', :group do let(:path) { issues_group_path(group, milestone_title: milestone.title, assignee_id: user.id) } end + + it 'updates atom feed link for group issues' do + visit issues_group_path(group, milestone_title: milestone.title, assignee_id: user.id) + link = find('.nav-controls a[title="Subscribe to RSS feed"]', visible: false) + params = CGI.parse(URI.parse(link[:href]).query) + auto_discovery_link = find('link[type="application/atom+xml"]', visible: false) + auto_discovery_params = CGI.parse(URI.parse(auto_discovery_link[:href]).query) + + expect(params).to include('feed_token' => [user.feed_token]) + expect(params).to include('milestone_title' => [milestone.title]) + expect(params).to include('assignee_id' => [user.id.to_s]) + expect(auto_discovery_params).to include('feed_token' => [user.feed_token]) + expect(auto_discovery_params).to include('milestone_title' => [milestone.title]) + expect(auto_discovery_params).to include('assignee_id' => [user.id.to_s]) + end end context 'URL has a trailing slash' do diff --git a/spec/lib/gitlab/metrics/samplers/ruby_sampler_spec.rb b/spec/lib/gitlab/metrics/samplers/ruby_sampler_spec.rb index 091645ee86f..7972ff253fe 100644 --- a/spec/lib/gitlab/metrics/samplers/ruby_sampler_spec.rb +++ b/spec/lib/gitlab/metrics/samplers/ruby_sampler_spec.rb @@ -45,7 +45,7 @@ describe Gitlab::Metrics::Samplers::RubySampler do it 'adds a metric containing garbage collection time statistics' do expect(GC::Profiler).to receive(:total_time).and_return(0.24) - expect(sampler.metrics[:total_time]).to receive(:set).with({}, 240) + expect(sampler.metrics[:total_time]).to receive(:increment).with({}, 0.24) sampler.sample end diff --git a/spec/rubocop/cop/migration/update_large_table_spec.rb b/spec/rubocop/cop/migration/update_large_table_spec.rb index ef724fc8bad..5e08eb4f772 100644 --- a/spec/rubocop/cop/migration/update_large_table_spec.rb +++ b/spec/rubocop/cop/migration/update_large_table_spec.rb @@ -32,6 +32,14 @@ describe RuboCop::Cop::Migration::UpdateLargeTable do include_examples 'large tables', 'add_column_with_default' end + context 'for the change_column_type_concurrently method' do + include_examples 'large tables', 'change_column_type_concurrently' + end + + context 'for the rename_column_concurrently method' do + include_examples 'large tables', 'rename_column_concurrently' + end + context 'for the update_column_in_batches method' do include_examples 'large tables', 'update_column_in_batches' end @@ -60,6 +68,18 @@ describe RuboCop::Cop::Migration::UpdateLargeTable do expect(cop.offenses).to be_empty end + it 'registers no offense for change_column_type_concurrently' do + inspect_source("change_column_type_concurrently :#{table}, :column, default: true") + + expect(cop.offenses).to be_empty + end + + it 'registers no offense for update_column_in_batches' do + inspect_source("rename_column_concurrently :#{table}, :column, default: true") + + expect(cop.offenses).to be_empty + end + it 'registers no offense for update_column_in_batches' do inspect_source("add_column_with_default :#{table}, :column, default: true") |