Remove metrics-elb-full in favor of metrics-elb + fix --scheme and --fetch_age #265

multani · 2017-12-11T10:35:58Z

Both were doing mostly the same, in slightly different ways.

Once fixed, metrics-elb.rb has more features than metrics-elb-full.rb
(such as --period and --statistics, which are not available in the -full
variant) and supports the AWS-SDK v2, which helps for #240

Pull Request Checklist

I had a look at the metrics-elb-full.rb. It's actually very similar to metrics-elb.rb, beside the SDK version used the differences are:

the -full variant properly process the --scheme flag, whereas the normal variant ignores it
the -full variant can look for data in the past using the --fetch_age flag as an offset respectively to the current time, the normal variant as the same flag which isn't use
the normal variant can directly look up a past value by passing a specific date, the -full variant can't
the normal variant can select the period, it's fixed to 60 seconds in the -full variant
by default, the -full variant looks up to 60 seconds in the past (--fetch_age is 60 seconds by default), whereas the normal variant just looks up to the current time.

I started to port metrics-elb-full.rb to the AWS-SDK v2, but I wondered if it was necessary and if it would be better to just drop this -full variant in favor of the other one (which is already using AWS-SDK v2). Also, while testing it, I realized it was displaying lot of garbage (see the tests artifacts below). I eventually did the following:

removed metrics-elb-full.rb (backward incompatibility)
moved the scheme support from metrics-elb-full.rb to metrics-elb.rb
fixed the --fetch_age flag (backward incompatibility)

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

Purpose

This helps to advance #240

Tests artifacts

$ bundle exec bin/metrics-elb-full.rb -r eu-west-1 -n test
Latency
{:average=>0.000667694029114056, :unit=>"Seconds", :timestamp=>2017-12-11 10:20:00 UTC}
test.Latency 0.000667694029114056 1512987600
RequestCount
{:unit=>"Count", :sum=>213.0, :timestamp=>2017-12-11 10:20:00 UTC}
test.RequestCount 213.0 1512987600
HTTPCode_Backend_2XX
{:unit=>"Count", :sum=>213.0, :timestamp=>2017-12-11 10:20:00 UTC}
test.HTTPCode_Backend_2XX 213.0 1512987600
HTTPCode_ELB_5XX
{:unit=>"Count", :sum=>295.0, :timestamp=>2017-12-11 10:20:00 UTC}
test.HTTPCode_ELB_5XX 295.0 1512987600

$ bundle exec bin/metrics-elb.rb -r eu-west-1 -n test
loadbalancer.test.latency.average 0.000667694029114056 1512987600
loadbalancer.test.requestcount.sum 213.0 1512987600
loadbalancer.test.httpcode_backend_2xx.sum 213.0 1512987600
loadbalancer.test.httpcode_elb_5xx.sum 295.0 1512987600

$ bundle exec bin/metrics-elb-full.rb -r eu-west-1 -n test -f 120
HTTPCode_ELB_5XX
{:unit=>"Count", :sum=>535.0, :timestamp=>2017-12-11 10:19:00 UTC}
test.HTTPCode_ELB_5XX 535.0 1512987540

$ bundle exec bin/metrics-elb.rb -r eu-west-1 -n test -f 120
loadbalancer.test.httpcode_elb_5xx.sum 535.0 1512987540

$ bundle exec bin/metrics-elb-full.rb -r eu-west-1 -n test -s xxx
Latency
{:average=>0.000667694029114056, :unit=>"Seconds", :timestamp=>2017-12-11 10:20:00 UTC}
xxx.test.Latency 0.000667694029114056 1512987600
RequestCount
{:unit=>"Count", :sum=>213.0, :timestamp=>2017-12-11 10:20:00 UTC}
xxx.test.RequestCount 213.0 1512987600
HTTPCode_Backend_2XX
{:unit=>"Count", :sum=>213.0, :timestamp=>2017-12-11 10:20:00 UTC}
xxx.test.HTTPCode_Backend_2XX 213.0 1512987600
HTTPCode_ELB_5XX
{:unit=>"Count", :sum=>295.0, :timestamp=>2017-12-11 10:20:00 UTC}
xxx.test.HTTPCode_ELB_5XX 295.0 1512987600

