You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/08/27 19:22:52 UTC
[GitHub] [cassandra] dcapwell commented on a change in pull request #719: Add metric for client concurrent byte throttle
dcapwell commented on a change in pull request #719:
URL: https://github.com/apache/cassandra/pull/719#discussion_r478637438
##########
File path: src/java/org/apache/cassandra/utils/EstimatedHistogram.java
##########
@@ -365,6 +370,57 @@ public int hashCode()
return Objects.hashCode(getBucketOffsets(), getBuckets(false));
}
+ public int size()
+ {
+ return toIntExact(count());
+ }
+
+ public void update(long l)
+ {
+ this.add(l);
+ }
+
+ static class EstimatedHistogramSnapshot extends UniformSnapshot
Review comment:
this class doesn't actually snapshot the data (mutations to the backing histogram after snapshot impact the snapshot's results), and produces confusing results (longs mean different things).
EstimatedHistogram buckets represent their values and the long in them are the counts of those values, UniformSnapshot expects the longs to be the values themselves.
A better way to deal with this is to keep this class, but snapshot by calling `new EstimatedHistogram(histogram.getBuckets(false))` and have this class extend `com.codahale.metrics.Snapshot` directly.
##########
File path: src/java/org/apache/cassandra/metrics/ClientMetrics.java
##########
@@ -85,6 +85,10 @@ public synchronized void init(Collection<Server> servers)
registerGauge("connectedNativeClientsByUser", this::countConnectedClientsByUser);
registerGauge("connections", this::connectedClients);
registerGauge("clientsByProtocolVersion", this::recentClientStats);
+ registerGauge("concurrentRequestsInBytes", this::concurrentRequestsInBytes);
+
+ Metrics.register(factory.createMetricName("concurrentRequestsInBytesByIp"),
Review comment:
this will have the snapshot but not the count; count will be zero (see `com.codahale.metrics.JmxReporter.JmxHistogram#getCount`)
##########
File path: src/java/org/apache/cassandra/utils/EstimatedHistogram.java
##########
@@ -365,6 +370,57 @@ public int hashCode()
return Objects.hashCode(getBucketOffsets(), getBuckets(false));
}
+ public int size()
+ {
+ return toIntExact(count());
+ }
+
+ public void update(long l)
+ {
+ this.add(l);
Review comment:
nit: don't need `this.`
##########
File path: doc/source/operating/metrics.rst
##########
@@ -645,13 +645,16 @@ Reported name format:
**JMX MBean**
``org.apache.cassandra.metrics:type=Client name=<MetricName>``
-============================== =============================== ===========
-Name Type Description
-============================== =============================== ===========
-connectedNativeClients Gauge<Integer> Number of clients connected to this nodes native protocol server
-connections Gauge<List<Map<String, String>> List of all connections and their state information
-connectedNativeClientsByUser Gauge<Map<String, Int> Number of connnective native clients by username
-============================== =============================== ===========
+============================== ================================ ===========
+Name Type Description
+============================== ================================ ===========
+connectedNativeClients Gauge<Integer> Number of clients connected to this nodes native protocol server
+connections Gauge<List<Map<String, String>> List of all connections and their state information
+connectedNativeClientsByUser Gauge<Map<String, Int> Number of connnective native clients by username
+clientsByProtocolVersion Gauge<List<Map<String, String>>> List of up to last 100 connections including protocol version. Can be reset with clearConnectionHistory operation in org.apache.cassandra.db:StorageService mbean.
Review comment:
for me: this is already there, just documenting
##########
File path: doc/source/operating/metrics.rst
##########
@@ -645,13 +645,16 @@ Reported name format:
**JMX MBean**
``org.apache.cassandra.metrics:type=Client name=<MetricName>``
-============================== =============================== ===========
-Name Type Description
-============================== =============================== ===========
-connectedNativeClients Gauge<Integer> Number of clients connected to this nodes native protocol server
-connections Gauge<List<Map<String, String>> List of all connections and their state information
-connectedNativeClientsByUser Gauge<Map<String, Int> Number of connnective native clients by username
-============================== =============================== ===========
+============================== ================================ ===========
+Name Type Description
+============================== ================================ ===========
+connectedNativeClients Gauge<Integer> Number of clients connected to this nodes native protocol server
+connections Gauge<List<Map<String, String>> List of all connections and their state information
+connectedNativeClientsByUser Gauge<Map<String, Int> Number of connnective native clients by username
+clientsByProtocolVersion Gauge<List<Map<String, String>>> List of up to last 100 connections including protocol version. Can be reset with clearConnectionHistory operation in org.apache.cassandra.db:StorageService mbean.
+concurrentRequestsInBytes Gauge<Long> How many concurrent bytes used in current processing requests
+concurrentRequestsInBytesByIp Histogram How many concurrent bytes used in current processing requests by individual ips
Review comment:
this metric is confusing to me as its not broken down by ip, instead it takes the ip's `RequestsInBytes` and updates a histogram, so more shows the breakdown of bytes concurrently, so better name would be something like `concurrentRequestsInBytesHisto`.
When I read `concurrentRequestsInBytesByIp` I expect `Map<IP, Long>` and showing the amount of bytes broken down by IP
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org