Track all used feature flags during tests execution

Gather all used feature flags by `touch'ing` file
in a `tmp/feature_flags/`.

Print as part of `rspec:coverage` that a given
feature flag is unused.

.gitlab/ci/rails.gitlab-ci.yml

@@ -34,6 +34,7 @@
       - rspec_profiling/
       - tmp/capybara/
       - tmp/memory_test/
+      - tmp/feature_flags/
       - log/*.log
     reports:
       junit: junit_rspec.xml
@@ -360,6 +361,7 @@ rspec:coverage:
     - run_timed_command "bundle install --jobs=$(nproc) --path=vendor --retry=3 --quiet --without default development test production puma unicorn kerberos metrics omnibus ed25519"
     - run_timed_command "bundle exec scripts/merge-simplecov"
     - run_timed_command "bundle exec scripts/gather-test-memory-data"
+    - run_timed_command "bundle exec scripts/used-feature-flags"
   coverage: '/LOC \((\d+\.\d+%)\) covered.$/'
   artifacts:
     name: coverage

config/feature_flags/development/approval_rule.yml

----
-name: approval_rule
-introduced_by_url: 
-rollout_issue_url: 
-group: 
-type: development
-default_enabled: true

config/feature_flags/development/release_asset_link_editing.yml

----
-name: release_asset_link_editing
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/26821
-rollout_issue_url:
-group: group::release management
-type: development
-default_enabled: true

config/feature_flags/development/release_show_page.yml

----
-name: release_show_page
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/23792
-rollout_issue_url:
-group: group::release management
-type: development
-default_enabled: true

config/feature_flags/development/sql-set-operators.yml

----
-name: sql-set-operators
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/39786
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/39786
-group: group::access
-type: development
-default_enabled: false

config/feature_flags/development/sql_set_operators.yml

 ---
 name: sql_set_operators
-introduced_by_url:
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/39786#f99799ae4964b7650b877e081b669379d71bcca8
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/39786
+rollout_issue_url:
 group: group::access
 type: development
 default_enabled: false

config/initializers/0_marginalia.rb

@@ -21,4 +21,4 @@ Gitlab::Marginalia.set_application_name
 
 Gitlab::Marginalia.enable_sidekiq_instrumentation
 
-Gitlab::Marginalia.set_feature_cache
+Gitlab::Marginalia.set_enabled_from_feature_flag

doc/development/feature_flags/controls.md

@@ -248,13 +248,26 @@ Changes to the issue format can be submitted in the
 
 ## Cleaning up
 
-Once the change is deemed stable, submit a new merge request to remove the
-feature flag. This ensures the change is available to all users and self-managed
-instances. Make sure to add the ~"feature flag" label to this merge request so
-release managers are aware the changes are hidden behind a feature flag. If the
-merge request has to be picked into a stable branch, make sure to also add the
-appropriate `~"Pick into X.Y"` label (e.g. `~"Pick into 13.0"`).
-See [the process document](process.md#including-a-feature-behind-feature-flag-in-the-final-release) for further details.
+A feature flag should be removed as soon as it is no longer needed. Each additional
+feature flag in the codebase increases the complexity of the application
+and reduces confidence in our testing suite covering all possible combinations.
+Additionally, a feature flag overwritten in some of the environments can result
+in undefined and untested system behavior.
+
+To remove a feature flag:
+
+1. Open a new merge request with the ~"feature flag" label so
+   release managers are aware the changes are hidden behind a feature flag.
+1. If the merge request has to be picked into a stable branch, add the
+   appropriate `~"Pick into X.Y"` label, for example `~"Pick into 13.0"`.
+   See [the feature flag process](process.md#including-a-feature-behind-feature-flag-in-the-final-release)
+   for further details.
+1. Remove all references to the feature flag from the codebase.
+1. Remove the YAML definition for the feature from the repository.
+1. Clean up the feature flag from all environments with `/chatops run feature delete some_feature`.
+1. Close the rollout issue for the feature flag after the feature flag is removed from the codebase.
+
+### Cleanup ChatOps
 
 When a feature gate has been removed from the code base, the feature
 record still exists in the database that the flag was deployed too.

doc/development/feature_flags/development.md

@@ -153,6 +153,11 @@ default_enabled: false
 TIP: **Tip:**
 To create a feature flag that is only used in EE, add the `--ee` flag: `bin/feature-flag --ee`
 
+## Delete a feature flag
+
+See [cleaning up feature flags](controls.md#cleaning-up) for more information about
+deleting feature flags.
+
 ## Develop with a feature flag
 
 There are two main ways of using Feature Flags in the GitLab codebase:

ee/config/feature_flags/development/sectional_codeowners.yml

----
-name: sectional_codeowners
-introduced_by_url: 
-rollout_issue_url: 
-group: 
-type: development
-default_enabled: true

ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress.rb

@@ -12,8 +12,6 @@ module EE
         def perform
           if remaining?
             ::BackgroundMigrationWorker.perform_in(RESCHEDULE_DELAY, self.class.name)
-          else
-            ::Feature.enable(:approval_rule)
           end
         end
 

ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress_spec.rb

@@ -22,8 +22,6 @@ RSpec.describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRulesCheckP
       allow(Gitlab::BackgroundMigration).to receive(:exists?).with('MigrateApproverToApprovalRulesInBatch').and_return(false)
 
       described_class.new.perform
-
-      expect(Feature.enabled?(:approval_rule, default_enabled: true)).to eq(true)
     end
   end
 end

lib/gitlab/marginalia.rb

@@ -4,8 +4,6 @@ module Gitlab
   module Marginalia
     cattr_accessor :enabled, default: false
 
-    MARGINALIA_FEATURE_FLAG = :marginalia
-
     def self.set_application_name
       ::Marginalia.application_name = Gitlab.process_name
     end
@@ -16,15 +14,11 @@ module Gitlab
       end
     end
 
-    def self.cached_feature_enabled?
-      enabled
-    end
-
-    def self.set_feature_cache
+    def self.set_enabled_from_feature_flag
       # During db:create and db:bootstrap skip feature query as DB is not available yet.
       return false unless Gitlab::Database.cached_table_exists?('features')
 
-      self.enabled = Feature.enabled?(MARGINALIA_FEATURE_FLAG)
+      self.enabled = Feature.enabled?(:marginalia)
     end
   end
 end

lib/gitlab/marginalia/active_record_instrumentation.rb

@@ -5,7 +5,7 @@ module Gitlab
   module Marginalia
     module ActiveRecordInstrumentation
       def annotate_sql(sql)
-        Gitlab::Marginalia.cached_feature_enabled? ? super(sql) : sql
+        Gitlab::Marginalia.enabled ? super(sql) : sql
       end
     end
   end

scripts/used-feature-flags

+#!/usr/bin/env ruby
+
+class String
+  def red
+    "\e[31m#{self}\e[0m"
+  end
+
+  def yellow
+    "\e[33m#{self}\e[0m"
+  end
+
+  def green
+    "\e[32m#{self}\e[0m"
+  end
+
+  def bold
+    "\e[1m#{self}\e[0m"
+  end
+end
+
+flags_paths = [
+  'config/feature_flags/**/*.yml'
+]
+
+# For EE additionally process `ee/` feature flags
+if File.exist?('ee/app/models/license.rb') && !%w[true 1].include?(ENV['FOSS_ONLY'].to_s)
+  flags_paths << 'ee/config/feature_flags/**/*.yml'
+end
+
+all_flags = {}
+additional_flags = Set.new
+
+# Iterate all defined feature flags
+# to discover which were used
+flags_paths.each do |flags_path|
+  puts flags_path
+  Dir.glob(flags_path).each do |path|
+    feature_flag_name = File.basename(path, '.yml')
+
+    all_flags[feature_flag_name] = File.exist?(File.join('tmp', 'feature_flags', feature_flag_name + '.used'))
+  end
+end
+
+# Iterate all used feature flags
+# to discover which flags are undefined
+Dir.glob('tmp/feature_flags/*.used').each do |path|
+  feature_flag_name = File.basename(path, '.used')
+
+  additional_flags.add(feature_flag_name) unless all_flags[feature_flag_name]
+end
+
+used_flags = all_flags.select { |name, used| used }
+unused_flags = all_flags.reject { |name, used| used }
+
+puts "=========================================".green.bold
+puts "Feature Flags usage summary:".green.bold
+puts
+
+puts "- #{all_flags.count + additional_flags.count} was found"
+puts "- #{unused_flags.count} appear(s) to be UNUSED".yellow
+puts "- #{additional_flags.count} appear(s) to be unknown".yellow
+puts "- #{used_flags.count} appear(s) to be used".green
+puts
+
+if additional_flags.count > 0
+  puts "==================================================".green.bold
+  puts "There are feature flags that appears to be unknown".yellow
+  puts
+  puts "They appear to be used by CI, but we do lack their YAML definition".yellow
+  puts "This is likely expected, so feel free to ignore that list:".yellow
+  puts
+  additional_flags.sort.each do |name|
+    puts "- #{name}".yellow
+  end
+  puts
+end
+
+if unused_flags.count > 0
+  puts "========================================".green.bold
+  puts "These feature flags appears to be UNUSED".red.bold
+  puts
+  puts "If they are really no longer needed REMOVE their .yml definition".red
+  puts "If they are needed you need to ENSURE that their usage is covered with specs to continue.".red
+  puts
+  unused_flags.keys.sort.each do |name|
+    puts "- #{name}".yellow
+  end
+  puts
+  puts "Feature flag usage check failed.".red.bold
+  exit(1)
+end
+
+puts "Everything is fine here!".green
+puts

