Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for excluding fields from all concatenated fields #50

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tomas321
Copy link

I found it more difficult to include multiple fields from large data set, rather than excluding one or more fields from all concatenated fields. It is tested and working feature in my production ELK stack.

@ypid-geberit
Copy link

ypid-geberit commented Mar 19, 2019

Could you check if #44 works for you? The only thing that is missing is excluding nested keys which you have implemented. #44 is based on #41 which also fixes non-deterministic fingerprint for nested hashes by sorting. You introduced JSON serialization in this PR. In general I favor your solution of using an existing serialization format than inventing a new one. If this PR is preferred by the maintainers, I would like to check before this is merged/released if JSON in ruby supports deep sorting.

Also note that this PR seems to break backwards compatibility, see #41 for details.

@tomas321
Copy link
Author

tomas321 commented Apr 8, 2019

@ypid-geberit thank you for your response and sorry for the long wait. I have looked at both of your PRs and it seems to me that your serialization method in #44 excludes by top-level key only. I'm currently incorporating the JSON deep sort utilizing the deepsort gem. As you pointed out in #41 I will include the deep_sort parameter to default to false in order to maintain the backward compatibility.

@tomas321 tomas321 closed this Apr 8, 2019
@tomas321 tomas321 reopened this Apr 8, 2019
@ypid-geberit
Copy link

I'm currently incorporating the JSON deep sort utilizing the deepsort gem. As you pointed out in #41 I will include the deep_sort parameter to default to false in order to maintain the backward compatibility.

Sounds good. In this case, I would prefer your work to be merged instead of mine. Your patch will be a bigger change compared to how it was done in the past but I think it is for the better (using gems like json and deepsort).

@tomas321 tomas321 mentioned this pull request Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants