search

open-telemetry / opentelemetry-operator

Kubernetes Operator for OpenTelemetry Collector
Apache License 2.0
1.12k stars 395 forks source link

Enable "enable-multi-instrumentation" flag by default #3090

Open iblancasa opened 6 days ago

iblancasa commented 6 days ago

Component(s)

collector, auto-instrumentation

Is your feature request related to a problem? Please describe.

The features that are enabled by enable-multi-instrumentation have been there for a while and it might be a good time to enable them by default.

The only risk I can see is those user workloads using the instrumentation.opentelemetry.io/container-names would not be instrumented when they are restarted or recreated. We can implement a warning as part of the validation webhook + something as part of the upgrade routines in order to replace this annotation with the language-specific ones.

Describe the solution you'd like

.

Describe alternatives you've considered

No response

Additional context

No response

pavolloffay commented 4 days ago

@swiatekm @jaronoff97 any objections?

swiatekm commented 4 days ago

As far as I can see, this would be a breaking change, as the flag actually changes the behaviour, and different annotations must be used for multi-container instrumentation for the same language. Can we not avoid this breaking change, or at least allow the old annotations to work while the multi-instrumentation flag is enabled?

pavolloffay commented 4 days ago

@iblancasa can we write an upgrade procedure to migrate already injected agents?

iblancasa commented 4 days ago

. Can we not avoid this breaking change, or at least allow the old annotations to work while the multi-instrumentation flag is enabled? @swiatekm With a migration procedure should be enough. But I agree that it would be better if we could make the instrumentation.opentelemetry.io/container-names annotation work when the flag is enabled. I'll try to send a PR to make it work when the annotation is there (I'm not sure if that would be considered a breaking change too).

@iblancasa can we write an upgrade procedure to migrate already injected agents? @pavolloffay It is what I proposed in my first comment:

We can implement a warning as part of the validation webhook + something as part of the upgrade routines in order to replace this annotation with the language-specific ones.

I'll create a PR to address this.