Merge pull request #21631 from RobinClowers/fix-cache-instrumentation

Fix cache fetch instrumentation

3 files changed, 43 insertions, 13 deletions

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 19588d622c..b05a068dc1 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,3 +1,7 @@
+*   `Cache#fetch` instrumentation marks whether it was a `:hit`.
+
+    *Robin Clowers*
+
 *   `assert_difference` and `assert_no_difference` now returns the result of the
     yielded block.
 
diff --git a/activesupport/lib/active_support/cache.rb b/activesupport/lib/active_support/cache.rb
index 8253a76383..3996f583c2 100644
--- a/activesupport/lib/active_support/cache.rb
+++ b/activesupport/lib/active_support/cache.rb
@@ -277,13 +277,18 @@ module ActiveSupport
           options = merged_options(options)
           key = namespaced_key(name, options)
 
-          cached_entry = find_cached_entry(key, name, options) unless options[:force]
-          entry = handle_expired_entry(cached_entry, key, options)
+          instrument(:read, name, options) do |payload|
+            cached_entry = read_entry(key, options) unless options[:force]
+            payload[:super_operation] = :fetch if payload
+            entry = handle_expired_entry(cached_entry, key, options)
 
-          if entry
-            get_entry_value(entry, name, options)
-          else
-            save_block_result_to_cache(name, options) { |_name| yield _name }
+            if entry
+              payload[:hit] = true if payload
+              get_entry_value(entry, name, options)
+            else
+              payload[:hit] = false if payload
+              save_block_result_to_cache(name, options) { |_name| yield _name }
+            end
           end
         else
           read(name, options)
@@ -556,13 +561,6 @@ module ActiveSupport
           logger.debug(yield)
         end
 
-        def find_cached_entry(key, name, options)
-          instrument(:read, name, options) do |payload|
-            payload[:super_operation] = :fetch if payload
-            read_entry(key, options)
-          end
-        end
-
         def handle_expired_entry(entry, key, options)
           if entry && entry.expired?
             race_ttl = options[:race_condition_ttl].to_i
diff --git a/activesupport/test/caching_test.rb b/activesupport/test/caching_test.rb
index 74ceff44f9..c235dee5e1 100644
--- a/activesupport/test/caching_test.rb
+++ b/activesupport/test/caching_test.rb
@@ -490,6 +490,34 @@ module CacheStoreBehavior
     assert_equal({key => "bar"}, @cache.read_multi(key))
     assert @cache.delete(key)
   end
+
+  def test_cache_hit_instrumentation
+    key = "test_key"
+    subscribe_executed = false
+    ActiveSupport::Notifications.subscribe "cache_read.active_support" do |name, start, finish, id, payload|
+      subscribe_executed = true
+      assert_equal :fetch, payload[:super_operation]
+      assert payload[:hit]
+    end
+    assert @cache.write(key, "1", :raw => true)
+    assert @cache.fetch(key) {}
+    assert subscribe_executed
+  ensure
+    ActiveSupport::Notifications.unsubscribe "cache_read.active_support"
+  end
+
+  def test_cache_miss_instrumentation
+    subscribe_executed = false
+    ActiveSupport::Notifications.subscribe "cache_read.active_support" do |name, start, finish, id, payload|
+      subscribe_executed = true
+      assert_equal :fetch, payload[:super_operation]
+      assert_not payload[:hit]
+    end
+    assert_not @cache.fetch("bad_key") {}
+    assert subscribe_executed
+  ensure
+    ActiveSupport::Notifications.unsubscribe "cache_read.active_support"
+  end
 end
 
 # https://rails.lighthouseapp.com/projects/8994/tickets/6225-memcachestore-cant-deal-with-umlauts-and-special-characters