Skip to content

Commit

Permalink
Don't report empty histograms to FastForward (#140)
Browse files Browse the repository at this point in the history
*Rationale*

If histograms don't have any reported values, or have not been used
in a long time, the snapshot is empty and the histogram metrics
will not have a reasonable default value. All stats (min, max, median, etc) would
incorrectly be reported as zero

*Performance impact*

The performance impact should be minimal, since it only adds one additional
if-check when reporting histograms. Snapshot.size() is a fast method, simply
returning the length of its internal array.

For cases where snapshots are empty - i.e. unused histograms we should see
a performance improvement since we can skip emitting the data points to FastForward
as well as skipping the creation of the metric ids.
  • Loading branch information
spkrka authored Jun 5, 2023
1 parent 8fe6a55 commit 15ead80
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -286,14 +286,19 @@ private void reportCounter(MetricId key, Counting value) {
}

private void reportHistogram(MetricId key, Histogram value) {
final Snapshot snapshot = value.getSnapshot();
if (snapshot.size() == 0) {
return;
}

key = MetricId.join(prefix, key);

final Metric m = FastForward
.metric(key.getKey())
.attributes(key.getTags())
.attribute(METRIC_TYPE, "histogram");

reportHistogram(m, value.getSnapshot());
reportHistogram(m, snapshot);
}

private void reportMetered(MetricId key, Meter value) {
Expand All @@ -310,6 +315,11 @@ private void reportMetered(MetricId key, Meter value) {
}

private void reportTimer(MetricId key, Timer value) {
final Snapshot snapshot = value.getSnapshot();
if (snapshot.size() == 0) {
return;
}

key = MetricId.join(prefix, key);

final Metric m = FastForward
Expand All @@ -319,7 +329,7 @@ private void reportTimer(MetricId key, Timer value) {
.attribute("unit", "ns");

reportMetered(m, value);
reportHistogram(m, value.getSnapshot());
reportHistogram(m, snapshot);
}

private void reportDerivingMeter(MetricId key, DerivingMeter value) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -245,4 +245,26 @@ public void testKeyValuePrefixAddedOnce() throws Exception {

assertEquals(expectedKeys, actualKeys);
}

@Test
public void shouldNotReportEmptyHistogramsAndTimers() throws Exception {
ArgumentCaptor<Metric> argumentCaptor = ArgumentCaptor.forClass(Metric.class);

doNothing().when(fastForward).send(argumentCaptor.capture());

MetricId name = MetricId.build("thename");

registry.histogram(name.tagged("histogram", "true"));
registry.timer(name.tagged("timer", "true"));

reporter.start();

executorService.tick(REPORTING_PERIOD + REPORTING_PERIOD / 3, TimeUnit.MILLISECONDS);
verify(fastForward, atLeastOnce()).send(any(Metric.class));

Set<String> actualKeys = argumentCaptor.getAllValues().stream().map(Metric::getKey)
.collect(Collectors.toSet());

assertEquals(new HashSet<>(Arrays.asList("test.hi")), actualKeys);
}
}

0 comments on commit 15ead80

Please sign in to comment.