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

update reboot required metric in main rebootRequired func #919

Closed

Conversation

jackfrancis
Copy link
Collaborator

This PR moves the prometheus metric posting mechanism inside the rebootRequired func itself rather than doing a separate, parallel check every minute.

The downside to this is that the periodicity of the metric (and thus the resolution) will be reduced (assuming you're running a periodic config of more than once per minute.

Signed-off-by: Jack Francis <jackfrancis@gmail.com>
@evrardjp
Copy link
Collaborator

evrardjp commented May 13, 2024

You're touching something I have done on my fork long ago... But always hesitated to bring here.

The plus side is the fact it's easier to debug: The metrics represent the "kured view" as a whole, not only a goroutine view.

The negative side is a change of behaviour, which for me is very hard on ppl, because now it really behaves differently.

Next to that the method rebootRequired was not really a pure function, but it could be made more pure by extracting the logging. Now it cannot be made cleaner.

I think the whole method rebootRequired could do with a refactor, but I feel this is a step not heading to the right direction without a larger refactor.

What was the reason you wanted to clean the goroutine @jackfrancis ? Is there something that annoyed you in the current code?

Copy link

This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants