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.

Sign up for GitHub

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

Jump to bottom

[k8sattributesprocessor] Refactor informer initialization #36604

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

[k8sattributesprocessor] Refactor informer initialization #36604

wants to merge 8 commits into from
+653 −287

Conversation

swiatekm
Copy link
Contributor

@swiatekm swiatekm commented Nov 29, 2024 •
edited
Loading

Description

We'd like instances of the k8sattributes processor to share local K8s resource caches (represented by the informer concept from client-go). See #36234 for the justification and a broad overview. This change refactors internal interfaces and responsibilities of different components during initialization, in order to make this possible.

The way the processor currently initializes resources makes it impossible to share them effectively:

  • The processor creates and starts its own informers during startup. Instead, it needs to get them from an external provider and only attach its own handlers.
  • The processor creates its own K8s client. For informers to be sharable, this needs to happen outside the processor.
  • The interfaces used for creating informers need to take all the relevant arguments. For example, processors attach transform functions on their own. This needs to be moved to providers.

We address the issues above by making the following changes:

  • Informer providers are now responsible for starting informers. Informers are still stopped by the processor closing a channel passed in at the start.
  • Informer providers now accept transform functions as arguments.
  • The watch client provider is now an attribute of the processor, instead of a global variable mutated in tests.
  • We introduce a InformerProviders struct which contains all the necessary providers. This makes it easier to inject them during initialization as needed, and also simplifies tests.
  • The watch client doesn't use a K8s API client anymore. It only needed one for informers, and informer providers now embed this information.

This is a refactor without any externally visible changes. It is split rather cleanly into atomic commits, and may be easier to review that way. I'm submitting it as a single PR because I believe it is easier to understand this way, but I'm open to splitting it into parts.

Link to tracking issue

Part of #36234

Testing

Added some unit tests for the new code.

@github-actions github-actions bot added the processor/k8sattributes k8s Attributes processor label Nov 29, 2024
@swiatekm swiatekm force-pushed the chore/k8sattributes/refactor-informer-init branch from 04db117 to a4351c1 Compare November 29, 2024 19:52
@swiatekm swiatekm marked this pull request as ready for review November 29, 2024 20:07
@swiatekm swiatekm requested a review from a team as a code owner November 29, 2024 20:07
@swiatekm
Copy link
Contributor Author

swiatekm commented Dec 2, 2024

I haven't added a changelog entry because this change should be completely transparent to users. Considering I've made a whole bunch of changes though, I can see an argument to add one anyway.

@dmitryax dmitryax added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Dec 3, 2024
@dmitryax
Copy link
Member

dmitryax commented Dec 3, 2024

This is a good problem to solve. Potentially, it'd be great to share across all k8s components. However, I'm curious, have you considered using https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/internal/sharedcomponent for this particular problem?

@swiatekm
Copy link
Contributor Author

swiatekm commented Dec 3, 2024 •
edited
Loading

This is a good problem to solve. Potentially, it'd be great to share across all k8s components. However, I'm curious, have you considered using https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/internal/sharedcomponent for this particular problem?

I didn't know that existed, so no 😅. But as far as I can see, it just allows us to avoid making copies of identical components. The primary use case I'd like to address (not with this PR, but with a follow up) is having processors used for different signal types share the cache. With sharedcomponent, you can't have a logs processor act on traces, it's a different interface.

@swiatekm
Copy link
Contributor Author

swiatekm commented Dec 4, 2024

When it comes to sharing across all Kubernetes components, it should be possible. We'd have to come up with a common interface for informer providers, but I think standard options in client-go get us 90% there. Then the main thing we'd need to standardize would be client construction, but I think that's already done with internal/k8sconfig. The most challenging bit are transform functions, because we'd need a way of specifying their behaviour statically in a way that was consistent across all components. I think the k8sattributes processor is the only user of these right now, so it's not a problem we'd need to solve to get most of the benefit.

@ChrsMark
Copy link
Member

ChrsMark commented Dec 4, 2024 •
edited
Loading

I think the k8sattributes processor is the only user of these right now, so it's not a problem we'd need to solve to get most of the benefit.

k8scluster receiver also sets transform functions on the informer:

opentelemetry-collector-contrib/receiver/k8sclusterreceiver/watcher.go

Line 254 in ec784fb

err := informer.SetTransform(transformObject)
Not sure if that would hit the same issue though.

@swiatekm
Copy link
Contributor Author

swiatekm commented Dec 4, 2024

I think the k8sattributes processor is the only user of these right now, so it's not a problem we'd need to solve to get most of the benefit.

k8scluster receiver also sets transform functions on the informer:

opentelemetry-collector-contrib/receiver/k8sclusterreceiver/watcher.go

Line 254 in ec784fb

err := informer.SetTransform(transformObject)

Not sure if that would hit the same issue though.

Dang, that's unfortunate. At least it looks like their transform funcs don't depend on configuration. But I'm not sure if this would even be especially useful. You don't normally want multiple k8sclusterreceivers, and it's unlikely that you'd be able to share informers with other components unless you resign yourself to keeping all the data.

@ChrsMark ChrsMark mentioned this pull request Dec 5, 2024
Open
@swiatekm swiatekm force-pushed the chore/k8sattributes/refactor-informer-init branch from a4351c1 to 2d6e81f Compare December 5, 2024 15:46
@github-actions GitHub Actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Dec 20, 2024
fatsheep9146
fatsheep9146 reviewed Dec 25, 2024
View reviewed changes
processor/k8sattributesprocessor/processor.go
@@ -33,6 +33,9 @@ type kubernetesprocessor struct {
telemetrySettings component.TelemetrySettings
logger *zap.Logger
apiConfig k8sconfig.APIConfig
k8sClientProvider kube.APIClientsetProvider
kcProvider kube.ClientProvider
informerProviders *kube.InformerProviders
Copy link
Contributor

Choose a reason for hiding this comment

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

If the informerProviders is a nested field of kubernetesprocessor, how could we ensure the local caches of informer can be shared among all the kubernetes processor object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The actual providers are just functions, and their implementations can pull from a shared pool of informers. The informerProviders struct just contains all the providers potentially necessary for the processor to function. It only exists for convenience, so we don't have to pass around four provider functions everywhere.

@swiatekm swiatekm force-pushed the chore/k8sattributes/refactor-informer-init branch from 2d6e81f to d7ec6a5 Compare December 30, 2024 12:51
@swiatekm swiatekm requested a review from ChrsMark as a code owner December 30, 2024 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fatsheep9146 fatsheep9146 fatsheep9146 left review comments

@dmitryax dmitryax Awaiting requested review from dmitryax dmitryax is a code owner

@TylerHelmuth TylerHelmuth Awaiting requested review from TylerHelmuth TylerHelmuth is a code owner

@ChrsMark ChrsMark Awaiting requested review from ChrsMark ChrsMark is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@jpkrohling jpkrohling

Labels
processor/k8sattributes k8s Attributes processor Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@swiatekm @dmitryax @ChrsMark @fatsheep9146 @jpkrohling