From 97611c88fcbae6b025750e6ebf2061a3d87d9753 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Fri, 7 Jul 2017 02:34:51 +0200
Subject: [PATCH] Don't use Flipper for the Performance Bar
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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

Signed-off-by: Rémy Coutable <remy@rymai.me>
---
 app/models/application_setting.rb             |  50 ++++----
 config/initializers/flipper.rb                |   2 -
 .../monitoring/performance/performance_bar.md |  28 -----
 doc/development/feature_flags.md              |   3 +-
 lib/feature.rb                                |   8 --
 lib/gitlab/performance_bar.rb                 |  13 +-
 spec/lib/gitlab/performance_bar_spec.rb       |  66 +++++-----
 spec/models/application_setting_spec.rb       | 115 ++++++++++--------
 spec/support/stub_configuration.rb            |   4 -
 9 files changed, 122 insertions(+), 167 deletions(-)

diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index c6d8e45c86d..4f9c7235a30 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -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
diff --git a/config/initializers/flipper.rb b/config/initializers/flipper.rb
index bfab8c77a4b..8ec9613a4b7 100644
--- a/config/initializers/flipper.rb
+++ b/config/initializers/flipper.rb
@@ -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
diff --git a/doc/administration/monitoring/performance/performance_bar.md b/doc/administration/monitoring/performance/performance_bar.md
index 6ee21eae194..ee680c7b258 100644
--- a/doc/administration/monitoring/performance/performance_bar.md
+++ b/doc/administration/monitoring/performance/performance_bar.md
@@ -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
diff --git a/doc/development/feature_flags.md b/doc/development/feature_flags.md
index a51adcfbd3f..59e8a087e02 100644
--- a/doc/development/feature_flags.md
+++ b/doc/development/feature_flags.md
@@ -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)
diff --git a/lib/feature.rb b/lib/feature.rb
index c4e1741df52..363f66ba60e 100644
--- a/lib/feature.rb
+++ b/lib/feature.rb
@@ -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
diff --git a/lib/gitlab/performance_bar.rb b/lib/gitlab/performance_bar.rb
index aa033571de0..2da2ce45ebc 100644
--- a/lib/gitlab/performance_bar.rb
+++ b/lib/gitlab/performance_bar.rb
@@ -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
diff --git a/spec/lib/gitlab/performance_bar_spec.rb b/spec/lib/gitlab/performance_bar_spec.rb
index e77a1ebbb0d..8a586bdbf63 100644
--- a/spec/lib/gitlab/performance_bar_spec.rb
+++ b/spec/lib/gitlab/performance_bar_spec.rb
@@ -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
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index 4b7281d593a..fb485d0b2c6 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -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
diff --git a/spec/support/stub_configuration.rb b/spec/support/stub_configuration.rb
index 1d9946b0ed1..48f454c7187 100644
--- a/spec/support/stub_configuration.rb
+++ b/spec/support/stub_configuration.rb
@@ -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