spec/controllers/concerns/redis_tracking_spec.rb

@@ -6,6 +6,10 @@ RSpec.describe RedisTracking do
   let(:feature) { 'approval_rule' }
   let(:user) { create(:user) }
 
+  before do
+    skip_feature_flags_yaml_validation
+  end
+
   controller(ApplicationController) do
     include RedisTracking
 

spec/initializers/sidekiq_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'sidekiq' do
+  describe 'enable_reliable_fetch?' do
+    subject { enable_reliable_fetch? }
+
+    context 'when gitlab_sidekiq_reliable_fetcher is enabled' do
+      before do
+        stub_feature_flags(gitlab_sidekiq_reliable_fetcher: true)
+      end
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when gitlab_sidekiq_reliable_fetcher is disabled' do
+      before do
+        stub_feature_flags(gitlab_sidekiq_reliable_fetcher: false)
+      end
+
+      it { is_expected.to be_falsey }
+    end
+  end
+
+  describe 'enable_semi_reliable_fetch_mode?' do
+    subject { enable_semi_reliable_fetch_mode? }
+
+    context 'when gitlab_sidekiq_enable_semi_reliable_fetcher is enabled' do
+      before do
+        stub_feature_flags(gitlab_sidekiq_enable_semi_reliable_fetcher: true)
+      end
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when gitlab_sidekiq_enable_semi_reliable_fetcher is disabled' do
+      before do
+        stub_feature_flags(gitlab_sidekiq_enable_semi_reliable_fetcher: false)
+      end
+
+      it { is_expected.to be_falsey }
+    end
+  end
+end

