Skip to content

Commit

Permalink
Fix Atlas percentiles tags
Browse files Browse the repository at this point in the history
* Polish on Spring configuration
* Move scripts to start various monitoring systems to /scripts
  • Loading branch information
jkschneider committed Sep 5, 2017
1 parent f6bed73 commit 918ad8b
Show file tree
Hide file tree
Showing 24 changed files with 145 additions and 49 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public class SpectatorDistributionSummary extends AbstractMeter implements Distr
private final Quantiles quantiles;
private final Histogram<?> histogram;

SpectatorDistributionSummary(Id id, String description,
public SpectatorDistributionSummary(Id id, String description,
com.netflix.spectator.api.DistributionSummary distributionSummary,
Quantiles quantiles,
Histogram<?> histogram) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public SpectatorMeterRegistry(Registry registry, Clock clock) {
this.registry = registry;
}

private Collection<com.netflix.spectator.api.Tag> toSpectatorTags(Iterable<io.micrometer.core.instrument.Tag> tags) {
protected Collection<com.netflix.spectator.api.Tag> toSpectatorTags(Iterable<io.micrometer.core.instrument.Tag> tags) {
return stream(tags.spliterator(), false)
.map(t -> new BasicTag(t.getKey(), t.getValue()))
.collect(toList());
Expand Down Expand Up @@ -88,7 +88,7 @@ protected <T> io.micrometer.core.instrument.Gauge newGauge(Meter.Id id, String d
return new SpectatorGauge(id, description, gauge);
}

private Histogram<?> registerHistogramCounterIfNecessary(Meter.Id id, Histogram.Builder<?> histogramBuilder) {
protected Histogram<?> registerHistogramCounterIfNecessary(Meter.Id id, Histogram.Builder<?> histogramBuilder) {
if (histogramBuilder != null) {
return histogramBuilder
.bucketListener(bucket -> {
Expand All @@ -100,7 +100,7 @@ private Histogram<?> registerHistogramCounterIfNecessary(Meter.Id id, Histogram.
return null;
}

private void registerQuantilesGaugeIfNecessary(Meter.Id id, Quantiles quantiles, UnaryOperator<Double> scaling) {
protected void registerQuantilesGaugeIfNecessary(Meter.Id id, Quantiles quantiles, UnaryOperator<Double> scaling) {
if (quantiles != null) {
for (Double q : quantiles.monitored()) {
if (!Double.isNaN(q)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public class SpectatorTimer extends AbstractTimer {
private final Quantiles quantiles;
private final Histogram<?> histogram;

SpectatorTimer(Id id, String description, Timer timer,
public SpectatorTimer(Id id, String description, Timer timer,
Clock clock, Quantiles quantiles, Histogram<?> histogram) {
super(id, description, clock);
this.timer = timer;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,26 @@
public class Bucket<T> {
private final T tag;

/**
* {@code true} if this bucket is used in an aggregable percentile approximation.
*/
private final boolean percentile;

/**
* Cached string representation of {@code tag}.
*/
private String tagStr;

LongAdder value = new LongAdder();

public Bucket(T tag) {
public Bucket(T tag, boolean percentile, long initialValue) {
this.tag = tag;
this.percentile = percentile;
this.value.add(initialValue);
}

public Bucket(T tag, long initialValue) {
this.tag = tag;
this.value.add(initialValue);
public boolean isPercentile() {
return percentile;
}

public String getTag() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,25 +23,32 @@ class DefaultHistogramBuilder<T> implements Histogram.Builder<T> {
private final BucketFunction<T> f;
private Histogram.Type type = null;
private List<BucketListener<T>> bucketListeners = new ArrayList<>();
private boolean percentiles = false;

DefaultHistogramBuilder(BucketFunction<T> f) {
this.f = f;
}

@Override
public Histogram<T> create(TimeUnit baseTimeUnit, Histogram.Type defaultType) {
return new Histogram<>(f, type == null ? defaultType : type, bucketListeners);
return new Histogram<>(f, type == null ? defaultType : type, bucketListeners, percentiles);
}

@Override
public Histogram.Builder bucketListener(BucketListener<T> listener) {
public Histogram.Builder<T> bucketListener(BucketListener<T> listener) {
bucketListeners.add(listener);
return this;
}

@Override
public Histogram.Builder type(Histogram.Type type) {
public Histogram.Builder<T> type(Histogram.Type type) {
this.type = type;
return this;
}

@Override
public Histogram.Builder<T> usedForPercentiles() {
this.percentiles = true;
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,13 @@ public class Histogram<T> {
private final BucketFunction<? extends T> f;
private final Type type;
private final List<BucketListener<T>> bucketListeners;
private final boolean percentiles;

public Histogram(BucketFunction<? extends T> f, Type type, List<BucketListener<T>> listeners) {
public Histogram(BucketFunction<? extends T> f, Type type, List<BucketListener<T>> listeners, boolean percentiles) {
this.f = f;
this.type = type;
this.bucketListeners = listeners;
this.percentiles = percentiles;
}

public Collection<Bucket<T>> getBuckets() {
Expand All @@ -41,6 +43,10 @@ public boolean isCumulative() {
return type.equals(Type.Cumulative);
}

public boolean isPercentiles() {
return percentiles;
}

public Type getType() {
return type;
}
Expand All @@ -65,6 +71,12 @@ public interface Builder<T> {
* Set the histogram type explicitly, overriding the monitoring system's default histogram type.
*/
Builder type(Type type);

/**
* Hacky, but Atlas :percentiles math requires a different tag key than what we would place on
* an ordinary histogram
*/
Builder usedForPercentiles();
}

public static <U> Builder<U> function(BucketFunction<U> f) {
Expand Down Expand Up @@ -114,11 +126,11 @@ public static Builder<Double> exponentialTime(TimeUnit unit, double start, doubl
}

public static Builder<Double> percentiles() {
return new DefaultHistogramBuilder<>(percentilesFunction());
return new DefaultHistogramBuilder<>(percentilesFunction()).usedForPercentiles();
}

public static Builder<Double> percentilesTime() {
return new TimeScalingHistogramBuilder(percentilesFunction(), TimeUnit.NANOSECONDS);
return new TimeScalingHistogramBuilder(percentilesFunction(), TimeUnit.NANOSECONDS).usedForPercentiles();
}

private static BucketFunction<Double> percentilesFunction() {
Expand Down Expand Up @@ -160,8 +172,8 @@ public void observe(double value) {

if (isCumulative()) {
Map.Entry<T, Bucket<T>> ceiling = buckets.ceilingEntry(tag);
bucket = new Bucket<>(tag, ceiling == null ? 1 : ceiling.getValue().getValue() + 1);
} else bucket = new Bucket<>(tag, 1);
bucket = new Bucket<>(tag, percentiles, ceiling == null ? 1 : ceiling.getValue().getValue() + 1);
} else bucket = new Bucket<>(tag, percentiles, 1);

bucketListeners.forEach(listener -> listener.bucketAdded(bucket));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ class TimeScalingHistogramBuilder implements Histogram.Builder<Double> {
private final TimeUnit fUnits;
private Histogram.Type type = null;
private List<BucketListener<Double>> bucketListeners = new ArrayList<>();
private boolean percentiles = false;

TimeScalingHistogramBuilder(BucketFunction<Double> f, TimeUnit fUnits) {
this.f = f;
Expand All @@ -34,17 +35,17 @@ class TimeScalingHistogramBuilder implements Histogram.Builder<Double> {

@Override
public Histogram<Double> create(TimeUnit baseTimeUnit, Histogram.Type defaultType) {
return new Histogram<>(timeScale(f, baseTimeUnit), type == null ? defaultType : type, bucketListeners);
return new Histogram<>(timeScale(f, baseTimeUnit), type == null ? defaultType : type, bucketListeners, percentiles);
}

@Override
public Histogram.Builder type(Histogram.Type type) {
public Histogram.Builder<Double> type(Histogram.Type type) {
this.type = type;
return this;
}

@Override
public Histogram.Builder bucketListener(BucketListener<Double> listener) {
public Histogram.Builder<Double> bucketListener(BucketListener<Double> listener) {
bucketListeners.add(listener);
return this;
}
Expand All @@ -55,4 +56,10 @@ private BucketFunction<Double> timeScale(BucketFunction<Double> f, TimeUnit base
return TimeUtils.convert(unscaledBucket, fUnits, baseTimeUnit);
};
}

@Override
public Histogram.Builder<Double> usedForPercentiles() {
this.percentiles = true;
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
import io.micrometer.spring.export.influx.InfluxExportConfiguration;
import io.micrometer.spring.export.jmx.JmxExportConfiguration;
import io.micrometer.spring.export.prometheus.PrometheusExportConfiguration;
import io.micrometer.spring.scheduling.MetricsSchedulingAspect;
import io.micrometer.spring.scheduling.ScheduledMethodMetrics;
import io.micrometer.spring.web.MetricsRestTemplateConfiguration;
import io.micrometer.spring.web.MetricsServletRequestConfiguration;
import org.springframework.beans.factory.ObjectProvider;
Expand Down Expand Up @@ -104,8 +104,8 @@ public MeterRegistryConfigurationSupport(MeterRegistry registry,
@Bean
@ConditionalOnClass(name = "org.aspectj.lang.ProceedingJoinPoint")
@ConditionalOnProperty(value = "spring.aop.enabled", havingValue = "true", matchIfMissing = true)
public MetricsSchedulingAspect metricsSchedulingAspect(MeterRegistry registry) {
return new MetricsSchedulingAspect(registry);
public ScheduledMethodMetrics metricsSchedulingAspect(MeterRegistry registry) {
return new ScheduledMethodMetrics(registry);
}

@Configuration
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@
import java.util.concurrent.TimeUnit;

@Aspect
public class MetricsSchedulingAspect {
private static final Log logger = LogFactory.getLog(MetricsSchedulingAspect.class);
public class ScheduledMethodMetrics {
private static final Log logger = LogFactory.getLog(ScheduledMethodMetrics.class);

private final MeterRegistry registry;

public MetricsSchedulingAspect(MeterRegistry registry) {
public ScheduledMethodMetrics(MeterRegistry registry) {
this.registry = registry;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,13 @@ public class ControllerMetrics {

private final MeterRegistry registry;
private MetricsConfigurationProperties properties;
private final WebmvcTagConfigurer tagConfigurer;
private final WebServletTagConfigurer tagConfigurer;

private final Map<HttpServletRequest, Long> longTaskTimerIds = Collections.synchronizedMap(new IdentityHashMap<>());

public ControllerMetrics(MeterRegistry registry,
MetricsConfigurationProperties properties,
WebmvcTagConfigurer tagConfigurer) {
WebServletTagConfigurer tagConfigurer) {
this.registry = registry;
this.properties = properties;
this.tagConfigurer = tagConfigurer;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,15 @@
@EnableConfigurationProperties(MetricsConfigurationProperties.class)
public class MetricsServletRequestConfiguration extends WebMvcConfigurerAdapter {
@Bean
@ConditionalOnMissingBean(WebmvcTagConfigurer.class)
WebmvcTagConfigurer webmvcTagConfigurer() {
return new WebmvcTagConfigurer();
@ConditionalOnMissingBean(WebServletTagConfigurer.class)
WebServletTagConfigurer webmvcTagConfigurer() {
return new WebServletTagConfigurer();
}

@Bean
ControllerMetrics controllerMetrics(MeterRegistry registry,
MetricsConfigurationProperties properties,
WebmvcTagConfigurer configurer) {
WebServletTagConfigurer configurer) {
return new ControllerMetrics(registry, properties, configurer);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
/**
* Copyright 2017 Pivotal Software, Inc.
*
* <p>
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* <p>
* http://www.apache.org/licenses/LICENSE-2.0
* <p>
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
Expand All @@ -29,7 +29,9 @@
*
* @author Jon Schneider
*/
public class WebmvcTagConfigurer {
public class WebServletTagConfigurer {
public static WebServletTagConfigurer DEFAULT = new WebServletTagConfigurer();

/**
* Supplies default tags to long task timers.
*
Expand All @@ -50,11 +52,21 @@ public Iterable<Tag> httpLongRequestTags(HttpServletRequest request, Object hand
* @return A set of tags added to every Spring MVC HTTP request.
*/
public Iterable<Tag> httpRequestTags(HttpServletRequest request,
HttpServletResponse response,
Throwable ex) {
HttpServletResponse response,
Throwable ex) {
return defaultHttpRequestTags(request, response, ex);
}

private Iterable<Tag> defaultHttpRequestTags(HttpServletRequest request,
HttpServletResponse response,
Throwable ex) {
return asList(method(request), uri(request), exception(ex), status(response));
}

private Iterable<Tag> defaultHttpLongRequestTags(HttpServletRequest request, Object handler) {
return asList(method(request), uri(request));
}

/**
* @param request The HTTP request.
* @return A "method" tag whose value is a capitalized method (e.g. GET).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public class PersonController {
private List<String> people = Arrays.asList("mike", "suzy");

@GetMapping("/api/people")
@Timed(quantiles = 0.95)
@Timed(percentiles = true)
public List<String> allPeople() {
return people;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,7 @@ metrics:
graphite.enabled: false
influx.enabled: false
jmx.enabled: false
prometheus.enabled: false
prometheus.enabled: false

# % of the time, blow up on the Roulette endpoint
roulette.frequency: 0.8
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import io.micrometer.core.instrument.binder.LogbackMetrics;
import io.micrometer.core.instrument.binder.MeterBinder;
import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
import io.micrometer.core.instrument.stats.hist.Histogram;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.beans.factory.annotation.Autowired;
Expand Down Expand Up @@ -119,7 +120,6 @@ public RestTemplate restTemplate() {

@RestController
static class PersonController {
@Timed
@GetMapping("/api/people")
Set<String> personName() {
return Collections.singleton("Jon");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
@RunWith(SpringRunner.class)
@SpringBootTest
@TestPropertySource(properties = "metrics.useGlobalRegistry=false")
public class MetricsSchedulingAspectTest {
public class ScheduledMethodMetricsTest {

static CountDownLatch longTaskStarted = new CountDownLatch(1);
static CountDownLatch longTaskShouldComplete = new CountDownLatch(1);
Expand Down
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
Loading

0 comments on commit 918ad8b

Please sign in to comment.