Skip to content

[ISSUE #10724] Avoid raw gRPC response logs - #10725

Open
Aias00 wants to merge 1 commit into
apache:developfrom
Aias00:fix/proxy-response-writer-log-summary
Open

[ISSUE #10724] Avoid raw gRPC response logs#10725
Aias00 wants to merge 1 commit into
apache:developfrom
Aias00:fix/proxy-response-writer-log-summary

Conversation

@Aias00

@Aias00 Aias00 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • replace full gRPC response logging in ResponseWriter with a compact response summary
  • include response type and protobuf status code when available
  • add tests that verify receive-message payload bytes are not included in the summary

Fixes #10724

Tests

  • JAVA_HOME=$(/usr/libexec/java_home -v 1.8) mvn -pl proxy -Dtest=ResponseWriterTest clean test

Copilot AI review requested due to automatic review settings July 31, 2026 12:31

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 51.72414% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.24%. Comparing base (00e45b8) to head (7c32e8a).

Files with missing lines Patch % Lines
.../rocketmq/proxy/grpc/v2/common/ResponseWriter.java 51.72% 6 Missing and 8 partials ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop   #10725      +/-   ##
=============================================
- Coverage      48.31%   48.24%   -0.08%     
+ Complexity     13511    13486      -25     
=============================================
  Files           1380     1380              
  Lines         101091   101117      +26     
  Branches       13101    13109       +8     
=============================================
- Hits           48844    48780      -64     
- Misses         46285    46347      +62     
- Partials        5962     5990      +28     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by github-manager-bot

Summary

Replaces full gRPC response object logging in ResponseWriter with a compact summary that extracts only the protobuf type name and status code via reflection, preventing message payload bytes from appearing in logs.

Findings

  • [Info] The protobuf reflection approach (getDescriptorForType().getFullName() + status/code field introspection) is elegant and generic — works for any RocketMQ protobuf response type without hardcoding field names per type.
  • [Info] Wrapping the log.debug call with isDebugEnabled() guard avoids unnecessary summarizeResponse() computation when debug is off. Good optimization.
  • [Info] The fallback to getClass().getSimpleName() for non-protobuf objects is safe and informative.

Suggestions

  • [Warning] appendStatusCode does multiple reflection lookups on every call. For a hot path like response writing, consider caching the FieldDescriptor lookups (e.g., as a static ConcurrentHashMap<Class<?>, FieldDescriptor[]>). This is a minor optimization and can be deferred.
  • [Info] The instanceof MessageOrBuilder check is correct since all RocketMQ gRPC responses implement this interface.
  • Tests are well-structured — testSummarizeResponseDoesNotExposeMessagePayload explicitly verifies that secret-payload and body are absent, and testSummarizeResponseIncludesStatusCode verifies the status code extraction.

Automated review by github-manager-bot

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by github-manager-bot

Summary

Replaces raw protobuf response objects in gRPC ResponseWriter logs with a summarizeResponse() that extracts message type and status code. Also adds log.isDebugEnabled() guard for the debug-level write log.

Findings

  • [Info] ResponseWriter.java:57 — Adding log.isDebugEnabled() guard avoids unnecessary summarizeResponse() computation when debug is disabled. Good optimization.
  • [Info] ResponseWriter.java:76-88 — summarizeResponse() uses protobuf reflection (MessageOrBuilder.getDescriptorForType()) to extract the full message type name. This is a clean approach that works for any protobuf response type.
  • [Info] ResponseWriter.java:90-100 — appendStatusCode() safely handles cases where the response doesn't have a status field by checking for null FieldDescriptor.
  • [Info] Test coverage validates both protobuf and non-protobuf response types.

Suggestions

The protobuf reflection approach is elegant and generic. Well done.


Automated review by github-manager-bot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Avoid logging full gRPC responses in proxy ResponseWriter

4 participants