[Filebeat] Fix multiple modules in filebeat.yml #29952

legoguy1000 · 2022-01-23T07:29:53Z

What does this PR do?

Fix the issue when defining multiple of the same module in filebeat.yml

Why is it important?

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.

Author's Checklist

[ ]

How to test this PR locally

Related issues

Closes multiple - module: name configuration has different behaviour between modules.d/module.yml and filebeat.yml #29649

Use cases

Screenshots

Logs

mergify · 2022-01-23T07:30:27Z

This pull request does not have a backport label. Could you fix it @legoguy1000? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

elasticmachine · 2022-01-23T07:35:49Z

💔 Tests Failed

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-02-10T10:10:51.185+0000
Duration: 66 min 45 sec

Test stats 🧪

Test	Results
Failed	2
Passed	5880
Skipped	523
Total	6405

Test errors

Expand to view the tests failures

`Build&Test / filebeat-goIntegTest / TestParsersCloseTimeoutWithMultiline – github.com/elastic/beats/v7/filebeat/input/filestream`

Expand to view the error details

 Failed

Expand to view the stacktrace

 === RUN   TestParsersCloseTimeoutWithMultiline
    environment_test.go:193: 
        	Error Trace:	environment_test.go:193
        	            				parsers_integration_test.go:494
        	Error:      	Not equal: 
        	            	expected: 139
        	            	actual  : 101
        	Test:       	TestParsersCloseTimeoutWithMultiline
--- FAIL: TestParsersCloseTimeoutWithMultiline (2.01s)

`Build&Test / filebeat-goIntegTest / TestInput – github.com/elastic/beats/v7/filebeat/input/kafka`

Expand to view the error details

 Failed

Expand to view the stacktrace

 === RUN   TestInput
    kafka_integration_test.go:457: 
        	Error Trace:	kafka_integration_test.go:457
        	            				kafka_integration_test.go:144
        	Error:      	Not equal: 
        	            	expected: 3
        	            	actual  : 1
        	Test:       	TestInput
        	Messages:   	offset does not match, perhaps messages were not acknowledged
--- FAIL: TestInput (7.05s)

Steps errors

Expand to view the steps failures

`filebeat-goIntegTest - mage goIntegTest`

Took 5 min 52 sec . View more details here
Description: mage goIntegTest

`filebeat-goIntegTest - mage goIntegTest`

Took 2 min 5 sec . View more details here
Description: mage goIntegTest

`filebeat-goIntegTest - mage goIntegTest`

Took 2 min 3 sec . View more details here
Description: mage goIntegTest

`filebeat-windows-windows-2019 - mage build unitTest`

Took 2 min 28 sec . View more details here
Description: mage build unitTest

`Error signal`

Took 0 min 0 sec . View more details here
Description: Error 'hudson.AbortException: script returned exit code 1'

🐛 Flaky test report

❕ There are test failures but not known flaky tests.

Expand to view the summary

Genuine test errors

💔 There are test failures but not known flaky tests, most likely a genuine test failure.

Name: Build&Test / filebeat-goIntegTest / TestParsersCloseTimeoutWithMultiline – github.com/elastic/beats/v7/filebeat/input/filestream
Name: Build&Test / filebeat-goIntegTest / TestInput – github.com/elastic/beats/v7/filebeat/input/kafka

🤖 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!)

elasticmachine · 2022-01-27T19:19:58Z

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

kvch · 2022-01-28T11:53:24Z

jenkins run tests

filebeat/fileset/factory.go

filebeat/fileset/modules.go

legoguy1000 · 2022-01-28T14:20:22Z

I would be curious if u have any thoughts on there is a better way of doing this. Converting the registry from a Map to an Array of Maps seems a little jenky due to all the additional loops that I had to add everywhere else to accommodate them. During testing when it was still a map, I was adding numerical suffixes to the module names before adding to the map which then made them unique. Any thoughts on something like that? it would remove the need for all the additional loops.

kvch · 2022-01-28T15:51:09Z

I am still thinking. Another issue I found is that pipelines get loaded as many times as the module is configured. It is either problematic because we are overloading Elasticsearch with unnecessary requests. Also, there are module that can affect the pipeline contents with configuration options, so it might be overwritten in a later pipeline loading. It is not yet clear to me what should be the resolution, but I will get back to you with ideas/resolution.

Otherwise, your PR is great! I just need a little time to think.

legoguy1000 · 2022-01-28T18:13:08Z

I am still thinking. Another issue I found is that pipelines get loaded as many times as the module is configured. It is either problematic because we are overloading Elasticsearch with unnecessary requests. Also, there are module that can affect the pipeline contents with configuration options, so it might be overwritten in a later pipeline loading. It is not yet clear to me what should be the resolution, but I will get back to you with ideas/resolution.

Otherwise, your PR is great! I just need a little time to think.

