Modify Entity to support multi-category field #78

kaituo · 2021-05-26T22:10:15Z

Note: since there are a lot of dependencies, I only list the main class and test code to save reviewers' time. The build will fail due to missing dependencies. I will use that PR just for review. will not merge it. Will have a big one in the end and merge once after all review PRs get approved. Now the code is missing unit tests. Will add unit tests, run performance tests, and fix bugs before the official release.

Description

This PR modified Entity to include multiple categorical fields. One key change is to not use categorical values directly in the model document Id. The problem is that HCAD v1 uses the categorical value as part of the model document Id, but ES’s document Id can be at most 512 bytes. Categorical values are usually less than 256 characters, but can grow to 32766 in theory. HCAD v1 skips an entity if the entity's name is more than 256 characters. We cannot do that in v2 as that can reject a lot of entities. To overcome the obstacle, we hash categorical values to a 128-bit string (like SHA-1 that git uses) and use the hash as part of the model document Id.

We have choices to make regarding when to use the hash as part of a model document Id: for all HC detectors or a HC detector with multiple categorical fields. The challenge lies in providing backward compatibility of looking for a model checkpoint in the case of a HC detector with one categorical field. If using hashes for all HC detectors, we need two get requests to ensure that a model checkpoint exists. One uses the document Id without a hash, while one uses the document Id with a hash. The dual get requests are ineffective. If limiting hashes to a HC detector with multiple categorical fields, there is no backward compatibility issue. However, the code will be branchy. One may wonder if backward compatibility can be ignored; indeed, the old checkpoints will be gone after a transition period during upgrading. During the transition period, HC detectors can experience unnecessary cold starts as if the detectors were just started. Checkpoint index size can double if every model has two model documents. The transition period can be as long as 3 days since our checkpoint retention period is 3 days. There is no perfect solution. We prefer limiting hashes to an HC detector with multiple categorical fields as its customer impact is none.

Testing done:

Manually tested the model id of multi-category entity works, and the single-category entity does not break.

Check List

[ Y ] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

This PR modified Entity to include multiple categorical fields. One key change is to not use categorical values directly in the model document Id. The problem is that HCAD v1 uses the categorical value as part of the model document Id, but ES’s document Id can be at most 512 bytes. Categorical values are usually less than 256 characters, but can grow to 32766 in theory. HCAD v1 skips an entity if the entity's name is more than 256 characters. We cannot do that in v2 as that can reject a lot of entities. To overcome the obstacle, we hash categorical values to a 128-bit string (like SHA-1 that git uses) and use the hash as part of the model document Id. We have choices to make regarding when to use the hash as part of a model document Id: for all HC detectors or a HC detector with multiple categorical fields. The challenge lies in providing backward compatibility of looking for a model checkpoint in the case of a HC detector with one categorical field. If using hashes for all HC detectors, we need two get requests to ensure that a model checkpoint exists. One uses the document Id without a hash, while one uses the document Id with a hash. The dual get requests are ineffective. If limiting hashes to a HC detector with multiple categorical fields, there is no backward compatibility issue. However, the code will be branchy. One may wonder if backward compatibility can be ignored; indeed, the old checkpoints will be gone after a transition period during upgrading. During the transition period, HC detectors can experience unnecessary cold starts as if the detectors were just started. Checkpoint index size can double if every model has two model documents. The transition period can be as long as 3 days since our checkpoint retention period is 3 days. There is no perfect solution. We prefer limiting hashes to an HC detector with multiple categorical fields as its customer impact is none. Testing done: 1. Manually tested the model id of multi-category entity works, and the single-category entity does not break.

src/main/java/org/opensearch/ad/model/Entity.java

src/main/java/org/opensearch/ad/transport/GetAnomalyDetectorRequest.java

src/main/java/org/opensearch/ad/transport/GetAnomalyDetectorTransportAction.java

src/test/java/org/opensearch/ad/TestHelpers.java

ohltyler

LGTM

ylwu-amzn · 2021-06-16T04:58:32Z

src/main/java/org/opensearch/ad/AnomalyDetectorRunner.java

@@ -136,14 +144,17 @@ private void onFailure(Exception e, ActionListener<List<AnomalyResult>> listener
        // TODO return exception like IllegalArgumentException to explain data is not enough for preview
        // This also requires front-end change to handle error message correspondingly
        // We return empty list for now to avoid breaking front-end
+        if (e instanceof OpenSearchSecurityException) {
+            listener.onFailure(e);
+        }


If exception is OpenSearchSecurityException, will call onFailure first, then call onResponse, it will throw connection closed exception.

Add return in this if block, or put listener.onResponse(Collections.emptyList()); in else block.

yeah, fixed this one and another similar bug.

ylwu-amzn

LGTM

This PR is a conglomerate of the following PRs. #60 #64 #65 #67 #68 #69 #70 #71 #74 #75 #76 #77 #78 #79 #82 #83 #84 #92 #94 #93 #95 kaituo#1 kaituo#2 kaituo#3 kaituo#4 kaituo#5 kaituo#6 kaituo#7 kaituo#8 kaituo#9 kaituo#10 This spreadsheet contains the mappings from files to PR number (bug fix in my AD fork and tests are not included): https://gist.github.com/kaituo/9e1592c4ac4f2f449356cb93d0591167

This PR is a conglomerate of the following PRs. #60 #64 #65 #67 #68 #69 #70 #71 #74 #75 #76 #77 #78 #79 #82 #83 #84 #92 #94 #93 #95 kaituo#1 kaituo#2 kaituo#3 kaituo#4 kaituo#5 kaituo#6 kaituo#7 kaituo#8 kaituo#9 kaituo#10 This spreadsheet contains the mappings from files to PR number (bug fix in my AD fork and tests are not included): https://gist.github.com/kaituo/9e1592c4ac4f2f449356cb93d0591167

kaituo requested review from ylwu-amzn and ohltyler May 26, 2021 22:11