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

Check for case where field in fetch but not config #1553

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

Conversation

mtillberg
Copy link
Contributor

Better handling when a plugin has a field in fetch that wasn't in config. This specifically prevents inserting id=null rows into the state table.

Note: Munin 2.0 would display these fields with a "No .label provided" label and default drawing rules. These are now dropped.

@yunal
Copy link
Contributor

yunal commented Jul 3, 2023

I would sort of wanted to make mistakes more obvious and push them towards the UI and not hide them in logs. But at least, according to the comments, this will be in the logs.

@steveschnepp
Copy link
Member

steveschnepp commented Jul 3, 2023

@yunal I'm actually undecided. Having then not show up on the UI has actually its benefits, since someone will look deeper if he doesn't see his graph. And then having it in the logs makes it obvious.

Whereas the old 2.0 behavior was kindof not forcing folks to fix wrong plugins.

So. It will break some working-before plugins, which I don't like. Yet those where wrong in the first place.

In a nutshell, it feels like a -Werror which I usually like 😉

Better handling when a plugin has a field in fetch that wasn't in
config. This specifically prevents inserting id=null rows into the state
table.

Note: Munin 2.0 would display these fields with a "No .label provided"
label and default drawing rules. These are now dropped.
@steveschnepp
Copy link
Member

steveschnepp commented Jul 3, 2023

Oh, but upon looking at the code I see that :

  • the rrd are not updated either. That's bad as it will destroy data silently.
  • the logging is not very explicit, and only present if DEBUG is enabled, which is mostly never.

Once those 2 points are ok, I'm happy to merge it.

@mtillberg
Copy link
Contributor Author

For logging, there is a warning that's triggered in _db_state_update. In the syslog for my forced error case, I'm getting:

munin-update: ds_id(bind9_server_stats, requests_with_edns_0_received, 1688380931, 63091) is NULL, SELECT ds.id FROM ds...

For the data, it's an issue since rrd:file is a ds attribute, but there's no ds record. The pre-patch behavior was to lose data, specifically from _create_rrd_file:
In RRD: Error updating : opening '': No such file or directory
the patch just prevents the extra error.

I'll look at capturing the data and adding a UI record.

@steveschnepp
Copy link
Member

Modifying the UI isn't really needed. Just have a very explicit WARNING log fired is more than enough.

And, thanks for capturing the data inside an RRD.

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

Successfully merging this pull request may close these issues.

3 participants