kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Don't use Flipper for the Performance Bar

Browse source 
The implementation now simply rely on the
`performance_bar_allowed_group_id` Application Setting.

Signed-off-by: Rémy Coutable <remy@rymai.me>
This commit is contained in:
Rémy Coutable 2017-07-07 02:34:51 +02:00
parent 040eeb1039
commit 97611c88fc
No known key found for this signature in database
GPG key ID: 46DF07E5CD9E96AB
9 changed files with 122 additions and 167 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

50
app/models/application_setting.rb
View file

@ -338,39 +338,45 @@ class ApplicationSetting < ActiveRecord::Base
end
def performance_bar_allowed_group_id=(group_full_path)
group = Group.find_by_full_path(group_full_path)
return unless group && group.id != performance_bar_allowed_group_id
group_full_path = nil if group_full_path.blank?
super(group.id)
Gitlab::PerformanceBar.expire_allowed_user_ids_cache
if group_full_path.nil?
if group_full_path != performance_bar_allowed_group_id
super(group_full_path)
Gitlab::PerformanceBar.expire_allowed_user_ids_cache
end
return
end
group = Group.find_by_full_path(group_full_path)
if group
if group.id != performance_bar_allowed_group_id
super(group.id)
Gitlab::PerformanceBar.expire_allowed_user_ids_cache
end
else
super(nil)
Gitlab::PerformanceBar.expire_allowed_user_ids_cache
end
end
def performance_bar_allowed_group
Group.find_by_id(performance_bar_allowed_group_id)
end
# Return true is the Performance Bar is available globally or for the
# `performance_team` feature group
def performance_bar_enabled?
feature = Feature.get(:performance_bar)
feature.on? || feature.groups_value.include?('performance_team')
# Return true if the Performance Bar is enabled for a given group
def performance_bar_enabled
performance_bar_allowed_group_id.present?
end
# - If `enable` is true, enable the `performance_bar` feature for the
# `performance_team` feature group
# - If `enable` is false, disable the `performance_bar` feature globally
# - If `enable` is true, we early return since the actual attribute that holds
# the enabling/disabling is `performance_bar_allowed_group_id`
# - If `enable` is false, we set `performance_bar_allowed_group_id` to `nil`
def performance_bar_enabled=(enable)
feature = Feature.get(:performance_bar)
performance_bar_on = performance_bar_enabled?
return if enable
if enable && !performance_bar_on
feature.enable_group(:performance_team)
Gitlab::PerformanceBar.expire_allowed_user_ids_cache
elsif !enable && performance_bar_on
feature.disable
Gitlab::PerformanceBar.expire_allowed_user_ids_cache
end
self.performance_bar_allowed_group_id = nil
end
# Choose one of the available repository storage options. Currently all have

2
config/initializers/flipper.rb
View file

@ -3,6 +3,4 @@ require 'flipper/middleware/memoizer'
unless Rails.env.test?
Rails.application.config.middleware.use Flipper::Middleware::Memoizer,
lambda { Feature.flipper }
Feature.register_feature_groups
end

28
doc/administration/monitoring/performance/performance_bar.md
View file

