Prevent creating detector with duplicate name. Issue:#118 #134

yizheliu-amazon · 2020-05-22T05:35:39Z

Issue #, if available:
#118
Description of changes:
Add logic to prevent user from creating detector with duplicate name

Testing:

./gradlew build
./gradlew integTest

Manual testing:

// create detector cli-d-1-min-001
~/Downloads [ curl -X POST "localhost:9200/_opendistro/_anomaly_detection/detectors" -H 'Content-Type: application/json' -d'                                               ] 5:09 PM
{
    "name":"cli-d-1-min-001",
    "description":"Test nab daily jumpsup",
    "time_field":"timestamp",
    "indices":[
        "test-index"
    ],
    "feature_attributes":[
        {
            "feature_name":"feature-1",
            "feature_enabled":true,
            "aggregation_query":{
                "value_sum":{
                    "sum":{
                        "field":"feature-1"
                    }
                }
            }
        }
    ],
    "detection_interval":{
        "period":{
            "interval":1,
            "unit":"Minutes"
        }
    }
}
'
{"_id":"LNm2OXIB8qLpH1US_aNC","_version":1,"_seq_no":0,"_primary_term":1,"anomaly_detector":{"name":"cli-d-1-min-001","description":"Test nab daily jumpsup","time_field":"timestamp","indices":["test-index"],"filter_query":{"match_all":{"boost":1.0}},"detection_interval":{"period":{"interval":1,"unit":"Minutes"}},"window_delay":{"period":{"interval":0,"unit":"Seconds"}},"schema_version":0,"feature_attributes":[{"feature_id":"Jtm2OXIB8qLpH1US_KNj","feature_name":"feature-1","feature_enabled":true,"aggregation_query":{"value_sum":{"sum":{"field":"feature-1"}}}}]}}

// create detector cli-d-1-min-002
~/Downloads [ curl -X POST "localhost:9200/_opendistro/_anomaly_detection/detectors" -H 'Content-Type: application/json' -d'                                               ] 5:09 PM
{
    "name":"cli-d-1-min-002",
    "description":"Test nab daily jumpsup",
    "time_field":"timestamp",
    "indices":[
        "test-index"
    ],
    "feature_attributes":[
        {
            "feature_name":"feature-1",
            "feature_enabled":true,
            "aggregation_query":{
                "value_sum":{
                    "sum":{
                        "field":"feature-1"
                    }
                }
            }
        }
    ],
    "detection_interval":{
        "period":{
            "interval":1,
            "unit":"Minutes"
        }
    }
}
'
{"_id":"M9m3OXIB8qLpH1USKaM3","_version":1,"_seq_no":1,"_primary_term":1,"anomaly_detector":{"name":"cli-d-1-min-002","description":"Test nab daily jumpsup","time_field":"timestamp","indices":["test-index"],"filter_query":{"match_all":{"boost":1.0}},"detection_interval":{"period":{"interval":1,"unit":"Minutes"}},"window_delay":{"period":{"interval":0,"unit":"Seconds"}},"schema_version":0,"feature_attributes":[{"feature_id":"Ldm3OXIB8qLpH1USKaMu","feature_name":"feature-1","feature_enabled":true,"aggregation_query":{"value_sum":{"sum":{"field":"feature-1"}}}}]}}

// Update cli-d-1-min-002 name to cli-d-1-min-001 -> failed
~/Downloads [ curl -X PUT "localhost:9200/_opendistro/_anomaly_detection/detectors/M9m3OXIB8qLpH1USKaM3" -H 'Content-Type: application/json' -d'                           ] 5:10 PM
{
    "name":"cli-d-1-min-001",
    "description":"Test nab daily jumpsup--",
    "time_field":"timestamp",
    "indices":[
        "test-index"
    ],
    "feature_attributes":[
        {
            "feature_name":"feature-1",
            "feature_enabled":true,
            "aggregation_query":{
                "value_sum":{
                    "sum":{
                        "field":"feature-1"
                    }
                }
            }
        }
    ],
    "detection_interval":{
        "period":{
            "interval":1,
            "unit":"Minutes"
        }
    }
}
'
{"error":{"root_cause":[{"type":"illegal_argument_exception","reason":"Cannot create anomaly detector with name[cli-d-1-min-001] used by detectorId LNm2OXIB8qLpH1US_aNC."}],"type":"illegal_argument_exception","reason":"Cannot create anomaly detector with name[cli-d-1-min-001] used by detectorId LNm2OXIB8qLpH1US_aNC"},"status":400}

// Update cli-d-1-min-002 description -> pass
~/Downloads [ curl -X PUT "localhost:9200/_opendistro/_anomaly_detection/detectors/M9m3OXIB8qLpH1USKaM3" -H 'Content-Type: application/json' -d'                           ] 5:11 PM
{
    "name":"cli-d-1-min-002",
    "description":"Test nab daily jumpsup--updated",
    "time_field":"timestamp",
    "indices":[
        "test-index"
    ],
    "feature_attributes":[
        {
            "feature_name":"feature-1",
            "feature_enabled":true,
            "aggregation_query":{
                "value_sum":{
                    "sum":{
                        "field":"feature-1"
                    }
                }
            }
        }
    ],
    "detection_interval":{
        "period":{
            "interval":1,
            "unit":"Minutes"
        }
    }
}
'
{"_id":"M9m3OXIB8qLpH1USKaM3","_version":2,"_seq_no":2,"_primary_term":1,"anomaly_detector":{"name":"cli-d-1-min-002","description":"Test nab daily jumpsup--updated","time_field":"timestamp","indices":["test-index"],"filter_query":{"match_all":{"boost":1.0}},"detection_interval":{"period":{"interval":1,"unit":"Minutes"}},"window_delay":{"period":{"interval":0,"unit":"Seconds"}},"schema_version":0,"feature_attributes":[{"feature_id":"Odm4OXIB8qLpH1USqKNz","feature_name":"feature-1","feature_enabled":true,"aggregation_query":{"value_sum":{"sum":{"field":"feature-1"}}}}]}}

// Create detector with name cli-d-1-min-001 -> failed 
~/Downloads [ curl -X POST "localhost:9200/_opendistro/_anomaly_detection/detectors" -H 'Content-Type: application/json' -d'                                                                            ] 10:32 PM
{
    "name":"cli-d-1-min-001",
    "description":"new detector 003",
    "time_field":"timestamp",
    "indices":[
        "test-index"
    ],
    "feature_attributes":[
        {
            "feature_name":"feature-1",
            "feature_enabled":true,
            "aggregation_query":{
                "value_sum":{
                    "sum":{
                        "field":"feature-1"
                    }
                }
            }
        }
    ],
    "detection_interval":{
        "period":{
            "interval":1,
            "unit":"Minutes"
        }
    }
}
'
{"error":{"root_cause":[{"type":"illegal_argument_exception","reason":"Cannot create anomaly detector with name[cli-d-1-min-001] used by detectorId TYTeOnIBwvgo5IJfiD1y"}],"type":"illegal_argument_exception","reason":"Cannot create anomaly detector with name[cli-d-1-min-001] used by detectorId TYTeOnIBwvgo5IJfiD1y"},"status":400}

// Create detector cli-d-1-min-003 -> pass
~/Downloads [ curl -X POST "localhost:9200/_opendistro/_anomaly_detection/detectors" -H 'Content-Type: application/json' -d'                                                                            ] 10:34 PM
{
    "name":"cli-d-1-min-003",
    "description":"new detector 003",
    "time_field":"timestamp",
    "indices":[
        "test-index"
    ],
    "feature_attributes":[
        {
            "feature_name":"feature-1",
            "feature_enabled":true,
            "aggregation_query":{
                "value_sum":{
                    "sum":{
                        "field":"feature-1"
                    }
                }
            }
        }
    ],
    "detection_interval":{
        "period":{
            "interval":1,
            "unit":"Minutes"
        }
    }
}
'
{"_id":"WoTgOnIBwvgo5IJfcT0P","_version":1,"_seq_no":1,"_primary_term":1,"anomaly_detector":{"name":"cli-d-1-min-003","description":"new detector 003","time_field":"timestamp","indices":["test-index"],"filter_query":{"match_all":{"boost":1.0}},"detection_interval":{"period":{"interval":1,"unit":"Minutes"}},"window_delay":{"period":{"interval":0,"unit":"Seconds"}},"schema_version":0,"feature_attributes":[{"feature_id":"VITgOnIBwvgo5IJfcT0M","feature_name":"feature-1","feature_enabled":true,"aggregation_query":{"value_sum":{"sum":{"field":"feature-1"}}}}]}}

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…lasticsearch#118

wnbts · 2020-05-22T19:07:13Z

...com/amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java

+    private void checkADNameExists(String detectorId) throws IOException {
+        SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder()
+            // src/main/resources/mappings/anomaly-detectors.json#L14
+            .query(QueryBuilders.termQuery("name.keyword", anomalyDetector.getName()))


minor. can this query also include a condition like not this detector id to utilize es search instead of additional postprocessing?

that's a good point. will change to use must_not to exclude this detector id

wnbts · 2020-05-22T19:10:23Z

...com/amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java

+            String errorMsg = String.format("Cannot create anomaly detector with name[%s] used by detectorId %s", name, existingDetectorId);
+            logger.error(errorMsg);


minor. i would use a warning for this case since it's a harmless user error.

sure. will do

…duplicate name

ylwu-amzn · 2020-05-22T22:14:46Z

...com/amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java

+        // src/main/resources/mappings/anomaly-detectors.json#L14
+        boolQueryBuilder.must(QueryBuilders.termQuery("name.keyword", anomalyDetector.getName()));
+        // _id field does not allow "", but allows " "
+        boolQueryBuilder.mustNot(QueryBuilders.termQuery(RestHandlerUtils._ID, StringUtils.isBlank(detectorId) ? " " : detectorId));


How about we add mustNot condition only when detectorId is not blank?

I like the idea. Thanks!

ylwu-amzn · 2020-05-22T22:18:51Z

...com/amazon/opendistroforelasticsearch/ad/rest/handler/IndexAnomalyDetectorActionHandler.java

+        if (response.getHits().getTotalHits().value > 0) {
+            String errorMsg = String
+                .format(
+                    "Cannot create anomaly detector with name[%s] used by detectorId %s",


We may show this error message on Kibana. How about we make it more readable?
Cannot create anomaly detector with name [%s] as it's already used by detector [%s]

sure. will change that.

ylwu-amzn

LGTM. Thanks for the change.

Prevent creating detector with duplicate name. Issue:opendistro-for-e…

49f30a2

…lasticsearch#118

yizheliu-amazon requested review from ylwu-amzn and kaituo May 22, 2020 05:35

wnbts approved these changes May 22, 2020

View reviewed changes

Exclude detector with input detector id when searching detector with …

6f1c83c

…duplicate name

ylwu-amzn reviewed May 22, 2020

View reviewed changes

kaituo approved these changes May 22, 2020

View reviewed changes

Address Yaliang's comments and add 1 more test case

53a264c

yizheliu-amazon requested a review from ylwu-amzn May 22, 2020 23:14

ylwu-amzn approved these changes May 22, 2020

View reviewed changes

yizheliu-amazon merged commit 64d7f0e into opendistro-for-elasticsearch:master May 22, 2020

yizheliu-amazon deleted the forbid-duplicate branch May 22, 2020 23:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent creating detector with duplicate name. Issue:#118 #134

Prevent creating detector with duplicate name. Issue:#118 #134

yizheliu-amazon commented May 22, 2020

wnbts May 22, 2020

yizheliu-amazon May 22, 2020

wnbts May 22, 2020

yizheliu-amazon May 22, 2020

ylwu-amzn May 22, 2020

yizheliu-amazon May 22, 2020

ylwu-amzn May 22, 2020

yizheliu-amazon May 22, 2020

ylwu-amzn left a comment

		String errorMsg = String.format("Cannot create anomaly detector with name[%s] used by detectorId %s", name, existingDetectorId);
		logger.error(errorMsg);

Prevent creating detector with duplicate name. Issue:#118 #134

Prevent creating detector with duplicate name. Issue:#118 #134

Conversation

yizheliu-amazon commented May 22, 2020

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 left a comment

Choose a reason for hiding this comment