Merge branch...

Merge branch '327736-add-extra-arguments-to-metric-yaml-defintion-for-filtering-data-in-instrumentation-class' into 'master'

Add options arguments to Redis HLL Metric YAML definition for filtering data in instrumentation class

See merge request gitlab-org/gitlab!61685

changelogs/unreleased/327736-add-options-events-arguments-to-metric-yaml-defintion-for-filtering-data-in.yml

+---
+title: Add options events to Redis HLL metrics for filtering data
+merge_request: 61685
+author:
+type: other

config/metrics/counts_28d/20210216181508_i_quickactions_approve_monthly.yml

@@ -9,7 +9,10 @@ value_type: number
 status: data_available
 time_frame: 28d
 data_source: redis_hll
-instrumentation_class: CountUsersUsingApproveQuickActionMetric
+instrumentation_class: RedisHLLMetric
+options:
+  events:
+    - i_quickactions_approve
 distribution:
 - ce
 - ee

config/metrics/counts_7d/20210216181506_i_quickactions_approve_weekly.yml

@@ -9,7 +9,10 @@ value_type: number
 status: data_available
 time_frame: 7d
 data_source: redis_hll
-instrumentation_class: CountUsersUsingApproveQuickActionMetric
+instrumentation_class: RedisHLLMetric
+options:
+  events:
+    - i_quickactions_approve
 distribution:
 - ce
 - ee

config/metrics/schema.json

@@ -43,7 +43,7 @@
     "introduced_by_url": {
       "type": ["string", "null"]
     },
