search

sensu-plugins / sensu-plugins-aws

This plugin provides native AWS instrumentation for monitoring and metrics collection, including: health and metrics for various AWS services, such as EC2, RDS, ELB, and more, as well as handlers for EC2, SES, and SNS.
http://sensu-plugins.io
MIT License
81 stars 143 forks source link

All checks should pull from a standard library #1

Open mattyjones opened 9 years ago

mattyjones commented 9 years ago

There should be a single set of functions/class that all the checks utilize, there is a large amount of duplicated code here that can be extracted out with minimal effort.

bkett commented 9 years ago

@mattyjones When I refactored in aws-sdk, I made an attempt at this. The biggest problem at the time was there is a known issue in the library that sensu checks uses which prevented me from inheriting options. We can of course refactor out a lot of the other code, but without being able to inherit options in the checks it might be fairly underwhelming.

I'll update this post with the issue/lib I'm referring to. IIRC it's something opscode developed that hasn't been updated in a year.

mattyjones commented 9 years ago

@bkett When I look at this gem in particular I feel it is fairly critical and almost a 'core' gem due to the nature or all our environments. With that being said I certainly feel any extra effort in this case is effort very well spent. I am not as familiar with the aws-sdk gem as I should be so I am of limited help at this time, I had to write all my Amazon scripts with boto due to a language requirement of Python.

bkett commented 9 years ago

You could write Python in your Ruby! (jk)

Alright I'll work on it this weekend :)

mattyjones commented 9 years ago

@bkett Don't even go there. I have been using Ruby for about 6 months after using Python for several years. Every code review I go to at least one developer will smile and say this is really good Python code. Apparently I write awesome Python using Ruby :+1:

bkett commented 9 years ago

@mattyjones haha I feel for you, I'm in the exact same boat!

bkett commented 9 years ago

Ah yup, here's the blocking PR that's preventing me from inheriting options from mixlib/cli (which sensu::plugin uses). https://github.com/chef/mixlib-cli/pull/13

Given that this issue has been known for well..over a year and nothing's been done on the PR for more than 6 months I'm not going to focus on refactoring those out and instead have the base class contain more general methods spread across most of the plugins.

mattyjones commented 9 years ago

thats sounds fair enough

mattyjones commented 9 years ago

@bkett OT: I just dropped this as well GIR. Is it not ready for mainstream yet but as the repos multiply, it will become essential and I will be adding a lot more to it over the next ~week. If any of the tasks help, have at it.

bkett commented 9 years ago

@mattyjones Thanks, I'll check GIR out.

I decided to try and do some more investigation on the options issue, and I found a sort of hack around not getting inheritance from Mixlib by using composition. Good news is I will be able to refactor out the options. The upside to this approach is I can create classes containing the oft repeated options (aws_key for instance). The downside is I have to instantiate one of these objects in every check and then, (I'm still exploring but thus far I haven't found a workaround) I have to run parse_options on each object and then join the resulting hash with the one in the check.

I'll keep digging but the scope has gotten a little bigger than I initially planned so it'll take some more time :)

mattyjones commented 9 years ago

Time is what we have, none of us get paid for this and life goes one ;) Any work or help is appriciated by all.

Thanks!

bkett commented 9 years ago

@mattyjones Hey, quick update:

So I noticed that you set the aws-sdk gem in the gemspec to be greater than 2.0. I'd like to revert this to 1.0 and come up with a plan for transitioning to 2.0 at a later time for the reason that, well, 2.0 breaks ALL the things!

So I'm going to be setting it to the last version of 1.x (1.63.0) and we can move forward later.

sstarcher commented 9 years ago

For a standard library here is what I did in my fork. I can do it across the rest of the code if this is what you're looking for.
https://github.com/sstarcher/sensu-plugins-aws/commit/dfeb1a8419117339c6c9a44687e339c78682f940

Note that code also has conversion to aws-sdk v2

mattyjones commented 9 years ago

@bkett ^^ any thoughts

bkett commented 9 years ago

@mattyjones Sorry I've been AWOL for so long on the gem. I have been operating under aws-sdk-v1 so if you would prefer to upgrade then this would probably be a breaking change to everything under my fork.

sstarcher commented 9 years ago

@mattyjones I could certainly put in a pull request just for my code that adds the common module as it's not specifically tied into the aws-sdk v2 upgrade, but I have already upgraded 2 of the checks.

sstarcher commented 9 years ago

In relation to this I created a pull request for a new check which uses my common code.

16