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

Prefer ActiveRecord::Base.logger instead of a custom logger instance in RailsSemanticLogger::ActiveRecord::LogSubscriber #235

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kpumuk
Copy link

@kpumuk kpumuk commented Aug 18, 2024

Description of changes

In this change I propose to switch from using a custom logger instance SemanticLogger["ActiveRecord"] to ::ActiveRecord::Base.logger when logging events from the RailsSemanticLogger::ActiveRecord::LogSubscriber.

  • Current behaviour is inconsistent with what is expected from ActiveRecord. When a .logger class attribute is set on a component level (ActionController, ActionMailer, ActiveRecord), the log subscriber should respect it to allow debugging uses (for example, in the IRB console)
  • This is consistent with how logs are handled in ActiveJob, ActionMailer, etc in semantic logger already
  • Performance consideration for accessing the class variable (if there were any) have been addressed in Rails 7.0 (Make ActiveRecord::Base.logger a class_attribute rails/rails#42237)

What this solves

When in a Rails console, a simple way to turn on AR logging:

ActiveRecord::Base.logger = Logger.new(STDOUT)
# or temporary increase verbosity
ActiveRecord::Base.logger.level = Logger::DEBUG

Concerns

  • name changed from ActiveRecord to ActiveRecord::Base. To preserve old behaviour, one might create an initializer:
     ActiveSupport.on_load(:active_record) do
       ::ActiveRecord::Base.logger = SemanticLogger[::ActiveRecord]
     end
  • There might be a performance impact for Rails < 7.0, although Rails 6.1 official EoL is in a month (https://endoflife.date/rails)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…in RailsSemanticLogger::ActiveRecord::LogSubscriber
@reidmorrison
Copy link
Owner

This should achieve the same result for your environment:

RailsSemanticLogger::ActiveRecord::LogSubscriber.logger = ::ActiveRecord::Base.logger

@kpumuk
Copy link
Author

kpumuk commented Sep 9, 2024

That's true and yes, it is what we're doing now, but the behaviour is rather inconsistent with other Rails components, ActiveJob and ActionMailer, where such re-assignment is not necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants