Skip to content

Commit

Permalink
Revert "make new rails apps log to STDOUT"
Browse files Browse the repository at this point in the history 
This reverts commit b7d9d6e.

Per discussion with @jeremy and @rubys on Campfire.
  • Loading branch information
@steveklabnik
steveklabnik committed Mar 15, 2013
1 parent 29030d3 commit 1a90550
Show file tree
Hide file tree
Showing 6 changed files with 5 additions and 47 deletions.
8 changes: 0 additions & 8 deletions activesupport/lib/active_support/tagged_logging.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,6 @@ def tags_text
end
end

def self.create(f, formatter, level)
logger = ActiveSupport::Logger.new f
logger.formatter = formatter
logger = new(logger)
logger.level = ActiveSupport::Logger.const_get(level.to_s.upcase)
logger
end

def self.new(logger)
# Ensure we set a default formatter so we aren't extending nil!
logger.formatter ||= ActiveSupport::Logger::SimpleFormatter.new
Expand Down
11 changes: 0 additions & 11 deletions activesupport/test/tagged_logging_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,6 @@ def flush(*)
assert logger.formatter.respond_to?(:tagged)
end

test 'creates a tagged logger with the appropriate level and formatter' do
stringio = StringIO.new
logger = ActiveSupport::TaggedLogging.create(stringio, ActiveSupport::Logger::SimpleFormatter.new, :debug)
logger.debug("foo")

assert_not_nil logger.formatter
assert logger.formatter.respond_to?(:tagged)
assert_equal 0, logger.level
assert stringio.string.include?("foo")
end

test "tagged once" do
@logger.tagged("BCX") { @logger.info "Funky time" }
assert_equal "[BCX] Funky time\n", @output.string
Expand Down
4 changes: 0 additions & 4 deletions railties/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
## Rails 4.0.0 (unreleased) ##

* New rails apps log to STDOUT by default

*Terence Lee*

* Add support for generate scaffold password:digest

* adds password_digest attribute to the migration
Expand Down
6 changes: 5 additions & 1 deletion railties/lib/rails/application/bootstrap.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,11 @@ module Bootstrap
f.binmode
f.sync = config.autoflush_log # if true make sure every write flushes

logger = ActiveSupport::TaggedLogging.create(f, config.log_formatter, config.log_level)
logger = ActiveSupport::Logger.new f
logger.formatter = config.log_formatter
logger = ActiveSupport::TaggedLogging.new(logger)
logger.level = ActiveSupport::Logger.const_get(config.log_level.to_s.upcase)
logger
rescue StandardError
logger = ActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(STDERR))
logger.level = ActiveSupport::Logger::WARN
Expand Down
2 changes: 0 additions & 2 deletions railties/lib/rails/generators/rails/app/templates/config/application.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,5 @@ class Application < Rails::Application
# Disable the asset pipeline.
config.assets.enabled = false
<% end -%>

config.logger = ActiveSupport::TaggedLogging.create(STDOUT, config.log_formatter, config.log_level)
end
end
21 changes: 0 additions & 21 deletions railties/test/application/configuration_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -670,26 +670,5 @@ def index
end
end
end

test "set logger to STDOUT by default" do
stdout = capture(:stdout) do
app = build_app

controller :omg, <<-RUBY
class OmgController < ApplicationController
def index
Rails.logger.info "HI MOM"
render text: "omg"
end
end
RUBY

require "#{app_path}/config/environment"

get "/omg/index"
end

assert stdout.include?("HI MOM"), "STDOUT does not include 'HI MOM', #{stdout}"
end
end
end

7 comments on commit 1a90550

@pixeltrix
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@steveklabnik I have to agree with @tomstuart in his tweet:

“Per discussion on Campfire” is not a great commit message for an open source product.

Other people may have ideas/input on the reason it was reverted and this just shuts them out - even a sentence would help.

@rubys
Copy link
Contributor

@rubys rubys commented on 1a90550 Mar 16, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it is worth, I feel the same way about the original commit. While I agree philosophically with the intent of the change, there are consequences that need to be thought through and communicated. My understanding of the situation, attempting not to advocate for any particular position:

Before the original change, development logging was done to both stdout (rails server's window) and log/development.log. After this change, the logging only goes to stdout. Having logging go to two places is a bit unusual, but it is the way that people have grown accustomed to it working. At a minimum, the original commit needs to be accompanied by considerably more than a changelog entry. Also of note, many don't use rails server during development. Some analysis of how this change affects other servers commonly used during development is in order.

Before the original change, test logging was done to log/test.log. After this change, logging went to stdout. This makes it significantly harder to spot actual errors in test runs. Seeing this, nobody in campfire at the time advocated keeping this behavior when RAILS_ENV=test.

Before the original change, production logging was done to log/production.log. After this change, logging goes to wherever the server choses to send stdout. An informal poll of the people in the room at the time indicated that there wasn't a single common obvious answer to where logging should be sent.

My assessment is that there is indeed a case to be made that stdout is a better default for production, but it isn't a slam dunk. The change for development isn't quite so easy to defend. The original change is a clear loss for test.

@pixeltrix
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rubys thanks for explaining and yes, I agree the original commit needed more reasoning. We do try to get people to write informative commit messages but when you've badgered them about code style, tests, CHANGELOG entry, etc. it's all too tempting to hit that big green merge button instead of asking for one more thing and running the risk of them never wanting to contribute ever again. 😄

@rubys
Copy link
Contributor

@rubys rubys commented on 1a90550 Mar 16, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trust me, I'm not a Rails committer, and I've been on the receiving side of that badgering. :-)

@steveklabnik
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pixeltrix honestly, you're right, and frankly, it's partly due to my frustration with said discussion on Campfire.

Also, since the feature was only on master for less than an hour, I wasn't thinking much about writing a big message explaining the details: they'll get talked about in the new pull request.

@tomstuart
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @rubys and @steveklabnik, for explaining the situation. In hindsight I apologise for snarking on Twitter instead of asking a constructive question on GitHub.

@pixeltrix
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@steveklabnik ❤️ I know the feeling - it took ages to sort out what to do about eager loading for Rails 4 which was tough going at times.

Please sign in to comment.