Commit b7398bc1 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'fix-peek-on-puma' into 'master'

Fix Peek on Puma

Closes #66528

See merge request gitlab-org/gitlab-ce!32213
parents d6de7c65 c2452db5
......@@ -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'
......
......@@ -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)
......
......@@ -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
......
# 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
- 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 }
---
title: Fix performance bar on Puma
merge_request: 32213
author:
type: fixed
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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,
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment