thekrakken / java-grok

Simple API that allows you to easily parse logs and other files
http://grok.nflabs.com/
Other
360 stars 151 forks source link

Preserve the order of log format in capture #106

Closed SivaAccionLabs closed 6 years ago

SivaAccionLabs commented 6 years ago

Dear java-grok maintainers,

Please accept this PR.

Analysis Currently, capture is returning the unordered Map.

Resolution Return the map with the same order as log format(pattern).

Test Tested with different types of logs.

SivaAccionLabs commented 6 years ago

@anthonycorbacho @paulwellnerbou @ottobackwards Please review and merge this PR.

ottobackwards commented 6 years ago

First, thanks for the contribution. I have a couple of questions, not just for you but for the others as well:

  1. Should this be optional? Have a setOrderedCaptures(boolean), then have a function

private Map<String,Object> createCaptureMap() { if (orderCaptures) { return new LinkedHashMap<>(); } return new HashMap<>(); }



2.  I'm not sure that the documentation should be changed to LinkedHashMap

3. I would expect unit tests for this or any change to be included.
SivaAccionLabs commented 6 years ago

@ottobackwards Thank you so much for the review. This is the most urgent and important fix for our project.

  1. Need others views too.
  2. Agree with you, it's not mandatory to change to LinkedHashMap. Will remove the changes in the documentation.
    1. Sure, will add the unit tests.
SivaAccionLabs commented 6 years ago

@ottobackwards Have removed the changes in the documentation and made changes in the unit tests. Please review. Thanks.

ottobackwards commented 6 years ago

Thanks @SivaMaplelabs, could you change the tests or add new tests the specifically test that the map iteration matches the capture order? That is the point.

SivaAccionLabs commented 6 years ago

@ottobackwards Following lines test the "preserve order". https://github.com/thekrakken/java-grok/pull/106/files#diff-706258725dfb91a78f94974320bde826R57 https://github.com/thekrakken/java-grok/pull/106/files#diff-706258725dfb91a78f94974320bde826R72 Thanks

paulwellnerbou commented 6 years ago

@SivaMaplelabs , looks great. Unfortunately I don't have rights to publish it, I would have to create an own version again, as I did with https://github.com/paulwellnerbou/java-grok/releases/tag/0.1.8 and https://bintray.com/paulwellnerbou/maven/java-grok/0.1.8

SivaAccionLabs commented 6 years ago

@anthonycorbacho Could you please review and merge this PR?

anthonycorbacho commented 6 years ago

Hi,

Sure i will do it after landing, it will be in couple of hours.

On Wed, 3 Oct 2018 at 8:54 AM SivaMaplelabs notifications@github.com wrote:

@anthonycorbacho https://github.com/anthonycorbacho Could you please review and merge this PR?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/thekrakken/java-grok/pull/106#issuecomment-426626570, or mute the thread https://github.com/notifications/unsubscribe-auth/AC_n5fY1fMKMTGzS9SleGjGuRRFsQCrZks5uhLNxgaJpZM4W7D-6 .

SivaAccionLabs commented 6 years ago

Sure. Thanks.

anthonycorbacho commented 6 years ago

Reviewed and merged, thanks for your contribution 💃

SivaAccionLabs commented 6 years ago

@anthonycorbacho Thanks