Add logging option for btcrpcclient as subsystem RPCC.

[runeaune]

This isn't really a subsystem, rather a dependency, but being able to
turn the logging on from the command line can greatly simplify debugging
of connectivity issues.

[jrick]

Nice find, without this all logs in the btcrpcclient package get eaten and there's no way of turning it back on.

ok

[davecgh]

No real objections, although I wonder if the chain subsystem should tell the btcrpcclient to use its logger instead of having a separate subsystem? Allowing it to be specified separately doesn't hurt currently, but ultimately wallet will be able to work with both the current websockets RPC model as well as SPV in which case there will be no btcrpcclient dep.

EDIT: We can of course address that at the time SPV support gets merged as well.

[runeaune]

I'll make it inherit chain's logger if you think that's better. The way I understand you, it will be set to whatever chain logging is set to, and not have it's own label. Is that correct?

[davecgh]

Yeah it would all show up as "CHNS".

    case "CHNS":
        chainLog = logger
        chain.UseLogger(logger)
+       btcrpcclient.UseLogger(logger)

For what it's worth, I'm not really against it being like you have it now as a separate subsystem either, but I brought it up as a point of discussion since it might make sense to avoid a separate subsystem given the SPV plans.

What do you think @jrick?

[jrick]

Everything is set to the info level by default. If btcrpcclient shared the same logger as the chain package then there would be a lot more logging going on by default without any change in config. I'm not totally opposed to that but it depends how verbose it actually is.

[davecgh]

The logging is quite light for info. I just took a look and there is only a single Infof message when the connection is established. The rest are Warnf when receiving malformed messages, which obviously should never happen on TCP unless the client/server don't agree on the format, Debugf for notification registrations, and Tracef for all commands and notifications. The Tracef messages could obviously get fairly noisy.

[jrick]

Seems that sharing the logger with chain would be ok then, except perhaps we want to remove the duplicate log when it connects ("Established websocket RPC connection to btcd").

[runeaune]

Wouldn't that be the same if it's added with a separate subsystem label (assuming that the new label also defaults to info level)?

As for noise level, it seems pretty ok at info and above. Info level messages are logged for connect/disconnect only. As far as I can tell the biggest risk of log spam comes from warnings of invalid messages (eg. https://github.com/btcsuite/btcrpcclient/blob/master/notify.go#L199), although I don't know btcwallet well enough yet to judge how frequent those can be.

[runeaune]

Sorry, didn't see Dave's reply before replying myself. I'm going to change
the logging to use chain's logger and remove the duplicate log for
connection to btcd.

On Thu, 19 Nov 2015 at 16:40 Josh Rickmar notifications@github.com wrote:

Seems that sharing the logger with chain would be ok then, except perhaps
we want to remove the duplicate log when it connects ("Established
websocket RPC connection to btcd").

—
Reply to this email directly or view it on GitHub
#328 (comment).

[davecgh]

OK

[jrick]

ok.

Can you squash this into a single commit?

[runeaune]

Sure.