Merge pull request #30268 from ignatiusreza/instrumentation

add instrumentation for read_multi

4 files changed, 64 insertions, 49 deletions

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 3f77b191f9..8378de5061 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,3 +1,7 @@
+*   Add missing instrumentation for `read_multi` in `ActiveSupport::Cache::Store`.
+
+    *Ignatius Reza Lesmana*
+
 *   `assert_changes` will always assert that the expression changes,
     regardless of `from:` and `to:` argument combinations.
 
diff --git a/activesupport/lib/active_support/cache.rb b/activesupport/lib/active_support/cache.rb
index 8301b8c7cb..2d038dba77 100644
--- a/activesupport/lib/active_support/cache.rb
+++ b/activesupport/lib/active_support/cache.rb
@@ -357,23 +357,11 @@ module ActiveSupport
         options = names.extract_options!
         options = merged_options(options)
 
-        results = {}
-        names.each do |name|
-          key     = normalize_key(name, options)
-          version = normalize_version(name, options)
-          entry   = read_entry(key, options)
-
-          if entry
-            if entry.expired?
-              delete_entry(key, options)
-            elsif entry.mismatched?(version)
-              # Skip mismatched versions
-            else
-              results[name] = entry.value
-            end
+        instrument :read_multi, names, options do |payload|
+          read_multi_entries(names, options).tap do |results|
+            payload[:hits] = results.keys
           end
         end
-        results
       end
 
       # Cache Storage API to write multiple values at once.
@@ -414,14 +402,19 @@ module ActiveSupport
         options = names.extract_options!
         options = merged_options(options)
 
-        read_multi(*names, options).tap do |results|
-          writes = {}
+        instrument :read_multi, names, options do |payload|
+          read_multi_entries(names, options).tap do |results|
+            payload[:hits] = results.keys
+            payload[:super_operation] = :fetch_multi
 
-          (names - results.keys).each do |name|
-            results[name] = writes[name] = yield(name)
-          end
+            writes = {}
 
-          write_multi writes, options
+            (names - results.keys).each do |name|
+              results[name] = writes[name] = yield(name)
+            end
+
+            write_multi writes, options
+          end
         end
       end
 
@@ -538,6 +531,28 @@ module ActiveSupport
           raise NotImplementedError.new
         end
 
+        # Reads multiple entries from the cache implementation. Subclasses MAY
+        # implement this method.
+        def read_multi_entries(names, options)
+          results = {}
+          names.each do |name|
+            key     = normalize_key(name, options)
+            version = normalize_version(name, options)
+            entry   = read_entry(key, options)
+
+            if entry
+              if entry.expired?
+                delete_entry(key, options)
+              elsif entry.mismatched?(version)
+                # Skip mismatched versions
+              else
+                results[name] = entry.value
+              end
+            end
+          end
+          results
+        end
+
         # Writes multiple entries to the cache implementation. Subclasses MAY
         # implement this method.
         def write_multi_entries(hash, options)
diff --git a/activesupport/lib/active_support/cache/mem_cache_store.rb b/activesupport/lib/active_support/cache/mem_cache_store.rb
index df8bc8e43e..00a3670d64 100644
--- a/activesupport/lib/active_support/cache/mem_cache_store.rb
+++ b/activesupport/lib/active_support/cache/mem_cache_store.rb
@@ -91,28 +91,6 @@ module ActiveSupport
         end
       end
 
-      # Reads multiple values from the cache using a single call to the
-      # servers for all keys. Options can be passed in the last argument.
-      def read_multi(*names)
-        options = names.extract_options!
-        options = merged_options(options)
-
-        keys_to_names = Hash[names.map { |name| [normalize_key(name, options), name] }]
-
-        raw_values = @data.get_multi(keys_to_names.keys)
-        values = {}
-
-        raw_values.each do |key, value|
-          entry = deserialize_entry(value)
-
-          unless entry.expired? || entry.mismatched?(normalize_version(keys_to_names[key], options))
-            values[keys_to_names[key]] = entry.value
-          end
-        end
-
-        values
-      end
-
       # Increment a cached value. This method uses the memcached incr atomic
       # operator and can only be used on values written with the :raw option.
       # Calling it on a value not stored with :raw will initialize that value
@@ -170,6 +148,24 @@ module ActiveSupport
           end
         end
 
+        # Reads multiple entries from the cache implementation.
+        def read_multi_entries(names, options)
+          keys_to_names = Hash[names.map { |name| [normalize_key(name, options), name] }]
+
+          raw_values = @data.get_multi(keys_to_names.keys)
+          values = {}
+
+          raw_values.each do |key, value|
+            entry = deserialize_entry(value)
+
+            unless entry.expired? || entry.mismatched?(normalize_version(keys_to_names[key], options))
+              values[keys_to_names[key]] = entry.value
+            end
+          end
+
+          values
+        end
+
         # Delete an entry from the cache.
         def delete_entry(key, options)
           rescue_error_with(false) { @data.delete(key) }
diff --git a/activesupport/test/cache/cache_store_write_multi_test.rb b/activesupport/test/cache/cache_store_write_multi_test.rb
index 5b6fd678c5..7d606e3f7b 100644
--- a/activesupport/test/cache/cache_store_write_multi_test.rb
+++ b/activesupport/test/cache/cache_store_write_multi_test.rb
@@ -19,7 +19,7 @@ end
 
 class CacheStoreWriteMultiInstrumentationTest < ActiveSupport::TestCase
   setup do
-    @cache = ActiveSupport::Cache.lookup_store(:null_store)
+    @cache = ActiveSupport::Cache.lookup_store(:memory_store)
   end
 
   test "instrumentation" do
@@ -35,15 +35,15 @@ class CacheStoreWriteMultiInstrumentationTest < ActiveSupport::TestCase
   end
 
   test "instrumentation with fetch_multi as super operation" do
-    skip "fetch_multi isn't instrumented yet"
+    @cache.write("b", "bb")
 
-    events = with_instrumentation "write_multi" do
+    events = with_instrumentation "read_multi" do
       @cache.fetch_multi("a", "b") { |key| key * 2 }
     end
 
-    assert_equal %w[ cache_write_multi.active_support ], events.map(&:name)
-    assert_nil events[0].payload[:super_operation]
-    assert !events[0].payload[:hit]
+    assert_equal %w[ cache_read_multi.active_support ], events.map(&:name)
+    assert_equal :fetch_multi, events[0].payload[:super_operation]
+    assert_equal ["b"], events[0].payload[:hits]
   end
 
   private