Measure Gitaly n+1 blocksgitaly-n-plus-one-metrics

This change adds additional monitoring to Gitaly n+1 blocks so that
we can prioritise fixes for these blocks according to their production
expense.

23 files changed, 52 insertions, 39 deletions

diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb
index 65d14781d92..588f9220671 100644
--- a/app/controllers/dashboard/projects_controller.rb
+++ b/app/controllers/dashboard/projects_controller.rb
@@ -53,7 +53,7 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController
 
   def render_projects
     # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/40260
-    Gitlab::GitalyClient.allow_n_plus_1_calls do
+    Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#40260.1") do
       render
     end
   end
diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb
index fc708400657..f98cce82305 100644
--- a/app/controllers/projects/branches_controller.rb
+++ b/app/controllers/projects/branches_controller.rb
@@ -26,7 +26,7 @@ class Projects::BranchesController < Projects::ApplicationController
         @merged_branch_names = repository.merged_branch_names(@branches.map(&:name))
 
         # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/48097
-        Gitlab::GitalyClient.allow_n_plus_1_calls do
+        Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#48097.1") do
           @max_commits = @branches.reduce(0) do |memo, branch|
             diverging_commit_counts = repository.diverging_commit_counts(branch)
             [memo, diverging_commit_counts.values_at(:behind, :ahead, :distance)]
@@ -35,7 +35,7 @@ class Projects::BranchesController < Projects::ApplicationController
         end
 
         # https://gitlab.com/gitlab-org/gitlab-ce/issues/48097
-        Gitlab::GitalyClient.allow_n_plus_1_calls do
+        Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#48097.2") do
           render
         end
       end
diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb
index 32cefe54613..632ac10399e 100644
--- a/app/controllers/projects/merge_requests/creations_controller.rb
+++ b/app/controllers/projects/merge_requests/creations_controller.rb
@@ -13,7 +13,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
 
   def new
     # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/40934
-    Gitlab::GitalyClient.allow_n_plus_1_calls do
+    Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#40934") do
       define_new_vars
     end
   end
@@ -91,7 +91,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
     params[:merge_request] ||= ActionController::Parameters.new(source_project: @project)
 
     # Gitaly N+1 issue: https://gitlab.com/gitlab-org/gitlab-ce/issues/58096
-    Gitlab::GitalyClient.allow_n_plus_1_calls do
+    Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#58096") do
       @merge_request = ::MergeRequests::BuildService.new(project, current_user, merge_request_params.merge(diff_options: diff_options)).execute
     end
   end
diff --git a/app/controllers/root_controller.rb b/app/controllers/root_controller.rb
index f1b39125a48..ba653c62cc9 100644
--- a/app/controllers/root_controller.rb
+++ b/app/controllers/root_controller.rb
@@ -16,7 +16,7 @@ class RootController < Dashboard::ProjectsController
 
   def index
     # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/40260
-    Gitlab::GitalyClient.allow_n_plus_1_calls do
+    Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#40260.2") do
       super
     end
   end
diff --git a/app/models/blob.rb b/app/models/blob.rb
index d528bef8b19..09b295489d8 100644
--- a/app/models/blob.rb
+++ b/app/models/blob.rb
@@ -111,7 +111,7 @@ class Blob < SimpleDelegator
 
   def load_all_data!
     # Endpoint needed: https://gitlab.com/gitlab-org/gitaly/issues/756
-    Gitlab::GitalyClient.allow_n_plus_1_calls do
+    Gitlab::GitalyClient.allow_n_plus_1_calls("gitaly#756") do
       super(project.repository) if project
     end
   end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index df2dc9c49eb..b13bebeca35 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -667,7 +667,7 @@ class MergeRequest < ApplicationRecord
     fetch_ref!
 
     # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37435
-    Gitlab::GitalyClient.allow_n_plus_1_calls do
+    Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#38327") do
       merge_request_diffs.create!
       reload_merge_request_diff
     end
diff --git a/app/models/network/graph.rb b/app/models/network/graph.rb
index ecbeb24ee0a..b5001a35e28 100644
--- a/app/models/network/graph.rb
+++ b/app/models/network/graph.rb
@@ -41,7 +41,7 @@ module Network
     #
     def collect_commits
       # https://gitlab.com/gitlab-org/gitlab-ce/issues/58013
-      Gitlab::GitalyClient.allow_n_plus_1_calls do
+      Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#58013") do
         find_commits(count_to_display_commit_in_center).map do |commit|
           # Decorate with app/model/network/commit.rb
           Network::Commit.new(commit)
