diff options
author | Rémy Coutable <remy@rymai.me> | 2019-09-02 11:43:21 +0300 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2019-09-02 11:43:21 +0300 |
commit | b7398bc1fdad6c721cfa35ff17cebc2cb00c8240 (patch) | |
tree | 48592d3a9f6d495b8b47b31c60dfa6eb17a00bb2 | |
parent | d6de7c65500b73f2e7f899e0c0a224aafca3d4d1 (diff) | |
parent | c2452db59cf045f01244b8d72e14cb0144d19722 (diff) |
Merge branch 'fix-peek-on-puma' into 'master'
Fix Peek on Puma
Closes #66528
See merge request gitlab-org/gitlab-ce!32213
-rw-r--r-- | Gemfile | 3 | ||||
-rw-r--r-- | Gemfile.lock | 10 | ||||
-rw-r--r-- | app/controllers/concerns/with_performance_bar.rb | 16 | ||||
-rw-r--r-- | app/helpers/performance_bar_helper.rb | 4 | ||||
-rw-r--r-- | app/views/peek/_bar.html.haml | 4 | ||||
-rw-r--r-- | changelogs/unreleased/fix-peek-on-puma.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/gitaly_client.rb | 8 | ||||
-rw-r--r-- | lib/gitlab/performance_bar.rb | 6 | ||||
-rw-r--r-- | lib/gitlab/performance_bar/redis_adapter_when_peek_enabled.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/rugged_instrumentation.rb | 8 | ||||
-rw-r--r-- | lib/peek/views/active_record.rb | 2 | ||||
-rw-r--r-- | lib/peek/views/redis_detailed.rb | 6 | ||||
-rw-r--r-- | spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/performance_bar_spec.rb | 26 | ||||
-rw-r--r-- | spec/lib/peek/views/rugged_spec.rb | 2 |
15 files changed, 54 insertions, 52 deletions
@@ -295,7 +295,8 @@ gem 'gettext', '~> 3.2.2', require: false, group: :development gem 'batch-loader', '~> 1.4.0' # Perf bar -gem 'peek', '~> 1.0.1' +# https://gitlab.com/gitlab-org/gitlab-ee/issues/13996 +gem 'gitlab-peek', '~> 0.0.1', require: 'peek' # Snowplow events tracking gem 'snowplow-tracker', '~> 0.6.1' diff --git a/Gemfile.lock b/Gemfile.lock index ea2c44a2992..0803cf7f752 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -149,8 +149,6 @@ GEM adamantium (~> 0.2.0) equalizer (~> 0.0.9) concurrent-ruby (1.1.5) - concurrent-ruby-ext (1.1.5) - concurrent-ruby (= 1.1.5) connection_pool (2.2.2) contracts (0.11.0) crack (0.4.3) @@ -321,6 +319,8 @@ GEM opentracing (~> 0.4) redis (> 3.0.0, < 5.0.0) gitlab-markup (1.7.0) + gitlab-peek (0.0.1) + railties (>= 4.0.0) gitlab-sidekiq-fetcher (0.5.1) sidekiq (~> 5) gitlab-styles (2.8.0) @@ -635,10 +635,6 @@ GEM parser (2.6.3.0) ast (~> 2.4.0) parslet (1.8.2) - peek (1.0.1) - concurrent-ruby (>= 0.9.0) - concurrent-ruby-ext (>= 0.9.0) - railties (>= 4.0.0) pg (1.1.4) po_to_json (1.0.1) json (>= 1.6.0) @@ -1100,6 +1096,7 @@ DEPENDENCIES github-markup (~> 1.7.0) gitlab-labkit (~> 0.5) gitlab-markup (~> 1.7.0) + gitlab-peek (~> 0.0.1) gitlab-sidekiq-fetcher (= 0.5.1) gitlab-styles (~> 2.7) gitlab_omniauth-ldap (~> 2.1.1) @@ -1170,7 +1167,6 @@ DEPENDENCIES omniauth_crowd (~> 2.2.0) omniauth_openid_connect (~> 0.3.1) org-ruby (~> 0.9.12) - peek (~> 1.0.1) pg (~> 1.1) premailer-rails (~> 1.9.7) prometheus-client-mmap (~> 0.9.8) diff --git a/app/controllers/concerns/with_performance_bar.rb b/app/controllers/concerns/with_performance_bar.rb index 4e0ae3c59eb..93ded59900d 100644 --- a/app/controllers/concerns/with_performance_bar.rb +++ b/app/controllers/concerns/with_performance_bar.rb @@ -3,15 +3,25 @@ module WithPerformanceBar extend ActiveSupport::Concern - def peek_enabled? - return false unless Gitlab::PerformanceBar.enabled?(current_user) + included do + before_action :set_peek_enabled_for_current_request + end + + 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 5228930293c..9725f640be9 100644 --- a/app/views/peek/_bar.html.haml +++ b/app/views/peek/_bar.html.haml @@ -1,6 +1,6 @@ -- return unless peek_enabled? +- return unless performance_bar_enabled? #js-peek{ data: { env: Peek.env, - request_id: Peek.request_id, + request_id: peek_request_id, peek_url: "#{peek_routes_path}/results" }, class: Peek.env } diff --git a/changelogs/unreleased/fix-peek-on-puma.yml b/changelogs/unreleased/fix-peek-on-puma.yml new file mode 100644 index 00000000000..1071607b628 --- /dev/null +++ b/changelogs/unreleased/fix-peek-on-puma.yml @@ -0,0 +1,5 @@ +--- +title: Fix performance bar on Puma +merge_request: 32213 +author: +type: fixed diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index d65c0d3e78d..2ac99b1ff02 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 2d997760c46..cddd4f18cc3 100644 --- a/lib/gitlab/performance_bar/redis_adapter_when_peek_enabled.rb +++ b/lib/gitlab/performance_bar/redis_adapter_when_peek_enabled.rb @@ -4,8 +4,8 @@ module Gitlab module PerformanceBar module RedisAdapterWhenPeekEnabled - def save - super unless ::Peek.request_id.blank? + def save(request_id) + 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 a35783c1971..bbc9f11e90f 100644 --- a/lib/peek/views/active_record.rb +++ b/lib/peek/views/active_record.rb @@ -27,7 +27,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 |