Add model id in anomaly result. #75
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The front end needs to query for entities ordered by the descending order of anomaly grades and the number of anomalies. After supporting multi-category fields, it is hard to write such queries since the entity information is stored in a nested object array. Also, the front end has all code/queries/ helper functions in place to rely on a single key per entity combo. This PR adds model id to anomaly result to help the transition to multi-categorical field less painful. Testing done: 1. Tested HCAD results have model id now.
ohltyler
reviewed
Jun 7, 2021
| @@ -597,7 +597,8 @@ private void detectAnomaly( | |||
| error, | |||
| null, | |||
| adTask.getDetector().getUser(), | |||
| anomalyDetectionIndices.getSchemaVersion(ADIndex.RESULT) | |||
| anomalyDetectionIndices.getSchemaVersion(ADIndex.RESULT), | |||
| null | |||
There was a problem hiding this comment.
Why set to null here, can the model id not be pulled from the rcf variable? Where is the model_id actually added into the results exactly?
There was a problem hiding this comment.
^ I may have answered my own question - is this because this is related to historical detector results and model_id is not needed in that case? If so, how do we plan on handling the future case of historical HC detectors?
There was a problem hiding this comment.
yes, historical detector has no model id, bu task id. Adding a model id may not be a good design as
- historical detector does not use it. It uses task id. Front-end needs two sets of logic for real-time and historical detectors.
- it increases the size of result index doc.
We may be better off using the new result search API. The only issue is the timeline. We may not be able to finish appsec before 1.1 release. Any suggestion?
There was a problem hiding this comment.
I see. Makes sense to not include it, and we can use task_id instead of model_id for aggregating HC historical results, if the new result search API is still not ready. Thanks for the clarification
There was a problem hiding this comment.
For historical results, we just store detector level task id in AD result index. I think we need to store model id in historical result as well to search entity's results.
ohltyler
approved these changes
Jun 7, 2021
kaituo
added a commit
to kaituo/anomaly-detection-1
that referenced
this pull request
Jun 11, 2021
First, I threw an exception instead of returning the exception through listener callback in CheckpointDao. This can cause semaphores not released. Second, I didn’t check AnomalyResult.getAnomalyGrade() returns null or not, which can cause a null pointer exception. Also, please ignore model id related changes in AnomalyResult. The changes are included in a separate PR: opensearch-project#75 Testing done: 1. Wrote unit tests for the above errors.
kaituo
added a commit
that referenced
this pull request
Jul 12, 2021
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
ohltyler
pushed a commit
to ohltyler/anomaly-detection-2
that referenced
this pull request
Sep 1, 2021
…ject#121) This PR is a conglomerate of the following PRs. opensearch-project#60 opensearch-project#64 opensearch-project#65 opensearch-project#67 opensearch-project#68 opensearch-project#69 opensearch-project#70 opensearch-project#71 opensearch-project#74 opensearch-project#75 opensearch-project#76 opensearch-project#77 opensearch-project#78 opensearch-project#79 opensearch-project#82 opensearch-project#83 opensearch-project#84 opensearch-project#92 opensearch-project#94 opensearch-project#93 opensearch-project#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
ohltyler
pushed a commit
to ohltyler/anomaly-detection-2
that referenced
this pull request
Sep 1, 2021
…ject#121) This PR is a conglomerate of the following PRs. opensearch-project#60 opensearch-project#64 opensearch-project#65 opensearch-project#67 opensearch-project#68 opensearch-project#69 opensearch-project#70 opensearch-project#71 opensearch-project#74 opensearch-project#75 opensearch-project#76 opensearch-project#77 opensearch-project#78 opensearch-project#79 opensearch-project#82 opensearch-project#83 opensearch-project#84 opensearch-project#92 opensearch-project#94 opensearch-project#93 opensearch-project#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
ohltyler
pushed a commit
that referenced
this pull request
Sep 1, 2021
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
The front end needs to query for entities ordered by the descending order of anomaly grades and the number of anomalies. After supporting multi-category fields, it is hard to write such queries since the entity information is stored in a nested object array. Also, the front end has all code/queries/ helper functions in place to rely on a single key per entity combo. This PR adds model id to anomaly result to help the transition to multi-categorical field less painful.
Testing done:
Check List
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.