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