diff --git a/lib/datadog/tracing/contrib/active_support/cache/instrumentation.rb b/lib/datadog/tracing/contrib/active_support/cache/instrumentation.rb index 854b6b3d587..9bd6ffc9176 100644 --- a/lib/datadog/tracing/contrib/active_support/cache/instrumentation.rb +++ b/lib/datadog/tracing/contrib/active_support/cache/instrumentation.rb @@ -16,7 +16,7 @@ module Instrumentation # @param action [String] type of cache operation. Will be set as the span resource. # @param key [Object] redis cache key. Used for actions with a single key locator. # @param multi_key [Array] list of redis cache keys. Used for actions with a multiple key locators. - def trace(action, key: nil, multi_key: nil) + def trace(action, store, key: nil, multi_key: nil) return yield unless enabled? # create a new ``Span`` and add it to the tracing context @@ -29,7 +29,7 @@ def trace(action, key: nil, multi_key: nil) span.set_tag(Tracing::Metadata::Ext::TAG_COMPONENT, Ext::TAG_COMPONENT) span.set_tag(Tracing::Metadata::Ext::TAG_OPERATION, Ext::TAG_OPERATION_CACHE) - set_backend(span) + span.set_tag(Ext::TAG_CACHE_BACKEND, store) if store set_cache_key(span, key, multi_key) yield @@ -50,13 +50,6 @@ def nested_multiread? current_span && current_span.name == Ext::SPAN_CACHE && current_span.resource == Ext::RESOURCE_CACHE_MGET end - def set_backend(span) - if defined?(::Rails) - store, = *Array.wrap(::Rails.configuration.cache_store).flatten - span.set_tag(Ext::TAG_CACHE_BACKEND, store) - end - end - def set_cache_key(span, single_key, multi_key) if multi_key resolved_key = multi_key.map { |key| ::ActiveSupport::Cache.expand_cache_key(key) } @@ -73,61 +66,110 @@ def enabled? Tracing.enabled? && Datadog.configuration.tracing[:active_support][:enabled] end + # Instance methods injected into the cache client + module InstanceMethods + private + + # The name of the store is never saved. + # ActiveSupport looks up stores by converting a symbol into a 'require' path, + # then "camelizing" it for a `const_get` call: + # ``` + # require "active_support/cache/#{store}" + # ActiveSupport::Cache.const_get(store.to_s.camelize) + # ``` + # @see https://github.com/rails/rails/blob/261975dbef77731d2c76f907f1076c5132ebc0e4/activesupport/lib/active_support/cache.rb#L139-L149 + # + # As `self` is the store object, we can reverse engineer + # the original symbol by converting the class name to snake case: + # e.g. ActiveSupport::Cache::RedisStore -> active_support/cache/redis_store + # In this case, `redis_store` is the store name. + # + # Because there's no API retrieve only the class name + # (only `RedisStore`, and not `ActiveSupport::Cache::RedisStore`) + # the easiest way to retrieve the store symbol is to convert the fully qualified + # name using the Rails-provided method `#underscore`, which is the reverse of `#camelize`, + # then extracting the last part of it. + # + # Also, this method caches the store name, given this value will be retrieve + # multiple times and involves string manipulation. + def dd_store_name + return @store_name if defined?(@store_name) + + # DEV: String#underscore is available through ActiveSupport, and is + # DEV: the exact reverse operation to `#camelize`. + @store_name = self.class.name.underscore.match(%r{active_support/cache/(.*)})[1] + end + end + # Defines instrumentation for ActiveSupport cache reading module Read + include InstanceMethods + def read(*args, &block) return super if Instrumentation.nested_read? - Instrumentation.trace(Ext::RESOURCE_CACHE_GET, key: args[0]) { super } + Instrumentation.trace(Ext::RESOURCE_CACHE_GET, dd_store_name, key: args[0]) { super } end end # Defines instrumentation for ActiveSupport cache reading of multiple keys module ReadMulti + include InstanceMethods + def read_multi(*keys, &block) return super if Instrumentation.nested_multiread? - Instrumentation.trace(Ext::RESOURCE_CACHE_MGET, multi_key: keys) { super } + Instrumentation.trace(Ext::RESOURCE_CACHE_MGET, dd_store_name, multi_key: keys) { super } end end # Defines instrumentation for ActiveSupport cache fetching module Fetch + include InstanceMethods + def fetch(*args, &block) return super if Instrumentation.nested_read? - Instrumentation.trace(Ext::RESOURCE_CACHE_GET, key: args[0]) { super } + Instrumentation.trace(Ext::RESOURCE_CACHE_GET, dd_store_name, key: args[0]) { super } end end # Defines instrumentation for ActiveSupport cache fetching of multiple keys module FetchMulti + include InstanceMethods + def fetch_multi(*args, &block) return super if Instrumentation.nested_multiread? keys = args[-1].instance_of?(Hash) ? args[0..-2] : args - Instrumentation.trace(Ext::RESOURCE_CACHE_MGET, multi_key: keys) { super } + Instrumentation.trace(Ext::RESOURCE_CACHE_MGET, dd_store_name, multi_key: keys) { super } end end # Defines instrumentation for ActiveSupport cache writing module Write + include InstanceMethods + def write(*args, &block) - Instrumentation.trace(Ext::RESOURCE_CACHE_SET, key: args[0]) { super } + Instrumentation.trace(Ext::RESOURCE_CACHE_SET, dd_store_name, key: args[0]) { super } end end # Defines instrumentation for ActiveSupport cache writing of multiple keys module WriteMulti + include InstanceMethods + def write_multi(hash, options = nil) - Instrumentation.trace(Ext::RESOURCE_CACHE_MSET, multi_key: hash.keys) { super } + Instrumentation.trace(Ext::RESOURCE_CACHE_MSET, dd_store_name, multi_key: hash.keys) { super } end end # Defines instrumentation for ActiveSupport cache deleting module Delete + include InstanceMethods + def delete(*args, &block) - Instrumentation.trace(Ext::RESOURCE_CACHE_DELETE, key: args[0]) { super } + Instrumentation.trace(Ext::RESOURCE_CACHE_DELETE, dd_store_name, key: args[0]) { super } end end end diff --git a/lib/datadog/tracing/contrib/active_support/cache/patcher.rb b/lib/datadog/tracing/contrib/active_support/cache/patcher.rb index 9a98a4ebe53..2d1431732e1 100644 --- a/lib/datadog/tracing/contrib/active_support/cache/patcher.rb +++ b/lib/datadog/tracing/contrib/active_support/cache/patcher.rb @@ -28,6 +28,9 @@ def patch patch_cache_store_delete end + # This method is overwritten by + # `datadog/tracing/contrib/active_support/cache/redis.rb` + # with more complex behavior. def cache_store_class(meth) ::ActiveSupport::Cache::Store end diff --git a/spec/datadog/tracing/contrib/rails/cache_spec.rb b/spec/datadog/tracing/contrib/rails/cache_spec.rb index cc7f9d24481..67dbbbbd4e6 100644 --- a/spec/datadog/tracing/contrib/rails/cache_spec.rb +++ b/spec/datadog/tracing/contrib/rails/cache_spec.rb @@ -24,7 +24,7 @@ let(:key) { 'custom-key' } let(:multi_keys) { %w[custom-key-1 custom-key-2 custom-key-3] } - shared_examples 'a no-op when instrumentation is disabled' do + shared_examples 'an instrumented cache method' do context 'disabled at integration level' do before { Datadog.configuration.tracing[:active_support].enabled = false } after { Datadog.configuration.tracing[:active_support].reset! } @@ -47,6 +47,24 @@ expect { subject }.to_not(change { fetch_spans }) end end + + context "with a cache different from Rails' default store" do + let(:cache) { ActiveSupport::Cache::MemoryStore.new } + + before do + expect(cache).to_not be_a(Rails.cache.class) # Sanity check that they are different + end + + it 'returns the matching backend type' do + subject + expect(spans[0].get_tag('rails.cache.backend')).to eq('memory_store') + end + end + + it_behaves_like 'measured span for integration', false do + before { subject } + let(:span) { spans[0] } + end end describe '#read' do @@ -54,13 +72,7 @@ before { cache.write(key, 50) } - it_behaves_like 'a no-op when instrumentation is disabled' - - it_behaves_like 'measured span for integration', false do - before { read } - - let(:span) { spans.first } - end + it_behaves_like 'an instrumented cache method' it do expect(read).to eq(50) @@ -71,7 +83,7 @@ expect(get.span_type).to eq('cache') expect(get.resource).to eq('GET') expect(get.service).to eq('rails-cache') - expect(get.get_tag('rails.cache.backend').to_s).to eq('file_store') + expect(get.get_tag('rails.cache.backend')).to eq('file_store') expect(get.get_tag('rails.cache.key')).to eq(key) expect(set.name).to eq('rails.cache') @@ -92,13 +104,7 @@ before { multi_keys.each { |key| cache.write(key, 50 + key[-1].to_i) } } - it_behaves_like 'a no-op when instrumentation is disabled' - - it_behaves_like 'measured span for integration', false do - before { read_multi } - - let(:span) { spans[0] } - end + it_behaves_like 'an instrumented cache method' it do expect(read_multi).to eq(Hash[multi_keys.zip([51, 52, 53])]) @@ -108,7 +114,7 @@ expect(get.span_type).to eq('cache') expect(get.resource).to eq('MGET') expect(get.service).to eq('rails-cache') - expect(get.get_tag('rails.cache.backend').to_s).to eq('file_store') + expect(get.get_tag('rails.cache.backend')).to eq('file_store') expect(JSON.parse(get.get_tag('rails.cache.keys'))).to eq(multi_keys) expect(get.get_tag(Datadog::Tracing::Metadata::Ext::TAG_COMPONENT)) .to eq('active_support') @@ -128,11 +134,7 @@ describe '#write' do subject(:write) { cache.write(key, 50) } - it_behaves_like 'a no-op when instrumentation is disabled' - - it_behaves_like 'measured span for integration', false do - before { write } - end + it_behaves_like 'an instrumented cache method' it do write @@ -140,7 +142,7 @@ expect(span.span_type).to eq('cache') expect(span.resource).to eq('SET') expect(span.service).to eq('rails-cache') - expect(span.get_tag('rails.cache.backend').to_s).to eq('file_store') + expect(span.get_tag('rails.cache.backend')).to eq('file_store') expect(span.get_tag('rails.cache.key')).to eq(key) expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_COMPONENT)) @@ -181,11 +183,7 @@ end end - it_behaves_like 'a no-op when instrumentation is disabled' - - it_behaves_like 'measured span for integration', false do - before { write_multi } - end + it_behaves_like 'an instrumented cache method' it do write_multi @@ -193,7 +191,7 @@ expect(span.span_type).to eq('cache') expect(span.resource).to eq('MSET') expect(span.service).to eq('rails-cache') - expect(span.get_tag('rails.cache.backend').to_s).to eq('file_store') + expect(span.get_tag('rails.cache.backend')).to eq('file_store') expect(JSON.parse(span.get_tag('rails.cache.keys'))).to eq(multi_keys) expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_COMPONENT)) @@ -244,10 +242,7 @@ describe '#delete' do subject(:delete) { cache.delete(key) } - it_behaves_like 'a no-op when instrumentation is disabled' - it_behaves_like 'measured span for integration', false do - before { delete } - end + it_behaves_like 'an instrumented cache method' it do delete @@ -255,7 +250,7 @@ expect(span.span_type).to eq('cache') expect(span.resource).to eq('DELETE') expect(span.service).to eq('rails-cache') - expect(span.get_tag('rails.cache.backend').to_s).to eq('file_store') + expect(span.get_tag('rails.cache.backend')).to eq('file_store') expect(span.get_tag('rails.cache.key')).to eq(key) expect(span.get_tag(Datadog::Tracing::Metadata::Ext::TAG_COMPONENT)) @@ -268,14 +263,7 @@ describe '#fetch' do subject(:fetch) { cache.fetch(key) { 'default' } } - it_behaves_like 'a no-op when instrumentation is disabled' - - it_behaves_like 'measured span for integration', false do - before { fetch } - # Choose either GET or SET span - - let(:span) { spans.sample } - end + it_behaves_like 'an instrumented cache method' context 'with exception' do subject(:fetch) { cache.fetch('exception') { raise 'oops' } } @@ -287,7 +275,7 @@ expect(span.span_type).to eq('cache') expect(span.resource).to eq('GET') expect(span.service).to eq('rails-cache') - expect(span.get_tag('rails.cache.backend').to_s).to eq('file_store') + expect(span.get_tag('rails.cache.backend')).to eq('file_store') expect(span.get_tag('rails.cache.key')).to eq('exception') expect(span.get_tag('error.type')).to eq('RuntimeError') expect(span.get_tag('error.message')).to eq('oops') @@ -310,14 +298,7 @@ end end - it_behaves_like 'a no-op when instrumentation is disabled' - - it_behaves_like 'measured span for integration', false do - before { fetch_multi } - # Choose either GET or SET span - - let(:span) { spans.sample } - end + it_behaves_like 'an instrumented cache method' context 'with exception' do subject(:fetch_multi) { cache.fetch_multi('exception', 'another', 'one') { raise 'oops' } } @@ -328,7 +309,7 @@ expect(span.span_type).to eq('cache') expect(span.resource).to eq('MGET') expect(span.service).to eq('rails-cache') - expect(span.get_tag('rails.cache.backend').to_s).to eq('file_store') + expect(span.get_tag('rails.cache.backend')).to eq('file_store') expect(span.get_tag('rails.cache.keys')).to eq('["exception", "another", "one"]') expect(span.get_tag('error.type')).to eq('RuntimeError') expect(span.get_tag('error.message')).to eq('oops')