Add WAF metrics #275
Add WAF metrics #275
Conversation
|
Sorry I started reviewing this then closed the window without submitting the feedback, let me go make a pass and ensure I was complete with my review. |
There was a problem hiding this comment.
Overall I think this looks good. I see a few enhancements to make:
- leverage already existing library functions over redefining the same thing
- support multiple metric formatters, while I am not saying you need to add support for it but making it easier to add those in the future seems like a good idea minimally.
bin/metrics-waf.rb
Outdated
| @cloud_watch ||= Aws::CloudWatch::Client.new | ||
| end | ||
|
|
||
| def cloud_watch_metric(metric_name, value) |
There was a problem hiding this comment.
Is there something that this does not work for?
If so can we enhance it instead?
bin/metrics-waf.rb
Outdated
| require 'sensu-plugins-aws' | ||
| require 'time' | ||
|
|
||
| class WafMetrics < Sensu::Plugin::Metric::CLI::Graphite |
There was a problem hiding this comment.
rather than making it directly use graphite only format if possible we should use generic and use an option to control the output format. This means that someone using influxdb does not need to rewrite the check just add the option and choose to specify their format.
sensu-plugins/sensu-plugin#185
In order to use this new functionality we would need to bump sensu-plugin ~> 2.4 which is fine since we are already using ~> 2.0 it remains backwards compatible.
|
@majormoses - fixed comments |
Pull Request Checklist
Is this in reference to an existing issue?
General
Update Changelog following the conventions laid out here
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 Compatibility Issues
sensu-plugins/community#97