Should be easy to add a internal check to the LoadPipelines function to not try to load pipelines if a module ahs already been loaded. Doesn't help the possible conflicts as u mentioned. However I would have to assume that issue would currently exist since you can have multiple of the same module set in the modules.d/*.yml files so there would already be a conflict/duplicate requests right??

filebeat/fileset/modules.go

kvch · 2022-02-02T16:59:17Z

I am still thinking. Another issue I found is that pipelines get loaded as many times as the module is configured. It is either problematic because we are overloading Elasticsearch with unnecessary requests. Also, there are module that can affect the pipeline contents with configuration options, so it might be overwritten in a later pipeline loading. It is not yet clear to me what should be the resolution, but I will get back to you with ideas/resolution.
Otherwise, your PR is great! I just need a little time to think.

Should be easy to add a internal check to the LoadPipelines function to not try to load pipelines if a module ahs already been loaded. Doesn't help the possible conflicts as u mentioned. However I would have to assume that issue would currently exist since you can have multiple of the same module set in the modules.d/*.yml files so there would already be a conflict/duplicate requests right??

Yes, the issue already exists in Beats. But I do not want to increase the scope of this PR. I will open an issue in beats to track the problem.

I was thinking about the registry and what I came up with is instead of having a list of maps, we can just create actual data type for modules just like for filesets.

type Registry struct {
    registry []Module
    log logp.Logger
}

type Module struct {
    filesets []Fileset
    config ModuleConfig
}

And Fileset does not need a pointer to ModuleConfig, keeping the name of the module is enough.

WDYT?

legoguy1000 · 2022-02-02T17:40:30Z

@kvch That could probably work. YOur thought for the Fileset struct would be something like below by replacing the *ModuleConfig with a string field with the module name?

type Fileset struct {
	name        string
	mname     string
	fcfg        *FilesetConfig
	modulePath  string
	manifest    *manifest
	vars        map[string]interface{}
	pipelineIDs []string
}

looking at filebeat/fileset/fileset.go and filebeat/fileset/modules.go, looks like the pointer is really just used to get the module name.

kvch · 2022-02-02T17:53:25Z

Issue: #30165

kvch · 2022-02-02T17:53:57Z

@kvch That could probably work. YOur thought for the Fileset struct would be something like below by replacing the *ModuleConfig with a string field with the module name?

Exactly.

legoguy1000 · 2022-02-02T17:58:34Z

Ok, I can try to make that update later today if I get a break.

legoguy1000 · 2022-02-02T19:59:57Z

@kvch just pushed changes, locally seems to function properly

kvch · 2022-02-04T16:07:47Z

jenkins run tests

kvch · 2022-02-04T16:09:12Z

filebeat/fileset/modules.go

