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 2022/03/14 16:27:09 UTC
[GitHub] [pulsar] cbornet opened a new pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy
cbornet opened a new pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681
<!--
### Contribution Checklist
- Name the pull request in the form "[Issue XYZ][component] Title of the pull request", where *XYZ* should be replaced by the actual issue number.
Skip *Issue XYZ* if there is no associated github issue for this pull request.
Skip *component* if you are unsure about which is the best component. E.g. `[docs] Fix typo in produce method`.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
### 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 pul
### 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)
- [x] `no-need-doc`
This should be first documented for the broker.
- [ ] `doc`
(If this PR contains doc changes)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a change in pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy
Posted by GitBox <gi...@apache.org>.
cbornet commented on a change in pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681#discussion_r826752954
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java
##########
@@ -414,5 +428,16 @@ public Authentication getProxyClientAuthenticationPlugin() {
return this.proxyClientAuthentication;
}
+ public void addPrometheusRawMetricsProvider(PrometheusRawMetricsProvider metricsProvider) {
+ if (metricsServlet == null) {
+ if (pendingMetricsProviders == null) {
Review comment:
Yes the whole state machine must be synchronised. And so it's not necessary to use a concurrent structure.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a change in pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy
Posted by GitBox <gi...@apache.org>.
cbornet commented on a change in pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681#discussion_r827109405
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java
##########
@@ -356,6 +374,10 @@ public void close() throws IOException {
}
}
+ private synchronized void resetMetricsServlet() {
Review comment:
done
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681#discussion_r826191920
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java
##########
@@ -414,5 +418,9 @@ public Authentication getProxyClientAuthenticationPlugin() {
return this.proxyClientAuthentication;
}
+ public void addPrometheusRawMetricsProvider(PrometheusRawMetricsProvider metricsProvider) {
+ metricsProviders.add(metricsProvider);
+ }
Review comment:
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.
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyServiceStarter.java
##########
@@ -240,7 +240,10 @@ public static void addWebServerHandlers(WebServer server,
ProxyConfiguration config,
ProxyService service,
BrokerDiscoveryProvider discoveryProvider) throws Exception {
- server.addServlet("/metrics", new ServletHolder(MetricsServlet.class),
+ PrometheusMetricsServlet metricsServlet =
+ new PrometheusMetricsServlet(-1L, config.getClusterName());
+ service.getMetricsProviders().forEach(metricsServlet::addRawMetricsProvider);
Review comment:
Is there a race here that could prevent some metrics providers from getting added to the `metricsServlet`? In looking at the broker's implementation, it ensures that the `metricsServlet` is updated if the servlet has already been created.
https://github.com/apache/pulsar/blob/b0213b225f194b47a5dcd63ef4a26f55c4c820b6/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java#L1518-L1527
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a change in pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy
Posted by GitBox <gi...@apache.org>.
cbornet commented on a change in pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681#discussion_r827109899
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -697,16 +701,7 @@ public void start() throws PulsarServerException {
this.brokerAdditionalServlets = AdditionalServlets.load(config);
this.webService = new WebService(this);
- this.metricsServlet = new PrometheusMetricsServlet(
- this, config.isExposeTopicLevelMetricsInPrometheus(),
- config.isExposeConsumerLevelMetricsInPrometheus(),
- config.isExposeProducerLevelMetricsInPrometheus(),
- config.isSplitTopicAndPartitionLabelInPrometheus());
- if (pendingMetricsProviders != null) {
- pendingMetricsProviders.forEach(provider -> metricsServlet.addRawMetricsProvider(provider));
- this.pendingMetricsProviders = null;
- }
-
+ createMetricsServlet();
Review comment:
There should be only one thread here.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a change in pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy
Posted by GitBox <gi...@apache.org>.
cbornet commented on a change in pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681#discussion_r826261087
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyServiceStarter.java
##########
@@ -240,7 +240,10 @@ public static void addWebServerHandlers(WebServer server,
ProxyConfiguration config,
ProxyService service,
BrokerDiscoveryProvider discoveryProvider) throws Exception {
- server.addServlet("/metrics", new ServletHolder(MetricsServlet.class),
+ PrometheusMetricsServlet metricsServlet =
+ new PrometheusMetricsServlet(-1L, config.getClusterName());
+ service.getMetricsProviders().forEach(metricsServlet::addRawMetricsProvider);
Review comment:
I've added support for late registration similar to what is done in the broker. Can you check again ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a change in pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy
Posted by GitBox <gi...@apache.org>.
cbornet commented on a change in pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681#discussion_r826263896
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java
##########
@@ -414,5 +418,9 @@ public Authentication getProxyClientAuthenticationPlugin() {
return this.proxyClientAuthentication;
}
+ public void addPrometheusRawMetricsProvider(PrometheusRawMetricsProvider metricsProvider) {
+ metricsProviders.add(metricsProvider);
+ }
Review comment:
@codelipenghui as you contributed the code for the broker, can you say if we should use concurrent structures for `PulsarService`'s `pendingMetricsProviders` ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681#discussion_r826720567
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java
##########
@@ -414,5 +428,16 @@ public Authentication getProxyClientAuthenticationPlugin() {
return this.proxyClientAuthentication;
}
+ public void addPrometheusRawMetricsProvider(PrometheusRawMetricsProvider metricsProvider) {
+ if (metricsServlet == null) {
+ if (pendingMetricsProviders == null) {
Review comment:
if we want to support concurrency here, then we must ensure consistency here.
so:
- use `synchronised` to guard accesses to `metricsServlet` and to `pendingMetricsProviders`
- use a threadsafe List implementation and not `LinkedList` (this could not be needed if we guard with `synchronised` every access to the list
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681#discussion_r826503773
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java
##########
@@ -414,5 +418,9 @@ public Authentication getProxyClientAuthenticationPlugin() {
return this.proxyClientAuthentication;
}
+ public void addPrometheusRawMetricsProvider(PrometheusRawMetricsProvider metricsProvider) {
+ metricsProviders.add(metricsProvider);
+ }
Review comment:
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.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681#discussion_r826506648
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/stats/prometheus/PrometheusMetricsServlet.java
##########
@@ -28,36 +28,28 @@
import java.util.concurrent.Executors;
import javax.servlet.AsyncContext;
import javax.servlet.ServletException;
+import javax.servlet.ServletOutputStream;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
-import org.apache.pulsar.broker.PulsarService;
-import org.eclipse.jetty.http.HttpStatus;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class PrometheusMetricsServlet extends HttpServlet {
private static final long serialVersionUID = 1L;
+ private static final int HTTP_STATUS_OK_200 = 200;
+ private static final int HTTP_STATUS_INTERNAL_SERVER_ERROR_500 = 500;
Review comment:
Any reason for introducing new http status code?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a change in pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy
Posted by GitBox <gi...@apache.org>.
cbornet commented on a change in pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681#discussion_r826214827
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyServiceStarter.java
##########
@@ -240,7 +240,10 @@ public static void addWebServerHandlers(WebServer server,
ProxyConfiguration config,
ProxyService service,
BrokerDiscoveryProvider discoveryProvider) throws Exception {
- server.addServlet("/metrics", new ServletHolder(MetricsServlet.class),
+ PrometheusMetricsServlet metricsServlet =
+ new PrometheusMetricsServlet(-1L, config.getClusterName());
+ service.getMetricsProviders().forEach(metricsServlet::addRawMetricsProvider);
Review comment:
I was under the impression that PrometheusRawMetricsProviders would be added during the ProxyService startup and not after. Do we really need to handle the case that they are added later ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681#discussion_r826223760
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyServiceStarter.java
##########
@@ -240,7 +240,10 @@ public static void addWebServerHandlers(WebServer server,
ProxyConfiguration config,
ProxyService service,
BrokerDiscoveryProvider discoveryProvider) throws Exception {
- server.addServlet("/metrics", new ServletHolder(MetricsServlet.class),
+ PrometheusMetricsServlet metricsServlet =
+ new PrometheusMetricsServlet(-1L, config.getClusterName());
+ service.getMetricsProviders().forEach(metricsServlet::addRawMetricsProvider);
Review comment:
If I understand it correctly, I thought the protocol handlers themselves are calling the `addPrometheusRawMetricsProvider` method. As such, they could do so at any time. I agree that it's a case we could chose to not cover (as long as we document the behavior for proxy protocol handlers). My main thought is that it is a point of divergence from the broker's protocol handler implementation.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681#discussion_r826840769
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java
##########
@@ -429,13 +433,15 @@ public Authentication getProxyClientAuthenticationPlugin() {
}
public void addPrometheusRawMetricsProvider(PrometheusRawMetricsProvider metricsProvider) {
- if (metricsServlet == null) {
- if (pendingMetricsProviders == null) {
- pendingMetricsProviders = new LinkedList<>();
+ synchronized (this) {
Review comment:
what about moving 'synchronized' to the signature of the method ?
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java
##########
@@ -347,7 +349,9 @@ public void close() throws IOException {
proxyAdditionalServlets = null;
}
- metricsServlet = null;
+ synchronized (this) {
Review comment:
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
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java
##########
@@ -256,10 +256,12 @@ public void start() throws Exception {
this.serviceUrlTls = null;
}
- this.metricsServlet = new PrometheusMetricsServlet(-1L, proxyConfig.getClusterName());
- if (pendingMetricsProviders != null) {
- pendingMetricsProviders.forEach(provider -> metricsServlet.addRawMetricsProvider(provider));
- this.pendingMetricsProviders = null;
+ synchronized (this) {
Review comment:
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;
}
}
```
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -697,14 +699,17 @@ public void start() throws PulsarServerException {
this.brokerAdditionalServlets = AdditionalServlets.load(config);
this.webService = new WebService(this);
- 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;
+
+ synchronized (this) {
Review comment:
what about moving this code to a dedicated method ?
```
private synchronised void initMetricsServlet() {
..
}
```
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -1516,13 +1521,15 @@ public ResourceUsageTransportManager getResourceUsageTransportManager() {
}
public void addPrometheusRawMetricsProvider(PrometheusRawMetricsProvider metricsProvider) {
- if (metricsServlet == null) {
- if (pendingMetricsProviders == null) {
- pendingMetricsProviders = new LinkedList<>();
+ synchronized (this) {
Review comment:
what about moving 'synchronized' to the signature of the method ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681#discussion_r826287052
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyServiceStarter.java
##########
@@ -240,7 +240,10 @@ public static void addWebServerHandlers(WebServer server,
ProxyConfiguration config,
ProxyService service,
BrokerDiscoveryProvider discoveryProvider) throws Exception {
- server.addServlet("/metrics", new ServletHolder(MetricsServlet.class),
+ PrometheusMetricsServlet metricsServlet =
+ new PrometheusMetricsServlet(-1L, config.getClusterName());
+ service.getMetricsProviders().forEach(metricsServlet::addRawMetricsProvider);
Review comment:
Thanks @cbornet. That looks good to me.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681#discussion_r826868614
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -697,16 +701,7 @@ public void start() throws PulsarServerException {
this.brokerAdditionalServlets = AdditionalServlets.load(config);
this.webService = new WebService(this);
- this.metricsServlet = new PrometheusMetricsServlet(
- this, config.isExposeTopicLevelMetricsInPrometheus(),
- config.isExposeConsumerLevelMetricsInPrometheus(),
- config.isExposeProducerLevelMetricsInPrometheus(),
- config.isSplitTopicAndPartitionLabelInPrometheus());
- if (pendingMetricsProviders != null) {
- pendingMetricsProviders.forEach(provider -> metricsServlet.addRawMetricsProvider(provider));
- this.pendingMetricsProviders = null;
- }
-
+ createMetricsServlet();
Review comment:
below we have a non synchronised access to metricsServlet.
I suggest to use a `synchronised` getter and use it while calling `addWebServerHandlers`
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java
##########
@@ -356,6 +374,10 @@ public void close() throws IOException {
}
}
+ private synchronized void resetMetricsServlet() {
Review comment:
getMetricsServlet should be `synchronized` as well (spotbugs should not pass)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #14681: Add support of PrometheusRawMetricsProvider for the Pulsar-Proxy
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #14681:
URL: https://github.com/apache/pulsar/pull/14681#discussion_r826717953
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/stats/prometheus/PrometheusMetricsServlet.java
##########
@@ -28,36 +28,28 @@
import java.util.concurrent.Executors;
import javax.servlet.AsyncContext;
import javax.servlet.ServletException;
+import javax.servlet.ServletOutputStream;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
-import org.apache.pulsar.broker.PulsarService;
-import org.eclipse.jetty.http.HttpStatus;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class PrometheusMetricsServlet extends HttpServlet {
private static final long serialVersionUID = 1L;
+ private static final int HTTP_STATUS_OK_200 = 200;
+ private static final int HTTP_STATUS_INTERNAL_SERVER_ERROR_500 = 500;
Review comment:
@codelipenghui
because we are not importing org.eclipse.jetty.http.HttpStatus anymore
500 is used below
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org