From af7998f117beb05a5822d1554bac49db4dc6daf1 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Fri, 17 Nov 2023 15:59:12 -0500 Subject: [PATCH 1/4] handle null input --- src/main/java/io/cryostat/agent/triggers/TriggerParser.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/io/cryostat/agent/triggers/TriggerParser.java b/src/main/java/io/cryostat/agent/triggers/TriggerParser.java index 391a45e6..2909b570 100644 --- a/src/main/java/io/cryostat/agent/triggers/TriggerParser.java +++ b/src/main/java/io/cryostat/agent/triggers/TriggerParser.java @@ -41,7 +41,7 @@ public TriggerParser(FlightRecorderHelper flightRecorderHelper) { public List parse(String[] args) { List triggers = new ArrayList<>(); - if (args.length < 1) { + if (args == null || args.length < 1) { log.trace("Agent args were empty, no Triggers were defined"); return triggers; } From 6168f380b61258b624c2648632828407116d430a Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Fri, 17 Nov 2023 15:59:26 -0500 Subject: [PATCH 2/4] strip leading/trailing whitespaces --- .../java/io/cryostat/agent/triggers/SmartTrigger.java | 10 +++++----- .../java/io/cryostat/agent/triggers/TriggerParser.java | 1 + 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/main/java/io/cryostat/agent/triggers/SmartTrigger.java b/src/main/java/io/cryostat/agent/triggers/SmartTrigger.java index 5f2cef27..b8c544a3 100644 --- a/src/main/java/io/cryostat/agent/triggers/SmartTrigger.java +++ b/src/main/java/io/cryostat/agent/triggers/SmartTrigger.java @@ -24,8 +24,8 @@ public class SmartTrigger { private static final String DURATION_PATTERN_STR = - "(TargetDuration[<>=]+duration\\(['\"](\\d+[sSmMhH]+)['\"]\\))"; - private static final String DEFINITION_PATTERN_STR = "(.+)(?:;)" + DURATION_PATTERN_STR; + "(TargetDuration\\s*[<>=]+\\s*duration\\(['\"](\\d+[sSmMhH]+)['\"]\\))"; + private static final String DEFINITION_PATTERN_STR = "(.+)\\s*(?:;)\\s*" + DURATION_PATTERN_STR; private static final Pattern DEFINITION_PATTERN = Pattern.compile(DEFINITION_PATTERN_STR); public enum TriggerState { @@ -56,10 +56,10 @@ public SmartTrigger(String expression, String templateName) { this.state = TriggerState.NEW; Matcher m = DEFINITION_PATTERN.matcher(expression); if (m.matches()) { - triggerCondition = m.group(1); - durationConstraint = m.group(2).replaceAll("'", "\""); + triggerCondition = m.group(1).strip(); + durationConstraint = m.group(2).replaceAll("'", "\"").strip(); /* Duration.parse requires timestamps in ISO8601 Duration format */ - targetDuration = Duration.parse("PT" + m.group(3)); + targetDuration = Duration.parse("PT" + m.group(3).strip()); } else { triggerCondition = expression; durationConstraint = ""; diff --git a/src/main/java/io/cryostat/agent/triggers/TriggerParser.java b/src/main/java/io/cryostat/agent/triggers/TriggerParser.java index 2909b570..4a0379a8 100644 --- a/src/main/java/io/cryostat/agent/triggers/TriggerParser.java +++ b/src/main/java/io/cryostat/agent/triggers/TriggerParser.java @@ -53,6 +53,7 @@ public List parse(String[] args) { String[] expressions = triggerDefinitions.split(","); for (String s : expressions) { + s = s.strip(); Matcher m = EXPRESSION_PATTERN.matcher(s); if (m.matches()) { String constraintString = m.group(1); From da53b4d9b0b1e052641d1b9f88239997f7c403a8 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Fri, 17 Nov 2023 15:59:49 -0500 Subject: [PATCH 3/4] add unit test --- .../agent/triggers/TriggerParserTest.java | 217 ++++++++++++++++++ 1 file changed, 217 insertions(+) create mode 100644 src/test/java/io/cryostat/agent/triggers/TriggerParserTest.java diff --git a/src/test/java/io/cryostat/agent/triggers/TriggerParserTest.java b/src/test/java/io/cryostat/agent/triggers/TriggerParserTest.java new file mode 100644 index 00000000..04c6e736 --- /dev/null +++ b/src/test/java/io/cryostat/agent/triggers/TriggerParserTest.java @@ -0,0 +1,217 @@ +/* + * Copyright The Cryostat Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.cryostat.agent.triggers; + +import java.time.Duration; +import java.util.ArrayList; +import java.util.List; + +import io.cryostat.agent.FlightRecorderHelper; +import io.cryostat.agent.triggers.SmartTrigger.TriggerState; + +import org.hamcrest.MatcherAssert; +import org.hamcrest.Matchers; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.NullSource; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +class TriggerParserTest { + + @Mock FlightRecorderHelper helper; + + TriggerParser parser; + + @BeforeEach + void setup() { + this.parser = new TriggerParser(helper); + } + + @ParameterizedTest + @MethodSource("emptyCases") + @NullSource + void testEmptyCases(List args) { + MatcherAssert.assertThat( + parser.parse(args == null ? null : args.toArray(new String[0])), + Matchers.equalTo(List.of())); + } + + @Test + void testSingleSimpleTrigger() { + Mockito.when(helper.isValidTemplate(Mockito.anyString())).thenReturn(true); + String[] in = new String[] {"[ProcessCpuLoad>0.2]~profile"}; + List out = parser.parse(in); + + MatcherAssert.assertThat(out, Matchers.hasSize(1)); + SmartTrigger trigger = out.get(0); + + MatcherAssert.assertThat(trigger.getExpression(), Matchers.equalTo("ProcessCpuLoad>0.2")); + MatcherAssert.assertThat(trigger.getRecordingTemplateName(), Matchers.equalTo("profile")); + MatcherAssert.assertThat(trigger.getDurationConstraint(), Matchers.emptyString()); + MatcherAssert.assertThat( + trigger.getTriggerCondition(), Matchers.equalTo("ProcessCpuLoad>0.2")); + MatcherAssert.assertThat(trigger.getState(), Matchers.equalTo(TriggerState.NEW)); + MatcherAssert.assertThat( + trigger.getTargetDuration(), Matchers.equalTo(Duration.ofSeconds(0))); + MatcherAssert.assertThat(trigger.getTimeConditionFirstMet(), Matchers.nullValue()); + } + + @Test + void testSingleComplexTrigger() { + Mockito.when(helper.isValidTemplate(Mockito.anyString())).thenReturn(true); + String[] in = + new String[] {"[ProcessCpuLoad>0.2;TargetDuration>duration(\"30s\")]~profile"}; + List out = parser.parse(in); + + MatcherAssert.assertThat(out, Matchers.hasSize(1)); + SmartTrigger trigger = out.get(0); + + MatcherAssert.assertThat( + trigger.getExpression(), + Matchers.equalTo("ProcessCpuLoad>0.2;TargetDuration>duration(\"30s\")")); + MatcherAssert.assertThat(trigger.getRecordingTemplateName(), Matchers.equalTo("profile")); + MatcherAssert.assertThat( + trigger.getDurationConstraint(), + Matchers.equalTo("TargetDuration>duration(\"30s\")")); + MatcherAssert.assertThat( + trigger.getTriggerCondition(), Matchers.equalTo("ProcessCpuLoad>0.2")); + MatcherAssert.assertThat(trigger.getState(), Matchers.equalTo(TriggerState.NEW)); + MatcherAssert.assertThat( + trigger.getTargetDuration(), Matchers.equalTo(Duration.ofSeconds(30))); + MatcherAssert.assertThat(trigger.getTimeConditionFirstMet(), Matchers.nullValue()); + } + + @Test + void testSingleComplexTriggerWithWhitespace() { + Mockito.when(helper.isValidTemplate(Mockito.anyString())).thenReturn(true); + String[] in = + new String[] { + "[ProcessCpuLoad > 0.2 ; TargetDuration > duration(\"30s\")]~profile" + }; + List out = parser.parse(in); + + MatcherAssert.assertThat(out, Matchers.hasSize(1)); + SmartTrigger trigger = out.get(0); + + MatcherAssert.assertThat( + trigger.getExpression(), + Matchers.equalTo("ProcessCpuLoad > 0.2 ; TargetDuration > duration(\"30s\")")); + MatcherAssert.assertThat(trigger.getRecordingTemplateName(), Matchers.equalTo("profile")); + MatcherAssert.assertThat( + trigger.getDurationConstraint(), + Matchers.equalTo("TargetDuration > duration(\"30s\")")); + MatcherAssert.assertThat( + trigger.getTriggerCondition(), Matchers.equalTo("ProcessCpuLoad > 0.2")); + MatcherAssert.assertThat(trigger.getState(), Matchers.equalTo(TriggerState.NEW)); + MatcherAssert.assertThat( + trigger.getTargetDuration(), Matchers.equalTo(Duration.ofSeconds(30))); + MatcherAssert.assertThat(trigger.getTimeConditionFirstMet(), Matchers.nullValue()); + } + + @Test + void testMultipleComplexTriggerWithWhitespace() { + Mockito.when(helper.isValidTemplate(Mockito.anyString())).thenReturn(true); + String[] in = + new String[] { + "[ProcessCpuLoad>0.2 ; TargetDuration>duration(\"30s\")]~profile," + + " [(HeapMemoryUsagePercent > 50 && NonHeapMemoryUsage > 1) ||" + + " SystemCpuLoad > 4 ; TargetDuration > duration(\"2m\")]~default.jfc" + }; + List out = parser.parse(in); + + MatcherAssert.assertThat(out, Matchers.hasSize(2)); + + SmartTrigger trigger1 = out.get(0); + MatcherAssert.assertThat( + trigger1.getExpression(), + Matchers.equalTo("ProcessCpuLoad>0.2 ; TargetDuration>duration(\"30s\")")); + MatcherAssert.assertThat(trigger1.getRecordingTemplateName(), Matchers.equalTo("profile")); + MatcherAssert.assertThat( + trigger1.getDurationConstraint(), + Matchers.equalTo("TargetDuration>duration(\"30s\")")); + MatcherAssert.assertThat( + trigger1.getTriggerCondition(), Matchers.equalTo("ProcessCpuLoad>0.2")); + MatcherAssert.assertThat(trigger1.getState(), Matchers.equalTo(TriggerState.NEW)); + MatcherAssert.assertThat( + trigger1.getTargetDuration(), Matchers.equalTo(Duration.ofSeconds(30))); + MatcherAssert.assertThat(trigger1.getTimeConditionFirstMet(), Matchers.nullValue()); + + SmartTrigger trigger2 = out.get(1); + MatcherAssert.assertThat( + trigger2.getExpression(), + Matchers.equalTo( + "(HeapMemoryUsagePercent > 50 && NonHeapMemoryUsage > 1) || SystemCpuLoad >" + + " 4 ; TargetDuration > duration(\"2m\")")); + MatcherAssert.assertThat(trigger2.getRecordingTemplateName(), Matchers.equalTo("default")); + MatcherAssert.assertThat( + trigger2.getDurationConstraint(), + Matchers.equalTo("TargetDuration > duration(\"2m\")")); + MatcherAssert.assertThat( + trigger2.getTriggerCondition(), + Matchers.equalTo( + "(HeapMemoryUsagePercent > 50 && NonHeapMemoryUsage > 1) || SystemCpuLoad >" + + " 4")); + MatcherAssert.assertThat(trigger2.getState(), Matchers.equalTo(TriggerState.NEW)); + MatcherAssert.assertThat( + trigger2.getTargetDuration(), Matchers.equalTo(Duration.ofMinutes(2))); + MatcherAssert.assertThat(trigger2.getTimeConditionFirstMet(), Matchers.nullValue()); + } + + @Test + void testMultipleComplexTriggerWithWhitespaceWhenOnlyOneTemplateValid() { + Mockito.when(helper.isValidTemplate(Mockito.anyString())) + .thenReturn(true) + .thenReturn(false); + String[] in = + new String[] { + "[ProcessCpuLoad>0.2 ; TargetDuration>duration(\"30s\")]~profile," + + " [(HeapMemoryUsagePercent > 50 && NonHeapMemoryUsage > 1) ||" + + " SystemCpuLoad > 4 ; TargetDuration > duration(\"2m\")]~default.jfc" + }; + List out = parser.parse(in); + + MatcherAssert.assertThat(out, Matchers.hasSize(1)); + + SmartTrigger trigger1 = out.get(0); + MatcherAssert.assertThat( + trigger1.getExpression(), + Matchers.equalTo("ProcessCpuLoad>0.2 ; TargetDuration>duration(\"30s\")")); + MatcherAssert.assertThat(trigger1.getRecordingTemplateName(), Matchers.equalTo("profile")); + MatcherAssert.assertThat( + trigger1.getDurationConstraint(), + Matchers.equalTo("TargetDuration>duration(\"30s\")")); + MatcherAssert.assertThat( + trigger1.getTriggerCondition(), Matchers.equalTo("ProcessCpuLoad>0.2")); + MatcherAssert.assertThat(trigger1.getState(), Matchers.equalTo(TriggerState.NEW)); + MatcherAssert.assertThat( + trigger1.getTargetDuration(), Matchers.equalTo(Duration.ofSeconds(30))); + MatcherAssert.assertThat(trigger1.getTimeConditionFirstMet(), Matchers.nullValue()); + } + + static List> emptyCases() { + List> l = new ArrayList<>(); + l.add(List.of()); + l.add(List.of("")); + l.add(List.of(" ")); + return l; + } +} From 0c213073ddb1af5e5c95c66cb65e8ad6c852a159 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Fri, 17 Nov 2023 16:02:09 -0500 Subject: [PATCH 4/4] remove whitespaces during parsing --- .../cryostat/agent/triggers/SmartTrigger.java | 6 +++--- .../cryostat/agent/triggers/TriggerParser.java | 2 +- .../agent/triggers/TriggerParserTest.java | 18 ++++++++---------- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/main/java/io/cryostat/agent/triggers/SmartTrigger.java b/src/main/java/io/cryostat/agent/triggers/SmartTrigger.java index b8c544a3..54f21623 100644 --- a/src/main/java/io/cryostat/agent/triggers/SmartTrigger.java +++ b/src/main/java/io/cryostat/agent/triggers/SmartTrigger.java @@ -56,10 +56,10 @@ public SmartTrigger(String expression, String templateName) { this.state = TriggerState.NEW; Matcher m = DEFINITION_PATTERN.matcher(expression); if (m.matches()) { - triggerCondition = m.group(1).strip(); - durationConstraint = m.group(2).replaceAll("'", "\"").strip(); + triggerCondition = m.group(1).replaceAll("\\s", ""); + durationConstraint = m.group(2).replaceAll("'", "\"").replaceAll("\\s", ""); /* Duration.parse requires timestamps in ISO8601 Duration format */ - targetDuration = Duration.parse("PT" + m.group(3).strip()); + targetDuration = Duration.parse("PT" + m.group(3).replaceAll("\\s", "")); } else { triggerCondition = expression; durationConstraint = ""; diff --git a/src/main/java/io/cryostat/agent/triggers/TriggerParser.java b/src/main/java/io/cryostat/agent/triggers/TriggerParser.java index 4a0379a8..3024c5c3 100644 --- a/src/main/java/io/cryostat/agent/triggers/TriggerParser.java +++ b/src/main/java/io/cryostat/agent/triggers/TriggerParser.java @@ -53,7 +53,7 @@ public List parse(String[] args) { String[] expressions = triggerDefinitions.split(","); for (String s : expressions) { - s = s.strip(); + s = s.replaceAll("\\s", ""); Matcher m = EXPRESSION_PATTERN.matcher(s); if (m.matches()) { String constraintString = m.group(1); diff --git a/src/test/java/io/cryostat/agent/triggers/TriggerParserTest.java b/src/test/java/io/cryostat/agent/triggers/TriggerParserTest.java index 04c6e736..fa977d76 100644 --- a/src/test/java/io/cryostat/agent/triggers/TriggerParserTest.java +++ b/src/test/java/io/cryostat/agent/triggers/TriggerParserTest.java @@ -114,13 +114,13 @@ void testSingleComplexTriggerWithWhitespace() { MatcherAssert.assertThat( trigger.getExpression(), - Matchers.equalTo("ProcessCpuLoad > 0.2 ; TargetDuration > duration(\"30s\")")); + Matchers.equalTo("ProcessCpuLoad>0.2;TargetDuration>duration(\"30s\")")); MatcherAssert.assertThat(trigger.getRecordingTemplateName(), Matchers.equalTo("profile")); MatcherAssert.assertThat( trigger.getDurationConstraint(), - Matchers.equalTo("TargetDuration > duration(\"30s\")")); + Matchers.equalTo("TargetDuration>duration(\"30s\")")); MatcherAssert.assertThat( - trigger.getTriggerCondition(), Matchers.equalTo("ProcessCpuLoad > 0.2")); + trigger.getTriggerCondition(), Matchers.equalTo("ProcessCpuLoad>0.2")); MatcherAssert.assertThat(trigger.getState(), Matchers.equalTo(TriggerState.NEW)); MatcherAssert.assertThat( trigger.getTargetDuration(), Matchers.equalTo(Duration.ofSeconds(30))); @@ -143,7 +143,7 @@ void testMultipleComplexTriggerWithWhitespace() { SmartTrigger trigger1 = out.get(0); MatcherAssert.assertThat( trigger1.getExpression(), - Matchers.equalTo("ProcessCpuLoad>0.2 ; TargetDuration>duration(\"30s\")")); + Matchers.equalTo("ProcessCpuLoad>0.2;TargetDuration>duration(\"30s\")")); MatcherAssert.assertThat(trigger1.getRecordingTemplateName(), Matchers.equalTo("profile")); MatcherAssert.assertThat( trigger1.getDurationConstraint(), @@ -159,17 +159,15 @@ void testMultipleComplexTriggerWithWhitespace() { MatcherAssert.assertThat( trigger2.getExpression(), Matchers.equalTo( - "(HeapMemoryUsagePercent > 50 && NonHeapMemoryUsage > 1) || SystemCpuLoad >" - + " 4 ; TargetDuration > duration(\"2m\")")); + "(HeapMemoryUsagePercent>50&&NonHeapMemoryUsage>1)||SystemCpuLoad>4;TargetDuration>duration(\"2m\")")); MatcherAssert.assertThat(trigger2.getRecordingTemplateName(), Matchers.equalTo("default")); MatcherAssert.assertThat( trigger2.getDurationConstraint(), - Matchers.equalTo("TargetDuration > duration(\"2m\")")); + Matchers.equalTo("TargetDuration>duration(\"2m\")")); MatcherAssert.assertThat( trigger2.getTriggerCondition(), Matchers.equalTo( - "(HeapMemoryUsagePercent > 50 && NonHeapMemoryUsage > 1) || SystemCpuLoad >" - + " 4")); + "(HeapMemoryUsagePercent>50&&NonHeapMemoryUsage>1)||SystemCpuLoad>4")); MatcherAssert.assertThat(trigger2.getState(), Matchers.equalTo(TriggerState.NEW)); MatcherAssert.assertThat( trigger2.getTargetDuration(), Matchers.equalTo(Duration.ofMinutes(2))); @@ -194,7 +192,7 @@ void testMultipleComplexTriggerWithWhitespaceWhenOnlyOneTemplateValid() { SmartTrigger trigger1 = out.get(0); MatcherAssert.assertThat( trigger1.getExpression(), - Matchers.equalTo("ProcessCpuLoad>0.2 ; TargetDuration>duration(\"30s\")")); + Matchers.equalTo("ProcessCpuLoad>0.2;TargetDuration>duration(\"30s\")")); MatcherAssert.assertThat(trigger1.getRecordingTemplateName(), Matchers.equalTo("profile")); MatcherAssert.assertThat( trigger1.getDurationConstraint(),