Abstract sirupsen/logrus for the core CP library

[andig]

It would be great if logging were configurable, i.e. a custom logger could be injected.

[andig]

I could create a PR for a small logrus abstraction but would need help feeding it into the whole setup.

[andig]

A default logger interface could look something like this:

var Log Logger

type Logger interface {
    Print(v ...interface{})
    Printf(format string, v ...interface{})
    Debug(v ...interface{})
    Debugf(format string, v ...interface{})
    Info(v ...interface{})
    Infof(format string, v ...interface{})
    Warn(v ...interface{})
    Warnf(format string, v ...interface{})
    Error(v ...interface{})
    Errorf(format string, v ...interface{})
    Fatal(v ...interface{})
    Fatalf(format string, v ...interface{})
}

type Entry interface {
    Logger
    WithField(key, val string) Entry
    WithFields(fields map[string]interface{}) Entry
}

One could add an init method to populate sirupsen/logrus by default or even leave it nil to avoid the dependency.

[lorenzodonini]

Yes, that is a very good point.

I'd actually love an opinion on this one, since there is another imho valid approach, compared to the one you posted:

type Logger func(...interface{})

type FormattedLogger func(...interface{})

var debugLog Logger = nil
var formatLog FormattedLogger = nil

func SetLogger(logger Logger) {
    debugLog = logger
}

func SetFormattedLogger(logger FormattedLogger) {
        formatLog = logger
}

And an application would just have to hook it up like this:

ocpplog.SetLogger(logrus.Debug)
ocpplog.SetFormattedLogger(logrus.Debugf)

It's less flexible but very minimal, and does not require any further implementation.

What do you think?

[andig]

I'd honestly prefer a more minimal logging interface- the ability to differentiate from trace to fatal seems important though. I'm not sure why you need two different ones?

Something like this would be somewhat minimal:

type Logger interface {
    Print(v ...interface{})
    Printf(format string, v ...interface{})
    Debug(v ...interface{})
    Debugf(format string, v ...interface{})
    Info(v ...interface{})
    Infof(format string, v ...interface{})
    Warn(v ...interface{})
    Warnf(format string, v ...interface{})
    Error(v ...interface{})
    Errorf(format string, v ...interface{})
    Fatal(v ...interface{})
    Fatalf(format string, v ...interface{})
}

You'd still need nil checks if the logger is not initialized or assign a nil implementation be default. Setting logging level might also be added.

See https://github.com/influxdata/influxdb-client-go/blob/master/log/logger.go for something I've contributed to.

[lorenzodonini]

Well, the library itself should not really throw anything other than errors or debug information. There might be some more levels showing up right now, but that can be optimized.

Anyways, feel free to add your suggested solution. Do you want me to add you as a collaborator? So you can branch from here directly and I can help out adjusting the existing log statements.

[andig]

Imho you don't need

ocpplog.SetLogger(logrus.Debug)
ocpplog.SetFormattedLogger(logrus.Debugf)

It's usual for a logger to support the plain and formatted version afaiks.

Imho there are 3 reasons of logging (i.e. error handling in context of this library):

1. synchronous errors - these should be returned, caller should handle (logging is handling)
2. intermediate errors that can be retried internally - these might either be logged (need a logging abstraction then like log.Logger as interface) or communicated (need something like errorChan then). Note sure this case currently exists?
3. errors from async operations. You could either make the operation synchronous and leave it to the caller to wrapp error handling inside a go func or return result using something like an errorChan or resultChan (or both). Any of these channels could also serve as indicator that the async operation has finished

Generic debug logging should probably not be part of the final library. If it's necessary it could be handled similar to 2.

[andig]

I'll make a suggestion for the core library.

[andig]

Suggestion is done, just added the websocket library as well. If this is the direction you want to go in I can see these points:

• validate ich client/server and websocket error handling should be connected
• validate if client/server error handler should be a richer type (struct something)
• update examples to use the error handler
• update readme