-
Notifications
You must be signed in to change notification settings - Fork 18
Add detector state page #65
Add detector state page #65
Conversation
if (!failureDetail) { | ||
return { | ||
cause: 'unknown error', | ||
actionItem: DEFAULT_ACTION_ITEM, | ||
}; | ||
} |
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.
is this the case where the detector has an unexpected failure (DETECTOR_STATE.UNEXPECTED_FAILURE
)?
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.
could maybe add this as a DETECTOR_UNEXPECTED_FAILURE
const in the constants file
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.
yes.
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.
I will add 'We might have bugs' case to the DETECTOR_INIT_FAILURES
, but may keep this piece of code just in case of not handled exception from AD backend.
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.
cool, sounds good
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.
LGTM! Thanks for the change
...(detectorState !== undefined ? { curState: detectorState.state } : {}), | ||
...(detectorState !== undefined | ||
? //@ts-ignore | ||
{ initializationError: detectorState.error } |
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.
maybe only assign initializationError
when the detector state is DETECTOR_STATE.INIT_FAILURE
. I'm just thinking that with the current code could assign an unexpected failure error to initializationError
. Although I see this is a small thing since the state is handled in the switch statement in DetectorStateDetails
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.
I guessed it is okay to assign error
to initializationError
. During initializing, detector.error
does return message like "No full shingle in current detection window" or "No data in current detection window", which can be helpful if we want to show details about initialization.
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.
Yeah, I see. Actually, that probably makes more sense. Sounds good to leave
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.
LGTM, just minor things. Nice!
keyword: 'Exceeded memory limit', | ||
cause: 'lack of memory', | ||
actionItem: | ||
"Try deleting or stop other detectors that you don't actively use, increase your cluster size, or reduce the number of features in this detector.", |
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.
another action item: scale up with an instance type of more memory
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.
done
Issue #, if available:
#46
Description of changes:
Add detector state page; fix minor issue that renderTime is not exported
Detector stopped:
Detector never started:
Detector is manually stopped:
Detector is initializing:
Detector initialization failure:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.