michaeljmarshall commented on a change in pull request #11500:
URL: https://github.com/apache/pulsar/pull/11500#discussion_r684673683
##########
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/metrics/ManagedCursorMetrics.java
##########
@@ -77,8 +85,32 @@ public ManagedCursorMetrics(PulsarService pulsar) {
metrics.put("brk_ml_cursor_persistLedgerErrors",
cStats.getPersistLedgerErrors());
metrics.put("brk_ml_cursor_persistZookeeperSucceed",
cStats.getPersistZookeeperSucceed());
metrics.put("brk_ml_cursor_persistZookeeperErrors",
cStats.getPersistZookeeperErrors());
+ metrics.put("brk_ml_cursor_writeLedgerSize",
cStats.getWriteCursorLedgerSize());
+ metrics.put("brk_ml_cursor_writeLedgerLogicalSize",
cStats.getWriteCursorLedgerLogicalSize());
+ metrics.put("brk_ml_cursor_readLedgerSize",
cStats.getReadCursorLedgerSize());
metricsCollection.add(metrics);
+
+ nsTotalNonContiguousDeletedMessagesRange +=
cursor.getTotalNonContiguousDeletedMessagesRange();
+ nsPersistLedgerSucceed += cStats.getPersistLedgerSucceed();
+ nsPersistLedgerErrors += cStats.getPersistLedgerErrors();
+ nsPersistZookeeperSucceed +=
cStats.getPersistZookeeperSucceed();
+ nsPersistZookeeperErrors += cStats.getPersistZookeeperErrors();
+ nsWriteCursorLedgerSize += cStats.getWriteCursorLedgerSize();
+ nsWriteCursorLedgerLogicalSize +=
cStats.getWriteCursorLedgerLogicalSize();
+ nsReadCursorLedgerSize += cStats.getReadCursorLedgerSize();
}
+
+ Metrics metrics = createMetricsByDimension(namespace);
+ metrics.put("brk_ml_cursor_nonContiguousDeletedMessagesRange",
+ nsTotalNonContiguousDeletedMessagesRange);
+ metrics.put("brk_ml_cursor_persistLedgerSucceed",
nsPersistLedgerSucceed);
+ metrics.put("brk_ml_cursor_persistLedgerErrors",
nsPersistLedgerErrors);
+ metrics.put("brk_ml_cursor_persistZookeeperSucceed",
nsPersistZookeeperSucceed);
+ metrics.put("brk_ml_cursor_persistZookeeperErrors",
nsPersistZookeeperErrors);
+ metrics.put("brk_ml_cursor_writeLedgerSize",
nsWriteCursorLedgerSize);
+ metrics.put("brk_ml_cursor_writeLedgerLogicalSize",
nsWriteCursorLedgerLogicalSize);
+ metrics.put("brk_ml_cursor_readLedgerSize",
nsReadCursorLedgerSize);
+ metricsCollection.add(metrics);
Review comment:
What is the purpose of adding these metrics with a single dimension?
Coming from a prometheus background (I'm not familiar with other metrics
providers), these single dimension metrics seem like an anti-pattern. These
metrics are already provided in the above metrics with the `ledger_name` and
`cursor_name` dimensions. A user can easily get the count at the namespace
level by just aggregating on the `namespace` dimension. Further, if they were
to aggregate on all metrics named `brk_ml_cursor_persistLedgerSucceed`, they
would get double the actual value of the metrics, which is problematic.
##########
File path:
pulsar-broker/src/test/java/org/apache/pulsar/broker/stats/PrometheusMetricsTest.java
##########
@@ -1036,7 +1036,7 @@ public void testManagedCursorPersistStats() throws
Exception {
Multimap<String, Metric> metrics = parseMetrics(metricsStr);
List<Metric> cm = (List<Metric>)
metrics.get("pulsar_ml_cursor_persistLedgerSucceed");
- assertEquals(cm.size(), 1);
+ assertEquals(cm.size(), 2);
Review comment:
This became `2` because of the extra namespace metric. If you aggregated
the `value` of each `Metric`, you will see that the metric is essentially
double reported for the namespace.
--
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: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
