diff options
author | Zeger-Jan van de Weg <git@zjvandeweg.nl> | 2018-11-16 12:53:44 +0300 |
---|---|---|
committer | Zeger-Jan van de Weg <git@zjvandeweg.nl> | 2018-11-16 14:16:25 +0300 |
commit | 35239a6aec84c47895ad6664a9b1075be12bd105 (patch) | |
tree | 52395a00d1d09ad73d527e7e1bbb0832079f63e2 | |
parent | e7a40cbe1637c7331e3397ed29a5e66ee7a1ade6 (diff) |
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.
-rw-r--r-- | changelogs/unreleased/zj-improve-gitaly-pb.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/git/blob.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/git/repository.rb | 6 | ||||
-rw-r--r-- | lib/gitlab/gitaly_client.rb | 125 |
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] ||= {} |