[break] Fix MetricName comparator for tags, MetricName#safeTags() returns SortedMap

tritium-caffeine/src/main/java/com/palantir/tritium/metrics/caffeine/CaffeineCacheStats.java

@@ -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);
     }
 }

tritium-metrics/src/main/java/com/palantir/tritium/metrics/InternalCacheMetrics.java

@@ -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;
@@ -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();
     }

tritium-registry/src/main/java/com/palantir/tritium/metrics/registry/AbstractTaggedMetricRegistry.java

@@ -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;
@@ -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(

tritium-registry/src/main/java/com/palantir/tritium/metrics/registry/MetricName.java

@@ -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).
@@ -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();
 
     static Builder builder() {
         return new Builder();

tritium-registry/src/test/java/com/palantir/tritium/metrics/registry/MetricNameTest.java

@@ -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);
     }
 }

tritium-registry/src/test/java/com/palantir/tritium/metrics/registry/TaggedMetricRegistryTest.java

@@ -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);