Merge branch '41771-reduce-cardinality-of-metrics' into 'master'

Reduce the cardinality of GitLab metrics

Closes #41771

See merge request gitlab-org/gitlab-ce!16443

7 files changed, 167 insertions, 49 deletions

diff --git a/spec/lib/gitlab/metrics/method_call_spec.rb b/spec/lib/gitlab/metrics/method_call_spec.rb
index 41a9d1d9c90..d9379cfe674 100644
--- a/spec/lib/gitlab/metrics/method_call_spec.rb
+++ b/spec/lib/gitlab/metrics/method_call_spec.rb
@@ -5,6 +5,10 @@ describe Gitlab::Metrics::MethodCall do
   let(:method_call) { described_class.new('Foo#bar', :Foo, '#bar', transaction) }
 
   describe '#measure' do
+    after do
+      described_class.reload_metric!(:gitlab_method_call_duration_seconds)
+    end
+
     it 'measures the performance of the supplied block' do
       method_call.measure { 'foo' }
 
@@ -20,8 +24,6 @@ describe Gitlab::Metrics::MethodCall do
 
       context 'prometheus instrumentation is enabled' do
         before do
-          allow(Feature.get(:prometheus_metrics_method_instrumentation)).to receive(:enabled?).and_call_original
-          described_class.measurement_enabled_cache_expires_at.value = Time.now.to_i - 1
           Feature.get(:prometheus_metrics_method_instrumentation).enable
         end
 
@@ -31,30 +33,12 @@ describe Gitlab::Metrics::MethodCall do
           end
         end
 
-        it 'caches subsequent invocations of feature check' do
-          10.times do
-            method_call.measure { 'foo' }
-          end
-
-          expect(Feature.get(:prometheus_metrics_method_instrumentation)).to have_received(:enabled?).once
-        end
-
-        it 'expires feature check cache after 1 minute' do
-          method_call.measure { 'foo' }
-
-          Timecop.travel(1.minute.from_now) do
-            method_call.measure { 'foo' }
-          end
-
-          Timecop.travel(1.minute.from_now + 1.second) do
-            method_call.measure { 'foo' }
-          end
-
-          expect(Feature.get(:prometheus_metrics_method_instrumentation)).to have_received(:enabled?).twice
+        it 'metric is not a NullMetric' do
+          expect(described_class).not_to be_instance_of(Gitlab::Metrics::NullMetric)
         end
 
         it 'observes the performance of the supplied block' do
-          expect(described_class.call_duration_histogram)
+          expect(described_class.gitlab_method_call_duration_seconds)
             .to receive(:observe)
                   .with({ module: :Foo, method: '#bar' }, be_a_kind_of(Numeric))
 
@@ -64,14 +48,12 @@ describe Gitlab::Metrics::MethodCall do
 
       context 'prometheus instrumentation is disabled' do
         before do
-          described_class.measurement_enabled_cache_expires_at.value = Time.now.to_i - 1
-
           Feature.get(:prometheus_metrics_method_instrumentation).disable
         end
 
-        it 'does not observe the performance' do
-          expect(described_class.call_duration_histogram)
-            .not_to receive(:observe)
+        it 'observes using NullMetric' do
+          expect(described_class.gitlab_method_call_duration_seconds).to be_instance_of(Gitlab::Metrics::NullMetric)
+          expect(described_class.gitlab_method_call_duration_seconds).to receive(:observe)
 
           method_call.measure { 'foo' }
         end
@@ -81,12 +63,10 @@ describe Gitlab::Metrics::MethodCall do
     context 'when measurement is below threshold' do
       before do
         allow(method_call).to receive(:above_threshold?).and_return(false)
-
-        Feature.get(:prometheus_metrics_method_instrumentation).enable
       end
 
       it 'does not observe the performance' do
-        expect(described_class.call_duration_histogram)
+        expect(described_class.gitlab_method_call_duration_seconds)
           .not_to receive(:observe)
 
         method_call.measure { 'foo' }
@@ -96,7 +76,7 @@ describe Gitlab::Metrics::MethodCall do
 
   describe '#to_metric' do
     it 'returns a Metric instance' do
-      expect(method_call).to receive(:real_time).and_return(4.0001)
+      expect(method_call).to receive(:real_time).and_return(4.0001).twice
       expect(method_call).to receive(:cpu_time).and_return(3.0001)
 
       method_call.measure { 'foo' }
