feat(helm): expose the traces and logs signals, not just metrics#644
Merged
Conversation
The chart described otel.exporterEndpoint as the endpoint "its meter exports to" and wired a single OTEL_EXPORTER_OTLP_ENDPOINT. Since RFC 0038/0039/0040 that endpoint carries three signals — metrics, Ourios's own logs bridged to OTLP, and request-scoped traces — so the docs understated it and there was no way to shape the two new ones. The operational gap that mattered: OTEL_TRACES_SAMPLER defaults to parentbased_always_on, so the chart shipped 100%-sampled traces with no knob short of hand-writing extraEnv. Adds per-signal values mapping 1:1 onto standard OTEL_* names — Ourios models no bespoke telemetry config (RFC 0038 slice 4), so the chart stays a thin mapping rather than a second vocabulary: otel.traces.enabled / .sampler / .samplerArg otel.metrics.enabled / .exportInterval otel.logs.enabled Defaults render no new env at all, so existing installs are unchanged. README gains a "Self-telemetry" section covering what each signal carries, why sampling wants setting deliberately, and why pointing the endpoint at this same release's receiver is a feedback loop (ingesting a batch emits logs and spans about ingesting it) — dogfood through a Collector instead. Verified with helm lint plus helm template across defaults, all-signals disabled, and the sampler-ratio path. Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
|
Warning Review limit reached
Next review available in: 45 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 (3)
📝 WalkthroughWalkthroughThe Helm chart adds structured self-telemetry values, renders corresponding OTEL environment variables, documents the configuration, and introduces render assertions that run in CI after Helm lint. ChangesHelm OTEL configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HelmValues
participant HelmTemplate
participant RenderAssertions
participant CI
HelmValues->>HelmTemplate: supply OTEL configuration
HelmTemplate->>RenderAssertions: render OTEL_* environment variables
RenderAssertions->>RenderAssertions: compare rendered values with expectations
CI->>RenderAssertions: run chart render assertions after helm lint
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Ourios Helm chart’s self-telemetry configuration so deployments can explicitly control metrics, logs, and traces export behavior (and trace sampling), aligning the chart values with standard OTEL_* environment variables and the current three-signal reality.
Changes:
- Extend
values.yamltelemetry configuration to cover per-signal enablement plus trace sampler knobs and metric export interval. - Render standard per-signal
OTEL_*env vars (*_EXPORTER=none, sampler, export interval) from chart values. - Expand the chart README with a “Self-telemetry” section and document the new values.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| deploy/helm/ourios/values.yaml | Adds per-signal telemetry configuration keys and clarifying documentation/comments. |
| deploy/helm/ourios/templates/_helpers.tpl | Emits per-signal OTEL_* env vars (exporter enablement, trace sampler, metric export interval). |
| deploy/helm/ourios/README.md | Documents the three signals, sampling guidance, and the new Helm values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
deploy/helm/ourios/templates/_helpers.tpl:202
- Accessing
.Values.otel.traces.*directly will fail to render if a release is upgraded with--reuse-valuesfrom an older chart version whereotelonly containedexporterEndpoint(notracesmap). Guard nested lookups so missing maps default toenabled: trueand so string settings are read viadig(nil-safe).
{{- with .Values.otel.exporterEndpoint }}
- name: OTEL_EXPORTER_OTLP_ENDPOINT
value: {{ . | quote }}
{{- end }}
{{- if not .Values.otel.traces.enabled }}
deploy/helm/ourios/templates/_helpers.tpl:225
- Same nil-pointer risk applies to
.Values.otel.metrics.*and.Values.otel.logs.*when upgrading with reused older values. Use a nil-safe lookup for the nested maps and preserve an explicitly configuredenabled: false(avoiddefaulton booleans, since it treatsfalseas empty).
{{- if not .Values.otel.metrics.enabled }}
- name: OTEL_METRICS_EXPORTER
value: "none"
{{- end }}
{{- with .Values.otel.metrics.exportInterval }}
- name: OTEL_METRIC_EXPORT_INTERVAL
value: {{ . | quote }}
{{- end }}
{{- if not .Values.otel.logs.enabled }}
- name: OTEL_LOGS_EXPORTER
value: "none"
{{- end }}
…al keys
Direct traversal of .Values.otel.traces/metrics/logs nil-pointers on
`helm upgrade --reuse-values` from a release predating those keys: Helm
carries forward the OLD chart's `otel` map (exporterEndpoint only) and
does not merge the new chart's defaults, so the nested maps are absent.
Error: nil pointer evaluating interface {}.enabled
Reads them with `dig` instead. `dig` keys off *existence* rather than
emptiness, which matters here — `default true .enabled` would silently
flip an explicit `enabled: false` back on.
Adds deploy/helm/render-tests.sh, five assertions over the values
combinations the kind install does not reach: defaults emit nothing,
both absent-map upgrade paths, explicit-false is honoured, and the
sampler/interval mapping. Verified it fails on the pre-fix template
(both upgrade-path cases) and passes on the fix.
Wired into deploy-test.yml beside `helm lint` — the chart had no
render-level check before this. POSIX sed only, so it runs the same on
macOS and the Linux runner.
Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
deploy/helm/ourios/templates/_helpers.tpl:230
- Same issue as
samplerArg: usingwithhere treats numeric0as empty, so--set otel.metrics.exportInterval=0would omitOTEL_METRIC_EXPORT_INTERVAL. If0should be allowed (e.g., to disable periodic export), render when the stringified value is non-empty.
{{- with dig "metrics" "exportInterval" "" $otel }}
- name: OTEL_METRIC_EXPORT_INTERVAL
value: {{ . | quote }}
{{- end }}
Go templates treat numeric 0 as empty, so the `with`-guarded fields silently discarded `--set otel.traces.samplerArg=0`. That is a real setting, not a degenerate one: traceidratio 0 samples nothing the caller has not already sampled, which is what an operator picks when they want Ourios spans only for traces their own client chose to sample. Coerces with toString and compares against "" instead. Same treatment for sampler and metrics.exportInterval so the three read alike and nobody reintroduces `with` by copying the odd one out. Adds an assertion for the zero case; verified it fails against the `with`-based template and passes on the fix. Also fixes a stale cross-reference — the README's values table said "see Self-telemetry below" for a section that is above it (line 120 vs line 340). Now an anchor link, so it survives sections moving. Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
jensholdgaard
added a commit
that referenced
this pull request
Jul 26, 2026
…eys (#645) * fix(helm)!: configure the OTel SDK via verbatim extraEnv, not chart keys 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> * refactor(helm): keep commonEnv intact; role env is a wrapper around it 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> --------- Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The chart documented
otel.exporterEndpointas the OTLP endpoint "its meter exports to" and wired exactly one env var. That stopped being true at RFC 0038/0039/0040 — the endpoint now carries three signals:/mcp, sweep (RFC 0038), continuing an inbound caller's trace (RFC 0039), with a DataFusion operator span tree under a query (RFC 0040)The docs understating it was the reported problem. The operational gap underneath it is the one worth fixing:
OTEL_TRACES_SAMPLERdefaults toparentbased_always_on, so the chart shipped 100%-sampled traces with no knob short of hand-writingextraEnv.What
Per-signal values that map 1:1 onto standard
OTEL_*names:enabled: falserendersOTEL_{TRACES,METRICS,LOGS}_EXPORTER=none. Ourios models no bespoke telemetry config — it reads the SDK's own variables (RFC 0038 slice 4) — so the chart stays a thin mapping rather than inventing a second vocabulary. Anything unmodelled (per-signal endpoints, headers,OTEL_SDK_DISABLED, protocol) still goes throughextraEnv.README gains a Self-telemetry section: what each signal carries, why sampling wants setting deliberately, and why pointing
exporterEndpointat this same release's receiver is a feedback loop — ingesting a batch emits logs and spans about ingesting it. Dogfooding is intended, but route it through a Collector.Compatibility
No behaviour change at defaults.
helm templatewith default values renders no new env vars, so existing installs are byte-identical.Verification
deploy-test.ymlis nightly/dispatch, not per-PR, so this was verified locally:helm lint deploy/helm/ourios— 0 failedhelm templateat defaults — zeroOTEL_*env emittedhelm templatewith all three signals disabled + endpoint +exportInterval— correct=nonetrio on all three workloadshelm templatewithsampler=parentbased_traceidratio,samplerArg=0.01— both vars emittedHappy to run
deploy-test.ymlviaworkflow_dispatchon this head if you'd like the kind smoke test before merge.Invariants
Chart-only; no
CLAUDE.md§3 invariant or §4 hazard is touched. §6.3 ("observability of ourselves") is the directive this serves — it makes the two newer signals actually configurable in a deployment.Summary by CodeRabbit
New Features
Bug Fixes