umputun / reproxy

Simple edge server / reverse proxy
http://reproxy.io
MIT License
1.25k stars 92 forks source link

Support custom logger for plugins #115

Open ShoshinNikita opened 3 years ago

ShoshinNikita commented 3 years ago

I noticed that github.com/umputun/reproxy/lib uses the global logger provided by github.com/go-pkgz/lgr. It can be inconvenient because log records can have different formats (especially when using github.com/sirupsen/logrus or github.com/uber-go/zap).

I think it makes sense to support custom logger. Logger declaration can look like this (these methods cover all levels of github.com/go-pkgz/lgr):

type Logger interface {
    Tracef(format string, args ...interface{})
    Debugf(format string, args ...interface{})
    Infof(format string, args ...interface{})
    Warnf(format string, args ...interface{})
    Errorf(format string, args ...interface{})
    Panicf(format string, args ...interface{})
    Fatalf(format string, args ...interface{})
}

If custom logger (Plugin.Log) is not set, github.com/go-pkgz/lgr will be used (via a wrapper).

I can work on this, but first, I would like to hear your opinion.

umputun commented 3 years ago

Good point. Enforcing the lib package to use a particular logger wasn't a good idea, I have planned to make it right but completely forgot. Maybe all we need here is a single method logger interface like I do in go-pkgz/auth (and injected like this) ?

this way we still keep the common logging format and the common logging presentation, but the user can inject any custom logger with a single Logf(format string, args ...interface{}) method. If one needs to adopt the current Printf calls to the "leveled" calls, it can be easily done based on the prefix, and this way logrus could be integrated without much of a headache.

ShoshinNikita commented 3 years ago

I don't like the idea of an interface with a single method. Every user who wants leveled logging has to write the same checks based on some magic constants (why DEBUG and not DBG or [DBG], for example).

Another option is to add 2 interfaces:

Plugin will use AdvancedLogger. But users will able to pass just Logger. Something like this:

type Plugin struct {
    // ...
    log AdvancedLogger
}

func (p *Plugin) SetLogger(log Logger) {
    if advanced, ok := log.(AdvancedLogger); ok {
        p.log = advanced
        return
    }
    p.log = advancedLogger{log}
}

// advancedLogger implements AdvancedLogger interface for Logger
//
// TODO: not sure about the naming
type advancedLogger struct {
    log Logger
}

func (l advancedLogger) Tracef(format string, args ...interface{}) { l.log.Logf("TRACE "+format, args...) }
// And so on...

However I think there are only 2 types of users:

So, most users will use SetLogger to pass AdvancedLogger

umputun commented 3 years ago

I would rather keep the logging interface closer to stdlib and not to particular leveled loggers. I think the adopter from Lelveled interface to Logf based interface can be provided as a part of lib/logging package, in a similar way as the example above provides Std adopter.