You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/04/07 01:35:16 UTC
[GitHub] [pulsar] jerrypeng opened a new pull request #10156: Exposing prometheus metrics for Pulsar function local run mode
jerrypeng opened a new pull request #10156:
URL: https://github.com/apache/pulsar/pull/10156
### 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
- [ ] Make sure that the change passes the CI checks.
*(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:)*
- *Added integration tests for end-to-end deployment with large payloads (10MB)*
- *Extended integration test for recovery after broker failure*
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (yes / no)
- The public API: (yes / no)
- The schema: (yes / no / don't know)
- The default values of configurations: (yes / no)
- The wire protocol: (yes / no)
- The rest endpoints: (yes / no)
- The admin cli options: (yes / no)
- Anything that affects deployment: (yes / no / don't know)
### Documentation
- Does this pull request introduce a new feature? (yes / no)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
- If a feature is not applicable for documentation, explain why?
- If a feature is not documented yet in this PR, please create a followup issue for adding the documentation
--
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
[GitHub] [pulsar] jerrypeng merged pull request #10156: Exposing prometheus metrics for Pulsar function local run mode
Posted by GitBox <gi...@apache.org>.
jerrypeng merged pull request #10156:
URL: https://github.com/apache/pulsar/pull/10156
--
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
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #10156: Exposing prometheus metrics for Pulsar function local run mode
Posted by 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
[GitHub] [pulsar] michaeljmarshall commented on pull request #10156: Exposing prometheus metrics for Pulsar function local run mode
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #10156:
URL: https://github.com/apache/pulsar/pull/10156#issuecomment-817088500
@jerrypeng - would you be able to take a look at my questions on this merged PR? Since were using the same metrics registry, I don't think we need to expose metrics on multiple ports.
--
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
[GitHub] [pulsar] jerrypeng commented on a change in pull request #10156: Exposing prometheus metrics for Pulsar function local run mode
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10156:
URL: https://github.com/apache/pulsar/pull/10156#discussion_r612143812
##########
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:
There needs to be a range of ports because many function instances can be run via single LocalRun process
--
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
[GitHub] [pulsar] jerrypeng commented on pull request #10156: Exposing prometheus metrics for Pulsar function local run mode
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on pull request #10156:
URL: https://github.com/apache/pulsar/pull/10156#issuecomment-818452825
@michaeljmarshall yup you are right. I created a new PR:
https://github.com/apache/pulsar/pull/10208
--
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
[GitHub] [pulsar] srkukarni commented on a change in pull request #10156: Exposing prometheus metrics for Pulsar function local run mode
Posted by GitBox <gi...@apache.org>.
srkukarni commented on a change in pull request #10156:
URL: https://github.com/apache/pulsar/pull/10156#discussion_r608332060
##########
File path: pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java
##########
@@ -569,15 +582,20 @@ private void startThreadedMode(org.apache.pulsar.functions.proto.Function.Functi
instanceConfig.setFunctionId(UUID.randomUUID().toString());
instanceConfig.setInstanceId(i + instanceIdOffset);
instanceConfig.setMaxBufferedTuples(1024);
- instanceConfig.setPort(FunctionCommon.findAvailablePort());
Review comment:
What about this?
--
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
[GitHub] [pulsar] jerrypeng commented on a change in pull request #10156: Exposing prometheus metrics for Pulsar function local run mode
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10156:
URL: https://github.com/apache/pulsar/pull/10156#discussion_r608336936
##########
File path: pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java
##########
@@ -569,15 +582,20 @@ private void startThreadedMode(org.apache.pulsar.functions.proto.Function.Functi
instanceConfig.setFunctionId(UUID.randomUUID().toString());
instanceConfig.setInstanceId(i + instanceIdOffset);
instanceConfig.setMaxBufferedTuples(1024);
- instanceConfig.setPort(FunctionCommon.findAvailablePort());
Review comment:
There is no point in setting any of these ports. They are not used when running in threaded mode for local run
--
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
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #10156: Exposing prometheus metrics for Pulsar function local run mode
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #10156:
URL: https://github.com/apache/pulsar/pull/10156#discussion_r612165891
##########
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:
> 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.
--
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
[GitHub] [pulsar] michaeljmarshall edited a comment on pull request #10156: Exposing prometheus metrics for Pulsar function local run mode
Posted by GitBox <gi...@apache.org>.
michaeljmarshall edited a comment on pull request #10156:
URL: https://github.com/apache/pulsar/pull/10156#issuecomment-817088500
@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.
--
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
[GitHub] [pulsar] jerrypeng commented on a change in pull request #10156: Exposing prometheus metrics for Pulsar function local run mode
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10156:
URL: https://github.com/apache/pulsar/pull/10156#discussion_r612143140
##########
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:
@michaeljmarshall yup thus after some thinking, I can up with a new PR:
https://github.com/apache/pulsar/pull/10208
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.
--
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