Make performance bar enabled checks consistent

Previously, we called the `peek_enabled?` method like so:

    prepend_before_action :set_peek_request_id, if: :peek_enabled?

Now we don't have a `set_peek_request_id` method, so we don't need that
line. However, the `peek_enabled?` part had a side-effect: it would also
populate the request store cache for whether the performance bar was
enabled for the current request or not.

This commit makes that side-effect explicit, and replaces all uses of
`peek_enabled?` with the more explicit
`Gitlab::PerformanceBar.enabled_for_request?`. There is one spec that
still sets `SafeRequestStore[:peek_enabled]` directly, because it is
contrasting behaviour with and without a request store enabled.

The upshot is:

1. We still set the value in one place. We make it more explicit that
   that's what we're doing.
2. Reading that value uses a consistent method so it's easier to find in
   future.

12 files changed, 39 insertions, 45 deletions

diff --git a/app/controllers/concerns/with_performance_bar.rb b/app/controllers/concerns/with_performance_bar.rb
index b19d6eb9439..93ded59900d 100644
--- a/app/controllers/concerns/with_performance_bar.rb
+++ b/app/controllers/concerns/with_performance_bar.rb
@@ -4,20 +4,24 @@ module WithPerformanceBar
   extend ActiveSupport::Concern
 
   included do
-    before_action :peek_enabled? # Warm cache
+    before_action :set_peek_enabled_for_current_request
   end
 
-  protected
-
-  def peek_enabled?
-    return false unless Gitlab::PerformanceBar.enabled?(current_user)
+  private
 
+  def set_peek_enabled_for_current_request
     Gitlab::SafeRequestStore.fetch(:peek_enabled) { cookie_or_default_value }
   end
 
-  private
+  # Needed for Peek's routing to work;
+  # Peek::ResultsController#restrict_non_access calls this method.
+  def peek_enabled?
+    Gitlab::PerformanceBar.enabled_for_request?
+  end
 
   def cookie_or_default_value
+    return false unless Gitlab::PerformanceBar.enabled_for_user?(current_user)
+
     if cookies[:perf_bar_enabled].present?
       cookies[:perf_bar_enabled] == 'true'
     else
diff --git a/app/helpers/performance_bar_helper.rb b/app/helpers/performance_bar_helper.rb
index 7518cec160c..b225e4206a9 100644
--- a/app/helpers/performance_bar_helper.rb
+++ b/app/helpers/performance_bar_helper.rb
@@ -1,9 +1,7 @@
 # frozen_string_literal: true
 
 module PerformanceBarHelper
-  # This is a hack since using `alias_method :performance_bar_enabled?, :peek_enabled?`
-  # in WithPerformanceBar breaks tests (but works in the browser).
   def performance_bar_enabled?
-    peek_enabled?
+    Gitlab::PerformanceBar.enabled_for_request?
   end
 end
diff --git a/app/views/peek/_bar.html.haml b/app/views/peek/_bar.html.haml
index 3c2c4d30535..9725f640be9 100644
--- a/app/views/peek/_bar.html.haml
+++ b/app/views/peek/_bar.html.haml
@@ -1,4 +1,4 @@
-- return unless peek_enabled?
+- return unless performance_bar_enabled?
 
 #js-peek{ data: { env: Peek.env,
          request_id: peek_request_id,
diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb
index 201db9fec26..0aba0ff2bb2 100644
--- a/lib/gitlab/gitaly_client.rb
+++ b/lib/gitlab/gitaly_client.rb
@@ -157,7 +157,7 @@ module Gitlab
 
       # Keep track, separately, for the performance bar
       self.query_time += duration
-      if peek_enabled?
+      if Gitlab::PerformanceBar.enabled_for_request?
         add_call_details(feature: "#{service}##{rpc}", duration: duration, request: request_hash, rpc: rpc,
                          backtrace: Gitlab::Profiler.clean_backtrace(caller))
       end
@@ -335,17 +335,13 @@ module Gitlab
       Gitlab::SafeRequestStore["gitaly_call_permitted"] = 0
     end
 
-    def self.peek_enabled?
-      Gitlab::SafeRequestStore[:peek_enabled]
-    end
-
     def self.add_call_details(details)
       Gitlab::SafeRequestStore['gitaly_call_details'] ||= []
       Gitlab::SafeRequestStore['gitaly_call_details'] << details
     end
 
     def self.list_call_details
-      return [] unless peek_enabled?
+      return [] unless Gitlab::PerformanceBar.enabled_for_request?
 
       Gitlab::SafeRequestStore['gitaly_call_details'] || []
     end
