Allow to enable the Performance Bar for a group from the admin area

Signed-off-by: Rémy Coutable <remy@rymai.me>

app/controllers/admin/application_settings_controller.rb

@@ -126,6 +126,8 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
       :metrics_port,
       :metrics_sample_interval,
       :metrics_timeout,
+      :performance_bar_allowed_group_id,
+      :performance_bar_enabled,
       :recaptcha_enabled,
       :recaptcha_private_key,
       :recaptcha_site_key,

app/models/application_setting.rb

@@ -234,6 +234,7 @@ class ApplicationSetting < ActiveRecord::Base
       koding_url: nil,
       max_artifacts_size: Settings.artifacts['max_size'],
       max_attachment_size: Settings.gitlab['max_attachment_size'],
+      performance_bar_allowed_group_id: nil,
       plantuml_enabled: false,
       plantuml_url: nil,
       recaptcha_enabled: false,
@@ -336,6 +337,42 @@ class ApplicationSetting < ActiveRecord::Base
     super(levels.map { |level| Gitlab::VisibilityLevel.level_value(level) })
   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
+
+    super(group.id)
+    Gitlab::PerformanceBar.expire_allowed_user_ids_cache
+  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')
+  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
+  def performance_bar_enabled=(enable)
+    feature = Feature.get(:performance_bar)
+    performance_bar_on = performance_bar_enabled?
+
+    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
+  end
+
   # Choose one of the available repository storage options. Currently all have
   # equal weighting.
   def pick_repository_storage

app/views/admin/application_settings/_form.html.haml

@@ -332,6 +332,22 @@
                 %strong.cred WARNING:
                 Environment variable `prometheus_multiproc_dir` does not exist or is not pointing to a valid directory.
 
+  %fieldset
+    %legend Profiling - Performance Bar
+    %p
+      Enable the Performance Bar for a given group.
+      = link_to icon('question-circle'), help_page_path('administration/monitoring/performance/performance_bar')
+    .form-group
+      .col-sm-offset-2.col-sm-10
+        .checkbox
+          = f.label :performance_bar_enabled do
+            = f.check_box :performance_bar_enabled
+            Enable the Performance Bar
+    .form-group
+      = f.label :performance_bar_allowed_group_id, 'Allowed group', class: 'control-label col-sm-2'
+      .col-sm-10
+        = f.text_field :performance_bar_allowed_group_id, class: 'form-control', placeholder: 'my-org/my-group', value: @application_setting.performance_bar_allowed_group&.full_path
+
   %fieldset
     %legend Background Jobs
     %p

config/gitlab.yml.example

@@ -459,11 +459,6 @@ production: &base
     # is the normal way to deploy Gitaly.
     token:
 
-  # Performance bar settings
-  performance_bar:
-    # This setting controls what group can see the performance bar.
-    # allowed_group: my-org/performance-group
-
   #
   # 4. Advanced settings
   # ==========================

config/initializers/1_settings.rb

@@ -485,12 +485,6 @@ Settings.rack_attack.git_basic_auth['bantime'] ||= 1.hour
 Settings['gitaly'] ||= Settingslogic.new({})
 Settings.gitaly['enabled'] = true if Settings.gitaly['enabled'].nil?
 
-#
-# Performance bar
-#
-Settings['performance_bar'] ||= Settingslogic.new({})
-Settings.performance_bar['allowed_group'] ||= nil
-
 #
 # Webpack settings
 #

db/migrate/20170706151212_add_performance_bar_allowed_group_id_to_application_settings.rb

@@ -0,0 +1,9 @@
+class AddPerformanceBarAllowedGroupIdToApplicationSettings < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :application_settings, :performance_bar_allowed_group_id, :integer
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170703102400) do
+ActiveRecord::Schema.define(version: 20170706151212) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -126,6 +126,7 @@ ActiveRecord::Schema.define(version: 20170703102400) do
     t.boolean "prometheus_metrics_enabled", default: false, null: false
     t.boolean "help_page_hide_commercial_content", default: false
     t.string "help_page_support_url"
