Missing metric (#2235)

* Fix NPE when removing a non-existent metric; add tests for removal Signed-off-by: tim.quinn@oracle.com <tim.quinn@oracle.com>
helidon-io · Aug 5, 2020 · 9db48af · 9db48af
1 parent 68ad56d
commit 9db48af
Show file tree

Hide file tree

Showing 2 changed files with 76 additions and 4 deletions.
diff --git a/metrics/metrics/src/main/java/io/helidon/metrics/Registry.java b/metrics/metrics/src/main/java/io/helidon/metrics/Registry.java
@@ -204,7 +204,11 @@ public ConcurrentGauge concurrentGauge(Metadata metadata, Tag... tags) {
      */
     @Override
     public synchronized boolean remove(String name) {
-        final boolean result = allMetricIDsByName.get(name).stream()
+        final List<MetricID> metricIDs = allMetricIDsByName.get(name);
+        if (metricIDs == null) {
+            return false;
+        }
+        final boolean result = metricIDs.stream()
                 .map(metricID -> allMetrics.remove(metricID) != null)
                 .reduce((a, b) -> a || b)
                 .orElse(false);
@@ -221,9 +225,12 @@ public synchronized boolean remove(String name) {
      */
     @Override
     public synchronized boolean remove(MetricID metricID) {
-        final List<MetricID> likeNamedMetrics = allMetricIDsByName.get(metricID.getName());
-        likeNamedMetrics.remove(metricID);
-        if (likeNamedMetrics.isEmpty()) {
+        final List<MetricID> metricIDS = allMetricIDsByName.get(metricID.getName());
+        if (metricIDS == null) {
+            return false;
+        }
+        metricIDS.remove(metricID);
+        if (metricIDS.isEmpty()) {
             allMetricIDsByName.remove(metricID.getName());
             allMetadata.remove(metricID.getName());
         }

diff --git a/metrics/metrics/src/test/java/io/helidon/metrics/RegistryTest.java b/metrics/metrics/src/test/java/io/helidon/metrics/RegistryTest.java
@@ -18,6 +18,9 @@
 
 import org.eclipse.microprofile.metrics.Counter;
 import org.eclipse.microprofile.metrics.Metadata;
+import org.eclipse.microprofile.metrics.Metric;
+import org.eclipse.microprofile.metrics.MetricFilter;
+import org.eclipse.microprofile.metrics.MetricID;
 import org.eclipse.microprofile.metrics.MetricRegistry;
 import org.eclipse.microprofile.metrics.MetricType;
 import org.eclipse.microprofile.metrics.MetricUnits;
@@ -26,8 +29,11 @@
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 
+import java.util.Map;
+
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 /**
@@ -148,4 +154,63 @@ void testSameIDSameReuseDifferentOtherMetadata() {
                 () -> registry.counter(metadata2, tag1));
         assertThat(ex.getMessage(), containsString("conflicts with"));
     }
+
+    @Test
+    void testRemovalOfExistingMetricByName() {
+        Metadata metadata = Metadata.builder()
+                .withName("counter6")
+                .withType(MetricType.COUNTER)
+                .build();
+        registry.counter(metadata);
+        registry.counter(metadata, tag1);
+        // Removing by name should remove all metrics with that name, regardless of tags.
+        boolean result = registry.remove(metadata.getName());
+        assertThat(result, is(true));
+        Map<MetricID, Counter> counters = registry.getCounters(new MetricNameFilter(metadata.getName()));
+        assertThat(counters.size(), is(0));
+    }
+
+    @Test
+    void testRemovalOfExistingMetricByMetricID() {
+        Metadata metadata = Metadata.builder()
+                .withName("counter7")
+                .withType(MetricType.COUNTER)
+                .build();
+
+        registry.counter(metadata);
+        registry.counter(metadata, tag1);
+        MetricID metricID = new MetricID(metadata.getName(), tag1);
+        // Removing by MetricID should leave other like-named metrics intact.
+        boolean result = registry.remove(metricID);
+        assertThat(result, is(true));
+        Map<MetricID, Counter> counters = registry.getCounters(new MetricNameFilter(metadata.getName()));
+        assertThat(counters.size(), is(1));
+    }
+
+    @Test
+    void testRemovalOfMissingMetricByName() {
+        boolean result = registry.remove("NOT_THERE");
+        assertThat(result, is(false));
+    }
+
+    @Test
+    void testRemovalOfMissingMetricByID() {
+        MetricID metricID = new MetricID("NOT_THERE");
+        boolean result = registry.remove(metricID);
+        assertThat(result, is(false));
+    }
+
+    private static class MetricNameFilter implements MetricFilter {
+
+        private final String name;
+
+        private MetricNameFilter(String name) {
+            this.name = name;
+        }
+
+        @Override
+        public boolean matches(MetricID metricID, Metric metric) {
+            return metricID.getName().equals(name);
+        }
+    }
 }