logstash-plugins / logstash-filter-multiline

Apache License 2.0
18 stars 16 forks source link

Do not add @timestamp if it is already in dst. #3562 #18

Open slamp opened 9 years ago

slamp commented 9 years ago

PR to correct issue: https://github.com/elastic/logstash/issues/3562

This avoids building up a huge array of timestamps and taking only the first one later on in function merge().

jsvd commented 9 years ago

Would you consider adding a test showing the expected behaviour regarding @timestamp? For examples of tests you can check https://github.com/logstash-plugins/logstash-filter-multiline/blob/master/spec/filters/multiline_spec.rb#L119-L133

slamp commented 9 years ago

1/ I'm not sure putting the test after will have the same behavior as this is a recursive function.

2/ I will try to add a test (I will have to learn a little more ruby first)

3/ I'm not able to sign the "CLA". I got a 404 when clicking "details" < https://github.com/logstash-plugins/logstash-filter-multiline/blob/master/CONTRIBUTING.md#contribution-steps >

purbon commented 9 years ago

Thanks a lot for your contribution @slamp , in order to move forward with your PR is going to be necessary to sign the CLA agreement, you can find more information at https://www.elastic.co/contributor-agreement (this is the correct link!)

On the other side, it would be super nice if you can add test for this change. Don't hesitate to ask any question regarding you might have, more than looking forward to help.

elasticsearch-release commented 9 years ago

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

slamp commented 9 years ago

I signed the CLA. I did not have time to add a test.