Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support custom logger for plugins #115

Open
ShoshinNikita opened this issue Nov 14, 2021 · 3 comments · May be fixed by #116
Open

Support custom logger for plugins #115

ShoshinNikita opened this issue Nov 14, 2021 · 3 comments · May be fixed by #116

Comments

@ShoshinNikita
Copy link
Contributor

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
Copy link
Owner

umputun commented Nov 14, 2021

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
Copy link
Contributor Author

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:

  • Logger with a single method Logf(format string, args ...interface{})
  • AdvancedLogger with all methods from my first comment

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:

  • users who don't care about logging at all and won't set a custom logger
  • users who want full control over logging

So, most users will use SetLogger to pass AdvancedLogger

@umputun
Copy link
Owner

umputun commented Nov 14, 2021

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.

@ShoshinNikita ShoshinNikita linked a pull request Nov 15, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants