RailsCache metrics now includes fetch_hit/fetch_miss and read_hit/read_miss info.instrument-cache-fetch-hit-and-misses

3 files changed, 116 insertions, 2 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 19bd1f0ba31..354fa8832a8 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -23,6 +23,7 @@ v 8.10.0 (unreleased)
   - Remove unused front-end variable -> default_issues_tracker
   - Add API endpoint for a group issues !4520 (mahcsig)
   - Add Bugzilla integration !4930 (iamtjg)
+  - RailsCache metris now includes fetch_hit/fetch_miss and read_hit/read_miss info.
   - Allow [ci skip] to be in any case and allow [skip ci]. !4785 (simon_w)
   - Add basic system information like memory and disk usage to the admin panel
 
diff --git a/lib/gitlab/metrics/subscribers/rails_cache.rb b/lib/gitlab/metrics/subscribers/rails_cache.rb
index 277c860fbef..aaed2184f44 100644
--- a/lib/gitlab/metrics/subscribers/rails_cache.rb
+++ b/lib/gitlab/metrics/subscribers/rails_cache.rb
@@ -2,11 +2,21 @@ module Gitlab
   module Metrics
     module Subscribers
       # Class for tracking the total time spent in Rails cache calls
+      # http://guides.rubyonrails.org/active_support_instrumentation.html
       class RailsCache < ActiveSupport::Subscriber
         attach_to :active_support
 
         def cache_read(event)
           increment(:cache_read, event.duration)
+
+          return unless current_transaction
+          return if event.payload[:super_operation] == :fetch
+
+          if event.payload[:hit]
+            current_transaction.increment(:cache_read_hit_count, 1)
+          else
+            current_transaction.increment(:cache_read_miss_count, 1)
+          end
         end
 
         def cache_write(event)
@@ -24,13 +34,13 @@ module Gitlab
         def cache_fetch_hit(event)
           return unless current_transaction
 
-          current_transaction.increment(:cache_fetch_hit, 1)
+          current_transaction.increment(:cache_read_hit_count, 1)
         end
 
         def cache_generate(event)
           return unless current_transaction
 
-          current_transaction.increment(:cache_fetch_miss, 1)
+          current_transaction.increment(:cache_read_miss_count, 1)
         end
 
         def increment(key, duration)
diff --git a/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb b/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb
index d824dc54438..d986c6fac43 100644
--- a/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb
+++ b/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb
@@ -13,6 +13,61 @@ describe Gitlab::Metrics::Subscribers::RailsCache do
 
       subscriber.cache_read(event)
     end
+
+    context 'with a transaction' do
+      before do
+        allow(subscriber).to receive(:current_transaction).
+          and_return(transaction)
+      end
+
+      context 'with hit event' do
+        let(:event) { double(:event, duration: 15.2, payload: { hit: true }) }
+
+        it 'increments the cache_read_hit count' do
+          expect(transaction).to receive(:increment).
+            with(:cache_read_hit_count, 1)
+          expect(transaction).to receive(:increment).
+            with(any_args).at_least(1) # Other calls
+
+          subscriber.cache_read(event)
+        end
+
+        context 'when super operation is fetch' do
+          let(:event) { double(:event, duration: 15.2, payload: { hit: true, super_operation: :fetch }) }
+
+          it 'does not increment cache read miss' do
+            expect(transaction).not_to receive(:increment).
+              with(:cache_read_hit_count, 1)
+
+            subscriber.cache_read(event)
+          end
+        end
+      end
+
+      context 'with miss event' do
+        let(:event) { double(:event, duration: 15.2, payload: { hit: false }) }
+
+        it 'increments the cache_read_miss count' do
+          expect(transaction).to receive(:increment).
+            with(:cache_read_miss_count, 1)
+          expect(transaction).to receive(:increment).
+            with(any_args).at_least(1) # Other calls
+
+          subscriber.cache_read(event)
+        end
+
+        context 'when super operation is fetch' do
+          let(:event) { double(:event, duration: 15.2, payload: { hit: false, super_operation: :fetch }) }
+
+          it 'does not increment cache read miss' do
+            expect(transaction).not_to receive(:increment).
+              with(:cache_read_miss_count, 1)
+
+            subscriber.cache_read(event)
+          end
+        end
+      end
+    end
   end
 
   describe '#cache_write' do
@@ -42,6 +97,54 @@ describe Gitlab::Metrics::Subscribers::RailsCache do
     end
   end
 
+  describe '#cache_fetch_hit' do
+    context 'without a transaction' do
+      it 'returns' do
+        expect(transaction).not_to receive(:increment)
+
+        subscriber.cache_fetch_hit(event)
+      end
+    end
+
+    context 'with a transaction' do
+      before do
+        allow(subscriber).to receive(:current_transaction).
+          and_return(transaction)
+      end
+
+      it 'increments the cache_read_hit count' do
+        expect(transaction).to receive(:increment).
+          with(:cache_read_hit_count, 1)
+
+        subscriber.cache_fetch_hit(event)
+      end
+    end
+  end
+
+  describe '#cache_generate' do
+    context 'without a transaction' do
+      it 'returns' do
+        expect(transaction).not_to receive(:increment)
+
+        subscriber.cache_generate(event)
+      end
+    end
+
+    context 'with a transaction' do
+      before do
+        allow(subscriber).to receive(:current_transaction).
+          and_return(transaction)
+      end
+
+      it 'increments the cache_fetch_miss count' do
+        expect(transaction).to receive(:increment).
+          with(:cache_read_miss_count, 1)
+
+        subscriber.cache_generate(event)
+      end
+    end
+  end
+
   describe '#increment' do
     context 'without a transaction' do
       it 'returns' do