-
Notifications
You must be signed in to change notification settings - Fork 18
Add 'No data' state for historical detectors #364
Add 'No data' state for historical detectors #364
Conversation
Will confirm with UX on the changes before merging. |
server/routes/utils/adHelpers.ts
Outdated
@@ -325,15 +325,18 @@ export const appendTaskInfo = ( | |||
return detectorMapWithTaskInfo; | |||
}; | |||
|
|||
// Four checks/transformations need to be made here: | |||
// Five checks/transformations need to be made here: |
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.
minor: change Five
to Following
, so that we don't need to change it every time we have new checks.
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.
Agreed, changed.
@@ -69,6 +69,7 @@ export enum DETECTOR_STATE { | |||
FEATURE_REQUIRED = 'Feature required', | |||
INIT_FAILURE = 'Initialization failure', | |||
UNEXPECTED_FAILURE = 'Unexpected failure', | |||
NO_DATA = 'No data', |
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.
Not related with this PR. But we may need to test more edge cases like not enough data, exceed our max running AD task limit etc.
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, agreed. I think this "no data" case can be pretty common, so definitely good to have it separated here, but definitely doesn't cover all cases that would currently fall under the 'Unexpected failure' state. I'm assuming those other cases would fall under the FAILED
state from the backend, and it's probably a matter of parsing out the error message to categorize into other states similar to what's done here.
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, we can also parse the error message for "FAILED" state. But not so easy for exceptions which has no readable error message.
Issue #, if available: #361
Description of changes:
This PR adds a 'No data' state to the historical detectors, to make it more clear to the user when a historical detector's detection date range doesn't return any results / no data to aggregate over. Previously, this error was captured under the vague 'Unexpected failure' detector state, and provided no actionable steps for the user to try to fix it.
Specific changes:
Screenshots:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.