Added `-R` to check-process.rb for RSS checks #46

jgnagy · 2017-06-16T21:40:23Z

Added -R to check-process.rb to address #45.
Also includes some minor rubocop fixes

Pull Request Checklist

Is this in reference to an existing issue?
Yes, #45.

General

Update Changelog following the conventions laid out on Keep A Changelog
Update README with any necessary configuration snippets
Binstubs are created if needed
RuboCop passes
Existing tests pass

New Plugins

Tests
Add the plugin to the README
Does it have a complete header as outlined here

Purpose

Known Compatablity Issues

Also includes some minor rubocop fixes

majormoses

Overall looks good just some minor tweaks and some clarification

majormoses · 2017-06-24T16:15:00Z

CHANGELOG.md

@@ -4,6 +4,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 This CHANGELOG follows the format listed at [Keep A Changelog](http://keepachangelog.com/)

 ## [Unreleased]
+## Changed


## Added as this is new functionality not changing existing.

majormoses · 2017-06-24T16:17:35Z

bin/check-process.rb

@@ -287,7 +300,7 @@ def run

    if config[:metric]
      # #YELLOW
-      count = procs.map { |p| p[config[:metric]].to_i }.reduce { |a, b| a + b } # rubocop:disable SingleLineBlockParams
+      count = procs.map { |p| p[config[:metric]].to_i }.reduce { |a, e| a + e }


I don't understand this change or why it would fix rubocop you just renamed the variables...can you expand?

This comes from Style/SingleLineBlockParams. Here are the tests rubocop runs for an example of what led to the message:

Name `reduce` block params `|a, e|`.

I'm perfectly happy to put things back how I found them, but I guessed that a previous committer may not have fully understood the SingleLineBlockParams cop and could have avoided the rubocop:disable. Switching b to e makes everything happy.

Might have had something to do with my editor vs. running rubocop from the CLI, but I ran bundle exec rubocop and needed to make this change for it to pass locally, though Atom was perfectly happy before I made the change.

Let me know how to proceed.

I don't see a real change so unless someone can explain why one is good and the other is bad I am going to assume that this is a false positive, highly subjective / opinionated, or just a bad rule. I would prefer to ignore behavior rather than make a change without a reason. Would you be open to creating an issue on rubocop and feel free to tag me on this? If there is something that makes this special I would like to know.

I'll undo this change in a commit in just a second.

For your reference, the docs for rubocop for this rule are here:

http://rubocop.readthedocs.io/en/latest/cops_style/#stylesinglelineblockparams

The relevant section is {"reduce"=>["acc", "elem"]}, {"inject"=>["acc", "elem"]}.

I'm guessing acc there is the "acculumator value (memo)" and elem is for "element" from the Ruby Standard Library docs for Enumerable. So shortening those names to a and e makes sense. Opinionated? Perhaps. But at least I can see where the logic comes from.

Either way, not trying to impose more rules than this repo's maintainers would care to adhere to, so I'll revert this change.

majormoses · 2017-06-26T05:51:51Z

bin/check-process.rb

    procs.select! { |p| p[:rss].to_f > config[:rss] } if config[:rss]
+    # Ensure RSS is under this value


This comment is wrong...swap it with the comment above.

I can clarify (or remove entirely) the comments I added there, but I'm not sure they are incorrect. Looking at both RSS-related selects:

# Ensure RSS is over this value procs.select! { |p| p[:rss].to_f > config[:rss] } if config[:rss] # Ensure RSS is under this value procs.select! { |p| p[:over_rss].to_f < config[:over_rss] } if config[:over_rss]

The first (and original) selects only those processes (p) whose reported RSS (:rss) is greater than the supplied value in config (thus my comment of # Ensure RSS is over this value).

The second selects only those processes whose reported RSS is less than the supplied value in the config (thus my comment of # Ensure RSS is under this value). Albeit, the use of "under" and the config key of :over_rss is not super clear, my intention on the new -R is to alert when the RSS value goes over the value you specify on the command line, so I think it still fits.

That said, my last comment made me see that the process (p) key should be :rss for that comparison. You'll see another commit in just a second.

that must have been what threw me off, makes perfect sense now.

This still looks wrong to me:

# Alert if RSS is over this value + option :over_rss,

+ # Ensure RSS is over this value procs.select! { |p| p[:rss].to_f > config[:rss] } if config[:rss] + # Ensure RSS is under this value + procs.select! { |p| p[:rss].to_f < config[:over_rss] } if config[:over_rss]

I would write it like this which makes more sense to me:

# Alert if RSS is under this value + option :rss_under, # Alert if RSS is over this value + option :rss_over,

+ # Ensure RSS is over this value procs.select! { |p| p[:rss].to_f > config[:rss_over] } if config[:rss_over] + # Ensure RSS is under this value + procs.select! { |p| p[:rss].to_f < config[:rss_under] } if config[:rss_under]

I am wondering if we should just always log a message saying something to this effect:

INFO: checking if there are (greater|less) than (warn|crit) thresholds CheckProcess OK: Found 6 matching processes; cmd /google/; rss > 368381

Actually I am almost tempted to say there should be no defaults for any of the crit/warnings if we want to stay flexible and keep it all in the same check. Let the user tell us what they want to check as we cant have sane defaults for both scenarios as they are mutually exclusive from what I see.

This would obviously be a breaking change @eheydrick thoughts?

I'm thankful that I'm not the only one that finds this check's options less than intuitive. I'm happy with postponing / canceling this PR until this discussion is concluded. I'll probably just write a really simple check to do just what I need for now. I'm also happy to help contribute to a library too if that would be valuable.

you can do what you need with the existing features as outlined above. you just need to set -w, -c to what they should be. Let's close out this PR and continue the discussion in the issue. I really do want to improve this but what is proposed is already available so unless we make things more intuitive I don't see anything that should be addressed here.

majormoses · 2017-06-26T06:45:24Z

This looks good now, I will take a second pass and hopefully do some testing and release tomorrow.

majormoses · 2017-06-28T16:10:04Z

closing per: #46 (comment)

Jonathan Gnagy added 2 commits June 16, 2017 14:35

Added -R to check-process.rb to address sensu-plugins#45

13f4dd8

Also includes some minor rubocop fixes

Updating Changelog

70d2fd8

majormoses self-requested a review June 24, 2017 16:14

majormoses requested changes Jun 24, 2017

View reviewed changes

majormoses added the Feedback Requested label Jun 24, 2017

Jonathan Gnagy added 2 commits June 25, 2017 19:54

Correcting CHANGELOG per @majormoses

c75579d

Even more correct CHANGELOG... h2 -> h3

8e555a6

majormoses requested changes Jun 26, 2017

View reviewed changes

Jonathan Gnagy added 2 commits June 25, 2017 23:21

Fixing bad process hash key in :over_rss compare

bca53db

Reverting rubocop-related reduce var name change

fd34458

majormoses closed this Jun 28, 2017

majormoses mentioned this pull request Jun 28, 2017

check-process.rb filtering behavior does not work as expected #45

Closed

majormoses mentioned this pull request Jul 12, 2017

check-process.rb has too many options and is too hard to use #48

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `-R` to check-process.rb for RSS checks #46

Added `-R` to check-process.rb for RSS checks #46

jgnagy commented Jun 16, 2017 •

edited

Loading

majormoses left a comment

majormoses Jun 24, 2017

jgnagy Jun 26, 2017

majormoses Jun 24, 2017

jgnagy Jun 26, 2017

majormoses Jun 26, 2017 •

edited

Loading

jgnagy Jun 26, 2017 •

edited

Loading

majormoses Jun 26, 2017

jgnagy Jun 26, 2017

jgnagy Jun 26, 2017

majormoses Jun 26, 2017 •

edited

Loading

majormoses Jun 28, 2017 •

edited

Loading

majormoses Jun 28, 2017

majormoses Jun 28, 2017

majormoses Jun 28, 2017

jgnagy Jun 28, 2017

majormoses Jun 28, 2017

majormoses commented Jun 26, 2017

majormoses commented Jun 28, 2017

		procs.select! { \|p\| p[:rss].to_f > config[:rss] } if config[:rss]
		# Ensure RSS is under this value

Added -R to check-process.rb for RSS checks #46

Added -R to check-process.rb for RSS checks #46

Conversation

jgnagy commented Jun 16, 2017 • edited Loading

Pull Request Checklist

General

New Plugins

Purpose

Known Compatablity Issues

majormoses left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majormoses Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

jgnagy Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majormoses Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

majormoses Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majormoses commented Jun 26, 2017

majormoses commented Jun 28, 2017

Added `-R` to check-process.rb for RSS checks #46

Added `-R` to check-process.rb for RSS checks #46

jgnagy commented Jun 16, 2017 •

edited

Loading

majormoses Jun 26, 2017 •

edited

Loading

jgnagy Jun 26, 2017 •

edited

Loading

majormoses Jun 26, 2017 •

edited

Loading

majormoses Jun 28, 2017 •

edited

Loading