[GitHub] [pulsar] asafm commented on a diff in pull request #18102: [improve][broker] Add gzip compression support for /metrics endpoint

[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