Skip to content

Commit 333b521

Browse files
authored
Changed entryCount from int to long in HistogramStatistic to prevent overflow (#131)
* Changed int to long in Histogram to prevent overflow * Tests to check int overflow in Histogram Statistics
1 parent 9bc654e commit 333b521

File tree

2 files changed

+40
-3
lines changed

2 files changed

+40
-3
lines changed

src/main/java/com/arpnetworking/tsdcore/statistics/HistogramStatistic.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -342,7 +342,7 @@ public Histogram(final int precision) {
342342
_packMask = (1 << (_precision + EXPONENT_BITS + 1)) - 1;
343343
}
344344

345-
private int _entriesCount = 0;
345+
private long _entriesCount = 0;
346346
private final Double2LongSortedMap _data = new Double2LongAVLTreeMap();
347347
private final int _precision;
348348
private final long _truncateMask;

src/test/java/com/arpnetworking/tsdcore/statistics/HistogramStatisticTest.java

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,9 @@ public void histogramAccumulateQuantities() {
4040
final CalculatedValue<HistogramStatistic.HistogramSupportingData> value = accumulator.calculate(Collections.emptyMap());
4141
final HistogramStatistic.HistogramSupportingData supportingData = value.getData();
4242
final HistogramStatistic.HistogramSnapshot histogram = supportingData.getHistogramSnapshot();
43+
Assert.assertEquals(100L, histogram.getEntriesCount());
4344
for (final Double2LongMap.Entry entry : histogram.getValues()) {
44-
Assert.assertEquals(entry.getLongValue(), 1L);
45+
Assert.assertEquals(1L, entry.getLongValue());
4546
}
4647
}
4748

@@ -59,8 +60,9 @@ public void histogramAccumulateHistogram() {
5960
final CalculatedValue<HistogramStatistic.HistogramSupportingData> value = merged.calculate(Collections.emptyMap());
6061
final HistogramStatistic.HistogramSupportingData supportingData = value.getData();
6162
final HistogramStatistic.HistogramSnapshot histogram = supportingData.getHistogramSnapshot();
63+
Assert.assertEquals(100L, histogram.getEntriesCount());
6264
for (final Double2LongMap.Entry entry : histogram.getValues()) {
63-
Assert.assertEquals(entry.getLongValue(), 1L);
65+
Assert.assertEquals(1L, entry.getLongValue());
6466
}
6567
}
6668

@@ -90,6 +92,7 @@ public void histogramAccumulateMultipleHistogram() {
9092
final CalculatedValue<HistogramStatistic.HistogramSupportingData> value = merged.calculate(Collections.emptyMap());
9193
final HistogramStatistic.HistogramSupportingData supportingData = value.getData();
9294
final HistogramStatistic.HistogramSnapshot histogram = supportingData.getHistogramSnapshot();
95+
Assert.assertEquals(251L, histogram.getEntriesCount());
9396
for (final Double2LongMap.Entry entry : histogram.getValues()) {
9497
final int val = (int) entry.getDoubleKey();
9598
if (val < 50) {
@@ -104,6 +107,38 @@ public void histogramAccumulateMultipleHistogram() {
104107
Assert.assertEquals(2000d, histogram.getValueAtPercentile(99.9d), 1d);
105108
}
106109

110+
/**
111+
* Check that neither the entries count or bucket values overflow an integer while accumulating.
112+
*/
113+
@Test
114+
public void histogramAccumulateHistogramsCheckNoOverflow() {
115+
final Accumulator<HistogramStatistic.HistogramSupportingData> merged = HISTOGRAM_STATISTIC.createCalculator();
116+
117+
for (int i = 0; i < 3; ++i) {
118+
final HistogramStatistic.Histogram histogram = new HistogramStatistic.Histogram();
119+
histogram.recordValue(0, 1000000000);
120+
final HistogramStatistic.HistogramSupportingData supportingData = new HistogramStatistic.HistogramSupportingData.Builder()
121+
.setHistogramSnapshot(histogram.getSnapshot())
122+
.build();
123+
final CalculatedValue<HistogramStatistic.HistogramSupportingData> calculatedValue =
124+
new CalculatedValue.Builder<HistogramStatistic.HistogramSupportingData>()
125+
.setValue(new Quantity.Builder().setValue((double) 1000000000).build())
126+
.setData(supportingData)
127+
.build();
128+
merged.accumulate(calculatedValue);
129+
}
130+
131+
final CalculatedValue<HistogramStatistic.HistogramSupportingData> value = merged.calculate(Collections.emptyMap());
132+
final HistogramStatistic.HistogramSupportingData supportingData = value.getData();
133+
final HistogramStatistic.HistogramSnapshot histogram = supportingData.getHistogramSnapshot();
134+
Assert.assertEquals(3000000000L, histogram.getEntriesCount());
135+
for (final Double2LongMap.Entry entry : histogram.getValues()) {
136+
if (entry.getDoubleKey() == 0) {
137+
Assert.assertEquals(3000000000L, entry.getLongValue());
138+
}
139+
}
140+
}
141+
107142
@Test
108143
public void histogramQuantityConversion() {
109144
final Accumulator<HistogramStatistic.HistogramSupportingData> accumulator = HISTOGRAM_STATISTIC.createCalculator();
@@ -185,6 +220,7 @@ public void histogramUnitConversion() {
185220
}
186221

187222
Assert.assertEquals(99.5d, histogram.getValueAtPercentile(99.9d), 1d);
223+
Assert.assertEquals(100, histogram.getEntriesCount());
188224
}
189225

190226
@Test
@@ -199,6 +235,7 @@ public void histogramEnds() {
199235

200236
Assert.assertEquals(10d, histogram.getValueAtPercentile(0), 1d);
201237
Assert.assertEquals(50d, histogram.getValueAtPercentile(100), 1d);
238+
Assert.assertEquals(2, histogram.getEntriesCount());
202239
}
203240

204241
private static final StatisticFactory STATISTIC_FACTORY = new StatisticFactory();

0 commit comments

Comments
 (0)