Update operator integration statuses and cli arg options #1251
Conversation
|
jvoravong
changed the title
…e to values.schema.json validation blocker
| yaml_file_path: 'helm-charts/splunk-otel-collector/Chart.yaml' | ||
| dependency_name: 'opentelemetry-operator' | ||
| # TODO: Add this back once this chart has been updated to adhere to | ||
| # the operator chart schema validation rules. See: https://github.com/open-telemetry/opentelemetry-helm-charts/pull/1065 |
There was a problem hiding this comment.
What extra properties do we supply?
There was a problem hiding this comment.
The upstream operator chart doesn't have a field to enable adding default instrumentation anymore since open-telemetry/opentelemetry-helm-charts#1065 was added. This affected us and other companies.
- Our use of
instrumentation.*values fails validation tests - Planning on working with the upstream to add default instrumentation to the operator chart
- Upstream added default instrumentation to the upstream kube-stack chart [opentelemetry-kube-stack] Enable autoinstrumentation creation open-telemetry/opentelemetry-helm-charts#1078
- We are likely going to have to align a little with upstream for enabling default instrumentation
There was a problem hiding this comment.
doesn't have a field to enable adding default instrumentation
Which field in instrumentation.*?
There was a problem hiding this comment.
I cannot find instrumentation section in https://github.com/open-telemetry/opentelemetry-helm-charts/blob/main/charts/opentelemetry-operator/values.yaml. Is this only used on our side?
There was a problem hiding this comment.
We should've not used a section that is supposed to be passed to the sub-chart for our own purposes. Now we should either:
- move
instrumentationout fromoperatorbreaking users relying on this option - or change the
operatoralias breaking user passing stuff to the operator.
The second option seems preferable to me. Similar to what you do, but instead of removing subchart, remove the alias
There was a problem hiding this comment.
I understand. But this is still wrong. open-telemetry/opentelemetry-helm-charts#1065 should not be reverted. There is nothing wrong with that PR
There was a problem hiding this comment.
Trying to explain why it's wrong: Imagine the sub-chart adds support of the instrumentation section with conflicting behavior.
There was a problem hiding this comment.
Actually, I think we should move instrumentation out of operator and keep the alias. Otherwise we break operator.enabled and operator.admissionWebhooks.
There was a problem hiding this comment.
- Yeah option 1 is starting to sound good to me. Going to think on it a little more. I wanted to handle updating our instrumentation helm values as part of a separate ticket.
- I should also mention the related blocker for this PR. The operator replaced several instrumentation feature gates with command-line arguments incrementally from version 0.95.0 (used before this PR) to version 0.99.0 (used after this PR). This is why I want to upgrade to operator v0.99.0 in this PR. Otherwise, our documentation would have to include feature gates and CLI arguments at the same time which is more confusing.
There was a problem hiding this comment.
This ticket is blocked until we complete the following in a separate PR.
- Complete option 1, move operator.instrumentation out of the operator sub-chart values.
- Upgrade the operator sub-chart to the latest version
- This will include moving some code from this PR to the separate PR.
Description: