feat: refactor logger module #122
Conversation
Flying-Tom
force-pushed
the
logger-refactor
branch
from
f293247 to
731b305
Compare
|
CI fix: success workflow |
|
apache/dubbo-go#2838 problaly need this PR to be merged, please review it. |
Flying-Tom
force-pushed
the
logger-refactor
branch
from
b39d32b to
143d9da
Compare
Flying-Tom
force-pushed
the
logger-refactor
branch
5 times, most recently
from
a1959bf to
374a74f
Compare
|
Error caused by the bump PR #118 |
Flying-Tom
force-pushed
the
logger-refactor
branch
4 times, most recently
from
98da2cb to
6d0125b
Compare
Flying-Tom
force-pushed
the
logger-refactor
branch
from
6d0125b to
4350380
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the logger module to adapt it for dubbo-go by updating the logger API and integrating logrus alongside zap. Key changes include:
- Renaming the internal logger field from "dynamicLevel" to "DynamicLevel" (making it exported).
- Modifying the SetLoggerLevel function to return a boolean status.
- Updating the go.mod file with the logrus dependency and adjusting import aliases in test files.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| time/sleep_test.go | Changed the import alias from the original package name to "gxlog". |
| log/logger/logger.go | Updated field naming and API for setting logger level with multiple backends. |
| go.mod | Added and adjusted the dependency for logrus. |
Comments suppressed due to low confidence (2)
log/logger/logger.go:36
- [nitpick] The renaming of 'dynamicLevel' to 'DynamicLevel' changes its visibility from unexported to exported. Please confirm that this change is intended and consistent with the module's design.
- dynamicLevel zap.AtomicLevel
+ DynamicLevel zap.AtomicLevel
time/sleep_test.go:33
- [nitpick] The change to use the alias 'gxlog' should be reviewed to ensure that all references in the test file are updated accordingly for consistency.
- "github.com/dubbogo/gost/log"
+ gxlog "github.com/dubbogo/gost/log"
WalkthroughThe changes introduce support for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Logger (OpsLogger)
participant zap.Logger
participant logrus.Logger
User->>Logger: SetLoggerLevel(level)
alt zap logger
Logger->>zap.Logger: Parse and set level
zap.Logger-->>Logger: Success/Failure
else logrus logger
Logger->>logrus.Logger: Parse and set level
logrus.Logger-->>Logger: Success/Failure
else Unsupported logger
Logger-->>User: Return false
end
Logger-->>User: Return true/false
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
♻️ Duplicate comments (2)
log/logger/logger.go (2)
37-37: Export DynamicLevel field for compatibility with dubbo-go.Changing from
dynamicLeveltoDynamicLevelexports the field, which appears necessary for compatibility with dubbo-go as mentioned in the PR objectives.
135-152: Approve the enhanced SetLoggerLevel implementation.The implementation now properly:
- Handles both zap and logrus logger types
- Provides appropriate error handling for level parsing
- Returns success/failure status for better error management
This improves compatibility with dubbo-go while maintaining robust error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
go.mod(1 hunks)log/logger/logger.go(4 hunks)time/sleep_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ubuntu-latest - Go1.23
- GitHub Check: ubuntu-latest - Go1.23
🔇 Additional comments (5)
time/sleep_test.go (1)
33-33: Approve the import alias addition.Adding the
gxlogalias for the log package is a good practice that makes it clear this is a custom logging package and avoids confusion with the standard library'slogpackage.log/logger/logger.go (4)
20-26: Approve the logrus import addition.Adding support for the logrus logging library is a good enhancement that provides more flexibility in logger implementations and aligns with the dubbo-go logger compatibility goal.
107-107: Approve the updated field initialization.The initialization correctly uses the renamed field
DynamicLevel.
121-126: Approve the return value addition to SetLoggerLevel function.Adding a boolean return value to indicate success/failure improves error handling and makes the API more robust.
129-132: Approve the interface update for consistency.Updating the
OpsLoggerinterface to match the implementation changes maintains consistency across the codebase.
| @@ -12,6 +12,7 @@ require ( | |||
| github.com/natefinch/lumberjack v2.0.0+incompatible | |||
| github.com/pkg/errors v0.9.1 | |||
| github.com/shirou/gopsutil/v3 v3.22.2 | |||
| github.com/sirupsen/logrus v1.7.0 | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Approve the logrus dependency move to direct requirements.
The change correctly moves github.com/sirupsen/logrus from indirect to direct requirements, which aligns with its new direct usage in the logger implementation.
🏁 Script executed:
#!/bin/bash
# Check if there's a newer version of logrus available
curl -s https://api.github.com/repos/sirupsen/logrus/releases/latest | grep -o '"tag_name": ".*"'
# Show comparison between v1.7.0 and latest
echo "Current version is v1.7.0 (from 2020). Consider upgrading if a newer stable version is available."Length of output: 325
Approve direct logrus dependency – consider upgrading to the latest stable
The change correctly promotes github.com/sirupsen/logrus to a direct requirement (line 15 in go.mod). However, the current version is v1.7.0 (released in 2020) and the latest stable is v1.9.3. To benefit from recent fixes and features, please bump the version in your go.mod:
• File: go.mod
- github.com/sirupsen/logrus v1.7.0
+ github.com/sirupsen/logrus v1.9.3📝 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.
| github.com/sirupsen/logrus v1.7.0 | |
| - github.com/sirupsen/logrus v1.7.0 | |
| + github.com/sirupsen/logrus v1.9.3 |
🤖 Prompt for AI Agents
In go.mod at line 15, the logrus dependency is correctly moved to a direct
requirement but is pinned to an outdated version v1.7.0. Update the version to
the latest stable release, v1.9.3, by changing the version number on line 15 to
v1.9.3 to ensure you get recent fixes and features.
What this PR does: refactor
gost/log/loggermodule to adapt todubbo-gologgerWhich issue(s) this PR fixes:
Fixes
dubbo-go#2526Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Summary by CodeRabbit
New Features
Refactor
Style