Expose managed ledger bookie client metric to prometheus #6814

hangc0276 · 2020-04-25T03:36:26Z

Motivation

Pulsar use bookkeeper as distributed log storage, and init a bookie client to read/write data from/to bookkeeper. However the pulsar bookie client use default NullStatsLogger.INSTANCE to expose runtime metric, which doesn't expose to prometheus or other state storage. When tuning pulsar bookie client performance, we doesn't have any bookie metric to measure where is the bottleneck.

Changes

I implement a prometheus state provider, and use it to trace bookie client runtime metric, and expose it to prometheus.

Please help take a look, if it's ok, i will add test case and update the metric document.

hangc0276 · 2020-04-25T05:12:39Z

/pulsarbot run-failure-checks

codelipenghui

The change looks good. Could you please help add the description of the new metric into https://pulsar.apache.org/docs/en/reference-metrics/#broker? This is very helpful for users to understand these new metrics.

codelipenghui · 2020-04-25T06:37:01Z

...r/src/main/java/org/apache/bookkeeper/mledger/stats/prometheus/PrometheusTextFormatUtil.java

+        // bookie_journal_JOURNAL_ADD_ENTRY{success="false",quantile="0.5",} NaN
+        // bookie_journal_JOURNAL_ADD_ENTRY{success="false",quantile="0.75",} NaN
+        // bookie_journal_JOURNAL_ADD_ENTRY{success="false",quantile="0.95",} NaN
+        // bookie_journal_JOURNAL_ADD_ENTRY{success="false",quantile="0.99",} NaN
+        // bookie_journal_JOURNAL_ADD_ENTRY{success="false",quantile="0.999",} NaN
+        // bookie_journal_JOURNAL_ADD_ENTRY{success="false",quantile="0.9999",} NaN


Can we get these metrics at the bookie client side? If can't, I think it's better to use some bookie client metrics in the example.

I have checked the bookie client metric code, some class constructor and functions are package private and can't be accessed in pulsar.

