fix: use zap logger #184
fix: use zap logger #184
Conversation
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
WalkthroughThe changes in this pull request involve a comprehensive transition from the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
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: 4
🧹 Outside diff range and nitpick comments (8)
internal/cli/build/build.go (1)
64-70: LGTM with a minor suggestion: Final logging statements updated for Zap.The changes in this section are correct and consistent with the Zap logger's API:
zlog.Named("Builder")is used instead ofzlog.WithGroup("Builder").- Error logging has been updated to use
zap.Error(err).- Info logging has been updated to use
zap.String().These modifications ensure that the logging is consistent with the new framework.
Consider splitting the info log message into two separate fields for better parsing:
zlog.Info("Build finished successfully", zap.String("status", "success"), zap.String("binary_location", *out))This change would make it easier to parse and query the log output programmatically.
internal/cli/root.go (2)
64-68: LGTM: Improved logger initialization with error handling.The addition of error handling for logger initialization is a good improvement. It ensures that any issues during logger setup are caught and reported.
Consider wrapping the error for better context:
zlog, err := logger.BuildLogger(config.Log["level"], config.Log["mode"]) if err != nil { - return err + return fmt.Errorf("failed to build logger: %w", err) }This change would provide more context about where the error occurred, which can be helpful for debugging.
Line range hint
1-94: Summary: Logging changes implemented, but verification needed.The changes in this file successfully transition the logging mechanism to use the zap logger, which is consistent with the PR objectives. Error handling for logger initialization has been improved, which is a positive change.
However, it's important to note that these changes don't directly address the issue of base64 encoded log entries mentioned in the PR objectives and linked issue #2028. To ensure full resolution of the reported problem:
- Verify that the new logging implementation correctly outputs log entries without base64 encoding.
- Consider adding a test case that specifically checks for this behavior.
- Review other files in the PR to ensure that any log entry generation points are updated to use the new logging mechanism correctly.
Consider implementing a custom log encoder or formatter that ensures all log entries are human-readable and not base64 encoded. This might involve creating a custom
zapcore.Encoderthat handles any potential binary data appropriately.github/pool.go (2)
118-118: Consider using zap.Error for error loggingThe error logging call has been updated to use zap logger methods, which is correct. However, there are two points to consider:
- The log level has been changed from Error to Warn. Please verify if this change is intentional.
- For better error logging practices, consider using
zap.Error(err)instead ofzap.Any("error", err).Here's a suggested improvement:
-p.log.Warn("failed to close response body, continuing", zap.Any("error", err)) +p.log.Error("failed to close response body, continuing", zap.Error(err))
Line range hint
1-205: Summary: Successful transition to zap loggerThe changes in this file successfully implement the transition from
slogtozaplogger. All necessary updates have been made, including import statements, struct field types, function signatures, and logging calls. This transition addresses the PR objective of improving logging functionality.Regarding the linked issue #2028 about incorrectly outputted log entries as base64 encoded strings, this change lays the groundwork for improved logging. However, to fully address the issue, please ensure that:
- The zap logger is configured to output logs in a human-readable format rather than base64 encoded strings.
- Any custom log formatting or encoding is reviewed and adjusted if necessary.
To complete this transition:
- Verify that all callers of the
newPoolfunction have been updated to pass a zap logger.- Review the logger configuration to ensure it outputs logs in the desired format.
- Consider implementing a custom log encoder if needed to handle any special cases that were causing the base64 encoding issue.
If you need assistance with any of these steps, please let me know.
builder/builder_test.go (2)
84-85: LGTM: Zap logger initialized and used in NewBuilder.The changes correctly initialize a new zap logger and pass it to
NewBuilder, which aligns with the PR objective of transitioning to thezaplogger.Consider handling the error returned by
zap.NewDevelopment(). While ignoring it might be acceptable in a test setup, it's generally a good practice to handle errors, even in test code. You could usezap.NewExample()instead, which doesn't return an error, or handle the error like this:l, err := zap.NewDevelopment() if err != nil { t.Fatalf("Failed to create logger: %v", err) }
Logging Output Not Captured in Tests
The tests utilize the
zaplogger; however, no log entries were found during execution. This indicates that logging might not be correctly configured to output during testing.Actions Required:
- Verify that the
zaplogger inbuilder_test.gois configured to write logs tostdoutor another accessible destination.- Ensure that the logger is not being suppressed or redirected in a way that prevents log output during test runs.
Category:
🔗 Analysis chain
Line range hint
1-214: Verify log output format.The changes successfully transition the logging framework from
slogtozap. However, to fully address the issue mentioned in #2028 regarding incorrectly formatted log entries, it's important to verify that the log output is now correctly formatted and not base64 encoded.To verify the log output format, you can run the tests with verbose output and check the log entries. Here's a script to help with that:
This script runs the tests, captures the output, checks for potential base64 encoded entries, and displays a sample of log entries for manual verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Run tests and check log output format # Run tests with verbose output go test -v ./builder -run TestBuilder > test_output.log 2>&1 # Check for base64 encoded log entries if grep -q "log\":\"[A-Za-z0-9+/=]\{20,\}" test_output.log; then echo "WARNING: Possible base64 encoded log entries found." grep "log\":\"[A-Za-z0-9+/=]\{20,\}" test_output.log else echo "No base64 encoded log entries found." fi # Display a sample of log entries for manual verification echo "Sample log entries:" grep "level" test_output.log | head -n 5 # Clean up rm test_output.logLength of output: 289
Script:
#!/bin/bash # Description: Run tests and extract zap log entries for format verification # Run tests and capture all output go test -v ./builder -run TestBuilder > test_output.log 2>&1 # Extract log entries containing 'level' and 'msg' to verify zap formatting grep '"level":' test_output.log > extracted_logs.json # Check if any log entries were extracted if [ -s extracted_logs.json ]; then echo "Log entries found:" cat extracted_logs.json | head -n 10 else echo "No log entries containing 'level' found in the test output." fi # Clean up rm -f test_output.log extracted_logs.jsonLength of output: 285
Script:
#!/bin/bash # Description: Search for zap logger usage in the builder package and its tests # Search for 'zap.' usage in builder package echo "Searching for 'zap.' usage in builder package:" rg 'zap\.' ./builder # Search for logging statements in tests echo -e "\nSearching for logging statements in builder_test.go:" rg 'zap\.' ./builder/builder_test.go # Search for any remaining 'slog' usage to ensure it's fully replaced echo -e "\nVerifying no 'slog' usage remains in the builder package:" rg 'slog\.' ./builder # Summary echo -e "\nSummary:" slog_count=$(rg -c 'slog\.' ./builder) zap_count=$(rg -c 'zap\.' ./builder) echo "Total 'zap.' usages found: $zap_count" echo "Total 'slog.' usages found: $slog_count"Length of output: 2323
logger/logger.go (1)
132-137: Review the padding logic inColoredNameEncoderfor consistent formattingIn
ColoredNameEncoder, the function pads the service name with spaces to ensure a minimum length of 12 characters. However, if the service name exceeds 12 characters, the padding will not align the names in the log output, potentially causing misalignment.Consider using a more robust method for aligning service names, such as formatted strings with fixed widths or utilizing a tab writer for consistent alignment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
- .golangci.yml (1 hunks)
- builder/builder.go (14 hunks)
- builder/builder_test.go (2 hunks)
- github/pool.go (7 hunks)
- github/repo.go (8 hunks)
- gitlab/repo.go (6 hunks)
- go.mod (2 hunks)
- internal/cli/build/build.go (4 hunks)
- internal/cli/root.go (2 hunks)
- logger/logger.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (42)
internal/cli/build/build.go (6)
12-12: LGTM: Import statement for Zap logger added.The addition of the Zap logger import is consistent with the PR objective of fixing the logging issue. This change is necessary for the transition from
log/slogtogo.uber.org/zap.
34-34: LGTM: Logging statement updated for Zap in GitLab section.The change from
zlog.WithGroup("GITLAB")tozlog.Named("GITLAB")is correct and consistent with the Zap logger's API. This modification maintains the same logical structure of the log output while adapting to the new logging framework.
46-50: LGTM: Logging statements updated for Zap in GitHub section.The changes in this section are correct and consistent with the Zap logger's API:
zlog.Named("GITHUB")is used instead ofzlog.WithGroup("GITHUB").- Error logging has been updated to use
zap.Error(err), which is the proper way to log errors with Zap.These modifications ensure that the logging is consistent with the new framework while maintaining the same logical structure and error handling.
55-56: LGTM: Error logging updated for Zap in error handling section.The error logging statement has been correctly updated to use
zap.Error(err), which is the proper way to log errors with the Zap logger. This change ensures consistency with the new logging framework and maintains the same error handling logic.
Line range hint
1-74: Overall assessment: Successful transition to Zap logger.The changes in this file successfully implement the transition from
log/slogtogo.uber.org/zapfor logging. All modifications are consistent with the Zap logger's API and maintain the existing logging structure and error handling logic. These changes align well with the PR objective of fixing the logging issue.Key points:
- Import statement for Zap logger added.
- Function signature updated to use Zap logger.
- All logging statements (error, info, and named loggers) updated to use Zap's methods.
The implementation should resolve the issue of incorrectly outputted log entries mentioned in the PR objectives. However, to ensure full compatibility and prevent any regressions, consider the following:
- Run comprehensive tests to verify that all logging functionality works as expected with the new Zap logger.
- Check the log output in various scenarios to confirm that the log entries are no longer base64 encoded and are easily readable.
- Update any log parsing or analysis tools to accommodate the new log format produced by Zap.
#!/bin/bash # Description: Verify logging output is not base64 encoded # Test: Run the application and check its log output # Replace 'your_app' with the actual command to run your application your_app 2>&1 | tee app_output.log # Check if the output contains base64 encoded strings if grep -qP '(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?' app_output.log; then echo "Warning: Possible base64 encoded strings found in the log output." else echo "No base64 encoded strings detected in the log output." fi # Clean up rm app_output.logThis change significantly improves the logging mechanism and should enhance debugging capabilities as intended.
19-19: LGTM: Function signature updated to use Zap logger.The change from
*slog.Loggerto*zap.Loggerin theBindCommandfunction signature is consistent with the transition to the Zap logging framework. This change aligns with the PR objective.To ensure this change doesn't introduce issues elsewhere in the codebase, please run the following verification script:
✅ Verification successful
LGTM: Verification successful for Zap logger integration.
All references to
slog.Loggerhave been removed, and theBindCommandfunction calls have been appropriately updated to use*zap.Logger. The changes align with the migration to the Zap logging framework and do not introduce any issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to slog.Logger in the codebase # and verify that all calls to BindCommand have been updated. # Test 1: Search for any remaining references to slog.Logger echo "Searching for remaining references to slog.Logger:" rg --type go 'slog\.Logger' # Test 2: Verify that all calls to BindCommand have been updated echo "Checking calls to BindCommand:" rg --type go 'BindCommand\s*\([^)]*\)'Length of output: 440
Script:
#!/bin/bash # Description: Check for any remaining references to slog.Logger in the codebase # and verify that all calls to BindCommand have been updated. # Test 1: Search for any remaining references to slog.Logger echo "Searching for remaining references to slog.Logger:" rg --type go 'slog\.Logger' # Test 2: Verify that all calls to BindCommand have been updated echo "Checking calls to BindCommand:" rg --type go 'BindCommand\s*\([^)]*\)'Length of output: 427
go.mod (2)
14-14: Addition of zap logger aligns with PR objectivesThe addition of
go.uber.org/zap v1.27.0is in line with the PR objective of fixing the logging issue. Zap is a high-performance, structured logging library for Go, which should help address the base64 encoding problem mentioned in the linked issue #2028.To ensure that zap is being used correctly throughout the project, run the following script:
#!/bin/bash # Description: Check for proper usage of zap logger # Test: Search for zap logger usage rg --type go 'zap\.New|zap\.NewProduction|zap\.NewDevelopment' # Test: Check if there are any remaining uses of the old logger rg --type go 'log/slog' echo "Please review the output above to ensure zap logger is being used correctly and the old logger has been fully replaced."
3-3: Verify compatibility with Go 1.23.2The Go version has been updated from 1.23 to 1.23.2. While this is a minor version update and generally a good practice, please ensure that this version is compatible with all project dependencies and requirements.
To verify the compatibility, please run the following script:
✅ Verification successful
Verify Complete Replacement of Old Logger (
slog)Please run the following script to ensure that the old logger (
slog) has been fully removed from the codebase:
Go Version and Logger Update Verified Successfully
- Go Version: The update to Go 1.23.2 is compatible with all project dependencies.
- Logging Library: The
zaplogger has been correctly integrated, and all instances of the oldsloglogger have been fully removed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if all dependencies are compatible with Go 1.23.2 # Test: Run go mod tidy and check for any compatibility issues go mod tidy if [ $? -ne 0 ]; then echo "Compatibility issues detected. Please review the output above." else echo "No compatibility issues detected with Go 1.23.2" fiLength of output: 810
Script:
# #!/bin/bash # Description: Check for any remaining usages of the old logger 'slog' rg --type go 'slog\.' || echo "No remaining 'slog' usages found."Length of output: 106
internal/cli/root.go (2)
14-14: LGTM: Import of zap logger added.The addition of the
"go.uber.org/zap"import is consistent with the PR objective of transitioning to the zap logger. This change is a necessary step in improving the logging functionality as described in the PR summary.
70-70: LGTM: Logger assignment after successful initialization.The assignment of the new logger to
lgafter successful initialization and error checking is correct.However, to ensure that this change addresses the issue mentioned in the PR objectives (incorrect output of log entries as base64 encoded strings), please verify the log output format. Run the following script to check for any remaining base64 encoded log entries:
If any results are found, they may indicate areas that still need attention to fully resolve the logging issue.
gitlab/repo.go (7)
16-16: LGTM: Zap logger import added correctly.The addition of the
go.uber.org/zapimport is consistent with the PR objective of switching to the zap logger. This change is necessary for the subsequent modifications in the file.
25-25: LGTM: GLRepo struct updated to use zap logger.The
logfield in theGLRepostruct has been correctly updated to use*zap.Logger. This change is consistent with the PR objective and ensures that the struct uses the new logging framework.
28-28: LGTM: NewGLRepoInfo function signature updated correctly.The
NewGLRepoInfofunction signature has been properly updated to accept a*zap.Loggerinstead of a*slog.Logger. This change is consistent with the PR objective and ensures compatibility with the updatedGLRepostruct.
46-46: LGTM: Logging statement updated to use zap logger correctly.The logging statement has been properly updated to use zap's
Debugmethod with structured logging. The change maintains the original log level and information while adopting zap's efficient key-value pair approach withzap.String.
73-73: LGTM: Second logging statement updated correctly.This logging statement has also been properly updated to use zap's
Debugmethod with structured logging. The change maintains the original log level and information while correctly implementing zap's key-value pair approach.
88-88: LGTM: Remaining logging statements updated correctly.The remaining logging statements on lines 88 and 126 have been properly updated to use zap's
Debugmethod with structured logging. These changes maintain the original log levels and information while correctly implementing zap's key-value pair approach.Also applies to: 126-126
Line range hint
1-140: Summary: Successful migration to zap logger with no apparent issues.The changes in this file successfully migrate the logging from slog to zap logger. All necessary updates have been made:
- Import statement added for zap
GLRepostruct updated to use zap loggerNewGLRepoInfofunction signature updated- All logging statements in
GetPluginsModDatamethod updated to use zap's structured loggingThe core logic of the functions remains unchanged, and the
toPtrfunction is unaffected. These changes align well with the PR objective of fixing the logging issue and should contribute to resolving the problem of incorrectly outputted log entries.However, to fully address the issue mentioned in #2028 regarding base64 encoded log entries, we may need to verify if any additional changes are required in other parts of the codebase or in the logger configuration.
To ensure that the logging issue is fully resolved, we should verify the logger configuration and output format. Run the following script to check for any remaining base64 encoding in the logs:
✅ Verification successful
Logging migration to Zap successful with no base64 encoding detected.
The migration from slog to zap logger has been successfully implemented across the codebase. Verification indicates that:
- No base64 encoding functions are used in the codebase.
- All logger configurations utilize zap.
- No remaining usage of slog logger is detected.
These results confirm that the PR fully addresses the logging issues without introducing base64 encoded log entries.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for base64 encoded log entries in the codebase # Search for base64 encoding functions in the entire codebase echo "Searching for base64 encoding functions:" rg 'base64\.StdEncoding\.Encode|base64\.StdEncoding\.EncodeToString' --type go # Search for zap logger configuration echo "\nSearching for zap logger configuration:" rg 'zap\.Config|zap\.New|zap\.NewProduction|zap\.NewDevelopment' --type go # Search for any remaining slog usage echo "\nSearching for any remaining slog usage:" rg 'slog\.' --type goLength of output: 1121
.golangci.yml (2)
Line range hint
1-38: Clarification needed: Relevance of linter change to PR objectivesWhile the update from 'exportloopref' to 'copyloopvar' is a good improvement for the linting process, it's not clear how this change relates to the PR objectives of fixing logging issues and base64 encoded log entries. Could you please clarify the relevance of this change to the main goals of the PR? Is this part of a larger refactoring effort not mentioned in the PR description?
38-38: Linter update: Approve the change from 'exportloopref' to 'copyloopvar'The replacement of 'exportloopref' with 'copyloopvar' in the linters section is a good update. 'copyloopvar' is a newer linter that checks for pointers to enclosing loop variables, which can help catch potential issues in the codebase.
To ensure this change doesn't introduce any new linting errors, please run the following command in your local development environment:
This will verify that the new linter doesn't flag any issues in the existing codebase. If it does, those issues should be addressed before merging this PR.
github/pool.go (6)
16-16: LGTM: Correct import for zap loggerThe import statement for
go.uber.org/zapis correctly added, which is necessary for the transition to the zap logging framework.
29-29: LGTM: Logger type updated in processor structThe
logfield type in theprocessorstruct is correctly updated from*slog.Loggerto*zap.Logger. This change is consistent with the transition to the zap logging framework and will affect all methods using thelogfield.
64-68: LGTM: Logging calls updated to use zap loggerThe logging calls within the
runmethod have been correctly updated to use zap logger methods. The log level (Debug) and fields (String) are properly translated from slog to zap.
96-96: LGTM: Logging call updated to use zap loggerThe logging call within the
out:label block has been correctly updated to use zap logger methods. The log level (Debug) and fields (String) are properly translated from slog to zap.
158-158: LGTM: Logging call updated to use zap loggerThe logging call within the replace block has been correctly updated to use zap logger methods. The log level (Debug) and fields (String) are properly translated from slog to zap.
40-40: LGTM: newPool function signature updatedThe
newPoolfunction signature is correctly updated to accept*zap.Loggerinstead of*slog.Logger. This change is consistent with the transition to the zap logging framework.Please ensure that all callers of the
newPoolfunction have been updated to pass a zap logger. Run the following script to verify:github/repo.go (7)
18-18: LGTM: Zap logger import added.The addition of the zap logger import is consistent with the PR objective of transitioning to the zap logging framework.
34-34: LGTM: Logger field type updated in GHRepo struct.The
logfield type has been correctly updated to*zap.Logger, which aligns with the transition to the zap logging framework.
37-37: LGTM: NewGHRepoInfo function signature updated.The function signature has been correctly updated to accept a
*zap.Logger, which is consistent with the transition to the zap logging framework.
56-56: LGTM: Logging calls updated to use zap logger in DownloadTemplate method.The logging calls have been successfully transitioned from slog to zap, maintaining consistent log levels, messages, and structured logging practices. This change aligns with the PR objective and improves the logging mechanism.
Also applies to: 66-66, 73-73, 88-88, 103-103
149-149: LGTM: Logging call updated to use zap logger in extract function.The logging call has been successfully transitioned to use the zap logger, maintaining the log level, message, and structured logging practice. This change is consistent with the overall transition to the zap logging framework.
204-204: LGTM: Enhanced logging context in GetPluginsModData method.The use of
log.Named("pool")is a good improvement. It creates a new logger with an added name segment, which will provide more context in the logs from the pool executor. This change not only aligns with the transition to zap but also enhances the overall logging structure.
Line range hint
1-224: Summary: Successful transition to zap logger with improved logging context.The changes in this file consistently implement the transition from slog to zap logger, aligning with the PR objective. Key points:
- All logger type references and import statements have been updated.
- Logging calls have been modified to use zap methods while maintaining the same log levels and messages.
- Structured logging practices have been preserved.
- The use of
log.Named()in the GetPluginsModData method adds improved context to logs.These changes should address the issue of incorrectly outputted log entries (as mentioned in the linked issue #2028) by ensuring proper conversion of raw payloads using the updated logger.
To ensure all logger transitions are complete, run the following script:
✅ Verification successful
Verification Successful: All slog imports and usages have been removed, and zap logger is consistently implemented across the codebase.
Key confirmations:
- No remaining imports of
log/slog.- No lingering usages of
slogin the code.zap.Loggeris actively used in multiple relevant files, ensuring a complete and consistent transition.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all slog imports and usages have been replaced with zap # Test 1: Check for any remaining slog imports echo "Checking for remaining slog imports:" rg --type go 'import.*"log/slog"' # Test 2: Check for any remaining slog usage echo "Checking for remaining slog usage:" rg --type go '\bslog\.' # Test 3: Verify zap logger is used consistently echo "Verifying consistent use of zap logger:" rg --type go 'zap\.Logger'Length of output: 1078
builder/builder_test.go (1)
9-9: LGTM: Import statement for zap logger added.The import statement for
go.uber.org/zapis correctly added, which aligns with the PR objective of transitioning to thezaplogger.builder/builder.go (8)
19-19: LGTM: Import statement updated for zap logger.The import statement has been correctly updated to include the zap logger package, which is consistent with the switch from slog to zap logging.
102-102: LGTM: Logging calls updated to use zap's structured logging format.The logging calls have been correctly updated to use zap's structured logging format. This change addresses the issue mentioned in the PR objectives about incorrectly outputting log entries. The use of
zap.String()for key-value pairs ensures that the log entries will be properly formatted and not encoded as base64 strings.Also applies to: 116-116, 166-166
273-273: LGTM: Logging call in goBuildCmd updated to use zap's structured logging format.The logging call has been correctly updated to use zap's structured logging format. This change is consistent with the overall switch to zap logging and addresses the issue of incorrectly outputting log entries.
288-288: LGTM: Logging calls in goModDowloadCmd and goModTidyCmd updated to use zap's structured logging format.The logging calls in both
goModDowloadCmdandgoModTidyCmdmethods have been correctly updated to use zap's structured logging format. These changes are consistent with the overall switch to zap logging and address the issue of incorrectly outputting log entries.Also applies to: 303-303
318-318: LGTM: Logging calls in getDepsReplace updated to use zap's structured logging format.The logging calls in the
getDepsReplacemethod have been correctly updated to use zap's structured logging format. These changes are consistent with the overall switch to zap logging and address the issue of incorrectly outputting log entries.Also applies to: 329-329
Line range hint
1-377: Overall assessment: Changes successfully implement zap logging and address PR objectives.The changes in this file consistently implement the switch from slog to zap logging across all relevant parts of the code. This includes updates to the
Builderstruct, theNewBuilderfunction, and all logging calls throughout the file. The use of zap's structured logging format (zap.String()for key-value pairs) ensures that log entries will be properly formatted and not encoded as base64 strings, directly addressing the issue mentioned in the PR objectives.These changes should resolve the problem of Velox incorrectly outputting log entries as base64 encoded strings, making debugging easier for users. The implementation is thorough and consistent, maintaining the overall structure and logic of the code while enhancing the logging functionality.
46-46: LGTM: NewBuilder function signature updated for zap logger.The
NewBuilderfunction signature has been correctly updated to accept*zap.Loggerinstead of*slog.Logger.To ensure this change doesn't introduce any issues, let's verify the usage of the
NewBuilderfunction across the codebase:#!/bin/bash # Search for any calls to NewBuilder to ensure they're passing the correct logger type rg --type go 'NewBuilder\('
41-41: LGTM: Builder struct updated for zap logger.The
Builderstruct has been correctly updated to use*zap.Loggerinstead of*slog.Logger.To ensure this change doesn't introduce any issues, let's verify the usage of the
Builderstruct across the codebase:logger/logger.go (1)
23-111: Logger configuration is well-structured and leverages zap's capabilities effectivelyThe
BuildLoggerfunction correctly configures the logger based on the specified mode and level. The use of custom encoders enhances the log output for better readability in different environments.
Reason for This PR
closes: roadrunner-server/roadrunner#2028
Description of Changes
License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
Release Notes
New Features
zaplogging library across multiple components.Bug Fixes
Documentation
Chores
zaplibrary.