open-telemetry / opentelemetry-helm-charts

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

Enable "env" configuration for jaeger deployment #515

Closed tglanz closed 1 year ago

tglanz commented 1 year ago

I'd like to use an ElasticSearch cluster as a storage backend for Jaeger. For this I need to set some environment variables in Jaeger's containers. Some of the variables should be taken from the k8s cluster's secrets so it would be ideal to have an option configuring something like

jaeger:
  env:
  - name: ELASTIC_USERNAME
     valueFrom:
       secretKeyRef:
         name: elastic-credentials
         key: username
  - name: ELASTIC_PASSWORD
     valueFrom:
       secretKeyRef:
         name: elastic-credentials
         key: password
tglanz commented 1 year ago

Am wiling to work on it if that's something your'e interested in adding

TylerHelmuth commented 1 year ago

@tglanz I think you're right, this should be supported.

But this brings up the point again that @dmitryax has made in the past: should we be using the jaeger helm chart as a subchart instead of adding these capabilities ourselves? A quick look at the jaegar chart and it appears to support ElasticSearch as a storage solution as well as env vars, affinities, tolerations, and node selectors.

@puckpuck do you see any downside to using the jaeger chart as a sub chart?

tglanz commented 1 year ago

@TylerHelmuth I'm not sure but it looks like Jaeger's chart doesn't support in-memory mode? Which is this chart's default

puckpuck commented 1 year ago

Moving to a Jaeger chart is a good idea here.

The downside is we may break backward compatibility for anyone that configured something specific to the Jaeger deployment.

dmitryax commented 1 year ago

Yes, I believe we need to use the jaeger helm chart as a sub-chart. We may be able to support the current jaeger config interface as deprecated and translate it into the sub-chart config, but it may be too much work

TylerHelmuth commented 1 year ago

@tglanz it looks like the chart can support in-memory mode/all-in-one which is good news for us.

Since this chart is relatively new, for a demo, and doesn't supply that many configuration options for jaeger I would be in support of introducing the jaeger subchart as a breaking change.

puckpuck commented 1 year ago

The breaking change would also be the catalyst to do a proper readme for the chart as well finally.

puckpuck commented 1 year ago

I went down the path of using Jaeger as a sub-chart.

Unfortunately, the Jaeger Helm chart doesn't implement a way to pass in extra args for the all-in-one mode. I still think doing Jaeger as a sub-chart is a good idea, but now I need to figure out how to PR a change into the Jaeger helm chart.