This is an automated email from the ASF dual-hosted git repository. shuber pushed a commit to branch UNOMI-188-rule-event-type-optimization in repository https://gitbox.apache.org/repos/asf/unomi.git
commit 07b3b7eafe7d17c7a39ff52e9263e33130d29c74 Author: Serge Huber <shu...@jahia.com> AuthorDate: Mon Jul 5 15:33:25 2021 +0200 UNOMI-188 Rule event type optimization - New optimization for rules : rule condition are parsed to determine the event types they handle. This is done using a new ParserHelper method that navigates the tree of conditions to resolve which eventTypeCondition are used. - Settings to deactivate the new optimization in case it causes issues - Integration tests to validate that the parsing of conditions is behaving as expected - Performance tests to validate the performance improvement of the optimization --- .../apache/unomi/api/services/RulesService.java | 34 +++++++ .../test/java/org/apache/unomi/itests/AllITs.java | 1 + .../org/apache/unomi/itests/ConditionBuilder.java | 6 +- .../org/apache/unomi/itests/RuleServiceIT.java | 109 ++++++++++++++++++++- .../main/resources/etc/custom.system.properties | 4 + .../apache/unomi/services/impl/ParserHelper.java | 57 +++++++++-- .../services/impl/rules/RulesServiceImpl.java | 66 +++++++++++-- .../resources/OSGI-INF/blueprint/blueprint.xml | 2 + .../main/resources/org.apache.unomi.services.cfg | 5 + 9 files changed, 264 insertions(+), 20 deletions(-) diff --git a/api/src/main/java/org/apache/unomi/api/services/RulesService.java b/api/src/main/java/org/apache/unomi/api/services/RulesService.java index 786ecfd..144a604 100644 --- a/api/src/main/java/org/apache/unomi/api/services/RulesService.java +++ b/api/src/main/java/org/apache/unomi/api/services/RulesService.java @@ -17,6 +17,7 @@ package org.apache.unomi.api.services; +import org.apache.unomi.api.Event; import org.apache.unomi.api.Item; import org.apache.unomi.api.Metadata; import org.apache.unomi.api.PartialList; @@ -104,4 +105,37 @@ public interface RulesService { * @return the Set of tracked conditions for the specified item */ Set<Condition> getTrackedConditions(Item item); + + /** + * Retrieves all the matching rules for a specific event + * @param event the event we want to retrieve all the matching rules for + * @return a set of rules that match the event passed in the parameters + */ + public Set<Rule> getMatchingRules(Event event); + + /** + * Refresh the rules for this instance by reloading them from the persistence backend + */ + public void refreshRules(); + + /** + * Set settings of the persistence service + * + * @param fieldName name of the field to set + * @param value value of the field to set + * @throws NoSuchFieldException if the field does not exist + * @throws IllegalAccessException field is not accessible to be changed + */ + void setSetting(String fieldName, Object value) throws NoSuchFieldException, IllegalAccessException; + + /** + * Get settings of the persistence service + * + * @param fieldName name of the field to get + * @return an object corresponding to the field that was accessed + * @throws NoSuchFieldException if the field does not exist + * @throws IllegalAccessException field is not accessible to be changed + */ + Object getSetting(String fieldName) throws NoSuchFieldException, IllegalAccessException; + } diff --git a/itests/src/test/java/org/apache/unomi/itests/AllITs.java b/itests/src/test/java/org/apache/unomi/itests/AllITs.java index 9dd9910..4642094 100644 --- a/itests/src/test/java/org/apache/unomi/itests/AllITs.java +++ b/itests/src/test/java/org/apache/unomi/itests/AllITs.java @@ -50,6 +50,7 @@ import org.junit.runners.Suite.SuiteClasses; PatchIT.class, ContextServletIT.class, SecurityIT.class, + RuleServiceIT.class, GraphQLEventIT.class, GraphQLListIT.class, GraphQLProfileIT.class, diff --git a/itests/src/test/java/org/apache/unomi/itests/ConditionBuilder.java b/itests/src/test/java/org/apache/unomi/itests/ConditionBuilder.java index 2420aed..2d01f97 100644 --- a/itests/src/test/java/org/apache/unomi/itests/ConditionBuilder.java +++ b/itests/src/test/java/org/apache/unomi/itests/ConditionBuilder.java @@ -63,6 +63,10 @@ public class ConditionBuilder { return new PropertyCondition(conditionTypeId, propertyName, definitionsService); } + public ConditionItem condition(String conditionTypeId) { + return new ConditionItem(conditionTypeId, definitionsService); + } + public abstract class ComparisonCondition extends ConditionItem { ComparisonCondition(String conditionTypeId, DefinitionsService definitionsService) { @@ -287,7 +291,7 @@ public class ConditionBuilder { } } - public abstract class ConditionItem { + public class ConditionItem { protected Condition condition; diff --git a/itests/src/test/java/org/apache/unomi/itests/RuleServiceIT.java b/itests/src/test/java/org/apache/unomi/itests/RuleServiceIT.java index ee25a09..917bea3 100644 --- a/itests/src/test/java/org/apache/unomi/itests/RuleServiceIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/RuleServiceIT.java @@ -16,25 +16,33 @@ */ package org.apache.unomi.itests; -import org.apache.unomi.api.Metadata; +import org.apache.unomi.api.*; import org.apache.unomi.api.rules.Rule; import org.apache.unomi.api.services.DefinitionsService; +import org.apache.unomi.api.services.EventService; import org.apache.unomi.api.services.RulesService; import org.apache.unomi.persistence.spi.PersistenceService; import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; +import org.ops4j.pax.exam.junit.PaxExam; +import org.ops4j.pax.exam.spi.reactors.ExamReactorStrategy; +import org.ops4j.pax.exam.spi.reactors.PerSuite; import org.ops4j.pax.exam.util.Filter; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import javax.inject.Inject; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; +import java.util.*; + +import static org.junit.Assert.*; /** * Integration tests for the Unomi rule service. */ +@RunWith(PaxExam.class) +@ExamReactorStrategy(PerSuite.class) public class RuleServiceIT extends BaseIT { private final static Logger LOGGER = LoggerFactory.getLogger(RuleServiceIT.class); @@ -48,6 +56,10 @@ public class RuleServiceIT extends BaseIT { @Inject @Filter(timeout = 600000) + protected EventService eventService; + + @Inject + @Filter(timeout = 600000) protected PersistenceService persistenceService; @Inject @@ -74,5 +86,96 @@ public class RuleServiceIT extends BaseIT { assertNull("Expected rule actions to be null", nullRule.getActions()); assertNull("Expected rule condition to be null", nullRule.getCondition()); assertEquals("Invalid rule name", TEST_RULE_ID + "_name", nullRule.getMetadata().getName()); + rulesService.removeRule(TEST_RULE_ID); + refreshPersistence(); + rulesService.refreshRules(); + } + + @Test + public void testRuleEventTypeOptimization() throws InterruptedException { + + ConditionBuilder builder = new ConditionBuilder(definitionsService); + Rule simpleEventTypeRule = new Rule(new Metadata(TEST_SCOPE, "simple-event-type-rule", "Simple event type rule", "A rule with a simple condition to match an event type")); + simpleEventTypeRule.setCondition(builder.condition("eventTypeCondition").parameter("eventTypeId", "view").build()); + rulesService.setRule(simpleEventTypeRule); + Rule complexEventTypeRule = new Rule(new Metadata(TEST_SCOPE, "complex-event-type-rule", "Complex event type rule", "A rule with a complex condition to match multiple event types with negations")); + complexEventTypeRule.setCondition( + builder.not( + builder.or( + builder.condition("eventTypeCondition").parameter( "eventTypeId", "view"), + builder.condition("eventTypeCondition").parameter("eventTypeId", "form") + ) + ).build() + ); + rulesService.setRule(complexEventTypeRule); + + refreshPersistence(); + rulesService.refreshRules(); + + Profile profile = new Profile(UUID.randomUUID().toString()); + Session session = new Session(UUID.randomUUID().toString(), profile, new Date(), TEST_SCOPE); + Event viewEvent = new Event(UUID.randomUUID().toString(), "view", session, profile, TEST_SCOPE, null, null, new Date()); + Set<Rule> matchingRules = rulesService.getMatchingRules(viewEvent); + + assertTrue("Simple rule should be matched", matchingRules.contains(simpleEventTypeRule)); + assertFalse("Complex rule should NOT be matched", matchingRules.contains(complexEventTypeRule)); + + rulesService.removeRule(simpleEventTypeRule.getItemId()); + rulesService.removeRule(complexEventTypeRule.getItemId()); + refreshPersistence(); + rulesService.refreshRules(); + } + + @Test + public void testRuleOptimizationPerf() throws NoSuchFieldException, IllegalAccessException { + Profile profile = new Profile(UUID.randomUUID().toString()); + Session session = new Session(UUID.randomUUID().toString(), profile, new Date(), TEST_SCOPE); + + rulesService.setSetting("optimizedRulesActivated", false); + + LOGGER.info("Running unoptimized rules performance test..."); + long unoptimizedRunTime = runEventTest(profile, session); + + rulesService.setSetting("optimizedRulesActivated", true); + + LOGGER.info("Running optimized rules performance test..."); + long optimizedRunTime = runEventTest(profile, session); + + LOGGER.info("Unoptimized run time = {}ms, optimized run time = {}ms. Improvement={}x", unoptimizedRunTime, optimizedRunTime, ((double) unoptimizedRunTime) / ((double) optimizedRunTime)); + assertTrue("Optimized run time should be smaller than unoptimized", unoptimizedRunTime > optimizedRunTime); + } + + private long runEventTest(Profile profile, Session session) { + Event viewEvent = generateViewEvent(session, profile); + int loopCount = 0; + long startTime = System.currentTimeMillis(); + while (loopCount < 500) { + eventService.send(viewEvent); + viewEvent = generateViewEvent(session, profile); + loopCount++; + } + return System.currentTimeMillis() - startTime; + } + + private Event generateViewEvent(Session session, Profile profile) { + CustomItem sourceItem = new CustomItem(); + sourceItem.setScope(TEST_SCOPE); + + CustomItem targetItem = new CustomItem(); + targetItem.setScope(TEST_SCOPE); + Map<String,Object> targetProperties = new HashMap<>(); + + Map<String,Object> pageInfo = new HashMap<>(); + pageInfo.put("language", "en"); + pageInfo.put("destinationURL", "https://www.acme.com/test-page.html"); + pageInfo.put("referringURL", "https://unomi.apache.org"); + pageInfo.put("pageID", "ITEM_ID_PAGE"); + pageInfo.put("pagePath", "/test-page.html"); + pageInfo.put("pageName", "Test page"); + + targetProperties.put("pageInfo", pageInfo); + + targetItem.setProperties(targetProperties); + return new Event(UUID.randomUUID().toString(), "view", session, profile, TEST_SCOPE, sourceItem, targetItem, new Date()); } } diff --git a/package/src/main/resources/etc/custom.system.properties b/package/src/main/resources/etc/custom.system.properties index 5538ada..19e4824 100644 --- a/package/src/main/resources/etc/custom.system.properties +++ b/package/src/main/resources/etc/custom.system.properties @@ -177,6 +177,10 @@ org.apache.unomi.segment.recalculate.period=${env:UNOMI_SEGMENT_RECALCULATE_PERI org.apache.unomi.rules.refresh.interval=${env:UNOMI_RULES_REFRESH_INTERVAL:-1000} # The interval in milliseconds to use to reload the rules statistics org.apache.unomi.rules.statistics.refresh.interval=${env:UNOMI_RULES_STATISTICS_REFRESH_INTERVAL:-10000} +# If this setting is active, the rules engine will try to classify the events by event type internally which makes +# rules execution a lot faster. If there are any problems detected with rules execution, you might want to try to turn +# off the optimization and file a bug report if this fixed the problem. +org.apache.unomi.rules.optimizationActivated=${env:UNOMI_RULES_OPTIMIZATION_ACTIVATED:-true} ####################################################################################################################### ## Third Party server settings ## diff --git a/services/src/main/java/org/apache/unomi/services/impl/ParserHelper.java b/services/src/main/java/org/apache/unomi/services/impl/ParserHelper.java index ef9b9de..45016cb 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/ParserHelper.java +++ b/services/src/main/java/org/apache/unomi/services/impl/ParserHelper.java @@ -48,7 +48,7 @@ public class ParserHelper { final List<String> result = new ArrayList<String>(); visitConditions(rootCondition, new ConditionVisitor() { @Override - public void visit(Condition condition) { + public void visit(Condition condition, Stack<String> conditionTypeStack) { if (condition.getConditionType() == null) { ConditionType conditionType = definitionsService.getConditionType(condition.getConditionTypeId()); if (conditionType != null) { @@ -63,7 +63,7 @@ public class ParserHelper { } } } - }); + }, new Stack<>()); return result.isEmpty(); } @@ -71,31 +71,33 @@ public class ParserHelper { final List<String> result = new ArrayList<String>(); visitConditions(rootCondition, new ConditionVisitor() { @Override - public void visit(Condition condition) { + public void visit(Condition condition, Stack<String> conditionTypeStack) { result.add(condition.getConditionTypeId()); } - }); + }, new Stack<>()); return result; } - private static void visitConditions(Condition rootCondition, ConditionVisitor visitor) { - visitor.visit(rootCondition); + private static void visitConditions(Condition rootCondition, ConditionVisitor visitor, Stack<String> conditionTypeStack) { + visitor.visit(rootCondition, conditionTypeStack); + conditionTypeStack.push(rootCondition.getConditionTypeId()); // recursive call for sub-conditions as parameters for (Object parameterValue : rootCondition.getParameterValues().values()) { if (parameterValue instanceof Condition) { Condition parameterValueCondition = (Condition) parameterValue; - visitConditions(parameterValueCondition, visitor); + visitConditions(parameterValueCondition, visitor, conditionTypeStack); } else if (parameterValue instanceof Collection) { @SuppressWarnings("unchecked") Collection<Object> valueList = (Collection<Object>) parameterValue; for (Object value : valueList) { if (value instanceof Condition) { Condition valueCondition = (Condition) value; - visitConditions(valueCondition, visitor); + visitConditions(valueCondition, visitor, conditionTypeStack); } } } } + conditionTypeStack.pop(); } public static boolean resolveActionTypes(DefinitionsService definitionsService, Rule rule) { @@ -144,6 +146,43 @@ public class ParserHelper { } interface ConditionVisitor { - void visit(Condition condition); + void visit(Condition condition, Stack<String> conditionTypeStack); + } + + public static Set<String> resolveConditionEventTypes(Condition rootCondition) { + if (rootCondition == null) { + return new HashSet<>(); + } + EventTypeConditionVisitor eventTypeConditionVisitor = new EventTypeConditionVisitor(); + visitConditions(rootCondition, eventTypeConditionVisitor, new Stack<>()); + return eventTypeConditionVisitor.getEventTypeIds(); + } + + static class EventTypeConditionVisitor implements ConditionVisitor { + + private Set<String> eventTypeIds = new HashSet<>(); + + public void visit(Condition condition, Stack<String> conditionTypeStack) { + if ("eventTypeCondition".equals(condition.getConditionTypeId())) { + String eventTypeId = (String) condition.getParameter("eventTypeId"); + if (eventTypeId == null) { + logger.warn("Null eventTypeId found!"); + } else { + // we must now check the stack to see how many notConditions we have in the parent stack + if (conditionTypeStack.contains("notCondition")) { + logger.warn("Found complex negative event type condition, will always evaluate rule"); + eventTypeIds.add("*"); + } else { + eventTypeIds.add(eventTypeId); + } + } + } else if (condition.getConditionType().getParentCondition() != null) { + visitConditions(condition.getConditionType().getParentCondition(), this, conditionTypeStack); + } + } + + public Set<String> getEventTypeIds() { + return eventTypeIds; + } } } diff --git a/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java index 4c601d2..1422eeb 100644 --- a/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java +++ b/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java @@ -36,6 +36,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; +import java.lang.reflect.Field; import java.net.URL; import java.util.*; import java.util.concurrent.ConcurrentHashMap; @@ -64,6 +65,9 @@ public class RulesServiceImpl implements RulesService, EventListenerService, Syn private List<RuleListenerService> ruleListeners = new CopyOnWriteArrayList<RuleListenerService>(); + private Map<String,Set<Rule>> rulesByEventType = new HashMap<>(); + private Boolean optimizedRulesActivated = true; + public void setBundleContext(BundleContext bundleContext) { this.bundleContext = bundleContext; } @@ -96,6 +100,10 @@ public class RulesServiceImpl implements RulesService, EventListenerService, Syn this.rulesStatisticsRefreshInterval = rulesStatisticsRefreshInterval; } + public void setOptimizedRulesActivated(Boolean optimizedRulesActivated) { + this.optimizedRulesActivated = optimizedRulesActivated; + } + public void postConstruct() { logger.debug("postConstruct {" + bundleContext.getBundle() + "}"); @@ -157,9 +165,20 @@ public class RulesServiceImpl implements RulesService, EventListenerService, Syn Boolean hasEventAlreadyBeenRaisedForSession = null; Boolean hasEventAlreadyBeenRaisedForProfile = null; - List<Rule> allItems = allRules; + Set<Rule> eventTypeRules = new HashSet<>(allRules); // local copy to avoid concurrency issues + if (optimizedRulesActivated) { + eventTypeRules = rulesByEventType.get(event.getEventType()); + if (eventTypeRules == null || eventTypeRules.isEmpty()) { + return matchedRules; + } + eventTypeRules = new HashSet<>(eventTypeRules); + Set<Rule> allEventRules = rulesByEventType.get("*"); + if (allEventRules != null && !allEventRules.isEmpty()) { + eventTypeRules.addAll(allEventRules); // retrieve rules that should always be evaluated. + } + } - for (Rule rule : allItems) { + for (Rule rule : eventTypeRules) { if (!rule.getMetadata().isEnabled()) { continue; } @@ -240,12 +259,23 @@ public class RulesServiceImpl implements RulesService, EventListenerService, Syn allRuleStatistics.put(ruleStatistics.getItemId(), ruleStatistics); } + public void refreshRules() { + try { + allRules = getAllRules(); + } catch (Throwable t) { + logger.error("Error loading rules from persistence back-end", t); + } + } + private List<Rule> getAllRules() { List<Rule> allItems = persistenceService.getAllItems(Rule.class, 0, -1, "priority").getList(); + Map<String,Set<Rule>> newRulesByEventType = new HashMap<>(); for (Rule rule : allItems) { ParserHelper.resolveConditionType(definitionsService, rule.getCondition(), "rule " + rule.getItemId()); + updateRulesByEventType(newRulesByEventType, rule); ParserHelper.resolveActionTypes(definitionsService, rule); } + this.rulesByEventType = newRulesByEventType; return allItems; } @@ -335,6 +365,7 @@ public class RulesServiceImpl implements RulesService, EventListenerService, Syn Rule rule = persistenceService.load(ruleId, Rule.class); if (rule != null) { ParserHelper.resolveConditionType(definitionsService, rule.getCondition(), "rule " + rule.getItemId()); + updateRulesByEventType(rulesByEventType, rule); ParserHelper.resolveActionTypes(definitionsService, rule); } return rule; @@ -348,6 +379,7 @@ public class RulesServiceImpl implements RulesService, EventListenerService, Syn if (condition != null) { if (rule.getMetadata().isEnabled() && !rule.getMetadata().isMissingPlugins()) { ParserHelper.resolveConditionType(definitionsService, condition, "rule " + rule.getItemId()); + updateRulesByEventType(rulesByEventType, rule); definitionsService.extractConditionBySystemTag(condition, "eventCondition"); } } @@ -381,15 +413,23 @@ public class RulesServiceImpl implements RulesService, EventListenerService, Syn persistenceService.remove(ruleId, Rule.class); } + @Override + public void setSetting(String fieldName, Object value) throws NoSuchFieldException, IllegalAccessException { + Field field = this.getClass().getDeclaredField(fieldName); + field.set(this, value); + } + + @Override + public Object getSetting(String fieldName) throws NoSuchFieldException, IllegalAccessException { + Field field = this.getClass().getDeclaredField(fieldName); + return field.get(this); + } + private void initializeTimers() { TimerTask task = new TimerTask() { @Override public void run() { - try { - allRules = getAllRules(); - } catch (Throwable t) { - logger.error("Error loading rules from persistence back-end", t); - } + refreshRules(); } }; schedulerService.getScheduleExecutorService().scheduleWithFixedDelay(task, 0,rulesRefreshInterval, TimeUnit.MILLISECONDS); @@ -507,4 +547,16 @@ public class RulesServiceImpl implements RulesService, EventListenerService, Syn } } + private void updateRulesByEventType(Map<String,Set<Rule>> rulesByEventType, Rule rule) { + Set<String> eventTypeIds = ParserHelper.resolveConditionEventTypes(rule.getCondition()); + for (String eventTypeId : eventTypeIds) { + Set<Rule> rules = rulesByEventType.get(eventTypeId); + if (rules == null) { + rules = new HashSet<>(); + } + rules.add(rule); + rulesByEventType.put(eventTypeId, rules); + } + } + } diff --git a/services/src/main/resources/OSGI-INF/blueprint/blueprint.xml b/services/src/main/resources/OSGI-INF/blueprint/blueprint.xml index d302661..55c41cb 100644 --- a/services/src/main/resources/OSGI-INF/blueprint/blueprint.xml +++ b/services/src/main/resources/OSGI-INF/blueprint/blueprint.xml @@ -43,6 +43,7 @@ <cm:property name="rules.refresh.interval" value="1000"/> <cm:property name="rules.statistics.refresh.interval" value="10000"/> <cm:property name="events.shouldBeCheckedEventSourceId" value="false"/> + <cm:property name="rules.optimizationActivated" value="true"/> </cm:default-properties> </cm:property-placeholder> @@ -165,6 +166,7 @@ <property name="schedulerService" ref="schedulerServiceImpl"/> <property name="rulesRefreshInterval" value="${services.rules.refresh.interval}"/> <property name="rulesStatisticsRefreshInterval" value="${services.rules.statistics.refresh.interval}"/> + <property name="optimizedRulesActivated" value="${services.rules.optimizationActivated}"/> </bean> <service id="rulesService" ref="rulesServiceImpl"> <interfaces> diff --git a/services/src/main/resources/org.apache.unomi.services.cfg b/services/src/main/resources/org.apache.unomi.services.cfg index a1535b4..ee23a4c 100644 --- a/services/src/main/resources/org.apache.unomi.services.cfg +++ b/services/src/main/resources/org.apache.unomi.services.cfg @@ -69,3 +69,8 @@ rules.statistics.refresh.interval=${org.apache.unomi.rules.statistics.refresh.in # The indicator should be checked is there a sourceId in the system or not events.shouldBeCheckedEventSourceId=${org.apache.unomi.events.shouldBeCheckedEventSourceId:-false} + +# If this setting is active, the rules engine will try to classify the events by event type internally which makes +# rules execution a lot faster. If there are any problems detected with rules execution, you might want to try to turn +# off the optimization and file a bug report if this fixed the problem. +rules.optimizationActivated=${org.apache.unomi.rules.optimizationActivated:-true} \ No newline at end of file