From 16259008d51ad14e3700024d51fca3cd966f2d76 Mon Sep 17 00:00:00 2001 From: yxxhero <11087727+yxxhero@users.noreply.github.com> Date: Mon, 7 Sep 2026 20:34:10 +0800 Subject: [PATCH] feat: opt-in OpenTelemetry tracing and metrics (experimental) (#2769) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(telemetry): add opt-in OpenTelemetry tracing (PR 1: lifecycle + root span) Implements the first increment of docs/proposals/otel-tracing.md (#2767): - pkg/telemetry: SDK setup from standard OTEL_* env vars (autoexport for exporter selection, env-driven sampler/propagators, OTEL_SDK_DISABLED), command-span lifecycle, no-op-by-default accessors - --otel-tracing flag / HELMFILE_OTEL_TRACING env switch - root span "helmfile " with file/environment/selectors/exit_code attributes; TRACEPARENT-based remote-parent extraction for CI correlation - shutdown flush on both normal-exit and signal paths (nil-safe, 5s bound) - app.New derives its context from telemetry.CommandContext() (Background-identical when tracing is disabled) - docs: otel.md user guide, experimental-features entry, design proposal - tests: hermetic unit tests, app context-contract pinning, flag registration Telemetry problems never fail a run: exporter misconfiguration and export errors degrade to disabled with a warning. When disabled, behavior and performance are identical to before (no-op tracer, no goroutines, no network). Refs: #2767, #2758 Signed-off-by: yxxhero * feat(telemetry): trace every external process + trace-context bridges (PR 2) Implements the second increment of docs/proposals/otel-tracing.md (#2767): - pkg/helmexec/span.go: one span per external process started by helmfile (helm invocations, hooks, plugin execs) at the ShellRunner choke point — helm.exec (with helm.subcommand) vs os.exec, with redacted exec.args, exec.exit_code, and error status on failure - pkg/helmexec/redact.go: shared argument redaction with two profiles; legacy is byte-identical to the historical exit-error behavior (existing goldens unchanged), strict (spans) additionally covers --set=k=v and credential flags; exit_error.go now uses the shared helper - orphan-trace bridges with bit-identical cancellation semantics (context.WithoutCancel of the command context): both kubedog call sites (state.go) and hook execution (event.Bus gains an optional Ctx consumed by its default runner; state.go sets it, nil falls back to TODO as before) - OTLP end-to-end test (in-process httptest receiver, no external collector): span export, error status/exit code, redaction, and parent-linkage to the command span - docs/otel.md updated to the now-traced surface Verified end-to-end with the console exporter: helmfile template on a local chart yields the command span plus helm.exec spans for helm version/dependency/template, all nested under it. Refs: #2767 Signed-off-by: yxxhero * feat(telemetry): state-loading and hook spans (PR 3a) Implements the third increment of docs/proposals/otel-tracing.md (#2767): - helmfile.discover_states around findDesiredStateFiles and helmfile.load around loadDesiredStateFromYamlWithBaseDir; both cover all callers (incl. nested helmfiles) with no signature changes - helmfile.render / helmfile.parse children per document part, parented through a traceCtx field on the unexported desiredStateLoader struct (set once at its single construction site) - helmfile.hook span per hook execution: Trigger's per-hook body extracted into runHook (readability win on its own), the hook's subprocess span nests under it via a per-hook ctx-swapped ShellRunner clone (cancellation unchanged — Bus.Ctx never carries cancellation by contract) - pkg/telemetry/otlptest: shared in-process OTLP/HTTP receiver harness, now used by helmexec, event, and app span tests - golden span-tree test at the app layer (root -> discover -> load -> render/parse, via the exectest fake helm) and a hook-span nesting test - nil-ctx guard for App literals built directly by tests (App.spanParentCtx) Verified end-to-end with the console exporter: a template run over a gotmpl state file with a prepare hook yields the full tree with the hook's os.exec nested under helmfile.hook. Refs: #2767 Signed-off-by: yxxhero * feat(telemetry): per-release spans nested under the load span (PR 3b) Implements the per-release increment of docs/proposals/otel-tracing.md (#2767) — spans nest command -> load -> release -> helm exec: - helmexec.HelmContext gains an optional Ctx carrying the per-release span context; the execer's new execWithContext funnel consumes it via a per-call runner clone (runnerWithCtx) so the shared, cached execer is never mutated across concurrent workers. The seven Interface methods that take a HelmContext (Sync/Diff/ReleaseStatus/List/DecryptSecret/ Delete/Test) route through it; nil Ctx behaves exactly as before. exec() lost its always-nil override parameter on the way (unparam). - pkg/state/span.go: SetTraceContext + startReleaseSpan/endReleaseSpan helpers (release/namespace/chart/labels attributes, sorted for stable output); a typed-nil guard (releaseErrAsError) avoids the classic nil-pointer-in-interface trap on *ReleaseError. - release spans in the worker loops: SyncReleases, DiffReleases, DeleteReleasesForSync, PrepareCharts, and iterateOnReleases (status/ delete/test via a new verb parameter); their HelmContext is stamped with the release span context where one is built. - pkg/app sets st.SetTraceContext(loadCtx) right after loading a state file, rooting all per-release spans under helmfile.load. - bridged one more detached tracking call found on the way (trackReleaseIfEnabled's context.Background in the sync worker). - golden test: release span present, nested under load, correct attributes; unit test for the runnerWithCtx clone semantics. Verified with the console exporter: helmfile template yields release.prepare(demo) under load with full attributes. Refs: #2767 Signed-off-by: yxxhero * feat(telemetry): nest status/delete/test execs under their release spans Completes the per-release exec nesting for the iterateOnReleases-based loops (docs/proposals/otel-tracing.md §4.4 phase 2): the do closures now receive the release span context and stamp it into their HelmContext, so helm status/delete/test subprocess spans nest under helmfile.release. like sync/diff already did. - scatterGatherReleases/iterateOnReleases/doWithReleaseSpan: do gains a context parameter (the release span context) - ReleaseStatuses/DeleteReleases/TestReleases closures stamp HelmContext.Ctx from it - integration test with a real execer (version-probe shim binary): the release's status subprocess nests under helmfile.release.status, same trace, with helm.subcommand=status This also makes the otel.md claim ("upgrade, diff, delete, status, test nested under the release span") fully accurate. Refs: #2767 Signed-off-by: yxxhero * feat(telemetry): OTel metrics — helm exec duration and release results (PR 4) Implements the metrics increment of docs/proposals/otel-tracing.md (#2767) on the same provider, switch, and resource as traces: - pkg/telemetry/metrics.go: helmfile.helm.exec.duration histogram (subcommand, success) and helmfile.release.count counter (verb, result). Instruments come from the otel global meter, so recording at call sites is branch-free no-op when telemetry is disabled. - Setup builds the resource once and installs both providers; reader selection delegates to autoexport (OTEL_METRICS_EXPORTER: otlp | console | prometheus | none), the OTLP reader's interval honors OTEL_METRIC_EXPORT_INTERVAL (read by the SDK). Shutdown flushes both providers (errors.Join). StartCommandSpan now carries the meter provider across state transitions (fixes a nil-shutdown panic). - helmexec: finishExecSpan records exec duration for helm binaries; state: endReleaseSpan counts release outcomes for sync/diff/delete/ status/test/prepare (diff counted as success when no hard error). - otlptest: recorder routes by OTLP path (/v1/traces vs /v1/metrics) and decodes metrics; new FindMetric helper. - tests: metrics recorded as no-op when disabled, provider enabled with the none exporter, degradation on an invalid metrics exporter, and an integration assertion (status exec duration datapoint + one successful release.count) in the shim-based state test. Verified with the console exporter: helmfile template emits helmfile.helm.exec.duration per subcommand (version/dependency/ template) and helmfile.release.count{verb=prepare,result=success}=1. Refs: #2767 Signed-off-by: yxxhero * docs: complete OTel documentation coverage - docs/cli.md: --otel-tracing in the CLI reference help block (verbatim from the cobra output) - CHANGELOG.md: [Unreleased] Added entry for tracing + metrics - docs/index.md: Observability highlight linking docs/otel.md - docs/proposals/otel-tracing.md: add OTEL_METRICS_EXPORTER / OTEL_METRIC_EXPORT_INTERVAL rows to the env-var table and note the periodic reader + bounded metric cardinality in §7 Refs: #2767 Signed-off-by: yxxhero * fix: drop unused id parameter from parsePart (unparam) The id parameter was never used inside the span wrapper; the caller's id variable is still used for the render calls and error messages. Refs: #2769 Signed-off-by: yxxhero * fix(telemetry): address review — redaction gaps, kubedog valve, phantom metrics Addresses all Copilot review comments on #2769: Security (span payloads): - exec.args: positional arguments are additionally passed through helmexec.RedactedURL, so credentials embedded in chart/repository URLs (AddRepo, RegistryLogin, OCI refs) are masked exactly like log output - release spans sanitize helmfile.chart the same way - error statuses no longer embed raw errors (which contain rendered commands, arguments, and subprocess output): the command span, release spans, hook spans, and exec spans now use generic descriptions; the concrete exit code remains an attribute, and RecordError on the root span is dropped Correctness: - kubedog safety valve restored: execWithContext now attaches the per-release span into the runner's own context instead of replacing it, so trackHandle.Cancel() can interrupt a wedged helm again and app cancellation semantics stay exactly as before the PR - diff release spans/metrics: real failures are recorded (exit code 2 "changes detected" still counts as success); previously every diff was exported as successful - skipped releases no longer emit phantom spans and inflate helmfile.release.count: iterateOnReleases callers pass a skip predicate (skipUndesired for status/test; delete deletes undesired releases and passes nil) - Setup shuts down the already-constructed tracer provider (bounded) when the metrics provider fails, instead of abandoning its batch goroutine Tests: URL redaction cases (masked/untouched), spanAttachedContext preserves the runner cancellation chain while attaching the caller's span, skipUndesired, and a failing-hook span asserting the generic message. Refs: #2769 Signed-off-by: yxxhero * fix: lint — restore nolint placement and avoid nil context literal - the skipUndesired insertion had displaced the // nolint: unparam directive off iterateOnReleases (helm param is intentionally unused there); also fixes a skipDesired/skipUndesired comment typo - use a typed nil in TestSpanAttachedContext (staticcheck SA1012) Refs: #2769 Signed-off-by: yxxhero * fix(telemetry): address review round 2 — remote-ref redaction, wrapper helm binaries, hook release attribution Addresses all 6 new review comments on #2769: Security (remote references): - new helmexec.RedactedRef sanitizes go-getter style references for telemetry: forced-form prefixes (git::, s3::) preserved, whole URL userinfo masked (usernames carry tokens too), credential-bearing query parameters masked using pkg/remote's heuristic (token/password/secret/ key/signature). Applied to helmfile.file (command span), helmfile.path (discover_states), helmfile.chart (release spans), and exec.args — log-time RedactedURL is untouched so log output is unchanged Correctness: - wrapper helm binaries (--helm-binary custom names) are now classified as helm operations by an explicit context marker stamped in the execer funnel, instead of the executable-basename heuristic; the same classification gates helmfile.helm.exec.duration, so the metric no longer misses wrapper invocations (classifyExec) - release-scoped hooks (presync/postsync/preuninstall/postuninstall/ cleanup in the sync/delete/diff workers) now attach their helmfile.hook spans to the active helmfile.release.* span via a variadic parent on the trigger functions; global hooks keep the command context and all 29 existing call sites compile unchanged; hook cancellation stays detached (WithoutCancel) as before - signal-terminated runs (Shutdown with exitCode 130/143 and nil error) now mark the command span with error status, consistent with their nonzero exit code Tests: RedactedRef table (forced forms, userinfo, s3/token query params, untouched cases), classifyExec marker case, hookTraceContext parent attribution + non-cancellability + fallback. Refs: #2769 Signed-off-by: yxxhero * fix(telemetry): address review round 3 — redaction corner cases, value runners Addresses 5 of the 6 new review comments on #2769 (the sixth — an unused strings import in exit_error.go — is a false positive: Indent still uses strings.Split/Builder and the package compiles): - RedactArgs read the previous token from the progressively redacted output, so {--set, --set-string, secret} leaked the secret (the masked value hid the following flag). Read the previous token from the original input, restoring the legacy contract for adjacent secret flags - RedactedRef fails closed for malformed references: URL-like refs with invalid percent escapes export a fully redacted value, and an unparseable query is dropped entirely instead of exported verbatim - ShellRunner has value receivers, so a ShellRunner VALUE satisfies the Runner API; the helm marker stamping and the per-release span attachment now handle both value and pointer forms (matching WithContext), so value-runner callers keep release nesting and the helm.exec classification/metric Regression tests: adjacent secret flags (legacy + strict), malformed URL-like ref, malformed query, value-runner marker + span attachment. Refs: #2769 Signed-off-by: yxxhero * fix(telemetry): stamp the helm marker on the stdin funnel too execStdIn (registry login, repo add) called the runner directly, so wrapper --helm-binary names were misclassified as os.exec and omitted from helmfile.helm.exec.duration on that path. The marking now goes through a shared markHelmRunner helper (value and pointer ShellRunner forms) used by both execution funnels. Refs: #2769 Signed-off-by: yxxhero * fix(telemetry): redact helm's --kube-token in strict profile Helm's global --kube-token carries a bearer token; both the two-argument and inline forms are now masked in span exec.args (legacy exit-error output is untouched, matching its historical behavior). Refs: #2769 Signed-off-by: yxxhero * fix(telemetry): OTel metrics best-practice alignment - helmfile.helm.exec.duration now declares explicit bucket boundaries tuned for seconds-scale helm invocations (5ms…600s); the SDK defaults are millisecond-oriented and lumped every sub-5s invocation — the common case — into the first bucket, defeating the histogram - instruments are re-created under the installed provider with the instrumentation scope version stamped (Setup-time, race-free) - helmfile.release.count declares the {release} curly-annotation unit per the metrics naming conventions Tested end-to-end via the OTLP integration test: exported bounds are the tuned set, units are asserted, and the scope carries the version. Refs: #2769 Signed-off-by: yxxhero * feat(telemetry): per-release duration metrics behind an opt-in switch New helmfile.release.duration histogram (seconds, same tuned buckets) with bounded dimensions by default (verb, result). Setting HELMFILE_OTEL_METRICS_PER_RELEASE=true adds helmfile.release and helmfile.namespace, answering "which release is slow" from dashboards: - well-suited to bounded CI runs; long-lived centralized collection needs a backend capacity/TTL story (documented in docs/otel.md) - per-release timing remains available in traces without the flag - env read per call (release operations are low-frequency, and tests toggle it) endReleaseSpan now takes the release and the operation start time; the five worker-loop call sites pass them (doWithReleaseSpan, SyncReleases, DeleteReleasesForSync, PrepareCharts, DiffReleases). Verified end-to-end with the console exporter (default dims vs per-release) and OTLP integration tests pinning both modes. Refs: #2767, #2769 Signed-off-by: yxxhero * refactor(telemetry): maintainability pass over the runner/metric plumbing - StartCommandSpan copies the tracingState struct instead of enumerating fields by hand — that pattern dropped the meter provider once already - the two value/pointer ShellRunner switches (helm marker, span attachment) are unified into one withRunnerCtx helper; the duplication caused two review rounds of value-form misses - classifyExec derives the helm classification from the span name (helmExecSpanName constant) instead of returning a third parallel bool - metrics: shared outcomeAttrs for the verb/result dimensions, and the bucket slice renamed to durationBuckets with a comment covering both histograms that use it No behavior change; full -race suite green, lint clean. Refs: #2769 Signed-off-by: yxxhero * refactor(telemetry): consolidate test env lists, trace bridges, and hook prep; sync the design doc Maintainability: - HermeticEnvVars is now exported from pkg/telemetry (the owner of the env surface) and used by both telemetry tests and otlptest — the two copies had already drifted once (HELMFILE_OTEL_METRICS_PER_RELEASE needed updating in both) - kubedogTraceContext and hookTraceContext were the same concept written twice; unified into traceOnlyContext(parent...) in span.go Readability: - runHook's nested kubectl rewrite extracted into prepareKubectlHook with guard-clause structure Accuracy (docs ↔ code, drifted over five review rounds): - §4.4 now describes the implemented mechanism: the release span is INJECTED into the runner's own context (preserving the kubedog safety valve) rather than the runner context being replaced, and helm classification is marker-based for wrapper binaries - §5 exec span rows list the actual attributes incl. URL/query masking - §6 strict profile documents RedactedRef, --kube-token, and the adjacent-token guarantee No behavior change; full -race suite green, lint clean. Refs: #2769 Signed-off-by: yxxhero * refactor(telemetry): drop the dead noop state, relocate skipUndesired, sync user-facing accuracy - tracingState.noop was dead weight in the enabled state and a copy-surface in every transition; a single package-level noopTracerProvider now backs Tracer while disabled - skipUndesired moved next to doWithReleaseSpan in span.go, its only conceptual home (span/metric suppression, not run plumbing) - accuracy: the package doc, --otel-tracing flag help, experimental-features entry, and CHANGELOG now all say tracing AND metrics and list the third instrument (helmfile.release.duration with the HELMFILE_OTEL_METRICS_PER_RELEASE opt-in) — these had drifted when the metric was added; the PR description's metric table is updated to match as well No behavior change; full -race suite green (except the pre-existing network-dependent TestStorage_resolveFile flake), lint clean. Refs: #2769 Signed-off-by: yxxhero * refactor(telemetry): flatten Setup, name the prefix bound, dedupe test fake; fix instrument-count drift Readability/maintainability: - Setup drops from 56 to 39 lines: provider construction (including the shutdown-tracer-on-meter-failure recovery) moves to newProviders in exporter.go next to the constructors it composes - refredact's magic 16 becomes maxForcedFormPrefix with a comment - span_test's hand-rolled fakeRunner removed in favor of the existing mockRunner (same package) Accuracy: - "Two instruments" wording survived in docs/otel.md and the design proposal §7 after helmfile.release.duration was added; both now say three and mention the per-release opt-in No behavior change; full -race suite green (except the pre-existing network flake), lint clean. Refs: #2769 Signed-off-by: yxxhero * refactor(telemetry): co-locate span machinery, drop a dead export, fix docs nits - the span plumbing helpers (markHelmExec, withRunnerCtx, markHelmRunner, spanAttachedContext) move from exec.go to span.go, next to the marker type and classifiers they serve — exec.go keeps only the funnel call sites - otlptest.SpanNames was never used outside the package; unexported - isHelmBinary's comment now states it is the FALLBACK classifier (funnel invocations are marker-classified), replacing the outdated "cosmetic distinction" framing from before the marker existed - docs/otel.md: release-scoped hooks nest under their release span (added in review round 2, never documented) No behavior change; full -race suite green, lint clean. Refs: #2769 Signed-off-by: yxxhero --------- Signed-off-by: yxxhero --- CHANGELOG.md | 1 + cmd/root.go | 32 ++ cmd/root_test.go | 12 + docs/cli.md | 3 + docs/experimental-features.md | 14 + docs/index.md | 2 + docs/otel.md | 110 ++++++ docs/proposals/otel-tracing.md | 555 +++++++++++++++++++++++++++ go.mod | 36 +- go.sum | 2 + main.go | 39 ++ mkdocs.yml | 1 + pkg/app/app.go | 37 +- pkg/app/app_load_span_test.go | 190 +++++++++ pkg/app/app_new_context_test.go | 67 ++++ pkg/app/desired_state_file_loader.go | 36 +- pkg/app/two_pass_renderer.go | 9 + pkg/config/global.go | 11 + pkg/envvar/const.go | 2 + pkg/event/bus.go | 221 +++++++---- pkg/event/bus_span_test.go | 113 ++++++ pkg/event/bus_test.go | 27 ++ pkg/helmexec/context.go | 9 + pkg/helmexec/exec.go | 89 +++-- pkg/helmexec/exec_test.go | 12 +- pkg/helmexec/exit_error.go | 11 +- pkg/helmexec/redact.go | 106 +++++ pkg/helmexec/redact_test.go | 241 ++++++++++++ pkg/helmexec/refredact.go | 91 +++++ pkg/helmexec/refredact_test.go | 66 ++++ pkg/helmexec/runner.go | 34 +- pkg/helmexec/span.go | 168 ++++++++ pkg/helmexec/span_test.go | 166 ++++++++ pkg/state/span.go | 131 +++++++ pkg/state/span_test.go | 256 ++++++++++++ pkg/state/state.go | 105 +++-- pkg/state/state_run.go | 13 +- pkg/telemetry/export_test.go | 5 + pkg/telemetry/exporter.go | 205 ++++++++++ pkg/telemetry/exporter_test.go | 198 ++++++++++ pkg/telemetry/metrics.go | 135 +++++++ pkg/telemetry/otlptest/otlptest.go | 195 ++++++++++ pkg/telemetry/telemetry.go | 236 ++++++++++++ pkg/telemetry/telemetry_test.go | 207 ++++++++++ 44 files changed, 4027 insertions(+), 172 deletions(-) create mode 100644 docs/otel.md create mode 100644 docs/proposals/otel-tracing.md create mode 100644 pkg/app/app_load_span_test.go create mode 100644 pkg/app/app_new_context_test.go create mode 100644 pkg/event/bus_span_test.go create mode 100644 pkg/helmexec/redact.go create mode 100644 pkg/helmexec/redact_test.go create mode 100644 pkg/helmexec/refredact.go create mode 100644 pkg/helmexec/refredact_test.go create mode 100644 pkg/helmexec/span.go create mode 100644 pkg/helmexec/span_test.go create mode 100644 pkg/state/span.go create mode 100644 pkg/state/span_test.go create mode 100644 pkg/telemetry/export_test.go create mode 100644 pkg/telemetry/exporter.go create mode 100644 pkg/telemetry/exporter_test.go create mode 100644 pkg/telemetry/metrics.go create mode 100644 pkg/telemetry/otlptest/otlptest.go create mode 100644 pkg/telemetry/telemetry.go create mode 100644 pkg/telemetry/telemetry_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index ea93d000..d430e865 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ### Added +- Add opt-in OpenTelemetry tracing and metrics (`--otel-tracing` / `HELMFILE_OTEL_TRACING`, experimental): command/state-load/release/hook spans with one span per helm subprocess, plus `helmfile.helm.exec.duration`, `helmfile.release.duration`, and `helmfile.release.count` metrics (per-release dimensions opt-in via `HELMFILE_OTEL_METRICS_PER_RELEASE`), exported via standard `OTEL_*` environment variables. See [docs/otel.md](docs/otel.md) and the [design proposal](docs/proposals/otel-tracing.md) (#2767) - Add support for `conditionTemplate` and allow `condition` to be set directly to `true` or `false`. - Add `--allow-failed-releases` global flag to continue preparing charts for the remaining releases when chart preparation fails for a release; failed releases are skipped and all failures are reported at the end (#2616) diff --git a/cmd/root.go b/cmd/root.go index fede132a..dd417d69 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -7,6 +7,7 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" + "go.opentelemetry.io/otel/attribute" "go.szostok.io/version/extension" "go.uber.org/zap" @@ -17,6 +18,7 @@ import ( "github.com/helmfile/helmfile/pkg/errors" "github.com/helmfile/helmfile/pkg/helmexec" "github.com/helmfile/helmfile/pkg/runtime" + "github.com/helmfile/helmfile/pkg/telemetry" ) var logger *zap.SugaredLogger @@ -75,6 +77,19 @@ func NewRootCmd(globalConfig *config.GlobalOptions) (*cobra.Command, error) { } logger = helmexec.NewLogger(logOut, logLevel) globalConfig.SetLogger(logger) + + // OpenTelemetry tracing (experimental). Setup and StartCommandSpan + // are no-ops when tracing is disabled, so the default path is + // unchanged. Configuration beyond the on/off switch comes from the + // standard OTEL_* environment variables. + cmdName := "helmfile " + c.Name() + telemetry.Setup(c.Context(), telemetry.Options{ + Enabled: globalImpl.OtelTracing(), + Version: version.Version(), + Logger: logger, + }) + telemetry.StartCommandSpan(cmdName, commandSpanAttributes(cmdName, globalImpl)...) + return nil }, } @@ -121,6 +136,20 @@ func NewRootCmd(globalConfig *config.GlobalOptions) (*cobra.Command, error) { return cmd, nil } +// commandSpanAttributes builds the root-span attributes from the resolved +// global options. Values that can be overridden per release appear again on +// child spans; the service identity (service.name/service.version) lives on +// the OTel resource, not on spans. +func commandSpanAttributes(cmdName string, g *config.GlobalImpl) []attribute.KeyValue { + return []attribute.KeyValue{ + attribute.String("helmfile.command", cmdName), + // FileOrDir may be a remote go-getter reference with credentials. + attribute.String("helmfile.file", helmexec.RedactedRef(g.FileOrDir())), + attribute.String("helmfile.environment", g.Env()), + attribute.StringSlice("helmfile.selectors", g.Selectors()), + } +} + func setGlobalOptionsForRootCmd(fs *pflag.FlagSet, globalOptions *config.GlobalOptions) { fs.StringVarP(&globalOptions.HelmBinary, "helm-binary", "b", "", fmt.Sprintf(`Path to the helm binary. Overrides "HELMFILE_HELM_BINARY" OS environment variable when specified (default %q)`, app.DefaultHelmBinary)) fs.StringVarP(&globalOptions.KustomizeBinary, "kustomize-binary", "k", "", fmt.Sprintf(`Path to the kustomize binary. Overrides "HELMFILE_KUSTOMIZE_BINARY" OS environment variable when specified (default %q)`, app.DefaultKustomizeBinary)) @@ -164,6 +193,9 @@ It only applies for the Helm CLI commands, Stdout/Stderr for Hooks are still dis Useful when file order matters for dependencies (e.g., databases before applications). When processing multiple files, paths are resolved without changing the process working directory, so relative environment variables like KUBECONFIG work correctly.`) + fs.BoolVar(&globalOptions.OtelTracing, "otel-tracing", globalOptions.OtelTracing, `Enable OpenTelemetry tracing and metrics (experimental). +Configure exporters with standard OTEL_* environment variables (e.g. OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_TRACES_EXPORTER, OTEL_METRICS_EXPORTER). +Overrides "HELMFILE_OTEL_TRACING" OS environment variable when specified. See docs/otel.md`) // avoid 'pflag: help requested' error (#251) fs.BoolP("help", "h", false, "help for helmfile") } diff --git a/cmd/root_test.go b/cmd/root_test.go index 7b34d1ee..ff4af7b4 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/helmfile/helmfile/pkg/config" "github.com/helmfile/helmfile/pkg/errors" @@ -70,3 +71,14 @@ func TestToCLIError(t *testing.T) { }) } } + +func TestRootCmdRegistersOtelTracingFlag(t *testing.T) { + rootCmd, err := NewRootCmd(&config.GlobalOptions{}) + require.NoError(t, err) + + flag := rootCmd.PersistentFlags().Lookup("otel-tracing") + require.NotNil(t, flag, "--otel-tracing flag should be registered") + assert.Equal(t, "bool", flag.Value.Type()) + assert.Equal(t, "false", flag.DefValue) + assert.Contains(t, flag.Usage, "HELMFILE_OTEL_TRACING") +} diff --git a/docs/cli.md b/docs/cli.md index 90f40015..427afcc6 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -54,6 +54,9 @@ Flags: --log-level string Set log level. Overrides "HELMFILE_LOG_LEVEL" OS environment variable when specified (default "info") -n, --namespace string Set namespace. Overrides "HELMFILE_NAMESPACE" OS environment variable when specified. Uses the namespace set in the context by default, and is available in templates as {{ .Namespace }} --no-color Output without color. Overrides "HELMFILE_NO_COLOR" and "NO_COLOR" OS environment variables when specified + --otel-tracing Enable OpenTelemetry tracing (experimental). + Configure the exporter with standard OTEL_* environment variables (e.g. OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_TRACES_EXPORTER). + Overrides "HELMFILE_OTEL_TRACING" OS environment variable when specified. See docs/otel.md -q, --quiet Silence output. Equivalent to log-level warn. Overrides "HELMFILE_QUIET" OS environment variable when specified --repo-retries int Number of times to retry "helm repo add/update" and "helm registry login" on failure, with exponential backoff (1s, 2s, 4s, ..., capped at 30s). Set to 0 to disable retries. Overrides "HELMFILE_REPO_RETRIES" OS environment variable when specified -l, --selector stringArray Only run using the releases that match labels. Labels can take the form of foo=bar or foo!=bar. diff --git a/docs/experimental-features.md b/docs/experimental-features.md index 6cbb3e1b..61a41d4e 100644 --- a/docs/experimental-features.md +++ b/docs/experimental-features.md @@ -28,3 +28,17 @@ See [Selectors and needs](releases.md#selectors) for detailed examples. ## HCL helmfile-values-file support HCL language is supported for environment values files (`.hcl` suffix). This was introduced as experimental in PR #1423 and is now a stable feature. See [Environments](environments.md#hcl-specifications) for details. + +## otel-tracing + +OpenTelemetry tracing support: export a trace of a helmfile run to any OTLP-compatible backend. The current increments emit the command-level root span, state-loading spans (discover/load/render/parse), one span per external process (helm invocations, hooks, plugin execs), per-hook spans, per-release spans for the sync/diff/delete/status/test/prepare paths, and metrics (`helmfile.helm.exec.duration`, `helmfile.release.duration`, `helmfile.release.count`; per-release dimensions opt-in via `HELMFILE_OTEL_METRICS_PER_RELEASE`) (the remaining loops are being added incrementally) — see [the design proposal](https://github.com/helmfile/helmfile/blob/main/docs/proposals/otel-tracing.md). + +Unlike the features above, `otel-tracing` is **not** gated by `HELMFILE_EXPERIMENTAL`. It is off by default and enabled explicitly per run: + +```bash +helmfile --otel-tracing -e production apply +# or +export HELMFILE_OTEL_TRACING=true +``` + +All exporter, sampler, and propagator settings come from the standard `OTEL_*` environment variables (e.g. `OTEL_EXPORTER_OTLP_ENDPOINT`, `OTEL_TRACES_EXPORTER`). See [OpenTelemetry Tracing](otel.md) for details. diff --git a/docs/index.md b/docs/index.md index dc47d7d4..3e0d83ca 100644 --- a/docs/index.md +++ b/docs/index.md @@ -51,6 +51,8 @@ To avoid upgrades for each iteration of `helm`, the `helmfile` executable delega **Patch**: JSON/Strategic-Merge Patch Kubernetes resources before `helm-install`ing, without forking upstream charts (See [#673](https://github.com/roboll/helmfile/pull/673)) +**Observability**: Opt-in [OpenTelemetry traces and metrics](otel.md) for CI/CD runs — see where time goes, per release and per helm invocation (experimental) + ## Installation * download one of [releases](https://github.com/helmfile/helmfile/releases) diff --git a/docs/otel.md b/docs/otel.md new file mode 100644 index 00000000..2fd3b210 --- /dev/null +++ b/docs/otel.md @@ -0,0 +1,110 @@ +# OpenTelemetry Tracing + +Helmfile can export [OpenTelemetry](https://opentelemetry.io/) traces of a run, which is especially useful in CI/CD: see where time is spent, which helm invocations dominate, how label selectors fan out, and what to target for improvement. + +Tracing is **experimental** and **off by default**. It currently emits the command-level root span, state-loading spans (discover/load/render/parse), one span per external process (helm invocations, hooks, plugin execs), per-hook spans, and per-release spans for the sync/diff/delete/status/test/prepare paths. + +## Enabling + +```bash +helmfile --otel-tracing -e production -l tier=backend apply +``` + +or via environment variable (handy for CI): + +```bash +export HELMFILE_OTEL_TRACING=true +helmfile sync +``` + +`--otel-tracing` overrides `HELMFILE_OTEL_TRACING` when specified. + +## Configuration + +Everything beyond the on/off switch uses the standard [OpenTelemetry environment variables](https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/); helmfile defines no telemetry-specific variables of its own. + +| Variable | Default | Notes | +|---|---|---| +| `OTEL_EXPORTER_OTLP_ENDPOINT` | `http://localhost:4318` | OTLP endpoint; use `http://host:4317` for gRPC | +| `OTEL_EXPORTER_OTLP_PROTOCOL` | `http/protobuf` | or `grpc` | +| `OTEL_EXPORTER_OTLP_HEADERS` | — | e.g. `authorization=Bearer ` | +| `OTEL_TRACES_EXPORTER` | `otlp` | `otlp` \| `console` \| `none` | +| `OTEL_METRICS_EXPORTER` | `otlp` | `otlp` \| `console` \| `prometheus` \| `none` — see [Metrics](#metrics) | +| `OTEL_TRACES_SAMPLER` (+ `OTEL_TRACES_SAMPLER_ARG`) | `parentbased_always_on` | | +| `OTEL_SERVICE_NAME` | `helmfile` | | +| `OTEL_RESOURCE_ATTRIBUTES` | — | e.g. `cicd.pipeline=deploy,cicd.run_id=4821` | +| `OTEL_PROPAGATORS` | `tracecontext,baggage` | | +| `OTEL_SDK_DISABLED` | `false` | standard kill switch | + +HTTP proxies (`HTTPS_PROXY`/`HTTP_PROXY`) are honored automatically by the Go runtime. + +## Quick start without a collector + +To inspect what would be exported, print spans as JSON on stdout: + +```bash +OTEL_TRACES_EXPORTER=console helmfile --otel-tracing -l name=myrelease template +``` + +## Quick start with Jaeger + +```bash +docker run --rm -p 16686:16686 -p 4318:4318 jaegertracing/all-in-one:latest + +OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4318 \ + helmfile --otel-tracing -e production apply +# Open http://localhost:16686 and pick service "helmfile" +``` + +## What gets traced + +The root span is named after the command (`helmfile apply`, `helmfile sync`, ...) and carries `helmfile.command`, `helmfile.file`, `helmfile.environment`, and `helmfile.selectors` attributes, plus `helmfile.exit_code` and the error (if any) at the end. + +Every external process helmfile starts — each helm invocation, hook command, and plugin exec — gets its own span nested under the command span: `helm.exec` (with `helm.subcommand`) for helm binaries, `os.exec` otherwise, both carrying `exec.command`, redacted `exec.args`, and `exec.exit_code` on failure. + +State loading is traced too: `helmfile.discover_states`, one `helmfile.load` per state file (including nested helmfiles), with `helmfile.render` and `helmfile.parse` children — rendering is frequently the hidden time sink. Each hook execution produces a `helmfile.hook` span (with `hook.event` and `hook.name`) that its subprocess span nests under — and release-scoped hooks (presync/postsync & co. within sync/diff/delete) nest under their release's span, keeping full release attribution. + +Per-release spans (`helmfile.release.sync` / `.diff` / `.delete` / `.status` / `.test` / `.prepare`) carry the release name, namespace, chart, and labels, and nest under the state-file load span — with the release's helm subprocesses (upgrade, diff, delete, status, test) nested under the release span. The remaining loops (flag preparation, template/lint/unittest) are being added incrementally; see [the design proposal](proposals/otel-tracing.md) for the full taxonomy. + +Secret-bearing command arguments (`--set`, `--set-file`, `--username`, `--password`, ...) are never recorded in span attributes — they are masked before export. + +## Metrics + +Tracing and metrics share the same switch and resource; metrics are exported through the standard metrics environment variables: + +| Variable | Default | Notes | +|---|---|---| +| `OTEL_METRICS_EXPORTER` | `otlp` | `otlp` \| `console` \| `prometheus` \| `none` | +| `OTEL_METRIC_EXPORT_INTERVAL` | `60000` (ms) | periodic export interval; the final flush happens at exit | + +Three instruments are emitted: + +| Metric | Type | Attributes | +|---|---|---| +| `helmfile.helm.exec.duration` | histogram (seconds, buckets tuned for 5ms–600s helm invocations) | `subcommand`, `success` | +| `helmfile.release.duration` | histogram (seconds, same buckets) | `verb`, `result` — plus `helmfile.release` and `helmfile.namespace` when per-release metrics are enabled (below) | +| `helmfile.release.count` | counter (unit `{release}`) | `verb` (sync/diff/delete/status/test/prepare), `result` (success/error) | + +### Per-release metrics (opt-in, high cardinality) + +By default metric dimensions are bounded enumerations — release names never appear, so time-series count stays constant regardless of fleet size. If you need per-release durations on dashboards, opt in: + +```bash +HELMFILE_OTEL_METRICS_PER_RELEASE=true helmfile --otel-tracing sync +``` + +This adds `helmfile.release` and `helmfile.namespace` to `helmfile.release.duration`. Time-series count then scales with your release fleet: well-suited to bounded CI runs, but long-lived centralized collection needs a backend capacity/TTL story. (Per-release timing is always available in traces, without this flag.) + +Both dimensions are bounded enumerations — never release names — so metric cardinality stays constant regardless of fleet size. The instrumentation scope (`helmfile`) carries the helmfile version. + +Inspect them without a collector with `OTEL_METRICS_EXPORTER=console`. + +## CI trace correlation + +If your CI system injects a W3C `TRACEPARENT` environment variable into jobs, helmfile joins that trace automatically, so helmfile spans appear inside your pipeline's trace. + +## Troubleshooting + +- **No spans arrive**: check `OTEL_EXPORTER_OTLP_ENDPOINT`/protocol match your collector (4318 for `http/protobuf`, 4317 for `grpc`); verify with `OTEL_TRACES_EXPORTER=console`. +- **Tracing breaks nothing**: exporter/sampler misconfiguration and export failures never fail a helmfile run; at worst you get no spans. +- **Still stuck?** Set `OTEL_SDK_DISABLED=false` explicitly and rerun with `--log-level debug`. diff --git a/docs/proposals/otel-tracing.md b/docs/proposals/otel-tracing.md new file mode 100644 index 00000000..24c7bfc0 --- /dev/null +++ b/docs/proposals/otel-tracing.md @@ -0,0 +1,555 @@ +# Proposal: OpenTelemetry Tracing Support + +- **Issue**: [#2767 — feat-request: OpenTelemetry (tracing) support](https://github.com/helmfile/helmfile/issues/2767) +- **Status**: Draft +- **Origin discussion**: [#2758](https://github.com/helmfile/helmfile/discussions/2758) + +## 1. Summary + +Add opt-in OpenTelemetry (OTel) distributed tracing to helmfile. When enabled, a helmfile +run produces a trace whose spans cover the full execution timeline — state-file discovery, +template rendering, per-release operations, hooks, and every helm subprocess helmfile itself +starts — exported via OTLP to any OTel-compatible backend (Jaeger, Tempo, Zipkin, +Honeycomb, Datadog, Grafana Cloud, cloud-vendor collectors, ...). + +This directly serves the motivating use case from CI/CD: *see where time is spent, what runs +most often (label selectors), and what to target for improvement* — the same capability +Terragrunt ships today ([Terragrunt OTel docs](https://docs.terragrunt.com/troubleshooting/open-telemetry/)). + +When disabled (the default), behavior and performance are identical to today: a no-op tracer +provider, no goroutines, no network, no exported spans (explicit guarantees in §7). + +### Example resulting trace + +``` +helmfile sync file=helmfile.yaml env=production (root, ~2m) +├─ helmfile.discover_states (~5ms) +├─ helmfile.load file=helmfile.yaml (~800ms) +│ ├─ helmfile.render pass=values (~400ms) +│ └─ helmfile.parse (~50ms) +├─ helmfile.repos.update (~4s) +│ └─ helm.exec subcommand="repo update" (~4s) +├─ helmfile.release.prepare release=gateway chart=gateway (~9s) +│ └─ helm.exec subcommand="dependency build" (~8s) +├─ helmfile.release.sync release=envoy ns=ingress (~21s) +│ ├─ helmfile.hook event=presync (~2s) +│ │ └─ os.exec command="./migrate.sh" (~2s) +│ ├─ helm.exec subcommand="upgrade --install envoy ..." (~18s) +│ └─ helmfile.hook event=postsync (~1s) +├─ helmfile.release.sync release=api ns=apps (~35s) +└─ helmfile.wait release=api (kubedog) (~30s) +``` + +## 2. Goals and non-goals + +### Goals + +1. **Opt-in, zero-cost-when-off**: tracing disabled by default; no measurable overhead or + behavior change when not requested. +2. **Standard OTLP export** configured through the [OTel environment-variable + specification](https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/) + — no helmfile-specific duplicate knobs for endpoints/protocols/headers/sampling. +3. **Meaningful hierarchy**: spans nest (command → state file → release → hook/subprocess) + so the critical path of a sync/apply is visible at a glance — *including* the kubedog and + hook paths, which today run on detached contexts (§4.3). +4. **Safe by default**: span attributes never contain secret-bearing values + (`--set` values, registry credentials, secret refs). +5. **CI-friendly**: W3C `tracecontext` propagation so a CI system that injects + `TRACEPARENT` gets spans correlated into its own trace. + +### Non-goals (for the initial implementation) + +- OTel **metrics** and **log export** (the SDK setup leaves room; see §12 for follow-ups). +- Tracing *inside* the helm binary or cluster-side (helm/chart hooks are separate processes; + context injection into them is an open question, §14). +- Fixing the pre-existing cancellation gaps discovered during design (kubedog path rooted at + `context.Background()`, hooks at `context.TODO()` — §4.3). This proposal bridges them for + *trace context only*, with byte-identical cancellation semantics; semantic fixes are + reported separately. +- Any helmfile-config-file surface (`helmfile.yaml`) for telemetry — CLI flags + env vars only, + matching how observability tooling is usually injected by the platform, not the state author. + +## 3. User-facing design + +### 3.1 Activation + +| Mechanism | Default | Description | +|---|---|---| +| `--otel-tracing` (persistent flag) | `false` | Enable tracing for this run | +| `HELMFILE_OTEL_TRACING=true` (env) | `false` | Same, via environment (CI-friendly) | + +The feature ships as **experimental** initially and is listed in +`docs/experimental-features.md` (see §11 Rollout). We do *not* require +`HELMFILE_EXPERIMENTAL=otel-tracing`: tracing is purely additive, invisible when off, and +gating it twice would complicate CI adoption. The experimental label sets stability +expectations only. (Note: this differs from the current entries in that document, which are +gated by `HELMFILE_EXPERIMENTAL`; the entry will say so explicitly.) + +### 3.2 Exporter & SDK configuration — standard OTel env vars only + +When `--otel-tracing` is set, helmfile initializes the OTel SDK honoring the standard +environment variables. Exporter selection is delegated to +`go.opentelemetry.io/contrib/exporters/autoexport` (already in the module graph, §8) rather +than hand-rolled, so helmfile maintains no exporter-construction code of its own: + +| Variable | Default in helmfile | Read by | Notes | +|---|---|---|---| +| `OTEL_EXPORTER_OTLP_ENDPOINT` | `http://localhost:4318` | otlp client (SDK) | default follows the protocol: 4318 with `http/protobuf` (below), 4317 with `grpc` | +| `OTEL_EXPORTER_OTLP_PROTOCOL` / `..._TRACES_PROTOCOL` | `http/protobuf` | autoexport | `grpc`, `http/protobuf` (traces-specific var wins; `http/json` is not supported by autoexport traces in v0.67.0) | +| `OTEL_EXPORTER_OTLP_HEADERS` / `..._TRACES_HEADERS` | — | otlp client (SDK) | e.g. collector auth tokens | +| `OTEL_EXPORTER_OTLP_TIMEOUT` / `..._TRACES_TIMEOUT` | `10s` | otlp client (SDK) | per-export timeout | +| `OTEL_EXPORTER_OTLP_INSECURE` | per endpoint scheme | otlp client (SDK) | plaintext export for local collectors | +| `OTEL_TRACES_EXPORTER` | `otlp` | autoexport | `otlp` \| `console` \| `none` (verified against autoexport v0.67.0; more values possible via `RegisterSpanExporter`) | +| `OTEL_TRACES_SAMPLER` (+`..._ARG`) | `parentbased_always_on` | our wrapper | standard samplers (the Go SDK core does not parse this env itself) | +| `OTEL_SERVICE_NAME` | `helmfile` | SDK resource | service identity (`resource` `WithFromEnv`; verified in sdk v1.44.0 `resource/env.go`) | +| `OTEL_RESOURCE_ATTRIBUTES` | — | SDK resource | e.g. `deployment.environment=ci,cicd.pipeline=release` (same verified reader) | +| `OTEL_PROPAGATORS` | `tracecontext,baggage` | our wrapper | extract parent from CI-injected `TRACEPARENT` (SDK core does not parse this env either) | +| `OTEL_METRICS_EXPORTER` | `otlp` | autoexport (metrics) | `otlp` \| `console` \| `prometheus` \| `none` — delivered in PR 4 | +| `OTEL_METRIC_EXPORT_INTERVAL` | `60000` (ms) | SDK metric reader | periodic export interval; final flush happens at exit | +| `OTEL_SDK_DISABLED` | `false` | our wrapper | standard kill switch (the Go SDK does not read this one itself) | + +`console` (JSON spans on **stdout**, via `stdouttrace`) exists for local debugging without a +collector — note stdout, so it does not interleave with helmfile's stderr logs; +`none` produces no export (used by tests and for pure-propagation setups). + +HTTP proxies (`HTTPS_PROXY`/`HTTP_PROXY`) are honored automatically by the Go HTTP/gRPC +stacks — no helmfile-specific proxy configuration. + +Minimal CI example: + +```bash +export OTEL_EXPORTER_OTLP_ENDPOINT="https://otel.example.com" +export OTEL_EXPORTER_OTLP_HEADERS="authorization=Bearer ${OTEL_TOKEN}" +export OTEL_SERVICE_NAME="helmfile-ci" +export OTEL_RESOURCE_ATTRIBUTES="cicd.pipeline=deploy,cicd.run_id=4821" +helmfile --otel-tracing -e production -l tier=backend apply +``` + +## 4. Architecture + +### 4.1 New package `pkg/telemetry` + +Single, dependency-light façade over the OTel SDK. No other package imports OTel SDK +exporters directly. + +``` +pkg/telemetry/ +├── telemetry.go // Setup/Shutdown lifecycle, CommandContext(), Tracer(name) +├── exporter.go // thin autoexport wiring + propagators (no exporter construction) +└── telemetry_test.go +``` + +Public surface sketch: + +```go +package telemetry + +type Options struct { + Enabled bool + Version string // helmfile version, recorded as service.version + Logger *zap.SugaredLogger // for one-line diagnostics, never span data +} + +// Setup initializes the global provider. Idempotent; a no-op when +// opts.Enabled is false. Exporter/sampler misconfiguration and +// OTEL_SDK_DISABLED=true degrade to disabled with a warning — telemetry +// problems never fail a helmfile run. +func Setup(ctx context.Context, opts Options) + +// StartCommandSpan starts the root span for one command invocation, joins a +// remote parent from TRACEPARENT/TRACESTATE/BAGGAGE env vars, and makes its +// context the one returned by CommandContext. No-op when disabled. +func StartCommandSpan(command string, attrs ...attribute.KeyValue) + +// CommandContext returns the root command span's context, or +// context.Background() when telemetry is disabled. Single source of truth +// for deriving App.ctx. +func CommandContext() context.Context + +// Tracer returns a tracer for the given instrumentation scope. Never nil: +// returns the OTel no-op tracer when telemetry is disabled, so callers need +// no `if enabled` branches. Sanctioned scopes: ScopeHelmfile, ScopeHelm. +func Tracer(name string) trace.Tracer + +// Shutdown ends the command span (recording runErr and exitCode), flushes +// buffered spans bounded by ctx, and reverts to disabled. Idempotent and +// nil-safe (safe before Setup, twice, or on a signal that raced Setup). +// ShutdownTimeout is the recommended flush bound. +func Shutdown(ctx context.Context, runErr error, exitCode int) error +``` + +Design constraints for maintainability: + +- **No domain knowledge in this package.** Span-attribute redaction rules live with the + existing redaction code in `pkg/helmexec` (§6); telemetry only consumes the result. + A telemetry package that knows about `--set` semantics would be a layering violation. +- Sanctioned instrumentation scopes are only `"helmfile"` (app/state layer) and `"helm"` + (helmexec layer), documented in `telemetry.go`. + +### 4.2 Lifecycle + +``` +main.go cmd/root.go +────── ─────────── +rootCmd.Execute() ─────────────► PersistentPreRunE: + logger setup (existing) + telemetry.Setup(ctx, opts) ← provider + exporters + start root span "helmfile " + RunE / subcommand execution … (child spans attach via context) +errChan <- Execute() + ┌────────────────────────────────┐ +shutdown: │ end root span (status=error on │ + rootSpan.End() │ failure, exit code attr) │ + provider.Shutdown(ctx 5s) ◄─────┘ │ +``` + +- `PersistentPreRunE` (`cmd/root.go`) is the natural init point: it already centralizes + logger construction from `GlobalOptions`, and receives the `*cobra.Command`, so + `c.Name()` gives the root span name (`helmfile sync`, ...). +- **`App.ctx` derivation requires no signature changes.** `app.New(conf)` is called from + every subcommand (`cmd/*.go`, 22 call sites) and currently roots its context at + `context.Background()` (`pkg/app/app.go`). Instead of threading a parameter through all + callers, `app.New` replaces that single `context.Background()` with + `telemetry.CommandContext()` — Background-identical when tracing is off, span-rooted + when on. Cancellation is unaffected (`context.WithCancel` on either parent behaves the + same for SIGINT handling in `main.go`). +- **Shutdown must run even on failure and on signals.** The end of + `rootCmd.Execute()` in `main.go` (both the `errChan` and the `SIGINT`/`SIGTERM` paths; + on the error path *before* `errors.HandleExitCoder`, which terminates via `OsExiter`, + and on the signal path after `app.CleanWaitGroup.Wait()` and *before* + `os.Exit(130/143)`) calls the returned shutdown func with a 5s-timeout context so buffered + spans are flushed. The stored shutdown must be nil-safe — a signal arriving before + `PersistentPreRunE` completed (i.e. before `Setup`) must be a no-op, never a panic. This + is the one behavior that is easy to get wrong and is explicitly tested (§10). +- Setup failures (e.g. unusable exporter configuration) are logged as a warning and + **do not fail the run** — telemetry must never break deployments. Export errors after + startup surface only through OTel's own error handler, wired to the zap logger. + +### 4.3 Verified context-reality map (as of this writing) + +All claims below were checked against the code; line numbers are anchors for reviewers: + +| Path | Today | Consequence for tracing | +|---|---|---| +| cmd → app | `app.New` roots at `context.Background()`; `ctx, Cancel = WithCancel(ctx)` (`pkg/app/app.go`, `New`) | span must be injected here (§4.2) | +| app → all helm execs | `getHelm()` constructs the `ShellRunner` with `Ctx: a.ctx` (`pkg/app/app.go:1008`); the resulting `execer` is **cached per (helm binary, kube-context)** in `a.helms` and shared by all releases and workers (`pkg/app/app.go:982–1021`) | once `App.ctx` is span-rooted, every non-kubedog helm call nests automatically, with **zero changes** to `getHelm`. The shared-instance cache is also why per-release contexts must ride per-call parameters, never mutation of the shared execer | +| kubedog path (sync with tracking) | `startBackgroundKubedogTracking(gocontext.Background(), …)` (`pkg/state/state.go:1294`) → `bufferHelmOutput` derives `releaseCtx := context.WithCancel(ctx)` and swaps it in via `execer.WithContext(releaseCtx)` (`pkg/state/helmx.go:363–365`) | helm execs on this path run on a **Background-rooted** context; runner-level spans would become **orphan traces**. Bridged in §4.4 | +| hooks | both `event.Bus` constructions (`triggerGlobalReleaseEvent`, `triggerReleaseEvent`, `pkg/state/state.go:3666, 3703`) duplicate the same literal and pass **no** `Runner`, so the default kicks in: `ShellRunner{Dir: bus.BasePath, Logger: bus.Logger, Ctx: goContext.TODO()}` with an inline comment acknowledging it should be `app.Ctx` (`pkg/event/bus.go:61–71`) | hook execs are detached; spans would be orphans. Bridged in §4.4 | +| non-kubedog release workers | release loops (`SyncReleases` etc., `pkg/state/state.go:1212 ff.`) call the shared `helmexec.Interface` with a `HelmContext` (`pkg/helmexec/context.go`) that carries **no go-context** | per-release spans need the §4.4 mechanism | +| subprocess funnel | exactly three `ShellRunner` construction sites exist (verified exhaustive): `pkg/app/app.go:129` (`Init`, `Ctx: a.ctx`), `pkg/app/app.go:1006` (`getHelm`, `Ctx: a.ctx`), and the hooks default (`pkg/event/bus.go:62`, `Ctx: TODO` — §4.4 bridge). Every external process helmfile itself starts goes through `Execute`/`ExecuteStdIn` (`pkg/helmexec/runner.go`); helm commands additionally funnel through `execer.exec()` (`pkg/helmexec/exec.go:1207`). Exception: kustomize executes inside the chartify library, outside this funnel (§12) | one instrumentation point covers everything except chartify-internal execs; spans nest wherever the runner's `Ctx` carries a span | + +Two pre-existing gaps surfaced by this analysis — kubedog tracking not being cancellable via +`App.ctx`, and hooks likewise — are **out of scope** for this proposal beyond trace-context +bridging (§4.4), because fixing their *cancellation* semantics would be a behavior change. +They should be reported as separate issues. + +### 4.4 Context plan + +**Phase 1 (no state-package changes):** + +1. Root span ctx via `telemetry.CommandContext()` → `app.New` (§4.2). Every `a.ctx`-rooted + exec nests. Discover/load/render spans in `pkg/app` also need no signature changes: + the `helmfile.load` span starts inside `loadDesiredStateFromYamlWithBaseDir` + (`pkg/app/app.go:932`) with `a.ctx` as parent — its two call sites (app.go:1045 and + the nested-helmfile path at app.go:1280) are thereby both covered — and `helmfile.render` + children parent through an unexported `ctx` field on the `desiredStateLoader` struct + (`pkg/app/desired_state_file_loader.go:29`), set once at its single construction site + (`pkg/app/app.go:938`). Zero method-signature changes, zero exported API. +2. Runner-level spans in `ShellRunner.Execute`/`ExecuteStdIn` nest for all non-kubedog, + non-hook execs automatically. +3. **Orphan-bridge for kubedog and hooks, with identical cancellation semantics:** + - `pkg/state/state.go:1294`: pass `context.WithoutCancel(telemetry.CommandContext())` + instead of `gocontext.Background()`. `WithoutCancel` preserves values (the span) + while dropping cancellation — and `Background` never carried cancellation anyway, so + SIGINT/timeout behavior is **bit-for-bit unchanged**; only trace context is added. + (Phase 1 uses the root span from `telemetry.CommandContext()`, which requires no new + plumbing in `pkg/state`; phase 2 re-parents under the per-release `st.traceCtx`.) + - `pkg/event/bus.go`: add an optional `Ctx context.Context` field to `Bus`; the default + runner construction uses `bus.Ctx` when set, `TODO` when nil (so behavior is unchanged + for any nil-Ctx caller). The two construction sites in `pkg/state/state.go:3666, 3703` + set it from the same span-rooted, cancel-stripped context. `Dir`/`Logger` wiring of + the default runner is untouched. + +**Phase 2 (per-release spans, still zero changes to `helmexec.Interface` signatures):** + +- Start `helmfile.release.` spans in the seven release-worker loops — six + `scatterGather` sites in `pkg/state/state.go` (`prepareSyncReleases`:894, + `DeleteReleasesForSync`:1135, `SyncReleases`:1241, `PrepareCharts`:2328, + `prepareDiffReleases`:3068, `DiffReleases`:3266) plus `iterateOnReleases` + (`pkg/state/state_run.go:59`, the shared loop behind test/lint/unittest-style + iteration) — all following the same `scatterGather` shape. (An eighth `scatterGather` + sites, `scatterGatherEnvSecretFiles` at `pkg/state/create.go:486`, decrypts environment + secrets rather than processing releases; an optional `helmfile.env_secrets` span there + is a follow-up in the spirit of §4.5 item 6.) +- Release spans are rooted via a new unexported `traceCtx` field on `HelmState`, exposed + through a purely additive exported setter called by `pkg/app` right after state + creation. (Why a setter and not a constructor parameter: `st.logger` is injected through + `state.NewCreator` — a 9-positional-parameter exported function, + `pkg/state/create.go:80` — so threading a context through it would churn a public + signature used by the app loader; an additive setter touches no existing signature and + defaults to nil, i.e. current behavior.) +- Carry the span context per call by adding an optional `Ctx context.Context` field to + `HelmContext` (`pkg/helmexec/context.go`), stamped **inside** `createHelmContext` + (`pkg/state/state.go:3168`) so all eight call sites (state.go:1007, 1026, 1147, 1261, + 3034, 3184, 3358, 3374) inherit it without per-call-site edits. +- Funnel it inside `helmexec`: the `execer` methods that take a `HelmContext` pass its + `Ctx` to `execWithContext`, which **injects the release span into the runner's own + context** (`trace.ContextWithSpan`) rather than replacing it — cancellation authority + stays with the runner (including the kubedog safety valve installed via + `WithContext`), which a naive context replacement would have overridden. A nil `Ctx` + behaves exactly like the plain funnel. Helm-vs-other classification comes from an + explicit marker stamped in the execer funnels (`withRunnerCtx`/`markHelmRunner`, + value and pointer runner forms), not from the executable basename — so wrapper + `--helm-binary` names classify correctly. The `exectest.Helm` fake is unaffected: app + tests pre-seed `App.helms` with it (`pkg/app/app_template_test.go:115–116`), so it + replaces the whole `helmexec.Interface` and bypasses `execer` internals entirely. + *Why per-call threading rather than the existing `WithContext` clone: `WithContext` + (`pkg/helmexec/exec.go:251`) is suited to whole-execution substitution (kubedog path), + but release workers share one cached execer across concurrent workers (§4.3), so a + per-release context must travel with the per-release `HelmContext` parameter.* + +### 4.5 Instrumentation points (in priority order) + +1. **`helmexec.ShellRunner.Execute` / `ExecuteStdIn`** (`pkg/helmexec/runner.go`) — the single + choke point for *every* external process started by helmfile itself: helm invocations, + hooks, and helmfile plugin execs. One span per subprocess: name `helm.exec` when `cmd` + is the helm binary, else `os.exec`. The one exception is kustomize, which runs inside + the `github.com/helmfile/chartify` library (see §5/§12). This instrumentation alone + delivers most of the requested value (where does time go). +2. **Root command span** — `cmd/root.go` (see §4.2). +3. **State loading** — span `helmfile.load` started inside + `loadDesiredStateFromYamlWithBaseDir` (`pkg/app/app.go:932`, covering both callers + incl. nested helmfiles), with `helmfile.render`/`helmfile.parse` children in + `two_pass_renderer.go` parented via the loader-struct `ctx` field (§4.4 step 1) — + rendering is frequently the hidden time sink. +4. **Per-release operations** — release loops in `pkg/state/state.go` (phase 2, §4.4). +5. **Hooks** — `pkg/event/bus.go` `Trigger` (`pkg/event/bus.go:56`): span `helmfile.hook` + with `hook.event`, `hook.name` attributes; naturally parents the `os.exec` span of the + hook command once the §4.4 bridge is in place. +6. **(Phase 2)** kubedog wait spans, vals/remote secret resolution, `helm repo` retry loops. + +## 5. Span taxonomy + +| Span name | Attributes (beyond standard `otel.*`) | Notes | +|---|---|---| +| `helmfile ` (root) | `helmfile.command`, `helmfile.file`, `helmfile.environment`, `helmfile.selectors`, `helmfile.exit_code` | `error` status + recorded error on failure. Service identity (`service.name`, `service.version`) lives on the OTel resource, not on spans | +| `helmfile.discover_states` | `helmfile.path` | `findDesiredStateFiles` (`pkg/app/app.go:1642`) | +| `helmfile.load` | `helmfile.state_file` | one per file in `helmfile.d`, nested helmfiles | +| `helmfile.render` | `helmfile.state_file`, `helmfile.pass`=`values`\|`main` | two-pass rendering (`pkg/app/two_pass_renderer.go`) | +| `helmfile.repos.update` | — | wraps `helm repo update` | +| `helmfile.release.prepare` | `helmfile.release`, `helmfile.namespace`, `helmfile.chart`, `helmfile.chart_version` | chart pull/build/registry login | +| `helmfile.release.sync` / `.diff` / `.template` / `.delete` / `.test` / `.lint` / `.unittest` | same as above + `helmfile.labels` | one per selected release (phase 2) | +| `helmfile.hook` | `hook.event` (presync/…), `hook.name` | | +| `helm.exec` / `os.exec` | `exec.command`, `exec.args` (strict-redacted, plus URL userinfo/query masking via `RedactedRef`), `exec.redacted`, `exec.exit_code` (on failure), `helm.subcommand` (helm only) | one per external process; helm classification is marker-based (wrapper binaries included); release identity comes from the parent release span. Hooks, kustomize (invisible, §12), plugin execs land here too | +Attribute values are strings/ints only; no structured payloads, no output capture in spans +(output already flows through logs). + +## 6. Security and redaction + +Traces leave the machine they run on. Ground rules, checked against what exists today: + +1. **The existing redaction is not sufficient on its own.** The exit-error path + (`pkg/helmexec/exit_error.go:8–20`) redacts only the argument *following* a flag whose + name starts with `--set` (two-argument form). It does not cover the single-argument + `--set=key=value` form, nor credential-bearing flags such as `--username` or + `--password`. (Registry passwords themselves already travel via stdin — + `--password-stdin`, `pkg/helmexec/exec.go` `RegistryLogin` — but usernames appear in + args.) Therefore: +2. **One shared redaction implementation, two profiles — spans get the strict superset + without touching existing error output.** Extract the exit-error redaction into an + exported helper in `pkg/helmexec` (keeping it in the domain that owns flag semantics) + with two profiles: + - `legacy`: byte-identical to today's exit-error behavior — the current goldens in + `pkg/helmexec/exit_error_test.go` (which assert the exact `--set` / `*** STRIP ***` + shape) keep passing unchanged. The exit-error path switches to this profile, so + error messages are unchanged. + - `strict`: the superset required for spans — all `--set*` forms including + `--set=k=v`, plus `--username`, `--password`, `--key-file`, + `--kube-token`, ...; positional arguments are additionally passed through + `helmexec.RedactedRef`, which masks go-getter forced forms (`git::`, + `s3::`), whole URL userinfo, and credential-bearing query parameters, + failing closed for malformed references. The previous token is always + read from the original input so adjacent secret flags cannot leak. + Both profiles are the same code path, so the span view is guaranteed at least as + redacted as the error view. *Unifying* the two profiles (i.e. tightening exit-error + messages too) would change observable output and is deliberately deferred to a + separate follow-up PR with its own test updates — this proposal changes no existing + message content. +3. **Never record**: `vals://`-resolved values, environment variables, exporter headers. + `OTEL_EXPORTER_OTLP_HEADERS` is the only place collector credentials live; helmfile + never logs it. +4. Chart/repo URLs go through the existing `redactedURL` (`pkg/helmexec/exec.go:184`) — + credentials embedded in URLs are masked. +5. Release *names*, namespaces, chart names, and label selectors are assumed non-secret + (consistent with existing helmfile log output). +6. Note for reviewers: `execer.exec` today logs the *full unredacted* command line at Debug + level (`pkg/helmexec/exec.go:1218`). Span attributes deliberately do **not** mirror that + log line; §10 pins this with a redaction test. + +## 7. Performance and zero-impact guarantees + +**When disabled (the default):** +- `telemetry.Tracer` returns the OTel no-op tracer. No-op span start/end is a few ns and + allocation-free; provider setup, exporters, and the batch worker goroutine never start. +- `telemetry.CommandContext()` returns `context.Background()`; `app.New` behaves exactly as + today. No flag checks appear in hot loops. + +**When enabled:** +- Standard SDK BatchSpanProcessor (5s interval / 512-span batches); metrics + use a periodic reader (default 60s, `OTEL_METRIC_EXPORT_INTERVAL`) with a + final flush at exit. Three instruments exist — `helmfile.helm.exec.duration`, + `helmfile.release.duration`, and `helmfile.release.count` — so metric + cardinality stays tiny by default (bounded by subcommands and verbs, never + by release names); `HELMFILE_OTEL_METRICS_PER_RELEASE` opts into + name/namespace dimensions for bounded CI runs. A + `sync --concurrency=16` run produces at most one span per helm invocation plus one per + release — hundreds, not tens of thousands. Export happens off the critical path; the + only synchronous cost is the ≤5s shutdown flush, paid only when tracing is on. +- SDK spans and exporters are goroutine-safe; helmfile's parallel release workers need no + extra locking. + +**Functional-impact checklist (the review criteria for every PR in §11):** +1. No `helmexec.Interface` signature changes in any phase; `getHelm()` unchanged. +2. `app.New` changes one line (`Background()` → `telemetry.CommandContext()`), no call-site + churn; cancellation semantics identical. +3. Kubedog and hook bridging uses `context.WithoutCancel`, which drops cancellation and + keeps values — the swapped-out parents (`Background`/`TODO`) never propagated + cancellation either, so SIGINT/timeout behavior is unchanged. +4. Telemetry setup or export failures never fail or slow the run (warning log only, export + off the critical path). +5. All pre-existing behavior, including the known cancellation gaps of §4.3 and the exact + content of exit-error messages (legacy redaction profile, §6.2), is preserved + bit-for-bit; gap fixes and redaction unification are out of scope and filed separately. + +## 8. Dependency impact + +The OTel libraries are **already in the module graph as indirect dependencies**, required +transitively by existing direct dependencies (`helm.sh/helm/v4` v4.2.4 requires +`go.opentelemetry.io/otel` v1.44.0; helm v3 and `helmfile/vals` also carry otel modules). +Every module this proposal would import — `otel`, `otel/trace`, `otel/sdk`, +`otel/exporters/otlp/otlptrace/otlptracegrpc`, `.../otlptracehttp`, +`otel/exporters/stdout/stdouttrace`, and `contrib/exporters/autoexport` v0.67.0 — is +already pinned in `go.sum` (verified), so promoting them to direct requires brings +**zero new modules to download**. No conflict with existing OTel usage in the process: +none of helmfile's direct dependencies registers OTel globals in library code — verified +no `SetTracerProvider` call sites under helm v3/v4 `pkg/`, vals, or chartify (helm's own +OTel wiring, where present, lives in its CLI layer, not the libraries helmfile imports). +Choosing `autoexport` (§3.2) also means +helmfile maintains no exporter-construction code; its `RegisterSpanExporter` hook covers +any future backend (e.g. zipkin) without helmfile changes. Note the resulting defaults: +`http/protobuf` on `localhost:4318` unless the user overrides the protocol/endpoint. + +## 9. Code layout of the change + +``` +cmd/root.go // --otel-tracing flag, Setup call, root span, shutdown handoff +main.go // shutdown on both exit and signal paths +pkg/config/global.go // OtelTracing option (+ accessor on GlobalImpl) +pkg/envvar/const.go // OtelTracing = "HELMFILE_OTEL_TRACING" +pkg/telemetry/… // new package (§4.1) +pkg/app/app.go // app.New: Background() → telemetry.CommandContext(); helmfile.load span; loader ctx wiring +pkg/app/desired_state_file_loader.go // ctx field on the unexported desiredStateLoader struct (render-span parent) +pkg/app/two_pass_renderer.go // helmfile.render/helmfile.parse spans +pkg/helmexec/runner.go // helm.exec / os.exec spans (single choke point) +pkg/helmexec/redact.go // shared args redaction, legacy+strict profiles (extracted from exit_error.go) +pkg/helmexec/exit_error.go // calls shared helper with legacy profile (output byte-identical) +pkg/helmexec/context.go // HelmContext.Ctx field (phase 2) +pkg/helmexec/exec.go // execCtx funnel beside exec/execStdIn (phase 2) +pkg/state/state.go // WithoutCancel bridges (kubedog call site + both event.Bus constructions); release spans (phase 2) +pkg/state/helmx.go // (no change — bridge happens at its caller) +pkg/event/bus.go // optional Ctx field consumed by the default runner +docs/experimental-features.md // feature entry → promoted out when stable +docs/otel.md // user guide (config, backends, CI recipes, sample trace) +``` + +## 10. Testing strategy + +1. **Unit (`pkg/telemetry`)**: table-driven tests for enabled/disabled no-op guarantees, + `CommandContext()` identity, default resource attributes (`service.name=helmfile`, + `service.version` from `pkg/app/version`), and propagator extraction of `TRACEPARENT`. + Global state reset via `export_test.go` so tests stay isolated. +2. **Redaction (`pkg/helmexec`)**: table-driven tests for both profiles — `legacy` pinned + byte-identical by the existing `exit_error_test.go` goldens; `strict` covering `--set v` + and `--set=k=v`, `--set-string`/`--set-file`/`--set-json`, `--username`/`--password`, + benign flags untouched. Pins §6.6. +3. **Span-hierarchy golden tests**: drive `App` with the existing fake helm + (`pkg/exectest/helm.go`, pre-seeded into `App.helms` the way current app tests do) and + an in-memory exporter; assert the span tree (names, parent + links, order) for `template`/`sync` over a small fixture — catches context-plumbing + regressions. Includes an **orphan-span regression test** for the kubedog and hook paths + (§4.3): every exported span must have the root span as an ancestor. +4. **OTLP end-to-end**: an `httptest` server speaking OTLP/HTTP+protobuf, pointed at by + `OTEL_EXPORTER_OTLP_ENDPOINT`; decode exported payloads (`go.opentelemetry.io/proto/otlp`, + already in the graph) and assert count/attributes. Runs in unit-test context — no + external collector needed in CI. +5. **Lifecycle tests**: shutdown flushes on command failure and on SIGINT, so spans never + vanish on failing deploys — the case users care about most. (Requires extracting + `main.go`'s signal/select/exit logic into a small pure function; the extraction itself + is behavior-preserving and covered by the same tests.) Also covers the nil-shutdown + race noted in §4.2. +6. **Regression suite**: existing `make test` must pass unchanged with tracing compiled in + but disabled. Note the existing `pkg/app` tests construct `&App{...}` literals directly + and never call `app.New`, so they do *not* guard the §4.2 one-liner — PR 1 adds a + targeted test asserting `app.New` roots cancellation (and the span) exactly as before. + +## 11. Documentation & rollout + +1. `docs/otel.md` — user guide: enabling, env vars, collector recipes (Jaeger all-in-one, + Grafana Tempo, vendor SaaS), CI correlation via `TRACEPARENT`, sample trace reading. + Linked from `docs/index.md`. +2. Feature listed under **experimental** in `docs/experimental-features.md` for one or two + minor releases (feedback on span taxonomy is the main thing that may change), then + promoted to stable with a CHANGELOG entry. +3. PR sequence (each independently shippable and revertible, each measured against the + §7 functional-impact checklist): + - **PR 1**: `pkg/telemetry` + flag/env + root span + lifecycle + `app.New` one-liner + + docs + tests (§4.1–4.2). + - **PR 2**: `ShellRunner` instrumentation + shared redaction extraction/extension (§4.4 + phase-1 steps 2–3, §6) — the core value. + - **PR 3**: load/render spans + hook bridging + per-release spans via + `HelmContext.Ctx`/`execCtx` (§4.4 phase 2). + - **PR 4 (post-stabilization)**: metrics (e.g. `helmfile.helm.exec.duration` histogram, + `helmfile.release.count` by result), kubedog/vals spans. + +## 12. Future work (explicitly out of scope for v1) + +- ~~Metrics pipeline on the same provider (`otel/sdk/metric` with the same env-var config)~~ — delivered: `helmfile.helm.exec.duration`, `helmfile.release.count`, and `helmfile.release.duration` via `autoexport.NewMetricReader` (`OTEL_METRICS_EXPORTER`); per-release name/namespace attributes on the duration histogram are opt-in via `HELMFILE_OTEL_METRICS_PER_RELEASE` (bounded dimensions by default). +- `TRACEPARENT` injection into helm subprocess env so chart-test hooks / plugins can extend + the helmfile trace. +- Subprocesses started inside `github.com/helmfile/chartify` (kustomize, and any helm calls + chartify makes) are invisible to `ShellRunner` instrumentation; options are a wrapper + span around each chartify call in `pkg/state`, or upstream OTel support in chartify. +- Log-to-trace correlation (zap OTel appender). +- Remaining per-release instrumentation: the flag-preparation loops + (`prepareSyncReleases`/`prepareDiffReleases`) and the exec nesting for + `helmexec.Interface` methods that take no `HelmContext` + (TemplateRelease/Lint/Unittest/Fetch) — their exec spans currently sit flat + under the load span while the release span covers the surrounding work. +- Fixing the kubedog/hook cancellation gaps (§4.3) — separate issues filed from this design. + +## 13. Alternatives considered + +| Alternative | Why rejected | +|---|---| +| Structured logs + collector-side parsing | No hierarchy/timing guarantees; every backend needs custom parsing; poor UX. | +| Prometheus metrics only | Shows counts/durations but not critical paths or nesting; the request is explicitly about tracing where time goes. | +| Hand-rolled exporter selection in helmfile | `autoexport` already exists in the dependency graph and implements the spec env vars (verified: v0.67.0 supports `otlp`/`console`/`none`, protocol dispatch, `none` detection); hand-rolled code is pure maintenance burden and would drift from the spec. | +| Helmfile-specific env vars for endpoint/headers etc. | Duplicates the OTel spec; standard vars are already what platform teams configure. | +| Full `context.Context` refactor of `pkg/state` first | Large, risky churn unrelated to the feature; the `HelmContext.Ctx`/`execCtx` design achieves nesting without it. | +| Using the existing `WithContext` clone for per-release spans | The cached, shared execer (`a.helms`) serves concurrent workers; per-release contexts must travel with the per-call `HelmContext` parameter, not via instance substitution. | +| Config-file (`helmfile.yaml`) telemetry settings | Telemetry is an operational/platform concern, not state authoring; flag+env matches how it's injected in CI. | + +## 14. Open questions + +1. **Propagation into helm subprocesses**: should helmfile inject `TRACEPARENT` into the + child process env (opt-in) so hooks/plugins can continue the trace? (§12) +2. **Span-name taxonomy stability**: do we commit to the §5 names as stable API for + dashboard authors during the experimental window, or reserve the right to rename? + Proposal: rename freely while experimental, freeze on promotion. +3. **`helmfile.d` parallel mode**: state-file spans are siblings under the root span — is a + synthetic `helmfile.parallel` grouping span wanted, or does flat suffice? +4. **Trace output when `--log-level=debug`**: duplicate a compact span tree to stderr at + shutdown for quick local triage without a collector? +5. **Root span noise for trivial commands**: `helmfile version`/`help` also run + `PersistentPreRunE` — export a root span for them, or skip? (Proposal: skip via a + short denylist; cosmetic.) diff --git a/go.mod b/go.mod index 8646602d..d28e238f 100644 --- a/go.mod +++ b/go.mod @@ -30,6 +30,13 @@ require ( github.com/werf/kubedog v0.13.1-0.20260217150136-ed58edf34eac github.com/zclconf/go-cty v1.19.0 github.com/zclconf/go-cty-yaml v1.2.0 + go.opentelemetry.io/contrib/exporters/autoexport v0.67.0 + go.opentelemetry.io/otel v1.44.0 + go.opentelemetry.io/otel/metric v1.44.0 + go.opentelemetry.io/otel/sdk v1.44.0 + go.opentelemetry.io/otel/sdk/metric v1.44.0 + go.opentelemetry.io/otel/trace v1.44.0 + go.opentelemetry.io/proto/otlp v1.10.0 go.szostok.io/version v1.2.0 go.uber.org/zap v1.28.0 go.yaml.in/yaml/v2 v2.4.4 @@ -115,7 +122,7 @@ require ( google.golang.org/api v0.291.0 // indirect google.golang.org/genproto v0.0.0-20260622175928-b703f567277d // indirect google.golang.org/grpc v1.83.1 // indirect - google.golang.org/protobuf v1.36.12-0.20260120151049-f2248ac996af // indirect + google.golang.org/protobuf v1.36.12-0.20260120151049-f2248ac996af gopkg.in/ini.v1 v1.67.3 // indirect sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect sigs.k8s.io/yaml v1.6.0 // indirect @@ -187,6 +194,7 @@ require ( github.com/aws/aws-sdk-go-v2/service/sts v1.48.0 // indirect github.com/aws/smithy-go v1.28.1 // indirect github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect + github.com/beorn7/perks v1.0.1 // indirect github.com/blang/semver/v4 v4.0.0 // indirect github.com/cenkalti/backoff/v4 v4.3.0 // indirect github.com/cenkalti/backoff/v5 v5.0.3 // indirect @@ -259,6 +267,7 @@ require ( github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect github.com/googleapis/enterprise-certificate-proxy v0.3.19 // indirect github.com/gregjones/httpcache v0.0.0-20190611155906-901d90724c79 // indirect + github.com/grpc-ecosystem/grpc-gateway/v2 v2.28.0 // indirect github.com/hashicorp/go-retryablehttp v0.7.8 // indirect github.com/hashicorp/go-safetemp v1.0.0 // indirect github.com/hashicorp/go-secure-stdlib/parseutil v0.2.0 // indirect @@ -299,6 +308,11 @@ require ( github.com/peterbourgon/diskv v2.0.1+incompatible // indirect github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c // indirect github.com/planetscale/vtprotobuf v0.6.1-0.20240319094008-0393e58bdf10 // indirect + github.com/prometheus/client_golang v1.23.2 // indirect + github.com/prometheus/client_model v0.6.2 // indirect + github.com/prometheus/common v0.67.5 // indirect + github.com/prometheus/otlptranslator v1.0.0 // indirect + github.com/prometheus/procfs v0.20.1 // indirect github.com/rs/zerolog v1.26.1 // indirect github.com/rubenv/sql-migrate v1.8.1 // indirect github.com/russross/blackfriday/v2 v2.1.0 // indirect @@ -330,15 +344,23 @@ require ( github.com/zalando/go-keyring v0.2.6 // indirect go.mongodb.org/mongo-driver v1.17.9 // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect + go.opentelemetry.io/contrib/bridges/prometheus v0.67.0 // indirect go.opentelemetry.io/contrib/detectors/gcp v1.44.0 // indirect go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.69.0 // indirect go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.69.0 // indirect - go.opentelemetry.io/otel v1.44.0 // indirect - go.opentelemetry.io/otel/metric v1.44.0 // indirect - go.opentelemetry.io/otel/sdk v1.44.0 // indirect - go.opentelemetry.io/otel/sdk/metric v1.44.0 // indirect - go.opentelemetry.io/otel/trace v1.44.0 // indirect - go.opentelemetry.io/proto/otlp v1.10.0 // indirect + go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc v0.19.0 // indirect + go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp v0.19.0 // indirect + go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc v1.43.0 // indirect + go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp v1.43.0 // indirect + go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.43.0 // indirect + go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.43.0 // indirect + go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.43.0 // indirect + go.opentelemetry.io/otel/exporters/prometheus v0.65.0 // indirect + go.opentelemetry.io/otel/exporters/stdout/stdoutlog v0.19.0 // indirect + go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v1.44.0 // indirect + go.opentelemetry.io/otel/exporters/stdout/stdouttrace v1.43.0 // indirect + go.opentelemetry.io/otel/log v0.19.0 // indirect + go.opentelemetry.io/otel/sdk/log v0.19.0 // indirect go.uber.org/multierr v1.11.0 // indirect golang.org/x/crypto v0.54.0 // indirect golang.org/x/exp v0.0.0-20251219203646-944ab1f22d93 // indirect diff --git a/go.sum b/go.sum index 34349e57..6edae423 100644 --- a/go.sum +++ b/go.sum @@ -876,6 +876,8 @@ go.opentelemetry.io/otel/sdk v1.44.0 h1:nHYwb9lK+fJPU/dnT6s7W7Z8itMWyqrnVfbheVYr go.opentelemetry.io/otel/sdk v1.44.0/go.mod h1:Osuydd3Se74nqjAKxid74N5eC+jfEqfTegHRnq58oK0= go.opentelemetry.io/otel/sdk/log v0.19.0 h1:scYVLqT22D2gqXItnWiocLUKGH9yvkkeql5dBDiXyko= go.opentelemetry.io/otel/sdk/log v0.19.0/go.mod h1:vFBowwXGLlW9AvpuF7bMgnNI95LiW10szrOdvzBHlAg= +go.opentelemetry.io/otel/sdk/log/logtest v0.19.0 h1:BEbF7ZBB6qQloV/Ub1+3NQoOUnVtcGkU3XX4Ws3GQfk= +go.opentelemetry.io/otel/sdk/log/logtest v0.19.0/go.mod h1:Lua81/3yM0wOmoHTokLj9y9ADeA02v1naRrVrkAZuKk= go.opentelemetry.io/otel/sdk/metric v1.44.0 h1:3LlKgI+VjbVsjNRFZJZAJ30WjXC5VkNRks6si09iEfI= go.opentelemetry.io/otel/sdk/metric v1.44.0/go.mod h1:5B5pMARnXxKhltooO4xUuCBorl65a4EpnTalObqOigA= go.opentelemetry.io/otel/trace v1.44.0 h1:jxF5CsGYCe74MCRx2X4g7WsY/VBKRqqpNvXlX/6gtIk= diff --git a/main.go b/main.go index 0197027a..698bc208 100644 --- a/main.go +++ b/main.go @@ -1,6 +1,7 @@ package main import ( + gocontext "context" "os" "os/signal" "syscall" @@ -9,6 +10,7 @@ import ( "github.com/helmfile/helmfile/pkg/app" "github.com/helmfile/helmfile/pkg/config" "github.com/helmfile/helmfile/pkg/errors" + "github.com/helmfile/helmfile/pkg/telemetry" ) func main() { @@ -35,6 +37,8 @@ func main() { app.Cancel() app.CleanWaitGroup.Wait() + shutdownTelemetry(nil, signalExitCode(sig)) + // See http://tldp.org/LDP/abs/html/exitcodes.html switch sig { case syscall.SIGINT: @@ -44,6 +48,41 @@ func main() { } } case err := <-errChan: + shutdownTelemetry(err, exitCodeOf(err)) errors.HandleExitCoder(err) } } + +// shutdownTelemetry flushes buffered spans before the process exits. It is a +// no-op when tracing was never enabled. Flush failures are deliberately +// ignored: telemetry must not mask the command's own result. +func shutdownTelemetry(runErr error, exitCode int) { + ctx, cancel := gocontext.WithTimeout(gocontext.Background(), telemetry.ShutdownTimeout) + defer cancel() + _ = telemetry.Shutdown(ctx, runErr, exitCode) +} + +// signalExitCode maps termination signals to conventional exit codes, matching +// the os.Exit calls in main. +func signalExitCode(sig os.Signal) int { + switch sig { + case syscall.SIGINT: + return 130 + case syscall.SIGTERM: + return 143 + default: + return 1 + } +} + +// exitCodeOf extracts the exit code from the command result for the root-span +// attribute; success is 0. +func exitCodeOf(err error) int { + if err == nil { + return 0 + } + if exitErr, ok := err.(errors.ExitCoder); ok { + return exitErr.ExitCode() + } + return 1 +} diff --git a/mkdocs.yml b/mkdocs.yml index 36cf6e08..4b531c16 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -32,6 +32,7 @@ nav: - Advanced Features: advanced-features.md - Hooks: hooks.md - Secrets: remote-secrets.md + - OpenTelemetry Tracing: otel.md - Shared Configuration: shared-configuration-across-teams.md - Integrations: integrations.md - Experimental Features: experimental-features.md diff --git a/pkg/app/app.go b/pkg/app/app.go index 2d89d2f2..38c084a5 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -12,6 +12,8 @@ import ( "text/tabwriter" "github.com/helmfile/vals" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/trace" "go.uber.org/zap" "github.com/helmfile/helmfile/pkg/argparser" @@ -22,6 +24,7 @@ import ( "github.com/helmfile/helmfile/pkg/plugins" "github.com/helmfile/helmfile/pkg/remote" "github.com/helmfile/helmfile/pkg/state" + "github.com/helmfile/helmfile/pkg/telemetry" ) var CleanWaitGroup sync.WaitGroup @@ -76,7 +79,11 @@ type HelmRelease struct { } func New(conf ConfigProvider) *App { - ctx := goContext.Background() + // telemetry.CommandContext returns context.Background when tracing is + // disabled, so this is behavior-identical to the previous explicit + // Background() while rooting the app context under the command span when + // tracing is on. + ctx := telemetry.CommandContext() ctx, Cancel = goContext.WithCancel(ctx) return Init(&App{ @@ -935,6 +942,13 @@ func (a *App) loadDesiredStateFromYamlWithBaseDir(file string, baseDir string, o op = opts[0] } + // The load span covers remote fetching, rendering, and parsing of one + // state file; render/parse spans attach through the loader's traceCtx. + loadCtx, loadSpan := telemetry.Tracer(telemetry.ScopeHelmfile).Start(a.spanParentCtx(), "helmfile.load", + trace.WithAttributes(attribute.String("helmfile.state_file", file)), + ) + defer loadSpan.End() + ld := &desiredStateLoader{ fs: a.fs, env: a.Env, @@ -943,6 +957,7 @@ func (a *App) loadDesiredStateFromYamlWithBaseDir(file string, baseDir string, o logger: a.Logger, remote: a.remote, baseDir: baseDir, + traceCtx: loadCtx, overrideKubeContext: a.OverrideKubeContext, overrideHelmBinary: a.OverrideHelmBinary, @@ -957,6 +972,9 @@ func (a *App) loadDesiredStateFromYamlWithBaseDir(file string, baseDir string, o return nil, err } + // Per-release spans (pkg/state) parent under the load span. + st.SetTraceContext(loadCtx) + st.SetKubeconfig(a.Kubeconfig) return st, nil @@ -1639,7 +1657,24 @@ func (a *App) WrapWithoutSelector(converge func(*state.HelmState, helmexec.Inter } } +// spanParentCtx returns the context app-layer spans attach to. Tests +// construct App literals without a context, so nil falls back to Background +// (with tracing disabled, span starts are no-ops anyway). +func (a *App) spanParentCtx() goContext.Context { + if a.ctx != nil { + return a.ctx + } + return goContext.Background() +} + func (a *App) findDesiredStateFiles(specifiedPath string, opts LoadOpts) ([]string, error) { + _, span := telemetry.Tracer(telemetry.ScopeHelmfile).Start(a.spanParentCtx(), "helmfile.discover_states", + // specifiedPath is captured before Remote.Locate resolves it; it may + // be a remote reference carrying credentials in userinfo or query. + trace.WithAttributes(attribute.String("helmfile.path", helmexec.RedactedRef(specifiedPath))), + ) + defer span.End() + path, err := a.remote.Locate(specifiedPath, "states") if err != nil { return nil, fmt.Errorf("locate: %v", err) diff --git a/pkg/app/app_load_span_test.go b/pkg/app/app_load_span_test.go new file mode 100644 index 00000000..cbf9bb7c --- /dev/null +++ b/pkg/app/app_load_span_test.go @@ -0,0 +1,190 @@ +package app + +import ( + goContext "context" + "os" + "path/filepath" + "testing" + + "github.com/helmfile/vals" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + v1 "go.opentelemetry.io/proto/otlp/trace/v1" + + "github.com/helmfile/helmfile/pkg/exectest" + ffs "github.com/helmfile/helmfile/pkg/filesystem" + "github.com/helmfile/helmfile/pkg/helmexec" + "github.com/helmfile/helmfile/pkg/telemetry" + "github.com/helmfile/helmfile/pkg/telemetry/otlptest" +) + +// TestLoadSpanHierarchy drives a template run with telemetry enabled and the +// exectest fake helm, then asserts the state-loading span tree: +// root → discover_states → load → { render, parse }. This is the golden test +// for the app-layer context plumbing (docs/proposals/otel-tracing.md §10.3). +func TestLoadSpanHierarchy(t *testing.T) { + rec := otlptest.NewRecorder(t) + otlptest.SetupTelemetry(t, rec, "helmfile template") + + files := map[string]string{ + "/path/to/helmfile.yaml.gotmpl": ` +releases: +- name: demo + chart: incubator/raw +`, + } + + valsRuntime, err := vals.New(vals.Options{CacheSize: 32}) + require.NoError(t, err) + + helm := &exectest.Helm{ + FailOnUnexpectedList: true, + FailOnUnexpectedDiff: true, + } + + app := appWithFs(&App{ + OverrideHelmBinary: DefaultHelmBinary, + fs: &ffs.FileSystem{Glob: filepath.Glob}, + OverrideKubeContext: "default", + DisableKubeVersionAutoDetection: true, + Env: "default", + Logger: helmexec.NewLogger(os.Stderr, "warn"), + helms: map[helmKey]helmexec.Interface{ + createHelmKey("helm", "default"): helm, + }, + valsRuntime: valsRuntime, + ctx: telemetry.CommandContext(), + }, files) + + err = app.Template(applyConfig{ + concurrency: 1, + includeTransitiveNeeds: true, + logger: app.Logger, + }) + require.NoError(t, err) + + otlptest.ShutdownTelemetry(t) + + spans := rec.Spans(t) + find := func(name string) *v1.Span { + return otlptest.FindSpanWhere(t, spans, func(s *v1.Span) bool { return s.Name == name }, name) + } + + root := find("helmfile template") + discover := find("helmfile.discover_states") + load := find("helmfile.load") + render := find("helmfile.render") + parse := find("helmfile.parse") + + // Every span joins the command trace started by SetupTelemetry. + for _, s := range []*v1.Span{discover, load, render, parse} { + assert.Equal(t, root.TraceId, s.TraceId, "%s must join the command trace", s.Name) + } + + assert.Equal(t, root.SpanId, discover.ParentSpanId, "discover nests under root") + assert.Equal(t, root.SpanId, load.ParentSpanId, "load nests under root") + assert.Equal(t, load.SpanId, render.ParentSpanId, "render nests under load") + assert.Equal(t, load.SpanId, parse.ParentSpanId, "parse nests under load") + + file, ok := otlptest.AttrString(load, "helmfile.state_file") + require.True(t, ok) + assert.Contains(t, file, "helmfile.yaml.gotmpl") +} + +// TestLoadSpansAbsentWhenTelemetryDisabled pins that with telemetry disabled +// (the default) a template run behaves exactly as before and exports nothing. +func TestLoadSpansAbsentWhenTelemetryDisabled(t *testing.T) { + files := map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: demo + chart: incubator/raw +`, + } + + helm := &exectest.Helm{} + + valsRuntime, err := vals.New(vals.Options{CacheSize: 32}) + require.NoError(t, err) + + app := appWithFs(&App{ + OverrideHelmBinary: DefaultHelmBinary, + fs: &ffs.FileSystem{Glob: filepath.Glob}, + OverrideKubeContext: "default", + DisableKubeVersionAutoDetection: true, + Env: "default", + Logger: helmexec.NewLogger(os.Stderr, "warn"), + helms: map[helmKey]helmexec.Interface{ + createHelmKey("helm", "default"): helm, + }, + valsRuntime: valsRuntime, + ctx: goContext.Background(), + }, files) + + tmplErr := app.Template(applyConfig{ + concurrency: 1, + logger: app.Logger, + }) + require.NoError(t, tmplErr) +} + +// TestReleaseSpanHierarchy runs a sync with the exectest fake helm and +// asserts per-release spans: present, parented under the load span, and +// carrying the release identity attributes. +func TestReleaseSpanHierarchy(t *testing.T) { + rec := otlptest.NewRecorder(t) + otlptest.SetupTelemetry(t, rec, "helmfile sync") + + files := map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: demo + chart: incubator/raw + namespace: apps +`, + } + + valsRuntime, err := vals.New(vals.Options{CacheSize: 32}) + require.NoError(t, err) + + helm := &exectest.Helm{} + + app := appWithFs(&App{ + OverrideHelmBinary: DefaultHelmBinary, + fs: &ffs.FileSystem{Glob: filepath.Glob}, + OverrideKubeContext: "default", + DisableKubeVersionAutoDetection: true, + Env: "default", + Logger: helmexec.NewLogger(os.Stderr, "warn"), + helms: map[helmKey]helmexec.Interface{ + createHelmKey("helm", "default"): helm, + }, + valsRuntime: valsRuntime, + ctx: telemetry.CommandContext(), + }, files) + + syncErr := app.Sync(applyConfig{ + concurrency: 1, + logger: app.Logger, + }) + require.NoError(t, syncErr) + + otlptest.ShutdownTelemetry(t) + + spans := rec.Spans(t) + release := otlptest.FindSpanWhere(t, spans, func(s *v1.Span) bool { return s.Name == "helmfile.release.sync" }, "release sync span") + load := otlptest.FindSpanWhere(t, spans, func(s *v1.Span) bool { return s.Name == "helmfile.load" }, "load span") + + assert.Equal(t, load.TraceId, release.TraceId, "release span must join the load span's trace") + assert.Equal(t, load.SpanId, release.ParentSpanId, "release span must nest under the load span") + + name, ok := otlptest.AttrString(release, "helmfile.release") + require.True(t, ok) + assert.Equal(t, "demo", name) + ns, ok := otlptest.AttrString(release, "helmfile.namespace") + require.True(t, ok) + assert.Equal(t, "apps", ns) + chart, ok := otlptest.AttrString(release, "helmfile.chart") + require.True(t, ok) + assert.Equal(t, "incubator/raw", chart) +} diff --git a/pkg/app/app_new_context_test.go b/pkg/app/app_new_context_test.go new file mode 100644 index 00000000..a9ed48ba --- /dev/null +++ b/pkg/app/app_new_context_test.go @@ -0,0 +1,67 @@ +package app + +import ( + gocontext "context" + "testing" + "time" + + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/trace" + + "github.com/helmfile/helmfile/pkg/config" + "github.com/helmfile/helmfile/pkg/telemetry" +) + +func newAppForTest() *App { + globalImpl := config.NewGlobalImpl(&config.GlobalOptions{}) + printEnvImpl := config.NewPrintEnvImpl(globalImpl, &config.PrintEnvOptions{}) + return New(printEnvImpl) +} + +func requireCancelable(t *testing.T, a *App) { + t.Helper() + require.NotNil(t, a.ctx) + require.NotNil(t, Cancel) + Cancel() + select { + case <-a.ctx.Done(): + case <-time.After(time.Second): + t.Fatal("app context was not canceled by app.Cancel()") + } +} + +// TestNewPreservesContextContract pins the app.New context behavior that the +// telemetry integration relies on: with tracing disabled (the default), the +// app context is rooted at context.Background and remains cancellable exactly +// as before the telemetry.CommandContext() change. +func TestNewPreservesContextContract(t *testing.T) { + a := newAppForTest() + requireCancelable(t, a) +} + +// TestNewAdoptsTelemetryCommandContext verifies that app.New derives its +// context from the telemetry command span when tracing is enabled: the app +// context carries the command span's trace, so downstream spans nest under it, +// and cancellation still works. +func TestNewAdoptsTelemetryCommandContext(t *testing.T) { + t.Setenv("OTEL_TRACES_EXPORTER", "none") + t.Cleanup(func() { + _ = telemetry.Shutdown(gocontext.Background(), nil, 0) + }) + + telemetry.Setup(gocontext.Background(), telemetry.Options{Enabled: true, Version: "test"}) + telemetry.StartCommandSpan("helmfile test") + + commandSC := trace.SpanFromContext(telemetry.CommandContext()).SpanContext() + require.True(t, commandSC.IsValid(), "command span should be recording") + + a := newAppForTest() + + // WithCancel preserves context values, so the command span — and its trace + // ID — must still be visible through the app context. + appSC := trace.SpanFromContext(a.ctx).SpanContext() + require.True(t, appSC.IsValid()) + require.Equal(t, commandSC.TraceID(), appSC.TraceID(), "app context must derive from the command span context") + + requireCancelable(t, a) +} diff --git a/pkg/app/desired_state_file_loader.go b/pkg/app/desired_state_file_loader.go index 8f7b251d..c43fa857 100644 --- a/pkg/app/desired_state_file_loader.go +++ b/pkg/app/desired_state_file_loader.go @@ -2,6 +2,7 @@ package app import ( "bytes" + goContext "context" "errors" "fmt" "os" @@ -10,6 +11,8 @@ import ( "dario.cat/mergo" "github.com/helmfile/vals" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/trace" "go.uber.org/zap" "github.com/helmfile/helmfile/pkg/environment" @@ -19,6 +22,7 @@ import ( "github.com/helmfile/helmfile/pkg/policy" "github.com/helmfile/helmfile/pkg/remote" "github.com/helmfile/helmfile/pkg/state" + "github.com/helmfile/helmfile/pkg/telemetry" ) const ( @@ -32,6 +36,11 @@ type desiredStateLoader struct { overrideKustomizeBinary string enableLiveOutput bool + // traceCtx carries the helmfile.load span context; render/parse spans + // attach to it. nil falls back to context.Background, which with tracing + // disabled makes span starts no-ops. + traceCtx goContext.Context + env string namespace string chart string @@ -47,6 +56,14 @@ type desiredStateLoader struct { lockFilePath string } +// spanCtx returns the loader's span parent context. +func (ld *desiredStateLoader) spanCtx() goContext.Context { + if ld.traceCtx != nil { + return ld.traceCtx + } + return goContext.Background() +} + func (ld *desiredStateLoader) Load(f string, opts LoadOpts) (*state.HelmState, error) { var overrodeEnv *environment.Environment @@ -238,6 +255,16 @@ func (a *desiredStateLoader) rawLoad(yaml []byte, baseDir, file string, evaluate return st, nil } +// parsePart wraps rawLoad for one document part in a helmfile.parse span. +func (ld *desiredStateLoader) parsePart(rawContent []byte, baseDir, filename string, evaluateBases bool, env, overrodeEnv *environment.Environment) (*state.HelmState, error) { + _, span := telemetry.Tracer(telemetry.ScopeHelmfile).Start(ld.spanCtx(), "helmfile.parse", + trace.WithAttributes(attribute.String("helmfile.state_file", filename)), + ) + defer span.End() + + return ld.rawLoad(rawContent, baseDir, filename, evaluateBases, env, overrodeEnv) +} + func (ld *desiredStateLoader) load(env, overrodeEnv *environment.Environment, baseDir, filename string, content []byte, evaluateBases bool) (*state.HelmState, error) { // Allows part-splitting to work with CLRF-ed content normalizedContent := bytes.ReplaceAll(content, []byte("\r\n"), []byte("\n")) @@ -280,14 +307,7 @@ func (ld *desiredStateLoader) load(env, overrodeEnv *environment.Environment, ba rawContent = part } - currentState, err := ld.rawLoad( - rawContent, - baseDir, - filename, - evaluateBases, - env, - overrodeEnv, - ) + currentState, err := ld.parsePart(rawContent, baseDir, filename, evaluateBases, env, overrodeEnv) if err != nil { return nil, err } diff --git a/pkg/app/two_pass_renderer.go b/pkg/app/two_pass_renderer.go index 637f99be..b59d113e 100644 --- a/pkg/app/two_pass_renderer.go +++ b/pkg/app/two_pass_renderer.go @@ -5,8 +5,12 @@ import ( "fmt" "strings" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/trace" + "github.com/helmfile/helmfile/pkg/environment" "github.com/helmfile/helmfile/pkg/state" + "github.com/helmfile/helmfile/pkg/telemetry" "github.com/helmfile/helmfile/pkg/tmpl" ) @@ -33,6 +37,11 @@ func (r *desiredStateLoader) renderTemplatesToYamlWithEnv(baseDir, filename stri } func (r *desiredStateLoader) twoPassRenderTemplateToYaml(inherited, overrode *environment.Environment, baseDir, filename string, content []byte) (*bytes.Buffer, error) { + _, span := telemetry.Tracer(telemetry.ScopeHelmfile).Start(r.spanCtx(), "helmfile.render", + trace.WithAttributes(attribute.String("helmfile.state_file", filename)), + ) + defer span.End() + var phase string r.logger.Debugf("%srendering starting for \"%s\": inherited=%v, overrode=%v", phase, filename, inherited, overrode) diff --git a/pkg/config/global.go b/pkg/config/global.go index ce18e4b7..3a2ff126 100644 --- a/pkg/config/global.go +++ b/pkg/config/global.go @@ -84,6 +84,8 @@ type GlobalOptions struct { LogOutput io.Writer // SequentialHelmfiles is true if helmfile.d files should be processed sequentially instead of in parallel. SequentialHelmfiles bool + // OtelTracing is true if OpenTelemetry tracing should be enabled for this run. + OtelTracing bool } // Logger returns the logger to use. @@ -383,6 +385,15 @@ func (g *GlobalImpl) Interactive() bool { return os.Getenv(envvar.Interactive) == "true" } +// OtelTracing returns true if OpenTelemetry tracing is enabled via the +// --otel-tracing flag or the HELMFILE_OTEL_TRACING environment variable. +func (g *GlobalImpl) OtelTracing() bool { + if g.GlobalOptions.OtelTracing { + return true + } + return os.Getenv(envvar.OtelTracing) == "true" +} + // Args returns the args to use for helm func (g *GlobalImpl) Args() string { args := g.GlobalOptions.Args diff --git a/pkg/envvar/const.go b/pkg/envvar/const.go index 6894d109..985bd881 100644 --- a/pkg/envvar/const.go +++ b/pkg/envvar/const.go @@ -26,6 +26,8 @@ const ( CacheHome = "HELMFILE_CACHE_HOME" Interactive = "HELMFILE_INTERACTIVE" RepoRetry = "HELMFILE_REPO_RETRIES" + OtelTracing = "HELMFILE_OTEL_TRACING" + OtelMetricsPerRelease = "HELMFILE_OTEL_METRICS_PER_RELEASE" RenderYaml = "HELMFILE_RENDER_YAML" // force helmfile.yaml to be rendered as template regardless of extension, expecting "true" lower case // AWSSDKLogLevel controls AWS SDK logging level diff --git a/pkg/event/bus.go b/pkg/event/bus.go index 76e11eba..5e022e63 100644 --- a/pkg/event/bus.go +++ b/pkg/event/bus.go @@ -7,12 +7,16 @@ import ( "strconv" "strings" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/codes" + "go.opentelemetry.io/otel/trace" "go.uber.org/zap" "github.com/helmfile/helmfile/pkg/environment" "github.com/helmfile/helmfile/pkg/envvar" "github.com/helmfile/helmfile/pkg/filesystem" "github.com/helmfile/helmfile/pkg/helmexec" + "github.com/helmfile/helmfile/pkg/telemetry" "github.com/helmfile/helmfile/pkg/tmpl" ) @@ -34,6 +38,12 @@ type Bus struct { Runner helmexec.Runner Hooks []Hook + // Ctx, when set, is used by the lazily-constructed default Runner so that + // hook subprocesses join the current trace. It should carry trace context + // without cancellation (see the WithoutCancel bridge in pkg/state); nil + // falls back to context.TODO(), the historical behavior. + Ctx goContext.Context + BasePath string StateFilePath string Namespace string @@ -59,12 +69,16 @@ func (bus *Bus) Trigger(evt string, evtErr error, context map[string]any) (bool, } if bus.Runner == nil { + ctx := bus.Ctx + if ctx == nil { + // It would be better to pass app.Ctx here, but it requires a lot of work. + // It seems that this code only for running hooks, which took not to long time as helm. + ctx = goContext.TODO() + } bus.Runner = helmexec.ShellRunner{ Dir: bus.BasePath, Logger: bus.Logger, - // It would be better to pass app.Ctx here, but it requires a lot of work. - // It seems that this code only for running hooks, which took not to long time as helm. - Ctx: goContext.TODO(), + Ctx: ctx, } } @@ -79,77 +93,142 @@ func (bus *Bus) Trigger(evt string, evtErr error, context map[string]any) (bool, continue } - var err error - - name := hook.Name - if name == "" { - if hook.Kubectl != nil { - name = "kubectlApply" - } else { - name = hook.Command - } - } - - if hook.Kubectl != nil { - if hook.Command != "" { - bus.Logger.Warnf("warn: ignoring command '%s' given within a kubectlApply hook", hook.Command) - } - hook.Command = "kubectl" - if val, found := hook.Kubectl["filename"]; found { - if _, found := hook.Kubectl["kustomize"]; found { - return false, fmt.Errorf("hook[%s]: kustomize & filename cannot be used together", name) - } - hook.Args = append([]string{"apply", "-f"}, val) - } else if val, found := hook.Kubectl["kustomize"]; found { - hook.Args = append([]string{"apply", "-k"}, val) - } else { - return false, fmt.Errorf("hook[%s]: either kustomize or filename must be given", name) - } - } - - bus.Logger.Debugf("hook[%s]: stateFilePath=%s, basePath=%s\n", name, bus.StateFilePath, bus.BasePath) - - data := map[string]any{ - "Environment": bus.Env, - "Namespace": bus.Namespace, - "Event": event{ - Name: evt, - Error: evtErr, - }, - } - for k, v := range context { - data[k] = v - } - render := tmpl.NewTextRenderer(bus.Fs, bus.BasePath, data) - - bus.Logger.Debugf("hook[%s]: triggered by event \"%s\"\n", name, evt) - - command, err := render.RenderTemplateText(hook.Command) + hookExecuted, err := bus.runHook(hook, evt, evtErr, context) if err != nil { - return false, fmt.Errorf("hook[%s]: %v", name, err) + return false, err } - - args := make([]string, len(hook.Args)) - for i, raw := range hook.Args { - args[i], err = render.RenderTemplateText(raw) - if err != nil { - return false, fmt.Errorf("hook[%s]: %v", name, err) - } - } - - bytes, err := bus.Runner.Execute(command, args, map[string]string{}, false) - bus.Logger.Debugf("hook[%s]: %s\n", name, string(bytes)) - if hook.ShowLogs { - prefix := fmt.Sprintf("\nhook[%s] logs | ", evt) - bus.Logger.Infow(prefix + strings.ReplaceAll(string(bytes), "\n", prefix)) - } - - if err != nil { - return false, fmt.Errorf("hook[%s]: command `%s` failed: %v", name, command, err) - } - - executed = true + executed = executed || hookExecuted } return executed, nil } + +// runHook renders and executes a single hook; the returned bool reports +// whether the hook ran. The whole hook execution is wrapped in one +// helmfile.hook span (a no-op when telemetry is disabled), and the hook's +// subprocess span nests under it via hookRunner. +func (bus *Bus) runHook(hook Hook, evt string, evtErr error, context map[string]any) (executed bool, err error) { + name := hook.Name + if name == "" { + if hook.Kubectl != nil { + name = "kubectlApply" + } else { + name = hook.Command + } + } + + hookCtx, span := telemetry.Tracer(telemetry.ScopeHelmfile).Start(bus.spanParent(), "helmfile.hook", + trace.WithAttributes( + attribute.String("hook.event", evt), + attribute.String("hook.name", name), + ), + ) + defer func() { + if err != nil { + // The raw error embeds the rendered command (which may contain + // templated credentials) and the runner's detailed exit error; + // keep the span description generic. + span.SetStatus(codes.Error, "hook failed") + } + span.End() + }() + + if hook.Kubectl != nil { + if err := bus.prepareKubectlHook(&hook, name); err != nil { + return false, err + } + } + + bus.Logger.Debugf("hook[%s]: stateFilePath=%s, basePath=%s\n", name, bus.StateFilePath, bus.BasePath) + + data := map[string]any{ + "Environment": bus.Env, + "Namespace": bus.Namespace, + "Event": event{ + Name: evt, + Error: evtErr, + }, + } + for k, v := range context { + data[k] = v + } + render := tmpl.NewTextRenderer(bus.Fs, bus.BasePath, data) + + bus.Logger.Debugf("hook[%s]: triggered by event \"%s\"\n", name, evt) + + command, err := render.RenderTemplateText(hook.Command) + if err != nil { + return false, fmt.Errorf("hook[%s]: %v", name, err) + } + + args := make([]string, len(hook.Args)) + for i, raw := range hook.Args { + args[i], err = render.RenderTemplateText(raw) + if err != nil { + return false, fmt.Errorf("hook[%s]: %v", name, err) + } + } + + bytes, err := bus.hookRunner(hookCtx).Execute(command, args, map[string]string{}, false) + bus.Logger.Debugf("hook[%s]: %s\n", name, string(bytes)) + if hook.ShowLogs { + prefix := fmt.Sprintf("\nhook[%s] logs | ", evt) + bus.Logger.Infow(prefix + strings.ReplaceAll(string(bytes), "\n", prefix)) + } + + if err != nil { + return false, fmt.Errorf("hook[%s]: command `%s` failed: %v", name, command, err) + } + + return true, nil +} + +// prepareKubectlHook rewrites a kubectlApply hook into the equivalent +// explicit kubectl command, rejecting invalid configurations. +func (bus *Bus) prepareKubectlHook(hook *Hook, name string) error { + if hook.Command != "" { + bus.Logger.Warnf("warn: ignoring command '%s' given within a kubectlApply hook", hook.Command) + } + hook.Command = "kubectl" + + if val, found := hook.Kubectl["filename"]; found { + if _, found := hook.Kubectl["kustomize"]; found { + return fmt.Errorf("hook[%s]: kustomize & filename cannot be used together", name) + } + hook.Args = append([]string{"apply", "-f"}, val) + return nil + } + if val, found := hook.Kubectl["kustomize"]; found { + hook.Args = append([]string{"apply", "-k"}, val) + return nil + } + return fmt.Errorf("hook[%s]: either kustomize or filename must be given", name) +} + +// spanParent returns the context hook spans attach to. +func (bus *Bus) spanParent() goContext.Context { + if bus.Ctx != nil { + return bus.Ctx + } + return goContext.Background() +} + +// hookRunner returns a runner whose context is the hook span's, so the +// subprocess span started inside ShellRunner nests under the hook span. The +// cancellation semantics are unchanged: hookCtx derives from Bus.Ctx, which +// by contract never carries cancellation. Non-ShellRunner runners (test +// fakes) are returned unchanged. +func (bus *Bus) hookRunner(hookCtx goContext.Context) helmexec.Runner { + switch r := bus.Runner.(type) { + case *helmexec.ShellRunner: + clone := *r + clone.Ctx = hookCtx + return &clone + case helmexec.ShellRunner: + clone := r + clone.Ctx = hookCtx + return clone + default: + return bus.Runner + } +} diff --git a/pkg/event/bus_span_test.go b/pkg/event/bus_span_test.go new file mode 100644 index 00000000..62f263e2 --- /dev/null +++ b/pkg/event/bus_span_test.go @@ -0,0 +1,113 @@ +package event + +import ( + goContext "context" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + v1 "go.opentelemetry.io/proto/otlp/trace/v1" + "go.uber.org/zap" + + "github.com/helmfile/helmfile/pkg/filesystem" + "github.com/helmfile/helmfile/pkg/telemetry" + "github.com/helmfile/helmfile/pkg/telemetry/otlptest" +) + +// TestHookSpanExported runs one hook through the default ShellRunner with +// telemetry enabled and asserts the helmfile.hook span and its child os.exec +// span (proving the hook's subprocess nests under the hook span, not the +// command span directly). +func TestHookSpanExported(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("uses the unix true binary") + } + + rec := otlptest.NewRecorder(t) + otlptest.SetupTelemetry(t, rec, "helmfile test") + + bus := &Bus{ + Hooks: []Hook{ + { + Name: "migrate", + Events: []string{"presync"}, + Command: "true", + }, + { + Name: "other-event-hook", + Events: []string{"postsync"}, + Command: "true", + }, + }, + Logger: zap.NewNop().Sugar(), + Fs: filesystem.DefaultFileSystem(), + Ctx: goContext.WithoutCancel(telemetry.CommandContext()), + } + + executed, err := bus.Trigger("presync", nil, nil) + require.NoError(t, err) + assert.True(t, executed, "the presync hook should have run") + + otlptest.ShutdownTelemetry(t) + + spans := rec.Spans(t) + hook := otlptest.FindSpanWhere(t, spans, func(s *v1.Span) bool { return s.Name == "helmfile.hook" }, "hook span") + + evt, ok := otlptest.AttrString(hook, "hook.event") + require.True(t, ok) + assert.Equal(t, "presync", evt) + name, ok := otlptest.AttrString(hook, "hook.name") + require.True(t, ok) + assert.Equal(t, "migrate", name) + + // Exactly one hook span: the postsync hook must not run for presync. + for _, s := range spans { + if s.Name == "helmfile.hook" { + id, _ := otlptest.AttrString(s, "hook.name") + assert.Equal(t, "migrate", id, "only the matching hook should produce a span") + } + } + + // The hook's subprocess span nests under the hook span. + exec := otlptest.FindSpanWhere(t, spans, func(s *v1.Span) bool { return s.Name == "os.exec" }, "hook exec span") + assert.Equal(t, hook.TraceId, exec.TraceId) + assert.Equal(t, hook.SpanId, exec.ParentSpanId, "os.exec span must nest under the helmfile.hook span") +} + +// TestHookSpanFailureMessageIsGeneric runs a failing hook and asserts the +// span's error status does not embed the rendered command or runner details. +func TestHookSpanFailureMessageIsGeneric(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("uses the unix false binary") + } + + rec := otlptest.NewRecorder(t) + otlptest.SetupTelemetry(t, rec, "helmfile test") + + bus := &Bus{ + Hooks: []Hook{ + { + Name: "fail", + Events: []string{"presync"}, + Command: "false", + Args: []string{"--secret", "hunter2"}, + }, + }, + Logger: zap.NewNop().Sugar(), + Fs: filesystem.DefaultFileSystem(), + Ctx: goContext.WithoutCancel(telemetry.CommandContext()), + } + + _, err := bus.Trigger("presync", nil, nil) + require.Error(t, err) + + otlptest.ShutdownTelemetry(t) + + hook := otlptest.FindSpanWhere(t, rec.Spans(t), func(s *v1.Span) bool { return s.Name == "helmfile.hook" }, "hook span") + require.NotNil(t, hook.Status) + assert.Equal(t, v1.Status_STATUS_CODE_ERROR, hook.Status.Code) + assert.Equal(t, "hook failed", hook.Status.Message) + assert.NotContains(t, hook.Status.Message, "false") + assert.NotContains(t, hook.Status.Message, "hunter2") +} diff --git a/pkg/event/bus_test.go b/pkg/event/bus_test.go index 2334f175..9cf92798 100644 --- a/pkg/event/bus_test.go +++ b/pkg/event/bus_test.go @@ -1,6 +1,7 @@ package event import ( + goContext "context" "errors" "fmt" "io" @@ -13,6 +14,7 @@ import ( "github.com/helmfile/helmfile/pkg/environment" ffs "github.com/helmfile/helmfile/pkg/filesystem" + "github.com/helmfile/helmfile/pkg/helmexec" ) type runner struct { @@ -343,3 +345,28 @@ func TestTriggerCleanupEventWithNilError(t *testing.T) { t.Errorf("expected arg %q, got %q", expectedArg, call.args[0]) } } + +func TestBusDefaultRunnerUsesCtxWhenSet(t *testing.T) { + ctx, cancel := goContext.WithCancel(goContext.Background()) + defer cancel() + + bus := &Bus{Ctx: ctx, Logger: zap.NewNop().Sugar()} + + _, err := bus.Trigger("presync", nil, nil) + require.NoError(t, err) + + runner, ok := bus.Runner.(helmexec.ShellRunner) + require.True(t, ok, "default runner should be a ShellRunner") + require.Equal(t, ctx, runner.Ctx, "default runner must use Bus.Ctx when set") +} + +func TestBusDefaultRunnerFallsBackToTODO(t *testing.T) { + bus := &Bus{Logger: zap.NewNop().Sugar()} + + _, err := bus.Trigger("presync", nil, nil) + require.NoError(t, err) + + runner, ok := bus.Runner.(helmexec.ShellRunner) + require.True(t, ok) + require.Equal(t, goContext.TODO(), runner.Ctx, "nil Bus.Ctx must preserve the historical TODO context") +} diff --git a/pkg/helmexec/context.go b/pkg/helmexec/context.go index 55df302b..f4d4eae6 100644 --- a/pkg/helmexec/context.go +++ b/pkg/helmexec/context.go @@ -1,6 +1,7 @@ package helmexec import ( + "context" "io" ) @@ -8,4 +9,12 @@ type HelmContext struct { HistoryMax int WorkerIndex int Writer io.Writer + + // Ctx, when set, carries the per-release span context so that helm + // subprocesses started for this release nest under the release span + // (consumed by the execer's execWithContext funnel; nil falls back to the + // runner's own context, i.e. the historical behavior). Cancellation + // semantics are unchanged either way: callers derive it from the app + // context, which is where the runner's context comes from too. + Ctx context.Context } diff --git a/pkg/helmexec/exec.go b/pkg/helmexec/exec.go index e7995758..e747a43e 100644 --- a/pkg/helmexec/exec.go +++ b/pkg/helmexec/exec.go @@ -181,12 +181,20 @@ func GetPluginVersion(name, pluginsDir string) (*semver.Version, error) { return nil, fmt.Errorf("plugin %s not installed", name) } -func redactedURL(chart string) string { - chartURL, err := url.ParseRequestURI(chart) +// RedactedURL returns ref with any password in the URL userinfo replaced, +// matching helmfile's log redaction (url.URL.Redacted). Non-URL strings are +// returned unchanged. Telemetry reuses it so span attributes are sanitized at +// least as strictly as log output. +func RedactedURL(ref string) string { + refURL, err := url.ParseRequestURI(ref) if err != nil { - return chart + return ref } - return chartURL.Redacted() + return refURL.Redacted() +} + +func redactedURL(chart string) string { + return RedactedURL(chart) } // New for running helm commands @@ -415,7 +423,7 @@ func (helm *execer) AddRepo(name, repository, cafile, certfile, keyfile, usernam fmt.Fprintf(&buffer, "%s\n", password) return helm.execStdIn(args, map[string]string{}, &buffer) } - return helm.exec(args, map[string]string{}, nil) + return helm.exec(args, map[string]string{}) }) default: helm.logger.Errorf("ERROR: unknown type '%v' for repository %v", managed, name) @@ -434,7 +442,7 @@ func (helm *execer) UpdateRepo() error { helm.extra = savedExtra }() out, err := helm.retryRepoOp("update", func() ([]byte, error) { - return helm.exec([]string{"repo", "update"}, map[string]string{}, nil) + return helm.exec([]string{"repo", "update"}, map[string]string{}) }) helm.info(out) return err @@ -624,7 +632,7 @@ func (helm *execer) BuildDeps(name, chart string, flags ...string) error { args = append(args, "--plain-http") } - out, err := helm.exec(args, map[string]string{}, nil) + out, err := helm.exec(args, map[string]string{}) helm.info(out) return err } @@ -646,7 +654,7 @@ func (helm *execer) UpdateDeps(chart string) error { args = append(args, "--plain-http") } - out, err := helm.exec(args, map[string]string{}, nil) + out, err := helm.exec(args, map[string]string{}) helm.info(out) return err } @@ -658,7 +666,7 @@ func (helm *execer) SyncRelease(context HelmContext, name, chart, namespace stri flags = append(flags, "--history-max", strconv.Itoa(context.HistoryMax)) - out, err := helm.exec(append(append(preArgs, "upgrade", "--install", name, chart), flags...), env, nil) + out, err := helm.execWithContext(context.Ctx, append(append(preArgs, "upgrade", "--install", name, chart), flags...), env, nil) helm.info(out) return err } @@ -667,7 +675,7 @@ func (helm *execer) ReleaseStatus(context HelmContext, name string, flags ...str helm.logger.Infof("Getting status %v", name) preArgs := make([]string, 0) env := make(map[string]string) - out, err := helm.exec(append(append(preArgs, "status", name), flags...), env, nil) + out, err := helm.execWithContext(context.Ctx, append(append(preArgs, "status", name), flags...), env, nil) helm.info(out) return err } @@ -679,7 +687,7 @@ func (helm *execer) List(context HelmContext, filter string, flags ...string) (s args := []string{"list", "--filter", filter} enableLiveOutput := false - out, err := helm.exec(append(append(preArgs, args...), flags...), env, &enableLiveOutput) + out, err := helm.execWithContext(context.Ctx, append(append(preArgs, args...), flags...), env, &enableLiveOutput) // In v2 we have been expecting `helm list FILTER` prints nothing. // In v3 helm still prints the header like `NAME NAMESPACE REVISION UPDATED STATUS CHART APP VERSION`, // which confuses helmfile's existing logic that treats any non-empty output from `helm list` is considered as the indication @@ -734,7 +742,7 @@ func (helm *execer) DecryptSecret(context HelmContext, name string, flags ...str secretArg = "decrypt" } enableLiveOutput := false - secretBytes, err := helm.exec(append(append(preArgs, "secrets", secretArg, absPath), flags...), env, &enableLiveOutput) + secretBytes, err := helm.execWithContext(context.Ctx, append(append(preArgs, "secrets", secretArg, absPath), flags...), env, &enableLiveOutput) if err != nil { secret.err = err return "", err @@ -844,7 +852,7 @@ func (helm *execer) TemplateRelease(name string, chart string, flags ...string) return fmt.Errorf("output dir not found for template command") } - out, err := helm.exec(append(args, filteredFlags...), map[string]string{}, nil) + out, err := helm.exec(append(args, filteredFlags...), map[string]string{}) if err != nil { return err } @@ -876,7 +884,7 @@ func (helm *execer) TemplateRelease(name string, chart string, flags ...string) return nil } - out, err := helm.exec(append(args, flags...), map[string]string{}, nil) + out, err := helm.exec(append(args, flags...), map[string]string{}) if outputToFile { // With --output-dir is passed to helm-template, @@ -921,7 +929,7 @@ func (helm *execer) DiffRelease(context HelmContext, name, chart, namespace stri flags = helm.filterColorFlagsForHelm4(flags, env) } - out, err := helm.exec(append(append(preArgs, "diff", "upgrade", "--allow-unreleased", name, chart), flags...), env, overrideEnableLiveOutput) + out, err := helm.execWithContext(context.Ctx, append(append(preArgs, "diff", "upgrade", "--allow-unreleased", name, chart), flags...), env, overrideEnableLiveOutput) // Do our best to write STDOUT only when diff existed // Unfortunately, this works only when you run helmfile with `--detailed-exitcode` detailedExitcodeEnabled := false @@ -978,7 +986,7 @@ func (helm *execer) filterColorFlagsForHelm4(flags []string, env map[string]stri func (helm *execer) Lint(name, chart string, flags ...string) error { helm.logger.Infof("Linting release=%v, chart=%v", name, chart) - out, err := helm.exec(append([]string{"lint", chart}, flags...), map[string]string{}, nil) + out, err := helm.exec(append([]string{"lint", chart}, flags...), map[string]string{}) // Always write to stdout to write the linting result to eg. a file helm.write(nil, out) return err @@ -1003,14 +1011,14 @@ func (helm *execer) Unittest(name, chart string, flags ...string) error { } helm.logger.Infof("Unit testing release=%v, chart=%v", name, chart) - out, err := helm.exec(append([]string{"unittest", chart}, flags...), map[string]string{}, nil) + out, err := helm.exec(append([]string{"unittest", chart}, flags...), map[string]string{}) helm.write(nil, out) return err } func (helm *execer) Fetch(chart string, flags ...string) error { helm.logger.Infof("Fetching %v", redactedURL(chart)) - out, err := helm.exec(append([]string{"fetch", chart}, flags...), map[string]string{}, nil) + out, err := helm.exec(append([]string{"fetch", chart}, flags...), map[string]string{}) helm.info(out) return err } @@ -1032,7 +1040,7 @@ func (helm *execer) ChartPull(chart string, path string, flags ...string) error } else { helmArgs = []string{"chart", "pull", chart} } - out, err := helm.exec(helmArgs, map[string]string{"HELM_EXPERIMENTAL_OCI": "1"}, nil) + out, err := helm.exec(helmArgs, map[string]string{"HELM_EXPERIMENTAL_OCI": "1"}) helm.info(out) return err } @@ -1048,7 +1056,7 @@ func (helm *execer) ChartExport(chart string, path string) error { helm.logger.Infof("Exporting %v", chart) helmArgs = []string{"chart", "export", chart, "--destination", path} // no extra flags for before v3.7.0, details in helm chart export --help - out, err := helm.exec(helmArgs, map[string]string{"HELM_EXPERIMENTAL_OCI": "1"}, nil) + out, err := helm.exec(helmArgs, map[string]string{"HELM_EXPERIMENTAL_OCI": "1"}) helm.info(out) return err } @@ -1057,7 +1065,7 @@ func (helm *execer) DeleteRelease(context HelmContext, name string, flags ...str helm.logger.Infof("Deleting %v", name) preArgs := make([]string, 0) env := make(map[string]string) - out, err := helm.exec(append(append(preArgs, "delete", name), flags...), env, nil) + out, err := helm.execWithContext(context.Ctx, append(append(preArgs, "delete", name), flags...), env, nil) helm.info(out) return err } @@ -1067,7 +1075,7 @@ func (helm *execer) TestRelease(context HelmContext, name string, flags ...strin preArgs := make([]string, 0) env := make(map[string]string) args := []string{"test", name} - out, err := helm.exec(append(append(preArgs, args...), flags...), env, nil) + out, err := helm.execWithContext(context.Ctx, append(append(preArgs, args...), flags...), env, nil) helm.info(out) return err } @@ -1081,7 +1089,7 @@ func (helm *execer) AddPlugin(name, path, version string) error { } // Try with verification first - out, err := helm.exec([]string{"plugin", "install", path, "--version", version}, map[string]string{}, nil) + out, err := helm.exec([]string{"plugin", "install", path, "--version", version}, map[string]string{}) // If verification fails, retry without verification (unless enforced) if err != nil && strings.Contains(err.Error(), "does not support verification") { @@ -1090,7 +1098,7 @@ func (helm *execer) AddPlugin(name, path, version string) error { return fmt.Errorf("plugin %s does not support verification (remove --enforce-plugin-verification flag to allow unverified plugins)", name) } helm.logger.Debugf("Plugin %v does not support verification, retrying with --verify=false", name) - out, err = helm.exec([]string{"plugin", "install", path, "--version", version, "--verify=false"}, map[string]string{}, nil) + out, err = helm.exec([]string{"plugin", "install", path, "--version", version, "--verify=false"}, map[string]string{}) } helm.info(out) @@ -1121,7 +1129,7 @@ func (helm *execer) installHelmSecretsV4(version string) error { args = append(args, verifyFlag) } - out, err := helm.exec(args, map[string]string{}, nil) + out, err := helm.exec(args, map[string]string{}) if err != nil { return fmt.Errorf("failed to install %s: %w", plugin, err) } @@ -1159,7 +1167,7 @@ func helmSecretsRequiresSplitInstall(version string) bool { func (helm *execer) uninstallPlugin(name string) error { helm.logger.Infof("Uninstalling helm plugin %v", name) - out, err := helm.exec([]string{"plugin", "uninstall", name}, map[string]string{}, nil) + out, err := helm.exec([]string{"plugin", "uninstall", name}, map[string]string{}) if err == nil { helm.info(out) } @@ -1204,7 +1212,28 @@ func (helm *execer) UpdatePlugin(name, repo, version string) error { return helm.AddPlugin(name, repo, version) } -func (helm *execer) exec(args []string, env map[string]string, overrideEnableLiveOutput *bool) ([]byte, error) { +func (helm *execer) exec(args []string, env map[string]string) ([]byte, error) { + return helm.execWithRunner(helm.runner, args, env, nil) +} + +// execWithContext behaves like exec but attaches the span carried by ctx +// (from HelmContext.Ctx) so the subprocess span nests under the per-release +// span. Crucially, the subprocess keeps the runner's own context: replacing +// it would override specialized cancellation contexts such as the kubedog +// safety valve installed via execer.WithContext. A nil ctx is exactly exec. +func (helm *execer) execWithContext(ctx context.Context, args []string, env map[string]string, overrideEnableLiveOutput *bool) ([]byte, error) { + runner := helm.runner + if ctx != nil { + runner = withRunnerCtx(runner, func(runnerCtx context.Context) context.Context { + return spanAttachedContext(runnerCtx, ctx) + }) + } + return helm.execWithRunner(runner, args, env, overrideEnableLiveOutput) +} + +func (helm *execer) execWithRunner(runner Runner, args []string, env map[string]string, overrideEnableLiveOutput *bool) ([]byte, error) { + runner = markHelmRunner(runner) + cmdargs := args if len(helm.extra) > 0 { cmdargs = append(cmdargs, helm.extra...) @@ -1221,7 +1250,7 @@ func (helm *execer) exec(args []string, env map[string]string, overrideEnableLiv if overrideEnableLiveOutput != nil { enableLiveOutput = *overrideEnableLiveOutput } - outBytes, err := helm.runner.Execute(helm.helmBinary, cmdargs, env, enableLiveOutput) + outBytes, err := runner.Execute(helm.helmBinary, cmdargs, env, enableLiveOutput) return outBytes, err } @@ -1238,7 +1267,7 @@ func (helm *execer) execStdIn(args []string, env map[string]string, stdin io.Rea } cmd := fmt.Sprintf("exec: %s %s", helm.helmBinary, strings.Join(cmdargs, " ")) helm.logger.Debug(cmd) - outBytes, err := helm.runner.ExecuteStdIn(helm.helmBinary, cmdargs, env, stdin) + outBytes, err := markHelmRunner(helm.runner).ExecuteStdIn(helm.helmBinary, cmdargs, env, stdin) return outBytes, err } @@ -1359,7 +1388,7 @@ func (helm *execer) ShowChart(chartPath string) (chart.Metadata, error) { // metadata.Version to obtain the resolved version. func (helm *execer) ShowChartWithFlags(chartPath string, flags ...string) (chart.Metadata, error) { helmArgs := append([]string{"show", "chart", chartPath}, flags...) - out, err := helm.exec(helmArgs, map[string]string{}, nil) + out, err := helm.exec(helmArgs, map[string]string{}) if err != nil { return chart.Metadata{}, err } diff --git a/pkg/helmexec/exec_test.go b/pkg/helmexec/exec_test.go index b9ff4b02..40ef6962 100644 --- a/pkg/helmexec/exec_test.go +++ b/pkg/helmexec/exec_test.go @@ -1182,7 +1182,7 @@ func Test_exec(t *testing.T) { t.Errorf("unexpected error: %v", err) } env := map[string]string{} - _, err = helm.exec([]string{"version"}, env, nil) + _, err = helm.exec([]string{"version"}, env) expected := `exec: helm version ` if err != nil { @@ -1196,7 +1196,7 @@ func Test_exec(t *testing.T) { if err != nil { t.Errorf("unexpected error: %v", err) } - ret, _ := helm.exec([]string{"diff"}, env, nil) + ret, _ := helm.exec([]string{"diff"}, env) if len(ret) != 0 { t.Error("helmexec.exec() - expected empty return value") } @@ -1206,7 +1206,7 @@ func Test_exec(t *testing.T) { if err != nil { t.Errorf("unexpected error: %v", err) } - _, err = helm.exec([]string{"diff", "release", "chart", "--timeout 10", "--wait", "--wait-for-jobs"}, env, nil) + _, err = helm.exec([]string{"diff", "release", "chart", "--timeout 10", "--wait", "--wait-for-jobs"}, env) expected = `exec: helm --kubeconfig config --kube-context dev diff release chart --timeout 10 --wait --wait-for-jobs ` if err != nil { @@ -1217,7 +1217,7 @@ func Test_exec(t *testing.T) { } buffer.Reset() - _, err = helm.exec([]string{"version"}, env, nil) + _, err = helm.exec([]string{"version"}, env) expected = `exec: helm --kubeconfig config --kube-context dev version ` if err != nil { @@ -1229,7 +1229,7 @@ func Test_exec(t *testing.T) { buffer.Reset() helm.SetExtraArgs("foo") - _, err = helm.exec([]string{"version"}, env, nil) + _, err = helm.exec([]string{"version"}, env) expected = `exec: helm --kubeconfig config --kube-context dev version foo ` if err != nil { @@ -1245,7 +1245,7 @@ func Test_exec(t *testing.T) { t.Errorf("unexpected error: %v", err) } helm.SetHelmBinary("overwritten") - _, err = helm.exec([]string{"version"}, env, nil) + _, err = helm.exec([]string{"version"}, env) expected = `exec: overwritten version ` if err != nil { diff --git a/pkg/helmexec/exit_error.go b/pkg/helmexec/exit_error.go index 33e7b4d0..21aa37b1 100644 --- a/pkg/helmexec/exit_error.go +++ b/pkg/helmexec/exit_error.go @@ -11,10 +11,13 @@ func newExitError(path string, args []string, exitStatus int, err error, stderr, out += fmt.Sprintf("PATH:\n%s", Indent(path, " ")) out += "\n\nARGS:" - for i, a := range args { - if i > 0 && strings.HasPrefix(args[i-1], "--set") && stripArgsValuesOnExitError { - a = "*** STRIP ***" - } + // The legacy profile is byte-identical to the historical inline logic; + // the goldens in exit_error_test.go pin its exact output. + redacted := RedactArgs(args, RedactionLegacy) + if !stripArgsValuesOnExitError { + redacted = args + } + for i, a := range redacted { out += fmt.Sprintf("\n%s", Indent(fmt.Sprintf("%d: %s (%d bytes)", i, a, len(a)), " ")) } diff --git a/pkg/helmexec/redact.go b/pkg/helmexec/redact.go new file mode 100644 index 00000000..6f456b8f --- /dev/null +++ b/pkg/helmexec/redact.go @@ -0,0 +1,106 @@ +package helmexec + +import "strings" + +// RedactionProfile selects how aggressively RedactArgs masks secret-bearing +// command-line arguments. +type RedactionProfile int + +const ( + // RedactionLegacy reproduces the historical exit-error behavior + // byte-for-byte: only the argument *following* a flag whose name starts + // with "--set" is masked. The goldens in exit_error_test.go pin this + // output; changing it changes observable error messages. + RedactionLegacy RedactionProfile = iota + + // RedactionStrict masks every secret-bearing argument form known today: + // in addition to the legacy behavior it covers single-argument forms + // (--set=key=value) and credential flags (--username, --password, + // --key-file). Used for telemetry span attributes; span visibility must + // be at least as redacted as error messages. + RedactionStrict +) + +// redactedArg is the placeholder written in place of secret values; its exact +// bytes are pinned by exit_error_test.go. +const redactedArg = "*** STRIP ***" + +// strictNextArgFlags are the flags whose following argument is a secret. +var strictNextArgFlags = []string{ + "--set", + "--set-string", + "--set-file", + "--set-json", + "--set-literal", + "--username", + "--password", + "--key-file", + "--kube-token", +} + +// RedactArgs returns a copy of args with secret-bearing values masked +// according to profile. The input slice is never mutated. +func RedactArgs(args []string, profile RedactionProfile) []string { + if len(args) == 0 { + return args + } + + out := make([]string, len(args)) + copy(out, args) + + for i := range out { + // The previous token must be read from the ORIGINAL slice: reading + // the progressively redacted output would let a masked value hide a + // following secret (e.g. {"--set", "--set-string", "secret"}). + var prev string + if i > 0 { + prev = args[i-1] + } + + switch profile { + case RedactionLegacy: + if strings.HasPrefix(prev, "--set") { + out[i] = redactedArg + } + case RedactionStrict: + if isStrictNextArgFlag(prev) { + out[i] = redactedArg + } else if flag := strictInlineFlag(out[i]); flag != "" { + out[i] = flag + "=" + redactedArg + } + } + } + return out +} + +func isStrictNextArgFlag(arg string) bool { + for _, flag := range strictNextArgFlags { + if arg == flag { + return true + } + } + return false +} + +// strictInlineFlag returns the flag name when arg is a single-argument secret +// form such as "--set=key=value", or "" otherwise. +func strictInlineFlag(arg string) string { + for _, flag := range strictNextArgFlags { + if strings.HasPrefix(arg, flag+"=") { + return flag + } + } + return "" +} + +func equalArgs(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/pkg/helmexec/redact_test.go b/pkg/helmexec/redact_test.go new file mode 100644 index 00000000..b3fc63c0 --- /dev/null +++ b/pkg/helmexec/redact_test.go @@ -0,0 +1,241 @@ +package helmexec + +import ( + "context" + "fmt" + "reflect" + "testing" + + "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/attribute" +) + +func TestRedactArgsLegacy(t *testing.T) { + tests := []struct { + name string + args []string + want []string + }{ + { + name: "two-argument --set is masked", + args: []string{"upgrade", "--set", "secret=1"}, + want: []string{"upgrade", "--set", redactedArg}, + }, + { + name: "--set prefix covers --set-string/--set-file/--set-json", + args: []string{"--set-string", "a=b", "--set-file", "f", "--set-json", "{}"}, + want: []string{"--set-string", redactedArg, "--set-file", redactedArg, "--set-json", redactedArg}, + }, + { + name: "single-argument --set=k=v is NOT masked (documented legacy gap)", + args: []string{"--set=secret=1"}, + want: []string{"--set=secret=1"}, + }, + { + name: "credential flags are NOT masked (documented legacy gap)", + args: []string{"--username", "bob", "--password", "hunter2"}, + want: []string{"--username", "bob", "--password", "hunter2"}, + }, + { + name: "first argument is never masked", + args: []string{"--set"}, + want: []string{"--set"}, + }, + { + name: "adjacent secret flags do not leak (previous token read from original)", + args: []string{"--set", "--set-string", "secret"}, + want: []string{"--set", redactedArg, redactedArg}, + }, + { + name: "strict: adjacent secret flags do not leak", + args: []string{"--set", "--set", "a=b"}, + want: []string{"--set", redactedArg, redactedArg}, + }, + { + name: "empty args", + args: []string{}, + want: []string{}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := RedactArgs(tt.args, RedactionLegacy) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestRedactArgsStrict(t *testing.T) { + tests := []struct { + name string + args []string + want []string + }{ + { + name: "legacy two-argument forms remain masked", + args: []string{"upgrade", "--set", "secret=1", "--set-string", "s=2"}, + want: []string{"upgrade", "--set", redactedArg, "--set-string", redactedArg}, + }, + { + name: "single-argument --set=k=v is masked", + args: []string{"--set=secret=1", "--set-string=a=b", "--set-json={}"}, + want: []string{"--set=" + redactedArg, "--set-string=" + redactedArg, "--set-json=" + redactedArg}, + }, + { + name: "credential flags are masked", + args: []string{"registry", "login", "--username", "bob", "--password", "hunter2"}, + want: []string{"registry", "login", "--username", redactedArg, "--password", redactedArg}, + }, + { + name: "key-file is masked", + args: []string{"--key-file", "/etc/secrets/key"}, + want: []string{"--key-file", redactedArg}, + }, + { + name: "password-stdin has no value to mask", + args: []string{"--password-stdin"}, + want: []string{"--password-stdin"}, + }, + { + name: "kube-token is masked (two-argument form)", + args: []string{"--kube-context", "ctx", "upgrade", "--kube-token", "eyJhbGciOi..."}, + want: []string{"--kube-context", "ctx", "upgrade", "--kube-token", redactedArg}, + }, + { + name: "kube-token is masked (inline form)", + args: []string{"upgrade", "--kube-token=eyJhbGciOi..."}, + want: []string{"upgrade", "--kube-token=" + redactedArg}, + }, + { + name: "benign flags untouched", + args: []string{"upgrade", "--install", "envoy", "./chart", "--namespace", "ingress", "--reset-values"}, + want: []string{"upgrade", "--install", "envoy", "./chart", "--namespace", "ingress", "--reset-values"}, + }, + { + name: "prefix lookalikes untouched", + args: []string{"--settlement", "x", "--usernames", "y"}, + want: []string{"--settlement", "x", "--usernames", "y"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := RedactArgs(tt.args, RedactionStrict) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestRedactArgsDoesNotMutateInput(t *testing.T) { + orig := []string{"--set", "secret=1", "--set=x=y"} + before := reflect.ValueOf(orig).Pointer() + + _ = RedactArgs(orig, RedactionStrict) + + assert.Equal(t, []string{"--set", "secret=1", "--set=x=y"}, orig, "input slice must not be mutated") + assert.Equal(t, before, reflect.ValueOf(orig).Pointer(), "input backing array must be reused by the caller, not modified") +} + +func TestClassifyExec(t *testing.T) { + tests := []struct { + name string + cmd string + args []string + wantName string + wantAttrVals map[string][]string + ctxMarked bool + }{ + { + name: "helm invocation", + cmd: "/usr/local/bin/helm", + args: []string{"upgrade", "--install", "envoy", "./chart"}, + wantName: "helm.exec", + wantAttrVals: map[string][]string{ + "exec.command": {"helm"}, + "helm.subcommand": {"upgrade"}, + }, + }, + { + name: "helm with leading flags: subcommand is first positional", + cmd: "helm", + args: []string{"--kube-context", "ctx", "repo", "update"}, + wantName: "helm.exec", + wantAttrVals: map[string][]string{ + "helm.subcommand": {"repo"}, + }, + }, + { + name: "non-helm binary", + cmd: "./scripts/migrate.sh", + args: []string{"--verbose"}, + wantName: "os.exec", + wantAttrVals: map[string][]string{ + "exec.command": {"migrate.sh"}, + }, + }, + { + name: "wrapper helm binary classified via marker, not name", + cmd: "/opt/bin/custom-wrapper", + args: []string{"upgrade", "demo", "./chart"}, + wantName: "helm.exec", + wantAttrVals: map[string][]string{ + "exec.command": {"custom-wrapper"}, + "helm.subcommand": {"upgrade"}, + }, + ctxMarked: true, + }, + { + name: "secret args are redacted and flagged", + cmd: "helm", + args: []string{"upgrade", "--set", "pw=1", "--set=pw2=2"}, + wantName: "helm.exec", + wantAttrVals: map[string][]string{ + "exec.args": {"upgrade", "--set", redactedArg, "--set=" + redactedArg}, + "exec.redacted": {"true"}, + }, + }, + { + name: "credentials embedded in positional URLs are masked", + cmd: "helm", + args: []string{"repo", "add", "name", "https://user:token@charts.example.com/repo"}, + wantName: "helm.exec", + wantAttrVals: map[string][]string{ + "exec.args": {"repo", "add", "name", "https://xxxxx@charts.example.com/repo"}, + "exec.redacted": {"true"}, + }, + }, + { + name: "plain URLs are untouched", + cmd: "helm", + args: []string{"upgrade", "demo", "https://charts.example.com/repo/chart"}, + wantName: "helm.exec", + wantAttrVals: map[string][]string{ + "exec.args": {"upgrade", "demo", "https://charts.example.com/repo/chart"}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + if tt.ctxMarked { + ctx = context.WithValue(ctx, helmExecMarker{}, true) + } + name, attrs := classifyExec(ctx, tt.cmd, tt.args) + + assert.Equal(t, tt.wantName, name) + got := map[string][]string{} + for _, kv := range attrs { + switch kv.Value.Type() { + case attribute.STRING: + got[string(kv.Key)] = []string{kv.Value.AsString()} + case attribute.STRINGSLICE: + got[string(kv.Key)] = kv.Value.AsStringSlice() + case attribute.BOOL: + got[string(kv.Key)] = []string{fmt.Sprintf("%t", kv.Value.AsBool())} + } + } + for key, want := range tt.wantAttrVals { + assert.Equal(t, want, got[key], "attribute %s", key) + } + }) + } +} diff --git a/pkg/helmexec/refredact.go b/pkg/helmexec/refredact.go new file mode 100644 index 00000000..e2b3c98a --- /dev/null +++ b/pkg/helmexec/refredact.go @@ -0,0 +1,91 @@ +package helmexec + +import ( + "net/url" + "strings" +) + +// redactedRefValue is the placeholder substituted for credential-bearing +// userinfo and query-parameter values in telemetry attributes. +const redactedRefValue = "xxxxx" + +// sensitiveQueryKeys reports whether a query-parameter key is considered +// credential-bearing, mirroring the heuristic used by pkg/remote for cache +// keys (token/password/secret/key/signature substrings). +func sensitiveQueryKey(key string) bool { + lk := strings.ToLower(key) + return strings.Contains(lk, "token") || strings.Contains(lk, "password") || + strings.Contains(lk, "secret") || strings.Contains(lk, "key") || + strings.Contains(lk, "signature") +} + +// RedactedRef sanitizes a remote reference (state-file path, chart reference, +// repository URL) for telemetry export: go-getter forced-form prefixes +// (git::, s3::, …) are preserved, URL userinfo is masked, and +// credential-bearing query parameters have their values replaced. Non-URL +// strings are returned unchanged. It is deliberately at least as strict as +// the log-time RedactedURL. +// maxForcedFormPrefix bounds the length of a recognized go-getter forced-form +// prefix ("git::", "s3::", "hg::" …); longer "::"-containing prefixes are +// treated as part of an opaque reference instead. +const maxForcedFormPrefix = 16 + +func RedactedRef(ref string) string { + force := "" + rest := ref + // A go-getter forced form looks like "git::https://…": a short + // alphanumeric prefix followed by "::" at the start of the reference. + if i := strings.Index(rest, "::"); i > 0 && i <= maxForcedFormPrefix && isAlphanumericPrefix(rest[:i]) { + force = rest[:i+len("::")] + rest = rest[i+len("::"):] + } + + u, err := url.Parse(rest) + if err != nil { + if strings.Contains(rest, "://") { + // URL-like but malformed (e.g. a bad percent escape): fail + // closed — export a fully redacted value rather than risk + // leaking userinfo or query credentials. + return force + redactedRefValue + } + return ref + } + if u.Scheme == "" || u.Host == "" { + // Not a URL (e.g. "./charts/demo"); leave it alone. + return ref + } + + // Mask the whole userinfo: usernames are as likely to carry tokens as + // passwords are (e.g. "https://x-access-token@host"). + if u.User != nil { + u.User = url.User(redactedRefValue) + } + + if u.RawQuery != "" { + q, err := url.ParseQuery(u.RawQuery) + if err != nil { + // Malformed query: fail closed by dropping it entirely. + u.RawQuery = "" + } else { + for key, values := range q { + if sensitiveQueryKey(key) { + for i := range values { + values[i] = redactedRefValue + } + } + } + u.RawQuery = q.Encode() + } + } + + return force + u.String() +} + +func isAlphanumericPrefix(s string) bool { + for _, r := range s { + if !(r >= 'a' && r <= 'z' || r >= 'A' && r <= 'Z' || r >= '0' && r <= '9') { + return false + } + } + return len(s) > 0 +} diff --git a/pkg/helmexec/refredact_test.go b/pkg/helmexec/refredact_test.go new file mode 100644 index 00000000..6e661f23 --- /dev/null +++ b/pkg/helmexec/refredact_test.go @@ -0,0 +1,66 @@ +package helmexec + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRedactedRef(t *testing.T) { + tests := []struct { + name string + ref string + want string + }{ + { + name: "plain chart path unchanged", + ref: "./charts/demo", + want: "./charts/demo", + }, + { + name: "https userinfo fully masked (username may carry the token)", + ref: "https://user:token@charts.example.com/repo", + want: "https://xxxxx@charts.example.com/repo", + }, + { + name: "go-getter forced form preserved and sanitized", + ref: "git::https://x-access-token@github.com/org/repo.git//helmfile?ref=v1", + want: "git::https://xxxxx@github.com/org/repo.git//helmfile?ref=v1", + }, + { + name: "s3 query credentials masked", + ref: "s3::https://s3.amazonaws.com/bucket/helmfile?aws_access_key_id=AKIA&aws_secret_access_key=zzz®ion=us-east-1", + want: "s3::https://s3.amazonaws.com/bucket/helmfile?aws_access_key_id=xxxxx&aws_secret_access_key=xxxxx®ion=us-east-1", + }, + { + name: "generic token query param masked", + ref: "https://example.com/file?token=abc&path=ok", + want: "https://example.com/file?path=ok&token=xxxxx", + }, + { + name: "non-credential query untouched", + ref: "https://example.com/chart?ref=main&verify=true", + want: "https://example.com/chart?ref=main&verify=true", + }, + { + name: "URL without scheme or host returned unchanged", + ref: "git@github.com:org/repo.git", + want: "git@github.com:org/repo.git", + }, + { + name: "malformed URL-like ref fails fully redacted", + ref: "https://user:token@host/%zz", + want: redactedRefValue, + }, + { + name: "malformed query is dropped entirely", + ref: "https://host/file?token=secret%zz&ok=1", + want: "https://host/file", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, RedactedRef(tt.ref)) + }) + } +} diff --git a/pkg/helmexec/runner.go b/pkg/helmexec/runner.go index 8c0b374b..6fdb59e4 100644 --- a/pkg/helmexec/runner.go +++ b/pkg/helmexec/runner.go @@ -12,6 +12,7 @@ import ( "path/filepath" "strings" "syscall" + "time" "go.uber.org/zap" @@ -34,28 +35,51 @@ type ShellRunner struct { Ctx context.Context } -// Execute a shell command +// Execute a shell command. The whole execution is wrapped in one span +// (helm.exec/os.exec, see span.go); span overhead is nil when telemetry is +// disabled. func (shell ShellRunner) Execute(cmd string, args []string, env map[string]string, enableLiveOutput bool) ([]byte, error) { + ctx, span, isHelm := startExecSpan(shell.Ctx, cmd, args) + defer span.End() + + start := time.Now() + out, err := shell.run(ctx, cmd, args, env, enableLiveOutput) + finishExecSpan(span, isHelm, args, start, err) + return out, err +} + +func (shell ShellRunner) run(ctx context.Context, cmd string, args []string, env map[string]string, enableLiveOutput bool) ([]byte, error) { preparedCmd := exec.Command(cmd, args...) preparedCmd.Dir = shell.Dir preparedCmd.Env = mergeEnv(os.Environ(), env) if !enableLiveOutput { - return Output(shell.Ctx, preparedCmd, shell.StripArgsValuesOnExitError, &logWriterGenerator{ + return Output(ctx, preparedCmd, shell.StripArgsValuesOnExitError, &logWriterGenerator{ log: shell.Logger, }) } else { - return LiveOutput(shell.Ctx, preparedCmd, shell.StripArgsValuesOnExitError, os.Stdout) + return LiveOutput(ctx, preparedCmd, shell.StripArgsValuesOnExitError, os.Stdout) } } -// Execute a shell command +// Execute a shell command with the given stdin; wrapped in an exec span like +// Execute. func (shell ShellRunner) ExecuteStdIn(cmd string, args []string, env map[string]string, stdin io.Reader) ([]byte, error) { + ctx, span, isHelm := startExecSpan(shell.Ctx, cmd, args) + defer span.End() + + start := time.Now() + out, err := shell.runStdIn(ctx, cmd, args, env, stdin) + finishExecSpan(span, isHelm, args, start, err) + return out, err +} + +func (shell ShellRunner) runStdIn(ctx context.Context, cmd string, args []string, env map[string]string, stdin io.Reader) ([]byte, error) { preparedCmd := exec.Command(cmd, args...) preparedCmd.Dir = shell.Dir preparedCmd.Env = mergeEnv(os.Environ(), env) preparedCmd.Stdin = stdin - return Output(shell.Ctx, preparedCmd, shell.StripArgsValuesOnExitError, &logWriterGenerator{ + return Output(ctx, preparedCmd, shell.StripArgsValuesOnExitError, &logWriterGenerator{ log: shell.Logger, }) } diff --git a/pkg/helmexec/span.go b/pkg/helmexec/span.go new file mode 100644 index 00000000..9414b8d1 --- /dev/null +++ b/pkg/helmexec/span.go @@ -0,0 +1,168 @@ +package helmexec + +import ( + "context" + "errors" + "path/filepath" + "strings" + "time" + + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/codes" + "go.opentelemetry.io/otel/trace" + + "github.com/helmfile/helmfile/pkg/telemetry" +) + +// startExecSpan starts one span per external process started by helmfile +// (helm invocations, hooks, helmfile plugins). It is effectively free when +// telemetry is disabled: telemetry.Tracer then returns the OTel no-op tracer. +// The returned context derives from ctx (or Background when nil) and may be +// used for the subprocess itself without changing cancellation semantics. +// markHelmExec returns ctx (or Background when nil) stamped with the +// helm-invocation marker used for span classification. +func markHelmExec(ctx context.Context) context.Context { + if ctx == nil { + ctx = context.Background() + } + return context.WithValue(ctx, helmExecMarker{}, true) +} + +// withRunnerCtx returns a runner (value or pointer ShellRunner form) whose +// context is transformed by f; non-ShellRunner runners pass through +// unchanged. ShellRunner has value receivers, so both forms satisfy Runner. +func withRunnerCtx(runner Runner, f func(context.Context) context.Context) Runner { + switch shell := runner.(type) { + case *ShellRunner: + clone := *shell + clone.Ctx = f(clone.Ctx) + return &clone + case ShellRunner: + clone := shell + clone.Ctx = f(clone.Ctx) + return clone + default: + return runner + } +} + +// markHelmRunner returns a runner whose context carries the helm-invocation +// marker, so span classification and the helm duration metric do not rely on +// the executable basename. +func markHelmRunner(runner Runner) Runner { + return withRunnerCtx(runner, markHelmExec) +} + +// spanAttachedContext returns a context that keeps runnerCtx's cancellation +// chain but carries the span from spanCtx, so the subprocess span nests under +// the caller's span while the subprocess itself stays governed by the +// runner's own context (e.g. the kubedog safety valve). A nil runnerCtx falls +// back to spanCtx. +func spanAttachedContext(runnerCtx, spanCtx context.Context) context.Context { + if runnerCtx == nil { + return spanCtx + } + return trace.ContextWithSpan(runnerCtx, trace.SpanFromContext(spanCtx)) +} + +// helmExecMarker marks contexts of invocations made through the execer +// funnel, so span classification is authoritative even for wrapper binaries +// whose name does not start with "helm". +type helmExecMarker struct{} + +func startExecSpan(ctx context.Context, cmd string, args []string) (context.Context, trace.Span, bool) { + if ctx == nil { + ctx = context.Background() + } + + name, attrs := classifyExec(ctx, cmd, args) + ctx, span := telemetry.Tracer(telemetry.ScopeHelm).Start(ctx, name, trace.WithAttributes(attrs...)) + return ctx, span, name == helmExecSpanName +} + +// classifyExec builds the span name and attributes for one subprocess. +// Secret-bearing arguments are always redacted with the strict profile: span +// visibility must be at least as redacted as error messages (see redact.go). +// helmExecSpanName is the single source of truth for what counts as a helm +// invocation: the span name drives both classification and the helm duration +// metric gate. +const helmExecSpanName = "helm.exec" + +func classifyExec(ctx context.Context, cmd string, args []string) (string, []attribute.KeyValue) { + base := filepath.Base(cmd) + isHelm := isHelmBinary(base) + if marked, ok := ctx.Value(helmExecMarker{}).(bool); ok { + isHelm = marked + } + attrs := []attribute.KeyValue{ + attribute.String("exec.command", base), + } + + redacted := RedactArgs(args, RedactionStrict) + for i, a := range redacted { + // Positional arguments can be chart/repository URLs with embedded + // credentials (AddRepo, RegistryLogin, OCI and go-getter refs). + redacted[i] = RedactedRef(a) + } + attrs = append(attrs, attribute.StringSlice("exec.args", redacted)) + if !equalArgs(args, redacted) { + attrs = append(attrs, attribute.Bool("exec.redacted", true)) + } + + if isHelm { + if sub := helmSubcommand(args); sub != "" { + attrs = append(attrs, attribute.String("helm.subcommand", sub)) + } + return helmExecSpanName, attrs + } + return "os.exec", attrs +} + +// isHelmBinary is the FALLBACK classifier: a base name starting with "helm" +// ("helm", "helm3", "helm-dev") for processes started outside the execer +// funnels (e.g. the version probe in helmexec.New). Invocations through the +// funnels are classified authoritatively by the helmExecMarker, so wrapper +// --helm-binary names classify correctly too. +func isHelmBinary(base string) bool { + return strings.HasPrefix(base, "helm") +} + +// helmSubcommand returns the best-effort helm subcommand ("upgrade", +// "repo", ...) from raw args. Bare flags are assumed to consume the next +// argument as their value (true for the global --kube-context/--kubeconfig +// helmfile prepends); a rare bare boolean flag before the subcommand can +// misattribute the value — cosmetic only. +func helmSubcommand(args []string) string { + skipValue := false + for _, a := range args { + if skipValue { + skipValue = false + continue + } + if strings.HasPrefix(a, "-") { + skipValue = !strings.Contains(a, "=") + continue + } + return a + } + return "" +} + +// finishExecSpan records a finished process's outcome on its span and, for +// helm invocations (as classified by startExecSpan, wrapper binaries +// included), the helmfile.helm.exec.duration metric. +func finishExecSpan(span trace.Span, isHelm bool, args []string, start time.Time, err error) { + if isHelm { + telemetry.RecordHelmExecDuration(time.Since(start).Seconds(), helmSubcommand(args), err == nil) + } + if err == nil { + return + } + var exitErr ExitError + if errors.As(err, &exitErr) { + span.SetAttributes(attribute.Int("exec.exit_code", exitErr.ExitStatus())) + } + // The raw error may embed command arguments and subprocess output; keep + // the span description generic (the exit code is an attribute). + span.SetStatus(codes.Error, "command failed") +} diff --git a/pkg/helmexec/span_test.go b/pkg/helmexec/span_test.go new file mode 100644 index 00000000..28a08bef --- /dev/null +++ b/pkg/helmexec/span_test.go @@ -0,0 +1,166 @@ +package helmexec + +import ( + "context" + "io" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/trace" + "go.opentelemetry.io/otel/trace/noop" + commonpb "go.opentelemetry.io/proto/otlp/common/v1" + v1 "go.opentelemetry.io/proto/otlp/trace/v1" + + "github.com/helmfile/helmfile/pkg/telemetry" + "github.com/helmfile/helmfile/pkg/telemetry/otlptest" +) + +// TestShellRunnerExecSpansExported drives ShellRunner.Execute through a real +// OTLP export and asserts names, attributes, error status, and parent linkage +// to the command span (i.e. the runner spans nest, never orphan). +func TestShellRunnerExecSpansExported(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("uses the unix true/false binaries") + } + + rec := otlptest.NewRecorder(t) + otlptest.SetupTelemetry(t, rec, "helmfile test") + + shell := ShellRunner{ + Logger: NewLogger(io.Discard, "warn"), + Ctx: telemetry.CommandContext(), + } + + out, err := shell.Execute("true", nil, nil, false) + require.NoError(t, err) + assert.Empty(t, out) + + // A failing command must surface its exit code and error status. + _, err = shell.Execute("false", nil, nil, false) + var exitErr ExitError + require.ErrorAs(t, err, &exitErr) + + // Secret-bearing arguments must be redacted on the span. + _, err = shell.Execute("true", []string{"--set", "secret=1", "--set=also=2"}, nil, false) + require.NoError(t, err) + + otlptest.ShutdownTelemetry(t) + + spans := rec.Spans(t) + root := otlptest.FindSpanWhere(t, spans, func(s *v1.Span) bool { return s.Name == "helmfile test" }, "command span") + + success := otlptest.FindSpanWhere(t, spans, func(s *v1.Span) bool { + cmd, ok := otlptest.AttrString(s, "exec.command") + return s.Name == "os.exec" && ok && cmd == "true" && !otlptest.HasAttr(s, "exec.redacted") + }, "success os.exec span") + if success.Status != nil { + assert.Equal(t, v1.Status_STATUS_CODE_UNSET, success.Status.Code, "success span must not be marked error") + } + + failure := otlptest.FindSpanWhere(t, spans, func(s *v1.Span) bool { + cmd, ok := otlptest.AttrString(s, "exec.command") + return ok && cmd == "false" + }, "failure os.exec span") + require.NotNil(t, failure.Status) + assert.Equal(t, v1.Status_STATUS_CODE_ERROR, failure.Status.Code) + assert.EqualValues(t, 1, spanAttr(t, failure, "exec.exit_code").Value.GetIntValue()) + + redactedSpan := otlptest.FindSpanWhere(t, spans, func(s *v1.Span) bool { return otlptest.HasAttr(s, "exec.redacted") }, "redacted os.exec span") + assert.Equal(t, []string{"--set", redactedArg, "--set=" + redactedArg}, spanAttrStrings(t, redactedSpan, "exec.args")) + + // Every os.exec span must be a child of the command span, proving the + // runner-span nesting (no orphan traces). + for _, s := range spans { + if s.Name == "os.exec" { + assert.Equal(t, root.TraceId, s.TraceId, "os.exec span must join the command trace") + assert.Equal(t, root.SpanId, s.ParentSpanId, "os.exec span must nest under the command span") + } + } +} + +func spanAttr(t *testing.T, span *v1.Span, key string) *commonpb.KeyValue { + t.Helper() + for _, kv := range span.Attributes { + if kv.Key == key { + return kv + } + } + t.Fatalf("attribute %q not found on span %q", key, span.Name) + return nil +} + +func spanAttrStrings(t *testing.T, span *v1.Span, key string) []string { + t.Helper() + values := spanAttr(t, span, key).Value.GetArrayValue().GetValues() + out := make([]string, 0, len(values)) + for _, v := range values { + out = append(out, v.GetStringValue()) + } + return out +} + +// TestSpanAttachedContext pins the per-call context merge that lets helm +// subprocess spans nest under per-release spans WITHOUT overriding the +// runner's own cancellation context (the kubedog safety valve). +func TestSpanAttachedContext(t *testing.T) { + runnerCtx, valveCancel := context.WithCancel(context.Background()) + defer valveCancel() + + spanCtx, span := noop.NewTracerProvider().Tracer("test").Start(context.Background(), "release") + + merged := spanAttachedContext(runnerCtx, spanCtx) + + // The merged context still cancels with the runner's context. + assert.Equal(t, runnerCtx.Done(), merged.Done(), "cancellation authority must stay with the runner context") + + // ...and carries the caller's span for nesting. + assert.Equal(t, span, trace.SpanFromContext(merged), "the caller's span must be attached") + + // A typed nil runner context (distinct from the nil literal, per + // staticcheck) falls back to the span context. + var nilRunnerCtx context.Context + assert.Equal(t, spanCtx, spanAttachedContext(nilRunnerCtx, spanCtx)) +} + +// TestMarkHelmRunnerCoversBothFunnels pins that the shared marker helper — +// used by BOTH execWithRunner and execStdIn — stamps value and pointer +// runners alike, so wrapper binaries stay classified as helm operations on +// stdin-based invocations (registry login, repo add) too. +func TestMarkHelmRunnerCoversBothFunnels(t *testing.T) { + ptr := &ShellRunner{} + if v, ok := markHelmRunner(ptr).(*ShellRunner).Ctx.Value(helmExecMarker{}).(bool); !ok || !v { + t.Error("pointer runner must carry the helm marker") + } + + val := ShellRunner{} + markedValue, ok := markHelmRunner(val).(ShellRunner) + if !ok { + t.Fatal("value runner must stay a value runner") + } + if v, ok := markedValue.Ctx.Value(helmExecMarker{}).(bool); !ok || !v { + t.Error("value runner must carry the helm marker") + } + + fake := &mockRunner{} + assert.Same(t, fake, markHelmRunner(fake), "non-ShellRunner runners pass through unchanged") +} + +// TestValueRunnerClassification pins that ShellRunner values (which satisfy +// Runner via value receivers) receive the helm marker and span attachment +// exactly like pointer runners. +func TestValueRunnerClassification(t *testing.T) { + spanCtx, span := noop.NewTracerProvider().Tracer("test").Start(context.Background(), "release") + + // marker stamping + valueRunner := ShellRunner{} + marked := markHelmExec(valueRunner.Ctx) + if v, ok := marked.Value(helmExecMarker{}).(bool); !ok || !v { + t.Fatal("value runner ctx must carry the helm marker") + } + + // span attachment via the same path execWithContext uses for values + merged := spanAttachedContext(valueRunner.Ctx, spanCtx) + assert.Equal(t, span, trace.SpanFromContext(merged)) +} diff --git a/pkg/state/span.go b/pkg/state/span.go new file mode 100644 index 00000000..912bac6d --- /dev/null +++ b/pkg/state/span.go @@ -0,0 +1,131 @@ +package state + +import ( + gocontext "context" + "sort" + "time" + + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/codes" + "go.opentelemetry.io/otel/trace" + + "github.com/helmfile/helmfile/pkg/helmexec" + "github.com/helmfile/helmfile/pkg/telemetry" +) + +// traceOnlyContext returns a context carrying trace context — the command +// span, or the given parent (typically a per-release span) — while never +// propagating cancellation. The historically detached paths (kubedog +// tracking, hook execution) must keep their cancellation semantics, so only +// trace context is bridged (see docs/proposals/otel-tracing.md §4.4). With +// tracing disabled it is indistinguishable from Background. +func traceOnlyContext(parent ...gocontext.Context) gocontext.Context { + ctx := telemetry.CommandContext() + if len(parent) > 0 && parent[0] != nil { + ctx = parent[0] + } + return gocontext.WithoutCancel(ctx) +} + +// SetTraceContext sets the context used as the parent of per-release spans +// (pkg/app sets it to the helmfile.load span context right after loading a +// state file). A nil context keeps spans parented at Background — with +// tracing disabled, span starts are no-ops anyway. +func (st *HelmState) SetTraceContext(ctx gocontext.Context) { + st.traceCtx = ctx +} + +func (st *HelmState) releaseSpanParent() gocontext.Context { + if st.traceCtx != nil { + return st.traceCtx + } + return gocontext.Background() +} + +// startReleaseSpan starts one helmfile.release. span for a release, +// parented from the state's trace context. The returned context is meant to +// be stamped into the per-release helmexec.HelmContext so the release's helm +// subprocesses nest under the span. +func (st *HelmState) startReleaseSpan(verb string, release *ReleaseSpec) (gocontext.Context, trace.Span) { + attrs := []attribute.KeyValue{ + attribute.String("helmfile.release", release.Name), + attribute.String("helmfile.namespace", release.Namespace), + attribute.String("helmfile.chart", helmexec.RedactedRef(release.Chart)), + } + if release.Version != "" { + attrs = append(attrs, attribute.String("helmfile.chart_version", release.Version)) + } + if len(release.Labels) > 0 { + attrs = append(attrs, attribute.StringSlice("helmfile.labels", sortedLabelPairs(release.Labels))) + } + + ctx, span := telemetry.Tracer(telemetry.ScopeHelmfile).Start(st.releaseSpanParent(), "helmfile.release."+verb, + trace.WithAttributes(attrs...), + ) + return ctx, span +} + +// endReleaseSpan ends a release span, recording err (when non-nil) as the +// span's error status, and records the outcome on the helmfile.release.count +// and helmfile.release.duration metrics. Ending an already-ended span is a +// no-op, so it is safe to call from every exit path of a worker-loop item. +func endReleaseSpan(span trace.Span, verb string, release *ReleaseSpec, start time.Time, err error) { + if span == nil { + return + } + telemetry.RecordReleaseResult(verb, err) + telemetry.RecordReleaseDuration(time.Since(start).Seconds(), verb, err, release.Name, release.Namespace) + if err != nil { + // The raw error may embed helm command arguments and output; keep + // the span description generic. + span.SetStatus(codes.Error, "release operation failed") + } + span.End() +} + +func sortedLabelPairs(labels map[string]string) []string { + keys := make([]string, 0, len(labels)) + for k := range labels { + keys = append(keys, k) + } + sort.Strings(keys) + pairs := make([]string, 0, len(keys)) + for _, k := range keys { + pairs = append(pairs, k+"="+labels[k]) + } + return pairs +} + +// releaseErrAsError converts a *ReleaseError to error without the typed-nil +// trap: a nil *ReleaseError must become a nil error, or endReleaseSpan would +// call Error() on a nil pointer. +func releaseErrAsError(relErr *ReleaseError) error { + if relErr == nil { + return nil + } + return relErr +} + +// skipUndesired reports whether a release is disabled (not desired); callers +// whose callbacks short-circuit on !release.Desired() pass it so no span or +// metric is emitted for releases that run no operation. +func skipUndesired(release *ReleaseSpec) bool { + return !release.Desired() +} + +// doWithReleaseSpan runs do for one release under a helmfile.release. +// span, recording the returned error on the span, and hands do the span +// context so the release's helm subprocesses nest under the span. It is the +// convenience form used by the iterateOnReleases-based loops. +func (st *HelmState) doWithReleaseSpan(verb string, release ReleaseSpec, workerIndex int, skip func(*ReleaseSpec) bool, do func(gocontext.Context, ReleaseSpec, int) error) error { + if skip != nil && skip(&release) { + // Skipped releases run no operation: no span, no metric. The callback + // still runs so its short-circuit behavior is unchanged. + return do(gocontext.Background(), release, workerIndex) + } + ctx, span := st.startReleaseSpan(verb, &release) + start := time.Now() + err := do(ctx, release, workerIndex) + endReleaseSpan(span, verb, &release, start, err) + return err +} diff --git a/pkg/state/span_test.go b/pkg/state/span_test.go new file mode 100644 index 00000000..45f26b2f --- /dev/null +++ b/pkg/state/span_test.go @@ -0,0 +1,256 @@ +package state + +import ( + gocontext "context" + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/trace" + "go.opentelemetry.io/otel/trace/noop" + metricsv1 "go.opentelemetry.io/proto/otlp/metrics/v1" + v1 "go.opentelemetry.io/proto/otlp/trace/v1" + "go.uber.org/zap" + + "github.com/helmfile/helmfile/pkg/filesystem" + "github.com/helmfile/helmfile/pkg/helmexec" + "github.com/helmfile/helmfile/pkg/telemetry" + "github.com/helmfile/helmfile/pkg/telemetry/otlptest" +) + +// TestReleaseSpanExecNesting drives ReleaseStatuses through the real execer +// (with a shim binary that answers the version probe and otherwise exits 0) +// and telemetry enabled, then asserts the release span's subprocess span +// nests under it — the contract established by the +// HelmContext.Ctx/execWithContext funnel. +func TestReleaseSpanExecNesting(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("uses a unix shell shim") + } + + rec := otlptest.NewRecorder(t) + otlptest.SetupTelemetry(t, rec, "helmfile test") + st, helm := newShimState(t) + require.Empty(t, st.ReleaseStatuses(helm, 1)) + + otlptest.ShutdownTelemetry(t) + + spans := rec.Spans(t) + release := otlptest.FindSpanWhere(t, spans, func(s *v1.Span) bool { return s.Name == "helmfile.release.status" }, "release status span") + + name, ok := otlptest.AttrString(release, "helmfile.release") + require.True(t, ok) + assert.Equal(t, "demo", name) + + // The shim binary's name starts with "helm", so its spans are helm.exec. + // helmexec.New's version probe also produced one (parented at the command + // span); the release's own subprocess must be the "status" one, nested + // under the release span. + exec := otlptest.FindSpanWhere(t, spans, func(s *v1.Span) bool { + sub, ok := otlptest.AttrString(s, "helm.subcommand") + return s.Name == "helm.exec" && ok && sub == "status" + }, "release status subprocess span") + assert.Equal(t, release.TraceId, exec.TraceId, "subprocess must join the release span's trace") + assert.Equal(t, release.SpanId, exec.ParentSpanId, "subprocess must nest under the release span") + + assertMetrics(t, rec) +} + +// newShimState builds a one-release HelmState driven through the real +// execer with a shim binary that answers the version probe and otherwise +// exits 0 without side effects. Telemetry must already be set up. +func newShimState(t *testing.T) (*HelmState, helmexec.Interface) { + t.Helper() + shim := filepath.Join(t.TempDir(), "helm-shim") + require.NoError(t, os.WriteFile(shim, []byte("#!/bin/sh\ncase \"$1\" in version) echo 'v3.14.0' ;; esac\nexit 0\n"), 0o755)) + + shell := &helmexec.ShellRunner{ + Logger: zap.NewNop().Sugar(), + Ctx: telemetry.CommandContext(), + } + helm, err := helmexec.New(shim, helmexec.HelmExecOptions{}, zap.NewNop().Sugar(), "", "", shell) + require.NoError(t, err) + + return &HelmState{ + logger: zap.NewNop().Sugar(), + fs: filesystem.DefaultFileSystem(), + ReleaseSetSpec: ReleaseSetSpec{ + Releases: []ReleaseSpec{ + {Name: "demo", Namespace: "apps", Chart: "./charts/demo"}, + }, + }, + }, helm +} + +// TestReleaseDurationMetricDefaultDims pins that helmfile.release.duration is +// exported with only the bounded verb/result dimensions unless +// HELMFILE_OTEL_METRICS_PER_RELEASE is enabled. +func TestReleaseDurationMetricDefaultDims(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("uses a unix shell shim") + } + rec := otlptest.NewRecorder(t) + otlptest.SetupTelemetry(t, rec, "helmfile test") + st, helm := newShimState(t) + require.Empty(t, st.ReleaseStatuses(helm, 1)) + otlptest.ShutdownTelemetry(t) + + metrics := rec.Metrics(t) + duration := otlptest.FindMetric(t, metrics, "helmfile.release.duration") + dp := findHistogramPoint(t, duration, "verb", "status") + require.NotNil(t, dp) + assert.Positive(t, dp.GetCount()) + + seen := map[string]string{} + for _, attr := range dp.GetAttributes() { + seen[attr.GetKey()] = attr.GetValue().GetStringValue() + } + assert.Equal(t, map[string]string{"verb": "status", "result": "success"}, seen, + "release identity must NOT be attached by default (bounded cardinality)") +} + +// TestReleaseDurationMetricPerRelease pins the opt-in high-cardinality mode: +// HELMFILE_OTEL_METRICS_PER_RELEASE adds the release name and namespace. +func TestReleaseDurationMetricPerRelease(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("uses a unix shell shim") + } + rec := otlptest.NewRecorder(t) + otlptest.SetupTelemetry(t, rec, "helmfile test") + // Set AFTER SetupTelemetry, whose hermetic env reset clears it first. + t.Setenv("HELMFILE_OTEL_METRICS_PER_RELEASE", "true") + st, helm := newShimState(t) + require.Empty(t, st.ReleaseStatuses(helm, 1)) + otlptest.ShutdownTelemetry(t) + + duration := otlptest.FindMetric(t, rec.Metrics(t), "helmfile.release.duration") + dp := findHistogramPoint(t, duration, "helmfile.release", "demo") + require.NotNil(t, dp, "per-release datapoint must exist when enabled") + + seen := map[string]string{} + for _, attr := range dp.GetAttributes() { + seen[attr.GetKey()] = attr.GetValue().GetStringValue() + } + assert.Equal(t, map[string]string{ + "verb": "status", + "result": "success", + "helmfile.release": "demo", + "helmfile.namespace": "apps", + }, seen) +} + +// assertMetrics pins the two helmfile metrics recorded on this path: one +// helm.exec duration datapoint for the status subcommand, and one successful +// release.count increment for verb=status. +func assertMetrics(t *testing.T, rec *otlptest.Recorder) { + t.Helper() + + metrics := rec.Metrics(t) + + duration := otlptest.FindMetric(t, metrics, "helmfile.helm.exec.duration") + assert.Equal(t, "s", duration.GetUnit()) + dp := findHistogramPoint(t, duration, "subcommand", "status") + require.NotNil(t, dp, "duration histogram must have a subcommand=status datapoint") + assert.Positive(t, dp.GetCount(), "histogram must record at least one observation") + // Buckets must be the seconds-tuned set, not the SDK's millisecond- + // oriented defaults. + bounds := dp.GetExplicitBounds() + require.NotEmpty(t, bounds) + assert.Less(t, bounds[0], 0.01, "first bucket must resolve sub-second invocations") + assert.Contains(t, bounds, 1.0) + assert.Contains(t, bounds, 5.0) + assert.Greater(t, bounds[len(bounds)-1], 300.0, "top bucket must cover multi-minute waits") + + releaseDuration := otlptest.FindMetric(t, metrics, "helmfile.release.duration") + rdp := findHistogramPoint(t, releaseDuration, "verb", "status") + require.NotNil(t, rdp, "release duration must have a verb=status datapoint") + assert.Equal(t, "s", releaseDuration.GetUnit()) + + count := otlptest.FindMetric(t, metrics, "helmfile.release.count") + assert.Equal(t, "{release}", count.GetUnit(), "counters use curly-annotation units") + sum := sumCounter(t, count, map[string]string{"verb": "status", "result": "success"}) + assert.EqualValues(t, 1, sum, "release.count must count one successful status") + + // The instrumentation scope carries the helmfile version. + for _, sm := range rec.ScopeMetrics(t) { + if sm.Scope.GetName() == "helmfile" { + assert.Equal(t, "test", sm.Scope.GetVersion(), "scope version must be stamped") + } + } +} + +func findHistogramPoint(t *testing.T, m *metricsv1.Metric, key, value string) *metricsv1.HistogramDataPoint { + t.Helper() + for _, dp := range m.GetHistogram().GetDataPoints() { + for _, attr := range dp.GetAttributes() { + if attr.GetKey() == key && attr.GetValue().GetStringValue() == value { + return dp + } + } + } + return nil +} + +func sumCounter(t *testing.T, m *metricsv1.Metric, want map[string]string) int64 { + t.Helper() + for _, dp := range m.GetSum().GetDataPoints() { + matched := true + seen := map[string]string{} + for _, attr := range dp.GetAttributes() { + seen[attr.GetKey()] = attr.GetValue().GetStringValue() + } + for k, v := range want { + if seen[k] != v { + matched = false + break + } + } + if matched { + return dp.GetAsInt() + } + } + t.Fatalf("no %s datapoint with attributes %v", m.GetName(), want) + return 0 +} + +// TestSetTraceContext pins the parent wiring: per-release spans root at the +// trace context handed over by pkg/app (the load span), falling back to +// Background when unset. +func TestSetTraceContext(t *testing.T) { + st := &HelmState{} + assert.Equal(t, gocontext.Background(), st.releaseSpanParent(), "unset trace context must fall back to Background") + + type ctxKey struct{} + ctx := gocontext.WithValue(gocontext.Background(), ctxKey{}, "x") + st.SetTraceContext(ctx) + assert.Equal(t, ctx, st.releaseSpanParent()) +} + +func TestSkipUndesired(t *testing.T) { + assert.False(t, skipUndesired(&ReleaseSpec{}), "installed unset means desired") + + installed := false + assert.True(t, skipUndesired(&ReleaseSpec{Installed: &installed}), "installed=false must be skipped") + + installed = true + assert.False(t, skipUndesired(&ReleaseSpec{Installed: &installed}), "installed=true must run") +} + +// TestTraceOnlyContextParentsToRelease pins the trace-only bridge: with a +// parent (the release span context) hooks attach to it, stay non-cancellable, +// and fall back to the command context without one. +func TestTraceOnlyContextParentsToRelease(t *testing.T) { + noopTracer := noop.NewTracerProvider().Tracer("test") + parentCtx, parentSpan := noopTracer.Start(gocontext.Background(), "helmfile.release.sync") + + hookCtx := traceOnlyContext(parentCtx) + + assert.Equal(t, parentSpan, trace.SpanFromContext(hookCtx), "bridged context must carry the release span") + assert.Nil(t, hookCtx.Done(), "bridged context must remain non-cancellable (historical behavior)") + + fallback := traceOnlyContext() + assert.Equal(t, trace.SpanFromContext(telemetry.CommandContext()), trace.SpanFromContext(fallback), "no parent falls back to the command span") +} diff --git a/pkg/state/state.go b/pkg/state/state.go index 85a9c4ec..a2780e74 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -145,6 +145,9 @@ type HelmState struct { basePath string FilePath string + // traceCtx parents per-release spans; set via SetTraceContext (see span.go). + traceCtx gocontext.Context + ReleaseSetSpec `yaml:",inline"` logger *zap.SugaredLogger @@ -1158,9 +1161,12 @@ func (st *HelmState) DeleteReleasesForSync(affectedReleases *AffectedReleases, h func(workerIndex int) { for release := range jobQueue { var relErr *ReleaseError + itemStart := time.Now() + relCtx, relSpan := st.startReleaseSpan("delete", release) context := st.createHelmContext(release, workerIndex) + context.Ctx = relCtx - if _, err := st.triggerPresyncEvent(release, "sync"); err != nil { + if _, err := st.triggerPresyncEvent(release, "sync", relCtx); err != nil { relErr = newReleaseFailedError(release, err) } else { var args []string @@ -1173,13 +1179,13 @@ func (st *HelmState) DeleteReleasesForSync(affectedReleases *AffectedReleases, h m.Lock() start := time.Now() - if _, err := st.triggerReleaseEvent("preuninstall", nil, release, "sync"); err != nil { + if _, err := st.triggerReleaseEvent("preuninstall", nil, release, "sync", context.Ctx); err != nil { affectedReleases.DeleteFailed = append(affectedReleases.Failed, release) relErr = newReleaseFailedError(release, err) } else if err := helm.DeleteRelease(context, release.Name, deletionFlags...); err != nil { affectedReleases.DeleteFailed = append(affectedReleases.Failed, release) relErr = newReleaseFailedError(release, err) - } else if _, err := st.triggerReleaseEvent("postuninstall", nil, release, "sync"); err != nil { + } else if _, err := st.triggerReleaseEvent("postuninstall", nil, release, "sync", context.Ctx); err != nil { affectedReleases.DeleteFailed = append(affectedReleases.Failed, release) relErr = newReleaseFailedError(release, err) } else { @@ -1189,14 +1195,16 @@ func (st *HelmState) DeleteReleasesForSync(affectedReleases *AffectedReleases, h m.Unlock() } - if _, err := st.triggerPostsyncEvent(release, relErr, "sync"); err != nil { + if _, err := st.triggerPostsyncEvent(release, relErr, "sync", relCtx); err != nil { st.logger.Warnf("warn: %v\n", err) } - if _, err := st.TriggerCleanupEvent(release, "sync"); err != nil { + if _, err := st.TriggerCleanupEvent(release, "sync", relCtx); err != nil { st.logger.Warnf("warn: %v\n", err) } + endReleaseSpan(relSpan, "delete", release, itemStart, releaseErrAsError(relErr)) + if relErr == nil { results <- syncResult{} } else { @@ -1272,10 +1280,12 @@ func (st *HelmState) SyncReleases(affectedReleases *AffectedReleases, helm helme chart = normalizeChart(st.basePath, chart) } var relErr *ReleaseError + relCtx, relSpan := st.startReleaseSpan("sync", release) context := st.createHelmContext(release, workerIndex) + context.Ctx = relCtx start := time.Now() - if _, err := st.triggerPresyncEvent(release, "sync"); err != nil { + if _, err := st.triggerPresyncEvent(release, "sync", relCtx); err != nil { relErr = newReleaseFailedError(release, err) } else if !release.Desired() { installed, err := st.isReleaseInstalled(context, helm, *release) @@ -1285,13 +1295,13 @@ func (st *HelmState) SyncReleases(affectedReleases *AffectedReleases, helm helme var args []string deletionFlags := st.appendConnectionFlags(args, release) m.Lock() - if _, err := st.triggerReleaseEvent("preuninstall", nil, release, "sync"); err != nil { + if _, err := st.triggerReleaseEvent("preuninstall", nil, release, "sync", context.Ctx); err != nil { affectedReleases.Failed = append(affectedReleases.Failed, release) relErr = newReleaseFailedError(release, err) } else if err := helm.DeleteRelease(context, release.Name, deletionFlags...); err != nil { affectedReleases.Failed = append(affectedReleases.Failed, release) relErr = newReleaseFailedError(release, err) - } else if _, err := st.triggerReleaseEvent("postuninstall", nil, release, "sync"); err != nil { + } else if _, err := st.triggerReleaseEvent("postuninstall", nil, release, "sync", context.Ctx); err != nil { affectedReleases.Failed = append(affectedReleases.Failed, release) relErr = newReleaseFailedError(release, err) } else { @@ -1302,10 +1312,10 @@ func (st *HelmState) SyncReleases(affectedReleases *AffectedReleases, helm helme } else if release.UpdateStrategy == UpdateStrategyReinstallIfForbidden { relErr = st.performSyncOrReinstallOfRelease(affectedReleases, helm, context, release, chart, m, flags...) if relErr == nil { - relErr = st.trackReleaseIfEnabled(gocontext.Background(), release, helm, opts) + relErr = st.trackReleaseIfEnabled(traceOnlyContext(), release, helm, opts) } } else { - trackHandle, trackStarted := st.startBackgroundKubedogTracking(gocontext.Background(), release, helm, opts) + trackHandle, trackStarted := st.startBackgroundKubedogTracking(traceOnlyContext(), release, helm, opts) // trackHandle.Helm is a logger-scoped helm clone that // captures output to an in-memory buffer while tracking is // active. When tracking isn't running it's the original @@ -1358,7 +1368,7 @@ func (st *HelmState) SyncReleases(affectedReleases *AffectedReleases, helm helme if trackStarted { trackErr = trackHandle.Wait() } else { - trackErr = st.trackReleaseIfEnabled(gocontext.Background(), release, helm, opts) + trackErr = st.trackReleaseIfEnabled(traceOnlyContext(), release, helm, opts) } if trackErr != nil { m.Lock() @@ -1369,7 +1379,7 @@ func (st *HelmState) SyncReleases(affectedReleases *AffectedReleases, helm helme } } - if _, err := st.triggerPostsyncEvent(release, relErr, "sync"); err != nil { + if _, err := st.triggerPostsyncEvent(release, relErr, "sync", relCtx); err != nil { if relErr == nil { relErr = newReleaseFailedError(release, err) } else { @@ -1377,7 +1387,7 @@ func (st *HelmState) SyncReleases(affectedReleases *AffectedReleases, helm helme } } - if _, err := st.TriggerCleanupEvent(release, "sync"); err != nil { + if _, err := st.TriggerCleanupEvent(release, "sync", relCtx); err != nil { if relErr == nil { relErr = newReleaseFailedError(release, err) } else { @@ -1386,6 +1396,8 @@ func (st *HelmState) SyncReleases(affectedReleases *AffectedReleases, helm helme } release.duration = time.Since(start) + endReleaseSpan(relSpan, "sync", release, start, releaseErrAsError(relErr)) + if relErr == nil { results <- syncResult{} } else { @@ -1453,13 +1465,13 @@ func (st *HelmState) performSyncOrReinstallOfRelease(affectedReleases *AffectedR args = st.appendDeleteWaitFlags(args, release) deletionFlags := st.appendConnectionFlags(args, release) m.Lock() - if _, err := st.triggerReleaseEvent("preuninstall", nil, release, "sync"); err != nil { + if _, err := st.triggerReleaseEvent("preuninstall", nil, release, "sync", context.Ctx); err != nil { affectedReleases.Failed = append(affectedReleases.Failed, release) return newReleaseFailedError(release, err) } else if err := helm.DeleteRelease(context, release.Name, deletionFlags...); err != nil { affectedReleases.Failed = append(affectedReleases.Failed, release) return newReleaseFailedError(release, err) - } else if _, err := st.triggerReleaseEvent("postuninstall", nil, release, "sync"); err != nil { + } else if _, err := st.triggerReleaseEvent("postuninstall", nil, release, "sync", context.Ctx); err != nil { affectedReleases.Failed = append(affectedReleases.Failed, release) return newReleaseFailedError(release, err) } @@ -2354,7 +2366,10 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre if sharedChartKeys[st.getChartCacheKey(release)] { releaseOpts.ForceDownload = true } + _, relSpan := st.startReleaseSpan("prepare", release) + prepareStart := time.Now() result := st.prepareChartForRelease(release, helm, dir, helmfileCommand, releaseOpts, workerIndex) + endReleaseSpan(relSpan, "prepare", release, prepareStart, result.err) if result.err != nil { // Error results returned by prepareChartForRelease may lack the // release identity. Complete it here, so that the failure can be @@ -3292,6 +3307,9 @@ func (st *HelmState) DiffReleases(helm helmexec.Interface, additionalValues []st release := prep.release buf := &bytes.Buffer{} + relCtx, relSpan := st.startReleaseSpan("diff", release) + diffStart := time.Now() + releaseSuppressDiff := suppressDiff if prep.suppressDiff { releaseSuppressDiff = true @@ -3304,17 +3322,27 @@ func (st *HelmState) DiffReleases(helm helmexec.Interface, additionalValues []st chartPath = normalizeChart(st.basePath, chartPath) } + var diffSpanErr error if prep.upgradeDueToSkippedDiff { + // Code 2 (changes detected) is an expected outcome, not a + // span/metric error. results <- diffResult{release, &ReleaseError{ReleaseSpec: release, err: nil, Code: HelmDiffExitCodeChanged}, buf} } else if err := st.withChartOperationLock(release, chartPath, func() error { - return helm.DiffRelease(st.createHelmContextWithWriter(release, buf), release.Name, chartPath, release.Namespace, releaseSuppressDiff, flags...) + diffContext := st.createHelmContextWithWriter(release, buf) + diffContext.Ctx = relCtx + return helm.DiffRelease(diffContext, release.Name, chartPath, release.Namespace, releaseSuppressDiff, flags...) }); err != nil { + var relErr *ReleaseError switch e := err.(type) { case helmexec.ExitError: // Propagate any non-zero exit status from the external command like `helm` that is failed under the hood - results <- diffResult{release, &ReleaseError{release, err, e.ExitStatus()}, buf} + relErr = &ReleaseError{release, err, e.ExitStatus()} default: - results <- diffResult{release, &ReleaseError{release, err, 0}, buf} + relErr = &ReleaseError{release, err, 0} + } + results <- diffResult{release, relErr, buf} + if relErr.Code != HelmDiffExitCodeChanged { + diffSpanErr = relErr } } else { // diff succeeded, found no changes @@ -3322,10 +3350,12 @@ func (st *HelmState) DiffReleases(helm helmexec.Interface, additionalValues []st } if triggerCleanupEvents { - if _, err := st.TriggerCleanupEvent(prep.release, "diff"); err != nil { + if _, err := st.TriggerCleanupEvent(prep.release, "diff", relCtx); err != nil { st.logger.Warnf("warn: %v\n", err) } } + + endReleaseSpan(relSpan, "diff", release, diffStart, diffSpanErr) } }, func() { @@ -3356,7 +3386,7 @@ func (st *HelmState) DiffReleases(helm helmexec.Interface, additionalValues []st } func (st *HelmState) ReleaseStatuses(helm helmexec.Interface, workerLimit int) []error { - return st.scatterGatherReleases(helm, workerLimit, func(release ReleaseSpec, workerIndex int) error { + return st.scatterGatherReleases(helm, workerLimit, "status", skipUndesired, func(ctx gocontext.Context, release ReleaseSpec, workerIndex int) error { if !release.Desired() { return nil } @@ -3369,13 +3399,15 @@ func (st *HelmState) ReleaseStatuses(helm helmexec.Interface, workerLimit int) [ } flags = st.appendConnectionFlags(flags, &release) - return helm.ReleaseStatus(st.createHelmContext(&release, workerIndex), release.Name, flags...) + statusContext := st.createHelmContext(&release, workerIndex) + statusContext.Ctx = ctx + return helm.ReleaseStatus(statusContext, release.Name, flags...) }) } // DeleteReleases wrapper for executing helm delete on the releases func (st *HelmState) DeleteReleases(affectedReleases *AffectedReleases, helm helmexec.Interface, concurrency int, purge bool, cascade string) []error { - return st.scatterGatherReleases(helm, concurrency, func(release ReleaseSpec, workerIndex int) error { + return st.scatterGatherReleases(helm, concurrency, "delete", nil, func(ctx gocontext.Context, release ReleaseSpec, workerIndex int) error { st.ApplyOverrides(&release) flags := make([]string, 0) @@ -3386,9 +3418,10 @@ func (st *HelmState) DeleteReleases(affectedReleases *AffectedReleases, helm hel flags = append(flags, "--namespace", release.Namespace) } context := st.createHelmContext(&release, workerIndex) + context.Ctx = ctx start := time.Now() - if _, err := st.triggerReleaseEvent("preuninstall", nil, &release, "delete"); err != nil { + if _, err := st.triggerReleaseEvent("preuninstall", nil, &release, "delete", ctx); err != nil { release.duration = time.Since(start) affectedReleases.DeleteFailed = append(affectedReleases.Failed, &release) @@ -3403,7 +3436,7 @@ func (st *HelmState) DeleteReleases(affectedReleases *AffectedReleases, helm hel return err } - if _, err := st.triggerReleaseEvent("postuninstall", nil, &release, "delete"); err != nil { + if _, err := st.triggerReleaseEvent("postuninstall", nil, &release, "delete", ctx); err != nil { release.duration = time.Since(start) affectedReleases.DeleteFailed = append(affectedReleases.Failed, &release) @@ -3436,7 +3469,7 @@ func (st *HelmState) TestReleases(helm helmexec.Interface, cleanup bool, timeout o(&opts) } - return st.scatterGatherReleases(helm, concurrency, func(release ReleaseSpec, workerIndex int) error { + return st.scatterGatherReleases(helm, concurrency, "test", skipUndesired, func(ctx gocontext.Context, release ReleaseSpec, workerIndex int) error { if !release.Desired() { return nil } @@ -3460,7 +3493,9 @@ func (st *HelmState) TestReleases(helm helmexec.Interface, cleanup bool, timeout flags = st.appendConnectionFlags(flags, &release) flags = st.appendChartDownloadFlags(flags, &release) - return helm.TestRelease(st.createHelmContext(&release, workerIndex), release.Name, flags...) + testContext := st.createHelmContext(&release, workerIndex) + testContext.Ctx = ctx + return helm.TestRelease(testContext, release.Name, flags...) }) } @@ -3676,7 +3711,7 @@ func (st *HelmState) TriggerGlobalCleanupEvent(helmfileCommand string, evtErr er return st.triggerGlobalReleaseEvent("cleanup", evtErr, helmfileCommand) } -func (st *HelmState) triggerGlobalReleaseEvent(evt string, evtErr error, helmfileCmd string) (bool, error) { +func (st *HelmState) triggerGlobalReleaseEvent(evt string, evtErr error, helmfileCmd string, parent ...gocontext.Context) (bool, error) { bus := &event.Bus{ Hooks: st.Hooks, StateFilePath: st.FilePath, @@ -3686,6 +3721,7 @@ func (st *HelmState) triggerGlobalReleaseEvent(evt string, evtErr error, helmfil Env: st.Env, Logger: st.logger, Fs: st.fs, + Ctx: traceOnlyContext(parent...), } data := map[string]any{ "HelmfileCommand": helmfileCmd, @@ -3697,23 +3733,23 @@ func (st *HelmState) triggerPrepareEvent(r *ReleaseSpec, helmfileCommand string) return st.triggerReleaseEvent("prepare", nil, r, helmfileCommand) } -func (st *HelmState) TriggerCleanupEvent(r *ReleaseSpec, helmfileCommand string) (bool, error) { - return st.triggerReleaseEvent("cleanup", nil, r, helmfileCommand) +func (st *HelmState) TriggerCleanupEvent(r *ReleaseSpec, helmfileCommand string, parent ...gocontext.Context) (bool, error) { + return st.triggerReleaseEvent("cleanup", nil, r, helmfileCommand, parent...) } -func (st *HelmState) triggerPresyncEvent(r *ReleaseSpec, helmfileCommand string) (bool, error) { - return st.triggerReleaseEvent("presync", nil, r, helmfileCommand) +func (st *HelmState) triggerPresyncEvent(r *ReleaseSpec, helmfileCommand string, parent ...gocontext.Context) (bool, error) { + return st.triggerReleaseEvent("presync", nil, r, helmfileCommand, parent...) } -func (st *HelmState) triggerPostsyncEvent(r *ReleaseSpec, evtErr error, helmfileCommand string) (bool, error) { - return st.triggerReleaseEvent("postsync", evtErr, r, helmfileCommand) +func (st *HelmState) triggerPostsyncEvent(r *ReleaseSpec, evtErr error, helmfileCommand string, parent ...gocontext.Context) (bool, error) { + return st.triggerReleaseEvent("postsync", evtErr, r, helmfileCommand, parent...) } func (st *HelmState) TriggerPreapplyEvent(r *ReleaseSpec, helmfileCommand string) (bool, error) { return st.triggerReleaseEvent("preapply", nil, r, helmfileCommand) } -func (st *HelmState) triggerReleaseEvent(evt string, evtErr error, r *ReleaseSpec, helmfileCmd string) (bool, error) { +func (st *HelmState) triggerReleaseEvent(evt string, evtErr error, r *ReleaseSpec, helmfileCmd string, parent ...gocontext.Context) (bool, error) { bus := &event.Bus{ Hooks: r.Hooks, StateFilePath: st.FilePath, @@ -3723,6 +3759,7 @@ func (st *HelmState) triggerReleaseEvent(evt string, evtErr error, r *ReleaseSpe Env: st.Env, Logger: st.logger, Fs: st.fs, + Ctx: traceOnlyContext(parent...), } vals := st.Values() data := map[string]any{ diff --git a/pkg/state/state_run.go b/pkg/state/state_run.go index 53646bc9..08b8600e 100644 --- a/pkg/state/state_run.go +++ b/pkg/state/state_run.go @@ -1,6 +1,7 @@ package state import ( + gocontext "context" "errors" "fmt" "sort" @@ -41,14 +42,14 @@ func (st *HelmState) scatterGather(concurrency int, items int, produceInputs fun waitGroup.Wait() } -func (st *HelmState) scatterGatherReleases(helm helmexec.Interface, concurrency int, - do func(ReleaseSpec, int) error) []error { - return st.iterateOnReleases(helm, concurrency, st.Releases, do) +func (st *HelmState) scatterGatherReleases(helm helmexec.Interface, concurrency int, verb string, + skip func(*ReleaseSpec) bool, do func(gocontext.Context, ReleaseSpec, int) error) []error { + return st.iterateOnReleases(helm, concurrency, verb, skip, st.Releases, do) } // nolint: unparam -func (st *HelmState) iterateOnReleases(helm helmexec.Interface, concurrency int, inputs []ReleaseSpec, - do func(ReleaseSpec, int) error) []error { +func (st *HelmState) iterateOnReleases(helm helmexec.Interface, concurrency int, verb string, skip func(*ReleaseSpec) bool, inputs []ReleaseSpec, + do func(gocontext.Context, ReleaseSpec, int) error) []error { var errs []error inputsSize := len(inputs) @@ -67,7 +68,7 @@ func (st *HelmState) iterateOnReleases(helm helmexec.Interface, concurrency int, }, func(id int) { for release := range releases { - err := do(release, id) + err := st.doWithReleaseSpan(verb, release, id, skip, do) st.logger.Debugf("release %q processed", release.Name) results <- result{release: release, err: err} } diff --git a/pkg/telemetry/export_test.go b/pkg/telemetry/export_test.go new file mode 100644 index 00000000..f42d1b52 --- /dev/null +++ b/pkg/telemetry/export_test.go @@ -0,0 +1,5 @@ +package telemetry + +// Reset restores the pristine, disabled telemetry state. It exists for tests +// in this package and, through this export, for tests of other packages. +func Reset() { reset() } diff --git a/pkg/telemetry/exporter.go b/pkg/telemetry/exporter.go new file mode 100644 index 00000000..6496f737 --- /dev/null +++ b/pkg/telemetry/exporter.go @@ -0,0 +1,205 @@ +package telemetry + +import ( + gocontext "context" + "fmt" + "os" + "strconv" + "strings" + + "go.opentelemetry.io/contrib/exporters/autoexport" + "go.opentelemetry.io/otel/propagation" + sdkmetric "go.opentelemetry.io/otel/sdk/metric" + "go.opentelemetry.io/otel/sdk/resource" + sdktrace "go.opentelemetry.io/otel/sdk/trace" + semconv "go.opentelemetry.io/otel/semconv/v1.34.0" + "go.uber.org/zap" +) + +// defaultSamplerName is the OTel default sampler for a CLI tool: always sample +// unless a remote parent (e.g. an unsampled CI trace) says otherwise. +const defaultSamplerName = "parentbased_always_on" + +// HermeticEnvVars lists every environment variable this package reads. It is +// the single source of truth for tests (which clear them all) — add new +// variables here, not in test files. +var HermeticEnvVars = []string{ + "OTEL_TRACES_EXPORTER", + "OTEL_METRICS_EXPORTER", + "OTEL_TRACES_SAMPLER", + "OTEL_TRACES_SAMPLER_ARG", + "OTEL_PROPAGATORS", + "OTEL_SDK_DISABLED", + "OTEL_SERVICE_NAME", + "OTEL_RESOURCE_ATTRIBUTES", + "HELMFILE_OTEL_METRICS_PER_RELEASE", + "TRACEPARENT", + "TRACESTATE", + "BAGGAGE", +} + +// newTracerProvider builds the SDK provider. Exporter selection is delegated +// to autoexport (OTEL_TRACES_EXPORTER: otlp | console | none; protocol and +// endpoint via OTEL_EXPORTER_OTLP_*), so helmfile maintains no +// exporter-construction code of its own. +func newTracerProvider(ctx gocontext.Context, opts Options, res *resource.Resource) (*sdktrace.TracerProvider, error) { + exporter, err := autoexport.NewSpanExporter(ctx) + if err != nil { + return nil, fmt.Errorf("constructing trace exporter from OTEL_* environment: %w", err) + } + + return sdktrace.NewTracerProvider( + sdktrace.WithBatcher(exporter), + sdktrace.WithResource(res), + sdktrace.WithSampler(samplerFromEnv(opts.Logger)), + ), nil +} + +// newProviders builds the tracer and meter providers over one shared +// resource. If the metric provider fails after the tracer provider was +// constructed, the tracer provider is shut down (bounded) so its batch +// goroutine does not outlive the failed setup. +func newProviders(ctx gocontext.Context, opts Options) (*sdktrace.TracerProvider, *sdkmetric.MeterProvider, error) { + res, err := buildResource(opts.Version) + if err != nil { + return nil, nil, fmt.Errorf("building OTel resource: %w", err) + } + + provider, err := newTracerProvider(ctx, opts, res) + if err != nil { + return nil, nil, err + } + + meters, err := newMeterProvider(ctx, res) + if err != nil { + shutdownCtx, cancel := gocontext.WithTimeout(gocontext.Background(), ShutdownTimeout) + defer cancel() + _ = provider.Shutdown(shutdownCtx) + return nil, nil, err + } + + return provider, meters, nil +} + +// newMeterProvider builds the metrics provider. Reader selection is delegated +// to autoexport (OTEL_METRICS_EXPORTER: otlp | console | prometheus | none); +// the OTLP reader is a periodic reader whose interval honors +// OTEL_METRIC_EXPORT_INTERVAL (read by the SDK itself). +func newMeterProvider(ctx gocontext.Context, res *resource.Resource) (*sdkmetric.MeterProvider, error) { + reader, err := autoexport.NewMetricReader(ctx) + if err != nil { + return nil, fmt.Errorf("constructing metric reader from OTEL_* environment: %w", err) + } + + return sdkmetric.NewMeterProvider( + sdkmetric.WithReader(reader), + sdkmetric.WithResource(res), + ), nil +} + +// buildResource merges helmfile's defaults with OTEL_SERVICE_NAME / +// OTEL_RESOURCE_ATTRIBUTES overrides. resource.Merge(a, b) lets b win, so the +// environment-derived resource is merged last. +func buildResource(serviceVersion string) (*resource.Resource, error) { + defaults := resource.NewSchemaless( + semconv.ServiceName(DefaultServiceName), + semconv.ServiceVersion(serviceVersion), + ) + fromEnv, err := resource.New(gocontext.Background(), + resource.WithFromEnv(), + resource.WithTelemetrySDK(), + ) + if err != nil { + return nil, err + } + return resource.Merge(defaults, fromEnv) +} + +// samplerFromEnv parses OTEL_TRACES_SAMPLER / OTEL_TRACES_SAMPLER_ARG. Unknown +// or invalid values fall back to the default sampler with a warning — a bad +// sampler configuration must not fail the run. +func samplerFromEnv(logger *zap.SugaredLogger) sdktrace.Sampler { + name := strings.TrimSpace(os.Getenv("OTEL_TRACES_SAMPLER")) + if name == "" { + name = defaultSamplerName + } + arg := strings.TrimSpace(os.Getenv("OTEL_TRACES_SAMPLER_ARG")) + + switch name { + case "always_on": + return sdktrace.AlwaysSample() + case "always_off": + return sdktrace.NeverSample() + case "traceidratio": + return sdktrace.TraceIDRatioBased(parseSamplerRatio(arg, logger)) + case "parentbased_always_on": + return sdktrace.ParentBased(sdktrace.AlwaysSample()) + case "parentbased_always_off": + return sdktrace.ParentBased(sdktrace.NeverSample()) + case "parentbased_traceidratio": + return sdktrace.ParentBased(sdktrace.TraceIDRatioBased(parseSamplerRatio(arg, logger))) + default: + warnf(logger, "unsupported OTEL_TRACES_SAMPLER %q; falling back to %s", name, defaultSamplerName) + return sdktrace.ParentBased(sdktrace.AlwaysSample()) + } +} + +func parseSamplerRatio(arg string, logger *zap.SugaredLogger) float64 { + if arg == "" { + return 1.0 + } + ratio, err := strconv.ParseFloat(arg, 64) + if err != nil || ratio < 0 || ratio > 1 { + warnf(logger, "invalid OTEL_TRACES_SAMPLER_ARG %q; using 1.0", arg) + return 1.0 + } + return ratio +} + +// propagatorsFromEnv parses OTEL_PROPAGATORS (default "tracecontext,baggage"). +// Unsupported names are ignored with a warning rather than failing the run. +func propagatorsFromEnv(logger *zap.SugaredLogger) propagation.TextMapPropagator { + raw := os.Getenv("OTEL_PROPAGATORS") + if raw == "" { + raw = "tracecontext,baggage" + } + + var propagators []propagation.TextMapPropagator + for _, name := range strings.Split(raw, ",") { + switch strings.TrimSpace(name) { + case "tracecontext": + propagators = append(propagators, propagation.TraceContext{}) + case "baggage": + propagators = append(propagators, propagation.Baggage{}) + case "none": + return propagation.NewCompositeTextMapPropagator() + case "": + // Tolerate stray commas, e.g. "tracecontext,,baggage". + default: + warnf(logger, "unsupported OTEL_PROPAGATORS entry %q; ignoring", name) + } + } + return propagation.NewCompositeTextMapPropagator(propagators...) +} + +// sdkDisabledFromEnv reports whether OTEL_SDK_DISABLED disables the SDK. The +// OTel Go SDK core does not read this variable itself, so this wrapper honors +// the specification instead. +func sdkDisabledFromEnv() bool { + return strings.EqualFold(strings.TrimSpace(os.Getenv("OTEL_SDK_DISABLED")), "true") +} + +// propagatedEnvKeys are the W3C propagation headers that can appear as process +// environment variables; TRACEPARENT is how CI systems hand a parent context +// to child processes. +var propagatedEnvKeys = []string{"TRACEPARENT", "TRACESTATE", "BAGGAGE"} + +func envCarrier() propagation.MapCarrier { + carrier := propagation.MapCarrier{} + for _, key := range propagatedEnvKeys { + if v := os.Getenv(key); v != "" { + carrier[strings.ToLower(key)] = v + } + } + return carrier +} diff --git a/pkg/telemetry/exporter_test.go b/pkg/telemetry/exporter_test.go new file mode 100644 index 00000000..6d506de6 --- /dev/null +++ b/pkg/telemetry/exporter_test.go @@ -0,0 +1,198 @@ +package telemetry + +import ( + gocontext "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/propagation" + "go.opentelemetry.io/otel/sdk/resource" + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/trace" +) + +func attrValue(t *testing.T, res *resource.Resource, key string) (string, bool) { + t.Helper() + for _, kv := range res.Attributes() { + if string(kv.Key) == key { + return kv.Value.String(), true + } + } + return "", false +} + +func TestBuildResourceDefaults(t *testing.T) { + t.Setenv("OTEL_SERVICE_NAME", "") + t.Setenv("OTEL_RESOURCE_ATTRIBUTES", "") + + res, err := buildResource("v1.2.3") + require.NoError(t, err) + + name, ok := attrValue(t, res, "service.name") + require.True(t, ok, "service.name should be set") + assert.Equal(t, DefaultServiceName, name) + + version, ok := attrValue(t, res, "service.version") + require.True(t, ok, "service.version should be set") + assert.Equal(t, "v1.2.3", version) + + sdkName, ok := attrValue(t, res, "telemetry.sdk.name") + require.True(t, ok, "telemetry.sdk attributes should be set") + assert.Equal(t, "opentelemetry", sdkName) +} + +func TestBuildResourceEnvOverridesDefaults(t *testing.T) { + t.Setenv("OTEL_SERVICE_NAME", "helmfile-ci") + t.Setenv("OTEL_RESOURCE_ATTRIBUTES", "cicd.pipeline=deploy") + + res, err := buildResource("v1.2.3") + require.NoError(t, err) + + name, _ := attrValue(t, res, "service.name") + assert.Equal(t, "helmfile-ci", name, "OTEL_SERVICE_NAME must override the default") + + pipeline, ok := attrValue(t, res, "cicd.pipeline") + require.True(t, ok, "OTEL_RESOURCE_ATTRIBUTES should be merged") + assert.Equal(t, "deploy", pipeline) +} + +func TestSamplerFromEnv(t *testing.T) { + sampledTraceID, err := trace.TraceIDFromHex("0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f") + require.NoError(t, err) + sampledSpanID, err := trace.SpanIDFromHex("1e1e1e1e1e1e1e1e") + require.NoError(t, err) + + sampledParent := trace.NewSpanContext(trace.SpanContextConfig{ + TraceID: sampledTraceID, + SpanID: sampledSpanID, + TraceFlags: trace.FlagsSampled, + Remote: true, + }) + + // samplerDecision evaluates the sampler the way the SDK does, avoiding + // assertions on the sampler's concrete (unexported) type. + samplerDecision := func(sampler sdktrace.Sampler, parent *trace.SpanContext) sdktrace.SamplingDecision { + params := sdktrace.SamplingParameters{ + TraceID: sampledTraceID, + Name: "test", + } + if parent != nil { + params.ParentContext = trace.ContextWithSpanContext(gocontext.Background(), *parent) + } + return sampler.ShouldSample(params).Decision + } + + tests := []struct { + name string + sampler string + arg string + parent *trace.SpanContext + want sdktrace.SamplingDecision + }{ + {name: "default", want: sdktrace.RecordAndSample}, + {name: "always_on", sampler: "always_on", want: sdktrace.RecordAndSample}, + {name: "always_off", sampler: "always_off", want: sdktrace.Drop}, + {name: "parentbased_always_on root", sampler: "parentbased_always_on", want: sdktrace.RecordAndSample}, + {name: "parentbased_always_off root", sampler: "parentbased_always_off", want: sdktrace.Drop}, + {name: "parentbased_always_off respects sampled parent", sampler: "parentbased_always_off", parent: &sampledParent, want: sdktrace.RecordAndSample}, + {name: "parentbased_traceidratio zero", sampler: "parentbased_traceidratio", arg: "0", want: sdktrace.Drop}, + {name: "traceidratio zero", sampler: "traceidratio", arg: "0", want: sdktrace.Drop}, + {name: "traceidratio invalid arg falls back to 1.0", sampler: "traceidratio", arg: "not-a-number", want: sdktrace.RecordAndSample}, + {name: "unknown falls back to default", sampler: "bogus", want: sdktrace.RecordAndSample}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("OTEL_TRACES_SAMPLER", tt.sampler) + t.Setenv("OTEL_TRACES_SAMPLER_ARG", tt.arg) + + got := samplerDecision(samplerFromEnv(nil), tt.parent) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestPropagatorsFromEnv(t *testing.T) { + const traceparent = "00-0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f-1e1e1e1e1e1e1e1e-01" + + tests := []struct { + name string + env string + wantParent bool + }{ + {name: "default", env: "", wantParent: true}, + {name: "tracecontext", env: "tracecontext", wantParent: true}, + {name: "baggage and tracecontext", env: "baggage,tracecontext", wantParent: true}, + {name: "stray commas tolerated", env: "tracecontext,,baggage", wantParent: true}, + {name: "none", env: "none", wantParent: false}, + {name: "unsupported ignored", env: "bogus", wantParent: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("OTEL_PROPAGATORS", tt.env) + + prop := propagatorsFromEnv(nil) + ctx := prop.Extract(gocontext.Background(), propagation.MapCarrier{"traceparent": traceparent}) + + sc := trace.SpanFromContext(ctx).SpanContext() + assert.Equal(t, tt.wantParent, sc.IsValid()) + }) + } +} + +func TestPropagatorsFromEnvWarnsOnUnsupported(t *testing.T) { + t.Setenv("OTEL_PROPAGATORS", "tracecontext,bogus") + // Only smoke-checks that parsing succeeds; the warning is untestable + // without a logger, which propagatorsFromEnv tolerates being nil. + prop := propagatorsFromEnv(nil) + assert.NotNil(t, prop) +} + +func TestSDKDisabledFromEnv(t *testing.T) { + tests := []struct { + name string + env string + want bool + }{ + {name: "unset", env: "", want: false}, + {name: "true", env: "true", want: true}, + {name: "TRUE case-insensitive", env: "TRUE", want: true}, + {name: "false", env: "false", want: false}, + {name: "surrounding whitespace", env: " true ", want: true}, + {name: "other values", env: "1", want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("OTEL_SDK_DISABLED", tt.env) + assert.Equal(t, tt.want, sdkDisabledFromEnv()) + }) + } +} + +func TestEnvCarrier(t *testing.T) { + t.Setenv("TRACEPARENT", "00-abc-def-01") + t.Setenv("TRACESTATE", "") + t.Setenv("BAGGAGE", "k=v") + + carrier := envCarrier() + assert.Equal(t, "00-abc-def-01", carrier["traceparent"]) + assert.Equal(t, "k=v", carrier["baggage"]) + _, ok := carrier["tracestate"] + assert.False(t, ok, "empty env vars should be omitted") +} + +func TestBuildResourceKeyTypes(t *testing.T) { + // Guard against accidentally renaming resource attribute keys. + t.Setenv("OTEL_SERVICE_NAME", "") + t.Setenv("OTEL_RESOURCE_ATTRIBUTES", "") + + res, err := buildResource("v") + require.NoError(t, err) + + keys := map[string]bool{} + for _, kv := range res.Attributes() { + keys[string(kv.Key)] = true + } + assert.True(t, keys["service.name"]) + assert.True(t, keys["service.version"]) +} diff --git a/pkg/telemetry/metrics.go b/pkg/telemetry/metrics.go new file mode 100644 index 00000000..3dd76382 --- /dev/null +++ b/pkg/telemetry/metrics.go @@ -0,0 +1,135 @@ +package telemetry + +import ( + gocontext "context" + "os" + + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric" + + "github.com/helmfile/helmfile/pkg/envvar" +) + +// The instruments are created from the otel global meter: before Setup they +// are delegating no-op instruments, and once Setup installs the real meter +// provider they forward to it — so recording at call sites is branch-free +// when telemetry is disabled. Setup re-creates them under the installed +// provider with the instrumentation scope version set (unavailable at +// package init). + +var ( + execDurationHistogram metric.Float64Histogram + releaseResultCounter metric.Int64Counter + releaseDurationHistogram metric.Float64Histogram +) + +// durationBuckets are tuned for seconds-scale helm operations — from ~10ms +// probe calls to multi-minute upgrades and kubedog waits — and back both +// duration histograms. The SDK's default explicit-bucket boundaries +// ([0, 5, 10, …, 10000]) are millisecond-oriented and would lump every +// sub-5s operation — the common case — into the first bucket. +var durationBuckets = []float64{ + 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10, 30, 60, 120, 300, 600, +} + +func init() { + execDurationHistogram = newExecDurationHistogram(otel.Meter(ScopeHelmfile)) + releaseResultCounter = newReleaseResultCounter(otel.Meter(ScopeHelmfile)) + releaseDurationHistogram = newReleaseDurationHistogram(otel.Meter(ScopeHelmfile)) +} + +func newExecDurationHistogram(m metric.Meter) metric.Float64Histogram { + // Instrument creation through the global meter cannot fail (errors are + // only returned for duplicate or invalid names, and errors would be + // represented as no-op instruments anyway). + h, _ := m.Float64Histogram( + "helmfile.helm.exec.duration", + metric.WithUnit("s"), + metric.WithDescription("Duration of helm subprocess invocations started by helmfile, by subcommand and success."), + metric.WithExplicitBucketBoundaries(durationBuckets...), + ) + return h +} + +func newReleaseResultCounter(m metric.Meter) metric.Int64Counter { + c, _ := m.Int64Counter( + "helmfile.release.count", + metric.WithUnit("{release}"), + metric.WithDescription("Completed helmfile release operations, by verb and result."), + ) + return c +} + +func newReleaseDurationHistogram(m metric.Meter) metric.Float64Histogram { + h, _ := m.Float64Histogram( + "helmfile.release.duration", + metric.WithUnit("s"), + metric.WithDescription("Wall-clock duration of release operations (prepare..tracking), by verb and result; per-release name and namespace when HELMFILE_OTEL_METRICS_PER_RELEASE=true."), + metric.WithExplicitBucketBoundaries(durationBuckets...), + ) + return h +} + +// reinitMetrics re-creates the instruments under the installed provider with +// the instrumentation scope version stamped (Setup happens before any +// recording, so the swap is race-free in production use). +func reinitMetrics(version string) { + m := otel.Meter(ScopeHelmfile, metric.WithInstrumentationVersion(version)) + execDurationHistogram = newExecDurationHistogram(m) + releaseResultCounter = newReleaseResultCounter(m) + releaseDurationHistogram = newReleaseDurationHistogram(m) +} + +// RecordHelmExecDuration records the duration of one helm subprocess +// invocation. No-op when telemetry is disabled. +func RecordHelmExecDuration(seconds float64, subcommand string, success bool) { + execDurationHistogram.Record(gocontext.Background(), seconds, + metric.WithAttributes( + attribute.String("subcommand", subcommand), + attribute.Bool("success", success), + ), + ) +} + +// perReleaseMetrics reports whether metric attributes carrying the release +// identity are enabled (HELMFILE_OTEL_METRICS_PER_RELEASE=true). Read per +// call: release operations are low-frequency and tests toggle the variable. +func perReleaseMetrics() bool { + return os.Getenv(envvar.OtelMetricsPerRelease) == "true" +} + +// RecordReleaseDuration records the wall-clock duration of one release +// operation. Release name and namespace are attached only when +// HELMFILE_OTEL_METRICS_PER_RELEASE is set: they make time-series count +// proportional to the release fleet, which is fine for bounded CI runs but +// needs a capacity/TTL story for long-lived centralized collection. +func RecordReleaseDuration(seconds float64, verb string, err error, releaseName, namespace string) { + attrs := outcomeAttrs(verb, err) + if perReleaseMetrics() { + attrs = append(attrs, + attribute.String("helmfile.release", releaseName), + attribute.String("helmfile.namespace", namespace), + ) + } + releaseDurationHistogram.Record(gocontext.Background(), seconds, metric.WithAttributes(attrs...)) +} + +// outcomeAttrs are the bounded dimensions shared by the release outcome +// metrics: which operation ran, and how it ended. +func outcomeAttrs(verb string, err error) []attribute.KeyValue { + result := "success" + if err != nil { + result = "error" + } + return []attribute.KeyValue{ + attribute.String("verb", verb), + attribute.String("result", result), + } +} + +// RecordReleaseResult counts one completed release operation. No-op when +// telemetry is disabled. +func RecordReleaseResult(verb string, err error) { + releaseResultCounter.Add(gocontext.Background(), 1, metric.WithAttributes(outcomeAttrs(verb, err)...)) +} diff --git a/pkg/telemetry/otlptest/otlptest.go b/pkg/telemetry/otlptest/otlptest.go new file mode 100644 index 00000000..d57c6e5a --- /dev/null +++ b/pkg/telemetry/otlptest/otlptest.go @@ -0,0 +1,195 @@ +// Package otlptest provides a minimal in-process OTLP/HTTP receiver for +// asserting on exported spans in tests, without an external collector. +package otlptest + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "sync" + "testing" + + "github.com/stretchr/testify/require" + metricspb "go.opentelemetry.io/proto/otlp/collector/metrics/v1" + tracepb "go.opentelemetry.io/proto/otlp/collector/trace/v1" + metricsv1 "go.opentelemetry.io/proto/otlp/metrics/v1" + v1 "go.opentelemetry.io/proto/otlp/trace/v1" + "google.golang.org/protobuf/proto" + + "github.com/helmfile/helmfile/pkg/telemetry" +) + +// Recorder is a minimal OTLP/HTTP+protobuf receiver capturing raw export +// requests. Traces and metrics arrive on different paths (/v1/traces, +// /v1/metrics) and are decoded separately. +type Recorder struct { + Server *httptest.Server + + mu sync.Mutex + requests []capturedRequest +} + +type capturedRequest struct { + path string + body []byte +} + +// NewRecorder starts a receiver bound to the test's lifetime. +func NewRecorder(t *testing.T) *Recorder { + t.Helper() + rec := &Recorder{} + rec.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + if err != nil { + w.WriteHeader(http.StatusInternalServerError) + return + } + rec.mu.Lock() + rec.requests = append(rec.requests, capturedRequest{path: r.URL.Path, body: body}) + rec.mu.Unlock() + // An empty 200 body unmarshals to a valid (empty) protobuf response. + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(rec.Server.Close) + return rec +} + +// Spans decodes every captured /v1/traces request into a flat span list. +func (r *Recorder) Spans(t *testing.T) []*v1.Span { + t.Helper() + r.mu.Lock() + defer r.mu.Unlock() + var spans []*v1.Span + for _, req := range r.requests { + if req.path != "/v1/traces" { + continue + } + var msg tracepb.ExportTraceServiceRequest + require.NoError(t, proto.Unmarshal(req.body, &msg)) + for _, rs := range msg.ResourceSpans { + for _, ss := range rs.ScopeSpans { + spans = append(spans, ss.Spans...) + } + } + } + return spans +} + +// Metrics decodes every captured /v1/metrics request into a flat metric list. +func (r *Recorder) Metrics(t *testing.T) []*metricsv1.Metric { + t.Helper() + r.mu.Lock() + defer r.mu.Unlock() + var metrics []*metricsv1.Metric + for _, req := range r.requests { + if req.path != "/v1/metrics" { + continue + } + var msg metricspb.ExportMetricsServiceRequest + require.NoError(t, proto.Unmarshal(req.body, &msg)) + for _, rm := range msg.ResourceMetrics { + for _, sm := range rm.ScopeMetrics { + metrics = append(metrics, sm.Metrics...) + } + } + } + return metrics +} + +// SetupTelemetry enables telemetry against the recorder, mirroring the +// cmd/root wiring (Setup + StartCommandSpan with the given command name), and +// shuts telemetry down on cleanup. +func SetupTelemetry(t *testing.T, rec *Recorder, command string) { + t.Helper() + for _, key := range telemetry.HermeticEnvVars { + t.Setenv(key, "") + } + t.Setenv("OTEL_TRACES_EXPORTER", "otlp") + t.Setenv("OTEL_EXPORTER_OTLP_PROTOCOL", "http/protobuf") + t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", rec.Server.URL) + + telemetry.Setup(context.Background(), telemetry.Options{Enabled: true, Version: "test"}) + telemetry.StartCommandSpan(command) + t.Cleanup(func() { + _ = telemetry.Shutdown(context.Background(), nil, 0) + }) +} + +// ShutdownTelemetry flushes and disables telemetry before Spans assertions; +// tests that keep running afterward must not rely on telemetry being active. +func ShutdownTelemetry(t *testing.T) { + t.Helper() + _ = telemetry.Shutdown(context.Background(), nil, 0) +} + +// FindSpanWhere returns the first span matching pred, failing the test with a +// descriptive message otherwise. +func FindSpanWhere(t *testing.T, spans []*v1.Span, pred func(*v1.Span) bool, desc string) *v1.Span { + t.Helper() + for _, s := range spans { + if pred(s) { + return s + } + } + t.Fatalf("no span matching %q (spans: %v)", desc, spanNames(spans)) + return nil +} + +func spanNames(spans []*v1.Span) []string { + names := make([]string, 0, len(spans)) + for _, s := range spans { + names = append(names, s.Name) + } + return names +} + +// AttrString is the nil-safe attribute getter usable inside FindSpanWhere +// predicates, which run against spans that may lack the attribute entirely. +func AttrString(span *v1.Span, key string) (string, bool) { + for _, kv := range span.Attributes { + if kv.Key == key { + return kv.Value.GetStringValue(), true + } + } + return "", false +} + +// ScopeMetrics decodes every captured /v1/metrics request into the +// scope-metrics groups, exposing instrumentation scope names and versions. +func (r *Recorder) ScopeMetrics(t *testing.T) []*metricsv1.ScopeMetrics { + t.Helper() + r.mu.Lock() + defer r.mu.Unlock() + var out []*metricsv1.ScopeMetrics + for _, req := range r.requests { + if req.path != "/v1/metrics" { + continue + } + var msg metricspb.ExportMetricsServiceRequest + require.NoError(t, proto.Unmarshal(req.body, &msg)) + for _, rm := range msg.ResourceMetrics { + out = append(out, rm.ScopeMetrics...) + } + } + return out +} + +// FindMetric returns the metric with the given name, failing the test +// otherwise. +func FindMetric(t *testing.T, metrics []*metricsv1.Metric, name string) *metricsv1.Metric { + t.Helper() + for _, m := range metrics { + if m.GetName() == name { + return m + } + } + t.Fatalf("metric %q not found", name) + return nil +} + +// HasAttr reports whether the span carries the attribute at all. +func HasAttr(span *v1.Span, key string) bool { + _, ok := AttrString(span, key) + return ok +} diff --git a/pkg/telemetry/telemetry.go b/pkg/telemetry/telemetry.go new file mode 100644 index 00000000..78254227 --- /dev/null +++ b/pkg/telemetry/telemetry.go @@ -0,0 +1,236 @@ +// Package telemetry sets up OpenTelemetry tracing and metrics for helmfile. +// +// Both signals are strictly opt-in: when not enabled (the default), every +// function in this package is a no-op, Tracer returns the OTel no-op tracer, +// metric instruments are no-ops, and CommandContext returns +// context.Background — callers never need "is telemetry enabled?" branches. +// +// All exporter, sampler, and propagator configuration comes from the standard +// OTEL_* environment variables; helmfile defines no telemetry-specific +// environment variables of its own beyond HELMFILE_OTEL_TRACING (the on/off +// switch paired with the --otel-tracing flag). +// +// The only sanctioned instrumentation scopes are ScopeHelmfile (app/state +// layer) and ScopeHelm (helmexec layer). +package telemetry + +import ( + gocontext "context" + "errors" + "sync" + "sync/atomic" + "time" + + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/codes" + sdkmetric "go.opentelemetry.io/otel/sdk/metric" + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/trace" + "go.opentelemetry.io/otel/trace/noop" + "go.uber.org/zap" +) + +const ( + // ScopeHelmfile is the instrumentation scope for spans in the app/state layers. + ScopeHelmfile = "helmfile" + + // ScopeHelm is the instrumentation scope for spans in the helmexec layer. + ScopeHelm = "helm" + + // DefaultServiceName is the OTel service.name used when OTEL_SERVICE_NAME + // is unset. + DefaultServiceName = "helmfile" + + // ShutdownTimeout bounds the final flush of buffered spans on exit. It is + // applied by the caller that constructs the Shutdown context. + ShutdownTimeout = 5 * time.Second +) + +// Options controls telemetry initialization. +type Options struct { + // Enabled turns tracing on. Everything else is ignored when false. + Enabled bool + // Version is the helmfile version, recorded as the service.version + // resource attribute. + Version string + // Logger receives one-line diagnostics (never span data). May be nil. + Logger *zap.SugaredLogger +} + +// tracingState is the immutable snapshot read by the hot-path accessors +// (CommandContext, Tracer). Setup/StartCommandSpan/Shutdown replace it under +// stateMu; readers load it atomically. +type tracingState struct { + enabled bool + provider *sdktrace.TracerProvider + meters *sdkmetric.MeterProvider + cmdSpan trace.Span + cmdCtx gocontext.Context +} + +var ( + // stateMu serializes Setup/StartCommandSpan/Shutdown/reset transitions. + stateMu sync.Mutex + + // current holds the active tracingState; never nil after package init. + current atomic.Pointer[tracingState] +) + +func init() { + current.Store(disabledState()) +} + +// noopTracerProvider backs Tracer while telemetry is disabled: a single +// shared instance, since the noop tracer provider is stateless. +var noopTracerProvider = noop.NewTracerProvider() + +func disabledState() *tracingState { + return &tracingState{ + enabled: false, + cmdCtx: gocontext.Background(), + } +} + +// Setup initializes the global tracer provider from the standard OTEL_* +// environment variables (see exporter.go for what is honored). It is safe to +// call unconditionally: when opts.Enabled is false, or OTEL_SDK_DISABLED=true, +// or the exporter cannot be constructed, Setup logs a diagnostic and leaves +// telemetry disabled — telemetry problems never fail a helmfile run. +func Setup(ctx gocontext.Context, opts Options) { + if !opts.Enabled { + return + } + if ctx == nil { + ctx = gocontext.Background() + } + + stateMu.Lock() + defer stateMu.Unlock() + + if current.Load().enabled { + warnf(opts.Logger, "OpenTelemetry tracing is already initialized; ignoring duplicate Setup") + return + } + if sdkDisabledFromEnv() { + infof(opts.Logger, "OpenTelemetry tracing disabled by OTEL_SDK_DISABLED") + return + } + + provider, meters, err := newProviders(ctx, opts) + if err != nil { + warnf(opts.Logger, "OpenTelemetry tracing unavailable: %v", err) + return + } + + otel.SetTracerProvider(provider) + otel.SetMeterProvider(meters) + otel.SetTextMapPropagator(propagatorsFromEnv(opts.Logger)) + reinitMetrics(opts.Version) + + current.Store(&tracingState{ + enabled: true, + provider: provider, + meters: meters, + cmdCtx: gocontext.Background(), + }) + infof(opts.Logger, "OpenTelemetry tracing enabled") +} + +// StartCommandSpan starts the root span for one helmfile command invocation and +// makes its context the one returned by CommandContext (consumed by app.New). +// A remote parent is extracted from the TRACEPARENT/TRACESTATE/BAGGAGE +// environment variables so CI-injected trace contexts are honored. It is a +// no-op when tracing is disabled. +func StartCommandSpan(command string, attrs ...attribute.KeyValue) { + stateMu.Lock() + defer stateMu.Unlock() + + s := current.Load() + if s == nil || !s.enabled || s.provider == nil { + return + } + + parent := otel.GetTextMapPropagator().Extract(gocontext.Background(), envCarrier()) + ctx, span := s.provider.Tracer(ScopeHelmfile).Start(parent, command, trace.WithAttributes(attrs...)) + // Copy the whole state and override only the command fields: enumerating + // fields by hand has dropped one before (the meter provider). + next := *s + next.cmdSpan = span + next.cmdCtx = ctx + current.Store(&next) +} + +// CommandContext returns the context of the current command's root span, or +// context.Background when tracing is disabled. It is the single source of +// truth for deriving App.ctx. +func CommandContext() gocontext.Context { + if s := current.Load(); s != nil && s.cmdCtx != nil { + return s.cmdCtx + } + return gocontext.Background() +} + +// Tracer returns a tracer for the given instrumentation scope. It never +// returns nil: the OTel no-op tracer provider is used while telemetry is +// disabled, so callers need no enabled-checks. +func Tracer(name string) trace.Tracer { + if s := current.Load(); s != nil && s.enabled && s.provider != nil { + return s.provider.Tracer(name) + } + return noopTracerProvider.Tracer(name) +} + +// Shutdown ends the command span — recording runErr and exitCode on it when +// set — and flushes buffered spans to the exporter, bounded by ctx. It is +// idempotent and nil-safe: calling it before Setup, or twice, is fine. After +// Shutdown, telemetry behaves as disabled. +func Shutdown(ctx gocontext.Context, runErr error, exitCode int) error { + if ctx == nil { + ctx = gocontext.Background() + } + + stateMu.Lock() + defer stateMu.Unlock() + + s := current.Load() + current.Store(disabledState()) + if s == nil || !s.enabled || s.provider == nil { + return nil + } + + if s.cmdSpan != nil { + s.cmdSpan.SetAttributes(attribute.Int("helmfile.exit_code", exitCode)) + if runErr != nil || exitCode != 0 { + // The raw error may embed command arguments and subprocess + // output; keep the span description generic (the exit code is an + // attribute). + s.cmdSpan.SetStatus(codes.Error, "helmfile command failed") + } + s.cmdSpan.End() + } + return errors.Join( + s.provider.Shutdown(ctx), + s.meters.Shutdown(ctx), + ) +} + +// reset restores the pristine disabled state; used by tests (exported as Reset +// via export_test.go). +func reset() { + stateMu.Lock() + defer stateMu.Unlock() + current.Store(disabledState()) +} + +func infof(logger *zap.SugaredLogger, format string, args ...any) { + if logger != nil { + logger.Infof(format, args...) + } +} + +func warnf(logger *zap.SugaredLogger, format string, args ...any) { + if logger != nil { + logger.Warnf(format, args...) + } +} diff --git a/pkg/telemetry/telemetry_test.go b/pkg/telemetry/telemetry_test.go new file mode 100644 index 00000000..81c774db --- /dev/null +++ b/pkg/telemetry/telemetry_test.go @@ -0,0 +1,207 @@ +package telemetry + +import ( + gocontext "context" + "errors" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/trace" +) + +// setupForTest enables telemetry with the "none" exporter, so tests touch +// neither the network nor stdout while spans are still recorded locally. +func setupForTest(t *testing.T) { + t.Helper() + for _, key := range HermeticEnvVars { + t.Setenv(key, "") + } + t.Setenv("OTEL_TRACES_EXPORTER", "none") + t.Setenv("OTEL_METRICS_EXPORTER", "none") + Reset() + Setup(gocontext.Background(), Options{Enabled: true, Version: "test-version"}) + require.True(t, current.Load().enabled, "telemetry should be enabled after Setup") +} + +func TestDisabledByDefaultIsFullNoOp(t *testing.T) { + Reset() + + assert.Equal(t, gocontext.Background(), CommandContext()) + + // Tracer returns a no-op tracer: spans are non-recording. + _, span := Tracer(ScopeHelmfile).Start(CommandContext(), "span") + assert.False(t, span.IsRecording()) + span.End() + + // StartCommandSpan and Setup(disabled) are no-ops. + StartCommandSpan("helmfile sync", attribute.String("k", "v")) + Setup(gocontext.Background(), Options{Enabled: false}) + assert.Equal(t, gocontext.Background(), CommandContext()) + + assert.NoError(t, Shutdown(gocontext.Background(), nil, 0)) +} + +func TestAccessorsBeforeSetupAreNilSafe(t *testing.T) { + Reset() + + assert.Equal(t, gocontext.Background(), CommandContext()) + _, span := Tracer(ScopeHelm).Start(gocontext.Background(), "span") + assert.False(t, span.IsRecording()) + span.End() + assert.NoError(t, Shutdown(gocontext.Background(), nil, 0)) + assert.NoError(t, Shutdown(gocontext.Background(), nil, 0), "Shutdown is idempotent") +} + +func TestEnabledCommandSpanBecomesCommandContext(t *testing.T) { + setupForTest(t) + + StartCommandSpan("helmfile sync", attribute.String("helmfile.environment", "test")) + ctx := CommandContext() + require.NotEqual(t, gocontext.Background(), ctx) + + span := trace.SpanFromContext(ctx) + require.True(t, span.IsRecording()) + require.True(t, span.SpanContext().IsValid()) + + // Tracer switches to the real provider. + _, childSpan := Tracer(ScopeHelm).Start(ctx, "child") + assert.True(t, childSpan.IsRecording()) + childSpan.End() + + assert.NoError(t, Shutdown(gocontext.Background(), nil, 0)) + + // After Shutdown everything behaves as disabled again. + assert.Equal(t, gocontext.Background(), CommandContext()) + _, span = Tracer(ScopeHelmfile).Start(gocontext.Background(), "span") + assert.False(t, span.IsRecording()) + span.End() +} + +func TestTraceParentExtraction(t *testing.T) { + setupForTest(t) + + traceID := strings.Repeat("0f", 16) + spanID := strings.Repeat("1e", 8) + t.Setenv("TRACEPARENT", "00-"+traceID+"-"+spanID+"-01") + + StartCommandSpan("helmfile sync") + + sc := trace.SpanFromContext(CommandContext()).SpanContext() + require.True(t, sc.IsValid(), "command span should join the remote parent trace") + assert.Equal(t, traceID, sc.TraceID().String()) +} + +func TestSetupIdempotentAndReinitializable(t *testing.T) { + setupForTest(t) + + // A duplicate Setup is ignored rather than replacing the provider. + Setup(gocontext.Background(), Options{Enabled: true, Version: "other"}) + assert.NoError(t, Shutdown(gocontext.Background(), nil, 0)) + assert.False(t, current.Load().enabled) + + // After Shutdown, Setup can initialize a fresh provider. + Setup(gocontext.Background(), Options{Enabled: true, Version: "test-version"}) + require.True(t, current.Load().enabled) + StartCommandSpan("helmfile sync") + span := trace.SpanFromContext(CommandContext()) + require.True(t, span.IsRecording()) + assert.NoError(t, Shutdown(gocontext.Background(), nil, 0)) +} + +func TestSetupDegradesOnBadExporter(t *testing.T) { + for _, key := range HermeticEnvVars { + t.Setenv(key, "") + } + t.Setenv("OTEL_TRACES_EXPORTER", "not-a-real-exporter") + Reset() + + // Invalid exporter configuration must disable telemetry, not fail. + Setup(gocontext.Background(), Options{Enabled: true, Version: "test-version"}) + assert.False(t, current.Load().enabled) + assert.Equal(t, gocontext.Background(), CommandContext()) + assert.NoError(t, Shutdown(gocontext.Background(), nil, 0)) +} + +func TestSetupHonorsSDKDisabled(t *testing.T) { + for _, key := range HermeticEnvVars { + t.Setenv(key, "") + } + t.Setenv("OTEL_TRACES_EXPORTER", "none") + t.Setenv("OTEL_SDK_DISABLED", "true") + Reset() + + Setup(gocontext.Background(), Options{Enabled: true, Version: "test-version"}) + assert.False(t, current.Load().enabled) +} + +func TestShutdownRecordableWithoutCommandSpan(t *testing.T) { + setupForTest(t) + + // Shutdown without StartCommandSpan must not panic. + assert.NoError(t, Shutdown(gocontext.Background(), nil, 0)) +} + +func TestShutdownWithNilContext(t *testing.T) { + setupForTest(t) + StartCommandSpan("helmfile sync") + + // A nil context is part of the defensive contract; use a typed nil to + // satisfy staticcheck while still exercising the nil path. + var nilCtx gocontext.Context + assert.NoError(t, Shutdown(nilCtx, nil, 0)) +} + +func TestScopeConstants(t *testing.T) { + assert.Equal(t, "helmfile", ScopeHelmfile) + assert.Equal(t, "helm", ScopeHelm) +} + +func TestMetricsRecordIsNoOpWhenDisabled(t *testing.T) { + Reset() + + // Must not panic with telemetry never set up (noop instruments). + RecordHelmExecDuration(0.1, "template", true) + RecordReleaseResult("sync", nil) + RecordReleaseResult("sync", errors.New("boom")) + + assert.Equal(t, gocontext.Background(), CommandContext()) +} + +func TestSetupEnablesMetricsProvider(t *testing.T) { + setupForTest(t) // traces+metrics exporters = none + + // Recording through the global meter must reach the real provider without + // panicking; with the "none" exporter nothing is shipped. + RecordHelmExecDuration(0.25, "template", true) + RecordReleaseResult("status", nil) + + assert.True(t, current.Load().enabled) + assert.NoError(t, Shutdown(gocontext.Background(), nil, 0)) +} + +func TestSetupDegradesOnBadMetricExporter(t *testing.T) { + for _, key := range HermeticEnvVars { + t.Setenv(key, "") + } + t.Setenv("OTEL_TRACES_EXPORTER", "none") + t.Setenv("OTEL_METRICS_EXPORTER", "not-a-real-exporter") + Reset() + + // An invalid metrics exporter must disable telemetry, not fail the run. + Setup(gocontext.Background(), Options{Enabled: true, Version: "test-version"}) + assert.False(t, current.Load().enabled) +} + +func TestPerReleaseMetricsFlag(t *testing.T) { + t.Setenv("HELMFILE_OTEL_METRICS_PER_RELEASE", "") + assert.False(t, perReleaseMetrics()) + + t.Setenv("HELMFILE_OTEL_METRICS_PER_RELEASE", "true") + assert.True(t, perReleaseMetrics()) + + t.Setenv("HELMFILE_OTEL_METRICS_PER_RELEASE", "1") + assert.False(t, perReleaseMetrics(), "only the exact string true enables it") +}