diff --git a/app/models/project.rb b/app/models/project.rb
index 351d08eaf63..c07918cb441 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -2292,7 +2292,7 @@ class Project < ApplicationRecord
       next false if empty_repo?
 
       # Issue for N+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/49322
-      Gitlab::GitalyClient.allow_n_plus_1_calls do
+      Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#49322") do
         merge_requests_allowing_collaboration(branch_name).any? do |merge_request|
           merge_request.can_be_merged_by?(user)
         end
diff --git a/app/models/repository.rb b/app/models/repository.rb
index e05d3dd58ac..7d4b5f4ca96 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -607,7 +607,7 @@ class Repository
 
   def avatar
     # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/38327
-    Gitlab::GitalyClient.allow_n_plus_1_calls do
+    Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#38327") do
       if tree = file_on_head(:avatar)
         tree.path
       end
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index 4b199bd8fa8..b6d4439a616 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -15,7 +15,9 @@ module MergeRequests
 
     def refresh_merge_requests!
       # n + 1: https://gitlab.com/gitlab-org/gitlab-ce/issues/60289
-      Gitlab::GitalyClient.allow_n_plus_1_calls(&method(:find_new_commits))
+      Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#60289") do
+        find_new_commits
+      end
 
       # Be sure to close outstanding MRs before reloading them to avoid generating an
       # empty diff during a manual merge
diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb
index 1b46f6d8a72..b4b3cfec835 100644
--- a/app/services/notes/create_service.rb
+++ b/app/services/notes/create_service.rb
@@ -8,7 +8,7 @@ module Notes
       note = Notes::BuildService.new(project, current_user, params).execute
 
       # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37440
-      note_valid = Gitlab::GitalyClient.allow_n_plus_1_calls do
+      note_valid = Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#37440") do
         note.valid?
       end
 
