Merge branch 'am-improve-instrumentation-class-generator' into 'master'

Improve metric instrumentation generator [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!66549

doc/development/service_ping/metrics_instrumentation.md

@@ -107,7 +107,8 @@ To create a stub instrumentation for a Service Ping metric, you can use a dedica
 
 The generator takes the class name as an argument and the following options:
 
-- `--type=TYPE` Required. Indicates the metric type. It must be one of: `database`, `generic`, `redis_hll`.
+- `--type=TYPE` Required. Indicates the metric type. It must be one of: `database`, `generic`.
+- `--operation` Required for `database` type. It must be one of: `count`, `distinct_count`.
 - `--ee` Indicates if the metric is for EE.
 
 ```shell

lib/generators/gitlab/usage_metric/templates/database_instrumentation_class.rb.template

+# frozen_string_literal: true
+
+module Gitlab
+  module Usage
+    module Metrics
+      module Instrumentations
+        class <%= class_name %>Metric < DatabaseMetric
+          operation :<%= operation%>
+
+          relation do
+            # Insert ActiveRecord relation here
+          end
+        end
+      end
+    end
+  end
+end

spec/fixtures/lib/generators/gitlab/usage_metric_generator/sample_metric.rb → lib/generators/gitlab/usage_metric/templates/generic_instrumentation_class.rb.template

@@ -4,8 +4,9 @@ module Gitlab
   module Usage
     module Metrics
       module Instrumentations
-        class CountFooMetric < RedisHLLMetric
-          def value
+        class <%= class_name %>Metric < GenericMetric
+          value do
+            # Insert metric code logic here
           end
         end
       end

lib/generators/gitlab/usage_metric/usage_metric_generator.rb

@@ -11,21 +11,25 @@ module Gitlab
 
     ALLOWED_SUPERCLASSES = {
       generic: 'Generic',
-      database: 'Database',
-      redis_hll: 'RedisHLL'
+      database: 'Database'
     }.freeze
 
+    ALLOWED_OPERATIONS = %w(count distinct_count).freeze
+
     source_root File.expand_path('templates', __dir__)
 
     class_option :ee, type: :boolean, optional: true, default: false, desc: 'Indicates if instrumentation is for EE'
     class_option :type, type: :string, desc: "Metric type, must be one of: #{ALLOWED_SUPERCLASSES.keys.join(', ')}"
+    class_option :operation, type: :string, desc: "Metric operation, must be one of: #{ALLOWED_OPERATIONS.join(', ')}"
 
     argument :class_name, type: :string, desc: 'Instrumentation class name, e.g.: CountIssues'
 
     def create_class_files
       validate!
 
-      template "instrumentation_class.rb.template", file_path
+      template "database_instrumentation_class.rb.template", file_path if type == 'database'
+      template "generic_instrumentation_class.rb.template", file_path if type == 'generic'
+
       template "instrumentation_class_spec.rb.template", spec_file_path
     end
 
@@ -34,6 +38,7 @@ module Gitlab
     def validate!
       raise ArgumentError, "Type is required, valid options are #{ALLOWED_SUPERCLASSES.keys.join(', ')}" unless type.present?
       raise ArgumentError, "Unknown type '#{type}', valid options are #{ALLOWED_SUPERCLASSES.keys.join(', ')}" if metric_superclass.nil?
+      raise ArgumentError, "Unknown operation '#{operation}' valid operations are #{ALLOWED_OPERATIONS.join(', ')}" if type == 'database' && !ALLOWED_OPERATIONS.include?(operation)
     end
 
     def ee?
@@ -44,6 +49,10 @@ module Gitlab
       options[:type]
     end
 
+    def operation
+      options[:operation]
+    end
+
     def file_path
       dir = ee? ? EE_DIR : CE_DIR
 

spec/fixtures/lib/generators/gitlab/usage_metric_generator/sample_database_metric.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Usage
+    module Metrics
+      module Instrumentations
+        class CountFooMetric < DatabaseMetric
+          operation :count
+
+          relation do
+            # Insert ActiveRecord relation here
+          end
+        end
+      end
+    end
+  end
+end

lib/generators/gitlab/usage_metric/templates/instrumentation_class.rb.template → spec/fixtures/lib/generators/gitlab/usage_metric_generator/sample_generic_metric.rb

@@ -4,8 +4,9 @@ module Gitlab
   module Usage
     module Metrics
       module Instrumentations
-        class <%= class_name %>Metric < <%= metric_superclass %>Metric
-          def value
+        class CountFooMetric < GenericMetric
+          value do
+            # Insert metric code logic here
           end
         end
       end

spec/lib/generators/gitlab/usage_metric_generator_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe Gitlab::UsageMetricGenerator, :silence_stdout do
   let(:spec_ce_temp_dir) { Dir.mktmpdir }
   let(:spec_ee_temp_dir) { Dir.mktmpdir }
   let(:args) { ['CountFoo'] }
-  let(:options) { { 'type' => 'redis_hll' } }
+  let(:options) { { 'type' => 'generic' } }
 
   before do
     stub_const("#{described_class}::CE_DIR", ce_temp_dir)
@@ -30,27 +30,39 @@ RSpec.describe Gitlab::UsageMetricGenerator, :silence_stdout do
 
   describe 'Creating metric instrumentation files' do
     let(:sample_metric_dir) { 'lib/generators/gitlab/usage_metric_generator' }
-    let(:sample_metric) { fixture_file(File.join(sample_metric_dir, 'sample_metric.rb')) }
+    let(:generic_sample_metric) { fixture_file(File.join(sample_metric_dir, 'sample_generic_metric.rb')) }
+    let(:database_sample_metric) { fixture_file(File.join(sample_metric_dir, 'sample_database_metric.rb')) }
     let(:sample_spec) { fixture_file(File.join(sample_metric_dir, 'sample_metric_test.rb')) }
 
     it 'creates CE metric instrumentation files using the template' do
       described_class.new(args, options).invoke_all
 
-      expect_generated_file(ce_temp_dir, 'count_foo_metric.rb', sample_metric)
+      expect_generated_file(ce_temp_dir, 'count_foo_metric.rb', generic_sample_metric)
       expect_generated_file(spec_ce_temp_dir, 'count_foo_metric_spec.rb', sample_spec)
     end
 
     context 'with EE flag true' do
-      let(:options) { { 'type' => 'redis_hll', 'ee' => true } }
+      let(:options) { { 'type' => 'generic', 'ee' => true } }
 
       it 'creates EE metric instrumentation files using the template' do
         described_class.new(args, options).invoke_all
 
-        expect_generated_file(ee_temp_dir, 'count_foo_metric.rb', sample_metric)
+        expect_generated_file(ee_temp_dir, 'count_foo_metric.rb', generic_sample_metric)
         expect_generated_file(spec_ee_temp_dir, 'count_foo_metric_spec.rb', sample_spec)
       end
     end
 
+    context 'for database type' do
+      let(:options) { { 'type' => 'database', 'operation' => 'count' } }
+
+      it 'creates the metric instrumentation file using the template' do
+        described_class.new(args, options).invoke_all
+
+        expect_generated_file(ce_temp_dir, 'count_foo_metric.rb', database_sample_metric)
+        expect_generated_file(spec_ce_temp_dir, 'count_foo_metric_spec.rb', sample_spec)
+      end
+    end
+
     context 'with type option missing' do
       let(:options) { {} }
 
@@ -66,5 +78,21 @@ RSpec.describe Gitlab::UsageMetricGenerator, :silence_stdout do
         expect { described_class.new(args, options).invoke_all }.to raise_error(ArgumentError, /Unknown type 'some_other_type'/)
       end
     end
+
+    context 'without operation for database metric' do
+      let(:options) { { 'type' => 'database' } }
+
+      it 'raises an ArgumentError' do
+        expect { described_class.new(args, options).invoke_all }.to raise_error(ArgumentError, /Unknown operation ''/)
+      end
+    end
+
+    context 'with wrong operation for database metric' do
+      let(:options) { { 'type' => 'database', 'operation' => 'sleep' } }
+
+      it 'raises an ArgumentError' do
+        expect { described_class.new(args, options).invoke_all }.to raise_error(ArgumentError, /Unknown operation 'sleep'/)
+      end
+    end
   end
 end