Add a monitoring tab on the modern host details page #94
Conversation
This allows querying of the host results, which can be used in the UI or otherwise.
| const dispatch = useDispatch(); | ||
| const API_KEY = `get-monitoring-results-{hostId}`; | ||
| const status = useSelector(state => selectAPIStatus(state, API_KEY)); | ||
| const { results, itemCount, response } = useSelector(state => | ||
| selectAPIResponse(state, API_KEY) | ||
| ); | ||
|
|
||
| const fetchResults = useCallback( | ||
| () => { | ||
| if (!hostId) return; | ||
| dispatch( | ||
| get({ | ||
| key: API_KEY, | ||
| url: `/api/hosts/${hostId}/monitoring/results`, | ||
| params: { | ||
| per_page: 'all', | ||
| }, | ||
| }) | ||
| ); | ||
| }, | ||
| [API_KEY, dispatch, hostId], | ||
| ); | ||
|
|
||
| useEffect(() => { | ||
| fetchResults(); | ||
| }, [fetchResults]); |
There was a problem hiding this comment.
Can you use useAPI instead?
e.g.:
const {
response: { results },
status,
} = useAPI('get', `/api/hosts/${hostId}/monitoring/results`, `get-monitoring-results-{hostId}`);There was a problem hiding this comment.
I copied it from #94 so I wonder why that doesn't use useAPI.
There was a problem hiding this comment.
After playing with this for a while I think useAPI is an awkward API if you want to handle the error properly (like display Permission Denied). The response object is suddenly an instance of Error (from axios) that has a property response, which is not obvious.
Speaking of error handling, I wish there was some generic way to show an error.
There was a problem hiding this comment.
we are indeed not very consistent across the app,
though basically useAPI came to make things simpler, you can see it's less lines, imports and complexity..
it's implementation is basically what you did here which is also fine.
you can pass to the useAPI some options including:
errorToast: callback function that will trigger a toast notification with a message that you can set based on the error. e.g:errorToast: error => errorhandleError: also a callback that you can choose what to do with.- same also for
successToastandhandleSuccess
Hope it helps
| const STATUS_ICONS = { | ||
| 'ok': 'pficon-ok status-ok', | ||
| 'warning': 'pficon-info status-warn', | ||
| 'critical': 'pficon-error-circle-o status-error', | ||
| } |
There was a problem hiding this comment.
instead of using classnames, can you use the icons from PF4?
import {
ExclamationTriangleIcon,
CheckCircleIcon,
ExclamationCircleIcon,
} from '@patternfly/react-icons';const STATUS_ICONS = {
ok: CheckCircleIcon,
warning: ExclamationTriangleIcon,
critical: ExclamationCircleIcon,
}later I believe you can use it this way:
<Td>{STATUS_ICONS[result.status]} {result.status_label}</Td>There was a problem hiding this comment.
That is much nicer. My university programming professor would have yelled at me for using hardcoded strings instead of constants.
There was a problem hiding this comment.
This worked for me:
const STATUS_ICONS = {
'ok': CheckCircleIcon,
'warning': ExclamationTriangleIcon,
'critical': ExclamationCircleIcon,
};
const STATUS_STYLES = {
'ok': 'ok',
'warning': 'warn',
'critical': 'critical',
};
const status_icon = (result_status) => {
const cls = STATUS_ICONS[result_status] || QuestionCircleIcon;
const style = STATUS_STYLES[result_status] || 'question';
return createElement(cls, { className: `status-${style}` });
};There was a problem hiding this comment.
looks good,
do you still need to use the style?
if you do, there's an option to pass the className into the Icon component, so no need for the createElement
There was a problem hiding this comment.
I certainly needed createElement because the imported item is a class that you need to instantiate.
The style provides a color
|
I appreciate the review. This really was me trying to figure all this stuff out by reading existing code. I'll try to get back to it after the holidays |
|
For which hosts is this shown? I would expect, that it is only shown for monitored hosts? |
The tab is shown for all hosts, but the table with services is only rendered if there's a monitoring proxy assigned. |
Would it be possible to hide it for hosts, which are not monitored? |
This first introduces a REST API to get the monitoring results for a host. That is then shown as a tab on the host details page.
I debated a card in the details tab, but this leaves space for more complex items, such as downtime selection.
Some review needs to be done on the exact representation. I chose for a simple table, but perhaps we want to show more columns that also indicates downtime and acknowledgements.