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
122 stars 97 forks source link

Test: redo tests + add performance tests #154

Closed kares closed 4 years ago

kares commented 4 years ago

TL/DR there's 3 parts here (probably worth splitting if needed) :

the original motivation here was really just to add some performance tests.

however, existing unit tests ended up using the devutils helpers using the (old) Ruby pipeline. this turned out to be an issue as performance test would often hang (both on Travis and GH Actions).

so most unit tests were migrated to avoid the pipeline setup, there's a few left (marked as LEGACY) which were dependant on some LS state.

also, along the line, there's a lot of cp-ing on the docker files, we should decide (later) whether it won't be worth sharing those from a common sub-module (unfortunately we can not share the CI configuration much).

p.s. the performance test looks slightly weird allowing a 100% degradation (when using timeout compared to when it isn't used) but this is legit atm. the issue is not using workers to handle events (compared to real LS) and obviously CI slowness. this is likely to be re-visited later with a new Java-based pipeline setup (in devutils) for performance tests.

colinsurprenant commented 4 years ago

spec/filters/grok_spec.rb 💥 😮 Great stuff @kares! I am ok with merging this as one big specs overhaul PR. Overall LGTM - one question about the performance tests : are these planned to be executed upon every specs run?

kares commented 4 years ago

Thanks for looking at this Colin.

one question about the performance tests : are these planned to be executed upon every specs run?

yes - originally was directed not to-do so but since the overhead isn't really that much the conclusion was it's okay to run on every commit. tests might be a bit flaky with CI machines being unpredictable, so we'll see how it goes. since there's more work planned on the plugin testing front this might change.

colinsurprenant commented 4 years ago

@kares yeah - in terms of "smoke" testing significative performance regression it might be useful but I doubt it can be used for "real" performance analysis on CI, as you say, with unpredictable hosts environment/performance... but then the question is what are good threshold constants to fail the test on? We need to find constants that will not give too many false positives upon a slow host but then, if we are too permissive it might just defeat the purpose?

To me the tests will be really interesting for comparing performance on local hardware for sure and will help spot performance regressions when working in that code.