[improve][monitor]Add prometheusRawMetricsProvider support by hangc0276 · Pull Request #17531 · apache/pulsar

hangc0276 · 2022-09-08T01:11:53Z

Motivation

Currently, Pulsar Prometheus metric framework can generate new metrics from PrometheusRawMetricsProvider, and the current PrometheusMetricsProvider implementation doesn't support this interface, which leads to new created PrometheusMetricProvider instance can not integrate with the current metric system.

Modification

Pulsar prometheusMetricsProvider supports the following features.

Support label
Support PrometheusRawMetricsProvider interface.

This Pr just migrated from the BookKeeper Prometheus metrics provider.

Documentation

Check the box below or label this PR directly.

Need to update docs?

doc-required
(Your PR needs to update docs and you will update later)
doc-not-needed
(Please explain why)
doc
(Your PR contains doc changes)
doc-complete
(Docs have been already added)

Matching PR in the forked repository

PR in forked repository: hangc0276#4

hangc0276 · 2022-09-21T04:01:30Z

@asafm @tjiuming Would you please help take a look? thanks.

asafm · 2022-09-21T12:44:05Z

...c/main/java/org/apache/pulsar/broker/stats/prometheus/metrics/PrometheusMetricsProvider.java

+        labels = Collections.singletonMap(CLUSTER_NAME, conf.getString(CLUSTER_NAME, DEFAULT_CLUSTER_NAME));
+
+        executor.scheduleAtFixedRate(() -> {
+            rotateLatencyCollection();


removed catchingAndLoggingThrowables

I add it back.

asafm · 2022-09-21T12:44:33Z

...c/main/java/org/apache/pulsar/broker/stats/prometheus/metrics/PrometheusMetricsProvider.java

    public static final String DEFAULT_CLUSTER_NAME = "pulsar";

-    private String cluster;
-    private final CachingStatsProvider cachingStatsProvider;


@hangc0276 Previously there was a caching layer called cachingStatsProvider which saved in a map every StatsLogger created via getStatsLogger. If it was created previously it would return it.
Now it is removed

I add the cachingStatsProvider back with a small change.

asafm · 2022-09-21T12:47:17Z

...c/main/java/org/apache/pulsar/broker/stats/prometheus/metrics/PrometheusMetricsProvider.java

-        opStats.forEach((name, opStatLogger) -> PrometheusTextFormatUtil.writeOpStat(writer, name, cluster,
-                opStatLogger));
+        PrometheusTextFormat prometheusTextFormat = new PrometheusTextFormat();
+        PrometheusTextFormat.writeMetricsCollectedByPrometheusClient(writer, registry);


I don't get this line. Why would we want to emit all static default Prometheus collector into the writer, where we already have that in PulsarMetricsGenerator?

I removed it.

asafm · 2022-09-21T12:50:05Z

...c/main/java/org/apache/pulsar/broker/stats/prometheus/metrics/PrometheusMetricsProvider.java

-        counters.forEach((name, counter) -> PrometheusTextFormatUtil.writeCounter(writer, name, cluster, counter));
-        opStats.forEach((name, opStatLogger) -> PrometheusTextFormatUtil.writeOpStat(writer, name, cluster,
-                opStatLogger));
+        PrometheusTextFormat prometheusTextFormat = new PrometheusTextFormat();


writeAllMetrics is currently called from PrometheusMetricsGenerator:

generateManagedLedgerBookieClientMetrics(pulsar, stream);

After that call we have support for PrometheusRawMetricsProvider

if (metricsProviders != null) { for (PrometheusRawMetricsProvider metricsProvider : metricsProviders) { metricsProvider.generate(stream); } }

I haven't seen any registration of PrometheusMetricsProvider as PrometheusRawMetricsProvider other than the test. If you do register it, you will have duplicates.

Since writing directly to SimpleTextOutputFormat is more correct, I advise the following:

writeAllMetrics() should have to throw an exception and have no implementation since it shouldn't be called at all.

Fix the implementation of generateManagedLedgerBookieClientMetrics to call a method that uses SimpleTextOutputFormat and lose the declaration of implementation of PrometheusRawMetricsProvider, it can just be a public method.

The only caveat I see is this: Do you see any chance that we will have two different pairs of (scopeContext, gauge/metric/...) such that they have the same metric name, maybe the only difference is in the labels?

If you ditch that Writer based method, you can get rid of PrometheusTextFormat.java

Good point, updated the code.

Reiterate question: The only caveat I see is this: Do you see any chance that we will have two different pairs of (scopeContext, gauge/metric/...) such that they have the same metric name, maybe the only difference is in the labels?

Yes, there are many cases. Such as a metric name is publishrLatency, and the label with a different topic name. It represents different metrics.

I add CachingStatsLogger to cache the same scopeLabel instances to avoid creating too many instances with the same scope and label name.

Yes, there are many cases. Such as a metric name is publishrLatency, and the label with a different topic name. It represents different metrics.

Well @hangc0276 in that case you must use PrometheusMetricsStreams - this class guarantees the output will be grouped by metric name as Prometheus Text format dictates

asafm · 2022-09-21T13:28:56Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/stats/PrometheusMetricsTest.java

+        while((str = reader.readLine()) != null){
+            sb.append(str);
+        }
+        Assert.assertTrue(sb.toString().contains("test_metrics"));


This doesn't test the full functionality and I think it may be duplicate when calling and parsing.

asafm · 2022-09-21T13:49:21Z

...rc/main/java/org/apache/pulsar/broker/stats/prometheus/metrics/PrometheusTextFormatUtil.java

-                .append(success.toString()).append("\", quantile=\"")
-                .append(Double.toString(quantile)).append("\"} ")
-                .append(Double.toString(opStat.getQuantileValue(success, quantile))).append('\n');
+    public static void writeMetricsCollectedByPrometheusClient(SimpleTextOutputStream w, CollectorRegistry registry) {


Do we need this? Is this called from anywhere once we remove the call talked in the earlier comment?

Removed it.

asafm · 2022-09-21T14:19:32Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/stats/PrometheusMetricsTest.java

+        rawMetricsProvider.getStatsLogger("test").getOpStatsLogger("test_metrics")
+            .registerSuccessfulEvent(100, TimeUnit.NANOSECONDS);
+
+        getPulsar().addPrometheusRawMetricsProvider(rawMetricsProvider);


If you already have integration built it, and you go about querying the real metrics endpoint, why not check that you have a real BK client metric with the labels you expect to have but now you actually have them?

The PR is not mainly developed for the BK client metrics, and it is used for the plugin metrics integrated with Pulsar broker.

I don't have enough knowledge on

is used for the plugin metrics integrated with Pulsar broker.

Can you please elaborate and give references?

Oh, I read your code below.
I'm still amazed after spending weeks reading all the metrics code, I'm still discovering new bits.
I was wondering what was that weird PrometheusRawMetricsProvider nobody was actually using inside Pulsar.
So you're saying it's in fact a public API for other plugin developers to rely on that? Can you please give me a reference for such one?

Yes, it is a public API for other plugins which run within the Pulsar broker to expose metrics to the Pulsar broker metric port. A public plugin repo is KOP. https://github.com/streamnative/kop/blob/a9f56e9b0435429dd8d977ba948c6772e6fe5b86/kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/KafkaProtocolHandler.java#L280.
I'm developing another plugin, which also needs this public API to expose metrics.

@hangc0276 Ok, from my understanding, and correct me if I'm wrong here: This PR fixes the BK Metrics API implementation in such a way that it will support labels. But it doesn't fix another issue here: The ability to use PrometheusMetricsProvider by plugins.

So today, and in your PR, we only print the metrics registered to PrometheusMetricsProvider instantiated in ManagedLedgerFactory - which is good as we now get the BK client metrics with labels.

But, I believe the correct solution should be to instantiate PrometheusMetricsProvider in PulsarService, and pass it along to ManagedLedgerFactory and also available to retrieve by any plugin if needed.

The way I see it, plugins should use:

Prometheus Client library - preferred.

BK Metrics API implementation - not the best-preferred way.

Raw metrics provider, if all other means fail.
If we're already doing this, at least let's do this right. In your case, you know you want to use the PrometheusMetricsProvider maybe we should just expose it and use it properly.

asafm · 2022-09-21T14:22:41Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/stats/PrometheusMetricsTest.java

+        HttpClient httpClient = HttpClientBuilder.create().build();
+        final String metricsEndPoint = getPulsar().getWebServiceAddress() + "/metrics";
+        HttpResponse response = httpClient.execute(new HttpGet(metricsEndPoint));
+        InputStream inputStream = response.getEntity().getContent();


use EntityUtils.toString

asafm · 2022-09-21T14:23:10Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/stats/PrometheusMetricsTest.java

+        while((str = reader.readLine()) != null){
+            sb.append(str);
+        }
+        Assert.assertTrue(sb.toString().contains("test_metrics"));


Maybe parseMetrics()

I will use another PR to add checks for the metrics string format.

asafm · 2022-09-21T14:23:55Z

@asafm @tjiuming Would you please help take a look? thanks.

Done @hangc0276

asafm · 2022-09-21T14:29:10Z

For future readers of this PR:

Why is this PR needed?
Pulsar uses Bookkeeper Client, which has metrics. The way to get those metrics is to implement the BK Metric library API, which Pulsar has already done through PrometheusMetricsProvider, PrometheusStatsLogger, etc.
The problem with that implementation was that the API itself supported having labels per metric, but the implementation ignored the labels provided to it.

This PR aims to fix and add support for the labels given to it via the API methods.
The impact is that now Pulsar /metrics endpoint, which exposes the Pulsar metrics in Prometheus format, will contain the Bookkeeper client metrics with labels (which were previously stripped).

hangc0276 · 2022-09-22T14:52:31Z

For future readers of this PR:

Why is this PR needed? Pulsar uses Bookkeeper Client, which has metrics. The way to get those metrics is to implement the BK Metric library API, which Pulsar has already done through PrometheusMetricsProvider, PrometheusStatsLogger, etc. The problem with that implementation was that the API itself supported having labels per metric, but the implementation ignored the labels provided to it.

This PR aims to fix and add support for the labels given to it via the API methods. The impact is that now Pulsar /metrics endpoint, which exposes the Pulsar metrics in Prometheus format, will contain the Bookkeeper client metrics with labels (which were previously stripped).

@asafm Thanks for your explanation. Another purpose of this Pr is to support plugin metrics integrate with Pulsar broker's metrics through the interface.

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

Lines 904 to 914 in 63d4cf2

    
           private synchronized void createMetricsServlet() { 
        
               this.metricsServlet = new PulsarPrometheusMetricsServlet( 
        
                       this, config.isExposeTopicLevelMetricsInPrometheus(), 
        
                       config.isExposeConsumerLevelMetricsInPrometheus(), 
        
                       config.isExposeProducerLevelMetricsInPrometheus(), 
        
                       config.isSplitTopicAndPartitionLabelInPrometheus()); 
        
               if (pendingMetricsProviders != null) { 
        
                   pendingMetricsProviders.forEach(provider -> metricsServlet.addRawMetricsProvider(provider)); 
        
                   this.pendingMetricsProviders = null; 
        
               } 
        
           }

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

Lines 1673 to 1682 in 63d4cf2

    
           public synchronized void addPrometheusRawMetricsProvider(PrometheusRawMetricsProvider metricsProvider) { 
        
               if (metricsServlet == null) { 
        
                   if (pendingMetricsProviders == null) { 
        
                       pendingMetricsProviders = new LinkedList<>(); 
        
                   } 
        
                   pendingMetricsProviders.add(metricsProvider); 
        
               } else { 
        
                   this.metricsServlet.addRawMetricsProvider(metricsProvider); 
        
               } 
        
           }

hangc0276 · 2022-09-22T14:53:18Z

@asafm @tjiuming Would you please help take a look? thanks.

Done @hangc0276

@asafm Thanks for your review, I addressed all the comments, please help take a look again, thanks a lot.

asafm · 2022-09-22T18:07:02Z

For future readers of this PR:
Why is this PR needed? Pulsar uses Bookkeeper Client, which has metrics. The way to get those metrics is to implement the BK Metric library API, which Pulsar has already done through PrometheusMetricsProvider, PrometheusStatsLogger, etc. The problem with that implementation was that the API itself supported having labels per metric, but the implementation ignored the labels provided to it.
This PR aims to fix and add support for the labels given to it via the API methods. The impact is that now Pulsar /metrics endpoint, which exposes the Pulsar metrics in Prometheus format, will contain the Bookkeeper client metrics with labels (which were previously stripped).

@asafm Thanks for your explanation. Another purpose of this Pr is to support plugin metrics integrate with Pulsar broker's metrics through the interface.

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

Lines 904 to 914 in 63d4cf2

private synchronized void createMetricsServlet() {

this.metricsServlet = new PulsarPrometheusMetricsServlet(

this, config.isExposeTopicLevelMetricsInPrometheus(),

config.isExposeConsumerLevelMetricsInPrometheus(),

config.isExposeProducerLevelMetricsInPrometheus(),

config.isSplitTopicAndPartitionLabelInPrometheus());

if (pendingMetricsProviders != null) {

pendingMetricsProviders.forEach(provider -> metricsServlet.addRawMetricsProvider(provider));

this.pendingMetricsProviders = null;

}

}

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

Lines 1673 to 1682 in 63d4cf2

public synchronized void addPrometheusRawMetricsProvider(PrometheusRawMetricsProvider metricsProvider) {

if (metricsServlet == null) {

if (pendingMetricsProviders == null) {

pendingMetricsProviders = new LinkedList<>();

}

pendingMetricsProviders.add(metricsProvider);

} else {

this.metricsServlet.addRawMetricsProvider(metricsProvider);

}

}

How do you provide such support in your PR - that I haven't figured out yet

asafm · 2022-09-22T18:08:20Z

BTW: Force push is not good - I lose all context - I can't see only the changes you've added

hangc0276 · 2022-09-26T15:19:02Z

This PR changed the metric name for BK client, and it can only be released in the major version and needs to update the doc. /cc @Anonymitaet

asafm · 2022-09-29T12:39:15Z

...oker/src/main/java/org/apache/pulsar/broker/stats/prometheus/PrometheusMetricsGenerator.java

-        } catch (IOException e) {
-            // nop
-        }
+        ((PrometheusMetricsProvider) statsProvider).generate(stream);


It's weird to have a cast in software you wrote - you usually see this when you use third-party software. Why can't you change the original variable type?

asafm · 2022-09-29T14:33:34Z

...oker/src/main/java/org/apache/pulsar/broker/stats/prometheus/metrics/CachingStatsLogger.java

+
+    @Override
+    public OpStatsLogger getOpStatsLogger(String name) {
+        return opStatsLoggers.computeIfAbsent(scopeContext(name), x -> underlying.getOpStatsLogger(name));


Why would you cache it under key = scopeContext(name) and not just name?

asafm · 2022-09-29T14:37:45Z

...oker/src/main/java/org/apache/pulsar/broker/stats/prometheus/metrics/CachingStatsLogger.java

+    }
+
+    /**
+     Thread-scoped stats not currently supported.


Why is it not supported?

asafm · 2022-09-29T17:30:07Z

...c/main/java/org/apache/pulsar/broker/stats/prometheus/metrics/PrometheusMetricsProvider.java

+ * A <i>Prometheus</i> based {@link PrometheusRawMetricsProvider} implementation.
 */
-public class PrometheusMetricsProvider implements StatsProvider {
+public class PrometheusMetricsProvider implements StatsProvider, PrometheusRawMetricsProvider {


Based on my reply to the comment, I think it's redundant. We should have a single PrometheusMetricsProvider in PulsarService, and its metrics should be written explicitly in PrometheusMetricsGenerator

asafm · 2022-09-29T17:37:54Z

...er/src/main/java/org/apache/pulsar/broker/stats/prometheus/metrics/PrometheusTextFormat.java

+        // Example:
+        // # TYPE bookie_storage_entries_count gauge
+        // bookie_storage_entries_count 519
+        writeType(w, name, "gauge");


Now that you have grouped the metrics together by metric name, the type needs to be written once per metric name, so called from outside.

github-actions · 2022-10-30T02:16:17Z

The pr had no activity for 30 days, mark with Stale label.

tjiuming · 2022-10-30T19:06:10Z

I just wandering that why don't we use Prometheus lib, I think this implementation is hard to maintain

github-actions · 2022-12-01T02:12:50Z

The pr had no activity for 30 days, mark with Stale label.

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 8, 2022

hangc0276 requested review from 315157973, Jason918, Technoboy-, codelipenghui, dlg99, lhotari, merlimat and zymap September 8, 2022 01:12

hangc0276 self-assigned this Sep 8, 2022

hangc0276 added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/metrics release/2.9.4 release/2.8.5 release/2.11.1 release/2.10.3 labels Sep 8, 2022

hangc0276 added this to the 2.12.0 milestone Sep 8, 2022

Technoboy- force-pushed the chenhang/add_prometheusRawMetricsProvider_support branch from d1838c7 to 70444ec Compare September 8, 2022 02:34

asafm requested changes Sep 21, 2022

View reviewed changes

hangc0276 added 3 commits September 22, 2022 17:32

add prometheusRawMetricsProvider support

ff6ca6e

format code

a883290

address comments

2c10276

hangc0276 force-pushed the chenhang/add_prometheusRawMetricsProvider_support branch from 70444ec to 2c10276 Compare September 22, 2022 14:36

add CachingStatsLogger and test

6cf8f62

hangc0276 removed release/2.9.4 release/2.8.5 release/2.11.1 release/2.10.3 labels Sep 26, 2022

github-actions bot added doc-required Your PR changes impact docs and you will update later. and removed doc-not-needed Your PR changes do not impact docs labels Sep 26, 2022

ensure all the metrics with the same metric name are grouped togethoer

8704521

asafm reviewed Sep 29, 2022

View reviewed changes

github-actions bot added the Stale label Oct 30, 2022

github-actions bot removed the Stale label Oct 31, 2022

github-actions bot added the Stale label Dec 1, 2022

poorbarcode modified the milestones: 3.0.0, 3.1.0 Apr 10, 2023

Anonymitaet modified the milestones: 3.1.0, 3.2.0 Jul 24, 2023

Technoboy- added this to the 3.2.0 milestone Jul 31, 2023

Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023

coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024

lhotari modified the milestones: 4.0.0, 4.1.0 Oct 14, 2024

coderzc modified the milestones: 4.1.0, 4.2.0 Sep 1, 2025

Conversation

hangc0276 commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modification

Documentation

Matching PR in the forked repository

Uh oh!

hangc0276 commented Sep 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asafm commented Sep 21, 2022

Uh oh!

asafm commented Sep 21, 2022

Uh oh!

hangc0276 commented Sep 22, 2022

Uh oh!

hangc0276 commented Sep 22, 2022

Uh oh!

asafm commented Sep 22, 2022

Uh oh!

asafm commented Sep 22, 2022

Uh oh!

hangc0276 commented Sep 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hangc0276 commented Sep 8, 2022 •

edited

Loading