Fixes a few problems in metrics (#2240)

* Add non-public method to retrieve MetricID/HelidonMetric pairs given a name

* Make sure TYPE is emitted to Prometheus/OpenMetrics output only when requested

* Fix behavior of /metrics/registryName/metricName to report all matching metrics

Signed-off-by: tim.quinn@oracle.com <tim.quinn@oracle.com>

metrics/metrics/src/main/java/io/helidon/metrics/HelidonConcurrentGauge.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2020 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2018, 2020 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -100,11 +100,15 @@ public void prometheusData(StringBuilder sb, MetricID metricID, boolean withHelp
         sb.append(nameCurrent).append(prometheusTags(metricID.getTags()))
                 .append(" ").append(prometheusValue()).append('\n');
         final String nameMin = name + "_min";
-        prometheusType(sb, nameMin, metadata().getType());
+        if (withHelpType) {
+            prometheusType(sb, nameMin, metadata().getType());
+        }
         sb.append(nameMin).append(prometheusTags(metricID.getTags()))
                 .append(" ").append(getMin()).append('\n');
         final String nameMax = name + "_max";
-        prometheusType(sb, nameMax, metadata().getType());
+        if (withHelpType) {
+            prometheusType(sb, nameMax, metadata().getType());
+        }
         sb.append(nameMax).append(prometheusTags(metricID.getTags()))
                 .append(" ").append(getMax()).append('\n');
     }

metrics/metrics/src/main/java/io/helidon/metrics/HelidonMeter.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2020 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2018, 2020 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -93,31 +93,39 @@ public void prometheusData(StringBuilder sb, MetricID metricID, boolean withHelp
                 .append("\n");
 
         nameUnits = prometheusNameWithUnits(name, Optional.empty()) + "_rate_per_second";
-        prometheusType(sb, nameUnits, "gauge");
+        if (withHelpType) {
+            prometheusType(sb, nameUnits, "gauge");
+        }
         sb.append(nameUnits)
                 .append(tags)
                 .append(" ")
                 .append(getMeanRate())
                 .append("\n");
 
         nameUnits = prometheusNameWithUnits(name, Optional.empty()) + "_one_min_rate_per_second";
-        prometheusType(sb, nameUnits, "gauge");
+        if (withHelpType) {
+            prometheusType(sb, nameUnits, "gauge");
+        }
         sb.append(nameUnits)
                 .append(tags)
                 .append(" ")
                 .append(getOneMinuteRate())
                 .append("\n");
 
         nameUnits = prometheusNameWithUnits(name, Optional.empty()) + "_five_min_rate_per_second";
-        prometheusType(sb, nameUnits, "gauge");
+        if (withHelpType) {
+            prometheusType(sb, nameUnits, "gauge");
+        }
         sb.append(nameUnits)
                 .append(tags)
                 .append(" ")
                 .append(getFiveMinuteRate())
                 .append("\n");
 
         nameUnits = prometheusNameWithUnits(name, Optional.empty()) + "_fifteen_min_rate_per_second";
-        prometheusType(sb, nameUnits, "gauge");
+        if (withHelpType) {
+            prometheusType(sb, nameUnits, "gauge");
+        }
         sb.append(nameUnits)
                 .append(tags)
                 .append(" ")

metrics/metrics/src/main/java/io/helidon/metrics/MetricsSupport.java

@@ -397,7 +397,7 @@ public void configureEndpoint(Routing.Rules rules) {
                     String type = registry.type();
 
                     rules.get(context + "/" + type, (req, res) -> getAll(req, res, registry))
-                            .get(context + "/" + type + "/{metric}", (req, res) -> getOne(req, res, registry))
+                            .get(context + "/" + type + "/{metric}", (req, res) -> getByName(req, res, registry))
                             .options(context + "/" + type, (req, res) -> optionsAll(req, res, registry))
                             .options(context + "/" + type + "/{metric}", (req, res) -> optionsOne(req, res, registry));
                 });
@@ -421,20 +421,16 @@ public void update(Routing.Rules rules) {
         configureEndpoint(rules);
     }
 
-    private void getOne(ServerRequest req, ServerResponse res, Registry registry) {
+    private void getByName(ServerRequest req, ServerResponse res, Registry registry) {
         String metricName = req.path().param("metric");
 
         registry.getOptionalMetricEntry(metricName)
                 .ifPresentOrElse(entry -> {
                     MediaType mediaType = findBestAccepted(req.headers());
                     if (mediaType == MediaType.APPLICATION_JSON) {
-                        JsonObjectBuilder builder = JSON.createObjectBuilder();
-                        entry.getValue().jsonData(builder, entry.getKey());
-                        sendJson(res, builder.build());
+                        sendJson(res, jsonDataByName(registry, metricName));
                     } else if (mediaType == MediaType.TEXT_PLAIN) {
-                        final StringBuilder sb = new StringBuilder();
-                        entry.getValue().prometheusData(sb, entry.getKey(), true);
-                        res.send(sb.toString());
+                        res.send(prometheusDataByName(registry, metricName));
                     } else {
                         res.status(Http.Status.NOT_ACCEPTABLE_406);
                         res.send();
@@ -445,6 +441,26 @@ private void getOne(ServerRequest req, ServerResponse res, Registry registry) {
                 });
     }
 
+    static JsonObject jsonDataByName(Registry registry, String metricName) {
+        JsonObjectBuilder builder = new MetricsSupport.MergingJsonObjectBuilder(JSON.createObjectBuilder());
+        for (Map.Entry<MetricID, HelidonMetric> metricEntry : registry.getMetricsByName(metricName)) {
+            metricEntry.getValue()
+                    .jsonData(builder, metricEntry.getKey());
+        }
+        return builder.build();
+    }
+
+    static String prometheusDataByName(Registry registry, String metricName) {
+        final StringBuilder sb = new StringBuilder();
+        boolean isFirst = true;
+        for (Map.Entry<MetricID, HelidonMetric> metricEntry : registry.getMetricsByName(metricName)) {
+            metricEntry.getValue()
+                    .prometheusData(sb, metricEntry.getKey(), isFirst);
+            isFirst = false;
+        }
+        return sb.toString();
+    }
+
     private static void sendJson(ServerResponse res, JsonObject object) {
         res.send(JSONP_WRITER.marshall(object));
     }

metrics/metrics/src/main/java/io/helidon/metrics/Registry.java

@@ -387,6 +387,18 @@ <T extends HelidonMetric> Optional<T> getOptionalMetric(String metricName, Class
         return getOptionalMetric(new MetricID(metricName, tags), clazz);
     }
 
+    List<Map.Entry<MetricID, HelidonMetric>> getMetricsByName(String metricName) {
+        List<MetricID> metricIDs = allMetricIDsByName.get(metricName);
+        if (metricIDs == null) {
+            return Collections.EMPTY_LIST;
+        }
+        List<Map.Entry<MetricID, HelidonMetric>> result = new ArrayList<>();
+        for (MetricID metricID : metricIDs) {
+            result.add(new AbstractMap.SimpleEntry<>(metricID, allMetrics.get(metricID)));
+        }
+        return result;
+    }
+
     /**
      * Get internal map entry given a metric name. Synchronized for atomic access of more than
      * one internal map.

metrics/metrics/src/test/java/io/helidon/metrics/MetricsSupportTest.java

@@ -27,12 +27,14 @@
 import javax.json.Json;
 import javax.json.JsonArray;
 import javax.json.JsonBuilderFactory;
+import javax.json.JsonNumber;
 import javax.json.JsonObject;
 import javax.json.JsonObjectBuilder;
 
 import io.helidon.config.Config;
 import io.helidon.config.ConfigSources;
 
+import org.eclipse.microprofile.metrics.ConcurrentGauge;
 import org.eclipse.microprofile.metrics.Counter;
 import org.eclipse.microprofile.metrics.MetricID;
 import org.eclipse.microprofile.metrics.MetricRegistry;
@@ -42,6 +44,7 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 /**
@@ -57,6 +60,12 @@ class MetricsSupportTest {
 
     private static final MetricID METRIC_USED_HEAP = new MetricID("memory.usedHeap");
 
+    private static final String CONCURRENT_GAUGE_NAME = "appConcurrentGauge";
+    private static final int RED_CONCURRENT_GAUGE_COUNT = 1;
+    private static final int BLUE_CONCURRENT_GAUGE_COUNT = 2;
+
+    private static String globalTagsJsonSuffix;
+
     @BeforeAll
     static void initClass() {
         RegistryFactory rf = RegistryFactory.getInstance();
@@ -67,6 +76,23 @@ static void initClass() {
         Counter counter = app.counter("appCounter",
                 new Tag("color", "blue"), new Tag("brightness", "dim"));
         counter.inc();
+
+        ConcurrentGauge concurrentGauge = app.concurrentGauge(CONCURRENT_GAUGE_NAME, new Tag("color", "blue"));
+        for (int i = 0; i < BLUE_CONCURRENT_GAUGE_COUNT; i++) {
+            concurrentGauge.inc();
+        }
+
+        concurrentGauge = app.concurrentGauge(CONCURRENT_GAUGE_NAME, new Tag("color", "red"));
+        for (int i = 0; i < RED_CONCURRENT_GAUGE_COUNT; i++) {
+            concurrentGauge.inc();
+        }
+
+        String globalTags = System.getenv("MP_METRICS_TAGS");
+        if (globalTags == null) {
+            globalTagsJsonSuffix = "";
+        } else {
+            globalTagsJsonSuffix = ";" + globalTags.replaceAll(",", ";");
+        }
     }
 
     @Test
@@ -171,4 +197,19 @@ void testPrometheusDataNoTypeDups() throws Exception {
             }
         }
     }
+
+    @Test
+    void testJsonDataMultipleMetricsSameName() {
+        // Make sure the JSON format for all metrics matching a name lists the name once with tagged instances as children.
+        JsonObject multiple = MetricsSupport.jsonDataByName(app, CONCURRENT_GAUGE_NAME);
+        assertNotNull(multiple);
+        JsonObject top = multiple.getJsonObject(CONCURRENT_GAUGE_NAME);
+        assertNotNull(top);
+        JsonNumber blueNumber = top.getJsonNumber("current;color=blue" + globalTagsJsonSuffix);
+        assertNotNull(blueNumber);
+        assertEquals(BLUE_CONCURRENT_GAUGE_COUNT, blueNumber.longValue());
+        JsonNumber redNumber = top.getJsonNumber("current;color=red" + globalTagsJsonSuffix);
+        assertNotNull(redNumber);
+        assertEquals(RED_CONCURRENT_GAUGE_COUNT, redNumber.longValue());
+    }
 }