synchronize access to OpenSSL::Cipher, use digest package for digests, fix metric mangling under high load

[frapex]

1) synchronize access to OpenSSL::Cipher and use digest package for digests, because OpenSSL::Digest and OpenSSL::Cipher are not thread safe.

Refer to issue https://github.com/logstash-plugins/logstash-codec-collectd/issues/14 for more information.

2) Fix metric mangling under high load

The plugin mangles collectd metrics under some conditions. The bug is triggered when (for example) the percent metrics are send in rapid succession, one after another.

Refer to issue https://github.com/logstash-plugins/logstash-codec-collectd/issues/15 for more information.

[untergeek]

Super excited about this! Thank you so much!

I assume the tests still pass. Do you think any other tests are necessary?

[frapex]

Changes were applied to logstash-codec-collectd-1.0.1 which does not contain a spec file. I have not ran the testsuite because of this. The current tests should pass.

I do not think more tests are required. The thread-safety and metric mangling issues were hard to trigger, even in a busy production environment. Will be even harder (if not impossible) to capture them in a test.

[untergeek]

It looks like there is a spec file in the 1.0.1 branch to me: https://github.com/logstash-plugins/logstash-codec-collectd/blob/9299d7660f3ea17eb807ba243751cdd38f08d9fb/spec/codecs/collectd_spec.rb

[frapex]

No spec file is included in the logstash-1.5.2 vendor bundle gem logstash-codec-collectd-1.0.1. Could you run the tests? I do not have all the development tools installed (and cannot install them either).

[untergeek]

Having some difficulties getting the packets properly signed:

  Sign
    should parse a correctly signed packet (FAILED - 1)
    should not parse an incorrectly signed packet (FAILED - 2)
  Drop NaN event
    should drop an event with a NaN value when 'nan_handling' set to drop
  Warn on NaN event
    should replace a NaN with a zero and receive a warning when 'nan_handling' set to warn

Failures:

  1) LogStash::Codecs::Collectd Sign should parse a correctly signed packet
     Failure/Error: subject.decode(payload) do |event|
     NotImplementedError:
       Unsupported MAC algorithm (HMAC[-]e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855)
     # ./lib/logstash/codecs/collectd.rb:343:in `verify_signature'
     # ./lib/logstash/codecs/collectd.rb:411:in `decode'
     # ./spec/codecs/collectd_spec.rb:185:in `(root)'
     # /Users/buh/.rvm/gems/jruby-1.7.19/gems/rspec-wait-0.0.7/lib/rspec/wait.rb:46:in `(root)'

  2) LogStash::Codecs::Collectd Sign should not parse an incorrectly signed packet
     Failure/Error: subject.decode(payload) do |event|
     NotImplementedError:
       Unsupported MAC algorithm (HMAC[-]e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855)
     # ./lib/logstash/codecs/collectd.rb:343:in `verify_signature'
     # ./lib/logstash/codecs/collectd.rb:411:in `decode'
     # ./spec/codecs/collectd_spec.rb:195:in `(root)'
     # /Users/buh/.rvm/gems/jruby-1.7.19/gems/rspec-wait-0.0.7/lib/rspec/wait.rb:46:in `(root)'

[frapex]

Good that you ran the tests! My previous commit broke security level sign. verify_signature() should use Digest::HMAC instead of OpenSSL::HMAC.

Tested with +/- 500 collectd instances producing around 1000 collectd events per second. The two UDP listeners run without issues, no exceptions raised. It looks pretty stable to me.

[untergeek]

Tests pass now, thanks for the updated code.

Can you add a line or two to the CHANGELOG.md file?

[untergeek]

By the way, this will be committed soon, but likely not published to rubygems.org until the release of Logstash 2.0 (which is a few weeks away, still).

[frapex]

I updated the changelog. Great to hear that the changes will be committed soon.

It would be nice to see that my other pull requests (https://github.com/logstash-plugins/logstash-input-udp/pull/13 and https://github.com/logstash-plugins/logstash-filter-throttle/pull/5) will be part of logstash-2.0 also.

[untergeek]

With regard to the udp input, you may need to do some refactoring with recent changes for Logstash 2.0. There are likely to be some major changes in the UDP input. See: https://github.com/logstash-plugins/logstash-input-udp/issues/11 and https://github.com/logstash-plugins/logstash-input-udp/issues/4. If you have any comments, you could add them there.

We're at an engineering all-hands meetup in Barcelona this week, but we're ironing out ways to more responsive to plugin PRs and issues as I write this. We're going to make some strides in this area.

[frapex]

Ready!

[elasticsearch-bot]

Merged sucessfully into master!