Add filter for stats name #52

rthouvenin · 2017-09-18T19:41:39Z

Pull Request Checklist

Implements #43. Adds a command line option to metrics-per-process.py to filter with a regexp the stats to output. Prints everything by default for backward compatibility. The stats that are filtered out are not queried, which avoids getting permission errors for stats that are not required.

General

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

majormoses

Overall it looks ok ither than being unnecessarily complex. I would prefer if we followed other established patterns like the one in redis: https://github.com/sensu-plugins/sensu-plugins-redis/blob/2.2.1/bin/metrics-redis-graphite.rb I am not saying I am 100% forcing you to change but but I am certainly encouraging a more simple (which are typically less error prone). Also I need a testing artifact per: https://github.com/sensu-plugins/community/blob/master/PULL_REQUEST_PROCESS.md#7-testing-artifacts

majormoses · 2017-09-20T07:34:26Z

bin/metrics-per-process.py

+  'io_counters.read_bytes': lambda ctx: ctx.io_counters().read_bytes,
+  'io_counters.write_bytes': lambda ctx: ctx.io_counters().write_bytes,
+}
+MEMORY_STATS = ['rss', 'vms', 'shared', 'text', 'lib', 'data', 'dirty']


you should freeze constants.

you mean use a tuple instead of a list?

disregard I was clearly too tired when I reviewed this.

majormoses · 2017-09-20T07:41:13Z

bin/metrics-per-process.py

@@ -208,6 +227,12 @@ def main():
    dest    = 'graphite_scheme',
    metavar = 'GRAPHITE_SCHEME')

+  parser.add_option('-m', '--metrics',


I think that a name like this: https://github.com/sensu-plugins/sensu-plugins-redis/blob/2.2.1/bin/metrics-redis-graphite.rb#L94-L98 would be better

Sure. add_option('-k', '--filter-keys', ..., 'keys', 'KEYS_REGEXP')?

Is this an inclusive or exclusive filter? If inclusive I think --keys-regex otherwise --skip-keys-regex but I do not feel super strongly about it. @eheydrick ping

That or --metrics-regex given that metrics is the thing being collected and more specific.

I'd be 👍 with --metric-regexes to make it obvious that we are not requiring a single regex but a list of regexes to be used.

But it's actually a single regex, isn't it? Or do we want to have a comma-separated list of regexes to make it easier for the user to write the regex? IMO, separating keys with a | in a single regex isn't more difficult or very different from several regexes separated with a ,.

that is true when you have very simple regexes but the more complex the more it is a pain to look at. I'd prefer a comma separated list of regexes as this follows established patterns in both graphite and redis plugins. If we think that is overkill I am ok with limiting the scope but prefer consistency where possible.

majormoses · 2017-09-20T07:45:45Z

bin/metrics-per-process.py

@@ -89,6 +90,21 @@
 'CLOSING',
 'NONE'
 ]
+HANDLER_STATS = {


seems like there is a much simpler way to do this, if it matches the regex just call it in the loop (no need for a lambda) otherwise just move on. It's not that I am opposed to using lambda's when it makes sense this just seems overly complex without any real benefit over something more straightforward.

I also wasn't sure about which way to go. Without lambdas would be simpler, but it would replace the loop with a large collection of ifs like:

if metrics_regexp.match('cpu.user'): stats['cpu.user'] = process_handler.cpu_times().user

It's not as DRY as the lambdas because the stat name is repeated, as well as the "pattern" of checking the match and storing in the dictionary. If the loss in simplicity is not worth the gain, I have no problem going for the collection of ifs.

I tend to write really good python in my ruby so I have never really been opposed to a bunch of if statements, much easier to follow and less rubyisms (not that lambdas are really a rubyism) are I think desired in plugins where possible. Ideally with plugins I try to write it in the most simplest of forms. This means that traditional nocs, sysadmins, etc that do not really want to learn a ton of coding can still follow and fix bugs but that might just be me. At my work I take a similar stance for a very different reason, if I need to think about it at 3 AM (when I care the most about fixing an issue quickly so I can go back to bed) when I was just pulled out of bed does it make sense, if not it either needs some healthy comments explaining what is going on or it needs to be re-written.

+1 for comments on "tricky" looking code.

Makes total sense, I'll go for the if statements.

rthouvenin · 2017-09-30T15:43:42Z

Here is a gist with the output of a few test runs at this stage: https://gist.github.com/rthouvenin/f43ab687b85c7543b781668a3ac7b8a2

majormoses

One minor tweak on naming but otherwise I think it looks good. @eheydrick you want to take another pass at it?

majormoses · 2017-09-30T17:21:35Z

bin/metrics-per-process.py

@@ -107,60 +109,115 @@ def find_pids_from_name(process_name):
            pass
  return pids

-def stats_per_pid(pid):
-  '''Gets process stats using psutil module
+def other_stats(process_handler, metrics_regexp):


I think maybe something likeadditional_stats is more descriptive.

Done, let me know if I should squash all the commits

Nah, its good github now has an easy squash + merge button.

and honestly the history is not bad here.

majormoses · 2017-10-04T23:38:23Z

released: https://rubygems.org/gems/sensu-plugins-process-checks/versions/2.5.0

Add filter for stats name (sensu-plugins#43)

87eb6c8

majormoses reviewed Sep 20, 2017

View reviewed changes

majormoses added the Feedback Requested label Sep 20, 2017

majormoses requested a review from eheydrick September 20, 2017 08:00

rthouvenin added 2 commits September 30, 2017 16:40

Turn lamba collection into if collection

424fca0

Accept multiple regexes

1aa3d1d

majormoses requested changes Sep 30, 2017

View reviewed changes

majormoses added Status: Awaiting Response and removed Feedback Requested labels Oct 2, 2017

Better name for other_stats

c493725

majormoses added Type: Enhancement and removed Status: Awaiting Response labels Oct 4, 2017

majormoses merged commit fb21c75 into sensu-plugins:master Oct 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add filter for stats name #52

Add filter for stats name #52

rthouvenin commented Sep 18, 2017 •

edited

Loading

majormoses left a comment

majormoses Sep 20, 2017

rthouvenin Sep 25, 2017

majormoses Sep 26, 2017

majormoses Sep 20, 2017

rthouvenin Sep 25, 2017 •

edited

Loading

majormoses Sep 26, 2017

eheydrick Sep 26, 2017

majormoses Sep 26, 2017

rthouvenin Sep 26, 2017

majormoses Sep 26, 2017

rthouvenin Sep 26, 2017

majormoses Sep 20, 2017

rthouvenin Sep 25, 2017

majormoses Sep 26, 2017

eheydrick Sep 26, 2017

rthouvenin Sep 26, 2017

rthouvenin commented Sep 30, 2017

majormoses left a comment •

edited

Loading

majormoses Sep 30, 2017 •

edited

Loading

rthouvenin Oct 2, 2017

majormoses Oct 4, 2017

majormoses Oct 4, 2017

majormoses commented Oct 4, 2017

Add filter for stats name #52

Add filter for stats name #52

Conversation

rthouvenin commented Sep 18, 2017 • edited Loading

Pull Request Checklist

General

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

rthouvenin Sep 25, 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

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

rthouvenin commented Sep 30, 2017

majormoses left a comment • edited Loading

Choose a reason for hiding this comment

majormoses Sep 30, 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 commented Oct 4, 2017

rthouvenin commented Sep 18, 2017 •

edited

Loading

rthouvenin Sep 25, 2017 •

edited

Loading

majormoses left a comment •

edited

Loading

majormoses Sep 30, 2017 •

edited

Loading