Feature/allow zero numerator (#266)

* Add ability for numerator to be empty

CloudWatch returns empty data if there is no metric for it. This allows
us to set a default (0) to replace for the numeratort.

Remove the --no-data-ok flag because it doesn't make sense with the
above option. Instead we chang eit to --no-denominator-data-ok. If there
is no denominator data we will either `unknown` or `ok`.

Also refactored some to reuse more code. May refactor more.

* Allow backwards compatibility and add some tests

* Move #composite_check out of library, and update tests

* Split out some logic for easier legibility

* Fix rubo by disabling and adding changelog

Author: zbintliff

CHANGELOG.md

@@ -4,6 +4,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 This CHANGELOG follows the format listed [here](https://github.com/sensu-plugins/community/blob/master/HOW_WE_CHANGELOG.md)
 
 ## [Unreleased]
+- check-cloudwatch-composite-metric.rb: add flags `zero_denominator_data_ok`, `no_denominator_data_ok`, and `numerator_default` to add ability to allow numerator in composite to be 0. While leaving the functionality of `no_data_ok` the same, this change allows us to check to alert if the numerator has no data since 0/X is a valid alert case.
+- lib/cloudwatch-common.rb: added tests for majority of functions
 
 ## [10.0.3] - 2017-12-03
 ### Fixed

bin/check-cloudwatch-composite-metric.rb

@@ -85,14 +85,14 @@ class CloudWatchCompositeMetricCheck < Sensu::Plugin::Check::CLI
          description: 'CloudWatch metric unit'
 
   option :critical,
-         description: 'Trigger a critical when value is over VALUE',
+         description: 'Trigger a critical when value is over VALUE as a Percent',
          short: '-c VALUE',
          long: '--critical VALUE',
          proc: proc(&:to_f),
          required: true
 
   option :warning,
-         description: 'Trigger a warning when value is over VALUE',
+         description: 'Trigger a warning when value is over VALUE as a Percent',
          short: '-w VALUE',
          long: '--warning VALUE',
          proc: proc(&:to_f)
@@ -103,20 +103,97 @@ class CloudWatchCompositeMetricCheck < Sensu::Plugin::Check::CLI
          long: '--operator OPERATION',
          default: 'greater'
 
+  option :numerator_default,
+         long: '--numerator-default DEFAULT',
+         description: 'Default for numerator if no data is returned for metric',
+         proc: proc(&:to_f)
+
+  option :no_denominator_data_ok,
+         long: '--allow-no-denominator-data',
+         description: 'Returns ok if no data is returned from denominator metric',
+         boolean: true,
+         default: false
+
+  option :zero_denominator_data_ok,
+         long: '--allow-zero-denominator-data',
+         description: 'Returns ok if denominator metric is zero',
+         boolean: true,
+         default: false
+
   option :no_data_ok,
          short: '-O',
          long: '--allow-no-data',
-         description: 'Returns ok if no data is returned from the metric',
+         description: 'Returns ok if no data is returned from either metric',
          boolean: true,
          default: false
-
   include CloudwatchCommon
 
   def metric_desc
     "#{config[:namespace]}-#{config[:numerator_metric_name]}/#{config[:denominator_metric_name]}(#{dimension_string})"
   end
 
+  def numerator_data(metric_payload)
+    if resp_has_no_data(metric_payload, config[:statistics])
+      # If the numerator response has no data in it, see if there was a predefined default.
+      # If there is no predefined default it will return nil
+      config[:numerator_default]
+    else
+      read_value(metric_payload, config[:statistics]).to_f
+    end
+  end
+
+  # rubocop:disable Style/GuardClause
+  def composite_check
+    numerator_metric_resp = get_metric(config[:numerator_metric_name])
+    denominator_metric_resp = get_metric(config[:denominator_metric_name])
+
+    ## If the numerator is empty, then we see if there is a default. If there is a default
+    ## then we will pretend the numerator _isnt_ empty. That is
+    ## if empty but there is no default this will be true. If it is empty and there is a default
+    ## this will be false (i.e. there is data, following standard of dealing in the negative here)
+    no_num_data = numerator_data(numerator_metric_resp).nil?
+    no_den_data = resp_has_no_data(denominator_metric_resp, config[:statistics])
+    no_data = no_num_data || no_den_data
+
+    # no data in numerator or denominator this is to keep backwards compatibility
+    if no_data && config[:no_data_ok]
+      return :ok, "#{metric_desc} returned no data but that's ok"
+    elsif no_den_data && config[:no_denominator_data_ok]
+      return :ok, "#{config[:denominator_metric_name]} returned no data but that's ok"
+    elsif no_data ## This is legacy case
+      return :unknown, "#{metric_desc} could not be retrieved"
+    end
+
+    ## Now both the denominator and numerator have data (or a valid default)
+    denominator_value = read_value(denominator_metric_resp, config[:statistics]).to_f
+    if denominator_value.zero? && config[:zero_denominator_data_ok]
+      return :ok, "#{metric_desc}: denominator value is zero but that's ok"
+    elsif denominator_value.zero?
+      return :unknown, "#{metric_desc}: denominator value is zero"
+    end
+
+    ## We already checked if this value is nil so we know its not
+    numerator_value = numerator_data(numerator_metric_resp)
+    value = (numerator_value / denominator_value * 100).to_i
+    base_msg = "#{metric_desc} is #{value}: comparison=#{config[:compare]}"
+
+    if compare(value, config[:critical], config[:compare])
+      return :critical, "#{base_msg} threshold=#{config[:critical]}"
+    elsif config[:warning] && compare(value, config[:warning], config[:compare])
+      return :warning,  "#{base_msg} threshold=#{config[:warning]}"
+    else
+      threshold = config[:warning] || config[:critical]
+      return :ok, "#{base_msg}, will alarm at #{threshold}"
+    end
+  end
+  # rubocop:enable Style/GuardClause
+
   def run
-    composite_check config
+    status, msg = composite_check
+    if respond_to?(status)
+      send(status, msg)
+    else
+      unknown 'unknown exit status called'
+    end
   end
 end

lib/sensu-plugins-aws/cloudwatch-common.rb

@@ -3,7 +3,7 @@ module CloudwatchCommon
   include Common
 
   def client
-    Aws::CloudWatch::Client.new
+    @client ||= Aws::CloudWatch::Client.new
   end
 
   def read_value(resp, stats)
@@ -40,47 +40,13 @@ def metrics_request(config)
     }
   end
 
-  def composite_metrics_request(config, metric, fixed_time_now = Time.now)
-    {
-      namespace: config[:namespace],
-      metric_name: config[metric],
-      dimensions: config[:dimensions],
-      start_time: fixed_time_now - config[:period] * 10,
-      end_time: fixed_time_now,
-      period: config[:period],
-      statistics: [config[:statistics]],
-      unit: config[:unit]
-    }
+  def get_metric(metric)
+    client.get_metric_statistics(composite_metrics_request(metric))
   end
 
-  def composite_check(config)
-    fixed_time_now = Time.now
-    numerator_metric_resp = client.get_metric_statistics(composite_metrics_request(config, :numerator_metric_name, fixed_time_now))
-    denominator_metric_resp = client.get_metric_statistics(composite_metrics_request(config, :denominator_metric_name, fixed_time_now))
-
-    no_data = resp_has_no_data(numerator_metric_resp, config[:statistics]) || \
-              resp_has_no_data(denominator_metric_resp, config[:statistics])
-    if no_data && config[:no_data_ok]
-      ok "#{metric_desc} returned no data but that's ok"
-    elsif no_data && !config[:no_data_ok]
-      unknown "#{metric_desc} could not be retrieved"
-    end
-
-    denominator_value = read_value(denominator_metric_resp, config[:statistics]).to_f
-    if denominator_value.zero?
-      ok "#{metric_desc} denominator value is zero but that's ok"
-    end
-    numerator_value = read_value(numerator_metric_resp, config[:statistics]).to_f
-    value = (numerator_value / denominator_value * 100).to_i
-    base_msg = "#{metric_desc} is #{value}: comparison=#{config[:compare]}"
-
-    if compare(value, config[:critical], config[:compare])
-      critical "#{base_msg} threshold=#{config[:critical]}"
-    elsif config[:warning] && compare(value, config[:warning], config[:compare])
-      warning "#{base_msg} threshold=#{config[:warning]}"
-    else
-      ok "#{base_msg}, will alarm at #{!config[:warning].nil? ? config[:warning] : config[:critical]}"
-    end
+  def composite_metrics_request(metric)
+    ## config is a class variable but don't want to change signature
+    metrics_request(config).merge(metric_name: metric)
   end
 
   def self.parse_dimensions(dimension_string)

lib/sensu-plugins-aws/common.rb

@@ -17,8 +17,8 @@
 #
 
 module Common
-  def initialize
-    super()
+  def initialize(argv = ARGV)
+    super(argv)
     aws_config
   end