diff --git a/danger/analytics_instrumentation/Dangerfile b/danger/analytics_instrumentation/Dangerfile index bbb984939dcf0985a585858d22271789badd6536..6b2139adab94d94a817fa126d35ed4458e16a7c7 100644 --- a/danger/analytics_instrumentation/Dangerfile +++ b/danger/analytics_instrumentation/Dangerfile @@ -7,3 +7,5 @@ analytics_instrumentation.check! analytics_instrumentation.check_affected_scopes! analytics_instrumentation.check_usage_data_insertions! + +analytics_instrumentation.check_deprecated_data_sources! diff --git a/spec/tooling/danger/analytics_instrumentation_spec.rb b/spec/tooling/danger/analytics_instrumentation_spec.rb index 5dfde44081d3c2400fcb2be104e2123d89d3b2bb..79c75b2e89cde9d8666fd9275b3c9466f1cc5c59 100644 --- a/spec/tooling/danger/analytics_instrumentation_spec.rb +++ b/spec/tooling/danger/analytics_instrumentation_spec.rb @@ -231,4 +231,64 @@ end end end + + describe '#check_deprecated_data_sources!' do + let(:fake_project_helper) { instance_double(Tooling::Danger::ProjectHelper) } + + subject(:check_data_source) { analytics_instrumentation.check_deprecated_data_sources! } + + before do + allow(fake_helper).to receive(:added_files).and_return([added_file]) + allow(fake_helper).to receive(:changed_lines).with(added_file).and_return(changed_lines) + allow(analytics_instrumentation).to receive(:project_helper).and_return(fake_project_helper) + allow(analytics_instrumentation.project_helper).to receive(:file_lines).and_return(changed_lines.map { |line| line.delete_prefix('+') }) + end + + context 'when no metric definitions were modified' do + let(:added_file) { 'app/models/user.rb' } + let(:changed_lines) { ['+ data_source: redis,'] } + + it 'does not trigger warning' do + expect(analytics_instrumentation).not_to receive(:markdown) + + check_data_source + end + end + + context 'when metrics fields were modified' do + let(:added_file) { 'config/metrics/count7_d/example_metric.yml' } + + [:redis, :redis_hll].each do |source| + context "when source is #{source}" do + let(:changed_lines) { ["+ data_source: #{source}"] } + let(:template) do + <<~SUGGEST_COMMENT + ```suggestion + data_source: internal_events + ``` + + %<message>s + SUGGEST_COMMENT + end + + it 'issues a warning' do + expected_comment = format(template, message: Tooling::Danger::AnalyticsInstrumentation::CHANGE_DEPRECATED_DATA_SOURCE_MESSAGE) + expect(analytics_instrumentation).to receive(:markdown).with(expected_comment.strip, file: added_file, line: 1) + + check_data_source + end + end + end + + context 'when neither redis nor redis_hll used as a data_source' do + let(:changed_lines) { ['+ data_source: database,'] } + + it 'does not issue a warning' do + expect(analytics_instrumentation).not_to receive(:markdown) + + check_data_source + end + end + end + end end diff --git a/tooling/danger/analytics_instrumentation.rb b/tooling/danger/analytics_instrumentation.rb index 2f8066f4421c9c05f23cf46703227ef55bf9ca93..d49c0f9e6baade806fadc942243d78873dc9fff1 100644 --- a/tooling/danger/analytics_instrumentation.rb +++ b/tooling/danger/analytics_instrumentation.rb @@ -1,8 +1,12 @@ # frozen_string_literal: true +require_relative 'suggestor' + module Tooling module Danger module AnalyticsInstrumentation + include ::Tooling::Danger::Suggestor + METRIC_DIRS = %w[lib/gitlab/usage/metrics/instrumentations ee/lib/gitlab/usage/metrics/instrumentations].freeze APPROVED_LABEL = 'analytics instrumentation::approved' REVIEW_LABEL = 'analytics instrumentation::review pending' @@ -26,6 +30,10 @@ module AnalyticsInstrumentation Please use [Instrumentation Classes](https://docs.gitlab.com/ee/development/service_ping/metrics_instrumentation.html) instead. MSG + CHANGE_DEPRECATED_DATA_SOURCE_MESSAGE = <<~MSG + Redis and RedisHLL tracking is deprecated, consider using Internal Events tracking instead https://docs.gitlab.com/ee/development/internal_analytics/internal_event_instrumentation/quick_start.html#defining-event-and-metrics + MSG + WORKFLOW_LABELS = [ APPROVED_LABEL, REVIEW_LABEL @@ -58,6 +66,17 @@ def check_usage_data_insertions! warn format(CHANGED_USAGE_DATA_MESSAGE) end + def check_deprecated_data_sources! + new_metric_files.each do |filename| + add_suggestion( + filename: filename, + regex: /^\+?\s+data_source: redis\w*/, + replacement: 'data_source: internal_events', + comment_text: CHANGE_DEPRECATED_DATA_SOURCE_MESSAGE + ) + end + end + private def convert_to_table(items) @@ -99,6 +118,10 @@ def select_models(files) end end + def new_metric_files + helper.added_files.select { |f| f.include?('config/metrics') && f.end_with?('.yml') } + end + def each_metric(&block) METRIC_DIRS.each do |dir| Dir.glob(File.join(dir, '*.rb')).each(&block)