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

Attempt to fix issue #858 #859

Merged
merged 2 commits into from
May 25, 2013
Merged

Conversation

jccomputing
Copy link
Contributor

Hello

Here is an attempt to fix issue #858. It works with perfdata containing spaces and optionally surrounded by single quotes.

It does not handle range format for warn and crit.

Tested on our production system with Shinken sending perfdata to Graphite. Maybe the same problem exists with PNP4Nagios, but since we don't use it, I didn't have the opportunity to check it.

@naparuba
Copy link
Contributor

oh even with a test case, great!

I'll just give a look into a graphite setup, and should be ok for 1.4, (just in time :) ).


name = self.illegal_char.sub('_', elts[0])
name = self.illegal_char.sub('_', e.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the perfdata is malformed? (like iwth no = ) name will be None isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a previous version I added a test case to cover perfdata with equal sign but no value and the perfdata is simply ignored because it doesn't match the second regular expression. I didn't test when there is no equal sign.

I will test it on monday if possible, and include more unittests to cover these corner cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About replacing all non word characters by underscores: it would be great if we could allow more characters, because Graphite allows more characters than that.

I tried to dig in Graphite's code to find the exact list of allowed characters, but to no avail. In fact Carbon can use a lot of characters on Unix, only / and null are forbidden, but currently graphite-web can't handle parenthesis, UTF-8, etc.

I think that until we have a clear view on what's allowed on Graphite's side, we have to stick to the current behavior.

You'll find some info in this issue: graphite-project/graphite-web#242

@naparuba naparuba merged commit 456a823 into shinken-solutions:master May 25, 2013
@naparuba
Copy link
Contributor

Thanks it's merged :) I try with a very bad format name, and the only errors were about unicode (that are now catched).

Which name can I put into the THANKS file? :)

@jccomputing
Copy link
Contributor Author

Hello

My name is: Alexandre Veyrenc

@naparuba
Copy link
Contributor

Welcome on the THANKS file so :) (and in the 1.4 changelog ;) )

On Mon, May 27, 2013 at 9:23 AM, Jean-Claude Computing <
[email protected]> wrote:

Hello

My name is: Alexandre Veyrenc


Reply to this email directly or view it on GitHubhttps://github.com//pull/859#issuecomment-18485704
.

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.

2 participants