From aad6fcea77fa2a1397c5cc16fdfc364e66954f8f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Micha=C5=82=20Wielich?= <mwielich@gitlab.com>
Date: Thu, 21 Apr 2022 08:41:36 +0000
Subject: [PATCH] Add metric availability to instrumentation classes
Make it possible to define availability on a per-metric-class basis
---
.../service_ping/metrics_instrumentation.md | 66 +++++++++++++++++++
.../metrics/every_metric_definition_spec.rb | 1 +
lib/gitlab/usage/metric.rb | 16 +++--
.../metrics/instrumentations/base_metric.rb | 16 +++++
spec/lib/gitlab/usage/metric_spec.rb | 27 ++++++++
.../instrumentations/database_metric_spec.rb | 27 ++++++++
.../instrumentations/redis_hll_metric_spec.rb | 24 +++++++
.../instrumentations/redis_metric_spec.rb | 24 +++++++
8 files changed, 196 insertions(+), 5 deletions(-)
diff --git a/doc/development/service_ping/metrics_instrumentation.md b/doc/development/service_ping/metrics_instrumentation.md
index 3d56f3e777fcb..42f93d6ae24e6 100644
--- a/doc/development/service_ping/metrics_instrumentation.md
+++ b/doc/development/service_ping/metrics_instrumentation.md
@@ -40,6 +40,7 @@ We have built a domain-specific language (DSL) to define the metrics instrumenta
- `start`: Specifies the start value of the batch counting, by default is `relation.minimum(:id)`.
- `finish`: Specifies the end value of the batch counting, by default is `relation.maximum(:id)`.
- `cache_start_and_finish_as`: Specifies the cache key for `start` and `finish` values and sets up caching them. Use this call when `start` and `finish` are expensive queries that should be reused between different metric calculations.
+- `available?`: Specifies whether the metric should be reported. The default is `true`.
[Example of a merge request that adds a database metric](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/60022).
@@ -123,6 +124,37 @@ options:
counter_class: SourceCodeCounter
```
+### Availability-restrained Redis metrics
+
+If the Redis metric should only be available in the report under some conditions, then you must specify these conditions in a new class that is a child of the `RedisMetric` class.
+
+```ruby
+# frozen_string_literal: true
+
+module Gitlab
+ module Usage
+ module Metrics
+ module Instrumentations
+ class MergeUsageCountRedisMetric < RedisMetric
+ available? { Feature.enabled?(:merge_usage_data_missing_key_paths) }
+ end
+ end
+ end
+ end
+end
+```
+
+You must also use the class's name in the YAML setup.
+
+```yaml
+time_frame: all
+data_source: redis
+instrumentation_class: 'MergeUsageCountRedisMetric'
+options:
+ event: pushes
+ counter_class: SourceCodeCounter
+```
+
## Redis HyperLogLog metrics
[Example of a merge request that adds a `RedisHLL` metric](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/61685).
@@ -138,8 +170,42 @@ options:
- i_quickactions_approve
```
+### Availability-restrained Redis HyperLogLog metrics
+
+If the Redis HyperLogLog metric should only be available in the report under some conditions, then you must specify these conditions in a new class that is a child of the `RedisHLLMetric` class.
+
+```ruby
+# frozen_string_literal: true
+
+module Gitlab
+ module Usage
+ module Metrics
+ module Instrumentations
+ class MergeUsageCountRedisHLLMetric < RedisHLLMetric
+ available? { Feature.enabled?(:merge_usage_data_missing_key_paths) }
+ end
+ end
+ end
+ end
+end
+```
+
+You must also use the class's name in the YAML setup.
+
+```yaml
+time_frame: 28d
+data_source: redis_hll
+instrumentation_class: 'MergeUsageCountRedisHLLMetric'
+options:
+ events:
+ - i_quickactions_approve
+```
+
## Generic metrics
+- `value`: Specifies the value of the metric.
+- `available?`: Specifies whether the metric should be reported. The default is `true`.
+
[Example of a merge request that adds a generic metric](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/60256).
```ruby
diff --git a/ee/spec/config/metrics/every_metric_definition_spec.rb b/ee/spec/config/metrics/every_metric_definition_spec.rb
index dcffdbcbe2ced..655781d668f96 100644
--- a/ee/spec/config/metrics/every_metric_definition_spec.rb
+++ b/ee/spec/config/metrics/every_metric_definition_spec.rb
@@ -104,6 +104,7 @@ def assert_uses_all_nested_classes(parent_module)
if constant.is_a? Class
metric_class_instance = instance_double(constant)
expect(constant).to receive(:new).at_least(:once).and_return(metric_class_instance)
+ allow(metric_class_instance).to receive(:available?).and_return(true)
expect(metric_class_instance).to receive(:value).at_least(:once)
elsif constant.is_a? Module
assert_uses_all_nested_classes(constant)
diff --git a/lib/gitlab/usage/metric.rb b/lib/gitlab/usage/metric.rb
index 24e044c574002..cf48aa49938c3 100644
--- a/lib/gitlab/usage/metric.rb
+++ b/lib/gitlab/usage/metric.rb
@@ -18,19 +18,25 @@ def all
end
def with_value
- unflatten_key_path(intrumentation_object.value)
+ with_availability(proc { instrumentation_object.value })
end
def with_instrumentation
- unflatten_key_path(intrumentation_object.instrumentation)
+ with_availability(proc { instrumentation_object.instrumentation })
end
def with_suggested_name
- unflatten_key_path(intrumentation_object.suggested_name)
+ with_availability(proc { instrumentation_object.suggested_name })
end
private
+ def with_availability(value_proc)
+ return {} unless instrumentation_object.available?
+
+ unflatten_key_path(value_proc.call)
+ end
+
def unflatten_key_path(value)
::Gitlab::Usage::Metrics::KeyPathProcessor.process(definition.key_path, value)
end
@@ -39,8 +45,8 @@ def instrumentation_class
"Gitlab::Usage::Metrics::Instrumentations::#{definition.instrumentation_class}"
end
- def intrumentation_object
- instrumentation_class.constantize.new(
+ def instrumentation_object
+ @instrumentation_object ||= instrumentation_class.constantize.new(
time_frame: definition.time_frame,
options: definition.attributes[:options]
)
diff --git a/lib/gitlab/usage/metrics/instrumentations/base_metric.rb b/lib/gitlab/usage/metrics/instrumentations/base_metric.rb
index a264f9484f3a8..f76ed1753b28f 100644
--- a/lib/gitlab/usage/metrics/instrumentations/base_metric.rb
+++ b/lib/gitlab/usage/metrics/instrumentations/base_metric.rb
@@ -11,6 +11,18 @@ class BaseMetric
attr_reader :time_frame
attr_reader :options
+ class << self
+ def available?(&block)
+ return @metric_available = block if block_given?
+
+ return @metric_available.call if instance_variable_defined?('@metric_available')
+
+ true
+ end
+
+ attr_reader :metric_available
+ end
+
def initialize(time_frame:, options: {})
@time_frame = time_frame
@options = options
@@ -19,6 +31,10 @@ def initialize(time_frame:, options: {})
def instrumentation
value
end
+
+ def available?
+ self.class.available?
+ end
end
end
end
diff --git a/spec/lib/gitlab/usage/metric_spec.rb b/spec/lib/gitlab/usage/metric_spec.rb
index 19d2d3048eb7b..10ae94e746b43 100644
--- a/spec/lib/gitlab/usage/metric_spec.rb
+++ b/spec/lib/gitlab/usage/metric_spec.rb
@@ -51,4 +51,31 @@
expect(described_class.new(issue_count_metric_definiton).with_suggested_name).to eq({ counts: { issues: 'count_issues' } })
end
end
+
+ context 'unavailable metric' do
+ let(:instrumentation_class) { "UnavailableMetric" }
+ let(:issue_count_metric_definiton) do
+ double(:issue_count_metric_definiton,
+ attributes.merge({ attributes: attributes, instrumentation_class: instrumentation_class })
+ )
+ end
+
+ before do
+ unavailable_metric_class = Class.new(Gitlab::Usage::Metrics::Instrumentations::CountIssuesMetric) do
+ def available?
+ false
+ end
+ end
+
+ stub_const("Gitlab::Usage::Metrics::Instrumentations::#{instrumentation_class}", unavailable_metric_class)
+ end
+
+ [:with_value, :with_instrumentation, :with_suggested_name].each do |method_name|
+ describe "##{method_name}" do
+ it 'returns an empty hash' do
+ expect(described_class.new(issue_count_metric_definiton).public_send(method_name)).to eq({})
+ end
+ end
+ end
+ end
end
diff --git a/spec/lib/gitlab/usage/metrics/instrumentations/database_metric_spec.rb b/spec/lib/gitlab/usage/metrics/instrumentations/database_metric_spec.rb
index ea5ae1970dedf..cd7eb44c83d6c 100644
--- a/spec/lib/gitlab/usage/metrics/instrumentations/database_metric_spec.rb
+++ b/spec/lib/gitlab/usage/metrics/instrumentations/database_metric_spec.rb
@@ -71,6 +71,33 @@
end
end
+ context 'with availability defined' do
+ subject do
+ described_class.tap do |metric_class|
+ metric_class.relation { Issue }
+ metric_class.operation :count
+ metric_class.available? { false }
+ end.new(time_frame: 'all')
+ end
+
+ it 'responds to #available? properly' do
+ expect(subject.available?).to eq(false)
+ end
+ end
+
+ context 'with availability not defined' do
+ subject do
+ Class.new(described_class) do
+ relation { Issue }
+ operation :count
+ end.new(time_frame: 'all')
+ end
+
+ it 'responds to #available? properly' do
+ expect(subject.available?).to eq(true)
+ end
+ end
+
context 'with cache_start_and_finish_as called' do
subject do
described_class.tap do |metric_class|
diff --git a/spec/lib/gitlab/usage/metrics/instrumentations/redis_hll_metric_spec.rb b/spec/lib/gitlab/usage/metrics/instrumentations/redis_hll_metric_spec.rb
index 347a2c779cb2a..9730605153326 100644
--- a/spec/lib/gitlab/usage/metrics/instrumentations/redis_hll_metric_spec.rb
+++ b/spec/lib/gitlab/usage/metrics/instrumentations/redis_hll_metric_spec.rb
@@ -25,4 +25,28 @@
it 'raise exception if events options is not present' do
expect { described_class.new(time_frame: '28d') }.to raise_error(ArgumentError)
end
+
+ describe 'children classes' do
+ let(:options) { { events: ['i_quickactions_approve'] } }
+
+ context 'availability not defined' do
+ subject { Class.new(described_class).new(time_frame: nil, options: options) }
+
+ it 'returns default availability' do
+ expect(subject.available?).to eq(true)
+ end
+ end
+
+ context 'availability defined' do
+ subject do
+ Class.new(described_class) do
+ available? { false }
+ end.new(time_frame: nil, options: options)
+ end
+
+ it 'returns defined availability' do
+ expect(subject.available?).to eq(false)
+ end
+ end
+ end
end
diff --git a/spec/lib/gitlab/usage/metrics/instrumentations/redis_metric_spec.rb b/spec/lib/gitlab/usage/metrics/instrumentations/redis_metric_spec.rb
index fb3bd1ba8341d..831f775ec9aaa 100644
--- a/spec/lib/gitlab/usage/metrics/instrumentations/redis_metric_spec.rb
+++ b/spec/lib/gitlab/usage/metrics/instrumentations/redis_metric_spec.rb
@@ -20,4 +20,28 @@
it 'raises an exception if counter_class option is not present' do
expect { described_class.new(event: 'pushes') }.to raise_error(ArgumentError)
end
+
+ describe 'children classes' do
+ let(:options) { { event: 'pushes', counter_class: 'SourceCodeCounter' } }
+
+ context 'availability not defined' do
+ subject { Class.new(described_class).new(time_frame: nil, options: options) }
+
+ it 'returns default availability' do
+ expect(subject.available?).to eq(true)
+ end
+ end
+
+ context 'availability defined' do
+ subject do
+ Class.new(described_class) do
+ available? { false }
+ end.new(time_frame: nil, options: options)
+ end
+
+ it 'returns defined availability' do
+ expect(subject.available?).to eq(false)
+ end
+ end
+ end
end
--
GitLab