From cbf844505297cb791278878375a102958cc44fc6 Mon Sep 17 00:00:00 2001 From: David Fernandez Date: Fri, 4 Nov 2022 09:28:02 +0100 Subject: [PATCH] Manage packages_size statistic with a counter attribute Implement a feature flag support so that the between sync to async statistic update can be gated by that flag scoped to the related project. --- app/models/concerns/counter_attribute.rb | 35 +++++++++++++------ app/models/project_statistics.rb | 27 ++++++++------ .../packages_size_counter_attribute.yml | 8 +++++ .../models/concerns/counter_attribute_spec.rb | 25 ++++++++++--- spec/models/packages/package_file_spec.rb | 20 +++++++---- spec/models/packages/package_spec.rb | 24 ++++++++++--- spec/models/project_statistics_spec.rb | 14 ++++++-- spec/support/counter_attribute.rb | 5 ++- .../counter_attribute_shared_examples.rb | 3 +- 9 files changed, 118 insertions(+), 43 deletions(-) create mode 100644 config/feature_flags/development/packages_size_counter_attribute.yml diff --git a/app/models/concerns/counter_attribute.rb b/app/models/concerns/counter_attribute.rb index 03e062a98557a7..4dfaae674bfba6 100644 --- a/app/models/concerns/counter_attribute.rb +++ b/app/models/concerns/counter_attribute.rb @@ -17,6 +17,18 @@ # counter_attribute :storage_size # end # +# It's possible to define a conditional counter attribute. You need to pass a proc +# that must accept a single argument, the object instance on which this concern is +# included. +# +# @example: +# +# class ProjectStatistics +# include CounterAttribute +# +# counter_attribute :conditional_one, if: -> { |object| object.use_counter_attribute? } +# end +# # To increment the counter we can use the method: # delayed_increment_counter(:commit_count, 3) # @@ -49,12 +61,15 @@ module CounterAttribute WORKER_LOCK_TTL = 10.minutes class_methods do - def counter_attribute(attribute) - counter_attributes << attribute + def counter_attribute(attribute, if: nil) + counter_attributes << { + attribute: attribute, + if_proc: binding.local_variable_get(:if) # can't read `if` directly + } end def counter_attributes - @counter_attributes ||= Set.new + @counter_attributes ||= [] end def after_flush_callbacks @@ -65,10 +80,14 @@ def after_flush_callbacks def counter_attribute_after_flush(&callback) after_flush_callbacks << callback end + end - def counter_attribute_enabled?(attribute) - counter_attributes.include?(attribute) - end + def counter_attribute_enabled?(attribute) + counter_attribute = self.class.counter_attributes.find { |registered| registered[:attribute] == attribute } + return false unless counter_attribute + return true unless counter_attribute[:if_proc] + + counter_attribute[:if_proc].call(self) end # This method must only be called by FlushCounterIncrementsWorker @@ -167,10 +186,6 @@ def counter_lock_key(attribute) counter_key(attribute) + ':lock' end - def counter_attribute_enabled?(attribute) - self.class.counter_attribute_enabled?(attribute) - end - private def database_lock_key diff --git a/app/models/project_statistics.rb b/app/models/project_statistics.rb index 0570be85ad1e86..fb0ad178c91425 100644 --- a/app/models/project_statistics.rb +++ b/app/models/project_statistics.rb @@ -11,6 +11,7 @@ class ProjectStatistics < ApplicationRecord attribute :snippets_size, default: 0 counter_attribute :build_artifacts_size + counter_attribute :packages_size, if: -> (statistics) { Feature.enabled?(:packages_size_counter_attribute, statistics.project) } counter_attribute_after_flush do |project_statistic| project_statistic.refresh_storage_size! @@ -22,7 +23,7 @@ class ProjectStatistics < ApplicationRecord COLUMNS_TO_REFRESH = [:repository_size, :wiki_size, :lfs_objects_size, :commit_count, :snippets_size, :uploads_size, :container_registry_size].freeze INCREMENTABLE_COLUMNS = { - packages_size: %i[storage_size], + packages_size: %i[storage_size], # remove this along with packages_size_counter_attribute pipeline_artifacts_size: %i[storage_size], snippets_size: %i[storage_size] }.freeze @@ -124,20 +125,20 @@ def refresh_storage_size! # # For non-counter attributes, storage_size is updated depending on key => [columns] in INCREMENTABLE_COLUMNS def self.increment_statistic(project, key, amount) - raise ArgumentError, "Cannot increment attribute: #{key}" unless incrementable_attribute?(key) - return if amount == 0 - project.statistics.try do |project_statistics| - if counter_attribute_enabled?(key) - project_statistics.delayed_increment_counter(key, amount) - else - project_statistics.legacy_increment_statistic(key, amount) - end + project_statistics.increment_statistic(key, amount) end end - def self.incrementable_attribute?(key) - INCREMENTABLE_COLUMNS.key?(key) || counter_attribute_enabled?(key) + def increment_statistic(key, amount) + raise ArgumentError, "Cannot increment attribute: #{key}" unless incrementable_attribute?(key) + return if amount == 0 + + if counter_attribute_enabled?(key) + delayed_increment_counter(key, amount) + else + legacy_increment_statistic(key, amount) + end end def legacy_increment_statistic(key, amount) @@ -149,6 +150,10 @@ def legacy_increment_statistic(key, amount) private + def incrementable_attribute?(key) + INCREMENTABLE_COLUMNS.key?(key) || counter_attribute_enabled?(key) + end + def storage_size_components STORAGE_SIZE_COMPONENTS end diff --git a/config/feature_flags/development/packages_size_counter_attribute.yml b/config/feature_flags/development/packages_size_counter_attribute.yml new file mode 100644 index 00000000000000..bc24526589ca4a --- /dev/null +++ b/config/feature_flags/development/packages_size_counter_attribute.yml @@ -0,0 +1,8 @@ +--- +name: packages_size_counter_attribute +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/102978 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/381287 +milestone: '15.7' +type: development +group: group::package registry +default_enabled: false diff --git a/spec/models/concerns/counter_attribute_spec.rb b/spec/models/concerns/counter_attribute_spec.rb index 66ccd4559e5e60..fd1f0d3e836de3 100644 --- a/spec/models/concerns/counter_attribute_spec.rb +++ b/spec/models/concerns/counter_attribute_spec.rb @@ -8,12 +8,12 @@ let(:project_statistics) { create(:project_statistics) } let(:model) { CounterAttributeModel.find(project_statistics.id) } - it_behaves_like CounterAttribute, [:build_artifacts_size, :commit_count] do + it_behaves_like CounterAttribute, [:build_artifacts_size, :commit_count, :packages_size] do let(:model) { CounterAttributeModel.find(project_statistics.id) } end describe 'after_flush callbacks' do - let(:attribute) { model.class.counter_attributes.first } + let(:attribute) { model.class.counter_attributes.first[:attribute] } subject { model.flush_increments_to_database!(attribute) } @@ -80,13 +80,28 @@ end end - describe '.counter_attribute_enabled?' do + describe '#counter_attribute_enabled?' do it 'is true when counter attribute is defined' do - expect(CounterAttributeModel.counter_attribute_enabled?(:build_artifacts_size)).to be_truthy + expect(project_statistics.counter_attribute_enabled?(:build_artifacts_size)) + .to be_truthy end it 'is false when counter attribute is not defined' do - expect(CounterAttributeModel.counter_attribute_enabled?(:nope)).to be_falsey + expect(model.counter_attribute_enabled?(:nope)).to be_falsey + end + + context 'with a conditional counter attribute' do + [true, false].each do |enabled| + context "where the condition evaluates to #{enabled}" do + subject { model.counter_attribute_enabled?(:packages_size) } + + before do + model.allow_package_size_counter = enabled + end + + it { is_expected.to eq(enabled) } + end + end end end end diff --git a/spec/models/packages/package_file_spec.rb b/spec/models/packages/package_file_spec.rb index c665f738ead697..6dd64eb1b200cd 100644 --- a/spec/models/packages/package_file_spec.rb +++ b/spec/models/packages/package_file_spec.rb @@ -104,14 +104,20 @@ let_it_be(:package, reload: true) { create(:package) } context 'when the package file has an explicit size' do - it_behaves_like 'UpdateProjectStatistics' do - subject { build(:package_file, :jar, package: package, size: 42) } - end - end + subject { build(:package_file, :jar, package: package, size: 42) } - context 'when the package file does not have a size' do - it_behaves_like 'UpdateProjectStatistics' do - subject { build(:package_file, package: package, size: nil) } + it_behaves_like 'UpdateProjectStatistics', :packages_size + + context 'when packages_size_counter_attribute is disabled' do + before do + stub_feature_flags(packages_size_counter_attribute: false) + end + + it 'uses the legacy increment function' do + expect(package.project.statistics).to receive(:legacy_increment_statistic) + expect(package.project.statistics).not_to receive(:delayed_increment_counter) + subject.save! + end end end end diff --git a/spec/models/packages/package_spec.rb b/spec/models/packages/package_spec.rb index 241c585099c238..788efbe5862ec4 100644 --- a/spec/models/packages/package_spec.rb +++ b/spec/models/packages/package_spec.rb @@ -708,12 +708,26 @@ describe '#destroy' do let(:package) { create(:npm_package) } let(:package_file) { package.package_files.first } - let(:project_statistics) { ProjectStatistics.for_project_ids(package.project.id).first } + let(:project_statistics) { package.project.statistics } - it 'affects project statistics' do - expect { package.destroy! } - .to change { project_statistics.reload.packages_size } - .from(package_file.size).to(0) + subject(:destroy!) { package.destroy! } + + it 'updates the project statistics' do + expect(project_statistics).to receive(:delayed_increment_counter).with(:packages_size, -package_file.size) + + destroy! + end + + context 'when packages_size_counter_attribute is disabled' do + before do + stub_feature_flags(packages_size_counter_attribute: false) + end + + it 'affects project statistics' do + expect { destroy! } + .to change { project_statistics.reload.packages_size } + .from(package_file.size).to(0) + end end end diff --git a/spec/models/project_statistics_spec.rb b/spec/models/project_statistics_spec.rb index 9de31ea66e4c4f..6a1227c45f1dfb 100644 --- a/spec/models/project_statistics_spec.rb +++ b/spec/models/project_statistics_spec.rb @@ -506,20 +506,28 @@ context 'when adjusting :packages_size' do let(:stat) { :packages_size } - it_behaves_like 'a statistic that increases storage_size' + it_behaves_like 'a statistic that increases storage_size asynchronously' + + context 'with packages_size_counter_attribute disabled' do + before do + stub_feature_flags(packages_size_counter_attribute: false) + end + + it_behaves_like 'a statistic that increases storage_size' + end end context 'when the amount is 0' do it 'does not execute a query' do project - expect { described_class.increment_statistic(project.id, :build_artifacts_size, 0) } + expect { described_class.increment_statistic(project, :build_artifacts_size, 0) } .not_to exceed_query_limit(0) end end context 'when using an invalid column' do it 'raises an error' do - expect { described_class.increment_statistic(project.id, :id, 13) } + expect { described_class.increment_statistic(project, :id, 13) } .to raise_error(ArgumentError, "Cannot increment attribute: id") end end diff --git a/spec/support/counter_attribute.rb b/spec/support/counter_attribute.rb index 8bd40b72dcf95c..18db5c92644910 100644 --- a/spec/support/counter_attribute.rb +++ b/spec/support/counter_attribute.rb @@ -7,10 +7,13 @@ CounterAttributeModel.class_eval do include CounterAttribute + after_initialize { self.allow_package_size_counter = true } + counter_attribute :build_artifacts_size counter_attribute :commit_count + counter_attribute :packages_size, if: ->(instance) { instance.allow_package_size_counter } - attr_accessor :flushed + attr_accessor :flushed, :allow_package_size_counter counter_attribute_after_flush do |subject| subject.flushed = true diff --git a/spec/support/shared_examples/models/concerns/counter_attribute_shared_examples.rb b/spec/support/shared_examples/models/concerns/counter_attribute_shared_examples.rb index a658d02f09a1e5..81dbbe7758e647 100644 --- a/spec/support/shared_examples/models/concerns/counter_attribute_shared_examples.rb +++ b/spec/support/shared_examples/models/concerns/counter_attribute_shared_examples.rb @@ -12,7 +12,8 @@ end it 'defines a method to store counters' do - expect(model.class.counter_attributes.to_a).to eq(counter_attributes) + registered_attributes = model.class.counter_attributes.map { |e| e[:attribute] } # rubocop:disable Rails/Pluck + expect(registered_attributes).to contain_exactly(*counter_attributes) end counter_attributes.each do |attribute| -- GitLab