Simplify API proposal

[jamesrom]

Is there a reason why we need access to the logger directly? Logging is inherently global (stdout is all you need) so I can't think of a strong reason a developer might need direct access to the logger. If there is a reason, we can still expose it such that it hints to the engineer that they really should know what they are doing.

Some other thoughts I have about the existing API:

• From and Onto are a bit obscure, could lead to bugs where a logger is used in the wrong context
• Construction of fields is unintuitive. For example, it's not clear in the API that this wouldn't work:

logger := log.NewStandardLogger()
fields := log.With("greet", "world").WithLogger(logger)
logger.Info("hello")
// expected
// 2020-02-04T09:52:37.844322+11:00 INFO msg=hello greet=world
// got
// 2020-02-04T09:52:37.844322+11:00 INFO hello

• I would like to know the reasoning behind fields.WithFunc. It opens up a class of bugs and accidental misuse that seems far worse than the problem it solves.

With that said, here's my thinking on a simple pragmatic API.

type Fields map[string]interface{}

// Notice how fields are optional
func Debug(ctx context.Context, msg string, fields ...Fields) {}
func Info(ctx context.Context, msg string, fields ...Fields) {}
func Error(ctx context.Context, msg string, fields ...Fields) {}

func Debugf(ctx context.Context, format string, args ...interface{}) {}
func Infof(ctx context.Context, format string, args ...interface{}) {}
func Errorf(ctx context.Context, format string, args ...interface{}) {}

// Convenience function because go doesn't support multi variadic args
func (f Fields) Debugf(ctx context.Context, format string, args ...interface{}) {}
func (f Fields) Infof(ctx context.Context, format string, args ...interface{}) {}
func (f Fields) Errorf(ctx context.Context, format string, args ...interface{}) {}

func AddContextKey(ctx context.Context, key string, ctxKey interface{}) context.Context {}
func AddContextKeys(ctx context.Context, contextKeys Fields) context.Context {}
func AddField(ctx context.Context, key string, value interface{}) context.Context {}
func AddFields(ctx context.Context, fields Fields) context.Context {}

// Not sure we need this, but if we do we can just expose the logrus.Logger
// (otherwise we can reimpl our own logger, but I'm even less sure that's required)
func RawLogger(ctx context.Context) *logrus.Logger {}
func OverrideLogger(ctx context.Context, logger *logrus.Logger) context.Context {}

Example usage

// basic usage
ctx := context.Background()
log.Debug(ctx, "hello")
// msg="hello"

// format string usage
log.Debugf(ctx, "hello %s", "world")
// msg="hello world"

// fields usage
fields := log.Fields{"greet": "world"}
log.Debug(ctx, "hello", fields)
// msg="hello" greet="world"

// fields with format string usage
fields.Debugf(ctx, "hello %s", ":)")
// msg="hello :)" greet="world"

// adding context key
ctx = context.WithValue(ctx, "greet", "world")
ctx = log.AddContextKey(ctx, "greet")
log.Debug(ctx, "hello")
// msg="hello" greet="world"

// adding fields
ctx = log.AddField(ctx, "size", 100)
log.Debug(ctx, "increased size")
// msg="increased size" size=100 greet="world"

I'm willing to be persuaded here. I would love your thoughts.

[nofun97]

Regarding the WithFunc, it was added just for more flexibility should a field value depends on context or anything else.

For the design, the chain operation allows us to add all kinds of fields, configs, and logger in one operation. I know the chain operation is not idiomatic but I think it's more convenient than doing the Add* operations one by one.

I agree with the problem your first example present on accessing the logger directly. Because of #17 it might be good to hide the logger methods.

[daemonl]

It might be worth splitting this out into a few issues so we can discuss and track separately.

My take on the split would be:

• Do we need access to the logger directly
• Context methods could be clearer (From and Onto)
• WithFunc opens bug possibilities

And leave the remainder (Simpler API for fields vs logger, and the actual proposal) in this ticket

[jamesrom]

Regarding the WithFunc, it was added just for more flexibility should a field value depends on context or anything else.

YAGNI. Can you give a specific example of a problem it solves? I can give an example of a problem it introduces: you've now got a way to get into an infinite loop without really realising it.

fields.WithFunc("foo", func(ctx context.Context) interface{} {
    return util.getValueOfFoo(ctx)
}).Onto(ctx)

// somewhere else, some utility package etc
func getValueOfFoo(ctx context.Context) string {
    log.From(ctx).Info("getting value of foo") // infinite recursion here
    return "bar"
}