+    t.integer "performance_bar_allowed_group_id"
   end
 
   create_table "audit_events", force: :cascade do |t|

doc/administration/monitoring/performance/performance_bar.md

@@ -1,9 +1,5 @@
 # Performance Bar
 
->**Note:**
-Available since GitLab 9.4. For installations from source you'll have to
-configure it yourself.
-
 A Performance Bar can be displayed, to dig into the performance of a page. When
 activated, it looks as follows:
 
@@ -21,38 +17,44 @@ It allows you to:
 - profile the code used to generate the page, line by line
 ![Line profiling using the Performance Bar](img/performance_bar_line_profiling.png)
 
-## Enable the Performance Bar
+## Enable the Performance Bar via the Admin panel
 
-By default, the Performance Bar is disabled. You can enable it for a group
-and/or users. Note that it's possible to enable it for a group and for
-individual users at the same time.
+GitLab Performance Bar is disabled by default. To enable it for a given group,
+navigate to the Admin area in **Settings > Profiling - Performance Bar**
+(`/admin/application_settings`).
 
-1. Edit `/etc/gitlab/gitlab.rb`
-1. Find the following line, and set it to the group's **full path** that should
-be allowed to use the Performance Bar:
+The only required setting you need to set is the full path of the group that
+will be allowed to display the Performance Bar.
+Make sure _Enable the Performance Bar_ is checked and hit
+**Save** to save the changes.
 
-    ```ruby
-    gitlab_rails['performance_bar_allowed_group'] = 'your-org/your-performance-group'
-    ```
+---
 