diff --git a/spec/lib/gitlab/metrics/methods_spec.rb b/spec/lib/gitlab/metrics/methods_spec.rb
new file mode 100644
index 00000000000..9d41ed2442b
--- /dev/null
+++ b/spec/lib/gitlab/metrics/methods_spec.rb
@@ -0,0 +1,137 @@
+require 'spec_helper'
+
+describe Gitlab::Metrics::Methods do
+  subject { Class.new { include Gitlab::Metrics::Methods } }
+
+  shared_context 'metric' do |metric_type, *args|
+    let(:docstring) { 'description' }
+    let(:metric_name) { :sample_metric }
+
+    describe "#define_#{metric_type}" do
+      define_method(:call_define_metric_method) do |**args|
+        subject.__send__("define_#{metric_type}", metric_name, **args)
+      end
+
+      context 'metrics access method not defined' do
+        it "defines metrics accessing method" do
+          expect(subject).not_to respond_to(metric_name)
+
+          call_define_metric_method(docstring: docstring)
+
+          expect(subject).to respond_to(metric_name)
+        end
+      end
+
+      context 'metrics access method defined' do
+        before do
+          call_define_metric_method(docstring: docstring)
+        end
+
+        it 'raises error when trying to redefine method' do
+          expect { call_define_metric_method(docstring: docstring) }.to raise_error(ArgumentError)
+        end
+
+        context 'metric is not cached' do
+          it 'calls fetch_metric' do
+            expect(subject).to receive(:init_metric).with(metric_type, metric_name, docstring: docstring)
+
+            subject.public_send(metric_name)
+          end
+        end
+
+        context 'metric is cached' do
+          before do
+            subject.public_send(metric_name)
+          end
+
+          it 'returns cached metric' do
+            expect(subject).not_to receive(:init_metric)
+
+            subject.public_send(metric_name)
+          end
+        end
+      end
+    end
+
+    describe "#fetch_#{metric_type}" do
+      let(:null_metric) { Gitlab::Metrics::NullMetric.instance }
+
+      define_method(:call_fetch_metric_method) do |**args|
+        subject.__send__("fetch_#{metric_type}", metric_name, **args)
+      end
+
+      context "when #{metric_type} is not cached" do
+        it 'initializes counter metric' do
+          allow(Gitlab::Metrics).to receive(metric_type).and_return(null_metric)
+
+          call_fetch_metric_method(docstring: docstring)
+
+          expect(Gitlab::Metrics).to have_received(metric_type).with(metric_name, docstring, *args)
+        end
+      end
+
+      context "when #{metric_type} is cached" do
+        before do
+          call_fetch_metric_method(docstring: docstring)
+        end
+
+        it 'uses class metric cache' do
+          expect(Gitlab::Metrics).not_to receive(metric_type)
+
+          call_fetch_metric_method(docstring: docstring)
+        end
+
+        context 'when metric is reloaded' do
+          before do
+            subject.reload_metric!(metric_name)
+          end
+
+          it "initializes #{metric_type} metric" do
+            allow(Gitlab::Metrics).to receive(metric_type).and_return(null_metric)
+
+            call_fetch_metric_method(docstring: docstring)
+
+            expect(Gitlab::Metrics).to have_received(metric_type).with(metric_name, docstring, *args)
+          end
+        end
+      end
+
+      context 'when metric is configured with feature' do
+        let(:feature_name) { :some_metric_feature }
+        let(:metric) { call_fetch_metric_method(docstring: docstring, with_feature: feature_name) }
+
+        context 'when feature is enabled' do
+          before do
+            Feature.get(feature_name).enable
+          end
+
+          it "initializes #{metric_type} metric" do
+            allow(Gitlab::Metrics).to receive(metric_type).and_return(null_metric)
+
+            metric
+
+            expect(Gitlab::Metrics).to have_received(metric_type).with(metric_name, docstring, *args)
+          end
+        end
+
+        context 'when feature is disabled' do
+          before do
+            Feature.get(feature_name).disable
+          end
+
+          it "returns NullMetric" do
+            allow(Gitlab::Metrics).to receive(metric_type)
+
+            expect(metric).to be_instance_of(Gitlab::Metrics::NullMetric)
+
+            expect(Gitlab::Metrics).not_to have_received(metric_type)
+          end
+        end
+      end
+    end
+  end
+
+  include_examples 'metric', :counter, {}
+  include_examples 'metric', :gauge, {}, :all
+  include_examples 'metric', :histogram, {}, [0.005, 0.01, 0.1, 1, 10]
+end
diff --git a/spec/lib/gitlab/metrics/samplers/ruby_sampler_spec.rb b/spec/lib/gitlab/metrics/samplers/ruby_sampler_spec.rb
index 375cbf8a9ca..54781dd52fc 100644
--- a/spec/lib/gitlab/metrics/samplers/ruby_sampler_spec.rb
+++ b/spec/lib/gitlab/metrics/samplers/ruby_sampler_spec.rb
@@ -2,6 +2,11 @@ require 'spec_helper'
 
 describe Gitlab::Metrics::Samplers::RubySampler do
   let(:sampler) { described_class.new(5) }
+  let(:null_metric) { double('null_metric', set: nil, observe: nil) }
+
+  before do
+    allow(Gitlab::Metrics::NullMetric).to receive(:instance).and_return(null_metric)
+  end
 
   after do
     Allocations.stop if Gitlab::Metrics.mri?
@@ -17,12 +22,9 @@ describe Gitlab::Metrics::Samplers::RubySampler do
     end
 
     it 'adds a metric containing the memory usage' do
