Merge pull request #9412 from vespa-engine/gjoranv/new-metrics-proxy-fixes

Gjoranv/new metrics proxy fixes 

6 files changed, 93 insertions, 74 deletions

diff --git a/config-model/src/main/java/com/yahoo/config/model/producer/AbstractConfigProducer.java b/config-model/src/main/java/com/yahoo/config/model/producer/AbstractConfigProducer.java
index 65d94807d7f..c2834847423 100644
--- a/config-model/src/main/java/com/yahoo/config/model/producer/AbstractConfigProducer.java
+++ b/config-model/src/main/java/com/yahoo/config/model/producer/AbstractConfigProducer.java
@@ -406,7 +406,7 @@ public abstract class AbstractConfigProducer<CHILD extends AbstractConfigProduce
         return findInheritedClassLoader(clazz.getSuperclass(), producerName);
     }
 
-    public ClassLoader getConfigClassLoader(String producerName) {
+    protected ClassLoader getConfigClassLoader(String producerName) {
         ClassLoader classLoader = findInheritedClassLoader(getClass(), producerName);
         if (classLoader != null)
             return classLoader;
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/InstanceResolver.java b/config-model/src/main/java/com/yahoo/vespa/model/InstanceResolver.java
index bb817fcc7bc..f90b399d305 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/InstanceResolver.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/InstanceResolver.java
@@ -8,7 +8,6 @@ import com.yahoo.config.codegen.CNode;
 import com.yahoo.config.codegen.InnerCNode;
 import com.yahoo.config.codegen.LeafCNode;
 import com.yahoo.vespa.config.ConfigDefinitionKey;
-import com.yahoo.vespa.config.ConfigKey;
 import com.yahoo.yolean.Exceptions;
 
 import java.lang.reflect.Field;
@@ -16,7 +15,6 @@ import java.lang.reflect.Method;
 import java.util.List;
 import java.util.Map;
 
-import static com.yahoo.config.codegen.ConfiggenUtil.createClassName;
 
 /**
  * <p>
@@ -42,16 +40,14 @@ class InstanceResolver {
      * Resolves this config key into a correctly typed ConfigInstance using the given config builder.
      * FIXME: Make private once config overrides are deprecated.?
      *
-     * @param key   a ConfigKey
      * @param builder a ConfigBuilder to create the instance from.
      * @param targetDef the def to use
      * @return the config instance or null of no producer for this found in model
      */
-    static ConfigInstance resolveToInstance(ConfigKey<?> key, ConfigBuilder builder, InnerCNode targetDef) {
-        ConfigDefinitionKey defKey = new ConfigDefinitionKey(key);
+    static ConfigInstance resolveToInstance(ConfigInstance.Builder builder, InnerCNode targetDef) {
         try {
             if (targetDef != null) applyDef(builder, targetDef);
-            Class<? extends ConfigInstance> clazz = getConfigClass(defKey, builder.getClass().getClassLoader());
+            Class<? extends ConfigInstance> clazz = getConfigClass(builder.getClass());
             return clazz.getConstructor(builder.getClass()).newInstance(builder);
         } catch (Exception e) {
             throw new ConfigurationRuntimeException(e);
@@ -59,20 +55,6 @@ class InstanceResolver {
     }
 
     /**
-     * Resolves this config key into a correctly typed ConfigBuilder using the given config model.
-     * FIXME: Make private once config overrides are deprecated.?
-     *
-     * @return the config builder or null if no producer for this found in model
-     */
-    static ConfigBuilder resolveToBuilder(ConfigKey<?> key, VespaModel model) {
-        if (model == null) return null;
-        ConfigDefinitionKey defKey = new ConfigDefinitionKey(key);
-        ConfigInstance.Builder builder = model.createBuilder(defKey);
-        model.getConfig(builder, key.getConfigId());
-        return builder;
-    }
-
-    /**
      * If some fields on the builder are null now, set them from the def. Do recursively.
      * <p>
      * If the targetDef has some schema incompatibilities, they are not handled here
@@ -143,31 +125,28 @@ class InstanceResolver {
         }
     }
 
-    /**
-     * Returns a {@link ConfigInstance} of right type for given key using reflection
-     *
-     * @param  cKey a ConfigKey
-     * @return a {@link ConfigInstance} or null if not available in classpath
-     */
     @SuppressWarnings("unchecked")
-    private static Class<? extends ConfigInstance> getConfigClass(ConfigDefinitionKey cKey, ClassLoader instanceLoader) {
-        String className = createClassName(cKey.getName());
-        String fullClassName = packageName(cKey) + "." + className;
-        Class<?> clazz;
-        try {
-            clazz = instanceLoader != null ? instanceLoader.loadClass(fullClassName) : Class.forName(fullClassName);
-        } catch (ClassNotFoundException e) {
-            throw new ConfigurationRuntimeException("Could not find config class for key " + cKey, e);
+    private static Class<? extends ConfigInstance> getConfigClass(Class<? extends ConfigInstance.Builder> builderClass) {
+        Class<?> configClass = builderClass.getEnclosingClass();
+        if (configClass == null || ! ConfigInstance.class.isAssignableFrom(configClass)) {
+            throw new ConfigurationRuntimeException("Builder class " + builderClass + " has enclosing class " + configClass + ", which is not a ConfigInstance");
         }
-        if (! ConfigInstance.class.isAssignableFrom(clazz)) {
-            throw new ConfigurationRuntimeException(fullClassName + " is not a ConfigInstance subclass, can not produce config for " + cKey);
-        }
-        return (Class<? extends ConfigInstance>) clazz;
+        return (Class<? extends ConfigInstance>) configClass;
     }
 
-    static String packageName(ConfigDefinitionKey cKey) {
-        String prefix = "com.yahoo.";
+    static String packageName(ConfigDefinitionKey cKey, PackagePrefix packagePrefix) {
+        String prefix = packagePrefix.value;
         return prefix + (cKey.getNamespace().isEmpty() ? CNode.DEFAULT_NAMESPACE : cKey.getNamespace());
     }
 
+
+    enum PackagePrefix {
+        COM_YAHOO("com.yahoo."),
+        NONE("");
+
+        final String value;
+        PackagePrefix (String value) {
+            this.value = value;
+        }
+    }
 }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/VespaModel.java b/config-model/src/main/java/com/yahoo/vespa/model/VespaModel.java
index 12bf14c2242..4a187c9b9bf 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/VespaModel.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/VespaModel.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.model;
 
 import ai.vespa.rankingexpression.importer.configmodelview.ImportedMlModel;
+import com.yahoo.collections.Pair;
 import com.yahoo.config.ConfigBuilder;
 import com.yahoo.config.ConfigInstance;
 import com.yahoo.config.ConfigInstance.Builder;
@@ -33,6 +34,7 @@ import com.yahoo.searchdefinition.RankingConstants;
 import com.yahoo.searchdefinition.derived.AttributeFields;
 import com.yahoo.searchdefinition.derived.RankProfileList;
 import com.yahoo.searchdefinition.processing.Processing;
+import com.yahoo.vespa.model.InstanceResolver.PackagePrefix;
 import com.yahoo.vespa.model.container.ApplicationContainerCluster;
 import com.yahoo.vespa.model.container.search.QueryProfiles;
 import com.yahoo.vespa.model.ml.ConvertedModel;
@@ -357,7 +359,7 @@ public final class VespaModel extends AbstractConfigProducerRoot implements Seri
     public ConfigInstance.Builder getConfig(ConfigInstance.Builder builder, String configId) {
         checkId(configId);
         Optional<ConfigProducer> configProducer = getConfigProducer(configId);
-        if ( ! configProducer.isPresent()) return null;
+        if (configProducer.isEmpty()) return null;
 
         populateConfigBuilder(builder, configProducer.get());
         return builder;
@@ -381,7 +383,8 @@ public final class VespaModel extends AbstractConfigProducerRoot implements Seri
      */
     @Override
     public ConfigPayload getConfig(ConfigKey configKey, com.yahoo.vespa.config.buildergen.ConfigDefinition targetDef) {
-        ConfigBuilder builder = InstanceResolver.resolveToBuilder(configKey, this);
+        ConfigInstance.Builder builder = resolveToBuilder(configKey);
+        // TODO: remove if-statement, the builder can never be null.
         if (builder != null) {
             log.log(LogLevel.DEBUG, () -> "Found builder for " + configKey);
             ConfigPayload payload;
@@ -389,24 +392,37 @@ public final class VespaModel extends AbstractConfigProducerRoot implements Seri
             if (builder instanceof GenericConfig.GenericConfigBuilder) {
                 payload = getConfigFromGenericBuilder(builder);
             } else {
-                payload = getConfigFromBuilder(configKey, builder, innerCNode);
+                payload = getConfigFromBuilder(builder, innerCNode);
             }
             return (innerCNode != null) ? payload.applyDefaultsFromDef(innerCNode) : payload;
         }
         return null;
     }
 
-    private ConfigPayload getConfigFromBuilder(ConfigKey configKey, ConfigBuilder builder, InnerCNode targetDef) {
+    /**
+     * Resolves the given config key into a correctly typed ConfigBuilder
+     * and fills in the config from this model.
+     *
+     * @return A new config builder with config from this model filled in,.
+     */
+    private ConfigInstance.Builder resolveToBuilder(ConfigKey<?> key) {
+        ConfigDefinitionKey defKey = new ConfigDefinitionKey(key);
+        ConfigInstance.Builder builder = createBuilder(defKey);
+        getConfig(builder, key.getConfigId());
+        return builder;
+    }
+
+    private ConfigPayload getConfigFromBuilder(ConfigInstance.Builder builder, InnerCNode targetDef) {
         try {
-            ConfigInstance instance = InstanceResolver.resolveToInstance(configKey, builder, targetDef);
-            log.log(LogLevel.DEBUG, () -> "getConfigFromBuilder for " + configKey + ",instance=" + instance);
+            ConfigInstance instance = InstanceResolver.resolveToInstance(builder, targetDef);
+            log.log(LogLevel.DEBUG, () -> "getConfigFromBuilder for builder " + builder.getClass().getName() + ", instance=" + instance);
             return ConfigPayload.fromInstance(instance);
         } catch (ConfigurationRuntimeException e) {
             // This can happen in cases where services ask for config that no longer exist before they have been able
             // to reconfigure themselves. This happens for instance whenever jdisc reconfigures itself until
             // ticket 6599572 is fixed. When that happens, consider propagating a full error rather than empty payload
             // back to the client.
-            log.log(LogLevel.INFO, "Error resolving instance for key '" + configKey + "', returning empty config: " + Exceptions.toMessageString(e));
+            log.log(LogLevel.INFO, "Error resolving instance for builder '" + builder.getClass().getName() + "', returning empty config: " + Exceptions.toMessageString(e));
             return ConfigPayload.fromBuilder(new ConfigPayloadBuilder());
         }
     }
@@ -424,14 +440,15 @@ public final class VespaModel extends AbstractConfigProducerRoot implements Seri
         return keySet;
     }
 
-    public ConfigInstance.Builder createBuilder(ConfigDefinitionKey key) {
+    private ConfigInstance.Builder createBuilder(ConfigDefinitionKey key) {
         String className = createClassName(key.getName());
         Class<?> clazz;
 
-        final String fullClassName = InstanceResolver.packageName(key) + "." + className;
+        Pair<String, ClassLoader> fullClassNameAndLoader = getClassLoaderForProducer(key, className);
+        String fullClassName = fullClassNameAndLoader.getFirst();
+        ClassLoader classLoader = fullClassNameAndLoader.getSecond();
+
         final String builderName = fullClassName + "$Builder";
-        final String producerName = fullClassName + "$Producer";
-        ClassLoader classLoader = getConfigClassLoader(producerName);
         if (classLoader == null) {
             classLoader = getClass().getClassLoader();
             log.log(LogLevel.DEBUG, () -> "No producer found to get classloader from for " + fullClassName + ". Using default");
@@ -458,6 +475,24 @@ public final class VespaModel extends AbstractConfigProducerRoot implements Seri
     }
 
     /**
+     * Takes a candidate class name and tries to find a config producer for that class in the model. First, an
+     * attempt is made with the given full class name, and if unsuccessful, then with 'com.yahoo.' prefixed to it.
+     * Returns the full class name and the class loader that a producer was found for. If no producer was found,
+     * returns the full class name with prefix, and null for the class loader.
+     */
+    private Pair<String, ClassLoader> getClassLoaderForProducer(ConfigDefinitionKey key, String shortClassName) {
+        String fullClassNameWithComYahoo = InstanceResolver.packageName(key, PackagePrefix.COM_YAHOO) + "." + shortClassName;
+        String fullClassNameWithoutPrefix = InstanceResolver.packageName(key, PackagePrefix.NONE) + "." + shortClassName;
+        String producerSuffix = "$Producer";
+
+        ClassLoader loader = getConfigClassLoader(fullClassNameWithoutPrefix + producerSuffix);
+        if (loader != null) return new Pair<>(fullClassNameWithoutPrefix, loader);
+
+        return new Pair<>(fullClassNameWithComYahoo,
+                          getConfigClassLoader(fullClassNameWithComYahoo + producerSuffix));
+    }
+
+    /**
      * The set of all config ids present
      * @return set of config ids
      */
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainerCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainerCluster.java
index 3f60079b86e..1d195b3bc0a 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainerCluster.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainerCluster.java
@@ -29,6 +29,7 @@ import com.yahoo.vespa.model.admin.monitoring.builder.Metrics;
 import com.yahoo.vespa.model.container.ContainerCluster;
 
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.Map;
@@ -46,7 +47,7 @@ import static com.yahoo.vespa.model.admin.metricsproxy.MetricsProxyContainerClus
 import static com.yahoo.vespa.model.admin.monitoring.DefaultMetricsConsumer.getDefaultMetricsConsumer;
 import static com.yahoo.vespa.model.admin.monitoring.MetricSet.emptyMetricSet;
 import static com.yahoo.vespa.model.container.xml.BundleMapper.JarSuffix.JAR_WITH_DEPS;
-import static com.yahoo.vespa.model.container.xml.BundleMapper.bundlePathFromName;
+import static com.yahoo.vespa.model.container.xml.BundleMapper.absoluteBundlePath;
 
 /**
  * Container cluster for metrics proxy containers.
@@ -61,12 +62,9 @@ public class MetricsProxyContainerCluster extends ContainerCluster<MetricsProxyC
     public static final Logger log = Logger.getLogger(MetricsProxyContainerCluster.class.getName());
 
     private static final String METRICS_PROXY_NAME = "metrics-proxy";
-    private static final Path METRICS_PROXY_BUNDLE_FILE = bundlePathFromName(METRICS_PROXY_NAME, JAR_WITH_DEPS);
+    static final Path METRICS_PROXY_BUNDLE_FILE = absoluteBundlePath((Paths.get(METRICS_PROXY_NAME + JAR_WITH_DEPS.suffix)));
     static final String METRICS_PROXY_BUNDLE_NAME = "com.yahoo.vespa." + METRICS_PROXY_NAME;
 
-    static final String DEFAULT_NAME_IN_MONITORING_SYSTEM = "vespa";
-    static final int DEFAULT_MONITORING_INTERVAL = 1;
-
     static final class AppDimensionNames {
         static final String ZONE = "zone";
         static final String APPLICATION_ID = "applicationId";  // tenant.app.instance
@@ -108,8 +106,8 @@ public class MetricsProxyContainerCluster extends ContainerCluster<MetricsProxyC
 
     @Override
     public void getConfig(MonitoringConfig.Builder builder) {
-        builder.systemName(getSystemName())
-                .intervalMinutes(getIntervalMinutes());
+        getSystemName().ifPresent(builder::systemName);
+        getIntervalMinutes().ifPresent(builder::intervalMinutes);
     }
 
     @Override
@@ -149,19 +147,16 @@ public class MetricsProxyContainerCluster extends ContainerCluster<MetricsProxyC
         return Optional.empty();
     }
 
-    private String getSystemName() {
+    private Optional<String> getSystemName() {
         Monitoring monitoring = getMonitoringService();
-        if (monitoring != null && ! monitoring.getClustername().equals(""))
-            return monitoring.getClustername();
-        return DEFAULT_NAME_IN_MONITORING_SYSTEM;
+        return monitoring != null && ! monitoring.getClustername().equals("") ?
+                Optional.of(monitoring.getClustername()) : Optional.empty();
     }
 
-    private int getIntervalMinutes() {
+    private Optional<Integer> getIntervalMinutes() {
         Monitoring monitoring = getMonitoringService();
-        if (monitoring != null && monitoring.getInterval() != null) {
-            return monitoring.getInterval();
-        }
-        return DEFAULT_MONITORING_INTERVAL;
+        return monitoring != null ?
+                Optional.of(monitoring.getInterval()) : Optional.empty();
     }
 
     private void  addMetricsProxyComponent(Class<?> componentClass) {
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/BundleMapper.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/BundleMapper.java
index 4c5c14dad87..d8e678a5cd3 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/BundleMapper.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/BundleMapper.java
@@ -19,7 +19,7 @@ public class BundleMapper {
         JAR_WITH_DEPS("-jar-with-dependencies.jar"),
         DEPLOY("-deploy.jar");
 
-        private final String suffix;
+        public final String suffix;
 
         JarSuffix(String suffix) {
             this.suffix = suffix;
@@ -47,10 +47,6 @@ public class BundleMapper {
         return LIBRARY_PATH.resolve(fileName);
     }
 
-    public static Path bundlePathFromName(String name, JarSuffix suffix) {
-        return Paths.get(Defaults.getDefaults().underVespaHome(LIBRARY_PATH + name + suffix.suffix));
-    }
-
     /**
      * TODO: This is a temporary hack to ensure that users can use our internal components without
      * specifying the bundle in which the components reside. Ideally, this information
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainerClusterTest.java b/config-model/src/test/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainerClusterTest.java
index d89dd9b7564..22f7a10ab05 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainerClusterTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainerClusterTest.java
@@ -7,6 +7,7 @@ package com.yahoo.vespa.model.admin.metricsproxy;
 import ai.vespa.metricsproxy.core.ConsumersConfig;
 import ai.vespa.metricsproxy.metric.dimensions.ApplicationDimensionsConfig;
 import com.yahoo.config.provision.Zone;
+import com.yahoo.container.BundlesConfig;
 import com.yahoo.vespa.model.VespaModel;
 import com.yahoo.vespa.model.admin.metricsproxy.MetricsProxyContainerCluster.AppDimensionNames;
 import com.yahoo.vespa.model.admin.monitoring.Metric;
@@ -15,14 +16,16 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 
+import static com.yahoo.vespa.model.admin.metricsproxy.MetricsProxyContainerCluster.METRICS_PROXY_BUNDLE_FILE;
 import static com.yahoo.vespa.model.admin.metricsproxy.MetricsProxyContainerCluster.zoneString;
+import static com.yahoo.vespa.model.admin.metricsproxy.MetricsProxyModelTester.CLUSTER_CONFIG_ID;
 import static com.yahoo.vespa.model.admin.metricsproxy.MetricsProxyModelTester.MY_APPLICATION;
 import static com.yahoo.vespa.model.admin.metricsproxy.MetricsProxyModelTester.MY_INSTANCE;
 import static com.yahoo.vespa.model.admin.metricsproxy.MetricsProxyModelTester.MY_TENANT;
 import static com.yahoo.vespa.model.admin.metricsproxy.MetricsProxyModelTester.checkMetric;
 import static com.yahoo.vespa.model.admin.metricsproxy.MetricsProxyModelTester.consumersConfigFromModel;
-import static com.yahoo.vespa.model.admin.metricsproxy.MetricsProxyModelTester.getApplicationDimensionsConfig;
 import static com.yahoo.vespa.model.admin.metricsproxy.MetricsProxyModelTester.consumersConfigFromXml;
+import static com.yahoo.vespa.model.admin.metricsproxy.MetricsProxyModelTester.getApplicationDimensionsConfig;
 import static com.yahoo.vespa.model.admin.metricsproxy.MetricsProxyModelTester.getCustomConsumer;
 import static com.yahoo.vespa.model.admin.metricsproxy.MetricsProxyModelTester.getHostedModel;
 import static com.yahoo.vespa.model.admin.metricsproxy.MetricsProxyModelTester.getModel;
@@ -32,7 +35,9 @@ import static com.yahoo.vespa.model.admin.monitoring.NetworkMetrics.networkMetri
 import static com.yahoo.vespa.model.admin.monitoring.SystemMetrics.systemMetricSet;
 import static com.yahoo.vespa.model.admin.monitoring.VespaMetricSet.vespaMetricSet;
 import static java.util.Collections.singleton;
+import static org.hamcrest.CoreMatchers.endsWith;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 
 /**
@@ -49,6 +54,15 @@ public class MetricsProxyContainerClusterTest {
     @Rule
     public ExpectedException thrown = ExpectedException.none();
 
+    @Test
+    public void metrics_proxy_bundle_is_included_in_bundles_config() {
+        VespaModel model = getModel(servicesWithAdminOnly());
+        var builder = new BundlesConfig.Builder();
+        model.getConfig(builder, CLUSTER_CONFIG_ID);
+        BundlesConfig config = builder.build();
+        assertEquals(1, config.bundle().size());
+        assertThat(config.bundle(0).value(), endsWith(METRICS_PROXY_BUNDLE_FILE.toString()));
+    }
 
     @Test
     public void default_consumer_is_always_present_and_has_all_vespa_metrics_and_all_system_metrics() {