codelipenghui · 2020-04-25T06:38:02Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java

    private ManagedLedgerFactoryImpl(BookkeeperFactoryForCustomEnsemblePlacementPolicy bookKeeperGroupFactory, boolean isBookkeeperManaged, ZooKeeper zooKeeper,
            ManagedLedgerFactoryConfig config) throws Exception {
+        Configuration configuration = new ClientConfiguration();
+        configuration.addProperty("prometheusStatsLatencyRolloverSeconds", config.getPrometheusStatsLatencyRolloverSeconds());


Suggested change

configuration.addProperty("prometheusStatsLatencyRolloverSeconds", config.getPrometheusStatsLatencyRolloverSeconds());

configuration.addProperty(PrometheusMetricsProvider.PROMETHEUS_STATS_LATENCY_ROLLOVER_SECONDS, config.getPrometheusStatsLatencyRolloverSeconds());

codelipenghui · 2020-04-25T06:38:42Z

.../src/main/java/org/apache/bookkeeper/mledger/stats/prometheus/PrometheusMetricsProvider.java

+import java.io.IOException;
+import java.io.Writer;
+import java.lang.reflect.Field;
+import java.util.concurrent.*;


Avoid use import xxx.* and please check all.

hangc0276 · 2020-04-25T10:36:19Z

The change looks good. Could you please help add the description of the new metric into https://pulsar.apache.org/docs/en/reference-metrics/#broker? This is very helpful for users to understand these new metrics.

thanks for your feedback. I have update the document.

hangc0276 · 2020-04-27T12:17:04Z

/pulsarbot run-failure-checks

hangc0276 · 2020-05-01T16:24:32Z

/pulsarbot run-failure-checks

sijie

Just out of curiosity, why do you implement a new StatsProvider? BookKeeper already has the prometheus data provider. Why not re-use it?

hangc0276 · 2020-05-02T01:57:38Z

Just out of curiosity, why do you implement a new StatsProvider? BookKeeper already has the prometheus data provider. Why not re-use it?

The reasons of implementing a new prometheus statsProvider as follows:

bookkeeper's prometheus statsProvider will open an individual ip:port to provide http service, which can't be integrated into pulsar's now available prometheus http service which is just concat metric string into prometheus format.
some components of bookkeeper's prometheus statsProvider dependent on are package private, which can't be accessed out of org.apache.bookkeeper.stats.prometheus package,
- PrometheusStatsLogger has package private constructor
- SimpleGauge has package private getSample method
- PrometheusTextFormatUtil has package private writeGauge, writeCounter, writeOpStat method
- DataSketchesOpStatsLogger has package private constructor

whether have other new ways to reuse bookkeeper's prometheus statsProvider?

hangc0276 · 2020-05-02T04:19:51Z

/pulsarbot run-failure-checks

…nfiguration for stats expose

hangc0276 · 2020-05-06T07:47:51Z

/pulsarbot run-failure-checks

hangc0276 · 2020-05-06T10:58:05Z

/pulsarbot run-failure-checks

sijie · 2020-05-07T06:37:07Z

@hangc0276 sorry for late response. I generally don't think copying the files and maintain another implementation in Pulsar is a good idea.

bookkeeper's prometheus statsProvider will open an individual ip:port

the prometheus stats provider also work well with bookie HTTP server as well. There is a flag to disable the prometheus HTTP server in the stats provider by setting prometheusStatsHttpEnable to false.

https://github.com/apache/bookkeeper/blob/master/bookkeeper-stats-providers/prometheus-metrics-provider/src/main/java/org/apache/bookkeeper/stats/prometheus/PrometheusMetricsProvider.java#L63

some components of bookkeeper's prometheus statsProvider dependent

Why do you need to access those components? Can't you use the bookkeeper stats library? You can always use Stats.loadProvider to load the stats provider, no?

https://github.com/apache/bookkeeper/blob/master/bookkeeper-stats/src/main/java/org/apache/bookkeeper/stats/Stats.java#L43

hangc0276 · 2020-05-07T07:46:28Z

@hangc0276 sorry for late response. I generally don't think copying the files and maintain another implementation in Pulsar is a good idea.

bookkeeper's prometheus statsProvider will open an individual ip:port

the prometheus stats provider also work well with bookie HTTP server as well. There is a flag to disable the prometheus HTTP server in the stats provider by setting prometheusStatsHttpEnable to false.

https://github.com/apache/bookkeeper/blob/master/bookkeeper-stats-providers/prometheus-metrics-provider/src/main/java/org/apache/bookkeeper/stats/prometheus/PrometheusMetricsProvider.java#L63

some components of bookkeeper's prometheus statsProvider dependent

Why do you need to access those components? Can't you use the bookkeeper stats library? You can always use Stats.loadProvider to load the stats provider, no?

https://github.com/apache/bookkeeper/blob/master/bookkeeper-stats/src/main/java/org/apache/bookkeeper/stats/Stats.java#L43

Sorry, i am not found the flag. I will fix it soon. Thanks for your feedback.

ckdarby · 2020-05-15T20:24:43Z

@hangc0276 This would be very valuable to help us debug some throughput & performance issues we're seeing when on EBS and would like to pinpoint our bottlenecks.

Appreciate the work, do you need anything else to continue forward with this?

hangc0276 · 2020-05-16T13:34:05Z

@hangc0276 This would be very valuable to help us debug some throughput & performance issues we're seeing when on EBS and would like to pinpoint our bottlenecks.

Appreciate the work, do you need anything else to continue forward with this?

Thanks for your reply. With the bookie client metric, we could easily find the bottlenecks of pulsar. I need to use bookkeeper prometheus lib instead of the copy one, and fix the prometheus test case. Because the pulsar prometheus test case is conflict with bookkeeper prometheus output format. I will fix it soon.

will review later

hangc0276 · 2020-05-31T13:57:00Z

@sijie I try to use bookkeeper prometheus provider lib the export the bookie client metric, but failed. The exception is

21:49:06.142 [prometheus-stats-43-1] ERROR org.apache.bookkeeper.common.util.SafeRunnable - Unexpected throwable caught
java.lang.ExceptionInInitializerError: null
        at org.apache.pulsar.broker.stats.prometheus.PrometheusMetricsServlet.lambda$doGet$0(PrometheusMetricsServlet.java:70) ~[org.apache.pulsar-pulsar-broker-2.6.0-SNAPSHOT.jar:2.6.0-SNAPSHOT]
        at org.apache.bookkeeper.mledger.util.SafeRun$1.safeRun(SafeRun.java:32) ~[org.apache.pulsar-managed-ledger-2.6.0-SNAPSHOT.jar:2.6.0-SNAPSHOT]
        at org.apache.bookkeeper.common.util.SafeRunnable.run(SafeRunnable.java:36) [org.apache.bookkeeper-bookkeeper-common-4.10.0.jar:4.10.0]
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) [?:1.8.0_92]
        at java.util.concurrent.FutureTask.run(FutureTask.java:266) [?:1.8.0_92]
        at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$201(ScheduledThreadPoolExecutor.java:180) [?:1.8.0_92]
        at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:293) [?:1.8.0_92]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) [?:1.8.0_92]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) [?:1.8.0_92]
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [io.netty-netty-common-4.1.48.Final.jar:4.1.48.Final]
        at java.lang.Thread.run(Thread.java:745) [?:1.8.0_92]
