Merge branch '338029-merge-instrumentation-classes-data-to-service-ping-data' into 'master'

Merge instrumentation classes data to service ping data

See merge request gitlab-org/gitlab!68808

config/feature_flags/development/usage_data_instrumentation.yml

+---
+name: usage_data_instrumentation
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68808
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/338029
+milestone: '14.5'
+type: development
+group: group::product intelligence
+default_enabled: false

ee/spec/config/metrics/every_metric_definition_spec.rb

@@ -86,10 +86,37 @@ RSpec.describe 'Every metric definition' do
     stub_usage_data_connections
   end
 
-  it 'is included in the Usage Ping hash structure', :aggregate_failures do
-    msg = "see https://docs.gitlab.com/ee/development/service_ping/metrics_dictionary.html#metrics-added-dynamic-to-service-ping-payload"
-    expect(metric_files_key_paths).to match_array(usage_ping_key_paths)
-    expect(metric_files_key_paths).to match_array(usage_ping_key_paths), msg
+  context 'with usage_data_instrumentation feature flag' do
+    let(:msg) { 'see https://docs.gitlab.com/ee/development/service_ping/metrics_dictionary.html#metrics-added-dynamic-to-service-ping-payload' }
+
+    context 'when enabled' do
+      let(:instrumentation_class_keys) do
+        Gitlab::Usage::MetricDefinition.with_instrumentation_class.map(&:key)
+        .reject { |v| v =~ Regexp.union(ignored_usage_ping_key_patterns) }
+      end
+
+      let(:files_key_paths) do
+        (metric_files_key_paths + instrumentation_class_keys).to_set
+      end
+
+      before do
+        stub_feature_flags(usage_data_instrumentation: true)
+      end
+
+      it 'is included in the Usage Ping hash structure', :aggregate_failures do
+        expect(files_key_paths).to eql(usage_ping_key_paths.to_set), msg
+      end
+    end
+
+    context 'when disabled' do
+      before do
+        stub_feature_flags(usage_data_instrumentation: false)
+      end
+
+      it 'is included in the Usage Ping hash structure', :aggregate_failures do
+        expect(metric_files_key_paths).to match_array(usage_ping_key_paths), msg
+      end
+    end
   end
 
   context 'with value json schema' do

ee/spec/lib/ee/gitlab/usage_data_non_sql_metrics_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe Gitlab::UsageDataNonSqlMetrics do
   end
 
   describe '.uncached_data' do
-    it 'does not make DB calls' do
+    it 'does make instrumentations_class DB calls' do
       recorder = ActiveRecord::QueryRecorder.new do
         described_class.uncached_data
       end

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

@@ -62,7 +62,7 @@ RSpec.describe Gitlab::UsageData do
     subject { described_class.data }
 
     it 'gathers usage data' do
