Logging interface

[Goblinlordx]

Currently, this library is logging by default which doesn't seem very nice. There are some places where it seems to arbitrarily output logs. This prevents me from being able to do much with them (hide them, output them with a bit of formatting etc). Would it be possible to have the config receive a something like a simple Logger interface or something.

I am not sure where all the places that "log" automatically are but if these are the only spots then it could be something simple like:

type Logger interface {
  Printf(format string, v ...interface{})
}

Then you would pass something like log.New(os.Stderr, "", log.LstdFlags) (same thing as default log Logger) from default config but the user (of the library) could change out the logging as necessary. I also see flog in this same file... are there multiple logging libraries used throughout this project???

edit
Ah yes, flog or github.com/anacrolix/log has essentially the same issue in that same file and outputs things like:
2018-08-25 20:12:18 portfwd.go:31: discovered 0 upnp devices

It looks like flog has a bit of a different interface =/. Is there any way the logging interface can be simplified and generalized for all logging? The way this is logging doesn't really allow me to make the logs fit the format I would want nor does it let me prevent or redirect all the logging (without interacting outside with stdout).

[anacrolix]

There is some inconsistent logging going on. I haven't settled on a solution I'm happy with. There is use of "log", and "github.com/anacrolix/log", the latter through which I intend to allow python logging-style levels, handlers, formatters etc.

If you have any suggestions, or PRs to clean these up, feel free to contribute.

[Goblinlordx]

Sounds good~ I will see what I can do~

[yonson2]

I also ended up wanting to hide the logs outputted by the program, instead of providing a logger interface and unifying both logging libraries what I endep up doing (good enough for my use-case) was to simply ensure that those log calls are only called if the Debug property of the client is set to true.

You can see an example of how it looks like here, I haven't submitted a pull request as I don't know if this workaround is something that seems ok to you, I'll create it if @anacrolix is ok with it.

[anacrolix]

It's always difficult, one man's log is another man's noise. Are there specific log messages that are annoying you? Generally I try to log anything that may indicate degraded performance, or should not occur under favourable conditions. For example the UPnP debug message is difficult, as if UPnP discovery fails, performance may be greatly affected for some users.

[anacrolix]

Also note that making the logging system more complicated isn't useful to casual consumers of the library. They have to understand the system to know what to log for their purposes.

[anacrolix]

If you want to make a PR with your desired changes, I'm happy to accept many of them.

[yonson2]

It's always difficult, one man's log is another man's noise. Are there specific log messages that are annoying you? Generally I try to log anything that may indicate degraded performance, or should not occur under favourable conditions. For example the UPnP debug message is difficult, as if UPnP discovery fails, performance may be greatly affected for some users.

Well, my specific scenario was that of a cli utility that uses your library so I just wanted a way to hide all but fatal logs when running in a production environment (i.e. ready to be distributed binary) so the final user doesn't need to see them, there are a couple of log calls from elgatito/upnp that currently cannot be hidden but if the author seems like it would be a good idea I plan to send a PR there too.

If debug is set to true then all logs would be displayed as usual as I do think that having the option to see them is useful.

[yonson2]

@anacrolix were you able to check the PR #308?

[anacrolix]

@yonson2 Yes, it inspired me to standardize the logging. See the comment in #308. All log messages now route through the Client logger, which isn't yet exposed.

[anacrolix]

The Client Logger is now exposed, and also supported by the DHT. https://github.com/anacrolix/log has settled into something I'm a bit happier with.

[bsergean]

For posterity, if someone wants to override the log you can look at a use case here -> #551