open-telemetry / opentelemetry-helm-charts

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

make it possuble to just supply a full config #1160

Closed trondhindenes closed 1 week ago

trondhindenes commented 4 months ago

I'm finding it incredibly difficult to set up a custom collector config, because I need to disable the things I don't want. I can't just supply port values if I need non-standard ones either.

Please consider making the config easier by providing an option to just supply a full config and list of ports

JaredTan95 commented 4 months ago

have you tried --set configMap.create=false --set configMap.existingName=your-own-configmap-for-collector?

TylerHelmuth commented 4 months ago

You should be able to do

ports:
  custom:
    enabled: true
    containerPort: 9999
    servicePort: 9999
    hostPort: 9999
    protocol: TCP

to add custom ports to both the container and service. ports is a map so it will merge cleanly.

TylerHelmuth commented 4 months ago

Can you be more specific about what is difficult to configure? You can remove default components using helm's null feature such as

config:
  receivers:
    jaeger: null
    prometheus: null
    zipkin: null
  service:
    pipelines:
      traces:
        receivers:
          - otlp
      metrics: null
      logs: null

Full example: https://github.com/open-telemetry/opentelemetry-helm-charts/blob/main/charts/opentelemetry-collector/examples/deployment-otlp-traces/values.yaml

boxidau commented 3 months ago

+1 on this issue. Having to null out the default config requires merging the nulled structure into our config which seems a little messy. Changes to the default config would then be breaking changes on chart updates.

Having a second alternative key customConfig (or similar) in values where a user can define their entire config would be much easier to deal with in cases where a user has an already well formed config they'd like to use.

The other option of an externally managed ConfigMap is okay and gives the flexibility but loses the auto-reload functionality on config updates

lerminou commented 1 month ago

@TylerHelmuth , it seems to not work with null inside the config directly, when using a subchart: https://helm.sh/docs/chart_template_guide/values_files/#deleting-a-default-key https://github.com/helm/helm/pull/11440 https://github.com/helm/helm/issues/11617 https://github.com/helm/helm/issues/12637

If I try on my own, following the 'otel-trace only' https://github.com/open-telemetry/opentelemetry-helm-charts/tree/main/charts/opentelemetry-collector/examples/deployment-otlp-traces

I tried:

opentelemetry-collector:
  config:
    receivers:
      jaeger: null
      prometheus: null
      zipkin: null
    service:
      pipelines:
        traces:
          receivers:
            - otlp
        metrics: null
        logs: null

or more specific:

opentelemetry-collector:
  config:
    service:
      pipelines:
        traces:
          exporters:
            - otlp
          receivers:
            - otlp
            - zipkin
        metrics:
          exporters: {} // tried null or [] or {}
          processors: {}
          receivers: {}
        logs:
          exporters: {}
          processors: {}
          receivers: {}

in each case I have an error during the helm install:

coalesce.go:286: warning: cannot overwrite table with non table for otel-collector.opentelemetry-collector.config.service.pipelines.metrics (map[exporters:[debug] processors:[memory_limiter batch] receivers:[otlp prometheus]])
coalesce.go:286: warning: cannot overwrite table with non table for otel-collector.opentelemetry-collector.config.service.pipelines.metrics (map[exporters:[debug] processors:[memory_limiter batch] receivers:[otlp prometheus]])
coalesce.go:286: warning: cannot overwrite table with non table for otel-collector.opentelemetry-collector.config.service.pipelines.metrics (map[exporters:[debug] processors:[memory_limiter batch] receivers:[otlp prometheus]])

And the generated configmap in my cluster has all the keys.

Helm 3.13 or 3.15 so yeah, it's difficult to use

TylerHelmuth commented 1 month ago

I really really don't want to have to do anything special in the chart when this is truly a helm bug. But, considering this bug still isn't fixed, I'm willing to discuss work around solutions.

TylerHelmuth commented 1 month ago

Things I'd like to see in a solution:

  1. non-breaking change.
  2. Maybe still works with presets