Caused by: java.lang.IllegalArgumentException: Collector already registered that provides name: process_cpu_seconds_total
        at io.prometheus.client.CollectorRegistry.register(CollectorRegistry.java:54) ~[io.prometheus-simpleclient-0.5.0.jar:?]
        at io.prometheus.client.Collector.register(Collector.java:139) ~[io.prometheus-simpleclient-0.5.0.jar:?]
        at io.prometheus.client.Collector.register(Collector.java:132) ~[io.prometheus-simpleclient-0.5.0.jar:?]
        at io.prometheus.client.hotspot.DefaultExports.initialize(DefaultExports.java:23) ~[io.prometheus-simpleclient_hotspot-0.5.0.jar:?]
        at org.apache.pulsar.broker.stats.prometheus.PrometheusMetricsGenerator.<clinit>(PrometheusMetricsGenerator.java:61) ~[org.apache.pulsar-pulsar-broker-2.6.0-SNAPSHOT.jar:2.6.0-SNAPSHOT]
        ... 11 more

The reason is bookkeeper lib PrometheusMetricsProvider will export the follow metrics by default and can't turn off.

this.registerMetrics(new StandardExports());
this.registerMetrics(new MemoryPoolsExports());
this.registerMetrics(new GarbageCollectorExports());
this.registerMetrics(new ThreadExports());
this.registerMetrics(Gauge.build("jvm_memory_direct_bytes_used", "-").create().setChild(new Child() {
         public double get() {
                return PrometheusMetricsProvider.directMemoryUsage != null ? (double)PrometheusMetricsProvider.directMemoryUsage.longValue() : 0.0D / 0.0;
            }
        }, new String[0]));
        this.registerMetrics(Gauge.build("jvm_memory_direct_bytes_max", "-").create().setChild(new Child() {
            public double get() {
                return (double)PlatformDependent.maxDirectMemory();
            }
        }, new String[0]));

However the pulsar broker prometheus metric also export those metrics to prometheus, some of the metrics registered twice, which will lead to metric export failed.

Could you give me some ideas ?

codelipenghui · 2020-06-04T04:12:47Z

move to 2.7.0 first.

hangc0276 · 2020-06-05T03:39:22Z

/pulsarbot run-failure-checks

sijie

@hangc0276 we can add this change first. Can you add a configuration setting either in bookkeeper side or pulsar side to allow excluding jvm settings? So you can avoid this situation and eventually use the bookkeeper metrics library.

hangc0276 · 2020-06-08T03:47:06Z

@hangc0276 we can add this change first. Can you add a configuration setting either in bookkeeper side or pulsar side to allow excluding jvm settings? So you can avoid this situation and eventually use the bookkeeper metrics library.

@sijie Ok, I think it's better to add a configuration setting in bookkeeper side. As bookkeeper client metric, it should not import the external jvm and other metrics. I will fix it on bookkeeper side, but it depend on the bookkeeper release.

sijie · 2020-06-10T05:13:17Z

@hangc0276 sounds good to me.

Anonymitaet · 2020-06-10T06:20:21Z

Docs have been added: https://pulsar.apache.org/docs/en/next/reference-metrics/#managed-ledger-bookie-client-metrics

### Motivation Pulsar use bookkeeper as distributed log storage, and init a bookie client to read/write data from/to bookkeeper. However the pulsar bookie client use default ` NullStatsLogger.INSTANCE` to expose runtime metric, which doesn't expose to prometheus or other state storage. When tuning pulsar bookie client performance, we doesn't have any bookie metric to measure where is the bottleneck. ### Changes I implement a prometheus state provider, and use it to trace bookie client runtime metric, and expose it to prometheus.

codelipenghui requested review from codelipenghui, jiazhai, merlimat and sijie April 25, 2020 05:56

codelipenghui assigned hangc0276 Apr 25, 2020

