Add RFC: Instrumentation v1beta1#5079
Conversation
swiatekm
left a comment
There was a problem hiding this comment.
Looks good to me overall. I have a major question involving the annotation -> label change, and some minor ones.
E2E Test Results 38 files 271 suites 2h 19m 57s ⏱️ Results for commit 4f08a47. ♻️ This comment has been updated with latest results. |
swiatekm
left a comment
There was a problem hiding this comment.
LGTM overall, some questions about the annotation -> label migration.
| #### Deprecation path | ||
|
|
||
| 1. **v1beta1**: Both labels and annotations supported. Annotations deprecated but functional. | ||
| 2. **v1alpha1 removal**: Annotations no longer documented, labels become the primary mechanism. Annotations support is eventually dropped. |
There was a problem hiding this comment.
This isn't fully clear to me. Are we planning to support annotations until we go to v1? Or do they go away with v1alpha1?
There was a problem hiding this comment.
I have clarified it
- v1alpha1 removal: Labels become the primary mechanism. Annotations support is dropped.
| ```yaml | ||
| webhooks: | ||
| - name: mpod.kb.io | ||
| objectSelector: | ||
| matchExpressions: | ||
| - key: instrumentation.opentelemetry.io/inject-java | ||
| operator: Exists | ||
| - key: instrumentation.opentelemetry.io/inject-python | ||
| operator: Exists | ||
| # ... other languages | ||
| ``` | ||
|
|
||
| This reduces webhook invocations to only pods that have opted in via labels, improving cluster performance. Pods using legacy annotations will still pass through the webhook (no `objectSelector` match), maintaining backwards compatibility. |
There was a problem hiding this comment.
This only works if we're not supporting annotations, right? So it'll have to be an option in either the kustomize manifests or the Helm Chart.
There was a problem hiding this comment.
yes it can be only enforced once we don't support annotations. I would give it as opt-in before we fully drop support for the annotation.
There was a problem hiding this comment.
Added some clarification ot the readme.
jaronoff97
left a comment
There was a problem hiding this comment.
a few small thoughts, otherwise looks great. thank you Pavol! 🙇
| 1. Aligns with OpenTelemetry's [declarative configuration](https://github.com/open-telemetry/opentelemetry-configuration) initiative | ||
| 2. Fixes structural inconsistencies in the current API | ||
|
|
||
| ## Objectives |
There was a problem hiding this comment.
One thing i think we should also do is let users target specific otelcol CRDs rather than needing to hand write a svc / url. i.e. someone could target namespace/otelcol or something like that and we would validate that the collector exists and then write the url for them.
| spec: | ||
| declarativeConfig: | ||
| file_format: "0.4" | ||
| resource: |
There was a problem hiding this comment.
I know why it's like this, but i don't love that we mix camel case and snake case.
|
|
||
| The declarative config supports [environment variable substitution](https://opentelemetry.io/docs/specs/otel/configuration/file-configuration/#environment-variable-substitution) using the `${VAR}` syntax. This is useful for injecting secrets like API tokens without hardcoding them in the CR. Environment variables can be set via `spec.env` or per-language `env` fields. | ||
|
|
||
| This also addresses a limitation in v1alpha1 where Kubernetes `$(VAR)` substitution fails due to env var ordering ([#3022](https://github.com/open-telemetry/opentelemetry-operator/issues/3022)). Since `${VAR}` substitution in declarative config happens at SDK runtime rather than Kubernetes pod creation time, it works regardless of the order in which env vars are defined. |
There was a problem hiding this comment.
One thing that I think would be useful here is to add a "presets" section for setting the resource hierarchy like we do today, this would make it easier for users to opt out and then all we do internally is set the declarative config and let that figure out the layering later on.
There was a problem hiding this comment.
The resource attributes in v1beta1 will be handled by:
- spec.resource.k8sMetadata.enabled — toggle for K8s resource attributes
- spec.resource.k8sMetadata.includeUIDs — toggle for UID attributes
- spec.resource.serviceMetadata.enabled — toggle for service identity attributes
They are mutually exclusive. The service name resolution uses the spec https://opentelemetry.io/docs/specs/semconv/non-normative/k8s-attributes/#service-attributes
| metadata: | ||
| name: env-example | ||
| spec: | ||
| envConfig: |
There was a problem hiding this comment.
we should note that this will be removed/deprecated when we go to v1
There was a problem hiding this comment.
I don't think that is the goal. We might remove it if the SDKs stop supporting the env config
| 1. Aligns with OpenTelemetry's [declarative configuration](https://github.com/open-telemetry/opentelemetry-configuration) initiative | ||
| 2. Fixes structural inconsistencies in the current API | ||
|
|
||
| ## Objectives |
There was a problem hiding this comment.
Out of experience with multiple OTel adopters, treating the trace-context propagation format (and, specifically, opt-in to AWS XRay) as a first-class citizen in the CRD, pays off
There was a problem hiding this comment.
We already support in as a first-class citizen in the CRD. The goal is to support as much as possible from the stable config symbols.
|
|
||
| The operator will have a flag to control declarative configuration support (e.g., `--instrumentation-declarative-config=java,python,dotnet,cpp,nodejs`). This allows enabling support as SDKs mature without requiring operator upgrades. | ||
|
|
||
| #### Declarative config example |
There was a problem hiding this comment.
In the system packages SIG, we are having issues with the fact that all implementations of the declarative SDK but Java seem not to allow per-language overrides, e.g. to disable instrumentations. It seems to me you may face the same issue.
There was a problem hiding this comment.
Are you referring to this part of the config https://github.com/open-telemetry/opentelemetry-configuration/blob/main/schema-docs.md#experimentalinstrumentation- ?
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Description:
This PR adds RFC for Instrumenation v1beta1
Link to tracking Issue(s):
Related to #5060
Explanation fo managing multiple CRD versions: https://gist.github.com/pavolloffay/e8bf0ad2c42cd94e82f2354a3559894f
Testing:
Documentation: