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

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

@@ -17,9 +17,7 @@
 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.NavigableMap;
 import org.immutables.value.Value;
 
 @Value.Immutable(prehash = true)
@@ -30,25 +28,6 @@
         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;
-                    });
-
     /**
      * General/abstract measure (e.g. server.response-time).
      * <p>
@@ -61,11 +40,12 @@ public interface MetricName extends Comparable<MetricName> {
      * <p>
      * All tags and keys must be {@link Safe} to log.
      */
-    Map<String, String> safeTags();
+    @Value.NaturalOrder
+    NavigableMap<String, String> safeTags();
 
     @Override
     default int compareTo(MetricName that) {
-        return metricNameComparator.compare(this, that);
+        return MetricNames.metricNameComparator.compare(this, that);
     }
 
     static Builder builder() {

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

@@ -0,0 +1,53 @@
+/*
+ * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved.
+ *
+ * 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
+ *
+ * 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.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.palantir.tritium.metrics.registry;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.Comparator;
+import java.util.Iterator;
+import java.util.Map;
+
+final class MetricNames {
+
+    private MetricNames() {}
+
+    private static final Comparator<Map.Entry<String, String>> entryComparator =
+            Map.Entry.<String, String>comparingByKey()
+                    .thenComparing(Map.Entry.comparingByValue());
+
+    private static final Comparator<Map<String, String>> sizeComparator = Comparator.comparingInt(Map::size);
+
+    private static final Comparator<Map<String, String>> mapComparator = (m1, m2) -> {
+        Iterator<Map.Entry<String, String>> i1 = m1.entrySet().iterator();
+        Iterator<Map.Entry<String, String>> i2 = m2.entrySet().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;
+    };
+
+    @VisibleForTesting
+    static final Comparator<MetricName> metricNameComparator =
+            Comparator.comparing(MetricName::safeName)
+                    .thenComparing(MetricName::safeTags, sizeComparator)
+                    .thenComparing(MetricName::safeTags, mapComparator);
+}

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

@@ -27,27 +27,33 @@ 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).usingComparator(MetricNames.metricNameComparator).isEqualByComparingTo(two);
     }
 
     @Test
     public void compareNameOrder() {
         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")
+                .putSafeTags("key1", "value1")
+                .putSafeTags("key2", "value2")
                 .build();
 
         assertThat(one).isLessThan(two);
@@ -68,4 +74,54 @@ public void compareSameName() {
         assertThat(one).isLessThan(two);
         assertThat(two).isGreaterThan(one);
     }
+
+    @Test
+    public void compareTagsName() {
+        MetricName one = MetricName.builder()
+                .safeName("a")
+                .putSafeTags("keyA", "value1")
+                .putSafeTags("keyB", "value2")
+                .putSafeTags("keyB", "value1")
+                .build();
+        MetricName two = MetricName.builder()
+                .safeName("a")
+                .putSafeTags("keyC", "value2")
+                .putSafeTags("keyA", "value1")
+                .putSafeTags("keyB", "value1")
+                .build();
+
+        assertThat(one).isLessThan(two);
+        assertThat(two).isGreaterThan(one);
+    }
+
+    @Test
+    public void compareTagsSize() {
+        MetricName one = MetricName.builder()
+                .safeName("a")
+                .build();
+        MetricName two = MetricName.builder()
+                .safeName("a")
+                .putSafeTags("keyA", "value1")
+                .build();
+
+        assertThat(one).isLessThan(two);
+        assertThat(two).isGreaterThan(one);
+    }
+
+    @Test
+    public void compareTagsValue() {
+        MetricName one = MetricName.builder()
+                .safeName("a")
+                .putSafeTags("keyA", "value1")
+                .putSafeTags("keyB", "value2")
+                .build();
+        MetricName two = MetricName.builder()
+                .safeName("a")
+                .putSafeTags("keyB", "value3")
+                .putSafeTags("keyA", "value1")
+                .build();
+
+        assertThat(one).isLessThan(two);
+        assertThat(two).isGreaterThan(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);