$ bundle exec bin/metrics-elb.rb -r eu-west-1 -n test -s xxx
xxx.test.latency.average 0.000667694029114056 1512987600
xxx.test.requestcount.sum 213.0 1512987600
xxx.test.httpcode_backend_2xx.sum 213.0 1512987600
xxx.test.httpcode_elb_5xx.sum 295.0 1512987600

Known Compatibility Issues

Documented in the CHANGELOG

majormoses · 2017-12-21T20:35:43Z

bin/metrics-elb.rb

@@ -52,7 +52,7 @@ class ELBMetrics < Sensu::Plugin::Metric::CLI::Graphite
         description: 'Metric naming scheme, text to prepend to metric',
         short: '-s SCHEME',
         long: '--scheme SCHEME',
-         default: ''
+         default: 'loadbalancer'


this technically is also a breaking change and should be called out as well. I'd also suggest that maybe elb names more sense as there are now the following types of load balaners in aws and where possible I'd go for more specific:

ElasticLoadBalancer

NetworkLoadBalancer

ApplicationLoadBalaner

as the name implies I'd prefer elb

I'm not sure to follow what you are asking for here: would you prefer to have elb or one of the other more specific name? I guess we could have this scheme auto configured respectively to the actual load balancer being monitored, but in that case I would make that into another PR.

As for the value itself:

it's a breaking change compared to the -full variant, I'll document it in the changelog.

I will change it to elb if you prefer. That would be a breaking change for the normal variant as well in this case (which had loadbalancer hardcoded.)

majormoses · 2017-12-21T20:42:24Z

bin/metrics-elb.rb

-      static_value['loadbalancer.' + load_balancer_name + '.' + key + '.' + static] = static
-      result['loadbalancer.' + load_balancer_name + '.' + key + '.' + static] = r[:datapoints][0] unless r[:datapoints][0].nil?
+      keys =
+        if config[:scheme].nil?


What scenario would this be nil in other than explicitly setting setting it to nil? It previously had a default of '' (empty string) which is not the same as nil:

$ irb irb(main):001:0> a = '' => "" irb(main):002:0> a.nil? => false

I can't see a scenario where it is even possible, it accepts a string value so even if you specified nil I would imagine it would return the string of 'nil'

Also the if should go on the same line as the assignment.

Good catch, the test should be on the empty string instead of nil, I copied/pasted the wrong code from another plugin apparently...

I also moved the if on the same line as the assignment, although it's not very consistent with the other checks :)

majormoses · 2017-12-21T20:51:19Z

bin/metrics-elb.rb

@@ -52,7 +52,7 @@ class ELBMetrics < Sensu::Plugin::Metric::CLI::Graphite
         description: 'Metric naming scheme, text to prepend to metric',
         short: '-s SCHEME',
         long: '--scheme SCHEME',
-         default: ''
+         default: 'loadbalancer'

  option :fetch_age,


After reading through the code it feels to me like fetch_age should be renamed start_time does that sound right? If so we are already doing lots of breaking changes and might as well make it make more sense.

See comment below

majormoses · 2017-12-21T20:52:22Z

bin/metrics-elb.rb

@@ -106,8 +106,8 @@ def cloud_watch_metric(metric_name, value, load_balancer_name)
        }
      ],
      statistics: [value],
-      start_time: config[:end_time] - config[:period],
-      end_time: config[:end_time],
+      start_time: config[:end_time] - config[:fetch_age] - config[:period],


Based on this it seems like we should rename fetch_age to start_time or am I missing something?

It's not really the start time, it's the duration in the past you want to get the metrics from.

It actually makes more sense than having a start_time flag:

