Logging only feature flags for current or future milestones

Logging feature flags increase the size of an event payload by 25%.
With this commit, we avoid logging stale feature flags, which should
reduce the size of the payload, allowing for more events to be logged.

ee/spec/lib/ee/api/entities/experiment_spec.rb

@@ -18,7 +18,8 @@ RSpec.describe EE::API::Entities::Experiment do
         milestone: '13.7',
         type: 'experiment',
         group: 'group::adoption',
-        default_enabled: false
+        default_enabled: false,
+        log_state_changes: nil
       },
       current_status: {
         state: :off,

ee/spec/requests/api/experiments_spec.rb

@@ -63,7 +63,8 @@ RSpec.describe API::Experiments do
               milestone: '13.7',
               type: 'experiment',
               group: 'group::adoption',
-              default_enabled: false
+              default_enabled: false,
+              log_state_changes: nil
             },
             current_status: {
               state: :off,

lib/feature.rb

@@ -159,7 +159,7 @@ class Feature
     end
 
     def log_feature_flag_states?(key)
-      key != :feature_flag_state_logs && Feature.enabled?(:feature_flag_state_logs, type: :ops)
+      Feature::Definition.log_states?(key)
     end
 
     def log_feature_flag_state(key, feature_value)

lib/feature/definition.rb

@@ -82,6 +82,16 @@ class Feature
       attributes
     end
 
+    def for_upcoming_milestone?
+      return false unless milestone
+
+      Gitlab::VersionInfo.parse(milestone + '.999') >= Gitlab.version_info
+    end
+
+    def force_log_state_changes?
+      attributes[:log_state_changes]
+    end
+
     class << self
       def paths
         @paths ||= [Rails.root.join('config', 'feature_flags', '**', '*.yml')]
@@ -106,6 +116,14 @@ class Feature
         definitions.has_key?(key.to_sym)
       end
 
+      def log_states?(key)
+        return false if key == :feature_flag_state_logs
+        return false if Feature.disabled?(:feature_flag_state_logs, type: :ops)
+        return false unless (feature = get(key))
+
+        feature.force_log_state_changes? || feature.for_upcoming_milestone?
+      end
+
       def valid_usage!(key, type:, default_enabled:)
         if definition = get(key)
           definition.valid_usage!(type_in_code: type, default_enabled_in_code: default_enabled)

lib/feature/shared.rb

@@ -58,6 +58,7 @@ class Feature
       introduced_by_url
       rollout_issue_url
       milestone
+      log_state_changes
       type
       group
       default_enabled

spec/lib/feature/definition_spec.rb

@@ -161,6 +161,41 @@ RSpec.describe Feature::Definition do
     end
   end
 
+  describe '.for_upcoming_milestone?' do
+    using RSpec::Parameterized::TableSyntax
+
+    let(:definition) do
+      Feature::Definition.new("development/enabled_feature_flag.yml",
+                              name: :enabled_feature_flag,
+                              type: 'development',
+                              milestone: milestone,
+                              default_enabled: false)
+    end
+
+    before do
+      allow(Feature::Definition).to receive(:definitions) do
+        { definition.key => definition }
+      end
+
+      allow(Gitlab).to receive(:version_info).and_return(Gitlab::VersionInfo.parse(current_milestone))
+    end
+
+    subject { definition.for_upcoming_milestone? }
+
+    where(:ctx, :milestone, :current_milestone, :expected) do
+      'no milestone'               | nil     | '1.0.0'   | false
+      'upcoming milestone - major' | '2.3'   | '1.9.999' | true
+      'upcoming milestone - minor' | '2.3'   | '2.2.999' | true
+      'current milestone'          | '2.3'   | '2.3.999' | true
+      'past milestone - major'     | '1.9'   | '2.3.999' | false
+      'past milestone - minor'     | '2.2'   | '2.3.999' | false
+    end
+
+    with_them do
+      it {is_expected.to be(expected)}
+    end
+  end
+
   describe '.valid_usage!' do
     before do
       allow(described_class).to receive(:definitions) do
@@ -215,7 +250,42 @@ RSpec.describe Feature::Definition do
     end
   end
 
-  describe '.defaul_enabled?' do
+  describe '.log_states?' do
+    using RSpec::Parameterized::TableSyntax
+
+    let(:definition) do
+      Feature::Definition.new("development/enabled_feature_flag.yml",
+                              name: :enabled_feature_flag,
+                              type: 'development',
+                              milestone: milestone,
+                              log_state_changes: log_state_change,
+                              default_enabled: false)
+    end
+
+    before do
+      allow(Feature::Definition).to receive(:definitions) do
+        { definition.key => definition }
+      end
+
+      allow(Gitlab).to receive(:version_info).and_return(Gitlab::VersionInfo.new(10, 0, 0))
+    end
+
+    subject { Feature::Definition.log_states?(key) }
+
+    where(:ctx, :key, :milestone, :log_state_change, :expected) do
+      'When flag does not exist'                    | :no_flag              | "0.0"  | true  | false
+      'When flag is old, and logging is not forced' | :enabled_feature_flag | "0.0"  | false | false
+      'When flag is old, but logging is forced'     | :enabled_feature_flag | "0.0"  | true  | true
+      'When flag is current'                        | :enabled_feature_flag | "10.0" | true  | true
+      'Flag is upcoming'                            | :enabled_feature_flag | "10.0" | true  | true
+    end
+
+    with_them do
+      it { is_expected.to be(expected) }
+    end
+  end
+
+  describe '.default_enabled?' do
     subject { described_class.default_enabled?(key) }
 
     context 'when feature flag exist' do

spec/lib/feature_spec.rb

@@ -186,6 +186,17 @@ RSpec.describe Feature, stub_feature_flags: false do
     context 'logging is enabled', :request_store do
       before do
         allow(Feature).to receive(:log_feature_flag_states?).and_call_original
+
+        definition = Feature::Definition.new("development/enabled_feature_flag.yml",
+                                                         name: :enabled_feature_flag,
+                                                         type: 'development',
+                                                         log_state_changes: true,
+                                                         default_enabled: false)
+
+        allow(Feature::Definition).to receive(:definitions) do
+          { definition.key => definition }
+        end
+
         described_class.enable(:feature_flag_state_logs)
         described_class.enable(:enabled_feature_flag)
         described_class.enabled?(:enabled_feature_flag)
@@ -513,6 +524,82 @@ RSpec.describe Feature, stub_feature_flags: false do
     end
   end
 
+  describe '.log_feature_flag_states?' do
+    let(:log_state_changes) { false }
+    let(:milestone) { "0.0" }
+    let(:flag_name) { :some_flag }
+    let(:definition) do
+      Feature::Definition.new("development/#{flag_name}.yml",
+                              name: flag_name,
+                              type: 'development',
+                              milestone: milestone,
+                              log_state_changes: log_state_changes,
+                              default_enabled: false)
+    end
+
+    before do
+      Feature.enable(:feature_flag_state_logs)
+      Feature.enable(:some_flag)
+
+      allow(Feature).to receive(:log_feature_flag_states?).and_return(false)
+      allow(Feature).to receive(:log_feature_flag_states?).with(:feature_flag_state_logs).and_call_original
+      allow(Feature).to receive(:log_feature_flag_states?).with(:some_flag).and_call_original
+
+      allow(Feature::Definition).to receive(:definitions) do
+        { definition.key => definition }
+      end
+    end
+
+    subject { described_class.log_feature_flag_states?(flag_name) }
+
+    context 'when flag is feature_flag_state_logs' do
+      let(:milestone) { "14.6" }
+      let(:flag_name) { :feature_flag_state_logs }
+      let(:log_state_changes) { true }
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'when flag is old' do
+      it { is_expected.to be_falsey }
+    end
+
+    context 'when flag is old while log_state_changes is not present ' do
+      let(:definition) do
+        Feature::Definition.new("development/#{flag_name}.yml",
+                                name: flag_name,
+                                type: 'development',
+                                milestone: milestone,
+                                default_enabled: false)
+      end
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'when flag is old but log_state_changes is true' do
+      let(:log_state_changes) { true }
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when flag is new and not feature_flag_state_logs' do
+      let(:milestone) { "14.6" }
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when milestone is nil' do
+      let(:definition) do
+        Feature::Definition.new("development/#{flag_name}.yml",
+                                name: flag_name,
+                                type: 'development',
+                                default_enabled: false)
+      end
+
+      it { is_expected.to be_falsey }
+    end
+  end
+
   context 'caching with stale reads from the database', :use_clean_rails_redis_caching, :request_store, :aggregate_failures do
     let(:actor) { stub_feature_flag_gate('CustomActor:5') }
     let(:another_actor) { stub_feature_flag_gate('CustomActor:10') }

spec/lib/gitlab/lograge/custom_options_spec.rb

@@ -98,15 +98,23 @@ RSpec.describe Gitlab::Lograge::CustomOptions do
 
     context  'when feature flags are present', :request_store do
       before do
-        allow(Feature::Definition).to receive(:valid_usage!).and_return(true)
-
         allow(Feature).to receive(:log_feature_flag_states?).and_return(false)
 
+        definitions = {}
+        [:enabled_feature, :disabled_feature].each do |flag_name|
+          definitions[flag_name] = Feature::Definition.new("development/enabled_feature.yml",
+                                                           name: flag_name,
+                                                           type: 'development',
+                                                           log_state_changes: true,
+                                                           default_enabled: false)
+
+          allow(Feature).to receive(:log_feature_flag_states?).with(flag_name).and_call_original
+        end
+
+        allow(Feature::Definition).to receive(:definitions).and_return(definitions)
+
         Feature.enable(:enabled_feature)
         Feature.disable(:disabled_feature)
-
-        allow(Feature).to receive(:log_feature_flag_states?).with(:enabled_feature).and_call_original
-        allow(Feature).to receive(:log_feature_flag_states?).with(:disabled_feature).and_call_original
       end
 
       context 'and :feature_flag_log_states is enabled' do