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

[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