Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize on go-logr/logr instead of sirupsen/logrus #1419

Closed
jessesuen opened this issue Jan 24, 2024 · 10 comments · Fixed by #2073
Closed

Standardize on go-logr/logr instead of sirupsen/logrus #1419

jessesuen opened this issue Jan 24, 2024 · 10 comments · Fixed by #2073

Comments

@jessesuen
Copy link
Member

Proposed Feature

sirupsen/logrus is in maintenance mode. go-logr/logr is a logging API/wrapper that is agnostic to the actual underlying implementation.

Many go libraries, including sigs.k8s.io/controller-runtime/pkg/log, are starting to standardize on the generic go-logr/logr interface, and expect you to set a logr instance if you want log output. e.g.

package logging

import (
	runtimelog "sigs.k8s.io/controller-runtime/pkg/log"
	"github.com/go-logr/logr"
)


func init() {
	var mylogr logr.Logger
	runtimelog.SetLogger(mylogr)
}

NOTE: as I mentioned, go-logr/logr is just an interface, we still need to choose the implementation. This can still be sirupsen/logrus, or we can choose zap, klog. Recommend moving off sirupsen/logrus since it is deprecated, but the first step is to move to the go-logr/logr interface.

Motivation

sirupsen/logrus is no longer maintained

Suggested Implementation

@krancour
Copy link
Member

💯 on all of this.

@webstradev
Copy link
Contributor

Is this specifically for the logging package or for all logging done throughout Kargo.

@krancour
Copy link
Member

krancour commented Feb 7, 2024

Everywhere, but I wanted to take a closer look at this first. (Or you can?) I have no great love for logrus, but one very important feature of it that I don't want to lose is the ability to add key/val pairs to log entries. Not all loggers have that, unfortunately, and I like keeping the logs at least semi-structured.

@webstradev were you interested in working on this?

@webstradev
Copy link
Contributor

webstradev commented Feb 7, 2024

Yeah I'm happy to take a look at the implications of this.

One other consequence of switching to logr is not having warning, and fatal log level's.

I'll do a bit of digging into the logr docs to see what else might occur.

Would we want to move to logr first (sticking with logrus as the implementation) and then change implementations in the future?

And maybe one other question. Have you considered slog which was introduced in go 1.21?

@krancour
Copy link
Member

krancour commented Feb 7, 2024

Have you considered slog which was introduced in go 1.21?"

I'd be open to a cook-off, but would also want to hear @jessesuen's opinion.

@webstradev
Copy link
Contributor

FYI I'm not very opinionated on the implementation, i have yet to use slog myself other than a quick mess around but it might be worth considering.

@krancour
Copy link
Member

krancour commented Feb 7, 2024

@webstradev we're super appreciative of the assistance.

@bakunowski
Copy link

@webstradev Did you get a chance to look at this? I've dug around a little and got a provisional PR, but don't want to step on your toes.

@webstradev
Copy link
Contributor

No I haven't had the time yet so feel free to take it over.

@krancour
Copy link
Member

krancour commented Apr 2, 2024

Pulling this into v0.6.0 purely on the basis of being low-hanging fruit.

It can be deferred if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants