search

logstash-plugins / logstash-output-gelf

Apache License 2.0
15 stars 18 forks source link

Use .nil? for nil check #29

Open suhailpatel opened 6 years ago

suhailpatel commented 6 years ago

Currently the metadata nil check uses value == nil, unfortunately this causes issues with time fields and the time comparator with an exception:

[2017-11-15T23:01:51,491][FATAL][logstash.runner          ] An unexpected error occurred! {:error=>#<NoMethodError: undefined method `time' for nil:NilClass>, :backtrace=>["/usr/share/logstash/logstash-core/lib/logstash/timestamp.rb:13:in `<=>'", "org/jruby/RubyComparable.java:150:in `=='", "/usr/share/logstash/vendor/bundle/jruby/2.3.0/gems/logstash-output-gelf-3.1.4/lib/logstash/outputs/gelf.rb:146:in `block in receive'", "org/jruby/RubyHash.java:1343:in `each'", "/usr/share/logstash/vendor/bundle/jruby/2.3.0/gems/logstash-output-gelf-3.1.4/lib/logstash/outputs/gelf.rb:145:in `receive'", "/usr/share/logstash/logstash-core/lib/logstash/outputs/base.rb:92:in `block in multi_receive'", "org/jruby/RubyArray.java:1734:in `each'", "/usr/share/logstash/logstash-core/lib/logstash/outputs/base.rb:92:in `multi_receive'", "/usr/share/logstash/logstash-core/lib/logstash/output_delegator_strategies/legacy.rb:22:in `multi_receive'", "/usr/share/logstash/logstash-core/lib/logstash/output_delegator.rb:49:in `multi_receive'", "/usr/share/logstash/logstash-core/lib/logstash/pipeline.rb:538:in `block in output_batch'", "org/jruby/RubyHash.java:1343:in `each'", "/usr/share/logstash/logstash-core/lib/logstash/pipeline.rb:536:in `output_batch'", "/usr/share/logstash/logstash-core/lib/logstash/pipeline.rb:481:in `worker_loop'", "/usr/share/logstash/logstash-core/lib/logstash/pipeline.rb:439:in `block in start_workers'"]}

This modifies the code so the nil check is not an equality check but rather a Ruby nil? check

suhailpatel commented 6 years ago

I've signed the CLA but Github hasn't reflected it (https://www.elastic.co/contributor-agreement?license=icla).

Is there any step I need to take before it takes effect? Turns out leaving this comment refreshed the CLA status

suhailpatel commented 6 years ago

@jakelandis You seem to be the approver of PRs here, mind taking a look at this when you have a chance. I believe the bug is still present in the current versions

vitkovskii commented 5 years ago

@cwurm Hey :) Or somebody else, please merge this :)

Thanks a lot!