[ISSUE #10481] Cache OTel Attributes objects in broker/remoting metrics hot path#10485
[ISSUE #10481] Cache OTel Attributes objects in broker/remoting metrics hot path#10485wang-jiahua wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR focuses on reducing allocation/overhead in hot paths by caching frequently used metric label keys, computed Attributes, and last-used counters, plus a few small operational/metrics tweaks.
Changes:
- Add typed
AttributeKeysingletons and update metric label usage to reduce per-call allocations. - Introduce small caches for frequently reused metric
Attributesand for remoting code distribution counters. - Minor optimizations/changes: faster code→desc lookup, cached lowercase message type for metrics, and logback status listener config.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| remoting/src/main/java/org/apache/rocketmq/remoting/netty/RemotingCodeDistributionHandler.java | Adds a volatile cached holder to speed up repeated inbound/outbound counter increments. |
| remoting/src/main/java/org/apache/rocketmq/remoting/metrics/RemotingMetricsManager.java | Adds typed label keys and an Attributes cache for RPC metrics. |
| remoting/src/main/java/org/apache/rocketmq/remoting/metrics/RemotingMetricsConstant.java | Introduces pre-built OpenTelemetry AttributeKey constants. |
| remoting/src/main/java/org/apache/rocketmq/remoting/common/RemotingHelper.java | Replaces getOrDefault with a null-check lookup for desc maps. |
| common/src/main/java/org/apache/rocketmq/common/attribute/TopicMessageType.java | Caches lowercase metrics value to avoid repeated toLowerCase calls. |
| broker/src/main/resources/rmq.broker.logback.xml | Adds NopStatusListener, changing logback status reporting behavior. |
| broker/src/main/java/org/apache/rocketmq/broker/metrics/PopMetricsManager.java | Switches metric label puts to typed AttributeKey constants. |
| broker/src/main/java/org/apache/rocketmq/broker/metrics/BrokerMetricsManager.java | Switches label keys to typed variants and adds a small topic attributes cache. |
| broker/src/main/java/org/apache/rocketmq/broker/metrics/BrokerMetricsConstant.java | Introduces pre-built OpenTelemetry AttributeKey constants for broker metrics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review: Approved ✅
PR: #10485 — Cache OTel Attributes objects in broker/remoting metrics hot path
Type: Performance (9 files, +187/-47)
Assessment
Two-level cache (volatile inline + ConcurrentHashMap) for immutable OTel Attributes objects. Eliminates redundant allocations (~200-300 bytes each) for repeated parameter combinations. Builds on #10443.
Verdict
✅ Solid optimization with proper concurrency handling. Fixes #10481.
🤖 Automated review by oss-sentinel-ai
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review: Approved ✅
PR: #10485 — Cache OTel Attributes objects in broker/remoting metrics hot path
Type: Performance (9 files, +187/-47)
Assessment
Two-level cache (volatile inline + ConcurrentHashMap) for immutable OTel Attributes objects. Eliminates redundant allocations (~200-300 bytes each) for repeated parameter combinations. Builds on #10443.
Verdict
✅ Solid optimization with proper concurrency handling. Fixes #10481.
🤖 Automated review by oss-sentinel-ai
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review: Approved ✅
PR: #10485 — Cache OTel Attributes objects in broker/remoting metrics hot path
Type: Performance (9 files, +187/-47)
Assessment
Two-level cache (volatile inline + ConcurrentHashMap) for immutable OTel Attributes objects. Eliminates redundant allocations (~200-300 bytes each) for repeated parameter combinations. Builds on #10443.
Verdict
✅ Solid optimization with proper concurrency handling. Fixes #10481.
🤖 Automated review by oss-sentinel-ai
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review: Approved ✅
PR: #10485 — Cache OTel Attributes objects in broker/remoting metrics hot path
Type: Performance (9 files, +187/-47)
Assessment
Two-level cache (volatile inline + ConcurrentHashMap) for immutable OTel Attributes objects. Eliminates redundant allocations (~200-300 bytes each) for repeated parameter combinations. Builds on #10443.
Verdict
✅ Solid optimization with proper concurrency handling. Fixes #10481.
🤖 Automated review by oss-sentinel-ai
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review: Approved ✅
PR: #10485 — Cache OTel Attributes objects in broker/remoting metrics hot path
Type: Performance (9 files, +187/-47)
Assessment
Two-level cache (volatile inline + ConcurrentHashMap) for immutable OTel Attributes objects. Eliminates redundant allocations (~200-300 bytes each) for repeated parameter combinations. Builds on #10443.
Verdict
✅ Solid optimization with proper concurrency handling. Fixes #10481.
🤖 Automated review by oss-sentinel-ai
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review: Approved ✅
PR: #10485 — Cache OTel Attributes objects in broker/remoting metrics hot path
Type: Performance (9 files, +187/-47)
Assessment
Two-level cache (volatile inline + ConcurrentHashMap) for immutable OTel Attributes objects. Eliminates redundant allocations (~200-300 bytes each) for repeated parameter combinations. Builds on #10443.
Verdict
✅ Solid optimization with proper concurrency handling. Fixes #10481.
🤖 Automated review by oss-sentinel-ai
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review: Approved ✅
PR: #10485 — Cache OTel Attributes objects in broker/remoting metrics hot path
Type: Performance (9 files, +187/-47)
Assessment
Two-level cache (volatile inline + ConcurrentHashMap) for immutable OTel Attributes objects. Eliminates redundant allocations (~200-300 bytes each) for repeated parameter combinations. Builds on #10443.
Verdict
✅ Solid optimization with proper concurrency handling. Fixes #10481.
🤖 Automated review by oss-sentinel-ai
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review: Approved ✅
PR: #10485 — Cache OTel Attributes objects in broker/remoting metrics hot path
Type: Performance (9 files, +187/-47)
Assessment
Two-level cache (volatile inline + ConcurrentHashMap) for immutable OTel Attributes objects. Eliminates redundant allocations (~200-300 bytes each) for repeated parameter combinations. Builds on #10443.
Verdict
✅ Solid optimization with proper concurrency handling. Fixes #10481.
🤖 Automated review by oss-sentinel-ai
…static AttributeKey instances
… metrics hot path Builds on top of PR apache#10443 (which introduces the typed AttributeKey constants). After AttributeKey instances are de-duplicated, the next allocation hot spot in the metrics path is the Attributes object itself. For repeated parameter combinations the broker keeps rebuilding identical immutable Attributes (each ~200-300 byte). This commit introduces a two-level cache (volatile inline cache + ConcurrentHashMap fallback) at three points: - BrokerMetricsManager.getOrBuildTopicAttributes(topic, msgType, isSystem): 4-field volatile inline cache + CHM keyed by 'topic|msgType|isSystem'. - RemotingMetricsManager.getOrBuildAttributes(reqCode, respCode, isLongPolling, result): long-packed cache key (35 bits across 4 args, no String allocation), volatile inline cache + CHM. Falls back to direct build for unknown result values. - RemotingCodeDistributionHandler: replace two independent volatile fields (lastCode, lastAdder) with an immutable CachedCounter holder published through a single volatile reference. A single volatile read per inc() returns a self-consistent (code, adder) snapshot, eliminating a torn-read between the two fields under @sharable concurrent access from multiple Netty EventLoop threads.
wang-jiahua
force-pushed
the
perf/metrics-attributes-cache
branch
from
febdef8 to
3dfbe3a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #10485 +/- ##
=============================================
- Coverage 48.06% 48.04% -0.03%
- Complexity 13313 13336 +23
=============================================
Files 1377 1377
Lines 100632 100779 +147
Branches 12995 13019 +24
=============================================
+ Hits 48369 48419 +50
- Misses 46344 46414 +70
- Partials 5919 5946 +27 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Which Issue(s) This PR Fixes
Fixes #10481
Brief Description
Builds on top of #10443 (which introduces the typed
LABEL_*_KEYconstants).After
AttributeKeyinstances are de-duplicated by #10443, the next allocation hot spot in the metrics path is theAttributesobject itself. For repeated parameter combinations the broker keeps rebuilding identical immutableAttributes(each ~200–300 byte). This PR introduces a two-level cache (volatile inline cache +ConcurrentHashMapfallback) at three points:BrokerMetricsManager.getOrBuildTopicAttributes(topic, msgType, isSystem)— 4-field volatile inline cache + CHM keyed bytopic|msgType|isSystem.RemotingMetricsManager.getOrBuildAttributes(reqCode, respCode, isLongPolling, result)— long-packed cache key (35 bits across 4 args, noStringallocation), volatile inline cache + CHM. Falls back to direct build for unknownresultvalues.RemotingCodeDistributionHandler— replace two independentvolatilefields (lastCode,lastAdder) with an immutableCachedCounterholder published through a singlevolatilereference. A singlevolatileread perinc()returns a self-consistent(code, adder)snapshot, eliminating a torn-read between the two fields under@Sharableconcurrent access from multiple NettyEventLoopthreads.Dependency
This PR is stacked on top of #10443 (which provides the typed
LABEL_*_KEYconstants used by the cache builders). Until #10443 is merged, the diff againstdevelopwill include both PRs' changes (8 + 3 = 11 files). After #10443 is merged, this PR will be rebased ontodevelopand the diff will reduce to the 3 files in scope of this PR (105 +/-1 lines).How Did You Test This Change?
BrokerMetricsManagerTest(27 tests) passes.CachedCounterchange is reviewed against the original two-volatiledesign; the immutable holder is the standard idiom for atomically publishing a multi-field state under JMM (finalfield semantics + singlevolatileread).