[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #10156: Exposing prometheus metrics for Pulsar function local run mode

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

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



##########
File path: pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java
##########
@@ -544,6 +552,11 @@ private void startThreadedMode(org.apache.pulsar.functions.proto.Function.Functi
         if (functionConfig != null && functionConfig.getExposePulsarAdminClientEnabled() != null) {
             exposePulsarAdminClientEnabled = functionConfig.getExposePulsarAdminClientEnabled();
         }
+
+        // Collector Registry for prometheus metrics
+        CollectorRegistry collectorRegistry = new CollectorRegistry();

Review comment:
       If there is just one `CollectorRegistry` shared among threads (assuming parallelism > 1 here), won't each metrics port serve the same metrics?

##########
File path: pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java
##########
@@ -166,6 +171,8 @@ public RuntimeEnv convert(String value) {
     protected String secretsProviderClassName;
     @Parameter(names = "--secretsProviderConfig", description = "Whats the config for the secrets provider", hidden = true)
     protected String secretsProviderConfig;
+    @Parameter(names = "--metricsPortStart", description = "The starting port range for metrics server", hidden = true)

Review comment:
       Can you help me understand why it needs to be a range and not just a single port? If it's obvious and I'm simply missing something, perhaps it'd be valuable to describe why it needs to be a range here so that users understand the behavior when using this new feature.




-- 
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.

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