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/07/31 04:28:44 UTC
[GitHub] [pulsar] xuesongxs opened a new pull request, #16888: [Improve][broker] Add gzip compression before http service response
xuesongxs opened a new pull request, #16888:
URL: https://github.com/apache/pulsar/pull/16888
Fixes #16321
--
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 pull request #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1275188116
@xuesongxs - it looks like this PR stalled. It'd be great to move this along and get it merged. Do you plan on continuing work for this PR? Is the issue CI? Thanks!
--
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 diff in pull request #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#discussion_r960239598
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java:
##########
@@ -263,7 +264,14 @@ public void addServlet(String path, ServletHolder servletHolder, boolean require
});
}
filterInitializer.addFilters(context, requiresAuthentication);
- handlers.add(context);
+ // Enable compress on /metrics endpoint
+ if (path.equals("/metrics") && pulsar.getConfiguration().isCompressOutputMetricsInPrometheus()) {
Review Comment:
Great point @asafm.
##########
conf/broker.conf:
##########
@@ -1380,6 +1380,9 @@ metricsServletTimeoutMs=30000
# Enable or disable broker bundles metrics. The default value is false.
exposeBundlesMetricsInPrometheus=false
+# Enable or disable compress output metrics in prometheus. The default value is false.
+compressOutputMetricsInPrometheus=false
Review Comment:
Can we make it configurable which type of compression to use instead of making this a boolean?
--
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 #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1232467347
@xuesongxs please rebase 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] github-actions[bot] commented on pull request #16888: [improve][broker] Add gzip compression before http service response
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1200353478
@xuesongxs Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
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 diff in pull request #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#discussion_r960236341
##########
conf/broker.conf:
##########
@@ -1380,6 +1380,9 @@ metricsServletTimeoutMs=30000
# Enable or disable broker bundles metrics. The default value is false.
exposeBundlesMetricsInPrometheus=false
+# Enable or disable compress output metrics in prometheus. The default value is false.
+compressOutputMetricsInPrometheus=false
Review Comment:
Can we make it configurable which type of compression to use instead of making this a boolean? Even if we only initially support gzip, it would make it easier to expand this feature later if the configuration were the name of the algorithm.
--
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] tjiuming commented on pull request #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1232620607
besides, over all LGTM
--
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] nodece commented on a diff in pull request #16888: [improve][broker] Add gzip compression before http service response
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#discussion_r937440248
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java:
##########
@@ -263,6 +264,14 @@ public void addServlet(String path, ServletHolder servletHolder, boolean require
});
}
filterInitializer.addFilters(context, requiresAuthentication);
+ // Enable compress on /metrics endpoint
+ if (path.equals("/metrics") && pulsar.getConfiguration().isEnableCompressMetricsData()) {
+ GzipHandler gzipHandler = new GzipHandler();
+ gzipHandler.setHandler(context);
+ handlers.add(gzipHandler);
+ } else {
+ handlers.add(context);
+ }
handlers.add(context);
Review Comment:
```suggestion
```
--
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 pull request #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1233771038
> > Can we enable conditional compression that only compresses payloads above a certain size?
>
> Yes, that is supported in GzipHandler.
>
> https://www.eclipse.org/jetty/javadoc/jetty-9/org/eclipse/jetty/server/handler/gzip/GzipHandler.html#setMinGzipSize(int)
That seems like a good indication to me that we should just enable it for all HTTP endpoints.
--
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 #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
asafm commented on code in PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#discussion_r959451354
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java:
##########
@@ -263,7 +264,14 @@ public void addServlet(String path, ServletHolder servletHolder, boolean require
});
}
filterInitializer.addFilters(context, requiresAuthentication);
- handlers.add(context);
+ // Enable compress on /metrics endpoint
+ if (path.equals("/metrics") && pulsar.getConfiguration().isCompressOutputMetricsInPrometheus()) {
Review Comment:
This is quite confusing for someone coming later to this part of code. I would expect this to be where the /metrics is defined.
I prefer adding a boolean argument to `addServlet` asking if this servlet should be GZIpped and if so, then add wrap the servlet context handler with a `GZIpHandler`.
Then you can tweak the call to it :
```java
// Add metrics servlet
webService.addServlet("/metrics",
new ServletHolder(metricsServlet),
config.isAuthenticateMetricsEndpoint(), attributeMap);
```
by adding the `isGzip` based on `pulsar.getConfiguration().isCompressOutputMetricsInPrometheus()`
When that is done, the person looking at the call will have all the information at the end, and we won't duplicate `/metrics` twice
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/web/WebServiceTest.java:
##########
@@ -349,6 +350,36 @@ public void testBrokerReady() throws Exception {
assertEquals(res.getResponseBody(), "ok");
}
+ @Test
+ public void testCompressOutputMetricsInPrometheus() throws Exception {
+ compressOutputMetricsInPrometheus = true;
+ setupEnv(true, "1.0", true, false, false, false, -1, false);
+
+ String metricsUrl = pulsar.getWebServiceAddress() + "/metrics";
+
+ @Cleanup
+ AsyncHttpClient metricsClient = new DefaultAsyncHttpClient();
+ Response metricsRes = metricsClient.prepareGet(metricsUrl).execute().get();
Review Comment:
1. I don't really understand how this actually checks if the response was GZipped compressed?
2. I'm not even sure Transfer-Encoding header is used in GzipHandler. Usually they use Content Encoding.
3. `chunked` is not encoding used but `gzip`
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/web/WebServiceTest.java:
##########
@@ -349,6 +350,36 @@ public void testBrokerReady() throws Exception {
assertEquals(res.getResponseBody(), "ok");
}
+ @Test
+ public void testCompressOutputMetricsInPrometheus() throws Exception {
Review Comment:
If you accept my suggestion above, then the test shouldn't be here, since this functionality is not related to `WebService` but to `PulsarService`
--
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 #16888: [improve][broker] Add gzip compression before http service response
Posted by GitBox <gi...@apache.org>.
xuesongxs commented on code in PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#discussion_r938382885
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java:
##########
@@ -263,6 +264,14 @@ public void addServlet(String path, ServletHolder servletHolder, boolean require
});
}
filterInitializer.addFilters(context, requiresAuthentication);
+ // Enable compress on /metrics endpoint
+ if (path.equals("/metrics") && pulsar.getConfiguration().isEnableCompressMetricsData()) {
+ GzipHandler gzipHandler = new GzipHandler();
+ gzipHandler.setHandler(context);
+ handlers.add(gzipHandler);
+ } else {
+ handlers.add(context);
+ }
handlers.add(context);
Review Comment:
> Yes, the code was copied without deletion.
--
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 #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
asafm commented on code in PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#discussion_r960335069
##########
conf/broker.conf:
##########
@@ -1380,6 +1380,9 @@ metricsServletTimeoutMs=30000
# Enable or disable broker bundles metrics. The default value is false.
exposeBundlesMetricsInPrometheus=false
+# Enable or disable compress output metrics in prometheus. The default value is false.
+compressOutputMetricsInPrometheus=false
Review Comment:
Maybe it's best to add such a configuration when we will add support for more compressions?
--
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 pull request #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
xuesongxs commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1283494321
I created a new PR to replace it.
https://github.com/apache/pulsar/pull/18102
From: Michael Marshall
Date: 2022-10-19 09:42
To: apache/pulsar
CC: xuesongxs; Mention
Subject: Re: [apache/pulsar] [improve][broker] Add gzip compression support for /metrics endpoint (PR #16888)
@xuesongxs - it looks like something went wrong with your most recent commit. The diff is now very large.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
--
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 #16888: [Improve][broker] Add gzip compression before http service response
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1200345834
@xuesongxs Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
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 closed pull request #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
xuesongxs closed pull request #16888: [improve][broker] Add gzip compression support for /metrics endpoint
URL: 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] michaeljmarshall commented on pull request #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1283263620
@xuesongxs - it looks like something went wrong with your most recent commit. The diff is now very large.
--
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] tjiuming commented on pull request #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1232608598
> Having gzip support for all http endpoints could be useful too. I wonder if it could be simply enabled for all by default. @codelipenghui wdyt?
I remember this PR and penghui said that not all the endpoints has a big response body, make all the endpoints output compressed response may lead to CPU usage increase
--
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 #16888: [improve][broker] Add gzip compression before http service response
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1232466108
@asafm @tjiuming please review
--
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 #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1233768383
> Can we enable conditional compression that only compresses payloads above a certain size?
Yes, that is supported in GzipHandler.
https://www.eclipse.org/jetty/javadoc/jetty-9/org/eclipse/jetty/server/handler/gzip/GzipHandler.html#setMinGzipSize(int)
--
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 diff in pull request #16888: [improve][broker] Add gzip compression before http service response
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#discussion_r940062040
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/web/WebServiceTest.java:
##########
@@ -416,6 +430,7 @@ private void setupEnv(boolean enableFilter, String minApiVersion, boolean allowU
config.setAdvertisedAddress("localhost"); // TLS certificate expects localhost
config.setMetadataStoreUrl("zk:localhost:2181");
config.setHttpMaxRequestSize(10 * 1024);
+ config.setEnableCompressMetricsData(true);
Review Comment:
This will change all the tests in this class run with compression enabled, and no tests to cover the case of without compression
##########
conf/broker.conf:
##########
@@ -833,6 +833,9 @@ maxHttpServerConnections=2048
# Max concurrent web requests
maxConcurrentHttpRequests=1024
+# enable compression metrics data when the HTTP service responds to the client
+enableCompressMetricsData=false
Review Comment:
compressOutputMetricsInPrometheus=false
Just make the name consistent with the existing flags of prometheus
And please move it under the `### --- Metrics --- ###` section
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/web/WebServiceTest.java:
##########
@@ -349,6 +349,20 @@ public void testBrokerReady() throws Exception {
assertEquals(res.getResponseBody(), "ok");
}
+ @Test
+ public void testCompressMetricsData() throws Exception {
Review Comment:
Is it able to check whether the data has been compressed? I think the test can also get passed if the flag is disabled.
--
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 #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
xuesongxs commented on code in PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#discussion_r960263732
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/web/WebServiceTest.java:
##########
@@ -349,6 +350,36 @@ public void testBrokerReady() throws Exception {
assertEquals(res.getResponseBody(), "ok");
}
+ @Test
+ public void testCompressOutputMetricsInPrometheus() throws Exception {
+ compressOutputMetricsInPrometheus = true;
+ setupEnv(true, "1.0", true, false, false, false, -1, false);
+
+ String metricsUrl = pulsar.getWebServiceAddress() + "/metrics";
+
+ @Cleanup
+ AsyncHttpClient metricsClient = new DefaultAsyncHttpClient();
+ Response metricsRes = metricsClient.prepareGet(metricsUrl).execute().get();
Review Comment:
```
HttpClient client = new HttpClient();
client.start();
ContentResponse response = client.GET(metricsUrl);
assertEquals(response.getHeaders().get("Content-Encoding"), "gzip");
client.stop();
```
This code should be used to check.
--
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 pull request #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
asafm commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1233894144
> > > Can we enable conditional compression that only compresses payloads above a certain size?
> >
> >
> > Yes, that is supported in GzipHandler.
> > https://www.eclipse.org/jetty/javadoc/jetty-9/org/eclipse/jetty/server/handler/gzip/GzipHandler.html#setMinGzipSize(int)
>
> That seems like a good indication to me that we should just enable it for all HTTP endpoints.
One interesting question here is if we experience any issues currently with other HTTP endpoints. In Prometheus, the shear response size of 300 MB is worth compressing since the issue with a timeout. I wonder which other endpoints have similar issues?
--
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] tjiuming commented on pull request #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1232611277
for the tests, I remember the HTTP response code is always 302, so they didn't passed, does it fixed?
--
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 #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1264529469
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] lhotari commented on a diff in pull request #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#discussion_r959260507
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/web/WebServiceTest.java:
##########
@@ -88,6 +88,7 @@ public class WebServiceTest {
private PulsarService pulsar;
private String BROKER_LOOKUP_URL;
private String BROKER_LOOKUP_URL_TLS;
+ private Boolean compressOutputMetricsInPrometheus;
Review Comment:
```suggestion
private boolean compressOutputMetricsInPrometheus;
```
--
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 pull request #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
asafm commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1232806697
> @asafm @tjiuming please review
@lhotari Done. I suggested several 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] michaeljmarshall commented on pull request #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1233761782
> > Having gzip support for all http endpoints could be useful too. I wonder if it could be simply enabled for all by default. @codelipenghui wdyt?
>
> I remember this PR and penghui said that not all the endpoints has a big response body, make all the endpoints output compressed response may lead to CPU usage increase
Can we enable conditional compression that only compresses payloads above a certain size?
--
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 #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1232469594
@addisonj please review
--
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 #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1232469244
Having gzip support for all http endpoints could be useful too. I wonder if it could be simply enabled for all by default. @codelipenghui wdyt?
--
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 #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1233295679
> @lhotari Done. I suggested several changes.
@asafm great work on the review! Thanks
--
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 pull request #16888: [improve][broker] Add gzip compression support for /metrics endpoint
Posted by GitBox <gi...@apache.org>.
xuesongxs commented on PR #16888:
URL: https://github.com/apache/pulsar/pull/16888#issuecomment-1275503739
> @xuesongxs - it looks like this PR stalled. It'd be great to move this along and get it merged. Do you plan on continuing work for this PR? Is the issue CI? Thanks!
I will continue to do this PR this week.
--
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