From fc0dad39a90cd58278b6399aa64658ed69344ae6 Mon Sep 17 00:00:00 2001
From: "tim.quinn@oracle.com" <tim.quinn@oracle.com>
Date: Tue, 29 Nov 2022 05:19:49 -0600
Subject: [PATCH] Fix incorrect tags comparison when trying to match metric IDs

---
 .../io/helidon/metrics/api/MetricStore.java   | 10 ++-
 .../helidon/metrics/api/MetricStoreTests.java | 70 +++++++++++++++++++
 2 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/metrics/api/src/main/java/io/helidon/metrics/api/MetricStore.java b/metrics/api/src/main/java/io/helidon/metrics/api/MetricStore.java
index 73443a39725..4535679ab39 100644
--- a/metrics/api/src/main/java/io/helidon/metrics/api/MetricStore.java
+++ b/metrics/api/src/main/java/io/helidon/metrics/api/MetricStore.java
@@ -408,7 +408,7 @@ private HelidonMetric getMetricLocked(String metricName, Tag... tags) {
             return null;
         }
         for (MetricID metricID : metricIDsForName) {
-            if (metricID.getName().equals(metricName) && Arrays.equals(metricID.getTagsAsArray(), tags)) {
+            if (metricID.getName().equals(metricName) && tagsMatch(tags, metricID.getTags())) {
                 return allMetrics.get(metricID);
             }
         }
@@ -531,6 +531,14 @@ private <S> S access(Lock lock, Callable<S> action) {
         }
     }
 
+    private static boolean tagsMatch(Tag[] tags, Map<String, String> tagMap) {
+        Map<String, String> newTags = new TreeMap<>();
+        for (Tag tag : tags) {
+            newTags.put(tag.getTagName(), tag.getTagValue());
+        }
+        return newTags.equals(tagMap);
+    }
+
     private static void enforceConsistentMetadata(Metadata existingMetadata, Metadata newMetadata) {
         if (!metadataMatches(existingMetadata, newMetadata)) {
             throw new IllegalArgumentException("New metadata conflicts with existing metadata with the same name; existing: "
diff --git a/metrics/api/src/test/java/io/helidon/metrics/api/MetricStoreTests.java b/metrics/api/src/test/java/io/helidon/metrics/api/MetricStoreTests.java
index 23670c17fb2..94f1618e2a8 100644
--- a/metrics/api/src/test/java/io/helidon/metrics/api/MetricStoreTests.java
+++ b/metrics/api/src/test/java/io/helidon/metrics/api/MetricStoreTests.java
@@ -23,6 +23,10 @@
 import org.eclipse.microprofile.metrics.Tag;
 import org.junit.jupiter.api.Test;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
+
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 class MetricStoreTests {
@@ -56,4 +60,70 @@ void testConflictingMetadata() {
         assertThrows(IllegalArgumentException.class, () ->
                 store.getOrRegisterMetric(meta2, Counter.class, NO_TAGS));
     }
+
+    @Test
+    void testSameNameNoTags() {
+        Metadata metadata = Metadata.builder()
+                .withName("a")
+                .withType(MetricType.COUNTER)
+                .build();
+
+        NoOpMetricRegistry registry = NoOpMetricRegistry.create(MetricRegistry.Type.APPLICATION);
+
+        MetricStore store = MetricStore.create(REGISTRY_SETTINGS,
+                                               NoOpMetricRegistry.NO_OP_METRIC_FACTORIES,
+                                               null,
+                                               null,
+                                               MetricRegistry.Type.APPLICATION,
+                                               registry::toImpl);
+
+        Counter counter1 = store.getOrRegisterMetric(metadata, Counter.class, NO_TAGS);
+        Counter counter2 = store.getOrRegisterMetric(metadata, Counter.class, NO_TAGS);
+        assertThat("Counters with no tags", counter1, is(counter2));
+    }
+
+    @Test
+    void testSameNameSameTwoTags() {
+        Tag[] tags = {new Tag("foo", "1"), new Tag("bar", "1")};
+        Metadata metadata = Metadata.builder()
+                .withName("a")
+                .withType(MetricType.COUNTER)
+                .build();
+
+        NoOpMetricRegistry registry = NoOpMetricRegistry.create(MetricRegistry.Type.APPLICATION);
+
+        MetricStore store = MetricStore.create(REGISTRY_SETTINGS,
+                                               NoOpMetricRegistry.NO_OP_METRIC_FACTORIES,
+                                               null,
+                                               null,
+                                               MetricRegistry.Type.APPLICATION,
+                                               registry::toImpl);
+
+        Counter counter1 = store.getOrRegisterMetric(metadata, Counter.class, tags);
+        Counter counter2 = store.getOrRegisterMetric(metadata, Counter.class, tags);
+        assertThat("Counters with same two tags", counter1, is(counter2));
+    }
+
+    @Test
+    void testSameNameOverlappingButDifferentTags() {
+        Tag[] tags1 = {new Tag("foo", "1"), new Tag("bar", "1"), new Tag("baz", "1")};
+        Tag[] tags2 = {new Tag("foo", "1"), new Tag("bar", "1")};
+        Metadata metadata = Metadata.builder()
+                .withName("a")
+                .withType(MetricType.COUNTER)
+                .build();
+
+        NoOpMetricRegistry registry = NoOpMetricRegistry.create(MetricRegistry.Type.APPLICATION);
+
+        MetricStore store = MetricStore.create(REGISTRY_SETTINGS,
+                                               NoOpMetricRegistry.NO_OP_METRIC_FACTORIES,
+                                               null,
+                                               null,
+                                               MetricRegistry.Type.APPLICATION,
+                                               registry::toImpl);
+
+        Counter counter1 = store.getOrRegisterMetric(metadata, Counter.class, tags1);
+        Counter counter2 = store.getOrRegisterMetric(metadata, Counter.class, tags2);
+        assertThat("Counters with overlapping but different tags", counter1, not(is(counter2)));
+    }
 }