Introduce structured logging

[sagikazarmark]

Is your feature request related to a problem?

Logs are currently formatted strings with a single message. If someone were to extract any information it would require parsing the log messages.

Describe the solution you'd like to see

Introduce structured logging. Probably replace the logger interface with this: https://github.com/logur/logur

Describe alternatives you've considered

I don't see an alternative: the current logging strategy is kinda useless IMHO.

Additional context

I noticed this when working on the run group in the serve command: log messages contain server names, listen addresses.

[sagikazarmark]

I'd like to work on this, but we should reach a consensus first.

I'm not a huge fan of the current logging strategy, because:

• it's not structured, so searching in logs is not easy
• the logger interface is basically an abstraction over Logrus

The ideal logger interface should be owned by Dex. Logur provides a template for that (both map[string]interface{} and key-value based).

Another nice logging library/abstraction is go-kit/log. It's also key-value based, but it's not easy to decouple from the code with an interface.

As for the implementation: Uber's Zap seems to be the most popular choice these days.

[nabokihms]

Thank you Mark. It feels like logs is one of our weak spots.

1. I like the idea of refactoring the logger interface for our needs. A couple of times I thought about how good it will be if we have labels (especially in combination with JSON format).
2. +1 for zap.

[sagikazarmark]

I came up with a few different options:

Option 1

The simplest interface with key-value pair support.

type Logger interface {
	Debug(msg string, kvs ...interface{})
	Info(msg string, kvs ...interface{})
	Warn(msg string, kvs ...interface{})
	Error(msg string, kvs ...interface{})
}

Usage:

logger.Info("Something happened", "in", "storage")

Option 2

A bit more complex interface with some level of type (and argument count) safety.

type Logger interface {
	Debug(msg string, kvs ...KeyValue)
	Info(msg string, kvs ...KeyValue)
	Warn(msg string, kvs ...KeyValue)
	Error(msg string, kvs ...KeyValue)
}

type KeyValue struct {
	Key   string
	Value interface{}
}

func KV(key string, value interface{}) KeyValue {
	return KeyValue{
		Key:   key,
		Value: value,
	}
}

Usage:

logger.Info("Something happened", log.KV("at", "storage"))

Option 3

Zap's API without directly importing Zap in the rest of the application. Has a bit higher maintenance cost.

type Logger interface {
	Debug(msg string, fields ...Field)
	Info(msg string, fields ...Field)
	Warn(msg string, fields ...Field)
	Error(msg string, fields ...Field)

	With(fields ...Field) Logger
}

type Field = zap.Field

func String(key string, value string) Field {
	return zap.String(key, value)
}

Usage:

logger.Info("Something happened", log.String("at", "storage"))

Option 4:

Just use Zap as is. Generally, I'm not a huge fan of hard-wiring dependencies into my applications, but Go lacks the consensus on a logger interface and Zap's API is type safe and fast.

---

Going with one or the other doesn't mean we have to set that in stone for ever.

For example, we could go with option 1 and move to one of the Zap solutions later. The goal is to have some sort of structured logging and to improve developer experience. (But if we can avoid shooting us in the leg or dead ends, that would be great as well)

[sagikazarmark]

This has been long open and while I'd really like to make progress here, the fact that there is an effort to make a standard library structured logger kept me back in the last couple months.

We need to make a decision here: go with Zap or wait and hope that Go 1.21 will have a structured logger.

[nabokihms]

golang/go#56345 (comment)

[sagikazarmark]

Yep, I saw that. Though it seems there are lots of concerns raised, so it may not be the final version yet.