diff options
author | Grzegorz Bizon <grzegorz@gitlab.com> | 2018-01-30 23:58:28 +0300 |
---|---|---|
committer | Grzegorz Bizon <grzegorz@gitlab.com> | 2018-01-30 23:58:28 +0300 |
commit | a74413980bd9fa2a9b5dd8dbee8307f14063fbcb (patch) | |
tree | 36cdea9e28641a1c6a29f94dc73b277cced17d30 /spec | |
parent | 6f32fa6628785094e7bbf94f22272abe4991d14a (diff) | |
parent | d4c768ce09daac5f6bcfebae2313dc943961b356 (diff) |
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
Diffstat (limited to 'spec')
-rw-r--r-- | spec/lib/gitlab/metrics/method_call_spec.rb | 44 | ||||
-rw-r--r-- | spec/lib/gitlab/metrics/methods_spec.rb | 137 | ||||
-rw-r--r-- | spec/lib/gitlab/metrics/samplers/ruby_sampler_spec.rb | 22 | ||||
-rw-r--r-- | spec/lib/gitlab/metrics/subscribers/action_view_spec.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/metrics/subscribers/active_record_spec.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb | 7 | ||||
-rw-r--r-- | spec/lib/gitlab/metrics_spec.rb | 2 |
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 |