Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Passing through unadulterated log levels to third party loggers breaks some third party loggers #14114

Closed
noahhl opened this Issue · 3 comments

5 participants

@noahhl

Since e0a521c, config.log_level is passed through to Rails.logger.level without converting to a numeric value unless it's using the stdlib Logger.

This fixed some third party loggers, but broke others that do quack like the stdlib Logger, most specifically https://github.com/sparklemotion/sysloglogger, causing an error like:

comparison of Symbol with 1 failed
sysloglogger-dacf8ef846d7/lib/syslog/logger.rb:189:in `<='
sysloglogger-dacf8ef846d7/lib/syslog/logger.rb:189:in `add'
sysloglogger-dacf8ef846d7/lib/syslog/logger.rb:102:in `info'

sysloglogger is easily patched ala noahhl/sysloglogger@b29bf08, so perhaps the fix should be there (though I'd argue it's trying to be a good citizen and act like the stdlib logger), but there's definitely a change in behavior here that impacts some loggers.

cc/ @dhh

@dhh dhh added this to the 4.1.0 milestone
@andrewgarner andrewgarner referenced this issue from a commit in moneyadviceservice/frontend
@andrewgarner andrewgarner Configure numeric log level in production cdb4414
@senny senny was assigned by dhh
@noahhl

Previous patch for sysloglogger doesn't quite show the extent of the damage -- also needed noahhl/sysloglogger@e83fb75. I can only imagine what else this is breaking.

I'm obviously a fan of restoring the prior behavior in Rails.

@jeremy
Owner

/cc @senny, @rafaelfranca re #11665

Think this whole line of changes should be reverted.

config.log_level is a config-time setting, not a runtime knob to change the running app's logger level.

Furthermore, we expect loggers to quack like stdlib logger. If log4r needs different #level= assignment, using a Logger-quacking wrapper is the way to do it.

@guilleiguaran guilleiguaran referenced this issue from a commit
@guilleiguaran guilleiguaran Revert "Only lookup `config.log_level` for stdlib `::Logger`. Closes #…
…11665."

This reverts commit e0a521c.

Conflicts:
	railties/CHANGELOG.md

We expect loggers to quack like stdlib logger. If log4r needs different
level= assignment, using a Logger-quacking wrapper is the way to do it.

Fixes #14114.
d094d7e
@guilleiguaran guilleiguaran closed this issue from a commit
@guilleiguaran guilleiguaran Revert "Only lookup `config.log_level` for stdlib `::Logger`. Closes #…
…11665."

This reverts commit e0a521c.

Conflicts:
	railties/CHANGELOG.md

We expect loggers to quack like stdlib logger. If log4r needs different
level= assignment, using a Logger-quacking wrapper is the way to do it.

Fixes #14114.
b1867c8
@guilleiguaran guilleiguaran referenced this issue from a commit
@guilleiguaran guilleiguaran Revert "Only lookup `config.log_level` for stdlib `::Logger`. Closes #…
…11665."

This reverts commit e2b2539.

Conflicts:
	railties/CHANGELOG.md

We expect loggers to quack like stdlib logger. If log4r needs different
level= assignment, using a Logger-quacking wrapper is the way to do it.

Fixes #14114.
20f475c
@guilleiguaran

@jeremy @senny @noahhl I've reverted it to the previous behavior

@ttosch ttosch referenced this issue from a commit
@guilleiguaran guilleiguaran Revert "Only lookup `config.log_level` for stdlib `::Logger`. Closes #…
…11665."

This reverts commit e0a521c.

Conflicts:
	railties/CHANGELOG.md

We expect loggers to quack like stdlib logger. If log4r needs different
level= assignment, using a Logger-quacking wrapper is the way to do it.

Fixes #14114.
a1317e4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.