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

mysql_connections_per_user: fix for non-alphanumeric symbols in fields #783

Merged
merged 1 commit into from Jul 21, 2019

Conversation

n0guest
Copy link
Contributor

@n0guest n0guest commented Dec 11, 2016

Hi.

This is small fix for a non-alphanumeric symbols in field names plus print real username in label.

Copy link
Collaborator

@sumpfralle sumpfralle left a comment

Choose a reason for hiding this comment

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

Hello n0guest,

thank you for fixing the fieldname handling!

Please take a look at my comments in the code below.

If you feel like cleaning up a bit, you may also want to improve some more details of the plugin:

  • change the documentation into the POD style (see plugins/munin/munin_stats_plugins for a good example)
  • change the graph_category to "db" (see list of recommended categories)
  • improve test_service: it should always exit with zero - even for "no" cases. Additionally its call (around line 72) looks like the function would return (which is not the case).
  • add an example graph (see PluginGallery) to the repository

It would be great, if you could fix a few of these issues, as well. But of course, you may also feel free to just fix the fieldname handling.
Thank you!

@@ -100,6 +100,9 @@ sub print_graph_data() {
if($print_user eq "root") {
$print_user = "root_";
}
if ( $print_user =~ /[^A-Za-z0-9]/ ) {
$print_user =~ s/[^A-Za-z0-9]/_/;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use the perl module "Munin::Plugin" for generating a proper fieldname according to munin's restrictions. Just use clean_fieldname($print_user). This function also handles the "root" exception.
Thus you can avoid duplicate code.

@sumpfralle
Copy link
Collaborator

@n0guest: did I possibly overwhelm you with my wishlist? :)

In case you want to focus on this single issue, then I could also merge your proposal as it is. Just tell me ...

@n0guest
Copy link
Contributor Author

n0guest commented Jan 11, 2017

@n0guest: did I possibly overwhelm you with my wishlist? :)

Nope, you didn't. Just little overwhelmed at work within few last weeks. But i'll try to follow your requests at this week or next.

@sumpfralle
Copy link
Collaborator

@n0guest: just a friendly (and patient) reminder ...

@sumpfralle
Copy link
Collaborator

Ping?

@sumpfralle
Copy link
Collaborator

@n0guest: any news?

@sumpfralle sumpfralle merged commit 6635130 into munin-monitoring:master Jul 21, 2019
@sumpfralle
Copy link
Collaborator

I merged you original fix. Feel to improve the other bits of the plugins (mentioned in my first comment), if you feel like ...

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

Successfully merging this pull request may close these issues.

None yet

2 participants