[receiver/k8sobjectsreceiver] Properly deep copy the K8sObjectsConfig struct #39644

petern48 · 2025-04-25T00:19:46Z

Description

In newReceiver() , we perform a shallow copy of the K8sObjectsConfig objects instead of a deep one.

Link to tracking issue

Testing

Added TestDeepCopy inside of config_test.go

petern48 · 2025-04-25T00:22:51Z

The original issue advised to refactor the config struct to separate mutable fields into a new struct. I tried that, but I felt it was much cleaner to simply keep it as is. I'm not sure if there was reasoning other than code readability, but I'm happy to change this if reviewers still want the refactor.

ChrsMark · 2025-04-29T07:47:48Z

receiver/k8sobjectsreceiver/config.go

@@ -129,3 +129,40 @@ func (c *Config) getValidObjects() (map[string][]*schema.GroupVersionResource, e
 	}
 	return validObjects, nil
 }
+
+func (k *K8sObjectsConfig) DeepCopy() *K8sObjectsConfig {
+	copied := &K8sObjectsConfig{


Re-using the same struct might not be the best option here. I would define a new one called for example K8sObjectTarget. That'd probably be with non-public fields unless there is need for having them public.

Apart from making it clear in the name that it's not about "Config" anymore will ensure that we won't have issues in the future in case we want to diverge the 2 different structs. WDYT?

Got it. Sounds good. The one thing is that we have to keep the fields public for mapstructure to work properly. (I tried unexporting to namespaces and excludeWatchType and things broke)

receiver/k8sobjectsreceiver/receiver.go

ChrsMark

Thank's @petern48 for working on this! I left few suggestions.

atoulme · 2025-04-29T17:49:40Z

Moving to draft while you review suggestions, please mark ready to review once done.

petern48 · 2025-04-29T19:17:42Z

@ChrsMark I incorporated your feedback. Let me know if this is what you had in mind.

ChrsMark · 2025-04-30T07:42:59Z

receiver/k8sobjectsreceiver/config.go

+	FieldSelector   string        `mapstructure:"field_selector"`
+	Interval        time.Duration `mapstructure:"interval"`
+	ResourceVersion string        `mapstructure:"resource_version"`
+	K8sObjectTarget `mapstructure:",squash"`


Why this needs to be part of the config? I was thinking that those 2 should be independent. Then you can do the following to instantiate the targets:

targetObject := newTargetObject(configObject)

After this point we don't need the K8sObjectsConfig object anymore and we only work with the targetObject which is of type K8sObjectTarget.

I see. It makes a lot more sense that you were considering separating the objects altogether. However, I'm not quite seeing where you imagine this being stored. Don't we still need to maintain one K8sObjectTarget per corresponding object?

The only way I can see doing that while keeping it outside of K8ObjectsConfig is by adding a new corresponding slice inside of Config, like below. Is that what you had in mind?

type Config struct { k8sconfig.APIConfig `mapstructure:",squash"` Objects []*K8sObjectsConfig `mapstructure:"objects"` + Targets []*K8sObjectTarget `mapstructure:"targets"`

Or do you mean to somehow separate it from the Config object as well? Although that doesn't add up since the example line you gave indicates the config object would have that info to create the new copy.
targetObject := newTargetObject(configObject)

I think we already store the "copies" at

opentelemetry-collector-contrib/receiver/k8sobjectsreceiver/receiver.go

Line 33 in e0646e1

objects []*K8sObjectsConfig

as part of the k8sobjectsreceiver instance. I would just use this field and have the full/deep copy to happen at newReceiver.

I think we already store the "copies" at

opentelemetry-collector-contrib/receiver/k8sobjectsreceiver/receiver.go

Line 33 in e0646e1

objects []*K8sObjectsConfig

as part of the k8sobjectsreceiver instance. I would just use this field and have the full/deep copy to happen at newReceiver.

Sorry, I'm still confused. This sounds like what the current implementation is doing. Maybe I'm missing something, but I don't see how we can accomplish this while fulfilling your earlier request of moving K8sObjectTarget outside of the config struct (K8sObjectsConfig). Wouldn't K8sObjectTarget have to be inside of K8sObjectsConfig if we want to re-use this field?

My suggestion is to make objects field of type K8sObjectTarget. Then objects will be deep copies of the initially provided K8sObjectsConfig objects of the Config.

After this point we only interact and handle with the objects field (type K8sObjectTarget) and we don't need the K8sObjectsConfig any more.

Otherwise we should only interact with K8sObjectsConfig in read-only mode to unpack the configurations and create mutable K8sObjectTarget objects that will be the ones we will be using during the execution of the receiver.

I've looked into it, and I don't think it's quite possible to never touch the kr.config object after newReceiver() since many of the struct methods use fields that are inside of immutable fields in k8sObjectConfig, that aren't in K8sObjectTarget. We need some way to access those fields in the struct methods, so using the k8sObjectConfigs through kr.config looks like the easiest way. Let me know if you think otherwise. I'll need a little more time to figure out and propose a clean way to implement this.

use fields that are inside of immutable fields in k8sObjectConfig, that aren't in K8sObjectTarget

Why then we don't make them part of the K8sObjectTarget as well?

Well then, at that point, we've gone full circle and have a struct named K8sObjectTarget that's the same as the k8sObjectConfig struct as we originally did, so effectively all we would be doing is renaming the struct... I'm honestly not seeing tremendous value from trying to refactor the code anymore. If we remove the K8sObjectTarget struct and simply deep copy the original k8sObjectConfig, then we can fix the deep copy issue without adding additional complexity. WDYT?

I see. The reason for introducing a 2nd struct was described at #39644 (comment).

However, let's solve the deep copy issue for now using the same struct (K8sObjectConfig) as you suggest (removing the use of K8sObjectTarget since it's essentially the same) and maybe we can revisit in a follow-up PR.

Thank's!

Got it, yeah. I just reverted to the deep copy-only code.

atoulme · 2025-05-09T18:24:27Z

Please address the feedback of the codeowner and mark ready for review once done.

github-actions · 2025-05-28T05:21:16Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

…tor-contrib into refactor_k8_config

ChrsMark

LGTM

ChrsMark · 2025-06-03T07:36:17Z

.chloggen/refactor_k8_config.yaml

+component: k8sobjectsreceiver
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Properly deep copy the K8sObjectsConfig struct in the k8sobjectsreceiver and separate mutable fields


This would need an update? Probably we don't need a change-log at all since it's a code refactoring.

Got it, just removed it. You'll need to add the Skip changelog label for CI to pass, as mentioned here.

… struct (open-telemetry#39644)  #### Description In `newReceiver()` , we perform a shallow copy of the K8sObjectsConfig objects instead of a deep one.  #### Link to tracking issue Fixes open-telemetry#39553  #### Testing Added `TestDeepCopy` inside of `config_test.go`

… struct (open-telemetry#39644)  #### Description In `newReceiver()` , we perform a shallow copy of the K8sObjectsConfig objects instead of a deep one.  #### Link to tracking issue Fixes open-telemetry#39553  #### Testing Added `TestDeepCopy` inside of `config_test.go`

petern48 added 4 commits April 24, 2025 16:57

Deep copy the K8sObjectsConfig in the k8sobjectsreceiver

994de1c

Add changelog

3bc2f2a

Add TestDeepCopy test

8ec99dc

Export method

34db20f

petern48 requested review from dmitryax, TylerHelmuth, ChrsMark and a team as code owners April 25, 2025 00:19

github-actions bot assigned bogdandrutu Apr 25, 2025

github-actions bot added the receiver/k8sobjects label Apr 25, 2025

github-actions bot requested a review from hvaghani221 April 25, 2025 00:20

gofmt

1384283

atoulme added the waiting-for-code-owners label Apr 27, 2025

github-actions bot mentioned this pull request Apr 29, 2025

Weekly Report: 2025-04-22 - 2025-04-29 #39708

Closed

ChrsMark reviewed Apr 29, 2025

View reviewed changes

atoulme removed the waiting-for-code-owners label Apr 29, 2025

atoulme marked this pull request as draft April 29, 2025 17:49

petern48 added 3 commits April 29, 2025 11:45

Pull out fields to new K8sObjectTarget struct

653f26c

Update tests

c885f01

Update changelog msg

e4b3c92

petern48 marked this pull request as ready for review April 29, 2025 19:15

github-actions bot assigned atoulme Apr 29, 2025

petern48 requested a review from ChrsMark April 29, 2025 19:16

Merge branch 'main' into refactor_k8_config

07a7d92

ChrsMark reviewed Apr 30, 2025

View reviewed changes

atoulme marked this pull request as draft May 9, 2025 18:24

github-actions bot added Stale and removed Stale labels May 28, 2025

petern48 added 2 commits June 2, 2025 11:41

Merge branch 'main' of github.com:open-telemetry/opentelemetry-collec…

ef47b66

…tor-contrib into refactor_k8_config

Revert struct refactoring

35c3fb7

petern48 force-pushed the refactor_k8_config branch from f1c78f9 to 35c3fb7 Compare June 2, 2025 18:43

petern48 marked this pull request as ready for review June 2, 2025 19:04

github-actions bot assigned mx-psi Jun 2, 2025

petern48 requested a review from ChrsMark June 2, 2025 19:05

atoulme added the waiting-for-code-owners label Jun 3, 2025

github-actions bot mentioned this pull request Jun 3, 2025

Weekly Report: 2025-05-27 - 2025-06-03 #40424

Closed

ChrsMark approved these changes Jun 3, 2025

View reviewed changes

Remove changelog

0e4c41b

TylerHelmuth added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jun 4, 2025

TylerHelmuth approved these changes Jun 4, 2025

View reviewed changes

TylerHelmuth merged commit 32aec72 into open-telemetry:main Jun 4, 2025
186 of 187 checks passed

github-actions bot added this to the next release milestone Jun 4, 2025

petern48 deleted the refactor_k8_config branch June 30, 2025 00:27

[receiver/k8sobjectsreceiver] Properly deep copy the K8sObjectsConfig struct #39644

[receiver/k8sobjectsreceiver] Properly deep copy the K8sObjectsConfig struct #39644

Uh oh!

Conversation

petern48 commented Apr 25, 2025

Description

Link to tracking issue

Testing

Uh oh!

petern48 commented Apr 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ChrsMark left a comment

Choose a reason for hiding this comment

Uh oh!

atoulme commented Apr 29, 2025

Uh oh!

petern48 commented Apr 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrsMark Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atoulme commented May 9, 2025

Uh oh!

github-actions bot commented May 28, 2025

Uh oh!

ChrsMark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ChrsMark Jun 2, 2025 •

edited

Loading