-      expect(Gitlab::Metrics::System).to receive(:memory_usage)
-                                           .and_return(9000)
+      expect(Gitlab::Metrics::System).to receive(:memory_usage).and_return(9000)
 
-      expect(sampler.metrics[:memory_usage]).to receive(:set)
-                                                  .with({}, 9000)
-                                                  .and_call_original
+      expect(sampler.metrics[:memory_usage]).to receive(:set).with({}, 9000)
 
       sampler.sample
     end
@@ -31,9 +33,7 @@ describe Gitlab::Metrics::Samplers::RubySampler do
       expect(Gitlab::Metrics::System).to receive(:file_descriptor_count)
                                            .and_return(4)
 
-      expect(sampler.metrics[:file_descriptors]).to receive(:set)
-                                                      .with({}, 4)
-                                                      .and_call_original
+      expect(sampler.metrics[:file_descriptors]).to receive(:set).with({}, 4)
 
       sampler.sample
     end
@@ -49,16 +49,14 @@ describe Gitlab::Metrics::Samplers::RubySampler do
     it 'adds a metric containing garbage collection time statistics' do
       expect(GC::Profiler).to receive(:total_time).and_return(0.24)
 
-      expect(sampler.metrics[:total_time]).to receive(:set)
-                                                .with({}, 240)
-                                                .and_call_original
+      expect(sampler.metrics[:total_time]).to receive(:set).with({}, 240)
 
       sampler.sample
     end
 
     it 'adds a metric containing garbage collection statistics' do
       GC.stat.keys.each do |key|
-        expect(sampler.metrics[key]).to receive(:set).with({}, anything).and_call_original
+        expect(sampler.metrics[key]).to receive(:set).with({}, anything)
       end
 
       sampler.sample
diff --git a/spec/lib/gitlab/metrics/subscribers/action_view_spec.rb b/spec/lib/gitlab/metrics/subscribers/action_view_spec.rb
index eca75a4fac1..9f3af1acef7 100644
--- a/spec/lib/gitlab/metrics/subscribers/action_view_spec.rb
+++ b/spec/lib/gitlab/metrics/subscribers/action_view_spec.rb
@@ -32,7 +32,7 @@ describe Gitlab::Metrics::Subscribers::ActionView do
     end
 
     it 'observes view rendering time' do
-      expect(subscriber.send(:metric_view_rendering_duration_seconds))
+      expect(described_class.gitlab_view_rendering_duration_seconds)
         .to receive(:observe)
               .with({ view: 'app/views/x.html.haml' }, 2.1)
 
diff --git a/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb b/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb
index 9b3698fb4a8..4e7bd433a9c 100644
--- a/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb
+++ b/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb
@@ -25,7 +25,7 @@ describe Gitlab::Metrics::Subscribers::ActiveRecord do
         expect(subscriber).to receive(:current_transaction)
                                 .at_least(:once)
                                 .and_return(transaction)
-        expect(subscriber.send(:metric_sql_duration_seconds)).to receive(:observe).with({}, 0.002)
+        expect(described_class.send(:gitlab_sql_duration_seconds)).to receive(:observe).with({}, 0.002)
         subscriber.sql(event)
       end
 
diff --git a/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb b/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb
index 58e28592cf9..6795c1ab56b 100644
--- a/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb
+++ b/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb
@@ -144,7 +144,10 @@ describe Gitlab::Metrics::Subscribers::RailsCache do
     end
 
     context 'with a transaction' do
+      let(:metric_cache_misses_total) { double('metric_cache_misses_total', increment: nil) }
+
       before do
+        allow(subscriber).to receive(:metric_cache_misses_total).and_return(metric_cache_misses_total)
         allow(subscriber).to receive(:current_transaction)
                                .and_return(transaction)
       end
@@ -157,9 +160,9 @@ describe Gitlab::Metrics::Subscribers::RailsCache do
       end
 
       it 'increments the cache_read_miss total' do
-        expect(subscriber.send(:metric_cache_misses_total)).to receive(:increment).with({})
-
         subscriber.cache_generate(event)
+
+        expect(metric_cache_misses_total).to have_received(:increment).with({})
       end
     end
   end
diff --git a/spec/lib/gitlab/metrics_spec.rb b/spec/lib/gitlab/metrics_spec.rb
index 1619fbd88b1..9e405e9f736 100644
--- a/spec/lib/gitlab/metrics_spec.rb
+++ b/spec/lib/gitlab/metrics_spec.rb
@@ -20,7 +20,7 @@ describe Gitlab::Metrics do
 
     context 'prometheus metrics enabled in config' do
       before do
-        allow(described_class).to receive(:current_application_settings).and_return(prometheus_metrics_enabled: true)
+        allow(Gitlab::CurrentSettings).to receive(:current_application_settings).and_return(prometheus_metrics_enabled: true)
       end
 
       context 'when metrics folder is present' do