Skip to content

[ISSUE #10730] Avoid raw local POP message logs - #10731

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

[ISSUE #10730] Avoid raw local POP message logs#10731
Aias00 wants to merge 1 commit into
apache:developfrom
Aias00:fix/proxy-local-messageext-log-summary

Conversation

@Aias00

@Aias00 Aias00 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • replace direct MessageExt logging in LocalMessageService POP offset warning with a compact message summary
  • keep topic, msgId, queue, offsets, body size, and property keys while omitting body bytes and property values
  • add a targeted test for the summary helper

Fixes #10730

Tests

  • JAVA_HOME=$(/usr/libexec/java_home -v 1.8) mvn -pl proxy -Dtest=LocalMessageServiceTest#testSummarizeMessageExtDoesNotExposeBodyOrPropertyValues test

Note: running the full local LocalMessageServiceTest class in this workspace currently hits an existing Java 8/local dependency signature issue in testPopMessageWriteAndFlush (ByteBuffer.flip()Ljava/nio/ByteBuffer;), unrelated to this log-summary change.

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

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 50.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.22%. Comparing base (00e45b8) to head (3a172f6).

Files with missing lines Patch % Lines
...tmq/proxy/service/message/LocalMessageService.java 50.00% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop   #10731      +/-   ##
=============================================
- Coverage      48.31%   48.22%   -0.10%     
+ Complexity     13511    13479      -32     
=============================================
  Files           1380     1380              
  Lines         101091   101102      +11     
  Branches       13101    13103       +2     
=============================================
- Hits           48844    48752      -92     
- Misses         46285    46363      +78     
- Partials        5962     5987      +25     

☔ 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 raw MessageExt toString() logging in LocalMessageService.popMessage with a compact summary that omits body bytes and property values, reducing log noise and preventing accidental data exposure.

Findings

  • [Info] summarizeMessageExt is well-structured with proper null checks for messageExt, body, and properties. Using TreeSet for property keys ensures deterministic output — good for log parsing.
  • [Info] The bodySize fallback to 0 when getBody() == null is correct and consistent.

Suggestions

  • Cross-PR note: PR #10729 introduces an identical summarizeMessageExt method in ConsumerProcessor. Consider extracting this into a shared utility class (e.g., proxy/utils/MessageExtSummary.java) to avoid duplication and simplify future maintenance.
  • Minor: the log message prefix "Queue offset [{}] of msg is strange, not equal to the stored in msg, msgSummary:{}" could be slightly more readable with a space after the colon before the summary.

Tests

Test testSummarizeMessageExtDoesNotExposeBodyOrPropertyValues correctly asserts that body content and property values are absent from the output. Good coverage.


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 MessageExt.toString() in log output with a concise summarizeMessageExt() that logs only key metadata (topic, msgId, queueId, queueOffset, commitLogOffset, bodySize, propertyKeys) without exposing message body content.

Findings

  • [Info] LocalMessageService.java:292 — Good change: avoids dumping full message content in warn logs. The summary includes enough context for debugging (offsets, msgId) without leaking body data.
  • [Info] LocalMessageService.java:317-328 — summarizeMessageExt() implementation is clean. Null-safe, uses TreeSet for deterministic key ordering.
  • [Info] Test coverage validates null input and normal cases.

Cross-repo Note

The same summarizeMessageExt() method appears in PR #10729 (ConsumerProcessor). Consider extracting to a shared utility class (e.g., MessageExtUtil) to avoid duplication once both PRs are merged.


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 raw MessageExt bodies in LocalMessageService

4 participants