[GitHub] [zookeeper] li4wang commented on a change in pull request #1644: ZOOKEEPER-4211: Expose Per Namespace Quota Metrics to Prometheus

[GitBox <gi...@apache.org>]

li4wang commented on a change in pull request #1644:
URL: https://github.com/apache/zookeeper/pull/1644#discussion_r598217478



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/ServerMetrics.java
##########
@@ -259,6 +260,8 @@ private ServerMetrics(MetricsProvider metricsProvider) {
         WATCH_BYTES = metricsContext.getCounter("watch_bytes");
 
         JVM_PAUSE_TIME = metricsContext.getSummary("jvm_pause_time_ms", DetailLevel.ADVANCED);
+
+        QUOTA_EXCEEDED_ERROR_PER_NAMESPACE = metricsContext.getSummarySet(QuotaMetricsUtils.QUOTA_EXCEEDED_ERROR_PER_NAMESPACE, DetailLevel.BASIC);

Review comment:
       SummarySet is used because it provides count (i.e. cnt) and the grouping capability.  Yes, you are right, in this case, since the count is always added by 1, the cnt and sum are the same with PrometheusMetricsProvider and the min/max/avg with DefaultMetricsProvider are always 1 in this case.  User can just use the cnt and ignore the others.
   
   Is there any reason that CounterSet is not supported? Are you think we should add a CounterSet in this PR?
   
   I added GaugeSet for other quota metrics, as SummarySet really can not be used for them.
   
   
   




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org