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 regex support functionality #31424

Merged
merged 14 commits into from
May 2, 2022
Merged

Add regex support functionality #31424

merged 14 commits into from
May 2, 2022

Conversation

gonzaloe
Copy link
Contributor

@gonzaloe gonzaloe commented Apr 26, 2022

  • Enhancement

What does this PR do?

Added regex support for drop_fields processor as requested in #12923

Fields that are declared between slashes (as in Painless scripts) are going to be considered regex and all matches within field names will be removed

Why is it important?

So deleting field-by-field with name can be avoided while using modules that declare a big amount of fields.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
    - [ ] I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

Configure a field using a regex pattern under drop_fields processor, on fields array, between slashes /. For e.g.:

drop_fields:
  fields: ["/field_to_be_deleted.*/"]

Related issues

@gonzaloe gonzaloe requested a review from a team as a code owner April 26, 2022 18:22
@gonzaloe gonzaloe requested review from rdner and faec and removed request for a team April 26, 2022 18:22
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Apr 26, 2022
@cla-checker-service
Copy link

cla-checker-service bot commented Apr 26, 2022

💚 CLA has been signed

@gonzaloe gonzaloe added the Team:Integrations Label for the Integrations team label Apr 26, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Apr 26, 2022
@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 26, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-05-01T18:12:43.474+0000

  • Duration: 155 min 24 sec

Test stats 🧪

Test Results
Failed 0
Passed 22259
Skipped 1934
Total 24193

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@rdner
Copy link
Member

rdner commented Apr 27, 2022

@kvch I suspect this PR will be affected by your refactoring here, right?

should we wait until the migration is merged?

@kvch
Copy link
Contributor

kvch commented Apr 27, 2022

Please wait. My PR is gigantic already.

@gonzaloe
Copy link
Contributor Author

@kvch could you ping me when your PR gets merged? I'll adapt my changes and rebase then

@mergify
Copy link
Contributor

mergify bot commented Apr 28, 2022

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b drop-fields-support-regex upstream/drop-fields-support-regex
git merge upstream/main
git push upstream drop-fields-support-regex

@rdner
Copy link
Member

rdner commented Apr 28, 2022

@gonzaloe thank you for waiting, now the big change is merged, please update your PR.

@gonzaloe
Copy link
Contributor Author

thanks for the notification @rdner, seems i'll have to move the mapstr implementation to elastic-agent-libs project, then import a new version here once that code is merged, is there a label to pause the review for now? or should i ping back once the other PR gets merged and everything working here again?

@gonzaloe
Copy link
Contributor Author

@rdner in case you can take a quick look there -> elastic/elastic-agent-libs#45

@rdner
Copy link
Member

rdner commented Apr 29, 2022

@gonzaloe it's approved, feel free to proceed.

@mergify
Copy link
Contributor

mergify bot commented Apr 29, 2022

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b drop-fields-support-regex upstream/drop-fields-support-regex
git merge upstream/main
git push upstream drop-fields-support-regex

@gonzaloe
Copy link
Contributor Author

@rdner thanks! I've updated the dependency already :)
one more doubt so far:

  • about the bug I pointed out on the drop_field package, should I go ahead and fix it, or is that code not being used? (deletion is being done while traversing the array from left to right)

@rdner
Copy link
Member

rdner commented Apr 29, 2022

about the bug I pointed out on the drop_field package, should I go ahead and fix it, or is that code not being used? (deletion is being done while traversing the array from left to right)

@gonzaloe sorry I missed where you mentioned the bug. Regardless, I think we should make one change at a time. Let's finish with this PR first and address any other bug later.

After updating a dependency one must run make update from the root of the repository and it seems like there is one build error reported by the linter too.

@gonzaloe
Copy link
Contributor Author

@rdner perfect about addressing the other bug, i'll create a separate PR after. I understand about the make update, now should be addressed that (and the change on dependency for the lint warning). Thanks

@rdner
Copy link
Member

rdner commented Apr 29, 2022

@gonzaloe I have to ask you to run goimport on libbeat/processors/actions/drop_fields.go and libbeat/processors/actions/drop_fields_test.go, otherwise the CI (libbeat-checks) would not pass. It runs goimport and checks that there is no diff afterwards.

You can also run this step locally with make check.

@gonzaloe
Copy link
Contributor Author

@rdner sorry for the inconvenience, I had first the problem with elastic-agent-libs and ran the update, but then with the new config package, but forgot to run make update now 😅 . I believe things should be in order

@rdner
Copy link
Member

rdner commented Apr 30, 2022

/test

Copy link
Member

@rdner rdner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see e2e tests passed here https://beats-ci.elastic.co/blue/organizations/jenkins/e2e-tests%2Fe2e-testing-mbp/detail/main/906/pipeline but it's not reported to Github for some reason. I think it's okay to merge it now.

libbeat/processors/actions/drop_fields.go Outdated Show resolved Hide resolved
@gonzaloe
Copy link
Contributor Author

gonzaloe commented May 2, 2022

@rdner good morning, E2E Tests are configured to run in every branch once the previous steps are successful? or only run in main/version branches?

@rdner
Copy link
Member

rdner commented May 2, 2022

@gonzaloe the E2E tests passed (https://beats-ci.elastic.co/blue/organizations/jenkins/e2e-tests%2Fe2e-testing-mbp/detail/main/908/pipeline) I think something has been changed and they are not reported to Github correctly anymore. I'm going to approve the PR, we can merge it now.

Copy link
Member

@rdner rdner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,

@gonzaloe please open another PR with the documentation change.

@rdner rdner merged commit 6562ce9 into main May 2, 2022
@rdner rdner deleted the drop-fields-support-regex branch May 2, 2022 11:00
kush-elastic pushed a commit to kush-elastic/beats that referenced this pull request May 2, 2022
Now it's possible to set a regular expression instead of just a field name in order to drop fields.

Co-authored-by: Gonzalo Espeche <gonzalo.espeche@elastic.co>
chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
Now it's possible to set a regular expression instead of just a field name in order to drop fields.

Co-authored-by: Gonzalo Espeche <gonzalo.espeche@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libbeat Team:Integrations Label for the Integrations team v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[filebeat] Allow wildcards for drop_fields
4 participants