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 server: Allow to explicitly specify whether to output Rails's log to stdout #28266

Merged
merged 1 commit into from Jul 8, 2018

Conversation

Projects
None yet
4 participants
@doits
Copy link
Contributor

doits commented Mar 3, 2017

Summary

Allow to explicitly specify whether to output Rails's log to stdout

Before Rails' logger output is mirrored to std out if:

* environment is development and
* the process is not daemonized

It was not possible to change that behaviour, e.g. to disable log output
in that case or enable it in other cases.

Now you can explicitly disable or enable output with the new command
line switch -l, regardless of any other circumstances.

// enable output even in production
rails server -e production -l

// disable output in development
rails server -e development -l false

Enabling output when daemonized still makes no sense (since tty is
detached), so the flag is ignored then and daemonizing always disables
output.

If the command line flag -l is not specified, the old behaviour still
applies, so this change is completely backward compatible.

@doits doits force-pushed the Stellenticket:allow_disable_server_stdout_logging branch 4 times, most recently Mar 3, 2017

@maclover7 maclover7 added the railties label Mar 11, 2017

@gmcgibbon
Copy link
Member

gmcgibbon left a comment

Still an issue on Rails 6 (a0061d2). I can think of a few instances where I wanted to log output to STDOUT in a different environment but couldn't so 👍

railties/lib/rails/commands/server/server_command.rb Outdated
@@ -112,12 +112,18 @@ class ServerCommand < Base # :nodoc:
desc: "Specifies the PID file."
class_option "dev-caching", aliases: "-C", type: :boolean, default: nil,
desc: "Specifies whether to perform caching in development."
class_option "log-stdout", aliases: "-l", type: :boolean, default: nil,

This comment has been minimized.

@gmcgibbon

gmcgibbon Jul 7, 2018

Member

Should the alias be -s to signify output is going to STDOUT or maybe -v for verbose? -l is good too but I'd like to hear some other opinions on this.

This comment has been minimized.

@kaspth

kaspth Jul 7, 2018

Member

I don't think this has enough use to need a short hand.

This comment has been minimized.

@doits

doits Jul 7, 2018

Author Contributor

I just removed the shorthand.

railties/lib/rails/commands/server/server_command.rb Outdated

def initialize(args = [], local_options = {}, config = {})
@original_options = local_options
super
@server = self.args.shift
@log_stdout = options[:daemon].blank? && (options[:environment] || Rails.env) == "development"
@log_stdout =

This comment has been minimized.

@gmcgibbon

gmcgibbon Jul 7, 2018

Member

Maybe this should be moved to a memoized private function named something like log_to_stdout?? I think with the logic change this makes sense.

This comment has been minimized.

@kaspth

kaspth Jul 7, 2018

Member

Seems good. I wouldn't even memoize it. It's not a candidate for being called thousands of times, so the 2-3 calls in a server boot are fine to redo the work.

This comment has been minimized.

@doits

doits Jul 7, 2018

Author Contributor

Alright, removed the memoizing.

railties/lib/rails/commands/server/server_command.rb Outdated
@@ -112,12 +112,18 @@ class ServerCommand < Base # :nodoc:
desc: "Specifies the PID file."
class_option "dev-caching", aliases: "-C", type: :boolean, default: nil,
desc: "Specifies whether to perform caching in development."
class_option "log-stdout", aliases: "-l", type: :boolean, default: nil,

This comment has been minimized.

@kaspth

kaspth Jul 7, 2018

Member

Thor automatically dasherizes options for the CLI. So let's call this :log_stdout, which makes it easier to access in options as well.

This comment has been minimized.

@doits

doits Jul 7, 2018

Author Contributor

👍 After rebasing I saw this is used for other options as well, so amended it.

railties/lib/rails/commands/server/server_command.rb Outdated
@@ -112,12 +112,18 @@ class ServerCommand < Base # :nodoc:
desc: "Specifies the PID file."
class_option "dev-caching", aliases: "-C", type: :boolean, default: nil,
desc: "Specifies whether to perform caching in development."
class_option "log-stdout", aliases: "-l", type: :boolean, default: nil,

This comment has been minimized.

@kaspth

kaspth Jul 7, 2018

Member

I don't think this has enough use to need a short hand.

railties/lib/rails/commands/server/server_command.rb Outdated
@@ -112,12 +112,18 @@ class ServerCommand < Base # :nodoc:
desc: "Specifies the PID file."
class_option "dev-caching", aliases: "-C", type: :boolean, default: nil,
desc: "Specifies whether to perform caching in development."
class_option "log-stdout", aliases: "-l", type: :boolean, default: nil,
desc: "Specifies whether to output Rails' log to std out (true = always, false = never, default = in development). Running as a Daemon always disables output."