codelipenghui added triage/week-18 area/metrics labels Apr 25, 2020

codelipenghui added this to the 2.6.0 milestone Apr 25, 2020

codelipenghui reviewed Apr 25, 2020

View reviewed changes

codelipenghui added the doc-required Your PR changes impact docs and you will update later. label Apr 25, 2020

hangc0276 added 5 commits April 25, 2020 17:44

expose managed ledger bookie client metric to prometheus

becdde7

format metric name

1013404

format name

703ccfe

format code

881d71f

update document

6d7075b

hangc0276 force-pushed the managed_ledger_metric branch from c78374d to 6d7075b Compare April 25, 2020 10:30

fix a bug and turn on traceTaskExecution

6193c5a

update metric labels

cde977f

sijie requested changes May 1, 2020

View reviewed changes

simplify code

dba0bc1

hangc0276 requested review from codelipenghui and sijie May 2, 2020 02:56

hangc0276 added 2 commits May 4, 2020 19:29

change prometheus stats logger to ManagedLedgerFactory and support co…

19510f1

…nfiguration for stats expose

remove unnecessary dependency

dfabeec

fix test case

1923536

jiazhai previously approved these changes May 30, 2020

View reviewed changes

jiazhai self-requested a review May 30, 2020 10:00

Merge branch 'master' into managed_ledger_metric

b4c5b38

codelipenghui modified the milestones: 2.6.0, 2.7.0 Jun 4, 2020

Merge branch 'master' into managed_ledger_metric

782c3c3

codelipenghui modified the milestones: 2.7.0, 2.6.0 Jun 5, 2020

jiazhai approved these changes Jun 5, 2020

View reviewed changes

sijie approved these changes Jun 8, 2020

View reviewed changes

codelipenghui merged commit b983abf into apache:master Jun 8, 2020

Anonymitaet removed the doc-required Your PR changes impact docs and you will update later. label Jun 10, 2020

This was referenced Oct 26, 2020

Flaky-test: testManagedLedgerBookieClientStats(org.apache.pulsar.broker.stats.PrometheusMetricsTest) fails very often on CI #8374

Closed

Issue 8473: testManagedLedgerBookieClientStats fails very often on CI #8379

Merged

	configuration.addProperty("prometheusStatsLatencyRolloverSeconds", config.getPrometheusStatsLatencyRolloverSeconds());
	configuration.addProperty(PrometheusMetricsProvider.PROMETHEUS_STATS_LATENCY_ROLLOVER_SECONDS, config.getPrometheusStatsLatencyRolloverSeconds());

Expose managed ledger bookie client metric to prometheus #6814

Expose managed ledger bookie client metric to prometheus #6814

Uh oh!

Conversation

hangc0276 commented Apr 25, 2020

Motivation

Changes

Uh oh!

hangc0276 commented Apr 25, 2020

Uh oh!

codelipenghui left a comment

Choose a reason for hiding this comment

Uh oh!

codelipenghui Apr 25, 2020

Choose a reason for hiding this comment

Uh oh!

hangc0276 Apr 25, 2020

Choose a reason for hiding this comment

Uh oh!

codelipenghui Apr 25, 2020

Choose a reason for hiding this comment

Uh oh!

codelipenghui Apr 25, 2020

Choose a reason for hiding this comment

Uh oh!

hangc0276 commented Apr 25, 2020

Uh oh!

hangc0276 commented Apr 27, 2020

Uh oh!

hangc0276 commented May 1, 2020

Uh oh!

sijie left a comment

Choose a reason for hiding this comment

Uh oh!

hangc0276 commented May 2, 2020

Uh oh!

hangc0276 commented May 2, 2020

Uh oh!

hangc0276 commented May 6, 2020

Uh oh!

hangc0276 commented May 6, 2020

Uh oh!

sijie commented May 7, 2020

Uh oh!

hangc0276 commented May 7, 2020

Uh oh!

ckdarby commented May 15, 2020

Uh oh!

hangc0276 commented May 16, 2020

Uh oh!

hangc0276 commented May 31, 2020

Uh oh!

codelipenghui commented Jun 4, 2020

Uh oh!

hangc0276 commented Jun 5, 2020

Uh oh!

sijie left a comment

Choose a reason for hiding this comment

Uh oh!

hangc0276 commented Jun 8, 2020

Uh oh!

sijie commented Jun 10, 2020

Uh oh!

Anonymitaet commented Jun 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants