[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy

[GitBox <gi...@apache.org>]

michaeljmarshall commented on a change in pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681#discussion_r826191920



##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java
##########
@@ -414,5 +418,9 @@ public Authentication getProxyClientAuthenticationPlugin() {
         return this.proxyClientAuthentication;
     }
 
+    public void addPrometheusRawMetricsProvider(PrometheusRawMetricsProvider metricsProvider) {
+        metricsProviders.add(metricsProvider);
+    }

Review comment:
       Since this method could be called from other threads in protocol handlers, I think we should consider using a concurrent data structure to back the `metricsProviders` list. Technically, it appears that the current implementation in the broker does not use a concurrent data structure, so I could be wrong about this class's thread safety requirements.

##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyServiceStarter.java
##########
@@ -240,7 +240,10 @@ public static void addWebServerHandlers(WebServer server,
                                      ProxyConfiguration config,
                                      ProxyService service,
                                      BrokerDiscoveryProvider discoveryProvider) throws Exception {
-        server.addServlet("/metrics", new ServletHolder(MetricsServlet.class),
+        PrometheusMetricsServlet metricsServlet =
+                new PrometheusMetricsServlet(-1L, config.getClusterName());
+        service.getMetricsProviders().forEach(metricsServlet::addRawMetricsProvider);

Review comment:
       Is there a race here that could prevent some metrics providers from getting added to the `metricsServlet`? In looking at the broker's implementation, it ensures that the `metricsServlet` is updated if the servlet has already been created.
   
   https://github.com/apache/pulsar/blob/b0213b225f194b47a5dcd63ef4a26f55c4c820b6/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java#L1518-L1527




-- 
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-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org