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.

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

Move main.go config into controller package #198

Merged
merged 6 commits into from
Oct 24, 2024

Conversation

dmathieu
Copy link
Member

This is the first PR moving the logic in main.go into a controller package, so that logic can be shared with the collector receiver setup.
See #160 (comment)

This first PR moves the arguments configuration and validation into the new package.

@dmathieu dmathieu marked this pull request as ready for review October 22, 2024 10:10
@dmathieu dmathieu requested review from a team as code owners October 22, 2024 10:10
@@ -0,0 +1,123 @@
package controller // import "go.opentelemetry.io/ebpf-profiler/internal/controller"
Copy link
Contributor

Choose a reason for hiding this comment

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

What about naming this internal package args instead of controller?
From a controller I expect something different, than providing a general configuration and validate it.
As this package is likely reused in the OTel profiling controller part, it should not get mixed and suggest something it doesn't do.

Suggested change
package controller // import "go.opentelemetry.io/ebpf-profiler/internal/controller"
package args // import "go.opentelemetry.io/ebpf-profiler/internal/args"

Copy link
Member Author

@dmathieu dmathieu Oct 23, 2024

Choose a reason for hiding this comment

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

This is meant to be the first of a few PRs, with the intent to extract the setup of main.go into a shared package.
I named this package controller, because what it will do in the end is run the agent, either standalone, or as a collector receiver.

I'm open to a name different than controller, but args would be specific to this config, which doesn't do enough to be in its own package IMHO.

main.go Outdated Show resolved Hide resolved
@dmathieu
Copy link
Member Author

CI failure is a network error.

Copy link
Member

@christos68k christos68k left a comment

Choose a reason for hiding this comment

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

LGTM

We can further refine things (e.g. maybe move CLI setup to internal/controller/args.go) once we have a better idea of how the OTel collector integration will look.

Copy link
Contributor

@rockdaboot rockdaboot left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@rockdaboot rockdaboot merged commit 2c2cb6b into open-telemetry:main Oct 24, 2024
23 checks passed
@dmathieu dmathieu deleted the controller-config branch October 24, 2024 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants