TwP / logging-rails

Railtie for integrating the Logging framework with Rails
81 stars 27 forks source link

breaks ActiveSupport::OrderedOptions #10

Closed igorbernstein closed 9 years ago

igorbernstein commented 12 years ago

ActiveSupport::OrderedOptions relies on method_missing to configure a rails application. By default logging-rails monkey patches Object to include Logging.globally. This makes it impossible to configure loggers:

Rails.configuration.assets.logger = Logging::Logger[Sprockets]
puts Rails.configuration.assets.logger.name # ActiveSupport::OrderedOptions

My current workaround is:

ActiveSupport::OrderedOptions.send :undef_method, :logger
TwP commented 11 years ago

Huh! Let me look into that one. It would explain curious behavior that I have observed elsewhere.

The workaround looks like the correct solution.

pkuczynski commented 10 years ago

@TwP I can confirm that solution from @igorbernstein works for me too! When could you add it in?

gingerlime commented 9 years ago

After updating logging-rails to the latest version, I no longer have logger globally accessible by default like it used to (now that the monkey-patching is gone). It seems like classes are able to access it, but not modules...

What's the best way to access logger consistently in all places then? (other than doing include Logging.globally?)

p.s. the gem still seems to have this include Logging.globally, but not the version on github.

TwP commented 9 years ago

The Logging.globally method creates a Module that then gets included into the Object class. The module defines a logger method that does this:

def logger
  @_logging_logger ||= Logging.logger[self]
end

So that method definition is at odds with ActiveSupport for whatever bizarre reason. Not including the module in the global scope is the solution here.

But as @gingerlime points out, what do you do if you want that logger method everywhere? You'll just have to include a module somewhere else in your code.

I'm working on a Logging release, and logging-rails will get updated, too.

gingerlime commented 9 years ago

Thanks for the update / clarification @TwP !

In our case, we certainly did use logger everywhere, so when I switched to using the github version, things were broken, until I manually added Logging.globally. Not sure if it's going to affect many users once the logging-rails update goes into rubygems and people upgrade though, but it feels like a potentially backwards-incompatible change to me.

I'm wondering if there's some middle-grounds solution to this. Something that doesn't break ActiveSupport, but is still easy and DRY to use from within Rails??

igorbernstein commented 9 years ago

The issue is not ActiveSupport ... its anything that relies on method_missing and doesn't inherit from BasicObject directly.

Monkey patching Object is generally a bad idea because you have no control how far the effects ripple. I really don't understand why including the Logging module in all the classes that need logging is problematic.

Having said that, if you are still hellbent on monkey patching Object, you can probably hack around this issue by introducing the logger

gingerlime commented 9 years ago

Hi @igorbernstein, I'm sorry but I'm not sure I follow you completely.

Are you saying that logging-rails should continue to monkey-patch Object, so all objects have access to logger or that it shouldn't do it?

I know that monkey-patching is generally fraught with danger, and should be avoided. Particularly when it comes to monkey-patching things like Object. But is there another / better way to allow all modules and classes in my rails app to have access to logger and maintain the context info of where the logging took place in the code?

igorbernstein commented 9 years ago

I would just include the module everywhere it's needed...the cost of an extra include line per class hierarchy is well worth not having to worry about these kind of issues

TwP commented 9 years ago

Including the Logging::Rails::Mixin in the locations where you want the logger method is the way to go here. A bit of a pain, but it seems the best way forward.

The logger method is intelligent, so you would only need to include it in a few places: ActiveRecord::Base for models, ApplicationController for controller items. Etc.

Or you can continue using include Logging.globally in the Object class and then do the undef magic in ActiveSupport::OrderedOptions.

TwP commented 9 years ago

Digging into the rails source a bit, method_missing appears in a lot of classes. All of these will exhibit odd behaviors around logging and the logger method.

gingerlime commented 9 years ago

Thanks for clarifying.

Sorry to nag, but just wondering about the actual syntax to include this. I should add an include Logging::Rails::Mixin in each and every one of the classes and modules that are not part of the model / controller hierarchy. Right?

I'm somehow still having trouble with this. e.g. lets say I have a module in lib

module MyModule
  include Logging::Rails::Mixin

  def my_method
    logger.info('my method called')
  end
end

When I try to include my module, I get an error:

irb(main):001:0> include MyModule
NameError: method `logger' not defined in MyModule
   from /usr/lib/ruby/gems/2.2.0/gems/logging-rails-0.5.0/lib/logging/rails/mixin.rb:18:in `remove_method'
  from /usr/lib/ruby/gems/2.2.0/gems/logging-rails-0.5.0/lib/logging/rails/mixin.rb:18:in `included'
  from /tmp/bla/lib/my_module.rb:2:in `include'
  ...

Same happens with plain-ruby classes that are on the autoload_path (e.g. lib). I'm probably not including the right thing, or am I doing some other silly mistake?

(tested using the console on a new rails 4.2 app with the latest logging-rails from rubygems.org)

joker-777 commented 9 years ago

Hi @TwP ,

I have similar problems like @gingerlime. It would be great if you could give us more help here.

Thanks,