Skip to content

Commit

Permalink
Don't report empty histograms to FastForward
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 committed Jun 5, 2023
1 parent 8fe6a55 commit a90084d
Show file tree
Hide file tree
Showing 2 changed files with 36 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 @@ -21,6 +21,8 @@
import com.spotify.metrics.core.MetricId;
import com.spotify.metrics.core.SemanticMetricRegistry;
import com.spotify.metrics.tags.EnvironmentTagExtractor;

import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
Expand Down Expand Up @@ -245,4 +247,26 @@ public void testKeyValuePrefixAddedOnce() throws Exception {

assertEquals(expectedKeys, actualKeys);
}

@Test
public void shouldNotReportEmptyHistogramsAndTimers() throws IOException {
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 a90084d

Please sign in to comment.