-1. Save the file and [reconfigure GitLab][reconfigure] for the changes to
-   take effect
-1. The Performance Bar can then be enabled via the
-   [Features API](../../../api/features.md#set-or-create-a-feature) (see below).
+![GitLab Performance Bar Admin Settings](img/performance_bar_configuration_settings.png)
 
-### Enable for the `performance_team` feature group
+---
 
-The `performance_team` feature group maps to the group specified by the
-`performance_bar_allowed_group` setting you've set in the previous step.
+## 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
 ```
 
-### Enable for specific users
+### For specific users
 
-It's possible to enable the Performance Bar for specific users in addition to a
-group, or even instead of a group:
+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

lib/gitlab/performance_bar.rb

@@ -1,27 +1,29 @@
 module Gitlab
   module PerformanceBar
+    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?(:gitlab_performance_bar, current_user)
+      Feature.enabled?(:performance_bar, current_user)
     end
 
     def self.allowed_user?(user)
-      return false unless allowed_group_name
+      return false unless allowed_group_id
 
       allowed_user_ids.include?(user.id)
     end
 
-    def self.allowed_group_name
-      Gitlab.config.performance_bar.allowed_group
+    def self.allowed_group_id
+      current_application_settings.performance_bar_allowed_group_id
     end
 
     def self.allowed_user_ids
-      Rails.cache.fetch(cache_key, expires_in: ALLOWED_USER_IDS_TIME_TO_LIVE) do
-        group = Group.find_by_full_path(allowed_group_name)
+      Rails.cache.fetch(ALLOWED_USER_IDS_KEY, expires_in: ALLOWED_USER_IDS_TIME_TO_LIVE) do
+        group = Group.find_by_id(allowed_group_id)
 
         if group
           GroupMembersFinder.new(group).execute.pluck(:user_id)
@@ -31,8 +33,8 @@ module Gitlab
       end
     end
 
-    def self.cache_key
-      "#{ALLOWED_USER_IDS_KEY}:#{allowed_group_name}"
+    def self.expire_allowed_user_ids_cache
+      Rails.cache.delete(ALLOWED_USER_IDS_KEY)
     end
   end
 end

spec/features/user_can_display_performance_bar_spec.rb

@@ -38,17 +38,17 @@ describe 'User can display performance bar', :js do
       visit root_path
     end
 
-    context 'when the gitlab_performance_bar feature is disabled' do
+    context 'when the performance_bar feature is disabled' do
       before do
-        Feature.disable('gitlab_performance_bar')
+        Feature.disable(:performance_bar)
       end
 
       it_behaves_like 'performance bar is disabled'
     end
 
-    context 'when the gitlab_performance_bar feature is enabled' do
+    context 'when the performance_bar feature is enabled' do
       before do
-        Feature.enable('gitlab_performance_bar')
+        Feature.enable(:performance_bar)
       end
 
       it_behaves_like 'performance bar is disabled'
@@ -62,17 +62,17 @@ describe 'User can display performance bar', :js do
       visit root_path
     end
 
-    context 'when the gitlab_performance_bar feature is disabled' do
+    context 'when the performance_bar feature is disabled' do
       before do
-        Feature.disable('gitlab_performance_bar')
+        Feature.disable(:performance_bar)
       end
 
       it_behaves_like 'performance bar is disabled'
     end
 
-    context 'when the gitlab_performance_bar feature is enabled' do
+    context 'when the performance_bar feature is enabled' do
       before do
-        Feature.enable('gitlab_performance_bar')
+        Feature.enable(:performance_bar)
       end
 
       it_behaves_like 'performance bar is enabled'

spec/lib/gitlab/performance_bar_spec.rb

@@ -10,7 +10,7 @@ describe Gitlab::PerformanceBar do
       actor = double('actor')
 
       expect(Feature).to receive(:enabled?)
-        .with(:gitlab_performance_bar, actor).and_return(false)
+        .with(:performance_bar, actor).and_return(false)
 
       expect(described_class.enabled?(actor)).to be_falsy
     end
@@ -19,7 +19,7 @@ describe Gitlab::PerformanceBar do
       actor = double('actor')
 
       expect(Feature).to receive(:enabled?)
-        .with(:gitlab_performance_bar, actor).and_return(true)
+        .with(:performance_bar, actor).and_return(true)
 
       expect(described_class.enabled?(actor)).to be_truthy
     end

spec/models/application_setting_spec.rb

@@ -214,6 +214,151 @@ describe ApplicationSetting, models: true do
     end
   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'
+
+        expect(setting.performance_bar_allowed_group_id).to be_nil
+      end
+
+      context 'with a path to an existing group' do
+        let(:group) { create(:group) }
+
+        it 'persists a valid group path and clears allowed user IDs cache' do
+          expect(Gitlab::PerformanceBar).to receive(:expire_allowed_user_ids_cache)
+
+          setting.performance_bar_allowed_group_id = group.full_path
+
+          expect(setting.performance_bar_allowed_group_id).to eq(group.id)
+        end
+
+        context 'when the given path is the same' do
+          before do
+            setting.performance_bar_allowed_group_id = group.full_path
+          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 = group.full_path
+          end
+        end
+      end
+    end
+
+    describe 'performance_bar_allowed_group' do
+      context 'with no performance_bar_allowed_group_id saved' do
+        it 'returns nil' do
+          expect(setting.performance_bar_allowed_group).to be_nil
+        end
+      end
+
+      context 'with a performance_bar_allowed_group_id saved' do
+        let(:group) { create(:group) }
+
+        before do
+          setting.performance_bar_allowed_group_id = group.full_path
+        end
+
+        it 'returns the group' do
+          expect(setting.performance_bar_allowed_group).to eq(group)
+        end
+      end
+    end
+
+    describe 'performance_bar_enabled?' do
+      context 'with the Performance Bar is enabled globally' do
+        before do
+          Feature.enable(:performance_bar)
+        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
+        end
+      end
+    end
+
+    describe 'performance_bar_enabled=' do
+      context 'when the performance bar is enabled' do
+        before do
+          Feature.enable(:performance_bar)
+        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
+          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
+          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)
+            setting.performance_bar_enabled = true
+
+            expect(setting).to be_performance_bar_enabled
+          end
+        end
+
+        context 'when passing false' 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 = false
+
+            expect(setting).not_to be_performance_bar_enabled
+          end
+        end
+      end
+    end
+  end
+
   describe 'usage ping settings' do
     context 'when the usage ping is disabled in gitlab.yml' do
       before do