-	filesets, _ := reg.registry[module]
-	for name := range filesets {
-		list = append(list, name)
+func (reg *ModuleRegistry) ModuleConfiguredFilesets(module Module) (list []string, err error) {


Why are we returning an error param in this function? What can return an error? Am I missing something?

I had to modify the function for it to make sense with the new structure so now the possibility of error no longer exists and I can remove it.

kvch · 2022-02-04T16:10:08Z

I like your changes, let's see what the CI thinks.

kvch · 2022-02-07T09:06:44Z

Please add a changelog entry, then this PR is ready to be merged.

filebeat/fileset/modules.go

mergify · 2022-02-07T13:24:21Z

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 29649-module-fix upstream/29649-module-fix
git merge upstream/main
git push upstream 29649-module-fix

kvch · 2022-02-08T11:10:04Z

CHANGELOG.next.asciidoc

@@ -101,6 +101,13 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...main[Check the HEAD dif

 - tcp/unix input: Stop accepting connections after socket is closed. {pull}29712[29712]
 - Fix using log_group_name_prefix in aws-cloudwatch input. {pull}29695[29695]
+- aws-s3: Stop trying to increase SQS message visibility after ReceiptHandleIsInvalid errors. {pull}29480[29480]


Could you please remove the extra changelog entries?

mergify · 2022-02-08T11:10:45Z

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 29649-module-fix upstream/29649-module-fix
git merge upstream/main
git push upstream 29649-module-fix

kvch · 2022-02-08T12:21:50Z

Once green, I will merge it. Thanks @legoguy1000

legoguy1000 · 2022-02-08T14:44:44Z

@kvch Any thoughts on why the Python Integration tests would have failed??

kvch · 2022-02-09T14:43:39Z

The failures are unrelated to your change. We updated the testing environment because we are releasing a new version. There might be a few hiccups here and there.

kvch · 2022-02-09T14:43:47Z

jenkins run tests

mergify · 2022-02-10T06:14:30Z

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 29649-module-fix upstream/29649-module-fix
git merge upstream/main
git push upstream 29649-module-fix

kvch · 2022-02-10T12:31:06Z

The test failures are unrelated. I will disable the Parser test for now in a follow up PR.

@legoguy1000 Thanks for your contribution!

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

mergify bot added the backport-skip Skip notification from the automated backport with mergify label Jan 23, 2022

mtojek added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Jan 24, 2022

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

legoguy1000 marked this pull request as ready for review January 27, 2022 19:19

kvch self-assigned this Jan 28, 2022

kvch reviewed Jan 28, 2022

View reviewed changes

filebeat/fileset/factory.go Outdated Show resolved Hide resolved

kvch reviewed Jan 28, 2022

View reviewed changes

filebeat/fileset/modules.go Outdated Show resolved Hide resolved

kvch reviewed Feb 2, 2022

View reviewed changes

filebeat/fileset/modules.go Outdated Show resolved Hide resolved

kvch reviewed Feb 4, 2022

View reviewed changes

kvch added the backport-v8.1.0 Automated backport with mergify label Feb 7, 2022

kvch reviewed Feb 7, 2022

View reviewed changes

filebeat/fileset/modules.go Outdated Show resolved Hide resolved

kvch removed the backport-v8.1.0 Automated backport with mergify label Feb 7, 2022

legoguy1000 added 4 commits February 7, 2022 13:24

elastic#29649: Fix multiple modules in filebeat.yml

0e9dbd2

Resolve comments

ab5505a

Change format of registry

fa7961c

Add Changelog

c49678c

legoguy1000 force-pushed the 29649-module-fix branch from b58e3d6 to c49678c Compare February 7, 2022 13:25

legoguy1000 requested a review from kvch February 7, 2022 13:25

kvch reviewed Feb 8, 2022

View reviewed changes

cleanup changelog

915fedf

kvch approved these changes Feb 8, 2022

View reviewed changes

Merge branch 'main' into 29649-module-fix

f756c32

Merge branch 'main' into 29649-module-fix

3209e29

kvch merged commit 5617b65 into elastic:main Feb 10, 2022

legoguy1000 deleted the 29649-module-fix branch May 9, 2022 01:30

[Filebeat] Fix multiple modules in filebeat.yml #29952

[Filebeat] Fix multiple modules in filebeat.yml #29952

Conversation

legoguy1000 commented Jan 23, 2022 • edited Loading

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 Jan 23, 2022

elasticmachine commented Jan 23, 2022 • edited by jenkins-beats-ci bot Loading

💔 Tests Failed

Build stats

Test stats 🧪

Test errors

Build&Test / filebeat-goIntegTest / TestParsersCloseTimeoutWithMultiline – github.com/elastic/beats/v7/filebeat/input/filestream

Build&Test / filebeat-goIntegTest / TestInput – github.com/elastic/beats/v7/filebeat/input/kafka

Steps errors

filebeat-goIntegTest - mage goIntegTest

filebeat-goIntegTest - mage goIntegTest

filebeat-goIntegTest - mage goIntegTest

filebeat-windows-windows-2019 - mage build unitTest

Error signal

🐛 Flaky test report

Genuine test errors

🤖 GitHub comments

elasticmachine commented Jan 27, 2022

kvch commented Jan 28, 2022

legoguy1000 commented Jan 28, 2022

kvch commented Jan 28, 2022

legoguy1000 commented Jan 28, 2022 • edited Loading

kvch commented Feb 2, 2022

legoguy1000 commented Feb 2, 2022 • edited Loading

kvch commented Feb 2, 2022

kvch commented Feb 2, 2022

legoguy1000 commented Feb 2, 2022

legoguy1000 commented Feb 2, 2022

kvch commented Feb 4, 2022

kvch Feb 4, 2022

Choose a reason for hiding this comment

legoguy1000 Feb 4, 2022

Choose a reason for hiding this comment

kvch commented Feb 4, 2022

kvch commented Feb 7, 2022

mergify bot commented Feb 7, 2022

kvch Feb 8, 2022

Choose a reason for hiding this comment

mergify bot commented Feb 8, 2022

kvch commented Feb 8, 2022

legoguy1000 commented Feb 8, 2022

kvch commented Feb 9, 2022

kvch commented Feb 9, 2022

mergify bot commented Feb 10, 2022

kvch commented Feb 10, 2022

legoguy1000 commented Jan 23, 2022 •

edited

Loading

elasticmachine commented Jan 23, 2022 •

edited by jenkins-beats-ci bot

Loading

`Build&Test / filebeat-goIntegTest / TestParsersCloseTimeoutWithMultiline – github.com/elastic/beats/v7/filebeat/input/filestream`

`Build&Test / filebeat-goIntegTest / TestInput – github.com/elastic/beats/v7/filebeat/input/kafka`

`filebeat-goIntegTest - mage goIntegTest`

`filebeat-goIntegTest - mage goIntegTest`

`filebeat-goIntegTest - mage goIntegTest`

`filebeat-windows-windows-2019 - mage build unitTest`

`Error signal`

legoguy1000 commented Jan 28, 2022 •

edited

Loading

legoguy1000 commented Feb 2, 2022 •

edited

Loading