diff --git a/lib/gitlab/performance_bar.rb b/lib/gitlab/performance_bar.rb
index 07439d8e011..68af290d069 100644
--- a/lib/gitlab/performance_bar.rb
+++ b/lib/gitlab/performance_bar.rb
@@ -6,7 +6,11 @@ module Gitlab
     EXPIRY_TIME_L1_CACHE = 1.minute
     EXPIRY_TIME_L2_CACHE = 5.minutes
 
-    def self.enabled?(user = nil)
+    def self.enabled_for_request?
+      Gitlab::SafeRequestStore[:peek_enabled]
+    end
+
+    def self.enabled_for_user?(user = nil)
       return true if Rails.env.development?
       return true if user&.admin?
       return false unless user && allowed_group_id
diff --git a/lib/gitlab/performance_bar/redis_adapter_when_peek_enabled.rb b/lib/gitlab/performance_bar/redis_adapter_when_peek_enabled.rb
index 9595ced0177..cddd4f18cc3 100644
--- a/lib/gitlab/performance_bar/redis_adapter_when_peek_enabled.rb
+++ b/lib/gitlab/performance_bar/redis_adapter_when_peek_enabled.rb
@@ -5,7 +5,7 @@ module Gitlab
   module PerformanceBar
     module RedisAdapterWhenPeekEnabled
       def save(request_id)
-        super if ::Peek.enabled? && request_id.present?
+        super if ::Gitlab::PerformanceBar.enabled_for_request? && request_id.present?
       end
     end
   end
diff --git a/lib/gitlab/rugged_instrumentation.rb b/lib/gitlab/rugged_instrumentation.rb
index 8bb8c547ae1..c2b55431547 100644
--- a/lib/gitlab/rugged_instrumentation.rb
+++ b/lib/gitlab/rugged_instrumentation.rb
@@ -27,19 +27,15 @@ module Gitlab
       SafeRequestStore.active?
     end
 
-    def self.peek_enabled?
-      SafeRequestStore[:peek_enabled]
-    end
-
     def self.add_call_details(details)
-      return unless peek_enabled?
+      return unless Gitlab::PerformanceBar.enabled_for_request?
 
       Gitlab::SafeRequestStore[:rugged_call_details] ||= []
       Gitlab::SafeRequestStore[:rugged_call_details] << details
     end
 
     def self.list_call_details
-      return [] unless peek_enabled?
+      return [] unless Gitlab::PerformanceBar.enabled_for_request?
 
       Gitlab::SafeRequestStore[:rugged_call_details] || []
     end
diff --git a/lib/peek/views/active_record.rb b/lib/peek/views/active_record.rb
index 2d78818630d..250c7d6aa8f 100644
--- a/lib/peek/views/active_record.rb
+++ b/lib/peek/views/active_record.rb
@@ -9,7 +9,7 @@ module Peek
         super
 
         subscribe('sql.active_record') do |_, start, finish, _, data|
-          if Gitlab::SafeRequestStore.store[:peek_enabled]
+          if Gitlab::PerformanceBar.enabled_for_request?
             unless data[:cached]
               detail_store << {
                 duration: finish - start,
diff --git a/lib/peek/views/redis_detailed.rb b/lib/peek/views/redis_detailed.rb
index f36f581d5e9..84041b6be73 100644
--- a/lib/peek/views/redis_detailed.rb
+++ b/lib/peek/views/redis_detailed.rb
@@ -16,7 +16,7 @@ module Gitlab
       private
 
       def add_call_details(duration, args)
-        return unless peek_enabled?
+        return unless Gitlab::PerformanceBar.enabled_for_request?
         # redis-rb passes an array (e.g. [:get, key])
         return unless args.length == 1
 
@@ -27,10 +27,6 @@ module Gitlab
         }
       end
 
-      def peek_enabled?
-        Gitlab::SafeRequestStore.store[:peek_enabled]
-      end
-
       def detail_store
         ::Gitlab::SafeRequestStore['redis_call_details'] ||= []
       end
diff --git a/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb b/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb
index 1a4168f7317..474240cf620 100644
--- a/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb
+++ b/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb
@@ -35,7 +35,7 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
     let(:args) { ['refs/heads/master', 1] }
 
     before do
-      allow(Gitlab::RuggedInstrumentation).to receive(:peek_enabled?).and_return(true)
+      allow(Gitlab::PerformanceBar).to receive(:enabled_for_request?).and_return(true)
     end
 
     it 'instruments Rugged call' do
diff --git a/spec/lib/gitlab/performance_bar_spec.rb b/spec/lib/gitlab/performance_bar_spec.rb
index 8d8ac2aebbe..816db49d94a 100644
--- a/spec/lib/gitlab/performance_bar_spec.rb
+++ b/spec/lib/gitlab/performance_bar_spec.rb
@@ -6,14 +6,14 @@ describe Gitlab::PerformanceBar do
   shared_examples 'allowed user IDs are cached' do
     before do
       # Warm the caches
