[libbeat] Add lowercase_fields and uppercase_fields processors #34022

davidifr · 2022-12-12T14:36:40Z

What does this PR do?

The PR adds lowercase and uppercase processors to libbeat.

Why is it important?

It's a basic requirement that came up from multiple users which are currently using different solutions in order to work around this missing feature (using script or rename processors for example).

Checklist

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

Author's Checklist

[ ]

How to test this PR locally

Related issues

Closes Filebeat Processors - make uppercase and lowercase processors (as in ES ingest nodes) available to filebeat #22254

Use cases

Users who desire to lowercase/uppercase some fields are currently required to use the script or rename processors.
With this PR, users will no longer need to use the above mentioned work arounds.

Screenshots

Logs

mergify · 2022-12-12T14:37:16Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @davidifr? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

elasticmachine · 2022-12-12T14:42:18Z

❕ Build Aborted

The PR is not allowed to run in the CI yet

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-12-12T14:36:58.913+0000
Duration: 5 min 14 sec

Steps errors

Expand to view the steps failures

`Load a resource file from a library`

Took 0 min 0 sec . View more details here
Description: approval-list/elastic/beats.yml

`Error signal`

Took 0 min 0 sec . View more details here
Description: githubApiCall: The REST API call https://api.github.com/orgs/elastic/members/davidifr return the message : java.lang.Exception: httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/davidifr : httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/davidifr : Code: 404Error: {"message":"User does not exist or is not a member of the organization","documentation_url":"https://docs.github.com/rest/reference/orgs#check-organization-membership-for-a-user"}

🤖 GitHub comments

Expand to view the 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!)

sonarqubecloud · 2022-12-12T14:43:02Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
41.2% Duplication

elasticmachine · 2022-12-12T18:40:14Z

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

rdner

Thank you very much for your contribution and especially for the full test coverage.

I think we should definitely improve the docs and naming of the processors. And we could refactor these 2 processor implementation into one parameterising the field changing function to avoid code duplication.

rdner · 2022-12-16T09:46:10Z

libbeat/processors/actions/docs/lowercase_fields.asciidoc

+[source,json]
+-------------------------------------------------------------------------------
+{
+  "a.b": {}
+}
+-------------------------------------------------------------------------------


This example is very confusing.

What is it supposed to demonstrate? Is it the end result?

Does it lowercase the last field name (after the last .) or all the keys on the path? We need more examples covering these edge cases.

Yes, the end result.

only the last field in a given path.

I agree, I should definitely improve the documentation and provide more examples.

rdner · 2022-12-16T09:47:29Z

libbeat/processors/actions/docs/uppercase_fields.asciidoc

+[source,json]
+-------------------------------------------------------------------------------
+{
+  "a.B": {}
+}
+-------------------------------------------------------------------------------


The same comment from the lowercase doc applies here. We need more examples "before" and "after" and to explicitly explain what segments of the dot notated paths are affected.

Agreed, before and after cases would make a good examples for users.

rdner · 2022-12-16T09:49:30Z

libbeat/processors/actions/lowercase_fields.go

