[GitHub] [incubator-pinot] jihaozh commented on a change in pull request #5398: [TE] Added generic support for configuring Data Quality rules like SLA from Yaml

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

jihaozh commented on a change in pull request #5398:
URL: https://github.com/apache/incubator-pinot/pull/5398#discussion_r428388089



##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/DataQualityMergeWrapper.java
##########
@@ -0,0 +1,99 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.thirdeye.detection.wrapper;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+import org.apache.pinot.thirdeye.datalayer.dto.DetectionConfigDTO;
+import org.apache.pinot.thirdeye.datalayer.dto.MergedAnomalyResultDTO;
+import org.apache.pinot.thirdeye.detection.DataProvider;
+import org.apache.pinot.thirdeye.detection.algorithm.MergeWrapper;
+import org.apache.pinot.thirdeye.detection.spi.model.AnomalySlice;
+
+
+/**
+ * The Data Quality Merge Wrapper. This merge wrapper is specifically designed keeping the sla anomalies in mind.
+ * We might need to revisit this when more data quality rules are added. Fundamentally, the data sla anomalies are never
+ * merged as we want to keep re-notifying users if the sla is missed after every detection. This merger will ensure no
+ * duplicate sla anomalies are created if the detection runs more frequently and will serve as a placeholder for future
+ * merging logic.
+ */
+public class DataQualityMergeWrapper extends MergeWrapper {
+  private static final String PROP_GROUP_KEY = "groupKey";
+
+  public DataQualityMergeWrapper(DataProvider provider, DetectionConfigDTO config, long startTime, long endTime) {
+    super(provider, config, startTime, endTime);
+  }
+
+  @Override
+  protected List<MergedAnomalyResultDTO> retrieveAnomaliesFromDatabase(List<MergedAnomalyResultDTO> generated) {
+    AnomalySlice effectiveSlice = this.slice.withDetectionId(this.config.getId())
+        .withStart(this.getStartTime(generated) - this.maxGap - 1)
+        .withEnd(this.getEndTime(generated) + this.maxGap + 1);
+
+    Collection<MergedAnomalyResultDTO> anomalies =
+        this.provider.fetchAnomalies(Collections.singleton(effectiveSlice)).get(effectiveSlice);
+
+    return anomalies.stream().filter(anomaly -> !anomaly.isChild()).collect(Collectors.toList());

Review comment:
       should this merger only retrieve and handle the anomalies with SLA type?

##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/yaml/translator/builder/DetectionConfigTranslatorBuilder.java
##########
@@ -0,0 +1,223 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.thirdeye.detection.yaml.translator.builder;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.commons.collections4.MapUtils;
+import org.apache.pinot.thirdeye.datalayer.dto.MetricConfigDTO;
+import org.apache.pinot.thirdeye.detection.ConfigUtils;
+import org.apache.pinot.thirdeye.detection.DataProvider;
+import org.apache.pinot.thirdeye.detection.DetectionUtils;
+import org.apache.pinot.thirdeye.detection.annotation.registry.DetectionRegistry;
+import org.apache.pinot.thirdeye.detection.wrapper.ChildKeepingMergeWrapper;
+import org.apache.pinot.thirdeye.detection.wrapper.EntityAnomalyMergeWrapper;
+import org.apache.pinot.thirdeye.detection.wrapper.GrouperWrapper;
+import org.apache.pinot.thirdeye.detection.yaml.translator.DetectionMetricAttributeHolder;
+import org.apache.pinot.thirdeye.rootcause.impl.MetricEntity;
+
+import static org.apache.pinot.thirdeye.detection.yaml.DetectionConfigTuner.*;
+
+
+/**
+ * This is the root of the detection config translator builder. Other translators
+ * extend from this class.
+ */
+public abstract class DetectionConfigTranslatorBuilder {
+
+  public static final String PROP_SUB_ENTITY_NAME = "subEntityName";
+  public static final String PROP_DIMENSION_EXPLORATION = "dimensionExploration";
+  public static final String PROP_FILTERS = "filters";
+
+  static final String PROP_DETECTION = "detection";
+  static final String PROP_FILTER = "filter";
+  static final String PROP_QUALITY = "quality";
+  static final String PROP_TYPE = "type";
+  static final String PROP_CLASS_NAME = "className";
+  static final String PROP_PARAMS = "params";
+  static final String PROP_METRIC_URN = "metricUrn";
+  static final String PROP_DIMENSION_FILTER_METRIC = "dimensionFilterMetric";
+  static final String PROP_NESTED_METRIC_URNS = "nestedMetricUrns";
+  static final String PROP_RULES = "rules";
+  static final String PROP_GROUPER = "grouper";
+  static final String PROP_NESTED = "nested";
+  static final String PROP_BASELINE_PROVIDER = "baselineValueProvider";
+  static final String PROP_DETECTOR = "detector";
+  static final String PROP_MOVING_WINDOW_DETECTION = "isMovingWindowDetection";
+  static final String PROP_WINDOW_DELAY = "windowDelay";
+  static final String PROP_WINDOW_DELAY_UNIT = "windowDelayUnit";
+  static final String PROP_WINDOW_SIZE = "windowSize";
+  static final String PROP_WINDOW_UNIT = "windowUnit";
+  static final String PROP_FREQUENCY = "frequency";
+  static final String PROP_MERGER = "merger";
+  static final String PROP_TIMEZONE = "timezone";
+  static final String PROP_NAME = "name";
+
+  static final String DEFAULT_BASELINE_PROVIDER_YAML_TYPE = "RULE_BASELINE";
+  static final String PROP_BUCKET_PERIOD = "bucketPeriod";
+  static final String PROP_CACHE_PERIOD_LOOKBACK = "cachingPeriodLookback";
+
+  static final String PROP_ALERTS = "alerts";
+  static final String COMPOSITE_ALERT = "COMPOSITE_ALERT";
+
+  final DetectionMetricAttributeHolder metricAttributesMap;
+  final DataProvider dataProvider;
+  static final DetectionRegistry DETECTION_REGISTRY = DetectionRegistry.getInstance();
+
+  DetectionConfigTranslatorBuilder(DetectionMetricAttributeHolder metricAttributesMap, DataProvider dataProvider) {

Review comment:
       nit: Why it's called `DetectionConfigTranslatorBuilder` instead of `DetectionConfigBuilder` or `DetectionPropertyBuilder`. Because it seems the result it produces is a config property, not the translator.




----------------------------------------------------------------
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



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