Use EstimatedHistogram in metricPercentilesAsArray(JmxHistogramMBean) #331
Conversation
| @@ -1594,15 +1594,13 @@ public long getStorageMetric(String metricName) | |||
|
|
|||
| public double[] metricPercentilesAsArray(CassandraMetricsRegistry.JmxHistogramMBean metric) | |||
| { | |||
| BufferSamples bs = new BufferSamples(metric.values()); | |||
There was a problem hiding this comment.
I expect this was to get some caching done, by removing this we will loose this caching
for direct fetching (ala histograms) it won't be a prob
but I can imagine this can be a prob e.g. for e.g. toppartitions which does sampling and polls periodically, can you test your change with toppartitions? ( or if it doesn't use this call, then check all possible calls and make sure removing caching won't break them)
There was a problem hiding this comment.
I see @amnonh introduced this wrapper , Amnon is it safe to get rid of it?
There was a problem hiding this comment.
ref ee5e2b9
this explains why, so you are reverting Amnons optimalization ;-)
There was a problem hiding this comment.
Oh, I see. How about moving the logic to jmx side? I mean adding metricPercentilesAsArray to jmx JmxHistogramMBean interface and then
public double[] metricPercentilesAsArray(CassandraMetricsRegistry.JmxHistogramMBean metric) {
return metric.metricPercentilesAsArray();
}
We will have one call to jmx and be able to differentiate between the versions of histogram.
|
Inside Scylla, we have the old-style histograms we inherit from Cassandra and the new kind (actual histograms) these days for most of them we even have the derivative histogram, which is what we are really after in this case. Move to use the real histograms and deprecate the old kind. |
|
@Deexie and I reviewed the code again today and it looks the bug is the intrrpretation of the JMXHistogramMBean values on the Nodetool NodeProbe side as buffer samples. Let's see if we can reconstruct the EstimatedHistogram from values() using the HistogramBuilder and use it to retrieve the percentiles. This way we preserve the fix of getting the values in a single call rather than a series of calls to get each percentile, and we do the conversion correctly. |
NodeProbe::metricPercentilesAsArray(JmxHistogramMBean) is expected to return an array with percentiles obtained from a given histogram. Currently BufferSamples object is created out of the original jmx histogram's values and its getValue method is used to create an array. BufferSamples::getValue(double) expects data in different format than the provided one, though. EstimatedHistogram, which is compactible with original histogram, is created out of metric values. Fixes: scylladb#330.
Deexie
force-pushed
the
metric-percentiles
branch
from
9eb86fc
to
0905f11
Compare
Deexie
changed the title
| eh.percentile(0.98), | ||
| eh.percentile(0.99), | ||
| eh.min(), | ||
| eh.max()}; |
There was a problem hiding this comment.
Yes, it does :D The results look reasonable and the test passes
NodeProbe::metricPercentilesAsArray(JmxHistogramMBean) is expected to return an array with percentiles obtained from a given histogram. Currently BufferSamples object is created out of the original jmx histogram's values and its getValue method is used to create an array. BufferSamples::getValue(double) expects data in different format than the provided one, though. EstimatedHistogram, which is compactible with original histogram, is created out of metric values. Fixes: #330. Closes: #331. (cherry picked from commit eb3c43f)
NodeProbe::metricPercentilesAsArray(JmxHistogramMBean) is expected to return an array with percentiles obtained from a given histogram. Currently BufferSamples object is created out of the original jmx histogram's values and its getValue method is used to create an array. BufferSamples::getValue(double) expects data in different format than the provided one, though. EstimatedHistogram, which is compactible with original histogram, is created out of metric values. Fixes: #330. Closes: #331. (cherry picked from commit eb3c43f)
NodeProbe::metricPercentilesAsArray(JmxHistogramMBean) is expected
to return an array with percentiles obtained from a given histogram.
Currently BufferSamples object is created out of the original jmx
histogram's values and its getValue method is used to create an array.
BufferSamples::getValue(double) expects data in different format than
the provided one, though.
EstimatedHistogram, which is compactible with original histogram,
is created out of metric values.
Fixes: #330.