Use instrumentation classes for Service Ping metrics

Now we can use only instrumentation classes to add a new metric to
Service Ping, without changing usage_data.rb.

Changelog: changed

config/feature_flags/development/merge_service_ping_instrumented_metrics.yml

----
-name: merge_service_ping_instrumented_metrics
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/77629
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/352964
-milestone: '14.8'
-type: development
-group: group::product intelligence
-default_enabled: true

ee/spec/lib/ee/gitlab/usage/service_ping_report_spec.rb

@@ -7,23 +7,20 @@ RSpec.describe Gitlab::Usage::ServicePingReport, :use_clean_rails_memory_store_c
 
   let(:usage_data) { { uuid: "1111", counts: { issue: 0 } } }
 
-  context 'when feature merge_service_ping_instrumented_metrics enabled' do
-    context 'for conditional metrics inclusion' do
-      before do
-        stub_feature_flags(merge_service_ping_instrumented_metrics: true)
-        stub_usage_data_connections
-        stub_object_store_settings
-        stub_prometheus_queries
-        memoized_constants = Gitlab::UsageData::CE_MEMOIZED_VALUES
-        memoized_constants += Gitlab::UsageData::EE_MEMOIZED_VALUES if defined? Gitlab::UsageData::EE_MEMOIZED_VALUES
-        memoized_constants.each { |v| Gitlab::UsageData.clear_memoization(v) }
-        stub_database_flavor_check('Cloud SQL for PostgreSQL')
-        allow(License).to receive(:current)
-      end
+  context 'for conditional metrics inclusion' do
+    before do
+      stub_usage_data_connections
+      stub_object_store_settings
+      stub_prometheus_queries
+      memoized_constants = Gitlab::UsageData::CE_MEMOIZED_VALUES
+      memoized_constants += Gitlab::UsageData::EE_MEMOIZED_VALUES if defined? Gitlab::UsageData::EE_MEMOIZED_VALUES
+      memoized_constants.each { |v| Gitlab::UsageData.clear_memoization(v) }
+      stub_database_flavor_check('Cloud SQL for PostgreSQL')
+      allow(License).to receive(:current)
+    end
 
-      it 'does not raise errors' do
-        expect { described_class.for(output: :all_metrics_values) }.not_to raise_error
-      end
+    it 'does not raise errors' do
+      expect { described_class.for(output: :all_metrics_values) }.not_to raise_error
     end
   end
 end

ee/spec/lib/ee/gitlab/usage_data_spec.rb

@@ -83,9 +83,6 @@ RSpec.describe Gitlab::UsageData do
     end
 
     it 'gathers usage counts', :aggregate_failures do
