asafm commented on code in PR #17531:
URL: https://github.com/apache/pulsar/pull/17531#discussion_r983479898
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/stats/PrometheusMetricsTest.java:
##########
@@ -1634,6 +1643,29 @@ public static Multimap<String, Metric>
parseMetrics(String metrics) {
return parsed;
}
+ @Test
+ public void testRawMetricsProvider() throws IOException {
+ PrometheusMetricsProvider rawMetricsProvider = new
PrometheusMetricsProvider();
+ rawMetricsProvider.start(new PropertiesConfiguration());
+
rawMetricsProvider.getStatsLogger("test").getOpStatsLogger("test_metrics")
+ .registerSuccessfulEvent(100, TimeUnit.NANOSECONDS);
+
+ getPulsar().addPrometheusRawMetricsProvider(rawMetricsProvider);
Review Comment:
@hangc0276 Ok, from my understanding, and correct me if I'm wrong here: This
PR fixes the BK Metrics API implementation in such a way that it will support
labels. *But* it doesn't fix another issue here: The ability to *use*
`PrometheusMetricsProvider` by plugins.
So today, and in your PR, we only print the metrics registered to
`PrometheusMetricsProvider` instantiated in `ManagedLedgerFactory` - which is
good as we now get the BK client metrics with labels.
*But*, I believe the correct solution should be to instantiate
`PrometheusMetricsProvider` in `PulsarService`, and pass it along to
`ManagedLedgerFactory` and also available to retrieve by any plugin if needed.
The way I see it, plugins should use:
1. Prometheus Client library - preferred.
2. BK Metrics API implementation - not the best-preferred way.
3. Raw metrics provider, if all other means fail.
If we're already doing this, at least let's do this right. In your case, you
know you want to use the `PrometheusMetricsProvider` maybe we should just
expose it and use it properly.
--
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]
