- Notifications
You must be signed in to change notification settings - Fork2.7k
fix: replace logrus with slog#781
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 |
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please double-check the io.Writer to stdLogger as file logOutput is desired. I updated the commit to follow the GH recommendation
mattdholloway commentedJul 30, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@ricochet Could you run |
- Adjust some logs to use structured outputs- Set stdioserver log prefix as const- Do not export test func removeTimeAttrSigned-off-by: Bailey Hayes <behayes2@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good!
3479d02 intogithub:mainUh oh!
There was an error while loading.Please reload this page.
- Adjust some logs to use structured outputs- Set stdioserver log prefix as const- Do not export test func removeTimeAttrSigned-off-by: Bailey Hayes <behayes2@gmail.com>Co-authored-by: Matt Holloway <mattdholloway@github.com>Co-authored-by: JoannaaKL <joannaakl@github.com>
Thegolang'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