Conversation
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
This PR adds a clean logger abstraction layer to the Go client, enabling applications to inject their own logging framework. The implementation is well-structured and the format string fixes address real bugs.
Review
Correctness ✅
- The
Loggerinterface is minimal and idiomatic Go. - Format string fixes are correct:
%w→%v(since%wis only valid infmt.Errorf),{}→%s/%d/%v(Go uses printf-style, not brace-style),%d→%sfor string clientID. - The
internalLoggeradapter correctly bridges the public interface to internal call sites.
Compatibility ✅
SetLogger/ResetLoggerare additive; existing code continues to work with the default Zap logger.- No breaking changes to public APIs.
Tests ✅
- Good coverage with a recording logger that verifies delegation.
- Race condition test included.
Documentation ✅
- README addition with a complete
log/slogadapter example is helpful.
One minor note: SetLogger panics on nil input. This is acceptable Go idiom (fail-fast), but a doc comment clarification like "Panics if logger is nil" could be added for discoverability.
LGTM.
Automated review by github-manager-bot
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Adds custom logger injection support to the Go client. Well-structured feature addition with clean API design.
Key changes reviewed:
- Framework-neutral
Loggerinterface withDebug/Info/Warn/Error/Withmethods - Global
SetLogger/ResetLoggerAPI for application-level logger configuration - Existing Zap-backed logger preserved as default via
internalLoggerwrapper - Producers and consumers inherit the configured logger and
client_idcontext - Fixes invalid
%wformat directives in log calls (Zap'sErrorfuses%v, not%w) - Documented
log/slogJSON adapter example in README - Thread-safe
SetLoggerwith mutex protection for concurrent access
Minor observations:
- The
SetLoggercall insideInitLogger()creates a slight recursion risk ifSetLoggerwere to callInitLogger— but the current implementation is safe (no circular call) - Test coverage includes
TestSetLoggerIsUsedByNewClientswith race detection
LGTM 👍
Automated review by github-manager-bot
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
This PR adds a framework-neutral Logger interface and SetLogger API, allowing applications to route RocketMQ logs through their existing logging framework. The implementation is well-designed:
Strengths:
- Clean public interface (
Logger) with minimal surface area (Debug/Info/Warn/Error/With) internalLoggerwrapper preserves backward compatibility with formatted methods (Debugf, Infof, etc.)- Correctly fixes invalid
%wformat directives →%v(since%wis only valid infmt.Errorf) - Good documentation with
log/slogadapter example in README - Tests included for the new logger functionality
SetLoggerpanics on nil input (fail-fast)
Minor observations:
- The
fatalLoggerinterface is defined but not used in the visible diff — consider documenting its intended use or removing if unused ResetLoggercomment could mention that it also restores environment variable configuration
Overall, this is a solid enhancement that improves the client's integration with application logging frameworks. LGTM.
Automated review by github-manager-bot
|
This PR has conflicts with the base branch and cannot be merged. Please rebase or merge the base branch into your branch and resolve the conflicts: git fetch origin
git checkout issue-1306-go-custom-logger
git rebase origin/master
# resolve conflicts, then:
git push --force-with-leaseThis is a one-time reminder. Feel free to @mention me for a re-review after conflicts are resolved. Automated notification by github-manager-bot |
Introduce a framework-neutral Logger interface and SetLogger API while preserving the existing Zap-backed default. Add producer and consumer inheritance coverage, JSON slog documentation, and correct invalid log format directives exposed by the adapter.
21a03ce to
f97284a
Compare
|
This PR was originally titled [ISSUE #1306] [Go] Support custom logger injection and was previously approved for that scope. Since the last review, 9 new commits have been added that expand the PR to include:
The PR title and description no longer match the actual changes. This makes it difficult for reviewers to understand the full scope and for maintainers to evaluate the change holistically. Recommendation: Please consider splitting this into separate, focused PRs:
This helps reviewers focus on one concern at a time and makes the git history clearer. Automated review by github-manager |
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Reviewed 14 changed file(s) in this PR.
Findings
- Critical: 0
- Warning: 0
- Info: 1
Please review the inline comments for specific suggestions.
Automated review by github-manager-bot
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed after new commits. The custom logger injection feature for the Go client is well-implemented:
- Clean
Loggerinterface withDebug/Info/Warn/Error/Withmethods SetLogger/ResetLoggerAPI for application-level logger injectioninternalLoggerwrapper preserves format-string compatibility- Format string fixes:
%wchanged to%vforErrorfcalls — correct since%wis only valid infmt.Errorf, not inSugaredLogger.Errorf - README documentation with a concrete
slogadapter example - Thread-safe by design via interface contract
LGTM.
Automated review by github-manager-bot
Which Issue(s) This PR Fixes
Fixes #1306
Brief Description
Loggerinterface and globalSetLoggerAPI.client_idcontext.log/slogJSON adapter example.How Did You Test This Change?
go buildgo test -vgo test ./...go test -race -run TestSetLoggerIsUsedByNewClients -count=1git diff --checkNote:
go vet ./...continues to report the existingcontext.WithTimeoutcancel warnings inclient_manager.goandconn.go; those warnings are present on the base commit and are outside this change.