logger: do not log stack w/ info(), debug() and spam()

[braydonf]

This reverts previous behavior to not include the stack for info(), debug() and spam() level logs and will continue to include the stack in all cases for error() and warning() logs as was previously resolved.

cc: @BluSyn

[BluSyn]

This works. I like the addition of tests.

👍

[braydonf]

Great!

[pinheadmz]

I'm unclear on the motivation - spam is the most verbose log level, I turn it on for testing & development, why exclude the stack traces?

[braydonf]

I'm not aware of where spam is used for errors currently, see the level numbers defined at https://github.com/bcoin-org/blgr/blob/master/lib/logger.js#L796-L803

[pinheadmz]

Right, exactly: log-level=spam reports ALL messages, it includes errors, warnings, infos, and debugs. When I set to spam I expect to see every single log message in the codebase

[braydonf]

I think the idea is that only logger.warning and logger.error are used for unexpected errors, where a stack trace is useful, and other cases the error should be expected and thus the stack isn't needed.

[pinheadmz]

Hm. One of the reasons this change is important is because of bcoin-org/bcoin#762, I assume that user's log was set to the default debug which wouldn't report stack traces after this is merged, right?

[braydonf]

No, the stack would be displayed. I think you have the logger level setting confused with the logger level function. In this PR, logger.error always displays the stack trace, as does logger.warning.

[nodech]

Warnings are used for logical warnings, where we know what happened and we are just announcing it (that's how I have seen it used at least). I am not sure if that needs stack traces.

errors definitely need stack traces.

[braydonf]

The use of logger.warning with an instance of Error is very infrequent. There is one use of logger.warning with an Error instance in bcoin, see https://github.com/bcoin-org/bcoin/blob/master/lib/net/pool.js#L2172-L2181 I don't think it's an issue, and would be helpful.

[nodech]

I see that would definitely obscure the source. Would be nice to expand on those log messages as well, so you can get more context but that's part of the error messages not this PR.

So we have two usage, as errors that are not essential but could be important and user warnings that are just informational and not an error.