Merge branch '346640-add-new-service-to-use-instrumentation-classes' into 'master'

Add new service to use instrumentation classes

See merge request gitlab-org/gitlab!77629

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: false

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

@@ -83,6 +83,8 @@ 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)
 

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

@@ -16,6 +16,7 @@ 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/metric_definition.rb

@@ -80,6 +80,10 @@ module Gitlab
           @all ||= definitions.map { |_key_path, definition| definition }
         end
 
+        def not_removed
+          all.select { |definition| definition.attributes[:status] != 'removed' }.index_by(&:key_path)
+        end
+
         def with_instrumentation_class
           all.select { |definition| definition.attributes[:instrumentation_class].present? && definition.available? }
         end

lib/gitlab/usage/service_ping/instrumented_payload.rb

+# frozen_string_literal: true
+
+# Service Ping payload build using the instrumentation classes
+# for given metrics key_paths and output method
+module Gitlab
+  module Usage
+    module ServicePing
+      class InstrumentedPayload
+        attr_reader :metrics_key_paths
+        attr_reader :output_method
+
+        def initialize(metrics_key_paths, output_method)
+          @metrics_key_paths = metrics_key_paths
+          @output_method = output_method
+        end
+
+        def build
+          metrics_key_paths.map do |key_path|
+            compute_instrumental_value(key_path, output_method)
+          end.reduce({}, :deep_merge)
+        end
+
+        private
+
+        # Not all metrics defintions have instrumentation classes
+        # The value can be computed only for those that have it
+        def instrumented_metrics_defintions
+          Gitlab::Usage::MetricDefinition.with_instrumentation_class
+        end
+
+        def compute_instrumental_value(key_path, output_method)
+          definition = instrumented_metrics_defintions.find { |df| df.key_path == key_path }
+
+          return {} unless definition.present?
+
+          Gitlab::Usage::Metric.new(definition).method(output_method).call
+        end
+      end
+    end
+  end
+end

lib/gitlab/usage/service_ping/payload_keys_processor.rb

+# frozen_string_literal: true
+
+# Process the UsageData payload to get the keys that have a metric defintion
+# Get the missing keys from the payload
+module Gitlab
+  module Usage
+    module ServicePing
+      class PayloadKeysProcessor
+        attr_reader :old_payload
+
+        def initialize(old_payload)
+          @old_payload = old_payload
+        end
+
+        def key_paths
+          @key_paths ||= payload_keys.to_a.flatten.compact
+        end
+
+        def missing_instrumented_metrics_key_paths
+          @missing_key_paths ||= metrics_with_instrumentation.map(&:key_path) - key_paths
+        end
+
+        private
+
+        def payload_keys(payload = old_payload, parents = [])
+          return unless payload.is_a?(Hash)
+
+          payload.map do |key, value|
+            if has_metric_definition?(key, parents)
+              parents.dup.append(key).join('.')
+            else
+              payload_keys(value, parents.dup << key) if value.is_a?(Hash)
+            end
+          end
+        end
+
+        def has_metric_definition?(key, parent_keys)
+          key_path = parent_keys.dup.append(key).join('.')
+          metric_definitions.key?(key_path)
+        end
+
+        def metric_definitions
+          ::Gitlab::Usage::MetricDefinition.not_removed
+        end
+
+        def metrics_with_instrumentation
+          ::Gitlab::Usage::MetricDefinition.with_instrumentation_class
+        end
+      end
+    end
+  end
+end

lib/gitlab/usage/service_ping_report.rb

@@ -7,16 +7,29 @@ module Gitlab
         def for(output:, cached: false)
           case output.to_sym
           when :all_metrics_values
-            all_metrics_values(cached)
+            with_instrumentation_classes(all_metrics_values(cached), :with_value)
           when :metrics_queries
-            metrics_queries
+            with_instrumentation_classes(metrics_queries, :with_instrumentation)
           when :non_sql_metrics_values
-            non_sql_metrics_values
+            with_instrumentation_classes(non_sql_metrics_values, :with_instrumentation)
           end
         end
 
         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_payload = Gitlab::Usage::ServicePing::InstrumentedPayload.new(instrumented_metrics_key_paths, output_method).build
+
+            old_payload.deep_merge(instrumented_payload)
+          else
+            old_payload
+          end
+        end
+
         def all_metrics_values(cached)
           Rails.cache.fetch('usage_data', force: !cached, expires_in: 2.weeks) do
             Gitlab::UsageData.data

lib/gitlab/usage_data.rb

@@ -70,10 +70,9 @@ 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),
-            boards: add_metric('CountBoardsMetric', time_frame: 'all'),
             ci_builds: count(::Ci::Build),
             ci_internal_pipelines: count(::Ci::Pipeline.internal),
             ci_external_pipelines: count(::Ci::Pipeline.external),
