Refactor ConfigModel Construction in StandardActivity (#2104)

* Refactor ConfigModel Construction in StandardActivity

* Tracking ConfigModel Param Classes

* ActivityDef ConfigMap and default ActivityDriver

* More complete config model for ActivityDef plus other small improvements

* Add NBAdvisor to StandardActivity

* NBAdvisor for Workload Params, Tags, and Bindings

* OF: Formatting

* Improve ActivityDef ConfigModel

* Refactor NBAdvisor output adding Debug level

* NBAdvisor output using fluent method

* Slight refactor and always fail condition

* SrInterpolator uses NBAdvisorPoint and NBAdvisorOutput improvements

* Check Cmd with NBAdvisor

* Add Advisor Name and Description to Logs

* Fix review comments

nb-engine/nb-engine-cli/src/test/java/io/nosqlbench/engine/cli/NBCLIScenarioPreprocessorTest.java

@@ -101,7 +101,7 @@ public class NBCLIScenarioPreprocessorTest {
 
     @Test
     public void testThatTemplateParamsAreExpandedAndNotRemovedOverride() {
-        NBCLIOptions opts = new NBCLIOptions(new String[]{"scenario_test", "template_test", "cycles-test=20"}, NBCLIOptions.Mode.ParseAllOptions);
+        NBCLIOptions opts = new NBCLIOptions(new String[]{"scenario_test", "template_test", "cycles_test=20"}, NBCLIOptions.Mode.ParseAllOptions);
         List<Cmd> cmds = opts.getCommands();
         assertThat(cmds.size()).isEqualTo(1);
         assertThat(cmds.get(0).getArgMap()).isEqualTo(Map.of(
@@ -109,7 +109,7 @@ public class NBCLIScenarioPreprocessorTest {
             "alias", "with_template",
             "container", "template_test",
             "cycles", "20",
-            "cycles-test", "20",
+            "cycles_test", "20",
             "driver", "stdout",
             "labels", "workload:scenario_test,scenario:template_test",
             "step", "with_template",
@@ -119,14 +119,14 @@ public class NBCLIScenarioPreprocessorTest {
 
     @Test
     public void testThatUndefValuesAreUndefined() {
-        NBCLIOptions opts = new NBCLIOptions(new String[]{"scenario_test", "schema_only", "cycles-test=20"}, NBCLIOptions.Mode.ParseAllOptions);
+        NBCLIOptions opts = new NBCLIOptions(new String[]{"scenario_test", "schema_only", "cycles_test=20"}, NBCLIOptions.Mode.ParseAllOptions);
         List<Cmd> cmds = opts.getCommands();
         assertThat(cmds.size()).isEqualTo(1);
         assertThat(cmds.get(0).getArgMap()).isEqualTo(Map.of(
             "_impl", "run",
             "alias", "schema",
             "container", "schema_only",
-            "cycles-test", "20",
+            "cycles_test", "20",
             "driver", "stdout",
             "labels", "workload:scenario_test,scenario:schema_only",
             "step", "schema",
@@ -136,7 +136,7 @@ public class NBCLIScenarioPreprocessorTest {
         NBCLIOptions opts1 = new NBCLIOptions(new String[]{"scenario_test", "schema_only", "doundef=20"}, NBCLIOptions.Mode.ParseAllOptions);
         List<Cmd> cmds1 = opts1.getCommands();
         assertThat(cmds1.size()).isEqualTo(1);
-        assertThat(cmds1.get(0).getArgValueOrNull("cycles-test")).isNull();
+        assertThat(cmds1.get(0).getArgValueOrNull("cycles_test")).isNull();
     }
 
     @Test
@@ -150,7 +150,7 @@ public class NBCLIScenarioPreprocessorTest {
         Path absolute = rel.toAbsolutePath();
         assertThat(absolute).exists();
 
-        NBCLIOptions opts = new NBCLIOptions(new String[]{absolute.toString(), "schema_only", "cycles-test=20"}, NBCLIOptions.Mode.ParseAllOptions);
+        NBCLIOptions opts = new NBCLIOptions(new String[]{absolute.toString(), "schema_only", "cycles_test=20"}, NBCLIOptions.Mode.ParseAllOptions);
         List<Cmd> cmds = opts.getCommands();
         assertThat(cmds.size()).isGreaterThan(0);
     }
@@ -161,7 +161,7 @@ public class NBCLIScenarioPreprocessorTest {
         //TODO: This might change?
         String urlScenario = "https://raw.githubusercontent.com/nosqlbench/nosqlbench/main/engine-cli/src/test/resources/activities/scenario_test.yaml";
 
-        NBCLIOptions opts = new NBCLIOptions(new String[]{urlScenario, "schema_only", "cycles-test=20"}, NBCLIOptions.Mode.ParseAllOptions);
+        NBCLIOptions opts = new NBCLIOptions(new String[]{urlScenario, "schema_only", "cycles_test=20"}, NBCLIOptions.Mode.ParseAllOptions);
         List<Cmd> cmds = opts.getCommands();
         assertThat(cmds.size()).isGreaterThan(0);
     }
@@ -174,14 +174,14 @@ public class NBCLIScenarioPreprocessorTest {
 
     @Test
     public void testSubStepSelection() {
-        NBCLIOptions opts = new NBCLIOptions(new String[]{"scenario_test", "schema_only", "cycles-test=20"}, NBCLIOptions.Mode.ParseAllOptions);
+        NBCLIOptions opts = new NBCLIOptions(new String[]{"scenario_test", "schema_only", "cycles_test=20"}, NBCLIOptions.Mode.ParseAllOptions);
         List<Cmd> cmds = opts.getCommands();
         assertThat(cmds.size()).isEqualTo(1);
         assertThat(cmds.get(0).getArgMap()).isEqualTo(Map.of(
             "_impl", "run",
             "alias", "schema",
             "container", "schema_only",
-            "cycles-test", "20",
+            "cycles_test", "20",
             "driver", "stdout",
             "labels", "workload:scenario_test,scenario:schema_only",
             "step", "schema",
@@ -234,7 +234,7 @@ public class NBCLIScenarioPreprocessorTest {
             .isThrownBy(() -> NBCLIScenarioPreprocessor.splitCommand(unclosedQuoteCmd))
             .withMessageContaining("Unclosed quote found in scenario cmd");
     }
-    
+
     @Test
     public void testThatSuggestionsAreShownForDirectStepNameUsage() {
         assertThatExceptionOfType(BasicError.class)

nb-engine/nb-engine-cli/src/test/resources/activities/scenario_test.yaml

@@ -9,7 +9,7 @@ scenarios:
     schema: run driver=stdout workload==scenario_test tags=block:"schema.*" doundef==undef
 
   template_test:
-    with_template: run driver=stdout cycles=TEMPLATE(cycles-test,10)
+    with_template: run driver=stdout cycles=TEMPLATE(cycles_test,10)
 
   duplicate_param:
     schema: run driver=stdout workload==scenario_test threads=auto tags=block:"schema.*" threads=1 doundef==undef

nb-engine/nb-engine-core/src/main/java/io/nosqlbench/engine/api/activityimpl/SimpleActivity.java

@@ -97,7 +97,7 @@ public class SimpleActivity extends NBStatusComponent implements Activity, Invok
                 activityDef.getParams().set("alias", workloadOpt.get());
             } else {
                 activityDef.getParams().set("alias",
-                    activityDef.getActivityType().toUpperCase(Locale.ROOT)
+                    activityDef.getActivityDriver().toUpperCase(Locale.ROOT)
                         + nameEnumerator);
                 nameEnumerator++;
             }
@@ -540,7 +540,8 @@ public class SimpleActivity extends NBStatusComponent implements Activity, Invok
      * @return The sequence of operations as determined by filtering and ratios
      */
     @Deprecated(forRemoval = true)
-    protected <O> OpSequence<OpDispenser<? extends O>> createOpSequence(Function<OpTemplate, OpDispenser<? extends O>> opinit, boolean strict, DriverAdapter<?, ?> defaultAdapter) {
+    protected <O> OpSequence<OpDispenser<? extends O>> createOpSequence(Function<OpTemplate,
+        OpDispenser<? extends O>> opinit, boolean strict, DriverAdapter<?, ?> defaultAdapter) {
 
         List<OpTemplate> stmts = loadOpTemplates(defaultAdapter);
 

nb-engine/nb-engine-core/src/main/java/io/nosqlbench/engine/api/activityimpl/uniform/StandardActivity.java

@@ -27,12 +27,13 @@ import io.nosqlbench.adapters.api.activityimpl.uniform.Space;
 import io.nosqlbench.adapters.api.activityimpl.uniform.decorators.SyntheticOpTemplateProvider;
 import io.nosqlbench.adapters.api.activityimpl.uniform.flowtypes.CycleOp;
 import io.nosqlbench.adapters.api.templating.ParsedOp;
+import io.nosqlbench.nb.api.advisor.NBAdvisorPoint;
+import io.nosqlbench.nb.api.advisor.conditions.Conditions;
 import io.nosqlbench.nb.api.engine.metrics.instruments.MetricCategory;
 import io.nosqlbench.nb.api.lifecycle.Shutdownable;
 import io.nosqlbench.nb.api.components.core.NBComponent;
 import io.nosqlbench.nb.api.config.standard.*;
 import io.nosqlbench.nb.api.engine.activityimpl.ActivityDef;
-import io.nosqlbench.nb.api.errors.BasicError;
 import io.nosqlbench.nb.api.errors.OpConfigError;
 import io.nosqlbench.nb.api.labels.NBLabels;
 import io.nosqlbench.nb.api.components.events.NBEvent;
@@ -67,10 +68,17 @@ public class StandardActivity<R extends java.util.function.LongFunction, S> exte
 
     public StandardActivity(NBComponent parent, ActivityDef activityDef) {
         super(parent, activityDef);
-        OpsDocList workload;
-
-        Optional<String> yaml_loc = activityDef.getParams().getOptionalString("yaml", "workload");
+        NBAdvisorPoint<String> paramsAdvisor = create().advisor(b -> b.name("Workload"));
+        paramsAdvisor.add(Conditions.ValidNameError);
+        List<DriverAdapter<CycleOp<?>, Space>> adapterlist = new ArrayList<>();
+        List<ParsedOp> pops = new ArrayList<>();
+        ConcurrentHashMap<String, OpMapper<? extends CycleOp<?>, ? extends Space>> mappers = new ConcurrentHashMap<>();
+        NBConfigModel activityModel = activityDef.getConfigModel();
+        String defaultDriverName = activityDef.getActivityDriver();
+        DriverAdapter<CycleOp<?>, Space> defaultAdapter = getDriverAdapter(defaultDriverName);
         NBConfigModel yamlmodel;
+        OpsDocList workload;
+        Optional<String> yaml_loc = activityDef.getParams().getOptionalString("yaml", "workload");
         if (yaml_loc.isPresent()) {
             Map<String, Object> disposable = new LinkedHashMap<>(activityDef.getParams());
             workload = OpsLoader.loadPath(yaml_loc.get(), disposable, "activities");
@@ -78,88 +86,56 @@ public class StandardActivity<R extends java.util.function.LongFunction, S> exte
         } else {
             yamlmodel = ConfigModel.of(StandardActivity.class).asReadOnly();
         }
-
-        Optional<String> defaultDriverName = activityDef.getParams().getOptionalString("driver");
-        Optional<DriverAdapter<?, ?>> defaultAdapter = defaultDriverName
-            .flatMap(name -> ServiceSelector.of(name, ServiceLoader.load(DriverAdapterLoader.class)).get())
-            .map(l -> l.load(this, NBLabels.forKV()));
-
-        if (defaultDriverName.isPresent() && defaultAdapter.isEmpty()) {
-            throw new BasicError("Unable to load '" + defaultDriverName.get() + "' driver adapter.\n"+
-                                 "Rebuild NB5 to include this driver adapter. "+
-                                 "Change '<activeByDefault>false</activeByDefault>' for the driver in "+
-                                 "'./nb-adapters/pom.xml' and './nb-adapters/nb-adapters-included/pom.xml' first.");
-        }
-
-        // HERE, op templates are loaded before drivers are loaded
-        List<OpTemplate> opTemplates = loadOpTemplates(defaultAdapter.orElse(null));
-
-
-        List<ParsedOp> pops = new ArrayList<>();
-        List<DriverAdapter<CycleOp<?>, Space>> adapterlist = new ArrayList<>();
+        yamlmodel.log();
         NBConfigModel supersetConfig = ConfigModel.of(StandardActivity.class).add(yamlmodel);
-
-        Optional<String> defaultDriverOption = defaultDriverName;
-        ConcurrentHashMap<String, OpMapper<? extends CycleOp<?>, ? extends Space>> mappers = new ConcurrentHashMap<>();
+        //NBConfigModel supersetConfig = ConfigModel.of(StandardActivity.class).add(activityModel);
+        // Load the op templates
+        List<OpTemplate> opTemplates = loadOpTemplates(defaultAdapter);
         for (OpTemplate ot : opTemplates) {
-//            ParsedOp incompleteOpDef = new ParsedOp(ot, NBConfiguration.empty(), List.of(), this);
+            logger.info(() -> "StandardActivity.opTemplate = "+ot);
             String driverName = ot.getOptionalStringParam("driver", String.class)
                 .or(() -> ot.getOptionalStringParam("type", String.class))
-                .or(() -> defaultDriverOption)
-                .orElseThrow(() -> new OpConfigError("Unable to identify driver name for op template:\n" + ot));
-
-//            String driverName = ot.getOptionalStringParam("driver")
-//                .or(() -> activityDef.getParams().getOptionalString("driver"))
-//                .orElseThrow(() -> new OpConfigError("Unable to identify driver name for op template:\n" + ot));
-
-
-            // HERE
+                .orElse(defaultDriverName);
             if (!adapters.containsKey(driverName)) {
-
-                DriverAdapter<CycleOp<?>,Space> adapter = Optional.of(driverName)
-                    .flatMap(
-                        name -> ServiceSelector.of(
-                                name,
-                                ServiceLoader.load(DriverAdapterLoader.class)
-                            )
-                            .get())
-                    .map(
-                        l -> l.load(
-                            this,
-                            NBLabels.forKV()
-                        )
-                    )
-                    .orElseThrow(() -> new OpConfigError("driver adapter not present for name '" + driverName + "'"));
-
+                DriverAdapter<CycleOp<?>,Space> adapter =
+                    defaultDriverName.equals(driverName) ? defaultAdapter : getDriverAdapter(driverName);
                 NBConfigModel combinedModel = yamlmodel;
+                //NBConfigModel combinedModel = activityModel;
                 NBConfiguration combinedConfig = combinedModel.matchConfig(activityDef.getParams());
-
                 if (adapter instanceof NBConfigurable configurable) {
                     NBConfigModel adapterModel = configurable.getConfigModel();
                     supersetConfig.add(adapterModel);
-
                     combinedModel = adapterModel.add(yamlmodel);
+                    //combinedModel = adapterModel.add(activityModel);
                     combinedConfig = combinedModel.matchConfig(activityDef.getParams());
                     configurable.applyConfig(combinedConfig);
                 }
                 adapters.put(driverName, adapter);
                 mappers.put(driverName, adapter.getOpMapper());
             }
-
+            paramsAdvisor.validateAll(ot.getParams().keySet());
+            paramsAdvisor.validateAll(ot.getTags().keySet());
+            //TO-DO - paramsAdvisor.validateAll(ot.getBindings().keySet());
             supersetConfig.assertValidConfig(activityDef.getParams().getStringStringMap());
-
+            supersetConfig.log();
             DriverAdapter<CycleOp<?>, Space> adapter = adapters.get(driverName);
             adapterlist.add(adapter);
             ParsedOp pop = new ParsedOp(ot, adapter.getConfiguration(), List.of(adapter.getPreprocessor()), this);
+            logger.info("StandardActivity.pop="+pop);
             Optional<String> discard = pop.takeOptionalStaticValue("driver", String.class);
             pops.add(pop);
         }
+        logger.info(() -> "StandardActivity.opTemplate loop complete");
 
-        if (defaultDriverOption.isPresent()) {
-            long matchingDefault = mappers.keySet().stream().filter(n -> n.equals(defaultDriverOption.get())).count();
-            if (0 == matchingDefault) {
-                logger.warn("All op templates used a different driver than the default '{}'", defaultDriverOption.get());
-            }
+        paramsAdvisor.setName("Workload", "Check parameters, template, and binding names")
+            .logName().evaluate();
+
+        paramsAdvisor.clear().setName("Superset", "Check overall parameters");
+        paramsAdvisor.validateAll(supersetConfig.getNamedParams().keySet());
+        paramsAdvisor.logName().evaluate();
+
+        if (0 == mappers.keySet().stream().filter(n -> n.equals(defaultDriverName)).count()) {
+            logger.warn(() -> "All op templates used a different driver than the default '" + defaultDriverName + "'");
         }
 
         try {
@@ -191,6 +167,17 @@ public class StandardActivity<R extends java.util.function.LongFunction, S> exte
         );
     }
 
+    private DriverAdapter<CycleOp<?>, Space> getDriverAdapter(String driverName) {
+        return Optional.of(driverName)
+            .flatMap(name -> ServiceSelector.of(name, ServiceLoader.load(DriverAdapterLoader.class)).get())
+            .map(l -> l.load(this, NBLabels.forKV())
+            )
+            .orElseThrow(() -> new OpConfigError("Unable to load '" + driverName + "' driver adapter.\n"+
+                "Rebuild NB5 to include this driver adapter. "+
+                "Change '<activeByDefault>false</activeByDefault>' for the driver in "+
+                "'./nb-adapters/pom.xml' and './nb-adapters/nb-adapters-included/pom.xml' first."));
+    }
+
     @Override
     public void initActivity() {
         super.initActivity();
@@ -217,7 +204,6 @@ public class StandardActivity<R extends java.util.function.LongFunction, S> exte
     @Override
     public synchronized void onActivityDefUpdate(ActivityDef activityDef) {
         super.onActivityDefUpdate(activityDef);
-
         for (DriverAdapter<?, ?> adapter : adapters.values()) {
             if (adapter instanceof NBReconfigurable configurable) {
                 NBConfigModel cfgModel = configurable.getReconfigModel();
@@ -279,7 +265,6 @@ public class StandardActivity<R extends java.util.function.LongFunction, S> exte
         return super.getLabels();
     }
 
-
     @Override
     public void onEvent(NBEvent event) {
         switch (event) {
@@ -295,5 +280,4 @@ public class StandardActivity<R extends java.util.function.LongFunction, S> exte
         }
     }
 
-
 }

nb-engine/nb-engine-core/src/main/java/io/nosqlbench/engine/cmdstream/Cmd.java

@@ -16,6 +16,8 @@
 
 package io.nosqlbench.engine.cmdstream;
 
+import io.nosqlbench.nb.api.advisor.NBAdvisorPoint;
+import io.nosqlbench.nb.api.advisor.conditions.Conditions;
 import io.nosqlbench.nb.api.errors.BasicError;
 import jakarta.validation.constraints.NotNull;
 import org.apache.logging.log4j.Logger;
@@ -37,7 +39,6 @@ public class Cmd {
     private final Map<String, CmdArg> cmdArgs;
     private final String stepName;
 
-
     public String getTargetContext() {
         return targetContextName;
     }
@@ -86,6 +87,11 @@ public class Cmd {
 //    }
 //
     public Cmd(@NotNull String cmdTypeOrName, Map<String, CmdArg> argmap) {
+        NBAdvisorPoint<String> advisor = new NBAdvisorPoint<String>("Command","Check command arguments");
+        advisor.add(Conditions.ValidNameError);
+        advisor.validate(cmdTypeOrName);
+        advisor.validateAll(argmap.keySet());
+        advisor.logName().evaluate();
         this.cmdType = CmdType.valueOfAnyCaseOrIndirect(cmdTypeOrName);
         this.targetContextName = DEFAULT_TARGET_CONTEXT;
         this.stepName = "";

nb-engine/nb-engine-core/src/main/java/io/nosqlbench/engine/core/lifecycle/scenario/container/ContainerActivitiesController.java

@@ -421,11 +421,7 @@ public class ContainerActivitiesController extends NBBaseComponent {
         }
         try {
             result = future.get(timeoutMs, TimeUnit.MILLISECONDS);
-        } catch (ExecutionException e) {
-            throw new RuntimeException(e);
-        } catch (InterruptedException e) {
-            throw new RuntimeException(e);
-        } catch (TimeoutException e) {
+        } catch (ExecutionException | InterruptedException | TimeoutException e) {
             throw new RuntimeException(e);
         }
         return null != result;

nb-engine/nb-engine-core/src/main/java/io/nosqlbench/engine/core/lifecycle/scenario/container/NBBufferedContainer.java

@@ -21,6 +21,8 @@ import io.nosqlbench.engine.core.lifecycle.activity.ActivitiesProgressIndicator;
 import io.nosqlbench.engine.core.lifecycle.scenario.execution.ContextShutdownHook;
 import io.nosqlbench.engine.core.lifecycle.scenario.execution.NBCommandResult;
 import io.nosqlbench.engine.core.lifecycle.scenario.execution.NBInvokableCommand;
+import io.nosqlbench.nb.api.advisor.NBAdvisorPoint;
+import io.nosqlbench.nb.api.advisor.conditions.Conditions;
 import io.nosqlbench.nb.api.annotations.Annotation;
 import io.nosqlbench.nb.api.annotations.Layer;
 import io.nosqlbench.nb.api.labels.NBLabels;
@@ -50,6 +52,8 @@ public class NBBufferedContainer extends NBBaseComponent implements NBContainer
     private Exception error;
     private long endedAtMillis;
     private final Map<String, String> vars = new LinkedHashMap<>();
+    private final List<NBAdvisorPoint<?>> advisors = new ArrayList<>();
+    private NBAdvisorPoint<String> paramsAdvisor;
 
     public enum IOType {
         connected,
@@ -155,6 +159,10 @@ public class NBBufferedContainer extends NBBaseComponent implements NBContainer
     private final static Pattern pattern = Pattern.compile("\\$\\{(?<name>[a-zA-Z_][a-zA-Z0-9_.]*)}");
 
     private NBCommandParams interpolate(NBCommandParams params, Map<String, String> vars, String stepname) {
+        logger.debug("NBBufferedContainer.interpolate stepname=" + stepname);
+        paramsAdvisor = create().advisor(b -> b.name("Check params"));
+        paramsAdvisor.add(Conditions.ValidNameError);
+        paramsAdvisor.validateAll(vars.keySet());
         Map<String, String> interpolated = new LinkedHashMap<>();
         params.forEach((k, v) -> {
             Matcher varmatcher = pattern.matcher(v);
@@ -170,6 +178,9 @@ public class NBBufferedContainer extends NBBaseComponent implements NBContainer
             varmatcher.appendTail(sb);
             interpolated.put(k, sb.toString());
         });
+        paramsAdvisor.validateAll(interpolated.keySet());
+        paramsAdvisor.setName("Container", "Check container and variable names")
+            .logName().evaluate();
         return NBCommandParams.of(interpolated);
     }
 
@@ -190,9 +201,7 @@ public class NBBufferedContainer extends NBBaseComponent implements NBContainer
                     Object value = component.getAccessor().invoke(record);
                     results.put(stepname+"."+name,value.toString());
                     filtered.add(name);
-                } catch (IllegalAccessException e) {
-                    throw new RuntimeException(e);
-                } catch (InvocationTargetException e) {
+                } catch (IllegalAccessException | InvocationTargetException e) {
                     throw new RuntimeException(e);
                 }
             }
@@ -204,9 +213,7 @@ public class NBBufferedContainer extends NBBaseComponent implements NBContainer
                 try {
                     Object value = property.invoke(record);
                     results.put(stepname+"."+property.getName(),value.toString());
-                } catch (IllegalAccessException e) {
-                    throw new RuntimeException(e);
-                } catch (InvocationTargetException e) {
+                } catch (IllegalAccessException | InvocationTargetException e) {
                     throw new RuntimeException(e);
                 }
             }