allows disable pod events enrichment with deployment name #28521

newly12 · 2021-10-19T05:07:02Z

What does this PR do?

allows disable pod events enrichment with deployment name

Why is it important?

allows user to disable this enrichment due to the potential rate limit #28149

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

Relates Autodiscover could reach to k8s client's default rate limit #28149

Use cases

Screenshots

Logs

mergify · 2021-10-19T05:07:10Z

This pull request does not have a backport label. Could you fix it @newly12? 🙏
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 · 2021-10-19T05:12:09Z

💚 Build Succeeded

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: 2021-11-05T15:06:57.136+0000
Duration: 140 min 47 sec
Commit: 821f06f

Test stats 🧪

Test	Results
Failed	0
Passed	53548
Skipped	5225
Total	58773

💚 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.

elasticmachine · 2021-10-20T11:04:16Z

Pinging @elastic/integrations (Team:Integrations)

ChrsMark

@newly12 this change looks good to me however it will require a changelog entry and adding this option to docs. Thank you!

newly12 · 2021-10-28T13:05:58Z

@ChrsMark Thanks for reviewing this!

mergify · 2021-11-02T18:00: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 deployment_name upstream/deployment_name
git merge upstream/master
git push upstream deployment_name

ChrsMark

Hey @newly12 ! This is looking good! I just left 2 comments to consider.

ChrsMark · 2021-11-02T12:18:17Z

libbeat/common/kubernetes/metadata/metadata.go

