Verifying multi-entity detectors #240

kaituo · 2020-10-07T19:19:11Z

Issue #, if available:

Description of changes:

This PR adds categorical fields' number and type check. We only support one categorical field, and the categorical field can only be of type keyword and ip. We also limit the max multi-entity detectors to 10.

Testing done:

added unit tests
did manual testing.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov · 2020-10-07T19:21:06Z

Codecov Report

Merging #240 into master will increase coverage by 1.14%.
The diff coverage is 69.53%.

@@             Coverage Diff              @@
##             master     #240      +/-   ##
============================================
+ Coverage     71.70%   72.84%   +1.14%     
- Complexity     1367     1425      +58     
============================================
  Files           157      160       +3     
  Lines          6513     6680     +167     
  Branches        493      508      +15     
============================================
+ Hits           4670     4866     +196     
+ Misses         1610     1567      -43     
- Partials        233      247      +14

Flag	Coverage Δ	Complexity Δ
#cli	`79.27% <ø> (ø)`	`0.00 <ø> (ø)`
#plugin	`72.06% <69.53%> (+1.30%)`	`1425.00 <26.00> (+58.00)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...stroforelasticsearch/ad/AnomalyDetectorPlugin.java	`93.54% <ø> (ø)`	`10.00 <0.00> (ø)`
...distroforelasticsearch/ad/constant/CommonName.java	`66.66% <ø> (ø)`	`1.00 <0.00> (ø)`
...arch/ad/transport/IndexAnomalyDetectorRequest.java	`46.80% <50.00%> (+1.64%)`	`11.00 <4.00> (+4.00)`
...stroforelasticsearch/ad/model/AnomalyDetector.java	`64.02% <56.52%> (-2.26%)`	`51.00 <6.00> (+5.00)`	⬇️
...est/handler/IndexAnomalyDetectorActionHandler.java	`51.41% <73.23%> (+35.93%)`	`26.00 <14.00> (+24.00)`
...transport/IndexAnomalyDetectorTransportAction.java	`92.00% <83.33%> (+0.33%)`	`3.00 <1.00> (+1.00)`
...search/ad/rest/RestIndexAnomalyDetectorAction.java	`46.93% <88.88%> (+4.38%)`	`3.00 <0.00> (ø)`
...ticsearch/ad/settings/AnomalyDetectorSettings.java	`100.00% <100.00%> (ø)`	`1.00 <1.00> (ø)`
...ransport/DeleteAnomalyDetectorTransportAction.java	`59.03% <0.00%> (-2.41%)`	`16.00% <0.00%> (ø%)`
... and 12 more

ylwu-amzn · 2020-10-09T04:30:15Z

...com/amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java

