add anomaly localization implementation #103

wnbts · 2021-12-06T21:19:44Z

Description

This change contains the implementation of the core anomaly localization solution. It does not have integration with ml-commons, which will be in separate pr from after this.

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
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.

.../main/java/org/opensearch/ml/engine/algorithms/anomalylocalization/AnomalyLocalizerImpl.java

ylwu-amzn · 2021-12-07T20:35:00Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/anomalylocalization/Input.java

+ * Information about aggregate, time, etc to localize.
+ */
+@Data
+public class Input {


How about renaming this class as AnomalyLocalizationInput and implementing org.opensearch.ml.common.parameter.Input?

We should put input/output in common package.

Same comment for org.opensearch.ml.engine.algorithms.anomalylocalization.Output

Implementing the input/output/executor interface, and all integration for ml-common framework, will be in a separate pr following all steps for integration.

The input and output are specific to this solution, thus in the package specifically named with the solution. I am not sure it should be separate from its own module/package and be moved to a common package as it, unlike generic Input/Output interfaces, is not common to other solutions.

ylwu-amzn · 2021-12-07T20:45:27Z

...hms/src/main/java/org/opensearch/ml/engine/algorithms/anomalylocalization/HybridCounter.java

+
+    protected static int SKETCH_THRESHOLD = 10_000;
+
+    private Counter counter = new Hashmap();


How about renaming class Hashmap as HashMapCounter or ExactCounter ? Hashmap looks confusing as java has HashMap

i renamed it to HashMapCounter

ylwu-amzn · 2021-12-07T21:47:48Z

...hms/src/main/java/org/opensearch/ml/engine/algorithms/anomalylocalization/HybridCounter.java

+            Map<List<String>, Double> hashmap = ((Hashmap) counter).getKeyValues();
+            boolean hasNegative = hashmap.values().stream().anyMatch(v -> v < 0);
+            Counter newCounter;
+            if (hasNegative) {


Can you add some comments to explain when the count will be negative ?

i added a comment that says the aggregate value for a key, say average(value), can be negative.

jackiehanyang · 2021-12-07T21:27:20Z

.../main/java/org/opensearch/ml/engine/algorithms/anomalylocalization/AnomalyLocalizerImpl.java

+     */
+    @Override
+    @SneakyThrows
+    public void getLocalizationResults(Input input, ActionListener<Output> listener) {


nit: add final keyword whenever it's applicable.
e.g.,(final Input input, final ActionListener<Output> listener)

i would suggest no for a few reasons.

it doesn't improve code readability, just imagine lots of final keywords in almost every line of code. good implementation is correct and the way to verify is via testing, (think of other general programming languages such as javascript, python). specific language constructs help, more general better approach is static code analysis and code style checks and other automated tools (such as code guru).

There are abundant examples or convention in opensearch.
https://github.com/opensearch-project/OpenSearch/blob/996d33adb22ac6a523962166f3677f8dc6ac9c1f/server/src/main/java/org/opensearch/action/search/SearchTransportService.java#L109
https://github.com/opensearch-project/OpenSearch/blob/996d33adb22ac6a523962166f3677f8dc6ac9c1f/server/src/main/java/org/opensearch/search/internal/ReaderContext.java#L60

jackiehanyang · 2021-12-07T21:30:16Z

.../main/java/org/opensearch/ml/engine/algorithms/anomalylocalization/AnomalyLocalizerImpl.java

+    }
+
+    private boolean isResultComplete(Output.Result result) {
+        return result.getBuckets().stream().allMatch(e -> e.getCompleted() == null || e.getCompleted().get() == true);


curiosity question: why we consider the result is complete when getCompleted() filed is null?

added a comment that when a bucket does not need localization, such as the base bucket or the bucket has no change over the base bucket.

jackiehanyang · 2021-12-07T21:42:43Z

...hms/src/main/java/org/opensearch/ml/engine/algorithms/anomalylocalization/HybridCounter.java

+
+    protected static int SKETCH_THRESHOLD = 10_000;
+
+    private Counter counter = new Hashmap();


ylwu-amzn · 2021-12-07T21:55:41Z

...ithms/src/main/java/org/opensearch/ml/engine/algorithms/anomalylocalization/CountSketch.java

+import lombok.extern.log4j.Log4j2;
+
+/**
+ * Count sketch implementation.


How about adding more description or some link of Sketch, so others can learn how sketch works?

i added links to the references in the java doc.

ylwu-amzn · 2021-12-07T22:00:45Z

...ithms/src/main/java/org/opensearch/ml/engine/algorithms/anomalylocalization/CountSketch.java

+    /**
+     * Constructor.
+     */
+    public CountSketch() {


CountMinSketch and CountSketch have some duplicate code. Is it possible to refactor to remove the duplicate, like adding one filed to indicate whether the CountSketch is min or not.

the two algorithms share some similarity but not quite same. there is not too much to refactor from/to, which would however make the algorithms much less clear to understand.

.../main/java/org/opensearch/ml/engine/algorithms/anomalylocalization/AnomalyLocalizerImpl.java

ylwu-amzn · 2021-12-07T22:16:27Z

.../main/java/org/opensearch/ml/engine/algorithms/anomalylocalization/AnomalyLocalizerImpl.java

+            long end = Math.max(input.getEndTime(), anomalyStart + input.getMinTimeInterval());
+            int numBuckets = Math.min((int) ((end - anomalyStart) / input.getMinTimeInterval()), MAX_TIME_BUCKETS - 1);
+            long bucketInterval = (end - anomalyStart) / numBuckets;
+            long start = Math.min(input.getStartTime(), anomalyStart - bucketInterval);


Should we move this line before line 335 and calculate numBuckets as

Math.min((int) ((end - start) / input.getMinTimeInterval()), MAX_TIME_BUCKETS - 1);

it should be anomalyStart because localization is aimed at anomalous time, which is after the given anomalyStart, thus anomaly localization. the start is used for the base bucket.

ylwu-amzn · 2021-12-07T22:21:16Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/anomalylocalization/Input.java

+@Data
+public class Input {
+
+    private final String indexName; // name of the data index


Do we plan to support index pattern like abc* ?

FromAnomalyLocalizerImpl, index name used in search request directly, so index pattern is ok. Maybe change the comment to show index pattern also supported?

ylwu-amzn · 2021-12-07T22:58:13Z

.../main/java/org/opensearch/ml/engine/algorithms/anomalylocalization/AnomalyLocalizerImpl.java

+    /**
+     * Implementation of localization.
+     * <p>
+     * This localization finds the largest contributors to changes in aggregated data over time.


How about add more javadoc for these key steps in different methods?

yes, i added javadocs to the submethods. let me know if some important ones are missing or unclear.

ylwu-amzn · 2021-12-09T02:00:34Z

How about keeping each commit in PR stage? So PR reviewer can see what changed between commits easily rather than reread the whole PR.

wnbts · 2021-12-09T17:40:20Z

How about keeping each commit in PR stage? So PR reviewer can see what changed between commits easily rather than reread the whole PR.

see the second commit for update

ylwu-amzn

LGTM. Thanks for the change!

Signed-off-by: lai <57818076+wnbts@users.noreply.github.com>

wnbts requested a review from a team December 6, 2021 21:19

ylwu-amzn reviewed Dec 7, 2021

View reviewed changes

jackiehanyang reviewed Dec 7, 2021

View reviewed changes

ylwu-amzn reviewed Dec 7, 2021

View reviewed changes

.../main/java/org/opensearch/ml/engine/algorithms/anomalylocalization/AnomalyLocalizerImpl.java Show resolved Hide resolved

ylwu-amzn reviewed Dec 7, 2021

View reviewed changes

ylwu-amzn previously approved these changes Dec 15, 2021

View reviewed changes

jackiehanyang previously approved these changes Dec 30, 2021

View reviewed changes

wnbts added 2 commits December 30, 2021 14:20

add anomaly localization implementation

99a7749

Signed-off-by: lai <57818076+wnbts@users.noreply.github.com>

add anomaly localization implementation

e99f6aa

Signed-off-by: lai <57818076+wnbts@users.noreply.github.com>

wnbts dismissed stale reviews from jackiehanyang and ylwu-amzn via e99f6aa December 30, 2021 22:21

wnbts merged commit 0b36612 into opensearch-project:main Dec 30, 2021

ylwu-amzn added the feature label Mar 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add anomaly localization implementation #103

add anomaly localization implementation #103

wnbts commented Dec 6, 2021

ylwu-amzn Dec 7, 2021

wnbts Dec 7, 2021

ylwu-amzn Dec 7, 2021

jackiehanyang Dec 7, 2021

wnbts Dec 9, 2021

ylwu-amzn Dec 7, 2021

wnbts Dec 9, 2021

jackiehanyang Dec 7, 2021

wnbts Dec 9, 2021

jackiehanyang Dec 7, 2021

wnbts Dec 9, 2021

jackiehanyang Dec 7, 2021

ylwu-amzn Dec 7, 2021

wnbts Dec 9, 2021

ylwu-amzn Dec 7, 2021

wnbts Dec 9, 2021

ylwu-amzn Dec 7, 2021

wnbts Dec 9, 2021

ylwu-amzn Dec 7, 2021

ylwu-amzn Dec 7, 2021 •

edited

Loading

ylwu-amzn Dec 7, 2021

wnbts Dec 9, 2021

ylwu-amzn commented Dec 9, 2021

wnbts commented Dec 9, 2021

ylwu-amzn left a comment


		protected static int SKETCH_THRESHOLD = 10_000;

		private Counter counter = new Hashmap();

add anomaly localization implementation #103

add anomaly localization implementation #103

Conversation

wnbts commented Dec 6, 2021

Description

Check List

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

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

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 Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylwu-amzn commented Dec 9, 2021

wnbts commented Dec 9, 2021

ylwu-amzn left a comment

Choose a reason for hiding this comment

ylwu-amzn Dec 7, 2021 •

edited

Loading