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.

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

[rails] Redis instrumentation follows users settings #86

Merged
merged 3 commits into from
Mar 8, 2017

Conversation

palazzem
Copy link
Contributor

@palazzem palazzem commented Mar 7, 2017

What it does

In the previous implementation Redis is automatically instrumented despite users settings. This caused many issues when the Rails module is available but you're running external components (i.e. rake or Sidekiq).

  • no actions are done in the before_configuration hook
  • all actions are done in the after_initialize hook, so users' settings defined in the tracing initializer are honored
  • auto-instrumentation output defaults to debug log

Related to

@palazzem palazzem added the bug Involves a bug label Mar 7, 2017
@palazzem palazzem added this to the 0.5.0 milestone Mar 7, 2017
@palazzem palazzem requested a review from ufoot March 7, 2017 11:33
assert_equal(true, Rails.cache.respond_to?(:data), "cache '#{Rails.cache}' has no data")
pin = Datadog::Pin.get_from(Rails.cache.data)

# get the Redis pin accessing private methods (only Rails 3.x)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's reasonable to do that in a test

::Rails.cache.is_a?(::ActiveSupport::Cache::RedisStore)
Datadog::Tracer.log.debug('Enabling auto-instrumentation for redis-rails connector')

# backward compatibility: Rails 3.x doesn't have `cache=` method
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rails 3.x relies in a RAILS_CACHE global; Rails > 3 deprecated this global in favor of a plain Rails.cache

@@ -1,15 +0,0 @@
module RedisStoreDataAccessor
Copy link
Contributor Author

Choose a reason for hiding this comment

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

had to remove this one because it was obfuscating how Rails 3.x works with cache. Think it should be fine now 👍

# We include 'redis-rails' here if it's available, doing it later
# (typically in initialize callback) does not work, it does not
# get loaded in the right context.
require 'redis-rails'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't need to import that here, because it will be available after cache initialization (if present)

Copy link
Contributor

@ufoot ufoot left a comment

Choose a reason for hiding this comment

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

LGTM! This should indeed be much less error-prone than the previous impl.

@palazzem palazzem merged commit 0a12034 into master Mar 8, 2017
@palazzem palazzem deleted the palazzem/rails-redis branch March 8, 2017 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants