From dc9a9e377fd815fadc99b68f0853b7693dda97f9 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Fri, 26 Aug 2022 17:11:19 +0200 Subject: [PATCH 1/7] Add instrumentation for cache efficiency metrics **Problem** We don't have a common library to calculate hit ratio and cache generation duration metrics. **Solution** Add a class Gitlab::Cache::Metrics with methods to support hit-ratio and cache generation duration metrics. Changelog: added --- lib/gitlab/cache/metrics.rb | 82 ++++++++++++++ spec/lib/gitlab/cache/metrics_spec.rb | 150 ++++++++++++++++++++++++++ 2 files changed, 232 insertions(+) create mode 100644 lib/gitlab/cache/metrics.rb create mode 100644 spec/lib/gitlab/cache/metrics_spec.rb diff --git a/lib/gitlab/cache/metrics.rb b/lib/gitlab/cache/metrics.rb new file mode 100644 index 00000000000000..ff0b2f3e0d4ad6 --- /dev/null +++ b/lib/gitlab/cache/metrics.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +# Instrumentation for cache efficiency metrics +module Gitlab + module Cache + class Metrics + def initialize( + caller_id:, + cache_identifier:, + feature_category: ::Gitlab::FeatureCategories::FEATURE_CATEGORY_DEFAULT, + backing_resource: :unknown + ) + @caller_id = caller_id + @cache_identifier = cache_identifier + @feature_category = feature_category + @backing_resource = backing_resource + end + + # Increase cache counter with number of cache hits/misses + # + # @param name [Symbol] counter name + # @param total [Integer] total number of calls + # @param miss [Integer] number of misses + # @param caller_id [String] caller ID + def increase_cache_metric(name:, total:, miss:) + return unless caller_id + + hit = total - miss + + counter = Gitlab::Metrics.counter(name, "#{name.to_s.humanize} counter") + + counter.increment(labels.merge(cache_hit: true), hit) if hit > 0 + counter.increment(labels.merge(cache_hit: false), miss) if miss > 0 + end + + # Measure the duration of cacheable action + # + # @param name [Symbol] histogram name + # @param desc [String] histogram description + # @param buckets [Array] histogram buckets + # + # @example + # observe_cache_generation( + # name: :gitlab_duration_seconds, + # desc: 'Duration of action in seconds', + # buckets: [1, 2, 3] + # ) do + # cacheable_action + # end + # + def observe_cache_generation(name:, desc:, buckets:, &block) + histogram = Gitlab::Metrics.histogram( + name, + desc, + labels, + buckets + ) + + real_start = Gitlab::Metrics::System.monotonic_time + + value = yield + + histogram.observe({}, Gitlab::Metrics::System.monotonic_time - real_start) + + value + end + + private + + def labels + @labels ||= { + caller_id: caller_id, + cache_identifier: cache_identifier, + feature_category: feature_category, + backing_resource: backing_resource + } + end + + attr_reader :caller_id, :cache_identifier, :feature_category, :backing_resource + end + end +end diff --git a/spec/lib/gitlab/cache/metrics_spec.rb b/spec/lib/gitlab/cache/metrics_spec.rb new file mode 100644 index 00000000000000..7e7c794c9119ca --- /dev/null +++ b/spec/lib/gitlab/cache/metrics_spec.rb @@ -0,0 +1,150 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Cache::Metrics do + subject(:metrics) do + described_class.new( + caller_id: caller_id, + cache_identifier: cache_identifier, + feature_category: feature_category, + backing_resource: backing_resource + ) + end + + let(:caller_id) { 'caller-id' } + let(:cache_identifier) { 'ApplicationController#show' } + let(:feature_category) { :source_code_management } + let(:backing_resource) { :unknown } + + describe '#increase_cache_metric' do + subject do + metrics.increase_cache_metric(name: name, total: total, miss: miss) + end + + let(:name) { :my_cache } + let(:total) { 5 } + let(:miss) { 2 } + let(:counter_mock) { instance_double(Prometheus::Client::Counter) } + + before do + allow(Gitlab::Metrics).to receive(:counter).with(name, 'My cache counter').and_return(counter_mock) + end + + it 'increments number of hits and misses' do + expect(counter_mock) + .to receive(:increment) + .with( + { + caller_id: caller_id, + cache_identifier: cache_identifier, + feature_category: feature_category, + backing_resource: backing_resource, + cache_hit: true + }, 3 + ).once + + expect(counter_mock) + .to receive(:increment) + .with( + { + caller_id: caller_id, + cache_identifier: cache_identifier, + feature_category: feature_category, + backing_resource: backing_resource, + cache_hit: false + }, 2 + ).once + + subject + end + + context 'when caller_id is missing' do + let(:caller_id) { nil } + + it 'does not increment number of hits and misses' do + expect(Gitlab::Metrics).not_to receive(:counter) + + subject + end + end + + context 'when zero misses' do + let(:miss) { 0 } + + it 'increments only number of hits' do + expect(counter_mock) + .to receive(:increment) + .with( + { + caller_id: caller_id, + cache_identifier: cache_identifier, + feature_category: feature_category, + backing_resource: backing_resource, + cache_hit: true + }, 5 + ).once + + subject + end + end + + context 'when zero hits' do + let(:miss) { total } + + it 'increments only number of misses' do + expect(counter_mock) + .to receive(:increment) + .with( + { + caller_id: caller_id, + cache_identifier: cache_identifier, + feature_category: feature_category, + backing_resource: backing_resource, + cache_hit: false + }, 5 + ).once + + subject + end + end + end + + describe '#observe_cache_generation' do + subject do + metrics.observe_cache_generation( + name: name, + desc: desc, + buckets: buckets + ) { action } + end + + let(:name) { :gitlab_duration_seconds } + let(:desc) { 'Duration in seconds' } + let(:buckets) { [1, 2, 3] } + let(:action) { 'action' } + let(:histogram_mock) { instance_double(Prometheus::Client::Histogram) } + + before do + allow(Gitlab::Metrics::System).to receive(:monotonic_time).and_return(100.0, 500.0) + end + + it 'updates histogram metric' do + expect(Gitlab::Metrics).to receive(:histogram).with( + name, + desc, + { + caller_id: caller_id, + cache_identifier: cache_identifier, + feature_category: feature_category, + backing_resource: backing_resource + }, + buckets + ).and_return(histogram_mock) + + expect(histogram_mock).to receive(:observe).with({}, 400.0) + + is_expected.to eq(action) + end + end +end -- GitLab From 7ee64a4e4277659fa3188a1cf1022c8c5ad261ef Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Wed, 5 Oct 2022 19:00:01 +0200 Subject: [PATCH 2/7] Use metrics for protected branches cache --- .../protected_branches/cache_service.rb | 26 ++++++++++++++++++- .../protected_branches/cache_service_spec.rb | 13 ++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/app/services/protected_branches/cache_service.rb b/app/services/protected_branches/cache_service.rb index 8c521f4ebcba32..e7a82b1fa73e0f 100644 --- a/app/services/protected_branches/cache_service.rb +++ b/app/services/protected_branches/cache_service.rb @@ -13,6 +13,12 @@ def fetch(ref_name, dry_run: false) Gitlab::Redis::Cache.with do |redis| cached_result = redis.hget(redis_key, record) + metrics.increase_cache_metric( + name: :gitlab_protected_branches_cache_operations_total, + total: 1, + miss: cached_result.nil? ? 1 : 0 + ) + decoded_result = Gitlab::Redis::Boolean.decode(cached_result) unless cached_result.nil? # If we're dry-running, don't break because we need to check against @@ -20,7 +26,9 @@ def fetch(ref_name, dry_run: false) # If the result is nil we'll need to run the block, so don't break yet. break decoded_result unless dry_run || decoded_result.nil? - calculated_value = yield + calculated_value = metrics.observe_cache_generation(**histogram_params) do + yield + end check_and_log_discrepancy(decoded_result, calculated_value, ref_name) if dry_run @@ -64,5 +72,21 @@ def check_and_log_discrepancy(cached_value, real_value, ref_name) def redis_key @redis_key ||= [CACHE_ROOT_KEY, @project.id].join(':') end + + def metrics + @metrics ||= Gitlab::Cache::Metrics.new( + caller_id: Gitlab::ApplicationContext.current_context_attribute(:caller_id), + cache_identifier: "#{self.class}#fetch", + feature_category: :source_code_management + ) + end + + def histogram_params + { + name: :gitlab_protected_branches_cacheless_real_duration_seconds, + desc: 'Duration of calculating of protected branch value in real time', + buckets: [0, 1, 2] + } + end end end diff --git a/spec/services/protected_branches/cache_service_spec.rb b/spec/services/protected_branches/cache_service_spec.rb index 00d1e8b5457ba4..34afafb7015c5a 100644 --- a/spec/services/protected_branches/cache_service_spec.rb +++ b/spec/services/protected_branches/cache_service_spec.rb @@ -111,5 +111,18 @@ expect(service.fetch('not-found') { true }).to eq(true) end end + + describe 'metrics' do + it 'records hit ratio metrics' do + expect_next_instance_of(Gitlab::Cache::Metrics) do |metrics| + expect(metrics).to receive(:increase_cache_metric) + .with(name: :gitlab_protected_branches_cache_operations_total, total: 1, miss: 1).once + expect(metrics).to receive(:increase_cache_metric) + .with(name: :gitlab_protected_branches_cache_operations_total, total: 1, miss: 0).exactly(4).times + end + + 5.times { service.fetch('main') { true } } + end + end end # rubocop:enable Style/RedundantFetchBlock -- GitLab From cdb080f261d762127350736ae4e2cbffc787ae24 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Fri, 14 Oct 2022 16:37:27 +0200 Subject: [PATCH 3/7] Apply code review suggestions --- .../protected_branches/cache_service.rb | 24 ++-- lib/gitlab/cache/metrics.rb | 49 +++++--- spec/lib/gitlab/cache/metrics_spec.rb | 113 ++++++++---------- .../protected_branches/cache_service_spec.rb | 6 +- 4 files changed, 94 insertions(+), 98 deletions(-) diff --git a/app/services/protected_branches/cache_service.rb b/app/services/protected_branches/cache_service.rb index e7a82b1fa73e0f..852aa6f6204f86 100644 --- a/app/services/protected_branches/cache_service.rb +++ b/app/services/protected_branches/cache_service.rb @@ -7,28 +7,26 @@ class CacheService < ProtectedBranches::BaseService CACHE_EXPIRE_IN = 1.day CACHE_LIMIT = 1000 - def fetch(ref_name, dry_run: false) + def fetch(ref_name, dry_run: false, &block) record = OpenSSL::Digest::SHA256.hexdigest(ref_name) Gitlab::Redis::Cache.with do |redis| cached_result = redis.hget(redis_key, record) - metrics.increase_cache_metric( - name: :gitlab_protected_branches_cache_operations_total, - total: 1, - miss: cached_result.nil? ? 1 : 0 - ) + if cached_result.nil? + metrics.increment_cache_miss + else + metrics.increment_cache_hit - decoded_result = Gitlab::Redis::Boolean.decode(cached_result) unless cached_result.nil? + decoded_result = Gitlab::Redis::Boolean.decode(cached_result) + end # If we're dry-running, don't break because we need to check against # the real value to ensure the cache is working properly. # If the result is nil we'll need to run the block, so don't break yet. break decoded_result unless dry_run || decoded_result.nil? - calculated_value = metrics.observe_cache_generation(**histogram_params) do - yield - end + calculated_value = metrics.observe_cache_generation(**histogram_params, &block) check_and_log_discrepancy(decoded_result, calculated_value, ref_name) if dry_run @@ -77,15 +75,15 @@ def metrics @metrics ||= Gitlab::Cache::Metrics.new( caller_id: Gitlab::ApplicationContext.current_context_attribute(:caller_id), cache_identifier: "#{self.class}#fetch", - feature_category: :source_code_management + feature_category: :source_code_management, + name: :protected_branches_cache ) end def histogram_params { - name: :gitlab_protected_branches_cacheless_real_duration_seconds, desc: 'Duration of calculating of protected branch value in real time', - buckets: [0, 1, 2] + buckets: [0, 1, 5] } end end diff --git a/lib/gitlab/cache/metrics.rb b/lib/gitlab/cache/metrics.rb index ff0b2f3e0d4ad6..feb481d8cc2644 100644 --- a/lib/gitlab/cache/metrics.rb +++ b/lib/gitlab/cache/metrics.rb @@ -7,35 +7,33 @@ class Metrics def initialize( caller_id:, cache_identifier:, + name:, feature_category: ::Gitlab::FeatureCategories::FEATURE_CATEGORY_DEFAULT, backing_resource: :unknown ) @caller_id = caller_id @cache_identifier = cache_identifier + @name = name @feature_category = feature_category @backing_resource = backing_resource end - # Increase cache counter with number of cache hits/misses + # Increase cache counter with number of cache hits # - # @param name [Symbol] counter name - # @param total [Integer] total number of calls - # @param miss [Integer] number of misses - # @param caller_id [String] caller ID - def increase_cache_metric(name:, total:, miss:) - return unless caller_id - - hit = total - miss - - counter = Gitlab::Metrics.counter(name, "#{name.to_s.humanize} counter") + # @param count [Integer] total number of cache hits + def increment_cache_hit(count = 1) + increase_cache_metric(total: count, miss: 0) + end - counter.increment(labels.merge(cache_hit: true), hit) if hit > 0 - counter.increment(labels.merge(cache_hit: false), miss) if miss > 0 + # Increase cache counter with number of cache misses + # + # @param count [Integer] total number of cache misses + def increment_cache_miss(count = 1) + increase_cache_metric(total: count, miss: count) end # Measure the duration of cacheable action # - # @param name [Symbol] histogram name # @param desc [String] histogram description # @param buckets [Array] histogram buckets # @@ -48,9 +46,9 @@ def increase_cache_metric(name:, total:, miss:) # cacheable_action # end # - def observe_cache_generation(name:, desc:, buckets:, &block) + def observe_cache_generation(desc:, buckets:, &block) histogram = Gitlab::Metrics.histogram( - name, + "#{name}_duration_seconds".to_sym, desc, labels, buckets @@ -67,6 +65,23 @@ def observe_cache_generation(name:, desc:, buckets:, &block) private + # Increase cache counter with number of cache hits/misses + # + # @param name [Symbol] counter name + # @param total [Integer] total number of calls + # @param miss [Integer] number of misses + # @param caller_id [String] caller ID + def increase_cache_metric(total:, miss:) + return unless caller_id + + hit = total - miss + + counter = Gitlab::Metrics.counter("#{name}_operations_total".to_sym, "#{name.to_s.humanize} counter") + + counter.increment(labels.merge(cache_hit: true), hit) if hit > 0 + counter.increment(labels.merge(cache_hit: false), miss) if miss > 0 + end + def labels @labels ||= { caller_id: caller_id, @@ -76,7 +91,7 @@ def labels } end - attr_reader :caller_id, :cache_identifier, :feature_category, :backing_resource + attr_reader :caller_id, :cache_identifier, :name, :feature_category, :backing_resource end end end diff --git a/spec/lib/gitlab/cache/metrics_spec.rb b/spec/lib/gitlab/cache/metrics_spec.rb index 7e7c794c9119ca..9a72216415243c 100644 --- a/spec/lib/gitlab/cache/metrics_spec.rb +++ b/spec/lib/gitlab/cache/metrics_spec.rb @@ -7,6 +7,7 @@ described_class.new( caller_id: caller_id, cache_identifier: cache_identifier, + name: name, feature_category: feature_category, backing_resource: backing_resource ) @@ -14,24 +15,46 @@ let(:caller_id) { 'caller-id' } let(:cache_identifier) { 'ApplicationController#show' } + let(:name) { :my_cache } let(:feature_category) { :source_code_management } let(:backing_resource) { :unknown } - describe '#increase_cache_metric' do - subject do - metrics.increase_cache_metric(name: name, total: total, miss: miss) + let(:counter_mock) { instance_double(Prometheus::Client::Counter) } + + before do + allow(Gitlab::Metrics).to receive(:counter) + .with( + :my_cache_operations_total, + 'My cache counter' + ).and_return(counter_mock) + end + + shared_examples 'hit/miss cache' do + context 'when caller_id is missing' do + let(:caller_id) { nil } + + it 'does not increment number of hits and misses' do + expect(Gitlab::Metrics).not_to receive(:counter) + + subject + end end - let(:name) { :my_cache } - let(:total) { 5 } - let(:miss) { 2 } - let(:counter_mock) { instance_double(Prometheus::Client::Counter) } + context 'when count is 0' do + let(:count) { 0 } - before do - allow(Gitlab::Metrics).to receive(:counter).with(name, 'My cache counter').and_return(counter_mock) + it 'does not increase number of hits and misses' do + expect(counter_mock).not_to receive(:increment) + end end + end + + describe '#increment_cache_hit' do + subject { metrics.increment_cache_hit(count) } + + let(:count) { 5 } - it 'increments number of hits and misses' do + it 'increments number of hits' do expect(counter_mock) .to receive(:increment) .with( @@ -41,9 +64,21 @@ feature_category: feature_category, backing_resource: backing_resource, cache_hit: true - }, 3 + }, count ).once + subject + end + + it_behaves_like 'hit/miss cache' + end + + describe '#increment_cache_miss' do + subject { metrics.increment_cache_miss(count) } + + let(:count) { 5 } + + it 'increments number of misses' do expect(counter_mock) .to receive(:increment) .with( @@ -53,73 +88,23 @@ feature_category: feature_category, backing_resource: backing_resource, cache_hit: false - }, 2 + }, count ).once subject end - context 'when caller_id is missing' do - let(:caller_id) { nil } - - it 'does not increment number of hits and misses' do - expect(Gitlab::Metrics).not_to receive(:counter) - - subject - end - end - - context 'when zero misses' do - let(:miss) { 0 } - - it 'increments only number of hits' do - expect(counter_mock) - .to receive(:increment) - .with( - { - caller_id: caller_id, - cache_identifier: cache_identifier, - feature_category: feature_category, - backing_resource: backing_resource, - cache_hit: true - }, 5 - ).once - - subject - end - end - - context 'when zero hits' do - let(:miss) { total } - - it 'increments only number of misses' do - expect(counter_mock) - .to receive(:increment) - .with( - { - caller_id: caller_id, - cache_identifier: cache_identifier, - feature_category: feature_category, - backing_resource: backing_resource, - cache_hit: false - }, 5 - ).once - - subject - end - end + it_behaves_like 'hit/miss cache' end describe '#observe_cache_generation' do subject do metrics.observe_cache_generation( - name: name, desc: desc, buckets: buckets ) { action } end - let(:name) { :gitlab_duration_seconds } let(:desc) { 'Duration in seconds' } let(:buckets) { [1, 2, 3] } let(:action) { 'action' } @@ -131,7 +116,7 @@ it 'updates histogram metric' do expect(Gitlab::Metrics).to receive(:histogram).with( - name, + :my_cache_duration_seconds, desc, { caller_id: caller_id, diff --git a/spec/services/protected_branches/cache_service_spec.rb b/spec/services/protected_branches/cache_service_spec.rb index 34afafb7015c5a..d7a3258160bc1e 100644 --- a/spec/services/protected_branches/cache_service_spec.rb +++ b/spec/services/protected_branches/cache_service_spec.rb @@ -115,10 +115,8 @@ describe 'metrics' do it 'records hit ratio metrics' do expect_next_instance_of(Gitlab::Cache::Metrics) do |metrics| - expect(metrics).to receive(:increase_cache_metric) - .with(name: :gitlab_protected_branches_cache_operations_total, total: 1, miss: 1).once - expect(metrics).to receive(:increase_cache_metric) - .with(name: :gitlab_protected_branches_cache_operations_total, total: 1, miss: 0).exactly(4).times + expect(metrics).to receive(:increment_cache_miss).once + expect(metrics).to receive(:increment_cache_hit).exactly(4).times end 5.times { service.fetch('main') { true } } -- GitLab From 2f7a9a846585c712941f0eb84715a1cd94e7a491 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Tue, 18 Oct 2022 14:38:56 +0200 Subject: [PATCH 4/7] Apply code review suggestions --- .../protected_branches/cache_service.rb | 12 +----- lib/gitlab/cache/metrics.rb | 38 +++++++++---------- lib/gitlab/feature_categories.rb | 1 + spec/lib/gitlab/cache/metrics_spec.rb | 19 +++------- 4 files changed, 26 insertions(+), 44 deletions(-) diff --git a/app/services/protected_branches/cache_service.rb b/app/services/protected_branches/cache_service.rb index 852aa6f6204f86..09d61f4ecf35bc 100644 --- a/app/services/protected_branches/cache_service.rb +++ b/app/services/protected_branches/cache_service.rb @@ -26,7 +26,7 @@ def fetch(ref_name, dry_run: false, &block) # If the result is nil we'll need to run the block, so don't break yet. break decoded_result unless dry_run || decoded_result.nil? - calculated_value = metrics.observe_cache_generation(**histogram_params, &block) + calculated_value = metrics.observe_cache_generation(&block) check_and_log_discrepancy(decoded_result, calculated_value, ref_name) if dry_run @@ -75,16 +75,8 @@ def metrics @metrics ||= Gitlab::Cache::Metrics.new( caller_id: Gitlab::ApplicationContext.current_context_attribute(:caller_id), cache_identifier: "#{self.class}#fetch", - feature_category: :source_code_management, - name: :protected_branches_cache + feature_category: Gitlab::FeatureCategories::FEATURE_CATEGORY_SOURCE_CODE ) end - - def histogram_params - { - desc: 'Duration of calculating of protected branch value in real time', - buckets: [0, 1, 5] - } - end end end diff --git a/lib/gitlab/cache/metrics.rb b/lib/gitlab/cache/metrics.rb index feb481d8cc2644..8f7d5d733632b6 100644 --- a/lib/gitlab/cache/metrics.rb +++ b/lib/gitlab/cache/metrics.rb @@ -4,16 +4,16 @@ module Gitlab module Cache class Metrics + DEFAULT_BUCKETS = [0, 1, 5].freeze + def initialize( caller_id:, cache_identifier:, - name:, feature_category: ::Gitlab::FeatureCategories::FEATURE_CATEGORY_DEFAULT, backing_resource: :unknown ) @caller_id = caller_id @cache_identifier = cache_identifier - @name = name @feature_category = feature_category @backing_resource = backing_resource end @@ -34,26 +34,12 @@ def increment_cache_miss(count = 1) # Measure the duration of cacheable action # - # @param desc [String] histogram description - # @param buckets [Array] histogram buckets - # # @example - # observe_cache_generation( - # name: :gitlab_duration_seconds, - # desc: 'Duration of action in seconds', - # buckets: [1, 2, 3] - # ) do + # observe_cache_generation do # cacheable_action # end # - def observe_cache_generation(desc:, buckets:, &block) - histogram = Gitlab::Metrics.histogram( - "#{name}_duration_seconds".to_sym, - desc, - labels, - buckets - ) - + def observe_cache_generation(&block) real_start = Gitlab::Metrics::System.monotonic_time value = yield @@ -76,12 +62,22 @@ def increase_cache_metric(total:, miss:) hit = total - miss - counter = Gitlab::Metrics.counter("#{name}_operations_total".to_sym, "#{name.to_s.humanize} counter") + counter = Gitlab::Metrics.counter(:redis_hit_miss_operations, "Hit/miss Redis cache counter") + + return counter.increment(labels.merge(cache_hit: true), hit) if hit > 0 - counter.increment(labels.merge(cache_hit: true), hit) if hit > 0 counter.increment(labels.merge(cache_hit: false), miss) if miss > 0 end + def histogram + @histogram ||= Gitlab::Metrics.histogram( + :redis_cache_generation_duration_seconds, + 'Duration of Redis cache generation', + labels, + DEFAULT_BUCKETS + ) + end + def labels @labels ||= { caller_id: caller_id, @@ -91,7 +87,7 @@ def labels } end - attr_reader :caller_id, :cache_identifier, :name, :feature_category, :backing_resource + attr_reader :caller_id, :cache_identifier, :feature_category, :backing_resource end end end diff --git a/lib/gitlab/feature_categories.rb b/lib/gitlab/feature_categories.rb index d06f3b14fed59c..40ff7457cbbbd9 100644 --- a/lib/gitlab/feature_categories.rb +++ b/lib/gitlab/feature_categories.rb @@ -3,6 +3,7 @@ module Gitlab class FeatureCategories FEATURE_CATEGORY_DEFAULT = 'unknown' + FEATURE_CATEGORY_SOURCE_CODE = :source_code_management attr_reader :categories diff --git a/spec/lib/gitlab/cache/metrics_spec.rb b/spec/lib/gitlab/cache/metrics_spec.rb index 9a72216415243c..f338473273ea27 100644 --- a/spec/lib/gitlab/cache/metrics_spec.rb +++ b/spec/lib/gitlab/cache/metrics_spec.rb @@ -7,7 +7,6 @@ described_class.new( caller_id: caller_id, cache_identifier: cache_identifier, - name: name, feature_category: feature_category, backing_resource: backing_resource ) @@ -15,7 +14,6 @@ let(:caller_id) { 'caller-id' } let(:cache_identifier) { 'ApplicationController#show' } - let(:name) { :my_cache } let(:feature_category) { :source_code_management } let(:backing_resource) { :unknown } @@ -24,8 +22,8 @@ before do allow(Gitlab::Metrics).to receive(:counter) .with( - :my_cache_operations_total, - 'My cache counter' + :redis_hit_miss_operations, + 'Hit/miss Redis cache counter' ).and_return(counter_mock) end @@ -99,14 +97,9 @@ describe '#observe_cache_generation' do subject do - metrics.observe_cache_generation( - desc: desc, - buckets: buckets - ) { action } + metrics.observe_cache_generation { action } end - let(:desc) { 'Duration in seconds' } - let(:buckets) { [1, 2, 3] } let(:action) { 'action' } let(:histogram_mock) { instance_double(Prometheus::Client::Histogram) } @@ -116,15 +109,15 @@ it 'updates histogram metric' do expect(Gitlab::Metrics).to receive(:histogram).with( - :my_cache_duration_seconds, - desc, + :redis_cache_generation_duration_seconds, + 'Duration of Redis cache generation', { caller_id: caller_id, cache_identifier: cache_identifier, feature_category: feature_category, backing_resource: backing_resource }, - buckets + [0, 1, 5] ).and_return(histogram_mock) expect(histogram_mock).to receive(:observe).with({}, 400.0) -- GitLab From e5215efa5ff31488c29184a712173b792223058c Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Wed, 19 Oct 2022 21:01:57 +0200 Subject: [PATCH 5/7] Apply code review suggestions --- .../protected_branches/cache_service.rb | 3 +- lib/gitlab/cache/metrics.rb | 30 +++++++++++-------- lib/gitlab/feature_categories.rb | 9 +++++- spec/lib/gitlab/cache/metrics_spec.rb | 18 +++++++++++ spec/lib/gitlab/feature_categories_spec.rb | 24 ++++++++++++++- 5 files changed, 69 insertions(+), 15 deletions(-) diff --git a/app/services/protected_branches/cache_service.rb b/app/services/protected_branches/cache_service.rb index 09d61f4ecf35bc..a664ff0542f967 100644 --- a/app/services/protected_branches/cache_service.rb +++ b/app/services/protected_branches/cache_service.rb @@ -75,7 +75,8 @@ def metrics @metrics ||= Gitlab::Cache::Metrics.new( caller_id: Gitlab::ApplicationContext.current_context_attribute(:caller_id), cache_identifier: "#{self.class}#fetch", - feature_category: Gitlab::FeatureCategories::FEATURE_CATEGORY_SOURCE_CODE + feature_category: :source_code_management, + backing_resource: :cpu ) end end diff --git a/lib/gitlab/cache/metrics.rb b/lib/gitlab/cache/metrics.rb index 8f7d5d733632b6..193791b99bee2d 100644 --- a/lib/gitlab/cache/metrics.rb +++ b/lib/gitlab/cache/metrics.rb @@ -5,17 +5,19 @@ module Gitlab module Cache class Metrics DEFAULT_BUCKETS = [0, 1, 5].freeze + VALID_BACKING_RESOURCES = [:cpu, :database, :gitaly, :memory, :unknown].freeze + DEFAULT_BACKING_RESOURCE = :unknown def initialize( caller_id:, cache_identifier:, feature_category: ::Gitlab::FeatureCategories::FEATURE_CATEGORY_DEFAULT, - backing_resource: :unknown + backing_resource: DEFAULT_BACKING_RESOURCE ) @caller_id = caller_id @cache_identifier = cache_identifier - @feature_category = feature_category - @backing_resource = backing_resource + @feature_category = Gitlab::FeatureCategories.default.get!(feature_category) + @backing_resource = fetch_backing_resource!(backing_resource) end # Increase cache counter with number of cache hits @@ -51,24 +53,22 @@ def observe_cache_generation(&block) private - # Increase cache counter with number of cache hits/misses - # - # @param name [Symbol] counter name - # @param total [Integer] total number of calls - # @param miss [Integer] number of misses - # @param caller_id [String] caller ID + attr_reader :caller_id, :cache_identifier, :feature_category, :backing_resource + def increase_cache_metric(total:, miss:) return unless caller_id hit = total - miss - counter = Gitlab::Metrics.counter(:redis_hit_miss_operations, "Hit/miss Redis cache counter") - return counter.increment(labels.merge(cache_hit: true), hit) if hit > 0 counter.increment(labels.merge(cache_hit: false), miss) if miss > 0 end + def counter + @counter ||= Gitlab::Metrics.counter(:redis_hit_miss_operations, "Hit/miss Redis cache counter") + end + def histogram @histogram ||= Gitlab::Metrics.histogram( :redis_cache_generation_duration_seconds, @@ -87,7 +87,13 @@ def labels } end - attr_reader :caller_id, :cache_identifier, :feature_category, :backing_resource + def fetch_backing_resource!(resource) + return resource if VALID_BACKING_RESOURCES.include?(resource) + + raise "Unknown backing resource: #{resource}" if Gitlab.dev_or_test_env? + + DEFAULT_BACKING_RESOURCE + end end end end diff --git a/lib/gitlab/feature_categories.rb b/lib/gitlab/feature_categories.rb index 40ff7457cbbbd9..17586a94d7eb8f 100644 --- a/lib/gitlab/feature_categories.rb +++ b/lib/gitlab/feature_categories.rb @@ -3,7 +3,6 @@ module Gitlab class FeatureCategories FEATURE_CATEGORY_DEFAULT = 'unknown' - FEATURE_CATEGORY_SOURCE_CODE = :source_code_management attr_reader :categories @@ -32,6 +31,14 @@ def from_request(request) category end + def get!(feature_category) + return feature_category if valid?(feature_category) + + raise "Unknown feature category: #{feature_category}" if Gitlab.dev_or_test_env? + + FEATURE_CATEGORY_DEFAULT + end + def valid?(category) categories.include?(category.to_s) end diff --git a/spec/lib/gitlab/cache/metrics_spec.rb b/spec/lib/gitlab/cache/metrics_spec.rb index f338473273ea27..40a0ad24dfc6a9 100644 --- a/spec/lib/gitlab/cache/metrics_spec.rb +++ b/spec/lib/gitlab/cache/metrics_spec.rb @@ -27,6 +27,24 @@ ).and_return(counter_mock) end + describe '#initialize' do + context 'when backing resource is not supported' do + let(:backing_resource) { 'foo' } + + it { expect { metrics }.to raise_error(RuntimeError) } + + context 'when on production' do + before do + allow(Gitlab).to receive(:dev_or_test_env?).and_return(false) + end + + it 'does not raise an exception' do + expect { metrics }.not_to raise_error + end + end + end + end + shared_examples 'hit/miss cache' do context 'when caller_id is missing' do let(:caller_id) { nil } diff --git a/spec/lib/gitlab/feature_categories_spec.rb b/spec/lib/gitlab/feature_categories_spec.rb index 477da900d0a330..a35166a4499deb 100644 --- a/spec/lib/gitlab/feature_categories_spec.rb +++ b/spec/lib/gitlab/feature_categories_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Gitlab::FeatureCategories do let(:fake_categories) { %w(foo bar) } - subject { described_class.new(fake_categories) } + subject(:feature_categories) { described_class.new(fake_categories) } describe "#valid?" do it "returns true if category is known", :aggregate_failures do @@ -14,6 +14,28 @@ end end + describe '#get!' do + subject { feature_categories.get!(category) } + + let(:category) { 'foo' } + + it { is_expected.to eq('foo') } + + context 'when category does not exist' do + let(:category) { 'zzz' } + + it { expect { subject }.to raise_error(RuntimeError) } + + context 'when on production' do + before do + allow(Gitlab).to receive(:dev_or_test_env?).and_return(false) + end + + it { is_expected.to eq('unknown') } + end + end + end + describe "#from_request" do let(:request_env) { {} } let(:verified) { true } -- GitLab From 7dc9d6ac11019ffae22c49e4e02672059a0a2d8c Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Fri, 21 Oct 2022 16:59:14 +0200 Subject: [PATCH 6/7] Apply code review suggestions --- lib/gitlab/cache/metrics.rb | 16 +++------------- spec/lib/gitlab/cache/metrics_spec.rb | 26 +------------------------- 2 files changed, 4 insertions(+), 38 deletions(-) diff --git a/lib/gitlab/cache/metrics.rb b/lib/gitlab/cache/metrics.rb index 193791b99bee2d..4e0208ee850d5b 100644 --- a/lib/gitlab/cache/metrics.rb +++ b/lib/gitlab/cache/metrics.rb @@ -24,14 +24,14 @@ def initialize( # # @param count [Integer] total number of cache hits def increment_cache_hit(count = 1) - increase_cache_metric(total: count, miss: 0) + counter.increment(labels.merge(cache_hit: true), count) end # Increase cache counter with number of cache misses # # @param count [Integer] total number of cache misses def increment_cache_miss(count = 1) - increase_cache_metric(total: count, miss: count) + counter.increment(labels.merge(cache_hit: false), count) end # Measure the duration of cacheable action @@ -55,18 +55,8 @@ def observe_cache_generation(&block) attr_reader :caller_id, :cache_identifier, :feature_category, :backing_resource - def increase_cache_metric(total:, miss:) - return unless caller_id - - hit = total - miss - - return counter.increment(labels.merge(cache_hit: true), hit) if hit > 0 - - counter.increment(labels.merge(cache_hit: false), miss) if miss > 0 - end - def counter - @counter ||= Gitlab::Metrics.counter(:redis_hit_miss_operations, "Hit/miss Redis cache counter") + @counter ||= Gitlab::Metrics.counter(:redis_hit_miss_operations_total, "Hit/miss Redis cache counter") end def histogram diff --git a/spec/lib/gitlab/cache/metrics_spec.rb b/spec/lib/gitlab/cache/metrics_spec.rb index 40a0ad24dfc6a9..cdb9d32a9d7c1f 100644 --- a/spec/lib/gitlab/cache/metrics_spec.rb +++ b/spec/lib/gitlab/cache/metrics_spec.rb @@ -22,7 +22,7 @@ before do allow(Gitlab::Metrics).to receive(:counter) .with( - :redis_hit_miss_operations, + :redis_hit_miss_operations_total, 'Hit/miss Redis cache counter' ).and_return(counter_mock) end @@ -45,26 +45,6 @@ end end - shared_examples 'hit/miss cache' do - context 'when caller_id is missing' do - let(:caller_id) { nil } - - it 'does not increment number of hits and misses' do - expect(Gitlab::Metrics).not_to receive(:counter) - - subject - end - end - - context 'when count is 0' do - let(:count) { 0 } - - it 'does not increase number of hits and misses' do - expect(counter_mock).not_to receive(:increment) - end - end - end - describe '#increment_cache_hit' do subject { metrics.increment_cache_hit(count) } @@ -85,8 +65,6 @@ subject end - - it_behaves_like 'hit/miss cache' end describe '#increment_cache_miss' do @@ -109,8 +87,6 @@ subject end - - it_behaves_like 'hit/miss cache' end describe '#observe_cache_generation' do -- GitLab From 145cc34e006172b73d34b0430f694134ac75c8f2 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Mon, 24 Oct 2022 10:26:45 +0200 Subject: [PATCH 7/7] Code review updates --- .../monitoring/prometheus/gitlab_metrics.md | 2 ++ lib/gitlab/cache/metrics.rb | 14 ++++++-------- spec/lib/gitlab/cache/metrics_spec.rb | 12 ++++-------- 3 files changed, 12 insertions(+), 16 deletions(-) diff --git a/doc/administration/monitoring/prometheus/gitlab_metrics.md b/doc/administration/monitoring/prometheus/gitlab_metrics.md index ba33bcc47f5f9f..bbbfa90e0d403f 100644 --- a/doc/administration/monitoring/prometheus/gitlab_metrics.md +++ b/doc/administration/monitoring/prometheus/gitlab_metrics.md @@ -148,6 +148,8 @@ The following metrics are available: | `gitlab_ci_build_trace_errors_total` | Counter | 14.4 | Total amount of different error types on a build trace | `error_reason` | | `gitlab_presentable_object_cacheless_render_real_duration_seconds` | Histogram | 15.3 | Duration of real time spent caching and representing specific web request objects | `controller`, `action` | | `cached_object_operations_total` | Counter | 15.3 | Total number of objects cached for specific web requests | `controller`, `action` | +| `redis_hit_miss_operations_total` | Counter | 15.6 | Total number of Redis cache hits and misses | `cache_hit`, `caller_id`, `cache_identifier`, `feature_category`, `backing_resource` | +| `redis_cache_generation_duration_seconds` | Histogram | 15.6 | Time to generate Redis cache | `cache_hit`, `caller_id`, `cache_identifier`, `feature_category`, `backing_resource` | ## Metrics controlled by a feature flag diff --git a/lib/gitlab/cache/metrics.rb b/lib/gitlab/cache/metrics.rb index 4e0208ee850d5b..0143052beb1a63 100644 --- a/lib/gitlab/cache/metrics.rb +++ b/lib/gitlab/cache/metrics.rb @@ -20,18 +20,16 @@ def initialize( @backing_resource = fetch_backing_resource!(backing_resource) end - # Increase cache counter with number of cache hits + # Increase cache hit counter # - # @param count [Integer] total number of cache hits - def increment_cache_hit(count = 1) - counter.increment(labels.merge(cache_hit: true), count) + def increment_cache_hit + counter.increment(labels.merge(cache_hit: true)) end - # Increase cache counter with number of cache misses + # Increase cache miss counter # - # @param count [Integer] total number of cache misses - def increment_cache_miss(count = 1) - counter.increment(labels.merge(cache_hit: false), count) + def increment_cache_miss + counter.increment(labels.merge(cache_hit: false)) end # Measure the duration of cacheable action diff --git a/spec/lib/gitlab/cache/metrics_spec.rb b/spec/lib/gitlab/cache/metrics_spec.rb index cdb9d32a9d7c1f..d8103837708ab8 100644 --- a/spec/lib/gitlab/cache/metrics_spec.rb +++ b/spec/lib/gitlab/cache/metrics_spec.rb @@ -46,9 +46,7 @@ end describe '#increment_cache_hit' do - subject { metrics.increment_cache_hit(count) } - - let(:count) { 5 } + subject { metrics.increment_cache_hit } it 'increments number of hits' do expect(counter_mock) @@ -60,7 +58,7 @@ feature_category: feature_category, backing_resource: backing_resource, cache_hit: true - }, count + } ).once subject @@ -68,9 +66,7 @@ end describe '#increment_cache_miss' do - subject { metrics.increment_cache_miss(count) } - - let(:count) { 5 } + subject { metrics.increment_cache_miss } it 'increments number of misses' do expect(counter_mock) @@ -82,7 +78,7 @@ feature_category: feature_category, backing_resource: backing_resource, cache_hit: false - }, count + } ).once subject -- GitLab