Add progress bar for initialization #253
Conversation
yizheliu-amazon
added
the
enhancement
| @@ -343,7 +346,7 @@ export const DetectorDetail = (props: DetectorDetailProps) => { | |||
| <EuiFlexGroup> | |||
| <EuiFlexItem> | |||
| <EuiTabs> | |||
| {tabs.map(tab => ( | |||
| {tabs.map((tab) => ( | |||
There was a problem hiding this comment.
automatically changed by linter I guess. same for the other similar changes.
| </EuiFlexGroup> | ||
| <EuiSpacer size="l" /> | ||
| </div> | ||
| ) : null} | ||
| <EuiButton | ||
| onClick={props.onSwitchToConfiguration} |
There was a problem hiding this comment.
Why do we have a switch to configuration button when the configuration tab is only a finger click away?
There was a problem hiding this comment.
I know this was just part of the existing approved UI, but agreed that it doesn't really seem necessary with the tab right above. Guess it was for quickly navigating to config page since a config change must have occurred for initialization process to begin again / for callout to appear again.
There was a problem hiding this comment.
Yeah, this already exists in UX design since long time ago. This configuration button is shown in callout because all the messages in callout mentions detector configuration, and such button can direct user quickly to detector config page .
| @@ -292,6 +295,17 @@ export function AnomalyResults(props: AnomalyResultsProps) { | |||
| </p> | |||
| ) : isInitializingNormally ? ( | |||
| <p> | |||
| {detector.initProgress | |||
There was a problem hiding this comment.
Do we want to show the message besides isInitializingNormally? When it is over time, I can see a need for customer to ask when it is gonna finish.
There was a problem hiding this comment.
good point, will add it for overtime case
There was a problem hiding this comment.
will also add that message to missing data case as well.
| @@ -110,6 +114,13 @@ export function AnomalyResults(props: AnomalyResultsProps) { | |||
| const monitors = useSelector((state: AppState) => state.alerting.monitors); | |||
| const monitor = get(monitors, `${detectorId}.0`); | |||
|
|
|||
| const [featureMissingSeverity, setFeatureMissingSeverity] = useState< | |||
There was a problem hiding this comment.
simply move 2 lines of code from below to above.
| const getInitProgressMessage = () => { | ||
| return detector && isDetectorInitializing && detector.initProgress | ||
| ? `The detector needs to capture approximately | ||
| ${detector.initProgress.neededShingles} data points for initializing. If your data stream is continuous, this process will take around |
There was a problem hiding this comment.
How about change ${detector.initProgress.neededShingles} data points for initializing to ${detector.initProgress.neededShingles} more data points for initializing?
Confirm with tech writer about the wording.
There was a problem hiding this comment.
Sorry, seems I misunderstood it. Here the ${detector.initProgress.neededShingles} is the total data points needed or we still need such more data points? Same question for estimatedMinutesLeft
There was a problem hiding this comment.
“data points” should be replaced by shingles.
There was a problem hiding this comment.
re Yaliang: we still need such more data points.
There was a problem hiding this comment.
“data points” should be replaced by shingles.
Should we expose shingle, which is one internal term, to customer? I think data points expresses the same thing, and it is more user-friendly.
There was a problem hiding this comment.
hmm, I see, sound like shingles are "consecutive data points", is it right?
There was a problem hiding this comment.
Right. A shingle is a consecutive sequence of the most recent records.
There was a problem hiding this comment.
what about using "consecutive data points", my point is that shingle is not intuitive word for user, and we only have definition of what shingle process means in the doc
There was a problem hiding this comment.
after discussing offline with Kaituo, we may remove the message regarding shingle/data point which may be confusing to user, while provide needed time and percentage only.
There was a problem hiding this comment.
Wording has been confirmed with tech writer.
| return detector && isDetectorInitializing && detector.initProgress | ||
| ? `The detector needs to capture approximately | ||
| ${detector.initProgress.neededShingles} data points for initializing. If your data stream is continuous, this process will take around | ||
| ${detector.initProgress.estimatedMinutesLeft} minutes; if not, it may take even longer. ` |
There was a problem hiding this comment.
Just confirm, is the detector state based on how many data points in the tree? I remember our testing detector on NAB data will become running quickly before capturing 128 data points as cold start will build RCF model first. I'm asking this to make sure the detector state, neededShingles and estimatedMinutesLeft can match.
There was a problem hiding this comment.
is the detector state based on how many data points in the tree
yes
The number would be the total data points in the tree before cold start, and needed shingles after cold start. In your NAB example, we should only see this message briefly and the progress bar should disappear as initialization is finished.
| const getInitProgressMessage = () => { | ||
| return detector && isDetectorInitializing && detector.initProgress | ||
| ? `The detector needs to capture approximately | ||
| ${detector.initProgress.neededShingles} data points for initializing. If your data stream is continuous, this process will take around |
There was a problem hiding this comment.
“data points” should be replaced by shingles.
| const getInitProgressMessage = () => { | ||
| return detector && isDetectorInitializing && detector.initProgress | ||
| ? `The detector needs to capture approximately | ||
| ${detector.initProgress.neededShingles} data points for initializing. If your data stream is continuous, this process will take around |
There was a problem hiding this comment.
re Yaliang: we still need such more data points.
| return detector && isDetectorInitializing && detector.initProgress | ||
| ? `The detector needs to capture approximately | ||
| ${detector.initProgress.neededShingles} data points for initializing. If your data stream is continuous, this process will take around | ||
| ${detector.initProgress.estimatedMinutesLeft} minutes; if not, it may take even longer. ` |
There was a problem hiding this comment.
is the detector state based on how many data points in the tree
yes
The number would be the total data points in the tree before cold start, and needed shingles after cold start. In your NAB example, we should only see this message briefly and the progress bar should disappear as initialization is finished.
Issue #, if available:
Description of changes:
Add progress bar for initialization
Initializing normally:
When initializing is overtime:
When data is missing during initializing:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.