logstash-filter-multiline icon indicating copy to clipboard operation
logstash-filter-multiline copied to clipboard

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

Open slamp opened this issue 9 years ago • 5 comments

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().

slamp avatar Aug 30 '15 12:08 slamp

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

jsvd avatar Sep 02 '15 17:09 jsvd

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 >

slamp avatar Sep 02 '15 19:09 slamp

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.

purbon avatar Sep 04 '15 12:09 purbon

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'.

elasticsearch-release avatar Nov 02 '15 11:11 elasticsearch-release

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

slamp avatar Nov 02 '15 19:11 slamp