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/10/19 05:40:32 UTC
[GitHub] [pulsar] xuesongxs opened a new pull request, #18102: [improve][broker] Add gzip compression support for /metrics endpoint
xuesongxs opened a new pull request, #18102:
URL: https://github.com/apache/pulsar/pull/18102
Fixes #16321
Replace the PR: https://github.com/apache/pulsar/pull/16888
--
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] github-actions[bot] commented on pull request #18102: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #18102:
URL: https://github.com/apache/pulsar/pull/18102#issuecomment-1364436737
The pr had no activity for 30 days, mark with Stale label.
--
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] github-actions[bot] commented on pull request #18102: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #18102:
URL: https://github.com/apache/pulsar/pull/18102#issuecomment-1283472193
@xuesongxs Please add the following content to your PR description and select a checkbox:
```
- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
```
--
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] xuesongxs commented on a diff in pull request #18102: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
xuesongxs commented on code in PR #18102:
URL: https://github.com/apache/pulsar/pull/18102#discussion_r1000060836
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -327,6 +327,18 @@ public class ServiceConfiguration implements PulsarConfiguration {
+ "(0 to disable limiting)")
private int maxHttpServerConnections = 2048;
+ @FieldContext(
+ category = CATEGORY_SERVER,
+ doc = "Enable or disable compress output metrics in prometheus"
Review Comment:
> Since it's only Gzip, let's write it?
The rest of the work will be left to you.
--
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
Re: [PR] [improve][broker] Add gzip compression support for /metrics endpoint [pulsar]
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #18102:
URL: https://github.com/apache/pulsar/pull/18102#discussion_r1407602598
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/web/WebServiceTest.java:
##########
@@ -349,6 +352,34 @@ public void testBrokerReady() throws Exception {
assertEquals(res.getResponseBody(), "ok");
}
+ @Test
+ public void testCompressOutputMetricsInPrometheus() throws Exception {
+ compressPrometheusMetricsOutput = true;
+ setupEnv(true, "1.0", true, false, false, false, -1, false);
+
+ String metricsUrl = pulsar.getWebServiceAddress() + "/metrics";
+
+ HttpClient client = new HttpClient();
+ client.start();
+ ContentResponse response = client.GET(metricsUrl);
+ assertEquals(response.getStatus(), 200);
+ assertEquals(response.getHeaders().get("Content-Encoding"), "gzip");
Review Comment:
We need to test the response payload at the same time.
--
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] lhotari commented on pull request #18102: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #18102:
URL: https://github.com/apache/pulsar/pull/18102#issuecomment-1324675769
Regarding the comment by @asafm in https://github.com/apache/pulsar/pull/16888#issuecomment-1233894144 , I think that all endpoints would benefit of compression. Having the compression support doesn't mean that the response will always be compressed, it's only if the client sends a `Accept-Encoding: gzip` header.
--
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
Re: [PR] [improve][broker] Add gzip compression support for /metrics endpoint [pulsar]
Posted by "asafm (via GitHub)" <gi...@apache.org>.
asafm commented on PR #18102:
URL: https://github.com/apache/pulsar/pull/18102#issuecomment-1831703737
I still have pending comments from 2022 @hangc0276 :)
--
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
Re: [PR] [improve][broker] Add gzip compression support for /metrics endpoint [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari closed pull request #18102: [improve][broker] Add gzip compression support for /metrics endpoint
URL: https://github.com/apache/pulsar/pull/18102
--
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] asafm commented on a diff in pull request #18102: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
asafm commented on code in PR #18102:
URL: https://github.com/apache/pulsar/pull/18102#discussion_r1000584617
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -327,6 +327,18 @@ public class ServiceConfiguration implements PulsarConfiguration {
+ "(0 to disable limiting)")
private int maxHttpServerConnections = 2048;
+ @FieldContext(
+ category = CATEGORY_SERVER,
+ doc = "Enable or disable compress output metrics in prometheus"
Review Comment:
`Enable or disable compress output metrics in prometheus` --> `Enable or disable GZip compression of Prometheus metrics endpoint response`
--
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] asafm commented on a diff in pull request #18102: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
asafm commented on code in PR #18102:
URL: https://github.com/apache/pulsar/pull/18102#discussion_r999595092
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java:
##########
@@ -262,7 +263,16 @@ public void addServlet(String path, ServletHolder servletHolder, boolean require
});
}
filterInitializer.addFilters(context, requiresAuthentication);
- handlers.add(context);
+ // Enable compress on /metrics endpoint
+ if (isCompress) {
+ GzipHandler gzipHandler = new GzipHandler();
+ gzipHandler.setMinGzipSize(pulsar.getConfiguration().getMinGzipSize());
Review Comment:
So you can only enable compression for the Prometheus servlet, but decide min gzip size on any? This doesn't make sense to me.
I would leave`addServlet` method as is, uncompressed, and add another method accepting `CompressionConfiguration(enabled, minResponseSize)`
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -327,6 +327,18 @@ public class ServiceConfiguration implements PulsarConfiguration {
+ "(0 to disable limiting)")
private int maxHttpServerConnections = 2048;
+ @FieldContext(
+ category = CATEGORY_SERVER,
+ doc = "Enable or disable compress output metrics in prometheus"
+ )
+ private boolean compressOutputMetricsInPrometheus = false;
Review Comment:
It's not really in Prometheus, so maybe `compressPrometheusMetricsOutput`?
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -327,6 +327,18 @@ public class ServiceConfiguration implements PulsarConfiguration {
+ "(0 to disable limiting)")
private int maxHttpServerConnections = 2048;
+ @FieldContext(
+ category = CATEGORY_SERVER,
+ doc = "Enable or disable compress output metrics in prometheus"
Review Comment:
Since it's only Gzip, let's write it?
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -327,6 +327,18 @@ public class ServiceConfiguration implements PulsarConfiguration {
+ "(0 to disable limiting)")
private int maxHttpServerConnections = 2048;
+ @FieldContext(
+ category = CATEGORY_SERVER,
+ doc = "Enable or disable compress output metrics in prometheus"
+ )
+ private boolean compressOutputMetricsInPrometheus = false;
+
+ @FieldContext(
+ category = CATEGORY_SERVER,
+ doc = "Set the minimum response size to trigger dynamic compression"
+ )
+ private int minGzipSize = 32;
Review Comment:
The name is not linked in any way to the other property. Perhaps `compressPrometheusMetricsOutputMinResponseSize`?
--
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
Re: [PR] [improve][broker] Add gzip compression support for /metrics endpoint [pulsar]
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #18102:
URL: https://github.com/apache/pulsar/pull/18102#issuecomment-1829699728
@xuesongxs Please rebase the 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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] xuesongxs commented on a diff in pull request #18102: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
xuesongxs commented on code in PR #18102:
URL: https://github.com/apache/pulsar/pull/18102#discussion_r1000059960
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -327,6 +327,18 @@ public class ServiceConfiguration implements PulsarConfiguration {
+ "(0 to disable limiting)")
private int maxHttpServerConnections = 2048;
+ @FieldContext(
+ category = CATEGORY_SERVER,
+ doc = "Enable or disable compress output metrics in prometheus"
Review Comment:
> Since it's only Gzip, let's write it?
Ok, you'd better revise it.
--
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
Re: [PR] [improve][broker] Add gzip compression support for /metrics endpoint [pulsar]
Posted by "asafm (via GitHub)" <gi...@apache.org>.
asafm commented on PR #18102:
URL: https://github.com/apache/pulsar/pull/18102#issuecomment-1831702622
> I tested the Prometheus metric with 1000 topics and enabled the compression to get 33x data saving.
>
> ```
> # curl -H 'Accept-Encoding: gzip' -o kk0.log.gz http://localhost:8080/metrics/
> % Total % Received % Xferd Average Speed Time Time Time Current
> Dload Upload Total Spent Left Speed
> 100 927k 0 927k 0 0 1910k 0 --:--:-- --:--:-- --:--:-- 1941k
> # curl -o kk1.log http://localhost:8080/metrics/
> % Total % Received % Xferd Average Speed Time Time Time Current
> Dload Upload Total Spent Left Speed
> 100 33.2M 0 33.2M 0 0 73.1M 0 --:--:-- --:--:-- --:--:-- 74.4M
> # ll -lrt |grep kk
> -rw-r--r-- 1 hangc staff 928K Nov 28 18:46 kk0.log.gz
> -rw-r--r-- 1 hangc staff 33M Nov 28 18:46 kk1.log
> ```
>
> Due to the response being compressed according to the client request header, I think we can add the GzipHandler for all the endpoints instead of the metric endpoint.
>
> However, the Prometheus server sends a fetch metric request with `Accept-Encoding: gzip` header, which will fetch the metrics with compression. [prometheus/prometheus#12319](https://github.com/prometheus/prometheus/issues/12319). It may cause the Prometheus server to run into high CPU usage. [prometheus/blackbox_exporter#270](https://github.com/prometheus/blackbox_exporter/issues/270).
>
> For the master branch of Prometheus supports disabling compression in the client request header [prometheus/prometheus#13166](https://github.com/prometheus/prometheus/pull/13166), I'm not sure if we need to add a flag to control whether the Pulsar HTTP server supports compression or not. @lhotari @asafm @codelipenghui Do you guys have any ideas?
Since `io.prometheus.client.exporter.HTTPServer` supports returning compresses responses based on headers, Pulsar shouldn't be different.
--
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
Re: [PR] [improve][broker] Add gzip compression support for /metrics endpoint [pulsar]
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #18102:
URL: https://github.com/apache/pulsar/pull/18102#issuecomment-1829642655
I tested the Prometheus metric with 1000 topics and enabled the compression to get 33x data saving.
```
# curl -H 'Accept-Encoding: gzip' -o kk0.log.gz http://localhost:8080/metrics/
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 927k 0 927k 0 0 1910k 0 --:--:-- --:--:-- --:--:-- 1941k
# curl -o kk1.log http://localhost:8080/metrics/
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 33.2M 0 33.2M 0 0 73.1M 0 --:--:-- --:--:-- --:--:-- 74.4M
# ll -lrt |grep kk
-rw-r--r-- 1 hangc staff 928K Nov 28 18:46 kk0.log.gz
-rw-r--r-- 1 hangc staff 33M Nov 28 18:46 kk1.log
```
Due to the response being compressed according to the client request header, I think we can add the GzipHandler for all the endpoints instead of the metric endpoint.
However, the Prometheus server sends a fetch metric request with `Accept-Encoding: gzip` header, which will fetch the metrics with compression. https://github.com/prometheus/prometheus/issues/12319. It may cause the Prometheus server to run into high CPU usage. https://github.com/prometheus/blackbox_exporter/issues/270.
For the master branch of Prometheus supports disabling compression in the client request header https://github.com/prometheus/prometheus/pull/13166, I'm not sure if we need to add a flag to control whether the Pulsar HTTP server supports compression or not. @lhotari @asafm @codelipenghui Do you guys have any ideas?
--
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
Re: [PR] [improve][broker] Add gzip compression support for /metrics endpoint [pulsar]
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #18102:
URL: https://github.com/apache/pulsar/pull/18102#discussion_r1407652362
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java:
##########
@@ -999,7 +1000,7 @@ private void addBrokerAdditionalServlets(WebService webService,
((AdditionalServletWithPulsarService) additionalServlet).setPulsarService(this);
}
webService.addServlet(servletWithClassLoader.getBasePath(), servletWithClassLoader.getServletHolder(),
- config.isAuthenticationEnabled(), attributeMap);
+ config.isAuthenticationEnabled(), attributeMap, false);
Review Comment:
Can we apply this GzipHandle support for all the endpoints instead of the metric endpoint? It will benefit for all the REST response.
##########
conf/broker.conf:
##########
@@ -1428,6 +1428,12 @@ metricsServletTimeoutMs=30000
# Enable or disable broker bundles metrics. The default value is false.
exposeBundlesMetricsInPrometheus=false
+# Enable or disable GZip compression of Prometheus metrics endpoint response. The default value is false.
+compressPrometheusMetricsOutput=false
+
+# Set the minimum response size to trigger dynamic compression. The default value is 32.
Review Comment:
Can we use the default value and remove this configuration?
--
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
Re: [PR] [improve][broker] Add gzip compression support for /metrics endpoint [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #18102:
URL: https://github.com/apache/pulsar/pull/18102#issuecomment-1966201327
Replaced by #21667
--
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