Attempt to fix issue #858 #859

Merged
merged 2 commits into from May 25, 2013

Conversation

Projects
None yet
2 participants
@jccomputing
Contributor

jccomputing commented May 24, 2013

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

This comment has been minimized.

Show comment Hide comment
@naparuba

naparuba May 25, 2013

Owner

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 :) ).

Owner

naparuba commented May 25, 2013

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)

This comment has been minimized.

Show comment Hide comment
@naparuba

naparuba May 25, 2013

Owner

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

@naparuba

naparuba May 25, 2013

Owner

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

This comment has been minimized.

Show comment Hide comment
@jccomputing

jccomputing May 25, 2013

Contributor

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.

@jccomputing

jccomputing May 25, 2013

Contributor

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.

This comment has been minimized.

Show comment Hide comment
@jccomputing

jccomputing May 25, 2013

Contributor

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

@jccomputing

jccomputing May 25, 2013

Contributor

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

- value = raw
- name_value = {name: raw}
+ name_value = {name: e.value}
+ if e.warning and e.critical:

This comment has been minimized.

Show comment Hide comment
@naparuba

naparuba May 25, 2013

Owner

Quite better than the len() way :p

@naparuba

naparuba May 25, 2013

Owner

Quite better than the len() way :p

@naparuba naparuba merged commit 456a823 into naparuba:master May 25, 2013

@naparuba

This comment has been minimized.

Show comment Hide comment
@naparuba

naparuba May 25, 2013

Owner

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? :)

Owner

naparuba commented May 25, 2013

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

This comment has been minimized.

Show comment Hide comment
@jccomputing

jccomputing May 27, 2013

Contributor

Hello

My name is: Alexandre Veyrenc

Contributor

jccomputing commented May 27, 2013

Hello

My name is: Alexandre Veyrenc

@naparuba

This comment has been minimized.

Show comment Hide comment
@naparuba

naparuba May 27, 2013

Owner

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

On Mon, May 27, 2013 at 9:23 AM, Jean-Claude Computing <
notifications@github.com> wrote:

Hello

My name is: Alexandre Veyrenc


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

Owner

naparuba commented May 27, 2013

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

On Mon, May 27, 2013 at 9:23 AM, Jean-Claude Computing <
notifications@github.com> wrote:

Hello

My name is: Alexandre Veyrenc


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

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