fix(helm)!: configure the OTel SDK via verbatim extraEnv, not chart keys#645
Conversation
Reverts #644's per-signal values surface (otel.traces/metrics/logs) the same day it landed, before any chart publish carried it. The maintainer call: OTel is opinionated and its users already know its env-var contract — the chart wrapping OTEL_TRACES_SAMPLER and friends in chart-specific keys couples us to a naming vocabulary we don't own. Both review-found bugs in #644 (the --reuse-values nil-pointer and the numeric-zero samplerArg drop) existed only because of that re-modeling; a verbatim passthrough cannot have either bug class. This is the chart- level mirror of RFC 0038 slice 4, which removed bespoke telemetry config from the binary in favour of the universal OTEL_* variables. The one OTel knob that stays modeled is otel.exporterEndpoint: deployment topology (where this release sends its telemetry) is the chart's to own; SDK behaviour is not. S3 config stays explicit values for the inverse reason — a hard dependency whose contract is ours. What replaces the keys: - global extraEnv (existed) documented as the OTel surface, with the spec-defined variables listed in a comment block + links to the authoritative pages (sdk-environment-variables, protocol/exporter) - per-role receiver/querier/compactor.extraEnv (new), appended after the global list — Kubernetes resolves a duplicate name to the last entry, so two roles can carry different values for the same variable (e.g. per-role OTEL_RESOURCE_ATTRIBUTES), pinned by render tests - ourios.commonEnv becomes role-aware ourios.workloadEnv; the s3 credentials envFrom path is untouched (asserted by a render test) BREAKING CHANGE: the otel.traces/otel.metrics/otel.logs values from #644 are removed (never published in a chart release; merged to main earlier the same day). Set the corresponding OTEL_* variables in extraEnv instead. Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Helm chart now models only the OTLP exporter endpoint and passes other OpenTelemetry settings through global or role-specific ChangesOpenTelemetry environment passthrough
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HelmValues
participant workloadEnv
participant Workload
HelmValues->>workloadEnv: provide endpoint and extraEnv values
workloadEnv->>workloadEnv: append global extraEnv then role extraEnv
workloadEnv->>Workload: render ordered env entries
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR reverts the Helm chart’s modeled OpenTelemetry SDK configuration surface (per-signal otel.* keys) in favor of passing OTel’s standard OTEL_* environment variables through verbatim via extraEnv, while keeping otel.exporterEndpoint as the single modeled telemetry value owned by the chart. It also introduces per-role extraEnv (receiver/querier/compactor) so role-specific overrides can be applied via Kubernetes’ “last env entry wins” behavior.
Changes:
- Remove per-signal OTel chart values and document/configure OTel SDK behavior via global + per-role
extraEnv. - Replace
ourios.commonEnvwith role-awareourios.workloadEnvand wire it into all three workloads. - Rewrite Helm render assertions to validate the new env passthrough/override contract and guard upgrade/reuse-values shapes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| deploy/helm/render-tests.sh | Reworks render assertions to validate verbatim extraEnv passthrough, per-role scoping, and duplicate-key ordering. |
| deploy/helm/ourios/values.yaml | Removes per-signal otel.* keys; documents OTel SDK env var contract and adds per-role extraEnv. |
| deploy/helm/ourios/templates/receiver-statefulset.yaml | Switches env rendering to ourios.workloadEnv for the receiver role. |
| deploy/helm/ourios/templates/querier-deployment.yaml | Switches env rendering to ourios.workloadEnv for the querier role. |
| deploy/helm/ourios/templates/compactor-deployment.yaml | Switches env rendering to ourios.workloadEnv for the compactor role. |
| deploy/helm/ourios/templates/_helpers.tpl | Implements ourios.workloadEnv and appends role-specific extraEnv after global extraEnv. |
| deploy/helm/ourios/README.md | Updates “Self-telemetry” docs and values table to reflect extraEnv-based OTel configuration and role overrides. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review of the diff showed the S3-adjacent lines (AWS_DEFAULT_REGION and its region guard) changing for no functional reason: switching the helper's calling convention to (dict "root" "role") forced a mechanical .Values -> $root.Values rewrite of every line inside it. That is noise in a PR whose contract is "S3 mechanics untouched". Restore ourios.commonEnv to root-context — its S3 lines are now byte-identical to main — and layer the role extraEnv in a thin ourios.workloadEnv wrapper that includes it. Render output is unchanged (the 10 assertions pass before and after). Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
Why
Reverts most of #644's values surface the same day it landed, on a maintainer call about ownership:
The per-signal keys (
otel.traces.sampler,otel.metrics.exportInterval, …) re-modeled the OTel SDK's env-var vocabulary as chart values — a naming contract the chart doesn't own. The evidence that the coupling had real cost is #644's own review history: both bugs found there (the--reuse-valuesnil-pointer, the numeric-zerosamplerArgdrop) existed only because the chart interpreted those values. A verbatim passthrough cannot have either bug class.This is also the chart-level mirror of RFC 0038 slice 4, which removed bespoke telemetry config from the binary in favour of the universal
OTEL_*variables.The ownership line
otel.exporterEndpointextraEnvWhat
extraEnv(pre-existing) documented as the OTel surface, with the spec-defined variables listed in a comment block + links to the two authoritative pages (general SDK, OTLP exporter) — the list itself was checked against the OTel docs, not written from memory.receiver/querier/compactor.extraEnv(new), appended after the global list. Kubernetes resolves a duplicate env name to the last entry, so two roles can carry different values for the same variable — the motivating case is per-roleOTEL_RESOURCE_ATTRIBUTES.ourios.commonEnv→ role-awareourios.workloadEnv. Thes3CredentialsEnvFromhelper is byte-identical to main, and a render assertion pins that the s3envFromis unaffected by role env being set.Breaking change
otel.traces/metrics/logsvalues are removed. Marked!for honesty, though the blast radius is nil in practice: the keys merged to main earlier today andchart.yml(dispatch-only) never published them; no chart release ever carried them.Verification
deploy/helm/render-tests.shrewritten for the new contract — 10 assertions: defaults render noOTEL_*, endpoint renders,--reuse-valuesshape safety, global passthrough verbatim, role scoping both directions, the duplicate-key ordering on all three roles, and the s3 envFrom no-touch. All green locally +helm lintclean. (deploy-test.ymlremains nightly/dispatch, not per-PR.)Invariants
Chart-only. No
CLAUDE.md§3 invariant or §4 hazard touched; §6.3 self-observability is served with a smaller owned surface.Summary by CodeRabbit
New Features
OTEL_*environment variables.Documentation
Breaking Changes
extraEnvinstead.