feat(triggers): allow complex expressions, add caching for performance

README.md

@@ -40,16 +40,19 @@ shutdown so that the cause of an unexpected JVM shutdown might be captured for l
 
 ## SMART TRIGGERS
 
-`cryostat-agent` supports smart triggers that listen to the values of the MBean Counters and can start recordings based
+`cryostat-agent` supports Smart Triggers that listen to the values of the MBean Counters and can start recordings based
 on a set of constraints specified by the user.
 
-The general form of a smart trigger expression is as follows:
+The general form of a Smart Trigger expression is as follows:
 
 ```
-[constraint1(&&/||)constraint2...constraintN]~recordingTemplateNameOrLabel
+[constraint1(&&/||)constraint2...constraintN;durationConstraint]~recordingTemplateNameOrLabel
 ```
 
-Either the filename or label XML tag of the `${templateName}.jfc` may be used to specify the event template to use. For example, the JDK distribution ships with a `default.jfc` file containing the top-level `<configuration label="Continuous">` element. This template may be specified in the Smart Trigger definition as any of `default.jfc`, `default`, or `Continuous`.
+Either the filename or label XML tag of the `${templateName}.jfc` may be used to specify the event template to use. For
+example, the JDK distribution ships with a `default.jfc` file containing the top-level
+`<configuration label="Continuous">` element. This template may be specified in the Smart Trigger definition as any of
+`default.jfc`, `default`, or `Continuous`.
 
 An example for listening to CPU Usage and starting a recording using the Profiling template when it exceeds 0.2%:
 
