Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug fixes for single flow feature #77

Merged
merged 2 commits into from
Aug 30, 2021
Merged

Conversation

ohltyler
Copy link
Member

@ohltyler ohltyler commented Aug 27, 2021

Signed-off-by: Tyler Ohlsen ohltyler@amazon.com

Description

This PR fixes 2 bugs found when testing out the single flow feature:

  1. Historical analysis task not being taken into consideration when editing the detector settings or model config. The fix: add checks for if the historical task is running or not before editing, and add verbose messaging around that. Added a custom hook which will automatically close the modal if the detector task states have been changed after opening the modal (e.g., the historical analysis was RUNNING when initially opened, but was changed to FINISHED soon after).
  2. 'The detector was stopped' messaging showing up when selecting a heatmap cell for a historical high-cardinality detector, during a timeframe where the corresponding real-time task was not running. The fix: propagate the isHistorical prop to the relevant components, which will hide these annotations if the chart is being rendered in a historical context.

Testing:

  • Tested all 4 scenarios of detector tasks running (just realtime, just historical, both realtime & historical, neither realtime or historical), when clicking on the edit features or edit detector buttons to confirm the wording is correct, and the relevant tasks are stopped.

Screenshots of an example where both realtime and historical jobs were running:

Updated wording in modal:
Screen Shot 2021-08-27 at 11 56 42 AM

Updated wording in toast:
Screen Shot 2021-08-27 at 11 57 14 AM

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler ohltyler added the v1.1.0 Version 1.1.0 label Aug 27, 2021
@ohltyler ohltyler requested review from kaituo and ylwu-amzn August 27, 2021 18:41
@ohltyler
Copy link
Member Author

ohltyler commented Aug 27, 2021

E2E tests failing due to upstream changes in Dashboards which bumped the version in the 1.0 branch from 1.0.0 to 1.0.1

Will fix in a separate PR to run against the explicit 1.0.0 tag, then re-run the tests here.

get(detector, 'taskState') === DETECTOR_STATE.INIT;
const runningJobsAsString =
isRTJobRunning && isHistoricalJobRunning
? 'detector and historical analysis'
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we use sth like "real-time" instead of detector? I think the main contribution of single flow is to let real time and historical analyses share a detector.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, so the frontend still primarily corresponds starting and stopping a "Detector" with the real-time aspect of it, like how it was before. Single flow basically merged historical detectors into a detector's "Historical analysis". The UI & wording is primarily around this idea now, where you create a detector, and you can either (1) run it (which by default is real-time), and/or (2) run a historical analysis.

This wording was gone through UX and tech writer reviews. Let me know if you still have concerns.

@@ -575,6 +575,7 @@ export const AnomalyHistory = (props: AnomalyHistoryProps) => {
detector={props.detector}
monitor={props.monitor}
isHCDetector={isHCDetector}
isHistorical={props.isHistorical}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you point me the code link to 'The detector was stopped' error message? Cannot find it and thus cannot link between isHistorical and the appearance of 'The detector was stopped'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just asking, can we fix the issue "AD result chart of historical HC shows detector stopped message" by adding this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kaituo the annotations are created in disabledHistoryAnnotations, which is used in AnomalyDetailsChart, which is called in AnomaliesChart, which is called in AnomalyHistory

@ylwu-amzn Yes, see point 2 in description

@ohltyler ohltyler merged commit d7c2bf5 into opensearch-project:main Aug 30, 2021
@ohltyler ohltyler deleted the bugs branch August 30, 2021 22:25
ohltyler added a commit to ohltyler/anomaly-detection-dashboards-plugin-1 that referenced this pull request Sep 1, 2021
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
ohltyler added a commit that referenced this pull request Sep 1, 2021
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler ohltyler added the bug Something isn't working label Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v1.1.0 Version 1.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants