search

logstash-plugins / logstash-filter-grok

Grok plugin to parse unstructured (log) data into something structured.
https://www.elastic.co/guide/en/logstash/current/plugins-filters-grok.html
Apache License 2.0
124 stars 98 forks source link

Add new timeout_millis/tag_on_timeout option #88

Closed andrewvc closed 8 years ago

andrewvc commented 8 years ago

Resolves #82 .

As a note, the logic in the 'filter' function was convoluted before this patch, and is now even more so. Perhaps some refactoring could improve this, but that might necessitate changing grok behavior.

This PR needs more unit tests for the new TimeoutEnforcer class before it is no longer a WIP

andrewvc commented 8 years ago

@jordansissel going through the match and filter functions the logic has grown quite complex. @talevy and I have been discussing options for clarifying it.

  1. Hardcode break_on_match to always be true
  2. On timeouts and exceptions break execution as well, and apply the appropriate tag.
  3. Leave non-match behavior the same with the above caveats

As is this logic is just too hard to work with. There are some small refactors that could improve the code as is, but without a bigger change to the logic the impact there will be minimal.

Your thoughts sir?

andrewvc commented 8 years ago

I think I've addressed all our concerns. Per the last discussion with @talevy and @jordansissel I'll be leaving the logic around tagging/break_on_match alone.

The last thing to discuss is whether tag_on_timeout should stay singular like it is now, or match tag_on_failure and be an Array.

IMHO it should be singular, I don't think anyone would want multiples of it. It does introduce an inconsistency however. Thoughts @jordansissel @talevy ?

suyograo commented 8 years ago

The last thing to discuss is whether tag_on_timeout should stay singular like it is now, or match tag_on_failure and be an Array

My vote is on a singular tag as well. Not sure how adding another tag(s) is useful to the user..

talevy commented 8 years ago

I like the updated changes.

This PR needs more unit tests for the new TimeoutEnforcer class before it is no longer a WIP

do you still plan on adding those tests?

suyograo commented 8 years ago

@andrewvc please see my point on adding metrics..

andrewvc commented 8 years ago

@talevy thanks for the test reminder

@suyograo thanks for the metrics reminder, just pushed a fix there

andrewvc commented 8 years ago

@talevy I forgot, I did add tests on Friday https://github.com/logstash-plugins/logstash-filter-grok/pull/88/files#diff-08aa93b3d60105273d03b19fdfa48794R410 . Are these not sufficient?

suyograo commented 8 years ago

LGTM from my side. Nice work

talevy commented 8 years ago

LGTM

elasticsearch-bot commented 8 years ago

Andrew Cholakian merged this into the following branches!

Branch Commits
master 121d78b4fef5e4c5b3d331c9fd156ac414aa23f5