diff --git a/danger/product_intelligence/Dangerfile b/danger/product_intelligence/Dangerfile
index c38d87604cc22563411d10451c1b39732d98f241..1be549f139f18e2d4d1adaba91f8dc0ac755fe5e 100644
--- a/danger/product_intelligence/Dangerfile
+++ b/danger/product_intelligence/Dangerfile
@@ -1,3 +1,5 @@
# frozen_string_literal: true
product_intelligence.check!
+
+product_intelligence.check_affected_scopes!
diff --git a/spec/tooling/danger/product_intelligence_spec.rb b/spec/tooling/danger/product_intelligence_spec.rb
index ea08e3bc6dbc52b00f30828205cfef9c94aaf8a0..fab8b0c61fa272caf8c290bced882dbc0f2b2885 100644
--- a/spec/tooling/danger/product_intelligence_spec.rb
+++ b/spec/tooling/danger/product_intelligence_spec.rb
@@ -12,12 +12,16 @@
subject(:product_intelligence) { fake_danger.new(helper: fake_helper) }
let(:fake_danger) { DangerSpecHelper.fake_danger.include(described_class) }
- let(:changed_files) { ['metrics/counts_7d/test_metric.yml'] }
- let(:changed_lines) { ['+tier: ee'] }
+ let(:previous_label_to_add) { 'label_to_add' }
+ let(:labels_to_add) { [previous_label_to_add] }
+ let(:ci_env) { true }
+ let(:has_product_intelligence_label) { true }
before do
- allow(fake_helper).to receive(:all_changed_files).and_return(changed_files)
allow(fake_helper).to receive(:changed_lines).and_return(changed_lines)
+ allow(fake_helper).to receive(:labels_to_add).and_return(labels_to_add)
+ allow(fake_helper).to receive(:ci?).and_return(ci_env)
+ allow(fake_helper).to receive(:mr_has_labels?).with('product intelligence').and_return(has_product_intelligence_label)
end
describe '#check!' do
@@ -26,17 +30,13 @@
let(:markdown_formatted_list) { 'markdown formatted list' }
let(:review_pending_label) { 'product intelligence::review pending' }
let(:approved_label) { 'product intelligence::approved' }
- let(:ci_env) { true }
- let(:previous_label_to_add) { 'label_to_add' }
- let(:labels_to_add) { [previous_label_to_add] }
- let(:has_product_intelligence_label) { true }
+ let(:changed_files) { ['metrics/counts_7d/test_metric.yml'] }
+ let(:changed_lines) { ['+tier: ee'] }
before do
+ allow(fake_helper).to receive(:all_changed_files).and_return(changed_files)
allow(fake_helper).to receive(:changes_by_category).and_return(product_intelligence: changed_files, database: ['other_files.yml'])
- allow(fake_helper).to receive(:ci?).and_return(ci_env)
- allow(fake_helper).to receive(:mr_has_labels?).with('product intelligence').and_return(has_product_intelligence_label)
allow(fake_helper).to receive(:markdown_list).with(changed_files).and_return(markdown_formatted_list)
- allow(fake_helper).to receive(:labels_to_add).and_return(labels_to_add)
end
shared_examples "doesn't add new labels" do
@@ -121,4 +121,58 @@
end
end
end
+
+ describe '#check_affected_scopes!' do
+ let(:fixture_dir_glob) { Dir.glob(File.join('spec', 'tooling', 'fixtures', 'metrics', '*.rb')) }
+ let(:changed_lines) { ['+ scope :active, -> { iwhere(email: Array(emails)) }'] }
+
+ before do
+ allow(Dir).to receive(:glob).and_return(fixture_dir_glob)
+ allow(fake_helper).to receive(:markdown_list).with({ 'active' => fixture_dir_glob }).and_return('a')
+ end
+
+ context 'when a model was modified' do
+ let(:modified_files) { ['app/models/super_user.rb'] }
+
+ context 'when a scope is changed' do
+ context 'and a metrics uses the affected scope' do
+ it 'producing warning' do
+ expect(product_intelligence).to receive(:warn).with(%r{#{modified_files}})
+
+ product_intelligence.check_affected_scopes!
+ end
+ end
+
+ context 'when no metrics using the affected scope' do
+ let(:changed_lines) { ['+scope :foo, -> { iwhere(email: Array(emails)) }'] }
+
+ it 'doesnt do anything' do
+ expect(product_intelligence).not_to receive(:warn)
+
+ product_intelligence.check_affected_scopes!
+ end
+ end
+ end
+ end
+
+ context 'when an unrelated model with matching scope was modified' do
+ let(:modified_files) { ['app/models/post_box.rb'] }
+
+ it 'doesnt do anything' do
+ expect(product_intelligence).not_to receive(:warn)
+
+ product_intelligence.check_affected_scopes!
+ end
+ end
+
+ context 'when models arent modified' do
+ let(:modified_files) { ['spec/app/models/user_spec.rb'] }
+
+ it 'doesnt do anything' do
+ expect(product_intelligence).not_to receive(:warn)
+
+ product_intelligence.check_affected_scopes!
+ end
+ end
+ end
end
diff --git a/spec/tooling/fixtures/metrics/sample_instrumentation_metric.rb b/spec/tooling/fixtures/metrics/sample_instrumentation_metric.rb
new file mode 100644
index 0000000000000000000000000000000000000000..d6b86137b1b555c90eaf74a4a394282ad4493f11
--- /dev/null
+++ b/spec/tooling/fixtures/metrics/sample_instrumentation_metric.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+module Gitlab
+ module Usage
+ module Metrics
+ module Instrumentations
+ class ActiveUserCountMetric < DatabaseMetric
+ operation :count
+
+ relation { SuperUser.active }
+ end
+ end
+ end
+ end
+end
diff --git a/tooling/danger/product_intelligence.rb b/tooling/danger/product_intelligence.rb
index 621a7b509b0cfac883ca1104a0926668c04716e2..58e327408a1262f3ba0d1e6adc80b073b37b1cc7 100644
--- a/tooling/danger/product_intelligence.rb
+++ b/tooling/danger/product_intelligence.rb
@@ -4,15 +4,21 @@
module Tooling
module Danger
module ProductIntelligence
+ METRIC_DIRS = %w[lib/gitlab/usage/metrics/instrumentations ee/lib/gitlab/usage/metrics/instrumentations].freeze
APPROVED_LABEL = 'product intelligence::approved'
REVIEW_LABEL = 'product intelligence::review pending'
CHANGED_FILES_MESSAGE = <<~MSG
- For the following files, a review from the [Data team and Product Intelligence team](https://gitlab.com/groups/gitlab-org/analytics-section/product-intelligence/engineers/-/group_members?with_inherited_permissions=exclude) is recommended
- Please check the ~"product intelligence" [Service Ping guide](https://docs.gitlab.com/ee/development/service_ping/) or the [Snowplow guide](https://docs.gitlab.com/ee/development/snowplow/).
+ For the following files, a review from the [Data team and Product Intelligence team](https://gitlab.com/groups/gitlab-org/analytics-section/product-intelligence/engineers/-/group_members?with_inherited_permissions=exclude) is recommended
+ Please check the ~"product intelligence" [Service Ping guide](https://docs.gitlab.com/ee/development/service_ping/) or the [Snowplow guide](https://docs.gitlab.com/ee/development/snowplow/).
- For MR review guidelines, see the [Service Ping review guidelines](https://docs.gitlab.com/ee/development/service_ping/review_guidelines.html) or the [Snowplow review guidelines](https://docs.gitlab.com/ee/development/snowplow/review_guidelines.html).
+ For MR review guidelines, see the [Service Ping review guidelines](https://docs.gitlab.com/ee/development/service_ping/review_guidelines.html) or the [Snowplow review guidelines](https://docs.gitlab.com/ee/development/snowplow/review_guidelines.html).
- %<changed_files>s
+ %<changed_files>s
+
+ MSG
+
+ CHANGED_SCOPE_MESSAGE = <<~MSG
+ The following metrics could be affected by the modified scopes and require ~"product intelligence" review:
MSG
@@ -33,8 +39,61 @@ def check!
helper.labels_to_add.concat(labels_to_add) unless labels_to_add.empty?
end
+ def check_affected_scopes!
+ metric_scope_list = metric_scope_affected
+ return if metric_scope_list.empty?
+
+ warn CHANGED_SCOPE_MESSAGE + convert_to_table(metric_scope_list)
+ helper.labels_to_add.concat(missing_labels) unless missing_labels.empty?
+ end
+
private
+ def convert_to_table(items)
+ message = "Scope | Affected files |\n"
+ message += "--- | ----- |\n"
+ items.each_key do |scope|
+ affected_files = items[scope]
+ message += "`#{scope}`| `#{affected_files[0]}` |\n"
+ affected_files[1..]&.each do |file_name|
+ message += " | `#{file_name}` |\n"
+ end
+ end
+ message
+ end
+
+ def metric_scope_affected
+ select_models(helper.modified_files).each_with_object(Hash.new { |h, k| h[k] = [] }) do |file_name, matched_files|
+ helper.changed_lines(file_name).each do |mod_line, _i|
+ next unless mod_line =~ /^\+\s+scope :\w+/
+
+ affected_scope = mod_line.match(/:\w+/)
+ next if affected_scope.nil?
+
+ affected_class = File.basename(file_name, '.rb').split('_').map(&:capitalize).join
+ scope_name = "#{affected_class}.#{affected_scope[0][1..]}"
+
+ each_metric do |metric_def|
+ next unless File.read(metric_def).include?("relation { #{scope_name}")
+
+ matched_files[scope_name].push(metric_def)
+ end
+ end
+ end
+ end
+
+ def select_models(files)
+ files.select do |f|
+ f.start_with?('app/models/', 'ee/app/models/')
+ end
+ end
+
+ def each_metric(&block)
+ METRIC_DIRS.each do |dir|
+ Dir.glob(File.join(dir, '*.rb')).each(&block)
+ end
+ end
+
def missing_labels
return [] unless helper.ci?