@@ -61,7 +64,22 @@ An example for watching for the Thread Count to exceed 20 for longer than 10 sec
 Continuous template:
 
 ```
-[ThreadCount>20&&TargetDuration>duration("10s")]~Continuous
+[ThreadCount>20;TargetDuration>duration("10s")]~Continuous
+```
+
+The first part of the condition before the semicolon is a [Common Expression Language](https://github.com/google/cel-spec)
+expression for testing
+[various MBean metrics](https://github.com/cryostatio/cryostat-agent/blob/main/src/main/java/io/cryostat/agent/model/MBeanInfo.java)
+. The second part after the semicolon references a special variable, `TargetDuration`, which tracks the length of time
+that the first part of the condition has tested `true` for. This is converted to a `java.time.Duration` object and
+compared to `duration("10s")`, a special construct that is also converted into a `java.time.Duration` object
+representing the time threshold before this trigger activates. The `duration()` construct requires a `String` argument,
+which may be enclosed in single `'` or double `"` quotation marks.
+
+Smart Triggers may define more complex conditions that test multiple metrics:
+
+```
+[(ProcessCpuLoad>0.5||SystemCpuLoad>0.25)&&HeapMemoryUsagePercent>0.1;TargetDuration>duration('1m')]~Continuous
 ```
 
 These may be passed as an argument to the Cryostat Agent, for example:

src/main/java/io/cryostat/agent/model/MBeanInfo.java

@@ -45,12 +45,12 @@
 public class MBeanInfo {
 
     private final Logger log = LoggerFactory.getLogger(getClass());
-    private MBeanMetrics mBeanMetrics;
-    private Map<String, Object> simplifiedMetrics;
+    private final MBeanMetrics mBeanMetrics;
+    private final Map<String, Object> simplifiedMetrics;
 
     public MBeanInfo() {
         this.simplifiedMetrics = new HashMap<>();
-        mBeanMetrics =
+        this.mBeanMetrics =
                 new MBeanMetrics(
                         getRuntimeMetrics(),
                         getMemoryMetrics(),
@@ -64,7 +64,6 @@ public MBeanMetrics getMBeanMetrics() {
     }
 
     public Map<String, Object> getSimplifiedMetrics() {
-        // Map<String, Object> simplifiedMetrics = new HashMap<>();
         return Collections.unmodifiableMap(simplifiedMetrics);
     }
 

src/main/java/io/cryostat/agent/triggers/SmartTrigger.java

@@ -23,9 +23,10 @@
 
 public class SmartTrigger {
 
-    private static final String DURATION_PATTERN =
-            "(.+)(?:[&|]{2})(TargetDuration[<>=]+duration\\(\"(\\d+[sSmMhH]+)\"\\))";
-    private static final Pattern durationPattern = Pattern.compile(DURATION_PATTERN);
+    private static final String DURATION_PATTERN_STR =
+            "(TargetDuration[<>=]+duration\\(['\"](\\d+[sSmMhH]+)['\"]\\))";
+    private static final String DEFINITION_PATTERN_STR = "(.+)(?:;)" + DURATION_PATTERN_STR;
+    private static final Pattern DEFINITION_PATTERN = Pattern.compile(DEFINITION_PATTERN_STR);
 
     public enum TriggerState {
         /* Newly Created or Condition not met. */
@@ -38,7 +39,7 @@ public enum TriggerState {
         COMPLETE
     };
 
-    private final String expression;
+    private final String rawExpression;
     private final String durationConstraint;
     private final String triggerCondition;
     private final String recordingTemplate;
@@ -50,13 +51,13 @@ public enum TriggerState {
     private volatile TriggerState state;
 
     public SmartTrigger(String expression, String templateName) {
-        this.expression = expression;
+        this.rawExpression = expression;
         this.recordingTemplate = templateName;
         this.state = TriggerState.NEW;
-        Matcher m = durationPattern.matcher(expression);
+        Matcher m = DEFINITION_PATTERN.matcher(expression);
         if (m.matches()) {
             triggerCondition = m.group(1);
-            durationConstraint = m.group(2);
+            durationConstraint = m.group(2).replaceAll("'", "\"");
             /* Duration.parse requires timestamps in ISO8601 Duration format */
             targetDuration = Duration.parse("PT" + m.group(3));
         } else {
@@ -67,7 +68,7 @@ public SmartTrigger(String expression, String templateName) {
     }
 
     public String getExpression() {
-        return expression;
+        return rawExpression;
     }
 
     public TriggerState getState() {
@@ -109,7 +110,7 @@ public String getDurationConstraint() {
     @Override
     public int hashCode() {
         return Objects.hash(
-                expression,
+                rawExpression,
                 durationConstraint,
                 triggerCondition,
                 recordingTemplate,
@@ -128,7 +129,7 @@ public boolean equals(Object obj) {
             return false;
         }
         SmartTrigger other = (SmartTrigger) obj;
-        return Objects.equals(expression, other.expression)
+        return Objects.equals(rawExpression, other.rawExpression)
                 && Objects.equals(durationConstraint, other.durationConstraint)
                 && Objects.equals(triggerCondition, other.triggerCondition)
                 && Objects.equals(recordingTemplate, other.recordingTemplate)
@@ -137,10 +138,10 @@ public boolean equals(Object obj) {
 
     @Override
     public String toString() {
-        return "SmartTrigger [durationConstraint="
+        return "SmartTrigger [rawExpression="
+                + rawExpression
+                + ", durationConstraint="
                 + durationConstraint
-                + ", expression="
-                + expression
                 + ", recordingTemplate="
                 + recordingTemplate
                 + ", targetDuration="

src/main/java/io/cryostat/agent/triggers/TriggerEvaluator.java

@@ -19,9 +19,9 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Date;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.Future;
 import java.util.concurrent.ScheduledExecutorService;
@@ -37,6 +37,7 @@
 import com.google.api.expr.v1alpha1.Type.PrimitiveType;
 import org.projectnessie.cel.checker.Decls;
 import org.projectnessie.cel.tools.Script;
+import org.projectnessie.cel.tools.ScriptCreateException;
 import org.projectnessie.cel.tools.ScriptHost;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -46,9 +47,14 @@ public class TriggerEvaluator {
     private final ScheduledExecutorService scheduler;
     private final List<String> definitions;
     private final TriggerParser parser;
+    private final ScriptHost scriptHost;
     private final FlightRecorderHelper flightRecorderHelper;
     private final Harvester harvester;
     private final long evaluationPeriodMs;
+    private final ConcurrentHashMap<SmartTrigger, Script> conditionScriptCache =
+            new ConcurrentHashMap<>();
+    private final ConcurrentHashMap<SmartTrigger, Script> durationScriptCache =
+            new ConcurrentHashMap<>();
     private final ConcurrentLinkedQueue<SmartTrigger> triggers = new ConcurrentLinkedQueue<>();
     private Future<?> task;
     private final Logger log = LoggerFactory.getLogger(getClass());
@@ -63,6 +69,7 @@ public TriggerEvaluator(
         this.scheduler = scheduler;
         this.definitions = Collections.unmodifiableList(definitions);
         this.parser = parser;
+        this.scriptHost = ScriptHost.newBuilder().build();
         this.flightRecorderHelper = flightRecorderHelper;
         this.harvester = harvester;
         this.evaluationPeriodMs = evaluationPeriodMs;
@@ -112,6 +119,8 @@ private void evaluate() {
                         /* Trigger condition has been met, can remove it */
                         log.trace("Completed {} , removing", t);
                         triggers.remove(t);
+                        conditionScriptCache.remove(t);
+                        durationScriptCache.remove(t);
                         break;
                     case NEW:
                         // Simple Constraint, no duration specified so condition only needs to be
@@ -173,34 +182,58 @@ private void startRecording(SmartTrigger t) {
                             tr.getRecording().start();
                             t.setState(TriggerState.COMPLETE);
                             log.info(
-                                    "Started recording \"{}\" using template \"{}\"",
+                                    "Started recording \"{}\" using template \"{}\" due to trigger"
+                                            + " \"{}\"",
                                     recordingName,
-                                    t.getRecordingTemplateName());
+                                    t.getRecordingTemplateName(),
+                                    t.getExpression());
                         });
     }
 
     private boolean evaluateTriggerConstraint(SmartTrigger trigger, Duration targetDuration) {
         try {
-            Map<String, Object> scriptVars = new HashMap<>(new MBeanInfo().getSimplifiedMetrics());
-            ScriptHost scriptHost = ScriptHost.newBuilder().build();
-            Script script =
-                    scriptHost
-                            .buildScript(
-                                    Duration.ZERO.equals(targetDuration)
-                                            ? trigger.getTriggerCondition()
-                                            : trigger.getExpression())
-                            .withDeclarations(buildDeclarations(scriptVars))
-                            .build();
-            scriptVars.put("TargetDuration", targetDuration);
-            log.trace("evaluating mbean map:\n{}", scriptVars);
-            Boolean result = script.execute(Boolean.class, scriptVars);
-            return Boolean.TRUE.equals(result);
+            Map<String, Object> conditionVars = new MBeanInfo().getSimplifiedMetrics();
+            log.trace("evaluating mbean map:\n{}", conditionVars);
+            Boolean conditionResult =
+                    buildConditionScript(trigger, conditionVars)
+                            .execute(Boolean.class, conditionVars);
+
+            Map<String, Object> durationVar = Map.of("TargetDuration", targetDuration);
+            Boolean durationResult =
+                    Duration.ZERO.equals(targetDuration)
+                            ? Boolean.TRUE
+                            : buildDurationScript(trigger, durationVar)
+                                    .execute(Boolean.class, durationVar);
+
+            return Boolean.TRUE.equals(conditionResult) && Boolean.TRUE.equals(durationResult);
         } catch (Exception e) {
             log.error("Failed to create or execute script", e);
             return false;
         }
     }
 
+    private Script buildConditionScript(SmartTrigger trigger, Map<String, Object> scriptVars) {
+        return conditionScriptCache.computeIfAbsent(
+                trigger, t -> buildScript(t.getTriggerCondition(), scriptVars));
+    }
+
+    private Script buildDurationScript(SmartTrigger trigger, Map<String, Object> scriptVars) {
+        return durationScriptCache.computeIfAbsent(
+                trigger, t -> buildScript(t.getDurationConstraint(), scriptVars));
+    }
+
+    private Script buildScript(String script, Map<String, Object> scriptVars) {
+        try {
+            return scriptHost
+                    .buildScript(script)
+                    .withDeclarations(buildDeclarations(scriptVars))
+                    .build();
+        } catch (ScriptCreateException sce) {
+            log.error("Failed to create script", sce);
+            throw new RuntimeException(sce);
+        }
+    }
+
     private List<Decl> buildDeclarations(Map<String, Object> scriptVars) {
         ArrayList<Decl> decls = new ArrayList<>();
         for (Map.Entry<String, Object> s : scriptVars.entrySet()) {
@@ -209,17 +242,17 @@ private List<Decl> buildDeclarations(Map<String, Object> scriptVars) {
             log.trace("Declaring script var {} [{}]", key, parseType);
             decls.add(Decls.newVar(key, parseType));
         }
-        decls.add(Decls.newVar("TargetDuration", Decls.Duration));
         return decls;
     }
 
     private Type parseType(Object obj) {
         if (obj.getClass().equals(String.class)) return Decls.String;
-        else if (obj.getClass().equals(Double.class)) return Decls.Double;
-        else if (obj.getClass().equals(Integer.class)) return Decls.Int;
         else if (obj.getClass().equals(Boolean.class)) return Decls.Bool;
+        else if (obj.getClass().equals(Integer.class)) return Decls.Int;
         else if (obj.getClass().equals(Long.class))
             return Decls.newPrimitiveType(PrimitiveType.INT64);
+        else if (obj.getClass().equals(Double.class)) return Decls.Double;
+        else if (obj.getClass().equals(Duration.class)) return Decls.Duration;
         else
             // Default to String so we can still do some comparison
             return Decls.String;