diff --git a/doc/development/gitaly.md b/doc/development/gitaly.md
index a0585fed2fc..eb20172454f 100644
--- a/doc/development/gitaly.md
+++ b/doc/development/gitaly.md
@@ -135,8 +135,8 @@ of the [Gitaly Team](https://gitlab.com/groups/gl-gitaly/group_members) for assi
 Once the source has been found, wrap it in an `allow_n_plus_1_calls` block, as follows:
 
 ```ruby
-# n+1: link to n+1 issue
-Gitlab::GitalyClient.allow_n_plus_1_calls do
+# n+1: link to n+1 issue: gitlab.com/gitlab-org/gitlab-ce/issues/123
+Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#123") do
   # original code
   commits.each { |commit| ... }
 end
diff --git a/lib/banzai/filter/commit_reference_filter.rb b/lib/banzai/filter/commit_reference_filter.rb
index c3e5ac41cb8..d2880cb7e1d 100644
--- a/lib/banzai/filter/commit_reference_filter.rb
+++ b/lib/banzai/filter/commit_reference_filter.rb
@@ -23,7 +23,7 @@ module Banzai
 
         if project && project.valid_repo?
           # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/43894
-          Gitlab::GitalyClient.allow_n_plus_1_calls { project.commit(id) }
+          Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#43894") { project.commit(id) }
         end
       end
 
diff --git a/lib/banzai/filter/relative_link_filter.rb b/lib/banzai/filter/relative_link_filter.rb
index 80c84c0f622..34ae3cbb72e 100644
--- a/lib/banzai/filter/relative_link_filter.rb
+++ b/lib/banzai/filter/relative_link_filter.rb
@@ -153,7 +153,7 @@ module Banzai
 
       def uri_type(path)
         # https://gitlab.com/gitlab-org/gitlab-ce/issues/58657
-        Gitlab::GitalyClient.allow_n_plus_1_calls do
+        Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#58657") do
           @uri_types[path] ||= current_commit.uri_type(path)
         end
       end
diff --git a/lib/gitlab/checks/diff_check.rb b/lib/gitlab/checks/diff_check.rb
index ea0d8c85a66..19779f2993d 100644
--- a/lib/gitlab/checks/diff_check.rb
+++ b/lib/gitlab/checks/diff_check.rb
@@ -62,7 +62,7 @@ module Gitlab
       def process_commits
         logger.log_timed(LOG_MESSAGES[:diff_content_check]) do
           # n+1: https://gitlab.com/gitlab-org/gitlab-ee/issues/3593
-          ::Gitlab::GitalyClient.allow_n_plus_1_calls do
+          ::Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ee#3593") do
             commits.each do |commit|
               logger.check_timeout_reached
 
diff --git a/lib/gitlab/data_builder/push.rb b/lib/gitlab/data_builder/push.rb
index 40bda3410e1..d41abec76a8 100644
--- a/lib/gitlab/data_builder/push.rb
+++ b/lib/gitlab/data_builder/push.rb
@@ -73,7 +73,7 @@ module Gitlab
         # For performance purposes maximum 20 latest commits
         # will be passed as post receive hook data.
         # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/38259
-        commit_attrs = Gitlab::GitalyClient.allow_n_plus_1_calls do
+        commit_attrs = Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#38259") do
           commits_limited.map do |commit|
             commit.hook_attrs(with_changed_files: true)
           end
diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb
index 47976389af6..c4c91fbb538 100644
--- a/lib/gitlab/gitaly_client.rb
+++ b/lib/gitlab/gitaly_client.rb
@@ -38,6 +38,14 @@ module Gitlab
       base_labels Gitlab::Metrics::Transaction::BASE_LABELS.merge(gitaly_service: nil, rpc: nil)
     end
 
+    define_histogram :n_plus_one_histogram do
+      docstring "Time spent in Gitaly n plus one blocks, in seconds"
+    end
+
+    define_counter :n_plus_one_call_counter do
+      docstring "Calls made in Gitaly n plus one blocks"
+    end
+
     def self.stub(name, storage)
       MUTEX.synchronize do
         @stubs ||= {}
@@ -285,15 +293,18 @@ module Gitlab
     end
     private_class_method :enforce_gitaly_request_limits?
 
-    def self.allow_n_plus_1_calls
-      return yield unless Gitlab::SafeRequestStore.active?
+    def self.allow_n_plus_1_calls(call_site)
+      increment_call_count(:gitaly_call_count_exception_block_depth) if Gitlab::SafeRequestStore.active?
+      start_time = Gitlab::Metrics::System.monotonic_time
+      start_call_count = get_request_count
+      yield
+    ensure
+      labels = current_transaction_labels.merge(transaction: Gitlab::Database.inside_transaction? ? 1 : 0, call_site: call_site)
 
-      begin
-        increment_call_count(:gitaly_call_count_exception_block_depth)
-        yield
-      ensure
-        decrement_call_count(:gitaly_call_count_exception_block_depth)
-      end
+      n_plus_one_histogram.observe(labels, Gitlab::Metrics::System.monotonic_time - start_time)
+      n_plus_one_call_counter.increment(labels, get_request_count - start_call_count)
+
+      decrement_call_count(:gitaly_call_count_exception_block_depth) if Gitlab::SafeRequestStore.active?
     end
 
     # Normally a FindCommit RPC will cache the commit with its SHA
diff --git a/lib/gitlab/shell.rb b/lib/gitlab/shell.rb
index 93182607616..1c4f4676c79 100644
--- a/lib/gitlab/shell.rb
+++ b/lib/gitlab/shell.rb
@@ -262,7 +262,7 @@ module Gitlab
     #
     def add_namespace(storage, name)
       # https://gitlab.com/gitlab-org/gitlab-ce/issues/58012
-      Gitlab::GitalyClient.allow_n_plus_1_calls do
+      Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#58012") do
         Gitlab::GitalyClient::NamespaceService.new(storage).add(name)
       end
     rescue GRPC::InvalidArgument => e
diff --git a/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb b/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb
index 483c5ea9cff..70e987b313c 100644
--- a/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb
+++ b/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb
@@ -26,7 +26,7 @@ describe Gitlab::Cache::Ci::ProjectPipelineStatus, :clean_gitlab_redis_cache do
       end
 
       it 'loads 10 projects without hitting Gitaly call limit', :request_store do
-        projects = Gitlab::GitalyClient.allow_n_plus_1_calls do
+        projects = Gitlab::GitalyClient.allow_n_plus_1_calls("ProjectPipelineStatus") do
           (1..10).map { create(:project, :repository) }
         end
         Gitlab::GitalyClient.reset_counts
diff --git a/spec/lib/gitlab/gitaly_client_spec.rb b/spec/lib/gitlab/gitaly_client_spec.rb
index eed233f1f3e..f678016e657 100644
--- a/spec/lib/gitlab/gitaly_client_spec.rb
+++ b/spec/lib/gitlab/gitaly_client_spec.rb
@@ -131,13 +131,13 @@ describe Gitlab::GitalyClient do
   describe 'allow_n_plus_1_calls' do
     context 'when RequestStore is enabled', :request_store do
       it 'returns the result of the allow_n_plus_1_calls block' do
-        expect(described_class.allow_n_plus_1_calls { "result" }).to eq("result")
+        expect(described_class.allow_n_plus_1_calls("spec") { "result" }).to eq("result")
       end
     end
 
     context 'when RequestStore is not active' do
       it 'returns the result of the allow_n_plus_1_calls block' do
-        expect(described_class.allow_n_plus_1_calls { "something" }).to eq("something")
+        expect(described_class.allow_n_plus_1_calls("spec") { "something" }).to eq("something")
       end
     end
   end
@@ -218,7 +218,7 @@ describe Gitlab::GitalyClient do
 
       it 'allows the maximum number of calls to be exceeded within an allow_n_plus_1_calls block' do
         expect do
-          described_class.allow_n_plus_1_calls do
+          described_class.allow_n_plus_1_calls("spec") do
             call_gitaly(Gitlab::GitalyClient::MAXIMUM_GITALY_CALLS + 1)
           end
         end.not_to raise_error
@@ -226,7 +226,7 @@ describe Gitlab::GitalyClient do
 
       context 'when the maximum number of calls has been reached within an allow_n_plus_1_calls block' do
         before do
-          described_class.allow_n_plus_1_calls do
+          described_class.allow_n_plus_1_calls("spec") do
             call_gitaly(Gitlab::GitalyClient::MAXIMUM_GITALY_CALLS)
           end
         end
@@ -274,7 +274,7 @@ describe Gitlab::GitalyClient do
 
       it 'does not fail when the maximum number of calls is exceeded within an allow_n_plus_1_calls block' do
         expect do
-          described_class.allow_n_plus_1_calls do
+          described_class.allow_n_plus_1_calls("spec") do
             call_gitaly(Gitlab::GitalyClient::MAXIMUM_GITALY_CALLS + 1)
           end
         end.not_to raise_error
@@ -297,7 +297,7 @@ describe Gitlab::GitalyClient do
       context 'when enforce_gitaly_request_limits is called inside and outside of allow_n_plus_1_calls blocks' do
         before do
           described_class.enforce_gitaly_request_limits(:call)
-          described_class.allow_n_plus_1_calls do
+          described_class.allow_n_plus_1_calls("spec") do
             described_class.enforce_gitaly_request_limits(:call)
           end
         end
diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb
index 54e6abc2d3a..0d76cc98c6b 100644
--- a/spec/serializers/pipeline_serializer_spec.rb
+++ b/spec/serializers/pipeline_serializer_spec.rb
@@ -143,7 +143,7 @@ describe PipelineSerializer do
         # Since RequestStore.active? is true we have to allow the
         # gitaly calls in this block
         # Issue: https://gitlab.com/gitlab-org/gitlab-ce/issues/37772
-        Gitlab::GitalyClient.allow_n_plus_1_calls do
+        Gitlab::GitalyClient.allow_n_plus_1_calls("gitlab-ce#37772") do
           Ci::Pipeline::COMPLETED_STATUSES.each do |status|
             create_pipeline(status)
           end
diff --git a/spec/support/helpers/project_forks_helper.rb b/spec/support/helpers/project_forks_helper.rb
index bcb11a09b36..81588cc06fc 100644
--- a/spec/support/helpers/project_forks_helper.rb
+++ b/spec/support/helpers/project_forks_helper.rb
@@ -1,6 +1,6 @@
 module ProjectForksHelper
   def fork_project(project, user = nil, params = {})
-    Gitlab::GitalyClient.allow_n_plus_1_calls do
+    Gitlab::GitalyClient.allow_n_plus_1_calls("ProjectForksHelper#fork_project") do
       fork_project_direct(project, user, params)
     end
   end
@@ -50,7 +50,7 @@ module ProjectForksHelper
   end
 
   def fork_project_with_submodules(project, user = nil, params = {})
-    Gitlab::GitalyClient.allow_n_plus_1_calls do
+    Gitlab::GitalyClient.allow_n_plus_1_calls("ProjectForksHelper#fork_project_with_submodules") do
       forked_project = fork_project_direct(project, user, params)
       TestEnv.copy_repo(
         forked_project,
diff --git a/spec/support/shared_contexts/finders/merge_requests_finder_shared_contexts.rb b/spec/support/shared_contexts/finders/merge_requests_finder_shared_contexts.rb
index ab6687f1d07..f5a3b2fe272 100644
--- a/spec/support/shared_contexts/finders/merge_requests_finder_shared_contexts.rb
+++ b/spec/support/shared_contexts/finders/merge_requests_finder_shared_contexts.rb
@@ -8,7 +8,7 @@ RSpec.shared_context 'MergeRequestsFinder multiple projects with merge requests
   # but we don't care about potential N+1s when we're just creating several
   # projects in the setup phase.
   def allow_gitaly_n_plus_1
-    Gitlab::GitalyClient.allow_n_plus_1_calls do
+    Gitlab::GitalyClient.allow_n_plus_1_calls("MergeRequestsFinderSpec") do
       yield
     end
   end