@@ -96,7 +96,7 @@ func GetPodMetaGen(
 	if namespaceWatcher != nil && metaConf.Namespace.Enabled() {
 		namespaceMetaGen = NewNamespaceMetadataGenerator(metaConf.Namespace, namespaceWatcher.Store(), namespaceWatcher.Client())
 	}
-	metaGen := NewPodMetadataGenerator(cfg, podWatcher.Store(), podWatcher.Client(), nodeMetaGen, namespaceMetaGen)
+	metaGen := NewPodMetadataGenerator(cfg, podWatcher.Store(), podWatcher.Client(), nodeMetaGen, namespaceMetaGen, metaConf)


Can't you just pass the value of metaConf.Deployment as bool?

ChrsMark · 2021-11-02T12:21:06Z

libbeat/common/kubernetes/metadata/pod.go

@@ -107,7 +116,7 @@ func (p *pod) GenerateK8s(obj kubernetes.Resource, opts ...FieldOptions) common.
 		meta := p.namespace.GenerateFromName(po.GetNamespace())
 		if meta != nil {
 			// Use this in 8.0
-			//out.Put("namespace", meta["namespace"])
+			// out.Put("namespace", meta["namespace"])


@MichaelKatsoulis this line looks like a leftover from other PR, shall we open a fixup PR to remove it?

ChrsMark · 2021-11-02T12:22:57Z

libbeat/common/kubernetes/metadata/pod.go

-	namespace MetaGen) MetaGen {
+	namespace MetaGen,
+	metaCfg *AddResourceMetadataConfig) MetaGen {
+	addDeploymentMeta := true


In combination with the previous suggestion you might can move this one layer up to GetPodMetaGen function.

ChrsMark · 2021-11-02T12:23:53Z

metricbeat/module/kubernetes/util/kubernetes.go

@@ -139,7 +139,7 @@ func NewResourceMetadataEnricher(
 	cfg, _ := common.NewConfigFrom(&metaConfig)

 	metaGen := metadata.NewResourceMetadataGenerator(cfg, watcher.Client())
-	podMetaGen := metadata.NewPodMetadataGenerator(cfg, nil, watcher.Client(), nil, nil)
+	podMetaGen := metadata.NewPodMetadataGenerator(cfg, nil, watcher.Client(), nil, nil, &metadata.AddResourceMetadataConfig{Deployment: true})


Can't we leverage the value from the configuration here instead of setting always to true?

lol I was simply following same like the other arguments and meantime keep the same behavior like previous version. so current configuration doesn't have a section for namespace nor node, wondering if deployment should be added first there..

in the later commit I still passed a true to this function for the comment. we can discuss more on how this one should be handled properly.

Ah ok, I see! I think we can leave it as is for now and try to address it in general in another round. I will open an issue for this :).

# Conflicts: # libbeat/common/kubernetes/metadata/pod.go

newly12 · 2021-11-04T03:09:34Z

@ChrsMark updated per your suggestion.

ChrsMark

lgtm! Thanks for contributing this!

ChrsMark · 2021-11-04T14:21:59Z

/test

ChrsMark · 2021-11-04T19:59:53Z

@newly I see some lint errors, could you take a look?

newly12 · 2021-11-05T01:00:29Z

@ChrsMark just fixed. the CI failure seems to be non relevant to the code change(from this log).

ChrsMark · 2021-11-05T14:26:41Z

Maybe we could merge the latest upstream/master and check if it helps?

ChrsMark · 2021-11-08T08:56:45Z

CI passed, merging! Thank you @newly12 !

…in-the-package-binareis * upstream/master: allows disable pod events enrichment with deployment name (elastic#28521) Remove Docker input from Filebeat (elastic#28817) [breaking] Make default_field: false the default for all fields (elastic#28596) Osquerybeat: Improve osquery client connect code (elastic#28848) Add crawler metrics into the stats metricset for Enterprise Search (elastic#28790) Remove the now deprecated appsearch module from metricbeat (elastic#28850) Remove Beat generators (elastic#28816) chore: upload files to Google Storage when they exist (elastic#28836) Revert "chore(ci): disable E2E tests in Beats (elastic#28715)" (elastic#28812) Deprecate generating custom Beats (elastic#28814) [Metricbeat] upgrade flatbuffers to 1.12.1 (elastic#28094) Osquerybeat: Fix restart flags after previously bad config (elastic#28827) Force ECS and JSON logging for libbeat/logp (elastic#28573) Filebeat: Error on startup for unconfigured module (elastic#28818) Deprecate log input in favour of filestream (elastic#28623) Fix some spelling mistakes (elastic#28080)

allows disable pod events enrichment with deployment name

ef84c87

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 19, 2021

mergify bot added the backport-skip Skip notification from the automated backport with mergify label Oct 19, 2021

MichaelKatsoulis requested review from MichaelKatsoulis and ChrsMark October 19, 2021 19:08

MichaelKatsoulis added the Team:Integrations Label for the Integrations team label Oct 20, 2021

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Oct 20, 2021

ChrsMark reviewed Oct 25, 2021

View reviewed changes

newly12 added 2 commits October 28, 2021 16:30

Merge remote-tracking branch 'upstream/master' into deployment_name

859f9aa

update doc and regression fix

fb2e693

fix changelog

b8f5d63

ChrsMark mentioned this pull request Oct 29, 2021

add options to configure k8s client qps/burst #28151

Merged

6 tasks

ChrsMark self-assigned this Nov 1, 2021

ChrsMark reviewed Nov 3, 2021

View reviewed changes

newly12 added 2 commits November 4, 2021 10:48

Merge remote-tracking branch 'upstream/master' into deployment_name

9556f83

# Conflicts: # libbeat/common/kubernetes/metadata/pod.go

fix merge conflict issue

3a1a5ac

incorporate suggested changes

9c023bb

newly12 force-pushed the deployment_name branch from 3cf3c7c to 9c023bb Compare November 4, 2021 05:03

ChrsMark approved these changes Nov 4, 2021

View reviewed changes

ChrsMark mentioned this pull request Nov 4, 2021

Align k8s metadata configurations in Kubernetes module elastic/integrations#2131

Closed

fix syntax error

357bb69

Merge remote-tracking branch 'upstream/master' into deployment_name

821f06f

ChrsMark merged commit 3d19ec0 into elastic:master Nov 8, 2021

MichaelKatsoulis mentioned this pull request Sep 23, 2022

Backport #29133 to 7.17 #33165

Merged

ChrsMark mentioned this pull request Dec 22, 2022

Add option to disable pod events enrichment with deployment name #34097

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allows disable pod events enrichment with deployment name #28521

allows disable pod events enrichment with deployment name #28521

newly12 commented Oct 19, 2021 •

edited

Loading

mergify bot commented Oct 19, 2021

elasticmachine commented Oct 19, 2021 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

elasticmachine commented Oct 20, 2021

ChrsMark left a comment

newly12 commented Oct 28, 2021

mergify bot commented Nov 2, 2021

ChrsMark left a comment

ChrsMark Nov 2, 2021

ChrsMark Nov 2, 2021

ChrsMark Nov 2, 2021

ChrsMark Nov 2, 2021

newly12 Nov 4, 2021

newly12 Nov 4, 2021

ChrsMark Nov 4, 2021

newly12 commented Nov 4, 2021

ChrsMark left a comment

ChrsMark commented Nov 4, 2021

ChrsMark commented Nov 4, 2021

newly12 commented Nov 5, 2021

ChrsMark commented Nov 5, 2021

ChrsMark commented Nov 8, 2021

allows disable pod events enrichment with deployment name #28521

allows disable pod events enrichment with deployment name #28521

Conversation

newly12 commented Oct 19, 2021 • 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 Oct 19, 2021

elasticmachine commented Oct 19, 2021 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

elasticmachine commented Oct 20, 2021

ChrsMark left a comment

Choose a reason for hiding this comment

newly12 commented Oct 28, 2021

mergify bot commented Nov 2, 2021

ChrsMark left a comment

Choose a reason for hiding this comment

ChrsMark Nov 2, 2021

Choose a reason for hiding this comment

ChrsMark Nov 2, 2021

Choose a reason for hiding this comment

ChrsMark Nov 2, 2021

Choose a reason for hiding this comment

ChrsMark Nov 2, 2021

Choose a reason for hiding this comment

newly12 Nov 4, 2021

Choose a reason for hiding this comment

newly12 Nov 4, 2021

Choose a reason for hiding this comment

ChrsMark Nov 4, 2021

Choose a reason for hiding this comment

newly12 commented Nov 4, 2021

ChrsMark left a comment

Choose a reason for hiding this comment

ChrsMark commented Nov 4, 2021

ChrsMark commented Nov 4, 2021

newly12 commented Nov 5, 2021

ChrsMark commented Nov 5, 2021

ChrsMark commented Nov 8, 2021

newly12 commented Oct 19, 2021 •

edited

Loading

elasticmachine commented Oct 19, 2021 •

edited by jenkins-beats-ci bot

Loading