Make automatic log flushing configurable #4998

Merged
merged 3 commits into from Feb 22, 2012

Projects

None yet

5 participants

@felixbuenemann
Contributor

As discussed in GH #4277 this allows to configure, wether the application log should always be flushed (for debugging) or only as the OS sees fit (for performance).

Currently it defauts to no autoflushing in production and enables it everywhere else.

This needs some minor editing to apply against HEAD because of commit 55cc16f.

@felixbuenemann
Contributor

I'm not sure as to what's the best default for production enviroment. Before it was disabled, but 55cc16f re-enables it for production because of popular demand.

@NZKoz
Member
NZKoz commented Feb 10, 2012

Some quick thoughts, as I was one of the guys who spent hours looking for a bug that wasn't there due to missing log messages.

It should definitely default to flush, 99% of people won't notice the difference until they're faced with an error which isn't being logged. For people whose performance is adversely affected by sequentially appending to a file, they can figure out where to disable it. Given how niche that is, I don't think it needs to be advertised in production.rb.

As far as naming, the option should probably be called something like config.buffer_log_messages, or ideally config.logger.buffer_messages = true, but given that we let people assign any old thing to config.logger that's probably not possible.

@felixbuenemann
Contributor

I'd be open to go with flushing enabled as the default, however I think it's a good idea to advertise it in the production environment template, given that their isn't really much documentation on supported config options and digging around the source to find hidden config options is not so great.

@Juanmcuello
Contributor

I agree with @NZKoz, it should flush by default.

About advertising it in production.rb, I don't think it's necessary, but definitely should be documented somewhere, at least in Rails Guides. Not sure where else.

@felixbuenemann
Contributor

Rebased against upstream and changed default to flushing enabled.

@tenderlove tenderlove merged commit abf3f67 into rails:master Feb 22, 2012
@atambo

Your commit message says production template but this is the development template...I think this makes more sense in the production template.

Contributor

@atambo I've read the code a couple of times but didn't spot that. I'll supply a fix as PR.

Contributor

@atambo done, submitted PR gh-5140

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment