Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy #14681

cbornet · 2022-03-14T16:27:02Z

Motivation

PR #9021 added support for PrometheusRawMetricsProvider in the broker so that plugins can add their metrics to the Prometheus /metrics endpoint.
This change does the same for the Pulsar Proxy. Proxy plugins (additional servlets, extensions, ...) can then add their Prometheus providers with ProxyService::addPrometheusRawMetricsProvider.
This helps having common metrics code for servlets and handlers/extensions when they can be deployed both in the broker and the proxy.

Modifications

PrometheusRawMetricsProvider moved from pulsar-broker to pulsar-broker-common under the same package name so the change is non-breaking.
Make the PrometheusMetricsServlet non-specific to the broker and have another PulsarPrometheusMetricsServlet that extends it for the broker.
Extract PrometheusMetricsGenerator methods used by both the broker and the proxy to a PrometheusMetricsGeneratorUtils class.
Add a ProxyService::addPrometheusRawMetricsProvider method for use by plugins.
Make the proxy use PrometheusMetricsServlet instead of the classical MetricsServlet and add registered PrometheusRawMetricsProvider to it on WebServer startup. PrometheusMetricsServlet is aware of the Prometheus CollectorRegistry so it's backward compatible for existing plugins that are registering standard Prometheus metrics.
Add a test for the Proxy Prometheus /metrics endpoint.

Verifying this change

Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as org.apache.pulsar.broker.stats.PrometheusMetricsTest.
This change added tests and can be verified by running org.apache.pulsar.proxy.server.ProxyPrometheusMetricsTest

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) no
The public API: (yes / no) no
The schema: (yes / no / don't know) no
The default values of configurations: (yes / no) no
The wire protocol: (yes / no) no
The rest endpoints: (yes / no) no
The admin cli options: (yes / no) no
Anything that affects deployment: (yes / no / don't know) no

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

doc-required

(If you need help on updating docs, create a doc issue)
no-need-doc

This should be first documented for the broker.
doc

(If this PR contains doc changes)

eolivelli

Really great work !

IIUC this change is totally backward compatible

lhotari

LGTM, good work @cbornet

michaeljmarshall

Overall, looks good.

I think we need to address metrics providers that are added late to ensure they are added to the metricsServlet.

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyServiceStarter.java

michaeljmarshall · 2022-03-14T17:11:15Z

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java

+    public void addPrometheusRawMetricsProvider(PrometheusRawMetricsProvider metricsProvider) {
+        metricsProviders.add(metricsProvider);
+    }


Since this method could be called from other threads in protocol handlers, I think we should consider using a concurrent data structure to back the metricsProviders list. Technically, it appears that the current implementation in the broker does not use a concurrent data structure, so I could be wrong about this class's thread safety requirements.

@codelipenghui as you contributed the code for the broker, can you say if we should use concurrent structures for PulsarService's pendingMetricsProviders ?

Initially, it was only considered when loading plugins when starting the broker, which will not be modified after the broker started. To avoid any potential thread-safety issues(maybe some users want to modify after the broker started), using concurrent structures looks good to me.

dlg99

+1 to @michaeljmarshall 's question, otherwise LGTM to me

codelipenghui

LGTM, just left a minor comment.

codelipenghui · 2022-03-15T01:34:04Z

...common/src/main/java/org/apache/pulsar/broker/stats/prometheus/PrometheusMetricsServlet.java

+    private static final int HTTP_STATUS_OK_200 = 200;
+    private static final int HTTP_STATUS_INTERNAL_SERVER_ERROR_500 = 500;


Any reason for introducing new http status code?

@codelipenghui
because we are not importing org.eclipse.jetty.http.HttpStatus anymore
500 is used below

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java

eolivelli · 2022-03-15T10:58:08Z

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

-        if (metricsServlet == null) {
-            if (pendingMetricsProviders == null) {
-                pendingMetricsProviders = new LinkedList<>();
+        synchronized (this) {


what about moving 'synchronized' to the signature of the method ?

eolivelli · 2022-03-15T10:59:17Z

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java

-        if (pendingMetricsProviders != null) {
-            pendingMetricsProviders.forEach(provider -> metricsServlet.addRawMetricsProvider(provider));
-            this.pendingMetricsProviders = null;
+        synchronized (this) {


what about creating a method like this:

private synchronised void initMetricsServlet() { this.metricsServlet = new PrometheusMetricsServlet(-1L, proxyConfig.getClusterName()); if (pendingMetricsProviders != null) { pendingMetricsProviders.forEach(provider -> metricsServlet.addRawMetricsProvider(provider)); this.pendingMetricsProviders = null; } }

eolivelli · 2022-03-15T11:00:18Z

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java

        }

-        metricsServlet = null;
+        synchronized (this) {


what about using a synchronised method here instead of using "synchronized (this)" ?
probably it is a matter of taste, but I think that "synchronized (this)" is quite hard to read

eolivelli · 2022-03-15T11:00:35Z

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java

-        if (metricsServlet == null) {
-            if (pendingMetricsProviders == null) {
-                pendingMetricsProviders = new LinkedList<>();
+        synchronized (this) {


what about moving 'synchronized' to the signature of the method ?

eolivelli · 2022-03-15T11:01:06Z

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

-                pendingMetricsProviders.forEach(provider -> metricsServlet.addRawMetricsProvider(provider));
-                this.pendingMetricsProviders = null;
+
+            synchronized (this) {


what about moving this code to a dedicated method ?

private synchronised void initMetricsServlet() { .. }

eolivelli · 2022-03-15T11:27:32Z

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java

        }
    }

+    private synchronized void resetMetricsServlet() {


getMetricsServlet should be synchronized as well (spotbugs should not pass)

eolivelli · 2022-03-15T11:29:42Z

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

-                this.pendingMetricsProviders = null;
-            }
-
+            createMetricsServlet();


below we have a non synchronised access to metricsServlet.
I suggest to use a synchronised getter and use it while calling addWebServerHandlers

There should be only one thread here.

eolivelli

Lgtm

@michaeljmarshall please take another look

michaeljmarshall

LGTM. Thanks for addressing my comments @cbornet!

…che#14681)

) (cherry picked from commit ea95b28)

Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy

5c31ad0

eolivelli approved these changes Mar 14, 2022

View reviewed changes

lhotari approved these changes Mar 14, 2022

View reviewed changes

michaeljmarshall requested changes Mar 14, 2022

View reviewed changes

michaeljmarshall assigned cbornet Mar 14, 2022

michaeljmarshall added this to the 2.11.0 milestone Mar 14, 2022

michaeljmarshall added area/proxy area/metrics labels Mar 14, 2022

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

Handle late registration of PrometheusRawMetricsProvider

c21738d

michaeljmarshall requested review from codelipenghui, congbobo184 and hezhangjian March 14, 2022 18:53

dlg99 reviewed Mar 14, 2022

View reviewed changes

codelipenghui approved these changes Mar 15, 2022

View reviewed changes

eolivelli reviewed Mar 15, 2022

View reviewed changes

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java Show resolved Hide resolved

cbornet force-pushed the proxy-metrics branch from 867afdb to d32291e Compare March 15, 2022 10:19

eolivelli requested changes Mar 15, 2022

View reviewed changes

cbornet force-pushed the proxy-metrics branch from d32291e to 0257695 Compare March 15, 2022 11:20

eolivelli requested changes Mar 15, 2022

View reviewed changes

Synchronise modifications of metricsServlet and pendingMetricsProviders

3a11280

cbornet force-pushed the proxy-metrics branch from 0257695 to 3a11280 Compare March 15, 2022 14:50

dlg99 approved these changes Mar 15, 2022

View reviewed changes

eolivelli requested a review from michaeljmarshall March 15, 2022 19:57

eolivelli approved these changes Mar 15, 2022

View reviewed changes

michaeljmarshall approved these changes Mar 16, 2022

View reviewed changes

codelipenghui merged commit ea95b28 into apache:master Mar 16, 2022

cbornet deleted the proxy-metrics branch March 16, 2022 15:03

aparajita89 pushed a commit to aparajita89/pulsar that referenced this pull request Mar 21, 2022

Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy (apa…

5026a30

…che#14681)

Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022

Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy (apa…

8ac1ce4

…che#14681)

eolivelli pushed a commit that referenced this pull request Apr 27, 2022

Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy (#14681

b2f54ba

) (cherry picked from commit ea95b28)

eolivelli added release/2.10.1 cherry-picked/branch-2.10 labels Apr 27, 2022

		private static final int HTTP_STATUS_OK_200 = 200;
		private static final int HTTP_STATUS_INTERNAL_SERVER_ERROR_500 = 500;

Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy #14681

Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy #14681

Uh oh!

Conversation

cbornet commented Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

lhotari left a comment

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

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!

dlg99 left a comment

Choose a reason for hiding this comment

Uh oh!

codelipenghui left a comment

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!

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!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cbornet commented Mar 14, 2022 •

edited

Loading