+
+	var lower string
+	if strings.ContainsRune(field, '.') {
+		// In case of nested fields provided, we need to make sure to only modify the latest field in the chain


In my earlier comments I meant that this should have been added to the docs.

Also, how are we sure it's the desired behaviour for most of the users? Why not all the keys in the path?

I will add it to the docs as well.

Regarding the desired behavior, I think lowercasing/uppercasing all the keys in the path is kind of restricting in case of a user only wanting to apply the action to one specific field,
While providing the ability to only apply the action to the latest field, may give the user a more flexibility and control in to which fields does the user want to apply the action to.

What do you think?

rdner · 2022-12-16T09:52:47Z

libbeat/processors/actions/lowercase_fields.go

+		lastIndexRuneFunc := func(r rune) bool { return r == '.' }
+		idx := strings.LastIndexFunc(field, lastIndexRuneFunc)
+		lower = field[:idx+1] + strings.ToLower(field[idx+1:])


not sure it's worth using runes here but I think it's okay.

I actually wasn't sure it's required either, I will change that.

rdner · 2022-12-16T09:55:48Z

libbeat/processors/actions/docs/lowercase_fields.asciidoc

+<titleabbrev>lowercase_fields</titleabbrev>
++++
+
+The `lowercase_fields` processor specifies a list of fields to lowercase.


After reading this I thought it would lowercase the values, not keys. I think we should explicitly explain what this processor is changing. The same goes for the upercase-fields processor.

rdner · 2022-12-16T09:58:08Z

libbeat/processors/actions/lowercase_fields.go

+	"github.com/pkg/errors"
+)
+
+type lowerCaseProcessor struct {


I think it should be called lowerCaseFieldsProcessor in case we would want lowerCaseValuesProcessor in the future.

rdner · 2022-12-16T09:58:29Z

libbeat/processors/actions/uppercase_fields.go

+	"github.com/pkg/errors"
+)
+
+type upperCaseProcessor struct {


I think it should be called upperCaseFieldsProcessor in case we would want upperCaseValuesProcessor in the future.

rdner · 2022-12-16T10:01:37Z

libbeat/processors/actions/uppercase_fields.go

+	}
+
+	for _, field := range p.Fields {
+		if err := p.upperCaseField(event, field); err != nil {


There is a lot of code duplication in these two processors. The only part that differs is what function we're applying to the field name (last segment of the path).

I think it would be better to implement a changeFieldProcessor and a parameter called changeFunc. We would avoid a lot of code duplication if we did this.

It will be still exposed as upperCaseFieldProcessor and lowerCaseFieldProcessor for external use but in their constructor function they would just create and return a changeFieldProcessor with toLower or toUpper function as a parameter.

Just to clarify, since this comment and your first comment are kind of confusing to me.

You mentioned on your first comment that these 2 processors can be replaced by one so why do I need to keep their *CaseFieldProcessor struct?

Also, where should I put the changeFieldProcessor? should I create it in another file?
Or should I create a new file change_fields.go that will include the changeFieldProcessor struct with both of the lowerCase/upperCase constructors that will return a changeFieldProcessor each with their relevant changeFunc?

mergify · 2023-01-11T14:49:51Z

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 issue-22254 upstream/issue-22254
git merge upstream/main
git push upstream issue-22254

mr1716 · 2023-01-20T16:11:19Z

@davidifr any sense of when development will be continued? If not, no big deal.

cmacknz · 2023-04-20T19:29:24Z

Closing this one. Please reopen it when it ready for review again.

mr1716 · 2023-05-25T12:47:42Z

@davidifr have you had a chance to work on this recently?

zez3 · 2024-09-20T08:35:44Z

I think @davidifr is no longer an active account. It would be nice of someone would take over and finish this

mr1716 · 2024-09-26T12:39:39Z

@zez3 agreed!

[libbeat] Add lowercase_fields and uppercase_fields processors

81e9859

davidifr requested a review from a team as a code owner December 12, 2022 14:36

davidifr requested review from rdner and faec and removed request for a team December 12, 2022 14:36

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Dec 12, 2022

mergify bot assigned davidifr Dec 12, 2022

belimawr added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Dec 12, 2022

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Dec 12, 2022

cmacknz added the Team:Elastic-Agent Label for the Agent team label Dec 12, 2022

rdner reviewed Dec 16, 2022

View reviewed changes

rdner added the Filebeat Filebeat label Dec 16, 2022

cmacknz closed this Apr 20, 2023

khushijain21 mentioned this pull request Oct 24, 2024

[libbeat]: Add lowercase processor #41424

Merged

mergify bot mentioned this pull request Nov 5, 2024

[8.x](backport #41424) [libbeat]: Add lowercase processor #41526

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libbeat] Add lowercase_fields and uppercase_fields processors #34022

[libbeat] Add lowercase_fields and uppercase_fields processors #34022

davidifr commented Dec 12, 2022

mergify bot commented Dec 12, 2022

elasticmachine commented Dec 12, 2022

Build stats

`Load a resource file from a library`

`Error signal`

sonarqubecloud bot commented Dec 12, 2022

elasticmachine commented Dec 12, 2022

rdner left a comment

rdner Dec 16, 2022

davidifr Dec 18, 2022

rdner Dec 16, 2022

davidifr Dec 18, 2022 •

edited

Loading

rdner Dec 16, 2022

davidifr Dec 18, 2022

rdner Dec 16, 2022

davidifr Dec 18, 2022

rdner Dec 16, 2022

rdner Dec 16, 2022

rdner Dec 16, 2022

rdner Dec 16, 2022

davidifr Dec 18, 2022

mergify bot commented Jan 11, 2023

mr1716 commented Jan 20, 2023

cmacknz commented Apr 20, 2023

mr1716 commented May 25, 2023

zez3 commented Sep 20, 2024 •

edited

Loading

mr1716 commented Sep 26, 2024

[libbeat] Add lowercase_fields and uppercase_fields processors #34022

[libbeat] Add lowercase_fields and uppercase_fields processors #34022

Conversation

davidifr commented Dec 12, 2022

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

mergify bot commented Dec 12, 2022

elasticmachine commented Dec 12, 2022

❕ Build Aborted

Build stats

Steps errors

Load a resource file from a library

Error signal

🤖 GitHub comments

sonarqubecloud bot commented Dec 12, 2022

elasticmachine commented Dec 12, 2022

rdner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidifr Dec 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Jan 11, 2023

mr1716 commented Jan 20, 2023

cmacknz commented Apr 20, 2023

mr1716 commented May 25, 2023

zez3 commented Sep 20, 2024 • edited Loading

mr1716 commented Sep 26, 2024

`Load a resource file from a library`

`Error signal`

davidifr Dec 18, 2022 •

edited

Loading

zez3 commented Sep 20, 2024 •

edited

Loading