From 3835113291febddb4453c8c4863699e2569d8e34 Mon Sep 17 00:00:00 2001 From: Frederik Erbs Spang Thomsen Date: Tue, 27 Aug 2024 19:26:40 +0200 Subject: [PATCH] Align CacheStore Instrumentation keys and output --- activesupport/lib/active_support/cache.rb | 14 ++++--- .../lib/active_support/cache/file_store.rb | 14 ++++++- .../lib/active_support/cache/memory_store.rb | 8 +++- .../active_support/cache/redis_cache_store.rb | 7 +++- .../cache_instrumentation_behavior.rb | 41 ++++++++++++++++--- .../source/active_support_instrumentation.md | 5 --- 6 files changed, 67 insertions(+), 22 deletions(-) diff --git a/activesupport/lib/active_support/cache.rb b/activesupport/lib/active_support/cache.rb index 425b55fc6d086..66fd539a452e1 100644 --- a/activesupport/lib/active_support/cache.rb +++ b/activesupport/lib/active_support/cache.rb @@ -538,10 +538,11 @@ def read_multi(*names) options = names.extract_options! options = merged_options(options) + keys = names.map { |name| normalize_key(name, options) } - instrument_multi :read_multi, names, options do |payload| + instrument_multi :read_multi, keys, options do |payload| read_multi_entries(names, **options, event: payload).tap do |results| - payload[:hits] = results.keys + payload[:hits] = results.keys.map { |name| normalize_key(name, options) } end end end @@ -551,8 +552,9 @@ def write_multi(hash, options = nil) return hash if hash.empty? options = merged_options(options) + normalized_hash = hash.transform_keys { |key| normalize_key(key, options) } - instrument_multi :write_multi, hash, options do |payload| + instrument_multi :write_multi, normalized_hash, options do |payload| entries = hash.each_with_object({}) do |(name, value), memo| memo[normalize_key(name, options)] = Entry.new(value, **options.merge(version: normalize_version(name, options))) end @@ -596,9 +598,9 @@ def fetch_multi(*names) options = names.extract_options! options = merged_options(options) - + keys = names.map { |name| normalize_key(name, options) } writes = {} - ordered = instrument_multi :read_multi, names, options do |payload| + ordered = instrument_multi :read_multi, keys, options do |payload| if options[:force] reads = {} else @@ -610,7 +612,7 @@ def fetch_multi(*names) end writes.compact! if options[:skip_nil] - payload[:hits] = reads.keys + payload[:hits] = reads.keys.map { |name| normalize_key(name, options) } payload[:super_operation] = :fetch_multi ordered diff --git a/activesupport/lib/active_support/cache/file_store.rb b/activesupport/lib/active_support/cache/file_store.rb index 8d8bc7f8c3fdd..5e815b85c1f0b 100644 --- a/activesupport/lib/active_support/cache/file_store.rb +++ b/activesupport/lib/active_support/cache/file_store.rb @@ -58,7 +58,12 @@ def cleanup(options = nil) # cache.increment("baz") # => 6 # def increment(name, amount = 1, options = nil) - modify_value(name, amount, options) + options = merged_options(options) + key = normalize_key(name, options) + + instrument(:increment, key, amount: amount) do + modify_value(name, amount, options) + end end # Decrement a cached integer value. Returns the updated value. @@ -73,7 +78,12 @@ def increment(name, amount = 1, options = nil) # cache.decrement("baz") # => 4 # def decrement(name, amount = 1, options = nil) - modify_value(name, -amount, options) + options = merged_options(options) + key = normalize_key(name, options) + + instrument(:decrement, key, amount: amount) do + modify_value(name, -amount, options) + end end def delete_matched(matcher, options = nil) diff --git a/activesupport/lib/active_support/cache/memory_store.rb b/activesupport/lib/active_support/cache/memory_store.rb index 2fe57190d8e7b..9d727258e0bef 100644 --- a/activesupport/lib/active_support/cache/memory_store.rb +++ b/activesupport/lib/active_support/cache/memory_store.rb @@ -147,7 +147,9 @@ def pruning? # cache.increment("baz") # => 6 # def increment(name, amount = 1, options = nil) - modify_value(name, amount, options) + instrument(:increment, name, amount: amount) do + modify_value(name, amount, options) + end end # Decrement a cached integer value. Returns the updated value. @@ -162,7 +164,9 @@ def increment(name, amount = 1, options = nil) # cache.decrement("baz") # => 4 # def decrement(name, amount = 1, options = nil) - modify_value(name, -amount, options) + instrument(:decrement, name, amount: amount) do + modify_value(name, -amount, options) + end end # Deletes cache entries if the cache key matches a given pattern. diff --git a/activesupport/lib/active_support/cache/redis_cache_store.rb b/activesupport/lib/active_support/cache/redis_cache_store.rb index 4335f5c9a5de1..e55b87b0e5640 100644 --- a/activesupport/lib/active_support/cache/redis_cache_store.rb +++ b/activesupport/lib/active_support/cache/redis_cache_store.rb @@ -173,9 +173,12 @@ def read_multi(*names) return {} if names.empty? options = names.extract_options! - instrument_multi(:read_multi, names, options) do |payload| + options = merged_options(options) + keys = names.map { |name| normalize_key(name, options) } + + instrument_multi(:read_multi, keys, options) do |payload| read_multi_entries(names, **options).tap do |results| - payload[:hits] = results.keys + payload[:hits] = results.keys.map { |name| normalize_key(name, options) } end end end diff --git a/activesupport/test/cache/behaviors/cache_instrumentation_behavior.rb b/activesupport/test/cache/behaviors/cache_instrumentation_behavior.rb index a25bea26bbf82..ef5e642d6cbbd 100644 --- a/activesupport/test/cache/behaviors/cache_instrumentation_behavior.rb +++ b/activesupport/test/cache/behaviors/cache_instrumentation_behavior.rb @@ -14,7 +14,7 @@ def test_write_multi_instrumentation assert_equal %w[ cache_write_multi.active_support ], events.map(&:name) assert_nil events[0].payload[:super_operation] - assert_equal({ key_1 => value_1, key_2 => value_2 }, events[0].payload[:key]) + assert_equal({ normalized_key(key_1) => value_1, normalized_key(key_2) => value_2 }, events[0].payload[:key]) end def test_instrumentation_with_fetch_multi_as_super_operation @@ -29,8 +29,8 @@ def test_instrumentation_with_fetch_multi_as_super_operation assert_equal %w[ cache_read_multi.active_support ], events.map(&:name) assert_equal :fetch_multi, events[0].payload[:super_operation] - assert_equal [key_2, key_1], events[0].payload[:key] - assert_equal [key_1], events[0].payload[:hits] + assert_equal [normalized_key(key_2), normalized_key(key_1)], events[0].payload[:key] + assert_equal [normalized_key(key_1)], events[0].payload[:hits] assert_equal @cache.class.name, events[0].payload[:store] end @@ -59,8 +59,35 @@ def test_read_multi_instrumentation end assert_equal %w[ cache_read_multi.active_support ], events.map(&:name) - assert_equal [key_2, key_1], events[0].payload[:key] - assert_equal [key_1], events[0].payload[:hits] + assert_equal [normalized_key(key_2), normalized_key(key_1)], events[0].payload[:key] + assert_equal [normalized_key(key_1)], events[0].payload[:hits] + assert_equal @cache.class.name, events[0].payload[:store] + end + + def test_increment_instrumentation + key_1 = SecureRandom.uuid + @cache.write(key_1, 0) + + events = with_instrumentation "increment" do + @cache.increment(key_1) + end + + assert_equal %w[ cache_increment.active_support ], events.map(&:name) + assert_equal normalized_key(key_1), events[0].payload[:key] + assert_equal @cache.class.name, events[0].payload[:store] + end + + + def test_decrement_instrumentation + key_1 = SecureRandom.uuid + @cache.write(key_1, 0) + + events = with_instrumentation "decrement" do + @cache.decrement(key_1) + end + + assert_equal %w[ cache_decrement.active_support ], events.map(&:name) + assert_equal normalized_key(key_1), events[0].payload[:key] assert_equal @cache.class.name, events[0].payload[:store] end @@ -75,4 +102,8 @@ def with_instrumentation(method) ensure ActiveSupport::Notifications.unsubscribe event_name end + + def normalized_key(key) + @cache.send(:normalize_key, key, @cache.options) + end end diff --git a/guides/source/active_support_instrumentation.md b/guides/source/active_support_instrumentation.md index 507feaa339414..dd27dbd03fe1e 100644 --- a/guides/source/active_support_instrumentation.md +++ b/guides/source/active_support_instrumentation.md @@ -599,9 +599,6 @@ Cache stores may add their own data as well. #### `cache_increment.active_support` -This event is only emitted when using [`MemCacheStore`][ActiveSupport::Cache::MemCacheStore] -or [`RedisCacheStore`][ActiveSupport::Cache::RedisCacheStore]. - | Key | Value | | --------- | ----------------------- | | `:key` | Key used in the store | @@ -618,8 +615,6 @@ or [`RedisCacheStore`][ActiveSupport::Cache::RedisCacheStore]. #### `cache_decrement.active_support` -This event is only emitted when using the Memcached or Redis cache stores. - | Key | Value | | --------- | ----------------------- | | `:key` | Key used in the store |