[ISSUE #10490] Eliminate per-RPC allocation from Logback status, TopicMessageType toLowerCase, and RemotingHelper eager evaluation

[wang-jiahua]

Which Issue(s) This PR Fixes

Fixes #10490

Brief Description

Three independent per-RPC micro-allocations eliminated:

1. Logback NopStatusListener — Add <statusListener class="ch.qos.logback.core.status.NopStatusListener"/> to rmq.broker.logback.xml to suppress Logback internal status event callbacks.
2. TopicMessageType.metricsValue cache — getMetricsValue() was calling value.toLowerCase() per invocation. Now caches metricsValue as a final field with Locale.ROOT.
3. RemotingHelper eager evaluation fix — Map.getOrDefault(code, String.valueOf(code)) eagerly evaluates String.valueOf(code) even on cache hit. Changed to get() + null check.

Stacked on PR #10443. Will rebase on develop after #10443 merges to show only the 3 files.

How Did You Test This Change?

1. Full CI passes (maven-compile + bazel-compile on all JDK versions).
2. Commercial compatibility verified.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR reduces per-call allocations in hot paths by introducing reusable OpenTelemetry AttributeKey singletons for metrics labels and by avoiding eager default-string creation in request/response code formatting.

Changes:

• Add typed AttributeKey label constants for broker/remoting metrics and migrate selected call sites to use them.
• Optimize request/response code description helpers to avoid unnecessary String.valueOf(...) allocations when a mapping exists.
• Precompute/canonicalize metric label values and adjust logging configuration behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
remoting/src/main/java/org/apache/rocketmq/remoting/metrics/RemotingMetricsManager.java	Switches attribute building to use typed AttributeKey constant for protocol type.
remoting/src/main/java/org/apache/rocketmq/remoting/metrics/RemotingMetricsConstant.java	Introduces reusable typed AttributeKey singletons for remoting metric labels.
remoting/src/main/java/org/apache/rocketmq/remoting/common/RemotingHelper.java	Avoids eager default-string construction in code-to-description helpers.
common/src/main/java/org/apache/rocketmq/common/attribute/TopicMessageType.java	Caches lowercase metrics label value to avoid repeated toLowerCase() and enforce Locale.ROOT.
broker/src/main/resources/rmq.broker.logback.xml	Adds a NopStatusListener, changing visibility of Logback status output.
broker/src/main/java/org/apache/rocketmq/broker/metrics/PopMetricsManager.java	Migrates selected label puts to typed AttributeKey constants.
broker/src/main/java/org/apache/rocketmq/broker/metrics/BrokerMetricsManager.java	Migrates multiple metrics label puts to typed AttributeKey constants.
broker/src/main/java/org/apache/rocketmq/broker/metrics/BrokerMetricsConstant.java	Introduces reusable typed AttributeKey singletons for broker metric labels.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.14%. Comparing base (c270c60) to head (8d7a462).
⚠️ Report is 3 commits behind head on develop.

Files with missing lines	Patch %	Lines
...pache/rocketmq/remoting/common/RemotingHelper.java	0.00%	4 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10491      +/-   ##
=============================================
- Coverage      48.18%   48.14%   -0.05%     
+ Complexity     13407    13397      -10     
=============================================
  Files           1377     1377              
  Lines         100838   100843       +5     
  Branches       13034    13037       +3     
=============================================
- Hits           48590    48546      -44     
- Misses         46314    46323       +9     
- Partials        5934     5974      +40     

☔ 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]

Review by github-manager-bot

Summary

This PR eliminates three per-RPC micro-allocations on the hot path: Logback status listener overhead, TopicMessageType.metricsValue repeated toLowerCase(), and RemotingHelper eager String.valueOf() evaluation. Additionally, it pre-builds OTel AttributeKey singletons to avoid per-call InternalAttributeKeyImpl allocation.

Findings

• [Info] TopicMessageType.java — Caching metricsValue with Locale.ROOT is correct and avoids per-call allocation. Good use of Locale.ROOT for deterministic casing.

