search

logstash-plugins / logstash-codec-json

Apache License 2.0
23 stars 29 forks source link

Robust parsing #14

Closed andrewvc closed 9 years ago

andrewvc commented 9 years ago

This fixes #5 .

It's an open question as to whether scalar values should be parsed. I am of the opinion that they SHOULD be parsed, because the feedback mechanism for errors is more clear (Why are these non-json strings showing up in my output?) vs. (Hmmm there's some odd stuff in the debug log). I'm open to differing views here however.

andrewvc commented 9 years ago

FYI @jordansissel I included a stacktrace in the catchall handler because that really just shouldn't happen generally, so the cause would tend to be mysterious

jordansissel commented 9 years ago

Stand-alone scalar values aren't valid JSON, though. Is there a case you can think of where this helps?

jordansissel commented 9 years ago

Code and tests LGTM. Tests pass. Not sure about the idea, though. Let's discuss :)

jordansissel commented 9 years ago

The JSON spec doesn't seem to indicate what behavior should be taken on invalid JSON. @andrewvc's suggestion of handling valid-scalars-but-invalid-json as just setting message => data raw text is good and I think could help cover some other edge cases where programs send accidentally-invalid json and Logstash could handle it more gently.

jordansissel commented 9 years ago

+1 the idea. Will re-review once you update the tests for some of those edge cases (blank lines, etc) we discussed in the zoom.

jordansissel commented 9 years ago

LGTM

elasticsearch-bot commented 9 years ago

Merged sucessfully into master!