Open aleclarson opened 6 years ago
I don't see a strong need for options.logLevel
. Virtually all logging implementations have mechanisms for controlling log level behavior upstream. Having any default value whatsoever for options.logLevel
complicates things when you are providing a logging instance/function that you would expect to control that behavior.
Aside from that I think this is pretty fantastic.
@marshall007 Thanks for the feedback! I would argue that options.logLevel
is useful when options.log
is undefined or a function, providing easy filtering with only 13 lines of code. I think the best compromise is to make debug
the default. Then anyone using a logging library won't be disturbed with setting options.logLevel
for the rethinkdb
constructor.
Small change: When options.log
is a function, its arguments are now (level, message)
instead of (message, level)
, but any log
event listeners are still passed (message, level)
A few thoughts
Overall the first 2 points are the main reason why I think it's a bit wonky to have warn/info for rethinkdbdash.
I think it's fine to move the draining messages to debug, but the other messages should all be at the same level.
Good points, @neumino!
Here's an updated list of log levels per message:
server_status
changefeed returned an errorserver_status
The verbose
log level has been removed.
@neumino Are there any blockers on getting this merged? Should I update the docs too?
This provides improved logging, as requested by #325, #351, and https://github.com/neumino/rethinkdbdash/issues/334#issuecomment-293423825.
Overview
options.logLevel
is a string used to silence unwanted logsoptions.log
can be an object with{debug, verbose, info, warn, error}
methodsoptions.log
is a function, its arguments are(level, message)
options.log
is undefined, theconsole
methods are used by defaultoptions.log
is falsy (oroptions.silent
is truthy), log messages are silencedError
objectLog levels
The valid values for
options.logLevel
are based off winston, minus thesilly
level and custom levels.Log levels are assigned an integer representing their importance. This integer is used to silence logs whose log level has an integer of higher value. As an example, if
options.logLevel
equalsverbose
, onlydebug
logs are silenced. Likewise, ifoptions.logLevel
equalserror
, onlyerror
logs are not silenced.The default value of
options.logLevel
isinfo
, but you could argue thatdebug
is more appropriate so the end user can provide more context when something goes wrong.I made my best guesses on which log level should be used for each log-worthy event. Let me know if the classification of any events should be changed.
server_status
changefeed returned an errorserver_status
The
debug
events are always followed by anerror
event, which emits the stack trace.'error' event
I added an
error
event to the PoolMaster. This allows the end user to do whatever they like with theError
object (as requested in #325). My use case for this is overridingError.prepareStackTrace
to preserve the callsite objects, listening to theerror
event, and logging the error as JSON for easier consumption by logging dashboards.If you would like, I can separate this change into its own PR, but I think it's useful and harmless to anyone who doesn't need it.