tag _cefparsefailure on parse failure #26
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
breml
reviewed
Oct 10, 2016
There was a problem hiding this comment.
Over all it looks good to me, only a few minor suggestions.
The CEF specification () states in section Header Field Definitions that the version (field cef_version) is an integer. Maybe we should throw an exception if this is not the case as it is not valid CEF. This should go after https://github.com/logstash-plugins/logstash-codec-cef/pull/26/files#diff-60997264fb4df8acacfb3987dc3049a5L95
| @@ -114,6 +117,9 @@ def decode(data) | |||
| end | |||
|
|
|||
| yield event | |||
| rescue => e | |||
| @logger.error("Failed to decode event payload. Generating failure event with payload in message field.", :error => e.message, :backtrace => e.backtrace, :data => data) | |||
| yield LogStash::Event.new("message" => data, "tags" => ["_cefparsefailure"]) | |||
There was a problem hiding this comment.
logstash-filter-json allows to configure the tag for the failure case: https://github.com/logstash-plugins/logstash-filter-json/blob/master/lib/logstash/filters/json.rb#L60
should we provide similar behavior?
There was a problem hiding this comment.
they most used one (codec json) doesn't allow it, so I'm not that concerned, maybe we can have this as is and eventually add it if needed?
| @@ -114,6 +117,9 @@ def decode(data) | |||
| end | |||
|
|
|||
| yield event | |||
| rescue => e | |||
| @logger.error("Failed to decode event payload. Generating failure event with payload in message field.", :error => e.message, :backtrace => e.backtrace, :data => data) | |||
There was a problem hiding this comment.
I suggest to add some kind of reference to CEF to the error log message. While checking the logs it would be easier to understand the error.
Maybe something like: "Failed to decode CEF payload." ...
|
Forgot to paste the link to the CEF specification: https://www.protect724.hpe.com/servlet/JiveServlet/downloadBody/1072-102-9-20354/CommonEventFormatv23.pdf |
|
@jsvd Today I wanted to review your PR and let the spec tests run on my machine. Unfortunately this does not work. May be you can give me an hint what might be the problem. After updating to java 1.8.0_101 and running The same also happens on current master of this plugin. Therefore I think this problem is not related to your PR. |
|
@breml strange, I tried to replicate your issue with jruby 1.7.19 but failed to: |
|
@jsvd I tried again today even with removal of jruby and reinstallation with the same steps you executed. The bad news is: it does still not work completely. Does this mean anything to you? |
|
Today I tried again and I first removed the Gemfile.lock and reinstalled the Gems with This showed me, that your PR is depending on a recent version of the @jsvd what do you think? Other than that, there is no longer any reason to not merge this PR. |
|
To those getting here via Google 4 years later, just add |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #25