[ISSUE #10672] Sanitize proxy telemetry failure logs - #10673
Conversation
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Replaces full protobuf TextFormat.shortDebugString(command) logging in GrpcClientChannel.writeTelemetryCommand with a compact summarizeTelemetryCommand() method that logs only diagnostic-relevant fields (command type, nonce, transaction ID, client type, channel context) without exposing message payloads. Also adds missing channel context to the catch-block warning.
Findings
-
[Info]
GrpcClientChannel.java:289— The<<<TRUNCATED>>>marker inRECOVER_ORPHANED_TRANSACTION_COMMANDis clear, but consider(details omitted)or similar to better convey that fields are intentionally excluded rather than truncated mid-stream. Very minor. -
[Info]
GrpcClientChannel.java:296— Thedefaultbranch returns only the command case name. If future command types carry sensitive payloads, developers must remember to add explicit cases here. A brief comment like// Add explicit cases for commands with sensitive payloadswould help future maintainers. -
[Info]
GrpcClientChannelTest.java— Test correctly verifies thatVERIFY_MESSAGE_COMMANDsummary does not leak message body content. Good coverage of the key invariant. Consider adding a test for theRECOVER_ORPHANED_TRANSACTION_COMMANDcase as well, since that is the other command type explicitly called out in the issue.
Overall
Clean, well-targeted fix. The approach correctly addresses the payload leakage concern while preserving useful diagnostic information. No correctness, performance, or compatibility issues found.
Automated review by github-manager-bot
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #10673 +/- ##
=============================================
- Coverage 48.31% 48.24% -0.07%
+ Complexity 13511 13488 -23
=============================================
Files 1380 1380
Lines 101091 101112 +21
Branches 13101 13102 +1
=============================================
- Hits 48844 48784 -60
- Misses 46285 46345 +60
- Partials 5962 5983 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot (Re-review)
PR #10673: [ISSUE #10672] Sanitize proxy telemetry failure logs
Re-review scope: New commits a94c20b ("Address telemetry log review feedback") and b5c898b ("Improve telemetry summary test coverage")
Changes Since Last Review
summarizeTelemetryCommand()expanded to cover additional command types with diagnostic-relevant fields- Added channel context (
this) to the catch-block warning for better traceability - Improved test coverage for the telemetry summary method
Assessment
Correctness — The summary method correctly avoids exposing message payloads while providing useful diagnostic information.
Tests — Good coverage of the various telemetry command types.
Security — No sensitive data leakage in the sanitized log output.
Overall: LGTM — Feedback well addressed, clean implementation.
Automated re-review by github-manager-bot
Summary
TelemetryCommandlogging inGrpcClientChannel.writeTelemetryCommandwith a compact command summary.Motivation
Closes #10672.
Proxy gRPC telemetry write failures can happen after a client disconnects or when the telemetry stream is completed. The previous warning logs serialized the full protobuf command, which can include message payloads for commands such as
VerifyMessageCommandandRecoverOrphanedTransactionCommand.Review feedback addressed
RECOVER_ORPHANED_TRANSACTION_COMMANDsummaries withdetails omittedto make intentional field exclusion explicit.RecoverOrphanedTransactionCommandpayload redaction.summarizeTelemetryCommandcoverage for null/default commands, settings commands, thread-stack trace commands, and lite unsubscribe notifications.Tests
mvn -pl proxy -Dtest=GrpcClientChannelTest -DfailIfNoTests=false testResult: BUILD SUCCESS; 4 tests passed; checkstyle reported 0 violations. The run emits existing JaCoCo instrumentation warnings under the local JDK, but tests and build completed successfully.