Show what RPC is called in the performance bar

Now only the data was shown of the service, which is not valueable at
times given some of those expose a lot of RPCs.

4 files changed, 15 insertions, 123 deletions

diff --git a/changelogs/unreleased/zj-improve-gitaly-pb.yml b/changelogs/unreleased/zj-improve-gitaly-pb.yml
new file mode 100644
index 00000000000..506a0303d8a
--- /dev/null
+++ b/changelogs/unreleased/zj-improve-gitaly-pb.yml
@@ -0,0 +1,5 @@
+---
+title: Show what RPC is called in the performance bar
+merge_request: 23140
+author:
+type: other
diff --git a/lib/gitlab/git/blob.rb b/lib/gitlab/git/blob.rb
index 9dd1c484d59..2d25389594e 100644
--- a/lib/gitlab/git/blob.rb
+++ b/lib/gitlab/git/blob.rb
@@ -1,7 +1,5 @@
 # frozen_string_literal: true
 
-# Gitaly note: JV: seems to be completely migrated (behind feature flags).
-
 module Gitlab
   module Git
     class Blob
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 993955d1a6b..289796ae93b 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -885,12 +885,6 @@ module Gitlab
         Gitlab::GitalyClient::ConflictsService.new(self, our_commit_oid, their_commit_oid)
       end
 
-      def gitaly_migrate(method, status: Gitlab::GitalyClient::MigrationStatus::OPT_IN, &block)
-        wrapped_gitaly_errors do
-          Gitlab::GitalyClient.migrate(method, status: status, &block)
-        end
-      end
-
       def clean_stale_repository_files
         wrapped_gitaly_errors do
           gitaly_repository_client.cleanup if exists?
diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb
index d99a9f15371..613f0d31075 100644
--- a/lib/gitlab/gitaly_client.rb
+++ b/lib/gitlab/gitaly_client.rb
@@ -9,11 +9,6 @@ require 'grpc/health/v1/health_services_pb'
 module Gitlab
   module GitalyClient
     include Gitlab::Metrics::Methods
-    module MigrationStatus
-      DISABLED = 1
-      OPT_IN = 2
-      OPT_OUT = 3
-    end
 
     class TooManyInvocationsError < StandardError
       attr_reader :call_site, :invocation_count, :max_call_stack
@@ -31,7 +26,7 @@ module Gitlab
       end
     end
 
-    SERVER_VERSION_FILE = 'GITALY_SERVER_VERSION'.freeze
+    SERVER_VERSION_FILE = 'GITALY_SERVER_VERSION'
     MAXIMUM_GITALY_CALLS = 35
     CLIENT_NAME = (Sidekiq.server? ? 'gitlab-sidekiq' : 'gitlab-web').freeze
 
@@ -43,11 +38,6 @@ module Gitlab
 
     self.query_time = 0
 
-    define_histogram :gitaly_migrate_call_duration_seconds do
-      docstring "Gitaly migration call execution timings"
-      base_labels gitaly_enabled: nil, feature: nil
-    end
-
     define_histogram :gitaly_controller_action_duration_seconds do
       docstring "Gitaly endpoint histogram by controller and action combination"
       base_labels Gitlab::Metrics::Transaction::BASE_LABELS.merge(gitaly_service: nil, rpc: nil)
@@ -126,7 +116,6 @@ module Gitlab
     def self.call(storage, service, rpc, request, remote_storage: nil, timeout: nil)
       start = Gitlab::Metrics::System.monotonic_time
       request_hash = request.is_a?(Google::Protobuf::MessageExts) ? request.to_h : {}
-      @current_call_id ||= SecureRandom.uuid
 
       enforce_gitaly_request_limits(:call)
 
@@ -145,9 +134,7 @@ module Gitlab
         current_transaction_labels.merge(gitaly_service: service.to_s, rpc: rpc.to_s),
         duration)
 
-      add_call_details(id: @current_call_id, feature: service, duration: duration, request: request_hash)
-
-      @current_call_id = nil
+      add_call_details(feature: "#{service}##{rpc}", duration: duration, request: request_hash, rpc: rpc)
     end
 
     def self.handle_grpc_unavailable!(ex)
@@ -222,7 +209,7 @@ module Gitlab
       result
     end
 
-    SERVER_FEATURE_FLAGS = %w[gogit_findcommit].freeze
+    SERVER_FEATURE_FLAGS = %w[].freeze
 
     def self.server_feature_flags
       SERVER_FEATURE_FLAGS.map do |f|
@@ -237,82 +224,8 @@ module Gitlab
       params['gitaly_token'].presence || Gitlab.config.gitaly['token']
     end
 
