Add ability to check multiple values on optional flag

[project0]

Did some refactor to check multiple elements, which can be enabled by --element_regex_global.
Replaced the define do_exit by a Nagios module to handle the exit message and code.
Add rubocop definition.

Implements #32

[phrawzty]

Wow, this is a big one, and I'd like to take some time to review it properly. 😄

[phrawzty]

This will break compatibility for old Rubies. 😢 Given the, er, quaint nature of many a Nagios server out there, I'm hesitant to pull this trigger. Thoughts?

[project0]

Really? I didint seen a required binding to the rubygems lib. Do you know what breaks in 1.8.7?

[phrawzty]

(pr/33) $ ruby --version
ruby 1.8.7 (2013-06-27 patchlevel 374) [x86_64-linux]

(pr/33) $ gem list

*** LOCAL GEMS ***

json (1.8.3)

(pr/33) $ ./check_http_json.rb
./check_http_json.rb:22:in `require': no such file to load -- json (LoadError)
	from ./check_http_json.rb:22

(pr/33 *) $ grep -n rubygems ./check_http_json.rb 
22:require 'rubygems'

(pr/33 *) $ ./check_http_json.rb
UNKNOWN: Insufficient or incompatible arguments.
Must specify target URI or file.
Must specify a desired element.
Must specify an expected result OR the warn and crit thresholds.
UNKNOWN: "./check_http_json.rb --help" for more information.

[project0]

i re-added the rubygems with an comment for ruby compatibility

[phrawzty]

I understand the interest of using a module here, and I've no qualm with the code you've produced; however, I'm curious as to the why. As in: in this particular code base, why replace the otherwise functioning do_exit function with a module at all?

Part of the reason that everything is a function is because I wanted to keep the code simple and readable by sysadmins who might not otherwise be programmers. If one is coming from a world of, say, Bash, functions are easy to digest. By introducing a self-referencing class, as well as the getter & setter pattern, you're also introducing more advanced (and mildly Ruby-specific) concepts.

Again, this is something that I was actively trying to avoid in this particular code base. 😄 I'm open to debate on the topic though!

[project0]

Because this is a self instance which is required to hold the current state of the code level.
A module is accessible everywhere in the code, which reduces the complexity of usage ;-)

[project0]

As a compromise we can replace the getter/setter magic by native functions for better readability.

[phrawzty]

The module is accessible everywhere, but so is a global variable, for example. I realise that this isn't The Ruby Way™, but it is simple, and that matters. Can you see a way to implement this functionality using variable instead of a module? If not, that's cool, but I'd like to explore that option.

[phrawzty]

Thanks for cleaning up the trailing whitespace and stuff all over the place. 👍

[phrawzty]

I ran some manual tests (TODO: automated tests 😝) in the following versions:

• 1.8.7-p374
• 1.9.3-p551
• 2.4.0

After re-adding require rubygems for 1.8.7, I can confirm that this PR does indeed satisfy #32, so that's great!

Beyond the line-specific commentary above, I'd like to address the output a little bit. I don't know if I have any answers, but it's worth addressing since the new functionality may need to have amplified output and/or additional explanation in the docs.

Consider:

(pr/33) $ ./check_http_json.rb -u http://jsonplaceholder.typicode.com/users -E id -w 11 -c 15 --element_regex_global
OK: 

In the above example, the script (correctly) exits 0, but the OK message is empty. In every other instance (including other OK conditions) it contains something, which means this feels incorrect and could lead to a bad user experience.

Consider:

(pr/33) $ ./check_http_json.rb -u http://jsonplaceholder.typicode.com/users -E id -W 11 -C 15 --element_regex_global
OK: 9.id does not match 11 or 15

Ok, but what about [0-8].id - did they match? Although we can interpret the meaning from knowledge of the code, it's not necessarily intuitive.

Consider:

./check_http_json.rb -u http://jsonplaceholder.typicode.com/users -E id -w 5 -c 15 --element_regex_global
WARN: 9.id is above threshold value 5 (10)

This is true; however [5-8].id are also above the threshold value, which may be important to know as well.

The user experience here is important. I'm open to any thoughts you have on how to make this easier / smoother for the poor sysadmins that have to interpret these results at 02:00.

[phrawzty]

For the sake of completeness, the full output of the endpoint above is here: http://pastebin.com/fU6BRKNs

[project0]

WARN: 9.id is above threshold value 5 (10)
This is true; however [5-8].id are also above the threshold value, which may be important to know as well.

Yes, this is difficult to catch or represent in a pretty way. We are limited on the nagios output "one" line -> so i decided its enough to send out the last warning. On critical it exits immediately.
In reality the user will inspect the system and checks the related issues to the monitoring problem.
Of course, we can add all occurred issues, but then we need to reduce the message size, which may not be convenient.

Same like the OK message, but we can output the element/threshold/result parameter here. This will help to understand that the given options are used to verify everything is ok
For example:

./check_http_json.rb -u http://jsonplaceholder.typicode.com/users -E id -w 11 -c 15 --element_regex_global
OK: Elements(Regex) 'id' are within thresholds W:11 C:15
./check_http_json.rb -u http://jsonplaceholder.typicode.com/users -E id -W 11 -C 15 --element_regex_global
OK: Elements(Regex) 'id' did not match 11 or 15

[phrawzty]

Yes, this is difficult to catch or represent in a pretty way. We are limited on the nagios output "one" line -> so i decided its enough to send out the last warning. On critical it exits immediately.
In reality the user will inspect the system and checks the related issues to the monitoring problem.
Of course, we can add all occurred issues, but then we need to reduce the message size, which may not be convenient.

You make a good point. This is something we can play with later in any case, so let's not address it right now.

Same like the OK message, but we can output the element/threshold/result parameter here. This will help to understand that the given options are used to verify everything is ok

Yes - the OK message should definitely have some sort of positive confirmation in it.

[phrawzty]

In 1.8.7 this gets interpreted literally, as in:

(pr/33) $ ./check_http_json.rb -u http://jsonplaceholder.typicode.com/users -e 1.id -r 2
OK: %s does match %s1.id2

In 1.9.3 and 2.4.0:

(pr/33) $ ./check_http_json.rb -u http://jsonplaceholder.typicode.com/users -e 1.id -r 2
OK: ["%s does match %s", ["1.id", "2"]]

[phrawzty]

Same issue here with 1.8.7:

OK: %s does match %sFirst '1.id' (regex)2

1.9.3 and 2.4.0:

OK: ["%s does match %s", ["First '1.id' (regex)", "2"]]

[phrawzty]

Thanks for the conversation and reactivity on this PR. 💯

I think we're OK to merge from a code perspective; however, let's be sure to open an Issue to update the README to reflect the new functionality.

Ok with you?

[project0]

LGTM. Lets merge ;-)