From 535eae03284f274e5c8696af306ad0a81ca2d134 Mon Sep 17 00:00:00 2001 From: Amit Galitzky Date: Mon, 28 Feb 2022 17:37:17 +0000 Subject: [PATCH 01/13] adding model type validation and all its logic plus some test fixes Signed-off-by: Amit Galitzky --- .../exception/ADValidationException.java | 32 +- .../ad/constant/CommonErrorMessages.java | 20 + .../opensearch/ad/constant/CommonName.java | 4 +- .../opensearch/ad/model/AnomalyDetector.java | 2 + .../ad/model/DetectorValidationIssue.java | 22 +- .../ad/model/DetectorValidationIssueType.java | 5 +- .../opensearch/ad/model/ValidationAspect.java | 7 +- .../rest/AbstractAnomalyDetectorAction.java | 8 +- .../AbstractAnomalyDetectorActionHandler.java | 138 +++- .../IndexAnomalyDetectorActionHandler.java | 4 +- .../handler/ModelValidationActionHandler.java | 716 ++++++++++++++++++ .../ValidateAnomalyDetectorActionHandler.java | 49 +- .../ad/settings/AnomalyDetectorSettings.java | 9 + ...alidateAnomalyDetectorTransportAction.java | 18 +- .../org/opensearch/ad/util/ParseUtils.java | 38 + ...dateAnomalyDetectorActionHandlerTests.java | 17 +- .../org/opensearch/ad/ADIntegTestCase.java | 7 + .../java/org/opensearch/ad/TestHelpers.java | 30 +- .../ad/rest/AnomalyDetectorRestApiIT.java | 81 +- ...teAnomalyDetectorTransportActionTests.java | 59 +- 20 files changed, 1126 insertions(+), 140 deletions(-) create mode 100644 src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java diff --git a/src/main/java/org/opensearch/ad/common/exception/ADValidationException.java b/src/main/java/org/opensearch/ad/common/exception/ADValidationException.java index 427a2d4eb..e7d31d9ee 100644 --- a/src/main/java/org/opensearch/ad/common/exception/ADValidationException.java +++ b/src/main/java/org/opensearch/ad/common/exception/ADValidationException.java @@ -13,11 +13,13 @@ import org.opensearch.ad.model.AnomalyDetector; import org.opensearch.ad.model.DetectorValidationIssueType; +import org.opensearch.ad.model.IntervalTimeConfiguration; import org.opensearch.ad.model.ValidationAspect; public class ADValidationException extends AnomalyDetectionException { private final DetectorValidationIssueType type; private final ValidationAspect aspect; + private final IntervalTimeConfiguration intervalSuggestion; public DetectorValidationIssueType getType() { return type; @@ -27,14 +29,34 @@ public ValidationAspect getAspect() { return aspect; } + public IntervalTimeConfiguration getIntervalSuggestion() { + return intervalSuggestion; + } + public ADValidationException(String message, DetectorValidationIssueType type, ValidationAspect aspect) { - this(message, null, type, aspect); + this(message, null, type, aspect, null); + } + + public ADValidationException( + String message, + DetectorValidationIssueType type, + ValidationAspect aspect, + IntervalTimeConfiguration intervalSuggestion + ) { + this(message, null, type, aspect, intervalSuggestion); } - public ADValidationException(String message, Throwable cause, DetectorValidationIssueType type, ValidationAspect aspect) { + public ADValidationException( + String message, + Throwable cause, + DetectorValidationIssueType type, + ValidationAspect aspect, + IntervalTimeConfiguration intervalSuggestion + ) { super(AnomalyDetector.NO_ID, message, cause); this.type = type; this.aspect = aspect; + this.intervalSuggestion = intervalSuggestion; } @Override @@ -51,6 +73,12 @@ public String toString() { sb.append(aspect.getName()); } + if (intervalSuggestion != null) { + sb.append("interval Suggestion: "); + sb.append(intervalSuggestion.getInterval()); + sb.append(intervalSuggestion.getUnit()); + } + return sb.toString(); } } diff --git a/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java b/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java index 1d5b35859..a5f34c4a6 100644 --- a/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java +++ b/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java @@ -79,6 +79,8 @@ public static String getTooManyCategoricalFieldErr(int limit) { public static String INVALID_DETECTOR_NAME = "Valid characters for detector name are a-z, A-Z, 0-9, -(hyphen), _(underscore) and .(period)"; public static String DUPLICATE_FEATURE_AGGREGATION_NAMES = "Detector has duplicate feature aggregation query names: "; + public static String INVALID_TIMESTAMP = "Timestamp must be of type date"; + public static String NON_EXISTENT_TIMESTAMP = "Timestamp not found in index mapping"; public static String FAIL_TO_GET_DETECTOR = "Fail to get detector"; public static String FAIL_TO_GET_DETECTOR_INFO = "Fail to get detector info"; @@ -103,4 +105,22 @@ public static String getTooManyCategoricalFieldErr(int limit) { public static String INVALID_DETECTOR_NAME_SIZE = "Name should be shortened. The maximum limit is " + MAX_DETECTOR_NAME_SIZE + " characters."; + + public static String WINDOW_DELAY_REC = "We suggest using a window delay value of at least: "; + public static String NOT_ENOUGH_HISTORICAL_DATA = "There isn't enough historical data found with current configurations"; + public static String DETECTOR_INTERVAL_REC = "We suggest using a detector interval of : "; + public static String RAW_DATA_TOO_SPARSE = "Given index data is potentially too sparse for model training."; + public static String MODEL_VALIDATION_FAILED_UNEXPECTEDLY = "Model validation experienced issues completing."; + public static String FILTER_QUERY_TOO_SPARSE = "Data is too sparse after data filter is applied."; + public static String CATEGORY_FIELD_TOO_SPARSE = "Data is most likely too sparse with the given category fields"; + public static String FEATURE_QUERY_TOO_SPARSE = "Given data is most likely too sparse when given feature query is applied: "; + + // Modifying message for FEATURE below may break the parseADValidationException method of ValidateAnomalyDetectorTransportAction + public static final String FEATURE_INVALID_MSG_PREFIX = "Feature has an invalid query"; + public static final String FEATURE_WITH_EMPTY_DATA_MSG = FEATURE_INVALID_MSG_PREFIX + " returning empty aggregated data: "; + public static final String FEATURE_WITH_INVALID_QUERY_MSG = FEATURE_INVALID_MSG_PREFIX + " causing a runtime exception: "; + public static final String UNKNOWN_SEARCH_QUERY_EXCEPTION_MSG = + "Feature has an unknown exception caught while executing the feature query: "; + public static final String VALIDATION_FEATURE_FAILURE = "Validation failed for feature(s) of detector %s"; + } diff --git a/src/main/java/org/opensearch/ad/constant/CommonName.java b/src/main/java/org/opensearch/ad/constant/CommonName.java index f4f51f066..2c007b029 100644 --- a/src/main/java/org/opensearch/ad/constant/CommonName.java +++ b/src/main/java/org/opensearch/ad/constant/CommonName.java @@ -89,6 +89,7 @@ public class CommonName { public static final String TYPE = "type"; public static final String KEYWORD_TYPE = "keyword"; public static final String IP_TYPE = "ip"; + public static final String DATE = "date"; // used for updating mapping public static final String SCHEMA_VERSION_FIELD = "schema_version"; @@ -134,7 +135,8 @@ public class CommonName { // Validation // ====================================== // detector validation aspect - public static final String DETECTOR = "detector"; + public static final String DETECTOR_ASPECT = "detector"; + public static final String MODEL_ASPECT = "model"; // ====================================== // Used for custom AD result index diff --git a/src/main/java/org/opensearch/ad/model/AnomalyDetector.java b/src/main/java/org/opensearch/ad/model/AnomalyDetector.java index 3a7d8fff0..42e24372a 100644 --- a/src/main/java/org/opensearch/ad/model/AnomalyDetector.java +++ b/src/main/java/org/opensearch/ad/model/AnomalyDetector.java @@ -95,6 +95,8 @@ public class AnomalyDetector implements Writeable, ToXContentObject { public static final String USER_FIELD = "user"; public static final String DETECTOR_TYPE_FIELD = "detector_type"; public static final String RESULT_INDEX_FIELD = "result_index"; + public static final String GENERAL_DATA = "general_data"; + public static final String MODEL_VALIDATION_ISSUE = "aggregation_issue"; @Deprecated public static final String DETECTION_DATE_RANGE_FIELD = "detection_date_range"; diff --git a/src/main/java/org/opensearch/ad/model/DetectorValidationIssue.java b/src/main/java/org/opensearch/ad/model/DetectorValidationIssue.java index 196b624cf..8eb73b35e 100644 --- a/src/main/java/org/opensearch/ad/model/DetectorValidationIssue.java +++ b/src/main/java/org/opensearch/ad/model/DetectorValidationIssue.java @@ -41,7 +41,7 @@ public class DetectorValidationIssue implements ToXContentObject, Writeable { private final DetectorValidationIssueType type; private final String message; private Map subIssues; - private String suggestion; + private IntervalTimeConfiguration intervalSuggestion; public ValidationAspect getAspect() { return aspect; @@ -59,8 +59,8 @@ public Map getSubIssues() { return subIssues; } - public String getSuggestion() { - return suggestion; + public IntervalTimeConfiguration getIntervalSuggestion() { + return intervalSuggestion; } public DetectorValidationIssue( @@ -68,13 +68,13 @@ public DetectorValidationIssue( DetectorValidationIssueType type, String message, Map subIssues, - String suggestion + IntervalTimeConfiguration intervalSuggestion ) { this.aspect = aspect; this.type = type; this.message = message; this.subIssues = subIssues; - this.suggestion = suggestion; + this.intervalSuggestion = intervalSuggestion; } public DetectorValidationIssue(ValidationAspect aspect, DetectorValidationIssueType type, String message) { @@ -89,7 +89,7 @@ public DetectorValidationIssue(StreamInput input) throws IOException { subIssues = input.readMap(StreamInput::readString, StreamInput::readString); } if (input.readBoolean()) { - suggestion = input.readString(); + intervalSuggestion = IntervalTimeConfiguration.readFrom(input); } } @@ -104,9 +104,9 @@ public void writeTo(StreamOutput out) throws IOException { } else { out.writeBoolean(false); } - if (suggestion != null) { + if (intervalSuggestion != null) { out.writeBoolean(true); - out.writeGenericValue(suggestion); + intervalSuggestion.writeTo(out); } else { out.writeBoolean(false); } @@ -123,8 +123,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws } subIssuesBuilder.endObject(); } - if (suggestion != null) { - xContentBuilder.field(SUGGESTED_FIELD_NAME, suggestion); + if (intervalSuggestion != null) { + xContentBuilder.field(SUGGESTED_FIELD_NAME, intervalSuggestion); } return xContentBuilder.endObject().endObject(); @@ -140,7 +140,7 @@ public boolean equals(Object o) { return Objects.equal(getAspect(), anotherIssue.getAspect()) && Objects.equal(getMessage(), anotherIssue.getMessage()) && Objects.equal(getSubIssues(), anotherIssue.getSubIssues()) - && Objects.equal(getSuggestion(), anotherIssue.getSuggestion()) + && Objects.equal(getIntervalSuggestion(), anotherIssue.getIntervalSuggestion()) && Objects.equal(getType(), anotherIssue.getType()); } diff --git a/src/main/java/org/opensearch/ad/model/DetectorValidationIssueType.java b/src/main/java/org/opensearch/ad/model/DetectorValidationIssueType.java index 6395436e4..bb6e5d929 100644 --- a/src/main/java/org/opensearch/ad/model/DetectorValidationIssueType.java +++ b/src/main/java/org/opensearch/ad/model/DetectorValidationIssueType.java @@ -24,7 +24,10 @@ public enum DetectorValidationIssueType implements Name { FILTER_QUERY(AnomalyDetector.FILTER_QUERY_FIELD), WINDOW_DELAY(AnomalyDetector.WINDOW_DELAY_FIELD), GENERAL_SETTINGS(AnomalyDetector.GENERAL_SETTINGS), - RESULT_INDEX(AnomalyDetector.RESULT_INDEX_FIELD); + RESULT_INDEX(AnomalyDetector.RESULT_INDEX_FIELD), + GENERAL_DATA(AnomalyDetector.GENERAL_DATA), + MODEL_VALIDATION_ISSUE(AnomalyDetector.MODEL_VALIDATION_ISSUE); // TODO: this is a unique case where aggregation failed but not + // exception private String name; diff --git a/src/main/java/org/opensearch/ad/model/ValidationAspect.java b/src/main/java/org/opensearch/ad/model/ValidationAspect.java index 59d9179ab..a1583c875 100644 --- a/src/main/java/org/opensearch/ad/model/ValidationAspect.java +++ b/src/main/java/org/opensearch/ad/model/ValidationAspect.java @@ -28,7 +28,8 @@ * */ public enum ValidationAspect implements Name { - DETECTOR(CommonName.DETECTOR); + DETECTOR(CommonName.DETECTOR_ASPECT), + MODEL(CommonName.MODEL_ASPECT); private String name; @@ -48,8 +49,10 @@ public String getName() { public static ValidationAspect getName(String name) { switch (name) { - case CommonName.DETECTOR: + case CommonName.DETECTOR_ASPECT: return DETECTOR; + case CommonName.MODEL_ASPECT: + return MODEL; default: throw new IllegalArgumentException("Unsupported validation aspects"); } diff --git a/src/main/java/org/opensearch/ad/rest/AbstractAnomalyDetectorAction.java b/src/main/java/org/opensearch/ad/rest/AbstractAnomalyDetectorAction.java index 331c3151f..73ae47075 100644 --- a/src/main/java/org/opensearch/ad/rest/AbstractAnomalyDetectorAction.java +++ b/src/main/java/org/opensearch/ad/rest/AbstractAnomalyDetectorAction.java @@ -11,12 +11,7 @@ package org.opensearch.ad.rest; -import static org.opensearch.ad.settings.AnomalyDetectorSettings.DETECTION_INTERVAL; -import static org.opensearch.ad.settings.AnomalyDetectorSettings.DETECTION_WINDOW_DELAY; -import static org.opensearch.ad.settings.AnomalyDetectorSettings.MAX_ANOMALY_FEATURES; -import static org.opensearch.ad.settings.AnomalyDetectorSettings.MAX_MULTI_ENTITY_ANOMALY_DETECTORS; -import static org.opensearch.ad.settings.AnomalyDetectorSettings.MAX_SINGLE_ENTITY_ANOMALY_DETECTORS; -import static org.opensearch.ad.settings.AnomalyDetectorSettings.REQUEST_TIMEOUT; +import static org.opensearch.ad.settings.AnomalyDetectorSettings.*; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.settings.Settings; @@ -39,6 +34,7 @@ public AbstractAnomalyDetectorAction(Settings settings, ClusterService clusterSe this.maxSingleEntityDetectors = MAX_SINGLE_ENTITY_ANOMALY_DETECTORS.get(settings); this.maxMultiEntityDetectors = MAX_MULTI_ENTITY_ANOMALY_DETECTORS.get(settings); this.maxAnomalyFeatures = MAX_ANOMALY_FEATURES.get(settings); + // this.trainSampleTimeRangeInHours = TRAIN_SAMPLE_TIME_RANGE_IN_HOURS.get(settings); // TODO: will add more cluster setting consumer later // TODO: inject ClusterSettings only if clusterService is only used to get ClusterSettings clusterService.getClusterSettings().addSettingsUpdateConsumer(REQUEST_TIMEOUT, it -> requestTimeout = it); diff --git a/src/main/java/org/opensearch/ad/rest/handler/AbstractAnomalyDetectorActionHandler.java b/src/main/java/org/opensearch/ad/rest/handler/AbstractAnomalyDetectorActionHandler.java index 0f0995b38..2fda6dcdb 100644 --- a/src/main/java/org/opensearch/ad/rest/handler/AbstractAnomalyDetectorActionHandler.java +++ b/src/main/java/org/opensearch/ad/rest/handler/AbstractAnomalyDetectorActionHandler.java @@ -21,14 +21,17 @@ import static org.opensearch.common.xcontent.XContentParserUtils.ensureExpectedToken; import java.io.IOException; +import java.time.Clock; import java.time.Instant; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; import org.apache.commons.lang.StringUtils; @@ -60,9 +63,11 @@ import org.opensearch.ad.model.Feature; import org.opensearch.ad.model.MergeableList; import org.opensearch.ad.model.ValidationAspect; +import org.opensearch.ad.rest.RestValidateAnomalyDetectorAction; import org.opensearch.ad.settings.NumericSetting; import org.opensearch.ad.task.ADTaskManager; import org.opensearch.ad.transport.IndexAnomalyDetectorResponse; +import org.opensearch.ad.transport.ValidateAnomalyDetectorResponse; import org.opensearch.ad.util.MultiResponsesDelegateActionListener; import org.opensearch.ad.util.RestHandlerUtils; import org.opensearch.client.Client; @@ -82,6 +87,8 @@ import org.opensearch.search.builder.SearchSourceBuilder; import org.opensearch.transport.TransportService; +import com.google.common.collect.Sets; + /** * Abstract Anomaly detector REST action handler to process POST/PUT request. * POST request is for either validating or creating anomaly detector. @@ -102,6 +109,9 @@ *

This means that if the AD index doesn't exist at the time request is received it wont be created. * Furthermore, this means that the AD won't actually be created and all exceptions will be wrapped into * DetectorValidationResponses hence the user will be notified which validation checks didn't pass.

+ *

After completing all the first round of validation which is identical to the checks that are done for the + * create/update APIs, this code will check if the validation type is 'model' and if true it will + * instantiate the ModelValidationActionHandler class and run the non-blocker validation logic

* */ public abstract class AbstractAnomalyDetectorActionHandler { @@ -113,15 +123,9 @@ public abstract class AbstractAnomalyDetectorActionHandler DEFAULT_VALIDATION_ASPECTS = Sets.newHashSet(ValidationAspect.DETECTOR); protected final AnomalyDetectionIndices anomalyDetectionIndices; protected final String detectorId; @@ -146,6 +150,8 @@ public abstract class AbstractAnomalyDetectorActionHandler mappingsListener = ActionListener.wrap(getMappingsResponse -> { + boolean foundField = false; + Map>> mappingsByIndex = getMappingsResponse + .mappings(); + + for (Map> mappingsByType : mappingsByIndex.values()) { + for (Map mappingsByField : mappingsByType.values()) { + for (Map.Entry field2Metadata : mappingsByField.entrySet()) { + + GetFieldMappingsResponse.FieldMappingMetadata fieldMetadata = field2Metadata.getValue(); + if (fieldMetadata != null) { + // sourceAsMap returns sth like {host2={type=keyword}} with host2 being a nested field + Map fieldMap = fieldMetadata.sourceAsMap(); + if (fieldMap != null) { + for (Object type : fieldMap.values()) { + if (type instanceof Map) { + foundField = true; + Map metadataMap = (Map) type; + String typeName = (String) metadataMap.get(CommonName.TYPE); + if (!typeName.equals(CommonName.DATE)) { + listener + .onFailure( + new ADValidationException( + CommonErrorMessages.INVALID_TIMESTAMP, + DetectorValidationIssueType.TIMEFIELD_FIELD, + ValidationAspect.DETECTOR + ) + ); + return; + } + } + } + } + } + } + } + } + if (!foundField) { + listener + .onFailure( + new ADValidationException( + CommonErrorMessages.NON_EXISTENT_TIMESTAMP, + DetectorValidationIssueType.TIMEFIELD_FIELD, + ValidationAspect.DETECTOR + ) + ); + return; + } + prepareAnomalyDetectorIndexing(indexingDryRun); + }, error -> { + String message = String.format(Locale.ROOT, "Fail to get the index mapping of %s", anomalyDetector.getIndices()); + logger.error(message, error); + listener.onFailure(new IllegalArgumentException(message)); + }); + client.execute(GetFieldMappingsAction.INSTANCE, getMappingsRequest, mappingsListener); } /** @@ -676,8 +752,37 @@ protected void tryIndexingAnomalyDetector(boolean indexingDryRun) throws IOExcep if (!indexingDryRun) { indexAnomalyDetector(detectorId); } else { - logger.info("Skipping indexing detector. No issue found so far."); + finishDetectorValidationOrContinueToModelValidation(); + } + } + + protected Set getValidationTypes(String validationType) { + if (StringUtils.isBlank(validationType)) { + return DEFAULT_VALIDATION_ASPECTS; + } else { + Set typesInRequest = new HashSet<>(Arrays.asList(validationType.split(","))); + return ValidationAspect + .getNames(Sets.intersection(RestValidateAnomalyDetectorAction.ALL_VALIDATION_ASPECTS_STRS, typesInRequest)); + } + } + + protected void finishDetectorValidationOrContinueToModelValidation() { + logger.info("Skipping indexing detector. No blocking issue found so far."); + if (!getValidationTypes(validationType).contains(ValidationAspect.MODEL)) { listener.onResponse(null); + } else { + ModelValidationActionHandler modelValidationActionHandler = new ModelValidationActionHandler( + clusterService, + client, + (ActionListener) listener, + anomalyDetector, + requestTimeout, + xContentRegistry, + searchFeatureDao, + validationType, + clock + ); + modelValidationActionHandler.checkIfMultiEntityDetector(); } } @@ -816,7 +921,7 @@ protected void validateAnomalyDetectorFeatures(String detectorId, boolean indexi new MultiResponsesDelegateActionListener>>( validateFeatureQueriesListener, anomalyDetector.getFeatureAttributes().size(), - String.format(Locale.ROOT, VALIDATION_FEATURE_FAILURE, anomalyDetector.getName()), + String.format(Locale.ROOT, CommonErrorMessages.VALIDATION_FEATURE_FAILURE, anomalyDetector.getName()), false ); @@ -837,18 +942,17 @@ protected void validateAnomalyDetectorFeatures(String detectorId, boolean indexi new MergeableList>(new ArrayList>(Arrays.asList(aggFeatureResult))) ); } else { - String errorMessage = FEATURE_WITH_EMPTY_DATA_MSG + feature.getName(); + String errorMessage = CommonErrorMessages.FEATURE_WITH_EMPTY_DATA_MSG + feature.getName(); logger.error(errorMessage); multiFeatureQueriesResponseListener.onFailure(new OpenSearchStatusException(errorMessage, RestStatus.BAD_REQUEST)); } }, e -> { String errorMessage; if (isExceptionCausedByInvalidQuery(e)) { - errorMessage = FEATURE_WITH_INVALID_QUERY_MSG + feature.getName(); + errorMessage = CommonErrorMessages.FEATURE_WITH_INVALID_QUERY_MSG + feature.getName(); } else { - errorMessage = UNKNOWN_SEARCH_QUERY_EXCEPTION_MSG + feature.getName(); + errorMessage = CommonErrorMessages.UNKNOWN_SEARCH_QUERY_EXCEPTION_MSG + feature.getName(); } - logger.error(errorMessage, e); multiFeatureQueriesResponseListener.onFailure(new OpenSearchStatusException(errorMessage, RestStatus.BAD_REQUEST, e)); })); diff --git a/src/main/java/org/opensearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java b/src/main/java/org/opensearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java index e3dc3661c..6291c6472 100644 --- a/src/main/java/org/opensearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java +++ b/src/main/java/org/opensearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java @@ -97,7 +97,9 @@ public IndexAnomalyDetectorActionHandler( user, adTaskManager, searchFeatureDao, - false + null, + false, + null ); } diff --git a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java new file mode 100644 index 000000000..8a7d13a07 --- /dev/null +++ b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java @@ -0,0 +1,716 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.ad.rest.handler; + +import static org.opensearch.ad.settings.AnomalyDetectorSettings.CONFIG_BUCKET_MINIMUM_SUCCESS_RATE; +import static org.opensearch.ad.settings.AnomalyDetectorSettings.INTERVAL_BUCKET_MINIMUM_SUCCESS_RATE; +import static org.opensearch.ad.settings.AnomalyDetectorSettings.MAX_INTERVAL_REC_LENGTH_IN_MINUTES; +import static org.opensearch.ad.settings.AnomalyDetectorSettings.TOP_VALIDATE_TIMEOUT_IN_MILLIS; +import static org.opensearch.ad.util.ParseUtils.parseAggregators; +import static org.opensearch.ad.util.RestHandlerUtils.isExceptionCausedByInvalidQuery; + +import java.io.IOException; +import java.time.Clock; +import java.time.Duration; +import java.time.Instant; +import java.time.temporal.ChronoUnit; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.HashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.opensearch.OpenSearchStatusException; +import org.opensearch.action.ActionListener; +import org.opensearch.action.search.SearchRequest; +import org.opensearch.action.search.SearchResponse; +import org.opensearch.ad.common.exception.ADValidationException; +import org.opensearch.ad.common.exception.AnomalyDetectionException; +import org.opensearch.ad.common.exception.EndRunException; +import org.opensearch.ad.constant.CommonErrorMessages; +import org.opensearch.ad.feature.SearchFeatureDao; +import org.opensearch.ad.model.AnomalyDetector; +import org.opensearch.ad.model.DetectorValidationIssueType; +import org.opensearch.ad.model.Feature; +import org.opensearch.ad.model.IntervalTimeConfiguration; +import org.opensearch.ad.model.MergeableList; +import org.opensearch.ad.model.TimeConfiguration; +import org.opensearch.ad.model.ValidationAspect; +import org.opensearch.ad.settings.AnomalyDetectorSettings; +import org.opensearch.ad.transport.ValidateAnomalyDetectorResponse; +import org.opensearch.ad.util.MultiResponsesDelegateActionListener; +import org.opensearch.ad.util.ParseUtils; +import org.opensearch.client.Client; +import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.unit.TimeValue; +import org.opensearch.common.xcontent.NamedXContentRegistry; +import org.opensearch.index.query.BoolQueryBuilder; +import org.opensearch.index.query.QueryBuilder; +import org.opensearch.index.query.QueryBuilders; +import org.opensearch.rest.RestStatus; +import org.opensearch.search.aggregations.AggregationBuilder; +import org.opensearch.search.aggregations.AggregationBuilders; +import org.opensearch.search.aggregations.Aggregations; +import org.opensearch.search.aggregations.AggregatorFactories; +import org.opensearch.search.aggregations.BucketOrder; +import org.opensearch.search.aggregations.PipelineAggregatorBuilders; +import org.opensearch.search.aggregations.bucket.MultiBucketsAggregation; +import org.opensearch.search.aggregations.bucket.composite.CompositeAggregation; +import org.opensearch.search.aggregations.bucket.composite.TermsValuesSourceBuilder; +import org.opensearch.search.aggregations.bucket.histogram.DateHistogramInterval; +import org.opensearch.search.aggregations.bucket.histogram.Histogram; +import org.opensearch.search.aggregations.bucket.histogram.LongBounds; +import org.opensearch.search.aggregations.bucket.terms.Terms; +import org.opensearch.search.builder.SearchSourceBuilder; +import org.opensearch.search.sort.FieldSortBuilder; +import org.opensearch.search.sort.SortOrder; + +/** + *

This class executes all validation checks that are not blocking on the 'model' level. + * This mostly involves checking if the data is generally dense enough to complete model training + * which is based on if enough buckets in the last x intervals have at least 1 document present.

+ *

Initially different bucket aggregations are executed with with every configuration applied and with + * different varying intervals in order to find the best interval for the data. If no interval is found with all + * configuration applied then each configuration is tested sequentially for sparsity

+ */ +// TODO: potentially change where this is located +public class ModelValidationActionHandler { + protected static final String AGG_NAME_TOP = "top_agg"; + protected final AnomalyDetector anomalyDetector; + protected final ClusterService clusterService; + protected final Logger logger = LogManager.getLogger(AbstractAnomalyDetectorActionHandler.class); + protected final TimeValue requestTimeout; + protected final AnomalyDetectorActionHandler handler = new AnomalyDetectorActionHandler(); + protected final Client client; + protected final NamedXContentRegistry xContentRegistry; + protected final ActionListener listener; + protected final SearchFeatureDao searchFeatureDao; + protected final Clock clock; + protected final String validationType; + + /** + * Constructor function. + * + * @param clusterService ClusterService + * @param client ES node client that executes actions on the local node + * @param listener ES channel used to construct bytes / builder based outputs, and send responses + * @param anomalyDetector anomaly detector instance + * @param requestTimeout request time out configuration + * @param xContentRegistry Registry which is used for XContentParser + * @param searchFeatureDao Search feature DAO + * @param validationType Specified type for validation + * @param clock clock object to know when to timeout + */ + public ModelValidationActionHandler( + ClusterService clusterService, + Client client, + ActionListener listener, + AnomalyDetector anomalyDetector, + TimeValue requestTimeout, + NamedXContentRegistry xContentRegistry, + SearchFeatureDao searchFeatureDao, + String validationType, + Clock clock + ) { + this.clusterService = clusterService; + this.client = client; + this.listener = listener; + this.anomalyDetector = anomalyDetector; + this.requestTimeout = requestTimeout; + this.xContentRegistry = xContentRegistry; + this.searchFeatureDao = searchFeatureDao; + this.validationType = validationType; + this.clock = clock; + } + + // Need to first check if multi entity detector or not before doing any sort of validation. + // If detector is HCAD then we will find the top entity and treat that single entity for + // validation purposes + public void checkIfMultiEntityDetector() { + ActionListener> recommendationListener = ActionListener + .wrap(topEntity -> startIntervalRecommendation(topEntity), exception -> { + listener.onFailure(exception); + logger.error("Failed to get top entity for categorical field", exception); + }); + if (anomalyDetector.isMultientityDetector()) { + getTopEntity(recommendationListener); + } else { + recommendationListener.onResponse(Collections.emptyMap()); + } + } + + private void getTopEntity(ActionListener> topEntityListener) { + BoolQueryBuilder boolQueryBuilder = QueryBuilders.boolQuery().filter(anomalyDetector.getFilterQuery()); + AggregationBuilder bucketAggs; + Map topKeys = new HashMap<>(); + if (anomalyDetector.getCategoryField().size() == 1) { + bucketAggs = AggregationBuilders + .terms(AGG_NAME_TOP) + .field(anomalyDetector.getCategoryField().get(0)) + .order(BucketOrder.count(true)); + } else { + + bucketAggs = AggregationBuilders + .composite( + AGG_NAME_TOP, + anomalyDetector + .getCategoryField() + .stream() + .map(f -> new TermsValuesSourceBuilder(f).field(f)) + .collect(Collectors.toList()) + ) + .size(1000) + .subAggregation( + PipelineAggregatorBuilders + .bucketSort("bucketSort", Collections.singletonList(new FieldSortBuilder("_count").order(SortOrder.DESC))) + .size(1) + ); + } + + SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder() + .query(boolQueryBuilder) + .aggregation(bucketAggs) + .trackTotalHits(false) + .size(0); + SearchRequest searchRequest = new SearchRequest() + .indices(anomalyDetector.getIndices().toArray(new String[0])) + .source(searchSourceBuilder); + client.search(searchRequest, ActionListener.wrap(response -> { + Aggregations aggs = response.getAggregations(); + if (aggs == null) { + topEntityListener.onResponse(Collections.emptyMap()); + return; + } + if (anomalyDetector.getCategoryField().size() == 1) { + Terms entities = aggs.get(AGG_NAME_TOP); + Object key = entities + .getBuckets() + .stream() + .max(Comparator.comparingInt(entry -> (int) entry.getDocCount())) + .map(MultiBucketsAggregation.Bucket::getKeyAsString) + .orElse(null); + topKeys.put(anomalyDetector.getCategoryField().get(0), key); + } else { + CompositeAggregation compositeAgg = aggs.get(AGG_NAME_TOP); + topKeys + .putAll( + compositeAgg + .getBuckets() + .stream() + .flatMap(bucket -> bucket.getKey().entrySet().stream()) // this would create a flattened stream of map entries + .collect(Collectors.toMap(e -> e.getKey(), e -> e.getValue())) + ); + } + topEntityListener.onResponse(topKeys); + }, topEntityListener::onFailure)); + } + + private void startIntervalRecommendation(Map topEntity) { + getLatestDateForValidation(topEntity); + } + + private void getLatestDateForValidation(Map topEntity) { + ActionListener> latestTimeListener = ActionListener + .wrap(latest -> getSampleRangesForValidationChecks(latest, anomalyDetector, listener, topEntity), exception -> { + listener.onFailure(exception); + logger.error("Failed to create search request for last data point", exception); + return; + }); + searchFeatureDao.getLatestDataTime(anomalyDetector, latestTimeListener); + } + + private void getSampleRangesForValidationChecks( + Optional latestTime, + AnomalyDetector detector, + ActionListener listener, + Map topEntity + ) { + if (!latestTime.isPresent()) { + listener + .onFailure( + new ADValidationException( + CommonErrorMessages.NOT_ENOUGH_HISTORICAL_DATA, + DetectorValidationIssueType.GENERAL_DATA, + ValidationAspect.MODEL + ) + ); + return; + } else if (latestTime.get() <= 0) { + listener + .onFailure( + new ADValidationException( + CommonErrorMessages.NOT_ENOUGH_HISTORICAL_DATA, + DetectorValidationIssueType.GENERAL_DATA, + ValidationAspect.MODEL + ) + ); + return; + } + long timeRangeEnd = Math.min(Instant.now().toEpochMilli(), latestTime.get()); + try { + getBucketAggregates(timeRangeEnd, listener, topEntity); + } catch (IOException e) { + listener.onFailure(new EndRunException(detector.getDetectorId(), CommonErrorMessages.INVALID_SEARCH_QUERY_MSG, e, true)); + return; + } + } + + private void getBucketAggregates( + long latestTime, + ActionListener listener, + Map topEntity + ) throws IOException { + List featureFields = ParseUtils.getFeatureFieldNames(anomalyDetector, xContentRegistry); + AggregationBuilder aggregation = getBucketAggregation(latestTime); + BoolQueryBuilder query = QueryBuilders.boolQuery().filter(anomalyDetector.getFilterQuery()); + if (anomalyDetector.isMultientityDetector()) { + for (Map.Entry entry : topEntity.entrySet()) { + query.filter(QueryBuilders.termQuery(entry.getKey(), entry.getValue())); + } + } + if (anomalyDetector.isMultiCategoryDetector()) { + for (String category : anomalyDetector.getCategoryField()) { + query.filter(QueryBuilders.existsQuery(category)); + } + } + for (String featureField : featureFields) { + query.filter(QueryBuilders.existsQuery(featureField)); + } + SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder() + .query(query) + .aggregation(aggregation) + .size(0) + .timeout(requestTimeout); + SearchRequest searchRequest = new SearchRequest(anomalyDetector.getIndices().toArray(new String[0])).source(searchSourceBuilder); + ActionListener intervalListener = ActionListener + .wrap(interval -> processIntervalRecommendation(interval, latestTime), exception -> { + listener.onFailure(exception); + logger.error("Failed to get interval recommendation", exception); + return; + }); + client + .search( + searchRequest, + new ModelValidationActionHandler.DetectorIntervalRecommendationListener( + intervalListener, + searchRequest.source(), + (IntervalTimeConfiguration) anomalyDetector.getDetectionInterval(), + clock.millis() + TOP_VALIDATE_TIMEOUT_IN_MILLIS, + latestTime + ) + ); + } + + private double processBucketAggregationResults(Histogram buckets) { + int docCountOverOne = 0; + // For each entry + for (Histogram.Bucket entry : buckets.getBuckets()) { + if (entry.getDocCount() > 0) { + docCountOverOne++; + } + } + return (docCountOverOne / (double) getNumberOfSamples()); + } + + /** + * ActionListener class to handle bucketed search results in a paginated fashion. + * Note that the bucket_sort aggregation is a pipeline aggregation, and is executed + * after all non-pipeline aggregations (including the composite bucket aggregation). + * Because of this, the sorting is only done locally based on the buckets + * in the current page. To get around this issue, we use a max + * heap and add all results to the heap until there are no more result buckets, + * to get the globally sorted set of result buckets. + */ + class DetectorIntervalRecommendationListener implements ActionListener { + private final ActionListener intervalListener; + SearchSourceBuilder searchSourceBuilder; + IntervalTimeConfiguration detectorInterval; + private final long expirationEpochMs; + private final long latestTime; + + DetectorIntervalRecommendationListener( + ActionListener intervalListener, + SearchSourceBuilder searchSourceBuilder, + IntervalTimeConfiguration detectorInterval, + long expirationEpochMs, + long latestTime + ) { + this.intervalListener = intervalListener; + this.searchSourceBuilder = searchSourceBuilder; + this.detectorInterval = detectorInterval; + this.expirationEpochMs = expirationEpochMs; + this.latestTime = latestTime; + } + + private AggregationBuilder getNewAggregationBuilder(long newInterval) { + return AggregationBuilders + .dateHistogram("agg") + .field(anomalyDetector.getTimeField()) + .minDocCount(0) + .hardBounds(getTimeRangeBounds(latestTime, new IntervalTimeConfiguration(newInterval, ChronoUnit.MINUTES))) + .fixedInterval(DateHistogramInterval.minutes((int) newInterval)); + } + + private long convertIntervalToMinutes(IntervalTimeConfiguration interval) { + long currentInterval = interval.getInterval(); + if (interval.getUnit() == ChronoUnit.MILLIS) { + currentInterval /= 60000; + } + return currentInterval; + } + + @Override + public void onResponse(SearchResponse response) { + try { + Histogram aggregate = checkBucketResultErrors(response); + if (aggregate == null) { + return; + } + + double fullBucketRate = processBucketAggregationResults(aggregate); + long newInterval = (long) Math.ceil(convertIntervalToMinutes(detectorInterval) + 1); + // If rate is below success minimum then call search again. + if (fullBucketRate > INTERVAL_BUCKET_MINIMUM_SUCCESS_RATE) { + intervalListener.onResponse(this.detectorInterval); + } else if (expirationEpochMs < clock.millis()) { + listener + .onFailure( + new AnomalyDetectionException( + "Timed out getting interval recommendation. Please continue with detector creation." + ) + ); + logger.info("Timed out getting interval recommendation"); + } else if (newInterval < MAX_INTERVAL_REC_LENGTH_IN_MINUTES) { + this.detectorInterval = new IntervalTimeConfiguration(newInterval, ChronoUnit.MINUTES); + // Searching again using an updated interval + SearchSourceBuilder updatedSearchSourceBuilder = getSearchSourceBuilder( + searchSourceBuilder.query(), + getNewAggregationBuilder(newInterval) + ); + client + .search( + new SearchRequest() + .indices(anomalyDetector.getIndices().toArray(new String[0])) + .source(updatedSearchSourceBuilder), + this + ); + // this case means all intervals up to max interval recommendation length have been tried + // which further means the next step is to go through A/B validation checks + } else { + intervalListener.onResponse(null); + return; + } + + } catch (Exception e) { + onFailure(e); + } + } + + @Override + public void onFailure(Exception e) { + logger.error("Failed to paginate top anomaly results", e); + listener.onFailure(e); + } + } + + private void processIntervalRecommendation(IntervalTimeConfiguration interval, long latestTime) { + if (interval == null) { + checkRawDataSparsity(latestTime); + } else { + if (interval.equals(anomalyDetector.getDetectionInterval())) { + logger.info("Using the current interval there is enough dense data "); + // The rate of buckets with at least 1 doc with given interval is above the success rate + listener.onResponse(null); + return; + } + // return response with interval recommendation + listener + .onFailure( + new ADValidationException( + CommonErrorMessages.DETECTOR_INTERVAL_REC + interval.getInterval(), + DetectorValidationIssueType.DETECTION_INTERVAL, + ValidationAspect.MODEL, + interval + ) + ); + } + } + + private AggregationBuilder getBucketAggregation(long latestTime) { + return AggregationBuilders + .dateHistogram("agg") + .field(anomalyDetector.getTimeField()) + .minDocCount(0) + .hardBounds(getTimeRangeBounds(latestTime, (IntervalTimeConfiguration) anomalyDetector.getDetectionInterval())) + .fixedInterval(DateHistogramInterval.minutes((int) anomalyDetector.getDetectorIntervalInMinutes())); + } + + private SearchSourceBuilder getSearchSourceBuilder(QueryBuilder query, AggregationBuilder aggregation) { + return new SearchSourceBuilder().query(query).aggregation(aggregation).size(0).timeout(requestTimeout); + } + + private void checkRawDataSparsity(long latestTime) { + AggregationBuilder aggregation = getBucketAggregation(latestTime); + SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder().aggregation(aggregation).size(0).timeout(requestTimeout); + try { + AggregatorFactories.Builder internalAgg = parseAggregators( + anomalyDetector.getFeatureAttributes().get(0).getAggregation().toString(), + xContentRegistry, + anomalyDetector.getFeatureAttributes().get(0).getId() + ); + aggregation.subAggregation(internalAgg.getAggregatorFactories().iterator().next()); + SearchRequest searchRequest = new SearchRequest(anomalyDetector.getIndices().toArray(new String[0])) + .source(searchSourceBuilder); + client.search(searchRequest, ActionListener.wrap(response -> processRawDataResults(response, latestTime), listener::onFailure)); + } catch (Exception ex) { + listener.onFailure(ex); + } + } + + private Histogram checkBucketResultErrors(SearchResponse response) { + Aggregations aggs = response.getAggregations(); + if (aggs == null) { + // This would indicate some bug or some opensearch core changes that we are not aware of (we don't keep up-to-date with + // the large amounts of changes there). For this reason I'm not throwing a SearchException but instead a validation exception + // which will be converted to validation response. + logger.warn("Unexpected null aggregation."); + listener + .onFailure( + new ADValidationException( + CommonErrorMessages.MODEL_VALIDATION_FAILED_UNEXPECTEDLY, + DetectorValidationIssueType.MODEL_VALIDATION_ISSUE, + ValidationAspect.MODEL + ) + ); + return null; + } + Histogram aggregate = aggs.get("agg"); + if (aggregate == null) { + listener.onFailure(new IllegalArgumentException("Failed to find valid aggregation result")); + return null; + } + return aggregate; + } + + private void processRawDataResults(SearchResponse response, long latestTime) { + Histogram aggregate = checkBucketResultErrors(response); + if (aggregate == null) { + return; + } + double fullBucketRate = processBucketAggregationResults(aggregate); + if (fullBucketRate < CONFIG_BUCKET_MINIMUM_SUCCESS_RATE) { + listener + .onFailure( + new ADValidationException( + CommonErrorMessages.RAW_DATA_TOO_SPARSE, + DetectorValidationIssueType.INDICES, + ValidationAspect.MODEL + ) + ); + } else { + checkDataFilterSparsity(latestTime); + } + } + + private void checkDataFilterSparsity(long latestTime) { + AggregationBuilder aggregation = getBucketAggregation(latestTime); + BoolQueryBuilder query = QueryBuilders.boolQuery().filter(anomalyDetector.getFilterQuery()); + SearchSourceBuilder searchSourceBuilder = getSearchSourceBuilder(query, aggregation); + SearchRequest searchRequest = new SearchRequest(anomalyDetector.getIndices().toArray(new String[0])).source(searchSourceBuilder); + client.search(searchRequest, ActionListener.wrap(response -> processDataFilterResults(response, latestTime), listener::onFailure)); + } + + private void processDataFilterResults(SearchResponse response, long latestTime) { + Histogram aggregate = checkBucketResultErrors(response); + if (aggregate == null) { + return; + } + double fullBucketRate = processBucketAggregationResults(aggregate); + if (fullBucketRate < CONFIG_BUCKET_MINIMUM_SUCCESS_RATE) { + listener + .onFailure( + new ADValidationException( + CommonErrorMessages.FILTER_QUERY_TOO_SPARSE, + DetectorValidationIssueType.FILTER_QUERY, + ValidationAspect.MODEL + ) + ); + } else if (anomalyDetector.isMultientityDetector()) { + getTopEntityForCategoryField(latestTime); + } else { + try { + checkFeatureQuery(latestTime); + } catch (Exception ex) { + logger.error(ex); + listener.onFailure(ex); + } + } + } + + private void getTopEntityForCategoryField(long latestTime) { + ActionListener> getTopEntityListener = ActionListener + .wrap(topEntity -> checkCategoryFieldSparsity(topEntity, latestTime), exception -> { + listener.onFailure(exception); + logger.error("Failed to get top entity for categorical field", exception); + return; + }); + getTopEntity(getTopEntityListener); + } + + private void checkCategoryFieldSparsity(Map topEntity, long latestTime) { + if (topEntity.isEmpty()) { + listener + .onFailure( + new ADValidationException( + CommonErrorMessages.CATEGORY_FIELD_TOO_SPARSE, + DetectorValidationIssueType.CATEGORY, + ValidationAspect.MODEL + ) + ); + return; + } + BoolQueryBuilder query = QueryBuilders.boolQuery().filter(anomalyDetector.getFilterQuery()); + for (Map.Entry entry : topEntity.entrySet()) { + query.filter(QueryBuilders.termQuery(entry.getKey(), entry.getValue())); + } + AggregationBuilder aggregation = getBucketAggregation(latestTime); + SearchSourceBuilder searchSourceBuilder = getSearchSourceBuilder(query, aggregation); + SearchRequest searchRequest = new SearchRequest(anomalyDetector.getIndices().toArray(new String[0])).source(searchSourceBuilder); + client.search(searchRequest, ActionListener.wrap(response -> processTopEntityResults(response, latestTime), listener::onFailure)); + } + + private void processTopEntityResults(SearchResponse response, long latestTime) { + Histogram aggregate = checkBucketResultErrors(response); + if (aggregate == null) { + return; + } + double fullBucketRate = processBucketAggregationResults(aggregate); + if (fullBucketRate < CONFIG_BUCKET_MINIMUM_SUCCESS_RATE) { + listener + .onFailure( + new ADValidationException( + CommonErrorMessages.CATEGORY_FIELD_TOO_SPARSE, + DetectorValidationIssueType.CATEGORY, + ValidationAspect.MODEL + ) + ); + return; + } else { + try { + checkFeatureQuery(latestTime); + } catch (Exception ex) { + logger.error(ex); + listener.onFailure(ex); + } + } + } + + private void checkFeatureQuery(long latestTime) throws IOException { + ActionListener> validateFeatureQueriesListener = ActionListener + .wrap(response -> { windowDelayRecommendation(latestTime); }, exception -> { + listener + .onFailure( + new ADValidationException( + exception.getMessage(), + DetectorValidationIssueType.FEATURE_ATTRIBUTES, + ValidationAspect.DETECTOR + ) + ); + }); + MultiResponsesDelegateActionListener> multiFeatureQueriesResponseListener = + new MultiResponsesDelegateActionListener<>( + validateFeatureQueriesListener, + anomalyDetector.getFeatureAttributes().size(), + String.format(Locale.ROOT, CommonErrorMessages.VALIDATION_FEATURE_FAILURE, anomalyDetector.getName()), + false + ); + + for (Feature feature : anomalyDetector.getFeatureAttributes()) { + AggregationBuilder aggregation = getBucketAggregation(latestTime); + BoolQueryBuilder query = QueryBuilders.boolQuery().filter(anomalyDetector.getFilterQuery()); + List featureFields = ParseUtils.getFieldNamesForFeature(feature, xContentRegistry); + for (String featureField : featureFields) { + query.filter(QueryBuilders.existsQuery(featureField)); + } + SearchSourceBuilder searchSourceBuilder = getSearchSourceBuilder(query, aggregation); + SearchRequest searchRequest = new SearchRequest(anomalyDetector.getIndices().toArray(new String[0])) + .source(searchSourceBuilder); + client.search(searchRequest, ActionListener.wrap(response -> { + Histogram aggregate = checkBucketResultErrors(response); + if (aggregate == null) { + return; + } + double fullBucketRate = processBucketAggregationResults(aggregate); + if (fullBucketRate < CONFIG_BUCKET_MINIMUM_SUCCESS_RATE) { + multiFeatureQueriesResponseListener + .onFailure( + new ADValidationException( + CommonErrorMessages.FEATURE_QUERY_TOO_SPARSE + feature.getName(), + DetectorValidationIssueType.FEATURE_ATTRIBUTES, + ValidationAspect.MODEL + ) + ); + } else { + multiFeatureQueriesResponseListener + .onResponse(new MergeableList<>(new ArrayList<>(Collections.singletonList(new double[] { fullBucketRate })))); + } + }, e -> { + String errorMessage; + if (isExceptionCausedByInvalidQuery(e)) { + errorMessage = CommonErrorMessages.FEATURE_WITH_INVALID_QUERY_MSG + feature.getName(); + } else { + errorMessage = CommonErrorMessages.UNKNOWN_SEARCH_QUERY_EXCEPTION_MSG + feature.getName(); + } + logger.error(errorMessage, e); + multiFeatureQueriesResponseListener.onFailure(new OpenSearchStatusException(errorMessage, RestStatus.BAD_REQUEST, e)); + })); + } + } + + private void windowDelayRecommendation(long latestTime) { + long delayMillis = timeConfigToMilliSec(anomalyDetector.getWindowDelay()); + if ((Instant.now().toEpochMilli() - latestTime > delayMillis)) { + long minutesSinceLastStamp = TimeUnit.MILLISECONDS.toMinutes(Instant.now().toEpochMilli() - latestTime); + listener + .onFailure( + new ADValidationException( + CommonErrorMessages.WINDOW_DELAY_REC + minutesSinceLastStamp, + DetectorValidationIssueType.WINDOW_DELAY, + ValidationAspect.MODEL, + new IntervalTimeConfiguration(minutesSinceLastStamp, ChronoUnit.MINUTES) + ) + ); + return; + } + listener.onResponse(null); + } + + private LongBounds getTimeRangeBounds(long endMillis, IntervalTimeConfiguration detectorIntervalInMinutes) { + Long detectorInterval = timeConfigToMilliSec(detectorIntervalInMinutes); + Long startMillis = endMillis - ((long) getNumberOfSamples() * detectorInterval); + return new LongBounds(startMillis, endMillis); + } + + private int getNumberOfSamples() { + long interval = anomalyDetector.getDetectorIntervalInMilliseconds(); + return Math + .max( + (int) (Duration.ofHours(AnomalyDetectorSettings.TRAIN_SAMPLE_TIME_RANGE_IN_HOURS).toMillis() / interval), + AnomalyDetectorSettings.MIN_TRAIN_SAMPLES + ); + } + + private Long timeConfigToMilliSec(TimeConfiguration config) { + return Optional.ofNullable((IntervalTimeConfiguration) config).map(t -> t.toDuration().toMillis()).orElse(0L); + } +} diff --git a/src/main/java/org/opensearch/ad/rest/handler/ValidateAnomalyDetectorActionHandler.java b/src/main/java/org/opensearch/ad/rest/handler/ValidateAnomalyDetectorActionHandler.java index a88c40eff..e394d0318 100644 --- a/src/main/java/org/opensearch/ad/rest/handler/ValidateAnomalyDetectorActionHandler.java +++ b/src/main/java/org/opensearch/ad/rest/handler/ValidateAnomalyDetectorActionHandler.java @@ -11,17 +11,12 @@ package org.opensearch.ad.rest.handler; -import java.util.Arrays; -import java.util.HashSet; -import java.util.Set; +import java.time.Clock; -import org.apache.commons.lang3.StringUtils; import org.opensearch.action.ActionListener; import org.opensearch.ad.feature.SearchFeatureDao; import org.opensearch.ad.indices.AnomalyDetectionIndices; import org.opensearch.ad.model.AnomalyDetector; -import org.opensearch.ad.model.ValidationAspect; -import org.opensearch.ad.rest.RestValidateAnomalyDetectorAction; import org.opensearch.ad.transport.ValidateAnomalyDetectorResponse; import org.opensearch.client.Client; import org.opensearch.cluster.service.ClusterService; @@ -30,17 +25,12 @@ import org.opensearch.commons.authuser.User; import org.opensearch.rest.RestRequest; -import com.google.common.collect.Sets; - /** * Anomaly detector REST action handler to process POST request. * POST request is for validating anomaly detector against detector and/or model configs. */ public class ValidateAnomalyDetectorActionHandler extends AbstractAnomalyDetectorActionHandler { - private static final Set DEFAULT_VALIDATION_ASPECTS = Sets.newHashSet(ValidationAspect.DETECTOR); - private final Set aspects; - /** * Constructor function. * @@ -57,7 +47,8 @@ public class ValidateAnomalyDetectorActionHandler extends AbstractAnomalyDetecto * @param xContentRegistry Registry which is used for XContentParser * @param user User context * @param searchFeatureDao Search feature DAO - * @param validationType specified type for validation + * @param validationType Specified type for validation + * @param clock Clock object to know when to timeout */ public ValidateAnomalyDetectorActionHandler( ClusterService clusterService, @@ -73,7 +64,8 @@ public ValidateAnomalyDetectorActionHandler( NamedXContentRegistry xContentRegistry, User user, SearchFeatureDao searchFeatureDao, - String validationType + String validationType, + Clock clock ) { super( clusterService, @@ -95,36 +87,21 @@ public ValidateAnomalyDetectorActionHandler( user, null, searchFeatureDao, - true + validationType, + true, + clock ); - this.aspects = getValidationTypes(validationType); } - // All current validation that is done in the AbstractAnomalyDetectorActionHandler that is called + // If validation type is detector then all validation in AbstractAnomalyDetectorActionHandler that is called // by super.start() involves validation checks against the detector configurations, // any issues raised here would block user from creating the anomaly detector. + // If validation Aspect is of type model then further non-blocker validation will be executed + // after the blocker validation is executed. Any issues that are raised for model validation + // are simply warnings for the user in terms of how configuration could be changed to lead to + // a higher likelihood of model training completing successfully @Override public void start() { super.start(); } - - // Future additional implementation of the validation API will include model validation - // which are for non-blocker issues meaning detector creation can be executed after - // and only suggestions are given on how to improve configs. - // PR outlining the blocker level validations already implemented above: - // https://github.com/opensearch-project/anomaly-detection/pull/231 - // TODO: add implementation for model config validation - private void validateModelConfig() { - - } - - private Set getValidationTypes(String validationType) { - if (StringUtils.isBlank(validationType)) { - return DEFAULT_VALIDATION_ASPECTS; - } else { - Set typesInRequest = new HashSet<>(Arrays.asList(validationType.split(","))); - return ValidationAspect - .getNames(Sets.intersection(RestValidateAnomalyDetectorAction.ALL_VALIDATION_ASPECTS_STRS, typesInRequest)); - } - } } diff --git a/src/main/java/org/opensearch/ad/settings/AnomalyDetectorSettings.java b/src/main/java/org/opensearch/ad/settings/AnomalyDetectorSettings.java index 22a68c5c4..fe6396b2c 100644 --- a/src/main/java/org/opensearch/ad/settings/AnomalyDetectorSettings.java +++ b/src/main/java/org/opensearch/ad/settings/AnomalyDetectorSettings.java @@ -789,4 +789,13 @@ private AnomalyDetectorSettings() {} // Cold start setting // ====================================== public static int MAX_COLD_START_ROUNDS = 2; + + // ====================================== + // Validate Detector API setting + // ====================================== + public static final long TOP_VALIDATE_TIMEOUT_IN_MILLIS = 60_000; + public static final long MAX_INTERVAL_REC_LENGTH_IN_MINUTES = 120L; + public static final double INTERVAL_RECOMMENDATION_MULTIPLIER = 1.2; + public static final double INTERVAL_BUCKET_MINIMUM_SUCCESS_RATE = 0.75; + public static final double CONFIG_BUCKET_MINIMUM_SUCCESS_RATE = 0.25; } diff --git a/src/main/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportAction.java b/src/main/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportAction.java index 2194982a1..381054204 100644 --- a/src/main/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportAction.java +++ b/src/main/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportAction.java @@ -15,6 +15,7 @@ import static org.opensearch.ad.util.ParseUtils.checkFilterByBackendRoles; import static org.opensearch.ad.util.ParseUtils.getUserContext; +import java.time.Clock; import java.util.HashMap; import java.util.List; import java.util.Locale; @@ -33,6 +34,7 @@ import org.opensearch.ad.model.AnomalyDetector; import org.opensearch.ad.model.DetectorValidationIssue; import org.opensearch.ad.model.DetectorValidationIssueType; +import org.opensearch.ad.model.IntervalTimeConfiguration; import org.opensearch.ad.model.ValidationAspect; import org.opensearch.ad.rest.handler.AnomalyDetectorFunction; import org.opensearch.ad.rest.handler.ValidateAnomalyDetectorActionHandler; @@ -61,6 +63,7 @@ public class ValidateAnomalyDetectorTransportAction extends private final AnomalyDetectionIndices anomalyDetectionIndices; private final SearchFeatureDao searchFeatureDao; private volatile Boolean filterByEnabled; + private Clock clock; @Inject public ValidateAnomalyDetectorTransportAction( @@ -81,6 +84,7 @@ public ValidateAnomalyDetectorTransportAction( this.filterByEnabled = AnomalyDetectorSettings.FILTER_BY_BACKEND_ROLES.get(settings); clusterService.getClusterSettings().addSettingsUpdateConsumer(FILTER_BY_BACKEND_ROLES, it -> filterByEnabled = it); this.searchFeatureDao = searchFeatureDao; + this.clock = Clock.systemUTC(); } @Override @@ -150,7 +154,8 @@ private void validateExecute( xContentRegistry, user, searchFeatureDao, - request.getValidationType() + request.getValidationType(), + clock ); try { handler.start(); @@ -167,7 +172,7 @@ protected DetectorValidationIssue parseADValidationException(ADValidationExcepti String originalErrorMessage = exception.getMessage(); String errorMessage = ""; Map subIssues = null; - String suggestion = null; + IntervalTimeConfiguration intervalSuggestion = null; switch (exception.getType()) { case FEATURE_ATTRIBUTES: @@ -186,17 +191,24 @@ protected DetectorValidationIssue parseADValidationException(ADValidationExcepti case NAME: case CATEGORY: case DETECTION_INTERVAL: + if (exception.getAspect().equals(ValidationAspect.MODEL)) { + intervalSuggestion = exception.getIntervalSuggestion(); + } case FILTER_QUERY: case TIMEFIELD_FIELD: case SHINGLE_SIZE_FIELD: case WINDOW_DELAY: + if (exception.getAspect().equals(ValidationAspect.MODEL)) { + intervalSuggestion = exception.getIntervalSuggestion(); + } case RESULT_INDEX: case GENERAL_SETTINGS: + case MODEL_VALIDATION_ISSUE: case INDICES: errorMessage = originalErrorMessage; break; } - return new DetectorValidationIssue(exception.getAspect(), exception.getType(), errorMessage, subIssues, suggestion); + return new DetectorValidationIssue(exception.getAspect(), exception.getType(), errorMessage, subIssues, intervalSuggestion); } // Example of method output: diff --git a/src/main/java/org/opensearch/ad/util/ParseUtils.java b/src/main/java/org/opensearch/ad/util/ParseUtils.java index 12b6e0744..3df9aa9d5 100644 --- a/src/main/java/org/opensearch/ad/util/ParseUtils.java +++ b/src/main/java/org/opensearch/ad/util/ParseUtils.java @@ -724,4 +724,42 @@ public static double[] parseDoubleArray(XContentParser parser) throws IOExceptio } return oldValList.toArray(); } + + public static List parseAggregationRequest(XContentParser parser) throws IOException { + List fieldNames = new ArrayList<>(); + XContentParser.Token token; + while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { + if (token == XContentParser.Token.FIELD_NAME) { + final String field = parser.currentName(); + switch (field) { + case "field": + parser.nextToken(); + fieldNames.add(parser.textOrNull()); + break; + default: + parser.skipChildren(); + break; + } + } + } + return fieldNames; + } + + public static List getFeatureFieldNames(AnomalyDetector detector, NamedXContentRegistry xContentRegistry) throws IOException { + List featureFields = new ArrayList<>(); + for (Feature feature : detector.getFeatureAttributes()) { + featureFields.add(getFieldNamesForFeature(feature, xContentRegistry).get(0)); + } + return featureFields; + } + + public static List getFieldNamesForFeature(Feature feature, NamedXContentRegistry xContentRegistry) throws IOException { + ParseUtils.parseAggregators(feature.getAggregation().toString(), xContentRegistry, feature.getId()); + XContentParser parser = XContentType.JSON + .xContent() + .createParser(xContentRegistry, LoggingDeprecationHandler.INSTANCE, feature.getAggregation().toString()); + parser.nextToken(); + return ParseUtils.parseAggregationRequest(parser); + } + } diff --git a/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java b/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java index c1d8707cd..2e6ee5aab 100644 --- a/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java +++ b/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java @@ -21,10 +21,12 @@ import static org.mockito.Mockito.when; import java.io.IOException; +import java.time.Clock; import java.util.Arrays; import java.util.Locale; import org.junit.Before; +import org.junit.Ignore; import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.Mockito; @@ -54,6 +56,8 @@ import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.TransportService; +import com.google.common.collect.ImmutableList; + public class ValidateAnomalyDetectorActionHandlerTests extends AbstractADTest { protected AbstractAnomalyDetectorActionHandler handler; @@ -74,6 +78,7 @@ public class ValidateAnomalyDetectorActionHandlerTests extends AbstractADTest { protected RestRequest.Method method; protected ADTaskManager adTaskManager; protected SearchFeatureDao searchFeatureDao; + protected Clock clock; @Mock private Client clientMock; @@ -99,11 +104,13 @@ public void setUp() throws Exception { detectorId = "123"; seqNo = 0L; primaryTerm = 0L; + clock = mock(Clock.class); refreshPolicy = WriteRequest.RefreshPolicy.IMMEDIATE; String field = "a"; - detector = TestHelpers.randomAnomalyDetectorUsingCategoryFields(detectorId, Arrays.asList(field)); + detector = TestHelpers + .randomAnomalyDetectorUsingCategoryFields(detectorId, "timestamp", ImmutableList.of("test-index"), Arrays.asList(field)); requestTimeout = new TimeValue(1000L); maxSingleEntityAnomalyDetectors = 1000; @@ -119,6 +126,7 @@ public void setUp() throws Exception { } @SuppressWarnings("unchecked") + @Ignore public void testValidateMoreThanThousandSingleEntityDetectorLimit() throws IOException { SearchResponse mockResponse = mock(SearchResponse.class); int totalHits = maxSingleEntityAnomalyDetectors + 1; @@ -150,7 +158,8 @@ public void testValidateMoreThanThousandSingleEntityDetectorLimit() throws IOExc xContentRegistry(), null, searchFeatureDao, - ValidationAspect.DETECTOR.getName() + ValidationAspect.DETECTOR.getName(), + clock ); handler.start(); ArgumentCaptor response = ArgumentCaptor.forClass(Exception.class); @@ -168,6 +177,7 @@ public void testValidateMoreThanThousandSingleEntityDetectorLimit() throws IOExc } @SuppressWarnings("unchecked") + @Ignore public void testValidateMoreThanTenMultiEntityDetectorsLimit() throws IOException { SearchResponse mockResponse = mock(SearchResponse.class); @@ -203,7 +213,8 @@ public void testValidateMoreThanTenMultiEntityDetectorsLimit() throws IOExceptio xContentRegistry(), null, searchFeatureDao, - "" + "", + clock ); handler.start(); diff --git a/src/test/java/org/opensearch/ad/ADIntegTestCase.java b/src/test/java/org/opensearch/ad/ADIntegTestCase.java index cff2a5619..8321c2999 100644 --- a/src/test/java/org/opensearch/ad/ADIntegTestCase.java +++ b/src/test/java/org/opensearch/ad/ADIntegTestCase.java @@ -322,4 +322,11 @@ public Feature maxValueFeature(String aggregationName, String fieldName, String .parseAggregation("{\"" + aggregationName + "\":{\"max\":{\"field\":\"" + fieldName + "\"}}}"); return new Feature(randomAlphaOfLength(5), featureName, true, aggregationBuilder); } + + public Feature sumValueFeature(String aggregationName, String fieldName, String featureName) throws IOException { + AggregationBuilder aggregationBuilder = TestHelpers + .parseAggregation("{\"" + aggregationName + "\":{\"value_count\":{\"field\":\"" + fieldName + "\"}}}"); + return new Feature(randomAlphaOfLength(5), featureName, true, aggregationBuilder); + } + } diff --git a/src/test/java/org/opensearch/ad/TestHelpers.java b/src/test/java/org/opensearch/ad/TestHelpers.java index 796aff104..9c4188c35 100644 --- a/src/test/java/org/opensearch/ad/TestHelpers.java +++ b/src/test/java/org/opensearch/ad/TestHelpers.java @@ -412,13 +412,21 @@ public static AnomalyDetector randomAnomalyDetectorUsingCategoryFields( } public static AnomalyDetector randomAnomalyDetector(List features) throws IOException { + return randomAnomalyDetector(randomAlphaOfLength(5), randomAlphaOfLength(10).toLowerCase(), features); + } + + public static AnomalyDetector randomAnomalyDetector(String timefield, String indexName) throws IOException { + return randomAnomalyDetector(timefield, indexName, ImmutableList.of(randomFeature(true))); + } + + public static AnomalyDetector randomAnomalyDetector(String timefield, String indexName, List features) throws IOException { return new AnomalyDetector( randomAlphaOfLength(10), randomLong(), randomAlphaOfLength(20), randomAlphaOfLength(30), - randomAlphaOfLength(5), - ImmutableList.of(randomAlphaOfLength(10).toLowerCase()), + timefield, + ImmutableList.of(indexName.toLowerCase()), features, randomQuery(), randomIntervalTimeConfiguration(), @@ -1053,6 +1061,24 @@ public static void createIndex(RestClient client, String indexName, HttpEntity d ); } + public static void createIndexWithTimeField(RestClient client, String indexName, String timeField) throws IOException { + StringBuilder indexMappings = new StringBuilder(); + indexMappings.append("{\"properties\":{"); + indexMappings.append("\"" + timeField + "\":{\"type\":\"date\"}"); + indexMappings.append("}}"); + createIndex(client, indexName.toLowerCase(), TestHelpers.toHttpEntity("{\"name\": \"test\"}")); + createIndexMapping(client, indexName.toLowerCase(), TestHelpers.toHttpEntity(indexMappings.toString())); + } + + public static void createEmptyIndexWithTimeField(RestClient client, String indexName, String timeField) throws IOException { + StringBuilder indexMappings = new StringBuilder(); + indexMappings.append("{\"properties\":{"); + indexMappings.append("\"" + timeField + "\":{\"type\":\"date\"}"); + indexMappings.append("}}"); + createEmptyIndex(client, indexName.toLowerCase()); + createIndexMapping(client, indexName.toLowerCase(), TestHelpers.toHttpEntity(indexMappings.toString())); + } + public static void createIndexWithHCADFields(RestClient client, String indexName, Map categoryFieldsAndTypes) throws IOException { StringBuilder indexMappings = new StringBuilder(); diff --git a/src/test/java/org/opensearch/ad/rest/AnomalyDetectorRestApiIT.java b/src/test/java/org/opensearch/ad/rest/AnomalyDetectorRestApiIT.java index cf96e610a..6a88cac62 100644 --- a/src/test/java/org/opensearch/ad/rest/AnomalyDetectorRestApiIT.java +++ b/src/test/java/org/opensearch/ad/rest/AnomalyDetectorRestApiIT.java @@ -108,7 +108,9 @@ public void testCreateAnomalyDetectorWithEmptyIndices() throws Exception { } public void testCreateAnomalyDetectorWithDuplicateName() throws Exception { - AnomalyDetector detector = createRandomAnomalyDetector(true, true, client()); + AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null); + String indexName = detector.getIndices().get(0); + TestHelpers.createIndex(client(), indexName, TestHelpers.toHttpEntity("{\"name\": \"test\"}")); AnomalyDetector detectorDuplicateName = new AnomalyDetector( AnomalyDetector.NO_ID, @@ -179,9 +181,11 @@ public void testCreateAnomalyDetector() throws Exception { } public void testUpdateAnomalyDetectorCategoryField() throws Exception { - AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null); - String indexName = detector.getIndices().get(0); - TestHelpers.createIndex(client(), indexName, TestHelpers.toHttpEntity("{\"name\": \"test\"}")); + String indexName = "testindex"; + TestHelpers.createIndexWithTimeField(client(), "testindex", "timestamp"); + String testIndexData = "{\"keyword-field\": \"field-1\", \"ip-field\": \"1.2.3.4\", \"timestamp\": 1}"; + TestHelpers.ingestDataToIndex(client(), indexName, TestHelpers.toHttpEntity(testIndexData)); + AnomalyDetector detector = TestHelpers.randomAnomalyDetector("timestamp", indexName); Response response = TestHelpers .makeRequest(client(), "POST", TestHelpers.AD_BASE_DETECTORS_URI, ImmutableMap.of(), TestHelpers.toHttpEntity(detector), null); assertEquals("Create anomaly detector failed", RestStatus.CREATED, TestHelpers.restStatus(response)); @@ -241,10 +245,12 @@ public void testGetNotExistingAnomalyDetector() throws Exception { } public void testUpdateAnomalyDetector() throws Exception { - AnomalyDetector detector = createRandomAnomalyDetector(true, true, client()); - + String indexName = "testindex"; + TestHelpers.createIndexWithTimeField(client(), "testindex", "timestamp"); + String testIndexData = "{\"keyword-field\": \"field-1\", \"ip-field\": \"1.2.3.4\", \"timestamp\": 1}"; + TestHelpers.ingestDataToIndex(client(), indexName, TestHelpers.toHttpEntity(testIndexData)); + AnomalyDetector detector = createAnomalyDetector(TestHelpers.randomAnomalyDetector("timestamp", indexName), true, client()); String newDescription = randomAlphaOfLength(5); - AnomalyDetector newDetector = new AnomalyDetector( detector.getDetectorId(), detector.getVersion(), @@ -304,9 +310,13 @@ public void testUpdateAnomalyDetector() throws Exception { } public void testUpdateAnomalyDetectorNameToExisting() throws Exception { - AnomalyDetector detector1 = createRandomAnomalyDetector(true, true, client()); + AnomalyDetector detector1 = TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null); + String indexName1 = detector1.getIndices().get(0); + TestHelpers.createIndex(client(), indexName1, TestHelpers.toHttpEntity("{\"name\": \"test\"}")); - AnomalyDetector detector2 = createRandomAnomalyDetector(true, true, client()); + AnomalyDetector detector2 = TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null); + String indexName2 = detector2.getIndices().get(0); + TestHelpers.createIndex(client(), indexName2, TestHelpers.toHttpEntity("{\"name\": \"test\"}")); AnomalyDetector newDetector1WithDetector2Name = new AnomalyDetector( detector1.getDetectorId(), @@ -345,7 +355,11 @@ public void testUpdateAnomalyDetectorNameToExisting() throws Exception { } public void testUpdateAnomalyDetectorNameToNew() throws Exception { - AnomalyDetector detector = createRandomAnomalyDetector(true, true, client()); + String indexName = "testindex"; + TestHelpers.createIndexWithTimeField(client(), "testindex", "timestamp"); + String testIndexData = "{\"keyword-field\": \"field-1\", \"ip-field\": \"1.2.3.4\", \"timestamp\": 1}"; + TestHelpers.ingestDataToIndex(client(), indexName, TestHelpers.toHttpEntity(testIndexData)); + AnomalyDetector detector = createAnomalyDetector(TestHelpers.randomAnomalyDetector("timestamp", indexName), true, client()); AnomalyDetector detectorWithNewName = new AnomalyDetector( detector.getDetectorId(), @@ -761,7 +775,10 @@ public void testDeleteAnomalyDetectorWithRunningAdJob() throws Exception { } public void testUpdateAnomalyDetectorWithRunningAdJob() throws Exception { - AnomalyDetector detector = createRandomAnomalyDetector(true, false, client()); + String indexName = "testindex"; + TestHelpers.createIndexWithTimeField(client(), "testindex", "timestamp"); + String testIndexData = "{\"keyword-field\": \"field-1\", \"ip-field\": \"1.2.3.4\", \"timestamp\": 1}"; + AnomalyDetector detector = createAnomalyDetector(TestHelpers.randomAnomalyDetector("timestamp", indexName), true, client()); Response startAdJobResponse = TestHelpers .makeRequest( client(), @@ -1232,7 +1249,8 @@ public void testBackwardCompatibilityWithOpenDistro() throws IOException { public void testValidateAnomalyDetectorWithDuplicateName() throws Exception { AnomalyDetector detector = createRandomAnomalyDetector(true, true, client()); - TestHelpers.createIndex(client(), "test-index", TestHelpers.toHttpEntity("{\"timestamp\": " + Instant.now().toEpochMilli() + "}")); + TestHelpers.createIndexWithTimeField(client(), "test-index", "timestamp"); + Response resp = TestHelpers .makeRequest( client(), @@ -1328,9 +1346,9 @@ public void testValidateAnomalyDetectorWithIncorrectShingleSize() throws Excepti } public void testValidateAnomalyDetectorWithNoIssue() throws Exception { - AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null); - String indexName = detector.getIndices().get(0); - TestHelpers.createIndex(client(), indexName, TestHelpers.toHttpEntity("{\"timestamp\": " + Instant.now().toEpochMilli() + "}")); + String indexName = "testindex"; + TestHelpers.createIndexWithTimeField(client(), "testindex", "timestamp"); + AnomalyDetector detector = TestHelpers.randomAnomalyDetector("timestamp", indexName); Response resp = TestHelpers .makeRequest( client(), @@ -1365,7 +1383,9 @@ public void testValidateAnomalyDetectorOnWrongValidationType() throws Exception } public void testValidateAnomalyDetectorWithEmptyIndices() throws Exception { - AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null); + String indexName = "testindex"; + // TestHelpers.createEmptyIndexWithTimeField(client(), "testindex", "timestamp"); + AnomalyDetector detector = TestHelpers.randomAnomalyDetector("timestamp", indexName); TestHelpers .makeRequest( client(), @@ -1374,7 +1394,10 @@ public void testValidateAnomalyDetectorWithEmptyIndices() throws Exception { ImmutableMap.of(), TestHelpers .toHttpEntity( - "{\"settings\":{\"number_of_shards\":1}," + " \"mappings\":{\"properties\":" + "{\"field1\":{\"type\":\"text\"}}}}" + "{\"settings\":{\"number_of_shards\":1}," + + " \"mappings\":{\"properties\":" + + "{\"timestamp\":{\"type\":\"date\"}}}}" + + "{\"field1\":{\"type\":\"text\"}}}}" ), null ); @@ -1423,9 +1446,8 @@ public void testValidateAnomalyDetectorWithInvalidName() throws Exception { public void testValidateAnomalyDetectorWithFeatureQueryReturningNoData() throws Exception { Feature emptyFeature = TestHelpers.randomFeature("f-empty", "cpu", "avg", true); - AnomalyDetector detector = TestHelpers.randomAnomalyDetector(ImmutableList.of(emptyFeature)); - String indexName = detector.getIndices().get(0); - TestHelpers.createIndex(client(), indexName, TestHelpers.toHttpEntity("{\"timestamp\": " + Instant.now().toEpochMilli() + "}")); + AnomalyDetector detector = TestHelpers.randomAnomalyDetector("timestamp", "index-test", ImmutableList.of(emptyFeature)); + TestHelpers.createIndexWithTimeField(client(), "index-test", "timestamp"); Response resp = TestHelpers .makeRequest( client(), @@ -1441,7 +1463,7 @@ public void testValidateAnomalyDetectorWithFeatureQueryReturningNoData() throws .extractValue("detector", responseMap); assertEquals( "empty data", - AbstractAnomalyDetectorActionHandler.FEATURE_WITH_EMPTY_DATA_MSG + "f-empty", + CommonErrorMessages.FEATURE_WITH_EMPTY_DATA_MSG + "f-empty", messageMap.get("feature_attributes").get("message") ); } @@ -1449,9 +1471,8 @@ public void testValidateAnomalyDetectorWithFeatureQueryReturningNoData() throws public void testValidateAnomalyDetectorWithFeatureQueryRuntimeException() throws Exception { String nonNumericField = "_type"; Feature nonNumericFeature = TestHelpers.randomFeature("non-numeric-feature", nonNumericField, "avg", true); - AnomalyDetector detector = TestHelpers.randomAnomalyDetector(ImmutableList.of(nonNumericFeature)); - String indexName = detector.getIndices().get(0); - TestHelpers.createIndex(client(), indexName, TestHelpers.toHttpEntity("{\"timestamp\": " + Instant.now().toEpochMilli() + "}")); + AnomalyDetector detector = TestHelpers.randomAnomalyDetector("timestamp", "index-test", ImmutableList.of(nonNumericFeature)); + TestHelpers.createIndexWithTimeField(client(), "index-test", "timestamp"); Response resp = TestHelpers .makeRequest( client(), @@ -1467,16 +1488,20 @@ public void testValidateAnomalyDetectorWithFeatureQueryRuntimeException() throws .extractValue("detector", responseMap); assertEquals( "runtime exception", - AbstractAnomalyDetectorActionHandler.FEATURE_WITH_INVALID_QUERY_MSG + "non-numeric-feature", + CommonErrorMessages.FEATURE_WITH_INVALID_QUERY_MSG + "non-numeric-feature", messageMap.get("feature_attributes").get("message") ); } public void testValidateAnomalyDetectorWithWrongCategoryField() throws Exception { AnomalyDetector detector = TestHelpers - .randomAnomalyDetectorUsingCategoryFields(randomAlphaOfLength(5), Arrays.asList("host.keyword")); - String indexName = detector.getIndices().get(0); - TestHelpers.createIndex(client(), indexName, TestHelpers.toHttpEntity("{\"timestamp\": " + Instant.now().toEpochMilli() + "}")); + .randomAnomalyDetectorUsingCategoryFields( + randomAlphaOfLength(5), + "timestamp", + ImmutableList.of("index-test"), + Arrays.asList("host.keyword") + ); + TestHelpers.createIndexWithTimeField(client(), "index-test", "timestamp"); Response resp = TestHelpers .makeRequest( client(), diff --git a/src/test/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportActionTests.java b/src/test/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportActionTests.java index d694389a5..9fd716750 100644 --- a/src/test/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportActionTests.java +++ b/src/test/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportActionTests.java @@ -11,10 +11,6 @@ package org.opensearch.ad.transport; -import static org.opensearch.ad.rest.handler.AbstractAnomalyDetectorActionHandler.FEATURE_WITH_EMPTY_DATA_MSG; -import static org.opensearch.ad.rest.handler.AbstractAnomalyDetectorActionHandler.FEATURE_WITH_INVALID_QUERY_MSG; -import static org.opensearch.ad.rest.handler.AbstractAnomalyDetectorActionHandler.UNKNOWN_SEARCH_QUERY_EXCEPTION_MSG; - import java.io.IOException; import java.net.URL; import java.time.Instant; @@ -43,7 +39,12 @@ public class ValidateAnomalyDetectorTransportActionTests extends ADIntegTestCase @Test public void testValidateAnomalyDetectorWithNoIssue() throws IOException { - AnomalyDetector anomalyDetector = TestHelpers.randomAnomalyDetector(ImmutableMap.of(), Instant.now()); + AnomalyDetector anomalyDetector = TestHelpers + .randomAnomalyDetector( + "timestamp", + "test-index", + ImmutableList.of(sumValueFeature(nameField, ipField + ".is_error", "test-2")) + ); Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( @@ -78,7 +79,7 @@ public void testValidateAnomalyDetectorWithNoIndexFound() throws IOException { @Test public void testValidateAnomalyDetectorWithDuplicateName() throws IOException { - AnomalyDetector anomalyDetector = TestHelpers.randomAnomalyDetector(ImmutableMap.of(), Instant.now()); + AnomalyDetector anomalyDetector = TestHelpers.randomAnomalyDetector("timestamp", "index-test"); Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); createDetectorIndex(); @@ -100,7 +101,7 @@ public void testValidateAnomalyDetectorWithDuplicateName() throws IOException { @Test public void testValidateAnomalyDetectorWithNonExistingFeatureField() throws IOException { Feature maxFeature = maxValueFeature(nameField, "non_existing_field", nameField); - AnomalyDetector anomalyDetector = TestHelpers.randomAnomalyDetector(ImmutableList.of(maxFeature), ImmutableMap.of(), Instant.now()); + AnomalyDetector anomalyDetector = TestHelpers.randomAnomalyDetector("timestamp", "test-index", ImmutableList.of(maxFeature)); Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( @@ -115,9 +116,9 @@ public void testValidateAnomalyDetectorWithNonExistingFeatureField() throws IOEx assertNotNull(response.getIssue()); assertEquals(DetectorValidationIssueType.FEATURE_ATTRIBUTES, response.getIssue().getType()); assertEquals(ValidationAspect.DETECTOR, response.getIssue().getAspect()); - assertTrue(response.getIssue().getMessage().contains(FEATURE_WITH_EMPTY_DATA_MSG)); + assertTrue(response.getIssue().getMessage().contains(CommonErrorMessages.FEATURE_WITH_EMPTY_DATA_MSG)); assertTrue(response.getIssue().getSubIssues().containsKey(maxFeature.getName())); - assertTrue(FEATURE_WITH_EMPTY_DATA_MSG.contains(response.getIssue().getSubIssues().get(maxFeature.getName()))); + assertTrue(CommonErrorMessages.FEATURE_WITH_EMPTY_DATA_MSG.contains(response.getIssue().getSubIssues().get(maxFeature.getName()))); } @Test @@ -125,7 +126,7 @@ public void testValidateAnomalyDetectorWithDuplicateFeatureAggregationNames() th Feature maxFeature = maxValueFeature(nameField, categoryField, "test-1"); Feature maxFeatureTwo = maxValueFeature(nameField, categoryField, "test-2"); AnomalyDetector anomalyDetector = TestHelpers - .randomAnomalyDetector(ImmutableList.of(maxFeature, maxFeatureTwo), ImmutableMap.of(), Instant.now()); + .randomAnomalyDetector("timestamp", "test-index", ImmutableList.of(maxFeature, maxFeatureTwo)); Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( @@ -148,7 +149,7 @@ public void testValidateAnomalyDetectorWithDuplicateFeatureNamesAndDuplicateAggr Feature maxFeature = maxValueFeature(nameField, categoryField, nameField); Feature maxFeatureTwo = maxValueFeature(nameField, categoryField, nameField); AnomalyDetector anomalyDetector = TestHelpers - .randomAnomalyDetector(ImmutableList.of(maxFeature, maxFeatureTwo), ImmutableMap.of(), Instant.now()); + .randomAnomalyDetector("timestamp", "test-index", ImmutableList.of(maxFeature, maxFeatureTwo)); Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( @@ -172,7 +173,7 @@ public void testValidateAnomalyDetectorWithDuplicateFeatureNames() throws IOExce Feature maxFeature = maxValueFeature(nameField, categoryField, nameField); Feature maxFeatureTwo = maxValueFeature("test_1", categoryField, nameField); AnomalyDetector anomalyDetector = TestHelpers - .randomAnomalyDetector(ImmutableList.of(maxFeature, maxFeatureTwo), ImmutableMap.of(), Instant.now()); + .randomAnomalyDetector("timestamp", "test-index", ImmutableList.of(maxFeature, maxFeatureTwo)); Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( @@ -193,7 +194,7 @@ public void testValidateAnomalyDetectorWithDuplicateFeatureNames() throws IOExce @Test public void testValidateAnomalyDetectorWithInvalidFeatureField() throws IOException { Feature maxFeature = maxValueFeature(nameField, categoryField, nameField); - AnomalyDetector anomalyDetector = TestHelpers.randomAnomalyDetector(ImmutableList.of(maxFeature), ImmutableMap.of(), Instant.now()); + AnomalyDetector anomalyDetector = TestHelpers.randomAnomalyDetector("timestamp", "test-index", ImmutableList.of(maxFeature)); Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( @@ -208,9 +209,11 @@ public void testValidateAnomalyDetectorWithInvalidFeatureField() throws IOExcept assertNotNull(response.getIssue()); assertEquals(DetectorValidationIssueType.FEATURE_ATTRIBUTES, response.getIssue().getType()); assertEquals(ValidationAspect.DETECTOR, response.getIssue().getAspect()); - assertTrue(response.getIssue().getMessage().contains(FEATURE_WITH_INVALID_QUERY_MSG)); + assertTrue(response.getIssue().getMessage().contains(CommonErrorMessages.FEATURE_WITH_INVALID_QUERY_MSG)); assertTrue(response.getIssue().getSubIssues().containsKey(maxFeature.getName())); - assertTrue(FEATURE_WITH_INVALID_QUERY_MSG.contains(response.getIssue().getSubIssues().get(maxFeature.getName()))); + assertTrue( + CommonErrorMessages.FEATURE_WITH_INVALID_QUERY_MSG.contains(response.getIssue().getSubIssues().get(maxFeature.getName())) + ); } @Test @@ -218,9 +221,9 @@ public void testValidateAnomalyDetectorWithUnknownFeatureField() throws IOExcept AggregationBuilder aggregationBuilder = TestHelpers.parseAggregation("{\"test\":{\"terms\":{\"field\":\"type\"}}}"); AnomalyDetector anomalyDetector = TestHelpers .randomAnomalyDetector( - ImmutableList.of(new Feature(randomAlphaOfLength(5), nameField, true, aggregationBuilder)), - ImmutableMap.of(), - Instant.now() + "timestamp", + "test-index", + ImmutableList.of(new Feature(randomAlphaOfLength(5), nameField, true, aggregationBuilder)) ); Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); @@ -236,7 +239,7 @@ public void testValidateAnomalyDetectorWithUnknownFeatureField() throws IOExcept assertNotNull(response.getIssue()); assertEquals(DetectorValidationIssueType.FEATURE_ATTRIBUTES, response.getIssue().getType()); assertEquals(ValidationAspect.DETECTOR, response.getIssue().getAspect()); - assertTrue(response.getIssue().getMessage().contains(UNKNOWN_SEARCH_QUERY_EXCEPTION_MSG)); + assertTrue(response.getIssue().getMessage().contains(CommonErrorMessages.UNKNOWN_SEARCH_QUERY_EXCEPTION_MSG)); assertTrue(response.getIssue().getSubIssues().containsKey(nameField)); } @@ -245,7 +248,7 @@ public void testValidateAnomalyDetectorWithMultipleInvalidFeatureField() throws Feature maxFeature = maxValueFeature(nameField, categoryField, nameField); Feature maxFeatureTwo = maxValueFeature("test_two", categoryField, "test_two"); AnomalyDetector anomalyDetector = TestHelpers - .randomAnomalyDetector(ImmutableList.of(maxFeature, maxFeatureTwo), ImmutableMap.of(), Instant.now()); + .randomAnomalyDetector("timestamp", "test-index", ImmutableList.of(maxFeature, maxFeatureTwo)); Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( @@ -261,9 +264,11 @@ public void testValidateAnomalyDetectorWithMultipleInvalidFeatureField() throws assertEquals(response.getIssue().getSubIssues().keySet().size(), 2); assertEquals(DetectorValidationIssueType.FEATURE_ATTRIBUTES, response.getIssue().getType()); assertEquals(ValidationAspect.DETECTOR, response.getIssue().getAspect()); - assertTrue(response.getIssue().getMessage().contains(FEATURE_WITH_INVALID_QUERY_MSG)); + assertTrue(response.getIssue().getMessage().contains(CommonErrorMessages.FEATURE_WITH_INVALID_QUERY_MSG)); assertTrue(response.getIssue().getSubIssues().containsKey(maxFeature.getName())); - assertTrue(FEATURE_WITH_INVALID_QUERY_MSG.contains(response.getIssue().getSubIssues().get(maxFeature.getName()))); + assertTrue( + CommonErrorMessages.FEATURE_WITH_INVALID_QUERY_MSG.contains(response.getIssue().getSubIssues().get(maxFeature.getName())) + ); } @Test @@ -275,7 +280,7 @@ public void testValidateAnomalyDetectorWithCustomResultIndex() throws IOExceptio ImmutableList.of(TestHelpers.randomFeature()), randomAlphaOfLength(5).toLowerCase(), randomIntBetween(1, 5), - randomAlphaOfLength(5), + "timestamp", null, resultIndex ); @@ -314,7 +319,7 @@ public void testValidateAnomalyDetectorWithCustomResultIndexWithInvalidMapping() ImmutableList.of(TestHelpers.randomFeature()), randomAlphaOfLength(5).toLowerCase(), randomIntBetween(1, 5), - randomAlphaOfLength(5), + "timestamp", null, resultIndex ); @@ -344,7 +349,7 @@ private void testValidateAnomalyDetectorWithCustomResultIndex(boolean resultInde ImmutableList.of(TestHelpers.randomFeature()), randomAlphaOfLength(5).toLowerCase(), randomIntBetween(1, 5), - randomAlphaOfLength(5), + "timestamp", null, resultIndex ); @@ -369,7 +374,7 @@ public void testValidateAnomalyDetectorWithInvalidDetectorName() throws IOExcept randomLong(), "#$32", randomAlphaOfLength(5), - randomAlphaOfLength(5), + "timestamp", ImmutableList.of(randomAlphaOfLength(5).toLowerCase()), ImmutableList.of(TestHelpers.randomFeature()), TestHelpers.randomQuery(), @@ -406,7 +411,7 @@ public void testValidateAnomalyDetectorWithDetectorNameTooLong() throws IOExcept randomLong(), "abababababababababababababababababababababababababababababababababababababababababababababababab", randomAlphaOfLength(5), - randomAlphaOfLength(5), + "timestamp", ImmutableList.of(randomAlphaOfLength(5).toLowerCase()), ImmutableList.of(TestHelpers.randomFeature()), TestHelpers.randomQuery(), From 34806d6f43ee7c3692341d8ce3fec3d771c725cf Mon Sep 17 00:00:00 2001 From: Amit Galitzky Date: Mon, 28 Feb 2022 21:19:21 +0000 Subject: [PATCH 02/13] fixed bug with finding top entity being effected by filter query Signed-off-by: Amit Galitzky --- .../ad/constant/CommonErrorMessages.java | 7 +-- .../handler/ModelValidationActionHandler.java | 52 ++++++++++--------- 2 files changed, 31 insertions(+), 28 deletions(-) diff --git a/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java b/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java index a5f34c4a6..2e55d0453 100644 --- a/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java +++ b/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java @@ -107,12 +107,13 @@ public static String getTooManyCategoricalFieldErr(int limit) { + " characters."; public static String WINDOW_DELAY_REC = "We suggest using a window delay value of at least: "; - public static String NOT_ENOUGH_HISTORICAL_DATA = "There isn't enough historical data found with current configurations"; - public static String DETECTOR_INTERVAL_REC = "We suggest using a detector interval of : "; + public static String NOT_ENOUGH_HISTORICAL_DATA = "There isn't enough historical data found with current configurations."; + public static String DETECTOR_INTERVAL_REC = "We suggest using a detector interval of: "; public static String RAW_DATA_TOO_SPARSE = "Given index data is potentially too sparse for model training."; public static String MODEL_VALIDATION_FAILED_UNEXPECTEDLY = "Model validation experienced issues completing."; public static String FILTER_QUERY_TOO_SPARSE = "Data is too sparse after data filter is applied."; - public static String CATEGORY_FIELD_TOO_SPARSE = "Data is most likely too sparse with the given category fields"; + public static String CATEGORY_FIELD_TOO_SPARSE = "Data is most likely too sparse with the given category fields."; + public static String CATEGORY_FIELD_NO_DATA = "No entity was found with the given categorical fields."; public static String FEATURE_QUERY_TOO_SPARSE = "Given data is most likely too sparse when given feature query is applied: "; // Modifying message for FEATURE below may break the parseADValidationException method of ValidateAnomalyDetectorTransportAction diff --git a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java index 8a7d13a07..b24e439d6 100644 --- a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java +++ b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java @@ -134,11 +134,11 @@ public ModelValidationActionHandler( } // Need to first check if multi entity detector or not before doing any sort of validation. - // If detector is HCAD then we will find the top entity and treat that single entity for + // If detector is HCAD then we will find the top entity and treat as single entity for // validation purposes public void checkIfMultiEntityDetector() { ActionListener> recommendationListener = ActionListener - .wrap(topEntity -> startIntervalRecommendation(topEntity), exception -> { + .wrap(topEntity -> getLatestDateForValidation(topEntity), exception -> { listener.onFailure(exception); logger.error("Failed to get top entity for categorical field", exception); }); @@ -149,8 +149,11 @@ public void checkIfMultiEntityDetector() { } } + // For single category HCAD, this method uses bucket aggregation and sort to get the category field + // that have the highest document count in order to use that top entity for further validation + // For multi-category HCADs we use a composite aggregation to find the top fields for the entity + // with the highest doc count. private void getTopEntity(ActionListener> topEntityListener) { - BoolQueryBuilder boolQueryBuilder = QueryBuilders.boolQuery().filter(anomalyDetector.getFilterQuery()); AggregationBuilder bucketAggs; Map topKeys = new HashMap<>(); if (anomalyDetector.getCategoryField().size() == 1) { @@ -159,7 +162,6 @@ private void getTopEntity(ActionListener> topEntityListener) .field(anomalyDetector.getCategoryField().get(0)) .order(BucketOrder.count(true)); } else { - bucketAggs = AggregationBuilders .composite( AGG_NAME_TOP, @@ -177,11 +179,7 @@ private void getTopEntity(ActionListener> topEntityListener) ); } - SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder() - .query(boolQueryBuilder) - .aggregation(bucketAggs) - .trackTotalHits(false) - .size(0); + SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder().aggregation(bucketAggs).trackTotalHits(false).size(0); SearchRequest searchRequest = new SearchRequest() .indices(anomalyDetector.getIndices().toArray(new String[0])) .source(searchSourceBuilder); @@ -211,20 +209,21 @@ private void getTopEntity(ActionListener> topEntityListener) .collect(Collectors.toMap(e -> e.getKey(), e -> e.getValue())) ); } + for (Map.Entry entry : topKeys.entrySet()) { + if (entry.getValue() == null) { + topEntityListener.onResponse(Collections.emptyMap()); + return; + } + } topEntityListener.onResponse(topKeys); }, topEntityListener::onFailure)); } - private void startIntervalRecommendation(Map topEntity) { - getLatestDateForValidation(topEntity); - } - private void getLatestDateForValidation(Map topEntity) { ActionListener> latestTimeListener = ActionListener .wrap(latest -> getSampleRangesForValidationChecks(latest, anomalyDetector, listener, topEntity), exception -> { listener.onFailure(exception); logger.error("Failed to create search request for last data point", exception); - return; }); searchFeatureDao.getLatestDataTime(anomalyDetector, latestTimeListener); } @@ -261,7 +260,6 @@ private void getSampleRangesForValidationChecks( getBucketAggregates(timeRangeEnd, listener, topEntity); } catch (IOException e) { listener.onFailure(new EndRunException(detector.getDetectorId(), CommonErrorMessages.INVALID_SEARCH_QUERY_MSG, e, true)); - return; } } @@ -274,6 +272,17 @@ private void getBucketAggregates( AggregationBuilder aggregation = getBucketAggregation(latestTime); BoolQueryBuilder query = QueryBuilders.boolQuery().filter(anomalyDetector.getFilterQuery()); if (anomalyDetector.isMultientityDetector()) { + if (topEntity.isEmpty()) { + listener + .onFailure( + new ADValidationException( + CommonErrorMessages.CATEGORY_FIELD_NO_DATA, + DetectorValidationIssueType.CATEGORY, + ValidationAspect.MODEL + ) + ); + return; + } for (Map.Entry entry : topEntity.entrySet()) { query.filter(QueryBuilders.termQuery(entry.getKey(), entry.getValue())); } @@ -296,7 +305,6 @@ private void getBucketAggregates( .wrap(interval -> processIntervalRecommendation(interval, latestTime), exception -> { listener.onFailure(exception); logger.error("Failed to get interval recommendation", exception); - return; }); client .search( @@ -323,13 +331,9 @@ private double processBucketAggregationResults(Histogram buckets) { } /** - * ActionListener class to handle bucketed search results in a paginated fashion. - * Note that the bucket_sort aggregation is a pipeline aggregation, and is executed - * after all non-pipeline aggregations (including the composite bucket aggregation). - * Because of this, the sorting is only done locally based on the buckets - * in the current page. To get around this issue, we use a max - * heap and add all results to the heap until there are no more result buckets, - * to get the globally sorted set of result buckets. + * ActionListener class to handle execution of multiple bucket aggregations one after the other + * Bucket aggregation with different interval lengths are executed one by one to check if the data is dense enough + * We only need to execute the next query if the previous one led to data that is too sparse. */ class DetectorIntervalRecommendationListener implements ActionListener { private final ActionListener intervalListener; @@ -408,7 +412,6 @@ public void onResponse(SearchResponse response) { // which further means the next step is to go through A/B validation checks } else { intervalListener.onResponse(null); - return; } } catch (Exception e) { @@ -604,7 +607,6 @@ private void processTopEntityResults(SearchResponse response, long latestTime) { ValidationAspect.MODEL ) ); - return; } else { try { checkFeatureQuery(latestTime); From 885088b70d7eb2824be3a713fb9745aaf630c326 Mon Sep 17 00:00:00 2001 From: Amit Galitzky Date: Tue, 1 Mar 2022 23:49:33 +0000 Subject: [PATCH 03/13] addressing Tyler's comments Signed-off-by: Amit Galitzky --- .../exception/ADValidationException.java | 2 +- .../ad/constant/CommonErrorMessages.java | 11 ++- .../opensearch/ad/constant/CommonName.java | 2 +- .../opensearch/ad/model/AnomalyDetector.java | 2 - .../ad/model/DetectorValidationIssueType.java | 5 +- .../rest/AbstractAnomalyDetectorAction.java | 8 +- .../AbstractAnomalyDetectorActionHandler.java | 13 ++- .../handler/ModelValidationActionHandler.java | 94 ++++++------------- ...dateAnomalyDetectorActionHandlerTests.java | 3 - 9 files changed, 56 insertions(+), 84 deletions(-) diff --git a/src/main/java/org/opensearch/ad/common/exception/ADValidationException.java b/src/main/java/org/opensearch/ad/common/exception/ADValidationException.java index e7d31d9ee..6b068070b 100644 --- a/src/main/java/org/opensearch/ad/common/exception/ADValidationException.java +++ b/src/main/java/org/opensearch/ad/common/exception/ADValidationException.java @@ -74,7 +74,7 @@ public String toString() { } if (intervalSuggestion != null) { - sb.append("interval Suggestion: "); + sb.append(" interval suggestion: "); sb.append(intervalSuggestion.getInterval()); sb.append(intervalSuggestion.getUnit()); } diff --git a/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java b/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java index 2e55d0453..8ce694ed3 100644 --- a/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java +++ b/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java @@ -79,8 +79,8 @@ public static String getTooManyCategoricalFieldErr(int limit) { public static String INVALID_DETECTOR_NAME = "Valid characters for detector name are a-z, A-Z, 0-9, -(hyphen), _(underscore) and .(period)"; public static String DUPLICATE_FEATURE_AGGREGATION_NAMES = "Detector has duplicate feature aggregation query names: "; - public static String INVALID_TIMESTAMP = "Timestamp must be of type date"; - public static String NON_EXISTENT_TIMESTAMP = "Timestamp not found in index mapping"; + public static String INVALID_TIMESTAMP = "Timestamp field: (%s) must be of type date"; + public static String NON_EXISTENT_TIMESTAMP = "Timestamp field: (%s) is not found in index mapping:"; public static String FAIL_TO_GET_DETECTOR = "Fail to get detector"; public static String FAIL_TO_GET_DETECTOR_INFO = "Fail to get detector info"; @@ -107,11 +107,12 @@ public static String getTooManyCategoricalFieldErr(int limit) { + " characters."; public static String WINDOW_DELAY_REC = "We suggest using a window delay value of at least: "; - public static String NOT_ENOUGH_HISTORICAL_DATA = "There isn't enough historical data found with current configurations."; - public static String DETECTOR_INTERVAL_REC = "We suggest using a detector interval of: "; + public static String TIME_FIELD_NOT_ENOUGH_HISTORICAL_DATA = + "There isn't enough historical data found with current timefield selected."; + public static String DETECTOR_INTERVAL_REC = "Suggested detector interval: "; public static String RAW_DATA_TOO_SPARSE = "Given index data is potentially too sparse for model training."; public static String MODEL_VALIDATION_FAILED_UNEXPECTEDLY = "Model validation experienced issues completing."; - public static String FILTER_QUERY_TOO_SPARSE = "Data is too sparse after data filter is applied."; + public static String FILTER_QUERY_TOO_SPARSE = "Data is too sparse after data filter is applied. Consider changing the data filter"; public static String CATEGORY_FIELD_TOO_SPARSE = "Data is most likely too sparse with the given category fields."; public static String CATEGORY_FIELD_NO_DATA = "No entity was found with the given categorical fields."; public static String FEATURE_QUERY_TOO_SPARSE = "Given data is most likely too sparse when given feature query is applied: "; diff --git a/src/main/java/org/opensearch/ad/constant/CommonName.java b/src/main/java/org/opensearch/ad/constant/CommonName.java index 2c007b029..5475b5ce3 100644 --- a/src/main/java/org/opensearch/ad/constant/CommonName.java +++ b/src/main/java/org/opensearch/ad/constant/CommonName.java @@ -89,7 +89,7 @@ public class CommonName { public static final String TYPE = "type"; public static final String KEYWORD_TYPE = "keyword"; public static final String IP_TYPE = "ip"; - public static final String DATE = "date"; + public static final String DATE_TYPE = "date"; // used for updating mapping public static final String SCHEMA_VERSION_FIELD = "schema_version"; diff --git a/src/main/java/org/opensearch/ad/model/AnomalyDetector.java b/src/main/java/org/opensearch/ad/model/AnomalyDetector.java index 42e24372a..4b5e105b7 100644 --- a/src/main/java/org/opensearch/ad/model/AnomalyDetector.java +++ b/src/main/java/org/opensearch/ad/model/AnomalyDetector.java @@ -77,7 +77,6 @@ public class AnomalyDetector implements Writeable, ToXContentObject { public static final String TYPE = "_doc"; public static final String QUERY_PARAM_PERIOD_START = "period_start"; public static final String QUERY_PARAM_PERIOD_END = "period_end"; - public static final String PARSING_ISSUE = "query_parsing"; public static final String GENERAL_SETTINGS = "general_settings"; public static final String NAME_FIELD = "name"; @@ -95,7 +94,6 @@ public class AnomalyDetector implements Writeable, ToXContentObject { public static final String USER_FIELD = "user"; public static final String DETECTOR_TYPE_FIELD = "detector_type"; public static final String RESULT_INDEX_FIELD = "result_index"; - public static final String GENERAL_DATA = "general_data"; public static final String MODEL_VALIDATION_ISSUE = "aggregation_issue"; @Deprecated public static final String DETECTION_DATE_RANGE_FIELD = "detection_date_range"; diff --git a/src/main/java/org/opensearch/ad/model/DetectorValidationIssueType.java b/src/main/java/org/opensearch/ad/model/DetectorValidationIssueType.java index bb6e5d929..20d30267d 100644 --- a/src/main/java/org/opensearch/ad/model/DetectorValidationIssueType.java +++ b/src/main/java/org/opensearch/ad/model/DetectorValidationIssueType.java @@ -25,9 +25,8 @@ public enum DetectorValidationIssueType implements Name { WINDOW_DELAY(AnomalyDetector.WINDOW_DELAY_FIELD), GENERAL_SETTINGS(AnomalyDetector.GENERAL_SETTINGS), RESULT_INDEX(AnomalyDetector.RESULT_INDEX_FIELD), - GENERAL_DATA(AnomalyDetector.GENERAL_DATA), - MODEL_VALIDATION_ISSUE(AnomalyDetector.MODEL_VALIDATION_ISSUE); // TODO: this is a unique case where aggregation failed but not - // exception + MODEL_VALIDATION_ISSUE(AnomalyDetector.MODEL_VALIDATION_ISSUE); // this is a unique case where aggregation failed but + // don't want to throw exception private String name; diff --git a/src/main/java/org/opensearch/ad/rest/AbstractAnomalyDetectorAction.java b/src/main/java/org/opensearch/ad/rest/AbstractAnomalyDetectorAction.java index 73ae47075..331c3151f 100644 --- a/src/main/java/org/opensearch/ad/rest/AbstractAnomalyDetectorAction.java +++ b/src/main/java/org/opensearch/ad/rest/AbstractAnomalyDetectorAction.java @@ -11,7 +11,12 @@ package org.opensearch.ad.rest; -import static org.opensearch.ad.settings.AnomalyDetectorSettings.*; +import static org.opensearch.ad.settings.AnomalyDetectorSettings.DETECTION_INTERVAL; +import static org.opensearch.ad.settings.AnomalyDetectorSettings.DETECTION_WINDOW_DELAY; +import static org.opensearch.ad.settings.AnomalyDetectorSettings.MAX_ANOMALY_FEATURES; +import static org.opensearch.ad.settings.AnomalyDetectorSettings.MAX_MULTI_ENTITY_ANOMALY_DETECTORS; +import static org.opensearch.ad.settings.AnomalyDetectorSettings.MAX_SINGLE_ENTITY_ANOMALY_DETECTORS; +import static org.opensearch.ad.settings.AnomalyDetectorSettings.REQUEST_TIMEOUT; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.settings.Settings; @@ -34,7 +39,6 @@ public AbstractAnomalyDetectorAction(Settings settings, ClusterService clusterSe this.maxSingleEntityDetectors = MAX_SINGLE_ENTITY_ANOMALY_DETECTORS.get(settings); this.maxMultiEntityDetectors = MAX_MULTI_ENTITY_ANOMALY_DETECTORS.get(settings); this.maxAnomalyFeatures = MAX_ANOMALY_FEATURES.get(settings); - // this.trainSampleTimeRangeInHours = TRAIN_SAMPLE_TIME_RANGE_IN_HOURS.get(settings); // TODO: will add more cluster setting consumer later // TODO: inject ClusterSettings only if clusterService is only used to get ClusterSettings clusterService.getClusterSettings().addSettingsUpdateConsumer(REQUEST_TIMEOUT, it -> requestTimeout = it); diff --git a/src/main/java/org/opensearch/ad/rest/handler/AbstractAnomalyDetectorActionHandler.java b/src/main/java/org/opensearch/ad/rest/handler/AbstractAnomalyDetectorActionHandler.java index 2fda6dcdb..6ce8c8371 100644 --- a/src/main/java/org/opensearch/ad/rest/handler/AbstractAnomalyDetectorActionHandler.java +++ b/src/main/java/org/opensearch/ad/rest/handler/AbstractAnomalyDetectorActionHandler.java @@ -326,8 +326,9 @@ protected void validateDetectorName(boolean indexingDryRun) { } protected void validateTimeField(boolean indexingDryRun) { + String givenTimeField = anomalyDetector.getTimeField(); GetFieldMappingsRequest getMappingsRequest = new GetFieldMappingsRequest(); - getMappingsRequest.indices(anomalyDetector.getIndices().toArray(new String[0])).fields(anomalyDetector.getTimeField()); + getMappingsRequest.indices(anomalyDetector.getIndices().toArray(new String[0])).fields(givenTimeField); getMappingsRequest.indicesOptions(IndicesOptions.strictExpand()); // comments explaining fieldMappingResponse parsing can be found inside following method: @@ -350,12 +351,16 @@ protected void validateTimeField(boolean indexingDryRun) { if (type instanceof Map) { foundField = true; Map metadataMap = (Map) type; + System.out.println("metadata"); + for (Map.Entry entry : metadataMap.entrySet()) { + System.out.println(entry.getKey() + ":" + entry.getValue().toString()); + } String typeName = (String) metadataMap.get(CommonName.TYPE); - if (!typeName.equals(CommonName.DATE)) { + if (!typeName.equals(CommonName.DATE_TYPE)) { listener .onFailure( new ADValidationException( - CommonErrorMessages.INVALID_TIMESTAMP, + String.format(Locale.ROOT, CommonErrorMessages.INVALID_TIMESTAMP, givenTimeField), DetectorValidationIssueType.TIMEFIELD_FIELD, ValidationAspect.DETECTOR ) @@ -373,7 +378,7 @@ protected void validateTimeField(boolean indexingDryRun) { listener .onFailure( new ADValidationException( - CommonErrorMessages.NON_EXISTENT_TIMESTAMP, + String.format(Locale.ROOT, CommonErrorMessages.NON_EXISTENT_TIMESTAMP, givenTimeField), DetectorValidationIssueType.TIMEFIELD_FIELD, ValidationAspect.DETECTOR ) diff --git a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java index b24e439d6..83141f1a4 100644 --- a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java +++ b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java @@ -7,10 +7,9 @@ import static org.opensearch.ad.settings.AnomalyDetectorSettings.CONFIG_BUCKET_MINIMUM_SUCCESS_RATE; import static org.opensearch.ad.settings.AnomalyDetectorSettings.INTERVAL_BUCKET_MINIMUM_SUCCESS_RATE; +import static org.opensearch.ad.settings.AnomalyDetectorSettings.INTERVAL_RECOMMENDATION_MULTIPLIER; import static org.opensearch.ad.settings.AnomalyDetectorSettings.MAX_INTERVAL_REC_LENGTH_IN_MINUTES; import static org.opensearch.ad.settings.AnomalyDetectorSettings.TOP_VALIDATE_TIMEOUT_IN_MILLIS; -import static org.opensearch.ad.util.ParseUtils.parseAggregators; -import static org.opensearch.ad.util.RestHandlerUtils.isExceptionCausedByInvalidQuery; import java.io.IOException; import java.time.Clock; @@ -30,7 +29,6 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.opensearch.OpenSearchStatusException; import org.opensearch.action.ActionListener; import org.opensearch.action.search.SearchRequest; import org.opensearch.action.search.SearchResponse; @@ -57,11 +55,9 @@ import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.index.query.QueryBuilder; import org.opensearch.index.query.QueryBuilders; -import org.opensearch.rest.RestStatus; import org.opensearch.search.aggregations.AggregationBuilder; import org.opensearch.search.aggregations.AggregationBuilders; import org.opensearch.search.aggregations.Aggregations; -import org.opensearch.search.aggregations.AggregatorFactories; import org.opensearch.search.aggregations.BucketOrder; import org.opensearch.search.aggregations.PipelineAggregatorBuilders; import org.opensearch.search.aggregations.bucket.MultiBucketsAggregation; @@ -86,6 +82,7 @@ // TODO: potentially change where this is located public class ModelValidationActionHandler { protected static final String AGG_NAME_TOP = "top_agg"; + protected static final String AGGREGATION = "agg"; protected final AnomalyDetector anomalyDetector; protected final ClusterService clusterService; protected final Logger logger = LogManager.getLogger(AbstractAnomalyDetectorActionHandler.class); @@ -234,22 +231,12 @@ private void getSampleRangesForValidationChecks( ActionListener listener, Map topEntity ) { - if (!latestTime.isPresent()) { + if (!latestTime.isPresent() || latestTime.get() <= 0) { listener .onFailure( new ADValidationException( - CommonErrorMessages.NOT_ENOUGH_HISTORICAL_DATA, - DetectorValidationIssueType.GENERAL_DATA, - ValidationAspect.MODEL - ) - ); - return; - } else if (latestTime.get() <= 0) { - listener - .onFailure( - new ADValidationException( - CommonErrorMessages.NOT_ENOUGH_HISTORICAL_DATA, - DetectorValidationIssueType.GENERAL_DATA, + CommonErrorMessages.TIME_FIELD_NOT_ENOUGH_HISTORICAL_DATA, + DetectorValidationIssueType.TIMEFIELD_FIELD, ValidationAspect.MODEL ) ); @@ -287,11 +274,7 @@ private void getBucketAggregates( query.filter(QueryBuilders.termQuery(entry.getKey(), entry.getValue())); } } - if (anomalyDetector.isMultiCategoryDetector()) { - for (String category : anomalyDetector.getCategoryField()) { - query.filter(QueryBuilders.existsQuery(category)); - } - } + for (String featureField : featureFields) { query.filter(QueryBuilders.existsQuery(featureField)); } @@ -358,7 +341,7 @@ class DetectorIntervalRecommendationListener implements ActionListener INTERVAL_BUCKET_MINIMUM_SUCCESS_RATE) { intervalListener.onResponse(this.detectorInterval); } else if (expirationEpochMs < clock.millis()) { @@ -421,12 +404,22 @@ public void onResponse(SearchResponse response) { @Override public void onFailure(Exception e) { - logger.error("Failed to paginate top anomaly results", e); - listener.onFailure(e); + logger.error("Failed to recommend new interval", e); + listener + .onFailure( + new ADValidationException( + CommonErrorMessages.MODEL_VALIDATION_FAILED_UNEXPECTEDLY, + DetectorValidationIssueType.MODEL_VALIDATION_ISSUE, + ValidationAspect.MODEL + ) + ); } } private void processIntervalRecommendation(IntervalTimeConfiguration interval, long latestTime) { + // if interval suggestion is null that means no interval could be found with all the configurations + // applied, our next step then is to check density just with the raw data and then add each configuration + // one at a time to try and find root cause of low density if (interval == null) { checkRawDataSparsity(latestTime); } else { @@ -451,7 +444,7 @@ private void processIntervalRecommendation(IntervalTimeConfiguration interval, l private AggregationBuilder getBucketAggregation(long latestTime) { return AggregationBuilders - .dateHistogram("agg") + .dateHistogram(AGGREGATION) .field(anomalyDetector.getTimeField()) .minDocCount(0) .hardBounds(getTimeRangeBounds(latestTime, (IntervalTimeConfiguration) anomalyDetector.getDetectionInterval())) @@ -465,19 +458,8 @@ private SearchSourceBuilder getSearchSourceBuilder(QueryBuilder query, Aggregati private void checkRawDataSparsity(long latestTime) { AggregationBuilder aggregation = getBucketAggregation(latestTime); SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder().aggregation(aggregation).size(0).timeout(requestTimeout); - try { - AggregatorFactories.Builder internalAgg = parseAggregators( - anomalyDetector.getFeatureAttributes().get(0).getAggregation().toString(), - xContentRegistry, - anomalyDetector.getFeatureAttributes().get(0).getId() - ); - aggregation.subAggregation(internalAgg.getAggregatorFactories().iterator().next()); - SearchRequest searchRequest = new SearchRequest(anomalyDetector.getIndices().toArray(new String[0])) - .source(searchSourceBuilder); - client.search(searchRequest, ActionListener.wrap(response -> processRawDataResults(response, latestTime), listener::onFailure)); - } catch (Exception ex) { - listener.onFailure(ex); - } + SearchRequest searchRequest = new SearchRequest(anomalyDetector.getIndices().toArray(new String[0])).source(searchSourceBuilder); + client.search(searchRequest, ActionListener.wrap(response -> processRawDataResults(response, latestTime), listener::onFailure)); } private Histogram checkBucketResultErrors(SearchResponse response) { @@ -497,7 +479,7 @@ private Histogram checkBucketResultErrors(SearchResponse response) { ); return null; } - Histogram aggregate = aggs.get("agg"); + Histogram aggregate = aggs.get(AGGREGATION); if (aggregate == null) { listener.onFailure(new IllegalArgumentException("Failed to find valid aggregation result")); return null; @@ -548,6 +530,9 @@ private void processDataFilterResults(SearchResponse response, long latestTime) ValidationAspect.MODEL ) ); + // blocks below are executed if data is dense enough with filter query applied. + // If HCAD then category fields will be added to bucket aggregation to see if they + // are the root cause of the issues and if not the feature queries will be checked for sparsity } else if (anomalyDetector.isMultientityDetector()) { getTopEntityForCategoryField(latestTime); } else { @@ -571,17 +556,6 @@ private void getTopEntityForCategoryField(long latestTime) { } private void checkCategoryFieldSparsity(Map topEntity, long latestTime) { - if (topEntity.isEmpty()) { - listener - .onFailure( - new ADValidationException( - CommonErrorMessages.CATEGORY_FIELD_TOO_SPARSE, - DetectorValidationIssueType.CATEGORY, - ValidationAspect.MODEL - ) - ); - return; - } BoolQueryBuilder query = QueryBuilders.boolQuery().filter(anomalyDetector.getFilterQuery()); for (Map.Entry entry : topEntity.entrySet()) { query.filter(QueryBuilders.termQuery(entry.getKey(), entry.getValue())); @@ -625,7 +599,7 @@ private void checkFeatureQuery(long latestTime) throws IOException { new ADValidationException( exception.getMessage(), DetectorValidationIssueType.FEATURE_ATTRIBUTES, - ValidationAspect.DETECTOR + ValidationAspect.MODEL ) ); }); @@ -667,14 +641,8 @@ private void checkFeatureQuery(long latestTime) throws IOException { .onResponse(new MergeableList<>(new ArrayList<>(Collections.singletonList(new double[] { fullBucketRate })))); } }, e -> { - String errorMessage; - if (isExceptionCausedByInvalidQuery(e)) { - errorMessage = CommonErrorMessages.FEATURE_WITH_INVALID_QUERY_MSG + feature.getName(); - } else { - errorMessage = CommonErrorMessages.UNKNOWN_SEARCH_QUERY_EXCEPTION_MSG + feature.getName(); - } - logger.error(errorMessage, e); - multiFeatureQueriesResponseListener.onFailure(new OpenSearchStatusException(errorMessage, RestStatus.BAD_REQUEST, e)); + logger.error("failed getting results with feature query applied", e); + multiFeatureQueriesResponseListener.onFailure(e); })); } } diff --git a/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java b/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java index 2e6ee5aab..7465485b5 100644 --- a/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java +++ b/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java @@ -26,7 +26,6 @@ import java.util.Locale; import org.junit.Before; -import org.junit.Ignore; import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.Mockito; @@ -126,7 +125,6 @@ public void setUp() throws Exception { } @SuppressWarnings("unchecked") - @Ignore public void testValidateMoreThanThousandSingleEntityDetectorLimit() throws IOException { SearchResponse mockResponse = mock(SearchResponse.class); int totalHits = maxSingleEntityAnomalyDetectors + 1; @@ -177,7 +175,6 @@ public void testValidateMoreThanThousandSingleEntityDetectorLimit() throws IOExc } @SuppressWarnings("unchecked") - @Ignore public void testValidateMoreThanTenMultiEntityDetectorsLimit() throws IOException { SearchResponse mockResponse = mock(SearchResponse.class); From f169fdb85e9cc6ceba1da9378305ab2a8b54c20b Mon Sep 17 00:00:00 2001 From: Amit Galitzky Date: Tue, 1 Mar 2022 23:51:23 +0000 Subject: [PATCH 04/13] removed unecessary logging Signed-off-by: Amit Galitzky --- .../ad/rest/handler/AbstractAnomalyDetectorActionHandler.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/main/java/org/opensearch/ad/rest/handler/AbstractAnomalyDetectorActionHandler.java b/src/main/java/org/opensearch/ad/rest/handler/AbstractAnomalyDetectorActionHandler.java index 6ce8c8371..144c73545 100644 --- a/src/main/java/org/opensearch/ad/rest/handler/AbstractAnomalyDetectorActionHandler.java +++ b/src/main/java/org/opensearch/ad/rest/handler/AbstractAnomalyDetectorActionHandler.java @@ -351,10 +351,6 @@ protected void validateTimeField(boolean indexingDryRun) { if (type instanceof Map) { foundField = true; Map metadataMap = (Map) type; - System.out.println("metadata"); - for (Map.Entry entry : metadataMap.entrySet()) { - System.out.println(entry.getKey() + ":" + entry.getValue().toString()); - } String typeName = (String) metadataMap.get(CommonName.TYPE); if (!typeName.equals(CommonName.DATE_TYPE)) { listener From 628edf0dda7dfebddb27d59f1e24a59b3e70929f Mon Sep 17 00:00:00 2001 From: Amit Galitzky Date: Wed, 2 Mar 2022 19:27:17 +0000 Subject: [PATCH 05/13] combined all feature validations into one query, added ability to try lower intervals Signed-off-by: Amit Galitzky --- .../ad/constant/CommonErrorMessages.java | 3 +- .../handler/ModelValidationActionHandler.java | 155 ++++++++++-------- .../ad/settings/AnomalyDetectorSettings.java | 7 +- ...alidateAnomalyDetectorTransportAction.java | 9 +- 4 files changed, 97 insertions(+), 77 deletions(-) diff --git a/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java b/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java index 8ce694ed3..83f9b80a8 100644 --- a/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java +++ b/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java @@ -115,7 +115,8 @@ public static String getTooManyCategoricalFieldErr(int limit) { public static String FILTER_QUERY_TOO_SPARSE = "Data is too sparse after data filter is applied. Consider changing the data filter"; public static String CATEGORY_FIELD_TOO_SPARSE = "Data is most likely too sparse with the given category fields."; public static String CATEGORY_FIELD_NO_DATA = "No entity was found with the given categorical fields."; - public static String FEATURE_QUERY_TOO_SPARSE = "Given data is most likely too sparse when given feature query is applied: "; + public static String FEATURE_QUERY_TOO_SPARSE = + "Given data is most likely too sparse when given feature queries are applied. Consider revising feature queries."; // Modifying message for FEATURE below may break the parseADValidationException method of ValidateAnomalyDetectorTransportAction public static final String FEATURE_INVALID_MSG_PREFIX = "Feature has an invalid query"; diff --git a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java index 83141f1a4..055477472 100644 --- a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java +++ b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java @@ -7,8 +7,10 @@ import static org.opensearch.ad.settings.AnomalyDetectorSettings.CONFIG_BUCKET_MINIMUM_SUCCESS_RATE; import static org.opensearch.ad.settings.AnomalyDetectorSettings.INTERVAL_BUCKET_MINIMUM_SUCCESS_RATE; -import static org.opensearch.ad.settings.AnomalyDetectorSettings.INTERVAL_RECOMMENDATION_MULTIPLIER; +import static org.opensearch.ad.settings.AnomalyDetectorSettings.INTERVAL_RECOMMENDATION_DECREASING_MULTIPLIER; +import static org.opensearch.ad.settings.AnomalyDetectorSettings.INTERVAL_RECOMMENDATION_INCREASING_MULTIPLIER; import static org.opensearch.ad.settings.AnomalyDetectorSettings.MAX_INTERVAL_REC_LENGTH_IN_MINUTES; +import static org.opensearch.ad.settings.AnomalyDetectorSettings.MAX_TIMES_DECREASING_INTERVAL; import static org.opensearch.ad.settings.AnomalyDetectorSettings.TOP_VALIDATE_TIMEOUT_IN_MILLIS; import java.io.IOException; @@ -16,12 +18,10 @@ import java.time.Duration; import java.time.Instant; import java.time.temporal.ChronoUnit; -import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; import java.util.HashMap; import java.util.List; -import java.util.Locale; import java.util.Map; import java.util.Optional; import java.util.concurrent.TimeUnit; @@ -33,20 +33,16 @@ import org.opensearch.action.search.SearchRequest; import org.opensearch.action.search.SearchResponse; import org.opensearch.ad.common.exception.ADValidationException; -import org.opensearch.ad.common.exception.AnomalyDetectionException; import org.opensearch.ad.common.exception.EndRunException; import org.opensearch.ad.constant.CommonErrorMessages; import org.opensearch.ad.feature.SearchFeatureDao; import org.opensearch.ad.model.AnomalyDetector; import org.opensearch.ad.model.DetectorValidationIssueType; -import org.opensearch.ad.model.Feature; import org.opensearch.ad.model.IntervalTimeConfiguration; -import org.opensearch.ad.model.MergeableList; import org.opensearch.ad.model.TimeConfiguration; import org.opensearch.ad.model.ValidationAspect; import org.opensearch.ad.settings.AnomalyDetectorSettings; import org.opensearch.ad.transport.ValidateAnomalyDetectorResponse; -import org.opensearch.ad.util.MultiResponsesDelegateActionListener; import org.opensearch.ad.util.ParseUtils; import org.opensearch.client.Client; import org.opensearch.cluster.service.ClusterService; @@ -297,7 +293,9 @@ private void getBucketAggregates( searchRequest.source(), (IntervalTimeConfiguration) anomalyDetector.getDetectionInterval(), clock.millis() + TOP_VALIDATE_TIMEOUT_IN_MILLIS, - latestTime + latestTime, + false, + MAX_TIMES_DECREASING_INTERVAL ) ); } @@ -324,19 +322,25 @@ class DetectorIntervalRecommendationListener implements ActionListener intervalListener, SearchSourceBuilder searchSourceBuilder, IntervalTimeConfiguration detectorInterval, long expirationEpochMs, - long latestTime + long latestTime, + boolean decreasingInterval, + int numTimesDecreasing ) { this.intervalListener = intervalListener; this.searchSourceBuilder = searchSourceBuilder; this.detectorInterval = detectorInterval; this.expirationEpochMs = expirationEpochMs; this.latestTime = latestTime; + this.decreasingInterval = decreasingInterval; + this.numTimesDecreasing = numTimesDecreasing; } private AggregationBuilder getNewAggregationBuilder(long newInterval) { @@ -364,21 +368,41 @@ public void onResponse(SearchResponse response) { return; } + long newInterval; + if (decreasingInterval) { + newInterval = (long) Math + .ceil(convertIntervalToMinutes(detectorInterval) * INTERVAL_RECOMMENDATION_DECREASING_MULTIPLIER); + } else { + newInterval = (long) Math + .ceil(convertIntervalToMinutes(detectorInterval) * INTERVAL_RECOMMENDATION_INCREASING_MULTIPLIER); + } double fullBucketRate = processBucketAggregationResults(aggregate); - long newInterval = (long) Math.ceil(convertIntervalToMinutes(detectorInterval) * INTERVAL_RECOMMENDATION_MULTIPLIER); // If rate is above success minimum then return interval suggestion. if (fullBucketRate > INTERVAL_BUCKET_MINIMUM_SUCCESS_RATE) { intervalListener.onResponse(this.detectorInterval); } else if (expirationEpochMs < clock.millis()) { listener .onFailure( - new AnomalyDetectionException( - "Timed out getting interval recommendation. Please continue with detector creation." + new ADValidationException( + CommonErrorMessages.MODEL_VALIDATION_FAILED_UNEXPECTEDLY, + DetectorValidationIssueType.MODEL_VALIDATION_ISSUE, + ValidationAspect.MODEL ) ); logger.info("Timed out getting interval recommendation"); - } else if (newInterval < MAX_INTERVAL_REC_LENGTH_IN_MINUTES) { - this.detectorInterval = new IntervalTimeConfiguration(newInterval, ChronoUnit.MINUTES); + // keep trying higher intervals as new interval is below max, and we aren't decreasing yet + } else if (newInterval < MAX_INTERVAL_REC_LENGTH_IN_MINUTES && !decreasingInterval) { + searchWithDifferentInterval(newInterval); + // The below block is executed only the first time when new interval is above max and + // we aren't decreasing yet, at this point + } else if (newInterval >= MAX_INTERVAL_REC_LENGTH_IN_MINUTES && !decreasingInterval) { + IntervalTimeConfiguration givenInterval = (IntervalTimeConfiguration) anomalyDetector.getDetectionInterval(); + this.detectorInterval = new IntervalTimeConfiguration( + (long) Math.ceil(convertIntervalToMinutes(givenInterval) * INTERVAL_RECOMMENDATION_DECREASING_MULTIPLIER), + ChronoUnit.MINUTES + ); + this.decreasingInterval = true; + this.numTimesDecreasing -= 1; // Searching again using an updated interval SearchSourceBuilder updatedSearchSourceBuilder = getSearchSourceBuilder( searchSourceBuilder.query(), @@ -391,7 +415,13 @@ public void onResponse(SearchResponse response) { .source(updatedSearchSourceBuilder), this ); - // this case means all intervals up to max interval recommendation length have been tried + // In this case decreasingInterval has to be true already, so we will stop + // when the next new interval is below or equal to 0, or we have decreased up to max times + } else if (numTimesDecreasing >= 0 || newInterval <= 0) { + this.numTimesDecreasing -= 1; + searchWithDifferentInterval(newInterval); + // this case means all intervals up to max interval recommendation length and down to either + // 0 or until we tried 10 lower intervals than the one given have been tried // which further means the next step is to go through A/B validation checks } else { intervalListener.onResponse(null); @@ -402,6 +432,20 @@ public void onResponse(SearchResponse response) { } } + private void searchWithDifferentInterval(long newIntervalValue) { + this.detectorInterval = new IntervalTimeConfiguration(newIntervalValue, ChronoUnit.MINUTES); + // Searching again using an updated interval + SearchSourceBuilder updatedSearchSourceBuilder = getSearchSourceBuilder( + searchSourceBuilder.query(), + getNewAggregationBuilder(newIntervalValue) + ); + client + .search( + new SearchRequest().indices(anomalyDetector.getIndices().toArray(new String[0])).source(updatedSearchSourceBuilder), + this + ); + } + @Override public void onFailure(Exception e) { logger.error("Failed to recommend new interval", e); @@ -591,60 +635,37 @@ private void processTopEntityResults(SearchResponse response, long latestTime) { } } - private void checkFeatureQuery(long latestTime) throws IOException { - ActionListener> validateFeatureQueriesListener = ActionListener - .wrap(response -> { windowDelayRecommendation(latestTime); }, exception -> { - listener - .onFailure( - new ADValidationException( - exception.getMessage(), - DetectorValidationIssueType.FEATURE_ATTRIBUTES, - ValidationAspect.MODEL - ) - ); - }); - MultiResponsesDelegateActionListener> multiFeatureQueriesResponseListener = - new MultiResponsesDelegateActionListener<>( - validateFeatureQueriesListener, - anomalyDetector.getFeatureAttributes().size(), - String.format(Locale.ROOT, CommonErrorMessages.VALIDATION_FEATURE_FAILURE, anomalyDetector.getName()), - false - ); + private void processFeatureQuery(SearchResponse response, long latestTime) { + Histogram aggregate = checkBucketResultErrors(response); + if (aggregate == null) { + return; + } - for (Feature feature : anomalyDetector.getFeatureAttributes()) { - AggregationBuilder aggregation = getBucketAggregation(latestTime); - BoolQueryBuilder query = QueryBuilders.boolQuery().filter(anomalyDetector.getFilterQuery()); - List featureFields = ParseUtils.getFieldNamesForFeature(feature, xContentRegistry); - for (String featureField : featureFields) { - query.filter(QueryBuilders.existsQuery(featureField)); - } - SearchSourceBuilder searchSourceBuilder = getSearchSourceBuilder(query, aggregation); - SearchRequest searchRequest = new SearchRequest(anomalyDetector.getIndices().toArray(new String[0])) - .source(searchSourceBuilder); - client.search(searchRequest, ActionListener.wrap(response -> { - Histogram aggregate = checkBucketResultErrors(response); - if (aggregate == null) { - return; - } - double fullBucketRate = processBucketAggregationResults(aggregate); - if (fullBucketRate < CONFIG_BUCKET_MINIMUM_SUCCESS_RATE) { - multiFeatureQueriesResponseListener - .onFailure( - new ADValidationException( - CommonErrorMessages.FEATURE_QUERY_TOO_SPARSE + feature.getName(), - DetectorValidationIssueType.FEATURE_ATTRIBUTES, - ValidationAspect.MODEL - ) - ); - } else { - multiFeatureQueriesResponseListener - .onResponse(new MergeableList<>(new ArrayList<>(Collections.singletonList(new double[] { fullBucketRate })))); - } - }, e -> { - logger.error("failed getting results with feature query applied", e); - multiFeatureQueriesResponseListener.onFailure(e); - })); + double fullBucketRate = processBucketAggregationResults(aggregate); + if (fullBucketRate < CONFIG_BUCKET_MINIMUM_SUCCESS_RATE) { + listener + .onFailure( + new ADValidationException( + CommonErrorMessages.FEATURE_QUERY_TOO_SPARSE, + DetectorValidationIssueType.FEATURE_ATTRIBUTES, + ValidationAspect.MODEL + ) + ); + return; } + windowDelayRecommendation(latestTime); + } + + private void checkFeatureQuery(long latestTime) throws IOException { + List featureFields = ParseUtils.getFeatureFieldNames(anomalyDetector, xContentRegistry); + AggregationBuilder aggregation = getBucketAggregation(latestTime); + BoolQueryBuilder query = QueryBuilders.boolQuery().filter(anomalyDetector.getFilterQuery()); + for (String featureField : featureFields) { + query.filter(QueryBuilders.existsQuery(featureField)); + } + SearchSourceBuilder searchSourceBuilder = getSearchSourceBuilder(query, aggregation); + SearchRequest searchRequest = new SearchRequest(anomalyDetector.getIndices().toArray(new String[0])).source(searchSourceBuilder); + client.search(searchRequest, ActionListener.wrap(response -> processFeatureQuery(response, latestTime), listener::onFailure)); } private void windowDelayRecommendation(long latestTime) { diff --git a/src/main/java/org/opensearch/ad/settings/AnomalyDetectorSettings.java b/src/main/java/org/opensearch/ad/settings/AnomalyDetectorSettings.java index fe6396b2c..e24c6be26 100644 --- a/src/main/java/org/opensearch/ad/settings/AnomalyDetectorSettings.java +++ b/src/main/java/org/opensearch/ad/settings/AnomalyDetectorSettings.java @@ -795,7 +795,12 @@ private AnomalyDetectorSettings() {} // ====================================== public static final long TOP_VALIDATE_TIMEOUT_IN_MILLIS = 60_000; public static final long MAX_INTERVAL_REC_LENGTH_IN_MINUTES = 120L; - public static final double INTERVAL_RECOMMENDATION_MULTIPLIER = 1.2; + public static final double INTERVAL_RECOMMENDATION_INCREASING_MULTIPLIER = 1.2; + public static final double INTERVAL_RECOMMENDATION_DECREASING_MULTIPLIER = 0.8; public static final double INTERVAL_BUCKET_MINIMUM_SUCCESS_RATE = 0.75; public static final double CONFIG_BUCKET_MINIMUM_SUCCESS_RATE = 0.25; + // This value is set to decrease the number of times we decrease the interval when recommending a new one + // The reason we need a max is because user could give an arbitrarly large interval where we don't know even + // with multiplying the interval down how many intervals will be tried. + public static final int MAX_TIMES_DECREASING_INTERVAL = 10; } diff --git a/src/main/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportAction.java b/src/main/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportAction.java index 381054204..eebf626ec 100644 --- a/src/main/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportAction.java +++ b/src/main/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportAction.java @@ -172,8 +172,7 @@ protected DetectorValidationIssue parseADValidationException(ADValidationExcepti String originalErrorMessage = exception.getMessage(); String errorMessage = ""; Map subIssues = null; - IntervalTimeConfiguration intervalSuggestion = null; - + IntervalTimeConfiguration intervalSuggestion = exception.getIntervalSuggestion(); switch (exception.getType()) { case FEATURE_ATTRIBUTES: int firstLeftBracketIndex = originalErrorMessage.indexOf("["); @@ -191,16 +190,10 @@ protected DetectorValidationIssue parseADValidationException(ADValidationExcepti case NAME: case CATEGORY: case DETECTION_INTERVAL: - if (exception.getAspect().equals(ValidationAspect.MODEL)) { - intervalSuggestion = exception.getIntervalSuggestion(); - } case FILTER_QUERY: case TIMEFIELD_FIELD: case SHINGLE_SIZE_FIELD: case WINDOW_DELAY: - if (exception.getAspect().equals(ValidationAspect.MODEL)) { - intervalSuggestion = exception.getIntervalSuggestion(); - } case RESULT_INDEX: case GENERAL_SETTINGS: case MODEL_VALIDATION_ISSUE: From 7c33b24daf72cd7842e0ca4831d598750b31cfcf Mon Sep 17 00:00:00 2001 From: Amit Galitzky Date: Fri, 4 Mar 2022 17:41:42 +0000 Subject: [PATCH 06/13] addressed comments, changed response wording, top entity only from now to past Signed-off-by: Amit Galitzky --- .../ad/constant/CommonErrorMessages.java | 18 +- .../ad/feature/SearchFeatureDao.java | 18 ++ .../opensearch/ad/model/AnomalyDetector.java | 3 +- .../ad/model/DetectorValidationIssueType.java | 5 +- .../ad/model/IntervalTimeConfiguration.java | 7 + .../handler/ModelValidationActionHandler.java | 258 ++++++++++-------- .../ad/settings/AnomalyDetectorSettings.java | 4 +- ...alidateAnomalyDetectorTransportAction.java | 3 +- 8 files changed, 197 insertions(+), 119 deletions(-) diff --git a/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java b/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java index 83f9b80a8..17c492d6c 100644 --- a/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java +++ b/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java @@ -80,7 +80,7 @@ public static String getTooManyCategoricalFieldErr(int limit) { "Valid characters for detector name are a-z, A-Z, 0-9, -(hyphen), _(underscore) and .(period)"; public static String DUPLICATE_FEATURE_AGGREGATION_NAMES = "Detector has duplicate feature aggregation query names: "; public static String INVALID_TIMESTAMP = "Timestamp field: (%s) must be of type date"; - public static String NON_EXISTENT_TIMESTAMP = "Timestamp field: (%s) is not found in index mapping:"; + public static String NON_EXISTENT_TIMESTAMP = "Timestamp field: (%s) is not found in index mapping"; public static String FAIL_TO_GET_DETECTOR = "Fail to get detector"; public static String FAIL_TO_GET_DETECTOR_INFO = "Fail to get detector info"; @@ -106,17 +106,23 @@ public static String getTooManyCategoricalFieldErr(int limit) { + MAX_DETECTOR_NAME_SIZE + " characters."; - public static String WINDOW_DELAY_REC = "We suggest using a window delay value of at least: "; + public static String WINDOW_DELAY_REC = + "Latest seen data point is at least %d minutes ago, consider changing window delay to at least %d minutes."; public static String TIME_FIELD_NOT_ENOUGH_HISTORICAL_DATA = "There isn't enough historical data found with current timefield selected."; - public static String DETECTOR_INTERVAL_REC = "Suggested detector interval: "; - public static String RAW_DATA_TOO_SPARSE = "Given index data is potentially too sparse for model training."; + public static String DETECTOR_INTERVAL_REC = + "The selected detector interval might collect sparse data. Consider increasing interval range too: "; + public static String RAW_DATA_TOO_SPARSE = + "Given index data is potentially too sparse for model training. Consider changing interval length or ingesting more data"; public static String MODEL_VALIDATION_FAILED_UNEXPECTEDLY = "Model validation experienced issues completing."; public static String FILTER_QUERY_TOO_SPARSE = "Data is too sparse after data filter is applied. Consider changing the data filter"; - public static String CATEGORY_FIELD_TOO_SPARSE = "Data is most likely too sparse with the given category fields."; - public static String CATEGORY_FIELD_NO_DATA = "No entity was found with the given categorical fields."; + public static String CATEGORY_FIELD_TOO_SPARSE = + "Data is most likely too sparse with the given category fields. Consider revising category field/s or ingesting more data "; + public static String CATEGORY_FIELD_NO_DATA = + "No entity was found with the given categorical fields. Consider revising category field/s or ingesting more data"; public static String FEATURE_QUERY_TOO_SPARSE = "Given data is most likely too sparse when given feature queries are applied. Consider revising feature queries."; + public static String TIMEOUT_ON_INTERVAL_REC = "Timed out getting interval recommendation"; // Modifying message for FEATURE below may break the parseADValidationException method of ValidateAnomalyDetectorTransportAction public static final String FEATURE_INVALID_MSG_PREFIX = "Feature has an invalid query"; diff --git a/src/main/java/org/opensearch/ad/feature/SearchFeatureDao.java b/src/main/java/org/opensearch/ad/feature/SearchFeatureDao.java index 599d6aacd..0d044c3a1 100644 --- a/src/main/java/org/opensearch/ad/feature/SearchFeatureDao.java +++ b/src/main/java/org/opensearch/ad/feature/SearchFeatureDao.java @@ -496,6 +496,24 @@ public void getEntityMinDataTime(AnomalyDetector detector, Entity entity, Action ); } + /** + * Get the entity's earliest timestamps + * @param detector detector config + * @param listener listener to return back the requested timestamps + */ + public void getMinDataTime(AnomalyDetector detector, ActionListener> listener) { + SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder() + .aggregation(AggregationBuilders.min(AGG_NAME_MIN).field(detector.getTimeField())) + .trackTotalHits(false) + .size(0); + SearchRequest searchRequest = new SearchRequest().indices(detector.getIndices().toArray(new String[0])).source(searchSourceBuilder); + client + .search( + searchRequest, + ActionListener.wrap(response -> { listener.onResponse(parseMinDataTime(response)); }, listener::onFailure) + ); + } + private Optional parseMinDataTime(SearchResponse searchResponse) { Optional> mapOptional = Optional .ofNullable(searchResponse) diff --git a/src/main/java/org/opensearch/ad/model/AnomalyDetector.java b/src/main/java/org/opensearch/ad/model/AnomalyDetector.java index 4b5e105b7..2a8eea4f3 100644 --- a/src/main/java/org/opensearch/ad/model/AnomalyDetector.java +++ b/src/main/java/org/opensearch/ad/model/AnomalyDetector.java @@ -94,7 +94,8 @@ public class AnomalyDetector implements Writeable, ToXContentObject { public static final String USER_FIELD = "user"; public static final String DETECTOR_TYPE_FIELD = "detector_type"; public static final String RESULT_INDEX_FIELD = "result_index"; - public static final String MODEL_VALIDATION_ISSUE = "aggregation_issue"; + public static final String AGGREGATION = "aggregation_issue"; + public static final String TIMEOUT = "timeout"; @Deprecated public static final String DETECTION_DATE_RANGE_FIELD = "detection_date_range"; diff --git a/src/main/java/org/opensearch/ad/model/DetectorValidationIssueType.java b/src/main/java/org/opensearch/ad/model/DetectorValidationIssueType.java index 20d30267d..717ab5cde 100644 --- a/src/main/java/org/opensearch/ad/model/DetectorValidationIssueType.java +++ b/src/main/java/org/opensearch/ad/model/DetectorValidationIssueType.java @@ -25,8 +25,9 @@ public enum DetectorValidationIssueType implements Name { WINDOW_DELAY(AnomalyDetector.WINDOW_DELAY_FIELD), GENERAL_SETTINGS(AnomalyDetector.GENERAL_SETTINGS), RESULT_INDEX(AnomalyDetector.RESULT_INDEX_FIELD), - MODEL_VALIDATION_ISSUE(AnomalyDetector.MODEL_VALIDATION_ISSUE); // this is a unique case where aggregation failed but - // don't want to throw exception + TIMEOUT(AnomalyDetector.TIMEOUT), + AGGREGATION(AnomalyDetector.AGGREGATION); // this is a unique case where aggregation failed but + // don't want to throw exception private String name; diff --git a/src/main/java/org/opensearch/ad/model/IntervalTimeConfiguration.java b/src/main/java/org/opensearch/ad/model/IntervalTimeConfiguration.java index f1fcab5f9..aaa24f720 100644 --- a/src/main/java/org/opensearch/ad/model/IntervalTimeConfiguration.java +++ b/src/main/java/org/opensearch/ad/model/IntervalTimeConfiguration.java @@ -61,6 +61,13 @@ public static IntervalTimeConfiguration readFrom(StreamInput input) throws IOExc return new IntervalTimeConfiguration(input); } + public static long getIntervalInMinute(IntervalTimeConfiguration interval) { + if (interval.getUnit() == ChronoUnit.SECONDS) { + return interval.getInterval() / 60; + } + return interval.getInterval(); + } + @Override public void writeTo(StreamOutput out) throws IOException { out.writeLong(this.interval); diff --git a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java index 055477472..9e053292e 100644 --- a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java +++ b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java @@ -22,6 +22,7 @@ import java.util.Comparator; import java.util.HashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Optional; import java.util.concurrent.TimeUnit; @@ -51,6 +52,7 @@ import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.index.query.QueryBuilder; import org.opensearch.index.query.QueryBuilders; +import org.opensearch.index.query.RangeQueryBuilder; import org.opensearch.search.aggregations.AggregationBuilder; import org.opensearch.search.aggregations.AggregationBuilders; import org.opensearch.search.aggregations.Aggregations; @@ -147,69 +149,93 @@ public void checkIfMultiEntityDetector() { // For multi-category HCADs we use a composite aggregation to find the top fields for the entity // with the highest doc count. private void getTopEntity(ActionListener> topEntityListener) { - AggregationBuilder bucketAggs; - Map topKeys = new HashMap<>(); - if (anomalyDetector.getCategoryField().size() == 1) { - bucketAggs = AggregationBuilders - .terms(AGG_NAME_TOP) - .field(anomalyDetector.getCategoryField().get(0)) - .order(BucketOrder.count(true)); - } else { - bucketAggs = AggregationBuilders - .composite( - AGG_NAME_TOP, - anomalyDetector - .getCategoryField() - .stream() - .map(f -> new TermsValuesSourceBuilder(f).field(f)) - .collect(Collectors.toList()) - ) - .size(1000) - .subAggregation( - PipelineAggregatorBuilders - .bucketSort("bucketSort", Collections.singletonList(new FieldSortBuilder("_count").order(SortOrder.DESC))) - .size(1) - ); - } + ActionListener> minTimeListener = ActionListener.wrap(earliest -> { + if (earliest.isPresent() && earliest.get() > 0) { + // only look at top entities from the earliest time point to now so future data isn't used + RangeQueryBuilder rangeQuery = new RangeQueryBuilder(anomalyDetector.getTimeField()) + .from(earliest.get()) + .to(Instant.now().toEpochMilli()); + AggregationBuilder bucketAggs; + Map topKeys = new HashMap<>(); + if (anomalyDetector.getCategoryField().size() == 1) { + bucketAggs = AggregationBuilders + .terms(AGG_NAME_TOP) + .field(anomalyDetector.getCategoryField().get(0)) + .order(BucketOrder.count(true)); + } else { + bucketAggs = AggregationBuilders + .composite( + AGG_NAME_TOP, + anomalyDetector + .getCategoryField() + .stream() + .map(f -> new TermsValuesSourceBuilder(f).field(f)) + .collect(Collectors.toList()) + ) + .size(1000) + .subAggregation( + PipelineAggregatorBuilders + .bucketSort("bucketSort", Collections.singletonList(new FieldSortBuilder("_count").order(SortOrder.DESC))) + .size(1) + ); + } - SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder().aggregation(bucketAggs).trackTotalHits(false).size(0); - SearchRequest searchRequest = new SearchRequest() - .indices(anomalyDetector.getIndices().toArray(new String[0])) - .source(searchSourceBuilder); - client.search(searchRequest, ActionListener.wrap(response -> { - Aggregations aggs = response.getAggregations(); - if (aggs == null) { - topEntityListener.onResponse(Collections.emptyMap()); - return; - } - if (anomalyDetector.getCategoryField().size() == 1) { - Terms entities = aggs.get(AGG_NAME_TOP); - Object key = entities - .getBuckets() - .stream() - .max(Comparator.comparingInt(entry -> (int) entry.getDocCount())) - .map(MultiBucketsAggregation.Bucket::getKeyAsString) - .orElse(null); - topKeys.put(anomalyDetector.getCategoryField().get(0), key); - } else { - CompositeAggregation compositeAgg = aggs.get(AGG_NAME_TOP); - topKeys - .putAll( - compositeAgg + SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder() + .query(rangeQuery) + .aggregation(bucketAggs) + .trackTotalHits(false) + .size(0); + SearchRequest searchRequest = new SearchRequest() + .indices(anomalyDetector.getIndices().toArray(new String[0])) + .source(searchSourceBuilder); + client.search(searchRequest, ActionListener.wrap(response -> { + Aggregations aggs = response.getAggregations(); + if (aggs == null) { + topEntityListener.onResponse(Collections.emptyMap()); + return; + } + if (anomalyDetector.getCategoryField().size() == 1) { + Terms entities = aggs.get(AGG_NAME_TOP); + Object key = entities .getBuckets() .stream() - .flatMap(bucket -> bucket.getKey().entrySet().stream()) // this would create a flattened stream of map entries - .collect(Collectors.toMap(e -> e.getKey(), e -> e.getValue())) - ); - } - for (Map.Entry entry : topKeys.entrySet()) { - if (entry.getValue() == null) { - topEntityListener.onResponse(Collections.emptyMap()); - return; - } + .max(Comparator.comparingInt(entry -> (int) entry.getDocCount())) + .map(MultiBucketsAggregation.Bucket::getKeyAsString) + .orElse(null); + topKeys.put(anomalyDetector.getCategoryField().get(0), key); + } else { + CompositeAggregation compositeAgg = aggs.get(AGG_NAME_TOP); + topKeys + .putAll( + compositeAgg + .getBuckets() + .stream() + .flatMap(bucket -> bucket.getKey().entrySet().stream()) // this would create a flattened stream of map + // entries + .collect(Collectors.toMap(e -> e.getKey(), e -> e.getValue())) + ); + } + for (Map.Entry entry : topKeys.entrySet()) { + if (entry.getValue() == null) { + topEntityListener.onResponse(Collections.emptyMap()); + return; + } + } + topEntityListener.onResponse(topKeys); + }, topEntityListener::onFailure)); } - topEntityListener.onResponse(topKeys); - }, topEntityListener::onFailure)); + }, exception -> { + logger.error("Failed to create search request for minimum data point", exception); + listener + .onFailure( + new ADValidationException( + CommonErrorMessages.TIME_FIELD_NOT_ENOUGH_HISTORICAL_DATA, + DetectorValidationIssueType.TIMEFIELD_FIELD, + ValidationAspect.MODEL + ) + ); + }); + searchFeatureDao.getMinDataTime(anomalyDetector, minTimeListener); } private void getLatestDateForValidation(Map topEntity) { @@ -252,7 +278,10 @@ private void getBucketAggregates( Map topEntity ) throws IOException { List featureFields = ParseUtils.getFeatureFieldNames(anomalyDetector, xContentRegistry); - AggregationBuilder aggregation = getBucketAggregation(latestTime); + AggregationBuilder aggregation = getBucketAggregation( + latestTime, + (IntervalTimeConfiguration) anomalyDetector.getDetectionInterval() + ); BoolQueryBuilder query = QueryBuilders.boolQuery().filter(anomalyDetector.getFilterQuery()); if (anomalyDetector.isMultientityDetector()) { if (topEntity.isEmpty()) { @@ -343,23 +372,6 @@ class DetectorIntervalRecommendationListener implements ActionListener= MAX_INTERVAL_REC_LENGTH_IN_MINUTES && !decreasingInterval) { + } else if (newIntervalMinute >= MAX_INTERVAL_REC_LENGTH_IN_MINUTES && !decreasingInterval) { IntervalTimeConfiguration givenInterval = (IntervalTimeConfiguration) anomalyDetector.getDetectionInterval(); this.detectorInterval = new IntervalTimeConfiguration( - (long) Math.ceil(convertIntervalToMinutes(givenInterval) * INTERVAL_RECOMMENDATION_DECREASING_MULTIPLIER), + (long) Math + .floor( + IntervalTimeConfiguration.getIntervalInMinute(givenInterval) * INTERVAL_RECOMMENDATION_DECREASING_MULTIPLIER + ), ChronoUnit.MINUTES ); + if (detectorInterval.getInterval() <= 0) { + intervalListener.onResponse(null); + return; + } this.decreasingInterval = true; this.numTimesDecreasing -= 1; // Searching again using an updated interval SearchSourceBuilder updatedSearchSourceBuilder = getSearchSourceBuilder( searchSourceBuilder.query(), - getNewAggregationBuilder(newInterval) + getBucketAggregation(this.latestTime, new IntervalTimeConfiguration(newIntervalMinute, ChronoUnit.MINUTES)) ); client .search( @@ -417,9 +440,9 @@ public void onResponse(SearchResponse response) { ); // In this case decreasingInterval has to be true already, so we will stop // when the next new interval is below or equal to 0, or we have decreased up to max times - } else if (numTimesDecreasing >= 0 || newInterval <= 0) { + } else if (numTimesDecreasing >= 0 && newIntervalMinute > 0) { this.numTimesDecreasing -= 1; - searchWithDifferentInterval(newInterval); + searchWithDifferentInterval(newIntervalMinute); // this case means all intervals up to max interval recommendation length and down to either // 0 or until we tried 10 lower intervals than the one given have been tried // which further means the next step is to go through A/B validation checks @@ -432,12 +455,12 @@ public void onResponse(SearchResponse response) { } } - private void searchWithDifferentInterval(long newIntervalValue) { - this.detectorInterval = new IntervalTimeConfiguration(newIntervalValue, ChronoUnit.MINUTES); + private void searchWithDifferentInterval(long newIntervalMinuteValue) { + this.detectorInterval = new IntervalTimeConfiguration(newIntervalMinuteValue, ChronoUnit.MINUTES); // Searching again using an updated interval SearchSourceBuilder updatedSearchSourceBuilder = getSearchSourceBuilder( searchSourceBuilder.query(), - getNewAggregationBuilder(newIntervalValue) + getBucketAggregation(this.latestTime, new IntervalTimeConfiguration(newIntervalMinuteValue, ChronoUnit.MINUTES)) ); client .search( @@ -453,7 +476,7 @@ public void onFailure(Exception e) { .onFailure( new ADValidationException( CommonErrorMessages.MODEL_VALIDATION_FAILED_UNEXPECTEDLY, - DetectorValidationIssueType.MODEL_VALIDATION_ISSUE, + DetectorValidationIssueType.AGGREGATION, ValidationAspect.MODEL ) ); @@ -486,13 +509,13 @@ private void processIntervalRecommendation(IntervalTimeConfiguration interval, l } } - private AggregationBuilder getBucketAggregation(long latestTime) { + private AggregationBuilder getBucketAggregation(long latestTime, IntervalTimeConfiguration detectorInterval) { return AggregationBuilders .dateHistogram(AGGREGATION) .field(anomalyDetector.getTimeField()) - .minDocCount(0) - .hardBounds(getTimeRangeBounds(latestTime, (IntervalTimeConfiguration) anomalyDetector.getDetectionInterval())) - .fixedInterval(DateHistogramInterval.minutes((int) anomalyDetector.getDetectorIntervalInMinutes())); + .minDocCount(1) + .hardBounds(getTimeRangeBounds(latestTime, detectorInterval)) + .fixedInterval(DateHistogramInterval.minutes((int) IntervalTimeConfiguration.getIntervalInMinute(detectorInterval))); } private SearchSourceBuilder getSearchSourceBuilder(QueryBuilder query, AggregationBuilder aggregation) { @@ -500,7 +523,10 @@ private SearchSourceBuilder getSearchSourceBuilder(QueryBuilder query, Aggregati } private void checkRawDataSparsity(long latestTime) { - AggregationBuilder aggregation = getBucketAggregation(latestTime); + AggregationBuilder aggregation = getBucketAggregation( + latestTime, + (IntervalTimeConfiguration) anomalyDetector.getDetectionInterval() + ); SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder().aggregation(aggregation).size(0).timeout(requestTimeout); SearchRequest searchRequest = new SearchRequest(anomalyDetector.getIndices().toArray(new String[0])).source(searchSourceBuilder); client.search(searchRequest, ActionListener.wrap(response -> processRawDataResults(response, latestTime), listener::onFailure)); @@ -517,7 +543,7 @@ private Histogram checkBucketResultErrors(SearchResponse response) { .onFailure( new ADValidationException( CommonErrorMessages.MODEL_VALIDATION_FAILED_UNEXPECTEDLY, - DetectorValidationIssueType.MODEL_VALIDATION_ISSUE, + DetectorValidationIssueType.AGGREGATION, ValidationAspect.MODEL ) ); @@ -552,7 +578,10 @@ private void processRawDataResults(SearchResponse response, long latestTime) { } private void checkDataFilterSparsity(long latestTime) { - AggregationBuilder aggregation = getBucketAggregation(latestTime); + AggregationBuilder aggregation = getBucketAggregation( + latestTime, + (IntervalTimeConfiguration) anomalyDetector.getDetectionInterval() + ); BoolQueryBuilder query = QueryBuilders.boolQuery().filter(anomalyDetector.getFilterQuery()); SearchSourceBuilder searchSourceBuilder = getSearchSourceBuilder(query, aggregation); SearchRequest searchRequest = new SearchRequest(anomalyDetector.getIndices().toArray(new String[0])).source(searchSourceBuilder); @@ -604,7 +633,10 @@ private void checkCategoryFieldSparsity(Map topEntity, long late for (Map.Entry entry : topEntity.entrySet()) { query.filter(QueryBuilders.termQuery(entry.getKey(), entry.getValue())); } - AggregationBuilder aggregation = getBucketAggregation(latestTime); + AggregationBuilder aggregation = getBucketAggregation( + latestTime, + (IntervalTimeConfiguration) anomalyDetector.getDetectionInterval() + ); SearchSourceBuilder searchSourceBuilder = getSearchSourceBuilder(query, aggregation); SearchRequest searchRequest = new SearchRequest(anomalyDetector.getIndices().toArray(new String[0])).source(searchSourceBuilder); client.search(searchRequest, ActionListener.wrap(response -> processTopEntityResults(response, latestTime), listener::onFailure)); @@ -658,7 +690,10 @@ private void processFeatureQuery(SearchResponse response, long latestTime) { private void checkFeatureQuery(long latestTime) throws IOException { List featureFields = ParseUtils.getFeatureFieldNames(anomalyDetector, xContentRegistry); - AggregationBuilder aggregation = getBucketAggregation(latestTime); + AggregationBuilder aggregation = getBucketAggregation( + latestTime, + (IntervalTimeConfiguration) anomalyDetector.getDetectionInterval() + ); BoolQueryBuilder query = QueryBuilders.boolQuery().filter(anomalyDetector.getFilterQuery()); for (String featureField : featureFields) { query.filter(QueryBuilders.existsQuery(featureField)); @@ -675,7 +710,7 @@ private void windowDelayRecommendation(long latestTime) { listener .onFailure( new ADValidationException( - CommonErrorMessages.WINDOW_DELAY_REC + minutesSinceLastStamp, + String.format(Locale.ROOT, CommonErrorMessages.WINDOW_DELAY_REC, minutesSinceLastStamp, minutesSinceLastStamp), DetectorValidationIssueType.WINDOW_DELAY, ValidationAspect.MODEL, new IntervalTimeConfiguration(minutesSinceLastStamp, ChronoUnit.MINUTES) @@ -683,7 +718,16 @@ private void windowDelayRecommendation(long latestTime) { ); return; } - listener.onResponse(null); + // This case has been reached if no interval or window delay recommendation was found and no configuration took down bucket + // rate bellow the config bucket rate minimum. + listener + .onFailure( + new ADValidationException( + CommonErrorMessages.RAW_DATA_TOO_SPARSE, + DetectorValidationIssueType.INDICES, + ValidationAspect.MODEL + ) + ); } private LongBounds getTimeRangeBounds(long endMillis, IntervalTimeConfiguration detectorIntervalInMinutes) { diff --git a/src/main/java/org/opensearch/ad/settings/AnomalyDetectorSettings.java b/src/main/java/org/opensearch/ad/settings/AnomalyDetectorSettings.java index e24c6be26..4f609439c 100644 --- a/src/main/java/org/opensearch/ad/settings/AnomalyDetectorSettings.java +++ b/src/main/java/org/opensearch/ad/settings/AnomalyDetectorSettings.java @@ -793,8 +793,8 @@ private AnomalyDetectorSettings() {} // ====================================== // Validate Detector API setting // ====================================== - public static final long TOP_VALIDATE_TIMEOUT_IN_MILLIS = 60_000; - public static final long MAX_INTERVAL_REC_LENGTH_IN_MINUTES = 120L; + public static final long TOP_VALIDATE_TIMEOUT_IN_MILLIS = 10_000; + public static final long MAX_INTERVAL_REC_LENGTH_IN_MINUTES = 60L; public static final double INTERVAL_RECOMMENDATION_INCREASING_MULTIPLIER = 1.2; public static final double INTERVAL_RECOMMENDATION_DECREASING_MULTIPLIER = 0.8; public static final double INTERVAL_BUCKET_MINIMUM_SUCCESS_RATE = 0.75; diff --git a/src/main/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportAction.java b/src/main/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportAction.java index eebf626ec..7a9bd66f3 100644 --- a/src/main/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportAction.java +++ b/src/main/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportAction.java @@ -196,7 +196,8 @@ protected DetectorValidationIssue parseADValidationException(ADValidationExcepti case WINDOW_DELAY: case RESULT_INDEX: case GENERAL_SETTINGS: - case MODEL_VALIDATION_ISSUE: + case AGGREGATION: + case TIMEOUT: case INDICES: errorMessage = originalErrorMessage; break; From 83171b24242b141fc585cf7d450c0dd344cf640e Mon Sep 17 00:00:00 2001 From: Amit Galitzky Date: Mon, 7 Mar 2022 22:35:01 +0000 Subject: [PATCH 07/13] addressed more comments and fixed failing tests Signed-off-by: Amit Galitzky --- .../ad/model/DetectorValidationIssueType.java | 2 +- .../handler/ModelValidationActionHandler.java | 166 ++++++++--------- ...ndexAnomalyDetectorActionHandlerTests.java | 172 +++++++++--------- ...dateAnomalyDetectorActionHandlerTests.java | 115 ++++++++---- .../ad/AnomalyDetectorRestTestCase.java | 2 +- 5 files changed, 250 insertions(+), 207 deletions(-) diff --git a/src/main/java/org/opensearch/ad/model/DetectorValidationIssueType.java b/src/main/java/org/opensearch/ad/model/DetectorValidationIssueType.java index 717ab5cde..fd7975f3b 100644 --- a/src/main/java/org/opensearch/ad/model/DetectorValidationIssueType.java +++ b/src/main/java/org/opensearch/ad/model/DetectorValidationIssueType.java @@ -26,7 +26,7 @@ public enum DetectorValidationIssueType implements Name { GENERAL_SETTINGS(AnomalyDetector.GENERAL_SETTINGS), RESULT_INDEX(AnomalyDetector.RESULT_INDEX_FIELD), TIMEOUT(AnomalyDetector.TIMEOUT), - AGGREGATION(AnomalyDetector.AGGREGATION); // this is a unique case where aggregation failed but + AGGREGATION(AnomalyDetector.AGGREGATION); // this is a unique case where aggregation failed due to an issue in core but // don't want to throw exception private String name; diff --git a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java index 9e053292e..0b6f34711 100644 --- a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java +++ b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java @@ -149,93 +149,81 @@ public void checkIfMultiEntityDetector() { // For multi-category HCADs we use a composite aggregation to find the top fields for the entity // with the highest doc count. private void getTopEntity(ActionListener> topEntityListener) { - ActionListener> minTimeListener = ActionListener.wrap(earliest -> { - if (earliest.isPresent() && earliest.get() > 0) { - // only look at top entities from the earliest time point to now so future data isn't used - RangeQueryBuilder rangeQuery = new RangeQueryBuilder(anomalyDetector.getTimeField()) - .from(earliest.get()) - .to(Instant.now().toEpochMilli()); - AggregationBuilder bucketAggs; - Map topKeys = new HashMap<>(); - if (anomalyDetector.getCategoryField().size() == 1) { - bucketAggs = AggregationBuilders - .terms(AGG_NAME_TOP) - .field(anomalyDetector.getCategoryField().get(0)) - .order(BucketOrder.count(true)); - } else { - bucketAggs = AggregationBuilders - .composite( - AGG_NAME_TOP, - anomalyDetector - .getCategoryField() - .stream() - .map(f -> new TermsValuesSourceBuilder(f).field(f)) - .collect(Collectors.toList()) - ) - .size(1000) - .subAggregation( - PipelineAggregatorBuilders - .bucketSort("bucketSort", Collections.singletonList(new FieldSortBuilder("_count").order(SortOrder.DESC))) - .size(1) - ); - } - - SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder() - .query(rangeQuery) - .aggregation(bucketAggs) - .trackTotalHits(false) - .size(0); - SearchRequest searchRequest = new SearchRequest() - .indices(anomalyDetector.getIndices().toArray(new String[0])) - .source(searchSourceBuilder); - client.search(searchRequest, ActionListener.wrap(response -> { - Aggregations aggs = response.getAggregations(); - if (aggs == null) { - topEntityListener.onResponse(Collections.emptyMap()); - return; - } - if (anomalyDetector.getCategoryField().size() == 1) { - Terms entities = aggs.get(AGG_NAME_TOP); - Object key = entities + // Look at data back to the lower bound given the max interval we recommend or one given + long maxIntervalInMinutes = Math.max(MAX_INTERVAL_REC_LENGTH_IN_MINUTES, anomalyDetector.getDetectorIntervalInMinutes()); + LongBounds timeRangeBounds = getTimeRangeBounds( + Instant.now().toEpochMilli(), + new IntervalTimeConfiguration(maxIntervalInMinutes, ChronoUnit.MINUTES) + ); + RangeQueryBuilder rangeQuery = new RangeQueryBuilder(anomalyDetector.getTimeField()) + .from(timeRangeBounds.getMin()) + .to(timeRangeBounds.getMax()); + AggregationBuilder bucketAggs; + Map topKeys = new HashMap<>(); + if (anomalyDetector.getCategoryField().size() == 1) { + bucketAggs = AggregationBuilders + .terms(AGG_NAME_TOP) + .field(anomalyDetector.getCategoryField().get(0)) + .order(BucketOrder.count(true)); + } else { + bucketAggs = AggregationBuilders + .composite( + AGG_NAME_TOP, + anomalyDetector + .getCategoryField() + .stream() + .map(f -> new TermsValuesSourceBuilder(f).field(f)) + .collect(Collectors.toList()) + ) + .size(1000) + .subAggregation( + PipelineAggregatorBuilders + .bucketSort("bucketSort", Collections.singletonList(new FieldSortBuilder("_count").order(SortOrder.DESC))) + .size(1) + ); + } + SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder() + .query(rangeQuery) + .aggregation(bucketAggs) + .trackTotalHits(false) + .size(0); + SearchRequest searchRequest = new SearchRequest() + .indices(anomalyDetector.getIndices().toArray(new String[0])) + .source(searchSourceBuilder); + client.search(searchRequest, ActionListener.wrap(response -> { + Aggregations aggs = response.getAggregations(); + if (aggs == null) { + topEntityListener.onResponse(Collections.emptyMap()); + return; + } + if (anomalyDetector.getCategoryField().size() == 1) { + Terms entities = aggs.get(AGG_NAME_TOP); + Object key = entities + .getBuckets() + .stream() + .max(Comparator.comparingInt(entry -> (int) entry.getDocCount())) + .map(MultiBucketsAggregation.Bucket::getKeyAsString) + .orElse(null); + topKeys.put(anomalyDetector.getCategoryField().get(0), key); + } else { + CompositeAggregation compositeAgg = aggs.get(AGG_NAME_TOP); + topKeys + .putAll( + compositeAgg .getBuckets() .stream() - .max(Comparator.comparingInt(entry -> (int) entry.getDocCount())) - .map(MultiBucketsAggregation.Bucket::getKeyAsString) - .orElse(null); - topKeys.put(anomalyDetector.getCategoryField().get(0), key); - } else { - CompositeAggregation compositeAgg = aggs.get(AGG_NAME_TOP); - topKeys - .putAll( - compositeAgg - .getBuckets() - .stream() - .flatMap(bucket -> bucket.getKey().entrySet().stream()) // this would create a flattened stream of map - // entries - .collect(Collectors.toMap(e -> e.getKey(), e -> e.getValue())) - ); - } - for (Map.Entry entry : topKeys.entrySet()) { - if (entry.getValue() == null) { - topEntityListener.onResponse(Collections.emptyMap()); - return; - } - } - topEntityListener.onResponse(topKeys); - }, topEntityListener::onFailure)); + .flatMap(bucket -> bucket.getKey().entrySet().stream()) // this would create a flattened stream of map entries + .collect(Collectors.toMap(e -> e.getKey(), e -> e.getValue())) + ); } - }, exception -> { - logger.error("Failed to create search request for minimum data point", exception); - listener - .onFailure( - new ADValidationException( - CommonErrorMessages.TIME_FIELD_NOT_ENOUGH_HISTORICAL_DATA, - DetectorValidationIssueType.TIMEFIELD_FIELD, - ValidationAspect.MODEL - ) - ); - }); - searchFeatureDao.getMinDataTime(anomalyDetector, minTimeListener); + for (Map.Entry entry : topKeys.entrySet()) { + if (entry.getValue() == null) { + topEntityListener.onResponse(Collections.emptyMap()); + return; + } + } + topEntityListener.onResponse(topKeys); + }, topEntityListener::onFailure)); } private void getLatestDateForValidation(Map topEntity) { @@ -410,7 +398,8 @@ public void onResponse(SearchResponse response) { } else if (newIntervalMinute < MAX_INTERVAL_REC_LENGTH_IN_MINUTES && !decreasingInterval) { searchWithDifferentInterval(newIntervalMinute); // The below block is executed only the first time when new interval is above max and - // we aren't decreasing yet, at this point + // we aren't decreasing yet, at this point we will start decreasing for the first time + // if we are inside the below block } else if (newIntervalMinute >= MAX_INTERVAL_REC_LENGTH_IN_MINUTES && !decreasingInterval) { IntervalTimeConfiguration givenInterval = (IntervalTimeConfiguration) anomalyDetector.getDetectionInterval(); this.detectorInterval = new IntervalTimeConfiguration( @@ -718,8 +707,11 @@ private void windowDelayRecommendation(long latestTime) { ); return; } - // This case has been reached if no interval or window delay recommendation was found and no configuration took down bucket - // rate bellow the config bucket rate minimum. + // This case has been reached if no interval recommendation was found that leads to a bucket success rate of >= 0.75 + // but no single configuration during the following checks reduced the bucket success rate below 0.25 + // This means the rate with all configs applied was below 0.75 but the rate when checking each configuration at time + // was always above 0.25 meaning the best suggestion is to simply ingest more data since we have no more insight + // regarding the root cause of the lower density. listener .onFailure( new ADValidationException( diff --git a/src/test/java/org/opensearch/action/admin/indices/mapping/get/IndexAnomalyDetectorActionHandlerTests.java b/src/test/java/org/opensearch/action/admin/indices/mapping/get/IndexAnomalyDetectorActionHandlerTests.java index cbc4dabce..dcc8b6095 100644 --- a/src/test/java/org/opensearch/action/admin/indices/mapping/get/IndexAnomalyDetectorActionHandlerTests.java +++ b/src/test/java/org/opensearch/action/admin/indices/mapping/get/IndexAnomalyDetectorActionHandlerTests.java @@ -26,13 +26,13 @@ import java.util.Arrays; import java.util.Locale; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import org.junit.AfterClass; import org.junit.Before; import org.junit.BeforeClass; import org.junit.Ignore; import org.mockito.ArgumentCaptor; -import org.opensearch.OpenSearchStatusException; import org.opensearch.action.ActionListener; import org.opensearch.action.ActionRequest; import org.opensearch.action.ActionResponse; @@ -47,12 +47,10 @@ import org.opensearch.ad.AbstractADTest; import org.opensearch.ad.TestHelpers; import org.opensearch.ad.common.exception.ADValidationException; -import org.opensearch.ad.constant.CommonErrorMessages; import org.opensearch.ad.constant.CommonName; import org.opensearch.ad.feature.SearchFeatureDao; import org.opensearch.ad.indices.AnomalyDetectionIndices; import org.opensearch.ad.model.AnomalyDetector; -import org.opensearch.ad.model.Feature; import org.opensearch.ad.rest.handler.IndexAnomalyDetectorActionHandler; import org.opensearch.ad.task.ADTaskManager; import org.opensearch.ad.transport.IndexAnomalyDetectorResponse; @@ -69,8 +67,6 @@ import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.TransportService; -import com.google.common.collect.ImmutableList; - /** * * we need to put the test in the same package of GetFieldMappingsResponse @@ -204,26 +200,24 @@ public void testThreeCategoricalFields() throws IOException { } @SuppressWarnings("unchecked") - public void testNoCategoricalField() throws IOException { + public void testMoreThanTenThousandSingleEntityDetectors() throws IOException { SearchResponse mockResponse = mock(SearchResponse.class); int totalHits = 1001; when(mockResponse.getHits()).thenReturn(TestHelpers.createSearchHits(totalHits)); - doAnswer(invocation -> { - Object[] args = invocation.getArguments(); - assertTrue(String.format("The size of args is %d. Its content is %s", args.length, Arrays.toString(args)), args.length == 2); - - assertTrue(args[0] instanceof SearchRequest); - assertTrue(args[1] instanceof ActionListener); - - ActionListener listener = (ActionListener) args[1]; - listener.onResponse(mockResponse); + SearchResponse detectorResponse = mock(SearchResponse.class); + when(detectorResponse.getHits()).thenReturn(TestHelpers.createSearchHits(totalHits)); + SearchResponse userIndexResponse = mock(SearchResponse.class); + int userIndexHits = 0; + when(userIndexResponse.getHits()).thenReturn(TestHelpers.createSearchHits(userIndexHits)); - return null; - }).when(clientMock).search(any(SearchRequest.class), any()); + // extend NodeClient since its execute method is final and mockito does not allow to mock final methods + // we can also use spy to overstep the final methods + NodeClient client = getCustomNodeClient(detectorResponse, userIndexResponse); + NodeClient clientSpy = spy(client); handler = new IndexAnomalyDetectorActionHandler( clusterService, - clientMock, + clientSpy, transportService, channel, anomalyDetectionIndices, @@ -281,9 +275,11 @@ public void doE if (action.equals(SearchAction.INSTANCE)) { listener.onResponse((Response) detectorResponse); } else { + // passes first get field mapping call where timestamp has to be of type date + // fails on second call where categorical field is checked to be type keyword or IP // we need to put the test in the same package of GetFieldMappingsResponse since its constructor is package private GetFieldMappingsResponse response = new GetFieldMappingsResponse( - TestHelpers.createFieldMappings(detector.getIndices().get(0), field, TEXT_FIELD_TYPE) + TestHelpers.createFieldMappings(detector.getIndices().get(0), field, "date") ); listener.onResponse((Response) response); } @@ -337,7 +333,7 @@ private void testValidTypeTemplate(String filedTypeName) throws IOException { SearchResponse userIndexResponse = mock(SearchResponse.class); int userIndexHits = 0; when(userIndexResponse.getHits()).thenReturn(TestHelpers.createSearchHits(userIndexHits)); - + AtomicBoolean isPreCategoryMappingQuery = new AtomicBoolean(true); // extend NodeClient since its execute method is final and mockito does not allow to mock final methods // we can also use spy to overstep the final methods NodeClient client = new NodeClient(Settings.EMPTY, threadPool) { @@ -356,8 +352,13 @@ public void doE } else { listener.onResponse((Response) userIndexResponse); } + } else if (isPreCategoryMappingQuery.get()) { + isPreCategoryMappingQuery.set(false); + GetFieldMappingsResponse response = new GetFieldMappingsResponse( + TestHelpers.createFieldMappings(detector.getIndices().get(0), field, "date") + ); + listener.onResponse((Response) response); } else { - GetFieldMappingsResponse response = new GetFieldMappingsResponse( TestHelpers.createFieldMappings(detector.getIndices().get(0), field, filedTypeName) ); @@ -397,7 +398,7 @@ public void doE handler.start(); - verify(clientSpy, times(1)).execute(eq(GetFieldMappingsAction.INSTANCE), any(), any()); + verify(clientSpy, times(2)).execute(eq(GetFieldMappingsAction.INSTANCE), any(), any()); verify(channel).onFailure(response.capture()); Exception value = response.getValue(); assertTrue(value instanceof IllegalArgumentException); @@ -518,32 +519,77 @@ public void testUpdateTextField() throws IOException { testUpdateTemplate(TEXT_FIELD_TYPE); } + private NodeClient getCustomNodeClient(SearchResponse detectorResponse, SearchResponse userIndexResponse) { + return new NodeClient(Settings.EMPTY, threadPool) { + @Override + public void doExecute( + ActionType action, + Request request, + ActionListener listener + ) { + try { + if (action.equals(SearchAction.INSTANCE)) { + assertTrue(request instanceof SearchRequest); + SearchRequest searchRequest = (SearchRequest) request; + if (searchRequest.indices()[0].equals(ANOMALY_DETECTORS_INDEX)) { + listener.onResponse((Response) detectorResponse); + } else { + listener.onResponse((Response) userIndexResponse); + } + } else { + GetFieldMappingsResponse response = new GetFieldMappingsResponse( + TestHelpers.createFieldMappings(detector.getIndices().get(0), "timestamp", "date") + ); + listener.onResponse((Response) response); + } + } catch (IOException e) { + e.printStackTrace(); + } + } + }; + } + @SuppressWarnings("unchecked") public void testMoreThanTenMultiEntityDetectors() throws IOException { - SearchResponse mockResponse = mock(SearchResponse.class); - + String field = "a"; + AnomalyDetector detector = TestHelpers.randomAnomalyDetectorUsingCategoryFields(detectorId, Arrays.asList(field)); + SearchResponse detectorResponse = mock(SearchResponse.class); int totalHits = 11; + when(detectorResponse.getHits()).thenReturn(TestHelpers.createSearchHits(totalHits)); - when(mockResponse.getHits()).thenReturn(TestHelpers.createSearchHits(totalHits)); - - doAnswer(invocation -> { - Object[] args = invocation.getArguments(); - assertTrue(String.format("The size of args is %d. Its content is %s", args.length, Arrays.toString(args)), args.length == 2); - - assertTrue(args[0] instanceof SearchRequest); - assertTrue(args[1] instanceof ActionListener); - - ActionListener listener = (ActionListener) args[1]; - - listener.onResponse(mockResponse); + SearchResponse userIndexResponse = mock(SearchResponse.class); + int userIndexHits = 0; + when(userIndexResponse.getHits()).thenReturn(TestHelpers.createSearchHits(userIndexHits)); + // extend NodeClient since its execute method is final and mockito does not allow to mock final methods + // we can also use spy to overstep the final methods + NodeClient client = getCustomNodeClient(detectorResponse, userIndexResponse); + NodeClient clientSpy = spy(client); - return null; - }).when(clientMock).search(any(SearchRequest.class), any()); + handler = new IndexAnomalyDetectorActionHandler( + clusterService, + clientSpy, + transportService, + channel, + anomalyDetectionIndices, + detectorId, + seqNo, + primaryTerm, + refreshPolicy, + detector, + requestTimeout, + maxSingleEntityAnomalyDetectors, + maxMultiEntityAnomalyDetectors, + maxAnomalyFeatures, + method, + xContentRegistry(), + null, + adTaskManager, + searchFeatureDao + ); handler.start(); - ArgumentCaptor response = ArgumentCaptor.forClass(Exception.class); - verify(clientMock, times(1)).search(any(SearchRequest.class), any()); + verify(clientSpy, times(1)).search(any(SearchRequest.class), any()); verify(channel).onFailure(response.capture()); Exception value = response.getValue(); assertTrue(value instanceof IllegalArgumentException); @@ -708,50 +754,4 @@ public void testTenMultiEntityDetectorsUpdateExistingMultiEntityAd() throws IOEx assertTrue(value instanceof IllegalStateException); assertTrue(value.getMessage().contains("NodeClient has not been initialized")); } - - @SuppressWarnings("unchecked") - public void testCreateAnomalyDetectorWithDuplicateFeatureAggregationNames() throws IOException { - Feature featureOne = TestHelpers.randomFeature("featureName", "test-1"); - Feature featureTwo = TestHelpers.randomFeature("featureNameTwo", "test-1"); - AnomalyDetector anomalyDetector = TestHelpers.randomAnomalyDetector(ImmutableList.of(featureOne, featureTwo)); - SearchResponse mockResponse = mock(SearchResponse.class); - when(mockResponse.getHits()).thenReturn(TestHelpers.createSearchHits(9)); - doAnswer(invocation -> { - Object[] args = invocation.getArguments(); - assertTrue(String.format("The size of args is %d. Its content is %s", args.length, Arrays.toString(args)), args.length == 2); - assertTrue(args[0] instanceof SearchRequest); - assertTrue(args[1] instanceof ActionListener); - ActionListener listener = (ActionListener) args[1]; - listener.onResponse(mockResponse); - return null; - }).when(clientMock).search(any(SearchRequest.class), any()); - - handler = new IndexAnomalyDetectorActionHandler( - clusterService, - clientMock, - transportService, - channel, - anomalyDetectionIndices, - detectorId, - seqNo, - primaryTerm, - refreshPolicy, - anomalyDetector, - requestTimeout, - maxSingleEntityAnomalyDetectors, - maxMultiEntityAnomalyDetectors, - maxAnomalyFeatures, - method, - xContentRegistry(), - null, - adTaskManager, - searchFeatureDao - ); - ArgumentCaptor response = ArgumentCaptor.forClass(Exception.class); - handler.start(); - verify(channel).onFailure(response.capture()); - Exception value = response.getValue(); - assertTrue(value instanceof OpenSearchStatusException); - assertTrue(value.getMessage().contains(CommonErrorMessages.DUPLICATE_FEATURE_AGGREGATION_NAMES)); - } } diff --git a/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java b/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java index 7465485b5..d814c14c5 100644 --- a/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java +++ b/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java @@ -12,13 +12,13 @@ package org.opensearch.action.admin.indices.mapping.get; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; -import static org.mockito.Mockito.times; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.opensearch.ad.model.AnomalyDetector.ANOMALY_DETECTORS_INDEX; import java.io.IOException; import java.time.Clock; @@ -31,6 +31,10 @@ import org.mockito.Mockito; import org.mockito.MockitoAnnotations; import org.opensearch.action.ActionListener; +import org.opensearch.action.ActionRequest; +import org.opensearch.action.ActionResponse; +import org.opensearch.action.ActionType; +import org.opensearch.action.search.SearchAction; import org.opensearch.action.search.SearchRequest; import org.opensearch.action.search.SearchResponse; import org.opensearch.action.support.WriteRequest; @@ -47,6 +51,7 @@ import org.opensearch.ad.task.ADTaskManager; import org.opensearch.ad.transport.ValidateAnomalyDetectorResponse; import org.opensearch.client.Client; +import org.opensearch.client.node.NodeClient; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; @@ -129,22 +134,47 @@ public void testValidateMoreThanThousandSingleEntityDetectorLimit() throws IOExc SearchResponse mockResponse = mock(SearchResponse.class); int totalHits = maxSingleEntityAnomalyDetectors + 1; when(mockResponse.getHits()).thenReturn(TestHelpers.createSearchHits(totalHits)); - doAnswer(invocation -> { - Object[] args = invocation.getArguments(); - assertTrue(String.format("The size of args is %d. Its content is %s", args.length, Arrays.toString(args)), args.length == 2); + SearchResponse detectorResponse = mock(SearchResponse.class); + when(detectorResponse.getHits()).thenReturn(TestHelpers.createSearchHits(totalHits)); + SearchResponse userIndexResponse = mock(SearchResponse.class); + int userIndexHits = 0; + when(userIndexResponse.getHits()).thenReturn(TestHelpers.createSearchHits(userIndexHits)); - assertTrue(args[0] instanceof SearchRequest); - assertTrue(args[1] instanceof ActionListener); + // extend NodeClient since its execute method is final and mockito does not allow to mock final methods + // we can also use spy to overstep the final methods + NodeClient client = new NodeClient(Settings.EMPTY, threadPool) { + @Override + public void doExecute( + ActionType action, + Request request, + ActionListener listener + ) { + try { + if (action.equals(SearchAction.INSTANCE)) { + assertTrue(request instanceof SearchRequest); + SearchRequest searchRequest = (SearchRequest) request; + if (searchRequest.indices()[0].equals(ANOMALY_DETECTORS_INDEX)) { + listener.onResponse((Response) detectorResponse); + } else { + listener.onResponse((Response) userIndexResponse); + } + } else { + GetFieldMappingsResponse response = new GetFieldMappingsResponse( + TestHelpers.createFieldMappings(detector.getIndices().get(0), "timestamp", "date") + ); + listener.onResponse((Response) response); + } + } catch (IOException e) { + e.printStackTrace(); + } + } + }; - ActionListener listener = (ActionListener) args[1]; - listener.onResponse(mockResponse); - - return null; - }).when(clientMock).search(any(SearchRequest.class), any()); + NodeClient clientSpy = spy(client); handler = new ValidateAnomalyDetectorActionHandler( clusterService, - clientMock, + clientSpy, channel, anomalyDetectionIndices, TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null, true), @@ -161,7 +191,7 @@ public void testValidateMoreThanThousandSingleEntityDetectorLimit() throws IOExc ); handler.start(); ArgumentCaptor response = ArgumentCaptor.forClass(Exception.class); - verify(clientMock, never()).execute(eq(GetMappingsAction.INSTANCE), any(), any()); + verify(clientSpy, never()).execute(eq(GetMappingsAction.INSTANCE), any(), any()); verify(channel).onFailure(response.capture()); Exception value = response.getValue(); assertTrue(value instanceof ADValidationException); @@ -176,29 +206,51 @@ public void testValidateMoreThanThousandSingleEntityDetectorLimit() throws IOExc @SuppressWarnings("unchecked") public void testValidateMoreThanTenMultiEntityDetectorsLimit() throws IOException { - SearchResponse mockResponse = mock(SearchResponse.class); + String field = "a"; + AnomalyDetector detector = TestHelpers.randomAnomalyDetectorUsingCategoryFields(detectorId, Arrays.asList(field)); + SearchResponse detectorResponse = mock(SearchResponse.class); int totalHits = maxMultiEntityAnomalyDetectors + 1; + when(detectorResponse.getHits()).thenReturn(TestHelpers.createSearchHits(totalHits)); - when(mockResponse.getHits()).thenReturn(TestHelpers.createSearchHits(totalHits)); - - doAnswer(invocation -> { - Object[] args = invocation.getArguments(); - assertTrue(String.format("The size of args is %d. Its content is %s", args.length, Arrays.toString(args)), args.length == 2); + SearchResponse userIndexResponse = mock(SearchResponse.class); + int userIndexHits = 0; + when(userIndexResponse.getHits()).thenReturn(TestHelpers.createSearchHits(userIndexHits)); + // extend NodeClient since its execute method is final and mockito does not allow to mock final methods + // we can also use spy to overstep the final methods + NodeClient client = new NodeClient(Settings.EMPTY, threadPool) { + @Override + public void doExecute( + ActionType action, + Request request, + ActionListener listener + ) { + try { + if (action.equals(SearchAction.INSTANCE)) { + assertTrue(request instanceof SearchRequest); + SearchRequest searchRequest = (SearchRequest) request; + if (searchRequest.indices()[0].equals(ANOMALY_DETECTORS_INDEX)) { + listener.onResponse((Response) detectorResponse); + } else { + listener.onResponse((Response) userIndexResponse); + } + } else { + GetFieldMappingsResponse response = new GetFieldMappingsResponse( + TestHelpers.createFieldMappings(detector.getIndices().get(0), field, "date") + ); + listener.onResponse((Response) response); + } + } catch (IOException e) { + e.printStackTrace(); + } + } + }; - assertTrue(args[0] instanceof SearchRequest); - assertTrue(args[1] instanceof ActionListener); - - ActionListener listener = (ActionListener) args[1]; - - listener.onResponse(mockResponse); - - return null; - }).when(clientMock).search(any(SearchRequest.class), any()); + NodeClient clientSpy = spy(client); handler = new ValidateAnomalyDetectorActionHandler( clusterService, - clientMock, + clientSpy, channel, anomalyDetectionIndices, detector, @@ -214,9 +266,8 @@ public void testValidateMoreThanTenMultiEntityDetectorsLimit() throws IOExceptio clock ); handler.start(); - ArgumentCaptor response = ArgumentCaptor.forClass(Exception.class); - verify(clientMock, times(1)).search(any(SearchRequest.class), any()); + verify(clientSpy, never()).execute(eq(GetMappingsAction.INSTANCE), any(), any()); verify(channel).onFailure(response.capture()); Exception value = response.getValue(); assertTrue(value instanceof ADValidationException); diff --git a/src/test/java/org/opensearch/ad/AnomalyDetectorRestTestCase.java b/src/test/java/org/opensearch/ad/AnomalyDetectorRestTestCase.java index 510224124..c1b07fb6f 100644 --- a/src/test/java/org/opensearch/ad/AnomalyDetectorRestTestCase.java +++ b/src/test/java/org/opensearch/ad/AnomalyDetectorRestTestCase.java @@ -154,7 +154,7 @@ protected AnomalyDetector createAnomalyDetector(AnomalyDetector detector, Boolea // step comes next in terms of accessing/update/deleting the detector, this will help avoid // lots of flaky tests try { - Thread.sleep(2500); + Thread.sleep(5000); } catch (InterruptedException ex) { logger.error("Failed to sleep after creating detector", ex); } From daf60700c1c6de3624bc3a671e39f536c1368df0 Mon Sep 17 00:00:00 2001 From: Amit Galitzky Date: Mon, 7 Mar 2022 22:41:59 +0000 Subject: [PATCH 08/13] removed mindata query that isn't used anymore Signed-off-by: Amit Galitzky --- .../ad/feature/SearchFeatureDao.java | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/src/main/java/org/opensearch/ad/feature/SearchFeatureDao.java b/src/main/java/org/opensearch/ad/feature/SearchFeatureDao.java index 0d044c3a1..599d6aacd 100644 --- a/src/main/java/org/opensearch/ad/feature/SearchFeatureDao.java +++ b/src/main/java/org/opensearch/ad/feature/SearchFeatureDao.java @@ -496,24 +496,6 @@ public void getEntityMinDataTime(AnomalyDetector detector, Entity entity, Action ); } - /** - * Get the entity's earliest timestamps - * @param detector detector config - * @param listener listener to return back the requested timestamps - */ - public void getMinDataTime(AnomalyDetector detector, ActionListener> listener) { - SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder() - .aggregation(AggregationBuilders.min(AGG_NAME_MIN).field(detector.getTimeField())) - .trackTotalHits(false) - .size(0); - SearchRequest searchRequest = new SearchRequest().indices(detector.getIndices().toArray(new String[0])).source(searchSourceBuilder); - client - .search( - searchRequest, - ActionListener.wrap(response -> { listener.onResponse(parseMinDataTime(response)); }, listener::onFailure) - ); - } - private Optional parseMinDataTime(SearchResponse searchResponse) { Optional> mapOptional = Optional .ofNullable(searchResponse) From 6d629b438836e23c762737851666fac24eb3da3f Mon Sep 17 00:00:00 2001 From: Amit Galitzky Date: Tue, 8 Mar 2022 16:56:04 +0000 Subject: [PATCH 09/13] added more tests, more are needed still though Signed-off-by: Amit Galitzky --- .../ad/constant/CommonErrorMessages.java | 6 +- .../handler/ModelValidationActionHandler.java | 5 +- ...ndexAnomalyDetectorActionHandlerTests.java | 14 ++- ...dateAnomalyDetectorActionHandlerTests.java | 97 ++++++-------- .../ad/e2e/DetectionResultEvalutationIT.java | 92 ++++++++++++++ .../ad/rest/AnomalyDetectorRestApiIT.java | 91 ++++++-------- ...teAnomalyDetectorTransportActionTests.java | 118 +++++++++++------- 7 files changed, 252 insertions(+), 171 deletions(-) diff --git a/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java b/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java index 17c492d6c..80abce654 100644 --- a/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java +++ b/src/main/java/org/opensearch/ad/constant/CommonErrorMessages.java @@ -111,9 +111,9 @@ public static String getTooManyCategoricalFieldErr(int limit) { public static String TIME_FIELD_NOT_ENOUGH_HISTORICAL_DATA = "There isn't enough historical data found with current timefield selected."; public static String DETECTOR_INTERVAL_REC = - "The selected detector interval might collect sparse data. Consider increasing interval range too: "; + "The selected detector interval might collect sparse data. Consider changing interval length too: "; public static String RAW_DATA_TOO_SPARSE = - "Given index data is potentially too sparse for model training. Consider changing interval length or ingesting more data"; + "Source index data is potentially too sparse for model training. Consider changing interval length or ingesting more data"; public static String MODEL_VALIDATION_FAILED_UNEXPECTEDLY = "Model validation experienced issues completing."; public static String FILTER_QUERY_TOO_SPARSE = "Data is too sparse after data filter is applied. Consider changing the data filter"; public static String CATEGORY_FIELD_TOO_SPARSE = @@ -121,7 +121,7 @@ public static String getTooManyCategoricalFieldErr(int limit) { public static String CATEGORY_FIELD_NO_DATA = "No entity was found with the given categorical fields. Consider revising category field/s or ingesting more data"; public static String FEATURE_QUERY_TOO_SPARSE = - "Given data is most likely too sparse when given feature queries are applied. Consider revising feature queries."; + "Data is most likely too sparse when given feature queries are applied. Consider revising feature queries."; public static String TIMEOUT_ON_INTERVAL_REC = "Timed out getting interval recommendation"; // Modifying message for FEATURE below may break the parseADValidationException method of ValidateAnomalyDetectorTransportAction diff --git a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java index 0b6f34711..892fc5146 100644 --- a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java +++ b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java @@ -132,6 +132,8 @@ public ModelValidationActionHandler( // If detector is HCAD then we will find the top entity and treat as single entity for // validation purposes public void checkIfMultiEntityDetector() { + System.out.println("inside here"); + ActionListener> recommendationListener = ActionListener .wrap(topEntity -> getLatestDateForValidation(topEntity), exception -> { listener.onFailure(exception); @@ -276,7 +278,7 @@ private void getBucketAggregates( listener .onFailure( new ADValidationException( - CommonErrorMessages.CATEGORY_FIELD_NO_DATA, + CommonErrorMessages.CATEGORY_FIELD_TOO_SPARSE, DetectorValidationIssueType.CATEGORY, ValidationAspect.MODEL ) @@ -381,6 +383,7 @@ public void onResponse(SearchResponse response) { ); } double fullBucketRate = processBucketAggregationResults(aggregate); + System.out.println("full bucket rate: " + fullBucketRate); // If rate is above success minimum then return interval suggestion. if (fullBucketRate > INTERVAL_BUCKET_MINIMUM_SUCCESS_RATE) { intervalListener.onResponse(this.detectorInterval); diff --git a/src/test/java/org/opensearch/action/admin/indices/mapping/get/IndexAnomalyDetectorActionHandlerTests.java b/src/test/java/org/opensearch/action/admin/indices/mapping/get/IndexAnomalyDetectorActionHandlerTests.java index dcc8b6095..24f6c642b 100644 --- a/src/test/java/org/opensearch/action/admin/indices/mapping/get/IndexAnomalyDetectorActionHandlerTests.java +++ b/src/test/java/org/opensearch/action/admin/indices/mapping/get/IndexAnomalyDetectorActionHandlerTests.java @@ -212,7 +212,7 @@ public void testMoreThanTenThousandSingleEntityDetectors() throws IOException { // extend NodeClient since its execute method is final and mockito does not allow to mock final methods // we can also use spy to overstep the final methods - NodeClient client = getCustomNodeClient(detectorResponse, userIndexResponse); + NodeClient client = getCustomNodeClient(detectorResponse, userIndexResponse, detector, threadPool); NodeClient clientSpy = spy(client); handler = new IndexAnomalyDetectorActionHandler( @@ -501,7 +501,6 @@ public void doE } else { assertTrue(value.getMessage().contains(IndexAnomalyDetectorActionHandler.CATEGORICAL_FIELD_TYPE_ERR_MSG)); } - } @Ignore @@ -519,8 +518,13 @@ public void testUpdateTextField() throws IOException { testUpdateTemplate(TEXT_FIELD_TYPE); } - private NodeClient getCustomNodeClient(SearchResponse detectorResponse, SearchResponse userIndexResponse) { - return new NodeClient(Settings.EMPTY, threadPool) { + public static NodeClient getCustomNodeClient( + SearchResponse detectorResponse, + SearchResponse userIndexResponse, + AnomalyDetector detector, + ThreadPool pool + ) { + return new NodeClient(Settings.EMPTY, pool) { @Override public void doExecute( ActionType action, @@ -562,7 +566,7 @@ public void testMoreThanTenMultiEntityDetectors() throws IOException { when(userIndexResponse.getHits()).thenReturn(TestHelpers.createSearchHits(userIndexHits)); // extend NodeClient since its execute method is final and mockito does not allow to mock final methods // we can also use spy to overstep the final methods - NodeClient client = getCustomNodeClient(detectorResponse, userIndexResponse); + NodeClient client = getCustomNodeClient(detectorResponse, userIndexResponse, detector, threadPool); NodeClient clientSpy = spy(client); handler = new IndexAnomalyDetectorActionHandler( diff --git a/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java b/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java index d814c14c5..f9923541f 100644 --- a/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java +++ b/src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java @@ -18,7 +18,6 @@ import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import static org.opensearch.ad.model.AnomalyDetector.ANOMALY_DETECTORS_INDEX; import java.io.IOException; import java.time.Clock; @@ -31,11 +30,6 @@ import org.mockito.Mockito; import org.mockito.MockitoAnnotations; import org.opensearch.action.ActionListener; -import org.opensearch.action.ActionRequest; -import org.opensearch.action.ActionResponse; -import org.opensearch.action.ActionType; -import org.opensearch.action.search.SearchAction; -import org.opensearch.action.search.SearchRequest; import org.opensearch.action.search.SearchResponse; import org.opensearch.action.support.WriteRequest; import org.opensearch.ad.AbstractADTest; @@ -139,36 +133,41 @@ public void testValidateMoreThanThousandSingleEntityDetectorLimit() throws IOExc SearchResponse userIndexResponse = mock(SearchResponse.class); int userIndexHits = 0; when(userIndexResponse.getHits()).thenReturn(TestHelpers.createSearchHits(userIndexHits)); + AnomalyDetector singleEntityDetector = TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null, true); // extend NodeClient since its execute method is final and mockito does not allow to mock final methods // we can also use spy to overstep the final methods - NodeClient client = new NodeClient(Settings.EMPTY, threadPool) { - @Override - public void doExecute( - ActionType action, - Request request, - ActionListener listener - ) { - try { - if (action.equals(SearchAction.INSTANCE)) { - assertTrue(request instanceof SearchRequest); - SearchRequest searchRequest = (SearchRequest) request; - if (searchRequest.indices()[0].equals(ANOMALY_DETECTORS_INDEX)) { - listener.onResponse((Response) detectorResponse); - } else { - listener.onResponse((Response) userIndexResponse); - } - } else { - GetFieldMappingsResponse response = new GetFieldMappingsResponse( - TestHelpers.createFieldMappings(detector.getIndices().get(0), "timestamp", "date") - ); - listener.onResponse((Response) response); - } - } catch (IOException e) { - e.printStackTrace(); - } - } - }; + + NodeClient client = IndexAnomalyDetectorActionHandlerTests + .getCustomNodeClient(detectorResponse, userIndexResponse, singleEntityDetector, threadPool); + + // NodeClient client = new NodeClient(Settings.EMPTY, threadPool) { + // @Override + // public void doExecute( + // ActionType action, + // Request request, + // ActionListener listener + // ) { + // try { + // if (action.equals(SearchAction.INSTANCE)) { + // assertTrue(request instanceof SearchRequest); + // SearchRequest searchRequest = (SearchRequest) request; + // if (searchRequest.indices()[0].equals(ANOMALY_DETECTORS_INDEX)) { + // listener.onResponse((Response) detectorResponse); + // } else { + // listener.onResponse((Response) userIndexResponse); + // } + // } else { + // GetFieldMappingsResponse response = new GetFieldMappingsResponse( + // TestHelpers.createFieldMappings(detector.getIndices().get(0), "timestamp", "date") + // ); + // listener.onResponse((Response) response); + // } + // } catch (IOException e) { + // e.printStackTrace(); + // } + // } + // }; NodeClient clientSpy = spy(client); @@ -177,7 +176,7 @@ public void doE clientSpy, channel, anomalyDetectionIndices, - TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null, true), + singleEntityDetector, requestTimeout, maxSingleEntityAnomalyDetectors, maxMultiEntityAnomalyDetectors, @@ -218,34 +217,8 @@ public void testValidateMoreThanTenMultiEntityDetectorsLimit() throws IOExceptio when(userIndexResponse.getHits()).thenReturn(TestHelpers.createSearchHits(userIndexHits)); // extend NodeClient since its execute method is final and mockito does not allow to mock final methods // we can also use spy to overstep the final methods - NodeClient client = new NodeClient(Settings.EMPTY, threadPool) { - @Override - public void doExecute( - ActionType action, - Request request, - ActionListener listener - ) { - try { - if (action.equals(SearchAction.INSTANCE)) { - assertTrue(request instanceof SearchRequest); - SearchRequest searchRequest = (SearchRequest) request; - if (searchRequest.indices()[0].equals(ANOMALY_DETECTORS_INDEX)) { - listener.onResponse((Response) detectorResponse); - } else { - listener.onResponse((Response) userIndexResponse); - } - } else { - GetFieldMappingsResponse response = new GetFieldMappingsResponse( - TestHelpers.createFieldMappings(detector.getIndices().get(0), field, "date") - ); - listener.onResponse((Response) response); - } - } catch (IOException e) { - e.printStackTrace(); - } - } - }; - + NodeClient client = IndexAnomalyDetectorActionHandlerTests + .getCustomNodeClient(detectorResponse, userIndexResponse, detector, threadPool); NodeClient clientSpy = spy(client); handler = new ValidateAnomalyDetectorActionHandler( diff --git a/src/test/java/org/opensearch/ad/e2e/DetectionResultEvalutationIT.java b/src/test/java/org/opensearch/ad/e2e/DetectionResultEvalutationIT.java index eda4f9586..8b6ffce67 100644 --- a/src/test/java/org/opensearch/ad/e2e/DetectionResultEvalutationIT.java +++ b/src/test/java/org/opensearch/ad/e2e/DetectionResultEvalutationIT.java @@ -36,18 +36,24 @@ import org.apache.logging.log4j.core.Logger; import org.opensearch.ad.ODFERestTestCase; import org.opensearch.ad.TestHelpers; +import org.opensearch.ad.constant.CommonErrorMessages; import org.opensearch.client.Request; import org.opensearch.client.RequestOptions; +import org.opensearch.client.Response; import org.opensearch.client.RestClient; import org.opensearch.client.WarningsHandler; import org.opensearch.common.Strings; import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.json.JsonXContent; +import org.opensearch.common.xcontent.support.XContentMapValues; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.gson.JsonArray; +import com.google.gson.JsonElement; import com.google.gson.JsonObject; import com.google.gson.JsonParser; +import com.google.gson.JsonPrimitive; public class DetectionResultEvalutationIT extends ODFERestTestCase { protected static final Logger LOG = (Logger) LogManager.getLogger(DetectionResultEvalutationIT.class); @@ -323,4 +329,90 @@ private void disableResourceNotFoundFaultTolerence() throws IOException { adminClient().performRequest(request); } + + public void testValidationIntervalRecommendation() throws Exception { + RestClient client = client(); + long recDetectorIntervalMillis = 180000; + long recDetectorIntervalMinutes = recDetectorIntervalMillis / 60000; + System.out.println(recDetectorIntervalMinutes); + List data = createData(2000, recDetectorIntervalMillis); + indexTrainData("validation", data, 2000, client); + indexTestData(data, "validation", 2000, client); + long detectorInterval = 1; + String requestBody = String + .format( + Locale.ROOT, + "{ \"name\": \"test\", \"description\": \"test\", \"time_field\": \"timestamp\"" + + ", \"indices\": [\"validation\"], \"feature_attributes\": [{ \"feature_name\": \"feature 1\", \"feature_enabled\": " + + "\"true\", \"aggregation_query\": { \"Feature1\": { \"sum\": { \"field\": \"Feature1\" } } } }, { \"feature_name\"" + + ": \"feature 2\", \"feature_enabled\": \"true\", \"aggregation_query\": { \"Feature2\": { \"sum\": { \"field\": " + + "\"Feature2\" } } } }], \"detection_interval\": { \"period\": { \"interval\": %d, \"unit\": \"Minutes\" } }" + + ",\"window_delay\":{\"period\":{\"interval\":10,\"unit\":\"Minutes\"}}}", + detectorInterval + ); + Response resp = TestHelpers + .makeRequest( + client(), + "POST", + TestHelpers.AD_BASE_DETECTORS_URI + "/_validate/model", + ImmutableMap.of(), + toHttpEntity(requestBody), + null + ); + Map responseMap = entityAsMap(resp); + @SuppressWarnings("unchecked") + Map> messageMap = (Map>) XContentMapValues + .extractValue("model", responseMap); + assertEquals( + CommonErrorMessages.DETECTOR_INTERVAL_REC + recDetectorIntervalMinutes, + messageMap.get("detection_interval").get("message") + ); + } + + private List createData(int numOfDataPoints, long detectorIntervalMS) { + List list = new ArrayList<>(); + for (int i = 1; i < numOfDataPoints; i++) { + long valueFeature1 = randomLongBetween(1, 10000000); + long valueFeature2 = randomLongBetween(1, 10000000); + JsonObject obj = new JsonObject(); + JsonElement element = new JsonPrimitive(Instant.now().toEpochMilli() - (detectorIntervalMS * i)); + obj.add("timestamp", element); + obj.add("Feature1", new JsonPrimitive(valueFeature1)); + obj.add("Feature2", new JsonPrimitive(valueFeature2)); + list.add(obj); + } + return list; + } + + private void indexTrainData(String datasetName, List data, int trainTestSplit, RestClient client) throws Exception { + Request request = new Request("PUT", datasetName); + String requestBody = "{ \"mappings\": { \"properties\": { \"timestamp\": { \"type\": \"date\"}," + + " \"Feature1\": { \"type\": \"long\" }, \"Feature2\": { \"type\": \"long\" } } } }"; + request.setJsonEntity(requestBody); + client.performRequest(request); + Thread.sleep(1_000); + data.stream().limit(trainTestSplit).forEach(r -> { + try { + Request req = new Request("POST", String.format("/%s/_doc/", datasetName)); + req.setJsonEntity(r.toString()); + client.performRequest(req); + } catch (Exception e) { + throw new RuntimeException(e); + } + }); + Thread.sleep(1_000); + } + + private void indexTestData(List data, String datasetName, int trainTestSplit, RestClient client) throws Exception { + data.stream().skip(trainTestSplit).forEach(r -> { + try { + Request req = new Request("POST", String.format("/%s/_doc/", datasetName)); + req.setJsonEntity(r.toString()); + client.performRequest(req); + } catch (Exception e) { + throw new RuntimeException(e); + } + }); + Thread.sleep(1_000); + } } diff --git a/src/test/java/org/opensearch/ad/rest/AnomalyDetectorRestApiIT.java b/src/test/java/org/opensearch/ad/rest/AnomalyDetectorRestApiIT.java index 6a88cac62..f5c50a81f 100644 --- a/src/test/java/org/opensearch/ad/rest/AnomalyDetectorRestApiIT.java +++ b/src/test/java/org/opensearch/ad/rest/AnomalyDetectorRestApiIT.java @@ -58,6 +58,9 @@ public class AnomalyDetectorRestApiIT extends AnomalyDetectorRestTestCase { + protected static final String INDEX_NAME = "indexname"; + protected static final String TIME_FIELD = "timestamp"; + public void testCreateAnomalyDetectorWithNotExistingIndices() throws Exception { AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null); TestHelpers @@ -107,11 +110,16 @@ public void testCreateAnomalyDetectorWithEmptyIndices() throws Exception { ); } - public void testCreateAnomalyDetectorWithDuplicateName() throws Exception { - AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null); - String indexName = detector.getIndices().get(0); - TestHelpers.createIndex(client(), indexName, TestHelpers.toHttpEntity("{\"name\": \"test\"}")); + private AnomalyDetector createIndexAndGetAnomalyDetector() throws IOException { + TestHelpers.createIndexWithTimeField(client(), INDEX_NAME, TIME_FIELD); + String testIndexData = "{\"keyword-field\": \"field-1\", \"ip-field\": \"1.2.3.4\", \"timestamp\": 1}"; + TestHelpers.ingestDataToIndex(client(), INDEX_NAME, TestHelpers.toHttpEntity(testIndexData)); + AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TIME_FIELD, INDEX_NAME); + return detector; + } + public void testCreateAnomalyDetectorWithDuplicateName() throws Exception { + AnomalyDetector detector = createIndexAndGetAnomalyDetector(); AnomalyDetector detectorDuplicateName = new AnomalyDetector( AnomalyDetector.NO_ID, randomLong(), @@ -149,10 +157,7 @@ public void testCreateAnomalyDetectorWithDuplicateName() throws Exception { } public void testCreateAnomalyDetector() throws Exception { - AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null); - String indexName = detector.getIndices().get(0); - TestHelpers.createIndex(client(), indexName, TestHelpers.toHttpEntity("{\"name\": \"test\"}")); - + AnomalyDetector detector = createIndexAndGetAnomalyDetector(); updateClusterSettings(EnabledSetting.AD_PLUGIN_ENABLED, false); Exception ex = expectThrows( @@ -181,11 +186,7 @@ public void testCreateAnomalyDetector() throws Exception { } public void testUpdateAnomalyDetectorCategoryField() throws Exception { - String indexName = "testindex"; - TestHelpers.createIndexWithTimeField(client(), "testindex", "timestamp"); - String testIndexData = "{\"keyword-field\": \"field-1\", \"ip-field\": \"1.2.3.4\", \"timestamp\": 1}"; - TestHelpers.ingestDataToIndex(client(), indexName, TestHelpers.toHttpEntity(testIndexData)); - AnomalyDetector detector = TestHelpers.randomAnomalyDetector("timestamp", indexName); + AnomalyDetector detector = createIndexAndGetAnomalyDetector(); Response response = TestHelpers .makeRequest(client(), "POST", TestHelpers.AD_BASE_DETECTORS_URI, ImmutableMap.of(), TestHelpers.toHttpEntity(detector), null); assertEquals("Create anomaly detector failed", RestStatus.CREATED, TestHelpers.restStatus(response)); @@ -245,11 +246,7 @@ public void testGetNotExistingAnomalyDetector() throws Exception { } public void testUpdateAnomalyDetector() throws Exception { - String indexName = "testindex"; - TestHelpers.createIndexWithTimeField(client(), "testindex", "timestamp"); - String testIndexData = "{\"keyword-field\": \"field-1\", \"ip-field\": \"1.2.3.4\", \"timestamp\": 1}"; - TestHelpers.ingestDataToIndex(client(), indexName, TestHelpers.toHttpEntity(testIndexData)); - AnomalyDetector detector = createAnomalyDetector(TestHelpers.randomAnomalyDetector("timestamp", indexName), true, client()); + AnomalyDetector detector = createAnomalyDetector(createIndexAndGetAnomalyDetector(), true, client()); String newDescription = randomAlphaOfLength(5); AnomalyDetector newDetector = new AnomalyDetector( detector.getDetectorId(), @@ -313,7 +310,6 @@ public void testUpdateAnomalyDetectorNameToExisting() throws Exception { AnomalyDetector detector1 = TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null); String indexName1 = detector1.getIndices().get(0); TestHelpers.createIndex(client(), indexName1, TestHelpers.toHttpEntity("{\"name\": \"test\"}")); - AnomalyDetector detector2 = TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null); String indexName2 = detector2.getIndices().get(0); TestHelpers.createIndex(client(), indexName2, TestHelpers.toHttpEntity("{\"name\": \"test\"}")); @@ -355,12 +351,7 @@ public void testUpdateAnomalyDetectorNameToExisting() throws Exception { } public void testUpdateAnomalyDetectorNameToNew() throws Exception { - String indexName = "testindex"; - TestHelpers.createIndexWithTimeField(client(), "testindex", "timestamp"); - String testIndexData = "{\"keyword-field\": \"field-1\", \"ip-field\": \"1.2.3.4\", \"timestamp\": 1}"; - TestHelpers.ingestDataToIndex(client(), indexName, TestHelpers.toHttpEntity(testIndexData)); - AnomalyDetector detector = createAnomalyDetector(TestHelpers.randomAnomalyDetector("timestamp", indexName), true, client()); - + AnomalyDetector detector = createAnomalyDetector(createIndexAndGetAnomalyDetector(), true, client()); AnomalyDetector detectorWithNewName = new AnomalyDetector( detector.getDetectorId(), detector.getVersion(), @@ -775,10 +766,7 @@ public void testDeleteAnomalyDetectorWithRunningAdJob() throws Exception { } public void testUpdateAnomalyDetectorWithRunningAdJob() throws Exception { - String indexName = "testindex"; - TestHelpers.createIndexWithTimeField(client(), "testindex", "timestamp"); - String testIndexData = "{\"keyword-field\": \"field-1\", \"ip-field\": \"1.2.3.4\", \"timestamp\": 1}"; - AnomalyDetector detector = createAnomalyDetector(TestHelpers.randomAnomalyDetector("timestamp", indexName), true, client()); + AnomalyDetector detector = createAnomalyDetector(createIndexAndGetAnomalyDetector(), true, client()); Response startAdJobResponse = TestHelpers .makeRequest( client(), @@ -1208,10 +1196,7 @@ public void testDeleteAnomalyDetectorWhileRunning() throws Exception { public void testBackwardCompatibilityWithOpenDistro() throws IOException { // Create a detector - AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null); - String indexName = detector.getIndices().get(0); - TestHelpers.createIndex(client(), indexName, TestHelpers.toHttpEntity("{\"name\": \"test\"}")); - + AnomalyDetector detector = createIndexAndGetAnomalyDetector(); // Verify the detector is created using legacy _opendistro API Response response = TestHelpers .makeRequest( @@ -1248,9 +1233,7 @@ public void testBackwardCompatibilityWithOpenDistro() throws IOException { } public void testValidateAnomalyDetectorWithDuplicateName() throws Exception { - AnomalyDetector detector = createRandomAnomalyDetector(true, true, client()); - TestHelpers.createIndexWithTimeField(client(), "test-index", "timestamp"); - + AnomalyDetector detector = createAnomalyDetector(createIndexAndGetAnomalyDetector(), true, client()); Response resp = TestHelpers .makeRequest( client(), @@ -1262,7 +1245,9 @@ public void testValidateAnomalyDetectorWithDuplicateName() throws Exception { "{\"name\":\"" + detector.getName() + "\",\"description\":\"Test detector\",\"time_field\":\"timestamp\"," - + "\"indices\":[\"test-index\"],\"feature_attributes\":[{\"feature_name\":\"cpu-sum\",\"" + + "\"indices\":[\"" + + INDEX_NAME + + "\"],\"feature_attributes\":[{\"feature_name\":\"cpu-sum\",\"" + "feature_enabled\":true,\"aggregation_query\":{\"total_cpu\":{\"sum\":{\"field\":\"cpu\"}}}}," + "{\"feature_name\":\"error-sum\",\"feature_enabled\":true,\"aggregation_query\":" + "{\"total_error\":" @@ -1347,8 +1332,8 @@ public void testValidateAnomalyDetectorWithIncorrectShingleSize() throws Excepti public void testValidateAnomalyDetectorWithNoIssue() throws Exception { String indexName = "testindex"; - TestHelpers.createIndexWithTimeField(client(), "testindex", "timestamp"); - AnomalyDetector detector = TestHelpers.randomAnomalyDetector("timestamp", indexName); + TestHelpers.createIndexWithTimeField(client(), "testindex", TIME_FIELD); + AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TIME_FIELD, indexName); Response resp = TestHelpers .makeRequest( client(), @@ -1383,9 +1368,7 @@ public void testValidateAnomalyDetectorOnWrongValidationType() throws Exception } public void testValidateAnomalyDetectorWithEmptyIndices() throws Exception { - String indexName = "testindex"; - // TestHelpers.createEmptyIndexWithTimeField(client(), "testindex", "timestamp"); - AnomalyDetector detector = TestHelpers.randomAnomalyDetector("timestamp", indexName); + AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TIME_FIELD, INDEX_NAME); TestHelpers .makeRequest( client(), @@ -1446,8 +1429,8 @@ public void testValidateAnomalyDetectorWithInvalidName() throws Exception { public void testValidateAnomalyDetectorWithFeatureQueryReturningNoData() throws Exception { Feature emptyFeature = TestHelpers.randomFeature("f-empty", "cpu", "avg", true); - AnomalyDetector detector = TestHelpers.randomAnomalyDetector("timestamp", "index-test", ImmutableList.of(emptyFeature)); - TestHelpers.createIndexWithTimeField(client(), "index-test", "timestamp"); + AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TIME_FIELD, "index-test", ImmutableList.of(emptyFeature)); + TestHelpers.createIndexWithTimeField(client(), "index-test", TIME_FIELD); Response resp = TestHelpers .makeRequest( client(), @@ -1471,8 +1454,8 @@ public void testValidateAnomalyDetectorWithFeatureQueryReturningNoData() throws public void testValidateAnomalyDetectorWithFeatureQueryRuntimeException() throws Exception { String nonNumericField = "_type"; Feature nonNumericFeature = TestHelpers.randomFeature("non-numeric-feature", nonNumericField, "avg", true); - AnomalyDetector detector = TestHelpers.randomAnomalyDetector("timestamp", "index-test", ImmutableList.of(nonNumericFeature)); - TestHelpers.createIndexWithTimeField(client(), "index-test", "timestamp"); + AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TIME_FIELD, "index-test", ImmutableList.of(nonNumericFeature)); + TestHelpers.createIndexWithTimeField(client(), "index-test", TIME_FIELD); Response resp = TestHelpers .makeRequest( client(), @@ -1497,11 +1480,11 @@ public void testValidateAnomalyDetectorWithWrongCategoryField() throws Exception AnomalyDetector detector = TestHelpers .randomAnomalyDetectorUsingCategoryFields( randomAlphaOfLength(5), - "timestamp", + TIME_FIELD, ImmutableList.of("index-test"), Arrays.asList("host.keyword") ); - TestHelpers.createIndexWithTimeField(client(), "index-test", "timestamp"); + TestHelpers.createIndexWithTimeField(client(), "index-test", TIME_FIELD); Response resp = TestHelpers .makeRequest( client(), @@ -1538,7 +1521,7 @@ public void testSearchTopAnomalyResultsWithInvalidInputs() throws IOException { TestHelpers .randomAnomalyDetectorUsingCategoryFields( randomAlphaOfLength(10), - "timestamp", + TIME_FIELD, ImmutableList.of(indexName), categoryFieldsAndTypes.keySet().stream().collect(Collectors.toList()) ), @@ -1646,7 +1629,7 @@ public void testSearchTopAnomalyResultsWithInvalidInputs() throws IOException { TestHelpers .randomAnomalyDetectorUsingCategoryFields( randomAlphaOfLength(10), - "timestamp", + TIME_FIELD, ImmutableList.of(indexName), ImmutableList.of() ), @@ -1686,7 +1669,7 @@ public void testSearchTopAnomalyResultsOnNonExistentResultIndex() throws IOExcep TestHelpers .randomAnomalyDetectorUsingCategoryFields( randomAlphaOfLength(10), - "timestamp", + TIME_FIELD, ImmutableList.of(indexName), categoryFieldsAndTypes.keySet().stream().collect(Collectors.toList()) ), @@ -1725,7 +1708,7 @@ public void testSearchTopAnomalyResultsOnEmptyResultIndex() throws IOException { TestHelpers .randomAnomalyDetectorUsingCategoryFields( randomAlphaOfLength(10), - "timestamp", + TIME_FIELD, ImmutableList.of(indexName), categoryFieldsAndTypes.keySet().stream().collect(Collectors.toList()) ), @@ -1765,7 +1748,7 @@ public void testSearchTopAnomalyResultsOnPopulatedResultIndex() throws IOExcepti TestHelpers .randomAnomalyDetectorUsingCategoryFields( randomAlphaOfLength(10), - "timestamp", + TIME_FIELD, ImmutableList.of(indexName), categoryFieldsAndTypes.keySet().stream().collect(Collectors.toList()) ), @@ -1882,7 +1865,7 @@ public void testSearchTopAnomalyResultsWithCustomResultIndex() throws IOExceptio TestHelpers .randomAnomalyDetectorUsingCategoryFields( randomAlphaOfLength(10), - "timestamp", + TIME_FIELD, ImmutableList.of(indexName), categoryFieldsAndTypes.keySet().stream().collect(Collectors.toList()), customResultIndexName diff --git a/src/test/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportActionTests.java b/src/test/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportActionTests.java index 9fd716750..091664e97 100644 --- a/src/test/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportActionTests.java +++ b/src/test/java/org/opensearch/ad/transport/ValidateAnomalyDetectorTransportActionTests.java @@ -15,6 +15,7 @@ import java.net.URL; import java.time.Instant; import java.time.temporal.ChronoUnit; +import java.util.Locale; import org.junit.Test; import org.opensearch.ad.ADIntegTestCase; @@ -40,13 +41,8 @@ public class ValidateAnomalyDetectorTransportActionTests extends ADIntegTestCase @Test public void testValidateAnomalyDetectorWithNoIssue() throws IOException { AnomalyDetector anomalyDetector = TestHelpers - .randomAnomalyDetector( - "timestamp", - "test-index", - ImmutableList.of(sumValueFeature(nameField, ipField + ".is_error", "test-2")) - ); - Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); - ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); + .randomAnomalyDetector(timeField, "test-index", ImmutableList.of(sumValueFeature(nameField, ipField + ".is_error", "test-2"))); + ingestTestDataValidate(anomalyDetector.getIndices().get(0), Instant.now().minus(1, ChronoUnit.DAYS), 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( anomalyDetector, ValidationAspect.DETECTOR.getName(), @@ -79,9 +75,8 @@ public void testValidateAnomalyDetectorWithNoIndexFound() throws IOException { @Test public void testValidateAnomalyDetectorWithDuplicateName() throws IOException { - AnomalyDetector anomalyDetector = TestHelpers.randomAnomalyDetector("timestamp", "index-test"); - Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); - ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); + AnomalyDetector anomalyDetector = TestHelpers.randomAnomalyDetector(timeField, "index-test"); + ingestTestDataValidate(anomalyDetector.getIndices().get(0), Instant.now().minus(1, ChronoUnit.DAYS), 1, "error"); createDetectorIndex(); createDetector(anomalyDetector); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( @@ -101,9 +96,8 @@ public void testValidateAnomalyDetectorWithDuplicateName() throws IOException { @Test public void testValidateAnomalyDetectorWithNonExistingFeatureField() throws IOException { Feature maxFeature = maxValueFeature(nameField, "non_existing_field", nameField); - AnomalyDetector anomalyDetector = TestHelpers.randomAnomalyDetector("timestamp", "test-index", ImmutableList.of(maxFeature)); - Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); - ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); + AnomalyDetector anomalyDetector = TestHelpers.randomAnomalyDetector(timeField, "test-index", ImmutableList.of(maxFeature)); + ingestTestDataValidate(anomalyDetector.getIndices().get(0), Instant.now().minus(1, ChronoUnit.DAYS), 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( anomalyDetector, ValidationAspect.DETECTOR.getName(), @@ -126,9 +120,8 @@ public void testValidateAnomalyDetectorWithDuplicateFeatureAggregationNames() th Feature maxFeature = maxValueFeature(nameField, categoryField, "test-1"); Feature maxFeatureTwo = maxValueFeature(nameField, categoryField, "test-2"); AnomalyDetector anomalyDetector = TestHelpers - .randomAnomalyDetector("timestamp", "test-index", ImmutableList.of(maxFeature, maxFeatureTwo)); - Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); - ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); + .randomAnomalyDetector(timeField, "test-index", ImmutableList.of(maxFeature, maxFeatureTwo)); + ingestTestDataValidate(anomalyDetector.getIndices().get(0), Instant.now().minus(1, ChronoUnit.DAYS), 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( anomalyDetector, ValidationAspect.DETECTOR.getName(), @@ -149,9 +142,8 @@ public void testValidateAnomalyDetectorWithDuplicateFeatureNamesAndDuplicateAggr Feature maxFeature = maxValueFeature(nameField, categoryField, nameField); Feature maxFeatureTwo = maxValueFeature(nameField, categoryField, nameField); AnomalyDetector anomalyDetector = TestHelpers - .randomAnomalyDetector("timestamp", "test-index", ImmutableList.of(maxFeature, maxFeatureTwo)); - Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); - ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); + .randomAnomalyDetector(timeField, "test-index", ImmutableList.of(maxFeature, maxFeatureTwo)); + ingestTestDataValidate(anomalyDetector.getIndices().get(0), Instant.now().minus(1, ChronoUnit.DAYS), 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( anomalyDetector, ValidationAspect.DETECTOR.getName(), @@ -173,9 +165,8 @@ public void testValidateAnomalyDetectorWithDuplicateFeatureNames() throws IOExce Feature maxFeature = maxValueFeature(nameField, categoryField, nameField); Feature maxFeatureTwo = maxValueFeature("test_1", categoryField, nameField); AnomalyDetector anomalyDetector = TestHelpers - .randomAnomalyDetector("timestamp", "test-index", ImmutableList.of(maxFeature, maxFeatureTwo)); - Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); - ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); + .randomAnomalyDetector(timeField, "test-index", ImmutableList.of(maxFeature, maxFeatureTwo)); + ingestTestDataValidate(anomalyDetector.getIndices().get(0), Instant.now().minus(1, ChronoUnit.DAYS), 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( anomalyDetector, ValidationAspect.DETECTOR.getName(), @@ -194,9 +185,8 @@ public void testValidateAnomalyDetectorWithDuplicateFeatureNames() throws IOExce @Test public void testValidateAnomalyDetectorWithInvalidFeatureField() throws IOException { Feature maxFeature = maxValueFeature(nameField, categoryField, nameField); - AnomalyDetector anomalyDetector = TestHelpers.randomAnomalyDetector("timestamp", "test-index", ImmutableList.of(maxFeature)); - Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); - ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); + AnomalyDetector anomalyDetector = TestHelpers.randomAnomalyDetector(timeField, "test-index", ImmutableList.of(maxFeature)); + ingestTestDataValidate(anomalyDetector.getIndices().get(0), Instant.now().minus(1, ChronoUnit.DAYS), 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( anomalyDetector, ValidationAspect.DETECTOR.getName(), @@ -221,12 +211,11 @@ public void testValidateAnomalyDetectorWithUnknownFeatureField() throws IOExcept AggregationBuilder aggregationBuilder = TestHelpers.parseAggregation("{\"test\":{\"terms\":{\"field\":\"type\"}}}"); AnomalyDetector anomalyDetector = TestHelpers .randomAnomalyDetector( - "timestamp", + timeField, "test-index", ImmutableList.of(new Feature(randomAlphaOfLength(5), nameField, true, aggregationBuilder)) ); - Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); - ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); + ingestTestDataValidate(anomalyDetector.getIndices().get(0), Instant.now().minus(1, ChronoUnit.DAYS), 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( anomalyDetector, ValidationAspect.DETECTOR.getName(), @@ -248,9 +237,8 @@ public void testValidateAnomalyDetectorWithMultipleInvalidFeatureField() throws Feature maxFeature = maxValueFeature(nameField, categoryField, nameField); Feature maxFeatureTwo = maxValueFeature("test_two", categoryField, "test_two"); AnomalyDetector anomalyDetector = TestHelpers - .randomAnomalyDetector("timestamp", "test-index", ImmutableList.of(maxFeature, maxFeatureTwo)); - Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); - ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); + .randomAnomalyDetector(timeField, "test-index", ImmutableList.of(maxFeature, maxFeatureTwo)); + ingestTestDataValidate(anomalyDetector.getIndices().get(0), Instant.now().minus(1, ChronoUnit.DAYS), 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( anomalyDetector, ValidationAspect.DETECTOR.getName(), @@ -280,12 +268,11 @@ public void testValidateAnomalyDetectorWithCustomResultIndex() throws IOExceptio ImmutableList.of(TestHelpers.randomFeature()), randomAlphaOfLength(5).toLowerCase(), randomIntBetween(1, 5), - "timestamp", + timeField, null, resultIndex ); - Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); - ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); + ingestTestDataValidate(anomalyDetector.getIndices().get(0), Instant.now().minus(1, ChronoUnit.DAYS), 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( anomalyDetector, ValidationAspect.DETECTOR.getName(), @@ -319,12 +306,11 @@ public void testValidateAnomalyDetectorWithCustomResultIndexWithInvalidMapping() ImmutableList.of(TestHelpers.randomFeature()), randomAlphaOfLength(5).toLowerCase(), randomIntBetween(1, 5), - "timestamp", + timeField, null, resultIndex ); - Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); - ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); + ingestTestDataValidate(anomalyDetector.getIndices().get(0), Instant.now().minus(1, ChronoUnit.DAYS), 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( anomalyDetector, ValidationAspect.DETECTOR.getName(), @@ -349,12 +335,11 @@ private void testValidateAnomalyDetectorWithCustomResultIndex(boolean resultInde ImmutableList.of(TestHelpers.randomFeature()), randomAlphaOfLength(5).toLowerCase(), randomIntBetween(1, 5), - "timestamp", + timeField, null, resultIndex ); - Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); - ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); + ingestTestDataValidate(anomalyDetector.getIndices().get(0), Instant.now().minus(1, ChronoUnit.DAYS), 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( anomalyDetector, ValidationAspect.DETECTOR.getName(), @@ -374,7 +359,7 @@ public void testValidateAnomalyDetectorWithInvalidDetectorName() throws IOExcept randomLong(), "#$32", randomAlphaOfLength(5), - "timestamp", + timeField, ImmutableList.of(randomAlphaOfLength(5).toLowerCase()), ImmutableList.of(TestHelpers.randomFeature()), TestHelpers.randomQuery(), @@ -388,8 +373,7 @@ public void testValidateAnomalyDetectorWithInvalidDetectorName() throws IOExcept TestHelpers.randomUser(), null ); - Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); - ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); + ingestTestDataValidate(anomalyDetector.getIndices().get(0), Instant.now().minus(1, ChronoUnit.DAYS), 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( anomalyDetector, ValidationAspect.DETECTOR.getName(), @@ -411,7 +395,7 @@ public void testValidateAnomalyDetectorWithDetectorNameTooLong() throws IOExcept randomLong(), "abababababababababababababababababababababababababababababababababababababababababababababababab", randomAlphaOfLength(5), - "timestamp", + timeField, ImmutableList.of(randomAlphaOfLength(5).toLowerCase()), ImmutableList.of(TestHelpers.randomFeature()), TestHelpers.randomQuery(), @@ -425,8 +409,7 @@ public void testValidateAnomalyDetectorWithDetectorNameTooLong() throws IOExcept TestHelpers.randomUser(), null ); - Instant startTime = Instant.now().minus(1, ChronoUnit.DAYS); - ingestTestDataValidate(anomalyDetector.getIndices().get(0), startTime, 1, "error"); + ingestTestDataValidate(anomalyDetector.getIndices().get(0), Instant.now().minus(1, ChronoUnit.DAYS), 1, "error"); ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( anomalyDetector, ValidationAspect.DETECTOR.getName(), @@ -440,4 +423,47 @@ public void testValidateAnomalyDetectorWithDetectorNameTooLong() throws IOExcept assertEquals(ValidationAspect.DETECTOR, response.getIssue().getAspect()); assertTrue(response.getIssue().getMessage().contains("Name should be shortened. The maximum limit is")); } + + @Test + public void testValidateAnomalyDetectorWithNonExistentTimefield() throws IOException { + AnomalyDetector anomalyDetector = TestHelpers.randomAnomalyDetector(ImmutableMap.of(), Instant.now()); + ingestTestDataValidate(anomalyDetector.getIndices().get(0), Instant.now().minus(1, ChronoUnit.DAYS), 1, "error"); + ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( + anomalyDetector, + ValidationAspect.DETECTOR.getName(), + 5, + 5, + 5, + new TimeValue(5_000L) + ); + ValidateAnomalyDetectorResponse response = client().execute(ValidateAnomalyDetectorAction.INSTANCE, request).actionGet(5_000); + assertEquals(DetectorValidationIssueType.TIMEFIELD_FIELD, response.getIssue().getType()); + assertEquals(ValidationAspect.DETECTOR, response.getIssue().getAspect()); + assertEquals( + String.format(Locale.ROOT, CommonErrorMessages.NON_EXISTENT_TIMESTAMP, anomalyDetector.getTimeField()), + response.getIssue().getMessage() + ); + } + + @Test + public void testValidateAnomalyDetectorWithNonDateTimeField() throws IOException { + AnomalyDetector anomalyDetector = TestHelpers.randomAnomalyDetector(categoryField, "index-test"); + ingestTestDataValidate(anomalyDetector.getIndices().get(0), Instant.now().minus(1, ChronoUnit.DAYS), 1, "error"); + ValidateAnomalyDetectorRequest request = new ValidateAnomalyDetectorRequest( + anomalyDetector, + ValidationAspect.DETECTOR.getName(), + 5, + 5, + 5, + new TimeValue(5_000L) + ); + ValidateAnomalyDetectorResponse response = client().execute(ValidateAnomalyDetectorAction.INSTANCE, request).actionGet(5_000); + assertEquals(DetectorValidationIssueType.TIMEFIELD_FIELD, response.getIssue().getType()); + assertEquals(ValidationAspect.DETECTOR, response.getIssue().getAspect()); + assertEquals( + String.format(Locale.ROOT, CommonErrorMessages.INVALID_TIMESTAMP, anomalyDetector.getTimeField()), + response.getIssue().getMessage() + ); + } + } From 20bd99bbf0a5b8e106cc1418b84e2579a64ccc83 Mon Sep 17 00:00:00 2001 From: Amit Galitzky Date: Tue, 8 Mar 2022 16:58:01 +0000 Subject: [PATCH 10/13] removed logs Signed-off-by: Amit Galitzky --- .../ad/rest/handler/ModelValidationActionHandler.java | 3 --- .../org/opensearch/ad/e2e/DetectionResultEvalutationIT.java | 1 - 2 files changed, 4 deletions(-) diff --git a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java index 892fc5146..650105911 100644 --- a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java +++ b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java @@ -132,8 +132,6 @@ public ModelValidationActionHandler( // If detector is HCAD then we will find the top entity and treat as single entity for // validation purposes public void checkIfMultiEntityDetector() { - System.out.println("inside here"); - ActionListener> recommendationListener = ActionListener .wrap(topEntity -> getLatestDateForValidation(topEntity), exception -> { listener.onFailure(exception); @@ -383,7 +381,6 @@ public void onResponse(SearchResponse response) { ); } double fullBucketRate = processBucketAggregationResults(aggregate); - System.out.println("full bucket rate: " + fullBucketRate); // If rate is above success minimum then return interval suggestion. if (fullBucketRate > INTERVAL_BUCKET_MINIMUM_SUCCESS_RATE) { intervalListener.onResponse(this.detectorInterval); diff --git a/src/test/java/org/opensearch/ad/e2e/DetectionResultEvalutationIT.java b/src/test/java/org/opensearch/ad/e2e/DetectionResultEvalutationIT.java index 8b6ffce67..832f3e215 100644 --- a/src/test/java/org/opensearch/ad/e2e/DetectionResultEvalutationIT.java +++ b/src/test/java/org/opensearch/ad/e2e/DetectionResultEvalutationIT.java @@ -334,7 +334,6 @@ public void testValidationIntervalRecommendation() throws Exception { RestClient client = client(); long recDetectorIntervalMillis = 180000; long recDetectorIntervalMinutes = recDetectorIntervalMillis / 60000; - System.out.println(recDetectorIntervalMinutes); List data = createData(2000, recDetectorIntervalMillis); indexTrainData("validation", data, 2000, client); indexTestData(data, "validation", 2000, client); From 098bb93677ef7177d566934cdbf58fbd7d067288 Mon Sep 17 00:00:00 2001 From: Amit Galitzky Date: Tue, 8 Mar 2022 18:52:44 +0000 Subject: [PATCH 11/13] changed minimum success rate for raw data only to .75 Signed-off-by: Amit Galitzky --- .../rest/handler/ModelValidationActionHandler.java | 2 +- .../ad/e2e/DetectionResultEvalutationIT.java | 14 -------------- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java index 650105911..673677542 100644 --- a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java +++ b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java @@ -552,7 +552,7 @@ private void processRawDataResults(SearchResponse response, long latestTime) { return; } double fullBucketRate = processBucketAggregationResults(aggregate); - if (fullBucketRate < CONFIG_BUCKET_MINIMUM_SUCCESS_RATE) { + if (fullBucketRate < INTERVAL_BUCKET_MINIMUM_SUCCESS_RATE) { listener .onFailure( new ADValidationException( diff --git a/src/test/java/org/opensearch/ad/e2e/DetectionResultEvalutationIT.java b/src/test/java/org/opensearch/ad/e2e/DetectionResultEvalutationIT.java index 832f3e215..17cea8b5e 100644 --- a/src/test/java/org/opensearch/ad/e2e/DetectionResultEvalutationIT.java +++ b/src/test/java/org/opensearch/ad/e2e/DetectionResultEvalutationIT.java @@ -336,7 +336,6 @@ public void testValidationIntervalRecommendation() throws Exception { long recDetectorIntervalMinutes = recDetectorIntervalMillis / 60000; List data = createData(2000, recDetectorIntervalMillis); indexTrainData("validation", data, 2000, client); - indexTestData(data, "validation", 2000, client); long detectorInterval = 1; String requestBody = String .format( @@ -401,17 +400,4 @@ private void indexTrainData(String datasetName, List data, int train }); Thread.sleep(1_000); } - - private void indexTestData(List data, String datasetName, int trainTestSplit, RestClient client) throws Exception { - data.stream().skip(trainTestSplit).forEach(r -> { - try { - Request req = new Request("POST", String.format("/%s/_doc/", datasetName)); - req.setJsonEntity(r.toString()); - client.performRequest(req); - } catch (Exception e) { - throw new RuntimeException(e); - } - }); - Thread.sleep(1_000); - } } From e926907eca9f77db43b1686db5106cc12b1c3a5a Mon Sep 17 00:00:00 2001 From: Amit Galitzky Date: Tue, 8 Mar 2022 22:27:12 +0000 Subject: [PATCH 12/13] made sure to return window delay rec if its the only issue apparent Signed-off-by: Amit Galitzky --- build.gradle | 1 + .../handler/ModelValidationActionHandler.java | 46 +++++++++------ .../ad/e2e/DetectionResultEvalutationIT.java | 38 +++++++++++++ .../ad/rest/AnomalyDetectorRestApiIT.java | 56 ++++++++----------- 4 files changed, 92 insertions(+), 49 deletions(-) diff --git a/build.gradle b/build.gradle index f4d353204..039c708d3 100644 --- a/build.gradle +++ b/build.gradle @@ -480,6 +480,7 @@ List jacocoExclusions = [ 'org.opensearch.ad.AnomalyDetectorPlugin', 'org.opensearch.ad.settings.AnomalyDetectorSettings', + //TODO: Add more cases for model validation API, both UT and IT //TODO: add more test cases later for these package 'org.opensearch.ad.model.*', 'org.opensearch.ad.rest.*', diff --git a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java index 673677542..01e050015 100644 --- a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java +++ b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java @@ -481,6 +481,11 @@ private void processIntervalRecommendation(IntervalTimeConfiguration interval, l } else { if (interval.equals(anomalyDetector.getDetectionInterval())) { logger.info("Using the current interval there is enough dense data "); + // Check if there is a window delay recommendation if everything else is successful and send exception + if (Instant.now().toEpochMilli() - latestTime > timeConfigToMilliSec(anomalyDetector.getWindowDelay())) { + sendWindowDelayRec(latestTime); + return; + } // The rate of buckets with at least 1 doc with given interval is above the success rate listener.onResponse(null); return; @@ -692,26 +697,33 @@ private void checkFeatureQuery(long latestTime) throws IOException { client.search(searchRequest, ActionListener.wrap(response -> processFeatureQuery(response, latestTime), listener::onFailure)); } + private void sendWindowDelayRec(long latestTime) { + long minutesSinceLastStamp = TimeUnit.MILLISECONDS.toMinutes(Instant.now().toEpochMilli() - latestTime); + listener + .onFailure( + new ADValidationException( + String.format(Locale.ROOT, CommonErrorMessages.WINDOW_DELAY_REC, minutesSinceLastStamp, minutesSinceLastStamp), + DetectorValidationIssueType.WINDOW_DELAY, + ValidationAspect.MODEL, + new IntervalTimeConfiguration(minutesSinceLastStamp, ChronoUnit.MINUTES) + ) + ); + } + private void windowDelayRecommendation(long latestTime) { - long delayMillis = timeConfigToMilliSec(anomalyDetector.getWindowDelay()); - if ((Instant.now().toEpochMilli() - latestTime > delayMillis)) { - long minutesSinceLastStamp = TimeUnit.MILLISECONDS.toMinutes(Instant.now().toEpochMilli() - latestTime); - listener - .onFailure( - new ADValidationException( - String.format(Locale.ROOT, CommonErrorMessages.WINDOW_DELAY_REC, minutesSinceLastStamp, minutesSinceLastStamp), - DetectorValidationIssueType.WINDOW_DELAY, - ValidationAspect.MODEL, - new IntervalTimeConfiguration(minutesSinceLastStamp, ChronoUnit.MINUTES) - ) - ); + // Check if there is a better window-delay to recommend and if one was recommended + // then send exception and return, otherwise continue to let user know data is too sparse as explained below + if (Instant.now().toEpochMilli() - latestTime > timeConfigToMilliSec(anomalyDetector.getWindowDelay())) { + sendWindowDelayRec(latestTime); return; } - // This case has been reached if no interval recommendation was found that leads to a bucket success rate of >= 0.75 - // but no single configuration during the following checks reduced the bucket success rate below 0.25 - // This means the rate with all configs applied was below 0.75 but the rate when checking each configuration at time - // was always above 0.25 meaning the best suggestion is to simply ingest more data since we have no more insight - // regarding the root cause of the lower density. + // This case has been reached if following conditions are met: + // 1. no interval recommendation was found that leads to a bucket success rate of >= 0.75 + // 2. bucket success rate with the given interval and just raw data is also below 0.75. + // 3. no single configuration during the following checks reduced the bucket success rate below 0.25 + // This means the rate with all configs applied or just raw data was below 0.75 but the rate when checking each configuration at + // a time was always above 0.25 meaning the best suggestion is to simply ingest more data or change interval since + // we have no more insight regarding the root cause of the lower density. listener .onFailure( new ADValidationException( diff --git a/src/test/java/org/opensearch/ad/e2e/DetectionResultEvalutationIT.java b/src/test/java/org/opensearch/ad/e2e/DetectionResultEvalutationIT.java index 17cea8b5e..50750749d 100644 --- a/src/test/java/org/opensearch/ad/e2e/DetectionResultEvalutationIT.java +++ b/src/test/java/org/opensearch/ad/e2e/DetectionResultEvalutationIT.java @@ -367,6 +367,44 @@ public void testValidationIntervalRecommendation() throws Exception { ); } + public void testValidationWindowDelayRecommendation() throws Exception { + RestClient client = client(); + long recDetectorIntervalMillis = 180000; + // this would be equivalent to the window delay in this data test + long recDetectorIntervalMinutes = recDetectorIntervalMillis / 60000; + List data = createData(2000, recDetectorIntervalMillis); + indexTrainData("validation", data, 2000, client); + long detectorInterval = 4; + String requestBody = String + .format( + Locale.ROOT, + "{ \"name\": \"test\", \"description\": \"test\", \"time_field\": \"timestamp\"" + + ", \"indices\": [\"validation\"], \"feature_attributes\": [{ \"feature_name\": \"feature 1\", \"feature_enabled\": " + + "\"true\", \"aggregation_query\": { \"Feature1\": { \"sum\": { \"field\": \"Feature1\" } } } }, { \"feature_name\"" + + ": \"feature 2\", \"feature_enabled\": \"true\", \"aggregation_query\": { \"Feature2\": { \"sum\": { \"field\": " + + "\"Feature2\" } } } }], \"detection_interval\": { \"period\": { \"interval\": %d, \"unit\": \"Minutes\" } }" + + ",\"window_delay\":{\"period\":{\"interval\":1,\"unit\":\"Minutes\"}}}", + detectorInterval + ); + Response resp = TestHelpers + .makeRequest( + client(), + "POST", + TestHelpers.AD_BASE_DETECTORS_URI + "/_validate/model", + ImmutableMap.of(), + toHttpEntity(requestBody), + null + ); + Map responseMap = entityAsMap(resp); + @SuppressWarnings("unchecked") + Map> messageMap = (Map>) XContentMapValues + .extractValue("model", responseMap); + assertEquals( + String.format(Locale.ROOT, CommonErrorMessages.WINDOW_DELAY_REC, +recDetectorIntervalMinutes, recDetectorIntervalMinutes), + messageMap.get("window_delay").get("message") + ); + } + private List createData(int numOfDataPoints, long detectorIntervalMS) { List list = new ArrayList<>(); for (int i = 1; i < numOfDataPoints; i++) { diff --git a/src/test/java/org/opensearch/ad/rest/AnomalyDetectorRestApiIT.java b/src/test/java/org/opensearch/ad/rest/AnomalyDetectorRestApiIT.java index f5c50a81f..f04c11b3b 100644 --- a/src/test/java/org/opensearch/ad/rest/AnomalyDetectorRestApiIT.java +++ b/src/test/java/org/opensearch/ad/rest/AnomalyDetectorRestApiIT.java @@ -110,16 +110,20 @@ public void testCreateAnomalyDetectorWithEmptyIndices() throws Exception { ); } - private AnomalyDetector createIndexAndGetAnomalyDetector() throws IOException { - TestHelpers.createIndexWithTimeField(client(), INDEX_NAME, TIME_FIELD); + private AnomalyDetector createIndexAndGetAnomalyDetector(String indexName) throws IOException { + return createIndexAndGetAnomalyDetector(indexName, ImmutableList.of(TestHelpers.randomFeature(true))); + } + + private AnomalyDetector createIndexAndGetAnomalyDetector(String indexName, List features) throws IOException { + TestHelpers.createIndexWithTimeField(client(), indexName, TIME_FIELD); String testIndexData = "{\"keyword-field\": \"field-1\", \"ip-field\": \"1.2.3.4\", \"timestamp\": 1}"; - TestHelpers.ingestDataToIndex(client(), INDEX_NAME, TestHelpers.toHttpEntity(testIndexData)); - AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TIME_FIELD, INDEX_NAME); + TestHelpers.ingestDataToIndex(client(), indexName, TestHelpers.toHttpEntity(testIndexData)); + AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TIME_FIELD, indexName, features); return detector; } public void testCreateAnomalyDetectorWithDuplicateName() throws Exception { - AnomalyDetector detector = createIndexAndGetAnomalyDetector(); + AnomalyDetector detector = createIndexAndGetAnomalyDetector(INDEX_NAME); AnomalyDetector detectorDuplicateName = new AnomalyDetector( AnomalyDetector.NO_ID, randomLong(), @@ -157,7 +161,7 @@ public void testCreateAnomalyDetectorWithDuplicateName() throws Exception { } public void testCreateAnomalyDetector() throws Exception { - AnomalyDetector detector = createIndexAndGetAnomalyDetector(); + AnomalyDetector detector = createIndexAndGetAnomalyDetector(INDEX_NAME); updateClusterSettings(EnabledSetting.AD_PLUGIN_ENABLED, false); Exception ex = expectThrows( @@ -186,7 +190,7 @@ public void testCreateAnomalyDetector() throws Exception { } public void testUpdateAnomalyDetectorCategoryField() throws Exception { - AnomalyDetector detector = createIndexAndGetAnomalyDetector(); + AnomalyDetector detector = createIndexAndGetAnomalyDetector(INDEX_NAME); Response response = TestHelpers .makeRequest(client(), "POST", TestHelpers.AD_BASE_DETECTORS_URI, ImmutableMap.of(), TestHelpers.toHttpEntity(detector), null); assertEquals("Create anomaly detector failed", RestStatus.CREATED, TestHelpers.restStatus(response)); @@ -246,7 +250,7 @@ public void testGetNotExistingAnomalyDetector() throws Exception { } public void testUpdateAnomalyDetector() throws Exception { - AnomalyDetector detector = createAnomalyDetector(createIndexAndGetAnomalyDetector(), true, client()); + AnomalyDetector detector = createAnomalyDetector(createIndexAndGetAnomalyDetector(INDEX_NAME), true, client()); String newDescription = randomAlphaOfLength(5); AnomalyDetector newDetector = new AnomalyDetector( detector.getDetectorId(), @@ -307,13 +311,8 @@ public void testUpdateAnomalyDetector() throws Exception { } public void testUpdateAnomalyDetectorNameToExisting() throws Exception { - AnomalyDetector detector1 = TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null); - String indexName1 = detector1.getIndices().get(0); - TestHelpers.createIndex(client(), indexName1, TestHelpers.toHttpEntity("{\"name\": \"test\"}")); - AnomalyDetector detector2 = TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null); - String indexName2 = detector2.getIndices().get(0); - TestHelpers.createIndex(client(), indexName2, TestHelpers.toHttpEntity("{\"name\": \"test\"}")); - + AnomalyDetector detector1 = createIndexAndGetAnomalyDetector("index-test-one"); + AnomalyDetector detector2 = createIndexAndGetAnomalyDetector("index-test-two"); AnomalyDetector newDetector1WithDetector2Name = new AnomalyDetector( detector1.getDetectorId(), detector1.getVersion(), @@ -351,7 +350,7 @@ public void testUpdateAnomalyDetectorNameToExisting() throws Exception { } public void testUpdateAnomalyDetectorNameToNew() throws Exception { - AnomalyDetector detector = createAnomalyDetector(createIndexAndGetAnomalyDetector(), true, client()); + AnomalyDetector detector = createAnomalyDetector(createIndexAndGetAnomalyDetector(INDEX_NAME), true, client()); AnomalyDetector detectorWithNewName = new AnomalyDetector( detector.getDetectorId(), detector.getVersion(), @@ -766,7 +765,7 @@ public void testDeleteAnomalyDetectorWithRunningAdJob() throws Exception { } public void testUpdateAnomalyDetectorWithRunningAdJob() throws Exception { - AnomalyDetector detector = createAnomalyDetector(createIndexAndGetAnomalyDetector(), true, client()); + AnomalyDetector detector = createAnomalyDetector(createIndexAndGetAnomalyDetector(INDEX_NAME), true, client()); Response startAdJobResponse = TestHelpers .makeRequest( client(), @@ -1196,7 +1195,7 @@ public void testDeleteAnomalyDetectorWhileRunning() throws Exception { public void testBackwardCompatibilityWithOpenDistro() throws IOException { // Create a detector - AnomalyDetector detector = createIndexAndGetAnomalyDetector(); + AnomalyDetector detector = createIndexAndGetAnomalyDetector(INDEX_NAME); // Verify the detector is created using legacy _opendistro API Response response = TestHelpers .makeRequest( @@ -1233,7 +1232,7 @@ public void testBackwardCompatibilityWithOpenDistro() throws IOException { } public void testValidateAnomalyDetectorWithDuplicateName() throws Exception { - AnomalyDetector detector = createAnomalyDetector(createIndexAndGetAnomalyDetector(), true, client()); + AnomalyDetector detector = createAnomalyDetector(createIndexAndGetAnomalyDetector(INDEX_NAME), true, client()); Response resp = TestHelpers .makeRequest( client(), @@ -1331,9 +1330,7 @@ public void testValidateAnomalyDetectorWithIncorrectShingleSize() throws Excepti } public void testValidateAnomalyDetectorWithNoIssue() throws Exception { - String indexName = "testindex"; - TestHelpers.createIndexWithTimeField(client(), "testindex", TIME_FIELD); - AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TIME_FIELD, indexName); + AnomalyDetector detector = createIndexAndGetAnomalyDetector(INDEX_NAME); Response resp = TestHelpers .makeRequest( client(), @@ -1348,9 +1345,7 @@ public void testValidateAnomalyDetectorWithNoIssue() throws Exception { } public void testValidateAnomalyDetectorOnWrongValidationType() throws Exception { - AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TestHelpers.randomUiMetadata(), null); - String indexName = detector.getIndices().get(0); - TestHelpers.createIndex(client(), indexName, TestHelpers.toHttpEntity("{\"timestamp\": " + Instant.now().toEpochMilli() + "}")); + AnomalyDetector detector = createIndexAndGetAnomalyDetector(INDEX_NAME); TestHelpers .assertFailWith( ResponseException.class, @@ -1359,7 +1354,7 @@ public void testValidateAnomalyDetectorOnWrongValidationType() throws Exception .makeRequest( client(), "POST", - TestHelpers.AD_BASE_DETECTORS_URI + "/_validate/model", + TestHelpers.AD_BASE_DETECTORS_URI + "/_validate/models", ImmutableMap.of(), TestHelpers.toHttpEntity(detector), null @@ -1429,8 +1424,7 @@ public void testValidateAnomalyDetectorWithInvalidName() throws Exception { public void testValidateAnomalyDetectorWithFeatureQueryReturningNoData() throws Exception { Feature emptyFeature = TestHelpers.randomFeature("f-empty", "cpu", "avg", true); - AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TIME_FIELD, "index-test", ImmutableList.of(emptyFeature)); - TestHelpers.createIndexWithTimeField(client(), "index-test", TIME_FIELD); + AnomalyDetector detector = createIndexAndGetAnomalyDetector(INDEX_NAME, ImmutableList.of(emptyFeature)); Response resp = TestHelpers .makeRequest( client(), @@ -1452,10 +1446,8 @@ public void testValidateAnomalyDetectorWithFeatureQueryReturningNoData() throws } public void testValidateAnomalyDetectorWithFeatureQueryRuntimeException() throws Exception { - String nonNumericField = "_type"; - Feature nonNumericFeature = TestHelpers.randomFeature("non-numeric-feature", nonNumericField, "avg", true); - AnomalyDetector detector = TestHelpers.randomAnomalyDetector(TIME_FIELD, "index-test", ImmutableList.of(nonNumericFeature)); - TestHelpers.createIndexWithTimeField(client(), "index-test", TIME_FIELD); + Feature nonNumericFeature = TestHelpers.randomFeature("non-numeric-feature", "_type", "avg", true); + AnomalyDetector detector = createIndexAndGetAnomalyDetector(INDEX_NAME, ImmutableList.of(nonNumericFeature)); Response resp = TestHelpers .makeRequest( client(), From 45a00381d1dc700e120de14d3d7ca83fcea5716d Mon Sep 17 00:00:00 2001 From: Amit Galitzky Date: Tue, 8 Mar 2022 23:59:55 +0000 Subject: [PATCH 13/13] fixed rounding error in window delay so it always rounds up Signed-off-by: Amit Galitzky --- .../ad/rest/handler/ModelValidationActionHandler.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java index 01e050015..1e0cda02c 100644 --- a/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java +++ b/src/main/java/org/opensearch/ad/rest/handler/ModelValidationActionHandler.java @@ -25,7 +25,6 @@ import java.util.Locale; import java.util.Map; import java.util.Optional; -import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import org.apache.logging.log4j.LogManager; @@ -697,8 +696,8 @@ private void checkFeatureQuery(long latestTime) throws IOException { client.search(searchRequest, ActionListener.wrap(response -> processFeatureQuery(response, latestTime), listener::onFailure)); } - private void sendWindowDelayRec(long latestTime) { - long minutesSinceLastStamp = TimeUnit.MILLISECONDS.toMinutes(Instant.now().toEpochMilli() - latestTime); + private void sendWindowDelayRec(long latestTimeInMillis) { + long minutesSinceLastStamp = (long) Math.ceil((Instant.now().toEpochMilli() - latestTimeInMillis) / 60000.0); listener .onFailure( new ADValidationException(