[GitHub] [incubator-pinot] xiaohui-sun commented on a change in pull request #4271: Extend Detection Config Validator to validate composite(entity) alerts

[GitBox <gi...@apache.org>]

xiaohui-sun commented on a change in pull request #4271: Extend Detection Config Validator to validate composite(entity) alerts 
URL: https://github.com/apache/incubator-pinot/pull/4271#discussion_r290085297
 
 

 ##########
 File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/validators/DetectionConfigValidator.java
 ##########
 @@ -104,80 +112,155 @@ public void validateConfig(DetectionConfigDTO detectionConfig) throws IllegalArg
   /**
    * Validates the detection or filter rule accordingly based on {@param ruleType}
    */
-  private void validateRule(Map<String, Object> ruleYaml, int ruleIndex, String ruleType, Set<String> ruleNamesTaken) {
+  private void validateRule(String alertName, Map<String, Object> ruleYaml, int ruleIndex, String ruleType, Set<String> ruleNamesTaken) {
     Preconditions.checkArgument(ruleYaml.containsKey(PROP_TYPE),
-        "In rule no." + (ruleIndex) + ", " + ruleType + " rule type is missing.");
+        "Missing property " + ruleType + " (" + ruleType + ") for sub-alert " + alertName + " rule no. " + ruleIndex);
     String type = MapUtils.getString(ruleYaml, PROP_TYPE);
-    String name = MapUtils.getString(ruleYaml, PROP_NAME);
-    Preconditions.checkNotNull(name,
-        "In rule no." + (ruleIndex) + ", " + ruleType + " rule name for type " +  type + " is missing.");
+
+    Preconditions.checkArgument(ruleYaml.containsKey(PROP_NAME),
+        "Missing property " + ruleType + " (" + PROP_NAME + ") for sub-alert " + alertName + " rule no. " + ruleIndex);
+    String name = (String) ruleYaml.get(PROP_NAME);
+
     Preconditions.checkArgument(!ruleNamesTaken.contains(name),
-        "In rule No." + (ruleIndex) + ", found duplicate rule name, rule name must be unique within config." );
-    Preconditions.checkArgument(!name.contains(":"), "Sorry, rule name cannot contain \':\'");
+        "Duplicate rule name (" + name + ") found for sub-alert " + alertName + " rule no. " + ruleIndex + ". Names have to be unique within a config.");
+
+    Preconditions.checkArgument(!name.contains(":"),
+        "Illegal character (:) found in " + ruleType + " (" + PROP_NAME + ") for sub-alert " + alertName + " rule no. " + ruleIndex);
   }
 
