lib: support different loggers

[lundibundi]

Enables users to specify logger they want to use with our library. Also adds simple proxy to console.

As discussed in #102 I extract logger fuctionality to a different PR.

Also I'd like to add a few tests for console logger, but I don't know how to better implement it, as I don't think that either redirecting console output during the test or just checking stdout is a good solution, can someone suggest a solution?

[aqrln]

I find these names misleading, something like hasDebug etc would be better, IMO. I'd expect isDebug, isVerbose and alike to report the current logging level, which is not the case with these checks because these methods will be present in virtually every logger regardless of the logging level.

[lundibundi]

That was the actual reason I named them as such, I was mislead by issue in winston that actually suggested to check level this way.
I haven't found a good way to get debug level out of winston beside .level which gives a string, that's disappointing. Lucky enough bunyan returns a number which is a lot better. I guess we will allow user to provide a callback to check current log level and I'll write proxies for winston and bunyan to simplify their usage.

[aqrln]

It isn't possible to replace the logger before this statement will be executed, so it will always use the default logger.

[lundibundi]

@aqrln yeah, missed that. Should we set console as default logger or always log to console here?

[nechaido]

I'd like the logger to have method's like: warn, error, log, debug. Logger should decide for itself to log something or not, that way we'll have no need to write this kind of statements:
if (logger.isDebug()) logPacket(packet);.

[lundibundi]

@nechaido It already has those methods and decides by itself whether he should log or drop the message. The need in methods like isDebug() and such arises from the cost that preparation for logging has, for example in this PR I have to check jstp message type and copy if necessary while removing some fields to avoid leak of private data to logs (for handshake packet). Checking isDebug() is much cheaper than that. And also I don't think that implementing callback to check current logging level is that hard.

[nechaido]

@lundibundi this could be achieved by passing a function to logger which will do the preparation and then return the string to be logged.

[belochub]

@lundibundi, can't you make a wrapper (adapter) in our library which will check the logger level before the preparation, to avoid boilerplate checking like the one @nechaido mentioned?

[lundibundi]

@belochub I can do that but I see no reason for that complexity, whether we write
if (logger.isDebug()) logPacket(packet); or something like
logger.log('debug', logPacket.bind(null, packet))
More so the latter will probably be less efficient and takes more work to implement and support.

[nechaido]

@lundibundi and what about:

logger.debug(() => preparePacket(packet));

Looks ok for me and doesn't do any unnecessary preparation in release mod, only call to empty function.

Forgot about creating a function to be passed.

[belochub]

@lundibundi, what you said in #224 (comment) wasn't at all what I proposed to do.

[belochub]

@lundibundi, what I proposed will make

if (logger.isDebug()) logPacket(packet);

look like

logger.get().debug(packet);

or even

logger.debug(packet);

[lundibundi]

@belochub oh, sorry, I thought that you extended @nechaido idea. So you want a logger specifically to log jstp messages, that will check log level and if object is passed treat it as jstp message and log it same way logPacket does and if string is passed log it as is, is my understanding correct?

[belochub]

@lundibundi, yeah, something like that.

[lundibundi]

@belochub but its only usage are those 3 calls in connection.js and I'm not sure if it is plausible to implement it solely for those usages? Also I can move isDebug check into logPacket so there'll be only one check for isDebug() (in code), they only reason I left it outside was to avoid unnecessary call to logPacket.

[aqrln]

@lundibundi ping

[lundibundi]

@aqrln I'm waiting for the decision of our team, whether this is a good solution or not. We even got the slack vote which has apparently been flooded by github bot.

[belochub]

Closed in favor of #252 (see #250 for more information).