Only enable verbose_query_logs in Rails server #31690
Conversation
What about setting |
| @@ -94,7 +94,7 @@ def logger | |||
| def debug(progname = nil, &block) | |||
| return unless super | |||
|
|
|||
| if ActiveRecord::Base.verbose_query_logs | |||
| if defined?(Rails::Server) && ActiveRecord::Base.verbose_query_logs | |||
There was a problem hiding this comment.
Another thought: sometimes it's really useful to conditionally enable verbose logging in tests (and, maybe, console and other tasks too), i.e. manually turn it on and off; this hack make it impossible.
Moreover, ActiveRecord could be used without Rails at all.
There was a problem hiding this comment.
Yep. I don't love this first approach either. I'd rather it be configurable and off by default.
There was a problem hiding this comment.
Active Record should not know anything about railties. Maybe we should move this code to the railtie?
initializer 'set_verbose_logs' do
ActiveRecord::Base.verbose_query_logs = defined?(Rails::Server) && config.active_record.verbose_query_logs
end
There was a problem hiding this comment.
I think defined?(Rails::Server) is still the wrong approach, even if we move it to the Railtie. It will disable the feature in too many places: standalone web servers (puma), background job servers (sidekiq), even other built-in Rails contexts (bin/rails runner)...
We should disable it when the console is used, not enable it when the server is used. I like @palkan's suggestion of using the console do block to do that.
There was a problem hiding this comment.
Can we accomplish this by checking whether the FD that the logger is outputting to is interactive? (basically out.repond_to?(:tty?) && out.tty?)
There was a problem hiding this comment.
I don't think so, because both the server and console commands multiplex their log output to an interactive stream (STDOUT for the server and STERR for the console), but we only want to disable verbose query logs on the console.
Ideally we'd always write the verbose query logs to the log file and exclude them from STDOUT/STDERR, but I don't think that's possible with how the multiplexing is implemented.
There was a problem hiding this comment.
Let's use the console block for now in the railtie. If we have another case where we need it disabled too we can revisit.
There was a problem hiding this comment.
@rafaelfranca Updating the PR with your suggestion now.
olivierlacan
force-pushed
the
no-verbose-query-logs-in-console
branch
from
45ce745
to
4e0cc7a
Compare
| @@ -222,5 +222,11 @@ class Railtie < Rails::Railtie # :nodoc: | |||
| end | |||
| end | |||
| end | |||
|
|
|||
| initializer "active_record.set_verbose_logs" do | |||
There was a problem hiding this comment.
@rafaelfranca Let me know if this is what you had in mind?
Should fix rails#31688 unless someone can point me to a better way to achieve this goal. Essentially David's point was that verbose query logging when enabled in Rails console tends to make things very noisy. That's especially true if we display absolute paths to callsites which sadly is still the case when we detect a caller that isn't part of the Rails application — think gems. Discussed this with both @matthewd and @rafaelfranca and went back and forth between enabling if defined?(Rails::Server) or this implementation and this one makes more sense for now. Long term I think it'll make sense to let people override this default disabling in Rails Console because they might want to use the feature but for now it feels like the correct default behavior.
olivierlacan
force-pushed
the
no-verbose-query-logs-in-console
branch
from
4e0cc7a
to
1044e8b
Compare
|
@rafaelfranca Updated to use the console block in AR's railtie. Let me know if that works. Tests look like they're good aside from build errors. |
|
Awesome! Thank you for working on this |
Should fix #31688 unless someone can point me to a better way to achieve
this goal. Essentially David's point was that verbose query logging when
enabled in Rails console tends to make things very noisy.
That's especially true if we display absolute paths to callsites which
sadly is still the case when we detect a caller that isn't part of the
Rails application — think gems.
Related to #26815.
I haven't tested this locally yet so don't merge until the build has run and we can test it out a bit more.