Improve preview and exception handling in AnomalyResultTransportAction #128
Conversation
Codecov Report
@@ Coverage Diff @@
## main #128 +/- ##
============================================
+ Coverage 75.97% 76.89% +0.91%
- Complexity 2886 2902 +16
============================================
Files 264 263 -1
Lines 12435 12494 +59
Branches 1222 1223 +1
============================================
+ Hits 9448 9607 +159
+ Misses 2478 2369 -109
- Partials 509 518 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -103,7 +102,8 @@ | |||
| private final Interpolator interpolator; | |||
| private final ClientUtil clientUtil; | |||
| private int maxEntitiesForPreview; | |||
| private final Gson gson; | |||
| private int pageSize; | |||
There was a problem hiding this comment.
added. Just curious: did you see any bug/tickets related to not having volatile keyword?
There was a problem hiding this comment.
No, I always use volatile before fields which shared by multiple threads for system settings.
| @@ -182,6 +185,11 @@ public void getLatestDataTime(AnomalyDetector detector, ActionListener<Optional< | |||
| * @param listener listener to return back the entities | |||
| */ | |||
| public void getHighestCountEntities(AnomalyDetector detector, long startTime, long endTime, ActionListener<List<Entity>> listener) { | |||
| if (detector.getCategoryField() == null || detector.getCategoryField().isEmpty()) { | |||
There was a problem hiding this comment.
Use detector.isMultientityDetector() ?
| * in the natural order of composite keys. This is fine for Preview API. Preview API needs the | ||
| * top entities to make sure there is enough data for training and showing the results. We | ||
| * can paginate entities and filter out entities that do not have enough docs (e.g., 256 docs). | ||
| * As long as we have collected the desired number of entities (e.g., 5 entities), we can stop |
There was a problem hiding this comment.
How about add max pagination limit to avoid scanning all entities if no entity has enough docs?
There was a problem hiding this comment.
how about adding 1 minute expiration time as preview is time-sensitive? If we can finish within time, scanning all entities are acceptable.
There was a problem hiding this comment.
please check the upcoming commit
src/main/java/org/opensearch/ad/transport/PreviewAnomalyDetectorTransportAction.java
Show resolved
Hide resolved
src/main/java/org/opensearch/ad/transport/AnomalyResultTransportAction.java
Show resolved
Hide resolved
| topEntities.add(pageResults.get(i)); | ||
| } | ||
| Map<String, Object> afterKey = compositeAgg.afterKey(); | ||
| if (topEntities.size() >= maxEntitiesForPreview || afterKey == null || expirationEpochMs < clock.millis()) { |
There was a problem hiding this comment.
If timeout, should we use onFailure to show timeout warning on frontend ?
There was a problem hiding this comment.
how about if timeout and if there are entities found, log the timeout and send whatever we have? And if timeout and if there are no entities found, send failure as you said?
There was a problem hiding this comment.
That makes sense. We should only send out timeout failure if no entities found.
There was a problem hiding this comment.
changed. Please check the commit.
opensearch-project#128) * Improve preview and exception handling in AnomalyResultTransportAction This PR commits changes from the following PRs: kaituo#11 kaituo#12
opensearch-project#128) * Improve preview and exception handling in AnomalyResultTransportAction This PR commits changes from the following PRs: kaituo#11 kaituo#12
Signed-off-by: Kaituo Li kaituo@amazon.com
Description
This PR commits changes from the following PRs:
kaituo#11
kaituo#12
Issues Resolved
#129
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.