-
Notifications
You must be signed in to change notification settings - Fork 546
[manager] Run the mutating webhook with configuration, disabling instrumentation CRDs. #4201
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
base: main
Are you sure you want to change the base?
Conversation
e61a981
to
bea749c
Compare
) | ||
|
||
// InstrumentationSpec defines the desired state of OpenTelemetry SDK and instrumentation. | ||
type InstrumentationSpec struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we not reuse the instrumentation spec from the CRD package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you cannot because you get import cycles.
If we choose to have Config define the instrumentation CRD struct, we'd need to move it, and we no longer would generate it from the CRD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dependency is mostly incidental though. The CRD itself doesn't depend on the config struct, the webhooks do, and they just happen to be in the same package. There's several things we can do to fix this, like moving webhook implementations to a different package, or having them depend on their own config struct rather than the full operator one.
I would also much rather not define the instrumentation spec twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it? I assumed the Struct was derived from the CRD generation step. I can look further and break this down into smaller chunks.
I think I'm confused why we need to embed the instrumentations in the config struct if all we care about is disabling/enabling? |
If we disable the CRD, we can still enable autoinstrumentation. We just define everything in config. |
I don't think that makes sense to me – if the CRDs are disabled we shouldn't run autoinstrumentation because we have nothing defined? I don't understand why we want to put the CRDs in the config like this. |
No, they are defined in config. |
This PR is a draft showing a direction of being able to deploy the mutating webhook using exclusively static configuration rather than instrumentation CRDs.
There are 3 parts to it: