Various bug fixes (#502) (#507)

* Various bug fixes We display the error message "No features have been added to this anomaly detector. A feature is a metric used for anomaly detection. A detector can detect anomalies across one or more features" when the preview API fails to complete the preview request due to one of the following reasons: * The preview API cannot locate sufficient data to show sample anomaly results, as it requires more than 400 data points within the preview date range (from 5 days ago to the present time). * The preview API fails with an error (e.g., 5xx error). * No features have been added to the anomaly detector. * No features have been enabled in the anomaly detector. This pull request (PR) fixes the issue by displaying errors on a case-by-case basis. Additionally, this PR adds Prettier to the repository as it is used by Dashboards and other plugins (https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/DEVELOPER_GUIDE.md#prettier-and-linting). Furthermore, this PR standardizes the preview range to 7 days. Previously, we attempted to use 7 days but had to override it to 5 days due to two startTime fields in the parameters (refer to AnomalyDetectorData.js). This PR also resolves a bug where we show two links to the detector configuration (refer to getOverviewStats.js). This PR addresses a warning when displaying a detector URL on the monitor overview page (OverviewStat/OverviewStat). As the following error trace indicates, we only supported string or number as a value in Overview stats. This PR introduces a React element as another supported type. checkPropTypes.js:20 Warning: Failed prop type: Invalid prop `value` supplied to `OverviewStat`, expected one of type [string, number]. in OverviewStat (created by MonitorOverview) in MonitorOverview (created by MonitorDetails) in div (created by MonitorDetails) in MonitorDetails (created by Context.Consumer) in Route (created by Context.Consumer) in Switch (created by Context.Consumer) in div (created by Context.Consumer) in Main (created by Context.Consumer) in Route in Router (created by HashRouter) in HashRouter This PR also removes the unused file AnomalyHistory.js. Testing done: * Manually tested each preview failure scenario. * Manually tested alerting workflow of both HC and single stream detectors still works. * Added unit tests for new code. Signed-off-by: Kaituo Li <[email protected]> * add missing period Signed-off-by: Kaituo Li <[email protected]> * Don't block trigger creation when preview fails Creating/editing a monitor without setting a trigger means no alert is ever raised. This commit fixed the issue. Signed-off-by: Kaituo Li <[email protected]> --------- Signed-off-by: Kaituo Li <[email protected]> (cherry picked from commit 6dc848e) Co-authored-by: Kaituo Li <[email protected]>
opensearch-project · Mar 24, 2023 · 91785d7 · 91785d7
1 parent 89026fc
commit 91785d7
Show file tree

Hide file tree

Showing 14 changed files with 571 additions and 191 deletions.
diff --git a/DEVELOPER_GUIDE.md b/DEVELOPER_GUIDE.md
@@ -78,6 +78,16 @@ Example output: `./build/alertingDashboards-1.0.0-rc1.zip`
   1. `--no-base-path`: opt out the BasePathProxy.
   1. `--no-watch`: make sure your server has not restarted.
 
+### Formatting
+
+This codebase uses Prettier as our code formatter. All new code that is added has to be reformatted using the Prettier version listed in `package.json`. In order to keep consistent formatting across the project developers should only use the prettier CLI to reformat their code using the following command:
+
+```
+yarn prettier --write <relative file path>
+```
+
+> NOTE: There also exists prettier plugins on several editors that allow for automatic reformatting on saving the file. However using this is discouraged as you must ensure that the plugin uses the correct version of prettier (listed in `package.json`) before using such a plugin.
+
 ### Backport
 
 - [Link to backport documentation](https://github.com/opensearch-project/opensearch-plugins/blob/main/BACKPORT.md)
diff --git a/package.json b/package.json
@@ -40,7 +40,8 @@
     "formik": "^2.2.6",
     "lodash": "^4.17.21",
     "query-string": "^6.13.2",
-    "react-vis": "^1.8.1"
+    "react-vis": "^1.8.1",
+    "prettier": "^2.1.1"
   },
   "resolutions": {
     "ansi-regex": "^5.0.1",

diff --git a/...es/CreateMonitor/components/AnomalyDetectors/EmptyFeaturesMessage/EmptyFeaturesMessage.js b/...es/CreateMonitor/components/AnomalyDetectors/EmptyFeaturesMessage/EmptyFeaturesMessage.js
@@ -6,48 +6,94 @@
 import React from 'react';
 import PropTypes from 'prop-types';
 import { EuiEmptyPrompt, EuiButton, EuiText, EuiLoadingChart } from '@elastic/eui';
-import { OPENSEARCH_DASHBOARDS_AD_PLUGIN } from '../../../../../utils/constants';
+import {
+  OPENSEARCH_DASHBOARDS_AD_PLUGIN,
+  PREVIEW_ERROR_TYPE,
+} from '../../../../../utils/constants';
 
-const EmptyFeaturesMessage = (props) => (
-  <div
-    style={{
-      borderRadius: '5px',
-      padding: '10px',
-      border: '1px solid #D9D9D9',
-      height: '250px',
-      width: '100%',
-      ...props.containerStyle,
-    }}
-  >
-    {props.isLoading ? (
-      <EuiEmptyPrompt style={{ maxWidth: '45em' }} body={<EuiLoadingChart size="xl" />} />
-    ) : (
-      <EuiEmptyPrompt
-        style={{ maxWidth: '45em' }}
-        body={
-          <EuiText>
-            No features have been added to this anomaly detector. A feature is a metric that is used
-            for anomaly detection. A detector can discover anomalies across one or more features.
-          </EuiText>
-        }
-        actions={[
-          <EuiButton
-            data-test-subj="createButton"
-            href={`${OPENSEARCH_DASHBOARDS_AD_PLUGIN}#/detectors/${props.detectorId}/features`}
-            target="_blank"
-          >
-            Add Feature
-          </EuiButton>,
-        ]}
-      />
-    )}
-  </div>
-);
+/**
+ *
+ * @param {string} errorType error type
+ * @param {*} err error message
+ * @param {*} isHCDetector whether the detector is HC
+ * @returns error messages to show on the empty trigger page
+ */
+function getErrorMsg(errorType, err, isHCDetector) {
+  switch (errorType) {
+    case PREVIEW_ERROR_TYPE.NO_FEATURE:
+      return 'No features have been added to this anomaly detector. A feature is a metric that is used for anomaly detection. A detector can discover anomalies across one or more features.';
+    case PREVIEW_ERROR_TYPE.NO_ENABLED_FEATURES:
+      return 'No features have been enabled in this anomaly detector. A feature is a metric that is used for anomaly detection. A detector can discover anomalies across one or more features.';
+    default:
+      console.log('We only deal with feature related error type in this page: ' + errorType);
+      return '';
+  }
+}
+
+// FunctionComponent
+const ActionUI = ({ errorType, detectorId }) => {
+  switch (errorType) {
+    case PREVIEW_ERROR_TYPE.NO_FEATURE:
+      return (
+        <EuiButton
+          data-test-subj="createButton"
+          href={`${OPENSEARCH_DASHBOARDS_AD_PLUGIN}#/detectors/${detectorId}/features`}
+          target="_blank"
+        >
+          Add Feature
+        </EuiButton>
+      );
+    case PREVIEW_ERROR_TYPE.NO_ENABLED_FEATURES:
+      return (
+        <EuiButton
+          data-test-subj="editButton"
+          href={`${OPENSEARCH_DASHBOARDS_AD_PLUGIN}#/detectors/${detectorId}/features`}
+          target="_blank"
+        >
+          Enable Feature
+        </EuiButton>
+      );
+    default:
+      console.log('We only deal with feature related error type in this page: ' + errorType);
+      return <EuiText data-test-subj="callOut">''</EuiText>;
+  }
+};
+
+const EmptyFeaturesMessage = (props) => {
+  const errorMsg = getErrorMsg(props.previewErrorType, props.error, props.isHCDetector);
+
+  return (
+    <div
+      style={{
+        borderRadius: '5px',
+        padding: '10px',
+        border: '1px solid #D9D9D9',
+        height: '250px',
+        width: '100%',
+        ...props.containerStyle,
+      }}
+    >
+      {props.isLoading ? (
+        <EuiEmptyPrompt style={{ maxWidth: '45em' }} body={<EuiLoadingChart size="xl" />} />
+      ) : (
+        <EuiEmptyPrompt
+          data-test-subj="empty-prompt"
+          style={{ maxWidth: '45em' }}
+          body={<EuiText>{errorMsg}</EuiText>}
+          actions={[<ActionUI errorType={props.previewErrorType} detectorId={props.detectorId} />]}
+        />
+      )}
+    </div>
+  );
+};
 
 EmptyFeaturesMessage.propTypes = {
   detectorId: PropTypes.string,
   isLoading: PropTypes.bool.isRequired,
   containerStyle: PropTypes.object,
+  error: PropTypes.string.isRequired,
+  isHCDetector: PropTypes.bool.isRequired,
+  previewErrorType: PropTypes.number.isRequired,
 };
 EmptyFeaturesMessage.defaultProps = {
   detectorId: '',

diff --git a/...eateMonitor/components/AnomalyDetectors/EmptyFeaturesMessage/EmptyFeaturesMessage.test.js b/...eateMonitor/components/AnomalyDetectors/EmptyFeaturesMessage/EmptyFeaturesMessage.test.js
@@ -6,10 +6,21 @@
 import React from 'react';
 import { render } from 'enzyme';
 import { EmptyFeaturesMessage } from './EmptyFeaturesMessage';
+import { PREVIEW_ERROR_TYPE } from '../../../../../utils/constants';
 
 describe('EmptyFeaturesMessage', () => {
-  test('renders ', () => {
+  test('renders no feature', () => {
     const component = <EmptyFeaturesMessage detectorId="tempId" />;
     expect(render(component)).toMatchSnapshot();
   });
+  test('renders no enabled feature', () => {
+    const component = (
+      <EmptyFeaturesMessage
+        detectorId="tempId"
+        previewErrorType={PREVIEW_ERROR_TYPE.NO_ENABLED_FEATURES}
+      />
+    );
+    const wrapper = render(component);
+    expect(wrapper.find('[data-test-subj~="editButton"]').text()).toEqual('Enable Feature');
+  });
 });
diff --git a/...nts/AnomalyDetectors/EmptyFeaturesMessage/__snapshots__/EmptyFeaturesMessage.test.js.snap b/...nts/AnomalyDetectors/EmptyFeaturesMessage/__snapshots__/EmptyFeaturesMessage.test.js.snap
@@ -1,11 +1,12 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`EmptyFeaturesMessage renders  1`] = `
+exports[`EmptyFeaturesMessage renders no feature 1`] = `
 <div
   style="border-radius:5px;padding:10px;border:1px solid #D9D9D9;height:250px;width:100%"
 >
   <div
     class="euiEmptyPrompt"
+    data-test-subj="empty-prompt"
     style="max-width:45em"
   >
     <span
@@ -16,9 +17,7 @@ exports[`EmptyFeaturesMessage renders  1`] = `
       >
         <div
           class="euiText euiText--medium"
-        >
-          No features have been added to this anomaly detector. A feature is a metric that is used for anomaly detection. A detector can discover anomalies across one or more features.
-        </div>
+        />
       </div>
     </span>
     <div
@@ -30,23 +29,12 @@ exports[`EmptyFeaturesMessage renders  1`] = `
       <div
         class="euiFlexItem euiFlexItem--flexGrowZero"
       >
-        <a
-          class="euiButton euiButton--primary"
-          data-test-subj="createButton"
-          href="anomaly-detection-dashboards#/detectors/tempId/features"
-          rel="noopener noreferrer"
-          target="_blank"
+        <div
+          class="euiText euiText--medium"
+          data-test-subj="callOut"
         >
-          <span
-            class="euiButtonContent euiButton__content"
-          >
-            <span
-              class="euiButton__text"
-            >
-              Add Feature
-            </span>
-          </span>
-        </a>
+          ''
+        </div>
       </div>
     </div>
   </div>

diff --git a/public/pages/CreateMonitor/containers/AnomalyDetectors/AnomalyDetectorData.js b/public/pages/CreateMonitor/containers/AnomalyDetectors/AnomalyDetectorData.js
@@ -7,7 +7,7 @@ import React from 'react';
 import PropTypes from 'prop-types';
 import moment from 'moment';
 import { CoreContext } from '../../../../utils/CoreContext';
-import { AD_PREVIEW_DAYS } from '../../../../utils/constants';
+import { AD_PREVIEW_DAYS, DEFAULT_PREVIEW_ERROR_MSG } from '../../../../utils/constants';
 import { backendErrorNotification } from '../../../../utils/helpers';
 
 class AnomalyDetectorData extends React.Component {
@@ -25,6 +25,7 @@ class AnomalyDetectorData extends React.Component {
       previewStartTime: 0,
       previewEndTime: 0,
       isLoading: false,
+      error: '',
     };
     this.getPreviewData = this.getPreviewData.bind(this);
   }
@@ -39,6 +40,17 @@ class AnomalyDetectorData extends React.Component {
     }
   }
 
+  getPreviewErrorMessage(err) {
+    if (typeof err === 'string') return err;
+    if (err) {
+      if (err.msg === 'Bad Request') {
+        return err.response || DEFAULT_PREVIEW_ERROR_MSG;
+      }
+      if (err.msg) return err.msg;
+    }
+    return DEFAULT_PREVIEW_ERROR_MSG;
+  }
+
   async getPreviewData() {
     const { detectorId, startTime, endTime } = this.props;
     const { http: httpClient, notifications } = this.context;
@@ -47,7 +59,6 @@ class AnomalyDetectorData extends React.Component {
     });
     if (!detectorId) return;
     const requestParams = {
-      startTime: moment().subtract(AD_PREVIEW_DAYS, 'd').valueOf(),
       startTime: startTime,
       endTime: endTime,
       preview: this.props.preview,
@@ -69,13 +80,15 @@ class AnomalyDetectorData extends React.Component {
       } else {
         this.setState({
           isLoading: false,
+          error: getPreviewErrorMessage(response.error),
         });
         backendErrorNotification(notifications, 'get', 'detector results', response.error);
       }
     } catch (err) {
       console.error('Unable to get detectorResults', err);
       this.setState({
         isLoading: false,
+        error: err,
       });
     }
   }
@@ -91,7 +104,7 @@ AnomalyDetectorData.propTypes = {
 };
 AnomalyDetectorData.defaultProps = {
   preview: true,
-  startTime: moment().subtract(5, 'd').valueOf(),
+  startTime: moment().subtract(AD_PREVIEW_DAYS, 'd').valueOf(),
   endTime: moment().valueOf(),
 };
 

diff --git a/public/pages/CreateTrigger/containers/DefineTrigger/AnomalyDetectorTrigger.js b/public/pages/CreateTrigger/containers/DefineTrigger/AnomalyDetectorTrigger.js
@@ -11,18 +11,48 @@ import TriggerExpressions from '../../components/TriggerExpressions';
 import { AnomaliesChart } from '../../../CreateMonitor/components/AnomalyDetectors/AnomaliesChart';
 import { EmptyFeaturesMessage } from '../../../CreateMonitor/components/AnomalyDetectors/EmptyFeaturesMessage/EmptyFeaturesMessage';
 import { EmptyDetectorMessage } from '../../../CreateMonitor/components/AnomalyDetectors/EmptyDetectorMessage/EmptyDetectorMessage';
+import { PREVIEW_ERROR_TYPE } from '../../../../utils/constants';
+
+/**
+  *
+  * @param {string} err if there is any exception or error running preview API,
+     err is not empty.
+  * @param {*} features detector features
+  * @returns error type
+ */
+function getPreviewErrorType(err, features) {
+  if (features === undefined || features.length == 0) {
+    return PREVIEW_ERROR_TYPE.NO_FEATURE;
+  }
+
+  const enabledFeatures = features.filter((feature) => feature.featureEnabled);
+  if (enabledFeatures.length == 0) {
+    return PREVIEW_ERROR_TYPE.NO_ENABLED_FEATURES;
+  }
+
+  // if error is a non-empty string, return it.
+  if (err) return PREVIEW_ERROR_TYPE.PREVIEW_EXCEPTION;
+
+  // sparse data
+  return PREVIEW_ERROR_TYPE.SPARSE_DATA;
+}
 
 class AnomalyDetectorTrigger extends React.Component {
   constructor(props) {
     super(props);
   }
   render() {
     const { adValues, detectorId, fieldPath } = this.props;
+    const { httpClient, notifications } = this.context;
     return (
       <div style={{ padding: '0px 10px' }}>
         <AnomalyDetectorData
           detectorId={detectorId}
           render={(anomalyData) => {
+            const features = _.get(anomalyData, 'detector.featureAttributes', []);
+            const isHCDetector = !_.isEmpty(_.get(anomalyData, 'detector.categoryField', []));
+            const previewErrorType = getPreviewErrorType(anomalyData.error, features);
+
             // using lodash.get without worrying about whether an intermediate property is null or undefined.
             if (_.get(anomalyData, 'anomalyResult.anomalies', []).length > 0) {
               return (
@@ -66,11 +96,40 @@ class AnomalyDetectorTrigger extends React.Component {
                   />
                 </React.Fragment>
               );
+            } else if (_.isEmpty(detectorId)) {
+              return <EmptyDetectorMessage />;
+            } else if (
+              previewErrorType === PREVIEW_ERROR_TYPE.EXCEPTION ||
+              previewErrorType === PREVIEW_ERROR_TYPE.SPARSE_DATA
+            ) {
+              return (
+                <React.Fragment>
+                  <TriggerExpressions
+                    thresholdValue={adValues.anomalyGradeThresholdValue}
+                    thresholdEnum={adValues.anomalyGradeThresholdEnum}
+                    keyFieldName={`${fieldPath}anomalyDetector.anomalyGradeThresholdEnum`}
+                    valueFieldName={`${fieldPath}anomalyDetector.anomalyGradeThresholdValue`}
+                    label="Anomaly grade threshold"
+                  />
+                  <EuiSpacer size="m" />
+                  <TriggerExpressions
+                    thresholdValue={adValues.anomalyConfidenceThresholdValue}
+                    thresholdEnum={adValues.anomalyConfidenceThresholdEnum}
+                    keyFieldName={`${fieldPath}anomalyDetector.anomalyConfidenceThresholdEnum`}
+                    valueFieldName={`${fieldPath}anomalyDetector.anomalyConfidenceThresholdValue`}
+                    label="Anomaly confidence threshold"
+                  />
+                </React.Fragment>
+              );
             } else {
-              return _.isEmpty(detectorId) ? (
-                <EmptyDetectorMessage />
-              ) : (
-                <EmptyFeaturesMessage detectorId={detectorId} isLoading={anomalyData.isLoading} />
+              return (
+                <EmptyFeaturesMessage
+                  detectorId={detectorId}
+                  isLoading={anomalyData.isLoading}
+                  error={anomalyData.error}
+                  isHCDetector={isHCDetector}
+                  previewErrorType={previewErrorType}
+                />
               );
             }
           }}