• [Info] RemotingHelper.java — The get() + null check pattern replacing getOrDefault(code, String.valueOf(code)) is a valid optimization to avoid eager evaluation. Consider adding a brief inline comment explaining why the null-check pattern is used instead of getOrDefault, since the intent is not immediately obvious to readers.

• [Info] BrokerMetricsConstant.java / RemotingMetricsConstant.java — Pre-building AttributeKey singletons is a sound optimization for OTel hot paths. The alignment style (extra spaces for =) is consistent within the block but may trigger checkstyle rules if the project enforces no-multi-spaces. Worth verifying CI passes.

• [Warning] The PR description mentions this is stacked on PR #10443. The diff currently includes 8 files (5 extra from #10443). Reviewers should focus on the 3 core delta files. Please rebase onto develop after #10443 merges to keep the diff clean.

Suggestions

• Consider adding a microbenchmark (JMH) to quantify the per-RPC allocation reduction, especially for the AttributeKey singleton optimization. This would help justify the change and prevent regression.
• The NopStatusListener addition is fine, but consider documenting in a comment why it was added (suppress Logback internal status events to avoid per-RPC allocation).

Verdict

Clean performance optimization with no correctness concerns. The changes are well-targeted and low-risk.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Review by github-manager-bot

Summary

Combined performance optimization covering four areas: (1) pre-built AttributeKey singletons in metrics constants, (2) NopStatusListener in Logback config to suppress internal status output, (3) TopicMessageType.metricsValue pre-computed in constructor to avoid repeated toLowerCase(), and (4) RemotingHelper request/response code description lookup using direct get() + null check instead of getOrDefault().

Findings

• [Info] The NopStatusListener addition is a good defensive measure — Logback's internal status printer can cause unexpected allocations on the logging hot path.
• [Info] TopicMessageType.metricsValue pre-computation is clean — the enum constructor runs once at class-load, eliminating per-call toLowerCase() on the send path.
• [Warning] RemotingHelper.getRequestCodeDesc / getResponseCodeDesc: replacing getOrDefault(code, "") with get(code) + null check avoids allocating the empty-string default on every call. This is correct since the returned value is used for metrics labels where empty vs null doesn't matter for OTel. However, verify that no downstream code calls .isEmpty() or .equals("") on the returned value — those would now get NPE instead of false.
• [Info] The AttributeKey singletons here overlap with PR #10443. If both PRs target the same branch, ensure they don't conflict during merge.

Suggestions

• For RemotingHelper, consider adding a brief comment explaining why get() + null check is preferred over getOrDefault() (allocation avoidance on hot path), so future refactors don't "simplify" it back.

No blocking issues. Well-structured multi-part optimization.

---

Automated review by github-manager-bot

[RockteMQ-AI]

• [Info] The AttributeKey singleton pre-building in metrics constants (from earlier commits) eliminates per-call InternalAttributeKeyImpl allocation. This is a solid optimization for high-throughput scenarios.

Changes Since Last Review

The new commits (June 20) appear to maintain the same optimization strategy. All changes remain focused on reducing GC pressure on the hot path without altering functional behavior.

Verdict

✅ Clean, focused performance optimization. No blocking issues.

---

Automated re-review by github-manager-bot

[wang-jiahua]

Server Benchmark Results (2026-06-22)

Environment: 8C30G broker, Dragonwell JDK 21, 1KB msg, 128 queues, sync mode, 256 threads, 20s warmup + 30s collect

Producer-only (steady state, lines 3-5)

Test	TPS	Avg RT	GC	vs Baseline
Baseline	217k	1.18ms	354	—
#10491	199k	1.28ms	212	-8.3% TPS (within noise band), -40% GC ✅

Minor optimizations (TopicMessageType cache, RemotingHelper get+null check, NopStatusListener, metricsValue cache). The -8.3% TPS is at the edge of the noise band (baseline variance is 5-10%). GC reduction (-40%) confirms reduced allocation. These micro-optimizations are not on the Send hot path, so TPS impact is expected to be minimal in production.