+    }
+
+    /**
+     * Precondition: anomalyDetector.getCategoryField() != null.


What does this comment mean?

not true anymore. Removed.

ylwu-amzn · 2020-10-09T04:33:19Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyDetector.java

+        if (shingleSize != null && shingleSize < 1) {
+            throw new IllegalArgumentException("Shingle size must be a positive integer");
+        }
+        if (categoryField != null && categoryField.size() > 1) {


1 is magic number, how about we create CATEGORY_FIELD_LIMIT instant and use it at all other places. like IndexAnomalyDetectorActionHandler.java line 269:
if (categoryField.size() != 1) {

+1 I like the simpler check.

ylwu-amzn · 2020-10-09T04:36:20Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyDetector.java

@@ -87,6 +88,7 @@
    private final Map<String, Object> uiMetadata;
    private final Integer schemaVersion;
    private final Instant lastUpdateTime;
+    private final List<String> categoryField;


minor: if we plan to support multiple category fields, should we name this as "categoryFields" ?

yes, changed

saratvemulapalli

Overall the changes look good to me.

saratvemulapalli · 2020-10-12T17:10:28Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyDetector.java

@@ -87,6 +88,7 @@
    private final Map<String, Object> uiMetadata;
    private final Integer schemaVersion;
    private final Instant lastUpdateTime;
+    private final List<String> categoryField;


I see we just use 1 category field today, do we see possible use cases of multiple category fields ?

yes, maybe in the future

ylwu-amzn · 2020-10-12T19:39:47Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/constant/CommonName.java

@@ -22,6 +22,9 @@
    // index name for anomaly checkpoint of each model. One model one document.
    public static final String CHECKPOINT_INDEX_NAME = ".opendistro-anomaly-checkpoints";

+    // The alias of the index in which to write single-entity AD result history


Do we plan to write HC detector's result into another index?

not in the plan. The code would have to change a lot for that to happen. e.g., depends on there is categorical field or not, job scheduler will save to different places. Right now, job scheduler does not have access to AnomalyDetector object.

Got it, how about remove the "single-entity" from the comments?

ylwu-amzn · 2020-10-12T19:40:22Z

src/main/java/com/amazon/opendistroforelasticsearch/ad/model/AnomalyDetector.java

@@ -72,6 +75,7 @@
    private static final String SHINGLE_SIZE_FIELD = "shingle_size";
    private static final String LAST_UPDATE_TIME_FIELD = "last_update_time";
    public static final String UI_METADATA_FIELD = "ui_metadata";
+    public static final String CATEGORY_FIELD = "category_field";


Should we change to "category_fields" ? Same question for line 113, 130, and other places. Suggest to replace by searching all files.

Can we do it later? Both Yizhe and I need to change for this.

Sure, I'm ok

Thanks so much for your consideration.

ylwu-amzn

LGTM. Thanks for the change

saratvemulapalli

Changes look good to me.

This PR adds categorical fields' number and length check. We only support one categorical field, and the categorical field can only be of type keyword and ip. We also limit the max multi-entity detectors to 10. Testing done: 1. added unit tests 2. did manual testing.

kaituo requested review from ylwu-amzn and saratvemulapalli October 7, 2020 19:19

kaituo force-pushed the validateType branch from dce7469 to 33ede2c Compare October 8, 2020 16:18

kaituo changed the title ~~Verifying Categorical field's type and total number~~ Verifying multi-entity detectors Oct 8, 2020

ylwu-amzn reviewed Oct 9, 2020

View reviewed changes

ylwu-amzn mentioned this pull request Oct 9, 2020

Selectively store anomaly results when index pressure is high #241

Merged

saratvemulapalli reviewed Oct 12, 2020

View reviewed changes

ylwu-amzn reviewed Oct 12, 2020

View reviewed changes

ylwu-amzn approved these changes Oct 13, 2020

View reviewed changes

saratvemulapalli approved these changes Oct 13, 2020

View reviewed changes

kaituo added 4 commits October 13, 2020 14:01

Address comments

b278721

Fix flaky test

99c5a44

Merge from Sarat

ff37fd3

kaituo force-pushed the validateType branch from febd0a5 to ff37fd3 Compare October 13, 2020 21:12

kaituo merged commit 85e38ac into opendistro-for-elasticsearch:master Oct 13, 2020

ohltyler added the enhancement New feature or request label Oct 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verifying multi-entity detectors #240

Verifying multi-entity detectors #240

kaituo commented Oct 7, 2020 •

edited

Loading

codecov bot commented Oct 7, 2020 •

edited

Loading

ylwu-amzn Oct 9, 2020

kaituo Oct 12, 2020

ylwu-amzn Oct 9, 2020

saratvemulapalli Oct 12, 2020

kaituo Oct 12, 2020

ylwu-amzn Oct 9, 2020 •

edited

Loading

kaituo Oct 12, 2020

saratvemulapalli left a comment

saratvemulapalli Oct 12, 2020

kaituo Oct 12, 2020

ylwu-amzn Oct 12, 2020

kaituo Oct 12, 2020

ylwu-amzn Oct 12, 2020

kaituo Oct 13, 2020

ylwu-amzn Oct 12, 2020 •

edited

Loading

kaituo Oct 12, 2020

ylwu-amzn Oct 13, 2020

kaituo Oct 13, 2020

ylwu-amzn left a comment

saratvemulapalli left a comment

Verifying multi-entity detectors #240

Verifying multi-entity detectors #240

Conversation

kaituo commented Oct 7, 2020 • edited Loading

codecov bot commented Oct 7, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylwu-amzn Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saratvemulapalli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylwu-amzn Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylwu-amzn left a comment

Choose a reason for hiding this comment

saratvemulapalli left a comment

Choose a reason for hiding this comment

kaituo commented Oct 7, 2020 •

edited

Loading

codecov bot commented Oct 7, 2020 •

edited

Loading

ylwu-amzn Oct 9, 2020 •

edited

Loading

ylwu-amzn Oct 12, 2020 •

edited

Loading