@@ -167,6 +166,12 @@ 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/metric_definition_spec.rb

@@ -50,6 +50,28 @@ RSpec.describe Gitlab::Usage::MetricDefinition do
     expect { described_class.definitions }.not_to raise_error
   end
 
+  describe 'not_removed' do
+    let(:all_definitions) do
+      metrics_definitions = [
+        { key_path: 'metric1', instrumentation_class: 'RedisHLLMetric', status: 'active' },
+        { key_path: 'metric2', instrumentation_class: 'RedisHLLMetric', status: 'broken' },
+        { key_path: 'metric3', instrumentation_class: 'RedisHLLMetric', status: 'active' },
+        { key_path: 'metric4', instrumentation_class: 'RedisHLLMetric', status: 'removed' }
+      ]
+      metrics_definitions.map { |definition| described_class.new(definition[:key_path], definition.symbolize_keys) }
+    end
+
+    before do
+      allow(described_class).to receive(:all).and_return(all_definitions)
+    end
+
+    it 'includes metrics that are not removed' do
+      expect(described_class.not_removed.count).to eq(3)
+
+      expect(described_class.not_removed.keys).to match_array(%w(metric1 metric2 metric3))
+    end
+  end
+
   describe '#with_instrumentation_class' do
     let(:metric_status) { 'active' }
     let(:all_definitions) do

spec/lib/gitlab/usage/metrics/names_suggestions/generator_spec.rb

@@ -27,8 +27,8 @@ RSpec.describe Gitlab::Usage::Metrics::NamesSuggestions::Generator do
     context 'for count with default column metrics' do
       it_behaves_like 'name suggestion' do
         # corresponding metric is collected with count(Board)
-        let(:key_path) { 'counts.boards' }
-        let(:name_suggestion) { /count_boards/ }
+        let(:key_path) { 'counts.issues' }
+        let(:name_suggestion) { /count_issues/ }
       end
     end
 

spec/lib/gitlab/usage/service_ping/instrumented_payload_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Usage::ServicePing::InstrumentedPayload do
+  let(:uuid) { "0000-0000-0000" }
+
+  before do
+    allow(ApplicationRecord.connection).to receive(:transaction_open?).and_return(false)
+    allow(Gitlab::CurrentSettings).to receive(:uuid).and_return(uuid)
+  end
+
+  context 'when building service ping with values' do
+    let(:metrics_key_paths) { %w(counts.boards uuid redis_hll_counters.search.i_search_total_monthly) }
+    let(:expected_payload) do
+      {
+        counts: { boards: 0 },
+        redis_hll_counters: { search: { i_search_total_monthly: 0 } },
+        uuid: uuid
+      }
+    end
+
+    it 'builds the service ping payload for the metrics key_paths' do
+      expect(described_class.new(metrics_key_paths, :with_value).build).to eq(expected_payload)
+    end
+  end
+
+  context 'when building service ping with instrumentations' do
+    let(:metrics_key_paths) { %w(counts.boards uuid redis_hll_counters.search.i_search_total_monthly) }
+    let(:expected_payload) do
+      {
+        counts: { boards: "SELECT COUNT(\"boards\".\"id\") FROM \"boards\"" },
+        redis_hll_counters: { search: { i_search_total_monthly: 0 } },
+        uuid: uuid
+      }
+    end
+
+    it 'builds the service ping payload for the metrics key_paths' do
+      expect(described_class.new(metrics_key_paths, :with_instrumentation).build).to eq(expected_payload)
+    end
+  end
+
+  context 'when missing instrumentation class' do
+    it 'returns empty hash' do
+      expect(described_class.new(['counts.ci_builds'], :with_instrumentation).build).to eq({})
+      expect(described_class.new(['counts.ci_builds'], :with_value).build).to eq({})
+    end
+  end
+end

