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

Only enable verbose_query_logs in Rails server #31690

Merged

Conversation

@olivierlacan
Copy link
Contributor

@olivierlacan olivierlacan commented Jan 12, 2018

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.

@palkan
Copy link
Contributor

@palkan palkan commented Jan 12, 2018

@olivierlacan

a better way to achieve
this goal

What about setting verbose_query_logs to false in ActiveRecord::Railtie#console?

activerecord/lib/active_record/log_subscriber.rb Outdated
@@ -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

This comment has been minimized.

@palkan

palkan Jan 12, 2018
Contributor

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.

This comment has been minimized.

@olivierlacan

olivierlacan Jan 12, 2018
Author Contributor

Yep. I don't love this first approach either. I'd rather it be configurable and off by default.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 12, 2018
Member

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

This comment has been minimized.

@eugeneius

eugeneius Jan 12, 2018
Member

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.

This comment has been minimized.

@sgrif

sgrif Jan 12, 2018
Contributor

Can we accomplish this by checking whether the FD that the logger is outputting to is interactive? (basically out.repond_to?(:tty?) && out.tty?)

This comment has been minimized.

@eugeneius

eugeneius Jan 12, 2018
Member

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.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 18, 2018
Member

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.

This comment has been minimized.

@olivierlacan

olivierlacan Jan 18, 2018
Author Contributor

@rafaelfranca Updating the PR with your suggestion now.

@dhh dhh added this to the 5.2.0 milestone Jan 18, 2018
@olivierlacan olivierlacan force-pushed the olivierlacan:no-verbose-query-logs-in-console branch Jan 18, 2018
activerecord/lib/active_record/railtie.rb Outdated
@@ -222,5 +222,11 @@ class Railtie < Rails::Railtie # :nodoc:
end
end
end

initializer "active_record.set_verbose_logs" do

This comment has been minimized.

@olivierlacan

olivierlacan Jan 18, 2018
Author Contributor

@rafaelfranca Let me know if this is what you had in mind?

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.

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 olivierlacan force-pushed the olivierlacan:no-verbose-query-logs-in-console branch to 1044e8b Jan 23, 2018
@olivierlacan
Copy link
Contributor Author

@olivierlacan olivierlacan commented Jan 24, 2018

@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.

@rafaelfranca rafaelfranca merged commit 554dcc4 into rails:master Jan 24, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
codeclimate All good!
Details
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 24, 2018

Awesome! Thank you for working on this

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

Successfully merging this pull request may close these issues.

6 participants