This comment has been minimized.

@kaspth

kaspth Jul 7, 2018

Member

There's something about having a boolean mean 3 things that seems off to me. I'd think we'd only want --log-to-stdout (e.g. no --no-log-to-stdout) which just overrides whatever logging setup is there. Perhaps we should just honor it despite :daemon as well? Or throw an exception if both are enabled?

This comment has been minimized.

@doits

doits Jul 7, 2018

Author Contributor

Changed it so that it raises in case of being daemonized and logging enabled.

It is still possible to do --log-to-stdout false and "--no-log-to-stoud" afaik though, which is OK imo.

railties/lib/rails/commands/server/server_command.rb Outdated

def initialize(args = [], local_options = {}, config = {})
@original_options = local_options
super
@server = self.args.shift
@log_stdout = options[:daemon].blank? && (options[:environment] || Rails.env) == "development"
@log_stdout =

This comment has been minimized.

@kaspth

kaspth Jul 7, 2018

Member

Seems good. I wouldn't even memoize it. It's not a candidate for being called thousands of times, so the 2-3 calls in a server boot are fine to redo the work.

@doits doits force-pushed the Stellenticket:allow_disable_server_stdout_logging branch 4 times, most recently Jul 7, 2018

@doits

This comment has been minimized.

Copy link
Contributor Author

doits commented Jul 7, 2018

rebased and changed after feedback. I can squash into one commit if everything is ok for you.

railties/lib/rails/commands/server/server_command.rb Outdated
@@ -132,13 +134,14 @@ class ServerCommand < Base # :nodoc:
desc: "Specifies whether to perform caching in development."
class_option :restart, type: :boolean, default: nil, hide: true
class_option :early_hints, type: :boolean, default: nil, desc: "Enables HTTP/2 early hints."
class_option :log_to_stdout, type: :boolean, default: nil,
desc: "Specifies whether to output Rails' log to std out (true = always, false = never, default = in development)."

This comment has been minimized.

@kaspth

kaspth Jul 8, 2018

Member

Think we can tune this wording a bit:

class_option :log_to_stdout, type: :boolean, optional: true,
  desc: "Whether to log to stdout. Enabled by default in development when not daemonized."
railties/lib/rails/commands/server/server_command.rb Outdated
@@ -86,6 +86,8 @@ def create_tmp_directories
end

def log_to_stdout
raise "cannot log to stdout if daemonized" if options[:daemonize]

This comment has been minimized.

@kaspth

kaspth Jul 8, 2018

Member

Let's just skip this, once the app daemonizes it's pretty easy to see that nothing will be written to stdout. The option is misspelled too.

railties/lib/rails/commands/server/server_command.rb Outdated
def log_to_stdout?
return options[:log_to_stdout] unless options[:log_to_stdout].nil?

options[:daemon].blank? && environment == "development"

This comment has been minimized.

@kaspth

kaspth Jul 8, 2018

Member

Let's go with:

def log_to_stdout?
  options.fetch(:log_to_stdout) do
    options[:daemon].blank? && environment == "development"
  end
end

That, coupled with the optional: true, default: nil in the class_option should make it such that --log-to-stdout or --no-log-to-stdout always overrides.

@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented Jul 8, 2018

Yes, please squash your commits down to one 👍 — sorry for going so many rounds on this, it's been a while since I've last really touched the server command.

Allow to explicitly specify whether to output Rails' log to stdout
Before Rails' logger output is mirrored to std out if:

* environment is development and
* the process is not daemonized

It was not possible to change that behaviour, e.g. to disable log output
in that case or enable it in other cases.

Now you can explicitly disable or enable output with the new command
line switch `--log-to-stdout`, regardless of any other circumstances.

```
// enable output in production
rails server -e production --log-to-stdout

// disable output in development
rails server -e development --no-log-to-stdout
```

Enabling output when daemonized still makes no sense (since tty is
detached), but this is ignored for now.

If the command line flag is not specified, old behaviour still
applies, so this change is completely backward compatible.

@doits doits force-pushed the Stellenticket:allow_disable_server_stdout_logging branch to 889a7cc Jul 8, 2018

@doits

This comment has been minimized.

Copy link
Contributor Author

doits commented Jul 8, 2018

@kaspth thanks for review, I amended everything. Hopefully tests will pass 🤞

And np for going some rounds: We both want good code in the end, sometimes it takes some rounds. And it was my first time to touch the server command, so I didn't expect it to be bulletproof on first try anyway.

@kaspth kaspth merged commit 50a9ca6 into rails:master Jul 8, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented Jul 8, 2018

That's exactly the right attitude! Thanks, @doits 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.