spec/lib/gitlab/usage/service_ping/payload_keys_processor_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Usage::ServicePing::PayloadKeysProcessor do
+  context 'with an object metric' do
+    let(:payload) { { counts: { issues: 1, boards: 1 }, topology: { duration_d: 100 }, redis_hll_counters: { search: { i_search_total_monthly: 1 } } } }
+
+    it 'returns the payload keys that have a metric definition' do
+      expect(described_class.new(payload).key_paths).to match_array(['counts.issues', 'counts.boards', 'topology', 'redis_hll_counters.search.i_search_total_monthly'])
+    end
+  end
+
+  context 'with a missing metric definition' do
+    let(:payload) { { counts: { issues: 1, boards: 1 }, missing_definition: 1, topology: { duration_d: 100 } } }
+
+    it 'returns the payload keys that have a metric definition' do
+      expect(described_class.new(payload).key_paths).to match_array(['counts.issues', 'counts.boards', 'topology'])
+    end
+  end
+
+  context 'with array metric' do
+    let(:payload) { { counts: { issues: 1, boards: 1 }, settings: { collected_data_categories: ['standard'] }, topology: { duration_d: 100 } } }
+
+    it 'returns the payload keys that have a metric definition' do
+      expect(described_class.new(payload).key_paths).to match_array(['counts.issues', 'counts.boards', 'topology', 'settings.collected_data_categories'])
+    end
+  end
+
+  context 'missing_instrumented_metrics_key_paths' do
+    let(:payload) do
+      {
+        counts: { issues: 1, boards: 1 },
+        topology: { duration_d: 100 },
+        redis_hll_counters: { search: { i_search_total_monthly: 1 } }
+      }
+    end
+
+    let(:metrics_definitions) do
+      [
+        double(:issues, key_path: 'counts.issues'),
+        double(:topology, key_path: 'topology'),
+        double(:i_search_total_monthly, key_path: 'redis_hll_counters.search.i_search_total_monthly'),
+        double(:collected_data_categories, key_path: 'settings.collected_data_categories')
+      ]
+    end
+
+    before do
+      allow(::Gitlab::Usage::MetricDefinition).to receive(:with_instrumentation_class).and_return(metrics_definitions)
+    end
+
+    it 'returns the missing keys' do
+      expect(described_class.new(payload).missing_instrumented_metrics_key_paths).to match_array(['settings.collected_data_categories'])
+    end
+  end
+end

spec/lib/gitlab/usage/service_ping_report_spec.rb

@@ -3,66 +3,121 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Usage::ServicePingReport, :use_clean_rails_memory_store_caching do
-  let(:usage_data) { { uuid: "1111" } }
+  let(:usage_data) { { uuid: "1111", counts: { issue: 0 } } }
 
-  context 'for output: :all_metrics_values' do
-    it 'generates the service ping' do
-      expect(Gitlab::UsageData).to receive(:data)
+  context 'when feature merge_service_ping_instrumented_metrics enabled' do
+    before do
+      stub_feature_flags(merge_service_ping_instrumented_metrics: true)
 
-      described_class.for(output: :all_metrics_values)
+      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
-  end
 
-  context 'for output: :metrics_queries' do
-    it 'generates the service ping' do
-      expect(Gitlab::UsageDataQueries).to receive(: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 } })
+      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
 
-      described_class.for(output: :metrics_queries)
+        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
-  end
 
-  context 'for output: :non_sql_metrics_values' do
-    it 'generates the service ping' do
-      expect(Gitlab::UsageDataNonSqlMetrics).to receive(: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)
+        described_class.for(output: :metrics_queries)
+      end
     end
-  end
 
-  context 'when using cached' do
-    context 'for cached: true' do
-      let(:new_usage_data) { { uuid: "1112" } }
+    context 'for output: :non_sql_metrics_values' 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
+    end
 
-      it 'caches the values' do
-        allow(Gitlab::UsageData).to receive(:data).and_return(usage_data, new_usage_data)
+    context 'when using cached' do
+      context 'for cached: true' do
+        let(:new_usage_data) { { uuid: "1112" } }
 
-        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)
+        it 'caches the values' do
+          allow(Gitlab::UsageData).to receive(:data).and_return(usage_data, new_usage_data)
 
-        expect(Rails.cache.fetch('usage_data')).to eq(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(usage_data)
+        end
+
+        it 'writes to cache and 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(described_class.for(output: :all_metrics_values, cached: true)).to eq(new_usage_data)
+
+          expect(Rails.cache.fetch('usage_data')).to eq(new_usage_data)
+        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 no caching' do
+        let(:new_usage_data) { { uuid: "1112" } }
 
-        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)
+        it 'returns fresh data' do
+          allow(Gitlab::UsageData).to receive(:data).and_return(usage_data, new_usage_data)
 
-        expect(Rails.cache.fetch('usage_data')).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)).to eq(new_usage_data)
+
+          expect(Rails.cache.fetch('usage_data')).to eq(new_usage_data)
+        end
       end
     end
+  end
 
-    context 'when no caching' do
-      let(:new_usage_data) { { uuid: "1112" } }
+  context 'when feature merge_service_ping_instrumented_metrics disabled' do
+    before do
+      stub_feature_flags(merge_service_ping_instrumented_metrics: false)
+    end
 
-      it 'returns fresh data' do
-        allow(Gitlab::UsageData).to receive(:data).and_return(usage_data, 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 } })
+      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
+    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)
+    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(Rails.cache.fetch('usage_data')).to eq(new_usage_data)
+        described_class.for(output: :non_sql_metrics_values)
       end
     end
   end

spec/lib/gitlab/usage_data_spec.rb

@@ -507,6 +507,8 @@ 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)

spec/services/service_ping/build_payload_service_spec.rb

@@ -4,6 +4,10 @@ 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