-  /**
-   * Validate the the detection yaml configuration.
-   *
-   * @param detectionYaml the detection yaml configuration to be validated
-   */
-  @Override
-  public void validateYaml(Map<String, Object> detectionYaml) {
+  private void validateBasicAttributes(Map<String, Object> detectionYaml, String parentAlertName) {
+    Preconditions.checkArgument(detectionYaml.containsKey(PROP_NAME),
+        "Missing property ( " + PROP_NAME + " ) in one of the sub-alerts under " + parentAlertName);
+    String alertName = (String) detectionYaml.get(PROP_NAME);
+
+    Preconditions.checkArgument(detectionYaml.containsKey(PROP_TYPE),
+        "Missing property ( " + PROP_TYPE + " ) in sub-alert " + alertName);
+    String alertType = (String) detectionYaml.get(PROP_TYPE);
+
+    Preconditions.checkArgument(SUPPORTED_ALERT_TYPES.contains(alertType),
+        "Unsupported type (" + alertType + ") in sub-alert " + alertName);
+  }
+
+  private void validateMetricAlertConfig(Map<String, Object> detectionYaml, String parentAlertName)
+      throws IllegalArgumentException {
+    validateBasicAttributes(detectionYaml, parentAlertName);
+    String alertName = (String) detectionYaml.get(PROP_NAME);
+
     // Validate all compulsory fields
-    Preconditions.checkArgument(detectionYaml.containsKey(PROP_DETECTION_NAME), "Property missing " + PROP_DETECTION_NAME);
-    Preconditions.checkArgument(detectionYaml.containsKey(PROP_METRIC), "Property missing " + PROP_METRIC);
-    Preconditions.checkArgument(detectionYaml.containsKey(PROP_DATASET), "Property missing " + PROP_DATASET);
-    Preconditions.checkArgument(detectionYaml.containsKey(PROP_RULES), "Property missing " + PROP_RULES);
+    Preconditions.checkArgument(detectionYaml.containsKey(PROP_METRIC),
+        "Missing property (" + PROP_METRIC + ") in sub-alert " + alertName);
+    Preconditions.checkArgument(detectionYaml.containsKey(PROP_DATASET),
+        "Missing property (" + PROP_DATASET + ") in sub-alert " + alertName);
+    Preconditions.checkArgument(detectionYaml.containsKey(PROP_RULES),
+        "Missing property (" + PROP_RULES + ") in sub-alert " + alertName);
 
-    // Validate fields which shouldn't be defined at root level
-    Preconditions.checkArgument(!detectionYaml.containsKey(PROP_FILTER), "Please double check the filter"
-        + " config. Adding dimensions filters should be in the yaml root level using 'filters' as the key. Anomaly "
-        + "filter should be added in to the indentation level of detection yaml it applies to.");
+    // Validate fields which shouldn't be defined at this level
+    Preconditions.checkArgument(!detectionYaml.containsKey(PROP_FILTER),
+        "For sub-alert " + alertName + ", please double check the filter config. Adding dimensions filters"
+            + " should be in the yaml root level using 'filters' as the key. Anomaly filter should be added in to the"
+            + " indentation level of detection yaml it applies to.");
 
     // Check if the metric defined in the config exists
     MetricConfigDTO metricConfig = provider
         .fetchMetric(MapUtils.getString(detectionYaml, PROP_METRIC), MapUtils.getString(detectionYaml, PROP_DATASET));
-    Preconditions.checkNotNull(metricConfig, "Metric defined in the config cannot be found");
-    DatasetConfigDTO datasetConfig = provider
-        .fetchDatasets(Collections.singletonList(metricConfig.getDataset()))
-        .get(metricConfig.getDataset());
+    Preconditions.checkArgument(metricConfig != null,
+        "Invalid metric (not found) in sub-alert " + alertName);
 
     // We support only one grouper per metric
     Preconditions.checkArgument(ConfigUtils.getList(detectionYaml.get(PROP_GROUPER)).size() <= 1,
-        "Multiple groupers detected for metric. We support only one grouper per metric.");
+        "Multiple groupers detected for metric in sub-alert " + alertName);
 
     // Validate all the rules
     Set<String> names = new HashSet<>();
     List<Map<String, Object>> ruleYamls = ConfigUtils.getList(detectionYaml.get(PROP_RULES));
-    for (int i = 1; i <= ruleYamls.size(); i++) {
-      Map<String, Object> ruleYaml = ruleYamls.get(i - 1);
+    for (int ruleIndex = 1; ruleIndex <= ruleYamls.size(); ruleIndex++) {
+      Map<String, Object> ruleYaml = ruleYamls.get(ruleIndex - 1);
 
       // Validate detection rules
-      Preconditions.checkArgument(ruleYaml.containsKey(PROP_DETECTION), "In rule no." + (i) + ", detection rule is missing.");
+      Preconditions.checkArgument(ruleYaml.containsKey(PROP_DETECTION),
+          "Detection rule missing for sub-alert " + alertName + " rule no. " + ruleIndex);
       List<Map<String, Object>> detectionRuleYamls = ConfigUtils.getList(ruleYaml.get(PROP_DETECTION));
       for (Map<String, Object> detectionRuleYaml : detectionRuleYamls) {
-        validateRule(detectionRuleYaml, i, "detection", names);
-        names.add(MapUtils.getString(ruleYaml, PROP_NAME));
+        validateRule(alertName, detectionRuleYaml, ruleIndex, "detection", names);
+        names.add(MapUtils.getString(detectionRuleYaml, PROP_NAME));
       }
 
       // Validate filter rules
       if (ruleYaml.containsKey(PROP_FILTER)) {
         List<Map<String, Object>> filterRuleYamls = ConfigUtils.getList(ruleYaml.get(PROP_FILTER));
         for (Map<String, Object> filterRuleYaml : filterRuleYamls) {
-          validateRule(filterRuleYaml, i, "filter", names);
-          names.add(MapUtils.getString(ruleYaml, PROP_NAME));
+          validateRule(alertName, filterRuleYaml, ruleIndex, "filter", names);
+          names.add(MapUtils.getString(filterRuleYaml, PROP_NAME));
         }
       }
     }
 
     // Safety condition: Validate if maxDuration is greater than 15 minutes
     Map<String, Object> mergerProperties = MapUtils.getMap(detectionYaml, PROP_MERGER, new HashMap());
     if (mergerProperties.get(PROP_MAX_DURATION) != null) {
-      Preconditions.checkArgument(MapUtils.getLong(mergerProperties, PROP_MAX_DURATION) >= datasetConfig.bucketTimeGranularity().toMillis(),
+      DatasetConfigDTO datasetConfig = provider
+          .fetchDatasets(Collections.singletonList(metricConfig.getDataset()))
+          .get(metricConfig.getDataset());
+      Preconditions.checkArgument(
+          MapUtils.getLong(mergerProperties, PROP_MAX_DURATION) >= datasetConfig.bucketTimeGranularity().toMillis(),
           "The maxDuration field set is not acceptable. Please check the the document and set it correctly.");
     }
   }
 
+  private void validateCompositeAlertConfig(Map<String, Object> detectionYaml, String parentAlertName) throws IllegalArgumentException {
+    validateBasicAttributes(detectionYaml, parentAlertName);
+    String alertName = (String) detectionYaml.get(PROP_NAME);
+
+    // Validate all compulsory fields
+    Preconditions.checkArgument(detectionYaml.containsKey(PROP_ALERTS),
 
 Review comment:
   Let's set a default prop_alerts if  it is not provided.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org