fix: replace logrus with slog #781
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
25c669f to
8dab75f
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the logrus logging library with Go's standard library slog for structured logging. The change modernizes the logging approach by adopting the recommended standard library solution that aims to replace third-party logging libraries like logrus.
- Replace logrus imports with standard library slog across the codebase
- Convert logging calls to use structured logging with key-value pairs
- Update logger initialization and configuration to use slog handlers
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| go.mod | Remove logrus dependency from module requirements |
| pkg/log/io.go | Update IOLogger to use slog.Logger and convert log messages to structured format |
| pkg/log/io_test.go | Update test setup to use slog with custom time removal function |
| internal/ghmcp/server.go | Replace logrus with slog in server initialization and logging calls |
internal/ghmcp/server.go
Outdated
| stdLogger := log.New(logrusLogger.Writer(), "stdioserver", 0) | ||
| logger := slog.New(slogHandler) | ||
| logger.Info("starting server", "version", cfg.Version, "host", cfg.Host, "dynamicToolsets", cfg.DynamicToolsets, "readOnly", cfg.ReadOnly) | ||
| stdLogger := log.New(os.Stderr, stdioServerLogPrefix, 0) |
There was a problem hiding this comment.
The stdLogger is created with os.Stderr as output, but when a log file is configured, the slog logger uses the file while stdLogger still writes to stderr. This creates inconsistent logging destinations. Consider using the same output destination for both loggers.
Note: See the diff below for a potential fix:
@@ -206,18 +206,21 @@
stdioServer := server.NewStdioServer(ghServer)
var slogHandler slog.Handler
+ var logOutput io.Writer
if cfg.LogFilePath != "" {
file, err := os.OpenFile(cfg.LogFilePath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600)
if err != nil {
return fmt.Errorf("failed to open log file: %w", err)
}
+ logOutput = file
slogHandler = slog.NewTextHandler(file, &slog.HandlerOptions{Level: slog.LevelDebug})
} else {
+ logOutput = os.Stderr
slogHandler = slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelInfo})
}
logger := slog.New(slogHandler)
logger.Info("starting server", "version", cfg.Version, "host", cfg.Host, "dynamicToolsets", cfg.DynamicToolsets, "readOnly", cfg.ReadOnly)
- stdLogger := log.New(os.Stderr, stdioServerLogPrefix, 0)
+ stdLogger := log.New(logOutput, stdioServerLogPrefix, 0)
stdioServer.SetErrorLogger(stdLogger)
if cfg.ExportTranslations {
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Please double-check the io.Writer to stdLogger as file logOutput is desired. I updated the commit to follow the GH recommendation
|
@ricochet Could you run |
- Adjust some logs to use structured outputs - Set stdioserver log prefix as const - Do not export test func removeTimeAttr Signed-off-by: Bailey Hayes <behayes2@gmail.com>
8dab75f to
79f9af5
Compare
The golang's blog article introducing slog calls out logrus directly as tooling it aims to replace with a solution in the standard language library.
This change replaces logrus with slog and adjusts some of the log outputs to use structured logs.
Closes: #780