Fix no. 2 for 4444: Too verbose deprecations warnings

[guyboertje]

Better method of ensuring that args are not mutated by plugin initialize.

NOTE:

• I had to introduce LogStash::Util.deep_clone as Marshal.load(Marshal.dump(o)) does not work directly on args because there is logger instance in args and Marshal can't dump IO.
• I had to alter all the codec, filter, input and output base.rb initialize to deep_clone params, as well as the plugin.rb initialize to do config_init(@params).

[guyboertje]

Added specs
fixes #4444

[andrewvc]

I believe this will also fix logstash-plugins/logstash-output-tcp#13 (comment)

[andrewvc]

LGTM and tests pass (incl. plugin tests)

[colinsurprenant]

should we do a clone here or instantiate a new codec?

[purbon]

some question here? do we want to keep internal state, as I could read from the docs:

In general, clone and dup may have different semantics in descendant classes. While clone is used to duplicate an object, including its internal state, dup typically uses the class of the descendant object to create the new instance.

[guyboertje]

Codecs::Base does this

  def clone
    return self.class.new(params)
  end

and this will call deep_clone of the params given.

BTW codec base is the only subclass that has a clone method.

If anything we should call register after clone
e.g.

  def self.deep_clone(o)
    case o
    when Hash
      o.inject({}) {|h, (k,v)| h[k] = deep_clone(v); h }
    when Array
      o.map {|v| deep_clone(v) }
    when Fixnum, Symbol, IO, TrueClass, FalseClass, NilClass
      o
    when LogStash::Codecs::Base
      o.clone.tap {|c| c.register }
    when String
      o.dup
    else
      Marshal.load(Marshal.dump(o))
    end
  end

[guyboertje]

@colinsurprenant @purbon - we definitely do want to call clone here. IMO this exactly what this clone method is for.

[purbon]

seeing what clone is doing for codecs, makes sense for me.

@guyboertje calling register makes sense too.

[colinsurprenant]

👍

[purbon]

Small comments about unit test location and clone, but for the rest it looks good to me.

I tested manually with

input {
  generator { 
    message => "foobar"
    count => 1
  }
}

output {
  stdout { codec => rubydebug }
  elasticsearch {
    hosts => [ "127.0.0.1:9200" ]
    workers => 3
  }
}

and see

skywalker% ./bin/logstash -f ../manual_testing/2.2/output-es.config 
Settings: Default pipeline workers: 4
Logstash startup completed
{
       "message" => "foobar",
      "@version" => "1",
    "@timestamp" => "2016-01-14T08:46:47.604Z",
          "host" => "0.0.0.0",
      "sequence" => 0
}
Logstash shutdown completed

what means works as expected.

Thanks @guyboertje for putting this fix in place.

[elasticsearch-bot]

Guy Boertje merged this into the following branches!

Branch	Commits
master	1a4032b
2.2	88e3992
2.x	681719b

[driverpt]

@guyboertje , does this closes #5211 ?

[guyboertje]

@driverpt - No, not until the Event#clone method uses LogStash::Util.deep_clone - the PR has stalled.