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

Fix health check AlertNotification #2571

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

callmevladik
Copy link

@callmevladik callmevladik commented Nov 14, 2024

Describe the bug

AlertNotification has "Try again" button which doesn't do anything, the potential reason could be that some of the dependencies in useEffect are not updated or badly composed registerSetInterval function with some closure complexity.

To Reproduce

Steps to reproduce the bug:

  1. Go to any page with possible AlertNotification
  2. Mock error behaviour
  3. Click try again button, so that immediately after clicking the "healthz" request must be triggered instantly and AlertNotification must disappear, but actually nothing happens.

For testing purposes

As I didn't know how to mock server healthz response, I just setup [counter, setCounter] state, added condition "if counter % 2 === 0" inside performHealthCheck with setting error if true, else "return checkerFunction()...", also I added click handler which increments counter and invokes performHealthCheck with new counter value.

So with my mocking the scenario was:

  1. I see error, because counter === 0, I don't see any "healthz" requests.
  2. I click "Try again" and check browser network tab
  3. I see new "healthz" request instantly and AlertNotification disappears
  4. I see new "healthz" requests each 5 secs after

`export function PureAlertNotification({ checkerFunction }: PureAlertNotificationProps) {
const [networkStatusCheckTimeFactor, setNetworkStatusCheckTimeFactor] = React.useState(0);
const [error, setError] = React.useState<null | string | boolean>(null);
const { t } = useTranslation();
const { pathname } = useLocation();
const [counter, setCounter] = React.useState(0);

const performHealthCheck = React.useCallback((counter: number) => {
if (!window.navigator.onLine) {
setError('Offline');
return;
}

// Don't check for the cluster health if we are not on a cluster route.
if (!getCluster()) {
  setError(null);
  return;
}

if (counter % 2 === 0) {
  setError('Counter error');
} else {
  return checkerFunction()
    .then(() => setError(false))
    .catch(err => {
      setError(err.message);
      setNetworkStatusCheckTimeFactor(prev => prev + 1);
    });
}

}, []);

React.useEffect(() => {
const interval = setInterval(
() => performHealthCheck(counter),
(networkStatusCheckTimeFactor + 1) * NETWORK_STATUS_CHECK_TIME
);
return () => clearInterval(interval);
}, [performHealthCheck, networkStatusCheckTimeFactor, counter]);

// Make sure we do not show the alert notification if we are not on a cluster route.
React.useEffect(() => {
if (!getCluster()) {
setError(null);
}
}, [pathname]);

const showOnRoute = React.useMemo(() => {
for (const route of ROUTES_WITHOUT_ALERT) {
const routePath = getRoutePath(getRoute(route));
if (matchPath(pathname, routePath)?.isExact) {
return false;
}
}
return true;
}, [pathname]);

if (!error || !showOnRoute) {
return null;
}

const handleClick = () => {
setCounter(prev => {
const newValue = prev + 1;
performHealthCheck(newValue);
return newValue;
});
};

return (
<Alert
variant="filled"
severity="error"
sx={theme => ({
color: theme.palette.common.white,
background: theme.palette.error.main,
textAlign: 'center',
display: 'flex',
paddingTop: theme.spacing(0.5),
paddingBottom: theme.spacing(1),
paddingRight: theme.spacing(3),
justifyContent: 'center',
position: 'fixed',
zIndex: theme.zIndex.snackbar + 1,
top: '0',
alignItems: 'center',
left: '50%',
width: 'auto',
transform: 'translateX(-50%)',
})}
action={
<Button
sx={theme => ({
color: theme.palette.error.main,
borderColor: theme.palette.error.main,
background: theme.palette.common.white,
lineHeight: theme.typography.body2.lineHeight,
'&:hover': {
color: theme.palette.common.white,
borderColor: theme.palette.common.white,
background: theme.palette.error.dark,
},
})}
onClick={handleClick}
size="small"
>
{t('translation|Try Again')}

}
>
<Typography
variant="body2"
sx={theme => ({
paddingTop: theme.spacing(0.5),
fontWeight: 'bold',
fontSize: '16px',
})}
>
{t('translation|Lost connection to the cluster.')}


);
}`

AlertNotification has "Try again" button which doesn't do anything, because of some of the dependencies in useEffect are not updated or badly composed registerSetInterval function.
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Nov 14, 2024
@callmevladik callmevladik changed the title Fix AlertNotification Fix health check AlertNotification Nov 14, 2024
@skoeva
Copy link
Contributor

skoeva commented Nov 14, 2024

Hi, thanks for opening this PR! Regarding the issue here, it would be best to log it in the repo here

image

And then you can link the issue to this PR and avoid having a super long PR description. This should generally be reserved for a short description of the changes, along with any helpful instructions to test the changes for anyone reviewing.

Feel free to reference our contribution guidelines and reach out if you have any questions :)

@illume
Copy link
Collaborator

illume commented Nov 18, 2024

Hi @callmevladik

thanks for this fix! Appreciate it.

A few notes about the CI build errors:

  • The DCO check can be fixed by adding a sign off with git commit --amend -s
  • The test plugins failure looks to be unrelated to this PR (Please ignore, it will be fixed in another PR)
  • The frontend test failure is because of i18n. Please run make frontend-i18n-check from repo root to check it. I think if you added any translations you can run npm run i18n from within frontend/ which will generate empty translations for the other locales.

Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

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

I left a previous comment about how to address the CI job failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants