Repository navigation
Conversation
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by RockteMQ-AI
Summary
This PR adds a GRPCLogger field to the Go client Config, allowing users to inject a custom *zap.Logger for gRPC call logging. When nil, the default logger is used; when zap.NewNop(), gRPC logging is effectively disabled.
Findings
-
[Info]
config.go— The Javadoc-style comment onGRPCLoggeris clear and correctly documents ownership semantics ("The caller owns the logger and is responsible for syncing it"). Good practice. -
[Info]
client_manager.go:125— ThegrpcLoggerfield ondefaultClientManageris set directly from the config instartUp(). The nil-check inrunRpcJob(if cm.grpcLogger != nil) correctly handles the default case where no custom logger is provided. -
[Info]
client_manager.go:471-474— The logger is propagated viaWithRpcClientConnOption(WithZapLogger(cm.grpcLogger))only when non-nil. This means the default logger path is unchanged, preserving backward compatibility. -
[Info]
client_manager_test.go— The test covers three cases:nil(default), custom logger, andzap.NewNop()(disabled). The table-driven approach and the mock factory that captures the logger pointer are well-structured. The test verifies all three connection paths (initial, second endpoint, and recreated) receive the correct logger.
Suggestions
- Consider whether other language SDKs (Java, C++, Rust) should expose a similar configurable gRPC logger for consistency. This could be tracked as a follow-up issue.
- The
grpcLoggerfield is only set duringstartUp(). If the config is mutated after startup, the change won't take effect. This is acceptable for the current design but worth documenting.
Verdict
Clean, minimal change with good test coverage. The feature is useful for debugging gRPC issues in production and the implementation preserves backward compatibility.
Automated review by RockteMQ-AI
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Adds an optional Config.GRPCLogger field to the Go SDK, allowing callers to inject a custom *zap.Logger for gRPC call logging. Nil preserves default behavior; zap.NewNop() disables logging. Clean, minimal change with correct propagation through the client manager.
Findings
-
[Info]
client.go:691— Thecm.grpcLogger = cli.config.GRPCLoggerassignment is placed afterNewDefaultClientManager()but beforecm.startUp(). This is the correct position sincestartUp()triggers RPC connection establishment viarunRpcJob(), which is where the logger is consumed. The happens-before relationship between the write instartUp()and reads in spawned goroutines ensures thread safety without additional synchronization. -
[Info]
client_manager.go:471-474— The conditional propagation (if cm.grpcLogger != nil) correctly preserves the default logger path when no custom logger is configured. The use ofWithRpcClientConnOption(WithZapLogger(...))reuses the existing option mechanism, avoiding any new plumbing. -
[Info]
config.go— The field documentation clearly states ownership semantics ("The caller owns the logger and is responsible for syncing it") and thezap.NewNop()escape hatch. Good API design. -
[Info]
client_manager_test.go— The table-driven test covers all three cases (nil/default, custom, disabled) and verifies logger propagation across three connection paths (initial, additional endpoint, recreated). The pointer-identity check (got != want) is appropriate here since the logger is passed by reference and should be the exact same instance.
Suggestions
-
The
grpcLoggerfield is set once duringstartUp()and never changes. If a future need arises to update the logger at runtime (e.g. dynamic log level changes), async.atomic.Valueor mutex-protected setter would be needed. Not required now, but worth keeping in mind for the design contract. -
Consider whether the
GRPCLoggerconfig should be validated in theConfigvalidation method (if one exists) to reject non-nil but already-closed loggers. Low priority sincezap.Loggerdoes not expose a "closed" state.
Verdict
Clean, backward-compatible addition. The implementation is minimal and well-tested.
Automated review by github-manager-bot
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Allows users to configure a custom gRPC call logger for the Go client, enabling better integration with application logging frameworks.
Findings
- [Info] config.go:68-72 — Clean addition of GRPCLogger field to Config. The WithGRPCLogger option follows the existing functional options pattern.
- [Info] client_manager.go:125 — The grpcLogger field is properly integrated into the client manager. The nil check before passing to WithRpcClientConnOption is correct.
- [Info] client.go:691 — The logger is correctly propagated from config to client manager during startup.
- [Info] client_manager_test.go — Comprehensive tests verify the logger is properly passed through the RPC client options.
Verdict
LGTM. Clean implementation with good test coverage. This improves the Go client integration story.
Automated review by github-manager-bot
Which Issue(s) This PR Fixes
No linked issue.
Brief Description
Go SDK gRPC call logs use a default file logger at
/tmp/rocketmq-logger.log, while producer and consumer configuration does not expose a way to replace it.Add an optional
Config.GRPCLoggerfield and pass it through the client manager to RPC connections using the existingWithZapLoggeroption. Callers can supply their application's Zap logger, configure file rotation withpkg/zaplog, or disable gRPC call logging withzap.NewNop(). A nil value preserves the default behavior. Recreated connections retain the configured logger, and the caller owns its lifecycle.How Did You Test This Change?
cd golang && go test ./...cd golang && go test -race . -run '^TestClientGRPCLogger$' -count=1