From f2f25c0b89b7ad75405cddb7de92bf5020b092ae Mon Sep 17 00:00:00 2001 From: "Peter D. Stout" Date: Wed, 16 Sep 2015 07:30:11 -0700 Subject: [PATCH] refactor TagList to be more intuitive Previous behavior was based on some tests to reduce the cost of generating the dynamic ids in high volume paths compared to servo. However, it led to a number of surprising and confusing issues for users. With this change we want to clear up the behavior for users and we can optimize as needed in the future. It will now sort by the tag key and remove duplicates on update. The goal is to have more intuitive behavior for the user so they can rely on: 1. Any ids with the same set of tags will be equal. 2. Later additions will always override if the key already exists. So for `withTags(Iterable)` if there were entries with duplicate keys the iteration order would determine which one ended up getting used. --- .../com/netflix/spectator/api/DefaultId.java | 16 +- .../com/netflix/spectator/api/TagList.java | 247 ++++++++++++++++-- .../java/com/netflix/spectator/api/Utils.java | 4 +- .../spectator/api/CompatibilityTest.java | 14 +- .../netflix/spectator/api/DefaultIdTest.java | 8 +- .../spectator/api/ExtendedRegistryTest.java | 12 +- .../netflix/spectator/api/TagListTest.java | 153 ++++++++++- .../metrics3/MetricsRegistryTest.java | 2 +- 8 files changed, 399 insertions(+), 57 deletions(-) diff --git a/spectator-api/src/main/java/com/netflix/spectator/api/DefaultId.java b/spectator-api/src/main/java/com/netflix/spectator/api/DefaultId.java index 5989a0bfc..c2c9596ae 100644 --- a/spectator-api/src/main/java/com/netflix/spectator/api/DefaultId.java +++ b/spectator-api/src/main/java/com/netflix/spectator/api/DefaultId.java @@ -48,20 +48,22 @@ public DefaultId(String name) { } @Override public DefaultId withTag(Tag tag) { - return new DefaultId(name, new TagList(tag.key(), tag.value(), tags)); + return new DefaultId(name, tags == TagList.EMPTY ? new TagList(tag.key(), tag.value()) : tags.mergeTag(tag)); } @Override public DefaultId withTag(String key, String value) { - return new DefaultId(name, new TagList(key, value, tags)); + TagList tag = new TagList(key, value); + return new DefaultId(name, tags == TagList.EMPTY ? tag : tags.mergeTag(tag)); } @Override public DefaultId withTags(Iterable ts) { - TagList tmp = (tags == TagList.EMPTY) ? TagList.create(ts) : tags.prepend(ts); + TagList tmp = (tags == TagList.EMPTY) ? TagList.create(ts) : tags.mergeList(ts); return new DefaultId(name, tmp); } @Override public DefaultId withTags(Map ts) { - return withTags(TagList.create(ts)); + TagList tmp = (tags == TagList.EMPTY) ? TagList.create(ts) : tags.mergeMap(ts); + return new DefaultId(name, tmp); } /** @@ -69,7 +71,7 @@ public DefaultId(String name) { * {@code rollup(Collections.emptySet(), false)}. */ DefaultId normalize() { - return rollup(Collections.emptySet(), false); + return rollup(Collections.emptySet(), false); } /** @@ -115,8 +117,8 @@ DefaultId rollup(Set keys, boolean keep) { @Override public String toString() { StringBuilder buf = new StringBuilder(); buf.append(name); - for (Tag t : tags()) { - buf.append(":").append(t.key()).append("=").append(t.value()); + if (tags != TagList.EMPTY) { + buf.append(':').append(tags); } return buf.toString(); } diff --git a/spectator-api/src/main/java/com/netflix/spectator/api/TagList.java b/spectator-api/src/main/java/com/netflix/spectator/api/TagList.java index c81c52b0b..f1c296db5 100644 --- a/spectator-api/src/main/java/com/netflix/spectator/api/TagList.java +++ b/spectator-api/src/main/java/com/netflix/spectator/api/TagList.java @@ -17,15 +17,80 @@ import com.netflix.spectator.impl.Preconditions; +import java.util.Comparator; import java.util.Iterator; import java.util.Map; import java.util.NoSuchElementException; +import java.util.TreeMap; /** - * A tag list implemented as a singly linked list. Doesn't automatically dedup keys but supports - * a cheap prepend at the call site to allow for inexpensive dynamic ids. + * A tag list implemented as a singly linked list. The contents of the list are maintained in + * sorted order by key with no duplicates. */ final class TagList implements Iterable, Tag { + /** + * Utility class for sorting and deduplicating lists of tags. + */ + private static class TagSorterAndDeduplicator { + private static final Comparator REVERSE_STRING_COMPARATOR = + (String left, String right) -> right.compareTo(left); + + /** Map used to sort and deduplicate the presented tags. */ + private final Map map; + + /** + * Construct a new instance with no tags in it. + */ + TagSorterAndDeduplicator() { + map = new TreeMap<>(REVERSE_STRING_COMPARATOR); + } + + /** + * Adds the specified tag to the collected tags. It will overwrite any existing value + * associated the key in the specified tag. + * + * @param tag + * The tag to add to the collection. + */ + void addTag(Tag tag) { + map.put(tag.key(), tag); + } + + /** + * Adds the tags in the iterable to the collected tags. Any values associated with the tags in + * the iterable will overwrite any existing values with the same key that are already in the + * collection. + * + * @param tags + * The set of tags to add. + */ + void addTags(Iterable tags) { + for (Tag t : tags) { + map.put(t.key(), t); + } + } + + /** + * Adds the tags (key, value)-pairs to the collected tags. Any values associated with the tags + * in the map will overwrite any existing values with the same key that are already in the + * collection. + * + * @param tags + * The set of tags to add. + */ + void addTags(Map tags) { + for (Map.Entry t : tags.entrySet()) { + map.put(t.getKey(), new TagList(t.getKey(), t.getValue())); + } + } + + /** + * Returns the sorted set of tags. + */ + Iterable sortedTags() { + return map.values(); + } + } private final String key; private final String value; @@ -40,9 +105,10 @@ final class TagList implements Iterable, Tag { } /** - * Create a new instance with a new tag prepended to the list {@code next}. + * Create a new instance with a new tag prepended to the list {@code next}. Any entries in next + * should have keys that are lexicographically after the specified key. */ - TagList(String key, String value, TagList next) { + private TagList(String key, String value, TagList next) { this.key = Preconditions.checkNotNull(key, "key"); this.value = Preconditions.checkNotNull(value, "value"); this.next = next; @@ -101,20 +167,121 @@ public void remove() { return hc; } + @Override + public String toString() { + StringBuilder buf = new StringBuilder(); + TagList cur = next; + + buf.append(key).append('=').append(value); + while (cur != null) { + buf.append(":").append(cur.key()).append("=").append(cur.value()); + cur = cur.next; + } + return buf.toString(); + } + + /** + * Produces a list with with the specified tag merged with the existing values in this list. If + * the key of the specified tag matches an existing list entry, then the value of the specified + * tag will replace the existing value. + * + * @param tag + * Possibly null tag to merge into the list. + * @return + * A tag list with merged values. + */ + TagList mergeTag(Tag tag) { + if (tag == null) { + return this; + } else if (next == null) { + int comparison = key.compareTo(tag.key()); + + if (comparison == 0) { // Same key, so the specified value replaces the current value. + return new TagList(tag.key(), tag.value(), EMPTY); + } else if (comparison < 0) { // The key in this list is before the key in the specified list. + return new TagList(key, value, new TagList(tag.key(), tag.value(), EMPTY)); + } else { // The key in this list is after the key in the specified list. + return new TagList(tag.key(), tag.value(), this); + } + } else { + // Is it possible to optimize this case so as to reuse the tail of the existing TagList? + TagSorterAndDeduplicator entries = new TagSorterAndDeduplicator(); + + entries.addTags(this); + entries.addTag(tag); + + return createFromSortedTags(entries.sortedTags()); + } + } + /** - * Create a new list with the tags prepended to this list. + * Produces a list with the tags from this list merged with the tags in the specified list. For + * any keys present in both lists, the value from the specified list will replace the existing + * value. * * @param tags - * A set of tags to prepend. + * A set of tags to merge. * @return - * New tag list with the tags prepended. + * A tag list with the merged values. Based on the inputs the result may be this, tags, or + * a new object. */ - TagList prepend(Iterable tags) { - TagList head = this; - for (Tag t : tags) { - head = new TagList(t.key(), t.value(), head); + TagList mergeList(Iterable tags) { + if (tags == null) { + return this; + } + + Iterator iter = tags.iterator(); + + if (iter.hasNext()) { + Tag firstTag = iter.next(); + + if (iter.hasNext()) { + // Iterator has multiple entries so we need to sort them and remove any duplicates. + TagSorterAndDeduplicator entries = new TagSorterAndDeduplicator(); + + entries.addTags(this); + entries.addTags(tags); + + return createFromSortedTags(entries.sortedTags()); + } else { + // Single entry iterator. + return mergeTag(firstTag); + } + } else { + // Empty iterator + return this; + } + } + + /** + * Produces a list with the tags from this list merged with the tags in the specified list. For + * any keys present in both lists, the value from the specified list will replace the existing + * value. + * + * @param tags + * A set of tags to merge. + * @return + * A tag list with the merged values. Based on the inputs the result may be this or a new + * object. + */ + TagList mergeMap(Map tags) { + if (tags == null || tags.isEmpty()) { + return this; + } + + if (tags.size() == 1) { + Map.Entry entry = tags.entrySet().iterator().next(); + + return mergeTag(new TagList(entry.getKey(), entry.getValue(), EMPTY)); + } else { + // Iterator has multiple entries so we need to sort them and remove any duplicates. + TagSorterAndDeduplicator entries = new TagSorterAndDeduplicator(); + + entries.addTags(this); + entries.addTags(tags); + + return createFromSortedTags(entries.sortedTags()); } - return head; } /** @@ -129,11 +296,26 @@ static TagList create(Iterable tags) { if (tags == EMPTY || tags instanceof TagList) { return (TagList) tags; } else { - TagList head = EMPTY; - for (Tag t : tags) { - head = new TagList(t.key(), t.value(), head); + Iterator iter = tags.iterator(); + + if (iter.hasNext()) { + Tag firstTag = iter.next(); + + if (iter.hasNext()) { + // Iterator has multiple entries so we need to sort them and remove any duplicates. + TagSorterAndDeduplicator entries = new TagSorterAndDeduplicator(); + + entries.addTags(tags); + + return createFromSortedTags(entries.sortedTags()); + } else { + // Single entry iterator. + return new TagList(firstTag.key(), firstTag.value(), EMPTY); + } + } else { + // Empty iterator + return EMPTY; } - return head; } } @@ -147,8 +329,37 @@ static TagList create(Iterable tags) { */ static TagList create(Map tags) { TagList head = EMPTY; - for (Map.Entry t : tags.entrySet()) { - head = new TagList(t.getKey(), t.getValue(), head); + + if (tags.size() >= 2) { + TagSorterAndDeduplicator entries = new TagSorterAndDeduplicator(); + + for (Map.Entry t : tags.entrySet()) { + entries.addTag(new TagList(t.getKey(), t.getValue())); + } + head = createFromSortedTags(entries.sortedTags()); + } else { + for (Map.Entry t : tags.entrySet()) { + head = new TagList(t.getKey(), t.getValue(), head); + } + } + + return head; + } + + /** + * Create a tag list from a sorted list with no duplicates. The TagList is created with the + * entries in the reverse order of the entries in the provided argument. + * + * @param sortedTags + * The sorted collection of tags to use to create the list + * @return + * The newly constructed tag list or {@code EMPTY} if the iterable contains no entries + */ + private static TagList createFromSortedTags(Iterable sortedTags) { + TagList head = EMPTY; + + for (Tag t : sortedTags) { + head = new TagList(t.key(), t.value(), head); } return head; } diff --git a/spectator-api/src/main/java/com/netflix/spectator/api/Utils.java b/spectator-api/src/main/java/com/netflix/spectator/api/Utils.java index 4b59fbb7c..1020c9142 100644 --- a/spectator-api/src/main/java/com/netflix/spectator/api/Utils.java +++ b/spectator-api/src/main/java/com/netflix/spectator/api/Utils.java @@ -249,9 +249,9 @@ static Iterable toIterable(String[] tags) { if (tags.length % 2 == 1) { throw new IllegalArgumentException("size must be even, it is a set of key=value pairs"); } - TagList ts = TagList.EMPTY; + ArrayList ts = new ArrayList<>(tags.length); for (int i = 0; i < tags.length; i += 2) { - ts = new TagList(tags[i], tags[i + 1], ts); + ts.add(new TagList(tags[i], tags[i + 1])); } return ts; } diff --git a/spectator-api/src/test/java/com/netflix/spectator/api/CompatibilityTest.java b/spectator-api/src/test/java/com/netflix/spectator/api/CompatibilityTest.java index cbcf84920..7349e1a8b 100644 --- a/spectator-api/src/test/java/com/netflix/spectator/api/CompatibilityTest.java +++ b/spectator-api/src/test/java/com/netflix/spectator/api/CompatibilityTest.java @@ -37,24 +37,24 @@ public class CompatibilityTest { EXPECTED.add("Measurement(collection-size,1234567890,8.0)"); EXPECTED.add("Measurement(counter,1234567890,127.0)"); EXPECTED.add("Measurement(counter:a=b,1234567890,381.0)"); + EXPECTED.add("Measurement(dist:a=b:statistic=count,1234567890,15.0)"); + EXPECTED.add("Measurement(dist:a=b:statistic=totalAmount,1234567890,504.0)"); EXPECTED.add("Measurement(dist:statistic=count,1234567890,5.0)"); - EXPECTED.add("Measurement(dist:statistic=count:a=b,1234567890,15.0)"); EXPECTED.add("Measurement(dist:statistic=totalAmount,1234567890,168.0)"); - EXPECTED.add("Measurement(dist:statistic=totalAmount:a=b,1234567890,504.0)"); EXPECTED.add("Measurement(gauge,1234567890,49.0)"); EXPECTED.add("Measurement(gauge-age,1234567890,0.049)"); EXPECTED.add("Measurement(gauge-function,1234567890,65.0)"); - EXPECTED.add("Measurement(gauge:node=i-12345:asg=foo-dev-v001:cluster=foo-dev:app=foo,1234567890,7.0)"); + EXPECTED.add("Measurement(gauge:app=foo:asg=foo-dev-v001:cluster=foo-dev:node=i-12345,1234567890,7.0)"); + EXPECTED.add("Measurement(long-timer:a=b:statistic=activeTasks,1234567890,3.0)"); + EXPECTED.add("Measurement(long-timer:a=b:statistic=duration,1234567890,15120.0)"); EXPECTED.add("Measurement(long-timer:statistic=activeTasks,1234567890,1.0)"); - EXPECTED.add("Measurement(long-timer:statistic=activeTasks:a=b,1234567890,3.0)"); EXPECTED.add("Measurement(long-timer:statistic=duration,1234567890,10080.0)"); - EXPECTED.add("Measurement(long-timer:statistic=duration:a=b,1234567890,15120.0)"); EXPECTED.add("Measurement(map-size,1234567890,8.0)"); EXPECTED.add("Measurement(method-value,1234567890,22.0)"); + EXPECTED.add("Measurement(timer:a=b:statistic=count,1234567890,24.0)"); + EXPECTED.add("Measurement(timer:a=b:statistic=totalTime,1234567890,4.53852000126E14)"); EXPECTED.add("Measurement(timer:statistic=count,1234567890,8.0)"); - EXPECTED.add("Measurement(timer:statistic=count:a=b,1234567890,24.0)"); EXPECTED.add("Measurement(timer:statistic=totalTime,1234567890,1.51284000042E14)"); - EXPECTED.add("Measurement(timer:statistic=totalTime:a=b,1234567890,4.53852000126E14)"); } @Test diff --git a/spectator-api/src/test/java/com/netflix/spectator/api/DefaultIdTest.java b/spectator-api/src/test/java/com/netflix/spectator/api/DefaultIdTest.java index 0be4177df..64da13af2 100644 --- a/spectator-api/src/test/java/com/netflix/spectator/api/DefaultIdTest.java +++ b/spectator-api/src/test/java/com/netflix/spectator/api/DefaultIdTest.java @@ -58,7 +58,7 @@ public void testTagsEmpty() { @Test public void equalsContractTest() { TagList ts1 = new TagList("k1", "v1"); - TagList ts2 = new TagList("k2", "v2", ts1); + TagList ts2 = new TagList("k2", "v2").mergeTag(ts1); EqualsVerifier .forClass(DefaultId.class) .withPrefabValues(TagList.class, ts1, ts2) @@ -70,7 +70,7 @@ public void equalsContractTest() { public void testNormalize() { DefaultId id12 = (new DefaultId("foo")).withTag("k1", "v1").withTag("k2", "v2"); DefaultId id21 = (new DefaultId("foo")).withTag("k1", "v1").withTags(id12.tags()); - Assert.assertTrue(!id12.equals(id21)); + Assert.assertEquals(id12, id21); Assert.assertEquals(id12, id21.normalize()); } @@ -113,7 +113,7 @@ public void testRollupDedupingOfExcludedKey() { @Test public void testToString() { DefaultId id = (new DefaultId("foo")).withTag("k1", "v1").withTag("k2", "v2"); - Assert.assertEquals(id.toString(), "foo:k2=v2:k1=v1"); + Assert.assertEquals("foo:k1=v1:k2=v2", id.toString()); } @Test @@ -128,6 +128,6 @@ public void testWithTagsMap() { map.put("k1", "v1"); map.put("k2", "v2"); DefaultId id = (new DefaultId("foo")).withTags(map); - Assert.assertEquals(id.toString(), "foo:k2=v2:k1=v1"); + Assert.assertEquals("foo:k1=v1:k2=v2", id.toString()); } } diff --git a/spectator-api/src/test/java/com/netflix/spectator/api/ExtendedRegistryTest.java b/spectator-api/src/test/java/com/netflix/spectator/api/ExtendedRegistryTest.java index 8daf1c5b9..86faed0c6 100644 --- a/spectator-api/src/test/java/com/netflix/spectator/api/ExtendedRegistryTest.java +++ b/spectator-api/src/test/java/com/netflix/spectator/api/ExtendedRegistryTest.java @@ -40,7 +40,7 @@ public void init() { public void testCreateIdArray() { Registry r = new DefaultRegistry(); Id id1 = r.createId("foo", "bar", "baz", "k", "v"); - Id id2 = r.createId("foo", new TagList("k", "v", new TagList("bar", "baz"))); + Id id2 = r.createId("foo", new TagList("k", "v").mergeTag(new TagList("bar", "baz"))); Assert.assertEquals(id1, id2); } @@ -65,7 +65,7 @@ public void testCreateIdMap() { public void testCounterHelpers() { Registry r = new DefaultRegistry(); Counter c1 = r.counter("foo", "bar", "baz", "k", "v"); - Counter c2 = r.counter("foo", new TagList("k", "v", new TagList("bar", "baz"))); + Counter c2 = r.counter("foo", new TagList("k", "v").mergeTag(new TagList("bar", "baz"))); Counter c3 = r.counter("foo"); Assert.assertSame(c1, c2); Assert.assertNotSame(c1, c3); @@ -76,7 +76,7 @@ public void testDistributionSummaryHelpers() { Registry r = new DefaultRegistry(); DistributionSummary c1 = r.distributionSummary("foo", "bar", "baz", "k", "v"); DistributionSummary c2 = r.distributionSummary("foo", - new TagList("k", "v", new TagList("bar", "baz"))); + new TagList("k", "v").mergeTag(new TagList("bar", "baz"))); DistributionSummary c3 = r.distributionSummary("foo"); Assert.assertSame(c1, c2); Assert.assertNotSame(c1, c3); @@ -86,7 +86,7 @@ public void testDistributionSummaryHelpers() { public void testTimerHelpers() { Registry r = new DefaultRegistry(); Timer c1 = r.timer("foo", "bar", "baz", "k", "v"); - Timer c2 = r.timer("foo", new TagList("k", "v", new TagList("bar", "baz"))); + Timer c2 = r.timer("foo", new TagList("k", "v").mergeTag(new TagList("bar", "baz"))); Timer c3 = r.timer("foo"); Assert.assertSame(c1, c2); Assert.assertNotSame(c1, c3); @@ -100,7 +100,7 @@ public void testLongTaskTimerHelpers() { Meter m1 = r.get(c1.id()); Assert.assertEquals(c1.id(), m1.id()); // registration - LongTaskTimer c2 = r.longTaskTimer("foo", new TagList("k", "v", new TagList("bar", "baz"))); + LongTaskTimer c2 = r.longTaskTimer("foo", new TagList("k", "v").mergeTag(new TagList("bar", "baz"))); Assert.assertEquals(c1.id(), c2.id()); long t1 = c1.start(); @@ -123,7 +123,7 @@ public void testGaugeHelpers() { AtomicLong al4 = new AtomicLong(4L); Registry r = new DefaultRegistry(); AtomicLong v1 = r.gauge(r.createId("foo", "bar", "baz", "k", "v"), al1); - AtomicLong v2 = r.gauge("foo", new TagList("k", "v", new TagList("bar", "baz")), al2); + AtomicLong v2 = r.gauge("foo", new TagList("k", "v").mergeTag(new TagList("bar", "baz")), al2); AtomicLong v3 = r.gauge("foo", al4); Assert.assertSame(v1, al1); Assert.assertSame(v2, al2); diff --git a/spectator-api/src/test/java/com/netflix/spectator/api/TagListTest.java b/spectator-api/src/test/java/com/netflix/spectator/api/TagListTest.java index c7e4834e3..188c3015a 100644 --- a/spectator-api/src/test/java/com/netflix/spectator/api/TagListTest.java +++ b/spectator-api/src/test/java/com/netflix/spectator/api/TagListTest.java @@ -20,10 +20,14 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.Iterator; +import java.util.List; import java.util.Map; +import java.util.NoSuchElementException; @RunWith(JUnit4.class) public class TagListTest { @@ -32,16 +36,27 @@ public class TagListTest { public void equalsContractTest() { // NOTE: EqualsVerifier doesn't work with cached hash code TagList ts1 = new TagList("k1", "v1"); - TagList ts2 = new TagList("k2", "v2", ts1); - Assert.assertTrue(ts1.equals(ts1)); - Assert.assertTrue(ts2.equals(ts2)); - Assert.assertTrue(!ts1.equals(null)); - Assert.assertTrue(!ts1.equals(new Object())); - Assert.assertTrue(!ts1.equals(new TagList("k1", "v2"))); - Assert.assertTrue(!ts1.equals(new TagList("k2", "v1"))); - Assert.assertTrue(!ts1.equals(new TagList("k1", "v1", ts2))); - Assert.assertTrue(ts2.equals(new TagList("k2", "v2", ts1))); - Assert.assertTrue(ts2.equals(new TagList("k2", "v2", new TagList("k1", "v1")))); + TagList ts2 = new TagList("k2", "v2").mergeTag(ts1); + Assert.assertEquals(ts1, ts1); + Assert.assertEquals(ts2, ts2); + Assert.assertNotEquals(ts1, null); + Assert.assertNotEquals(ts1, new Object()); + Assert.assertNotEquals(ts1, new TagList("k1", "v2")); + Assert.assertNotEquals(ts1, new TagList("k2", "v1")); + Assert.assertNotEquals(ts1, new TagList("k1", "v1").mergeList(ts2)); + Assert.assertEquals(ts2, new TagList("k2", "v2").mergeTag(ts1)); + Assert.assertEquals(ts2, new TagList("k2", "v2").mergeTag(new TagList("k1", "v1"))); + } + + @Test + public void testToString() { + TagList ts1 = new TagList("k1", "v1"); + TagList ts2 = new TagList("k2", "v2").mergeTag(ts1); + TagList ts3 = new TagList("k3", "v3").mergeList(ts2); + + Assert.assertEquals("k1=v1", ts1.toString()); + Assert.assertEquals("k1=v1:k2=v2", ts2.toString()); + Assert.assertEquals("k1=v1:k2=v2:k3=v3", ts3.toString()); } @Test @@ -64,6 +79,22 @@ public void testNullValue() { new TagList("k", null); } + @Test(expected = UnsupportedOperationException.class) + public void testIteratorRemoveUnsupported() { + new TagList("k", "v").iterator().remove(); + } + + @Test(expected = NoSuchElementException.class) + public void testIteratorNext() { + TagList tag = new TagList("k", "v"); + Iterator iter = tag.iterator(); + + Assert.assertTrue(iter.hasNext()); + Assert.assertSame(tag, iter.next()); + Assert.assertFalse(iter.hasNext()); + iter.next(); + } + @Test public void testCreateFromMap() { Map m = new HashMap<>(); @@ -73,6 +104,16 @@ public void testCreateFromMap() { Assert.assertEquals(ts1, ts2); } + @Test + public void testCreateFromMapWithMultipleValues() { + Map m = new HashMap<>(); + m.put("k1", "v1"); + m.put("k2", "v2"); + TagList ts1 = TagList.create(m); + TagList ts2 = new TagList("k1", "v1").mergeTag(new TagList("k2", "v2")); + Assert.assertEquals(ts1, ts2); + } + @Test public void testCreateFromTagList() { TagList ts = new TagList("k", "v"); @@ -82,10 +123,98 @@ public void testCreateFromTagList() { } @Test - public void testCreateFromIterable() { - Collection coll = Collections.singleton(new TagList("k", "v")); + public void testCreateFromEmptyIterable() { + Assert.assertEquals(TagList.EMPTY, TagList.create(Collections.emptyList())); + } + + @Test + public void testCreateFromSingleValueIterable() { + Collection coll = Collections.singleton(new TagList("k", "v")); TagList ts1 = TagList.create(coll); TagList ts2 = new TagList("k", "v"); Assert.assertEquals(ts1, ts2); } + + @Test + public void testCreateFromMultiValueIterable() { + List coll = new ArrayList<>(); + coll.add(new TagList("k1", "v1")); + coll.add(new TagList("k2", "v2")); + TagList ts1 = TagList.create(coll); + TagList ts2 = new TagList("k1", "v1").mergeTag(new TagList("k2", "v2")); + Assert.assertEquals(ts1, ts2); + } + + @Test + public void testCreateFromEmptyMap() { + Assert.assertEquals(TagList.EMPTY, TagList.create(Collections.emptyMap())); + } + + @Test + public void testCreateFromSingleValueMap() { + Map tags = new HashMap<>(); + + tags.put("k", "v"); + Assert.assertEquals(new TagList("k", "v"), TagList.create(tags)); + } + + @Test + public void testCreateFromMultiValueMap() { + Map tags = new HashMap<>(); + + tags.put("k1", "v1"); + tags.put("k2", "v2"); + Assert.assertEquals(new TagList("k1", "v1").mergeTag(new TagList("k2", "v2")), TagList.create(tags)); + } + + @Test + public void testMergeNullTag() { + TagList expected = new TagList("k", "v"); + + Assert.assertSame(expected, expected.mergeTag(null)); + } + + @Test + public void testMergeTag() { + TagList initial = new TagList("k2", "v2"); + TagList update = new TagList("k1", "v1"); + TagList expected = new TagList("k1", "v1").mergeTag(new TagList("k2", "v2")); + + Assert.assertEquals(expected, initial.mergeTag(update)); + } + + @Test + public void testMergeTagWithSameKey() { + Iterable prefix = Collections.singletonList(new TagList("k1", "v1")); + TagList initial = new TagList("k1", "v1"); + TagList expected = new TagList("k1", "v2"); + TagList actual = initial.mergeTag(expected); + + Assert.assertNotSame(expected, actual); + Assert.assertEquals(expected, actual); + } + + @Test + public void testMergeMultipleValuesAsList() { + ArrayList prefix = new ArrayList<>(); + TagList initial = new TagList("k3", "v3"); + TagList expected = new TagList("k1", "v1").mergeTag(new TagList("k2", "v2")).mergeTag(new TagList("k3", "v3")); + + prefix.add(new TagList("k1", "v1")); + prefix.add(new TagList("k2", "v2")); + TagList actual = initial.mergeList(prefix); + Assert.assertEquals(expected, actual); + } + + @Test + public void testMergeMultipleValuesAsMap() { + Map extra = new HashMap<>(); + TagList initial = new TagList("k3", "v3"); + TagList expected = new TagList("k1", "v1").mergeTag(new TagList("k2", "v2")).mergeTag(new TagList("k3", "v3")); + + extra.put("k1", "v1"); + extra.put("k2", "v2"); + TagList actual = initial.mergeMap(extra); + Assert.assertEquals(expected, actual); + } } diff --git a/spectator-reg-metrics3/src/test/java/com/netflix/spectator/metrics3/MetricsRegistryTest.java b/spectator-reg-metrics3/src/test/java/com/netflix/spectator/metrics3/MetricsRegistryTest.java index 8ae929a8f..562deccae 100644 --- a/spectator-reg-metrics3/src/test/java/com/netflix/spectator/metrics3/MetricsRegistryTest.java +++ b/spectator-reg-metrics3/src/test/java/com/netflix/spectator/metrics3/MetricsRegistryTest.java @@ -36,7 +36,7 @@ public void metricName() { MetricRegistry codaRegistry = new MetricRegistry(); MetricsRegistry r = new MetricsRegistry(clock, codaRegistry); r.counter("foo", "id", "bar", "a", "b", "a", "c").increment(); - Assert.assertTrue(codaRegistry.getMeters().containsKey("foo.id-bar.a-c")); + Assert.assertTrue(codaRegistry.getMeters().containsKey("foo.a-c.id-bar")); } @Test