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/06/10 05:35:43 UTC
[GitHub] [pulsar] sijie opened a new pull request #10885: Only expose managed ledger metrics if topic level metrics is enabled
sijie opened a new pull request #10885:
URL: https://github.com/apache/pulsar/pull/10885
*Motivation*
Managed ledger metrics can be exploded when there are millions of topics. Currently, no way to turn it off.
--
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] lhotari commented on pull request #10885: Provide a flag to disable managed ledger metrics
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10885:
URL: https://github.com/apache/pulsar/pull/10885#issuecomment-858387465
There's a [test failure](https://github.com/apache/pulsar/pull/10885/checks?check_run_id=2790668248#step:10:6726)
```
[INFO] Running org.apache.pulsar.broker.stats.PrometheusMetricsTest
Error: Tests run: 43, Failures: 1, Errors: 0, Skipped: 33, Time elapsed: 36.699 s <<< FAILURE! - in org.apache.pulsar.broker.stats.PrometheusMetricsTest
Error: testManagedLedgerStats(org.apache.pulsar.broker.stats.PrometheusMetricsTest) Time elapsed: 0.242 s <<< FAILURE!
java.lang.AssertionError: expected [2] but found [0]
at org.testng.Assert.fail(Assert.java:99)
at org.testng.Assert.failNotEquals(Assert.java:1037)
at org.testng.Assert.assertEqualsImpl(Assert.java:140)
at org.testng.Assert.assertEquals(Assert.java:122)
at org.testng.Assert.assertEquals(Assert.java:907)
at org.testng.Assert.assertEquals(Assert.java:917)
at org.apache.pulsar.broker.stats.PrometheusMetricsTest.testManagedLedgerStats(PrometheusMetricsTest.java:759)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132)
at org.testng.internal.InvokeMethodRunnable.runOne(InvokeMethodRunnable.java:45)
at org.testng.internal.InvokeMethodRunnable.call(InvokeMethodRunnable.java:73)
at org.testng.internal.InvokeMethodRunnable.call(InvokeMethodRunnable.java:11)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
at java.base/java.lang.Thread.run(Thread.java:829)
```
it seems that the test config should use `exposeManagedLedgerMetricsInPrometheus=true` ?
--
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] sijie commented on pull request #10885: Provide a flag to disable managed ledger metrics
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #10885:
URL: https://github.com/apache/pulsar/pull/10885#issuecomment-858390549
@lhotari It doesn't generate excessive metrics. So it is not a blocker of 2.8.0.
--
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] sijie commented on pull request #10885: Provide a flag to disable managed ledger metrics
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #10885:
URL: https://github.com/apache/pulsar/pull/10885#issuecomment-858359959
it is not a problem in master. What Matteo has pointed out is already fixed in master.
So it is not a blocker for 2.8.0. I have marked it as 2.9.0
--
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] lhotari commented on pull request #10885: Provide a flag to disable managed ledger metrics
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10885:
URL: https://github.com/apache/pulsar/pull/10885#issuecomment-858384983
> it is not a problem in master. What Matteo has pointed out is already fixed in master.
>
> So it is not a blocker for 2.8.0. I have marked it as 2.9.0
Yes, the performance issue of generating the metrics in Pulsar is fixed in branch-2.8 & master. However, the excessive metrics have an impact on Prometheus when there are a lot of topics. That's the reason why it should be possible to disable these metrics for 2.8.0 so that users with a lot of topics can upgrade to 2.8.0 without having to add more capacity to Prometheus.
@sijie @merlimat Could we also mark this PR for 2.8.0 and cherry-pick to branch-2.8 before the next release candidate goes out?
--
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] lhotari commented on a change in pull request #10885: Provide a flag to disable managed ledger metrics
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10885:
URL: https://github.com/apache/pulsar/pull/10885#discussion_r648900096
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/prometheus/PrometheusMetricsGenerator.java
##########
@@ -131,14 +131,19 @@ private static void generateBrokerBasicMetrics(PulsarService pulsar, SimpleTextO
parseMetricsToPrometheusMetrics(new ManagedLedgerCacheMetrics(pulsar).generate(),
clusterName, Collector.Type.GAUGE, stream);
- // generate managedLedger metrics
- parseMetricsToPrometheusMetrics(new ManagedLedgerMetrics(pulsar).generate(),
+ // if Topic-level metrics is disabled, we shouldn't expose managed ledger metrics
+ if (pulsar.getConfiguration().isExposeTopicLevelMetricsInPrometheus()) {
Review comment:
@merlimat The generated Strings are already cached. I fixed that in #9735 .
--
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] merlimat commented on a change in pull request #10885: Provide a flag to disable managed ledger metrics
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #10885:
URL: https://github.com/apache/pulsar/pull/10885#discussion_r648904030
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/prometheus/PrometheusMetricsGenerator.java
##########
@@ -131,14 +131,19 @@ private static void generateBrokerBasicMetrics(PulsarService pulsar, SimpleTextO
parseMetricsToPrometheusMetrics(new ManagedLedgerCacheMetrics(pulsar).generate(),
clusterName, Collector.Type.GAUGE, stream);
- // generate managedLedger metrics
- parseMetricsToPrometheusMetrics(new ManagedLedgerMetrics(pulsar).generate(),
+ // if Topic-level metrics is disabled, we shouldn't expose managed ledger metrics
+ if (pulsar.getConfiguration().isExposeTopicLevelMetricsInPrometheus()) {
Review comment:
Yes, I reviewed that and forgot about it. I only found out after making the same change and getting conflicts with master. :(
--
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] sijie commented on pull request #10885: Provide a flag to disable managed ledger metrics
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #10885:
URL: https://github.com/apache/pulsar/pull/10885#issuecomment-858391734
keep the default value as before
--
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] codelipenghui merged pull request #10885: Provide a flag to disable managed ledger metrics
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #10885:
URL: https://github.com/apache/pulsar/pull/10885
--
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] merlimat commented on a change in pull request #10885: Only expose managed ledger metrics if topic level metrics is enabled
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #10885:
URL: https://github.com/apache/pulsar/pull/10885#discussion_r648866111
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/prometheus/PrometheusMetricsGenerator.java
##########
@@ -131,14 +131,19 @@ private static void generateBrokerBasicMetrics(PulsarService pulsar, SimpleTextO
parseMetricsToPrometheusMetrics(new ManagedLedgerCacheMetrics(pulsar).generate(),
clusterName, Collector.Type.GAUGE, stream);
- // generate managedLedger metrics
- parseMetricsToPrometheusMetrics(new ManagedLedgerMetrics(pulsar).generate(),
+ // if Topic-level metrics is disabled, we shouldn't expose managed ledger metrics
+ if (pulsar.getConfiguration().isExposeTopicLevelMetricsInPrometheus()) {
Review comment:
I think this would break the current metrics because the namespace aggregation happens in that class for multiple metrics.
The bigger problem with these metrics seems to be that the metric names with the bucket suffix are generated each time, and worse, using `String.format()` which is highly inefficient.
https://github.com/apache/pulsar/blob/6704f12104219611164aa2bb5bbdfc929613f1bf/pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/metrics/AbstractMetrics.java#L79-L87
Instead, we should be pre-generating these metrics/buckets strings like in https://github.com/apache/pulsar/blob/6704f12104219611164aa2bb5bbdfc929613f1bf/pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/NamespaceStats.java#L46-L63
--
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