Skip to content

Serialize fields before queueing it to the workqueue #10420

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yhabteab
Copy link
Member

Previously, all the event handlers of our perfdata writers were directly enqueued into their own work queue, which then extracted all the required metrics from the checkable object. However, since this is done a differently different thread than the one that is used to process the check result, the checkable could change its state in the meantime leading to an inconsistent state. Therefore, all the required metrics are now extracted within the same thread that triggered the event and then enqueued into the work queue. Apart from that, I've removed a lot of useless if (cr) ... statements, since a CheckResult handler is never going to be called without a valid CheckResult.

fixes #10406

@cla-bot cla-bot bot added the cla/signed label Apr 24, 2025
@yhabteab yhabteab added bug Something isn't working area/metrics General metrics handling labels Apr 24, 2025
@yhabteab yhabteab requested a review from julianbrost April 24, 2025 08:59
@julianbrost
Copy link
Contributor

Can you please try to split that huge commit into smaller pieces, making it easier to follow the individual changes. The PR description already gives a good example of a part that could easily be split off:

Apart from that, I've removed a lot of useless if (cr) ... statements, since a CheckResult handler is never going to be called without a valid CheckResult.

Ideally, there are separate commits for moving and changing code, otherwise, changes happening in the same step tend do be quite hidden.

@yhabteab yhabteab force-pushed the bundled-perfdata-writers-fix branch from 4e0294f to 3df997c Compare April 25, 2025 10:32
@yhabteab
Copy link
Member Author

Can you please try to split that huge commit into smaller pieces, making it easier to follow the individual changes.

Done!

yhabteab added 2 commits May 13, 2025 15:31
They're just useless, since a `CheckResult` handler is never going to be
called without a check result and a checkable can't exist without a
checkcommand.
@yhabteab yhabteab force-pushed the bundled-perfdata-writers-fix branch 2 times, most recently from 4ab4b33 to 0430d47 Compare May 13, 2025 15:17
@yhabteab yhabteab requested a review from jschmidt-icinga May 13, 2025 15:18
@yhabteab yhabteab added this to the 2.15.0 milestone May 13, 2025
Copy link
Contributor

@jschmidt-icinga jschmidt-icinga left a comment

Choose a reason for hiding this comment

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

Looks good to me now.

@yhabteab
Copy link
Member Author

Sorry, I had to force push again because I missed one cr serialization to move into the workqueue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/metrics General metrics handling bug Something isn't working cla/signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Perfdata writers may use incorrect Checkable states
3 participants