Replace go-kit/log with log/slog

[javiermolinar]

Tempo dependencies like Prometheus have started migrating from go-kit/log to the built-in slog.

prometheus/prometheus#14355

This can be a good opportunity to sunset go-kit

[Sheikh-Abubaker]

Hey @javiermolinar I'd like to work on it!

[javiermolinar]

Hey @javiermolinar I'd like to work on it!

Awesome. I think it's worth discussing the strategy

[Sheikh-Abubaker]

Awesome. I think it's worth discussing the strategy

Yeah sure! it's just that I don't exactly know about this issue however I'll checkout the the linked prometheus issue and try to comprehend.

[gozeloglu]

Hi @javiermolinar,

I checked the codebase and saw something can be important for this issue.

1. tempo/pkg/util/log/log.go

   Lines 11 to 14 in b4769e4

   	// Logger is a shared go-kit logger.
   	// TODO: Change all components to take a non-global logger via their constructors.
   	// Prefer accepting a non-global logger as an argument.
   	var Logger = kitlog.NewNopLogger()

   
   There is a TODO comment line and it can be a chance to apply it. As I understand that we should not use global Logger and implement NewLogger function to initialize a new logger. Is that correct?

2. If we create logger with New function, we should call it every struct initialization that we log something, right?

3. tempo/pkg/util/log/rate_limited_logger.go

   Lines 23 to 25 in b4769e4

   	// NewRateLimitedLogger returns a new RateLimitedLogger that logs at most logsPerSecond messages per second.
   	// TODO: migrate to the dskit rate limited logger
   	func NewRateLimitedLogger(logsPerSecond int, logger gkLog.Logger) *RateLimitedLogger {

   
   Should we remove rate_limited_logger.go?

4. However, there are some 3rd party package calls like

   tempo/cmd/tempo/app/modules.go

   Line 191 in b4769e4

   kvClient, err := kv.NewClient(t.cfg.Ingester.IngesterPartitionRing.KVStore, ring.GetPartitionRingCodec(), kv.RegistererWithKVName(prometheus.DefaultRegisterer, ingester.PartitionRingName+"-watcher"), util_log.Logger)

   but it uses the go-kit/log as here:
   https://github.com/grafana/dskit/blob/8abd91ce153f15ce4852d7f51e243816b3855eb8/kv/client.go#L129
   What do you think about it?

If the things I mentioned in the 2nd question are correct, I think, we should call the NewLogger function in the struct definition parts. For example, here:

tempo/modules/overrides/userconfigurable/api/api.go

Line 60 in b4769e4

logger: log.With(tempo_log.Logger, "component", "overrides-api"),

I think this line can be rewritten as

logger: tempo_log.NewLogger(tempo_log.WithKeyVals("component", "overrides-api"))

But, also we should update the type of the logger here:

tempo/modules/overrides/userconfigurable/api/api.go

Line 45 in b4769e4

logger log.Logger

I think it can be

type UserConfigOverridesAPI struct {
	// rest of the fields

	logger slog.Logger
}

We may need some options with the WithXxx prefixed function.

What do you think? Am I missing something?

[josephwoodward]

A number of questions first of all:

• How important are preserving matching log lines, ie are changes in log output considered a breaking change?
• If so then what guarantees (if any) exist over whether a log lines has changed?
• Is the intention to depend on the log/slog API directly or is there a common Tempo logging interface that is a preference?

One option might be to avoid a large change that requires all modules to be migrated to the log/slog API in one go and instead reduce the scope of the changes and do it in bits.

The first step would be to create a custom log/slog handler that matches the current logging interface (such as this go-kit/log interface):

type Logger interface {
	Log(keyvals ...interface{}) error
}

func With(logger Logger, keyvals ...interface{}) Logger
func WithPrefix(logger Logger, keyvals ...interface{}) Logger
func WithSuffix(logger Logger, keyvals ...interface{}) Logger

This would allow for removing the dependency on go-kit/log, then if the intention is to depend on the log/slog API directly then we could migrate to that bit by bit in separate PRs.

[github-actions]

This issue has been automatically marked as stale because it has not had any activity in the past 60 days.
The next time this stale check runs, the stale label will be removed if there is new activity. The issue will be closed after 15 days if there is no new activity.
Please apply keepalive label to exempt this Issue.