-      expect(subject.keys).to include(*%i(
+      expect(subject.keys).to include(*%w(
         historical_max_users
         license_add_ons
         license_plan
@@ -85,7 +85,7 @@ RSpec.describe Gitlab::UsageData do
       expect(count_data[:boards]).to eq(1)
       expect(count_data[:projects]).to eq(3)
 
-      expect(count_data.keys).to include(*%i(
+      expect(count_data.keys).to include(*%w(
         confidential_epics
         container_scanning_jobs
         coverage_fuzzing_jobs
@@ -200,10 +200,7 @@ RSpec.describe Gitlab::UsageData do
     it 'gathers license data' do
       license = ::License.current
 
-      expect(subject[:license_md5]).to eq(Digest::MD5.hexdigest(license.data))
       expect(subject[:license_id]).to eq(license.license_id)
-      expect(subject[:historical_max_users]).to eq(license.historical_max)
-      expect(subject[:licensee]).to eq(license.licensee)
       expect(subject[:license_user_count]).to eq(license.restricted_user_count)
       expect(subject[:license_starts_at]).to eq(license.starts_at)
       expect(subject[:license_expires_at]).to eq(license.expires_at)
@@ -212,6 +209,39 @@ RSpec.describe Gitlab::UsageData do
       expect(subject[:license_subscription_id]).to eq(license.subscription_id)
       expect(subject[:license_billable_users]).to eq(license.daily_billable_users_count)
     end
+
+    context 'with usage_data_instrumentation feature flag' do
+      let(:license) { ::License.current }
+
+      context 'when enabled' do
+        before do
+          stub_feature_flags(usage_data_instrumentation: true)
+        end
+
+        it 'returns fallback value to be overriden' do
+          expect(subject[:licensee]).to eq(Gitlab::Utils::UsageData::INSTRUMENTATION_CLASS_FALLBACK)
+          expect(subject[:license_md5]).to eq(Gitlab::Utils::UsageData::INSTRUMENTATION_CLASS_FALLBACK)
+          expect(subject[:historical_max_users]).to eq(Gitlab::Utils::UsageData::INSTRUMENTATION_CLASS_FALLBACK)
+
+          uncached_data = described_class.uncached_data
+          expect(uncached_data[:licensee]).to eq(license.licensee)
+          expect(uncached_data[:license_md5]).to eq(Digest::MD5.hexdigest(license.data))
+          expect(uncached_data[:historical_max_users]).to eq(license.historical_max)
+        end
+      end
+
+      context 'when disabled' do
+        before do
+          stub_feature_flags(usage_data_instrumentation: false)
+        end
+
+        it 'computes historical_max_users, licensee and license_md5 values' do
+          expect(subject[:licensee]).to eq(license.licensee)
+          expect(subject[:license_md5]).to eq(Digest::MD5.hexdigest(license.data))
+          expect(subject[:historical_max_users]).to eq(license.historical_max)
+        end
+      end
+    end
   end
 
   describe '.requirements_counts' do
@@ -541,7 +571,7 @@ RSpec.describe Gitlab::UsageData do
   end
 
   describe 'usage_activity_by_stage_release' do
-    it 'includes accurate usage_activity_by_stage data' do
+    before do
       stub_licensed_features(group_milestone_project_releases: true)
       group_milestone = create(:milestone, :on_group)
       project = create(:project, group: group_milestone.group)
@@ -551,15 +581,41 @@ RSpec.describe Gitlab::UsageData do
 
         create(:release, created_at: 3.days.ago, project: project, milestones: [group_milestone])
       end
+    end
 
-      expect(described_class.usage_activity_by_stage_release({})).to include(
-        projects_mirrored_with_pipelines_enabled: 2,
-        releases_with_group_milestones: 2
-      )
-      expect(described_class.usage_activity_by_stage_release(described_class.monthly_time_range_db_params)).to include(
-        projects_mirrored_with_pipelines_enabled: 1,
-        releases_with_group_milestones: 1
-      )
+    it 'includes accurate usage_activity_by_stage data' do
+      expect(described_class.usage_activity_by_stage_release({})).to include(projects_mirrored_with_pipelines_enabled: 2)
+      expect(described_class.usage_activity_by_stage_release(described_class.monthly_time_range_db_params)).to include(projects_mirrored_with_pipelines_enabled: 1)
+    end
+
+    context 'with usage_data_instrumentation feature flag' do
+      let(:license) { ::License.current }
+
+      context 'when enabled' do
+        before do
+          stub_feature_flags(usage_data_instrumentation: true)
+        end
+
+        it 'returns fallback value to be overriden' do
+          expect(described_class.usage_activity_by_stage_release({})).to include(releases_with_group_milestones: Gitlab::Utils::UsageData::INSTRUMENTATION_CLASS_FALLBACK)
+          expect(described_class.usage_activity_by_stage_release(described_class.monthly_time_range_db_params)).to include(releases_with_group_milestones: Gitlab::Utils::UsageData::INSTRUMENTATION_CLASS_FALLBACK)
+
+          uncached_data = described_class.uncached_data
+          expect(uncached_data[:usage_activity_by_stage][:release]).to include(releases_with_milestones: 2)
+          expect(uncached_data[:usage_activity_by_stage_monthly][:release]).to include(releases_with_milestones: 1)
+        end
+      end
+
+      context 'when disabled' do
+        before do
+          stub_feature_flags(usage_data_instrumentation: false)
+        end
+
+        it 'computes releases_with_group_milestones values' do
+          expect(described_class.usage_activity_by_stage_release({})).to include(releases_with_group_milestones: 2)
+          expect(described_class.usage_activity_by_stage_release(described_class.monthly_time_range_db_params)).to include(releases_with_group_milestones: 1)
+        end
+      end
     end
   end
 

lib/gitlab/usage/metric.rb

@@ -25,6 +25,10 @@ module Gitlab
         unflatten_key_path(intrumentation_object.instrumentation)
       end
 
+      def with_suggested_name
+        unflatten_key_path(intrumentation_object.suggested_name)
+      end
+
       private
 
       def unflatten_key_path(value)

lib/gitlab/usage/metrics/names_suggestions/generator.rb

@@ -18,6 +18,10 @@ module Gitlab
 
             private
 
+            def instrumentation_metrics
+              ::Gitlab::UsageDataMetrics.suggested_names
+            end
+
             def count(relation, column = nil, batch: true, batch_size: nil, start: nil, finish: nil)
               Gitlab::Usage::Metrics::NameSuggestion.for(:count, column: column, relation: relation)
             end

lib/gitlab/usage_data.rb

@@ -45,23 +45,10 @@ module Gitlab
         clear_memoized
 
         with_finished_at(:recording_ce_finished_at) do
-          license_usage_data
-            .merge(system_usage_data_license)
-            .merge(system_usage_data_settings)
-            .merge(system_usage_data)
-            .merge(system_usage_data_monthly)
-            .merge(system_usage_data_weekly)
-            .merge(features_usage_data)
-            .merge(components_usage_data)
-            .merge(object_store_usage_data)
-            .merge(topology_usage_data)
-            .merge(usage_activity_by_stage)
-            .merge(usage_activity_by_stage(:usage_activity_by_stage_monthly, monthly_time_range_db_params))
-            .merge(analytics_unique_visits_data)
-            .merge(compliance_unique_visits_data)
-            .merge(search_unique_visits_data)
-            .merge(redis_hll_counters)
-            .deep_merge(aggregated_metrics_data)
+          usage_data = usage_data_metrics
+          usage_data = usage_data.with_indifferent_access.deep_merge(instrumentation_metrics.with_indifferent_access) if Feature.enabled?(:usage_data_instrumentation)
+
+          usage_data
         end
       end
 
@@ -729,6 +716,30 @@ module Gitlab
 
       private
 
+      def usage_data_metrics
+        license_usage_data
+          .merge(system_usage_data_license)
+          .merge(system_usage_data_settings)
+          .merge(system_usage_data)
+          .merge(system_usage_data_monthly)
+          .merge(system_usage_data_weekly)
+          .merge(features_usage_data)
+          .merge(components_usage_data)
+          .merge(object_store_usage_data)
+          .merge(topology_usage_data)
+          .merge(usage_activity_by_stage)
+          .merge(usage_activity_by_stage(:usage_activity_by_stage_monthly, monthly_time_range_db_params))
+          .merge(analytics_unique_visits_data)
+          .merge(compliance_unique_visits_data)
+          .merge(search_unique_visits_data)
+          .merge(redis_hll_counters)
+          .deep_merge(aggregated_metrics_data)
+      end
+
+      def instrumentation_metrics
+        Gitlab::UsageDataMetrics.uncached_data # rubocop:disable UsageData/LargeTable
+      end
+
       def metric_time_period(time_period)
         time_period.present? ? '28d' : 'none'
       end

lib/gitlab/usage_data_metrics.rb

@@ -5,7 +5,17 @@ module Gitlab
     class << self
       # Build the Usage Ping JSON payload from metrics YAML definitions which have instrumentation class set
       def uncached_data
-        ::Gitlab::Usage::Metric.all.map(&:with_value).reduce({}, :deep_merge)
+        build_payload(:with_value)
+      end
+
+      def suggested_names
+        build_payload(:with_suggested_name)
+      end
+
+      private
+
+      def build_payload(method_symbol)
+        ::Gitlab::Usage::Metric.all.map(&method_symbol).reduce({}, :deep_merge)
       end
     end
   end

lib/gitlab/usage_data_non_sql_metrics.rb

@@ -6,7 +6,10 @@ module Gitlab
 
     class << self
       def uncached_data
-        super.with_indifferent_access.deep_merge(instrumentation_metrics_queries.with_indifferent_access)
+        # instrumentation_metrics is already included with feature flag enabled
+        return super if Feature.enabled?(:usage_data_instrumentation)
+
+        super.with_indifferent_access.deep_merge(instrumentation_metrics.with_indifferent_access)
       end
 
       def add_metric(metric, time_frame: 'none')
@@ -50,7 +53,7 @@ module Gitlab
 
       private
 
-      def instrumentation_metrics_queries
+      def instrumentation_metrics
         ::Gitlab::Usage::Metric.all.map(&:with_instrumentation).reduce({}, :deep_merge)
       end
     end

lib/gitlab/usage_data_queries.rb

@@ -6,7 +6,10 @@ module Gitlab
   class UsageDataQueries < UsageData
     class << self
       def uncached_data
-        super.with_indifferent_access.deep_merge(instrumentation_metrics_queries.with_indifferent_access)
+        # instrumentation_metrics is already included with feature flag enabled
+        return super if Feature.enabled?(:usage_data_instrumentation)
+
+        super.with_indifferent_access.deep_merge(instrumentation_metrics.with_indifferent_access)
       end
 
       def add_metric(metric, time_frame: 'none')
@@ -71,7 +74,7 @@ module Gitlab
 
       private
 
-      def instrumentation_metrics_queries
+      def instrumentation_metrics
         ::Gitlab::Usage::Metric.all.map(&:with_instrumentation).reduce({}, :deep_merge)
       end
     end

lib/gitlab/utils/usage_data.rb

@@ -43,8 +43,13 @@ module Gitlab
       HISTOGRAM_FALLBACK = { '-1' => -1 }.freeze
       DISTRIBUTED_HLL_FALLBACK = -2
       MAX_BUCKET_SIZE = 100
+      INSTRUMENTATION_CLASS_FALLBACK = -100
 
       def add_metric(metric, time_frame: 'none')
+        # Results of this method should be overwritten by instrumentation class values
+        # -100 indicates the metric was not properly merged.
+        return INSTRUMENTATION_CLASS_FALLBACK if Feature.enabled?(:usage_data_instrumentation)
+
         metric_class = "Gitlab::Usage::Metrics::Instrumentations::#{metric}".constantize
 
         metric_class.new(time_frame: time_frame).value

spec/lib/gitlab/usage/metric_spec.rb

@@ -45,4 +45,10 @@ RSpec.describe Gitlab::Usage::Metric do
       expect(described_class.new(issue_count_metric_definiton).with_instrumentation).to eq({ counts: { issues: "SELECT COUNT(\"issues\".\"id\") FROM \"issues\"" } })
     end
   end
+
+  describe '#with_suggested_name' do
+    it 'returns key_path metric with the corresponding generated query' do
+      expect(described_class.new(issue_count_metric_definiton).with_suggested_name).to eq({ counts: { issues: 'count_issues' } })
+    end
+  end
 end

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

@@ -25,10 +25,30 @@ RSpec.describe Gitlab::Usage::Metrics::NamesSuggestions::Generator do
     end
 
     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/ }
+      context 'with usage_data_instrumentation feature flag' do
+        context 'when enabled' do
+          before do
+            stub_feature_flags(usage_data_instrumentation: true)
+          end
+
+          it_behaves_like 'name suggestion' do
+            # corresponding metric is collected with ::Gitlab::UsageDataMetrics.suggested_names
+            let(:key_path) { 'counts.boards' }
+            let(:name_suggestion) { /count_boards/ }
+          end
+        end
+
+        context 'when disabled' do
+          before do
+            stub_feature_flags(usage_data_instrumentation: false)
+          end
+
+          it_behaves_like 'name suggestion' do
+            # corresponding metric is collected with count(Board)
+            let(:key_path) { 'counts.boards' }
+            let(:name_suggestion) { /count_boards/ }
+          end
+        end
       end
     end
 

spec/lib/gitlab/usage_data_metrics_spec.rb

@@ -76,4 +76,16 @@ RSpec.describe Gitlab::UsageDataMetrics do
       end
     end
   end
+
+  describe '.suggested_names' do
+    subject { described_class.suggested_names }
+
+    let(:suggested_names) do
+      ::Gitlab::Usage::Metric.all.map(&:with_suggested_name).reduce({}, :deep_merge)
+    end
+
+    it 'includes Service Ping suggested names' do
+      expect(subject).to match_array(suggested_names)
+    end
+  end
 end

spec/lib/gitlab/utils/usage_data_spec.rb

@@ -8,8 +8,26 @@ RSpec.describe Gitlab::Utils::UsageData do
   describe '#add_metric' do
     let(:metric) { 'UuidMetric'}
 
-    it 'computes the metric value for given metric' do
-      expect(described_class.add_metric(metric)).to eq(Gitlab::CurrentSettings.uuid)
+    context 'with usage_data_instrumentation feature flag' do
+      context 'when enabled' do
+        before do
+          stub_feature_flags(usage_data_instrumentation: true)
+        end
+
+        it 'returns -100 value to be overriden' do
+          expect(described_class.add_metric(metric)).to eq(-100)
+        end
+      end
+
+      context 'when disabled' do
+        before do
+          stub_feature_flags(usage_data_instrumentation: false)
+        end
+
+        it 'computes the metric value for given metric' do
+          expect(described_class.add_metric(metric)).to eq(Gitlab::CurrentSettings.uuid)
+        end
+      end
     end
   end