-    # Evaluates whether a feature toggle is on or off
-    def self.feature_enabled?(feature_name, status: MigrationStatus::OPT_IN)
-      # Disabled features are always off!
-      return false if status == MigrationStatus::DISABLED
-
-      feature = Feature.get("gitaly_#{feature_name}")
-
-      # If the feature has been set, always evaluate
-      if Feature.persisted?(feature)
-        if feature.percentage_of_time_value > 0
-          # Probabilistically enable this feature
-          return Random.rand() * 100 < feature.percentage_of_time_value
-        end
-
-        return feature.enabled?
-      end
-
-      # If the feature has not been set, the default depends
-      # on it's status
-      case status
-      when MigrationStatus::OPT_OUT
-        true
-      when MigrationStatus::OPT_IN
-        opt_into_all_features? && !explicit_opt_in_required.include?(feature_name)
-      else
-        false
-      end
-    rescue => ex
-      # During application startup feature lookups in SQL can fail
-      Rails.logger.warn "exception while checking Gitaly feature status for #{feature_name}: #{ex}"
-      false
-    end
-
-    # We have a mechanism to let GitLab automatically opt in to all Gitaly
-    # features. We want to be able to exclude some features from automatic
-    # opt-in. This function has an override in EE.
-    def self.explicit_opt_in_required
-      []
-    end
-
-    # opt_into_all_features? returns true when the current environment
-    # is one in which we opt into features automatically
-    def self.opt_into_all_features?
-      Rails.env.development? || ENV["GITALY_FEATURE_DEFAULT_ON"] == "1"
-    end
-    private_class_method :opt_into_all_features?
-
-    def self.migrate(feature, status: MigrationStatus::OPT_IN)
-      # Enforce limits at both the `migrate` and `call` sites to ensure that
-      # problems are not hidden by a feature being disabled
-      enforce_gitaly_request_limits(:migrate)
-
-      is_enabled  = feature_enabled?(feature, status: status)
-      metric_name = feature.to_s
-      metric_name += "_gitaly" if is_enabled
-
-      Gitlab::Metrics.measure(metric_name) do
-        # Some migrate calls wrap other migrate calls
-        allow_n_plus_1_calls do
-          feature_stack = Thread.current[:gitaly_feature_stack] ||= []
-          feature_stack.unshift(feature)
-          begin
-            start = Gitlab::Metrics::System.monotonic_time
-            @current_call_id = SecureRandom.uuid
-            call_details = { id: @current_call_id }
-            yield is_enabled
-          ensure
-            total_time = Gitlab::Metrics::System.monotonic_time - start
-            gitaly_migrate_call_duration_seconds.observe({ gitaly_enabled: is_enabled, feature: feature }, total_time)
-            feature_stack.shift
-            Thread.current[:gitaly_feature_stack] = nil if feature_stack.empty?
-
-            add_call_details(call_details.merge(feature: feature, duration: total_time))
-          end
-        end
-      end
+    def self.feature_enabled?(feature_name)
+      Feature.enabled?("gitaly_#{feature_name}")
     end
 
     # Ensures that Gitaly is not being abuse through n+1 misuse etc
@@ -368,38 +281,20 @@ module Gitlab
     end
     private_class_method :decrement_call_count
 
-    # Returns an estimate of the number of Gitaly calls made for this
-    # request
+    # Returns the of the number of Gitaly calls made for this request
     def self.get_request_count
-      return 0 unless Gitlab::SafeRequestStore.active?
-
-      gitaly_migrate_count = get_call_count("gitaly_migrate_actual")
-      gitaly_call_count = get_call_count("gitaly_call_actual")
-
-      # Using the maximum of migrate and call_count will provide an
-      # indicator of how many Gitaly calls will be made, even
-      # before a feature is enabled. This provides us with a single
-      # metric, but not an exact number, but this tradeoff is acceptable
-      if gitaly_migrate_count > gitaly_call_count
-        gitaly_migrate_count
-      else
-        gitaly_call_count
-      end
+      get_call_count("gitaly_call_actual")
     end
 
     def self.reset_counts
       return unless Gitlab::SafeRequestStore.active?
 
-      %w[migrate call].each do |call_site|
-        Gitlab::SafeRequestStore["gitaly_#{call_site}_actual"] = 0
-        Gitlab::SafeRequestStore["gitaly_#{call_site}_permitted"] = 0
-      end
+      Gitlab::SafeRequestStore["gitaly_call_actual"] = 0
+      Gitlab::SafeRequestStore["gitaly_call_permitted"] = 0
     end
 
     def self.add_call_details(details)
-      id = details.delete(:id)
-
-      return unless id && Gitlab::SafeRequestStore[:peek_enabled]
+      return unless Gitlab::SafeRequestStore[:peek_enabled]
 
       Gitlab::SafeRequestStore['gitaly_call_details'] ||= {}
       Gitlab::SafeRequestStore['gitaly_call_details'][id] ||= {}