Fix check-jenkins-health crash on 500 status response

[mdzidic]

Added fix to not raise exceptions but return the response.

Error log:

Check failed to run: 500 Internal Server Error, ["/opt/sensu/embedded/lib/ruby/gems/2.3.0/gems/rest-client-2.0.0/lib/restclient/abstract_response.rb:223:in exception_with_response'", "/opt/sensu/embedded/lib/ruby/gems/2.3.0/gems/rest-client-2.0.0/lib/restclient/abstract_response.rb:103:inreturn!'", "/opt/sensu/embedded/lib/ruby/gems/2.3.0/gems/rest-client-2.0.0/lib/restclient/request.rb:860:in process_result'", "/opt/sensu/embedded/lib/ruby/gems/2.3.0/gems/rest-client-2.0.0/lib/restclient/request.rb:776:inblock in transmit'", "/opt/sensu/embedded/lib/ruby/2.3.0/net/http.rb:853:in start'", "/opt/sensu/embedded/lib/ruby/gems/2.3.0/gems/rest-client-2.0.0/lib/restclient/request.rb:766:intransmit'", "/opt/sensu/embedded/lib/ruby/gems/2.3.0/gems/rest-client-2.0.0/lib/restclient/request.rb:215:in execute'", "/opt/sensu/embedded/lib/ruby/gems/2.3.0/gems/rest-client-2.0.0/lib/restclient/request.rb:52:inexecute'", "/opt/sensu/embedded/lib/ruby/gems/2.3.0/gems/rest-client-2.0.0/lib/restclient/resource.rb:51:in get'", "/etc/sensu/plugins/check-jenkins-health.rb:73:inrun'", "/opt/sensu/embedded/lib/ruby/gems/2.3.0/gems/sensu-plugin-1.4.4/lib/sensu-plugin/cli.rb:58:in `block in '"]

Pull Request Checklist

Is this in reference to an existing issue?

10

General

• [ ] Update Changelog following the conventions laid out on Keep A Changelog

• [ ] Update README with any necessary configuration snippets

• [ ] Binstubs are created if needed

• [ ] RuboCop passes

• [ ] Existing tests pass

Purpose

Known Compatibility Issues

[volatilemolotov]

Tried this and it works.

[majormoses]

@mdzidic any chance you plan on coming back and improving this?

[majormoses]

@mdzidic any chance of picking this back up?

[majormoses]

@mdzidic last ping before closing this.

[mdzidic]

@majormoses sorry for late reply, I'm not skilled enough for alternative solution, I tried few tricks but without success, are You able to write another logic?

[majormoses]

I will take a look this weekend if I have some time.

[majormoses]

this fell off my radar, I can't see what is going on with travis and can't seem to restart it:

I am gonna close and re-open the issue hoping it will start building and I can see the CI issues that need to be fixed regardless of the nicer way of doing it.

[majormoses]

I checked it out locally and saw these rubocop violations:

$ bundle exec rake default
/home/babrams/.rbenv/versions/2.3.5/bin/ruby -I/home/babrams/.rbenv/versions/2.3.5/lib/ruby/gems/2.3.0/gems/rspec-core-3.7.1/lib:/home/babrams/.rbenv/versions/2.3.5/lib/ruby/gems/2.3.0/gems/rspec-support-3.7.1/lib /home/babrams/.rbenv/versions/2.3.5/lib/ruby/gems/2.3.0/gems/rspec-core-3.7.1/exe/rspec
No examples found.

Finished in 0.00024 seconds (files took 0.04305 seconds to load)
0 examples, 0 failures

Files:           7
Modules:         2 (    2 undocumented)
Classes:         6 (    1 undocumented)
Constants:       7 (    7 undocumented)
Attributes:      0 (    0 undocumented)
Methods:         6 (    6 undocumented)
 23.81% documented
Running RuboCop...
/home/babrams/.rubocop.yml: Naming/AccessorMethodName has the wrong namespace - should be Style
/home/babrams/.rubocop.yml: Layout/SpaceBeforeFirstArg has the wrong namespace - should be Style
Inspecting 12 files
.......W....

Offenses:

bin/check-jenkins-health.rb:73:125: C: Space missing to the left of {.
    r = RestClient::Resource.new("#{https}://#{config[:server]}:#{config[:port]}#{config[:uri]}", timeout: config[:timeout]){|response, request, result| response }.get
                                                                                                                            ^
bin/check-jenkins-health.rb:73:125: C: Space between { and | missing.
    r = RestClient::Resource.new("#{https}://#{config[:server]}:#{config[:port]}#{config[:uri]}", timeout: config[:timeout]){|response, request, result| response }.get
                                                                                                                            ^^
bin/check-jenkins-health.rb:73:137: W: Unused block argument - request. If it's necessary, use _ or _request as an argument name to indicate that it won't be used.
    r = RestClient::Resource.new("#{https}://#{config[:server]}:#{config[:port]}#{config[:uri]}", timeout: config[:timeout]){|response, request, result| response }.get
                                                                                                                                        ^^^^^^^
bin/check-jenkins-health.rb:73:146: W: Unused block argument - result. If it's necessary, use _ or _result as an argument name to indicate that it won't be used.
    r = RestClient::Resource.new("#{https}://#{config[:server]}:#{config[:port]}#{config[:uri]}", timeout: config[:timeout]){|response, request, result| response }.get
                                                                                                                                                 ^^^^^^
bin/check-jenkins-health.rb:73:161: C: Line is too long. [167/160]
    r = RestClient::Resource.new("#{https}://#{config[:server]}:#{config[:port]}#{config[:uri]}", timeout: config[:timeout]){|response, request, result| response }.get
                                                                                                                                                                ^^^^^^^

12 files inspected, 5 offenses detected
RuboCop failed!

After doing some minor fixup:

$ bundle exec rake default
/home/babrams/.rbenv/versions/2.3.5/bin/ruby -I/home/babrams/.rbenv/versions/2.3.5/lib/ruby/gems/2.3.0/gems/rspec-core-3.7.1/lib:/home/babrams/.rbenv/versions/2.3.5/lib/ruby/gems/2.3.0/gems/rspec-support-3.7.1/lib /home/babrams/.rbenv/versions/2.3.5/lib/ruby/gems/2.3.0/gems/rspec-core-3.7.1/exe/rspec
No examples found.

Finished in 0.00025 seconds (files took 0.03589 seconds to load)
0 examples, 0 failures

Files:           7
Modules:         2 (    2 undocumented)
Classes:         6 (    1 undocumented)
Constants:       7 (    7 undocumented)
Attributes:      0 (    0 undocumented)
Methods:         6 (    6 undocumented)
 23.81% documented
Running RuboCop...
/home/babrams/.rubocop.yml: Naming/AccessorMethodName has the wrong namespace - should be Style
/home/babrams/.rubocop.yml: Layout/SpaceBeforeFirstArg has the wrong namespace - should be Style
Inspecting 12 files
............

12 files inspected, no offenses detected

If travis is still grumpy I will merge it and figure out what is going on.

[majormoses]

Well I still can't see the job log but the tests passed so I am :+1: to merge this.

[majormoses]

@mdzidic can I get you to add a changelog entry per our guidelines?

[majormoses]

@mdzidic I had some time and decided to fix it by actually rescuing specific exceptions, added ones not specifically related to this but ones I saw value in as well.

@mdzidic @volatilemolotov12 can you please retest it and if it works then we should be good to merge and release this.

[yuri-zubov]

I think it's better to use this fix https://github.com/sensu-plugins/sensu-plugins-jenkins/pull/33

[yuri-zubov]

I have just added your check too

[mdzidic]

So, what should we do now? 😄

[majormoses]

I think they are both roughly equivalent from what I see one returns the whole object the other only returns the response as we properly rescue I don't see anything in the new one that makes it better after I changed this PR. I'd like to better understand the benefits before making a decision.

[yuri-zubov]

@majormoses - yes, you absolutely right.

this implementation gives us

bin/check-jenkins-health.rb -t 15 -u /metrics/DEGQDAEPwAYhkLVw_2UCepQhw_15Mf28J2HDdVa21dd7l5rkuuJ-lzS22iahdIDd/healthcheck

JenkinsMetricsHealthChecker CRITICAL: request failed: {"disk-space":{"healthy":false,"message":"Only 0.267 Gb free on (master)"},"plugins":{"healthy":true,"message":"No failed plugins"},"temporary-space":{"healthy":false,"message":"Only 0.267 Gb free on (master)"},"thread-deadlock":{"healthy":true}}

my implementation gives us

bin/check-jenkins-health.rb -t 15 -u /metrics/DEGQDAEPwAYhkLVw_2UCepQhw_15Mf28J2HDdVa21dd7l5rkuuJ-lzS22iahdIDd/healthcheck

JenkinsMetricsHealthChecker CRITICAL: Jenkins health check 'disk-space' reported unhealthy state. Message: Only 0.267 Gb free on (master)

In my opinion answer more friendly from my code

if you are going to merge this PR this code must be changed

if [200, 500].include? r.code

to

if r.code == 200

because if you receive 500 from the server exemption will be executed on this line

rescue RestClient::ExceptionWithResponse => e
    critical "failed to #{e.response}"

[majormoses]

I'm curious to hear what others think but I think this is one of those rare moments where we can have our cake and eat it. We could add a --verbose flag and conditionally return the response whole object instead of just the response message.

because if you receive 500 from the server exemption will be executed

True while it would never mean anything we should remove it anyways to not confuse people to think that the below code applies to 500's anymore.

[majormoses]

@yuri-zubov @mdzidic as I did not hear back and I saw #25 was closed I assumed that this was a good idea, if you could please test it to make sure it looks good I will merge and release this tonight or tomorrow.

[majormoses]

I will fix this up tonight and release it now that it is tested and we are on the same page.

[majormoses]

released: https://rubygems.org/gems/sensu-plugins-jenkins/versions/1.6.1

Thanks everyone for bearing with me on this one, I think the community is much better off because of the effort.