coderzc commented on code in PR #25306:
URL: https://github.com/apache/pulsar/pull/25306#discussion_r2939860211
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/OpenTelemetryTopicStats.java:
##########
@@ -415,73 +415,94 @@ private void recordMetricsForTopic(Topic topic) {
var persistentTopicMetrics =
persistentTopic.getPersistentTopicMetrics();
var persistentTopicAttributes =
persistentTopic.getTopicAttributes();
+ // For persistent topics, get custom attributes once and reuse
+ var customLabels = persistentTopicAttributes.getCustomAttributes();
+ var attributesWithCustomLabels = persistentTopicAttributes
+ .buildAttributesWithCustomLabels(attributes, customLabels);
var managedLedger = persistentTopic.getManagedLedger();
var managedLedgerStats =
persistentTopic.getManagedLedger().getStats();
- storageCounter.record(managedLedgerStats.getStoredMessagesSize(),
attributes);
-
storageLogicalCounter.record(managedLedgerStats.getStoredMessagesLogicalSize(),
attributes);
-
storageBacklogCounter.record(managedLedger.getEstimatedBacklogSize(),
attributes);
- storageOffloadedCounter.record(managedLedger.getOffloadedSize(),
attributes);
-
storageInCounter.record(managedLedgerStats.getReadEntriesSucceededTotal(),
attributes);
-
storageOutCounter.record(managedLedgerStats.getAddEntrySucceedTotal(),
attributes);
+ storageCounter.record(managedLedgerStats.getStoredMessagesSize(),
attributesWithCustomLabels);
Review Comment:
Thanks, I kept `getCommonAttributes()` unchanged, but moved the custom-label
merge closer to `PersistentTopicAttributes` by introducing an explicit
per-scrape attribute snapshot there. This keeps the dynamic work out of the
common getter while also removing most of the repetitive merging from
`OpenTelemetryTopicStats`.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/OpenTelemetryTopicStats.java:
##########
@@ -415,73 +415,94 @@ private void recordMetricsForTopic(Topic topic) {
var persistentTopicMetrics =
persistentTopic.getPersistentTopicMetrics();
var persistentTopicAttributes =
persistentTopic.getTopicAttributes();
+ // For persistent topics, get custom attributes once and reuse
+ var customLabels = persistentTopicAttributes.getCustomAttributes();
+ var attributesWithCustomLabels = persistentTopicAttributes
+ .buildAttributesWithCustomLabels(attributes, customLabels);
var managedLedger = persistentTopic.getManagedLedger();
var managedLedgerStats =
persistentTopic.getManagedLedger().getStats();
- storageCounter.record(managedLedgerStats.getStoredMessagesSize(),
attributes);
-
storageLogicalCounter.record(managedLedgerStats.getStoredMessagesLogicalSize(),
attributes);
-
storageBacklogCounter.record(managedLedger.getEstimatedBacklogSize(),
attributes);
- storageOffloadedCounter.record(managedLedger.getOffloadedSize(),
attributes);
-
storageInCounter.record(managedLedgerStats.getReadEntriesSucceededTotal(),
attributes);
-
storageOutCounter.record(managedLedgerStats.getAddEntrySucceedTotal(),
attributes);
+ storageCounter.record(managedLedgerStats.getStoredMessagesSize(),
attributesWithCustomLabels);
Review Comment:
Thanks, I agree the merge logic should be centralized in
`PersistentTopicAttributes`.
My main concern is overriding `getCommonAttributes()`: today it is a cheap
accessor over pre-built static attributes, while custom labels are dynamic and
require reading topic properties. Hiding that work in a common getter makes the
behavior less obvious.
Also, changing `getCommonAttributes()` alone would not cover the pre-built
quota / transaction / compaction attributes. I think a better option is to add
explicit helper methods in `PersistentTopicAttributes` and fetch custom labels
once per topic recording, then reuse them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
