diff --git a/java/google/registry/monitoring/metrics/Counter.java b/java/google/registry/monitoring/metrics/Counter.java index d19f95353..45b6f50bb 100644 --- a/java/google/registry/monitoring/metrics/Counter.java +++ b/java/google/registry/monitoring/metrics/Counter.java @@ -20,8 +20,12 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.util.concurrent.AtomicLongMap; +import com.google.common.util.concurrent.Striped; import google.registry.monitoring.metrics.MetricSchema.Kind; import java.util.Map.Entry; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.locks.Lock; import javax.annotation.concurrent.ThreadSafe; import org.joda.time.Instant; @@ -29,17 +33,53 @@ import org.joda.time.Instant; * A metric which stores Long values. It is stateful and can be changed in increments. * *

This metric is generally suitable for counters, such as requests served or errors generated. + * + *

The start of the {@link MetricPoint#interval()} of values of instances of this metric will be + * set to the time that the metric was first set or last {@link #reset()}. */ @ThreadSafe public final class Counter extends AbstractMetric implements SettableMetric, IncrementableMetric { + /** + * The below constants replicate the default initial capacity, load factor, and concurrency level + * for {@link ConcurrentHashMap} as of Java SE 7. They are hardcoded here so that the concurrency + * level in {@code valueLocks} below can be set identically. + */ + private static final int HASHMAP_INITIAL_CAPACITY = 16; + private static final float HASHMAP_LOAD_FACTOR = 0.75f; + private static final int HASHMAP_CONCURRENCY_LEVEL = 16; + private static final String LABEL_COUNT_ERROR = "The count of labelValues must be equal to the underlying " + "MetricDescriptor's count of labels."; + /** + * A map of the {@link Counter} values, with a list of label values as the keys. + * + *

This should be modified in a critical section with {@code valueStartTimestamps} so that the + * values are in sync. + */ private final AtomicLongMap> values = AtomicLongMap.create(); + /** + * A map of the {@link Instant} that each value was created, with a list of label values as the + * keys. The start timestamp (as part of the {@link MetricPoint#interval()} can be used by + * implementations of {@link MetricWriter} to encode resets of monotonic counters. + */ + private final ConcurrentHashMap, Instant> valueStartTimestamps = + new ConcurrentHashMap<>( + HASHMAP_INITIAL_CAPACITY, HASHMAP_LOAD_FACTOR, HASHMAP_CONCURRENCY_LEVEL); + + /** + * A fine-grained lock to ensure that {@code values} and {@code valueStartTimestamps} are modified + * and read in a critical section. The initialization parameter is the concurrency level, set to + * match the default concurrency level of {@link ConcurrentHashMap}. + * + * @see Striped + */ + private final Striped valueLocks = Striped.lock(HASHMAP_CONCURRENCY_LEVEL); + Counter( String name, String description, @@ -49,8 +89,16 @@ public final class Counter extends AbstractMetric } @VisibleForTesting - void incrementBy(long offset, ImmutableList labelValues) { - values.addAndGet(labelValues, offset); + void incrementBy(long offset, Instant startTime, ImmutableList labelValues) { + Lock lock = valueLocks.get(labelValues); + lock.lock(); + + try { + values.addAndGet(labelValues, offset); + valueStartTimestamps.putIfAbsent(labelValues, startTime); + } finally { + lock.unlock(); + } } @Override @@ -58,14 +106,14 @@ public final class Counter extends AbstractMetric checkArgument(labelValues.length == this.getMetricSchema().labels().size(), LABEL_COUNT_ERROR); checkArgument(offset >= 0, "The offset provided must be non-negative"); - incrementBy(offset, ImmutableList.copyOf(labelValues)); + incrementBy(offset, Instant.now(), ImmutableList.copyOf(labelValues)); } @Override public final void increment(String... labelValues) { checkArgument(labelValues.length == this.getMetricSchema().labels().size(), LABEL_COUNT_ERROR); - incrementBy(1L, ImmutableList.copyOf(labelValues)); + incrementBy(1L, Instant.now(), ImmutableList.copyOf(labelValues)); } /** @@ -83,23 +131,87 @@ public final class Counter extends AbstractMetric } @VisibleForTesting - final ImmutableList> getTimestampedValues(Instant timestamp) { + final ImmutableList> getTimestampedValues(Instant endTimestamp) { ImmutableList.Builder> timestampedValues = new ImmutableList.Builder<>(); for (Entry, Long> entry : values.asMap().entrySet()) { - timestampedValues.add(MetricPoint.create(this, entry.getKey(), timestamp, entry.getValue())); + ImmutableList labelValues = entry.getKey(); + valueLocks.get(labelValues).lock(); + + Instant startTimestamp; + try { + startTimestamp = valueStartTimestamps.get(labelValues); + } finally { + valueLocks.get(labelValues).unlock(); + } + + timestampedValues.add( + MetricPoint.create(this, labelValues, startTimestamp, endTimestamp, entry.getValue())); + } return timestampedValues.build(); } @VisibleForTesting - final void set(Long value, ImmutableList labelValues) { - this.values.put(labelValues, value); + final void set(Long value, Instant startTime, ImmutableList labelValues) { + Lock lock = valueLocks.get(labelValues); + lock.lock(); + + try { + this.values.put(labelValues, value); + valueStartTimestamps.putIfAbsent(labelValues, startTime); + } finally { + lock.unlock(); + } } @Override public final void set(Long value, String... labelValues) { checkArgument(labelValues.length == this.getMetricSchema().labels().size(), LABEL_COUNT_ERROR); - set(value, ImmutableList.copyOf(labelValues)); + set(value, Instant.now(), ImmutableList.copyOf(labelValues)); + } + + @VisibleForTesting + final void reset(Instant startTime) { + // Lock the entire set of values so that all existing values will have a consistent timestamp + // after this call, without the possibility of interleaving with another reset() call. + Set> keys = values.asMap().keySet(); + for (int i = 0; i < valueLocks.size(); i++) { + valueLocks.getAt(i).lock(); + } + + for (ImmutableList labelValues : keys) { + this.values.put(labelValues, 0); + this.valueStartTimestamps.put(labelValues, startTime); + } + + for (int i = 0; i < valueLocks.size(); i++) { + valueLocks.getAt(i).unlock(); + } + } + + @Override + public final void reset() { + reset(Instant.now()); + } + + @VisibleForTesting + final void reset(Instant startTime, ImmutableList labelValues) { + Lock lock = valueLocks.get(labelValues); + lock.lock(); + + try { + this.values.put(labelValues, 0); + this.valueStartTimestamps.put(labelValues, startTime); + } finally { + lock.unlock(); + } + } + + @Override + public final void reset(String... labelValues) { + checkArgument(labelValues.length == this.getMetricSchema().labels().size(), LABEL_COUNT_ERROR); + + reset(Instant.now(), ImmutableList.copyOf(labelValues)); } } diff --git a/java/google/registry/monitoring/metrics/IncrementableMetric.java b/java/google/registry/monitoring/metrics/IncrementableMetric.java index c4ee92f2f..76f6bbfeb 100644 --- a/java/google/registry/monitoring/metrics/IncrementableMetric.java +++ b/java/google/registry/monitoring/metrics/IncrementableMetric.java @@ -23,9 +23,9 @@ package google.registry.monitoring.metrics; public interface IncrementableMetric extends Metric { /** - * Increments a metric by 1 for the given set of label values. + * Increments a metric by 1 for the given label values. * - * Use this method rather than {@link IncrementableMetric#incrementBy(long, String...)} if the + *

Use this method rather than {@link IncrementableMetric#incrementBy(long, String...)} if the * increment value is zero, as it will be slightly more performant. * *

If the metric is undefined for given label values, it will be incremented from zero. @@ -37,7 +37,7 @@ public interface IncrementableMetric extends Metric { void increment(String... labelValues); /** - * Increments a metric by the given non-negative offset for the given set of label values. + * Increments a metric by the given non-negative offset for the given label values. * *

If the metric is undefined for given label values, it will be incremented from zero. * @@ -48,4 +48,20 @@ public interface IncrementableMetric extends Metric { * @throws IllegalArgumentException if the offset is negative. */ void incrementBy(long offset, String... labelValues); + + /** + * Resets the value and start timestamp of the metric for the given label values. + * + *

This is useful if the counter is tracking a value that is reset as part of a retrying + * transaction, for example. + */ + void reset(String... labelValues); + + /** + * Atomically resets the value and start timestamp of the metric for all label values. + * + *

This is useful if the counter is tracking values that are reset as part of a retrying + * transaction, for example. + */ + void reset(); } diff --git a/java/google/registry/monitoring/metrics/Metric.java b/java/google/registry/monitoring/metrics/Metric.java index 796db663d..c97e47e33 100644 --- a/java/google/registry/monitoring/metrics/Metric.java +++ b/java/google/registry/monitoring/metrics/Metric.java @@ -24,8 +24,8 @@ import com.google.common.collect.ImmutableList; public interface Metric { /** - * Returns the list of the latest {@link MetricPoint} instances for every label-value combination - * tracked for this metric. + * Returns the latest {@link MetricPoint} instances for every label-value combination tracked for + * this metric. */ ImmutableList> getTimestampedValues(); diff --git a/java/google/registry/monitoring/metrics/MetricPoint.java b/java/google/registry/monitoring/metrics/MetricPoint.java index 9eff0f766..1a3c3ce53 100644 --- a/java/google/registry/monitoring/metrics/MetricPoint.java +++ b/java/google/registry/monitoring/metrics/MetricPoint.java @@ -19,10 +19,11 @@ import static com.google.common.base.Preconditions.checkArgument; import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; import org.joda.time.Instant; +import org.joda.time.Interval; /** - * Value type class to store a point-in-time snapshot of a {@link Metric} value for a given label - * value tuple. + * Value type class to store a snapshot of a {@link Metric} value for a given label value tuple and + * time {@link Interval}. */ @AutoValue public abstract class MetricPoint { @@ -34,21 +35,43 @@ public abstract class MetricPoint { MetricPoint() {} /** - * Returns a new {@link MetricPoint}. Callers should insure that the count of {@code labelValues} - * matches the count of labels for the given metric. + * Returns a new {@link MetricPoint} representing a value at a specific {@link Instant}. + * + *

Callers should insure that the count of {@code labelValues} matches the count of labels for + * the given metric. */ static MetricPoint create( Metric metric, ImmutableList labelValues, Instant timestamp, V value) { checkArgument( labelValues.size() == metric.getMetricSchema().labels().size(), LABEL_COUNT_ERROR); - return new AutoValue_MetricPoint<>(metric, labelValues, timestamp, value); + return new AutoValue_MetricPoint<>( + metric, labelValues, new Interval(timestamp, timestamp), value); + } + + /** + * Returns a new {@link MetricPoint} representing a value over an {@link Interval} from {@code + * startTime} to {@code endTime}. + * + *

Callers should insure that the count of {@code labelValues} matches the count of labels for + * the given metric. + */ + static MetricPoint create( + Metric metric, + ImmutableList labelValues, + Instant startTime, + Instant endTime, + V value) { + checkArgument( + labelValues.size() == metric.getMetricSchema().labels().size(), LABEL_COUNT_ERROR); + return new AutoValue_MetricPoint<>( + metric, labelValues, new Interval(startTime, endTime), value); } public abstract Metric metric(); public abstract ImmutableList labelValues(); - public abstract Instant timestamp(); + public abstract Interval interval(); public abstract V value(); } diff --git a/java/google/registry/monitoring/metrics/StackdriverWriter.java b/java/google/registry/monitoring/metrics/StackdriverWriter.java index 4a2b4aa2a..a212b295e 100644 --- a/java/google/registry/monitoring/metrics/StackdriverWriter.java +++ b/java/google/registry/monitoring/metrics/StackdriverWriter.java @@ -46,6 +46,7 @@ import java.util.logging.Logger; import javax.annotation.concurrent.NotThreadSafe; import javax.inject.Inject; import javax.inject.Named; +import org.joda.time.Interval; import org.joda.time.format.DateTimeFormatter; import org.joda.time.format.ISODateTimeFormat; @@ -251,6 +252,12 @@ public class StackdriverWriter implements MetricWriter { return descriptor; } + private static TimeInterval encodeTimeInterval(Interval nativeInterval) { + return new TimeInterval() + .setEndTime(DATETIME_FORMATTER.print(nativeInterval.getEnd())) + .setStartTime(DATETIME_FORMATTER.print(nativeInterval.getStart())); + } + /** * Encodes a {@link MetricPoint} into a Stackdriver {@link TimeSeries}. * @@ -298,9 +305,7 @@ public class StackdriverWriter implements MetricWriter { } Point encodedPoint = - new Point() - .setInterval(new TimeInterval().setEndTime(DATETIME_FORMATTER.print(point.timestamp()))) - .setValue(encodedValue); + new Point().setInterval(encodeTimeInterval(point.interval())).setValue(encodedValue); List encodedLabels = descriptor.getLabels(); // The MetricDescriptors returned by the GCM API have null fields rather than empty lists diff --git a/java/google/registry/monitoring/metrics/StoredMetric.java b/java/google/registry/monitoring/metrics/StoredMetric.java index a307aedf9..0c21a9c29 100644 --- a/java/google/registry/monitoring/metrics/StoredMetric.java +++ b/java/google/registry/monitoring/metrics/StoredMetric.java @@ -32,7 +32,11 @@ import org.joda.time.Instant; *

The values are stored and set over time. This metric is generally suitable for state * indicators, such as indicating that a server is in a RUNNING state or in a STOPPED state. * - *

See {@link Counter} for a subclass which is suitable for incremental values. + *

See {@link Counter} for a subclass which is suitable for stateful incremental values. + * + *

The {@link MetricPoint#interval()} of values of instances of this metric will always have a + * start time equal to the end time, since the metric value represents a point-in-time snapshot with + * no relationship to prior values. */ @ThreadSafe public class StoredMetric extends AbstractMetric implements SettableMetric { @@ -82,7 +86,8 @@ public class StoredMetric extends AbstractMetric implements SettableMetric final ImmutableList> getTimestampedValues(Instant timestamp) { ImmutableList.Builder> timestampedValues = new Builder<>(); for (Entry, V> entry : values.entrySet()) { - timestampedValues.add(MetricPoint.create(this, entry.getKey(), timestamp, entry.getValue())); + timestampedValues.add( + MetricPoint.create(this, entry.getKey(), timestamp, timestamp, entry.getValue())); } return timestampedValues.build(); diff --git a/java/google/registry/monitoring/metrics/VirtualMetric.java b/java/google/registry/monitoring/metrics/VirtualMetric.java index 56270c758..686548753 100644 --- a/java/google/registry/monitoring/metrics/VirtualMetric.java +++ b/java/google/registry/monitoring/metrics/VirtualMetric.java @@ -29,6 +29,10 @@ import org.joda.time.Instant; * *

This pattern works well for gauge-like metrics, such as CPU usage, memory usage, and file * descriptor counts. + * + *

The {@link MetricPoint#interval()} of values of instances of this metric will always have a + * start time equal to the end time, since the metric value represents a point-in-time snapshot with + * no relationship to prior values. */ @ThreadSafe public final class VirtualMetric extends AbstractMetric { @@ -77,7 +81,8 @@ public final class VirtualMetric extends AbstractMetric { ImmutableList.Builder> metricPoints = ImmutableList.builder(); for (Entry, V> entry : values.entrySet()) { - metricPoints.add(MetricPoint.create(this, entry.getKey(), timestamp, entry.getValue())); + metricPoints.add( + MetricPoint.create(this, entry.getKey(), timestamp, timestamp, entry.getValue())); } cardinality = values.size();