From b885ce3de1a25c2703621eef5baed3d7dc486173 Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Wed, 30 May 2018 11:57:49 +0100 Subject: [PATCH 1/8] [ML] Implement new rules design Rules allow users to supply a detector with domain knowledge that can improve the quality of the results. The model detects statistically anomalous results but it has no knowledge of the meaning of the values being modelled. For example, a detector that performs a population analysis over IP addresses could benefit from a list of IP addresses that the user knows to be safe. Then anomalous results for those IP addresses will not be created and will not affect the quantiles either. Another example would be a detector looking for anomalies in the median value of CPU utilization. A user might want to inform the detector that any results where the actual value is less than 5 is not interesting. This commit introduces a `custom_rules` field to the `Detector`. A detector may have multiple rules which are combined with `or`. A rule has 3 fields: `actions`, `scope` and `conditions`. Actions is a list of what should happen when the rule applies. The current options include `skip_result` and `skip_model_update`. The default value for `actions` is the `skip_result` action. Scope is optional and allows for applying filters on any of the partition/over/by field. When not defined the rule applies to all series. The `filter_id` needs to be specified to match the id of the filter to be used. Optionally, the `filter_type` can be specified as either `include` (default) or `exclude`. When set to `include` the rule applies to entities that are in the filter. When set to `exclude` the rule only applies to entities not in the filter. Conditions can be zero or more. A condition requires `applies_to` and `condition` to be specified. The `applied_to` value can be either `actual`, `typical` or `diff_from_typical` and it specifies the numerical value to which the `condition` applies. The `condition` has an `operator` (`lt`, `lte`, `gt`, `gte`) and a numerical `value`. Conditions are combined with `and` and allow to specify numerical conditions for then a rule applies. A rule must either have a scope or one or more conditions. Finally, a rule with scope and conditions applies when all of them apply. --- .../core/ml/calendars/ScheduledEvent.java | 4 +- .../xpack/core/ml/job/config/Condition.java | 46 +--- .../xpack/core/ml/job/config/Connective.java | 42 ---- .../core/ml/job/config/DetectionRule.java | 171 +++++-------- .../xpack/core/ml/job/config/Detector.java | 126 +++------- .../xpack/core/ml/job/config/FilterRef.java | 119 ++++++++++ .../xpack/core/ml/job/config/JobUpdate.java | 4 +- .../xpack/core/ml/job/config/Operator.java | 26 -- .../xpack/core/ml/job/config/RuleAction.java | 4 +- .../core/ml/job/config/RuleCondition.java | 202 +++------------- .../core/ml/job/config/RuleConditionType.java | 69 ------ .../xpack/core/ml/job/config/RuleScope.java | 114 +++++++++ .../xpack/core/ml/job/messages/Messages.java | 33 +-- .../ml/calendars/ScheduledEventTests.java | 15 +- .../ml/job/config/AnalysisConfigTests.java | 8 +- .../ml/job/config/DetectionRuleTests.java | 158 +++++------- .../core/ml/job/config/DetectorTests.java | 105 +++----- .../core/ml/job/config/FilterRefTests.java | 30 +++ .../core/ml/job/config/JobUpdateTests.java | 13 +- .../ml/job/config/RuleConditionTests.java | 224 +++--------------- .../core/ml/job/config/RuleScopeTests.java | 81 +++++++ .../ml/action/TransportOpenJobAction.java | 18 +- .../xpack/ml/job/JobManager.java | 1 + .../action/TransportOpenJobActionTests.java | 71 ++++++ .../xpack/ml/integration/JobProviderIT.java | 21 +- .../xpack/ml/job/JobManagerTests.java | 8 +- .../xpack/ml/job/config/ConditionTests.java | 55 +---- .../xpack/ml/job/config/ConnectiveTests.java | 79 ------ .../xpack/ml/job/config/OperatorTests.java | 76 +----- .../xpack/ml/job/config/RuleActionTests.java | 14 +- .../ml/job/config/RuleConditionTypeTests.java | 140 ----------- .../AutodetectCommunicatorTests.java | 7 +- .../ControlMsgToProcessWriterTests.java | 41 ++-- .../writer/FieldConfigWriterTests.java | 21 +- .../writer/ScheduledEventsWriterTests.java | 12 +- .../rest-api-spec/test/ml/filter_crud.yml | 12 +- .../rest-api-spec/test/ml/jobs_crud.yml | 126 ++++------ .../ml/integration/DetectionRulesIT.java | 148 +++++++----- .../ml/integration/ScheduledEventsIT.java | 4 - .../test/mixed_cluster/30_ml_jobs_crud.yml | 4 +- .../test/old_cluster/30_ml_jobs_crud.yml | 32 +-- .../test/upgraded_cluster/30_ml_jobs_crud.yml | 23 +- 42 files changed, 929 insertions(+), 1578 deletions(-) delete mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Connective.java create mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/FilterRef.java delete mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionType.java create mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleScope.java create mode 100644 x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/FilterRefTests.java create mode 100644 x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleScopeTests.java delete mode 100644 x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConnectiveTests.java delete mode 100644 x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/RuleConditionTypeTests.java diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEvent.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEvent.java index 68e1201816dc4..79e569987fa02 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEvent.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEvent.java @@ -16,7 +16,6 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.xpack.core.ml.MlMetaIndex; -import org.elasticsearch.xpack.core.ml.job.config.Connective; import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; import org.elasticsearch.xpack.core.ml.job.config.Operator; import org.elasticsearch.xpack.core.ml.job.config.RuleAction; @@ -148,8 +147,7 @@ public DetectionRule toDetectionRule(TimeValue bucketSpan) { conditions.add(RuleCondition.createTime(Operator.LT, bucketEndTime)); DetectionRule.Builder builder = new DetectionRule.Builder(conditions); - builder.setActions(RuleAction.FILTER_RESULTS, RuleAction.SKIP_SAMPLING); - builder.setConditionsConnective(Connective.AND); + builder.setActions(RuleAction.SKIP_RESULT, RuleAction.SKIP_MODEL_UPDATE); return builder.build(); } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Condition.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Condition.java index 7d3074df0ae28..6c8296327a508 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Condition.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Condition.java @@ -14,13 +14,10 @@ import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.xpack.core.ml.job.messages.Messages; import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper; import java.io.IOException; import java.util.Objects; -import java.util.regex.Pattern; -import java.util.regex.PatternSyntaxException; /** * A class that describes a condition. @@ -32,7 +29,7 @@ public class Condition implements ToXContentObject, Writeable { public static final ParseField VALUE_FIELD = new ParseField("value"); public static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( - CONDITION_FIELD.getPreferredName(), a -> new Condition((Operator) a[0], (String) a[1])); + CONDITION_FIELD.getPreferredName(), a -> new Condition((Operator) a[0], (double) a[1])); static { PARSER.declareField(ConstructingObjectParser.constructorArg(), p -> { @@ -41,52 +38,25 @@ public class Condition implements ToXContentObject, Writeable { } throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); }, Operator.OPERATOR_FIELD, ValueType.STRING); - PARSER.declareField(ConstructingObjectParser.constructorArg(), p -> { - if (p.currentToken() == XContentParser.Token.VALUE_STRING) { - return p.text(); - } - if (p.currentToken() == XContentParser.Token.VALUE_NULL) { - return null; - } - throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); - }, VALUE_FIELD, ValueType.STRING_OR_NULL); + PARSER.declareDouble(ConstructingObjectParser.constructorArg(), VALUE_FIELD); } private final Operator op; - private final String value; + private final double value; public Condition(StreamInput in) throws IOException { op = Operator.readFromStream(in); - value = in.readOptionalString(); + value = in.readDouble(); } @Override public void writeTo(StreamOutput out) throws IOException { op.writeTo(out); - out.writeOptionalString(value); + out.writeDouble(value); } - public Condition(Operator op, String value) { - if (value == null) { - throw ExceptionsHelper.badRequestException(Messages.getMessage(Messages.JOB_CONFIG_CONDITION_INVALID_VALUE_NULL)); - } - - if (op.expectsANumericArgument()) { - try { - Double.parseDouble(value); - } catch (NumberFormatException nfe) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_CONDITION_INVALID_VALUE_NUMBER, value); - throw ExceptionsHelper.badRequestException(msg); - } - } else { - try { - Pattern.compile(value); - } catch (PatternSyntaxException e) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_CONDITION_INVALID_VALUE_REGEX, value); - throw ExceptionsHelper.badRequestException(msg); - } - } - this.op = op; + public Condition(Operator op, double value) { + this.op = ExceptionsHelper.requireNonNull(op, Operator.OPERATOR_FIELD.getPreferredName()); this.value = value; } @@ -94,7 +64,7 @@ public Operator getOperator() { return op; } - public String getValue() { + public double getValue() { return value; } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Connective.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Connective.java deleted file mode 100644 index 0b4ad010fdd32..0000000000000 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Connective.java +++ /dev/null @@ -1,42 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.core.ml.job.config; - -import org.elasticsearch.common.io.stream.StreamInput; -import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.io.stream.Writeable; - -import java.io.IOException; -import java.util.Locale; - -public enum Connective implements Writeable { - OR, AND; - - /** - * Case-insensitive from string method. - * - * @param value - * String representation - * @return The connective type - */ - public static Connective fromString(String value) { - return Connective.valueOf(value.toUpperCase(Locale.ROOT)); - } - - public static Connective readFromStream(StreamInput in) throws IOException { - return in.readEnum(Connective.class); - } - - @Override - public void writeTo(StreamOutput out) throws IOException { - out.writeEnum(this); - } - - @Override - public String toString() { - return name().toLowerCase(Locale.ROOT); - } -} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRule.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRule.java index 0948e978c886e..636adf6830c7f 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRule.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRule.java @@ -6,40 +6,42 @@ package org.elasticsearch.xpack.core.ml.job.config; import org.elasticsearch.Version; -import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParseField; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.ConstructingObjectParser; +import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.ObjectParser; -import org.elasticsearch.common.xcontent.ObjectParser.ValueType; import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.xpack.core.ml.MlParserType; import org.elasticsearch.xpack.core.ml.job.messages.Messages; import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper; import java.io.IOException; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.EnumMap; import java.util.EnumSet; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.stream.Collectors; public class DetectionRule implements ToXContentObject, Writeable { + public static final Version VERSION_INTRODUCED = Version.V_6_4_0; + public static final ParseField DETECTION_RULE_FIELD = new ParseField("detection_rule"); - public static final ParseField ACTIONS_FIELD = new ParseField("actions", "rule_action"); - public static final ParseField TARGET_FIELD_NAME_FIELD = new ParseField("target_field_name"); - public static final ParseField TARGET_FIELD_VALUE_FIELD = new ParseField("target_field_value"); - public static final ParseField CONDITIONS_CONNECTIVE_FIELD = new ParseField("conditions_connective"); - public static final ParseField CONDITIONS_FIELD = new ParseField("conditions", "rule_conditions"); + public static final ParseField ACTIONS_FIELD = new ParseField("actions"); + public static final ParseField SCOPE_FIELD = new ParseField("scope"); + public static final ParseField CONDITIONS_FIELD = new ParseField("conditions"); // These parsers follow the pattern that metadata is parsed leniently (to allow for enhancements), whilst config is parsed strictly public static final ObjectParser METADATA_PARSER = @@ -55,87 +57,63 @@ public class DetectionRule implements ToXContentObject, Writeable { ObjectParser parser = PARSERS.get(parserType); assert parser != null; parser.declareStringArray(Builder::setActions, ACTIONS_FIELD); - parser.declareString(Builder::setTargetFieldName, TARGET_FIELD_NAME_FIELD); - parser.declareString(Builder::setTargetFieldValue, TARGET_FIELD_VALUE_FIELD); - parser.declareField(Builder::setConditionsConnective, p -> { - if (p.currentToken() == XContentParser.Token.VALUE_STRING) { - return Connective.fromString(p.text()); + parser.declareObject(Builder::setScope, (p, c) -> { + Map unparsedScope = p.map(); + if (unparsedScope.isEmpty()) { + return new RuleScope(); + } + ConstructingObjectParser filterRefParser = FilterRef.PARSERS.get(parserType); + Map scope = new HashMap<>(); + for (Map.Entry entry : unparsedScope.entrySet()) { + try (XContentBuilder builder = XContentFactory.jsonBuilder()) { + builder.map((Map) entry.getValue()); + try (XContentParser scopeParser = XContentFactory.xContent(builder.contentType()).createParser( + NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, Strings.toString(builder))) { + scope.put(entry.getKey(), filterRefParser.parse(scopeParser, null)); + } + } } - throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); - }, CONDITIONS_CONNECTIVE_FIELD, ValueType.STRING); + return new RuleScope(scope); + }, SCOPE_FIELD); parser.declareObjectArray(Builder::setConditions, (p, c) -> RuleCondition.PARSERS.get(parserType).apply(p, c), CONDITIONS_FIELD); } } private final EnumSet actions; - private final String targetFieldName; - private final String targetFieldValue; - private final Connective conditionsConnective; + private final RuleScope scope; private final List conditions; - private DetectionRule(EnumSet actions, @Nullable String targetFieldName, @Nullable String targetFieldValue, - Connective conditionsConnective, List conditions) { + private DetectionRule(EnumSet actions, RuleScope scope, List conditions) { this.actions = Objects.requireNonNull(actions); - this.targetFieldName = targetFieldName; - this.targetFieldValue = targetFieldValue; - this.conditionsConnective = Objects.requireNonNull(conditionsConnective); + this.scope = Objects.requireNonNull(scope); this.conditions = Collections.unmodifiableList(conditions); } public DetectionRule(StreamInput in) throws IOException { - actions = EnumSet.noneOf(RuleAction.class); - if (in.getVersion().before(Version.V_6_2_0)) { - actions.add(RuleAction.readFromStream(in)); - } else { - int actionsCount = in.readVInt(); - for (int i = 0; i < actionsCount; ++i) { - actions.add(RuleAction.readFromStream(in)); - } - } - - conditionsConnective = Connective.readFromStream(in); - int size = in.readVInt(); - conditions = new ArrayList<>(size); - for (int i = 0; i < size; i++) { - conditions.add(new RuleCondition(in)); - } - targetFieldName = in.readOptionalString(); - targetFieldValue = in.readOptionalString(); + actions = in.readEnumSet(RuleAction.class); + scope = new RuleScope(in); + conditions = in.readList(RuleCondition::new); } @Override public void writeTo(StreamOutput out) throws IOException { - if (out.getVersion().before(Version.V_6_2_0)) { - // Only filter_results is supported prior to 6.2.0 - RuleAction.FILTER_RESULTS.writeTo(out); - } else { - out.writeVInt(actions.size()); - for (RuleAction action : actions) { - action.writeTo(out); - } + if (out.getVersion().onOrAfter(Version.V_6_4_0)) { + out.writeEnumSet(actions); + scope.writeTo(out); + out.writeList(conditions); } - - conditionsConnective.writeTo(out); - out.writeVInt(conditions.size()); - for (RuleCondition condition : conditions) { - condition.writeTo(out); - } - out.writeOptionalString(targetFieldName); - out.writeOptionalString(targetFieldValue); } @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); builder.field(ACTIONS_FIELD.getPreferredName(), actions); - builder.field(CONDITIONS_CONNECTIVE_FIELD.getPreferredName(), conditionsConnective); - builder.field(CONDITIONS_FIELD.getPreferredName(), conditions); - if (targetFieldName != null) { - builder.field(TARGET_FIELD_NAME_FIELD.getPreferredName(), targetFieldName); + if (scope.isEmpty() == false) { + builder.field(SCOPE_FIELD.getPreferredName(), scope); } - if (targetFieldValue != null) { - builder.field(TARGET_FIELD_VALUE_FIELD.getPreferredName(), targetFieldValue); + if (conditions.isEmpty() == false) { + builder.field(CONDITIONS_FIELD.getPreferredName(), conditions); } builder.endObject(); return builder; @@ -145,18 +123,8 @@ public EnumSet getActions() { return actions; } - @Nullable - public String getTargetFieldName() { - return targetFieldName; - } - - @Nullable - public String getTargetFieldValue() { - return targetFieldValue; - } - - public Connective getConditionsConnective() { - return conditionsConnective; + public RuleScope getScope() { + return scope; } public List getConditions() { @@ -164,7 +132,7 @@ public List getConditions() { } public Set extractReferencedFilters() { - return conditions.stream().map(RuleCondition::getFilterId).filter(Objects::nonNull).collect(Collectors.toSet()); + return scope.getReferencedFilters(); } @Override @@ -179,29 +147,29 @@ public boolean equals(Object obj) { DetectionRule other = (DetectionRule) obj; return Objects.equals(actions, other.actions) - && Objects.equals(targetFieldName, other.targetFieldName) - && Objects.equals(targetFieldValue, other.targetFieldValue) - && Objects.equals(conditionsConnective, other.conditionsConnective) + && Objects.equals(scope, other.scope) && Objects.equals(conditions, other.conditions); } @Override public int hashCode() { - return Objects.hash(actions, targetFieldName, targetFieldValue, conditionsConnective, conditions); + return Objects.hash(actions, scope, conditions); } public static class Builder { - private EnumSet actions = EnumSet.of(RuleAction.FILTER_RESULTS); - private String targetFieldName; - private String targetFieldValue; - private Connective conditionsConnective = Connective.OR; + private EnumSet actions = EnumSet.of(RuleAction.SKIP_RESULT); + private RuleScope scope = new RuleScope(); private List conditions = Collections.emptyList(); + public Builder(RuleScope.Builder scope) { + this.scope = scope.build(); + } + public Builder(List conditions) { this.conditions = ExceptionsHelper.requireNonNull(conditions, CONDITIONS_FIELD.getPreferredName()); } - private Builder() { + Builder() { } public Builder setActions(List actions) { @@ -221,18 +189,8 @@ public Builder setActions(RuleAction... actions) { return this; } - public Builder setTargetFieldName(String targetFieldName) { - this.targetFieldName = targetFieldName; - return this; - } - - public Builder setTargetFieldValue(String targetFieldValue) { - this.targetFieldValue = targetFieldValue; - return this; - } - - public Builder setConditionsConnective(Connective connective) { - this.conditionsConnective = ExceptionsHelper.requireNonNull(connective, CONDITIONS_CONNECTIVE_FIELD.getPreferredName()); + public Builder setScope(RuleScope scope) { + this.scope = Objects.requireNonNull(scope); return this; } @@ -242,22 +200,11 @@ public Builder setConditions(List conditions) { } public DetectionRule build() { - if (targetFieldValue != null && targetFieldName == null) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_MISSING_TARGET_FIELD_NAME, targetFieldValue); - throw ExceptionsHelper.badRequestException(msg); - } - if (conditions == null || conditions.isEmpty()) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_REQUIRES_AT_LEAST_ONE_CONDITION); + if (scope.isEmpty() && conditions.isEmpty()) { + String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_REQUIRES_SCOPE_OR_CONDITION); throw ExceptionsHelper.badRequestException(msg); } - for (RuleCondition condition : conditions) { - if (condition.getType().isCategorical() && targetFieldName != null) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_CATEGORICAL_INVALID_OPTION, - DetectionRule.TARGET_FIELD_NAME_FIELD.getPreferredName()); - throw ExceptionsHelper.badRequestException(msg); - } - } - return new DetectionRule(actions, targetFieldName, targetFieldValue, conditionsConnective, conditions); + return new DetectionRule(actions, scope, conditions); } } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java index e5cf4b16f6e73..f175a4e6336ff 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java @@ -34,6 +34,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.TreeSet; import java.util.stream.Collectors; @@ -84,8 +85,7 @@ public String toString() { public static final ParseField PARTITION_FIELD_NAME_FIELD = new ParseField("partition_field_name"); public static final ParseField USE_NULL_FIELD = new ParseField("use_null"); public static final ParseField EXCLUDE_FREQUENT_FIELD = new ParseField("exclude_frequent"); - // TODO: Remove the deprecated detector_rules setting in 7.0 - public static final ParseField RULES_FIELD = new ParseField("rules", "detector_rules"); + public static final ParseField CUSTOM_RULES_FIELD = new ParseField("custom_rules"); public static final ParseField DETECTOR_INDEX = new ParseField("detector_index"); // These parsers follow the pattern that metadata is parsed leniently (to allow for enhancements), whilst config is parsed strictly @@ -113,7 +113,7 @@ public String toString() { throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); }, EXCLUDE_FREQUENT_FIELD, ObjectParser.ValueType.STRING); parser.declareObjectArray(Builder::setRules, (p, c) -> - DetectionRule.PARSERS.get(parserType).apply(p, c).build(), RULES_FIELD); + DetectionRule.PARSERS.get(parserType).apply(p, c).build(), CUSTOM_RULES_FIELD); parser.declareInt(Builder::setDetectorIndex, DETECTOR_INDEX); } } @@ -293,7 +293,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.field(EXCLUDE_FREQUENT_FIELD.getPreferredName(), excludeFrequent); } if (rules.isEmpty() == false) { - builder.field(RULES_FIELD.getPreferredName(), rules); + builder.field(CUSTOM_RULES_FIELD.getPreferredName(), rules); } // negative means "unknown", which should only happen for a 5.4 job if (detectorIndex >= 0 @@ -468,15 +468,17 @@ public int hashCode() { public static class Builder { /** - * Functions that do not support rules: + * Functions that do not support rule conditions: *
    *
  • lat_long - because it is a multivariate feature *
  • metric - because having the same conditions on min,max,mean is * error-prone + *
  • rare - because the actual/typical value is not something a user can anticipate + *
  • freq_rare - because the actual/typical value is not something a user can anticipate *
*/ - static final EnumSet FUNCTIONS_WITHOUT_RULE_SUPPORT = EnumSet.of( - DetectorFunction.LAT_LONG, DetectorFunction.METRIC); + static final EnumSet FUNCTIONS_WITHOUT_RULE_CONDITION_SUPPORT = EnumSet.of( + DetectorFunction.LAT_LONG, DetectorFunction.METRIC, DetectorFunction.RARE, DetectorFunction.FREQ_RARE); private String detectorDescription; private DetectorFunction function; @@ -598,14 +600,14 @@ public Detector build() { } DetectorFunction function = this.function == null ? DetectorFunction.METRIC : this.function; + for (DetectionRule rule : rules) { + validateRule(rule, function); + } if (rules.isEmpty() == false) { - if (FUNCTIONS_WITHOUT_RULE_SUPPORT.contains(function)) { + if (FUNCTIONS_WITHOUT_RULE_CONDITION_SUPPORT.contains(function)) { String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_NOT_SUPPORTED_BY_FUNCTION, function); throw ExceptionsHelper.badRequestException(msg); } - for (DetectionRule rule : rules) { - checkScoping(rule); - } } // partition, by and over field names cannot be duplicates @@ -691,96 +693,36 @@ private static boolean containsInvalidChar(String field) { return field.chars().anyMatch(Character::isISOControl); } - private void checkScoping(DetectionRule rule) throws ElasticsearchParseException { - String targetFieldName = rule.getTargetFieldName(); - checkTargetFieldNameIsValid(extractAnalysisFields(), targetFieldName); - for (RuleCondition condition : rule.getConditions()) { - List validOptions = Collections.emptyList(); - switch (condition.getType()) { - case CATEGORICAL: - case CATEGORICAL_COMPLEMENT: - validOptions = extractAnalysisFields(); - break; - case NUMERICAL_ACTUAL: - case NUMERICAL_TYPICAL: - case NUMERICAL_DIFF_ABS: - validOptions = getValidFieldNameOptionsForNumeric(rule); - break; - case TIME: - default: - break; - } - if (!validOptions.contains(condition.getFieldName())) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_INVALID_FIELD_NAME, validOptions, - condition.getFieldName()); - throw ExceptionsHelper.badRequestException(msg); - } - } + private void validateRule(DetectionRule rule, DetectorFunction function) { + checkFunctionHasRuleSupport(rule, function); + checkScoping(rule); } - private void checkTargetFieldNameIsValid(List analysisFields, String targetFieldName) - throws ElasticsearchParseException { - if (targetFieldName != null && !analysisFields.contains(targetFieldName)) { - String msg = - Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_INVALID_TARGET_FIELD_NAME, analysisFields, targetFieldName); + private void checkFunctionHasRuleSupport(DetectionRule rule, DetectorFunction function) { + if (ruleHasConditionOnResultValue(rule) && FUNCTIONS_WITHOUT_RULE_CONDITION_SUPPORT.contains(function)) { + String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_NOT_SUPPORTED_BY_FUNCTION, function); throw ExceptionsHelper.badRequestException(msg); } } - private List getValidFieldNameOptionsForNumeric(DetectionRule rule) { - List result = new ArrayList<>(); - if (overFieldName != null) { - result.add(byFieldName == null ? overFieldName : byFieldName); - } else if (byFieldName != null) { - result.add(byFieldName); - } - - if (rule.getTargetFieldName() != null) { - ScopingLevel targetLevel = ScopingLevel.from(this, rule.getTargetFieldName()); - result = result.stream().filter(field -> targetLevel.isHigherThan(ScopingLevel.from(this, field))) - .collect(Collectors.toList()); - } - - if (isEmptyFieldNameAllowed(rule)) { - result.add(null); - } - return result; - } - - private boolean isEmptyFieldNameAllowed(DetectionRule rule) { - List analysisFields = extractAnalysisFields(); - return analysisFields.isEmpty() || (rule.getTargetFieldName() != null && analysisFields.size() == 1); - } - - enum ScopingLevel { - PARTITION(3), - OVER(2), - BY(1); - - int level; - - ScopingLevel(int level) { - this.level = level; - } - - boolean isHigherThan(ScopingLevel other) { - return level > other.level; - } - - static ScopingLevel from(Detector.Builder detector, String fieldName) { - if (fieldName.equals(detector.partitionFieldName)) { - return ScopingLevel.PARTITION; - } - if (fieldName.equals(detector.overFieldName)) { - return ScopingLevel.OVER; - } - if (fieldName.equals(detector.byFieldName)) { - return ScopingLevel.BY; + private static boolean ruleHasConditionOnResultValue(DetectionRule rule) { + for (RuleCondition condition : rule.getConditions()) { + switch (condition.getAppliesTo()) { + case ACTUAL: + case TYPICAL: + case DIFF_FROM_TYPICAL: + return true; + case TIME: + default: + return false; } - throw ExceptionsHelper.badRequestException( - "fieldName '" + fieldName + "' does not match an analysis field"); } + return false; } + private void checkScoping(DetectionRule rule) { + Set analysisFields = new TreeSet<>(extractAnalysisFields()); + rule.getScope().validate(analysisFields); + } } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/FilterRef.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/FilterRef.java new file mode 100644 index 0000000000000..c2b1cd3384bb9 --- /dev/null +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/FilterRef.java @@ -0,0 +1,119 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.core.ml.job.config; + +import org.elasticsearch.common.ParseField; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.ConstructingObjectParser; +import org.elasticsearch.common.xcontent.ObjectParser; +import org.elasticsearch.common.xcontent.ToXContentObject; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.xpack.core.ml.MlParserType; + +import java.io.IOException; +import java.util.EnumMap; +import java.util.Locale; +import java.util.Map; +import java.util.Objects; + +public class FilterRef implements ToXContentObject, Writeable { + + public static final ParseField FILTER_REF_FIELD = new ParseField("filter_ref"); + public static final ParseField FILTER_ID = new ParseField("filter_id"); + public static final ParseField FILTER_TYPE = new ParseField("filter_type"); + + public enum FilterType { + INCLUDE, EXCLUDE; + + public static FilterType fromString(String value) { + return valueOf(value.toUpperCase(Locale.ROOT)); + } + + @Override + public String toString() { + return name().toLowerCase(Locale.ROOT); + } + } + + // These parsers follow the pattern that metadata is parsed leniently (to allow for enhancements), whilst config is parsed strictly + public static final ConstructingObjectParser METADATA_PARSER = + new ConstructingObjectParser<>(FILTER_REF_FIELD.getPreferredName(), true, + a -> new FilterRef((String) a[0], (FilterType) a[1])); + public static final ConstructingObjectParser CONFIG_PARSER = + new ConstructingObjectParser<>(FILTER_REF_FIELD.getPreferredName(), false, + a -> new FilterRef((String) a[0], (FilterType) a[1])); + public static final Map> PARSERS = new EnumMap<>(MlParserType.class); + + static { + PARSERS.put(MlParserType.METADATA, METADATA_PARSER); + PARSERS.put(MlParserType.CONFIG, CONFIG_PARSER); + for (MlParserType parserType : MlParserType.values()) { + ConstructingObjectParser parser = PARSERS.get(parserType); + assert parser != null; + parser.declareString(ConstructingObjectParser.constructorArg(), FILTER_ID); + parser.declareField(ConstructingObjectParser.optionalConstructorArg(), p -> { + if (p.currentToken() == XContentParser.Token.VALUE_STRING) { + return FilterType.fromString(p.text()); + } + throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); + }, FILTER_TYPE, ObjectParser.ValueType.STRING); + } + } + + private final String filterId; + private final FilterType filterType; + + public FilterRef(String filterId, FilterType filterType) { + this.filterId = Objects.requireNonNull(filterId); + this.filterType = filterType == null ? FilterType.INCLUDE : filterType; + } + + public FilterRef(StreamInput in) throws IOException { + filterId = in.readString(); + filterType = in.readEnum(FilterType.class); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeString(filterId); + out.writeEnum(filterType); + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.startObject(); + builder.field(FILTER_ID.getPreferredName(), filterId); + builder.field(FILTER_TYPE.getPreferredName(), filterType); + builder.endObject(); + return builder; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + + if (obj instanceof FilterRef == false) { + return false; + } + + FilterRef other = (FilterRef) obj; + return Objects.equals(filterId, other.filterId) && Objects.equals(filterType, other.filterType); + } + + @Override + public int hashCode() { + return Objects.hash(filterId, filterType); + } + + public String getFilterId() { + return filterId; + } +} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdate.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdate.java index 2c7ee538485b9..16243ed16edd4 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdate.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdate.java @@ -496,7 +496,7 @@ public static class DetectorUpdate implements Writeable, ToXContentObject { PARSER.declareInt(ConstructingObjectParser.optionalConstructorArg(), Detector.DETECTOR_INDEX); PARSER.declareStringOrNull(ConstructingObjectParser.optionalConstructorArg(), Job.DESCRIPTION); PARSER.declareObjectArray(ConstructingObjectParser.optionalConstructorArg(), (parser, parseFieldMatcher) -> - DetectionRule.CONFIG_PARSER.apply(parser, parseFieldMatcher).build(), Detector.RULES_FIELD); + DetectionRule.CONFIG_PARSER.apply(parser, parseFieldMatcher).build(), Detector.CUSTOM_RULES_FIELD); } private int detectorIndex; @@ -550,7 +550,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.field(Job.DESCRIPTION.getPreferredName(), description); } if (rules != null) { - builder.field(Detector.RULES_FIELD.getPreferredName(), rules); + builder.field(Detector.CUSTOM_RULES_FIELD.getPreferredName(), rules); } builder.endObject(); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Operator.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Operator.java index 5813a10c93bb3..8374ba0e1121b 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Operator.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Operator.java @@ -19,12 +19,6 @@ * Enum representing logical comparisons on doubles */ public enum Operator implements Writeable { - EQ { - @Override - public boolean test(double lhs, double rhs) { - return Double.compare(lhs, rhs) == 0; - } - }, GT { @Override public boolean test(double lhs, double rhs) { @@ -48,18 +42,6 @@ public boolean test(double lhs, double rhs) { public boolean test(double lhs, double rhs) { return Double.compare(lhs, rhs) <= 0; } - }, - MATCH { - @Override - public boolean match(Pattern pattern, String field) { - Matcher match = pattern.matcher(field); - return match.matches(); - } - - @Override - public boolean expectsANumericArgument() { - return false; - } }; public static final ParseField OPERATOR_FIELD = new ParseField("operator"); @@ -68,14 +50,6 @@ public boolean test(double lhs, double rhs) { return false; } - public boolean match(Pattern pattern, String field) { - return false; - } - - public boolean expectsANumericArgument() { - return true; - } - public static Operator fromString(String name) { return valueOf(name.trim().toUpperCase(Locale.ROOT)); } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleAction.java index 607961140be4e..499f3a47f0e06 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleAction.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleAction.java @@ -13,8 +13,8 @@ import java.util.Locale; public enum RuleAction implements Writeable { - FILTER_RESULTS, - SKIP_SAMPLING; + SKIP_RESULT, + SKIP_MODEL_UPDATE; /** * Case-insensitive from string method. diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleCondition.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleCondition.java index 6ca24c518d8fa..426a9b2d1bc6e 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleCondition.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleCondition.java @@ -5,7 +5,6 @@ */ package org.elasticsearch.xpack.core.ml.job.config; -import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -16,29 +15,24 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.xpack.core.ml.MlParserType; -import org.elasticsearch.xpack.core.ml.job.messages.Messages; -import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper; import java.io.IOException; import java.util.EnumMap; -import java.util.EnumSet; +import java.util.Locale; import java.util.Map; import java.util.Objects; public class RuleCondition implements ToXContentObject, Writeable { - public static final ParseField TYPE_FIELD = new ParseField("type", "condition_type"); + public static final ParseField APPLIES_TO_FIELD = new ParseField("applies_to"); public static final ParseField RULE_CONDITION_FIELD = new ParseField("rule_condition"); - public static final ParseField FIELD_NAME_FIELD = new ParseField("field_name"); - public static final ParseField FIELD_VALUE_FIELD = new ParseField("field_value"); - public static final ParseField FILTER_ID_FIELD = new ParseField(MlFilter.ID.getPreferredName(), "value_filter"); // These parsers follow the pattern that metadata is parsed leniently (to allow for enhancements), whilst config is parsed strictly public static final ConstructingObjectParser METADATA_PARSER = new ConstructingObjectParser<>(RULE_CONDITION_FIELD.getPreferredName(), true, - a -> new RuleCondition((RuleConditionType) a[0], (String) a[1], (String) a[2], (Condition) a[3], (String) a[4])); + a -> new RuleCondition((AppliesTo) a[0], (Condition) a[1])); public static final ConstructingObjectParser CONFIG_PARSER = new ConstructingObjectParser<>(RULE_CONDITION_FIELD.getPreferredName(), false, - a -> new RuleCondition((RuleConditionType) a[0], (String) a[1], (String) a[2], (Condition) a[3], (String) a[4])); + a -> new RuleCondition((AppliesTo) a[0], (Condition) a[1])); public static final Map> PARSERS = new EnumMap<>(MlParserType.class); @@ -50,113 +44,57 @@ public class RuleCondition implements ToXContentObject, Writeable { assert parser != null; parser.declareField(ConstructingObjectParser.constructorArg(), p -> { if (p.currentToken() == XContentParser.Token.VALUE_STRING) { - return RuleConditionType.fromString(p.text()); + return AppliesTo.fromString(p.text()); } throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); - }, TYPE_FIELD, ValueType.STRING); - parser.declareStringOrNull(ConstructingObjectParser.optionalConstructorArg(), FIELD_NAME_FIELD); - parser.declareStringOrNull(ConstructingObjectParser.optionalConstructorArg(), FIELD_VALUE_FIELD); + }, APPLIES_TO_FIELD, ValueType.STRING); parser.declareObject(ConstructingObjectParser.optionalConstructorArg(), Condition.PARSER, Condition.CONDITION_FIELD); - parser.declareStringOrNull(ConstructingObjectParser.optionalConstructorArg(), FILTER_ID_FIELD); } } - private final RuleConditionType type; - private final String fieldName; - private final String fieldValue; + private final AppliesTo appliesTo; private final Condition condition; - private final String filterId; public RuleCondition(StreamInput in) throws IOException { - type = RuleConditionType.readFromStream(in); + appliesTo = AppliesTo.readFromStream(in); condition = in.readOptionalWriteable(Condition::new); - fieldName = in.readOptionalString(); - fieldValue = in.readOptionalString(); - filterId = in.readOptionalString(); } @Override public void writeTo(StreamOutput out) throws IOException { - type.writeTo(out); + appliesTo.writeTo(out); out.writeOptionalWriteable(condition); - out.writeOptionalString(fieldName); - out.writeOptionalString(fieldValue); - out.writeOptionalString(filterId); } - RuleCondition(RuleConditionType type, String fieldName, String fieldValue, Condition condition, String filterId) { - this.type = type; - this.fieldName = fieldName; - this.fieldValue = fieldValue; + public RuleCondition(AppliesTo appliesTo, Condition condition) { + this.appliesTo = appliesTo; this.condition = condition; - this.filterId = filterId; - - verifyFieldsBoundToType(this); - verifyFieldValueRequiresFieldName(this); } public RuleCondition(RuleCondition ruleCondition) { - this.type = ruleCondition.type; - this.fieldName = ruleCondition.fieldName; - this.fieldValue = ruleCondition.fieldValue; + this.appliesTo = ruleCondition.appliesTo; this.condition = ruleCondition.condition; - this.filterId = ruleCondition.filterId; } @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); - builder.field(TYPE_FIELD.getPreferredName(), type); + builder.field(APPLIES_TO_FIELD.getPreferredName(), appliesTo); if (condition != null) { builder.field(Condition.CONDITION_FIELD.getPreferredName(), condition); } - if (fieldName != null) { - builder.field(FIELD_NAME_FIELD.getPreferredName(), fieldName); - } - if (fieldValue != null) { - builder.field(FIELD_VALUE_FIELD.getPreferredName(), fieldValue); - } - if (filterId != null) { - builder.field(FILTER_ID_FIELD.getPreferredName(), filterId); - } builder.endObject(); return builder; } - public RuleConditionType getType() { - return type; - } - - /** - * The field name for which the rule applies. Can be null, meaning rule - * applies to all results. - */ - public String getFieldName() { - return fieldName; - } - - /** - * The value of the field name for which the rule applies. When set, the - * rule applies only to the results that have the fieldName/fieldValue pair. - * When null, the rule applies to all values for of the specified field - * name. Only applicable when fieldName is not null. - */ - public String getFieldValue() { - return fieldValue; + public AppliesTo getAppliesTo() { + return appliesTo; } public Condition getCondition() { return condition; } - /** - * The unique identifier of a filter. Required when the rule type is - * categorical. Should be null for all other types. - */ - public String getFilterId() { - return filterId; - } - @Override public boolean equals(Object obj) { if (this == obj) { @@ -168,114 +106,40 @@ public boolean equals(Object obj) { } RuleCondition other = (RuleCondition) obj; - return Objects.equals(type, other.type) && Objects.equals(fieldName, other.fieldName) - && Objects.equals(fieldValue, other.fieldValue) && Objects.equals(condition, other.condition) - && Objects.equals(filterId, other.filterId); + return Objects.equals(appliesTo, other.appliesTo) && Objects.equals(condition, other.condition); } @Override public int hashCode() { - return Objects.hash(type, fieldName, fieldValue, condition, filterId); - } - - public static RuleCondition createCategorical(String fieldName, String filterId) { - return new RuleCondition(RuleConditionType.CATEGORICAL, fieldName, null, null, filterId); - } - - public static RuleCondition createNumerical(RuleConditionType conditionType, String fieldName, String fieldValue, - Condition condition ) { - if (conditionType.isNumerical() == false) { - throw new IllegalStateException("Rule condition type [" + conditionType + "] not valid for a numerical condition"); - } - return new RuleCondition(conditionType, fieldName, fieldValue, condition, null); + return Objects.hash(appliesTo, condition); } public static RuleCondition createTime(Operator operator, long epochSeconds) { - return new RuleCondition(RuleConditionType.TIME, null, null, new Condition(operator, Long.toString(epochSeconds)), null); - } - - private static void verifyFieldsBoundToType(RuleCondition ruleCondition) throws ElasticsearchParseException { - switch (ruleCondition.getType()) { - case CATEGORICAL: - case CATEGORICAL_COMPLEMENT: - verifyCategorical(ruleCondition); - break; - case NUMERICAL_ACTUAL: - case NUMERICAL_TYPICAL: - case NUMERICAL_DIFF_ABS: - verifyNumerical(ruleCondition); - break; - case TIME: - verifyTimeRule(ruleCondition); - break; - default: - throw new IllegalStateException(); - } + return new RuleCondition(AppliesTo.TIME, new Condition(operator, epochSeconds)); } - private static void verifyCategorical(RuleCondition ruleCondition) throws ElasticsearchParseException { - checkCategoricalHasNoField(Condition.CONDITION_FIELD.getPreferredName(), ruleCondition.getCondition()); - checkCategoricalHasNoField(RuleCondition.FIELD_VALUE_FIELD.getPreferredName(), ruleCondition.getFieldValue()); - checkCategoricalHasField(FILTER_ID_FIELD.getPreferredName(), ruleCondition.getFilterId()); - } + public enum AppliesTo implements Writeable { + ACTUAL, + TYPICAL, + DIFF_FROM_TYPICAL, + TIME; - private static void checkCategoricalHasNoField(String fieldName, Object fieldValue) throws ElasticsearchParseException { - if (fieldValue != null) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_CATEGORICAL_INVALID_OPTION, fieldName); - throw ExceptionsHelper.badRequestException(msg); + public static AppliesTo fromString(String value) { + return valueOf(value.toUpperCase(Locale.ROOT)); } - } - private static void checkCategoricalHasField(String fieldName, Object fieldValue) throws ElasticsearchParseException { - if (fieldValue == null) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_CATEGORICAL_MISSING_OPTION, fieldName); - throw ExceptionsHelper.badRequestException(msg); + public static AppliesTo readFromStream(StreamInput in) throws IOException { + return in.readEnum(AppliesTo.class); } - } - private static void verifyNumerical(RuleCondition ruleCondition) throws ElasticsearchParseException { - checkNumericalHasNoField(FILTER_ID_FIELD.getPreferredName(), ruleCondition.getFilterId()); - checkNumericalHasField(Condition.CONDITION_FIELD.getPreferredName(), ruleCondition.getCondition()); - if (ruleCondition.getFieldName() != null && ruleCondition.getFieldValue() == null) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_NUMERICAL_WITH_FIELD_NAME_REQUIRES_FIELD_VALUE); - throw ExceptionsHelper.badRequestException(msg); + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeEnum(this); } - checkNumericalConditionOparatorsAreValid(ruleCondition); - } - private static void checkNumericalHasNoField(String fieldName, Object fieldValue) throws ElasticsearchParseException { - if (fieldValue != null) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_NUMERICAL_INVALID_OPTION, fieldName); - throw ExceptionsHelper.badRequestException(msg); + @Override + public String toString() { + return name().toLowerCase(Locale.ROOT); } } - - private static void checkNumericalHasField(String fieldName, Object fieldValue) throws ElasticsearchParseException { - if (fieldValue == null) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_NUMERICAL_MISSING_OPTION, fieldName); - throw ExceptionsHelper.badRequestException(msg); - } - } - - private static void verifyFieldValueRequiresFieldName(RuleCondition ruleCondition) throws ElasticsearchParseException { - if (ruleCondition.getFieldValue() != null && ruleCondition.getFieldName() == null) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_MISSING_FIELD_NAME, - ruleCondition.getFieldValue()); - throw ExceptionsHelper.badRequestException(msg); - } - } - - static EnumSet VALID_CONDITION_OPERATORS = EnumSet.of(Operator.LT, Operator.LTE, Operator.GT, Operator.GTE); - - private static void checkNumericalConditionOparatorsAreValid(RuleCondition ruleCondition) throws ElasticsearchParseException { - Operator operator = ruleCondition.getCondition().getOperator(); - if (!VALID_CONDITION_OPERATORS.contains(operator)) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_NUMERICAL_INVALID_OPERATOR, operator); - throw ExceptionsHelper.badRequestException(msg); - } - } - - private static void verifyTimeRule(RuleCondition ruleCondition) { - checkNumericalConditionOparatorsAreValid(ruleCondition); - } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionType.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionType.java deleted file mode 100644 index aa563d001b5ca..0000000000000 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionType.java +++ /dev/null @@ -1,69 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.core.ml.job.config; - - -import org.elasticsearch.Version; -import org.elasticsearch.common.io.stream.StreamInput; -import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.io.stream.Writeable; - -import java.io.IOException; -import java.util.Locale; - -public enum RuleConditionType implements Writeable { - CATEGORICAL(false, true), - NUMERICAL_ACTUAL(true, false), - NUMERICAL_TYPICAL(true, false), - NUMERICAL_DIFF_ABS(true, false), - TIME(false, false), - CATEGORICAL_COMPLEMENT(false, true); - - private final boolean isNumerical; - private final boolean isCategorical; - - RuleConditionType(boolean isNumerical, boolean isCategorical) { - this.isNumerical = isNumerical; - this.isCategorical = isCategorical; - } - - public boolean isNumerical() { - return isNumerical; - } - - public boolean isCategorical() { - return isCategorical; - } - - /** - * Case-insensitive from string method. - * - * @param value - * String representation - * @return The condition type - */ - public static RuleConditionType fromString(String value) { - return RuleConditionType.valueOf(value.toUpperCase(Locale.ROOT)); - } - - public static RuleConditionType readFromStream(StreamInput in) throws IOException { - return in.readEnum(RuleConditionType.class); - } - - @Override - public void writeTo(StreamOutput out) throws IOException { - if (this == CATEGORICAL_COMPLEMENT && out.getVersion().before(Version.V_6_3_0)) { - out.writeEnum(CATEGORICAL); - } else { - out.writeEnum(this); - } - } - - @Override - public String toString() { - return name().toLowerCase(Locale.ROOT); - } -} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleScope.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleScope.java new file mode 100644 index 0000000000000..3b6ebbb274408 --- /dev/null +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleScope.java @@ -0,0 +1,114 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.core.ml.job.config; + +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.ToXContentObject; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.xpack.core.ml.job.messages.Messages; +import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper; + +import java.io.IOException; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; + +public class RuleScope implements ToXContentObject, Writeable { + + private final Map scope; + + public RuleScope() { + scope = Collections.emptyMap(); + } + + public RuleScope(Map scope) { + this.scope = Objects.requireNonNull(scope); + } + + public RuleScope(StreamInput in) throws IOException { + scope = in.readMap(StreamInput::readString, FilterRef::new); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeMap(scope, StreamOutput::writeString, (out1, value) -> value.writeTo(out1)); + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + return builder.map(scope); + } + + public boolean isEmpty() { + return scope.isEmpty(); + } + + public void validate(Set validKeys) { + Optional invalidKey = scope.keySet().stream().filter(k -> !validKeys.contains(k)).findFirst(); + if (invalidKey.isPresent()) { + throw ExceptionsHelper.badRequestException(Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_SCOPE_HAS_INVALID_KEY, + invalidKey.get(), validKeys)); + } + } + + public Set getReferencedFilters() { + return scope.values().stream().map(FilterRef::getFilterId).collect(Collectors.toSet()); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + + if (obj instanceof RuleScope == false) { + return false; + } + + RuleScope other = (RuleScope) obj; + return Objects.equals(scope, other.scope); + } + + @Override + public int hashCode() { + return Objects.hash(scope); + } + + public static Builder builder() { + return new Builder(); + } + + public static class Builder { + + private Map scope = new HashMap<>(); + + public Builder() {} + + public Builder(RuleScope otherScope) { + scope = new HashMap<>(otherScope.scope); + } + + public Builder exclude(String field, String filterId) { + scope.put(field, new FilterRef(filterId, FilterRef.FilterType.EXCLUDE)); + return this; + } + + public Builder include(String field, String filterId) { + scope.put(field, new FilterRef(filterId, FilterRef.FilterType.INCLUDE)); + return this; + } + + public RuleScope build() { + return new RuleScope(scope); + } + } +} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java index 7e5dc231e057a..2ad08ec29ec0b 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java @@ -88,35 +88,12 @@ public final class Messages { "categorization_filters require setting categorization_field_name"; public static final String JOB_CONFIG_CATEGORIZATION_ANALYZER_REQUIRES_CATEGORIZATION_FIELD_NAME = "categorization_analyzer requires setting categorization_field_name"; - public static final String JOB_CONFIG_CONDITION_INVALID_VALUE_NULL = "Invalid condition: the value field cannot be null"; - public static final String JOB_CONFIG_CONDITION_INVALID_VALUE_NUMBER = - "Invalid condition value: cannot parse a double from string ''{0}''"; - public static final String JOB_CONFIG_CONDITION_INVALID_VALUE_REGEX = - "Invalid condition value: ''{0}'' is not a valid regular expression"; - public static final String JOB_CONFIG_DETECTION_RULE_CONDITION_CATEGORICAL_INVALID_OPTION = - "Invalid detector rule: a categorical rule_condition does not support {0}"; - public static final String JOB_CONFIG_DETECTION_RULE_CONDITION_CATEGORICAL_MISSING_OPTION = - "Invalid detector rule: a categorical rule_condition requires {0} to be set"; - public static final String JOB_CONFIG_DETECTION_RULE_CONDITION_INVALID_FIELD_NAME = - "Invalid detector rule: field_name has to be one of {0}; actual was ''{1}''"; - public static final String JOB_CONFIG_DETECTION_RULE_CONDITION_MISSING_FIELD_NAME = - "Invalid detector rule: missing field_name in rule_condition where field_value ''{0}'' is set"; - public static final String JOB_CONFIG_DETECTION_RULE_CONDITION_NUMERICAL_INVALID_OPERATOR = - "Invalid detector rule: operator ''{0}'' is not allowed"; - public static final String JOB_CONFIG_DETECTION_RULE_CONDITION_NUMERICAL_INVALID_OPTION = - "Invalid detector rule: a numerical rule_condition does not support {0}"; - public static final String JOB_CONFIG_DETECTION_RULE_CONDITION_NUMERICAL_MISSING_OPTION = - "Invalid detector rule: a numerical rule_condition requires {0} to be set"; - public static final String JOB_CONFIG_DETECTION_RULE_CONDITION_NUMERICAL_WITH_FIELD_NAME_REQUIRES_FIELD_VALUE = - "Invalid detector rule: a numerical rule_condition with field_name requires that field_value is set"; - public static final String JOB_CONFIG_DETECTION_RULE_INVALID_TARGET_FIELD_NAME = - "Invalid detector rule: target_field_name has to be one of {0}; actual was ''{1}''"; - public static final String JOB_CONFIG_DETECTION_RULE_MISSING_TARGET_FIELD_NAME = - "Invalid detector rule: missing target_field_name where target_field_value ''{0}'' is set"; public static final String JOB_CONFIG_DETECTION_RULE_NOT_SUPPORTED_BY_FUNCTION = - "Invalid detector rule: function {0} does not support rules"; - public static final String JOB_CONFIG_DETECTION_RULE_REQUIRES_AT_LEAST_ONE_CONDITION = - "Invalid detector rule: at least one rule_condition is required"; + "Invalid detector rule: function {0} does not support conditions"; + public static final String JOB_CONFIG_DETECTION_RULE_REQUIRES_SCOPE_OR_CONDITION = + "Invalid detector rule: at least scope or a condition is required"; + public static final String JOB_CONFIG_DETECTION_RULE_SCOPE_HAS_INVALID_KEY = + "Invalid detector rule: scope key ''{0}'' is invalid; select from {1}"; public static final String JOB_CONFIG_FIELDNAME_INCOMPATIBLE_FUNCTION = "field_name cannot be used with function ''{0}''"; public static final String JOB_CONFIG_FIELD_VALUE_TOO_LOW = "{0} cannot be less than {1,number}. Value = {2,number}"; public static final String JOB_CONFIG_MODEL_MEMORY_LIMIT_TOO_LOW = "model_memory_limit must be at least 1 MiB. Value = {0,number}"; diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEventTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEventTests.java index f98eb9d5dcecc..89e72e4d24cb7 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEventTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEventTests.java @@ -11,12 +11,10 @@ import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.test.AbstractSerializingTestCase; -import org.elasticsearch.xpack.core.ml.job.config.Connective; import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; import org.elasticsearch.xpack.core.ml.job.config.Operator; import org.elasticsearch.xpack.core.ml.job.config.RuleAction; import org.elasticsearch.xpack.core.ml.job.config.RuleCondition; -import org.elasticsearch.xpack.core.ml.job.config.RuleConditionType; import org.joda.time.DateTime; import java.io.IOException; @@ -56,25 +54,22 @@ public void testToDetectionRule() { ScheduledEvent event = createTestInstance(); DetectionRule rule = event.toDetectionRule(TimeValue.timeValueSeconds(bucketSpanSecs)); - assertEquals(Connective.AND, rule.getConditionsConnective()); - assertEquals(rule.getActions(), EnumSet.of(RuleAction.FILTER_RESULTS, RuleAction.SKIP_SAMPLING)); - assertNull(rule.getTargetFieldName()); - assertNull(rule.getTargetFieldValue()); + assertEquals(rule.getActions(), EnumSet.of(RuleAction.SKIP_RESULT, RuleAction.SKIP_MODEL_UPDATE)); List conditions = rule.getConditions(); assertEquals(2, conditions.size()); - assertEquals(RuleConditionType.TIME, conditions.get(0).getType()); - assertEquals(RuleConditionType.TIME, conditions.get(1).getType()); + assertEquals(RuleCondition.AppliesTo.TIME, conditions.get(0).getAppliesTo()); + assertEquals(RuleCondition.AppliesTo.TIME, conditions.get(1).getAppliesTo()); assertEquals(Operator.GTE, conditions.get(0).getCondition().getOperator()); assertEquals(Operator.LT, conditions.get(1).getCondition().getOperator()); // Check times are aligned with the bucket - long conditionStartTime = Long.parseLong(conditions.get(0).getCondition().getValue()); + long conditionStartTime = (long) conditions.get(0).getCondition().getValue(); assertEquals(0, conditionStartTime % bucketSpanSecs); long bucketCount = conditionStartTime / bucketSpanSecs; assertEquals(bucketSpanSecs * bucketCount, conditionStartTime); - long conditionEndTime = Long.parseLong(conditions.get(1).getCondition().getValue()); + long conditionEndTime = (long) conditions.get(1).getCondition().getValue(); assertEquals(0, conditionEndTime % bucketSpanSecs); long eventTime = event.getEndTime().toEpochSecond() - conditionStartTime; diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/AnalysisConfigTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/AnalysisConfigTests.java index e64c01d7d0c3b..6c54eb78189a4 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/AnalysisConfigTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/AnalysisConfigTests.java @@ -486,11 +486,9 @@ public void testEquals_GivenDifferentCategorizationFilters() { assertFalse(config2.equals(config1)); } - public void testExtractReferencedLists() { - DetectionRule rule1 = new DetectionRule.Builder(Collections.singletonList(RuleCondition.createCategorical("foo", - "filter1"))).build(); - DetectionRule rule2 = new DetectionRule.Builder(Collections.singletonList(RuleCondition.createCategorical("foo", - "filter2"))).build(); + public void testExtractReferencedFilters() { + DetectionRule rule1 = new DetectionRule.Builder(RuleScope.builder().exclude("foo", "filter1")).build(); + DetectionRule rule2 = new DetectionRule.Builder(RuleScope.builder().exclude("foo", "filter2")).build(); Detector.Builder detector1 = new Detector.Builder("count", null); detector1.setByFieldName("foo"); detector1.setRules(Collections.singletonList(rule1)); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRuleTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRuleTests.java index 3aaf99ab730f8..4cbe3939cbce6 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRuleTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRuleTests.java @@ -5,100 +5,40 @@ */ package org.elasticsearch.xpack.core.ml.job.config; +import org.elasticsearch.ElasticsearchStatusException; import org.elasticsearch.common.io.stream.Writeable.Reader; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.test.AbstractSerializingTestCase; -import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.EnumSet; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; -public class DetectionRuleTests extends AbstractSerializingTestCase { - - public void testExtractReferencedLists() { - RuleCondition numericalCondition = - new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "field", "value", new Condition(Operator.GT, "5"), null); - List conditions = Arrays.asList( - numericalCondition, - RuleCondition.createCategorical("foo", "filter1"), - RuleCondition.createCategorical("bar", "filter2")); - - DetectionRule rule = new DetectionRule.Builder(conditions).build(); - - assertEquals(new HashSet<>(Arrays.asList("filter1", "filter2")), rule.extractReferencedFilters()); - } - - public void testEqualsGivenSameObject() { - DetectionRule rule = createFullyPopulated().build(); - assertTrue(rule.equals(rule)); - } - - public void testEqualsGivenString() { - assertFalse(createFullyPopulated().build().equals("a string")); - } +import static org.hamcrest.Matchers.equalTo; - public void testEqualsGivenDifferentTargetFieldName() { - DetectionRule rule1 = createFullyPopulated().build(); - DetectionRule rule2 = createFullyPopulated().setTargetFieldName("targetField2").build(); - assertFalse(rule1.equals(rule2)); - assertFalse(rule2.equals(rule1)); - } - - public void testEqualsGivenDifferentTargetFieldValue() { - DetectionRule rule1 = createFullyPopulated().build(); - DetectionRule rule2 = createFullyPopulated().setTargetFieldValue("targetValue2").build(); - assertFalse(rule1.equals(rule2)); - assertFalse(rule2.equals(rule1)); - } - - public void testEqualsGivenDifferentConnective() { - DetectionRule rule1 = createFullyPopulated().build(); - DetectionRule rule2 = createFullyPopulated().setConditionsConnective(Connective.OR).build(); - assertFalse(rule1.equals(rule2)); - assertFalse(rule2.equals(rule1)); - } - - public void testEqualsGivenRules() { - DetectionRule rule1 = createFullyPopulated().build(); - DetectionRule rule2 = createFullyPopulated().setConditions(createRule("10")).build(); - assertFalse(rule1.equals(rule2)); - assertFalse(rule2.equals(rule1)); - } +public class DetectionRuleTests extends AbstractSerializingTestCase { - public void testEqualsGivenEqual() { - DetectionRule rule1 = createFullyPopulated().build(); - DetectionRule rule2 = createFullyPopulated().build(); - assertTrue(rule1.equals(rule2)); - assertTrue(rule2.equals(rule1)); - assertEquals(rule1.hashCode(), rule2.hashCode()); + public void testBuildWithNeitherScopeNorCondition() { + ElasticsearchStatusException e = expectThrows(ElasticsearchStatusException.class, () -> new DetectionRule.Builder().build()); + assertThat(e.getMessage(), equalTo("Invalid detector rule: at least scope or a condition is required")); } - private static DetectionRule.Builder createFullyPopulated() { - return new DetectionRule.Builder(createRule("5")) - .setActions(EnumSet.of(RuleAction.FILTER_RESULTS, RuleAction.SKIP_SAMPLING)) - .setTargetFieldName("targetField") - .setTargetFieldValue("targetValue") - .setConditionsConnective(Connective.AND); - } + public void testExtractReferencedLists() { + DetectionRule rule = new DetectionRule.Builder(RuleScope.builder() + .exclude("foo", "filter1").include("bar", "filter2")) + .build(); - private static List createRule(String value) { - Condition condition = new Condition(Operator.GT, value); - return Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition, null)); + assertEquals(new HashSet<>(Arrays.asList("filter1", "filter2")), rule.extractReferencedFilters()); } @Override protected DetectionRule createTestInstance() { - int size = 1 + randomInt(20); - List ruleConditions = new ArrayList<>(size); - for (int i = 0; i < size; i++) { - // no need for random condition (it is already tested) - ruleConditions.addAll(createRule(Double.toString(randomDouble()))); - } - DetectionRule.Builder builder = new DetectionRule.Builder(ruleConditions); + DetectionRule.Builder builder = new DetectionRule.Builder(); if (randomBoolean()) { EnumSet actions = EnumSet.noneOf(RuleAction.class); @@ -109,13 +49,35 @@ protected DetectionRule createTestInstance() { builder.setActions(actions); } - if (randomBoolean()) { - builder.setConditionsConnective(randomFrom(Connective.values())); + boolean hasScope = randomBoolean(); + boolean hasConditions = randomBoolean(); + + if (!hasScope && !hasConditions) { + // at least one of the two should be present + if (randomBoolean()) { + hasScope = true; + } else { + hasConditions = true; + } } - if (randomBoolean()) { - builder.setTargetFieldName(randomAlphaOfLengthBetween(1, 20)); - builder.setTargetFieldValue(randomAlphaOfLengthBetween(1, 20)); + if (hasScope) { + Map scope = new HashMap<>(); + int scopeSize = randomIntBetween(1, 3); + for (int i = 0; i < scopeSize; i++) { + scope.put(randomAlphaOfLength(20), new FilterRef(randomAlphaOfLength(20), randomFrom(FilterRef.FilterType.values()))); + } + builder.setScope(new RuleScope(scope)); + } + + if (hasConditions) { + int size = randomIntBetween(1, 5); + List ruleConditions = new ArrayList<>(size); + for (int i = 0; i < size; i++) { + // no need for random condition (it is already tested) + ruleConditions.addAll(createCondition(randomDouble())); + } + builder.setConditions(ruleConditions); } return builder.build(); @@ -132,39 +94,35 @@ protected DetectionRule doParseInstance(XContentParser parser) { } @Override - protected DetectionRule mutateInstance(DetectionRule instance) throws IOException { + protected DetectionRule mutateInstance(DetectionRule instance) { List conditions = instance.getConditions(); + RuleScope scope = instance.getScope(); EnumSet actions = instance.getActions(); - String targetFieldName = instance.getTargetFieldName(); - String targetFieldValue = instance.getTargetFieldValue(); - Connective connective = instance.getConditionsConnective(); - switch (between(0, 3)) { + switch (between(0, 2)) { case 0: - conditions = new ArrayList<>(conditions); - conditions.addAll(createRule(Double.toString(randomDouble()))); + if (actions.size() == RuleAction.values().length) { + actions = EnumSet.of(randomFrom(RuleAction.values())); + } else { + actions = EnumSet.allOf(RuleAction.class); + } break; case 1: - targetFieldName = randomAlphaOfLengthBetween(5, 10); + conditions = new ArrayList<>(conditions); + conditions.addAll(createCondition(randomDouble())); break; case 2: - targetFieldValue = randomAlphaOfLengthBetween(5, 10); - if (targetFieldName == null) { - targetFieldName = randomAlphaOfLengthBetween(5, 10); - } - break; - case 3: - if (connective == Connective.AND) { - connective = Connective.OR; - } else { - connective = Connective.AND; - } + scope = new RuleScope.Builder(scope).include("another_field", "another_filter").build(); break; default: throw new AssertionError("Illegal randomisation branch"); } - return new DetectionRule.Builder(conditions).setActions(actions).setTargetFieldName(targetFieldName) - .setTargetFieldValue(targetFieldValue).setConditionsConnective(connective).build(); + return new DetectionRule.Builder(conditions).setActions(actions).setScope(scope).build(); + } + + private static List createCondition(double value) { + Condition condition = new Condition(Operator.GT, value); + return Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, condition)); } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectorTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectorTests.java index 1296928d68478..f350aa86d293d 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectorTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectorTests.java @@ -63,55 +63,46 @@ public void testEquals_GivenDifferentByFieldName() { Detector.Builder builder = new Detector.Builder(detector2); builder.setByFieldName("by2"); - Condition condition = new Condition(Operator.GT, "5"); - DetectionRule rule = new DetectionRule.Builder( - Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "by2", "val", condition, null))) - .setActions(RuleAction.FILTER_RESULTS).setTargetFieldName("over_field") - .setTargetFieldValue("targetValue") - .setConditionsConnective(Connective.AND) - .build(); - builder.setRules(Collections.singletonList(rule)); detector2 = builder.build(); assertFalse(detector1.equals(detector2)); } public void testExtractAnalysisFields() { - Detector detector = createDetector().build(); + DetectionRule rule = new DetectionRule.Builder( + Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, new Condition(Operator.GT, 5)))) + .setActions(RuleAction.SKIP_RESULT) + .build(); + Detector.Builder builder = createDetector(); + builder.setRules(Collections.singletonList(rule)); + Detector detector = builder.build(); assertEquals(Arrays.asList("by_field", "over_field", "partition"), detector.extractAnalysisFields()); - Detector.Builder builder = new Detector.Builder(detector); + builder.setPartitionFieldName(null); + detector = builder.build(); + assertEquals(Arrays.asList("by_field", "over_field"), detector.extractAnalysisFields()); + builder = new Detector.Builder(detector); - Condition condition = new Condition(Operator.GT, "5"); - DetectionRule rule = new DetectionRule.Builder( - Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition, null))) - .setActions(RuleAction.FILTER_RESULTS) - .setTargetFieldName("over_field") - .setTargetFieldValue("targetValue") - .setConditionsConnective(Connective.AND) - .build(); - builder.setRules(Collections.singletonList(rule)); builder.setByFieldName(null); + detector = builder.build(); + assertEquals(Collections.singletonList("over_field"), detector.extractAnalysisFields()); + builder = new Detector.Builder(detector); - rule = new DetectionRule.Builder( - Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition, null))) - .setActions(RuleAction.FILTER_RESULTS) - .setConditionsConnective(Connective.AND) - .build(); - builder.setRules(Collections.singletonList(rule)); builder.setOverFieldName(null); + detector = builder.build(); + assertTrue(detector.extractAnalysisFields().isEmpty()); } public void testExtractReferencedLists() { Detector.Builder builder = createDetector(); builder.setRules(Arrays.asList( - new DetectionRule.Builder(Collections.singletonList(RuleCondition.createCategorical("by_field", "list1"))).build(), - new DetectionRule.Builder(Collections.singletonList(RuleCondition.createCategorical("by_field", "list2"))).build())); + new DetectionRule.Builder(RuleScope.builder().exclude("by_field", "list1")).build(), + new DetectionRule.Builder(RuleScope.builder().exclude("by_field", "list2")).build())); Detector detector = builder.build(); assertEquals(new HashSet<>(Arrays.asList("list1", "list2")), detector.extractReferencedFilters()); @@ -139,13 +130,8 @@ private Detector.Builder createDetector() { detector.setOverFieldName("over_field"); detector.setPartitionFieldName("partition"); detector.setUseNull(true); - Condition condition = new Condition(Operator.GT, "5"); - DetectionRule rule = new DetectionRule.Builder( - Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "by_field", "val", condition, null))) - .setActions(RuleAction.FILTER_RESULTS) - .setTargetFieldName("over_field") - .setTargetFieldValue("targetValue") - .setConditionsConnective(Connective.AND) + DetectionRule rule = new DetectionRule.Builder(RuleScope.builder().exclude("partition", "partition_filter")) + .setActions(RuleAction.SKIP_RESULT) .build(); detector.setRules(Collections.singletonList(rule)); return detector; @@ -159,7 +145,7 @@ protected Detector createTestInstance() { detector = new Detector.Builder(function = randomFrom(Detector.COUNT_WITHOUT_FIELD_FUNCTIONS), null); } else { EnumSet functions = EnumSet.copyOf(Detector.FIELD_NAME_FUNCTIONS); - functions.removeAll(Detector.Builder.FUNCTIONS_WITHOUT_RULE_SUPPORT); + functions.removeAll(Detector.Builder.FUNCTIONS_WITHOUT_RULE_CONDITION_SUPPORT); detector = new Detector.Builder(function = randomFrom(functions), randomAlphaOfLengthBetween(1, 20)); } if (randomBoolean()) { @@ -181,10 +167,8 @@ protected Detector createTestInstance() { List rules = new ArrayList<>(size); for (int i = 0; i < size; i++) { // no need for random DetectionRule (it is already tested) - Condition condition = new Condition(Operator.GT, "5"); - rules.add(new DetectionRule.Builder( - Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition, null))) - .setTargetFieldName(fieldName).build()); + Condition condition = new Condition(Operator.GT, 5); + rules.add(new DetectionRule.Builder(Collections.singletonList(RuleConditionTests.createRandom())).build()); } detector.setRules(rules); } @@ -462,43 +446,26 @@ public void testVerify_GivenFunctionsThatCanHaveByField() { } } - public void testVerify_GivenInvalidRuleTargetFieldName() { - Detector.Builder detector = new Detector.Builder("mean", "metricVale"); - detector.setByFieldName("metricName"); - detector.setPartitionFieldName("instance"); - RuleCondition ruleCondition = - new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metricName", "metricVale", new Condition(Operator.LT, "5"), null); - DetectionRule rule = new DetectionRule.Builder(Collections.singletonList(ruleCondition)).setTargetFieldName("instancE").build(); - detector.setRules(Collections.singletonList(rule)); - - ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); - - assertEquals(Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_INVALID_TARGET_FIELD_NAME, - "[metricName, instance]", "instancE"), - e.getMessage()); - } - public void testVerify_GivenValidRule() { Detector.Builder detector = new Detector.Builder("mean", "metricVale"); detector.setByFieldName("metricName"); detector.setPartitionFieldName("instance"); - RuleCondition ruleCondition = - new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metricName", "CPU", new Condition(Operator.LT, "5"), null); - DetectionRule rule = new DetectionRule.Builder(Collections.singletonList(ruleCondition)).setTargetFieldName("instance").build(); + DetectionRule rule = new DetectionRule.Builder(Collections.singletonList(RuleConditionTests.createRandom())).build(); detector.setRules(Collections.singletonList(rule)); detector.build(); } - public void testVerify_GivenCategoricalRuleOnAllPartitioningFields() { + public void testVerify_GivenAllPartitioningFieldsAreScoped() { Detector.Builder detector = new Detector.Builder("count", null); detector.setPartitionFieldName("my_partition"); detector.setOverFieldName("my_over"); detector.setByFieldName("my_by"); - DetectionRule rule = new DetectionRule.Builder(Arrays.asList( - RuleCondition.createCategorical("my_partition", "my_filter_id"), - RuleCondition.createCategorical("my_over", "my_filter_id"), - RuleCondition.createCategorical("my_by", "my_filter_id") - )).build(); + + DetectionRule rule = new DetectionRule.Builder(RuleScope.builder() + .exclude("my_partition", "my_filter_id") + .exclude("my_over", "my_filter_id") + .exclude("my_by", "my_filter_id")) + .build(); detector.setRules(Collections.singletonList(rule)); detector.build(); @@ -509,16 +476,14 @@ public void testVerify_GivenCategoricalRuleOnInvalidField() { detector.setPartitionFieldName("my_partition"); detector.setOverFieldName("my_over"); detector.setByFieldName("my_by"); - DetectionRule rule = new DetectionRule.Builder(Collections.singletonList( - RuleCondition.createCategorical("my_metric", "my_filter_id") - )).build(); + + DetectionRule rule = new DetectionRule.Builder(RuleScope.builder().exclude("my_metric", "my_filter_id")).build(); detector.setRules(Collections.singletonList(rule)); ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); - assertEquals(Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_CONDITION_INVALID_FIELD_NAME, - "[my_by, my_over, my_partition]", "my_metric"), - e.getMessage()); + assertEquals(Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_SCOPE_HAS_INVALID_KEY, + "my_metric", "[my_by, my_over, my_partition]"), e.getMessage()); } public void testVerify_GivenSameByAndPartition() { diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/FilterRefTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/FilterRefTests.java new file mode 100644 index 0000000000000..241bf6593320c --- /dev/null +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/FilterRefTests.java @@ -0,0 +1,30 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.core.ml.job.config; + +import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.test.AbstractSerializingTestCase; + +import java.io.IOException; + +public class FilterRefTests extends AbstractSerializingTestCase { + + @Override + protected FilterRef createTestInstance() { + return new FilterRef(randomAlphaOfLength(20), randomFrom(FilterRef.FilterType.values())); + } + + @Override + protected FilterRef doParseInstance(XContentParser parser) throws IOException { + return FilterRef.CONFIG_PARSER.parse(parser, null); + } + + @Override + protected Writeable.Reader instanceReader() { + return FilterRef::new; + } +} \ No newline at end of file diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdateTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdateTests.java index 3663ff14e6302..23e7d929ca92c 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdateTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdateTests.java @@ -53,10 +53,9 @@ protected JobUpdate createTestInstance() { List detectionRules = null; if (randomBoolean()) { detectionRules = new ArrayList<>(); - Condition condition = new Condition(Operator.GT, "5"); + Condition condition = new Condition(Operator.GT, 5); detectionRules.add(new DetectionRule.Builder( - Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition, null))) - .setTargetFieldName("foo").build()); + Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, condition))).build()); } detectorUpdates.add(new JobUpdate.DetectorUpdate(i, detectorDescription, detectionRules)); } @@ -119,13 +118,11 @@ protected JobUpdate doParseInstance(XContentParser parser) { public void testMergeWithJob() { List detectorUpdates = new ArrayList<>(); List detectionRules1 = Collections.singletonList(new DetectionRule.Builder( - Collections.singletonList(new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, new Condition(Operator.GT, "5") - , null))) - .setTargetFieldName("mlcategory").build()); + Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, new Condition(Operator.GT, 5)))) + .build()); detectorUpdates.add(new JobUpdate.DetectorUpdate(0, "description-1", detectionRules1)); List detectionRules2 = Collections.singletonList(new DetectionRule.Builder(Collections.singletonList( - new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, new Condition(Operator.GT, "5"), null))) - .setTargetFieldName("host").build()); + new RuleCondition(RuleCondition.AppliesTo.ACTUAL, new Condition(Operator.GT, 5)))).build()); detectorUpdates.add(new JobUpdate.DetectorUpdate(1, "description-2", detectionRules2)); ModelPlotConfig modelPlotConfig = new ModelPlotConfig(randomBoolean(), randomAlphaOfLength(10)); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionTests.java index 882c590983aae..7beee0b68f1f6 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionTests.java @@ -5,36 +5,21 @@ */ package org.elasticsearch.xpack.core.ml.job.config; -import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.io.stream.Writeable.Reader; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.test.AbstractSerializingTestCase; -import org.elasticsearch.xpack.core.ml.job.messages.Messages; public class RuleConditionTests extends AbstractSerializingTestCase { @Override protected RuleCondition createTestInstance() { - Condition condition = null; - String fieldName = null; - String valueFilter = null; - String fieldValue = null; - RuleConditionType type = randomFrom(RuleConditionType.values()); - if (type.isCategorical()) { - valueFilter = randomAlphaOfLengthBetween(1, 20); - if (randomBoolean()) { - fieldName = randomAlphaOfLengthBetween(1, 20); - } - } else { - // no need to randomize, it is properly randomly tested in - // ConditionTest - condition = new Condition(Operator.LT, Long.toString(randomLong())); - if (randomBoolean()) { - fieldName = randomAlphaOfLengthBetween(1, 20); - fieldValue = randomAlphaOfLengthBetween(1, 20); - } - } - return new RuleCondition(type, fieldName, fieldValue, condition, valueFilter); + return createRandom(); + } + + public static RuleCondition createRandom() { + RuleCondition.AppliesTo appliesTo = randomFrom(RuleCondition.AppliesTo.values()); + Condition condition = new Condition(randomFrom(Operator.LT, Operator.LTE, Operator.GT, Operator.GTE), randomDouble()); + return new RuleCondition(appliesTo, condition); } @Override @@ -47,199 +32,52 @@ protected RuleCondition doParseInstance(XContentParser parser) { return RuleCondition.CONFIG_PARSER.apply(parser, null); } - public void testConstructor() { - RuleCondition condition = new RuleCondition(RuleConditionType.CATEGORICAL, null, null, null, "valueFilter"); - assertEquals(RuleConditionType.CATEGORICAL, condition.getType()); - assertNull(condition.getFieldName()); - assertNull(condition.getFieldValue()); - assertNull(condition.getCondition()); - } - public void testEqualsGivenSameObject() { - RuleCondition condition = new RuleCondition(RuleConditionType.CATEGORICAL, null, null, null, "valueFilter"); + RuleCondition condition = createRandom(); assertTrue(condition.equals(condition)); } public void testEqualsGivenString() { - assertFalse(new RuleCondition(RuleConditionType.CATEGORICAL, null, null, null, "filter").equals("a string")); - } - - public void testEqualsGivenDifferentType() { - RuleCondition condition1 = createFullyPopulated(); - RuleCondition condition2 = new RuleCondition(RuleConditionType.CATEGORICAL, null, null, null, "valueFilter"); - assertFalse(condition1.equals(condition2)); - assertFalse(condition2.equals(condition1)); - } - - public void testEqualsGivenDifferentFieldName() { - RuleCondition condition1 = createFullyPopulated(); - RuleCondition condition2 = new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metricNameaaa", "cpu", - new Condition(Operator.LT, "5"), null); - assertFalse(condition1.equals(condition2)); - assertFalse(condition2.equals(condition1)); - } - - public void testEqualsGivenDifferentFieldValue() { - RuleCondition condition1 = createFullyPopulated(); - RuleCondition condition2 = new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metricName", "cpuaaa", - new Condition(Operator.LT, "5"), null); - assertFalse(condition1.equals(condition2)); - assertFalse(condition2.equals(condition1)); - } - - public void testEqualsGivenDifferentCondition() { - RuleCondition condition1 = createFullyPopulated(); - RuleCondition condition2 = new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metricName", "cpu", - new Condition(Operator.GT, "5"), null); - assertFalse(condition1.equals(condition2)); - assertFalse(condition2.equals(condition1)); - } - - public void testEqualsGivenDifferentValueFilter() { - RuleCondition condition1 = new RuleCondition(RuleConditionType.CATEGORICAL, null, null, null, "myFilter"); - RuleCondition condition2 = new RuleCondition(RuleConditionType.CATEGORICAL, null, null, null, "myFilteraaa"); - assertFalse(condition1.equals(condition2)); - assertFalse(condition2.equals(condition1)); - } - - private static RuleCondition createFullyPopulated() { - return new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metricName", "cpu", new Condition(Operator.LT, "5"), null); - } - - public void testVerify_GivenCategoricalWithCondition() { - Condition condition = new Condition(Operator.MATCH, "text"); - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.CATEGORICAL, null, null, condition, null)); - assertEquals("Invalid detector rule: a categorical rule_condition does not support condition", e.getMessage()); - } - - public void testVerify_GivenCategoricalWithFieldValue() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.CATEGORICAL, "metric", "CPU", null, null)); - assertEquals("Invalid detector rule: a categorical rule_condition does not support field_value", e.getMessage()); - } - - public void testVerify_GivenCategoricalWithoutFilterId() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.CATEGORICAL, null, null, null, null)); - assertEquals("Invalid detector rule: a categorical rule_condition requires filter_id to be set", e.getMessage()); - } - - public void testVerify_GivenNumericalActualWithFilterId() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, null, "myFilter")); - assertEquals("Invalid detector rule: a numerical rule_condition does not support filter_id", e.getMessage()); - } - - public void testVerify_GivenNumericalActualWithoutCondition() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, null, null)); - assertEquals("Invalid detector rule: a numerical rule_condition requires condition to be set", e.getMessage()); - } - - public void testVerify_GivenNumericalActualWithFieldNameButNoFieldValue() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metric", null, new Condition(Operator.LT, "5"), null)); - assertEquals("Invalid detector rule: a numerical rule_condition with field_name requires that field_value is set", e.getMessage()); - } - - public void testVerify_GivenNumericalTypicalWithFilterId() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, null, "myFilter")); - assertEquals("Invalid detector rule: a numerical rule_condition does not support filter_id", e.getMessage()); - } - - public void testVerify_GivenNumericalTypicalWithoutCondition() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, null, null)); - assertEquals("Invalid detector rule: a numerical rule_condition requires condition to be set", e.getMessage()); - } - - public void testVerify_GivenNumericalDiffAbsWithFilterId() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_DIFF_ABS, null, null, null, "myFilter")); - assertEquals("Invalid detector rule: a numerical rule_condition does not support filter_id", e.getMessage()); - } - - public void testVerify_GivenNumericalDiffAbsWithoutCondition() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_DIFF_ABS, null, null, null, null)); - assertEquals("Invalid detector rule: a numerical rule_condition requires condition to be set", e.getMessage()); - } - - public void testVerify_GivenFieldValueWithoutFieldName() { - Condition condition = new Condition(Operator.LTE, "5"); - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_DIFF_ABS, null, "foo", condition, null)); - assertEquals("Invalid detector rule: missing field_name in rule_condition where field_value 'foo' is set", e.getMessage()); - } - - public void testVerify_GivenNumericalAndOperatorEquals() { - Condition condition = new Condition(Operator.EQ, "5"); - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition, null)); - assertEquals("Invalid detector rule: operator 'eq' is not allowed", e.getMessage()); - } - - public void testVerify_GivenNumericalAndOperatorMatch() { - Condition condition = new Condition(Operator.MATCH, "aaa"); - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition, null)); - assertEquals("Invalid detector rule: operator 'match' is not allowed", e.getMessage()); - } - - public void testVerify_GivenDetectionRuleWithInvalidCondition() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metricName", "CPU", new Condition(Operator.LT, "invalid"), - null)); - assertEquals(Messages.getMessage(Messages.JOB_CONFIG_CONDITION_INVALID_VALUE_NUMBER, "invalid"), e.getMessage()); - } - - public void testVerify_GivenValidCategorical() { - // no validation error: - new RuleCondition(RuleConditionType.CATEGORICAL, "metric", null, null, "myFilter"); - new RuleCondition(RuleConditionType.CATEGORICAL_COMPLEMENT, "metric", null, null, "myFilter"); + assertFalse(createRandom().equals("a string")); } - public void testVerify_GivenValidNumericalActual() { + public void testVerify_GivenValidActual() { // no validation error: - new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metric", "cpu", new Condition(Operator.GT, "5"), null); + new RuleCondition(RuleCondition.AppliesTo.ACTUAL, new Condition(Operator.GT, 5.0)); } - public void testVerify_GivenValidNumericalTypical() { + public void testVerify_GivenValidTypical() { // no validation error: - new RuleCondition(RuleConditionType.NUMERICAL_ACTUAL, "metric", "cpu", new Condition(Operator.GTE, "5"), null); + new RuleCondition(RuleCondition.AppliesTo.TYPICAL, new Condition(Operator.GTE, 5.0)); } - public void testVerify_GivenValidNumericalDiffAbs() { + public void testVerify_GivenValidDiffFromTypical() { // no validation error: - new RuleCondition(RuleConditionType.NUMERICAL_DIFF_ABS, "metric", "cpu", new Condition(Operator.LT, "5"), null); + new RuleCondition(RuleCondition.AppliesTo.DIFF_FROM_TYPICAL, new Condition(Operator.LT, 5.0)); } public void testCreateTimeBased() { RuleCondition timeBased = RuleCondition.createTime(Operator.GTE, 100L); - assertEquals(RuleConditionType.TIME, timeBased.getType()); + assertEquals(RuleCondition.AppliesTo.TIME, timeBased.getAppliesTo()); assertEquals(Operator.GTE, timeBased.getCondition().getOperator()); - assertEquals("100", timeBased.getCondition().getValue()); - assertNull(timeBased.getFieldName()); - assertNull(timeBased.getFieldValue()); - assertNull(timeBased.getFilterId()); + assertEquals(100.0, timeBased.getCondition().getValue(), 0.000001); } - public void testCreateTimeBased_GivenOperatorMatch() { - ElasticsearchException e = expectThrows(ElasticsearchException.class, - () -> RuleCondition.createTime(Operator.MATCH, 100L)); - assertEquals("Invalid detector rule: operator 'match' is not allowed", e.getMessage()); + public void testAppliesToFromString() { + assertEquals(RuleCondition.AppliesTo.ACTUAL, RuleCondition.AppliesTo.fromString("actual")); + assertEquals(RuleCondition.AppliesTo.ACTUAL, RuleCondition.AppliesTo.fromString("ACTUAL")); + assertEquals(RuleCondition.AppliesTo.TYPICAL, RuleCondition.AppliesTo.fromString("typical")); + assertEquals(RuleCondition.AppliesTo.TYPICAL, RuleCondition.AppliesTo.fromString("TYPICAL")); + assertEquals(RuleCondition.AppliesTo.DIFF_FROM_TYPICAL, RuleCondition.AppliesTo.fromString("diff_from_typical")); + assertEquals(RuleCondition.AppliesTo.DIFF_FROM_TYPICAL, RuleCondition.AppliesTo.fromString("DIFF_FROM_TYPICAL")); + assertEquals(RuleCondition.AppliesTo.TIME, RuleCondition.AppliesTo.fromString("time")); + assertEquals(RuleCondition.AppliesTo.TIME, RuleCondition.AppliesTo.fromString("TIME")); } - public void testCreateNumerical() { - RuleCondition ruleCondition = RuleCondition.createNumerical(RuleConditionType.NUMERICAL_ACTUAL, "foo", "bar", - new Condition(Operator.GTE, "100")); - assertEquals(RuleConditionType.NUMERICAL_ACTUAL, ruleCondition.getType()); - assertEquals(Operator.GTE, ruleCondition.getCondition().getOperator()); - assertEquals("100", ruleCondition.getCondition().getValue()); - assertEquals("foo", ruleCondition.getFieldName()); - assertEquals("bar", ruleCondition.getFieldValue()); - assertNull(ruleCondition.getFilterId()); + public void testAppliesToToString() { + assertEquals("actual", RuleCondition.AppliesTo.ACTUAL.toString()); + assertEquals("typical", RuleCondition.AppliesTo.TYPICAL.toString()); + assertEquals("diff_from_typical", RuleCondition.AppliesTo.DIFF_FROM_TYPICAL.toString()); + assertEquals("time", RuleCondition.AppliesTo.TIME.toString()); } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleScopeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleScopeTests.java new file mode 100644 index 0000000000000..f61fee7ac772b --- /dev/null +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleScopeTests.java @@ -0,0 +1,81 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.core.ml.job.config; + +import org.elasticsearch.ElasticsearchStatusException; +import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.util.set.Sets; +import org.elasticsearch.test.AbstractWireSerializingTestCase; + +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; + +public class RuleScopeTests extends AbstractWireSerializingTestCase { + + @Override + protected RuleScope createTestInstance() { + RuleScope.Builder scope = RuleScope.builder(); + int count = randomIntBetween(0, 3); + for (int i = 0; i < count; ++i) { + if (randomBoolean()) { + scope.include(randomAlphaOfLength(20), randomAlphaOfLength(20)); + } else { + scope.exclude(randomAlphaOfLength(20), randomAlphaOfLength(20)); + } + } + return scope.build(); + } + + @Override + protected Writeable.Reader instanceReader() { + return RuleScope::new; + } + + public void testValidate_GivenEmpty() { + RuleScope scope = RuleScope.builder().build(); + assertThat(scope.isEmpty(), is(true)); + + scope.validate(Sets.newHashSet("a", "b")); + } + + public void testValidate_GivenMultipleValidFields() { + RuleScope scope = RuleScope.builder() + .include("foo", "filter1") + .exclude("bar", "filter2") + .include("foobar", "filter3") + .build(); + assertThat(scope.isEmpty(), is(false)); + + scope.validate(Sets.newHashSet("foo", "bar", "foobar")); + } + + public void testValidate_GivenMultipleFieldsIncludingInvalid() { + RuleScope scope = RuleScope.builder() + .include("foo", "filter1") + .exclude("bar", "filter2") + .include("foobar", "filter3") + .build(); + assertThat(scope.isEmpty(), is(false)); + + ElasticsearchStatusException e = expectThrows(ElasticsearchStatusException.class, + () -> scope.validate(Sets.newHashSet("foo", "foobar"))); + assertThat(e.getMessage(), equalTo("Invalid detector rule: scope key 'bar' is invalid; select from [foo, foobar]")); + } + + public void testGetReferencedFilters_GivenEmpty() { + assertThat(RuleScope.builder().build().getReferencedFilters().isEmpty(), is(true)); + } + + public void testGetReferencedFilters_GivenMultipleFields() { + RuleScope scope = RuleScope.builder() + .include("foo", "filter1") + .exclude("bar", "filter2") + .include("foobar", "filter3") + .build(); + assertThat(scope.getReferencedFilters(), contains("filter1", "filter2", "filter3")); + } +} \ No newline at end of file diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java index f783dfbddc35a..3c44ca235efdd 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java @@ -54,6 +54,7 @@ import org.elasticsearch.xpack.core.ml.action.OpenJobAction; import org.elasticsearch.xpack.core.ml.action.PutJobAction; import org.elasticsearch.xpack.core.ml.action.UpdateJobAction; +import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; import org.elasticsearch.xpack.core.ml.job.config.Job; import org.elasticsearch.xpack.core.ml.job.config.JobState; import org.elasticsearch.xpack.core.ml.job.config.JobTaskStatus; @@ -646,9 +647,12 @@ public PersistentTasksCustomMetaData.Assignment getAssignment(OpenJobAction.JobP @Override public void validate(OpenJobAction.JobParams params, ClusterState clusterState) { + + TransportOpenJobAction.validate(params.getJobId(), MlMetadata.getMlMetadata(clusterState)); + checkJobWithRulesRequiresMinVersionOnAllNodes(params.getJobId(), clusterState); + // If we already know that we can't find an ml node because all ml nodes are running at capacity or // simply because there are no ml nodes in the cluster then we fail quickly here: - TransportOpenJobAction.validate(params.getJobId(), MlMetadata.getMlMetadata(clusterState)); PersistentTasksCustomMetaData.Assignment assignment = selectLeastLoadedMlNode(params.getJobId(), clusterState, maxConcurrentJobAllocations, fallbackMaxNumberOfOpenJobs, maxMachineMemoryPercent, logger); if (assignment.getExecutorNode() == null) { @@ -700,6 +704,18 @@ void setMaxMachineMemoryPercent(int maxMachineMemoryPercent) { } } + // Visible for testing + static void checkJobWithRulesRequiresMinVersionOnAllNodes(String jobId, ClusterState clusterState) { + // Jobs with rules should not be allowed to open unless the whole cluster has the min required version + Job job = MlMetadata.getMlMetadata(clusterState).getJobs().get(jobId); + if (job.getAnalysisConfig().getDetectors().stream().anyMatch(d -> d.getRules().isEmpty() == false) + && clusterState.nodes().getMinNodeVersion().before(DetectionRule.VERSION_INTRODUCED)) { + String msg = "Cannot open job [" + job.getId() + "] because jobs using custom_rules require all nodes to be " + + "on version [" + DetectionRule.VERSION_INTRODUCED + "] or higher"; + throw ExceptionsHelper.badRequestException(msg); + } + } + public static class JobTask extends AllocatedPersistentTask implements OpenJobAction.JobTaskMatcher { private static final Logger LOGGER = Loggers.getLogger(JobTask.class); diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java index 2042d917f6057..1fd73b96667b2 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java @@ -181,6 +181,7 @@ public void putJob(PutJobAction.Request request, AnalysisRegistry analysisRegist request.getJobBuilder().validateCategorizationAnalyzer(analysisRegistry, environment); Job job = request.getJobBuilder().build(new Date()); + if (job.getDataDescription() != null && job.getDataDescription().getFormat() == DataDescription.DataFormat.DELIMITED) { DEPRECATION_LOGGER.deprecated("Creating jobs with delimited data format is deprecated. Please use xcontent instead."); } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportOpenJobActionTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportOpenJobActionTests.java index 5ee07d63a968b..f2a67f81364d2 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportOpenJobActionTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportOpenJobActionTests.java @@ -36,9 +36,16 @@ import org.elasticsearch.xpack.core.ml.MlMetaIndex; import org.elasticsearch.xpack.core.ml.MlMetadata; import org.elasticsearch.xpack.core.ml.action.OpenJobAction; +import org.elasticsearch.xpack.core.ml.job.config.AnalysisConfig; +import org.elasticsearch.xpack.core.ml.job.config.Condition; +import org.elasticsearch.xpack.core.ml.job.config.DataDescription; +import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; +import org.elasticsearch.xpack.core.ml.job.config.Detector; import org.elasticsearch.xpack.core.ml.job.config.Job; import org.elasticsearch.xpack.core.ml.job.config.JobState; import org.elasticsearch.xpack.core.ml.job.config.JobTaskStatus; +import org.elasticsearch.xpack.core.ml.job.config.Operator; +import org.elasticsearch.xpack.core.ml.job.config.RuleCondition; import org.elasticsearch.xpack.core.ml.job.persistence.AnomalyDetectorsIndex; import org.elasticsearch.xpack.core.ml.job.persistence.AnomalyDetectorsIndexFields; import org.elasticsearch.xpack.core.ml.job.persistence.ElasticsearchMappings; @@ -49,6 +56,7 @@ import java.io.IOException; import java.net.InetAddress; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.Date; import java.util.HashMap; @@ -60,6 +68,7 @@ import static org.elasticsearch.xpack.core.ml.job.config.JobTests.buildJobBuilder; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -563,6 +572,68 @@ public void testNodeNameAndMlAttributes() { assertEquals("{_node_name1}{ml.machine_memory=5}{node.ml=true}", TransportOpenJobAction.nodeNameAndMlAttributes(node)); } + public void testCheckJobWithRulesRequiresMinVersionOnAllNodes_GivenNodeDoesNotMeetRequiredVersion() { + DetectionRule rule = new DetectionRule.Builder(Arrays.asList( + new RuleCondition(RuleCondition.AppliesTo.TYPICAL, new Condition(Operator.LT, 100.0)) + )).build(); + + Detector.Builder detector = new Detector.Builder("count", null); + detector.setRules(Arrays.asList(rule)); + AnalysisConfig.Builder analysisConfig = new AnalysisConfig.Builder(Arrays.asList(detector.build())); + DataDescription.Builder dataDescription = new DataDescription.Builder(); + Job.Builder job = new Job.Builder("foo"); + job.setAnalysisConfig(analysisConfig); + job.setDataDescription(dataDescription); + + MetaData metaData = MetaData.builder() + .putCustom(MLMetadataField.TYPE, new MlMetadata.Builder().putJob(job.build(new Date()), false).build()) + .build(); + + ClusterState cs = ClusterState.builder(new ClusterName("_name")) + .nodes(DiscoveryNodes.builder() + .add(new DiscoveryNode("_old_node", new TransportAddress(InetAddress.getLoopbackAddress(), 9200), Version.V_6_3_0)) + .add(new DiscoveryNode("_new_node", new TransportAddress(InetAddress.getLoopbackAddress(), 9201), Version.CURRENT)) + .localNodeId("_new_node") + .masterNodeId("_new_node")) + .metaData(metaData) + .build(); + + ElasticsearchStatusException e = expectThrows(ElasticsearchStatusException.class, + () -> TransportOpenJobAction.checkJobWithRulesRequiresMinVersionOnAllNodes("foo", cs)); + + assertThat(e.getMessage(), equalTo("Cannot open job [foo] because jobs using custom_rules " + + "require all nodes to be on version [6.4.0] or higher")); + } + + public void testCheckJobWithRulesRequiresMinVersionOnAllNodes_GivenAllNodesMeetRequiredVersion() { + DetectionRule rule = new DetectionRule.Builder(Arrays.asList( + new RuleCondition(RuleCondition.AppliesTo.TYPICAL, new Condition(Operator.LT, 100.0)) + )).build(); + + Detector.Builder detector = new Detector.Builder("count", null); + detector.setRules(Arrays.asList(rule)); + AnalysisConfig.Builder analysisConfig = new AnalysisConfig.Builder(Arrays.asList(detector.build())); + DataDescription.Builder dataDescription = new DataDescription.Builder(); + Job.Builder job = new Job.Builder("foo"); + job.setAnalysisConfig(analysisConfig); + job.setDataDescription(dataDescription); + + MetaData metaData = MetaData.builder() + .putCustom(MLMetadataField.TYPE, new MlMetadata.Builder().putJob(job.build(new Date()), false).build()) + .build(); + + ClusterState cs = ClusterState.builder(new ClusterName("_name")) + .nodes(DiscoveryNodes.builder() + .add(new DiscoveryNode("_old_node", new TransportAddress(InetAddress.getLoopbackAddress(), 9200), Version.V_6_4_0)) + .add(new DiscoveryNode("_new_node", new TransportAddress(InetAddress.getLoopbackAddress(), 9201), Version.CURRENT)) + .localNodeId("_new_node") + .masterNodeId("_new_node")) + .metaData(metaData) + .build(); + + TransportOpenJobAction.checkJobWithRulesRequiresMinVersionOnAllNodes("foo", cs); + } + public static void addJobTask(String jobId, String nodeId, JobState jobState, PersistentTasksCustomMetaData.Builder builder) { builder.addTask(MlMetadata.jobTaskId(jobId), OpenJobAction.TASK_NAME, new OpenJobAction.JobParams(jobId), new Assignment(nodeId, "test assignment")); diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java index 120f04e95e70b..6086860a44edc 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java @@ -27,29 +27,28 @@ import org.elasticsearch.xpack.core.ml.calendars.Calendar; import org.elasticsearch.xpack.core.ml.calendars.ScheduledEvent; import org.elasticsearch.xpack.core.ml.job.config.AnalysisConfig; -import org.elasticsearch.xpack.core.ml.job.config.Connective; import org.elasticsearch.xpack.core.ml.job.config.DataDescription; import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; import org.elasticsearch.xpack.core.ml.job.config.Detector; import org.elasticsearch.xpack.core.ml.job.config.Job; import org.elasticsearch.xpack.core.ml.job.config.MlFilter; import org.elasticsearch.xpack.core.ml.job.config.RuleAction; -import org.elasticsearch.xpack.core.ml.job.config.RuleCondition; +import org.elasticsearch.xpack.core.ml.job.config.RuleScope; import org.elasticsearch.xpack.core.ml.job.persistence.AnomalyDetectorsIndex; -import org.elasticsearch.xpack.ml.job.persistence.CalendarQueryBuilder; -import org.elasticsearch.xpack.ml.job.persistence.ScheduledEventsQueryBuilder; -import org.elasticsearch.xpack.ml.job.process.autodetect.params.AutodetectParams; import org.elasticsearch.xpack.core.ml.job.process.autodetect.state.DataCounts; +import org.elasticsearch.xpack.core.ml.job.process.autodetect.state.DataCountsTests; import org.elasticsearch.xpack.core.ml.job.process.autodetect.state.ModelSizeStats; import org.elasticsearch.xpack.core.ml.job.process.autodetect.state.ModelSnapshot; import org.elasticsearch.xpack.core.ml.job.process.autodetect.state.Quantiles; import org.elasticsearch.xpack.ml.LocalStateMachineLearning; import org.elasticsearch.xpack.ml.MachineLearning; import org.elasticsearch.xpack.ml.MlSingleNodeTestCase; +import org.elasticsearch.xpack.ml.job.persistence.CalendarQueryBuilder; import org.elasticsearch.xpack.ml.job.persistence.JobDataCountsPersister; import org.elasticsearch.xpack.ml.job.persistence.JobProvider; import org.elasticsearch.xpack.ml.job.persistence.JobResultsPersister; -import org.elasticsearch.xpack.core.ml.job.process.autodetect.state.DataCountsTests; +import org.elasticsearch.xpack.ml.job.persistence.ScheduledEventsQueryBuilder; +import org.elasticsearch.xpack.ml.job.process.autodetect.params.AutodetectParams; import org.junit.Before; import java.io.IOException; @@ -513,16 +512,12 @@ private AnalysisConfig.Builder createAnalysisConfig(List filterIds) { detector.setByFieldName("by_field"); if (!filterIds.isEmpty()) { - List conditions = new ArrayList<>(); - + RuleScope.Builder ruleScope = RuleScope.builder(); for (String filterId : filterIds) { - conditions.add(RuleCondition.createCategorical("by_field", filterId)); + ruleScope.exclude("by_field", filterId); } - DetectionRule.Builder rule = new DetectionRule.Builder(conditions) - .setActions(RuleAction.FILTER_RESULTS) - .setConditionsConnective(Connective.OR); - + DetectionRule.Builder rule = new DetectionRule.Builder(ruleScope).setActions(RuleAction.SKIP_RESULT); detector.setRules(Collections.singletonList(rule.build())); } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java index 64b3bfbab45c8..454f941d6c8b0 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/JobManagerTests.java @@ -19,6 +19,7 @@ import org.elasticsearch.env.Environment; import org.elasticsearch.env.TestEnvironment; import org.elasticsearch.index.analysis.AnalysisRegistry; +import org.elasticsearch.persistent.PersistentTasksCustomMetaData; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.core.ml.MLMetadataField; import org.elasticsearch.xpack.core.ml.MachineLearningField; @@ -32,8 +33,7 @@ import org.elasticsearch.xpack.core.ml.job.config.Job; import org.elasticsearch.xpack.core.ml.job.config.JobState; import org.elasticsearch.xpack.core.ml.job.config.MlFilter; -import org.elasticsearch.xpack.core.ml.job.config.RuleCondition; -import org.elasticsearch.persistent.PersistentTasksCustomMetaData; +import org.elasticsearch.xpack.core.ml.job.config.RuleScope; import org.elasticsearch.xpack.ml.job.categorization.CategorizationAnalyzerTests; import org.elasticsearch.xpack.ml.job.persistence.JobProvider; import org.elasticsearch.xpack.ml.job.process.autodetect.UpdateParams; @@ -177,9 +177,7 @@ public void onFailure(Exception e) { public void testUpdateProcessOnFilterChanged() { Detector.Builder detectorReferencingFilter = new Detector.Builder("count", null); detectorReferencingFilter.setByFieldName("foo"); - RuleCondition.createCategorical("foo", "foo_filter"); - DetectionRule filterRule = new DetectionRule.Builder(Collections.singletonList( - RuleCondition.createCategorical("foo", "foo_filter"))).build(); + DetectionRule filterRule = new DetectionRule.Builder(RuleScope.builder().exclude("foo", "foo_filter")).build(); detectorReferencingFilter.setRules(Collections.singletonList(filterRule)); AnalysisConfig.Builder filterAnalysisConfig = new AnalysisConfig.Builder(Collections.singletonList( detectorReferencingFilter.build())); diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConditionTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConditionTests.java index 09bae9fb8cb9b..970152d998748 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConditionTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConditionTests.java @@ -5,51 +5,26 @@ */ package org.elasticsearch.xpack.ml.job.config; -import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.io.stream.Writeable.Reader; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.test.AbstractSerializingTestCase; -import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.core.ml.job.config.Condition; import org.elasticsearch.xpack.core.ml.job.config.Operator; -import org.elasticsearch.xpack.core.ml.job.messages.Messages; import java.io.IOException; public class ConditionTests extends AbstractSerializingTestCase { - public void testSetValues() { - Condition cond = new Condition(Operator.EQ, "5"); - assertEquals(Operator.EQ, cond.getOperator()); - assertEquals("5", cond.getValue()); - } - - public void testHashCodeAndEquals() { - Condition cond1 = new Condition(Operator.MATCH, "regex"); - Condition cond2 = new Condition(Operator.MATCH, "regex"); - - assertEquals(cond1, cond2); - assertEquals(cond1.hashCode(), cond2.hashCode()); - - Condition cond3 = new Condition(Operator.EQ, "5"); - assertFalse(cond1.equals(cond3)); - assertFalse(cond1.hashCode() == cond3.hashCode()); - } - @Override protected Condition createTestInstance() { Operator op = randomFrom(Operator.values()); Condition condition; switch (op) { - case EQ: case GT: case GTE: case LT: case LTE: - condition = new Condition(op, Double.toString(randomDouble())); - break; - case MATCH: - condition = new Condition(op, randomAlphaOfLengthBetween(1, 20)); + condition = new Condition(op, randomDouble()); break; default: throw new AssertionError("Unknown operator selected: " + op); @@ -67,44 +42,20 @@ protected Condition doParseInstance(XContentParser parser) { return Condition.PARSER.apply(parser, null); } - public void testVerifyArgsNumericArgs() { - new Condition(Operator.LTE, "100"); - new Condition(Operator.GT, "10.0"); - } - - public void testVerify_GivenEmptyValue() { - ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, () -> new Condition(Operator.LT, "")); - assertEquals(Messages.getMessage(Messages.JOB_CONFIG_CONDITION_INVALID_VALUE_NUMBER, ""), e.getMessage()); - } - - public void testVerify_GivenInvalidRegex() { - ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, () -> new Condition(Operator.MATCH, "[*")); - assertEquals(Messages.getMessage(Messages.JOB_CONFIG_CONDITION_INVALID_VALUE_REGEX, "[*"), e.getMessage()); - } - - public void testVerify_GivenNullRegex() { - ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, - () -> new Condition(Operator.MATCH, null)); - assertEquals(Messages.getMessage(Messages.JOB_CONFIG_CONDITION_INVALID_VALUE_NULL, "[*"), e.getMessage()); - } - @Override protected Condition mutateInstance(Condition instance) throws IOException { Operator op = instance.getOperator(); - String value = instance.getValue(); + double value = instance.getValue(); switch (between(0, 1)) { case 0: Operator newOp = op; while (newOp == op) { newOp = randomFrom(Operator.values()); } - if (op == Operator.MATCH && newOp != Operator.MATCH) { - value = Double.toString(randomDouble()); - } op = newOp; break; case 1: - value = Double.toString(randomDouble()); + value = randomDouble(); break; default: throw new AssertionError("Illegal randomisation branch"); diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConnectiveTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConnectiveTests.java deleted file mode 100644 index c8f9b6d36cbca..0000000000000 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConnectiveTests.java +++ /dev/null @@ -1,79 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.ml.job.config; - -import org.elasticsearch.common.io.stream.BytesStreamOutput; -import org.elasticsearch.common.io.stream.StreamInput; -import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.xpack.core.ml.job.config.Connective; - -import java.io.IOException; - -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.equalTo; - -public class ConnectiveTests extends ESTestCase { - - public void testForString() { - assertEquals(Connective.OR, Connective.fromString("or")); - assertEquals(Connective.OR, Connective.fromString("OR")); - assertEquals(Connective.AND, Connective.fromString("and")); - assertEquals(Connective.AND, Connective.fromString("AND")); - } - - public void testToString() { - assertEquals("or", Connective.OR.toString()); - assertEquals("and", Connective.AND.toString()); - } - - public void testValidOrdinals() { - assertThat(Connective.OR.ordinal(), equalTo(0)); - assertThat(Connective.AND.ordinal(), equalTo(1)); - } - - public void testwriteTo() throws Exception { - try (BytesStreamOutput out = new BytesStreamOutput()) { - Connective.OR.writeTo(out); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(0)); - } - } - - try (BytesStreamOutput out = new BytesStreamOutput()) { - Connective.AND.writeTo(out); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(1)); - } - } - } - - public void testReadFrom() throws Exception { - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(0); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(Connective.readFromStream(in), equalTo(Connective.OR)); - } - } - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(1); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(Connective.readFromStream(in), equalTo(Connective.AND)); - } - } - } - - public void testInvalidReadFrom() throws Exception { - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(randomIntBetween(2, Integer.MAX_VALUE)); - try (StreamInput in = out.bytes().streamInput()) { - Connective.readFromStream(in); - fail("Expected IOException"); - } catch (IOException e) { - assertThat(e.getMessage(), containsString("Unknown Connective ordinal [")); - } - } - } -} diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/OperatorTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/OperatorTests.java index 07f4d4c5edfa2..469dbf822e005 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/OperatorTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/OperatorTests.java @@ -11,7 +11,6 @@ import org.elasticsearch.xpack.core.ml.job.config.Operator; import java.io.IOException; -import java.util.regex.Pattern; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; @@ -19,110 +18,63 @@ public class OperatorTests extends ESTestCase { public void testFromString() { - assertEquals(Operator.fromString("eq"), Operator.EQ); assertEquals(Operator.fromString("gt"), Operator.GT); assertEquals(Operator.fromString("gte"), Operator.GTE); assertEquals(Operator.fromString("lte"), Operator.LTE); assertEquals(Operator.fromString("lt"), Operator.LT); - assertEquals(Operator.fromString("match"), Operator.MATCH); assertEquals(Operator.fromString("Gt"), Operator.GT); - assertEquals(Operator.fromString("EQ"), Operator.EQ); assertEquals(Operator.fromString("GTE"), Operator.GTE); - assertEquals(Operator.fromString("Match"), Operator.MATCH); } public void testToString() { - assertEquals("eq", Operator.EQ.toString()); assertEquals("gt", Operator.GT.toString()); assertEquals("gte", Operator.GTE.toString()); assertEquals("lte", Operator.LTE.toString()); assertEquals("lt", Operator.LT.toString()); - assertEquals("match", Operator.MATCH.toString()); } public void testTest() { - assertTrue(Operator.GT.expectsANumericArgument()); assertTrue(Operator.GT.test(1.0, 0.0)); assertFalse(Operator.GT.test(0.0, 1.0)); - assertTrue(Operator.GTE.expectsANumericArgument()); assertTrue(Operator.GTE.test(1.0, 0.0)); assertTrue(Operator.GTE.test(1.0, 1.0)); assertFalse(Operator.GTE.test(0.0, 1.0)); - assertTrue(Operator.EQ.expectsANumericArgument()); - assertTrue(Operator.EQ.test(0.0, 0.0)); - assertFalse(Operator.EQ.test(1.0, 0.0)); - - assertTrue(Operator.LT.expectsANumericArgument()); assertTrue(Operator.LT.test(0.0, 1.0)); assertFalse(Operator.LT.test(0.0, 0.0)); - assertTrue(Operator.LTE.expectsANumericArgument()); assertTrue(Operator.LTE.test(0.0, 1.0)); assertTrue(Operator.LTE.test(1.0, 1.0)); assertFalse(Operator.LTE.test(1.0, 0.0)); } - public void testMatch() { - assertFalse(Operator.MATCH.expectsANumericArgument()); - assertFalse(Operator.MATCH.test(0.0, 1.0)); - - Pattern pattern = Pattern.compile("^aa.*"); - - assertTrue(Operator.MATCH.match(pattern, "aaaaa")); - assertFalse(Operator.MATCH.match(pattern, "bbaaa")); - } - - public void testValidOrdinals() { - assertThat(Operator.EQ.ordinal(), equalTo(0)); - assertThat(Operator.GT.ordinal(), equalTo(1)); - assertThat(Operator.GTE.ordinal(), equalTo(2)); - assertThat(Operator.LT.ordinal(), equalTo(3)); - assertThat(Operator.LTE.ordinal(), equalTo(4)); - assertThat(Operator.MATCH.ordinal(), equalTo(5)); - } - - public void testwriteTo() throws Exception { - try (BytesStreamOutput out = new BytesStreamOutput()) { - Operator.EQ.writeTo(out); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(0)); - } - } - + public void testWriteTo() throws Exception { try (BytesStreamOutput out = new BytesStreamOutput()) { Operator.GT.writeTo(out); try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(1)); + assertThat(in.readVInt(), equalTo(0)); } } try (BytesStreamOutput out = new BytesStreamOutput()) { Operator.GTE.writeTo(out); try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(2)); + assertThat(in.readVInt(), equalTo(1)); } } try (BytesStreamOutput out = new BytesStreamOutput()) { Operator.LT.writeTo(out); try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(3)); + assertThat(in.readVInt(), equalTo(2)); } } try (BytesStreamOutput out = new BytesStreamOutput()) { Operator.LTE.writeTo(out); try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(4)); - } - } - - try (BytesStreamOutput out = new BytesStreamOutput()) { - Operator.MATCH.writeTo(out); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(5)); + assertThat(in.readVInt(), equalTo(3)); } } } @@ -130,40 +82,28 @@ public void testwriteTo() throws Exception { public void testReadFrom() throws Exception { try (BytesStreamOutput out = new BytesStreamOutput()) { out.writeVInt(0); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(Operator.readFromStream(in), equalTo(Operator.EQ)); - } - } - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(1); try (StreamInput in = out.bytes().streamInput()) { assertThat(Operator.readFromStream(in), equalTo(Operator.GT)); } } try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(2); + out.writeVInt(1); try (StreamInput in = out.bytes().streamInput()) { assertThat(Operator.readFromStream(in), equalTo(Operator.GTE)); } } try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(3); + out.writeVInt(2); try (StreamInput in = out.bytes().streamInput()) { assertThat(Operator.readFromStream(in), equalTo(Operator.LT)); } } try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(4); + out.writeVInt(3); try (StreamInput in = out.bytes().streamInput()) { assertThat(Operator.readFromStream(in), equalTo(Operator.LTE)); } } - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(5); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(Operator.readFromStream(in), equalTo(Operator.MATCH)); - } - } } public void testInvalidReadFrom() throws Exception { diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/RuleActionTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/RuleActionTests.java index 1aeefc47ac61c..fadf81693934b 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/RuleActionTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/RuleActionTests.java @@ -15,27 +15,29 @@ public class RuleActionTests extends ESTestCase { public void testForString() { - assertEquals(RuleAction.FILTER_RESULTS, RuleAction.fromString("filter_results")); - assertEquals(RuleAction.FILTER_RESULTS, RuleAction.fromString("FILTER_RESULTS")); - assertEquals(RuleAction.SKIP_SAMPLING, RuleAction.fromString("SKip_sampLing")); + assertEquals(RuleAction.SKIP_RESULT, RuleAction.fromString("skip_result")); + assertEquals(RuleAction.SKIP_RESULT, RuleAction.fromString("SKIP_RESULT")); + assertEquals(RuleAction.SKIP_MODEL_UPDATE, RuleAction.fromString("skip_model_update")); + assertEquals(RuleAction.SKIP_MODEL_UPDATE, RuleAction.fromString("SKIP_MODEL_UPDATE")); } public void testToString() { - assertEquals("filter_results", RuleAction.FILTER_RESULTS.toString()); + assertEquals("skip_result", RuleAction.SKIP_RESULT.toString()); + assertEquals("skip_model_update", RuleAction.SKIP_MODEL_UPDATE.toString()); } public void testReadFrom() throws Exception { try (BytesStreamOutput out = new BytesStreamOutput()) { out.writeVInt(0); try (StreamInput in = out.bytes().streamInput()) { - assertThat(RuleAction.readFromStream(in), equalTo(RuleAction.FILTER_RESULTS)); + assertThat(RuleAction.readFromStream(in), equalTo(RuleAction.SKIP_RESULT)); } } try (BytesStreamOutput out = new BytesStreamOutput()) { out.writeVInt(1); try (StreamInput in = out.bytes().streamInput()) { - assertThat(RuleAction.readFromStream(in), equalTo(RuleAction.SKIP_SAMPLING)); + assertThat(RuleAction.readFromStream(in), equalTo(RuleAction.SKIP_MODEL_UPDATE)); } } } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/RuleConditionTypeTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/RuleConditionTypeTests.java deleted file mode 100644 index bec7ca24fbe68..0000000000000 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/RuleConditionTypeTests.java +++ /dev/null @@ -1,140 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.ml.job.config; - -import org.elasticsearch.common.io.stream.BytesStreamOutput; -import org.elasticsearch.common.io.stream.StreamInput; -import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.xpack.core.ml.job.config.RuleConditionType; - -import java.io.IOException; -import java.util.EnumSet; - -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.equalTo; - -public class RuleConditionTypeTests extends ESTestCase { - - public void testFromString() { - assertEquals(RuleConditionType.CATEGORICAL, RuleConditionType.fromString("categorical")); - assertEquals(RuleConditionType.CATEGORICAL, RuleConditionType.fromString("CATEGORICAL")); - assertEquals(RuleConditionType.NUMERICAL_ACTUAL, RuleConditionType.fromString("numerical_actual")); - assertEquals(RuleConditionType.NUMERICAL_ACTUAL, RuleConditionType.fromString("NUMERICAL_ACTUAL")); - assertEquals(RuleConditionType.NUMERICAL_TYPICAL, RuleConditionType.fromString("numerical_typical")); - assertEquals(RuleConditionType.NUMERICAL_TYPICAL, RuleConditionType.fromString("NUMERICAL_TYPICAL")); - assertEquals(RuleConditionType.NUMERICAL_DIFF_ABS, RuleConditionType.fromString("numerical_diff_abs")); - assertEquals(RuleConditionType.NUMERICAL_DIFF_ABS, RuleConditionType.fromString("NUMERICAL_DIFF_ABS")); - } - - public void testToString() { - assertEquals("categorical", RuleConditionType.CATEGORICAL.toString()); - assertEquals("categorical_complement", RuleConditionType.CATEGORICAL_COMPLEMENT.toString()); - assertEquals("numerical_actual", RuleConditionType.NUMERICAL_ACTUAL.toString()); - assertEquals("numerical_typical", RuleConditionType.NUMERICAL_TYPICAL.toString()); - assertEquals("numerical_diff_abs", RuleConditionType.NUMERICAL_DIFF_ABS.toString()); - } - - public void testValidOrdinals() { - assertThat(RuleConditionType.CATEGORICAL.ordinal(), equalTo(0)); - assertThat(RuleConditionType.NUMERICAL_ACTUAL.ordinal(), equalTo(1)); - assertThat(RuleConditionType.NUMERICAL_TYPICAL.ordinal(), equalTo(2)); - assertThat(RuleConditionType.NUMERICAL_DIFF_ABS.ordinal(), equalTo(3)); - } - - public void testwriteTo() throws Exception { - try (BytesStreamOutput out = new BytesStreamOutput()) { - RuleConditionType.CATEGORICAL.writeTo(out); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(0)); - } - } - - try (BytesStreamOutput out = new BytesStreamOutput()) { - RuleConditionType.NUMERICAL_ACTUAL.writeTo(out); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(1)); - } - } - - try (BytesStreamOutput out = new BytesStreamOutput()) { - RuleConditionType.NUMERICAL_TYPICAL.writeTo(out); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(2)); - } - } - - try (BytesStreamOutput out = new BytesStreamOutput()) { - RuleConditionType.NUMERICAL_DIFF_ABS.writeTo(out); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(in.readVInt(), equalTo(3)); - } - } - } - - public void testReadFrom() throws Exception { - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(0); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(RuleConditionType.readFromStream(in), equalTo(RuleConditionType.CATEGORICAL)); - } - } - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(1); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(RuleConditionType.readFromStream(in), equalTo(RuleConditionType.NUMERICAL_ACTUAL)); - } - } - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(2); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(RuleConditionType.readFromStream(in), equalTo(RuleConditionType.NUMERICAL_TYPICAL)); - } - } - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(3); - try (StreamInput in = out.bytes().streamInput()) { - assertThat(RuleConditionType.readFromStream(in), equalTo(RuleConditionType.NUMERICAL_DIFF_ABS)); - } - } - } - - public void testInvalidReadFrom() throws Exception { - try (BytesStreamOutput out = new BytesStreamOutput()) { - out.writeVInt(randomIntBetween(4, Integer.MAX_VALUE)); - try (StreamInput in = out.bytes().streamInput()) { - RuleConditionType.readFromStream(in); - fail("Expected IOException"); - } catch (IOException e) { - assertThat(e.getMessage(), containsString("Unknown RuleConditionType ordinal [")); - } - } - } - - public void testIsNumerical() { - for (RuleConditionType type : EnumSet.allOf(RuleConditionType.class)) { - boolean isNumerical = type.isNumerical(); - if (type == RuleConditionType.NUMERICAL_ACTUAL || - type == RuleConditionType.NUMERICAL_DIFF_ABS || - type == RuleConditionType.NUMERICAL_TYPICAL) { - assertTrue(isNumerical); - } else { - assertFalse(isNumerical); - } - } - } - - public void testIsCategorical() { - for (RuleConditionType type : EnumSet.allOf(RuleConditionType.class)) { - boolean isCategorical = type.isCategorical(); - if (type == RuleConditionType.CATEGORICAL || - type == RuleConditionType.CATEGORICAL_COMPLEMENT) { - assertTrue(isCategorical); - } else { - assertFalse(isCategorical); - } - } - } -} diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectCommunicatorTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectCommunicatorTests.java index 0aecad2b3a634..cedc65c2ee225 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectCommunicatorTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/AutodetectCommunicatorTests.java @@ -22,7 +22,7 @@ import org.elasticsearch.xpack.core.ml.job.config.Detector; import org.elasticsearch.xpack.core.ml.job.config.Job; import org.elasticsearch.xpack.core.ml.job.config.JobUpdate; -import org.elasticsearch.xpack.core.ml.job.config.RuleCondition; +import org.elasticsearch.xpack.core.ml.job.config.RuleScope; import org.elasticsearch.xpack.core.ml.job.process.autodetect.output.FlushAcknowledgement; import org.elasticsearch.xpack.ml.job.categorization.CategorizationAnalyzerTests; import org.elasticsearch.xpack.ml.job.persistence.StateStreamer; @@ -91,10 +91,7 @@ public void testWriteUpdateProcessMessage() throws IOException { when(process.isReady()).thenReturn(true); AutodetectCommunicator communicator = createAutodetectCommunicator(process, mock(AutoDetectResultProcessor.class)); - List conditions = Collections.singletonList( - RuleCondition.createCategorical("foo", "bar")); - - DetectionRule updatedRule = new DetectionRule.Builder(conditions).build(); + DetectionRule updatedRule = new DetectionRule.Builder(RuleScope.builder().exclude("foo", "bar")).build(); List detectorUpdates = Collections.singletonList( new JobUpdate.DetectorUpdate(0, "updated description", Collections.singletonList(updatedRule))); diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ControlMsgToProcessWriterTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ControlMsgToProcessWriterTests.java index 130476951abbd..ede2478bf32e5 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ControlMsgToProcessWriterTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ControlMsgToProcessWriterTests.java @@ -9,17 +9,16 @@ import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.core.ml.calendars.ScheduledEvent; import org.elasticsearch.xpack.core.ml.job.config.Condition; -import org.elasticsearch.xpack.core.ml.job.config.Connective; import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; import org.elasticsearch.xpack.core.ml.job.config.MlFilter; import org.elasticsearch.xpack.core.ml.job.config.ModelPlotConfig; import org.elasticsearch.xpack.core.ml.job.config.Operator; import org.elasticsearch.xpack.core.ml.job.config.RuleCondition; -import org.elasticsearch.xpack.core.ml.job.config.RuleConditionType; import org.elasticsearch.xpack.ml.job.process.autodetect.params.DataLoadParams; import org.elasticsearch.xpack.ml.job.process.autodetect.params.FlushJobParams; import org.elasticsearch.xpack.ml.job.process.autodetect.params.TimeRange; import org.junit.Before; +import org.mockito.ArgumentCaptor; import org.mockito.InOrder; import org.mockito.Mockito; @@ -31,6 +30,7 @@ import java.util.Optional; import java.util.stream.IntStream; +import static org.hamcrest.Matchers.equalTo; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -190,22 +190,18 @@ public void testWriteUpdateModelPlotMessage() throws IOException { public void testWriteUpdateDetectorRulesMessage() throws IOException { ControlMsgToProcessWriter writer = new ControlMsgToProcessWriter(lengthEncodedWriter, 4); - DetectionRule rule1 = new DetectionRule.Builder(createRule("5")).setTargetFieldName("targetField1") - .setTargetFieldValue("targetValue").setConditionsConnective(Connective.AND).build(); - DetectionRule rule2 = new DetectionRule.Builder(createRule("5")).setTargetFieldName("targetField2") - .setTargetFieldValue("targetValue").setConditionsConnective(Connective.AND).build(); + DetectionRule rule1 = new DetectionRule.Builder(createRule(5)).build(); + DetectionRule rule2 = new DetectionRule.Builder(createRule(5)).build(); writer.writeUpdateDetectorRulesMessage(2, Arrays.asList(rule1, rule2)); InOrder inOrder = inOrder(lengthEncodedWriter); inOrder.verify(lengthEncodedWriter).writeNumFields(4); inOrder.verify(lengthEncodedWriter, times(3)).writeField(""); inOrder.verify(lengthEncodedWriter).writeField("u[detectorRules]\ndetectorIndex=2\n" + - "rulesJson=[{\"actions\":[\"filter_results\"],\"conditions_connective\":\"and\",\"conditions\":" + - "[{\"type\":\"numerical_actual\",\"condition\":{\"operator\":\"gt\",\"value\":\"5\"}}]," + - "\"target_field_name\":\"targetField1\",\"target_field_value\":\"targetValue\"}," + - "{\"actions\":[\"filter_results\"],\"conditions_connective\":\"and\",\"conditions\":[" + - "{\"type\":\"numerical_actual\",\"condition\":{\"operator\":\"gt\",\"value\":\"5\"}}]," + - "\"target_field_name\":\"targetField2\",\"target_field_value\":\"targetValue\"}]"); + "rulesJson=[{\"actions\":[\"skip_result\"],\"conditions\":" + + "[{\"applies_to\":\"actual\",\"condition\":{\"operator\":\"gt\",\"value\":5.0}}]}," + + "{\"actions\":[\"skip_result\"],\"conditions\":[" + + "{\"applies_to\":\"actual\",\"condition\":{\"operator\":\"gt\",\"value\":5.0}}]}]"); verifyNoMoreInteractions(lengthEncodedWriter); } @@ -244,16 +240,17 @@ public void testWriteUpdateScheduledEventsMessage() throws IOException { InOrder inOrder = inOrder(lengthEncodedWriter); inOrder.verify(lengthEncodedWriter).writeNumFields(2); inOrder.verify(lengthEncodedWriter, times(1)).writeField(""); - inOrder.verify(lengthEncodedWriter).writeField("u[scheduledEvents]\n" + ArgumentCaptor capturedMessage = ArgumentCaptor.forClass(String.class); + inOrder.verify(lengthEncodedWriter).writeField(capturedMessage.capture()); + assertThat(capturedMessage.getValue(), equalTo("u[scheduledEvents]\n" + "scheduledevent.0.description = new year\n" - + "scheduledevent.0.rules = [{\"actions\":[\"filter_results\",\"skip_sampling\"],\"conditions_connective\":\"and\"," - + "\"conditions\":[{\"type\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":\"1514764800\"}}," - + "{\"type\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":\"1514851200\"}}]}]\n" + + "scheduledevent.0.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"]," + + "\"conditions\":[{\"applies_to\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":1.5147648E9}}," + + "{\"applies_to\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":1.5148512E9}}]}]\n" + "scheduledevent.1.description = Jan maintenance day\n" - + "scheduledevent.1.rules = [{\"actions\":[\"filter_results\",\"skip_sampling\"],\"conditions_connective\":\"and\"," - + "\"conditions\":[{\"type\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":\"1515196800\"}}," - + "{\"type\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":\"1515283200\"}}]}]\n" - ); + + "scheduledevent.1.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"]," + + "\"conditions\":[{\"applies_to\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":1.5151968E9}}," + + "{\"applies_to\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":1.5152832E9}}]}]\n")); verifyNoMoreInteractions(lengthEncodedWriter); } @@ -288,8 +285,8 @@ public void testWriteStartBackgroundPersistMessage() throws IOException { verifyNoMoreInteractions(lengthEncodedWriter); } - private static List createRule(String value) { + private static List createRule(double value) { Condition condition = new Condition(Operator.GT, value); - return Collections.singletonList(RuleCondition.createNumerical(RuleConditionType.NUMERICAL_ACTUAL, null, null, condition)); + return Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, condition)); } } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriterTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriterTests.java index 57917f6761bb7..6e40a0c14b54c 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriterTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriterTests.java @@ -10,7 +10,6 @@ import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.xpack.ml.MachineLearning; import org.elasticsearch.xpack.core.ml.calendars.ScheduledEvent; import org.elasticsearch.xpack.core.ml.job.config.AnalysisConfig; import org.elasticsearch.xpack.core.ml.job.config.Condition; @@ -19,7 +18,7 @@ import org.elasticsearch.xpack.core.ml.job.config.MlFilter; import org.elasticsearch.xpack.core.ml.job.config.Operator; import org.elasticsearch.xpack.core.ml.job.config.RuleCondition; -import org.elasticsearch.xpack.core.ml.job.config.RuleConditionType; +import org.elasticsearch.xpack.ml.MachineLearning; import org.ini4j.Config; import org.ini4j.Ini; import org.ini4j.Profile.Section; @@ -193,9 +192,8 @@ public void testWrite_GivenDetectorWithRules() throws IOException { Detector.Builder detector = new Detector.Builder("mean", "metricValue"); detector.setByFieldName("metricName"); detector.setPartitionFieldName("instance"); - RuleCondition ruleCondition = RuleCondition.createNumerical - (RuleConditionType.NUMERICAL_ACTUAL, "metricName", "metricValue", new Condition(Operator.LT, "5")); - DetectionRule rule = new DetectionRule.Builder(Collections.singletonList(ruleCondition)).setTargetFieldName("instance").build(); + RuleCondition ruleCondition = new RuleCondition(RuleCondition.AppliesTo.ACTUAL, new Condition(Operator.LT, 5)); + DetectionRule rule = new DetectionRule.Builder(Collections.singletonList(ruleCondition)).build(); detector.setRules(Collections.singletonList(rule)); AnalysisConfig.Builder builder = new AnalysisConfig.Builder(Collections.singletonList(detector.build())); @@ -255,14 +253,13 @@ public void testWrite_GivenScheduledEvents() throws IOException { verify(writer).write("detector.0.clause = count\n" + "scheduledevent.0.description = The Ashes\n" + - "scheduledevent.0.rules = [{\"actions\":[\"filter_results\",\"skip_sampling\"],\"conditions_connective\":\"and\"," + - "\"conditions\":[{\"type\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":\"1511395200\"}}," + - "{\"type\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":\"1515369600\"}}]}]\n" + + "scheduledevent.0.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"],\"conditions\":[{\"applies_to\":\"time\"," + + "\"condition\":{\"operator\":\"gte\",\"value\":1.5113952E9}},{\"applies_to\":\"time\",\"condition\":" + + "{\"operator\":\"lt\",\"value\":1.5153696E9}}]}]\n" + "scheduledevent.1.description = elasticon\n" + - "scheduledevent.1.rules = [{\"actions\":[\"filter_results\",\"skip_sampling\"],\"conditions_connective\":\"and\"," + - "\"conditions\":[{\"type\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":\"1519603200\"}}," + - "{\"type\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":\"1519862400\"}}]}]" + - "\n"); + "scheduledevent.1.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"]," + + "\"conditions\":[{\"applies_to\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":1.5196032E9}}," + + "{\"applies_to\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":1.5198624E9}}]}]\n"); verifyNoMoreInteractions(writer); } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ScheduledEventsWriterTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ScheduledEventsWriterTests.java index ae1b77b34089c..b5304c9e47365 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ScheduledEventsWriterTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ScheduledEventsWriterTests.java @@ -44,13 +44,13 @@ public void testWrite() throws IOException { new ScheduledEventsWriter(events, TimeValue.timeValueHours(1), buffer).write(); String expectedString = "scheduledevent.0.description = Black Friday\n" + - "scheduledevent.0.rules = [{\"actions\":[\"filter_results\",\"skip_sampling\"],\"conditions_connective\":\"and\"," + - "\"conditions\":[{\"type\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":\"1511395200\"}}," + - "{\"type\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":\"1515369600\"}}]}]\n" + + "scheduledevent.0.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"]," + + "\"conditions\":[{\"applies_to\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":1.5113952E9}}," + + "{\"applies_to\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":1.5153696E9}}]}]\n" + "scheduledevent.1.description = Blue Monday\n" + - "scheduledevent.1.rules = [{\"actions\":[\"filter_results\",\"skip_sampling\"],\"conditions_connective\":\"and\"," + - "\"conditions\":[{\"type\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":\"1519603200\"}}," + - "{\"type\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":\"1519862400\"}}]}]" + + "scheduledevent.1.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"]," + + "\"conditions\":[{\"applies_to\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":1.5196032E9}}," + + "{\"applies_to\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":1.5198624E9}}]}]" + "\n"; assertThat(buffer.toString(), equalTo(expectedString)); } diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml index 5203ead3ce8a9..d3165260f4b95 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/filter_crud.yml @@ -152,15 +152,11 @@ setup: "analysis_config" : { "bucket_span": "3600s", "detectors" :[{"function":"mean","field_name":"responsetime", "by_field_name": "airline", - "rules": [ + "custom_rules": [ { - "conditions": [ - { - "type": "categorical", - "field_name": "airline", - "filter_id": "filter-foo" - } - ] + "scope": { + "airline": {"filter_id": "filter-foo"} + } } ]}] }, diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/jobs_crud.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/jobs_crud.yml index ed6b9ead1a3b1..8a47dc845141b 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/jobs_crud.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/jobs_crud.yml @@ -301,10 +301,25 @@ { "groups": ["group-1", "group-2"], "description":"Post update description", - "detectors": [{"detector_index": 0, "rules": {"target_field_name": "airline", - "conditions": [ { "type": "numerical_actual", - "condition": {"operator": "gt", "value": "10" } } ] } }, - {"detector_index": 1, "description": "updated description"}], + "detectors": [ + { + "detector_index": 0, + "custom_rules":[ + { + "conditions": [ + { + "applies_to": "actual", + "condition": {"operator": "gt", "value": 10 } + } + ] + } + ] + }, + { + "detector_index": 1, + "description": "updated description" + } + ], "model_plot_config": { "enabled": false, "terms": "foobar" @@ -324,7 +339,8 @@ - match: { model_plot_config.enabled: false } - match: { model_plot_config.terms: "foobar" } - match: { analysis_config.categorization_filters: ["cat3.*"] } - - match: { analysis_config.detectors.0.rules.0.target_field_name: "airline" } + - match: { analysis_config.detectors.0.custom_rules.0.actions: ["skip_result"] } + - length: { analysis_config.detectors.0.custom_rules.0.conditions: 1 } - match: { analysis_config.detectors.0.detector_index: 0 } - match: { analysis_config.detectors.1.detector_description: "updated description" } - match: { analysis_config.detectors.1.detector_index: 1 } @@ -1223,17 +1239,21 @@ { "function": "count", "by_field_name": "country", - "rules": [ + "custom_rules": [ { - "actions": ["filter_results", "skip_sampling"], + "actions": ["skip_result", "skip_model_update"], + "scope": { + "country": {"filter_id": "safe_countries"} + }, "conditions": [ { - "type":"numerical_actual", - "field_name":"country", - "field_value": "uk", - "condition": {"operator":"lt","value":"33.3"} + "applies_to":"actual", + "condition": {"operator":"lt","value": 33.3} }, - {"type":"categorical", "field_name":"country", "filter_id": "foo"} + { + "applies_to":"typical", + "condition": {"operator":"lte","value": 42.0} + } ] } ] @@ -1248,83 +1268,21 @@ job_id: jobs-crud-rules - match: { count: 1 } - match: { - jobs.0.analysis_config.detectors.0.rules: [ + jobs.0.analysis_config.detectors.0.custom_rules: [ { - "actions": ["filter_results", "skip_sampling"], - "conditions_connective": "or", + "actions": ["skip_result", "skip_model_update"], + "scope": { + "country": {"filter_id": "safe_countries", "filter_type": "include"} + }, "conditions": [ { - "type":"numerical_actual", - "field_name":"country", - "field_value": "uk", - "condition": {"operator":"lt","value":"33.3"} - }, - {"type":"categorical", "field_name":"country", "filter_id": "foo"} - ] - } - ] - } - ---- -"Test job with pre 6.2 rules": - - - skip: - features: "warnings" - reason: certain rule fields were renamed in 6.2.0 - - - do: - warnings: - - Deprecated field [detector_rules] used, expected [rules] instead - - Deprecated field [rule_action] used, expected [actions] instead - - Deprecated field [rule_conditions] used, expected [conditions] instead - - Deprecated field [condition_type] used, expected [type] instead - - Deprecated field [value_filter] used, expected [filter_id] instead - xpack.ml.put_job: - job_id: jobs-crud-pre-6-2-rules - body: > - { - "analysis_config": { - "detectors": [ - { - "function": "count", - "by_field_name": "country", - "detector_rules": [ - { - "rule_action": "filter_results", - "rule_conditions": [ - { - "condition_type":"numerical_actual", - "field_name":"country", - "field_value": "uk", - "condition": {"operator":"lt","value":"33.3"} - }, - {"type":"categorical", "field_name":"country", "value_filter": "foo"} - ] - } - ] - } - ] + "applies_to":"actual", + "condition": {"operator":"lt","value": 33.3} }, - "data_description" : {} - } - - - do: - xpack.ml.get_jobs: - job_id: jobs-crud-pre-6-2-rules - - match: { count: 1 } - - match: { - jobs.0.analysis_config.detectors.0.rules: [ - { - "actions": ["filter_results"], - "conditions_connective": "or", - "conditions": [ { - "type":"numerical_actual", - "field_name":"country", - "field_value": "uk", - "condition": {"operator":"lt","value":"33.3"} - }, - {"type":"categorical", "field_name":"country", "filter_id": "foo"} + "applies_to":"typical", + "condition": {"operator":"lte","value": 42.0} + } ] } ] diff --git a/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/DetectionRulesIT.java b/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/DetectionRulesIT.java index 47ced4a96dde8..6fabb2550ec05 100644 --- a/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/DetectionRulesIT.java +++ b/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/DetectionRulesIT.java @@ -21,10 +21,9 @@ import org.elasticsearch.xpack.core.ml.job.config.MlFilter; import org.elasticsearch.xpack.core.ml.job.config.Operator; import org.elasticsearch.xpack.core.ml.job.config.RuleCondition; -import org.elasticsearch.xpack.core.ml.job.config.RuleConditionType; +import org.elasticsearch.xpack.core.ml.job.config.RuleScope; import org.elasticsearch.xpack.core.ml.job.results.AnomalyRecord; import org.junit.After; -import org.junit.Ignore; import java.io.IOException; import java.util.ArrayList; @@ -35,9 +34,7 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.stream.Collectors; -import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.isOneOf; @@ -48,38 +45,22 @@ public class DetectionRulesIT extends MlNativeAutodetectIntegTestCase { @After - public void cleanUpTest() throws Exception { + public void cleanUpTest() { cleanUp(); } - @AwaitsFix(bugUrl = "this test is muted temporarily until the new rules implementation is merged in") - public void testNumericalRule() throws Exception { - RuleCondition condition1 = RuleCondition.createNumerical( - RuleConditionType.NUMERICAL_ACTUAL, - "by_field", - "by_field_value_1", - new Condition(Operator.LT, "1000")); - RuleCondition condition2 = RuleCondition.createNumerical( - RuleConditionType.NUMERICAL_ACTUAL, - "by_field", - "by_field_value_2", - new Condition(Operator.LT, "500")); - RuleCondition condition3 = RuleCondition.createNumerical( - RuleConditionType.NUMERICAL_ACTUAL, - "by_field", - "by_field_value_3", - new Condition(Operator.LT, "100")); - DetectionRule rule = new DetectionRule.Builder(Arrays.asList(condition1, condition2, condition3)).build(); - - Detector.Builder detector = new Detector.Builder("max", "value"); - detector.setRules(Arrays.asList(rule)); - detector.setByFieldName("by_field"); + public void testCondition() throws Exception { + DetectionRule rule = new DetectionRule.Builder(Arrays.asList( + new RuleCondition(RuleCondition.AppliesTo.ACTUAL, new Condition(Operator.LT, 100.0)) + )).build(); - AnalysisConfig.Builder analysisConfig = new AnalysisConfig.Builder( - Arrays.asList(detector.build())); + Detector.Builder detector = new Detector.Builder("mean", "value"); + detector.setByFieldName("by_field"); + detector.setRules(Arrays.asList(rule)); + AnalysisConfig.Builder analysisConfig = new AnalysisConfig.Builder(Arrays.asList(detector.build())); analysisConfig.setBucketSpan(TimeValue.timeValueHours(1)); DataDescription.Builder dataDescription = new DataDescription.Builder(); - Job.Builder job = new Job.Builder("detection-rule-numeric-test"); + Job.Builder job = new Job.Builder("detection-rules-it-test-condition"); job.setAnalysisConfig(analysisConfig); job.setDataDescription(dataDescription); @@ -91,12 +72,11 @@ public void testNumericalRule() throws Exception { int totalBuckets = 2 * 24; // each half of the buckets contains one anomaly for each by field value Set anomalousBuckets = new HashSet<>(Arrays.asList(20, 44)); - List byFieldValues = Arrays.asList("by_field_value_1", "by_field_value_2", "by_field_value_3"); + List byFieldValues = Arrays.asList("low", "high"); Map anomalousValues = new HashMap<>(); - anomalousValues.put("by_field_value_1", 800); - anomalousValues.put("by_field_value_2", 400); - anomalousValues.put("by_field_value_3", 400); - int normalValue = 1; + anomalousValues.put("low", 99); + anomalousValues.put("high", 701); + int normalValue = 400; List data = new ArrayList<>(); for (int bucket = 0; bucket < totalBuckets; bucket++) { for (String byFieldValue : byFieldValues) { @@ -115,27 +95,14 @@ public void testNumericalRule() throws Exception { List records = getRecords(job.getId()); assertThat(records.size(), equalTo(1)); - assertThat(records.get(0).getByFieldValue(), equalTo("by_field_value_3")); + assertThat(records.get(0).getByFieldValue(), equalTo("high")); long firstRecordTimestamp = records.get(0).getTimestamp().getTime(); { // Update rules so that the anomalies suppression is inverted - RuleCondition newCondition1 = RuleCondition.createNumerical( - RuleConditionType.NUMERICAL_ACTUAL, - "by_field", - "by_field_value_1", - new Condition(Operator.GT, "1000")); - RuleCondition newCondition2 = RuleCondition.createNumerical( - RuleConditionType.NUMERICAL_ACTUAL, - "by_field", - "by_field_value_2", - new Condition(Operator.GT, "500")); - RuleCondition newCondition3 = RuleCondition.createNumerical( - RuleConditionType.NUMERICAL_ACTUAL, - "by_field", - "by_field_value_3", - new Condition(Operator.GT, "0")); - DetectionRule newRule = new DetectionRule.Builder(Arrays.asList(newCondition1, newCondition2, newCondition3)).build(); + DetectionRule newRule = new DetectionRule.Builder(Arrays.asList( + new RuleCondition(RuleCondition.AppliesTo.ACTUAL, new Condition(Operator.GT, 700.0)) + )).build(); JobUpdate.Builder update = new JobUpdate.Builder(job.getId()); update.setDetectorUpdates(Arrays.asList(new JobUpdate.DetectorUpdate(0, null, Arrays.asList(newRule)))); updateJob(job.getId(), update.build()); @@ -149,18 +116,15 @@ public void testNumericalRule() throws Exception { GetRecordsAction.Request recordsAfterFirstHalf = new GetRecordsAction.Request(job.getId()); recordsAfterFirstHalf.setStart(String.valueOf(firstRecordTimestamp + 1)); records = getRecords(recordsAfterFirstHalf); - assertThat(records.size(), equalTo(2)); - Set secondHaldRecordByFieldValues = records.stream().map(AnomalyRecord::getByFieldValue).collect(Collectors.toSet()); - assertThat(secondHaldRecordByFieldValues, contains("by_field_value_1", "by_field_value_2")); + assertThat(records.size(), equalTo(1)); + assertThat(records.get(0).getByFieldValue(), equalTo("low")); } - @AwaitsFix(bugUrl = "this test is muted temporarily until the new rules implementation is merged in") - public void testCategoricalRule() throws Exception { + public void testScope() throws Exception { MlFilter safeIps = new MlFilter("safe_ips", Arrays.asList("111.111.111.111", "222.222.222.222")); assertThat(putMlFilter(safeIps), is(true)); - RuleCondition condition = RuleCondition.createCategorical("ip", safeIps.getId()); - DetectionRule rule = new DetectionRule.Builder(Collections.singletonList(condition)).build(); + DetectionRule rule = new DetectionRule.Builder(RuleScope.builder().include("ip", "safe_ips")).build(); Detector.Builder detector = new Detector.Builder("count", null); detector.setRules(Arrays.asList(rule)); @@ -169,7 +133,7 @@ public void testCategoricalRule() throws Exception { AnalysisConfig.Builder analysisConfig = new AnalysisConfig.Builder(Collections.singletonList(detector.build())); analysisConfig.setBucketSpan(TimeValue.timeValueHours(1)); DataDescription.Builder dataDescription = new DataDescription.Builder(); - Job.Builder job = new Job.Builder("detection-rule-categorical-test"); + Job.Builder job = new Job.Builder("detection-rules-it-test-scope"); job.setAnalysisConfig(analysisConfig); job.setDataDescription(dataDescription); @@ -263,6 +227,70 @@ public void testCategoricalRule() throws Exception { closeJob(job.getId()); } + public void testScopeAndCondition() throws IOException { + // We have 2 IPs and they're both safe-listed. + List ips = Arrays.asList("111.111.111.111", "222.222.222.222"); + MlFilter safeIps = new MlFilter("safe_ips", ips); + assertThat(putMlFilter(safeIps), is(true)); + + // Ignore if ip in safe list AND actual < 10. + DetectionRule rule = new DetectionRule.Builder(RuleScope.builder().include("ip", "safe_ips")) + .setConditions(Arrays.asList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, new Condition(Operator.LT, 10.0)))) + .build(); + + Detector.Builder detector = new Detector.Builder("count", null); + detector.setRules(Arrays.asList(rule)); + detector.setOverFieldName("ip"); + + AnalysisConfig.Builder analysisConfig = new AnalysisConfig.Builder(Collections.singletonList(detector.build())); + analysisConfig.setBucketSpan(TimeValue.timeValueHours(1)); + DataDescription.Builder dataDescription = new DataDescription.Builder(); + Job.Builder job = new Job.Builder("detection-rules-it-test-scope-and-condition"); + job.setAnalysisConfig(analysisConfig); + job.setDataDescription(dataDescription); + + registerJob(job); + putJob(job); + openJob(job.getId()); + + long timestamp = 1509062400000L; + List data = new ArrayList<>(); + + // First, 20 buckets with a count of 1 for both IPs + for (int bucket = 0; bucket < 20; bucket++) { + for (String ip : ips) { + data.add(createIpRecord(timestamp, ip)); + } + timestamp += TimeValue.timeValueHours(1).getMillis(); + } + + // Now send anomalous count of 9 for 111.111.111.111 + for (int i = 0; i < 9; i++) { + data.add(createIpRecord(timestamp, "111.111.111.111")); + } + + // and 10 for 222.222.222.222 + for (int i = 0; i < 10; i++) { + data.add(createIpRecord(timestamp, "222.222.222.222")); + } + timestamp += TimeValue.timeValueHours(1).getMillis(); + + // Some more normal buckets + for (int bucket = 0; bucket < 3; bucket++) { + for (String ip : ips) { + data.add(createIpRecord(timestamp, ip)); + } + timestamp += TimeValue.timeValueHours(1).getMillis(); + } + + postData(job.getId(), joinBetween(0, data.size(), data)); + closeJob(job.getId()); + + List records = getRecords(job.getId()); + assertThat(records.size(), equalTo(1)); + assertThat(records.get(0).getOverFieldValue(), equalTo("222.222.222.222")); + } + private String createIpRecord(long timestamp, String ip) throws IOException { Map record = new HashMap<>(); record.put("time", timestamp); diff --git a/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/ScheduledEventsIT.java b/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/ScheduledEventsIT.java index 8410075ff5e27..6703e4ef2365b 100644 --- a/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/ScheduledEventsIT.java +++ b/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/ScheduledEventsIT.java @@ -20,7 +20,6 @@ import org.elasticsearch.xpack.core.ml.job.results.AnomalyRecord; import org.elasticsearch.xpack.core.ml.job.results.Bucket; import org.junit.After; -import org.junit.Ignore; import java.io.IOException; import java.time.Instant; @@ -42,7 +41,6 @@ public void cleanUpTest() { cleanUp(); } - @AwaitsFix(bugUrl = "this test is muted temporarily until the new rules implementation is merged in") public void testScheduledEvents() throws IOException { TimeValue bucketSpan = TimeValue.timeValueMinutes(30); @@ -154,7 +152,6 @@ public void testScheduledEvents() throws IOException { assertThat(records, is(empty())); } - @AwaitsFix(bugUrl = "this test is muted temporarily until the new rules implementation is merged in") public void testScheduledEventWithInterimResults() throws IOException { TimeValue bucketSpan = TimeValue.timeValueMinutes(30); Job.Builder job = createJob("scheduled-events-interim-results", bucketSpan); @@ -196,7 +193,6 @@ public void testScheduledEventWithInterimResults() throws IOException { /** * Test an open job picks up changes to scheduled events/calendars */ - @AwaitsFix(bugUrl = "this test is muted temporarily until the new rules implementation is merged in") public void testOnlineUpdate() throws Exception { TimeValue bucketSpan = TimeValue.timeValueMinutes(30); Job.Builder job = createJob("scheduled-events-online-update", bucketSpan); diff --git a/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/30_ml_jobs_crud.yml b/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/30_ml_jobs_crud.yml index 6ea8771c2374e..c1b238422e92e 100644 --- a/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/30_ml_jobs_crud.yml +++ b/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/30_ml_jobs_crud.yml @@ -91,9 +91,9 @@ wait_for_status: green --- -"Test get job with rules": +"Test job with pre 6.4 rules": - do: xpack.ml.get_jobs: - job_id: old-cluster-job-with-rules + job_id: job-with-old-rules - match: { count: 1 } diff --git a/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/30_ml_jobs_crud.yml b/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/30_ml_jobs_crud.yml index da36da301c1f8..3a3334f6907e9 100644 --- a/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/30_ml_jobs_crud.yml +++ b/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/30_ml_jobs_crud.yml @@ -134,15 +134,15 @@ } --- -"Test job with pre 6.2 rules": +"Test job with pre 6.4 rules": - skip: - version: "6.2.0 - " - reason: "Rules fields were renamed on 6.2.0" + version: "6.4.0 - " + reason: "Rules were replaced by custom_rules on 6.4.0" - do: xpack.ml.put_job: - job_id: old-cluster-job-with-rules + job_id: job-with-old-rules body: > { "analysis_config": { @@ -171,36 +171,22 @@ } --- -"Test job with post 6.2 rules": +"Test job with pre 6.4 rules - dummy job 6.4 onwards": - skip: - version: " - 6.1.99" - reason: "Rules fields were renamed on 6.2.0" + version: " - 6.3.99" + reason: "Rules replaced by custom_rules on 6.4.0" - do: xpack.ml.put_job: - job_id: old-cluster-job-with-rules + job_id: job-with-old-rules body: > { "analysis_config": { "detectors": [ { "function": "count", - "by_field_name": "country", - "rules": [ - { - "actions": ["filter_results"], - "conditions": [ - { - "type":"numerical_actual", - "field_name":"country", - "field_value": "uk", - "condition": {"operator":"lt","value":"33.3"} - }, - {"type":"categorical", "field_name":"country", "filter_id": "foo"} - ] - } - ] + "by_field_name": "country" } ] }, diff --git a/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/30_ml_jobs_crud.yml b/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/30_ml_jobs_crud.yml index 91d294572894d..6634722fac977 100644 --- a/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/30_ml_jobs_crud.yml +++ b/x-pack/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/30_ml_jobs_crud.yml @@ -100,29 +100,14 @@ setup: - match: { acknowledged: true } --- -"Test get job with rules": +"Test job with pre 6.4 rules": - do: xpack.ml.get_jobs: - job_id: old-cluster-job-with-rules + job_id: job-with-old-rules - match: { count: 1 } - - match: { - jobs.0.analysis_config.detectors.0.rules: [ - { - "actions": ["filter_results"], - "conditions_connective": "or", - "conditions": [ - { - "type":"numerical_actual", - "field_name":"country", - "field_value": "uk", - "condition": {"operator":"lt","value":"33.3"} - }, - {"type":"categorical", "field_name":"country", "filter_id": "foo"} - ] - } - ] - } + - is_false: jobs.0.analysis_config.detectors.0.rules + - is_false: jobs.0.analysis_config.detectors.0.custom_rules --- "Test get job with function shortcut should expand": From b88ed36a6e634e0c071c8b2b1c1b31f403b97cc2 Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Wed, 6 Jun 2018 13:55:17 +0100 Subject: [PATCH 2/8] Flatten rule condition object --- .../xpack/core/ml/job/config/Condition.java | 102 ------------------ .../core/ml/job/config/RuleCondition.java | 55 ++++++---- .../ml/calendars/ScheduledEventTests.java | 8 +- .../ml/job/config/DetectionRuleTests.java | 3 +- .../core/ml/job/config/DetectorTests.java | 3 +- .../core/ml/job/config/JobUpdateTests.java | 7 +- .../ml/job/config/RuleConditionTests.java | 14 +-- .../action/TransportOpenJobActionTests.java | 5 +- .../xpack/ml/integration/JobProviderIT.java | 11 +- .../xpack/ml/job/config/ConditionTests.java | 65 ----------- .../ControlMsgToProcessWriterTests.java | 16 ++- .../writer/FieldConfigWriterTests.java | 10 +- .../writer/ScheduledEventsWriterTests.java | 8 +- .../rest-api-spec/test/ml/jobs_crud.yml | 15 ++- .../ml/integration/DetectionRulesIT.java | 7 +- 15 files changed, 84 insertions(+), 245 deletions(-) delete mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Condition.java delete mode 100644 x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConditionTests.java diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Condition.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Condition.java deleted file mode 100644 index 6c8296327a508..0000000000000 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Condition.java +++ /dev/null @@ -1,102 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.core.ml.job.config; - -import org.elasticsearch.common.ParseField; -import org.elasticsearch.common.io.stream.StreamInput; -import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.io.stream.Writeable; -import org.elasticsearch.common.xcontent.ConstructingObjectParser; -import org.elasticsearch.common.xcontent.ObjectParser.ValueType; -import org.elasticsearch.common.xcontent.ToXContentObject; -import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper; - -import java.io.IOException; -import java.util.Objects; - -/** - * A class that describes a condition. - * The {@linkplain Operator} enum defines the available - * comparisons a condition can use. - */ -public class Condition implements ToXContentObject, Writeable { - public static final ParseField CONDITION_FIELD = new ParseField("condition"); - public static final ParseField VALUE_FIELD = new ParseField("value"); - - public static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( - CONDITION_FIELD.getPreferredName(), a -> new Condition((Operator) a[0], (double) a[1])); - - static { - PARSER.declareField(ConstructingObjectParser.constructorArg(), p -> { - if (p.currentToken() == XContentParser.Token.VALUE_STRING) { - return Operator.fromString(p.text()); - } - throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); - }, Operator.OPERATOR_FIELD, ValueType.STRING); - PARSER.declareDouble(ConstructingObjectParser.constructorArg(), VALUE_FIELD); - } - - private final Operator op; - private final double value; - - public Condition(StreamInput in) throws IOException { - op = Operator.readFromStream(in); - value = in.readDouble(); - } - - @Override - public void writeTo(StreamOutput out) throws IOException { - op.writeTo(out); - out.writeDouble(value); - } - - public Condition(Operator op, double value) { - this.op = ExceptionsHelper.requireNonNull(op, Operator.OPERATOR_FIELD.getPreferredName()); - this.value = value; - } - - public Operator getOperator() { - return op; - } - - public double getValue() { - return value; - } - - @Override - public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - builder.startObject(); - builder.field(Operator.OPERATOR_FIELD.getPreferredName(), op); - builder.field(VALUE_FIELD.getPreferredName(), value); - builder.endObject(); - return builder; - } - - @Override - public int hashCode() { - return Objects.hash(op, value); - } - - @Override - public boolean equals(Object obj) { - if (this == obj) { - return true; - } - if (obj == null) { - return false; - } - - if (getClass() != obj.getClass()) { - return false; - } - - Condition other = (Condition) obj; - return Objects.equals(this.op, other.op) && - Objects.equals(this.value, other.value); - } -} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleCondition.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleCondition.java index 426a9b2d1bc6e..378ceaca6c476 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleCondition.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleCondition.java @@ -23,16 +23,19 @@ import java.util.Objects; public class RuleCondition implements ToXContentObject, Writeable { - public static final ParseField APPLIES_TO_FIELD = new ParseField("applies_to"); + public static final ParseField RULE_CONDITION_FIELD = new ParseField("rule_condition"); + public static final ParseField APPLIES_TO_FIELD = new ParseField("applies_to"); + public static final ParseField VALUE_FIELD = new ParseField("value"); + // These parsers follow the pattern that metadata is parsed leniently (to allow for enhancements), whilst config is parsed strictly public static final ConstructingObjectParser METADATA_PARSER = new ConstructingObjectParser<>(RULE_CONDITION_FIELD.getPreferredName(), true, - a -> new RuleCondition((AppliesTo) a[0], (Condition) a[1])); + a -> new RuleCondition((AppliesTo) a[0], (Operator) a[1], (double) a[2])); public static final ConstructingObjectParser CONFIG_PARSER = new ConstructingObjectParser<>(RULE_CONDITION_FIELD.getPreferredName(), false, - a -> new RuleCondition((AppliesTo) a[0], (Condition) a[1])); + a -> new RuleCondition((AppliesTo) a[0], (Operator) a[1], (double) a[2])); public static final Map> PARSERS = new EnumMap<>(MlParserType.class); @@ -48,41 +51,45 @@ public class RuleCondition implements ToXContentObject, Writeable { } throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); }, APPLIES_TO_FIELD, ValueType.STRING); - parser.declareObject(ConstructingObjectParser.optionalConstructorArg(), Condition.PARSER, Condition.CONDITION_FIELD); + parser.declareField(ConstructingObjectParser.constructorArg(), p -> { + if (p.currentToken() == XContentParser.Token.VALUE_STRING) { + return Operator.fromString(p.text()); + } + throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); + }, Operator.OPERATOR_FIELD, ValueType.STRING); + parser.declareDouble(ConstructingObjectParser.constructorArg(), VALUE_FIELD); } } private final AppliesTo appliesTo; - private final Condition condition; + private final Operator operator; + private final double value; public RuleCondition(StreamInput in) throws IOException { appliesTo = AppliesTo.readFromStream(in); - condition = in.readOptionalWriteable(Condition::new); + operator = Operator.readFromStream(in); + value = in.readDouble(); } @Override public void writeTo(StreamOutput out) throws IOException { appliesTo.writeTo(out); - out.writeOptionalWriteable(condition); + operator.writeTo(out); + out.writeDouble(value); } - public RuleCondition(AppliesTo appliesTo, Condition condition) { + public RuleCondition(AppliesTo appliesTo, Operator operator, double value) { this.appliesTo = appliesTo; - this.condition = condition; - } - - public RuleCondition(RuleCondition ruleCondition) { - this.appliesTo = ruleCondition.appliesTo; - this.condition = ruleCondition.condition; + this.operator = operator; + this.value = value; } @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); builder.field(APPLIES_TO_FIELD.getPreferredName(), appliesTo); - if (condition != null) { - builder.field(Condition.CONDITION_FIELD.getPreferredName(), condition); - } + builder.field(Operator.OPERATOR_FIELD.getPreferredName(), operator); + builder.field(VALUE_FIELD.getPreferredName(), value); builder.endObject(); return builder; } @@ -91,8 +98,12 @@ public AppliesTo getAppliesTo() { return appliesTo; } - public Condition getCondition() { - return condition; + public Operator getOperator() { + return operator; + } + + public double getValue() { + return value; } @Override @@ -106,16 +117,16 @@ public boolean equals(Object obj) { } RuleCondition other = (RuleCondition) obj; - return Objects.equals(appliesTo, other.appliesTo) && Objects.equals(condition, other.condition); + return appliesTo == other.appliesTo && operator == other.operator && value == other.value; } @Override public int hashCode() { - return Objects.hash(appliesTo, condition); + return Objects.hash(appliesTo, operator, value); } public static RuleCondition createTime(Operator operator, long epochSeconds) { - return new RuleCondition(AppliesTo.TIME, new Condition(operator, epochSeconds)); + return new RuleCondition(AppliesTo.TIME, operator, epochSeconds); } public enum AppliesTo implements Writeable { diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEventTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEventTests.java index 89e72e4d24cb7..3d6c317542279 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEventTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEventTests.java @@ -60,16 +60,16 @@ public void testToDetectionRule() { assertEquals(2, conditions.size()); assertEquals(RuleCondition.AppliesTo.TIME, conditions.get(0).getAppliesTo()); assertEquals(RuleCondition.AppliesTo.TIME, conditions.get(1).getAppliesTo()); - assertEquals(Operator.GTE, conditions.get(0).getCondition().getOperator()); - assertEquals(Operator.LT, conditions.get(1).getCondition().getOperator()); + assertEquals(Operator.GTE, conditions.get(0).getOperator()); + assertEquals(Operator.LT, conditions.get(1).getOperator()); // Check times are aligned with the bucket - long conditionStartTime = (long) conditions.get(0).getCondition().getValue(); + long conditionStartTime = (long) conditions.get(0).getValue(); assertEquals(0, conditionStartTime % bucketSpanSecs); long bucketCount = conditionStartTime / bucketSpanSecs; assertEquals(bucketSpanSecs * bucketCount, conditionStartTime); - long conditionEndTime = (long) conditions.get(1).getCondition().getValue(); + long conditionEndTime = (long) conditions.get(1).getValue(); assertEquals(0, conditionEndTime % bucketSpanSecs); long eventTime = event.getEndTime().toEpochSecond() - conditionStartTime; diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRuleTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRuleTests.java index 4cbe3939cbce6..a57bd24eda0e1 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRuleTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRuleTests.java @@ -122,7 +122,6 @@ protected DetectionRule mutateInstance(DetectionRule instance) { } private static List createCondition(double value) { - Condition condition = new Condition(Operator.GT, value); - return Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, condition)); + return Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, value)); } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectorTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectorTests.java index f350aa86d293d..d03390ce6110f 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectorTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectorTests.java @@ -69,7 +69,7 @@ public void testEquals_GivenDifferentByFieldName() { public void testExtractAnalysisFields() { DetectionRule rule = new DetectionRule.Builder( - Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, new Condition(Operator.GT, 5)))) + Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, 5))) .setActions(RuleAction.SKIP_RESULT) .build(); Detector.Builder builder = createDetector(); @@ -167,7 +167,6 @@ protected Detector createTestInstance() { List rules = new ArrayList<>(size); for (int i = 0; i < size; i++) { // no need for random DetectionRule (it is already tested) - Condition condition = new Condition(Operator.GT, 5); rules.add(new DetectionRule.Builder(Collections.singletonList(RuleConditionTests.createRandom())).build()); } detector.setRules(rules); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdateTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdateTests.java index 23e7d929ca92c..c529d6ebfb368 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdateTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdateTests.java @@ -53,9 +53,8 @@ protected JobUpdate createTestInstance() { List detectionRules = null; if (randomBoolean()) { detectionRules = new ArrayList<>(); - Condition condition = new Condition(Operator.GT, 5); detectionRules.add(new DetectionRule.Builder( - Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, condition))).build()); + Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, 5))).build()); } detectorUpdates.add(new JobUpdate.DetectorUpdate(i, detectorDescription, detectionRules)); } @@ -118,11 +117,11 @@ protected JobUpdate doParseInstance(XContentParser parser) { public void testMergeWithJob() { List detectorUpdates = new ArrayList<>(); List detectionRules1 = Collections.singletonList(new DetectionRule.Builder( - Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, new Condition(Operator.GT, 5)))) + Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, 5))) .build()); detectorUpdates.add(new JobUpdate.DetectorUpdate(0, "description-1", detectionRules1)); List detectionRules2 = Collections.singletonList(new DetectionRule.Builder(Collections.singletonList( - new RuleCondition(RuleCondition.AppliesTo.ACTUAL, new Condition(Operator.GT, 5)))).build()); + new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, 5))).build()); detectorUpdates.add(new JobUpdate.DetectorUpdate(1, "description-2", detectionRules2)); ModelPlotConfig modelPlotConfig = new ModelPlotConfig(randomBoolean(), randomAlphaOfLength(10)); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionTests.java index 7beee0b68f1f6..07122818d5506 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleConditionTests.java @@ -18,8 +18,8 @@ protected RuleCondition createTestInstance() { public static RuleCondition createRandom() { RuleCondition.AppliesTo appliesTo = randomFrom(RuleCondition.AppliesTo.values()); - Condition condition = new Condition(randomFrom(Operator.LT, Operator.LTE, Operator.GT, Operator.GTE), randomDouble()); - return new RuleCondition(appliesTo, condition); + Operator operator = randomFrom(Operator.LT, Operator.LTE, Operator.GT, Operator.GTE); + return new RuleCondition(appliesTo, operator, randomDouble()); } @Override @@ -43,24 +43,24 @@ public void testEqualsGivenString() { public void testVerify_GivenValidActual() { // no validation error: - new RuleCondition(RuleCondition.AppliesTo.ACTUAL, new Condition(Operator.GT, 5.0)); + new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, 5.0); } public void testVerify_GivenValidTypical() { // no validation error: - new RuleCondition(RuleCondition.AppliesTo.TYPICAL, new Condition(Operator.GTE, 5.0)); + new RuleCondition(RuleCondition.AppliesTo.TYPICAL, Operator.GTE, 5.0); } public void testVerify_GivenValidDiffFromTypical() { // no validation error: - new RuleCondition(RuleCondition.AppliesTo.DIFF_FROM_TYPICAL, new Condition(Operator.LT, 5.0)); + new RuleCondition(RuleCondition.AppliesTo.DIFF_FROM_TYPICAL, Operator.LT, 5.0); } public void testCreateTimeBased() { RuleCondition timeBased = RuleCondition.createTime(Operator.GTE, 100L); assertEquals(RuleCondition.AppliesTo.TIME, timeBased.getAppliesTo()); - assertEquals(Operator.GTE, timeBased.getCondition().getOperator()); - assertEquals(100.0, timeBased.getCondition().getValue(), 0.000001); + assertEquals(Operator.GTE, timeBased.getOperator()); + assertEquals(100.0, timeBased.getValue(), 0.000001); } public void testAppliesToFromString() { diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportOpenJobActionTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportOpenJobActionTests.java index f2a67f81364d2..c28207332ad3c 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportOpenJobActionTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportOpenJobActionTests.java @@ -37,7 +37,6 @@ import org.elasticsearch.xpack.core.ml.MlMetadata; import org.elasticsearch.xpack.core.ml.action.OpenJobAction; import org.elasticsearch.xpack.core.ml.job.config.AnalysisConfig; -import org.elasticsearch.xpack.core.ml.job.config.Condition; import org.elasticsearch.xpack.core.ml.job.config.DataDescription; import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; import org.elasticsearch.xpack.core.ml.job.config.Detector; @@ -574,7 +573,7 @@ public void testNodeNameAndMlAttributes() { public void testCheckJobWithRulesRequiresMinVersionOnAllNodes_GivenNodeDoesNotMeetRequiredVersion() { DetectionRule rule = new DetectionRule.Builder(Arrays.asList( - new RuleCondition(RuleCondition.AppliesTo.TYPICAL, new Condition(Operator.LT, 100.0)) + new RuleCondition(RuleCondition.AppliesTo.TYPICAL, Operator.LT, 100.0) )).build(); Detector.Builder detector = new Detector.Builder("count", null); @@ -607,7 +606,7 @@ public void testCheckJobWithRulesRequiresMinVersionOnAllNodes_GivenNodeDoesNotMe public void testCheckJobWithRulesRequiresMinVersionOnAllNodes_GivenAllNodesMeetRequiredVersion() { DetectionRule rule = new DetectionRule.Builder(Arrays.asList( - new RuleCondition(RuleCondition.AppliesTo.TYPICAL, new Condition(Operator.LT, 100.0)) + new RuleCondition(RuleCondition.AppliesTo.TYPICAL, Operator.LT, 100.0) )).build(); Detector.Builder detector = new Detector.Builder("count", null); diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java index 6086860a44edc..7e0dc453f07ee 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java @@ -510,16 +510,15 @@ private Job.Builder createJob(String jobId, List filterIds, List private AnalysisConfig.Builder createAnalysisConfig(List filterIds) { Detector.Builder detector = new Detector.Builder("mean", "field"); detector.setByFieldName("by_field"); + List rules = new ArrayList<>(); - if (!filterIds.isEmpty()) { + for (String filterId : filterIds) { RuleScope.Builder ruleScope = RuleScope.builder(); - for (String filterId : filterIds) { - ruleScope.exclude("by_field", filterId); - } + ruleScope.include("by_field", filterId); - DetectionRule.Builder rule = new DetectionRule.Builder(ruleScope).setActions(RuleAction.SKIP_RESULT); - detector.setRules(Collections.singletonList(rule.build())); + rules.add(new DetectionRule.Builder(ruleScope).setActions(RuleAction.SKIP_RESULT).build()); } + detector.setRules(rules); return new AnalysisConfig.Builder(Collections.singletonList(detector.build())); } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConditionTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConditionTests.java deleted file mode 100644 index 970152d998748..0000000000000 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/config/ConditionTests.java +++ /dev/null @@ -1,65 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.ml.job.config; - -import org.elasticsearch.common.io.stream.Writeable.Reader; -import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.test.AbstractSerializingTestCase; -import org.elasticsearch.xpack.core.ml.job.config.Condition; -import org.elasticsearch.xpack.core.ml.job.config.Operator; - -import java.io.IOException; - -public class ConditionTests extends AbstractSerializingTestCase { - - @Override - protected Condition createTestInstance() { - Operator op = randomFrom(Operator.values()); - Condition condition; - switch (op) { - case GT: - case GTE: - case LT: - case LTE: - condition = new Condition(op, randomDouble()); - break; - default: - throw new AssertionError("Unknown operator selected: " + op); - } - return condition; - } - - @Override - protected Reader instanceReader() { - return Condition::new; - } - - @Override - protected Condition doParseInstance(XContentParser parser) { - return Condition.PARSER.apply(parser, null); - } - - @Override - protected Condition mutateInstance(Condition instance) throws IOException { - Operator op = instance.getOperator(); - double value = instance.getValue(); - switch (between(0, 1)) { - case 0: - Operator newOp = op; - while (newOp == op) { - newOp = randomFrom(Operator.values()); - } - op = newOp; - break; - case 1: - value = randomDouble(); - break; - default: - throw new AssertionError("Illegal randomisation branch"); - } - return new Condition(op, value); - } -} diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ControlMsgToProcessWriterTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ControlMsgToProcessWriterTests.java index ede2478bf32e5..8c32a5bb40d46 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ControlMsgToProcessWriterTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ControlMsgToProcessWriterTests.java @@ -8,7 +8,6 @@ import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.core.ml.calendars.ScheduledEvent; -import org.elasticsearch.xpack.core.ml.job.config.Condition; import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; import org.elasticsearch.xpack.core.ml.job.config.MlFilter; import org.elasticsearch.xpack.core.ml.job.config.ModelPlotConfig; @@ -199,9 +198,9 @@ public void testWriteUpdateDetectorRulesMessage() throws IOException { inOrder.verify(lengthEncodedWriter, times(3)).writeField(""); inOrder.verify(lengthEncodedWriter).writeField("u[detectorRules]\ndetectorIndex=2\n" + "rulesJson=[{\"actions\":[\"skip_result\"],\"conditions\":" + - "[{\"applies_to\":\"actual\",\"condition\":{\"operator\":\"gt\",\"value\":5.0}}]}," + + "[{\"applies_to\":\"actual\",\"operator\":\"gt\",\"value\":5.0}]}," + "{\"actions\":[\"skip_result\"],\"conditions\":[" + - "{\"applies_to\":\"actual\",\"condition\":{\"operator\":\"gt\",\"value\":5.0}}]}]"); + "{\"applies_to\":\"actual\",\"operator\":\"gt\",\"value\":5.0}]}]"); verifyNoMoreInteractions(lengthEncodedWriter); } @@ -245,12 +244,12 @@ public void testWriteUpdateScheduledEventsMessage() throws IOException { assertThat(capturedMessage.getValue(), equalTo("u[scheduledEvents]\n" + "scheduledevent.0.description = new year\n" + "scheduledevent.0.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"]," - + "\"conditions\":[{\"applies_to\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":1.5147648E9}}," - + "{\"applies_to\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":1.5148512E9}}]}]\n" + + "\"conditions\":[{\"applies_to\":\"time\",\"operator\":\"gte\",\"value\":1.5147648E9}," + + "{\"applies_to\":\"time\",\"operator\":\"lt\",\"value\":1.5148512E9}]}]\n" + "scheduledevent.1.description = Jan maintenance day\n" + "scheduledevent.1.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"]," - + "\"conditions\":[{\"applies_to\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":1.5151968E9}}," - + "{\"applies_to\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":1.5152832E9}}]}]\n")); + + "\"conditions\":[{\"applies_to\":\"time\",\"operator\":\"gte\",\"value\":1.5151968E9}," + + "{\"applies_to\":\"time\",\"operator\":\"lt\",\"value\":1.5152832E9}]}]\n")); verifyNoMoreInteractions(lengthEncodedWriter); } @@ -286,7 +285,6 @@ public void testWriteStartBackgroundPersistMessage() throws IOException { } private static List createRule(double value) { - Condition condition = new Condition(Operator.GT, value); - return Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, condition)); + return Collections.singletonList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, value)); } } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriterTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriterTests.java index 6e40a0c14b54c..bf08d09bf090c 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriterTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/FieldConfigWriterTests.java @@ -12,7 +12,6 @@ import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.core.ml.calendars.ScheduledEvent; import org.elasticsearch.xpack.core.ml.job.config.AnalysisConfig; -import org.elasticsearch.xpack.core.ml.job.config.Condition; import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; import org.elasticsearch.xpack.core.ml.job.config.Detector; import org.elasticsearch.xpack.core.ml.job.config.MlFilter; @@ -192,7 +191,7 @@ public void testWrite_GivenDetectorWithRules() throws IOException { Detector.Builder detector = new Detector.Builder("mean", "metricValue"); detector.setByFieldName("metricName"); detector.setPartitionFieldName("instance"); - RuleCondition ruleCondition = new RuleCondition(RuleCondition.AppliesTo.ACTUAL, new Condition(Operator.LT, 5)); + RuleCondition ruleCondition = new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.LT, 5); DetectionRule rule = new DetectionRule.Builder(Collections.singletonList(ruleCondition)).build(); detector.setRules(Collections.singletonList(rule)); @@ -254,12 +253,11 @@ public void testWrite_GivenScheduledEvents() throws IOException { verify(writer).write("detector.0.clause = count\n" + "scheduledevent.0.description = The Ashes\n" + "scheduledevent.0.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"],\"conditions\":[{\"applies_to\":\"time\"," + - "\"condition\":{\"operator\":\"gte\",\"value\":1.5113952E9}},{\"applies_to\":\"time\",\"condition\":" + - "{\"operator\":\"lt\",\"value\":1.5153696E9}}]}]\n" + + "\"operator\":\"gte\",\"value\":1.5113952E9},{\"applies_to\":\"time\",\"operator\":\"lt\",\"value\":1.5153696E9}]}]\n" + "scheduledevent.1.description = elasticon\n" + "scheduledevent.1.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"]," + - "\"conditions\":[{\"applies_to\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":1.5196032E9}}," + - "{\"applies_to\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":1.5198624E9}}]}]\n"); + "\"conditions\":[{\"applies_to\":\"time\",\"operator\":\"gte\",\"value\":1.5196032E9}," + + "{\"applies_to\":\"time\",\"operator\":\"lt\",\"value\":1.5198624E9}]}]\n"); verifyNoMoreInteractions(writer); } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ScheduledEventsWriterTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ScheduledEventsWriterTests.java index b5304c9e47365..2806aef128575 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ScheduledEventsWriterTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/writer/ScheduledEventsWriterTests.java @@ -45,12 +45,12 @@ public void testWrite() throws IOException { String expectedString = "scheduledevent.0.description = Black Friday\n" + "scheduledevent.0.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"]," + - "\"conditions\":[{\"applies_to\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":1.5113952E9}}," + - "{\"applies_to\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":1.5153696E9}}]}]\n" + + "\"conditions\":[{\"applies_to\":\"time\",\"operator\":\"gte\",\"value\":1.5113952E9}," + + "{\"applies_to\":\"time\",\"operator\":\"lt\",\"value\":1.5153696E9}]}]\n" + "scheduledevent.1.description = Blue Monday\n" + "scheduledevent.1.rules = [{\"actions\":[\"skip_result\",\"skip_model_update\"]," + - "\"conditions\":[{\"applies_to\":\"time\",\"condition\":{\"operator\":\"gte\",\"value\":1.5196032E9}}," + - "{\"applies_to\":\"time\",\"condition\":{\"operator\":\"lt\",\"value\":1.5198624E9}}]}]" + + "\"conditions\":[{\"applies_to\":\"time\",\"operator\":\"gte\",\"value\":1.5196032E9}," + + "{\"applies_to\":\"time\",\"operator\":\"lt\",\"value\":1.5198624E9}]}]" + "\n"; assertThat(buffer.toString(), equalTo(expectedString)); } diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/jobs_crud.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/jobs_crud.yml index 8a47dc845141b..df505176ae739 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/jobs_crud.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/jobs_crud.yml @@ -309,7 +309,8 @@ "conditions": [ { "applies_to": "actual", - "condition": {"operator": "gt", "value": 10 } + "operator": "gt", + "value": 10 } ] } @@ -1248,11 +1249,13 @@ "conditions": [ { "applies_to":"actual", - "condition": {"operator":"lt","value": 33.3} + "operator":"lt", + "value": 33.3 }, { "applies_to":"typical", - "condition": {"operator":"lte","value": 42.0} + "operator":"lte", + "value": 42.0 } ] } @@ -1277,11 +1280,13 @@ "conditions": [ { "applies_to":"actual", - "condition": {"operator":"lt","value": 33.3} + "operator":"lt", + "value": 33.3 }, { "applies_to":"typical", - "condition": {"operator":"lte","value": 42.0} + "operator":"lte", + "value": 42.0 } ] } diff --git a/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/DetectionRulesIT.java b/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/DetectionRulesIT.java index 6fabb2550ec05..aa53d6255cb8e 100644 --- a/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/DetectionRulesIT.java +++ b/x-pack/qa/ml-native-tests/src/test/java/org/elasticsearch/xpack/ml/integration/DetectionRulesIT.java @@ -12,7 +12,6 @@ import org.elasticsearch.search.sort.SortOrder; import org.elasticsearch.xpack.core.ml.action.GetRecordsAction; import org.elasticsearch.xpack.core.ml.job.config.AnalysisConfig; -import org.elasticsearch.xpack.core.ml.job.config.Condition; import org.elasticsearch.xpack.core.ml.job.config.DataDescription; import org.elasticsearch.xpack.core.ml.job.config.DetectionRule; import org.elasticsearch.xpack.core.ml.job.config.Detector; @@ -51,7 +50,7 @@ public void cleanUpTest() { public void testCondition() throws Exception { DetectionRule rule = new DetectionRule.Builder(Arrays.asList( - new RuleCondition(RuleCondition.AppliesTo.ACTUAL, new Condition(Operator.LT, 100.0)) + new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.LT, 100.0) )).build(); Detector.Builder detector = new Detector.Builder("mean", "value"); @@ -101,7 +100,7 @@ public void testCondition() throws Exception { { // Update rules so that the anomalies suppression is inverted DetectionRule newRule = new DetectionRule.Builder(Arrays.asList( - new RuleCondition(RuleCondition.AppliesTo.ACTUAL, new Condition(Operator.GT, 700.0)) + new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, 700.0) )).build(); JobUpdate.Builder update = new JobUpdate.Builder(job.getId()); update.setDetectorUpdates(Arrays.asList(new JobUpdate.DetectorUpdate(0, null, Arrays.asList(newRule)))); @@ -235,7 +234,7 @@ public void testScopeAndCondition() throws IOException { // Ignore if ip in safe list AND actual < 10. DetectionRule rule = new DetectionRule.Builder(RuleScope.builder().include("ip", "safe_ips")) - .setConditions(Arrays.asList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, new Condition(Operator.LT, 10.0)))) + .setConditions(Arrays.asList(new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.LT, 10.0))) .build(); Detector.Builder detector = new Detector.Builder("count", null); From 289856723a847fe36ea86d4ffd8f080cd01accad Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Wed, 6 Jun 2018 14:46:43 +0100 Subject: [PATCH 3/8] Address review comments --- .../xpack/core/ml/job/config/Detector.java | 2 +- .../xpack/core/ml/job/config/FilterRef.java | 4 + .../ml/action/TransportOpenJobAction.java | 25 ++-- .../action/TransportOpenJobActionTests.java | 136 ++++++++++-------- 4 files changed, 90 insertions(+), 77 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java index f175a4e6336ff..78dbe26e7b9f2 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java @@ -714,7 +714,7 @@ private static boolean ruleHasConditionOnResultValue(DetectionRule rule) { return true; case TIME: default: - return false; + throw new IllegalStateException("Unknown applies_to value [" + condition.getAppliesTo() + "]"); } } return false; diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/FilterRef.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/FilterRef.java index c2b1cd3384bb9..7f3fb56287965 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/FilterRef.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/FilterRef.java @@ -116,4 +116,8 @@ public int hashCode() { public String getFilterId() { return filterId; } + + public FilterType getFilterType() { + return filterType; + } } diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java index 3c44ca235efdd..5de7962169279 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportOpenJobAction.java @@ -191,6 +191,14 @@ static PersistentTasksCustomMetaData.Assignment selectLeastLoadedMlNode(String j continue; } + if (jobHasRules(job) && node.getVersion().before(DetectionRule.VERSION_INTRODUCED)) { + String reason = "Not opening job [" + jobId + "] on node [" + nodeNameAndVersion(node) + "], because jobs using " + + "custom_rules require a node of version [" + DetectionRule.VERSION_INTRODUCED + "] or higher"; + logger.trace(reason); + reasons.add(reason); + continue; + } + long numberOfAssignedJobs = 0; int numberOfAllocatingJobs = 0; long assignedJobMemory = 0; @@ -374,6 +382,10 @@ private static boolean nodeSupportsModelSnapshotVersion(DiscoveryNode node, Job return node.getVersion().onOrAfter(job.getModelSnapshotMinVersion()); } + private static boolean jobHasRules(Job job) { + return job.getAnalysisConfig().getDetectors().stream().anyMatch(d -> d.getRules().isEmpty() == false); + } + static String[] mappingRequiresUpdate(ClusterState state, String[] concreteIndices, Version minVersion, Logger logger) throws IOException { List indicesToUpdate = new ArrayList<>(); @@ -649,7 +661,6 @@ public PersistentTasksCustomMetaData.Assignment getAssignment(OpenJobAction.JobP public void validate(OpenJobAction.JobParams params, ClusterState clusterState) { TransportOpenJobAction.validate(params.getJobId(), MlMetadata.getMlMetadata(clusterState)); - checkJobWithRulesRequiresMinVersionOnAllNodes(params.getJobId(), clusterState); // If we already know that we can't find an ml node because all ml nodes are running at capacity or // simply because there are no ml nodes in the cluster then we fail quickly here: @@ -704,18 +715,6 @@ void setMaxMachineMemoryPercent(int maxMachineMemoryPercent) { } } - // Visible for testing - static void checkJobWithRulesRequiresMinVersionOnAllNodes(String jobId, ClusterState clusterState) { - // Jobs with rules should not be allowed to open unless the whole cluster has the min required version - Job job = MlMetadata.getMlMetadata(clusterState).getJobs().get(jobId); - if (job.getAnalysisConfig().getDetectors().stream().anyMatch(d -> d.getRules().isEmpty() == false) - && clusterState.nodes().getMinNodeVersion().before(DetectionRule.VERSION_INTRODUCED)) { - String msg = "Cannot open job [" + job.getId() + "] because jobs using custom_rules require all nodes to be " + - "on version [" + DetectionRule.VERSION_INTRODUCED + "] or higher"; - throw ExceptionsHelper.badRequestException(msg); - } - } - public static class JobTask extends AllocatedPersistentTask implements OpenJobAction.JobTaskMatcher { private static final Logger LOGGER = Loggers.getLogger(JobTask.class); diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportOpenJobActionTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportOpenJobActionTests.java index c28207332ad3c..6ef2d92d9c7c6 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportOpenJobActionTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/action/TransportOpenJobActionTests.java @@ -67,7 +67,6 @@ import static org.elasticsearch.xpack.core.ml.job.config.JobTests.buildJobBuilder; import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.equalTo; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -435,6 +434,62 @@ public void testSelectLeastLoadedMlNode_noNodesMatchingModelSnapshotMinVersion() assertNull(result.getExecutorNode()); } + public void testSelectLeastLoadedMlNode_jobWithRulesButNoNodeMeetsRequiredVersion() { + Map nodeAttr = new HashMap<>(); + nodeAttr.put(MachineLearning.ML_ENABLED_NODE_ATTR, "true"); + DiscoveryNodes nodes = DiscoveryNodes.builder() + .add(new DiscoveryNode("_node_name1", "_node_id1", new TransportAddress(InetAddress.getLoopbackAddress(), 9300), + nodeAttr, Collections.emptySet(), Version.V_6_2_0)) + .add(new DiscoveryNode("_node_name2", "_node_id2", new TransportAddress(InetAddress.getLoopbackAddress(), 9301), + nodeAttr, Collections.emptySet(), Version.V_6_3_0)) + .build(); + + PersistentTasksCustomMetaData.Builder tasksBuilder = PersistentTasksCustomMetaData.builder(); + addJobTask("job_with_rules", "_node_id1", null, tasksBuilder); + PersistentTasksCustomMetaData tasks = tasksBuilder.build(); + + ClusterState.Builder cs = ClusterState.builder(new ClusterName("_name")); + MetaData.Builder metaData = MetaData.builder(); + RoutingTable.Builder routingTable = RoutingTable.builder(); + addJobAndIndices(metaData, routingTable, jobWithRulesCreator(), "job_with_rules"); + cs.nodes(nodes); + metaData.putCustom(PersistentTasksCustomMetaData.TYPE, tasks); + cs.metaData(metaData); + cs.routingTable(routingTable.build()); + Assignment result = TransportOpenJobAction.selectLeastLoadedMlNode("job_with_rules", cs.build(), + 2, 10, 30, logger); + assertThat(result.getExplanation(), containsString( + "because jobs using custom_rules require a node of version [6.4.0] or higher")); + assertNull(result.getExecutorNode()); + } + + public void testSelectLeastLoadedMlNode_jobWithRulesAndNodeMeetsRequiredVersion() { + Map nodeAttr = new HashMap<>(); + nodeAttr.put(MachineLearning.ML_ENABLED_NODE_ATTR, "true"); + DiscoveryNodes nodes = DiscoveryNodes.builder() + .add(new DiscoveryNode("_node_name1", "_node_id1", new TransportAddress(InetAddress.getLoopbackAddress(), 9300), + nodeAttr, Collections.emptySet(), Version.V_6_2_0)) + .add(new DiscoveryNode("_node_name2", "_node_id2", new TransportAddress(InetAddress.getLoopbackAddress(), 9301), + nodeAttr, Collections.emptySet(), Version.V_6_4_0)) + .build(); + + PersistentTasksCustomMetaData.Builder tasksBuilder = PersistentTasksCustomMetaData.builder(); + addJobTask("job_with_rules", "_node_id1", null, tasksBuilder); + PersistentTasksCustomMetaData tasks = tasksBuilder.build(); + + ClusterState.Builder cs = ClusterState.builder(new ClusterName("_name")); + MetaData.Builder metaData = MetaData.builder(); + RoutingTable.Builder routingTable = RoutingTable.builder(); + addJobAndIndices(metaData, routingTable, jobWithRulesCreator(), "job_with_rules"); + cs.nodes(nodes); + metaData.putCustom(PersistentTasksCustomMetaData.TYPE, tasks); + cs.metaData(metaData); + cs.routingTable(routingTable.build()); + Assignment result = TransportOpenJobAction.selectLeastLoadedMlNode("job_with_rules", cs.build(), + 2, 10, 30, logger); + assertNotNull(result.getExecutorNode()); + } + public void testVerifyIndicesPrimaryShardsAreActive() { MetaData.Builder metaData = MetaData.builder(); RoutingTable.Builder routingTable = RoutingTable.builder(); @@ -571,68 +626,6 @@ public void testNodeNameAndMlAttributes() { assertEquals("{_node_name1}{ml.machine_memory=5}{node.ml=true}", TransportOpenJobAction.nodeNameAndMlAttributes(node)); } - public void testCheckJobWithRulesRequiresMinVersionOnAllNodes_GivenNodeDoesNotMeetRequiredVersion() { - DetectionRule rule = new DetectionRule.Builder(Arrays.asList( - new RuleCondition(RuleCondition.AppliesTo.TYPICAL, Operator.LT, 100.0) - )).build(); - - Detector.Builder detector = new Detector.Builder("count", null); - detector.setRules(Arrays.asList(rule)); - AnalysisConfig.Builder analysisConfig = new AnalysisConfig.Builder(Arrays.asList(detector.build())); - DataDescription.Builder dataDescription = new DataDescription.Builder(); - Job.Builder job = new Job.Builder("foo"); - job.setAnalysisConfig(analysisConfig); - job.setDataDescription(dataDescription); - - MetaData metaData = MetaData.builder() - .putCustom(MLMetadataField.TYPE, new MlMetadata.Builder().putJob(job.build(new Date()), false).build()) - .build(); - - ClusterState cs = ClusterState.builder(new ClusterName("_name")) - .nodes(DiscoveryNodes.builder() - .add(new DiscoveryNode("_old_node", new TransportAddress(InetAddress.getLoopbackAddress(), 9200), Version.V_6_3_0)) - .add(new DiscoveryNode("_new_node", new TransportAddress(InetAddress.getLoopbackAddress(), 9201), Version.CURRENT)) - .localNodeId("_new_node") - .masterNodeId("_new_node")) - .metaData(metaData) - .build(); - - ElasticsearchStatusException e = expectThrows(ElasticsearchStatusException.class, - () -> TransportOpenJobAction.checkJobWithRulesRequiresMinVersionOnAllNodes("foo", cs)); - - assertThat(e.getMessage(), equalTo("Cannot open job [foo] because jobs using custom_rules " + - "require all nodes to be on version [6.4.0] or higher")); - } - - public void testCheckJobWithRulesRequiresMinVersionOnAllNodes_GivenAllNodesMeetRequiredVersion() { - DetectionRule rule = new DetectionRule.Builder(Arrays.asList( - new RuleCondition(RuleCondition.AppliesTo.TYPICAL, Operator.LT, 100.0) - )).build(); - - Detector.Builder detector = new Detector.Builder("count", null); - detector.setRules(Arrays.asList(rule)); - AnalysisConfig.Builder analysisConfig = new AnalysisConfig.Builder(Arrays.asList(detector.build())); - DataDescription.Builder dataDescription = new DataDescription.Builder(); - Job.Builder job = new Job.Builder("foo"); - job.setAnalysisConfig(analysisConfig); - job.setDataDescription(dataDescription); - - MetaData metaData = MetaData.builder() - .putCustom(MLMetadataField.TYPE, new MlMetadata.Builder().putJob(job.build(new Date()), false).build()) - .build(); - - ClusterState cs = ClusterState.builder(new ClusterName("_name")) - .nodes(DiscoveryNodes.builder() - .add(new DiscoveryNode("_old_node", new TransportAddress(InetAddress.getLoopbackAddress(), 9200), Version.V_6_4_0)) - .add(new DiscoveryNode("_new_node", new TransportAddress(InetAddress.getLoopbackAddress(), 9201), Version.CURRENT)) - .localNodeId("_new_node") - .masterNodeId("_new_node")) - .metaData(metaData) - .build(); - - TransportOpenJobAction.checkJobWithRulesRequiresMinVersionOnAllNodes("foo", cs); - } - public static void addJobTask(String jobId, String nodeId, JobState jobState, PersistentTasksCustomMetaData.Builder builder) { builder.addTask(MlMetadata.jobTaskId(jobId), OpenJobAction.TASK_NAME, new OpenJobAction.JobParams(jobId), new Assignment(nodeId, "test assignment")); @@ -715,4 +708,21 @@ private ClusterState getClusterStateWithMappingsWithMetaData(Map return csBuilder.build(); } + private static Function jobWithRulesCreator() { + return jobId -> { + DetectionRule rule = new DetectionRule.Builder(Arrays.asList( + new RuleCondition(RuleCondition.AppliesTo.TYPICAL, Operator.LT, 100.0) + )).build(); + + Detector.Builder detector = new Detector.Builder("count", null); + detector.setRules(Arrays.asList(rule)); + AnalysisConfig.Builder analysisConfig = new AnalysisConfig.Builder(Arrays.asList(detector.build())); + DataDescription.Builder dataDescription = new DataDescription.Builder(); + Job.Builder job = new Job.Builder(jobId); + job.setAnalysisConfig(analysisConfig); + job.setDataDescription(dataDescription); + return job.build(new Date()); + }; + } + } From e8b42820a206ef133f665fc28b018cda5175400e Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Thu, 7 Jun 2018 12:19:40 +0100 Subject: [PATCH 4/8] Fix time condition function support check --- .../xpack/core/ml/job/config/Detector.java | 33 +++-- .../xpack/core/ml/job/config/RuleScope.java | 2 +- .../xpack/core/ml/job/messages/Messages.java | 6 +- .../core/ml/job/config/DetectorTests.java | 113 +++++++++++++----- 4 files changed, 101 insertions(+), 53 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java index 78dbe26e7b9f2..4b3029dfbc448 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java @@ -209,6 +209,19 @@ public String toString() { DetectorFunction.TIME_OF_WEEK ); + /** + * Functions that do not support rule conditions: + *
    + *
  • lat_long - because it is a multivariate feature + *
  • metric - because having the same conditions on min,max,mean is + * error-prone + *
  • rare - because the actual/typical value is not something a user can anticipate + *
  • freq_rare - because the actual/typical value is not something a user can anticipate + *
+ */ + static final EnumSet FUNCTIONS_WITHOUT_RULE_CONDITION_SUPPORT = EnumSet.of( + DetectorFunction.LAT_LONG, DetectorFunction.METRIC, DetectorFunction.RARE, DetectorFunction.FREQ_RARE); + /** * field names cannot contain any of these characters * ", \ @@ -467,19 +480,6 @@ public int hashCode() { public static class Builder { - /** - * Functions that do not support rule conditions: - *
    - *
  • lat_long - because it is a multivariate feature - *
  • metric - because having the same conditions on min,max,mean is - * error-prone - *
  • rare - because the actual/typical value is not something a user can anticipate - *
  • freq_rare - because the actual/typical value is not something a user can anticipate - *
- */ - static final EnumSet FUNCTIONS_WITHOUT_RULE_CONDITION_SUPPORT = EnumSet.of( - DetectorFunction.LAT_LONG, DetectorFunction.METRIC, DetectorFunction.RARE, DetectorFunction.FREQ_RARE); - private String detectorDescription; private DetectorFunction function; private String fieldName; @@ -603,12 +603,6 @@ public Detector build() { for (DetectionRule rule : rules) { validateRule(rule, function); } - if (rules.isEmpty() == false) { - if (FUNCTIONS_WITHOUT_RULE_CONDITION_SUPPORT.contains(function)) { - String msg = Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_NOT_SUPPORTED_BY_FUNCTION, function); - throw ExceptionsHelper.badRequestException(msg); - } - } // partition, by and over field names cannot be duplicates if (!emptyPartitionField) { @@ -713,6 +707,7 @@ private static boolean ruleHasConditionOnResultValue(DetectionRule rule) { case DIFF_FROM_TYPICAL: return true; case TIME: + return false; default: throw new IllegalStateException("Unknown applies_to value [" + condition.getAppliesTo() + "]"); } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleScope.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleScope.java index 3b6ebbb274408..553921f7ea933 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleScope.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleScope.java @@ -55,7 +55,7 @@ public boolean isEmpty() { public void validate(Set validKeys) { Optional invalidKey = scope.keySet().stream().filter(k -> !validKeys.contains(k)).findFirst(); if (invalidKey.isPresent()) { - throw ExceptionsHelper.badRequestException(Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_SCOPE_HAS_INVALID_KEY, + throw ExceptionsHelper.badRequestException(Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_SCOPE_HAS_INVALID_FIELD, invalidKey.get(), validKeys)); } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java index 2ad08ec29ec0b..79d8f068d91f8 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java @@ -89,11 +89,11 @@ public final class Messages { public static final String JOB_CONFIG_CATEGORIZATION_ANALYZER_REQUIRES_CATEGORIZATION_FIELD_NAME = "categorization_analyzer requires setting categorization_field_name"; public static final String JOB_CONFIG_DETECTION_RULE_NOT_SUPPORTED_BY_FUNCTION = - "Invalid detector rule: function {0} does not support conditions"; + "Invalid detector rule: function {0} does not support rules with conditions"; public static final String JOB_CONFIG_DETECTION_RULE_REQUIRES_SCOPE_OR_CONDITION = "Invalid detector rule: at least scope or a condition is required"; - public static final String JOB_CONFIG_DETECTION_RULE_SCOPE_HAS_INVALID_KEY = - "Invalid detector rule: scope key ''{0}'' is invalid; select from {1}"; + public static final String JOB_CONFIG_DETECTION_RULE_SCOPE_HAS_INVALID_FIELD = + "Invalid detector rule: scope field ''{0}'' is invalid; select from {1}"; public static final String JOB_CONFIG_FIELDNAME_INCOMPATIBLE_FUNCTION = "field_name cannot be used with function ''{0}''"; public static final String JOB_CONFIG_FIELD_VALUE_TOO_LOW = "{0} cannot be less than {1,number}. Value = {2,number}"; public static final String JOB_CONFIG_MODEL_MEMORY_LIMIT_TOO_LOW = "model_memory_limit must be at least 1 MiB. Value = {0,number}"; diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectorTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectorTests.java index d03390ce6110f..c9be0d3b89248 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectorTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/DetectorTests.java @@ -145,24 +145,22 @@ protected Detector createTestInstance() { detector = new Detector.Builder(function = randomFrom(Detector.COUNT_WITHOUT_FIELD_FUNCTIONS), null); } else { EnumSet functions = EnumSet.copyOf(Detector.FIELD_NAME_FUNCTIONS); - functions.removeAll(Detector.Builder.FUNCTIONS_WITHOUT_RULE_CONDITION_SUPPORT); detector = new Detector.Builder(function = randomFrom(functions), randomAlphaOfLengthBetween(1, 20)); } if (randomBoolean()) { detector.setDetectorDescription(randomAlphaOfLengthBetween(1, 20)); } - String fieldName = null; if (randomBoolean()) { - detector.setPartitionFieldName(fieldName = randomAlphaOfLengthBetween(6, 20)); + detector.setPartitionFieldName(randomAlphaOfLengthBetween(6, 20)); } else if (randomBoolean() && Detector.NO_OVER_FIELD_NAME_FUNCTIONS.contains(function) == false) { - detector.setOverFieldName(fieldName = randomAlphaOfLengthBetween(6, 20)); + detector.setOverFieldName(randomAlphaOfLengthBetween(6, 20)); } else if (randomBoolean()) { - detector.setByFieldName(fieldName = randomAlphaOfLengthBetween(6, 20)); + detector.setByFieldName(randomAlphaOfLengthBetween(6, 20)); } if (randomBoolean()) { detector.setExcludeFrequent(randomFrom(Detector.ExcludeFrequent.values())); } - if (randomBoolean()) { + if (Detector.FUNCTIONS_WITHOUT_RULE_CONDITION_SUPPORT.contains(function) == false && randomBoolean()) { int size = randomInt(10); List rules = new ArrayList<>(size); for (int i = 0; i < size; i++) { @@ -445,15 +443,6 @@ public void testVerify_GivenFunctionsThatCanHaveByField() { } } - public void testVerify_GivenValidRule() { - Detector.Builder detector = new Detector.Builder("mean", "metricVale"); - detector.setByFieldName("metricName"); - detector.setPartitionFieldName("instance"); - DetectionRule rule = new DetectionRule.Builder(Collections.singletonList(RuleConditionTests.createRandom())).build(); - detector.setRules(Collections.singletonList(rule)); - detector.build(); - } - public void testVerify_GivenAllPartitioningFieldsAreScoped() { Detector.Builder detector = new Detector.Builder("count", null); detector.setPartitionFieldName("my_partition"); @@ -470,21 +459,6 @@ public void testVerify_GivenAllPartitioningFieldsAreScoped() { detector.build(); } - public void testVerify_GivenCategoricalRuleOnInvalidField() { - Detector.Builder detector = new Detector.Builder("mean", "my_metric"); - detector.setPartitionFieldName("my_partition"); - detector.setOverFieldName("my_over"); - detector.setByFieldName("my_by"); - - DetectionRule rule = new DetectionRule.Builder(RuleScope.builder().exclude("my_metric", "my_filter_id")).build(); - detector.setRules(Collections.singletonList(rule)); - - ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); - - assertEquals(Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_SCOPE_HAS_INVALID_KEY, - "my_metric", "[my_by, my_over, my_partition]"), e.getMessage()); - } - public void testVerify_GivenSameByAndPartition() { Detector.Builder detector = new Detector.Builder("count", ""); detector.setByFieldName("x"); @@ -560,6 +534,85 @@ public void testVerify_GivenOverIsOver() { assertEquals("'over' is not a permitted value for over_field_name", e.getMessage()); } + public void testVerify_GivenRulesAndFunctionIsLatLong() { + Detector.Builder detector = new Detector.Builder("lat_long", "geo"); + detector.setRules(Collections.singletonList(new DetectionRule.Builder(Collections.singletonList( + new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, 42.0))).build())); + + ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); + + assertThat(e.getMessage(), equalTo("Invalid detector rule: function lat_long does not support rules with conditions")); + } + + public void testVerify_GivenRulesAndFunctionIsMetric() { + Detector.Builder detector = new Detector.Builder("metric", "some_metric"); + detector.setRules(Collections.singletonList(new DetectionRule.Builder(Collections.singletonList( + new RuleCondition(RuleCondition.AppliesTo.TYPICAL, Operator.GT, 42.0))).build())); + + ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); + + assertThat(e.getMessage(), equalTo("Invalid detector rule: function metric does not support rules with conditions")); + } + + public void testVerify_GivenRulesAndFunctionIsRare() { + Detector.Builder detector = new Detector.Builder("rare", null); + detector.setByFieldName("some_field"); + detector.setRules(Collections.singletonList(new DetectionRule.Builder(Collections.singletonList( + new RuleCondition(RuleCondition.AppliesTo.DIFF_FROM_TYPICAL, Operator.GT, 42.0))).build())); + + ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); + + assertThat(e.getMessage(), equalTo("Invalid detector rule: function rare does not support rules with conditions")); + } + + public void testVerify_GivenRulesAndFunctionIsFreqRare() { + Detector.Builder detector = new Detector.Builder("freq_rare", null); + detector.setByFieldName("some_field"); + detector.setOverFieldName("some_field2"); + detector.setRules(Collections.singletonList(new DetectionRule.Builder(Collections.singletonList( + new RuleCondition(RuleCondition.AppliesTo.ACTUAL, Operator.GT, 42.0))).build())); + + ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); + + assertThat(e.getMessage(), equalTo("Invalid detector rule: function freq_rare does not support rules with conditions")); + } + + public void testVerify_GivenTimeConditionRuleAndFunctionIsLatLong() { + Detector.Builder detector = new Detector.Builder("lat_long", "geo"); + detector.setRules(Collections.singletonList(new DetectionRule.Builder(Collections.singletonList( + new RuleCondition(RuleCondition.AppliesTo.TIME, Operator.GT, 42.0))).build())); + detector.build(); + } + + public void testVerify_GivenScopeRuleOnInvalidField() { + Detector.Builder detector = new Detector.Builder("mean", "my_metric"); + detector.setPartitionFieldName("my_partition"); + detector.setOverFieldName("my_over"); + detector.setByFieldName("my_by"); + + DetectionRule rule = new DetectionRule.Builder(RuleScope.builder().exclude("my_metric", "my_filter_id")).build(); + detector.setRules(Collections.singletonList(rule)); + + ElasticsearchException e = ESTestCase.expectThrows(ElasticsearchException.class, detector::build); + + assertEquals(Messages.getMessage(Messages.JOB_CONFIG_DETECTION_RULE_SCOPE_HAS_INVALID_FIELD, + "my_metric", "[my_by, my_over, my_partition]"), e.getMessage()); + } + + public void testVerify_GivenValidRule() { + Detector.Builder detector = new Detector.Builder("mean", "metricVale"); + detector.setByFieldName("metricName"); + detector.setPartitionFieldName("instance"); + DetectionRule rule = new DetectionRule.Builder(Collections.singletonList(RuleConditionTests.createRandom())) + .setScope(RuleScope.builder() + .include("metricName", "f1") + .exclude("instance", "f2") + .build()) + .build(); + detector.setRules(Collections.singletonList(rule)); + detector.build(); + } + public void testExcludeFrequentForString() { assertEquals(Detector.ExcludeFrequent.ALL, Detector.ExcludeFrequent.forString("all")); assertEquals(Detector.ExcludeFrequent.ALL, Detector.ExcludeFrequent.forString("ALL")); From cebce617281da8492fd750a521e6c71cf1b6d29d Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Thu, 7 Jun 2018 12:54:46 +0100 Subject: [PATCH 5/8] Write empty rules list to nodes with earlier versions --- .../xpack/core/ml/job/config/DetectionRule.java | 8 +++----- .../elasticsearch/xpack/core/ml/job/config/Detector.java | 6 +++++- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRule.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRule.java index 636adf6830c7f..dc6eb2ea182f9 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRule.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRule.java @@ -98,11 +98,9 @@ public DetectionRule(StreamInput in) throws IOException { @Override public void writeTo(StreamOutput out) throws IOException { - if (out.getVersion().onOrAfter(Version.V_6_4_0)) { - out.writeEnumSet(actions); - scope.writeTo(out); - out.writeList(conditions); - } + out.writeEnumSet(actions); + scope.writeTo(out); + out.writeList(conditions); } @Override diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java index 4b3029dfbc448..bae5e654ba4fa 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Detector.java @@ -276,7 +276,11 @@ public void writeTo(StreamOutput out) throws IOException { } else { out.writeBoolean(false); } - out.writeList(rules); + if (out.getVersion().onOrAfter(DetectionRule.VERSION_INTRODUCED)) { + out.writeList(rules); + } else { + out.writeList(Collections.emptyList()); + } if (out.getVersion().onOrAfter(Version.V_5_5_0)) { out.writeInt(detectorIndex); } From cb6f3bee1e705d5b9582e7729d1e701b5656dac2 Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Thu, 7 Jun 2018 13:09:51 +0100 Subject: [PATCH 6/8] Extract RuleScope parser --- .../core/ml/job/config/DetectionRule.java | 26 +---------------- .../xpack/core/ml/job/config/RuleScope.java | 29 +++++++++++++++++++ 2 files changed, 30 insertions(+), 25 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRule.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRule.java index dc6eb2ea182f9..fbdb2f6662a77 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRule.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/DetectionRule.java @@ -7,18 +7,12 @@ import org.elasticsearch.Version; import org.elasticsearch.common.ParseField; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; -import org.elasticsearch.common.xcontent.ConstructingObjectParser; -import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; -import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentFactory; -import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.xpack.core.ml.MlParserType; import org.elasticsearch.xpack.core.ml.job.messages.Messages; import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper; @@ -28,7 +22,6 @@ import java.util.Collections; import java.util.EnumMap; import java.util.EnumSet; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -57,24 +50,7 @@ public class DetectionRule implements ToXContentObject, Writeable { ObjectParser parser = PARSERS.get(parserType); assert parser != null; parser.declareStringArray(Builder::setActions, ACTIONS_FIELD); - parser.declareObject(Builder::setScope, (p, c) -> { - Map unparsedScope = p.map(); - if (unparsedScope.isEmpty()) { - return new RuleScope(); - } - ConstructingObjectParser filterRefParser = FilterRef.PARSERS.get(parserType); - Map scope = new HashMap<>(); - for (Map.Entry entry : unparsedScope.entrySet()) { - try (XContentBuilder builder = XContentFactory.jsonBuilder()) { - builder.map((Map) entry.getValue()); - try (XContentParser scopeParser = XContentFactory.xContent(builder.contentType()).createParser( - NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, Strings.toString(builder))) { - scope.put(entry.getKey(), filterRefParser.parse(scopeParser, null)); - } - } - } - return new RuleScope(scope); - }, SCOPE_FIELD); + parser.declareObject(Builder::setScope, RuleScope.parser(parserType), SCOPE_FIELD); parser.declareObjectArray(Builder::setConditions, (p, c) -> RuleCondition.PARSERS.get(parserType).apply(p, c), CONDITIONS_FIELD); } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleScope.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleScope.java index 553921f7ea933..b6b3b4e061bdd 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleScope.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/RuleScope.java @@ -5,11 +5,19 @@ */ package org.elasticsearch.xpack.core.ml.job.config; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.ConstructingObjectParser; +import org.elasticsearch.common.xcontent.ContextParser; +import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.xpack.core.ml.MlParserType; import org.elasticsearch.xpack.core.ml.job.messages.Messages; import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper; @@ -24,6 +32,27 @@ public class RuleScope implements ToXContentObject, Writeable { + public static ContextParser parser(MlParserType parserType) { + return (p, c) -> { + Map unparsedScope = p.map(); + if (unparsedScope.isEmpty()) { + return new RuleScope(); + } + ConstructingObjectParser filterRefParser = FilterRef.PARSERS.get(parserType); + Map scope = new HashMap<>(); + for (Map.Entry entry : unparsedScope.entrySet()) { + try (XContentBuilder builder = XContentFactory.jsonBuilder()) { + builder.map((Map) entry.getValue()); + try (XContentParser scopeParser = XContentFactory.xContent(builder.contentType()).createParser( + NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, Strings.toString(builder))) { + scope.put(entry.getKey(), filterRefParser.parse(scopeParser, null)); + } + } + } + return new RuleScope(scope); + }; + } + private final Map scope; public RuleScope() { From fde79b37ea9019add25110fb224e75ea9ee5d362 Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Thu, 7 Jun 2018 14:45:26 +0100 Subject: [PATCH 7/8] Add comment why Operator.EQ was not added --- .../org/elasticsearch/xpack/core/ml/job/config/Operator.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Operator.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Operator.java index 8374ba0e1121b..bfe9b0e3589ba 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Operator.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Operator.java @@ -43,6 +43,9 @@ public boolean test(double lhs, double rhs) { return Double.compare(lhs, rhs) <= 0; } }; + // EQ was considered but given the oddity of such a + // condition and the fact that it would be a numerically + // unstable condition, it was rejected. public static final ParseField OPERATOR_FIELD = new ParseField("operator"); From 4d3ff564e2ffb900be3fdd8af8bd425a83ece4ba Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Tue, 12 Jun 2018 22:21:34 +0100 Subject: [PATCH 8/8] Fix unit test --- .../elasticsearch/xpack/core/ml/job/config/RuleScopeTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleScopeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleScopeTests.java index f61fee7ac772b..10b9c29aba7ef 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleScopeTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/RuleScopeTests.java @@ -63,7 +63,7 @@ public void testValidate_GivenMultipleFieldsIncludingInvalid() { ElasticsearchStatusException e = expectThrows(ElasticsearchStatusException.class, () -> scope.validate(Sets.newHashSet("foo", "foobar"))); - assertThat(e.getMessage(), equalTo("Invalid detector rule: scope key 'bar' is invalid; select from [foo, foobar]")); + assertThat(e.getMessage(), equalTo("Invalid detector rule: scope field 'bar' is invalid; select from [foo, foobar]")); } public void testGetReferencedFilters_GivenEmpty() {