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

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?

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

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?

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?

	// 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()

	// 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 {

Replace go-kit/log with log/slog #4819

Description

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions