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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
