guard against fields with nil name

[joemiller]

Thanks for contributing to Logstash! If you haven't already signed our CLA, here's a handy link: https://www.elastic.co/contributor-agreement/

see #9

[purbon]

Thanks for your contribution

[purbon]

your code looks good, but I would also add a few tests for it, so we could prevent for going back in a regression. As soon as we get this in, I'm very happy to proceed further with this Pr.

[joemiller]

@purbon Glad to do that. I could use some help in running the tests. When I try to run them locally with the bundle exec rspec command stated in the README I get All examples were filtered out and no tests run. I also notice that the tests as run as travis are doing the same thing. https://travis-ci.org/logstash-plugins/logstash-filter-prune/builds/139280377

[magnusbaeck]

Sorry for being slow with this. The tests are apparently disabled right now, see #11. It's trivial to temporarily reenable them (just remove , :if => false in prune_spec.rb) so perhaps you can do that locally just to make sure your updated specs are okay?

[joemiller]

@magnusbaeck thanks! Working on a test now.

[joemiller]

@magnusbaeck I added a test. I think it is sufficient.

I did not re-enable the tests in the commit. I can do so if you'd like though.

[joemiller]

Update: working on a slightly different test. Standby

[joemiller]

Ok. I think it's ready now. I changed the unit test so that it is more explicit about what it's excercising.

[magnusbaeck]

I did not re-enable the tests in the commit. I can do so if you'd like though.

No thanks—let's do that separately.

The PR looks reasonable from a super quick look. I'll try to have a closer look during Monday.

[purbon]

can we add a log line for this behaviour? specially when we aim to debug this behaviour why is not creating the fields the user is expecting to?

[purbon]

@joemiller small question. When do you have this nil fields? can you write an example? can we do also better regexp? what do you think?

[magnusbaeck]

Thinking more about this, what is a user's expected behavior if you do get a field with a nil key (apart from not crashing, obviously)? The nil key can't possibly match the whitelist pattern(s) so from that perspective it makes sense to prune it. In other words, if you use the prune filter with a non-empty whitelist_names option any field with a nil name will be pruned.

can we do also better regexp?

Sorry, I don't understand this comment.

[purbon]

@magnusbaeck

re

can we do also better regexp?

I was thinking about another plugin, so please apologies :-(

@magnusbaeck I agree we should see this in user point of view, i agree with your reasoning. I end up mixing in this comment the kv filter and this one, shame on me.

[purbon]

so I guess if we add logs to this being skipped I think we are good to merge.

[andrewvc]

What's the status here? Any updates?

[joemiller]

Sorry, have not been able to work on this recently.

On Fri, Aug 5, 2016 at 12:32 PM Andrew Cholakian notifications@github.com
wrote:

What's the status here? Any updates?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAXDA_j8F8Y2pFoUQohizPhr2b3TvZ1bks5qc4-ugaJpZM4I7Bez
.

[aaabramov]

Is filtering nil values implemented? Currently, I am using a hack to recursively remove nil values from event.
I have described it here: https://medium.com/@aabrasha/filtering-out-nil-values-in-logstash-ec97c6cbb065

[magnusbaeck]

Is filtering nil values implemented?

No, see elastic/logstash#2707 (which, incorrectly IMHO, references back to this issue).