Closed c10l closed 8 years ago
I agree with your idea. I added an inline comment about the behavior when a json parse failure occurs. Let me know your thoughts :)
Ok, I went ahead and did it. I was going to squash the commits into just one but then I imagine this discussion would be mostly lost or unintelligible. Would it be a good idea to do that? Also, I wanted to add specs for this but I'm not sure how to do that.
If you feel comfortable squashing your commits, I welcome you to do it.
In terms of adding tests, it's up to you. (Please excuse any unknown jargon that follows) You can setup an input w/ json as codec, then send a gelf message over udp that is malformed and assert that an error is thrown, for example. Alternately, instead of using real networking you could make a test double of UDPSocket and make it return some pre-fabricated-and-invalid JSON when UDPSocket#recvfrom
is called. If you want help/guidance on this, let me know. Happy to help :)
Ok, I have hacked up a couple of specs. I don't find the way I did it particularly beautiful though, so strong criticisms and suggestions are most welcome! :smile:
@jordansissel can you take another look?
Is anything happening here? :) Would really love to see that in the next release of the plugin. We currently have a huge problems with failed json parsing. :-(
@bjoernhaeuser summing up a bunch of things to go across all the latest updates and issues and start to releasing these. I'll go through and get it out soon.
@vervas I'm in the process of rebasing this PR on the current master branch. Tests are still failing but I should push an update soon.
I am +1 on the idea of guarding against LogStash::Json::ParserError
but as mentioned in the code comments, I would not also preventively catch all other exceptions.
@colinsurprenant Since the parsing now occurs in a class method, how do I use the logger? @logger
is not instantiated for that scope.
Never mind, I found out. Pushed an updated version, please review.
@cassianoleal great stuff! But did you forget to push your logger mod? the logging statements are gone now...
I removed the logger because it doesn't seem to be used in any other plugins that deal with this kind of parse failure. I can add them back, but are they necessary? The messages are still going through the Logstash plumbing, correctly tagged with _jsonparsefailure
, which should be enough to identify and/or alert when a failure happens.
I'm happy either way.
@cassianoleal hm, I juste reviewed our _jsonparsefailure
and logging strategies and it is in fact rather inconsistent, in one case we don't log, in another we log warn and in a 3rd we log error.
Since this is in fact an error, I believe we should log at error level. I will go ahead and open an issue to clean this up in all json parsing plugins.
FYI, I created elastic/logstash#4857 to keep an eye on those json parsing error tagging+logging inconsistencies.
@colinsurprenant Log messages added (https://github.com/elastic/logstash/issues/4857). Mocking instead of monkey patching.
I've also deleted a spurious .mvn/extensions.xml
file that creeped into one of the commits -- no idea where that came from...
As always, please review and let me know if there's anything I can do better.
@cassianoleal where is the class method logger
defined? I only see it in the base LogStash::Plugin
class but as a private method?
Nice! we're getting there :D just clarify my logger
question and the constant string thingy and we'll be good to go!
That's precisely the logger it uses. If you look closely, that self.logger
is not actually a private class method. This might be a bug, but the private
directive is being called within the scope of the object, not the class.
@cassianoleal you are absolutely right, private sets the default access for subsequent object instance method, not class methods. I wonder if the intent was to make it a private class method? in any case I'd argue that it should be public so that we can use it with exactly this pattern here.
LGTM bump version, update the changelog and :shipit: - @vervas, did you want to manage that release?
hey @cassianoleal, are you taking care of bumping the version and updating the changelog?
@vervas nope, I'm just a humble PR contributor :smile:
My misunderstanding, thx :) Merged
oh, sorry, too late reply, but for the future, PR authors can certainly bump version: it's only a matter of increasing the version number in the gemspec file and adding an entry in the changelog. once we have the LGTM we usually do that last, as a final commit just prior to merging. But it can also be done by the maintainer too.
+1 for this fix
+1
@PhaedrusTheGreek @nikhuber this is already released but you have to manually install it with in your logstash setup since the latest version is not bundled by default.
@vervas Which version is the update in ?
I'm getting a warning about upgrading:
You are updating logstash-input-gelf to a new version 3.0.1, which may not be compatible with 2.0.7. are you sure you want to proceed (Y/N)?
Thanks!
2.0.8 should be your safest choice but 3.0.1 should be OK if you have the right version of Logstash. Have a look at https://github.com/logstash-plugins/logstash-input-gelf/blob/master/CHANGELOG.md to make sure you are covered.
@vervas , I'm getting version does not exist
$ bin/plugin install --version 2.0.8 logstash-input-gelf
...
Plugin logstash-input-gelf version 2.0.8 does not exist
ERROR: Installation aborted, verification failed for logstash-input-gelf 2.0.8
I don't see the Gem online either: https://rubygems.org/gems/logstash-input-gelf/
@PhaedrusTheGreek Thanks for pointing this out. I'll try to address this asap.
@vervas I am still getting that the version 2.0.8 does not exist:
# bin/plugin install --version 2.0.8 logstash-input-gelf
The use of bin/plugin is deprecated and will be removed in a feature release. Please use bin/logstash-plugin.
Ignoring ffi-1.9.13 because its extensions are not built. Try: gem pristine ffi --version 1.9.13
Validating logstash-input-gelf-2.0.8
Plugin logstash-input-gelf version 2.0.8 does not exist
ERROR: Installation aborted, verification failed for logstash-input-gelf 2.0.8
Logstash is 2.3.4:
# yum info logstash
Loaded plugins: fastestmirror
Loading mirror speeds from cached hostfile
Installed Packages
Name : logstash
Arch : noarch
Epoch : 1
Version : 2.3.4
Release : 1
Size : 136 M
Repo : installed
From repo : logstash
Summary : An extensible logging pipeline
URL : http://www.elasticsearch.org/overview/logstash/
License : ASL 2.0
Description : An extensible logging pipeline
@vervas trying to install in Logstash 2.3.4 , I'm getting the following:
├ $ bin/plugin install --version 2.0.8 logstash-input-gelf
The use of bin/plugin is deprecated and will be removed in a feature release. Please use bin/logstash-plugin.
Ignoring ffi-1.9.13 because its extensions are not built. Try: gem pristine ffi --version 1.9.13
Validating logstash-input-gelf-2.0.8
Installing logstash-input-gelf
Plugin version conflict, aborting
ERROR: Installation Aborted, message: Bundler could not find compatible versions for gem "logstash-core-plugin-api":
In snapshot (Gemfile.lock):
logstash-core-plugin-api (= 1.20.0)
In Gemfile:
logstash-input-ganglia (>= 0) java depends on
logstash-core-plugin-api (~> 1.0) java
logstash-input-ganglia (>= 0) java depends on
logstash-core-plugin-api (~> 1.0) java
logstash-input-ganglia (>= 0) java depends on
logstash-core-plugin-api (~> 1.0) java
.... Many More Dependency Messages ...
Any ideas on a workaround?
Logstash 2.4.0 won't take v2.0.8 either due to this dependency issue. I noticed that Logstash 5.x ships with v3.0.2 of the plugin - does that version contain the fix?
@vervas is there a working version for the latest stable release of logstash? We're losing a lot of logs because of this
Update: nvm, logstash 2.4.0 supports version 3.0.2 /cc @PhaedrusTheGreek
@alex88 @PhaedrusTheGreek Unfortunately I'm not involved with the project anymore since I couldn't really help. If you feel you can support this you can try to contact one of the original authors so that you can take over as a maintainer.
Not really, actually we're trying to migrate away from logstash
This should at least mitigate issue #26