Correctly set rails.cache.backend span tag for multiple stores

[marcotc]

What does this PR do?

This PR fixes the span tag rails.cache.backend in rails.cache spans to correctly represent the cache store object being instrumented.

Motivation

Today, we always set rails.cache.backend to Rails.configuration.cache_store, which is a global application value.
This works correctly for applications with a single cache store, but incorrectly reports this tag when multiple stores are used.

Additional Notes

How to test the change?

All changes are covered by testing.

[TonyCTHsu]

Nice, I think with sufficient repetitions, we should be confident to extract this.

[GustavoCaso]

Could you elaborate on why the need for this change? Should we extract this change into a separate PR?

[marcotc]

Sorry, @GustavoCaso, because it's a draft PR, I make a bunch of mess before getting it ready.
Mucking about with the Gemfile makes my testing easier locally.

[marcotc]

I also push my changes at the end of day in case my laptop explodes overnight.

[GustavoCaso]

I also push my changes at the end of day in case my laptop explodes overnight.

Good call

[GustavoCaso]

Would using regex for each command executed on the cache cause performance issues for the customer's application?

We could memoize the result on an instance variable

def store_name
  return @store_name if defined?(@store_name)
  @store_name = self.class.name.underscore.match(/active_support\/cache\/(.*)/)[1]
  @store_name
end

Also, we always are getting the last portion of the match. Could we use split(''/'').last rather than a regex?

def store_name
  return @store_name if defined?(@store_name)
  @store_name = self.class.name.underscore.split('/').last
  @store_name
end

[GustavoCaso]

Should we add a comment to explain that underscore is from Rails::ActiveSupport and that this codes works because is executed within the context of Rails

[marcotc]

Caching is great, I made those changes.

[marcotc]

Regarding "split+last" vs regex, I'm keeping the regex because it's the version that more closely mirrors ActiveSupport's logic (require "active_support/cache/#{store}").

And because we are only doing it once, as we are now caching, the performance impact will be almost nil.

[GustavoCaso]

I noticed that on the lib/datadog/tracing/contrib/active_support/cache/patcher.rb file, we have an outdated method
cache_store_class which takes symbol but no longer do anything with it.

dd-trace-rb/lib/datadog/tracing/contrib/active_support/cache/patcher.rb

Lines 31 to 45 in e3d2c37

	def cache_store_class(meth)
	::ActiveSupport::Cache::Store
	end
	def patch_cache_store_read
	cache_store_class(:read).prepend(Cache::Instrumentation::Read)
	end
	def patch_cache_store_read_multi
	cache_store_class(:read_multi).prepend(Cache::Instrumentation::ReadMulti)
	end
	def patch_cache_store_fetch
	cache_store_class(:fetch).prepend(Cache::Instrumentation::Fetch)
	end

Since we are changing related files, should we update it?

Since that change is not related to the main purpose of the PR, fell free to ignore it

[marcotc]

I noticed that on the lib/datadog/tracing/contrib/active_support/cache/patcher.rb file, we have an outdated method cache_store_class which takes symbol but no longer do anything with it.

dd-trace-rb/lib/datadog/tracing/contrib/active_support/cache/patcher.rb

Lines 31 to 45 in e3d2c37

	def cache_store_class(meth)
	::ActiveSupport::Cache::Store
	end
	def patch_cache_store_read
	cache_store_class(:read).prepend(Cache::Instrumentation::Read)
	end
	def patch_cache_store_read_multi
	cache_store_class(:read_multi).prepend(Cache::Instrumentation::ReadMulti)
	end
	def patch_cache_store_fetch
	cache_store_class(:fetch).prepend(Cache::Instrumentation::Fetch)
	end

Since we are changing related files, should we update it?

Since that change is not related to the main purpose of the PR, fell free to ignore it

This is because there's a conditional override for that method, depending if the gem https://github.com/redis-store/redis-activesupport is installed or not. It's still needed for such compatibility:

dd-trace-rb/lib/datadog/tracing/contrib/active_support/cache/redis.rb

Lines 31 to 37 in 94158db

	def cache_store_class(meth)
	if patch_redis?(meth)
	::ActiveSupport::Cache::RedisStore
	else
	super
	end
	end

[GustavoCaso]

This is because there's a conditional override for that method, depending if the gem https://github.com/redis-store/redis-activesupport is installed or not. It's still needed for such compatibility:
dd-trace-rb/lib/datadog/tracing/contrib/active_support/cache/redis.rb

Thanks for the context. It would be nice to note that rails 5.2.0+ has redis cache support. Maybe adding a comment that once we do not support old Rails version we can remove such code 😄

[GustavoCaso]

LGTM

[marcotc]

This is because there's a conditional override for that method, depending if the gem redis-store/redis-activesupport is installed or not. It's still needed for such compatibility:
dd-trace-rb/lib/datadog/tracing/contrib/active_support/cache/redis.rb

Thanks for the context. It would be nice to note that rails 5.2.0+ has redis cache support. Maybe adding a comment that once we do not support old Rails version we can remove such code 😄

@GustavoCaso I documented this in code, good call!

[pbstriker38]

This fails for custom stores that are not in active_support/cache/