centralize logging and make it pluggable

[devdazed]

This request does two things, it centralizes logging to the log.go to remove some code duplication with respect to logging. It also make the log an interface so that people can easily drop their own logging structures in.

The structure simply needs to implement the following methods:

    Debugf(format string, v ...interface{})
    Infof(format string, v ...interface{})
    Warnf(format string, v ...interface{})
    Errorf(format string, v ...interface{})

[mreiferson]

Thanks, it's a reasonable suggestion.

I explicitly didn't go this route when I refactored the API (edit: #30) because I just wanted it to expose a hook to set a standard library log.Logger (which itself is instantiated with an io.Writer, which can be used to "hook into" the stream of output).

So I'm 👎 on this.

I'm curious as to how you want to hook into this on your application side? For example, the existing API provides a level hook, and log.Logger provides the output hook, what other hook do you want? Maybe that would be a better foundation for discussion.

[devdazed]

well, with your method you force the use of the internal log.Logger we use go-logging which has a different api. Your SetLogger method requires this. I think it's a bit much to ask to hijack the writers of both and somehow merge them just to be able to use our logger. This is precisely the use case for interfaces, all the logger needs to do is implement the proper methods and it can be dropped in.

In our case, this was a simple wrapper looking something like:

// A simple wrapper around our logger
type SimpleReachLogger struct {
    *logging.Logger
}

func (l *SimpleReachLogger) Debugf(format string, v ...interface{}){
    l.Debug(format, v)
}

func (l *SimpleReachLogger) Infof(format string, v ...interface{}){
    l.Info(format, v)
}

func (l *SimpleReachLogger) Warnf(format string, v ...interface{}){
    l.Warning(format, v)
}

func (l *SimpleReachLogger) Errorf(format string, v ...interface{}){
    l.Error(format, v)
}

[devdazed]

Also, there are a few loggers out there that allow you to, with a single call, log to both a file and a syslog/rsyslog or aggregation. Which go-logging currently does. Not sure how we would even be able to do that here.

[mreiferson]

Also, there are a few loggers out there that allow you to, with a single call, log to both a file and a syslog/rsyslog or aggregation. Which go-logging currently does. Not sure how we would even be able to do that here.

Easiest thing to do would be to use http://golang.org/pkg/io/#MultiWriter as the io.Writer backing your log.Logger

[devdazed]

The point is that other people may want to have colorized logs, or may want to log JSON objects for later data analysis. This doesn't allow for that. Having an interface lets the user define how the logging is done and how it looks. In my experience no two people have the same idea for how application logs need to look, this gives them the option to have it their way.

[mreiferson]

@jehiah care to weigh in

[devdazed]

Maybe I went overboard when requiring all log levels in the interface, maybe something a little more lightweight could also do the job, similar to how mgo does it: https://github.com/go-mgo/mgo/blob/v2/log.go

[mreiferson]

I had been poking around as well and had also seen that approach in mgo. The problem is it doesn't pass the log level in that interface method (because it's matching the signature of the stdlib log.Logger.Output(..) method).

This still might be a good happy medium because you would be able to still pass a log.Logger like the current implementation, but could also implement your own type that can pivot on the first few bytes of the log message (i.e. [INF], [WRN], etc.) like you want to be able to do.

I kinda like this approach, actually. Want to try and whip that up?

[devdazed]

A user's Printf method might look something like this:

func (l *MyLogger) Printf(format string, v ...interface{}){
    switch v[0]{
    case nsq.LogLevelDebugPrefix:
        l.Debug(format[5:], v[1:]...)
    case nsq.LogLevelInfoPrefix:
        l.Info(format[5:], v[1:]...)
    case nsq.LogLevelWarningPrefix:
        l.Warning(format[5:], v[1:]...)
    case nsq.LogLevelErrorPrefix:
        l.Error(format[5:], v[1:]...)
    default:
        l.Info(format[5:], v...)
    }
}

[mreiferson]

Actually, I was suggesting the same interface as mgo:

type log_Logger interface {
    Output(calldepth int, s string) error
}

This way our changes here are completely backwards compatible (existing users can still just pass a stdlib *log.Logger). You can then implement your use case by:

type MyLogger struct {
    l *logging.Logger
}

func (m *MyLogger) Output(calldepth int, s string) {
    switch s[1:4] {
    case "DBG":
        m.l.Debug(s[5:])
    case "INF":
        m.l.Info(s[5:])
    case "WRN":
        m.l.Warning(s[5:])
    case "ERR":
        m.l.Error(s[5:])
    }
}

[mreiferson]

Whoops, your changes are also backwards compatible as *log.Logger implements Printf(...), but of the two options I think I prefer using Output(...) rather than stuffing var args.

[mreiferson]

They're both hacks, really, but they get the job done.

My preference to choose Output(...) over Printf(...) is that it would continue to promote the clever method chosen by mgo, and perhaps over time other libraries would also implement this same approach.

[devdazed]

the problem with using Output is that you never call it. You are expecting the struct to quack with Printf but you only specify that Output is declared. So if a person creates a logger and defines Output, but never defines Printf there will be a panic, assuming it allows you to even build.

[mreiferson]

I like it, thanks for working through this @devdazed

Do you mind squashing those commits down to 1?