spec/lib/marginalia_spec.rb

@@ -24,18 +24,6 @@ RSpec.describe 'Marginalia spec' do
     end
   end
 
-  def stub_feature(value)
-    allow(Gitlab::Marginalia).to receive(:cached_feature_enabled?).and_return(value)
-  end
-
-  def make_request(correlation_id)
-    request_env = Rack::MockRequest.env_for('/')
-
-    ::Labkit::Correlation::CorrelationId.use_id(correlation_id) do
-      MarginaliaTestController.action(:first_user).call(request_env)
-    end
-  end
-
   describe 'For rails web requests' do
     let(:correlation_id) { SecureRandom.uuid }
     let(:recorded) { ActiveRecord::QueryRecorder.new { make_request(correlation_id) } }
@@ -149,4 +137,17 @@ RSpec.describe 'Marginalia spec' do
       end
     end
   end
+
+  def stub_feature(value)
+    stub_feature_flags(marginalia: value)
+    Gitlab::Marginalia.set_enabled_from_feature_flag
+  end
+
+  def make_request(correlation_id)
+    request_env = Rack::MockRequest.env_for('/')
+
+    ::Labkit::Correlation::CorrelationId.use_id(correlation_id) do
+      MarginaliaTestController.action(:first_user).call(request_env)
+    end
+  end
 end

spec/spec_helper.rb

@@ -229,7 +229,7 @@ RSpec.configure do |config|
     end
 
     # Enable Marginalia feature for all specs in the test suite.
-    allow(Gitlab::Marginalia).to receive(:cached_feature_enabled?).and_return(true)
+    Gitlab::Marginalia.enabled = true
 
     # Stub these calls due to being expensive operations
     # It can be reenabled for specific tests via:

spec/support/helpers/stubbed_feature.rb

@@ -4,6 +4,14 @@
 module StubbedFeature
   extend ActiveSupport::Concern
 
+  prepended do
+    cattr_reader(:persist_used) do
+      # persist feature flags in CI
+      # nil: indicates that we do not want to persist used feature flags
+      Gitlab::Utils.to_boolean(ENV['CI']) ? {} : nil
+    end
+  end
+
   class_methods do
     # Turn stubbed feature flags on or off.
     def stub=(stub)
@@ -33,6 +41,8 @@ module StubbedFeature
       feature_flag = super
       return feature_flag unless stub?
 
+      persist_used!(args.first)
+
       # If feature flag is not persisted we mark the feature flag as enabled
       # We do `m.call` as we want to validate the execution of method arguments
       # and a feature flag state if it is not persisted
@@ -42,5 +52,17 @@ module StubbedFeature
 
       feature_flag
     end
+
+    # This method creates a temporary file in `tmp/feature_flags`
+    # if feature flag was touched during execution
+    def persist_used!(name)
+      return unless persist_used
+      return if persist_used[name]
+
+      persist_used[name] = true
+      FileUtils.touch(
+        Rails.root.join('tmp', 'feature_flags', name.to_s + ".used")
+      )
+    end
   end
 end