Replace logrus with slog #56
Conversation
WalkthroughWalkthroughThe changes involve a comprehensive transition from the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range, codebase verification and nitpick comments (1)
pkg/driver/controller_test.go (1)
Update documentation in
main.goregardingLogFormat.The
LogFormatfield has been removed from thegetDriverfunction, but it is still mentioned in the configuration documentation withinmain.go. Please update this documentation to reflect the current state of the codebase and avoid potential confusion. Ensure that no other parts of the codebase rely onLogFormatfor functionality.
main.go: Update or remove references toLogFormatin the configuration documentation.Analysis chain
Line range hint
178-178: Verify the impact of removingLogFormat.The removal of the
LogFormatfield from thegetDriverfunction simplifies the logging configuration. Please ensure that this change aligns with the new logging library's capabilities and does not adversely affect any logging behavior or dependent functionalities.Run the following script to verify the usage of the
LogFormatfield in other parts of the codebase:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `LogFormat` field in other parts of the codebase. # Test: Search for the `LogFormat` field usage. Expect: No occurrences outside the removed instance. rg --type go 'LogFormat'Length of output: 102
Script:
#!/bin/bash # Description: Investigate the usage of `LogFormat` in `main.go`. # Search for the context around `LogFormat` in `main.go` to understand its usage. rg --type go -A 5 -B 5 'LogFormat' main.goLength of output: 864
| @@ -1132,7 +1098,7 @@ func (c *Client) doUpdateVolume( | |||
| // expired for the CO to [presumably] retry. | |||
| return nil, nil | |||
| case codes.InvalidArgument: | |||
| log.Errorf("volume update refused by LB on bad arg: %s", st.Message()) | |||
| log.Error("volume update refused by LB on bad arg", "error", st.Message()) | |||
There was a problem hiding this comment.
Issue: Handle error logging for volume update refusal in UpdateVolume.
The logging of errors when a volume update is refused due to bad arguments is crucial for diagnosing issues. However, the log level should be adjusted to Warn instead of Error since this is not a system error but a user or request error.
Consider changing the log level to reflect the nature of the error:
- log.Error("volume update refused by LB on bad arg", "error", st.Message())
+ log.Warn("volume update refused by LB on bad arg", "error", st.Message())Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log.Error("volume update refused by LB on bad arg", "error", st.Message()) | |
| log.Warn("volume update refused by LB on bad arg", "error", st.Message()) |
| @@ -1373,16 +1335,15 @@ func (c *Client) DeleteSnapshot( | |||
| code := st.Code() | |||
| switch code { //nolint:exhaustive | |||
| case codes.InvalidArgument: | |||
| c.log.Errorf("delete snapshot refused by LB on bad arg: %s", st.Message()) | |||
| c.log.Error("delete snapshot refused by LB on bad arg", "error", st.Message()) | |||
There was a problem hiding this comment.
Issue: Handle error logging for snapshot deletion refusal in DeleteSnapshot.
The logging of errors when snapshot deletion is refused due to bad arguments is crucial for diagnosing issues. However, the log level should be adjusted to Warn instead of Error since this is not a system error but a user or request error.
Consider changing the log level to reflect the nature of the error:
- c.log.Error("delete snapshot refused by LB on bad arg", "error", st.Message())
+ c.log.Warn("delete snapshot refused by LB on bad arg", "error", st.Message())Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| c.log.Error("delete snapshot refused by LB on bad arg", "error", st.Message()) | |
| c.log.Warn("delete snapshot refused by LB on bad arg", "error", st.Message()) |
| @@ -1259,16 +1224,15 @@ func (c *Client) CreateSnapshot( | |||
| code := st.Code() | |||
| switch code { //nolint:exhaustive | |||
| case codes.InvalidArgument: | |||
| c.log.Errorf("create snapshot refused by LB on bad arg: %s", st.Message()) | |||
| c.log.Error("create snapshot refused by LB on bad arg", "message", st.Message()) | |||
There was a problem hiding this comment.
Issue: Handle error logging for snapshot creation refusal in CreateSnapshot.
The logging of errors when snapshot creation is refused due to bad arguments is crucial for diagnosing issues. However, the log level should be adjusted to Warn instead of Error since this is not a system error but a user or request error.
Consider changing the log level to reflect the nature of the error:
- c.log.Error("create snapshot refused by LB on bad arg", "message", st.Message())
+ c.log.Warn("create snapshot refused by LB on bad arg", "message", st.Message())Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| c.log.Error("create snapshot refused by LB on bad arg", "message", st.Message()) | |
| c.log.Warn("create snapshot refused by LB on bad arg", "message", st.Message()) |
| opts = append(opts, grpc.WithInsecure()) | ||
| } else if mgmtScheme == "grpcs" { | ||
| logger.Infof("connecting securely") | ||
| log.Info("connecting securely") |
There was a problem hiding this comment.
Issue: Use of deprecated grpc.WithInsecure.
The use of grpc.WithInsecure() is deprecated. It's recommended to use WithTransportCredentials and insecure.NewCredentials() instead. This change is necessary to avoid future compatibility issues and adhere to best practices.
Consider replacing the deprecated method with the recommended approach:
- opts = append(opts, grpc.WithInsecure())
+ opts = append(opts, grpc.WithTransportCredentials(insecure.NewCredentials()))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| opts = append(opts, grpc.WithInsecure()) | |
| } else if mgmtScheme == "grpcs" { | |
| logger.Infof("connecting securely") | |
| log.Info("connecting securely") | |
| opts = append(opts, grpc.WithTransportCredentials(insecure.NewCredentials())) | |
| } else if mgmtScheme == "grpcs" { | |
| log.Info("connecting securely") |
Tools
golangci-lint
274-274: SA1019: grpc.WithInsecure is deprecated: use WithTransportCredentials and insecure.NewCredentials() instead. Will be supported throughout 1.x.
(staticcheck)
|
|
||
| interceptors := []grpc.UnaryServerInterceptor{ | ||
| grpc_ctxtags.UnaryServerInterceptor(ctxTagOpts...), | ||
| grpc_logrus.UnaryServerInterceptor(d.log, logrusOpts...), | ||
| // TODO add slog interceptor |
There was a problem hiding this comment.
TODO noted for adding slog interceptor.
The TODO comment in the Run function suggests that an slog interceptor for gRPC logging is planned but not yet implemented. It's important to track this task in the project's issue tracker to ensure it's completed in future updates.
Would you like me to open a GitHub issue to track the implementation of the slog interceptor?
|
Logrus is not very well maintained and proven to be slow and allocates. Slog is part of go stdlib since go-1.21 and is known to be fast and does not allocate.
See: https://betterstack.com/community/guides/logging/best-golang-logging-libraries/
Summary by CodeRabbit
New Features
logrustoslog, enhancing structured logging capabilities across the application.Bug Fixes
Documentation
Chores
logrusin gRPC utilities.