Move clean-out of registries from afterStart to beforeStop; some othe…

…r clean-up

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

@@ -17,10 +17,8 @@
 package io.helidon.metrics;
 
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.Map;
 import java.util.Optional;
-import java.util.Set;
 import java.util.concurrent.Callable;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
@@ -60,7 +58,7 @@ public class RegistryFactory implements io.helidon.metrics.api.RegistryFactory {
      * @param appRegistry application registry to provide from the factory
      * @param vendorRegistry vendor registry to provide from the factory
      */
-    protected RegistryFactory(MetricsSettings metricsSettings, Registry appRegistry, Registry vendorRegistry) {
+    private RegistryFactory(MetricsSettings metricsSettings, Registry appRegistry, Registry vendorRegistry) {
         this.metricsSettings = metricsSettings;
         prometheusConfig = new HelidonPrometheusConfig(metricsSettings);
         metricFactory = HelidonMicrometerMetricFactory.create(Metrics.globalRegistry);
@@ -74,27 +72,6 @@ private RegistryFactory(MetricsSettings metricsSettings) {
              Registry.create(Registry.VENDOR_SCOPE, metricsSettings.registrySettings(Registry.VENDOR_SCOPE)));
     }
 
-    private void accessMetricsSettings(Runnable operation) {
-        metricsSettingsAccess.lock();
-        try {
-            operation.run();
-        } finally {
-            metricsSettingsAccess.unlock();
-        }
-    }
-
-    private <T> T accessMetricsSettings(Callable<T> callable) {
-        metricsSettingsAccess.lock();
-        try {
-            return callable.call();
-        } catch (Exception e) {
-            throw new RuntimeException(e);
-        } finally {
-            metricsSettingsAccess.unlock();
-        }
-    }
-
-
     /**
      * Create a new factory with default configuration, with pre-filled
      * {@link org.eclipse.microprofile.metrics.MetricRegistry.Type#VENDOR} and
@@ -210,23 +187,20 @@ public Optional<?> scrape(MediaType mediaType,
 
     @Override
     public Iterable<String> scopes() {
-
-        /*
-        If a caller invokes this method before we have created a base registry on-demand,
-        then we should artificially include "base" in the scopes. If the caller then asks for the
-        base registry, the getRegistry method will create it on demand.
-         */
-        if (!registries.containsKey(Registry.BASE_SCOPE)
-                && metricsSettings.baseMetricsSettings().isEnabled()) {
-            Set<String> augmentedScopes = new HashSet(registries.keySet());
-            augmentedScopes.add(Registry.BASE_SCOPE);
-            return augmentedScopes;
+        if (!registries.containsKey(Registry.BASE_SCOPE)) {
+            accessMetricsSettings(() -> registries.computeIfAbsent(Registry.BASE_SCOPE,
+                                                                   key -> BaseRegistry.create(metricsSettings)));
         }
         return registries.keySet();
     }
 
     @Override
     public void start() {
+        PeriodicExecutor.start();
+    }
+
+    @Override
+    public void stop() {
         /*
             Primarily for successive tests (e.g., in the TCK) which might share the same VM, delete each metric individually
             (which will trickle down into the delegate meter registry) and also clear out the collection of registries.
@@ -235,12 +209,27 @@ Primarily for successive tests (e.g., in the TCK) which might share the same VM,
                 .forEach(r -> r.getMetrics()
                         .forEach((id, m) -> r.remove(id)));
         registries.clear();
-        PeriodicExecutor.start();
+        PeriodicExecutor.stop();
     }
 
-    @Override
-    public void stop() {
-        PeriodicExecutor.stop();
+    private void accessMetricsSettings(Runnable operation) {
+        metricsSettingsAccess.lock();
+        try {
+            operation.run();
+        } finally {
+            metricsSettingsAccess.unlock();
+        }
+    }
+
+    private <T> T accessMetricsSettings(Callable<T> callable) {
+        metricsSettingsAccess.lock();
+        try {
+            return callable.call();
+        } catch (Exception e) {
+            throw new RuntimeException(e);
+        } finally {
+            metricsSettingsAccess.unlock();
+        }
     }
 }
 

nima/observe/metrics/src/main/java/io/helidon/nima/observe/metrics/MetricsFeature.java

@@ -181,10 +181,13 @@ protected void postSetup(HttpRouting.Builder defaultRouting, HttpRouting.Builder
     }
 
     private void getAll(ServerRequest req, ServerResponse res) {
-        getAll(req, res, req.query().all("scope", List::of), req.query().all("name", List::of));
+        getMatching(req, res, req.query().all("scope", List::of), req.query().all("name", List::of));
     }
 
-    private void getAll(ServerRequest req, ServerResponse res, Iterable<String> scopeSelection, Iterable<String> nameSelection) {
+    private void getMatching(ServerRequest req,
+                             ServerResponse res,
+                             Iterable<String> scopeSelection,
+                             Iterable<String> nameSelection) {
         MediaType mediaType = bestAccepted(req);
         res.header(Http.HeaderValues.CACHE_NO_CACHE);
         if (mediaType == null) {
@@ -226,10 +229,6 @@ private static KeyPerformanceIndicatorSupport.Context kpiContext(ServerRequest r
     }
 
     private void setUpEndpoints(HttpRules rules) {
-        Registry base = registryFactory.getRegistry(Registry.BASE_SCOPE);
-        Registry vendor = registryFactory.getRegistry(Registry.VENDOR_SCOPE);
-        Registry app = registryFactory.getRegistry(Registry.APPLICATION_SCOPE);
-
         // routing to root of metrics
         // As of Helidon 4, this is the only path we should need because scope-based or metric-name-based
         // selection should use query parameters instead of paths.
@@ -240,11 +239,15 @@ private void setUpEndpoints(HttpRules rules) {
         // As of Helidon 4, users should use /metrics?scope=xyz instead of /metrics/xyz, and
         // /metrics/?scope=xyz&name=abc instead of /metrics/xyz/abc. These routings are kept
         // temporarily for backward compatibility.
-        Stream.of(app, base, vendor)
+
+        Stream.of(Registry.APPLICATION_SCOPE,
+                  Registry.BASE_SCOPE,
+                  Registry.VENDOR_SCOPE)
+                .map(registryFactory::getRegistry)
                 .forEach(registry -> {
                     String type = registry.scope();
 
-                    rules.get("/" + type, (req, res) -> getAll(req, res, Set.of(type), Set.of()))
+                    rules.get("/" + type, (req, res) -> getMatching(req, res, Set.of(type), Set.of()))
                             .get("/" + type + "/{metric}", (req, res) -> getByName(req, res, Set.of(type))) // should use ?scope=
                             .options("/" + type, this::rejectOptions)
                             .options("/" + type + "/{metric}", this::rejectOptions);
@@ -253,7 +256,7 @@ private void setUpEndpoints(HttpRules rules) {
 
     private void getByName(ServerRequest req, ServerResponse res, Iterable<String> scopeSelection) {
         String metricName = req.path().pathParameters().value("metric");
-        getAll(req, res, scopeSelection, Set.of(metricName));
+        getMatching(req, res, scopeSelection, Set.of(metricName));
     }
 
     private void postRequestProcessing(PostRequestMetricsSupport prms,

nima/observe/metrics/src/main/java/module-info.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021, 2022 Oracle and/or its affiliates.
+ * Copyright (c) 2021, 2023 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.