using a "fetch age" flag, you don't have to specify the end time and can use a fixed "fetch age" duration to say: every time the check executes, get me the metrics from the last 60 seconds for example:

metrics-elb.rb --fetch_age 60s

using a "start time" flag, you would need to specify a specific time which would need to be updated every time you are executing the check (otherwise you get the same metrics all the times), which is not especially practical in the context in which Sensu works.

metrics-elb.rb --start-time 2018-02-06T17:45:15+01:00

Does it make more sense?

multani · 2018-02-06T16:56:45Z

@majormoses Thanks for the review and sorry for the late reply!
I took into account some of the changes you asked for, let me know how it goes.

There are some conflicts with bin/metrics-elb-full.rb due to the Rubocop cleaning you did, let me know if you prefer that I rebase the branch on top of master or if you prefer that I merge master in my branch and repush the result in this PR.

majormoses · 2018-02-07T08:04:37Z

prefer a rebase if you can.

…fetch_age Both were doing mostly the same, in slightly different ways. Once fixed, metrics-elb.rb has more features than metrics-elb-full.rb (such as --period and --statistics, which are not available in the `-full` variant) and supports the AWS-SDK v2, which helps for sensu-plugins#240

multani · 2018-02-07T09:30:36Z

On Wed, Feb 7, 2018, at 09:04, Ben Abrams wrote: prefer a rebase if you can.

Just did :)

majormoses

LGTM

Great work

majormoses · 2018-02-09T08:25:52Z

released: https://rubygems.org/gems/sensu-plugins-aws/versions/11.0.0

multani · 2018-02-09T09:00:44Z

@majormoses 💋

Can you also update #240 (comment)? metrics-elb-full.rb (well metrics-elb.rb) is now SDK v1 free 👍

majormoses requested changes Dec 21, 2017

View reviewed changes

majormoses requested a review from eheydrick December 21, 2017 20:53

majormoses added Impact: Breaking Change Status: Awaiting Response Status: Needs Review Type: Enhancement labels Dec 21, 2017

multani added 3 commits February 7, 2018 10:29

Add more breaking changes doc in the CHANGELOG. Default scheme to "elb"

b4eb33a

Fix the scheme comparison when it's empty

5e27219

multani force-pushed the fix-metrics-elb branch from 6ff1474 to 5e27219 Compare February 7, 2018 09:29

majormoses removed the Status: Awaiting Response label Feb 9, 2018

majormoses approved these changes Feb 9, 2018

View reviewed changes

majormoses merged commit 12bd392 into sensu-plugins:master Feb 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove metrics-elb-full in favor of metrics-elb + fix --scheme and --fetch_age #265

Remove metrics-elb-full in favor of metrics-elb + fix --scheme and --fetch_age #265

multani commented Dec 11, 2017

majormoses Dec 21, 2017

majormoses Dec 22, 2017

multani Feb 6, 2018

majormoses Dec 21, 2017

multani Feb 6, 2018

multani Feb 6, 2018

majormoses Dec 21, 2017

multani Feb 6, 2018

majormoses Dec 21, 2017

multani Feb 6, 2018

multani commented Feb 6, 2018

majormoses commented Feb 7, 2018

multani commented Feb 7, 2018 via email

majormoses left a comment

majormoses commented Feb 9, 2018

multani commented Feb 9, 2018

Remove metrics-elb-full in favor of metrics-elb + fix --scheme and --fetch_age #265

Remove metrics-elb-full in favor of metrics-elb + fix --scheme and --fetch_age #265

Conversation

multani commented Dec 11, 2017

Pull Request Checklist

General

Purpose

Tests artifacts

Known Compatibility Issues

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

multani commented Feb 6, 2018

majormoses commented Feb 7, 2018

multani commented Feb 7, 2018 via email

majormoses left a comment

Choose a reason for hiding this comment

majormoses commented Feb 9, 2018

multani commented Feb 9, 2018