logging: Migrate from logrus to slog #3814
Conversation
sayboras
added
the
release-note/minor
sayboras
force-pushed
the
pr/tammach/klog
branch
4 times, most recently
from
3a7c41a to
bde5071
Compare
sayboras
changed the title
sayboras
force-pushed
the
pr/tammach/klog
branch
2 times, most recently
from
794dcf8 to
0f618d6
Compare
|
/test |
sayboras
force-pushed
the
pr/tammach/klog
branch
7 times, most recently
from
9dc0e94 to
a211e5e
Compare
sayboras
force-pushed
the
pr/tammach/klog
branch
3 times, most recently
from
6f1d67f to
fa63584
Compare
There was a problem hiding this comment.
Thank you, that's a massive migration. I wanted to do something similar but never had the motivation!
I think something we could do next (of course in another PR) is to grep for log.*(fmt.Sprintf and see if we can replace those with proper structured logging.
| type FieldLogger interface { | ||
| Handler() slog.Handler | ||
| With(args ...any) *slog.Logger | ||
| WithGroup(name string) *slog.Logger |
There was a problem hiding this comment.
I was wondering, why not redefine the WithError to ease the change? I guess there was already a debate on this when they created slog :)
There was a problem hiding this comment.
FieldLogger is implemented by slog. Yes, slog didn't offer WithError function sadly.
| log.WithField(option.KeyConfigDir, configDir).WithError(err).Fatal("Failed to read config from directory") | ||
| logger.Fatal(log, "Failed to read config from directory", option.KeyConfigDir, configDir, logfields.Error, err) | ||
| } else { | ||
| log.WithField(option.KeyConfigDir, configDir).Info("Loaded config from directory") | ||
| log.Info("Loaded config from directory", option.KeyConfigDir, configDir) |
There was a problem hiding this comment.
How does that work? I'm a little bit confused about the use of logger. and log.
There was a problem hiding this comment.
logger.Fatal is syntactic sugar for the below, as slog didn't have Fatal method.
logger.Error(msg, args...)
os.Exit(-1)
There was a problem hiding this comment.
Yep, I'm also confused. When would we use log. and when would we use logger.? i.e. why isn't it logger.Info() on line 58?
There was a problem hiding this comment.
Sadly, we don't have log.{Fatal,Panic,Trace} in slog, so that is why we need to use logger.{Fatal,Panic,Trace} utility functions.
Ideally, there will be not much change related to developer experience, as log.{Error,Warn,Info,Debug} are more common.
There was a problem hiding this comment.
ok! For some reason I thought you were writing your own interface and implementing it with slog under the hood from #3814 (comment)
Signed-off-by: Tam Mach <[email protected]>
sayboras
force-pushed
the
pr/tammach/klog
branch
from
fa63584 to
28c4a98
Compare
Description
While the underlying issue didn't require to migrate to slog, taking this change to carry on with migration as:
Fixes: #3795
Changelog