-      stub_feature_flags(merge_service_ping_instrumented_metrics: false)
-
-      expect(count_data[:boards]).to eq(1)
       expect(count_data[:projects]).to eq(3)
 
       expect(count_data.keys).to include(*%i(

ee/spec/services/ee/service_ping/build_payload_service_spec.rb

@@ -16,7 +16,6 @@ RSpec.describe ServicePing::BuildPayloadService do
 
     before do
       allow(User).to receive(:single_user).and_return(double(:user, requires_usage_stats_consent?: false))
-      stub_feature_flags(merge_service_ping_instrumented_metrics: false)
     end
 
     context 'GitLab instance have a license' do

lib/gitlab/usage/service_ping_report.rb

@@ -18,16 +18,11 @@ module Gitlab
         private
 
         def with_instrumentation_classes(old_payload, output_method)
-          if Feature.enabled?(:merge_service_ping_instrumented_metrics, default_enabled: :yaml)
+          instrumented_metrics_key_paths = Gitlab::Usage::ServicePing::PayloadKeysProcessor.new(old_payload).missing_instrumented_metrics_key_paths
 
-            instrumented_metrics_key_paths = Gitlab::Usage::ServicePing::PayloadKeysProcessor.new(old_payload).missing_instrumented_metrics_key_paths
+          instrumented_payload = Gitlab::Usage::ServicePing::InstrumentedPayload.new(instrumented_metrics_key_paths, output_method).build
 
-            instrumented_payload = Gitlab::Usage::ServicePing::InstrumentedPayload.new(instrumented_metrics_key_paths, output_method).build
-
-            old_payload.deep_merge(instrumented_payload)
-          else
-            old_payload
-          end
+          old_payload.deep_merge(instrumented_payload)
         end
 
         def all_metrics_values(cached)

lib/gitlab/usage_data.rb

@@ -70,7 +70,7 @@ module Gitlab
       def system_usage_data
         issues_created_manually_from_alerts = count(Issue.with_alert_management_alerts.not_authored_by(::User.alert_bot), start: minimum_id(Issue), finish: maximum_id(Issue))
 
-        counts = {
+        {
           counts: {
             assignee_lists: count(List.assignee),
             ci_builds: count(::Ci::Build),
@@ -166,12 +166,6 @@ module Gitlab
             data[:snippets] = add(data[:personal_snippets], data[:project_snippets])
           end
         }
-
-        if Feature.disabled?(:merge_service_ping_instrumented_metrics, default_enabled: :yaml)
-          counts[:counts][:boards] = add_metric('CountBoardsMetric', time_frame: 'all')
-        end
-
-        counts
       end
       # rubocop: enable Metrics/AbcSize
 

spec/lib/gitlab/usage/service_ping_report_spec.rb

@@ -7,119 +7,86 @@ RSpec.describe Gitlab::Usage::ServicePingReport, :use_clean_rails_memory_store_c
 
   let(:usage_data) { { uuid: "1111", counts: { issue: 0 } } }
 
-  context 'when feature merge_service_ping_instrumented_metrics enabled' do
-    before do
-      stub_feature_flags(merge_service_ping_instrumented_metrics: true)
+  before do
+    allow_next_instance_of(Gitlab::Usage::ServicePing::PayloadKeysProcessor) do |instance|
+      allow(instance).to receive(:missing_key_paths).and_return([])
+    end
 
-      allow_next_instance_of(Gitlab::Usage::ServicePing::PayloadKeysProcessor) do |instance|
-        allow(instance).to receive(:missing_key_paths).and_return([])
-      end
+    allow_next_instance_of(Gitlab::Usage::ServicePing::InstrumentedPayload) do |instance|
+      allow(instance).to receive(:build).and_return({})
+    end
+  end
 
-      allow_next_instance_of(Gitlab::Usage::ServicePing::InstrumentedPayload) do |instance|
-        allow(instance).to receive(:build).and_return({})
-      end
+  context 'all_metrics_values' do
+    it 'generates the service ping when there are no missing values' do
+      expect(Gitlab::UsageData).to receive(:data).and_return(usage_data)
+      expect(described_class.for(output: :all_metrics_values)).to eq({ uuid: "1111", counts: { issue: 0 } })
     end
 
-    context 'all_metrics_values' do
-      it 'generates the service ping when there are no missing values' do
-        expect(Gitlab::UsageData).to receive(:data).and_return(usage_data)
-        expect(described_class.for(output: :all_metrics_values)).to eq({ uuid: "1111", counts: { issue: 0 } })
+    it 'generates the service ping with the missing values' do
+      expect_next_instance_of(Gitlab::Usage::ServicePing::PayloadKeysProcessor, usage_data) do |instance|
+        expect(instance).to receive(:missing_instrumented_metrics_key_paths).and_return(['counts.boards'])
       end
 
-      it 'generates the service ping with the missing values' do
-        expect_next_instance_of(Gitlab::Usage::ServicePing::PayloadKeysProcessor, usage_data) do |instance|
-          expect(instance).to receive(:missing_instrumented_metrics_key_paths).and_return(['counts.boards'])
-        end
-
-        expect_next_instance_of(Gitlab::Usage::ServicePing::InstrumentedPayload, ['counts.boards'], :with_value) do |instance|
-          expect(instance).to receive(:build).and_return({ counts: { boards: 1 } })
-        end
-
-        expect(Gitlab::UsageData).to receive(:data).and_return(usage_data)
-        expect(described_class.for(output: :all_metrics_values)).to eq({ uuid: "1111", counts: { issue: 0, boards: 1 } })
+      expect_next_instance_of(Gitlab::Usage::ServicePing::InstrumentedPayload, ['counts.boards'], :with_value) do |instance|
+        expect(instance).to receive(:build).and_return({ counts: { boards: 1 } })
       end
-    end
-
-    context 'for output: :metrics_queries' do
-      it 'generates the service ping' do
-        expect(Gitlab::UsageData).to receive(:data).and_return(usage_data)
 
-        described_class.for(output: :metrics_queries)
-      end
+      expect(Gitlab::UsageData).to receive(:data).and_return(usage_data)
+      expect(described_class.for(output: :all_metrics_values)).to eq({ uuid: "1111", counts: { issue: 0, boards: 1 } })
     end
+  end
 
-    context 'for output: :non_sql_metrics_values' do
-      it 'generates the service ping' do
-        expect(Gitlab::UsageData).to receive(:data).and_return(usage_data)
+  context 'for output: :metrics_queries' do
+    it 'generates the service ping' do
+      expect(Gitlab::UsageData).to receive(:data).and_return(usage_data)
 
-        described_class.for(output: :non_sql_metrics_values)
-      end
+      described_class.for(output: :metrics_queries)
     end
+  end
 
-    context 'when using cached' do
-      context 'for cached: true' do
-        let(:new_usage_data) { { uuid: "1112" } }
-
-        it 'caches the values' do
-          allow(Gitlab::UsageData).to receive(:data).and_return(usage_data, new_usage_data)
+  context 'for output: :non_sql_metrics_values' do
+    it 'generates the service ping' do
+      expect(Gitlab::UsageData).to receive(:data).and_return(usage_data)
 
-          expect(described_class.for(output: :all_metrics_values)).to eq(usage_data)
-          expect(described_class.for(output: :all_metrics_values, cached: true)).to eq(usage_data)
+      described_class.for(output: :non_sql_metrics_values)
+    end
+  end
 
-          expect(Rails.cache.fetch('usage_data')).to eq(usage_data)
-        end
+  context 'when using cached' do
+    context 'for cached: true' do
+      let(:new_usage_data) { { uuid: "1112" } }
 
-        it 'writes to cache and returns fresh data' do
-          allow(Gitlab::UsageData).to receive(:data).and_return(usage_data, new_usage_data)
+      it 'caches the values' do
+        allow(Gitlab::UsageData).to receive(:data).and_return(usage_data, new_usage_data)
 
-          expect(described_class.for(output: :all_metrics_values)).to eq(usage_data)
-          expect(described_class.for(output: :all_metrics_values)).to eq(new_usage_data)
-          expect(described_class.for(output: :all_metrics_values, cached: true)).to eq(new_usage_data)
+        expect(described_class.for(output: :all_metrics_values)).to eq(usage_data)
+        expect(described_class.for(output: :all_metrics_values, cached: true)).to eq(usage_data)
 
-          expect(Rails.cache.fetch('usage_data')).to eq(new_usage_data)
-        end
+        expect(Rails.cache.fetch('usage_data')).to eq(usage_data)
       end
 
-      context 'when no caching' do
-        let(:new_usage_data) { { uuid: "1112" } }
-
-        it 'returns fresh data' do
-          allow(Gitlab::UsageData).to receive(:data).and_return(usage_data, new_usage_data)
-
-          expect(described_class.for(output: :all_metrics_values)).to eq(usage_data)
-          expect(described_class.for(output: :all_metrics_values)).to eq(new_usage_data)
-
-          expect(Rails.cache.fetch('usage_data')).to eq(new_usage_data)
-        end
-      end
-    end
-  end
+      it 'writes to cache and returns fresh data' do
+        allow(Gitlab::UsageData).to receive(:data).and_return(usage_data, new_usage_data)
 
-  context 'when feature merge_service_ping_instrumented_metrics disabled' do
-    before do
-      stub_feature_flags(merge_service_ping_instrumented_metrics: false)
-    end
+        expect(described_class.for(output: :all_metrics_values)).to eq(usage_data)
+        expect(described_class.for(output: :all_metrics_values)).to eq(new_usage_data)
+        expect(described_class.for(output: :all_metrics_values, cached: true)).to eq(new_usage_data)
 
-    context 'all_metrics_values' do
-      it 'generates the service ping when there are no missing values' do
-        expect(Gitlab::UsageData).to receive(:data).and_return(usage_data)
-        expect(described_class.for(output: :all_metrics_values)).to eq({ uuid: "1111", counts: { issue: 0 } })
+        expect(Rails.cache.fetch('usage_data')).to eq(new_usage_data)
       end
     end
 
-    context 'for output: :metrics_queries' do
-      it 'generates the service ping' do
-        expect(Gitlab::UsageData).to receive(:data).and_return(usage_data)
+    context 'when no caching' do
+      let(:new_usage_data) { { uuid: "1112" } }
 
-        described_class.for(output: :metrics_queries)
-      end
-    end
+      it 'returns fresh data' do
+        allow(Gitlab::UsageData).to receive(:data).and_return(usage_data, new_usage_data)
 
-    context 'for output: :non_sql_metrics_values' do
-      it 'generates the service ping' do
-        expect(Gitlab::UsageData).to receive(:data).and_return(usage_data)
+        expect(described_class.for(output: :all_metrics_values)).to eq(usage_data)
+        expect(described_class.for(output: :all_metrics_values)).to eq(new_usage_data)
 
-        described_class.for(output: :non_sql_metrics_values)
+        expect(Rails.cache.fetch('usage_data')).to eq(new_usage_data)
       end
     end
   end

spec/lib/gitlab/usage_data_spec.rb

@@ -507,10 +507,7 @@ RSpec.describe Gitlab::UsageData, :aggregate_failures do
     end
 
     it 'gathers usage counts', :aggregate_failures do
-      stub_feature_flags(merge_service_ping_instrumented_metrics: false)
-
       count_data = subject[:counts]
-      expect(count_data[:boards]).to eq(1)
       expect(count_data[:projects]).to eq(4)
       expect(count_data.keys).to include(*UsageDataHelpers::COUNTS_KEYS)
       expect(UsageDataHelpers::COUNTS_KEYS - count_data.keys).to be_empty

spec/services/service_ping/build_payload_service_spec.rb

@@ -4,10 +4,6 @@ require 'spec_helper'
 
 RSpec.describe ServicePing::BuildPayloadService do
   describe '#execute', :without_license do
-    before do
-      stub_feature_flags(merge_service_ping_instrumented_metrics: false)
-    end
-
     subject(:service_ping_payload) { described_class.new.execute }
 
     include_context 'stubbed service ping metrics definitions' do

spec/support/helpers/usage_data_helpers.rb

@@ -26,7 +26,6 @@ module UsageDataHelpers
 
   COUNTS_KEYS = %i(
       assignee_lists
-      boards
       ci_builds
       ci_internal_pipelines
       ci_external_pipelines

spec/support/shared_contexts/services/service_ping/stubbed_service_ping_metrics_definitions_shared_context.rb

@@ -21,7 +21,7 @@ RSpec.shared_context 'stubbed service ping metrics definitions' do
 
   let(:optional_metrics) do
     [
-      metric_attributes('counts.boards', 'optional', 'number'),
+      metric_attributes('counts.boards', 'optional', 'number', 'CountBoardsMetric'),
       metric_attributes('gitaly.filesystems', '').except('data_category'),
       metric_attributes('usage_activity_by_stage.monitor.projects_with_enabled_alert_integrations_histogram', 'optional', 'object'),
       metric_attributes('topology', 'optional', 'object')
@@ -43,11 +43,14 @@ RSpec.shared_context 'stubbed service ping metrics definitions' do
     Gitlab::Usage::MetricDefinition.instance_variable_set(:@all, nil)
   end
 
-  def metric_attributes(key_path, category, value_type = 'string')
+  def metric_attributes(key_path, category, value_type = 'string', instrumentation_class = '')
     {
       'key_path' => key_path,
       'data_category' => category,
-      'value_type' => value_type
+      'value_type' => value_type,
+      'status' => 'active',
+      'instrumentation_class' => instrumentation_class,
+      'time_frame' => 'all'
     }
   end
 end