Skip to content

Commit

Permalink
Fix MetricName comparator for tags (#272)
Browse files Browse the repository at this point in the history 
Ensure `MetricName#safeTags` are compared in a total ordered fashion.
  • Loading branch information
@schlosna @bulldozer-bot
schlosna authored and bulldozer-bot[bot] committed May 26, 2019
1 parent 0fb0b91 commit c2b1222
Show file tree
Hide file tree
Showing 6 changed files with 69 additions and 55 deletions.
4 changes: 1 addition & 3 deletions tritium-caffeine/src/main/java/com/palantir/tritium/metrics/caffeine/CaffeineCacheStats.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,7 @@ public static void registerCache(TaggedMetricRegistry registry, Cache<?, ?> cach
});
}

static <K extends Comparable<K>> Map<K, Gauge<?>> createCacheGauges(
Cache<?, ?> cache,
Function<String, K> metricNamer) {
static <K> Map<K, Gauge<?>> createCacheGauges(Cache<?, ?> cache, Function<String, K> metricNamer) {
return InternalCacheMetrics.createMetrics(CaffeineStats.create(cache, 1, TimeUnit.SECONDS), metricNamer);
}
}
8 changes: 3 additions & 5 deletions tritium-metrics/src/main/java/com/palantir/tritium/metrics/InternalCacheMetrics.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import com.codahale.metrics.RatioGauge;
import com.google.common.annotations.Beta;
import com.google.common.cache.Cache;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.ImmutableMap;
import com.palantir.tritium.metrics.registry.MetricName;
import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;
import java.util.Map;
Expand All @@ -40,10 +40,8 @@
public final class InternalCacheMetrics {
private InternalCacheMetrics() {}

public static <K extends Comparable<K>> Map<K, Gauge<?>> createMetrics(
Stats stats,
Function<String, K> metricNamer) {
ImmutableSortedMap.Builder<K, Gauge<?>> builder = ImmutableSortedMap.naturalOrder();
public static <K> Map<K, Gauge<?>> createMetrics(Stats stats, Function<String, K> metricNamer) {
ImmutableMap.Builder<K, Gauge<?>> builder = ImmutableMap.builder();
stats.forEach((name, gauge) -> builder.put(metricNamer.apply(name), gauge));
return builder.build();
}
Expand Down
4 changes: 2 additions & 2 deletions ...try/src/main/java/com/palantir/tritium/metrics/registry/AbstractTaggedMetricRegistry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import com.codahale.metrics.Metric;
import com.codahale.metrics.Reservoir;
import com.codahale.metrics.Timer;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
import com.palantir.logsafe.SafeArg;
import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
Expand Down Expand Up @@ -143,7 +143,7 @@ public final Timer timer(MetricName metricName, Supplier<Timer> timerSupplier) {

@Override
public final Map<MetricName, Metric> getMetrics() {
ImmutableSortedMap.Builder<MetricName, Metric> result = ImmutableSortedMap.naturalOrder();
ImmutableMap.Builder<MetricName, Metric> result = ImmutableMap.builder();
result.putAll(registry);
taggedRegistries.forEach((tag, metrics) -> metrics.getMetrics()
.forEach((metricName, metric) -> result.put(
Expand Down
34 changes: 4 additions & 30 deletions tritium-registry/src/main/java/com/palantir/tritium/metrics/registry/MetricName.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,37 +17,15 @@
package com.palantir.tritium.metrics.registry;

import com.palantir.logsafe.Safe;
import java.util.Comparator;
import java.util.Iterator;
import java.util.Map;
import java.util.SortedMap;
import org.immutables.value.Value;

@Value.Immutable(prehash = true)
@Value.Style(
jdkOnly = true,
get = {"get*", "is*"},
overshadowImplementation = true,
visibility = Value.Style.ImplementationVisibility.PACKAGE)
public interface MetricName extends Comparable<MetricName> {

Comparator<Map.Entry<String, String>> entryComparator =
Map.Entry.<String, String>comparingByKey()
.thenComparing(Map.Entry.comparingByValue());
Comparator<MetricName> metricNameComparator =
Comparator.comparing(MetricName::safeName)
.thenComparing(metricName -> metricName.safeTags().entrySet(), (set1, set2) -> {
Iterator<Map.Entry<String, String>> i1 = set1.iterator();
Iterator<Map.Entry<String, String>> i2 = set2.iterator();
while (i1.hasNext() && i2.hasNext()) {
Map.Entry<String, String> e1 = i1.next();
Map.Entry<String, String> e2 = i2.next();
int compare = entryComparator.compare(e1, e2);
if (compare != 0) {
return compare;
}
}
return i1.hasNext() ? -1 : i2.hasNext() ? 1 : 0;
});
public interface MetricName {

/**
* General/abstract measure (e.g. server.response-time).
Expand All @@ -61,12 +39,8 @@ public interface MetricName extends Comparable<MetricName> {
* <p>
* All tags and keys must be {@link Safe} to log.
*/
Map<String, String> safeTags();

@Override
default int compareTo(MetricName that) {
return metricNameComparator.compare(this, that);
}
@Value.NaturalOrder
SortedMap<String, String> safeTags();

This comment has been minimized.

Sign in to view

Copy link
@JacekLach

JacekLach May 28, 2019 •
edited
Loading

this is a break:

java.lang.NoSuchMethodError: com.palantir.tritium.metrics.registry.MetricName.safeTags()Ljava/util/Map;
	at com.palantir.sls.logging.metric.ScheduledTaggedMetricLogReporter.metricLog(ScheduledTaggedMetricLogReporter.java:121)
	at com.palantir.sls.logging.metric.ScheduledTaggedMetricLogReporter.reportMetrics(ScheduledTaggedMetricLogReporter.java:102)
	at com.palantir.sls.logging.metric.ScheduledTaggedMetricLogReporter.lambda$report$2(ScheduledTaggedMetricLogReporter.java:91)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
	at com.palantir.tracing.DeferredTracer.withTrace(DeferredTracer.java:115)
	at com.palantir.tracing.Tracers$TracingAwareCallable.call(Tracers.java:343)
	at com.palantir.tracing.WrappingExecutorService.lambda$wrapTask$0(WrappingExecutorService.java:67)
	at com.codahale.metrics.InstrumentedExecutorService$InstrumentedRunnable.run(InstrumentedExecutorService.java:176)
	at com.palantir.tritium.metrics.TaggedMetricsExecutorService$TaggedMetricsRunnable.run(TaggedMetricsExecutorService.java:161)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.lang.Thread.run(Thread.java:834)

static Builder builder() {
return new Builder();
Expand Down
47 changes: 32 additions & 15 deletions tritium-registry/src/test/java/com/palantir/tritium/metrics/registry/MetricNameTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,45 +27,62 @@ public void compareSame() {
MetricName one = MetricName.builder()
.safeName("test")
.putSafeTags("key", "value")
.putSafeTags("key1", "value1")
.putSafeTags("key2", "value2")
.build();
MetricName two = MetricName.builder()
.safeName("test")
.putSafeTags("key2", "value2")
.putSafeTags("key", "value")
.putSafeTags("key1", "value1")
.build();

assertThat(one).isEqualTo(two);
assertThat(two).isEqualTo(one);
assertThat(one).isEqualByComparingTo(two);
assertThat(one).usingComparator(MetricName.metricNameComparator).isEqualByComparingTo(two);

assertThat(one.toString()).isEqualTo(two.toString());
}

@Test
public void compareName() {
assertThat(MetricName.builder().safeName("a").build())
.isEqualTo(MetricName.builder().safeName("a").build());

assertThat(MetricName.builder().safeName("a").build())
.isNotEqualTo(MetricName.builder().safeName("b").build());
}

@Test
public void compareNameOrder() {
public void compareTagNames() {
MetricName one = MetricName.builder()
.safeName("a")
.putSafeTags("key", "value")
.putSafeTags("key1", "value1")
.putSafeTags("key2", "value2")
.build();
MetricName two = MetricName.builder()
.safeName("b")
.putSafeTags("key", "value")
.safeName("a")
.putSafeTags("key1", "value1")
.putSafeTags("key3", "value2")
.build();

assertThat(one).isLessThan(two);
assertThat(two).isGreaterThan(one);
assertThat(one).isNotEqualTo(two);
assertThat(two).isNotEqualTo(one);
}

@Test
public void compareSameName() {
public void compareTagValues() {
MetricName one = MetricName.builder()
.safeName("test")
.putSafeTags("a", "value")
.safeName("a")
.putSafeTags("key1", "value1")
.putSafeTags("key2", "value2")
.build();
MetricName two = MetricName.builder()
.safeName("test")
.putSafeTags("b", "value")
.safeName("a")
.putSafeTags("key1", "value1")
.putSafeTags("key2", "valueZ")
.build();

assertThat(one).isLessThan(two);
assertThat(two).isGreaterThan(one);
assertThat(one).isNotEqualTo(two);
assertThat(two).isNotEqualTo(one);
}
}
27 changes: 27 additions & 0 deletions ...egistry/src/test/java/com/palantir/tritium/metrics/registry/TaggedMetricRegistryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,33 @@ public void testReplaceMetricRegistry() {
assertThat(registry.getMetrics()).isEmpty();
}

@Test
public void testGetMetrics() {
MetricName metricName = MetricName.builder()
.safeName("counter1")
.putSafeTags("tagA", Long.toString(1))
.putSafeTags("tagB", Long.toString(2))
.build();
Counter counter = registry.counter(metricName);
counter.inc();
Metric metric = registry.getMetrics().get(metricName);
assertThat(metric)
.isInstanceOf(Counter.class)
.isSameAs(counter)
.isSameAs(registry.counter(
MetricName.builder()
.safeName("counter1")
.putSafeTags("tagB", "2")
.putSafeTags("tagA", Long.toString(1))
.build()))
.isSameAs(registry.getMetrics().get(MetricName.builder()
.safeName("counter1")
.putSafeTags("tagA", Long.toString(1))
.putSafeTags("tagB", Integer.toString(2))
.build()));
assertThat(counter.getCount()).isEqualTo(1);
}

private void assertMetric(String name, String tagKey, String tagValue, Meter meter) {
assertThat(registry.getMetrics())
.containsEntry(MetricName.builder().safeName(name).putSafeTags(tagKey, tagValue).build(), meter);
Expand Down

0 comments on commit c2b1222

Please sign in to comment.