@ -33,31 +33,3 @@ Make sure _Enable the Performance Bar_ is checked and hit
![GitLab Performance Bar Admin Settings](img/performance_bar_configuration_settings.png)
---
## Enable the Performance Bar via the API
Under the hood, the Performance Bar activation is done via the `performance_bar`
[Feature Flag](../../../development/features_flags.md).
That means you can also enable or disable it via the
[Features API](../../../api/features.md#set-or-create-a-feature).
### For the `performance_team` feature group
The `performance_team` feature group maps to the group specified in your [Admin
area](#enable-the-performance-bar-via-the-admin-panel).
```
curl --data "feature_group=performance_team" --data "value=true" --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/features/performance_bar
```
### For specific users
It's also possible to enable the Performance Bar for specific users in addition
to a group, or even instead of a group:
```
curl --data "user=my_username" --data "value=true" --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/features/performance_bar
```
[reconfigure]: ../../restart_gitlab.md#omnibus-gitlab-reconfigure

3
doc/development/feature_flags.md
View file

@ -15,8 +15,7 @@ Starting from GitLab 9.4 we support feature groups via
Feature groups must be defined statically in `lib/feature.rb` (in the
`.register_feature_groups` method), but their implementation can obviously be
dynamic (querying the DB etc.). You can see how the `performance_team` feature
group for a concrete example.
dynamic (querying the DB etc.).
Once defined in `lib/feature.rb`, you will be able to activate a
feature for a given feature group via the [`feature_group` param of the features API](../api/features.md#set-or-create-a-feature)

8
lib/feature.rb
View file

@ -57,13 +57,5 @@ class Feature
Flipper.new(adapter)
end
end
def register_feature_groups
Flipper.register(:performance_team) do |actor|
user = actor.thing
user&.is_a?(User) && Gitlab::PerformanceBar.allowed_user?(user)
end
end
end
end

13
lib/gitlab/performance_bar.rb
View file

@ -3,16 +3,9 @@ module Gitlab
include Gitlab::CurrentSettings
ALLOWED_USER_IDS_KEY = 'performance_bar_allowed_user_ids'.freeze
# The time (in seconds) after which a set of allowed user IDs is expired
# automatically.
ALLOWED_USER_IDS_TIME_TO_LIVE = 10.minutes
def self.enabled?(current_user = nil)
Feature.enabled?(:performance_bar, current_user)
end
def self.allowed_user?(user)
return false unless allowed_group_id
def self.enabled?(user = nil)
return false unless user && allowed_group_id
allowed_user_ids.include?(user.id)
end
@ -22,7 +15,7 @@ module Gitlab
end
def self.allowed_user_ids
Rails.cache.fetch(ALLOWED_USER_IDS_KEY, expires_in: ALLOWED_USER_IDS_TIME_TO_LIVE) do
Rails.cache.fetch(ALLOWED_USER_IDS_KEY) do
group = Group.find_by_id(allowed_group_id)
if group

66
spec/lib/gitlab/performance_bar_spec.rb
View file

@ -1,65 +1,55 @@
require 'spec_helper'
describe Gitlab::PerformanceBar do
describe '.enabled?' do
it 'returns false when given actor is nil' do
expect(described_class.enabled?(nil)).to be_falsy
end
it 'returns false when feature is disabled' do
actor = double('actor')
expect(Feature).to receive(:enabled?)
.with(:performance_bar, actor).and_return(false)
expect(described_class.enabled?(actor)).to be_falsy
end
it 'returns true when feature is enabled' do
actor = double('actor')
expect(Feature).to receive(:enabled?)
.with(:performance_bar, actor).and_return(true)
expect(described_class.enabled?(actor)).to be_truthy
end
end
shared_examples 'allowed user IDs are cached in Redis for 10 minutes' do
shared_examples 'allowed user IDs are cached' do
before do
# Warm the Redis cache
described_class.allowed_user?(user)
described_class.enabled?(user)
end
it 'caches the allowed user IDs in cache', :caching do
expect do
expect(described_class.allowed_user?(user)).to be_truthy
expect(described_class.enabled?(user)).to be_truthy
end.not_to exceed_query_limit(0)
end
end
describe '.allowed_user?' do
describe '.enabled?' do
let(:user) { create(:user) }
before do
stub_performance_bar_setting(allowed_group: 'my-group')
stub_application_setting(performance_bar_allowed_group_id: -1)
end
context 'when allowed group does not exist' do
it 'returns false when given user is nil' do
expect(described_class.enabled?(nil)).to be_falsy
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
end
context 'when allowed group ID does not exist' do
it 'returns false' do
expect(described_class.allowed_user?(user)).to be_falsy
expect(described_class.enabled?(user)).to be_falsy
end
end
context 'when allowed group exists' do
let!(:my_group) { create(:group, path: 'my-group') }
before do
stub_application_setting(performance_bar_allowed_group_id: my_group.id)
end
context 'when user is not a member of the allowed group' do
it 'returns false' do
expect(described_class.allowed_user?(user)).to be_falsy
expect(described_class.enabled?(user)).to be_falsy
end
it_behaves_like 'allowed user IDs are cached in Redis for 10 minutes'
it_behaves_like 'allowed user IDs are cached'
end
context 'when user is a member of the allowed group' do
@ -68,10 +58,10 @@ describe Gitlab::PerformanceBar do
end
it 'returns true' do
expect(described_class.allowed_user?(user)).to be_truthy
expect(described_class.enabled?(user)).to be_truthy
end
it_behaves_like 'allowed user IDs are cached in Redis for 10 minutes'
it_behaves_like 'allowed user IDs are cached'
end
end
@ -81,11 +71,11 @@ describe Gitlab::PerformanceBar do
before do
create(:group, path: 'my-group')
nested_my_group.add_developer(user)
stub_performance_bar_setting(allowed_group: 'my-org/my-group')
stub_application_setting(performance_bar_allowed_group_id: nested_my_group.id)
end
it 'returns the nested group' do
expect(described_class.allowed_user?(user)).to be_truthy
expect(described_class.enabled?(user)).to be_truthy
end
end
@ -95,7 +85,7 @@ describe Gitlab::PerformanceBar do
end
it 'returns false' do
expect(described_class.allowed_user?(user)).to be_falsy
expect(described_class.enabled?(user)).to be_falsy
end
end
end

115
spec/models/application_setting_spec.rb
View file

@ -215,20 +215,27 @@ describe ApplicationSetting, models: true do
end
describe 'performance bar settings' do
before do
Flipper.unregister_groups
Flipper.register(:performance_team)
end
after do
Flipper.unregister_groups
end
describe 'performance_bar_allowed_group_id=' do
it 'does not persist an invalid group path' do
setting.performance_bar_allowed_group_id = 'foo'
context 'with a blank path' do
before do
setting.performance_bar_allowed_group_id = create(:group).full_path
end
expect(setting.performance_bar_allowed_group_id).to be_nil
it 'persists nil for a "" path and clears allowed user IDs cache' do
expect(Gitlab::PerformanceBar).to receive(:expire_allowed_user_ids_cache)
setting.performance_bar_allowed_group_id = ''
expect(setting.performance_bar_allowed_group_id).to be_nil
end
end
context 'with an invalid path' do
it 'does not persist an invalid group path' do
setting.performance_bar_allowed_group_id = 'foo'
expect(setting.performance_bar_allowed_group_id).to be_nil
end
end
context 'with a path to an existing group' do
@ -243,14 +250,28 @@ describe ApplicationSetting, models: true do
end
context 'when the given path is the same' do
before do
setting.performance_bar_allowed_group_id = group.full_path
context 'with a blank path' do
before do
setting.performance_bar_allowed_group_id = nil
end
it 'clears the cached allowed user IDs' do
expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
setting.performance_bar_allowed_group_id = ''
end
end
it 'clears the cached allowed user IDs' do
expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
context 'with a valid path' do
before do
setting.performance_bar_allowed_group_id = group.full_path
end
setting.performance_bar_allowed_group_id = group.full_path
it 'clears the cached allowed user IDs' do
expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
setting.performance_bar_allowed_group_id = group.full_path
end
end
end
end
@ -276,83 +297,71 @@ describe ApplicationSetting, models: true do
end
end
describe 'performance_bar_enabled?' do
context 'with the Performance Bar is enabled globally' do
describe 'performance_bar_enabled' do
context 'with the Performance Bar is enabled' do
let(:group) { create(:group) }
before do
Feature.enable(:performance_bar)
setting.performance_bar_allowed_group_id = group.full_path
end
it 'returns true' do
expect(setting).to be_performance_bar_enabled
end
end
context 'with the Performance Bar is enabled for the performance_team group' do
before do
Feature.enable_group(:performance_bar, :performance_team)
end
it 'returns true' do
expect(setting).to be_performance_bar_enabled
end
end
context 'with the Performance Bar is enabled for a specific user' do
before do
Feature.enable(:performance_team, create(:user))
end
it 'returns false' do
expect(setting).not_to be_performance_bar_enabled
expect(setting.performance_bar_enabled).to be_truthy
end
end
end
describe 'performance_bar_enabled=' do
context 'when the performance bar is enabled' do
let(:group) { create(:group) }
before do
Feature.enable(:performance_bar)
setting.performance_bar_allowed_group_id = group.full_path
end
context 'when passing true' do
it 'does not clear allowed user IDs cache' do
expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
setting.performance_bar_enabled = true
expect(setting).to be_performance_bar_enabled
expect(setting.performance_bar_allowed_group_id).to eq(group.id)
expect(setting.performance_bar_enabled).to be_truthy
end
end
context 'when passing false' do
it 'disables the performance bar and clears allowed user IDs cache' do
expect(Gitlab::PerformanceBar).to receive(:expire_allowed_user_ids_cache)
setting.performance_bar_enabled = false
expect(setting).not_to be_performance_bar_enabled
expect(setting.performance_bar_allowed_group_id).to be_nil
expect(setting.performance_bar_enabled).to be_falsey
end
end
end
context 'when the performance bar is disabled' do
before do
Feature.disable(:performance_bar)
end
context 'when passing true' do
it 'enables the performance bar and clears allowed user IDs cache' do
expect(Gitlab::PerformanceBar).to receive(:expire_allowed_user_ids_cache)
it 'does nothing and does not clear allowed user IDs cache' do
expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
setting.performance_bar_enabled = true
expect(setting).to be_performance_bar_enabled
expect(setting.performance_bar_allowed_group_id).to be_nil
expect(setting.performance_bar_enabled).to be_falsey
end
end
context 'when passing false' do
it 'does not clear allowed user IDs cache' do
it 'does nothing and does not clear allowed user IDs cache' do
expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
setting.performance_bar_enabled = false
expect(setting).not_to be_performance_bar_enabled
expect(setting.performance_bar_allowed_group_id).to be_nil
expect(setting.performance_bar_enabled).to be_falsey
end
end
end

4
spec/support/stub_configuration.rb
View file

@ -29,10 +29,6 @@ module StubConfiguration
allow(Gitlab.config.omniauth).to receive_messages(messages)
end
def stub_performance_bar_setting(messages)
allow(Gitlab.config.performance_bar).to receive_messages(messages)
end
private
# Modifies stubbed messages to also stub possible predicate versions