-
Notifications
You must be signed in to change notification settings - Fork 62
anomaly detection changes. #123
anomaly detection changes. #123
Conversation
Co-authored-by: mihirsoni <[email protected]>
Paste my local test result here as the "Unit test workflow" of Github Checks failed.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please have some folks from AD review this PR, there is too much business logic pertaining to AD in this PR with 69 file changes for me to fully review. Consider breaking apart PRs into reasonable lengths.
public/pages/CreateMonitor/components/AnomalyDetectors/AnomaliesChart/AnomaliesChart.js
Outdated
Show resolved
Hide resolved
public/pages/CreateMonitor/components/AnomalyDetectors/AnomaliesChart/AnomaliesChart.js
Outdated
Show resolved
Hide resolved
public/pages/CreateMonitor/containers/AnomalyDetectors/__tests__/AnomalyDetector.test.js
Outdated
Show resolved
Hide resolved
public/pages/CreateMonitor/containers/AnomalyDetectors/AnomalyDetectors.js
Outdated
Show resolved
Hide resolved
public/pages/CreateMonitor/components/MonitorDefinition/MonitorDefinition.js
Outdated
Show resolved
Hide resolved
public/pages/CreateMonitor/components/AnomalyDetectors/AnomaliesChart/AnomaliesChart.js
Show resolved
Hide resolved
public/pages/CreateMonitor/containers/CreateMonitor/utils/monitorQueryParams.js
Show resolved
Hide resolved
public/pages/CreateTrigger/containers/DefineTrigger/AnomalyDetectorTrigger.js
Outdated
Show resolved
Hide resolved
public/pages/MonitorDetails/containers/AnomalyHistory/AnomalyHistory.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some minor comments. feel free to check in after addressing them.
Post local Test Suites: 6 skipped, 78 passed, 78 of 84 total |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for changes
* anomaly detection changes. Co-authored-by: mihirsoni <[email protected]> * remove anomaly detector option when create monitor if AD plugin not installed * address comments: copyright, remove unused code,etc * remove extra showLoader Co-authored-by: mihirsoni <[email protected]>
* validateUrl: Increasing valid TLD length to 63 (#115) * Increasing valid TLD length to 63 According to RFC1035 (https://tools.ietf.org/html/rfc1035), the maximum size of a label is 63 octets. We need this increased because we use a domain that is larger than 4 characters. We cannot setup alerts as a result. * Add validateUrl test for TLD with >4 characters * Adding validateUrl test for TLD with >63 characters * anomaly detection changes. (#123) * anomaly detection changes. Co-authored-by: mihirsoni <[email protected]> * remove anomaly detector option when create monitor if AD plugin not installed * address comments: copyright, remove unused code,etc * remove extra showLoader Co-authored-by: mihirsoni <[email protected]> * change words and padding according to ux review comments (#132) * change words and padding according to ux review comments * fix padding of monitor schedule * rename monitor method description according to UX review comments (#133) * fix snapshot test and validate unit test Co-authored-by: mihirsoni <[email protected]>
Co-authored-by: mihirsoni [email protected]
Issue #, if available:
Create monitor
Create trigger
Monitor detail page
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.