Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Fix edge case where entities found for preview is empty #296

Merged
merged 2 commits into from
Oct 29, 2020
Merged

Fix edge case where entities found for preview is empty #296

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a06ebb
Fix edge case where entities found for preview is emtpy
yizheliu-amazon Oct 27, 2020
3dc9376
Add TODO to further improve reponse of Preview API and error handling…
yizheliu-amazon Oct 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/main/java/com/amazon/opendistroforelasticsearch/ad/AnomalyDetectorRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@ public void executeDetector(AnomalyDetector detector, Instant startTime, Instant
if (categoryField != null && !categoryField.isEmpty()) {
featureManager.getPreviewEntities(detector, startTime.toEpochMilli(), endTime.toEpochMilli(), ActionListener.wrap(entities -> {

if (entities == null || entities.isEmpty()) {
// 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
listener.onResponse(Collections.emptyList());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we return empty results or some failure like this https://github.com/opendistro-for-elasticsearch/anomaly-detection/blob/master/src/main/java/com/amazon/opendistroforelasticsearch/ad/feature/FeatureManager.java#L525

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such failure will be caught and empty result is still returned instead in onFailure()

I think it is okay to just return empty result

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you tell me why it would end up calling onFailure? listener is the rest channel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If entities is empty, code execution won't go into the for loop below, and listener has no chance to call onFailure or onResponse

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel it is better to return some failure and show that on kibana than return empty result. Customers may wonder why it returns nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if empty list is returned, existing Kibana will show corresponding message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment explaining why we want to return empty responses instead of errors? It helps code readability.

}
ActionListener<EntityAnomalyResult> entityAnomalyResultListener = ActionListener
.wrap(
entityAnomalyResult -> { listener.onResponse(entityAnomalyResult.getAnomalyResults()); },
Expand Down Expand Up @@ -116,6 +122,9 @@ public void executeDetector(AnomalyDetector detector, Instant startTime, Instant

private void onFailure(Exception e, ActionListener<List<AnomalyResult>> listener, String detectorId) {
logger.info("Fail to preview anomaly detector " + detectorId, e);
// 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
listener.onResponse(Collections.emptyList());
}

Expand Down