-    "extra": {
+    "options": {
       "type": "object"
     },
     "time_frame": {
@@ -56,7 +56,7 @@
     },
     "instrumentation_class": {
       "type": "string",
-      "pattern": "^(([A-Z][a-z]+)+::)*(([A-Z][a-z]+)+)$"
+      "pattern": "^(([A-Z][a-z]+)+::)*(([A-Z]+[a-z]+)+)$"
     },
     "distribution": {
       "type": "array",

doc/development/usage_ping/metrics_dictionary.md

@@ -43,7 +43,7 @@ Each metric is defined in a separate YAML file consisting of a number of fields:
 | `milestone`         | no       | The milestone when the metric is introduced. |
 | `milestone_removed` | no       | The milestone when the metric is removed. |
 | `introduced_by_url` | no       | The URL to the Merge Request that introduced the metric. |
-| `extra`             | no       | `object`: extra information needed to calculate the metric value. |
+| `options`           | no       | `object`: options information needed to calculate the metric value. |
 | `skip_validation`   | no       | This should **not** be set. [Used for imported metrics until we review, update and make them valid](https://gitlab.com/groups/gitlab-org/-/epics/5425). |
 
 ### Metric statuses

doc/development/usage_ping/metrics_instrumentation.md

@@ -53,20 +53,17 @@ end
 
 ## Redis HyperLogLog metrics
 
-[Example of a merge request that adds a `RedisHLL` metric](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/60089/diffs).
+[Example of a merge request that adds a `RedisHLL` metric](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/61685).
 
-```ruby
-module Gitlab
-  module Usage
-    module Metrics
-      module Instrumentations
-        class CountUsersUsingApproveQuickActionMetric < RedisHLLMetric
-          event_names :i_quickactions_approve
-        end
-      end
-    end
-  end
-end
+Count unique values for `i_quickactions_approve` event.
+
+```yaml
+time_frame: 28d
+data_source: redis_hll
+instrumentation_class: 'Gitlab::Usage::Metrics::Instrumentations::RedisHLLMetric'
+options:
+  events:
+    - i_quickactions_approve
 ```
 
 ## Generic metrics

lib/gitlab/usage/metrics/instrumentations/base_metric.rb

@@ -8,9 +8,11 @@ module Gitlab
           include Gitlab::Utils::UsageData
 
           attr_reader :time_frame
+          attr_reader :options
 
-          def initialize(time_frame:)
+          def initialize(time_frame:, options: {})
             @time_frame = time_frame
+            @options = options
           end
         end
       end

lib/gitlab/usage/metrics/instrumentations/count_users_using_approve_quick_action_metric.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module Usage
-    module Metrics
-      module Instrumentations
-        class CountUsersUsingApproveQuickActionMetric < RedisHLLMetric
-          event_names :i_quickactions_approve
-        end
-      end
-    end
-  end
-end

lib/gitlab/usage/metrics/instrumentations/redis_hll_metric.rb

@@ -7,20 +7,24 @@ module Gitlab
         class RedisHLLMetric < BaseMetric
           # Usage example
           #
-          # class CountUsersVisitingAnalyticsValuestreamMetric < RedisHLLMetric
-          #   event_names :g_analytics_valuestream
+          # In metric YAML defintion
+          # instrumentation_class: RedisHLLMetric
+          #   events:
+          #     - g_analytics_valuestream
           # end
-          class << self
-            def event_names(events = nil)
-              @metric_events = events
+          def initialize(time_frame:, options: {})
+            super
+
+            raise ArgumentError, "options events are required" unless metric_events.present?
           end
 
-            attr_reader :metric_events
+          def metric_events
+            options[:events]
           end
 
           def value
             redis_usage_data do
-              event_params = time_constraints.merge(event_names: self.class.metric_events)
+              event_params = time_constraints.merge(event_names: metric_events)
 
               Gitlab::UsageDataCounters::HLLRedisCounter.unique_events(**event_params)
             end
@@ -35,7 +39,7 @@ module Gitlab
             when '7d'
               { start_date: 7.days.ago.to_date, end_date: Date.current }
             else
-              raise "Unknown time frame: #{time_frame} for TimeConstraint"
+              raise "Unknown time frame: #{time_frame} for RedisHLLMetric"
             end
           end
         end

lib/gitlab/usage_data_metrics.rb

@@ -7,9 +7,12 @@ module Gitlab
       def uncached_data
         ::Gitlab::Usage::MetricDefinition.all.map do |definition|
           instrumentation_class = definition.attributes[:instrumentation_class]
+          options = definition.attributes[:options]
 
           if instrumentation_class.present?
-            metric_value = "Gitlab::Usage::Metrics::Instrumentations::#{instrumentation_class}".constantize.new(time_frame: definition.attributes[:time_frame]).value
+            metric_value = "Gitlab::Usage::Metrics::Instrumentations::#{instrumentation_class}".constantize.new(
+              time_frame: definition.attributes[:time_frame],
+              options: options).value
 
             metric_payload(definition.key_path, metric_value)
           else

spec/lib/gitlab/usage/metrics/instrumentations/count_users_using_approve_quick_action_metric_spec.rb → spec/lib/gitlab/usage/metrics/instrumentations/redis_hll_metric_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::Usage::Metrics::Instrumentations::CountUsersUsingApproveQuickActionMetric, :clean_gitlab_redis_shared_state do
+RSpec.describe Gitlab::Usage::Metrics::Instrumentations::RedisHLLMetric, :clean_gitlab_redis_shared_state do
   before do
     Gitlab::UsageDataCounters::HLLRedisCounter.track_event(:i_quickactions_approve, values: 1, time: 1.week.ago)
     Gitlab::UsageDataCounters::HLLRedisCounter.track_event(:i_quickactions_approve, values: 1, time: 2.weeks.ago)
@@ -10,6 +10,10 @@ RSpec.describe Gitlab::Usage::Metrics::Instrumentations::CountUsersUsingApproveQ
     Gitlab::UsageDataCounters::HLLRedisCounter.track_event(:i_quickactions_approve, values: 2, time: 2.months.ago)
   end
 
-  it_behaves_like 'a correct instrumented metric value', { time_frame: '28d', data_source: 'redis_hll' }, 2
-  it_behaves_like 'a correct instrumented metric value', { time_frame: '7d', data_source: 'redis_hll' }, 1
+  it_behaves_like 'a correct instrumented metric value', { time_frame: '28d', options: { events: ['i_quickactions_approve'] } }, 2
+  it_behaves_like 'a correct instrumented metric value', { time_frame: '7d', options: { events: ['i_quickactions_approve'] } }, 1
+
+  it 'raise exception if vents options is not present' do
+    expect { described_class.new(time_frame: '28d') }.to raise_error(ArgumentError)
+  end
 end

spec/support/gitlab/usage/metrics_instrumentation_shared_examples.rb

 # frozen_string_literal: true
 
-RSpec.shared_examples 'a correct instrumented metric value' do |options, expected_value|
-  let(:time_frame) { options[:time_frame] }
+RSpec.shared_examples 'a correct instrumented metric value' do |params, expected_value|
+  let(:time_frame) { params[:time_frame] }
+  let(:options) { params[:options] }
 
   before do
     allow(ActiveRecord::Base.connection).to receive(:transaction_open?).and_return(false)
   end
 
   it 'has correct value' do
-    expect(described_class.new(time_frame: time_frame).value).to eq(expected_value)
+    expect(described_class.new(time_frame: time_frame, options: options).value).to eq(expected_value)
   end
 end