fix(server/v2): use only one logger for app and server #22241
Conversation
julienrbrt
requested review from
kocubinski,
a team and
hieuvubk
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request focus on simplifying the command execution and server initialization processes within the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
|
@julienrbrt your pull request is missing a changelog! |
julienrbrt
added
the
backport/v0.52.x
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
server/v2/util.go (2)
33-41: LGTM with a minor suggestion.The new
GetViperFromContextfunction is well-implemented and follows good practices. It correctly retrieves the viper instance from the context and includes appropriate error handling.Consider using a more idiomatic Go approach for the type assertion:
v, ok := ctx.Value(corectx.ViperContextKey).(*viper.Viper) if !ok { panic(fmt.Sprintf("incorrect viper type %T: expected *viper.Viper. Have you forgot to set the viper in the context?", ctx.Value(corectx.ViperContextKey))) } return vThis change reduces the number of variables and makes the code slightly more concise.
48-57: LGTM with a minor suggestion.The new
GetLoggerFromContextfunction is well-implemented and consistent with theGetViperFromContextfunction. It correctly retrieves the logger instance from the context and includes appropriate error handling.Similar to the suggestion for
GetViperFromContext, consider using a more idiomatic Go approach for the type assertion:logger, ok := ctx.Value(corectx.LoggerContextKey).(log.Logger) if !ok { panic(fmt.Sprintf("incorrect logger type %T: expected log.Logger. Have you forgot to set the logger in the context?", ctx.Value(corectx.LoggerContextKey))) } return loggerThis change reduces the number of variables and makes the code slightly more concise.
simapp/v2/simdv2/cmd/config.go (1)
82-82: Approved: Log level configuration updated appropriately.The addition of "serverv2:info" to the log level configuration aligns with the PR objectives to address logging issues for server components. This change will enable informational logging for the serverv2 component while maintaining the existing log levels for other components, which should aid in debugging and monitoring.
For improved readability, consider using a multi-line string literal for the log level configuration. For example:
cfg.LogLevel = ` *:warn serverv2:info p2p:info state:info `This format makes it easier to read and modify individual log level settings in the future.
simapp/v2/simdv2/cmd/commands.go (1)
Line range hint
68-77: Approve changes with a minor suggestion for error handlingThe changes align with the PR objective of using a single logger for both app and server. The removal of the
loggerparameter from theserverv2.AddCommandsfunction call is appropriate.The error handling has been simplified, which improves code readability. However, we can further improve it by using a more idiomatic Go error handling pattern.
Consider updating the error handling to use the following pattern:
if err := serverv2.AddCommands( rootCmd, newApp, initServerConfig(), cometbft.New( &genericTxDecoder[T]{txConfig}, initCometOptions[T](), initCometConfig(), ), grpc.New[T](), serverstore.New[T](), telemetry.New[T](), ); err != nil { return err // or log.Fatal(err) depending on the function signature }This approach avoids the use of
panicand allows for more graceful error handling or propagation.server/v2/commands.go (2)
46-46: LGTM! Consider enhancing error handling.The removal of the
loggerparameter fromNewServercall aligns well with the PR objective of using a single logger. This simplification improves code clarity and maintainability.Consider adding error handling for the
NewServercall:server, err := NewServer(globalServerCfg, components...) if err != nil { return fmt.Errorf("failed to create new server: %w", err) }
171-176: LGTM! Consider enhancing error logging.The renaming of
logtologgerand the use ofNewLoggerfunction improve code clarity and align with the PR objectives.Consider logging the error before returning it:
logger, err := NewLogger(v, cmd.OutOrStdout()) if err != nil { fmt.Fprintf(cmd.ErrOrStderr(), "Failed to create logger: %v\n", err) return err }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (7)
- server/v2/commands.go (2 hunks)
- server/v2/server.go (3 hunks)
- server/v2/server_test.go (0 hunks)
- server/v2/util.go (1 hunks)
- simapp/v2/simdv2/cmd/commands.go (1 hunks)
- simapp/v2/simdv2/cmd/config.go (1 hunks)
- simapp/v2/simdv2/cmd/testnet.go (1 hunks)
💤 Files with no reviewable changes (1)
- server/v2/server_test.go
🧰 Additional context used
📓 Path-based instructions (6)
server/v2/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/util.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/testnet.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (8)
server/v2/util.go (3)
43-46: LGTM! Good refactoring.The modification to
GetViperFromCmdis a good improvement. It now leverages theGetViperFromContextfunction, promoting code reuse and maintaining a single source of truth for retrieving the viper instance from the context.
59-62: LGTM! Consistent refactoring.The modification to
GetLoggerFromCmdis a good improvement, consistent with the changes made toGetViperFromCmd. It now leverages theGetLoggerFromContextfunction, promoting code reuse and maintaining a single source of truth for retrieving the logger instance from the context.
33-62: Overall, excellent refactoring that improves code structure and consistency.The changes in this file successfully address the PR objectives by consolidating the logging mechanism and improving consistency in handling context values. The new functions
GetViperFromContextandGetLoggerFromContextprovide a centralized way to retrieve these instances, which should help in properly respecting command line flags for logging format.The refactoring of
GetViperFromCmdandGetLoggerFromCmdto use these new functions promotes code reuse and maintainability. These changes are likely to resolve the issue described in #22237 by ensuring that server components are properly configured to respect command line flags.Great job on improving the overall structure and consistency of the code!
server/v2/commands.go (1)
Line range hint
1-211: Update documentation and verify test coverage.The changes successfully address the issue of using a single logger for the app and server, aligning well with the PR objectives. The code structure is good and follows the Uber Golang style guide.
Please ensure that:
- The package documentation is updated to reflect the changes in logger handling.
- Any existing tests are updated to accommodate the new logger initialization process.
To verify test coverage, run the following script:
server/v2/server.go (3)
107-108: LGTM: Consistent logging approach in Stop method.The changes to the
Stopmethod mirror those in theStartmethod, maintaining a consistent approach to logging across theServerstruct's methods. This consistency improves code readability and maintainability.
86-87: LGTM: Logging now uses context-based logger.The change to use
GetLoggerFromContext(ctx)instead of a struct-level logger improves flexibility and context-awareness in logging. The addition of the module key enhances log clarity.To ensure the
GetLoggerFromContextfunction is properly implemented, please run:rg -A 10 'func GetLoggerFromContext'This will help verify that the function correctly retrieves the logger from the context.
59-59: LGTM: Server name updated to reflect version.The change from "server" to "serverv2" is consistent with the package name. This update might affect areas where the server name is used, such as logging or configuration.
To ensure this change doesn't introduce any inconsistencies, please run the following command to check for any remaining occurrences of the old server name:
✅ Verification successful
Verified: Server name updated correctly.
The change from
"server"to"serverv2"inserver/v2/server.gohas been consistently applied to all relevant instances ofserverName. No residual references to the old server name"server"were found, ensuring uniformity across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg '"server"' --type goLength of output: 598
Script:
#!/bin/bash # Search for any occurrences of 'server' in Go files rg 'server' --type goLength of output: 169194
Script:
#!/bin/bash # Search for all occurrences of 'serverName' in Go files rg 'serverName' --type goLength of output: 172
simapp/v2/simdv2/cmd/testnet.go (1)
346-346: LGTM! Simplified server initialization.The removal of the logger parameter from
serverv2.NewServeraligns with the PR objective of using a single logger for both app and server. This change simplifies the server initialization process.To ensure that logging functionality is still properly configured and accessible, please run the following verification script:
✅ Verification successful
Verified: Logging Configuration Intact
The removal of the
loggerparameter fromserverv2.NewServerhas been successfully verified. There are no remaining references to the old logging method, and the new logging implementation is consistently used throughout theserver/v2/directory.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that logging is still properly configured after the change. # Test 1: Check if there are any remaining references to the old logging method echo "Checking for old logging references:" rg --type go 'NewServer.*logger' # Test 2: Verify that the new logging method is properly implemented echo "Verifying new logging implementation:" rg --type go 'log\.' server/v2/ # Test 3: Check for any TODO or FIXME comments related to logging echo "Checking for TODO or FIXME comments related to logging:" rg --type go -e 'TODO.*log' -e 'FIXME.*log'Length of output: 6761
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
server/v2/server_test.go (2)
68-70: LGTM with a minor suggestion for error handlingThe changes align well with the PR objective of consolidating logging. The use of
SetServerContextis appropriate here.A minor suggestion to improve error handling:
- ctx, err := serverv2.SetServerContext(context.Background(), v, logger) - require.NoError(t, err) + ctx, err := serverv2.SetServerContext(context.Background(), v, logger) + require.NoError(t, err, "Failed to set server context")Adding an error message to
require.NoErrorcan help with debugging if the test fails.
Line range hint
41-117: Suggestion: Add test case for new logging behaviorThe changes to the
TestServerfunction are consistent with the PR objective of using a single logger for both the app and server. However, to ensure comprehensive test coverage, consider adding a specific test case that verifies the new logging behavior.For example, you could add a test that:
- Captures the log output.
- Performs an operation that triggers logging.
- Verifies that the log output contains the expected content and format.
This would help ensure that the logging changes are working as intended and provide better test coverage for the modifications made in this PR.
server/v2/util.go (3)
16-26: LGTM with a minor suggestion for improvementThe new
SetServerContextfunction is well-implemented and follows good practices. It properly handles nil contexts and clearly sets both logger and viper in the context.Consider adding error checking for nil
viperandloggerparameters to make the function more robust:func SetServerContext(ctx context.Context, viper *viper.Viper, logger log.Logger) (context.Context, error) { if ctx == nil { ctx = context.Background() } + if viper == nil { + return nil, errors.New("viper cannot be nil") + } + if logger == nil { + return nil, errors.New("logger cannot be nil") + } ctx = context.WithValue(ctx, corectx.LoggerContextKey, logger) ctx = context.WithValue(ctx, corectx.ViperContextKey, viper) return ctx, nil }
40-48: LGTM with a suggestion for improved error handlingThe new
GetViperFromContextfunction is well-implemented. It correctly retrieves the viper instance from the context and includes an informative panic message if the viper is not set or of incorrect type.Consider using a multi-value type assertion to avoid a potential panic if the context value is nil:
func GetViperFromContext(ctx context.Context) *viper.Viper { value := ctx.Value(corectx.ViperContextKey) - v, ok := value.(*viper.Viper) + v, ok := value.(*viper.Viper) if !ok { - panic(fmt.Sprintf("incorrect viper type %T: expected *viper.Viper. Have you forgot to set the viper in the context?", value)) + panic(fmt.Sprintf("incorrect or missing viper: expected *viper.Viper, got %T. Have you forgot to set the viper in the context?", value)) } return v }
55-69: LGTM with a suggestion for improved error handlingThe new
GetLoggerFromContextfunction and the changes toGetLoggerFromCmdare well-implemented. They follow a consistent pattern with their viper counterparts, which improves code readability and maintainability.For
GetLoggerFromContext, consider using a multi-value type assertion to avoid a potential panic if the context value is nil:func GetLoggerFromContext(ctx context.Context) log.Logger { v := ctx.Value(corectx.LoggerContextKey) - logger, ok := v.(log.Logger) + logger, ok := v.(log.Logger) if !ok { - panic(fmt.Sprintf("incorrect logger type %T: expected log.Logger. Have you forgot to set the logger in the context?", v)) + panic(fmt.Sprintf("incorrect or missing logger: expected log.Logger, got %T. Have you forgot to set the logger in the context?", v)) } return logger }The changes to
GetLoggerFromCmdlook good and align well with the PR objectives of consolidating logging mechanisms.server/v2/server.go (2)
86-87: LGTM: Updated logger usage in Start methodThe changes to retrieve the logger from the context and include the module key in the log message are good improvements. They align with the PR objective and enhance log clarity.
Consider adding an error log message before returning the error in the
Startmethod. This would provide more context in the logs when a start failure occurs:if err := g.Wait(); err != nil { + logger.With(log.ModuleKey, s.Name()).Error("failed to start servers", "err", err) return fmt.Errorf("failed to start servers: %w", err) }
107-108: LGTM: Updated logger usage in Stop methodThe changes to retrieve the logger from the context and include the module key in the log message are good improvements. They align with the PR objective and enhance log clarity.
Consider adding an error log message before returning the error in the
Stopmethod. This would provide more context in the logs when a stop failure occurs:- return g.Wait() + if err := g.Wait(); err != nil { + logger.With(log.ModuleKey, s.Name()).Error("failed to stop servers", "err", err) + return err + } + return nil
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
- server/v2/server.go (2 hunks)
- server/v2/server_test.go (2 hunks)
- server/v2/util.go (1 hunks)
- simapp/v2/simdv2/cmd/config.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- simapp/v2/simdv2/cmd/config.go
🧰 Additional context used
📓 Path-based instructions (3)
server/v2/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/server_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"server/v2/util.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (6)
server/v2/server_test.go (1)
103-103: LGTM: Proper context usageThe update to use the existing
ctxinstead ofcontext.TODO()is correct. This change ensures that the cancellation will properly propagate through the entire test, which is important for proper resource management and test behavior.server/v2/util.go (2)
28-38: LGTM! Good refactoringThe changes to
SetCmdServerContextare well-implemented. The function now leverages the newSetServerContextfunction, which simplifies the code and improves consistency. Error handling is properly implemented, and the function's purpose remains clear.
50-53: LGTM! Good refactoringThe changes to
GetViperFromCmdare well-implemented. The function now leverages the newGetViperFromContextfunction, which simplifies the code and improves consistency. This refactoring aligns well with the PR objectives of consolidating viper retrieval mechanisms.server/v2/server.go (3)
86-87: LGTM: Updated server name constantThe change from "server" to "serverv2" in the
serverNameconstant accurately reflects the version update. This improves clarity without affecting functionality.
Line range hint
1-268: Summary: Improved logging consistency and simplified server structureThe changes in this file successfully implement the PR objective of using a single logger for the app and server. Key improvements include:
- Updating the
serverNameconstant for clarity.- Removing the
loggerfield from theServerstruct and theloggerparameter from theNewServerfunction.- Modifying the
StartandStopmethods to use a logger from the context.- Adding module keys to log messages for better context.
These changes enhance logging consistency and simplify the server structure. However, it's important to ensure that these modifications don't introduce issues in other parts of the codebase that may have depended on the previous logging structure.
To ensure the changes don't introduce unintended side effects, please run the following verification:
#!/bin/bash # Description: Check for any remaining references to the removed logger field # Test: Search for any remaining references to s.logger echo "Checking for references to s.logger in the codebase:" rg --type go 's\.logger' . # Test: Search for any remaining NewServer calls with a logger parameter echo "Checking for NewServer calls with a logger parameter:" rg --type go 'NewServer\([^)]*logger[^)]*\)' .
86-87: LGTM: Removed logger field and parameterThe removal of the
loggerfield from theServerstruct and theloggerparameter from theNewServerfunction aligns with the PR objective of using a single logger. This simplification is a good step towards consistent logging.To ensure this change doesn't introduce any issues, please verify logger usage throughout the codebase:
(cherry picked from commit 1f941bb) # Conflicts: # server/v2/commands.go # server/v2/server.go # server/v2/server_test.go # server/v2/util.go
…) (#22249) Co-authored-by: Julien Robert <[email protected]>
Description
Closes: #22237
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores