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.

Sign up for GitHub

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 19 commits into
base: main
Choose a base branch
from
Draft

replacing logrus with slog v1 #2010

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
1b6f4d6
replacing logrus with slog v1
Dec 2, 2024
bd63c16
modifying entry interface
Dec 4, 2024
647228e
#1890 modifying logger pkg to use slog
Dec 4, 2024
366bc6e
Rectifying small mistakes
Dec 4, 2024
b5d0a06
adding option for writing in buf and resolving errors
Dec 5, 2024
e81aa10
Removing Out from Logger struct
Dec 5, 2024
28340f3
Resolving testing errors due to logrus
Dec 5, 2024
e765552
removing a few lint errors
Dec 5, 2024
ba79569
removinf lint errors for entry
Dec 7, 2024
f0bf927
Merge branch 'replacing-logrus' of github.com:JodhwaniMadhur/athens i…
Dec 7, 2024
5039841
Resolving lint errors for main and entry
Dec 7, 2024
f22cc6c
declogging entry interface to avoid interfacebloat
Dec 7, 2024
4859555
resolved comment related linter errors for entry
Dec 7, 2024
f9b398f
entry related comment linter issue resolution
Dec 7, 2024
01b47be
Merge branch 'main' into replacing-logrus
Dec 7, 2024
d48ecdf
removing unwanted methods from entry
Dec 8, 2024
e2f532c
adding printf for entry
Dec 8, 2024
0b291b0
resolving minor mistake
Dec 8, 2024
caf4ea3
changing no op logger for slog and removing logrus from go.mod and go…
Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/proxy/actions/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func App(logger *log.Logger, conf *config.Config) (http.Handler, error) {
conf.GoEnv,
)
if err != nil {
logger.Info(err)
logger.Info(err.Error())
} else {
defer flushTraces()
}
Expand All @@ -87,7 +87,7 @@ func App(logger *log.Logger, conf *config.Config) (http.Handler, error) {
// was specified by the user.
flushStats, err := observ.RegisterStatsExporter(r, conf.StatsExporter, Service)
if err != nil {
logger.Info(err)
logger.Info(err.Error())
} else {
defer flushStats()
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/proxy/actions/app_proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ type athensLoggerForRedis struct {
}

func (l *athensLoggerForRedis) Printf(ctx context.Context, format string, v ...any) {
l.logger.WithContext(ctx).Printf(format, v...)
l.logger.WithContext(ctx).Infof(format, v...)
}

func getSingleFlight(l *log.Logger, c *config.Config, s storage.Backend, checker storage.Checker) (stash.Wrapper, error) {
Expand Down
5 changes: 3 additions & 2 deletions cmd/proxy/actions/basicauth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ import (
"strings"
"testing"

"log/slog"

"github.com/gomods/athens/pkg/log"
"github.com/sirupsen/logrus"
)

var basicAuthTests = [...]struct {
Expand Down Expand Up @@ -70,7 +71,7 @@ func TestBasicAuth(t *testing.T) {
w := httptest.NewRecorder()
r := httptest.NewRequest(http.MethodGet, tc.path, nil)
r.SetBasicAuth(tc.user, tc.pass)
lggr := log.New("none", logrus.DebugLevel, "")
lggr := log.New("none", slog.LevelDebug, "")
buf := &bytes.Buffer{}
lggr.Out = buf
ctx := log.SetEntryInContext(context.Background(), lggr)
Expand Down
31 changes: 11 additions & 20 deletions cmd/proxy/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"flag"
"fmt"
stdlog "log"
"log/slog"
"net"
"net/http"
_ "net/http/pprof"
Expand All @@ -18,7 +19,6 @@ import (
"github.com/gomods/athens/pkg/build"
"github.com/gomods/athens/pkg/config"
athenslog "github.com/gomods/athens/pkg/log"
"github.com/sirupsen/logrus"
)

var (
Expand All @@ -37,26 +37,17 @@ func main() {
stdlog.Fatalf("Could not load config file: %v", err)
}

logLvl, err := logrus.ParseLevel(conf.LogLevel)
logLvl := slog.Level(0)
err = logLvl.UnmarshalText([]byte(conf.LogLevel))
if err != nil {
stdlog.Fatalf("Could not parse log level %q: %v", conf.LogLevel, err)
}

logger := athenslog.New(conf.CloudRuntime, logLvl, conf.LogFormat)

// Turn standard logger output into logrus Errors.
logrusErrorWriter := logger.WriterLevel(logrus.ErrorLevel)
defer func() {
if err := logrusErrorWriter.Close(); err != nil {
logger.WithError(err).Warn("Could not close logrus writer pipe")
}
}()
stdlog.SetOutput(logrusErrorWriter)
stdlog.SetFlags(stdlog.Flags() &^ (stdlog.Ldate | stdlog.Ltime))

handler, err := actions.App(logger, conf)
if err != nil {
logger.WithError(err).Fatal("Could not create App")
logger.With(err.Error()).Error("Could not create App")
JodhwaniMadhur marked this conversation as resolved.
Show resolved Hide resolved
JodhwaniMadhur marked this conversation as resolved.
Show resolved Hide resolved
}

srv := &http.Server{
Expand All @@ -75,7 +66,7 @@ func main() {
ctx, cancel := context.WithTimeout(context.Background(), time.Second*time.Duration(conf.ShutdownTimeout))
defer cancel()
if err := srv.Shutdown(ctx); err != nil {
logger.WithError(err).Fatal("Could not shut down server")
logger.With(err).Error("Could not shut down server")
}
close(idleConnsClosed)
}()
Expand All @@ -86,7 +77,7 @@ func main() {
// not to expose profiling data and avoid DoS attacks (profiling slows down the service)
// https://www.farsightsecurity.com/txt-record/2016/10/28/cmikk-go-remote-profiling/
logger.WithField("port", conf.PprofPort).Infof("starting pprof")
logger.Fatal(http.ListenAndServe(conf.PprofPort, nil)) //nolint:gosec // This should not be exposed to the world.
logger.Error(http.ListenAndServe(conf.PprofPort, nil).Error()) //nolint:gosec // This should not be exposed to the world.
JodhwaniMadhur marked this conversation as resolved.
Show resolved Hide resolved
JodhwaniMadhur marked this conversation as resolved.
Show resolved Hide resolved
}()
}

Expand All @@ -95,19 +86,19 @@ func main() {

if conf.UnixSocket != "" {
logger := logger.WithField("unixSocket", conf.UnixSocket)
logger.Info("Starting application")
logger.Infof("Starting application")
JodhwaniMadhur marked this conversation as resolved.
Show resolved Hide resolved
JodhwaniMadhur marked this conversation as resolved.
Show resolved Hide resolved

ln, err = net.Listen("unix", conf.UnixSocket)
if err != nil {
logger.WithError(err).Fatal("Could not listen on Unix domain socket")
logger.WithError(err).Fatalf("Could not listen on Unix domain socket")
}
} else {
logger := logger.WithField("tcpPort", conf.Port)
logger.Info("Starting application")
logger.Infof("Starting application")

ln, err = net.Listen("tcp", conf.Port)
if err != nil {
logger.WithError(err).Fatal("Could not listen on TCP port")
logger.WithError(err).Fatalf("Could not listen on TCP port")
}
}

Expand All @@ -118,7 +109,7 @@ func main() {
}

if !errors.Is(err, http.ErrServerClosed) {
logger.WithError(err).Fatal("Could not start server")
logger.WithError(err).Fatalf("Could not start server")
}

<-idleConnsClosed
Expand Down
5 changes: 4 additions & 1 deletion pkg/download/protocol.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,9 @@ func union(list1, list2 []string) []string {
func copyContextWithCustomTimeout(ctx context.Context, timeout time.Duration) (context.Context, context.CancelFunc) {
ctxCopy, cancel := context.WithTimeout(context.Background(), timeout)
ctxCopy = requestid.SetInContext(ctxCopy, requestid.FromContext(ctx))
ctxCopy = log.SetEntryInContext(ctxCopy, log.EntryFromContext(ctx))

if entry := log.EntryFromContext(ctx); entry != nil {
ctxCopy = log.SetEntryInContext(ctxCopy, &entry)
}
return ctxCopy, cancel
}
4 changes: 4 additions & 0 deletions pkg/download/protocol_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,8 +507,12 @@ func (e *testEntry) Debugf(format string, args ...any) {
func (*testEntry) Infof(format string, args ...any) {}
func (*testEntry) Warnf(format string, args ...any) {}
func (*testEntry) Errorf(format string, args ...any) {}
func (*testEntry) Fatalf(format string, args ...any) {}
func (*testEntry) WithFields(fields map[string]any) log.Entry { return nil }
func (*testEntry) SystemErr(err error) {}
func (*testEntry) WithContext(ctx context.Context) log.Entry { return nil }
func (*testEntry) WithError(err error) log.Entry { return nil }
func (*testEntry) WithField(key string, value any) log.Entry { return nil }

func Test_copyContextWithCustomTimeout(t *testing.T) {
testEntry := &testEntry{}
Expand Down
19 changes: 9 additions & 10 deletions pkg/errors/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,9 @@ package errors
import (
"errors"
"fmt"
"log/slog"
"net/http"
"runtime"

"github.com/sirupsen/logrus"
)

// Kind enums.
Expand Down Expand Up @@ -37,7 +36,7 @@ type Error struct {
Module M
Version V
Err error
Severity logrus.Level
Severity slog.Level
}

// Error returns the underlying error's
Expand Down Expand Up @@ -111,7 +110,7 @@ func E(op Op, args ...any) error {
e.Module = a
case V:
e.Version = a
case logrus.Level:
case slog.Level:
e.Severity = a
case int:
e.Kind = a
Expand All @@ -126,17 +125,17 @@ func E(op Op, args ...any) error {
// Severity returns the log level of an error
// if none exists, then the level is Error because
// it is an unexpected.
func Severity(err error) logrus.Level {
func Severity(err error) slog.Level {
var e Error
if !errors.As(err, &e) {
return logrus.ErrorLevel
return slog.LevelError
}

// if there's no severity (0 is Panic level in logrus
// which we should not use since cloud providers only have
// debug, info, warn, and error) then look for the
// child's severity.
if e.Severity < logrus.ErrorLevel {
if e.Severity < slog.LevelError {
return Severity(e.Err)
}

Expand All @@ -146,13 +145,13 @@ func Severity(err error) logrus.Level {
// Expect is a helper that returns an Info level
// if the error has the expected kind, otherwise
// it returns an Error level.
func Expect(err error, kinds ...int) logrus.Level {
func Expect(err error, kinds ...int) slog.Level {
for _, kind := range kinds {
if Kind(err) == kind {
return logrus.InfoLevel
return slog.LevelInfo
}
}
return logrus.ErrorLevel
return slog.LevelError
}

// Kind recursively searches for the
Expand Down
30 changes: 19 additions & 11 deletions pkg/log/entry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package log

import (
"context"
"log/slog"

"github.com/gomods/athens/pkg/errors"
"github.com/sirupsen/logrus"
)

// Entry is an abstraction to the
Expand All @@ -16,46 +18,52 @@ type Entry interface {
Infof(format string, args ...any)
Warnf(format string, args ...any)
Errorf(format string, args ...any)
Fatalf(format string, args ...any)

// Attach contextual information to the logging entry
WithFields(fields map[string]any) Entry

WithField(key string, value any) Entry

WithError(err error) Entry

WithContext(ctx context.Context) Entry

// SystemErr is a method that disects the error
// and logs the appropriate level and fields for it.
SystemErr(err error)
}
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.

Copy link
Author

Choose a reason for hiding this comment

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

ok, got the abstraction part but still a bit confused, so you mean to say that i should only have the function declarations in entry and then implement those in log.go like it was done before my changes for SystemErr implementation or like for WithFields implementation?

Copy link
Author

Choose a reason for hiding this comment

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

??

Copy link
Contributor

@marwan-at-work marwan-at-work Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I mean that you keep the interface exactly the same and just create a log/slog v1 implementation of that interface (in a separate file, within the same package). Therefore, nothing changes outside of the log package (and the errors/severity, due to the abstraction we talked about above)


type entry struct {
*logrus.Entry
*slog.Logger
}

func (e *entry) WithFields(fields map[string]any) Entry {
ent := e.Entry.WithFields(fields)
return &entry{ent}
ent := e.WithFields(fields)
return ent
}
JodhwaniMadhur marked this conversation as resolved.
Show resolved Hide resolved

func (e *entry) SystemErr(err error) {
var athensErr errors.Error
if !errors.AsErr(err, &athensErr) {
e.Error(err)
e.Error(err.Error())
return
}

ent := e.WithFields(errFields(athensErr))
switch errors.Severity(err) {
case logrus.WarnLevel:
case slog.LevelWarn:
ent.Warnf("%v", err)
case logrus.InfoLevel:
case slog.LevelInfo:
ent.Infof("%v", err)
case logrus.DebugLevel:
case slog.LevelDebug:
ent.Debugf("%v", err)
default:
ent.Errorf("%v", err)
}
}

func errFields(err errors.Error) logrus.Fields {
f := logrus.Fields{}
func errFields(err errors.Error) map[string]any {
f := map[string]any{}
f["operation"] = err.Op
f["kind"] = errors.KindText(err)
f["module"] = err.Module
Expand Down
Loading
Loading