-      described_class.enabled?(user)
+      described_class.enabled_for_user?(user)
     end
 
     it 'caches the allowed user IDs in cache', :use_clean_rails_memory_store_caching do
       expect do
         expect(described_class.l1_cache_backend).to receive(:fetch).and_call_original
         expect(described_class.l2_cache_backend).not_to receive(:fetch)
-        expect(described_class.enabled?(user)).to be_truthy
+        expect(described_class.enabled_for_user?(user)).to be_truthy
       end.not_to exceed_query_limit(0)
     end
 
@@ -22,7 +22,7 @@ describe Gitlab::PerformanceBar do
         expect do
           expect(described_class.l1_cache_backend).to receive(:fetch).and_call_original
           expect(described_class.l2_cache_backend).to receive(:fetch).and_call_original
-          expect(described_class.enabled?(user)).to be_truthy
+          expect(described_class.enabled_for_user?(user)).to be_truthy
         end.not_to exceed_query_limit(0)
       end
     end
@@ -32,7 +32,7 @@ describe Gitlab::PerformanceBar do
         expect do
           expect(described_class.l1_cache_backend).to receive(:fetch).and_call_original
           expect(described_class.l2_cache_backend).to receive(:fetch).and_call_original
-          expect(described_class.enabled?(user)).to be_truthy
+          expect(described_class.enabled_for_user?(user)).to be_truthy
         end.not_to exceed_query_limit(2)
       end
     end
@@ -41,7 +41,7 @@ describe Gitlab::PerformanceBar do
   it { expect(described_class.l1_cache_backend).to eq(Gitlab::ThreadMemoryCache.cache_backend) }
   it { expect(described_class.l2_cache_backend).to eq(Rails.cache) }
 
-  describe '.enabled?' do
+  describe '.enabled_for_user?' do
     let(:user) { create(:user) }
 
     before do
@@ -49,24 +49,24 @@ describe Gitlab::PerformanceBar do
     end
 
     it 'returns false when given user is nil' do
-      expect(described_class.enabled?(nil)).to be_falsy
+      expect(described_class.enabled_for_user?(nil)).to be_falsy
     end
 
     it 'returns true when given user is an admin' do
       user = build_stubbed(:user, :admin)
 
-      expect(described_class.enabled?(user)).to be_truthy
+      expect(described_class.enabled_for_user?(user)).to be_truthy
     end
 
     it 'returns false when allowed_group_id is nil' do
       expect(described_class).to receive(:allowed_group_id).and_return(nil)
 
-      expect(described_class.enabled?(user)).to be_falsy
+      expect(described_class.enabled_for_user?(user)).to be_falsy
     end
 
     context 'when allowed group ID does not exist' do
       it 'returns false' do
-        expect(described_class.enabled?(user)).to be_falsy
+        expect(described_class.enabled_for_user?(user)).to be_falsy
       end
     end
 
@@ -79,7 +79,7 @@ describe Gitlab::PerformanceBar do
 
       context 'when user is not a member of the allowed group' do
         it 'returns false' do
-          expect(described_class.enabled?(user)).to be_falsy
+          expect(described_class.enabled_for_user?(user)).to be_falsy
         end
 
         it_behaves_like 'allowed user IDs are cached'
@@ -91,7 +91,7 @@ describe Gitlab::PerformanceBar do
         end
 
         it 'returns true' do
-          expect(described_class.enabled?(user)).to be_truthy
+          expect(described_class.enabled_for_user?(user)).to be_truthy
         end
 
         it_behaves_like 'allowed user IDs are cached'
@@ -108,7 +108,7 @@ describe Gitlab::PerformanceBar do
       end
 
       it 'returns the nested group' do
-        expect(described_class.enabled?(user)).to be_truthy
+        expect(described_class.enabled_for_user?(user)).to be_truthy
       end
     end
 
@@ -118,7 +118,7 @@ describe Gitlab::PerformanceBar do
       end
 
       it 'returns false' do
-        expect(described_class.enabled?(user)).to be_falsy
+        expect(described_class.enabled_for_user?(user)).to be_falsy
       end
     end
   end
diff --git a/spec/lib/peek/views/rugged_spec.rb b/spec/lib/peek/views/rugged_spec.rb
index d07d6b51a1f..b9507f772d2 100644
--- a/spec/lib/peek/views/rugged_spec.rb
+++ b/spec/lib/peek/views/rugged_spec.rb
@@ -8,7 +8,7 @@ describe Peek::Views::Rugged, :request_store do
   let(:project) { create(:project) }
 
   before do
-    allow(Gitlab::RuggedInstrumentation).to receive(:peek_enabled?).and_return(true)
+    allow(Gitlab::PerformanceBar).to receive(:enabled_for_request?).and_return(true)
   end
 
   it 'returns no results' do