Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Always instrument ActiveSupport::Cache #15943

Merged
merged 1 commit into from
Jun 27, 2014
Merged

Always instrument ActiveSupport::Cache #15943

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 10 additions & 11 deletions activesupport/lib/active_support/cache.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
require 'active_support/core_ext/numeric/time'
require 'active_support/core_ext/object/to_param'
require 'active_support/core_ext/string/inflections'
require 'active_support/deprecation'

module ActiveSupport
# See ActiveSupport::Cache::Store for documentation.
Expand Down Expand Up @@ -178,14 +179,16 @@ def mute
@silence = previous_silence
end

# Set to +true+ if cache stores should be instrumented.
# Default is +false+.
# :deprecated:
def self.instrument=(boolean)
Thread.current[:instrument_cache_store] = boolean
ActiveSupport::Deprecation.warn "ActiveSupport::Cache.instrument is deprecated and will be removed in Rails 5. Instrumentation is now always on so you can safely stop using it."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should print a deprecation warning for these methods. Could you add it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is the preferred way to do it so please let me know if I should use a different method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need to improve the test to tell people we will remove these methods on future. Something like:

ActiveSupport::Cache.instrument is deprecated and will be removed on Rails 5. Instrumentation is now always on so you can safely stop of using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Message updated.

true
end

# :deprecated:
def self.instrument
Thread.current[:instrument_cache_store] || false
ActiveSupport::Deprecation.warn "ActiveSupport::Cache.instrument is deprecated and will be removed in Rails 5. Instrumentation is now always on so you can safely stop using it."
true
end

# Fetches data from the cache, using the given key. If there is data in
Expand Down Expand Up @@ -539,13 +542,9 @@ def namespaced_key(key, options)
def instrument(operation, key, options = nil)
log(operation, key, options)

if self.class.instrument
payload = { :key => key }
payload.merge!(options) if options.is_a?(Hash)
ActiveSupport::Notifications.instrument("cache_#{operation}.active_support", payload){ yield(payload) }
else
yield(nil)
end
payload = { :key => key }
payload.merge!(options) if options.is_a?(Hash)
ActiveSupport::Notifications.instrument("cache_#{operation}.active_support", payload){ yield(payload) }
end

def log(operation, key, options = nil)
Expand Down