open-telemetry / opentelemetry-helm-charts

OpenTelemetry Helm Charts
https://opentelemetry.io
Apache License 2.0
391 stars 470 forks source link

Chart doesn't respect null jaeger/zipkin/prometheus default receivers when used as a subchart #1357

Closed stonkie closed 3 days ago

stonkie commented 4 days ago

Hello,

The documentation says to set the default receivers to null to disable them. This doesn't work and still outputs the receiver configs, so a chizeled otel collector image that doesn't contain those receivers fails to load in the default charts.

Link to doc : https://github.com/open-telemetry/opentelemetry-helm-charts/blob/main/charts/opentelemetry-collector/README.md#basic-top-level-configuration

I would expect the jaeger entry to be absent from the ConfigMap here :

image

To show this way of configuring actually works :

image

To repro, drop this Chart.yaml file into a templates directory :

apiVersion: v2
name: opentelemetry-collector
description: Sets up an OpenTelemetry Collector
type: application
version: 1.0.0
appVersion: "0.77.0"
dependencies:
  - name: opentelemetry-collector
    repository: https://open-telemetry.github.io/opentelemetry-helm-charts
    version: 0.106.1

Then run those commands :

helm repo add opentelemetry-helm-charts https://open-telemetry.github.io/opentelemetry-helm-charts
helm dependency build
helm template test . --set "opentelemetry-collector.image.repository=test,opentelemetry-collector.mode=deployment,opentelemetry-collector.config.receivers.jaeger=null"
stonkie commented 4 days ago

Update : just tested with 0.106.1 instead of 0.104. Same result

TylerHelmuth commented 3 days ago

@stonkie are you using the opentelemetry-collector chart as a subchart?

TylerHelmuth commented 3 days ago

Here is an example showing how null does work to remove configuration when the chart is not used as a subchart.

Also remember our chart only supports Helm 3.9+. Please check that your helm version is within the supported range.

stonkie commented 3 days ago

Hi @TylerHelmuth,

Yeah I'm using it as a subchart named "opentelemetry-collector", hence the prefix to the variable names. We use this processor to lock versions and apply renovate monthly and to adjust some defaults and I figured it made for easy repro steps.

I do use a supported version of helm...

image

I also used the second screenshot to show the parameters were passed in properly.

But you're right, if I skip the subchart and use the chart directly, it applies properly.

image

I think there's something I misunderstand about null and how it merges into subchart values...

stonkie commented 3 days ago

This only occurs with subchart. Still haven't figured out exactly why, but it isn't related to a bug in the charts themselves as I initially thought.

stonkie commented 3 days ago

Looks like this is what I'm hitting : https://github.com/helm/helm/pull/12879

TylerHelmuth commented 3 days ago

Helm has a bug where using null to remove content from a subchart does not work. It is extremely annoying.

You'll want to use alternateConfig instead. You still would be able to null values from alternate config, so you should set no values for alternateConfig in the default values.yaml, and then your user config should set opentelemetry-collector.alternateConfig.

See https://github.com/open-telemetry/opentelemetry-helm-charts/pull/1301