From aeb4115c723e394490e99217a3ffbb419f4aed19 Mon Sep 17 00:00:00 2001 From: Jan Novak Date: Mon, 24 Aug 2026 11:30:27 +0200 Subject: [PATCH] Add tracing manifests and docs; clean up branch lint findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Manager env block (downward-API resource attrs, commented OTLP examples), architecture §10 + Decisions entries, README section. Lint: goconst constants, gofmt, logcheck (Setup now takes its logger from ctx via logf.FromContext). Co-Authored-By: Claude --- README.md | 19 ++++++ cmd/main.go | 3 +- config/manager/manager.yaml | 29 +++++++++ docs/architecture.md | 60 +++++++++++++++++++ .../2026-08-24-1025-otel-tracing.md | 28 ++++++++- internal/provider/metrics.go | 18 ++++-- internal/provider/tracing_test.go | 10 ++-- internal/tracing/logger.go | 2 + internal/tracing/tracing.go | 43 +++++++++---- internal/tracing/tracing_test.go | 39 ++++++------ 10 files changed, 207 insertions(+), 44 deletions(-) diff --git a/README.md b/README.md index a7783c9..a3a80dd 100644 --- a/README.md +++ b/README.md @@ -192,6 +192,25 @@ Test against a fake API seam rather than the real cloud — see the [internal/provider/gcp/gcp.go](internal/provider/gcp/gcp.go) for the pattern. +## Tracing + +OpenTelemetry tracing is built in but **off by default** — the operator +only exports spans when pointed at an OTLP receiver: + +```bash +# In config/manager/manager.yaml (commented-out block is already there): +# OTEL_EXPORTER_OTLP_ENDPOINT=http://jaeger-collector.observability:4318 +# Local dev: print spans to stdout instead +OTEL_TRACES_EXPORTER=console go run ./cmd --providers-config hack/providers-dev.yaml +``` + +Every reconcile, discovery API request, and GC sweep becomes a trace, with +child spans for provider and Kubernetes API calls; log lines inside a traced +operation carry matching `traceID`/`spanID` fields (point Grafana/Loki +derived fields at `traceID`). Sampling and endpoints follow the standard +`OTEL_*` env vars; `--trace-health-probes` additionally traces each health +probe (high volume). Details in `docs/architecture.md` §10. + ## Caveats — read these two **Changing a proxy changes its IP.** Proxies are immutable cattle: editing diff --git a/cmd/main.go b/cmd/main.go index a195907..2b85b59 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -42,6 +42,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/healthz" + logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" ctrlmetrics "sigs.k8s.io/controller-runtime/pkg/metrics" "sigs.k8s.io/controller-runtime/pkg/metrics/filters" @@ -157,7 +158,7 @@ func main() { // Off (no-op spans, unchanged logs) unless OTEL_* env opts in; see // internal/tracing. Shutdown is called explicitly after mgr.Start // returns — the os.Exit paths below skip defers. - tracingShutdown, err := tracing.Setup(context.Background(), setupLog, + tracingShutdown, err := tracing.Setup(logf.IntoContext(context.Background(), setupLog), "egress-proxies-operator", version.Resolve()) if err != nil { setupLog.Error(err, "Failed to set up tracing") diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index 7d832f5..5676abb 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -64,6 +64,8 @@ spec: - --leader-elect - --health-probe-bind-address=:8081 - --providers-config=/etc/proxy-operator/providers.yaml + # Add --trace-health-probes to emit one span per health probe + # (high volume: ~1/s per proxy). env: # Bearer token for the discovery API. Optional: without the # Secret the API serves unauthenticated (with a loud warning). @@ -76,6 +78,33 @@ spec: name: discovery-token key: token optional: true + # --- OpenTelemetry tracing --- + # Tracing is OFF until OTEL_EXPORTER_OTLP_ENDPOINT (or + # OTEL_TRACES_EXPORTER) is set; without it the operator runs + # exactly as before. The downward-API vars must stay listed + # before OTEL_RESOURCE_ATTRIBUTES: $(VAR) expansion only sees + # earlier-listed vars. + - name: POD_NAME + valueFrom: + fieldRef: + fieldPath: metadata.name + - name: POD_NAMESPACE + valueFrom: + fieldRef: + fieldPath: metadata.namespace + - name: OTEL_SERVICE_NAME + value: egress-proxies-operator + - name: OTEL_RESOURCE_ATTRIBUTES + value: k8s.pod.name=$(POD_NAME),k8s.namespace.name=$(POD_NAMESPACE) + # Point at an OTLP receiver (Tempo, Jaeger, otel-collector) to + # enable tracing; http/protobuf (4318) is the default protocol, + # set OTEL_EXPORTER_OTLP_PROTOCOL=grpc for 4317. + # - name: OTEL_EXPORTER_OTLP_ENDPOINT + # value: http://jaeger-collector.observability:4318 + # - name: OTEL_TRACES_SAMPLER + # value: parentbased_traceidratio + # - name: OTEL_TRACES_SAMPLER_ARG + # value: "0.1" image: controller:latest name: manager ports: diff --git a/docs/architecture.md b/docs/architecture.md index 7946caf..5c1053b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -300,6 +300,45 @@ Each consuming package defines its own small recorder interface `metrics.Metrics` satisfies all of them structurally, so no package other than `cmd/main.go` imports the metrics package. +### 10. Tracing (`internal/tracing/`) + +OpenTelemetry tracing, integrated with — not replacing — the logr/zap +logging. Everything hangs off standard `OTEL_*` env vars: with no +`OTEL_EXPORTER_OTLP_ENDPOINT` / `OTEL_TRACES_EXPORTER` set, no SDK is +installed, spans are no-ops, and logs are byte-for-byte what they were. +`OTEL_SDK_DISABLED=true` and `OTEL_TRACES_EXPORTER=none` force it off; +`console` prints spans to stdout for local dev; sampling follows +`OTEL_TRACES_SAMPLER(_ARG)`. + +Span topology — each unit of work is a new trace (watch events carry no +incoming trace context): + +- `Reconcile Proxy` (root, via a `reconcile.Reconciler` decorator) → + `reconcile.managed` / `reconcile.replaceInstance` / `reconcile.delete`, + plus `status.patch` from the deferred flush. `reconcileExternal` does no + I/O and is unspanned. +- `provider.create|get|delete|list` (client spans, `provider.WithTracing` + decorator wired outermost around `WithMetrics`); the GCP SDK's own + otelhttp transport contributes HTTP child spans automatically. +- Kubernetes API calls become client child spans via a wrapped + `rest.Config` transport — gated on an existing parent span, so informer + list/watch long-polls and leader-election renewals never create root + spans. The kube-apiserver ignores incoming `traceparent` by design; + these spans are leaves. +- Discovery API: one server span per request (named from the route + pattern), incoming W3C `traceparent` honored so clients' traces continue + into the operator; `/healthz` excluded. +- `gc.sweep` per GC pass; `health.probe` per probe **only** with + `--trace-health-probes` (default off — probes run ~1/s per proxy). + +Log correlation: logr sinks never see a context, so trace IDs ride on the +logger — `tracing.Start` re-derives the ctx logger from a captured base +with `traceID`/`spanID` values (lowerCamel, matching `reconcileID`; +Grafana/Loki derived-field regexes must match `traceID`, not `trace_id`). +Re-deriving from the base rather than layering keeps zap from emitting +duplicate keys on nested spans. GCP V(5) wire logs get the same keys from +the slog handler's ctx. + ## Decisions Judgment calls the spec left open, and deliberate deviations — recorded so @@ -404,3 +443,24 @@ they read as choices, not accidents. Chronological by build step. logger to everything running under the manager — fighting that would mean two logging systems in one process. Noted as a deviation rather than silently ignored. +- **Tracing is env-gated, not flag-gated:** it activates only when + `OTEL_EXPORTER_OTLP_ENDPOINT` / `OTEL_TRACES_EXPORTER` is set (user + decision). No collector configured means no SDK installed, no-op spans, + unchanged logs — the safe default for every existing deployment. The + one flag is `--trace-health-probes`, off by default, because probes at + ~1/s per proxy would dominate trace volume. +- **Trace keys are `traceID`/`spanID`**, lowerCamel like `reconcileID` and + `providerID`, deliberately not the `trace_id` many Grafana derived-field + examples assume — configure the derived-field regex accordingly. logr + sinks can't read ctx, so the IDs ride on the ctx logger, re-derived from + a captured base per span so zap never emits duplicate keys. +- **Probe→reconcile trace links are not attempted:** the health engine's + `GenericEvent` carries only namespace/name (no ctx), and the workqueue + coalesces events, so any link would be a guess. A health-triggered + reconcile starts a fresh trace; the probe that caused it is findable via + its own (opt-in) span and shared proxy attributes. +- **No `traceparent` toward probe targets:** probe transports stay + uninstrumented so trace headers can never leak through a proxy to + external sites. The kube-apiserver ignores incoming `traceparent` by + design (public endpoint), so k8s client spans are leaves — in-process + traces, not cross-process ones. diff --git a/docs/plans-executions/2026-08-24-1025-otel-tracing.md b/docs/plans-executions/2026-08-24-1025-otel-tracing.md index 9de685a..c4f89ab 100644 --- a/docs/plans-executions/2026-08-24-1025-otel-tracing.md +++ b/docs/plans-executions/2026-08-24-1025-otel-tracing.md @@ -10,7 +10,7 @@ Plan: `docs/plans/2026-08-24-1025-otel-tracing.md` - [x] Step 6 — Discovery server - [x] Step 7 — GC + health - [x] Step 8 — GCP wire-log enrichment -- [ ] Step 9 — Manifests + docs +- [x] Step 9 — Manifests + docs ## Step 1 — Dependencies @@ -155,3 +155,29 @@ before the elision pass (both values are well under the 1KiB threshold). The new `wirelog_test.go` covers the enrichment, that the JWT/token security filter still drops non-wire Debug records even with a span present, and that spanless records stay unchanged. + +## Step 9 — Manifests, docs, lint + +`config/manager/manager.yaml` gained the OTel env block (downward-API +`POD_NAME`/`POD_NAMESPACE` deliberately listed *before* +`OTEL_RESOURCE_ATTRIBUTES` — `$(VAR)` expansion only sees earlier vars), +with the OTLP endpoint and sampler left as commented examples so deployments +stay tracing-off by default. `docs/architecture.md` got "### 10. Tracing" +plus five Decisions entries; README got a Tracing section. + +`make lint` surfaced 16 new-on-branch issues (checked with +`golangci-lint run --new-from-rev main`; the other ~43 pre-date this work): + +- one gofmt slip, 13 goconst repeats — fixed with `env*`/`exporter*` + constants in `internal/tracing/tracing.go` shared by the tests; +- two logcheck hits ("function takes both a context and a logger"): + `Setup` now reads its logger from ctx (`logf.FromContext`, caller seeds + via `logf.IntoContext` — the linter's sanctioned pattern), and + `ContextWithLogger` carries a justified `//nolint:logcheck`, since being + the IntoContext-analogue is its purpose. + +Worth noting: pre-existing lint debt (50 goconst etc. on `main`) was left +untouched — `make lint` still fails overall; only branch-introduced issues +were cleaned. The lint fixes changed `Setup`'s signature after Step 2/4 had +landed, so those commits show the two-arg version; this commit is where it +settles. diff --git a/internal/provider/metrics.go b/internal/provider/metrics.go index 96b24fa..6c9dd08 100644 --- a/internal/provider/metrics.go +++ b/internal/provider/metrics.go @@ -50,17 +50,25 @@ func (i *instrumented) record(op string, err error) { i.rec.ProviderRequest(i.name, op, resultLabel(err)) } +const ( + resultOK = "ok" + resultNotFound = "not_found" + resultQuotaExceeded = "quota_exceeded" + resultPermanent = "permanent" + resultTransient = "transient" +) + func resultLabel(err error) string { switch Class(err) { case nil: - return "ok" + return resultOK case ErrNotFound: - return "not_found" + return resultNotFound case ErrQuotaExceeded: - return "quota_exceeded" + return resultQuotaExceeded case ErrPermanent: - return "permanent" + return resultPermanent default: - return "transient" + return resultTransient } } diff --git a/internal/provider/tracing_test.go b/internal/provider/tracing_test.go index cdb14c6..ca8c62c 100644 --- a/internal/provider/tracing_test.go +++ b/internal/provider/tracing_test.go @@ -33,7 +33,7 @@ func TestWithTracing_spanPerCall(t *testing.T) { call: func(p Provider) error { _, err := p.Create(context.Background(), CreateRequest{}); return err }, wantSpan: "provider.create", wantStatus: codes.Ok, - wantResult: "ok", + wantResult: resultOK, wantAttr: attribute.String("provider.id", "id-1"), }, { @@ -42,7 +42,7 @@ func TestWithTracing_spanPerCall(t *testing.T) { call: func(p Provider) error { _, err := p.Get(context.Background(), "id-1"); return err }, wantSpan: "provider.get", wantStatus: codes.Ok, - wantResult: "not_found", + wantResult: resultNotFound, wantAttr: attribute.String("provider.id", "id-1"), }, { @@ -51,7 +51,7 @@ func TestWithTracing_spanPerCall(t *testing.T) { call: func(p Provider) error { return p.Delete(context.Background(), "id-1") }, wantSpan: "provider.delete", wantStatus: codes.Error, - wantResult: "transient", + wantResult: resultTransient, wantAttr: attribute.String("provider.id", "id-1"), }, { @@ -60,7 +60,7 @@ func TestWithTracing_spanPerCall(t *testing.T) { call: func(p Provider) error { _, err := p.Create(context.Background(), CreateRequest{}); return err }, wantSpan: "provider.create", wantStatus: codes.Error, - wantResult: "quota_exceeded", + wantResult: resultQuotaExceeded, wantAttr: attribute.String("provider.name", "x"), }, { @@ -69,7 +69,7 @@ func TestWithTracing_spanPerCall(t *testing.T) { call: func(p Provider) error { _, err := p.ListByTag(context.Background()); return err }, wantSpan: "provider.list", wantStatus: codes.Ok, - wantResult: "ok", + wantResult: resultOK, wantAttr: attribute.Int("provider.instances", 0), }, } diff --git a/internal/tracing/logger.go b/internal/tracing/logger.go index 042bd1d..5d4a614 100644 --- a/internal/tracing/logger.go +++ b/internal/tracing/logger.go @@ -47,6 +47,8 @@ func StartSpan(ctx context.Context, tracer trace.Tracer, name string, opts ...tr // base for span enrichment. If ctx already carries a valid span (the HTTP // middleware calls this inside the otelhttp handler), the logger is // enriched immediately. +// +//nolint:logcheck // IntoContext-analogue: taking ctx and the logger to seed it with is the point. func ContextWithLogger(ctx context.Context, base logr.Logger) context.Context { ctx = context.WithValue(ctx, baseLoggerKey{}, base) if sc := trace.SpanContextFromContext(ctx); sc.IsValid() { diff --git a/internal/tracing/tracing.go b/internal/tracing/tracing.go index 4801d28..6a31b0b 100644 --- a/internal/tracing/tracing.go +++ b/internal/tracing/tracing.go @@ -10,7 +10,6 @@ import ( "os" "strings" - "github.com/go-logr/logr" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc" "go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp" @@ -19,6 +18,20 @@ import ( "go.opentelemetry.io/otel/sdk/resource" sdktrace "go.opentelemetry.io/otel/sdk/trace" semconv "go.opentelemetry.io/otel/semconv/v1.43.0" + logf "sigs.k8s.io/controller-runtime/pkg/log" +) + +const ( + envSDKDisabled = "OTEL_SDK_DISABLED" + envTracesExporter = "OTEL_TRACES_EXPORTER" + envOTLPEndpoint = "OTEL_EXPORTER_OTLP_ENDPOINT" + envOTLPTracesEndpoint = "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT" + envOTLPProtocol = "OTEL_EXPORTER_OTLP_PROTOCOL" + envOTLPTracesProtocol = "OTEL_EXPORTER_OTLP_TRACES_PROTOCOL" + + exporterOTLP = "otlp" + exporterConsole = "console" + exporterNone = "none" ) // maxQueueSize bounds the batch processor's span buffer; the default 2048 @@ -35,15 +48,19 @@ const maxQueueSize = 512 // OTEL_TRACES_EXPORTER=none force it off (the Go SDK does not implement // OTEL_SDK_DISABLED itself). Sampling, endpoints, TLS, and headers follow // the standard SDK/exporter env vars (OTEL_TRACES_SAMPLER, OTEL_EXPORTER_OTLP_*). -func Setup(ctx context.Context, log logr.Logger, service, version string) (func(context.Context) error, error) { +// +// Setup logs via the logger in ctx (logf.FromContext) rather than a +// parameter — the caller seeds it with logf.IntoContext. +func Setup(ctx context.Context, service, version string) (func(context.Context) error, error) { + log := logf.FromContext(ctx) noop := func(context.Context) error { return nil } - exporterEnv := strings.ToLower(strings.TrimSpace(os.Getenv("OTEL_TRACES_EXPORTER"))) - endpointSet := os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") != "" || - os.Getenv("OTEL_EXPORTER_OTLP_TRACES_ENDPOINT") != "" - sdkDisabled := strings.EqualFold(strings.TrimSpace(os.Getenv("OTEL_SDK_DISABLED")), "true") + exporterEnv := strings.ToLower(strings.TrimSpace(os.Getenv(envTracesExporter))) + endpointSet := os.Getenv(envOTLPEndpoint) != "" || + os.Getenv(envOTLPTracesEndpoint) != "" + sdkDisabled := strings.EqualFold(strings.TrimSpace(os.Getenv(envSDKDisabled)), "true") - if sdkDisabled || exporterEnv == "none" || (exporterEnv == "" && !endpointSet) { + if sdkDisabled || exporterEnv == exporterNone || (exporterEnv == "" && !endpointSet) { log.Info("tracing disabled", "reason", disabledReason(sdkDisabled, exporterEnv)) return noop, nil @@ -87,7 +104,7 @@ func disabledReason(sdkDisabled bool, exporterEnv string) string { switch { case sdkDisabled: return "OTEL_SDK_DISABLED=true" - case exporterEnv == "none": + case exporterEnv == exporterNone: return "OTEL_TRACES_EXPORTER=none" default: return "no OTEL_TRACES_EXPORTER or OTLP endpoint configured" @@ -99,10 +116,10 @@ func disabledReason(sdkDisabled bool, exporterEnv string) string { // metric/log/prometheus exporters into a trace-only binary. func newExporter(ctx context.Context, kind string) (sdktrace.SpanExporter, string, error) { switch kind { - case "", "otlp": - proto := strings.ToLower(strings.TrimSpace(os.Getenv("OTEL_EXPORTER_OTLP_TRACES_PROTOCOL"))) + case "", exporterOTLP: + proto := strings.ToLower(strings.TrimSpace(os.Getenv(envOTLPTracesProtocol))) if proto == "" { - proto = strings.ToLower(strings.TrimSpace(os.Getenv("OTEL_EXPORTER_OTLP_PROTOCOL"))) + proto = strings.ToLower(strings.TrimSpace(os.Getenv(envOTLPProtocol))) } switch proto { case "", "http/protobuf": @@ -114,9 +131,9 @@ func newExporter(ctx context.Context, kind string) (sdktrace.SpanExporter, strin default: return nil, "", fmt.Errorf("unsupported OTLP protocol %q (supported: http/protobuf, grpc)", proto) } - case "console": + case exporterConsole: exp, err := stdouttrace.New() - return exp, "console", err + return exp, exporterConsole, err default: return nil, "", fmt.Errorf("unsupported OTEL_TRACES_EXPORTER %q (supported: otlp, console, none)", kind) } diff --git a/internal/tracing/tracing_test.go b/internal/tracing/tracing_test.go index b90310b..ed6b013 100644 --- a/internal/tracing/tracing_test.go +++ b/internal/tracing/tracing_test.go @@ -5,24 +5,25 @@ import ( "testing" "time" - "github.com/go-logr/logr" "go.opentelemetry.io/otel" sdktrace "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/trace/noop" ) +const testOTLPEndpoint = "http://localhost:4318" + // clearOTelEnv pins every env var Setup reads, so developer shells with // OTEL_* set can't change test outcomes. Not parallel-safe by design // (t.Setenv forbids t.Parallel). func clearOTelEnv(t *testing.T) { t.Helper() for _, k := range []string{ - "OTEL_SDK_DISABLED", - "OTEL_TRACES_EXPORTER", - "OTEL_EXPORTER_OTLP_ENDPOINT", - "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT", - "OTEL_EXPORTER_OTLP_PROTOCOL", - "OTEL_EXPORTER_OTLP_TRACES_PROTOCOL", + envSDKDisabled, + envTracesExporter, + envOTLPEndpoint, + envOTLPTracesEndpoint, + envOTLPProtocol, + envOTLPTracesProtocol, } { t.Setenv(k, "") } @@ -38,50 +39,50 @@ func TestSetup_envGating(t *testing.T) { {name: "no env means disabled", env: nil, wantEnabled: false}, { name: "endpoint enables otlp", - env: map[string]string{"OTEL_EXPORTER_OTLP_ENDPOINT": "http://localhost:4318"}, + env: map[string]string{envOTLPEndpoint: testOTLPEndpoint}, wantEnabled: true, }, { name: "traces endpoint enables otlp", - env: map[string]string{"OTEL_EXPORTER_OTLP_TRACES_ENDPOINT": "http://localhost:4318"}, + env: map[string]string{envOTLPTracesEndpoint: testOTLPEndpoint}, wantEnabled: true, }, { name: "explicit console exporter", - env: map[string]string{"OTEL_TRACES_EXPORTER": "console"}, + env: map[string]string{envTracesExporter: exporterConsole}, wantEnabled: true, }, { name: "grpc protocol", - env: map[string]string{"OTEL_TRACES_EXPORTER": "otlp", "OTEL_EXPORTER_OTLP_PROTOCOL": "grpc"}, + env: map[string]string{envTracesExporter: exporterOTLP, envOTLPProtocol: "grpc"}, wantEnabled: true, }, { name: "exporter none wins over endpoint", env: map[string]string{ - "OTEL_TRACES_EXPORTER": "none", - "OTEL_EXPORTER_OTLP_ENDPOINT": "http://localhost:4318", + envTracesExporter: exporterNone, + envOTLPEndpoint: testOTLPEndpoint, }, wantEnabled: false, }, { name: "OTEL_SDK_DISABLED wins over everything", env: map[string]string{ - "OTEL_SDK_DISABLED": "true", - "OTEL_EXPORTER_OTLP_ENDPOINT": "http://localhost:4318", + envSDKDisabled: "true", + envOTLPEndpoint: testOTLPEndpoint, }, wantEnabled: false, }, { name: "unsupported exporter errors", - env: map[string]string{"OTEL_TRACES_EXPORTER": "jaeger"}, + env: map[string]string{envTracesExporter: "jaeger"}, wantErr: true, }, { name: "unsupported protocol errors", env: map[string]string{ - "OTEL_TRACES_EXPORTER": "otlp", - "OTEL_EXPORTER_OTLP_PROTOCOL": "http/json", + envTracesExporter: exporterOTLP, + envOTLPProtocol: "http/json", }, wantErr: true, }, @@ -100,7 +101,7 @@ func TestSetup_envGating(t *testing.T) { before := noop.NewTracerProvider() otel.SetTracerProvider(before) - shutdown, err := Setup(context.Background(), logr.Discard(), "test-svc", "abc123") + shutdown, err := Setup(context.Background(), "test-svc", "abc123") if tc.wantErr { if err == nil { t.Fatal("want error, got nil")