Exposing prometheus metrics for Pulsar function local run mode #10156
Conversation
pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java
Show resolved
Hide resolved
| @@ -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(); | |||
There was a problem hiding this comment.
If there is just one CollectorRegistry shared among threads (assuming parallelism > 1 here), won't each metrics port serve the same metrics?
There was a problem hiding this comment.
@michaeljmarshall yup thus after some thinking, I can up with a new PR:
For running instances as threads with in the local run JVM, it doesn't make sense to run a metrics server per instance. Better user experience to just run one metrics server and expose all of the metrics for all instances on the same port.
There was a problem hiding this comment.
Better user experience to just run one metrics server and expose all of the metrics for all instances on the same port.
I agree. Although, it looks like the new PR still has metrics at p ports where p = parallelism. I'll add a note to the new PR too, but wanted to comment here, where you mentioned a single port.
| @@ -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) | |||
There was a problem hiding this 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.
There was a problem hiding this comment.
There needs to be a range of ports because many function instances can be run via single LocalRun process
|
@jerrypeng - would you be able to take a look at my questions on this merged PR? Since we're using the same metrics registry, I don't think we need to expose metrics on multiple ports. |
|
@michaeljmarshall yup you are right. I created a new PR: |
| public static void registerDefaultCollectors(CollectorRegistry registry) { | ||
| // Add the JMX exporter for functionality similar to the kafka connect JMX metrics | ||
| try { | ||
| new JmxCollector("{}").register(registry); |
There was a problem hiding this comment.
@jerrypeng @srkukarni Isn't that line is a duplicate of all other exporters written below but with other names?
For example, JmxCollector create
# HELP java_lang_OperatingSystem_ProcessCpuTime java.lang:name=null,type=OperatingSystem,attribute=ProcessCpuTime
# TYPE java_lang_OperatingSystem_ProcessCpuTime untyped
java_lang_OperatingSystem_ProcessCpuTime 5.28804E8
and StandardExports creates:
process_cpu_seconds_total
Motivation
Currently, metrics are not exposes with running functions/sources/sinks in local run mode.
Modifications
Expose metrics in prometheus format but running a metrics server and allow users to specify the port the metrics server will start on
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If
yeswas chosen, please highlight the changesDocumentation