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

Add UX support for HC detector #314

Merged

Conversation

yizheliu-amazon
Copy link
Contributor

Issue #, if available:

Description of changes:
Add UX support for HC detector.

Preview for HC detector:

HC-Preview_0_-_2020-10-15_17 47 19

HC-Preview_1_-_2020-10-15_17 47 19

DetectorResult for HC detector:

HC_DetectorResult-0_-_2020-10-14_21 02 50
HC_DetectorResult-1_-_2020-10-14_21 02 50
HC_DetectorResult-2_-_2020-10-14_21 02 50

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

@yizheliu-amazon
Copy link
Contributor Author

will fix UT while reviewing

@yizheliu-amazon
Copy link
Contributor Author

current UT passed. will add more UT for new code.

Test Suites: 1 skipped, 51 passed, 51 of 52 total
Tests:       5 skipped, 230 passed, 235 total
Snapshots:   46 passed, 46 total
Time:        30.932s
Ran all test suites.
Done in 31.66s

};

const showLoader = useDelayedLoader(props.isLoading || isLoadingAlerts);
const showLoader = useDelayedLoader(props.isLoading);
Copy link
Contributor

Choose a reason for hiding this comment

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

Any impact to single entity detector by removing isLoadingAlerts ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no. it will be there for AnomalyDetailsChart

props.showAlerts === undefined || !props.showAlerts
? [
<EuiSpacer size="m" />,
<AnomalyOccurrenceChart
Copy link
Contributor

Choose a reason for hiding this comment

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

AnomalyOccurrenceChart is only used by HC detector and AnomalyDetailChart is only used single entity detector?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and AnomalyOccurrenceChart contains AnomalyDetailChart. AnomalyOccurrenceChart is actually wrapper of AnomalyDetailChart, making AnomalyDetailChart a separate chart so that it can be easily re-used in multiple places

@@ -0,0 +1,388 @@
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is refactored from AnomaliesChart.

Copy link
Contributor

@ohltyler ohltyler left a comment

Choose a reason for hiding this comment

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

LGTM!

@yizheliu-amazon yizheliu-amazon merged commit 8829407 into opendistro-for-elasticsearch:master Oct 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants