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

replacing logrus with slog v1 #2010

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

JodhwaniMadhur
Copy link

What is the problem I am trying to address?

Removing logrus and implementing slog

How is the fix applied?

Replaced logrus.

Implemented functions that are missing in slog but were present in slog

What GitHub issue(s) does this PR fix or close?

Fixes #1890

@JodhwaniMadhur JodhwaniMadhur requested a review from a team as a code owner December 2, 2024 16:27
Copy link
Collaborator

@Ullaakut Ullaakut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superficial review

cmd/proxy/main.go Outdated Show resolved Hide resolved
cmd/proxy/main.go Outdated Show resolved Hide resolved
cmd/proxy/main.go Outdated Show resolved Hide resolved
pkg/log/format.go Outdated Show resolved Hide resolved
pkg/middleware/log_entry.go Outdated Show resolved Hide resolved
pkg/middleware/log_entry_test.go Outdated Show resolved Hide resolved
pkg/middleware/log_entry_test.go Outdated Show resolved Hide resolved
pkg/log/log.go Outdated Show resolved Hide resolved
cmd/proxy/main.go Outdated Show resolved Hide resolved
cmd/proxy/main.go Outdated Show resolved Hide resolved
cmd/proxy/main.go Outdated Show resolved Hide resolved
pkg/log/log_test.go Outdated Show resolved Hide resolved
@marwan-at-work
Copy link
Contributor

marwan-at-work commented Dec 2, 2024

@JodhwaniMadhur the idea here is correct: we should replace logrus with log/slog. However, the approach should be as follows:

  1. Let's only change the athens/log package. So that importers of the athens/log package don't need to change at all. We simply implement a slog version of the Entry interface.
  2. This will fix some of your build errors above, and it will also divide up the PR and make it a smaller change rather than a large one.
  3. If we ever felt that we needed to change athens/log we can do so in a separate/follow up discussion.

Copy link
Collaborator

@Ullaakut Ullaakut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this doesn't compile/doesn't work as intended at the moment does it?

pkg/log/entry.go Outdated Show resolved Hide resolved
pkg/log/log.go Outdated Show resolved Hide resolved
@JodhwaniMadhur
Copy link
Author

I assume this doesn't compile/doesn't work as intended at the moment does it?

Nope, it doesn't. But will take care of everything.

thanks for all the suggestions.

@JodhwaniMadhur
Copy link
Author

JodhwaniMadhur commented Dec 3, 2024

@JodhwaniMadhur the idea here is correct: we should replace logrus with log/slog. However, the approach should be as follows:

  1. Let's only change the athens/log package. So that importers of the athens/log package don't need to change at all. We simply implement a slog version of the Entry interface.
  2. This will fix some of your build errors above, and it will also divide up the PR and make it a smaller change rather than a large one.
  3. If we ever felt that we needed to change athens/log we can do so in a separate/follow up discussion.

But then in the entry we have Severity coming in as logrus.Level which indeed should be then a generic levels decided by us or we switch to slog.Level, isn't it? and severity is outside of entry

pkg/log/entry.go Outdated Show resolved Hide resolved
@JodhwaniMadhur JodhwaniMadhur marked this pull request as draft December 4, 2024 19:27
@JodhwaniMadhur
Copy link
Author

can anyone guide me as to how I should modify the Severity function? as it was designed for logrus but for slog the TestSeverity fails

for logrus, the levels are
Panic = 0
Fatal = 1
err = 2
warn = 3
Info = 4
Debug = 5
Trace = 6

but for slog the levels are:
err = 8
warn = 4
info = 0
debug = -4

@marwan-at-work @nrwiersma @Ullaakut

Copy link
Contributor

@marwan-at-work marwan-at-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As to your question, if you look at the entry interface, we weren't using all of logrus's levels, we also were just using debug/info/warn/error, so let's just keep those.

LogOps
FormattedLogOps
ContextualLogOps
SystemLogger
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep this interface exactly the same and just implement it in terms of slog, why create all of the new methods above if they were not being used?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if I remove the rest, I will have to keep them in a separate interface and take those interfaces in entry since if I write all functions inside entry, the linter in the pipeline gives error for interfacebloat

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the extra methods, I can remove those but the problem for logrus vs slog levels and problems in Severity due to this still remains

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the extra methods, I can remove those but the problem for logrus vs slog levels and problems in Severity due to this still remains

I'm not sure how they still remain, all you need to do is create a separate file that implements Entry in terms of log/slog?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example:

type slogger struct {
	l *slog.Logger
}

// Debugf implements Entry.
func (s *slogger) Debugf(format string, args ...any) {
	s.l.Debug(fmt.Sprintf(format, args...))
}

// Errorf implements Entry.
func (s *slogger) Errorf(format string, args ...any) {
	s.l.Error(fmt.Sprintf(format, args...))
}

For severity in the errors package, you can continue to use logrus levels, or you can change it to slog, or better yet create an abstraction in the athens log package because the error package shouldn't import a third party lib

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An abstraction is preferred, imo.

@matt0x6F
Copy link
Contributor

matt0x6F commented Dec 8, 2024

I would expect to see a go.mod/go.sum change that shows we removed Logrus as a dependency

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

Successfully merging this pull request may close these issues.

Replace logrus with log/slog
5 participants