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

Various fixes to contrib/unbound_munin_ file #440

Merged
merged 5 commits into from Jun 15, 2021
Merged

Various fixes to contrib/unbound_munin_ file #440

merged 5 commits into from Jun 15, 2021

Conversation

kimheino
Copy link
Contributor

@kimheino kimheino commented Mar 9, 2021

The most important fix is "fix statistics after unbound restart / server reboot". Other changes are mostly cosmetic.

"unbound-control stats" lists only query types that has been seen
after unbound restart. Munin requires list of all types ever seen,
or the generated graphs are mostly empty after restart.

Fix this by adding a state file with list of seen query types etc.
Copy link
Member

@gthess gthess left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time for this!
I have some questions inline before accepting this.

Not a munin user but I am worried if this change catches other users by surprise or if it is not relevant to other users.
Do you see value in having an environment variable to toggle this new behavior? Then based on the environment variable update_seentags() could just write the state content (filtered) to the seentags file and the old behavior is also kept by default.

contrib/unbound_munin_ Outdated Show resolved Hide resolved
contrib/unbound_munin_ Outdated Show resolved Hide resolved
contrib/unbound_munin_ Outdated Show resolved Hide resolved
contrib/unbound_munin_ Show resolved Hide resolved
@gthess gthess self-assigned this May 26, 2021
@kimheino
Copy link
Contributor Author

Not a munin user but I am worried if this change catches other users by surprise or if it is not relevant to other users.
Do you see value in having an environment variable to toggle this new behavior? Then based on the environment variable update_seentags() could just write the state content (filtered) to the seentags file and the old behavior is also kept by default.

Old behavior has two fatal errors:

  1. Munin reports error if you restart unbound (tag vanishes).
  2. New tags will not be included to Munin's graph if they are not present at the first run of munin-async. So basically graph stays empty if you reboot and are using munin-async.

New behavior might include some tags that stays zero forever, but that should not matter. So no, I don't see any point including config option for switching between old/new.

I'll reply inline for others.

@gthess gthess merged commit 722d851 into NLnetLabs:master Jun 15, 2021
@gthess
Copy link
Member

gthess commented Jun 15, 2021

LGTM now, thanks for explaining the reason behind the changes.

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

Successfully merging this pull request may close these issues.

None yet

2 participants