From c5c7d1a4aafffd64c8ce9ec0221ff2cb35241c77 Mon Sep 17 00:00:00 2001 From: Brian Harrington Date: Thu, 28 Apr 2016 17:03:04 -0700 Subject: [PATCH 1/3] restrict search to used portion of array It was using the full array length before which can lead to ArrayIndexOutOfBoundsExceptions if a dedup occurs before an append. --- .../netflix/spectator/api/ArrayTagSet.java | 2 +- .../netflix/spectator/api/DefaultIdTest.java | 30 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/spectator-api/src/main/java/com/netflix/spectator/api/ArrayTagSet.java b/spectator-api/src/main/java/com/netflix/spectator/api/ArrayTagSet.java index 8fa4ffa13..404e3fc6a 100644 --- a/spectator-api/src/main/java/com/netflix/spectator/api/ArrayTagSet.java +++ b/spectator-api/src/main/java/com/netflix/spectator/api/ArrayTagSet.java @@ -99,7 +99,7 @@ ArrayTagSet add(String k, String v) { /** Add a new tag to the set. */ ArrayTagSet add(Tag tag) { Tag[] newTags; - int pos = Arrays.binarySearch(tags, tag, TAG_COMPARATOR); + int pos = Arrays.binarySearch(tags, 0, length, tag, TAG_COMPARATOR); if (pos < 0) { // Not found in list newTags = new Tag[length + 1]; 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 338d1aece..f4f36b2e9 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 @@ -130,4 +130,34 @@ public void testWithTagsMap() { DefaultId id = (new DefaultId("foo")).withTags(map); Assert.assertEquals("foo:k1=v1:k2=v2", id.toString()); } + + @Test + public void addTagAppend() { + new DefaultId("TotalTime") + .withTag("app", "foo") + .withTag("exception.thrown", "pvr"); + } + + @Test + public void addTagPrepend() { + new DefaultId("TotalTime") + .withTag("app", "foo") + .withTag("aaa", "pvr"); + } + + @Test + public void addTagInsert() { + new DefaultId("TotalTime") + .withTag("app", "foo") + .withTag("exception.thrown", "pvr") + .withTag("bbb", "bar"); + } + + @Test + public void dedupAndAppend() { + Id id = new DefaultId("TotalTime") + .withTag("app", "foo") + .withTags("app", "foo") + .withTag("exception.thrown", "pvr"); + } } From 2e01867b6e553ccc2c862a6e10025880f3469b12 Mon Sep 17 00:00:00 2001 From: Brian Harrington Date: Thu, 28 Apr 2016 19:27:33 -0700 Subject: [PATCH 2/3] improve test coverage of ArrayTagSet --- .../spectator/api/ArrayTagSetTest.java | 20 +++++++++++++++++++ .../netflix/spectator/api/DefaultIdTest.java | 10 +++++++--- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/spectator-api/src/test/java/com/netflix/spectator/api/ArrayTagSetTest.java b/spectator-api/src/test/java/com/netflix/spectator/api/ArrayTagSetTest.java index ab90c69e1..fb277eeba 100644 --- a/spectator-api/src/test/java/com/netflix/spectator/api/ArrayTagSetTest.java +++ b/spectator-api/src/test/java/com/netflix/spectator/api/ArrayTagSetTest.java @@ -261,4 +261,24 @@ public void testMergeMultipleValuesAsMap() { ArrayTagSet actual = initial.addAll(extra); Assert.assertEquals(expected, actual); } + + @Test + public void addAllTagArrayEmpty() { + ArrayTagSet ts = ArrayTagSet.EMPTY.addAll(new Tag[0]); + Assert.assertSame(ArrayTagSet.EMPTY, ts); + } + + @Test + public void addAllStringArrayEmpty() { + ArrayTagSet ts = ArrayTagSet.EMPTY.addAll(new String[0]); + Assert.assertSame(ArrayTagSet.EMPTY, ts); + } + + @Test + public void addAllIterable() { + // Add an arbitrary iterable that isn't a collection or ArrayTagSet + Collection tags = Collections.singletonList(new BasicTag("app", "foo")); + ArrayTagSet ts = ArrayTagSet.EMPTY.addAll(tags::iterator); + Assert.assertEquals(ArrayTagSet.EMPTY.addAll(tags), ts); + } } 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 f4f36b2e9..a5397eee6 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 @@ -133,24 +133,27 @@ public void testWithTagsMap() { @Test public void addTagAppend() { - new DefaultId("TotalTime") + Id id = new DefaultId("TotalTime") .withTag("app", "foo") .withTag("exception.thrown", "pvr"); + Assert.assertEquals("TotalTime:app=foo:exception.thrown=pvr", id.toString()); } @Test public void addTagPrepend() { - new DefaultId("TotalTime") + Id id = new DefaultId("TotalTime") .withTag("app", "foo") .withTag("aaa", "pvr"); + Assert.assertEquals("TotalTime:aaa=pvr:app=foo", id.toString()); } @Test public void addTagInsert() { - new DefaultId("TotalTime") + Id id = new DefaultId("TotalTime") .withTag("app", "foo") .withTag("exception.thrown", "pvr") .withTag("bbb", "bar"); + Assert.assertEquals("TotalTime:app=foo:bbb=bar:exception.thrown=pvr", id.toString()); } @Test @@ -159,5 +162,6 @@ public void dedupAndAppend() { .withTag("app", "foo") .withTags("app", "foo") .withTag("exception.thrown", "pvr"); + Assert.assertEquals("TotalTime:app=foo:exception.thrown=pvr", id.toString()); } } From 59dc2a8be9129ea3bf0f128e0d6a9294cbfec372 Mon Sep 17 00:00:00 2001 From: Brian Harrington Date: Thu, 28 Apr 2016 19:33:52 -0700 Subject: [PATCH 3/3] update recommended version to 0.38.1 --- README.md | 2 +- docs/ext/jvm-gc.md | 2 +- docs/ext/log4j2.md | 2 +- docs/index.md | 2 +- docs/intro/netflix.md | 4 ++-- docs/registry/metrics3.md | 2 +- docs/registry/servo.md | 2 +- docs/registry/tdigest.md | 2 +- 8 files changed, 9 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index ab137ed0e..8f9fb3905 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ To instrument your code you need to depend on the api library. This provides the for you to code against and build test cases. The only dependency is slf4j. ``` -com.netflix.spectator:spectator-api:0.38.0 +com.netflix.spectator:spectator-api:0.38.1 ``` If running at Netflix with the standard platform, see the diff --git a/docs/ext/jvm-gc.md b/docs/ext/jvm-gc.md index e236ad11e..900cb2f92 100644 --- a/docs/ext/jvm-gc.md +++ b/docs/ext/jvm-gc.md @@ -22,7 +22,7 @@ to 7u40. For G1 it is recommended to be on the latest version available. ### Dependencies ``` -com.netflix.spectator:spectator-ext-gc:0.38.0 +com.netflix.spectator:spectator-ext-gc:0.38.1 ``` ### Start Reporting diff --git a/docs/ext/log4j2.md b/docs/ext/log4j2.md index 940879c88..212675da3 100644 --- a/docs/ext/log4j2.md +++ b/docs/ext/log4j2.md @@ -8,7 +8,7 @@ log messages reported. To use it simply add a dependency: ``` -com.netflix.spectator:spectator-ext-log4j2:0.38.0 +com.netflix.spectator:spectator-ext-log4j2:0.38.1 ``` Then in your application initialization: diff --git a/docs/index.md b/docs/index.md index a579bf962..5d330b888 100644 --- a/docs/index.md +++ b/docs/index.md @@ -5,7 +5,7 @@ you are new to the library it is highly recommended to read the pages in the At a minimum you will need to: 1. Depend on the api library. It is in maven central, for gradle the dependency - would be `com.netflix.spectator:spectator-api:0.38.0`. + would be `com.netflix.spectator:spectator-api:0.38.1`. 2. Instrument some code, see the usage guides for [counters](intro/counter.md), [timers](intro/timer.md), and [gauges](intro/gauge.md). 3. Pick a registry to bind to when initializing the application. See the sidebar diff --git a/docs/intro/netflix.md b/docs/intro/netflix.md index 9fd86128c..4b08ff42a 100644 --- a/docs/intro/netflix.md +++ b/docs/intro/netflix.md @@ -13,7 +13,7 @@ section for the type of project you are working on: For libraries, the only dependency that should be needed is: ``` -com.netflix.spectator:spectator-api:0.38.0 +com.netflix.spectator:spectator-api:0.38.1 ``` The bindings to integrate internally should be included with the application. In your code, @@ -80,7 +80,7 @@ If you are only interested in getting the GC logging, there is a library with an singleton that can be used: ``` -com.netflix.spectator:spectator-nflx:0.38.0 +com.netflix.spectator:spectator-nflx:0.38.1 ``` Assuming you are using karyon/base-server or governator with `com.netflix` in the list of base diff --git a/docs/registry/metrics3.md b/docs/registry/metrics3.md index 99e8ba084..8f50b12e2 100644 --- a/docs/registry/metrics3.md +++ b/docs/registry/metrics3.md @@ -5,7 +5,7 @@ underlying implementation. To use the metrics registry, add a dependency on the `spectator-reg-metrics3` library. For gradle: ``` -com.netflix.spectator:spectator-reg-metrics3:0.38.0 +com.netflix.spectator:spectator-reg-metrics3:0.38.1 ``` Then when initializing the application, use the `MetricsRegistry`. For more diff --git a/docs/registry/servo.md b/docs/registry/servo.md index 71c3b630f..413b08596 100644 --- a/docs/registry/servo.md +++ b/docs/registry/servo.md @@ -5,7 +5,7 @@ implementation. To use the servo registry, add a dependency on the `spectator-reg-servo` library. For gradle: ``` -com.netflix.spectator:spectator-reg-servo:0.38.0 +com.netflix.spectator:spectator-reg-servo:0.38.1 ``` Then when initializing the application, use the `ServoRegistry`. If using guice diff --git a/docs/registry/tdigest.md b/docs/registry/tdigest.md index d5a5a3def..e5884235e 100644 --- a/docs/registry/tdigest.md +++ b/docs/registry/tdigest.md @@ -12,7 +12,7 @@ assume it is being used [internally at Netflix](Netflix-Integration). To use it simply add a dependency: ``` -com.netflix.spectator:spectator-reg-tdigest:0.38.0 +com.netflix.spectator:spectator-reg-tdigest:0.38.1 ``` Add the `TDigestModule` to the set of modules used with guice: