Use logging library instead of logging to console.error

[jordanbtucker]

It's generally bad practice for a module to log to the console by default, especially if it is logging information as errors.

Consider using logging modules like debug and winston and make {silent: true} the default.

Errors don't need to be logged, instead they should be thrown or sent as a callback parameter. It's up to the programmer who is using your library to handle and log errors. If your library encounters an error that it can gracefully handle and does not 'break' the application, consider logging it as a warning. (#316)

Allow users to configure the logging options by exposing a log property on the options object. This could be passed to winston.configure for example. This will give users the option to log to the console, or to a file, or to syslog, and even choose what logging levels they want (e.g. info, warn, error, debug, etc.).

See also #292, #320.

[neumino]

This is already available. You can just listen to the log event.
https://github.com/neumino/rethinkdbdash#connection-pool

[jordanbtucker]

You completely ignored most of my comments.

[neumino]

See inline:

It's generally bad practice for a module to log to the console by default, especially if it is logging information as errors.

I strongly disagree with that. The default should be a good one that covers the most common use case.

Consider using logging modules like debug and winston and make {silent: true} the default.

Node has a micro libraries ecosystem. I don't see a good reason to make rethinkdbdash depends on other libraries that it doesn't need.

Errors don't need to be logged, instead they should be thrown or sent as a callback parameter. It's up to the programmer who is using your library to handle and log errors. If your library encounters an error that it can gracefully handle and does not 'break' the application, consider logging it as a warning. (#316)

You can do that. There's no reason to change the default for that.

Allow users to configure the logging options by exposing a log property on the options object. This could be passed to winston.configure for example. This will give users the option to log to the console, or to a file, or to syslog, and even choose what logging levels they want (e.g. info, warn, error, debug, etc.).

This is already available.

I'm not sure to understand what's your point? That the default is bad?

[jordanbtucker]

The common use case should for a library to be silent regarding the console. When you build an express app, express doesn't automatically log to the console that it's listening for requests (especially to STDERR). The programmer does that herself in the listen callback if she wants to. In fact, there isn't even an option for express to do that automatically.

The messages that rethinkdbdash outputs to the console are really debugging messages. They're only suitable for when you are debugging your app or maybe logging to a file or syslog. (By the way, express uses the debug module, too.)

If rethinkdbdash was a CLI application, then it would be completely fine to write to the console, and that's what users would expect it to do. Writing to the console belongs to the domain of applications, not libraries. It's very strange for a library to start writing error messages to the console. That should only happen if the DEBUG environment variable is set. I can't even think of any other library that writes to the console (unless it has a CLI).

A log event is a rare API. You should really be emitting events specific to each action your library is taking rather than just a generic log event. Currently the log event just outputs text that programmers must parse to make sense of, and not all of the information is contained in one log message. For example, you send an error message in one event, then an internal error message in the next, then a stack trace in the next. There's no really way to tie these messages together if you're just reading from an event stream, at least not without maintain state based on the previous emitted event.

So, yes. The default is bad, and logging to the console really shouldn't be an option, and certainly writing to STDERR when there wasn't really an error is bad too, and emitting a log event is strange.

[neumino]

Can you propose an API first? I feel like we won't be able to make much progress without that.

[0xgeert]

@neumino : What I'm struggling with is the coarseness of doing something like this.r.getPoolMaster().on('log', log.error).

Currently, afaik, there's really no way to discern between logging messages (e.g.: entering slow grow mode) or actual errors. Part of the beauty of using logging libraries like Winston is that you can redirect certain log messages based on their loglevel (debug, info, warn, error, etc.)

Encoding logs including a loglevel would go a long way in being able to at least work on top of that. E.g.: something like

  this.r.getPoolMaster().on('log', (msg){
    //msg is of format: 
    //{
    //  level: info,
    //  msg: entering slow grow mode"
    //}
    //
    //or: 
    //
    //{
    //  level: error, 
    //  msg:  Fail to create a new connection for the connection pool,
    //  error: <the actual error object passed along verbatim>
    //}
  });

Thoughts?

[neumino]

That's fine for me, but the hard part is figuring out what's INFO, WARN and ERROR.

[jordanbtucker]

• INFO: Informational messages that explain what is happening.
• WARN: Warnings where something went wrong but the program was able to recover. These might relate to performance issues, configuration issues, or deprecate API calls.
• ERROR: An unrecoverable situation. These might be network or file system errors, fatal configuration issues, or any other situation where the program can't recover.

Follow those guidelines and you should be good.

[neumino]

Please do a pass on all error messages such that we can discuss a precise API.