fix conditionals regression

[colinsurprenant]

the performances modifications included in #2869 introduced a bug in the conditionals, this is a proposed fix. the problem is that any filter that generated extra events (ex clone) followed by conditionals, the extra events would not be checked by the conditionals.

this PR generates a new "cond_func_X" for each conditional, similar to the filter_func which takes multiple events as input and returns multiple events. these "cond_func_X" are called from the filter_func and within other "cond_func_X" if the conditionals are nested.

I did not do any performance tests for this, obviously this will have a performance penalty.

[jsvd]

I suggest a patch for this, that removes multi_filter since it no longer does all the work and you have to iterate through events outside of it.

diff --git a/lib/logstash/config/config_ast.rb b/lib/logstash/config/config_ast.rb
index 35ba4d4..ad014ec 100644
--- a/lib/logstash/config/config_ast.rb
+++ b/lib/logstash/config/config_ast.rb
@@ -230,7 +230,7 @@ module LogStash; module Config; module AST
         return "start_input(#{variable_name})"
       when "filter"
         return <<-CODE
-          events = #{variable_name}.multi_filter(events)
+          #{variable_name}.filter(event) {|new_event| events << new_event }
         CODE
       when "output"
         return "#{variable_name}.handle(event)\n"
diff --git a/lib/logstash/filters/base.rb b/lib/logstash/filters/base.rb
index 374fdd1..9039124 100644
--- a/lib/logstash/filters/base.rb
+++ b/lib/logstash/filters/base.rb
@@ -146,24 +146,6 @@ class LogStash::Filters::Base < LogStash::Plugin
     raise "#{self.class}#filter must be overidden"
   end # def filter

-
-  # in 1.5.0 multi_filter is meant to be used in the generated filter function in LogStash::Config::AST::Plugin only
-  # and is temporary until we refactor the filter method interface to accept events list and return events list,
-  # just list in multi_filter see https://github.com/elastic/logstash/issues/2872.
-  # refactoring the filter method will mean updating all plugins which we want to avoid doing for 1.5.0.
-  #
-  # @param events [Array<LogStash::Event] list of events to filter
-  # @return [Array<LogStash::Event] filtered events and any new events generated by the filter
-  public
-  def multi_filter(events)
-    result = []
-    events.each do |event|
-      result << event
-      filter(event){|new_event| result << new_event}
-    end
-    result
-  end
-
   public
   def execute(event, &block)
     filter(event, &block)
diff --git a/spec/filters/base_spec.rb b/spec/filters/base_spec.rb
index da547b7..132c0fe 100644
--- a/spec/filters/base_spec.rb
+++ b/spec/filters/base_spec.rb
@@ -23,24 +23,10 @@ describe LogStash::Filters::Base do
   end

   it "should provide class public API" do
-    [:register, :filter, :multi_filter, :execute, :threadsafe?, :filter_matched, :filter?, :teardown].each do |method|
+    [:register, :filter, :execute, :threadsafe?, :filter_matched, :filter?, :teardown].each do |method|
       expect(subject).to respond_to(method)
     end
   end
-
-  it "should multi_filter without new events" do
-    allow(subject).to receive(:filter) do |event, &block|
-      nil
-    end
-    expect(subject.multi_filter([:foo])).to eq([:foo])
-  end
-
-  it "should multi_filter with new events" do
-    allow(subject).to receive(:filter) do |event, &block|
-      block.call(:bar)
-    end
-    expect(subject.multi_filter([:foo])).to eq([:foo, :bar])
-  end
 end

 describe LogStash::Filters::NOOP do

[colinsurprenant]

uh! I guess that would work yes, need to analyse a bit further. I assume all specs are passing with that mod?

[jsvd]

@colinsurprenant all tests pass.

[ph]

The tests passes and it fixes the problems we saw with the conditionals.

However I am not a huge fan of using the class variable to accumulate the deferred conditionals, but I find it okay to merge it to fix the current regression.

[colinsurprenant]

@ph can you verify @jsvd proposed optimization?

Also, it's not a class variable, it's a module instance variable with class methods, it's a very good way to create singletons. I decided to use that because I could not figure how/where to store the deferred conditionals without ending up in stacking previous AST run when invoked multiple times as in the specs....

[ph]

@colinsurprenant Okay i've looked through diff code and the different versions of our code and I think we are safe with @jsvd fix and make the iteration outside of the filter. Also I believe it is the responsibility of the pipeline to deal with that. 👍

[ph]

@colinsurprenant @jsvd what is the status of this?

[jsvd]

@ph @colinsurprenant I have pushed my suggestion as a commit to this PR's branch. I request a final overview and a round of LGTM :)

[ph]

@jsvd I've ran the tests and did a quick sanity check, LGTM.

but lets get a 2 reviews on this :)

[colinsurprenant]

LGTM rebasing and merging!

[colinsurprenant]

for some reason the post-squash 2 commits are not listed in the commits tab, here they are:
573149b
3eb5ba6