From e65c597506f84471828dd52d17fd4ece731b63ed Mon Sep 17 00:00:00 2001 From: Jack Woods Date: Thu, 14 May 2026 11:19:58 -0400 Subject: [PATCH 01/17] =?UTF-8?q?feat(observability):=20direct-to-cloud=20?= =?UTF-8?q?OTLP=20=E2=80=94=20TLS=20+=20auth=20headers=20(#97)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #97. The OTLP exporters previously hardcoded WithInsecure(), so WaveHouse could only push to a plaintext-gRPC receiver — practically, a local sidecar collector or Alloy. Operators wanting to ship straight to Grafana Cloud, Honeycomb, or Datadog had to run a sidecar to terminate TLS + add the per-RPC auth header. This commit closes that gap. Config — four new fields, all defaults preserve current behavior: - otel.headers (WH_OTEL_HEADERS) — OTel-spec env-var format, comma-separated `key=value`. Values may contain `=` so base64 padding round-trips. Applied as gRPC metadata to every exporter. - otel.{traces,metrics,logs}.addr — optional per-signal endpoint override (Grafana Cloud uses distinct gateway hosts per signal). Empty means inherit otel.addr. Provider — scheme-aware endpoint: - `https://` → TLS via system root CAs (credentials.NewTLS). - `http://` or bare `host:port` → plaintext (backward-compat). - URL path component stripped (gRPC ignores it). - Headers applied uniformly to all three exporters (no per-signal header override — per-signal endpoint is the override knob). - ProviderConfig.TLSConfig is a test-only escape hatch; production leaves it nil for system-roots TLS. The FakeOTLPTLS integration test injects an ephemeral-cert tls.Config through this field. Validation — Validate() rejects malformed headers at boot (missing `=`, empty key) so a typo can't silently disable auth in production. The check is skipped when otel.enabled=false to keep yaml-iteration ergonomic. Tests: - Unit (internal/observability/endpoint_test.go): table-driven coverage of ParseEndpoint (all scheme cases incl. path stripping) and ParseOTelHeaders (whitespace, base64-with-trailing-=, malformed rejection). - Config: TestValidate_RejectsMalformedHeaders, TestValidate_AcceptsValidHeaders, TestValidate_HeadersIgnoredWhenOTelDisabled, plus defaults coverage. - Integration: TestOTel_TLSPath_Traces (FakeOTLPTLS with ephemeral ECDSA self-signed cert), TestOTel_Headers_AppliedToAllSignals (asserts authorization header on traces/metrics/logs via captured gRPC metadata), TestOTel_PerSignalEndpoint_Override (split traces and metrics across two receivers). FakeOTLP — NewFakeOTLPTLS(t) constructor + TLSConfig() accessor for the matching client config; gRPC metadata captured per-signal via new LastTraceHeaders / LastMetricHeaders / LastLogHeaders accessors. Existing plaintext NewFakeOTLP path unchanged. Docs — configuration.md gets the four new env-var rows and a rewritten TLS callout (the prior "limitation" note is gone). deployment.md adds a Direct-to-cloud subsection with worked examples for Honeycomb, Grafana Cloud (Basic auth via base64), and Datadog OTLP. AGENTS.md key design decision #15 grows one sentence noting that scheme-sniffing is the supported TLS knob (no separate `tls.enabled` boolean) and that per-signal endpoint is the override knob (not per-signal headers). Out of scope: mTLS / client-certificate auth. Co-Authored-By: Claude Opus 4.7 (1M context) --- AGENTS.md | 2 +- CHANGELOG.md | 1 + cmd/wavehouse/main.go | 7 ++ docs/src/content/docs/configuration.md | 10 +- docs/src/content/docs/deployment.md | 43 ++++++- internal/config/config.go | 50 ++++++++- internal/config/config_test.go | 63 +++++++++++ internal/observability/endpoint.go | 76 +++++++++++++ internal/observability/endpoint_test.go | 65 +++++++++++ internal/observability/provider.go | 75 ++++++++++--- internal/testutil/otlp.go | 142 ++++++++++++++++++++++-- tests/integration/otel_test.go | 118 ++++++++++++++++++++ 12 files changed, 623 insertions(+), 29 deletions(-) create mode 100644 internal/observability/endpoint.go create mode 100644 internal/observability/endpoint_test.go diff --git a/AGENTS.md b/AGENTS.md index 3232dfb2..fbedb7fd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -42,7 +42,7 @@ Twelve internal packages under `internal/`: 12. **Structured queries**: Type-safe query AST endpoint (`POST /v1/tables/{table}/query`) validated against schema, with permission enforcement, timestamp bucketing for cache optimization, and `DefaultMaxRows` (10,000) limit cap. 13. **Named query pipes**: Pre-defined SQL templates (inspired by Tinybird) with parameter binding, role restrictions, and caching. Stored in NATS KV with `.sql` file directory bootstrap. 14. **TypeScript SDK**: `@wavehouse/sdk` — zero-dependency client with typed query builder, real-time SSE, live queries with smart aggregation classification (incrementable/decomposable/poll), and codegen CLI. -15. **Observability invariants**: Stdout is *always* 100% — the slog logger fans out to stdout AND OTLP, and stdout sampling would silently hide records that scraping pipelines (Promtail/Alloy/Vector → Loki) are paying to store. Sampling knobs apply only to OTLP push. WARN+ERROR records always export at 100% regardless of `logs.sample_rate` — silently dropping errors during incidents would be a worse failure mode than the cost of forwarding them all (this is a *non-configurable* floor; do not expose it). gRPC OTel exporters dial lazily, so an unreachable collector never blocks startup; transient failures surface via the OTel SDK's error handler. The OTel Prometheus exporter (when enabled) uses a *private* `prometheus.Registry` to avoid leaking process/Go collectors that `prometheus.DefaultRegisterer` auto-registers into our `/metrics` output. When changing the logger, the sampler, or the provider wiring, preserve these invariants. +15. **Observability invariants**: Stdout is *always* 100% — the slog logger fans out to stdout AND OTLP, and stdout sampling would silently hide records that scraping pipelines (Promtail/Alloy/Vector → Loki) are paying to store. Sampling knobs apply only to OTLP push. WARN+ERROR records always export at 100% regardless of `logs.sample_rate` — silently dropping errors during incidents would be a worse failure mode than the cost of forwarding them all (this is a *non-configurable* floor; do not expose it). gRPC OTel exporters dial lazily, so an unreachable collector never blocks startup; transient failures surface via the OTel SDK's error handler. The OTel Prometheus exporter (when enabled) uses a *private* `prometheus.Registry` to avoid leaking process/Go collectors that `prometheus.DefaultRegisterer` auto-registers into our `/metrics` output. TLS is selected via *scheme sniffing* on `otel.addr` (`https://` → TLS, anything else → plaintext) — there is no separate `otel.tls.enabled` boolean. `otel.headers` is applied uniformly to all OTLP exporters (no per-signal header override); per-signal endpoint overrides (`otel.{traces,metrics,logs}.addr`) are the supported way to split signals across gateways. When changing the logger, the sampler, or the provider wiring, preserve these invariants. 16. **Bearer-token-only CORS posture**: WaveHouse is a Bearer-token API — `Authorization: Bearer ` on every authenticated request, no cookies, no session middleware. The CORS middleware (`internal/api/router.go` `corsMiddleware`) deliberately **never** emits `Access-Control-Allow-Credentials`, because (a) we don't need it (Bearer tokens are explicit request headers, not browser-managed credentials) and (b) the historical pairing of `Allow-Credentials: true` with `Allow-Origin: *` is a CORS spec violation that browsers reject. The `cors_allowed_origins` allowlist controls *which origins can read responses*, not cookie scope. CSRF protection is structural: cross-site requests can't smuggle a Bearer token because the browser won't auto-attach `Authorization` headers cross-origin. Do not reintroduce cookie-based auth or `Access-Control-Allow-Credentials` without a separate design discussion — the current posture is the answer to GitHub issues #29 and #30. ## Code Conventions diff --git a/CHANGELOG.md b/CHANGELOG.md index 19e05ae5..ae675cdc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Dependabot auto-merge no longer slipped past CI on consolidated-pipeline PRs** (`main branch protection` ruleset; `.github/workflows/project-orchestrator.yml`; `.github/workflows/claude-review.yml`; `AGENTS.md`): on commit `93b3206` the previously six-job CI was collapsed into a single job named `CI`, but three places still referenced the old multi-job names — the ruleset's `required_status_checks` (`Validate` + `Admin approval` only — `CI` not listed), the `bot-clean` check in `project-orchestrator.yml` (looking for `Build`/`Validate`/`Lint`/`Test`/`Integration Tests`/`SDK Tests`), and `REQUIRED_CHECKS` in `claude-review.yml` (same six). Net effect: a Dependabot PR opened at T+0 would have its `Validate` and `Admin approval` checks satisfied within ~30s (PR-title workflow + the auto-approval workflow), `gh pr merge --auto` would fire, GitHub would see all *required* checks green, and the PR would squash-merge before `ci.yml` even started running. Pre-consolidation runs (PR #90 etc.) didn't expose this because GitHub's auto-merge waits for in-flight checks as a courtesy, and the multi-job CI was producing checks before auto-merge fired — but with one collapsed `CI` check that didn't start until later, the courtesy wait disappeared. Fix: added `CI` to the ruleset's required-checks (now `Admin approval` / `CI` / `Validate`) via the `gh api PUT /repos/Wave-RF/WaveHouse/rulesets/15353356` round-trip, and updated both bot-clean lists to look for `CI` + `Validate`. AGENTS.md note about the required-check set updated to match. Diagnosis steps recorded inline as code comments in both workflow files so future-me knows where to look if the check name changes again. ### Added +- **Direct-to-cloud OTLP — TLS + auth headers, no sidecar required** (`internal/config/config.go`, `internal/observability/provider.go`, `internal/observability/endpoint.go`, `internal/testutil/otlp.go`, `cmd/wavehouse/main.go`, `tests/integration/otel_test.go`, `docs/src/content/docs/{configuration,deployment}.md`, `AGENTS.md`): closes [#97](https://github.com/Wave-RF/WaveHouse/issues/97). `otel.addr` is now scheme-aware — `https://` selects TLS (system root CAs by default), `http://` or a bare `host:port` stays plaintext gRPC (backward-compat with the prior `WithInsecure()` default). New `otel.headers` (`WH_OTEL_HEADERS`) takes the OTel spec env-var format — comma-separated `key=value`, values may contain `=` so base64 padding round-trips — and applies the parsed map as gRPC metadata to every OTLP exporter (traces, metrics, logs). Together these unlock direct push to Honeycomb / Grafana Cloud / Datadog OTLP without a sidecar. Per-signal endpoint overrides (`otel.{traces,metrics,logs}.addr`) point one signal at a different gateway than the top-level default (Grafana Cloud uses distinct trace/metric/log hosts in some regions); empty means inherit. Headers always apply uniformly — there is intentionally no per-signal header override. Test surface: `FakeOTLP` gains a `NewFakeOTLPTLS(t)` constructor (ephemeral self-signed cert + matching client `*tls.Config`) and per-signal `LastTraceHeaders / LastMetricHeaders / LastLogHeaders` accessors via captured gRPC metadata. New integration tests pin the TLS dial path, headers-on-all-signals propagation, and per-signal endpoint splitting. Validation rejects malformed `otel.headers` (missing `=`, empty key) at boot rather than letting a typo silently disable auth in production. mTLS / client-certificate auth is not yet supported. - **Astro / Starlight documentation site at `wavehouse.dev`** (`docs/`, `Makefile`, `.github/dependabot.yml`, `.github/workflows/ci.yml`, `.gitignore`, `.vscode/launch.json`): full content + tooling for the public docs (Getting Started, Why WaveHouse, Architecture, API Reference, TypeScript SDK, Configuration, Deployment, Development). Build-time mermaid via `rehype-mermaid` (inline-svg strategy, no client-side JS), LaTeX via `remark-math` + `rehype-katex`, image zoom via `starlight-image-zoom`, expressiveCode with github dark/light + `env`/`dns` Shiki language aliases. PostHog frontend telemetry (key is public by design — embedded in every visitor's browser). `editLink` pointed at `main`; `lastUpdated` on. Sidebar lives in `docs/src/config/sidebar.ts` as the single source of truth, consumed by both Starlight rendering and the LLM-friendly outputs below. Cloudflare Workers + Static Assets deployment via `docs/wrangler.jsonc` (no auto-deploy yet — wired up when the GH Actions workflow lands separately). Make targets stay minimal: `dev-docs` (Astro dev server on :4321), `build-docs`, `preview-docs` — the last serves the production build through `wrangler dev`, so the Cloudflare Worker's content negotiation (`Accept: text/markdown` / LLM-bot User-Agent → `.md` twin) is exercised end-to-end the same way it will be in production. - **Per-page `.md` twin + `llms.txt` family via `starlight-llm-tools`** (`docs/astro.config.mjs`, `docs/worker/index.ts`, `docs/wrangler.jsonc`): every doc is also served as raw markdown at `.md` with a navigation header (Section / Subpages / Related / HTML version pointers), plus three concatenated views — `/llms.txt` manifest, `/llms-full.txt` (every page, sidebar-ordered), `/llms-small.txt` (overview pages only). Page-title slot gains a **Copy Markdown** button and **Open with AI** dropdown (Claude / ChatGPT / Cursor). Content negotiation is handled by a Cloudflare Worker that re-exports the `cloudflare-md-router` package's default handler in one line — `Accept: text/markdown` or known LLM-bot User-Agent (GPTBot, ClaudeBot, PerplexityBot, Applebot-Extended, etc.) gets the `.md` twin transparently, with a fall-through to the HTML response when the twin doesn't exist. - **Two extracted plugin packages, MIT-licensed**: [`github.com/Wave-RF/cloudflare-md-router`](https://github.com/Wave-RF/cloudflare-md-router) (Workers handler + `createMdRouter()` factory + extensible bot-UA regex; consumed in `docs/worker/index.ts` as a one-line re-export) and [`github.com/Wave-RF/starlight-llm-tools`](https://github.com/Wave-RF/starlight-llm-tools) (Starlight plugin that auto-injects the four routes, the two components, and the `PageTitle` override; optionally calls into `starlight-glossary/transform` when present). Both pinned via `github:Wave-RF/...` in `docs/package.json`; lockfile pins the resolved commit SHAs. Pre-npm-publish state — switch to normal version specifiers when the packages land on npm. diff --git a/cmd/wavehouse/main.go b/cmd/wavehouse/main.go index cbdccc71..90fab7cf 100644 --- a/cmd/wavehouse/main.go +++ b/cmd/wavehouse/main.go @@ -102,12 +102,19 @@ func run() int { // wanted — Prometheus-only operation (Alloy/scrape, no collector) is a // first-class mode. The OTel SDK MeterProvider is the shared substrate. if cfg.OTel.Enabled || cfg.Prometheus.Enabled { + // Headers already passed config validation; the parser cannot fail + // here for input that satisfied Validate(). + headers, _ := observability.ParseOTelHeaders(cfg.OTel.Headers) otelShutdown, ph, err := observability.InitProvider(ctx, serviceName, observability.ProviderConfig{ Endpoint: cfg.OTel.Addr, + Headers: headers, + TracesEndpoint: cfg.OTel.Traces.Addr, TracesEnabled: cfg.OTel.Enabled && cfg.OTel.Traces.Enabled, TracesSampleRate: cfg.OTel.Traces.SampleRate, + MetricsEndpoint: cfg.OTel.Metrics.Addr, MetricsEnabled: cfg.OTel.Enabled && cfg.OTel.Metrics.Enabled, PrometheusEnabled: cfg.Prometheus.Enabled, + LogsEndpoint: cfg.OTel.Logs.Addr, LogsEnabled: cfg.OTel.Enabled && cfg.OTel.Logs.Enabled, }) if err != nil { diff --git a/docs/src/content/docs/configuration.md b/docs/src/content/docs/configuration.md index 2bfbd6cd..d963f095 100644 --- a/docs/src/content/docs/configuration.md +++ b/docs/src/content/docs/configuration.md @@ -113,16 +113,22 @@ The master switch is `otel.enabled`. When `true`, each signal (traces/metrics/lo **Sampling rates apply only to the OTLP push path.** Stdout always emits 100% of records — operators using a scraping-style pipeline (Promtail/Grafana Alloy → Loki, Vector, Fluent Bit, etc.) set the collection rate at the scraper, not the application. WaveHouse pushes telemetry to an OTel collector; the scraper world owns its own ingest policy. If you want to throttle OTLP volume for cost, lower the rates below. If you want to throttle Loki/Datadog Logs/etc., do it at that pipeline. -**TLS / direct-to-cloud limitation.** The OTLP exporters currently use `WithInsecure()` — plaintext gRPC only. WaveHouse cannot ship directly to TLS-protected OTLP endpoints (Grafana Cloud's OTLP gateway, Honeycomb, Datadog OTLP, etc.). The standard workaround is a sidecar collector (the OTel collector or Grafana Alloy) on `127.0.0.1:4317` that receives our plaintext OTLP and re-exports to the cloud endpoint with TLS + auth headers configured locally. Tracked in #97. +**TLS + direct-to-cloud OTLP.** `otel.addr` is scheme-aware: `https://` selects TLS (system root CAs), `http://` or a bare `host:port` stays plaintext gRPC. Combined with `otel.headers` for auth, WaveHouse can ship telemetry straight to TLS-protected cloud endpoints (Grafana Cloud's OTLP gateway, Honeycomb, Datadog OTLP, etc.) without a sidecar collector. A sidecar is still useful for egress queuing, batching, and tail-based sampling — it's just no longer required. See [Deployment → Direct-to-cloud OTLP](./deployment.md#direct-to-cloud-otlp) for worked examples. + +If different signals need different gateway hosts (Grafana Cloud's distinct trace / metric / log endpoints, say), set `otel.{traces,metrics,logs}.addr` to override the default for that signal — empty means inherit from `otel.addr`. | YAML Key | Env Var | Default | Description | | -------- | ------- | ------- | ----------- | | `otel.enabled` | `WH_OTEL_ENABLED` | `false` | Master switch. When `false`, no signals are initialized regardless of the sub-toggles below. | -| `otel.addr` | `WH_OTEL_ADDR` | `127.0.0.1:4317` | OTLP gRPC endpoint used by every enabled signal. Plain `host:port` — no scheme, plaintext gRPC only (see TLS note above). See `deployments/signoz/` for a local collector setup. | +| `otel.addr` | `WH_OTEL_ADDR` | `127.0.0.1:4317` | Default OTLP gRPC endpoint. Accepts `host:port` (plaintext), `http://host:port` (plaintext), or `https://host:port` (TLS via system root CAs). A trailing URL path is tolerated and stripped — gRPC routes by service name. See `deployments/signoz/` for a local plaintext collector setup. | +| `otel.headers` | `WH_OTEL_HEADERS` | *(empty)* | Comma-separated `key=value` pairs applied as gRPC metadata to every OTLP export — the standard auth knob for cloud endpoints (`authorization=Basic `, `x-honeycomb-team=`). Values may contain `=`; only the first `=` per segment splits key from value (base64 padding round-trips). Validated at config load. | | `otel.traces.enabled` | `WH_OTEL_TRACES_ENABLED` | `true` | Export traces via OTLP gRPC. | +| `otel.traces.addr` | `WH_OTEL_TRACES_ADDR` | *(empty)* | Per-signal override for `otel.addr` (same scheme rules). Empty means inherit. | | `otel.traces.sample_rate` | `WH_OTEL_TRACES_SAMPLE_RATE` | `1.0` | Head-based trace sampling rate in `[0.0, 1.0]`. `1.0` exports every trace; `0.0` exports none. Defaults to 100% (matches the OpenTelemetry SDK default); lower it for high-QPS production services where collector or backend cost is a concern. Best practice is "100% at the source, downsample at the collector" via tail-based sampling. Validated at config load. | | `otel.metrics.enabled` | `WH_OTEL_METRICS_ENABLED` | `true` | Export metrics + Go runtime metrics via OTLP gRPC. Periodic reader interval is fixed at 15s. Metrics are pre-aggregated so there is no sampling knob. | +| `otel.metrics.addr` | `WH_OTEL_METRICS_ADDR` | *(empty)* | Per-signal override for `otel.addr`. Empty means inherit. | | `otel.logs.enabled` | `WH_OTEL_LOGS_ENABLED` | `true` | Export logs via OTLP gRPC. Disabling this leaves stdout logging untouched — the OTel logger provider is simply not registered. | +| `otel.logs.addr` | `WH_OTEL_LOGS_ADDR` | *(empty)* | Per-signal override for `otel.addr`. Empty means inherit. | | `otel.logs.sample_rate` | `WH_OTEL_LOGS_SAMPLE_RATE` | `1.0` | OTLP export rate for `DEBUG`/`INFO` records, in `[0.0, 1.0]`. Validated at config load. `WARN` and `ERROR` records always export at 100% — dropping them silently during incidents is too dangerous to expose as a knob. **Stdout always receives 100% of records regardless of this rate** (see the scraper note above). | ### Prometheus diff --git a/docs/src/content/docs/deployment.md b/docs/src/content/docs/deployment.md index 2f6b8bd4..6f5a2ac8 100644 --- a/docs/src/content/docs/deployment.md +++ b/docs/src/content/docs/deployment.md @@ -314,9 +314,48 @@ Set `otel.enabled: true` (or `WH_OTEL_ENABLED=true`) and point `otel.addr` at th WaveHouse **pushes** to an OTel collector; scraping-style pipelines (Promtail/Grafana Alloy → Loki, Vector, Fluent Bit) read stdout directly and own their own sample rates. The `otel.{traces,logs}.sample_rate` knobs apply only to the OTLP push path. Stdout always emits 100%. The logger fans out to both stdout and OTLP, so stdout output never disappears regardless of collector state. gRPC exporters are lazy, so an unreachable collector does not block startup — transient export errors are surfaced via the OTel SDK's error handler instead. -### Pattern: SigNoz / Honeycomb / OTel-native backends +### Pattern: SigNoz / OTel-native backends (local collector) -Point `otel.addr` at the OTLP gRPC endpoint. All three signals (traces, metrics, logs) push through the same connection. This is the default and the simplest setup. +Point `otel.addr` at a plaintext OTLP gRPC endpoint. All three signals (traces, metrics, logs) push through the same connection. This is the default and the simplest setup. + +```yaml +otel: + enabled: true + addr: 127.0.0.1:4317 # bare host:port — plaintext gRPC +``` + +### Pattern: Direct-to-cloud OTLP (Honeycomb, Grafana Cloud, Datadog OTLP, etc.) + +`otel.addr` is scheme-aware: `https://` selects TLS, `http://` or no scheme stays plaintext. Combine with `otel.headers` for the per-RPC auth that every cloud OTLP gateway expects. No sidecar required — a sidecar is still useful for egress queuing, batching, and tail-based sampling, but is no longer the only way to get TLS + auth. + +**Honeycomb (single endpoint, per-RPC auth):** + +```bash +export WH_OTEL_ENABLED=true +export WH_OTEL_ADDR=https://api.honeycomb.io:443 +export WH_OTEL_HEADERS=x-honeycomb-team=YOUR_API_KEY +``` + +**Grafana Cloud OTLP gateway (Basic auth):** + +```bash +export WH_OTEL_ENABLED=true +export WH_OTEL_ADDR=https://otlp-gateway-prod-us-east-0.grafana.net:443 +# instanceID:token, base64-encoded; backslash-escape the space if your shell needs it +export WH_OTEL_HEADERS=authorization=Basic $(printf '%s' "$INSTANCE_ID:$TOKEN" | base64) +``` + +**Datadog OTLP intake:** + +```bash +export WH_OTEL_ENABLED=true +export WH_OTEL_ADDR=https://otlp.datadoghq.com:4317 +export WH_OTEL_HEADERS=dd-api-key=YOUR_API_KEY +``` + +If different signals need different gateway hosts (Grafana Cloud's traces/metrics/logs endpoints differ in some regions), set `WH_OTEL_TRACES_ADDR` / `WH_OTEL_METRICS_ADDR` / `WH_OTEL_LOGS_ADDR` to override the default per signal. Empty means inherit from `otel.addr`. Headers always apply to every exporter — there is intentionally no per-signal header override. + +mTLS / client-certificate auth is not yet supported; open an issue if you need it. ### Pattern: Grafana Cloud / Mimir / Loki / Tempo via Grafana Alloy diff --git a/internal/config/config.go b/internal/config/config.go index dee0695c..2a66afd4 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -32,9 +32,23 @@ type Config struct { // OTel configures the OpenTelemetry pipeline. `enabled` is the master switch; // when false, no signals are initialized regardless of the per-signal toggles. +// +// Addr accepts plain `host:port` (plaintext gRPC, backward-compat), `http://` +// (also plaintext), or `https://` (TLS — required for direct-to-cloud OTLP). +// A URL path component is tolerated and stripped (gRPC ignores it). +// +// Headers is a comma-separated list of `key=value` pairs applied to every OTLP +// exporter — the standard knob for auth against cloud endpoints +// (`authorization=Basic `, `x-honeycomb-team=`, etc.). Values may +// contain `=` (only the first `=` per segment splits key from value). +// +// Per-signal Addr overrides (Traces.Addr, Metrics.Addr, Logs.Addr) point one +// signal at a different endpoint than the top-level Addr — useful for Grafana +// Cloud, which uses distinct gateway hosts per signal. Empty means inherit. type OTel struct { Enabled bool `yaml:"enabled" env:"WH_OTEL_ENABLED" env-default:"false"` Addr string `yaml:"addr" env:"WH_OTEL_ADDR" env-default:"127.0.0.1:4317"` + Headers string `yaml:"headers" env:"WH_OTEL_HEADERS"` Traces OTelTraces `yaml:"traces"` Metrics OTelMetrics `yaml:"metrics"` Logs OTelLogs `yaml:"logs"` @@ -43,10 +57,12 @@ type OTel struct { type OTelTraces struct { Enabled bool `yaml:"enabled" env:"WH_OTEL_TRACES_ENABLED" env-default:"true"` SampleRate float64 `yaml:"sample_rate" env:"WH_OTEL_TRACES_SAMPLE_RATE" env-default:"1.0"` + Addr string `yaml:"addr" env:"WH_OTEL_TRACES_ADDR"` } type OTelMetrics struct { - Enabled bool `yaml:"enabled" env:"WH_OTEL_METRICS_ENABLED" env-default:"true"` + Enabled bool `yaml:"enabled" env:"WH_OTEL_METRICS_ENABLED" env-default:"true"` + Addr string `yaml:"addr" env:"WH_OTEL_METRICS_ADDR"` } // Prometheus controls a Prometheus exposition endpoint served alongside (or @@ -78,6 +94,7 @@ type Prometheus struct { type OTelLogs struct { Enabled bool `yaml:"enabled" env:"WH_OTEL_LOGS_ENABLED" env-default:"true"` SampleRate float64 `yaml:"sample_rate" env:"WH_OTEL_LOGS_SAMPLE_RATE" env-default:"1.0"` + Addr string `yaml:"addr" env:"WH_OTEL_LOGS_ADDR"` } type Server struct { @@ -146,6 +163,30 @@ type DLQ struct { Enabled bool `yaml:"enabled" env:"WH_DLQ_ENABLED" env-default:"true"` } +// validateOTelHeaders mirrors observability.ParseOTelHeaders for boot-time +// validation. Kept here (rather than importing observability) so config stays +// at the bottom of the dependency graph. +func validateOTelHeaders(s string) error { + s = strings.TrimSpace(s) + if s == "" { + return nil + } + for _, seg := range strings.Split(s, ",") { + seg = strings.TrimSpace(seg) + if seg == "" { + continue + } + i := strings.IndexByte(seg, '=') + if i < 0 { + return fmt.Errorf("header segment %q missing '='", seg) + } + if strings.TrimSpace(seg[:i]) == "" { + return fmt.Errorf("header segment %q has empty key", seg) + } + } + return nil +} + // Validate checks the loaded configuration for logical consistency. func (c *Config) Validate() error { if c.ClickHouse.HTTPScheme != "http" && c.ClickHouse.HTTPScheme != "https" { @@ -192,6 +233,13 @@ func (c *Config) Validate() error { return fmt.Errorf("otel.logs.sample_rate %g out of range [0.0, 1.0]", r) } } + // Parse headers eagerly so a malformed entry fails at boot rather than + // silently dropping auth in production. The same parser runs again at + // provider init; duplicating the validation here keeps config from + // importing the observability package. + if err := validateOTelHeaders(c.OTel.Headers); err != nil { + return fmt.Errorf("otel.headers: %w", err) + } } // Prometheus exposition is independent of OTel — operators can run it diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 974b0f77..3783b9ad 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -39,6 +39,10 @@ func TestLoad_Defaults(t *testing.T) { assert.True(t, cfg.OTel.Metrics.Enabled) assert.True(t, cfg.OTel.Logs.Enabled) assert.InEpsilon(t, 1.0, cfg.OTel.Logs.SampleRate, 0.0001) + assert.Equal(t, "", cfg.OTel.Headers) + assert.Equal(t, "", cfg.OTel.Traces.Addr) + assert.Equal(t, "", cfg.OTel.Metrics.Addr) + assert.Equal(t, "", cfg.OTel.Logs.Addr) } func TestLoad_FromYAML(t *testing.T) { @@ -312,6 +316,65 @@ func TestValidate_RejectsEmptyOTelAddrWhenEnabled(t *testing.T) { assert.Contains(t, err.Error(), "otel.addr") } +func TestValidate_RejectsMalformedHeaders(t *testing.T) { + t.Parallel() + cases := []struct { + name string + headers string + }{ + {name: "missing equals", headers: "not-a-pair"}, + {name: "empty key", headers: "=value"}, + {name: "mixed valid and invalid", headers: "a=1,broken"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + cfg := Config{ + Server: Server{Port: 8080}, + ClickHouse: ClickHouse{HTTPScheme: "http"}, + Schema: Schema{RefreshInterval: 60}, + OTel: OTel{ + Enabled: true, + Addr: "127.0.0.1:4317", + Headers: tc.headers, + }, + } + err := cfg.Validate() + require.Error(t, err) + assert.Contains(t, err.Error(), "otel.headers") + }) + } +} + +func TestValidate_AcceptsValidHeaders(t *testing.T) { + t.Parallel() + cfg := Config{ + Server: Server{Port: 8080}, + ClickHouse: ClickHouse{HTTPScheme: "http"}, + Schema: Schema{RefreshInterval: 60}, + OTel: OTel{ + Enabled: true, + Addr: "127.0.0.1:4317", + Headers: "authorization=Basic dXNlcjpwYXNz==,x-tenant-id=acme", + }, + } + assert.NoError(t, cfg.Validate()) +} + +func TestValidate_HeadersIgnoredWhenOTelDisabled(t *testing.T) { + t.Parallel() + // Iterating on yaml shouldn't fail boot for headers in a disabled block. + cfg := Config{ + Server: Server{Port: 8080}, + ClickHouse: ClickHouse{HTTPScheme: "http"}, + Schema: Schema{RefreshInterval: 60}, + OTel: OTel{ + Enabled: false, + Headers: "not-a-pair", + }, + } + assert.NoError(t, cfg.Validate()) +} + func TestValidate_SampleRatesIgnoredWhenSignalDisabled(t *testing.T) { t.Parallel() // Same idea one level down — when the master switch is on but the individual diff --git a/internal/observability/endpoint.go b/internal/observability/endpoint.go new file mode 100644 index 00000000..89de56b1 --- /dev/null +++ b/internal/observability/endpoint.go @@ -0,0 +1,76 @@ +package observability + +import ( + "crypto/tls" + "fmt" + "strings" +) + +// tlsConfigOrDefault returns the supplied config when non-nil. The OTel SDK's +// credentials.NewTLS expects a non-nil *tls.Config; passing nil panics. An +// empty &tls.Config{} delegates to system defaults (system root CAs, ALPN +// negotiation), which is what production wants for cloud OTLP endpoints. +func tlsConfigOrDefault(c *tls.Config) *tls.Config { + if c != nil { + return c + } + return &tls.Config{} +} + +// ParseEndpoint splits an OTLP endpoint string into the gRPC dial host and a +// useTLS flag. The OpenTelemetry SDK env-var convention is honored: an +// `https://` prefix selects TLS, while `http://` or a bare `host:port` stays +// plaintext (backward-compat with the prior WithInsecure() default). +// +// A URL path component is tolerated and stripped — gRPC routes by service name +// and ignores the path, so `https://otlp-gateway.example.com/otlp` and +// `https://otlp-gateway.example.com` dial the same way. +func ParseEndpoint(addr string) (host string, useTLS bool) { + switch { + case strings.HasPrefix(addr, "https://"): + useTLS = true + host = strings.TrimPrefix(addr, "https://") + case strings.HasPrefix(addr, "http://"): + host = strings.TrimPrefix(addr, "http://") + default: + host = addr + } + if i := strings.IndexByte(host, '/'); i >= 0 { + host = host[:i] + } + return host, useTLS +} + +// ParseOTelHeaders parses the OpenTelemetry-spec headers env-var format +// (`OTEL_EXPORTER_OTLP_HEADERS`) — comma-separated `key=value` pairs — into a +// map. Whitespace around the key and value is trimmed. Only the first `=` per +// segment splits key from value, so base64 trailing `=` in an Authorization +// header round-trips unchanged. An empty input yields an empty map. +// +// Returns an error (rather than silently dropping the segment) for malformed +// entries so Validate() can fail loud at boot rather than letting a typo +// silently disable auth in production. +func ParseOTelHeaders(s string) (map[string]string, error) { + s = strings.TrimSpace(s) + if s == "" { + return map[string]string{}, nil + } + out := map[string]string{} + for _, seg := range strings.Split(s, ",") { + seg = strings.TrimSpace(seg) + if seg == "" { + continue + } + i := strings.IndexByte(seg, '=') + if i < 0 { + return nil, fmt.Errorf("header segment %q missing '='", seg) + } + key := strings.TrimSpace(seg[:i]) + val := strings.TrimSpace(seg[i+1:]) + if key == "" { + return nil, fmt.Errorf("header segment %q has empty key", seg) + } + out[key] = val + } + return out, nil +} diff --git a/internal/observability/endpoint_test.go b/internal/observability/endpoint_test.go new file mode 100644 index 00000000..b59da76c --- /dev/null +++ b/internal/observability/endpoint_test.go @@ -0,0 +1,65 @@ +package observability + +import ( + "reflect" + "testing" +) + +func TestParseEndpoint(t *testing.T) { + cases := []struct { + name string + in string + wantHost string + wantTLS bool + }{ + {name: "bare host port", in: "otlp.example.com:4317", wantHost: "otlp.example.com:4317"}, + {name: "http scheme", in: "http://otlp.example.com:4317", wantHost: "otlp.example.com:4317"}, + {name: "https scheme", in: "https://otlp.example.com:443", wantHost: "otlp.example.com:443", wantTLS: true}, + {name: "https with path", in: "https://otlp-gateway.example.com/otlp", wantHost: "otlp-gateway.example.com", wantTLS: true}, + {name: "https with port and path", in: "https://otlp.example.com:443/v1/traces", wantHost: "otlp.example.com:443", wantTLS: true}, + {name: "empty", in: "", wantHost: ""}, + {name: "ipv4 plaintext", in: "127.0.0.1:4317", wantHost: "127.0.0.1:4317"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + gotHost, gotTLS := ParseEndpoint(tc.in) + if gotHost != tc.wantHost || gotTLS != tc.wantTLS { + t.Fatalf("ParseEndpoint(%q) = (%q, %v); want (%q, %v)", tc.in, gotHost, gotTLS, tc.wantHost, tc.wantTLS) + } + }) + } +} + +func TestParseOTelHeaders(t *testing.T) { + cases := []struct { + name string + in string + want map[string]string + wantErr bool + }{ + {name: "empty", in: "", want: map[string]string{}}, + {name: "whitespace only", in: " ", want: map[string]string{}}, + {name: "one pair", in: "x-honeycomb-team=abc123", want: map[string]string{"x-honeycomb-team": "abc123"}}, + {name: "two pairs", in: "a=1,b=2", want: map[string]string{"a": "1", "b": "2"}}, + {name: "whitespace around segments", in: " a = 1 , b = 2 ", want: map[string]string{"a": "1", "b": "2"}}, + {name: "value contains equals", in: "authorization=Basic dXNlcjpwYXNz==", want: map[string]string{"authorization": "Basic dXNlcjpwYXNz=="}}, + {name: "trailing comma tolerated", in: "a=1,", want: map[string]string{"a": "1"}}, + {name: "missing equals", in: "not-a-pair", wantErr: true}, + {name: "empty key", in: "=value", wantErr: true}, + {name: "empty value allowed", in: "k=", want: map[string]string{"k": ""}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := ParseOTelHeaders(tc.in) + if (err != nil) != tc.wantErr { + t.Fatalf("ParseOTelHeaders(%q) err = %v; wantErr = %v", tc.in, err, tc.wantErr) + } + if tc.wantErr { + return + } + if !reflect.DeepEqual(got, tc.want) { + t.Fatalf("ParseOTelHeaders(%q) = %v; want %v", tc.in, got, tc.want) + } + }) + } +} diff --git a/internal/observability/provider.go b/internal/observability/provider.go index 7ae2da03..2210735c 100644 --- a/internal/observability/provider.go +++ b/internal/observability/provider.go @@ -2,6 +2,7 @@ package observability import ( "context" + "crypto/tls" "errors" "log/slog" "net/http" @@ -24,6 +25,7 @@ import ( "go.opentelemetry.io/otel/sdk/resource" "go.opentelemetry.io/otel/sdk/trace" semconv "go.opentelemetry.io/otel/semconv/v1.24.0" + "google.golang.org/grpc/credentials" ) // runtimeStartOnce gates the `runtime.Start` call in InitProvider so that @@ -42,15 +44,39 @@ var runtimeStartOnce sync.Once // set — the underlying OTel MeterProvider is the shared substrate. When // PrometheusEnabled is true InitProvider returns a non-nil promHandler. // -// Endpoint is the OTLP gRPC target. It is dialed only by the OTLP exporters -// (traces / metrics-OTLP / logs); Prometheus-only operation leaves it unused. +// Endpoint is the OTLP gRPC target consulted by every enabled OTLP exporter +// unless that signal sets its own TracesEndpoint / MetricsEndpoint / +// LogsEndpoint override (empty means inherit). Each endpoint is parsed via +// ParseEndpoint, so `https://` selects TLS while bare `host:port` or `http://` +// stays plaintext for backward compatibility. +// +// Headers (set, key=value pairs already parsed by config) is applied to every +// OTLP exporter — the standard knob for auth against cloud endpoints. +// +// TLSConfig is a test-only escape hatch: when non-nil it overrides the default +// system-roots TLS config for `https://` endpoints. Production code leaves it +// nil; only the FakeOTLPTLS-driven integration tests populate it (with +// InsecureSkipVerify against an ephemeral self-signed cert). type ProviderConfig struct { Endpoint string + Headers map[string]string + TracesEndpoint string TracesEnabled bool TracesSampleRate float64 + MetricsEndpoint string MetricsEnabled bool PrometheusEnabled bool + LogsEndpoint string LogsEnabled bool + TLSConfig *tls.Config +} + +// pickEndpoint returns override if set, otherwise fallback. +func pickEndpoint(override, fallback string) string { + if override != "" { + return override + } + return fallback } // InitProvider sets up the OpenTelemetry pipeline, registering only the @@ -115,10 +141,17 @@ func InitProvider(ctx context.Context, serviceName string, cfg ProviderConfig) ( ) if cfg.TracesEnabled { - traceExporter, err := otlptracegrpc.New(ctx, - otlptracegrpc.WithEndpoint(cfg.Endpoint), - otlptracegrpc.WithInsecure(), - ) + host, useTLS := ParseEndpoint(pickEndpoint(cfg.TracesEndpoint, cfg.Endpoint)) + opts := []otlptracegrpc.Option{otlptracegrpc.WithEndpoint(host)} + if useTLS { + opts = append(opts, otlptracegrpc.WithTLSCredentials(credentials.NewTLS(tlsConfigOrDefault(cfg.TLSConfig)))) + } else { + opts = append(opts, otlptracegrpc.WithInsecure()) + } + if len(cfg.Headers) > 0 { + opts = append(opts, otlptracegrpc.WithHeaders(cfg.Headers)) + } + traceExporter, err := otlptracegrpc.New(ctx, opts...) if err != nil { handleErr(err) return shutdown, nil, err @@ -138,10 +171,17 @@ func InitProvider(ctx context.Context, serviceName string, cfg ProviderConfig) ( readers := []metric.Reader{} if cfg.MetricsEnabled { - metricExporter, err := otlpmetricgrpc.New(ctx, - otlpmetricgrpc.WithEndpoint(cfg.Endpoint), - otlpmetricgrpc.WithInsecure(), - ) + host, useTLS := ParseEndpoint(pickEndpoint(cfg.MetricsEndpoint, cfg.Endpoint)) + opts := []otlpmetricgrpc.Option{otlpmetricgrpc.WithEndpoint(host)} + if useTLS { + opts = append(opts, otlpmetricgrpc.WithTLSCredentials(credentials.NewTLS(tlsConfigOrDefault(cfg.TLSConfig)))) + } else { + opts = append(opts, otlpmetricgrpc.WithInsecure()) + } + if len(cfg.Headers) > 0 { + opts = append(opts, otlpmetricgrpc.WithHeaders(cfg.Headers)) + } + metricExporter, err := otlpmetricgrpc.New(ctx, opts...) if err != nil { handleErr(err) return shutdown, nil, err @@ -195,10 +235,17 @@ func InitProvider(ctx context.Context, serviceName string, cfg ProviderConfig) ( } if cfg.LogsEnabled { - logExporter, err := otlploggrpc.New(ctx, - otlploggrpc.WithEndpoint(cfg.Endpoint), - otlploggrpc.WithInsecure(), - ) + host, useTLS := ParseEndpoint(pickEndpoint(cfg.LogsEndpoint, cfg.Endpoint)) + opts := []otlploggrpc.Option{otlploggrpc.WithEndpoint(host)} + if useTLS { + opts = append(opts, otlploggrpc.WithTLSCredentials(credentials.NewTLS(tlsConfigOrDefault(cfg.TLSConfig)))) + } else { + opts = append(opts, otlploggrpc.WithInsecure()) + } + if len(cfg.Headers) > 0 { + opts = append(opts, otlploggrpc.WithHeaders(cfg.Headers)) + } + logExporter, err := otlploggrpc.New(ctx, opts...) if err != nil { handleErr(err) return shutdown, nil, err diff --git a/internal/testutil/otlp.go b/internal/testutil/otlp.go index 1fea94bd..518018df 100644 --- a/internal/testutil/otlp.go +++ b/internal/testutil/otlp.go @@ -15,15 +15,28 @@ // // Always call shutdown before asserting counts — the OTel SDK batches // exports and only drains on shutdown (or after the batch timeout). +// +// For TLS verification, use NewFakeOTLPTLS which mints an ephemeral self-signed +// cert and exposes TLSConfig() for a matching client-side config. package testutil import ( "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "math/big" "net" "sync" "testing" + "time" "google.golang.org/grpc" + "google.golang.org/grpc/credentials" + "google.golang.org/grpc/metadata" collogspb "go.opentelemetry.io/proto/otlp/collector/logs/v1" colmetricspb "go.opentelemetry.io/proto/otlp/collector/metrics/v1" @@ -37,19 +50,51 @@ import ( // log Export RPCs and captures every received payload. Cleanup is registered // on the *testing.T automatically. type FakeOTLP struct { - addr string - server *grpc.Server + addr string + server *grpc.Server + tlsConfig *tls.Config // non-nil only when constructed via NewFakeOTLPTLS mu sync.Mutex traces []*tracepb.ResourceSpans metrics []*metricspb.ResourceMetrics logs []*logspb.ResourceLogs + + // Captured gRPC request metadata, indexed by signal. Each Export call + // appends an entry. Tests assert on auth/header propagation through these. + traceHeaders []metadata.MD + metricHeaders []metadata.MD + logHeaders []metadata.MD } // NewFakeOTLP binds the receiver to 127.0.0.1 on a random port and starts -// serving. The server is stopped automatically when the test ends. +// serving plaintext gRPC. The server is stopped automatically when the test +// ends. func NewFakeOTLP(t *testing.T) *FakeOTLP { t.Helper() + return newFakeOTLP(t, nil) +} + +// NewFakeOTLPTLS is the TLS variant: an ephemeral self-signed cert (SAN +// 127.0.0.1) is generated, and the server listens with that cert. The matching +// client config is available via TLSConfig() — wire it into ProviderConfig so +// the OTel exporters trust the cert. Production code never sets ProviderConfig.TLSConfig; +// only this test path does. +func NewFakeOTLPTLS(t *testing.T) *FakeOTLP { + t.Helper() + + cert, clientCfg := ephemeralTLSPair(t) + serverCfg := &tls.Config{Certificates: []tls.Certificate{cert}} + + return newFakeOTLP(t, &fakeOTLPTLS{server: serverCfg, client: clientCfg}) +} + +type fakeOTLPTLS struct { + server *tls.Config + client *tls.Config +} + +func newFakeOTLP(t *testing.T, tlsCfg *fakeOTLPTLS) *FakeOTLP { + t.Helper() var lc net.ListenConfig lis, err := lc.Listen(t.Context(), "tcp", "127.0.0.1:0") @@ -57,10 +102,14 @@ func NewFakeOTLP(t *testing.T) *FakeOTLP { t.Fatalf("FakeOTLP listen: %v", err) } - r := &FakeOTLP{ - addr: lis.Addr().String(), - server: grpc.NewServer(), + var serverOpts []grpc.ServerOption + r := &FakeOTLP{addr: lis.Addr().String()} + if tlsCfg != nil { + serverOpts = append(serverOpts, grpc.Creds(credentials.NewTLS(tlsCfg.server))) + r.tlsConfig = tlsCfg.client } + r.server = grpc.NewServer(serverOpts...) + coltracepb.RegisterTraceServiceServer(r.server, &fakeTraceServer{parent: r}) colmetricspb.RegisterMetricsServiceServer(r.server, &fakeMetricsServer{parent: r}) collogspb.RegisterLogsServiceServer(r.server, &fakeLogsServer{parent: r}) @@ -76,10 +125,55 @@ func NewFakeOTLP(t *testing.T) *FakeOTLP { return r } +// ephemeralTLSPair mints a one-shot ECDSA self-signed cert valid for 127.0.0.1 +// and returns it along with a client tls.Config that trusts only this cert. +func ephemeralTLSPair(t *testing.T) (tls.Certificate, *tls.Config) { + t.Helper() + + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("FakeOTLPTLS: generate key: %v", err) + } + serial, err := rand.Int(rand.Reader, big.NewInt(1<<62)) + if err != nil { + t.Fatalf("FakeOTLPTLS: serial: %v", err) + } + tmpl := x509.Certificate{ + SerialNumber: serial, + Subject: pkix.Name{CommonName: "FakeOTLP"}, + NotBefore: time.Now().Add(-time.Minute), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + DNSNames: []string{"localhost"}, + } + der, err := x509.CreateCertificate(rand.Reader, &tmpl, &tmpl, &priv.PublicKey, priv) + if err != nil { + t.Fatalf("FakeOTLPTLS: create cert: %v", err) + } + cert := tls.Certificate{ + Certificate: [][]byte{der}, + PrivateKey: priv, + } + parsed, err := x509.ParseCertificate(der) + if err != nil { + t.Fatalf("FakeOTLPTLS: parse cert: %v", err) + } + pool := x509.NewCertPool() + pool.AddCert(parsed) + return cert, &tls.Config{RootCAs: pool, ServerName: "127.0.0.1"} +} + // Addr returns the listener address (e.g. "127.0.0.1:42891") suitable for // passing to observability.ProviderConfig.Endpoint. func (r *FakeOTLP) Addr() string { return r.addr } +// TLSConfig returns the client-side tls.Config that trusts this server's +// ephemeral cert. Returns nil when the server was constructed via the plaintext +// NewFakeOTLP. +func (r *FakeOTLP) TLSConfig() *tls.Config { return r.tlsConfig } + // SpanCount returns the total number of spans received across all RPCs. // Spans are flattened across resource and scope groupings. func (r *FakeOTLP) SpanCount() int { @@ -139,6 +233,27 @@ func (r *FakeOTLP) LogCountAtLevel(minSeverity int32) int { return n } +// LastTraceHeaders returns the gRPC metadata captured from the most recent +// trace Export RPC, or nil if none. +func (r *FakeOTLP) LastTraceHeaders() metadata.MD { return lastMD(&r.mu, r.traceHeaders) } + +// LastMetricHeaders returns the gRPC metadata captured from the most recent +// metric Export RPC, or nil if none. +func (r *FakeOTLP) LastMetricHeaders() metadata.MD { return lastMD(&r.mu, r.metricHeaders) } + +// LastLogHeaders returns the gRPC metadata captured from the most recent log +// Export RPC, or nil if none. +func (r *FakeOTLP) LastLogHeaders() metadata.MD { return lastMD(&r.mu, r.logHeaders) } + +func lastMD(mu *sync.Mutex, slice []metadata.MD) metadata.MD { + mu.Lock() + defer mu.Unlock() + if len(slice) == 0 { + return nil + } + return slice[len(slice)-1] +} + // Reset clears all captured payloads. Useful between test phases. func (r *FakeOTLP) Reset() { r.mu.Lock() @@ -146,6 +261,9 @@ func (r *FakeOTLP) Reset() { r.traces = nil r.metrics = nil r.logs = nil + r.traceHeaders = nil + r.metricHeaders = nil + r.logHeaders = nil } type fakeTraceServer struct { @@ -153,9 +271,11 @@ type fakeTraceServer struct { parent *FakeOTLP } -func (s *fakeTraceServer) Export(_ context.Context, req *coltracepb.ExportTraceServiceRequest) (*coltracepb.ExportTraceServiceResponse, error) { +func (s *fakeTraceServer) Export(ctx context.Context, req *coltracepb.ExportTraceServiceRequest) (*coltracepb.ExportTraceServiceResponse, error) { + md, _ := metadata.FromIncomingContext(ctx) s.parent.mu.Lock() s.parent.traces = append(s.parent.traces, req.GetResourceSpans()...) + s.parent.traceHeaders = append(s.parent.traceHeaders, md.Copy()) s.parent.mu.Unlock() return &coltracepb.ExportTraceServiceResponse{}, nil } @@ -165,9 +285,11 @@ type fakeMetricsServer struct { parent *FakeOTLP } -func (s *fakeMetricsServer) Export(_ context.Context, req *colmetricspb.ExportMetricsServiceRequest) (*colmetricspb.ExportMetricsServiceResponse, error) { +func (s *fakeMetricsServer) Export(ctx context.Context, req *colmetricspb.ExportMetricsServiceRequest) (*colmetricspb.ExportMetricsServiceResponse, error) { + md, _ := metadata.FromIncomingContext(ctx) s.parent.mu.Lock() s.parent.metrics = append(s.parent.metrics, req.GetResourceMetrics()...) + s.parent.metricHeaders = append(s.parent.metricHeaders, md.Copy()) s.parent.mu.Unlock() return &colmetricspb.ExportMetricsServiceResponse{}, nil } @@ -177,9 +299,11 @@ type fakeLogsServer struct { parent *FakeOTLP } -func (s *fakeLogsServer) Export(_ context.Context, req *collogspb.ExportLogsServiceRequest) (*collogspb.ExportLogsServiceResponse, error) { +func (s *fakeLogsServer) Export(ctx context.Context, req *collogspb.ExportLogsServiceRequest) (*collogspb.ExportLogsServiceResponse, error) { + md, _ := metadata.FromIncomingContext(ctx) s.parent.mu.Lock() s.parent.logs = append(s.parent.logs, req.GetResourceLogs()...) + s.parent.logHeaders = append(s.parent.logHeaders, md.Copy()) s.parent.mu.Unlock() return &collogspb.ExportLogsServiceResponse{}, nil } diff --git a/tests/integration/otel_test.go b/tests/integration/otel_test.go index aee9843b..77644a6a 100644 --- a/tests/integration/otel_test.go +++ b/tests/integration/otel_test.go @@ -313,3 +313,121 @@ func TestOTel_UnreachableEndpoint_DoesNotBlockStartupOrEmits(t *testing.T) { _ = shutdown(drainCtx) }() } + +// TestOTel_TLSPath_Traces locks in the https:// → TLS dial path. The fake +// receiver listens with an ephemeral self-signed cert; the exporter is given +// the matching client tls.Config via ProviderConfig.TLSConfig. If TLS wiring +// regresses (e.g. someone re-adds WithInsecure unconditionally), the dial +// will TLS-handshake against a plaintext server and the export drops. +func TestOTel_TLSPath_Traces(t *testing.T) { + guardOTelGlobals(t) + r := testutil.NewFakeOTLPTLS(t) + + shutdown, _ := initAndShutdown(t, observability.ProviderConfig{ + Endpoint: "https://" + r.Addr(), + TLSConfig: r.TLSConfig(), + TracesEnabled: true, + TracesSampleRate: 1.0, + }) + + _, span := otel.Tracer("test").Start(context.Background(), "tls-op") + span.End() + + drainCtx, drainCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer drainCancel() + require.NoError(t, shutdown(drainCtx)) + + assert.Equal(t, 1, r.SpanCount(), "TLS path must deliver the span end-to-end") +} + +// TestOTel_Headers_AppliedToAllSignals verifies that ProviderConfig.Headers +// propagates as gRPC metadata on every OTLP exporter (traces, metrics, logs). +// Direct-to-cloud auth depends on this — Honeycomb/Grafana Cloud both +// authenticate per-RPC via a header, so a single missing exporter would 401 +// silently for that signal. +func TestOTel_Headers_AppliedToAllSignals(t *testing.T) { + guardOTelGlobals(t) + r := testutil.NewFakeOTLP(t) + + headers := map[string]string{ + "authorization": "Bearer test-token", + "x-honeycomb-team": "abc123", + } + shutdown, _ := initAndShutdown(t, observability.ProviderConfig{ + Endpoint: r.Addr(), + Headers: headers, + TracesEnabled: true, + TracesSampleRate: 1.0, + MetricsEnabled: true, + LogsEnabled: true, + }) + + // Emit one of each signal. + _, span := otel.Tracer("test").Start(context.Background(), "auth-op") + span.End() + + counter, err := otel.GetMeterProvider().Meter("test").Int64Counter("hdr_counter") + require.NoError(t, err) + counter.Add(context.Background(), 1) + + lvl := &slog.LevelVar{} + lvl.Set(slog.LevelInfo) + logger := observability.NewLogger("wavehouse-test", lvl, true, 1.0) + logger.Info("auth-log") + + drainCtx, drainCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer drainCancel() + require.NoError(t, shutdown(drainCtx)) + + // gRPC metadata keys are lowercased on the wire — assert in lowercase. + for _, sig := range []struct { + name string + md func() []string + }{ + {"traces", func() []string { return r.LastTraceHeaders().Get("authorization") }}, + {"metrics", func() []string { return r.LastMetricHeaders().Get("authorization") }}, + {"logs", func() []string { return r.LastLogHeaders().Get("authorization") }}, + } { + t.Run(sig.name, func(t *testing.T) { + vals := sig.md() + require.NotEmpty(t, vals, "%s exporter dropped the authorization header", sig.name) + assert.Equal(t, "Bearer test-token", vals[0]) + }) + } + // Spot-check the second header on at least one signal — same map flows + // through all three so one check confirms multi-header support. + assert.Equal(t, []string{"abc123"}, r.LastTraceHeaders().Get("x-honeycomb-team")) +} + +// TestOTel_PerSignalEndpoint_Override sends traces to one receiver and metrics +// to another by setting TracesEndpoint on top of a default Endpoint. Grafana +// Cloud's distinct gateway hosts per signal are the headline use case. +func TestOTel_PerSignalEndpoint_Override(t *testing.T) { + guardOTelGlobals(t) + rDefault := testutil.NewFakeOTLP(t) + rTraces := testutil.NewFakeOTLP(t) + + shutdown, _ := initAndShutdown(t, observability.ProviderConfig{ + Endpoint: rDefault.Addr(), // metrics + logs land here + TracesEndpoint: rTraces.Addr(), // traces override + TracesEnabled: true, + TracesSampleRate: 1.0, + MetricsEnabled: true, + }) + + _, span := otel.Tracer("test").Start(context.Background(), "split-op") + span.End() + + counter, err := otel.GetMeterProvider().Meter("test").Int64Counter("split_counter") + require.NoError(t, err) + counter.Add(context.Background(), 1) + + drainCtx, drainCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer drainCancel() + require.NoError(t, shutdown(drainCtx)) + + assert.Equal(t, 1, rTraces.SpanCount(), "trace endpoint should have received the span") + assert.Zero(t, rDefault.SpanCount(), "default endpoint must NOT receive traces when overridden") + assert.GreaterOrEqual(t, rDefault.MetricCount(), 1, "default endpoint should receive metrics (no override)") + assert.Zero(t, rTraces.MetricCount(), "trace endpoint should NOT receive metrics") +} From 3f38b61ce891b5b236fe73a6a2fcca03c63ba202 Mon Sep 17 00:00:00 2001 From: Jack Woods Date: Thu, 14 May 2026 11:23:17 -0400 Subject: [PATCH 02/17] fix(observability): address review feedback for #97 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consolidates the cloud-OTLP review-cycle changes that landed on top of the original feature commit. - internal/observability/endpoint_test.go: migrate from reflect.DeepEqual + t.Fatalf to testify (require.Equal / require.Error / require.NoError) to match the rest of the suite. - internal/config/config_test.go: mark TestValidate_RejectsMalformedHeaders subtests t.Parallel() so they don't serialize the package tests. - cmd/wavehouse/main.go: defensive err-check on ParseOTelHeaders. The config validator (validateOTelHeaders) and main's runtime parser (ParseOTelHeaders) are two independent implementations against the same OTel spec; if they ever drift, fail loud at startup with the parse error rather than silently dropping the header map. - internal/config/config.go, internal/observability/endpoint.go: cross-reference "MUST stay in sync with X" comments on the two parsers, naming the drift surface explicitly. Future refactor: extract both to a leaf package both `config` and `observability` can import. - tests/integration/otel_test.go: extend TestOTel_PerSignalEndpoint_Override with a third FakeOTLP receiver for the metrics path and cross-asserts (rTraces.MetricCount==0, rMetrics.SpanCount==0) to catch copy-paste wiring bugs when a new signal type is added. - config.yaml: AGENTS.md §"Documentation & Consistency Sync" rule 10 — add the four `otel.headers` and per-signal `otel.{traces,metrics,logs}.addr` commented-out reference lines so config.yaml matches the new struct tags in internal/config/config.go. - docs/src/content/docs/deployment.md: Datadog example was structurally wrong (public-Datadog hostname doesn't accept direct OTLP). Reworked the Direct-to-cloud section around the actually-supported paths (Honeycomb + Grafana Cloud) and added a new "Pattern: Datadog (via local DDOT Collector)" subsection with host + k8s downward-API examples. Also fixed the Grafana Cloud base64 line (76-char line-wrap was leaking into the header value); switched to `base64 | tr -d '\n'`. - CHANGELOG.md: corrected the OTel `Unreleased / Added` bullet's "direct push to ... Datadog OTLP" claim — Datadog publishes no direct-to-cloud OTLP endpoint, so the entry now says Honeycomb + Grafana Cloud only and points at the DDOT subsection in deployment.md for Datadog operators. --- CHANGELOG.md | 2 +- cmd/wavehouse/main.go | 14 +++++++-- config.yaml | 6 +++- docs/src/content/docs/deployment.md | 38 +++++++++++++++++++------ internal/config/config.go | 9 +++++- internal/config/config_test.go | 1 + internal/observability/endpoint.go | 6 ++++ internal/observability/endpoint_test.go | 17 +++++------ tests/integration/otel_test.go | 24 ++++++++++------ 9 files changed, 85 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae675cdc..99598a3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Dependabot auto-merge no longer slipped past CI on consolidated-pipeline PRs** (`main branch protection` ruleset; `.github/workflows/project-orchestrator.yml`; `.github/workflows/claude-review.yml`; `AGENTS.md`): on commit `93b3206` the previously six-job CI was collapsed into a single job named `CI`, but three places still referenced the old multi-job names — the ruleset's `required_status_checks` (`Validate` + `Admin approval` only — `CI` not listed), the `bot-clean` check in `project-orchestrator.yml` (looking for `Build`/`Validate`/`Lint`/`Test`/`Integration Tests`/`SDK Tests`), and `REQUIRED_CHECKS` in `claude-review.yml` (same six). Net effect: a Dependabot PR opened at T+0 would have its `Validate` and `Admin approval` checks satisfied within ~30s (PR-title workflow + the auto-approval workflow), `gh pr merge --auto` would fire, GitHub would see all *required* checks green, and the PR would squash-merge before `ci.yml` even started running. Pre-consolidation runs (PR #90 etc.) didn't expose this because GitHub's auto-merge waits for in-flight checks as a courtesy, and the multi-job CI was producing checks before auto-merge fired — but with one collapsed `CI` check that didn't start until later, the courtesy wait disappeared. Fix: added `CI` to the ruleset's required-checks (now `Admin approval` / `CI` / `Validate`) via the `gh api PUT /repos/Wave-RF/WaveHouse/rulesets/15353356` round-trip, and updated both bot-clean lists to look for `CI` + `Validate`. AGENTS.md note about the required-check set updated to match. Diagnosis steps recorded inline as code comments in both workflow files so future-me knows where to look if the check name changes again. ### Added -- **Direct-to-cloud OTLP — TLS + auth headers, no sidecar required** (`internal/config/config.go`, `internal/observability/provider.go`, `internal/observability/endpoint.go`, `internal/testutil/otlp.go`, `cmd/wavehouse/main.go`, `tests/integration/otel_test.go`, `docs/src/content/docs/{configuration,deployment}.md`, `AGENTS.md`): closes [#97](https://github.com/Wave-RF/WaveHouse/issues/97). `otel.addr` is now scheme-aware — `https://` selects TLS (system root CAs by default), `http://` or a bare `host:port` stays plaintext gRPC (backward-compat with the prior `WithInsecure()` default). New `otel.headers` (`WH_OTEL_HEADERS`) takes the OTel spec env-var format — comma-separated `key=value`, values may contain `=` so base64 padding round-trips — and applies the parsed map as gRPC metadata to every OTLP exporter (traces, metrics, logs). Together these unlock direct push to Honeycomb / Grafana Cloud / Datadog OTLP without a sidecar. Per-signal endpoint overrides (`otel.{traces,metrics,logs}.addr`) point one signal at a different gateway than the top-level default (Grafana Cloud uses distinct trace/metric/log hosts in some regions); empty means inherit. Headers always apply uniformly — there is intentionally no per-signal header override. Test surface: `FakeOTLP` gains a `NewFakeOTLPTLS(t)` constructor (ephemeral self-signed cert + matching client `*tls.Config`) and per-signal `LastTraceHeaders / LastMetricHeaders / LastLogHeaders` accessors via captured gRPC metadata. New integration tests pin the TLS dial path, headers-on-all-signals propagation, and per-signal endpoint splitting. Validation rejects malformed `otel.headers` (missing `=`, empty key) at boot rather than letting a typo silently disable auth in production. mTLS / client-certificate auth is not yet supported. +- **Direct-to-cloud OTLP — TLS + auth headers, no sidecar required** (`internal/config/config.go`, `internal/observability/provider.go`, `internal/observability/endpoint.go`, `internal/testutil/otlp.go`, `cmd/wavehouse/main.go`, `tests/integration/otel_test.go`, `docs/src/content/docs/{configuration,deployment}.md`, `AGENTS.md`): closes [#97](https://github.com/Wave-RF/WaveHouse/issues/97). `otel.addr` is now scheme-aware — `https://` selects TLS (system root CAs by default), `http://` or a bare `host:port` stays plaintext gRPC (backward-compat with the prior `WithInsecure()` default). New `otel.headers` (`WH_OTEL_HEADERS`) takes the OTel spec env-var format — comma-separated `key=value`, values may contain `=` so base64 padding round-trips — and applies the parsed map as gRPC metadata to every OTLP exporter (traces, metrics, logs). Together these unlock direct push to Honeycomb and Grafana Cloud's OTLP gateway without a sidecar. Datadog does not publish a direct-to-cloud OTLP endpoint — its supported path remains the DDOT Collector embedded in the Datadog Agent (host or k8s), which WaveHouse reaches as a plaintext local OTLP receiver on `127.0.0.1:4317` (no headers — the API-key auth lives on the Agent); see `docs/deployment.md` for the worked example. Per-signal endpoint overrides (`otel.{traces,metrics,logs}.addr`) point one signal at a different gateway than the top-level default (Grafana Cloud uses distinct trace/metric/log hosts in some regions); empty means inherit. Headers always apply uniformly — there is intentionally no per-signal header override. Test surface: `FakeOTLP` gains a `NewFakeOTLPTLS(t)` constructor (ephemeral self-signed cert + matching client `*tls.Config`) and per-signal `LastTraceHeaders / LastMetricHeaders / LastLogHeaders` accessors via captured gRPC metadata. New integration tests pin the TLS dial path, headers-on-all-signals propagation, and per-signal endpoint splitting. Validation rejects malformed `otel.headers` (missing `=`, empty key) at boot rather than letting a typo silently disable auth in production. mTLS / client-certificate auth is not yet supported. - **Astro / Starlight documentation site at `wavehouse.dev`** (`docs/`, `Makefile`, `.github/dependabot.yml`, `.github/workflows/ci.yml`, `.gitignore`, `.vscode/launch.json`): full content + tooling for the public docs (Getting Started, Why WaveHouse, Architecture, API Reference, TypeScript SDK, Configuration, Deployment, Development). Build-time mermaid via `rehype-mermaid` (inline-svg strategy, no client-side JS), LaTeX via `remark-math` + `rehype-katex`, image zoom via `starlight-image-zoom`, expressiveCode with github dark/light + `env`/`dns` Shiki language aliases. PostHog frontend telemetry (key is public by design — embedded in every visitor's browser). `editLink` pointed at `main`; `lastUpdated` on. Sidebar lives in `docs/src/config/sidebar.ts` as the single source of truth, consumed by both Starlight rendering and the LLM-friendly outputs below. Cloudflare Workers + Static Assets deployment via `docs/wrangler.jsonc` (no auto-deploy yet — wired up when the GH Actions workflow lands separately). Make targets stay minimal: `dev-docs` (Astro dev server on :4321), `build-docs`, `preview-docs` — the last serves the production build through `wrangler dev`, so the Cloudflare Worker's content negotiation (`Accept: text/markdown` / LLM-bot User-Agent → `.md` twin) is exercised end-to-end the same way it will be in production. - **Per-page `.md` twin + `llms.txt` family via `starlight-llm-tools`** (`docs/astro.config.mjs`, `docs/worker/index.ts`, `docs/wrangler.jsonc`): every doc is also served as raw markdown at `.md` with a navigation header (Section / Subpages / Related / HTML version pointers), plus three concatenated views — `/llms.txt` manifest, `/llms-full.txt` (every page, sidebar-ordered), `/llms-small.txt` (overview pages only). Page-title slot gains a **Copy Markdown** button and **Open with AI** dropdown (Claude / ChatGPT / Cursor). Content negotiation is handled by a Cloudflare Worker that re-exports the `cloudflare-md-router` package's default handler in one line — `Accept: text/markdown` or known LLM-bot User-Agent (GPTBot, ClaudeBot, PerplexityBot, Applebot-Extended, etc.) gets the `.md` twin transparently, with a fall-through to the HTML response when the twin doesn't exist. - **Two extracted plugin packages, MIT-licensed**: [`github.com/Wave-RF/cloudflare-md-router`](https://github.com/Wave-RF/cloudflare-md-router) (Workers handler + `createMdRouter()` factory + extensible bot-UA regex; consumed in `docs/worker/index.ts` as a one-line re-export) and [`github.com/Wave-RF/starlight-llm-tools`](https://github.com/Wave-RF/starlight-llm-tools) (Starlight plugin that auto-injects the four routes, the two components, and the `PageTitle` override; optionally calls into `starlight-glossary/transform` when present). Both pinned via `github:Wave-RF/...` in `docs/package.json`; lockfile pins the resolved commit SHAs. Pre-npm-publish state — switch to normal version specifiers when the packages land on npm. diff --git a/cmd/wavehouse/main.go b/cmd/wavehouse/main.go index 90fab7cf..8d50a5dc 100644 --- a/cmd/wavehouse/main.go +++ b/cmd/wavehouse/main.go @@ -102,9 +102,17 @@ func run() int { // wanted — Prometheus-only operation (Alloy/scrape, no collector) is a // first-class mode. The OTel SDK MeterProvider is the shared substrate. if cfg.OTel.Enabled || cfg.Prometheus.Enabled { - // Headers already passed config validation; the parser cannot fail - // here for input that satisfied Validate(). - headers, _ := observability.ParseOTelHeaders(cfg.OTel.Headers) + // Validate() ran validateOTelHeaders, which mirrors ParseOTelHeaders — + // in practice the parse always succeeds here. The defensive check + // exists so that any future drift between the two implementations + // surfaces loudly instead of silently shipping OTLP exporters with no + // auth metadata (which would only show up as rejected-on-the-wire + // telemetry, not as a startup error). + headers, err := observability.ParseOTelHeaders(cfg.OTel.Headers) + if err != nil { + logger.Error("otel.headers parse failed after Validate accepted them; refusing to start with bad auth config", "error", err) + return 1 + } otelShutdown, ph, err := observability.InitProvider(ctx, serviceName, observability.ProviderConfig{ Endpoint: cfg.OTel.Addr, Headers: headers, diff --git a/config.yaml b/config.yaml index 38b0b1f7..a3c07b1e 100644 --- a/config.yaml +++ b/config.yaml @@ -19,14 +19,18 @@ server: otel: enabled: false # master switch — set true to export via OTLP gRPC - addr: "127.0.0.1:4317" + addr: "127.0.0.1:4317" # `https://...` → TLS, `http://...` or bare host:port → plaintext + # headers: "" # comma-separated key=value, e.g. "x-honeycomb-team=API_KEY" traces: enabled: true + # addr: "" # per-signal endpoint override; empty inherits otel.addr sample_rate: 1.0 # head-based, [0.0, 1.0]; tune down for high QPS metrics: enabled: true # OTLP push for metrics + # addr: "" # per-signal endpoint override; empty inherits otel.addr logs: enabled: true + # addr: "" # per-signal endpoint override; empty inherits otel.addr sample_rate: 1.0 # DEBUG/INFO OTLP rate; WARN+ always 100%, stdout always 100% # Prometheus exposition is independent of OTel — works on its own (Alloy / diff --git a/docs/src/content/docs/deployment.md b/docs/src/content/docs/deployment.md index 6f5a2ac8..66206f08 100644 --- a/docs/src/content/docs/deployment.md +++ b/docs/src/content/docs/deployment.md @@ -324,7 +324,7 @@ otel: addr: 127.0.0.1:4317 # bare host:port — plaintext gRPC ``` -### Pattern: Direct-to-cloud OTLP (Honeycomb, Grafana Cloud, Datadog OTLP, etc.) +### Pattern: Direct-to-cloud OTLP (Honeycomb, Grafana Cloud) `otel.addr` is scheme-aware: `https://` selects TLS, `http://` or no scheme stays plaintext. Combine with `otel.headers` for the per-RPC auth that every cloud OTLP gateway expects. No sidecar required — a sidecar is still useful for egress queuing, batching, and tail-based sampling, but is no longer the only way to get TLS + auth. @@ -341,21 +341,43 @@ export WH_OTEL_HEADERS=x-honeycomb-team=YOUR_API_KEY ```bash export WH_OTEL_ENABLED=true export WH_OTEL_ADDR=https://otlp-gateway-prod-us-east-0.grafana.net:443 -# instanceID:token, base64-encoded; backslash-escape the space if your shell needs it -export WH_OTEL_HEADERS=authorization=Basic $(printf '%s' "$INSTANCE_ID:$TOKEN" | base64) +# instanceID:token, base64-encoded (tr -d '\n' strips base64's 76-char line wrap) +export WH_OTEL_HEADERS=authorization=Basic $(printf '%s' "$INSTANCE_ID:$TOKEN" | base64 | tr -d '\n') ``` -**Datadog OTLP intake:** +If different signals need different gateway hosts (Grafana Cloud's traces/metrics/logs endpoints differ in some regions), set `WH_OTEL_TRACES_ADDR` / `WH_OTEL_METRICS_ADDR` / `WH_OTEL_LOGS_ADDR` to override the default per signal. Empty means inherit from `otel.addr`. Headers always apply to every exporter — there is intentionally no per-signal header override. + +mTLS / client-certificate auth is not yet supported; open an issue if you need it. + +### Pattern: Datadog (via local DDOT Collector) + +Datadog has no public direct-to-cloud OTLP endpoint — telemetry must transit a local OTLP receiver that re-exports to Datadog over Datadog's own protocol. The recommended receiver is the [DDOT Collector](https://docs.datadoghq.com/opentelemetry/setup/ddot_collector/) (Datadog Distribution of OpenTelemetry Collector), embedded in the Datadog Agent — GA on Kubernetes, Preview on Linux. It exposes a standard OTLP receiver on `4317` (gRPC) and `4318` (HTTP) and forwards to Datadog via the bundled `datadogexporter`. Point WaveHouse at the local receiver — the API-key auth lives on the Agent, not on the OTLP hop, so `WH_OTEL_HEADERS` stays empty. + +**Host (DDOT Collector on the same host as WaveHouse):** ```bash export WH_OTEL_ENABLED=true -export WH_OTEL_ADDR=https://otlp.datadoghq.com:4317 -export WH_OTEL_HEADERS=dd-api-key=YOUR_API_KEY +export WH_OTEL_ADDR=127.0.0.1:4317 # plaintext gRPC; no scheme +# WH_OTEL_HEADERS intentionally unset — DD_API_KEY is on the Agent ``` -If different signals need different gateway hosts (Grafana Cloud's traces/metrics/logs endpoints differ in some regions), set `WH_OTEL_TRACES_ADDR` / `WH_OTEL_METRICS_ADDR` / `WH_OTEL_LOGS_ADDR` to override the default per signal. Empty means inherit from `otel.addr`. Headers always apply to every exporter — there is intentionally no per-signal header override. +**Kubernetes (DDOT Collector via the Datadog Operator or Helm chart):** -mTLS / client-certificate auth is not yet supported; open an issue if you need it. +WaveHouse reaches the Agent on the same node via the downward API: + +```yaml +env: + - name: HOST_IP + valueFrom: + fieldRef: + fieldPath: status.hostIP + - name: WH_OTEL_ENABLED + value: "true" + - name: WH_OTEL_ADDR + value: "$(HOST_IP):4317" +``` + +See Datadog's [DDOT install guide](https://docs.datadoghq.com/opentelemetry/setup/ddot_collector/install/kubernetes/) for enabling the OTLP receiver on the Operator/Helm side. The legacy [OTLP Ingest in the Agent](https://docs.datadoghq.com/opentelemetry/setup/otlp_ingest_in_the_agent/) path (Agent ≥ 6.32 / 7.32) exposes the same receiver shape and works identically from WaveHouse's side — DDOT is the recommended replacement. ### Pattern: Grafana Cloud / Mimir / Loki / Tempo via Grafana Alloy diff --git a/internal/config/config.go b/internal/config/config.go index 2a66afd4..79c83da6 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -165,7 +165,14 @@ type DLQ struct { // validateOTelHeaders mirrors observability.ParseOTelHeaders for boot-time // validation. Kept here (rather than importing observability) so config stays -// at the bottom of the dependency graph. +// at the bottom of the dependency graph — importing observability would +// transitively pull the OTel SDK into every config consumer. +// +// MUST stay in sync with observability.ParseOTelHeaders in +// internal/observability/endpoint.go. cmd/wavehouse/main.go's defensive +// error-check on the post-Validate parse exists to catch any drift loudly, +// but the right answer is to not drift in the first place: any rule change +// here needs the same change there, and vice versa. func validateOTelHeaders(s string) error { s = strings.TrimSpace(s) if s == "" { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 3783b9ad..92658ccc 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -328,6 +328,7 @@ func TestValidate_RejectsMalformedHeaders(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { + t.Parallel() cfg := Config{ Server: Server{Port: 8080}, ClickHouse: ClickHouse{HTTPScheme: "http"}, diff --git a/internal/observability/endpoint.go b/internal/observability/endpoint.go index 89de56b1..50ee49b8 100644 --- a/internal/observability/endpoint.go +++ b/internal/observability/endpoint.go @@ -50,6 +50,12 @@ func ParseEndpoint(addr string) (host string, useTLS bool) { // Returns an error (rather than silently dropping the segment) for malformed // entries so Validate() can fail loud at boot rather than letting a typo // silently disable auth in production. +// +// MUST stay in sync with config.validateOTelHeaders in +// internal/config/config.go. config can't import observability without +// transitively pulling the OTel SDK into every config consumer, so the +// parsing rules are hand-mirrored. Any rule change here needs the same +// change there, and vice versa. func ParseOTelHeaders(s string) (map[string]string, error) { s = strings.TrimSpace(s) if s == "" { diff --git a/internal/observability/endpoint_test.go b/internal/observability/endpoint_test.go index b59da76c..2a65737b 100644 --- a/internal/observability/endpoint_test.go +++ b/internal/observability/endpoint_test.go @@ -1,8 +1,9 @@ package observability import ( - "reflect" "testing" + + "github.com/stretchr/testify/require" ) func TestParseEndpoint(t *testing.T) { @@ -23,9 +24,8 @@ func TestParseEndpoint(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { gotHost, gotTLS := ParseEndpoint(tc.in) - if gotHost != tc.wantHost || gotTLS != tc.wantTLS { - t.Fatalf("ParseEndpoint(%q) = (%q, %v); want (%q, %v)", tc.in, gotHost, gotTLS, tc.wantHost, tc.wantTLS) - } + require.Equal(t, tc.wantHost, gotHost) + require.Equal(t, tc.wantTLS, gotTLS) }) } } @@ -51,15 +51,12 @@ func TestParseOTelHeaders(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { got, err := ParseOTelHeaders(tc.in) - if (err != nil) != tc.wantErr { - t.Fatalf("ParseOTelHeaders(%q) err = %v; wantErr = %v", tc.in, err, tc.wantErr) - } if tc.wantErr { + require.Error(t, err) return } - if !reflect.DeepEqual(got, tc.want) { - t.Fatalf("ParseOTelHeaders(%q) = %v; want %v", tc.in, got, tc.want) - } + require.NoError(t, err) + require.Equal(t, tc.want, got) }) } } diff --git a/tests/integration/otel_test.go b/tests/integration/otel_test.go index 77644a6a..86b9697e 100644 --- a/tests/integration/otel_test.go +++ b/tests/integration/otel_test.go @@ -399,17 +399,23 @@ func TestOTel_Headers_AppliedToAllSignals(t *testing.T) { assert.Equal(t, []string{"abc123"}, r.LastTraceHeaders().Get("x-honeycomb-team")) } -// TestOTel_PerSignalEndpoint_Override sends traces to one receiver and metrics -// to another by setting TracesEndpoint on top of a default Endpoint. Grafana -// Cloud's distinct gateway hosts per signal are the headline use case. +// TestOTel_PerSignalEndpoint_Override verifies that TracesEndpoint and +// MetricsEndpoint each route their own signal to a distinct receiver while +// the default Endpoint sees neither. Grafana Cloud's per-signal gateway hosts +// are the headline use case. Both signal-type overrides go through the same +// pickEndpoint() helper in provider.go, so a copy-paste bug (e.g. the +// metrics exporter being wired to TracesEndpoint) would surface here as +// metrics landing on the wrong receiver — caught by the cross-asserts below. func TestOTel_PerSignalEndpoint_Override(t *testing.T) { guardOTelGlobals(t) rDefault := testutil.NewFakeOTLP(t) rTraces := testutil.NewFakeOTLP(t) + rMetrics := testutil.NewFakeOTLP(t) shutdown, _ := initAndShutdown(t, observability.ProviderConfig{ - Endpoint: rDefault.Addr(), // metrics + logs land here - TracesEndpoint: rTraces.Addr(), // traces override + Endpoint: rDefault.Addr(), // both signals overridden → default sees nothing + TracesEndpoint: rTraces.Addr(), + MetricsEndpoint: rMetrics.Addr(), TracesEnabled: true, TracesSampleRate: 1.0, MetricsEnabled: true, @@ -426,8 +432,10 @@ func TestOTel_PerSignalEndpoint_Override(t *testing.T) { defer drainCancel() require.NoError(t, shutdown(drainCtx)) - assert.Equal(t, 1, rTraces.SpanCount(), "trace endpoint should have received the span") + assert.Equal(t, 1, rTraces.SpanCount(), "TracesEndpoint should receive the span") + assert.GreaterOrEqual(t, rMetrics.MetricCount(), 1, "MetricsEndpoint should receive the metric") assert.Zero(t, rDefault.SpanCount(), "default endpoint must NOT receive traces when overridden") - assert.GreaterOrEqual(t, rDefault.MetricCount(), 1, "default endpoint should receive metrics (no override)") - assert.Zero(t, rTraces.MetricCount(), "trace endpoint should NOT receive metrics") + assert.Zero(t, rDefault.MetricCount(), "default endpoint must NOT receive metrics when overridden") + assert.Zero(t, rTraces.MetricCount(), "TracesEndpoint should NOT receive metrics (catches metrics-to-traces wiring bug)") + assert.Zero(t, rMetrics.SpanCount(), "MetricsEndpoint should NOT receive spans (catches traces-to-metrics wiring bug)") } From 7ae46655797786255fc5bf1af4fb1302b89391e9 Mon Sep 17 00:00:00 2001 From: Jack Woods Date: Thu, 14 May 2026 14:55:38 -0400 Subject: [PATCH 03/17] docs(observability): quote Grafana Cloud header export + drop Datadog from cloud OTLP list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback on #136: - deployment.md: unquoted RHS in the Grafana Cloud WH_OTEL_HEADERS example would split on the space after "Basic", dropping the base64 credential silently (auth still parses, every export 401s). Wrap the assignment in double-quotes — $() and inner "$VAR" expand correctly inside outer "". - configuration.md: TLS direct-to-cloud paragraph listed Datadog OTLP among supported targets, contradicting the worked example in deployment.md (Datadog has no public direct-to-cloud OTLP endpoint; the supported path is the local DDOT Collector). Remove Datadog from the list and link to the DDOT section. Also fix the broken Direct-to-cloud OTLP anchor to match the actual section ID. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/src/content/docs/configuration.md | 2 +- docs/src/content/docs/deployment.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/src/content/docs/configuration.md b/docs/src/content/docs/configuration.md index d963f095..7304e3a7 100644 --- a/docs/src/content/docs/configuration.md +++ b/docs/src/content/docs/configuration.md @@ -113,7 +113,7 @@ The master switch is `otel.enabled`. When `true`, each signal (traces/metrics/lo **Sampling rates apply only to the OTLP push path.** Stdout always emits 100% of records — operators using a scraping-style pipeline (Promtail/Grafana Alloy → Loki, Vector, Fluent Bit, etc.) set the collection rate at the scraper, not the application. WaveHouse pushes telemetry to an OTel collector; the scraper world owns its own ingest policy. If you want to throttle OTLP volume for cost, lower the rates below. If you want to throttle Loki/Datadog Logs/etc., do it at that pipeline. -**TLS + direct-to-cloud OTLP.** `otel.addr` is scheme-aware: `https://` selects TLS (system root CAs), `http://` or a bare `host:port` stays plaintext gRPC. Combined with `otel.headers` for auth, WaveHouse can ship telemetry straight to TLS-protected cloud endpoints (Grafana Cloud's OTLP gateway, Honeycomb, Datadog OTLP, etc.) without a sidecar collector. A sidecar is still useful for egress queuing, batching, and tail-based sampling — it's just no longer required. See [Deployment → Direct-to-cloud OTLP](./deployment.md#direct-to-cloud-otlp) for worked examples. +**TLS + direct-to-cloud OTLP.** `otel.addr` is scheme-aware: `https://` selects TLS (system root CAs), `http://` or a bare `host:port` stays plaintext gRPC. Combined with `otel.headers` for auth, WaveHouse can ship telemetry straight to TLS-protected cloud endpoints (Grafana Cloud's OTLP gateway, Honeycomb, etc.) without a sidecar collector. Datadog has no public direct-to-cloud OTLP endpoint — use the local DDOT Collector path described in the [deployment guide](./deployment.md#pattern-datadog-via-local-ddot-collector) instead. A sidecar is still useful for egress queuing, batching, and tail-based sampling — it's just no longer required. See [Deployment → Direct-to-cloud OTLP](./deployment.md#pattern-direct-to-cloud-otlp-honeycomb-grafana-cloud) for worked examples. If different signals need different gateway hosts (Grafana Cloud's distinct trace / metric / log endpoints, say), set `otel.{traces,metrics,logs}.addr` to override the default for that signal — empty means inherit from `otel.addr`. diff --git a/docs/src/content/docs/deployment.md b/docs/src/content/docs/deployment.md index 66206f08..7c53b439 100644 --- a/docs/src/content/docs/deployment.md +++ b/docs/src/content/docs/deployment.md @@ -342,7 +342,7 @@ export WH_OTEL_HEADERS=x-honeycomb-team=YOUR_API_KEY export WH_OTEL_ENABLED=true export WH_OTEL_ADDR=https://otlp-gateway-prod-us-east-0.grafana.net:443 # instanceID:token, base64-encoded (tr -d '\n' strips base64's 76-char line wrap) -export WH_OTEL_HEADERS=authorization=Basic $(printf '%s' "$INSTANCE_ID:$TOKEN" | base64 | tr -d '\n') +export WH_OTEL_HEADERS="authorization=Basic $(printf '%s' "$INSTANCE_ID:$TOKEN" | base64 | tr -d '\n')" ``` If different signals need different gateway hosts (Grafana Cloud's traces/metrics/logs endpoints differ in some regions), set `WH_OTEL_TRACES_ADDR` / `WH_OTEL_METRICS_ADDR` / `WH_OTEL_LOGS_ADDR` to override the default per signal. Empty means inherit from `otel.addr`. Headers always apply to every exporter — there is intentionally no per-signal header override. From e841dc3b495ac03c516469991dc44d10514a1be3 Mon Sep 17 00:00:00 2001 From: Jack Woods Date: Thu, 14 May 2026 15:04:13 -0400 Subject: [PATCH 04/17] test(observability): cover LogsEndpoint in per-signal override test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses claude review [SHOULD] feedback on #136: TestOTel_PerSignalEndpoint_Override previously only wired TracesEndpoint and MetricsEndpoint to distinct receivers and left LogsEndpoint untested with LogsEnabled=false. That left a real copy-paste-bug gap — a future refactor that accidentally routed the logs exporter through TracesEndpoint or MetricsEndpoint would not be caught. Add an rLogs receiver, enable logs, emit one log record, and extend the cross-assertion grid so every (signal × receiver) pair is checked: each per-signal endpoint sees exactly its own signal, the default endpoint sees nothing, and neither sibling sees the other two signals. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/integration/otel_test.go | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/tests/integration/otel_test.go b/tests/integration/otel_test.go index 86b9697e..833e89ac 100644 --- a/tests/integration/otel_test.go +++ b/tests/integration/otel_test.go @@ -399,26 +399,30 @@ func TestOTel_Headers_AppliedToAllSignals(t *testing.T) { assert.Equal(t, []string{"abc123"}, r.LastTraceHeaders().Get("x-honeycomb-team")) } -// TestOTel_PerSignalEndpoint_Override verifies that TracesEndpoint and -// MetricsEndpoint each route their own signal to a distinct receiver while -// the default Endpoint sees neither. Grafana Cloud's per-signal gateway hosts -// are the headline use case. Both signal-type overrides go through the same -// pickEndpoint() helper in provider.go, so a copy-paste bug (e.g. the -// metrics exporter being wired to TracesEndpoint) would surface here as -// metrics landing on the wrong receiver — caught by the cross-asserts below. +// TestOTel_PerSignalEndpoint_Override verifies that TracesEndpoint, +// MetricsEndpoint, and LogsEndpoint each route their own signal to a distinct +// receiver while the default Endpoint sees none. Grafana Cloud's per-signal +// gateway hosts are the headline use case. All three signal overrides go +// through the same pickEndpoint() helper in provider.go, so a copy-paste bug +// (e.g. the metrics exporter being wired to TracesEndpoint, or the logs +// exporter inheriting TracesEndpoint) would surface here as a signal landing +// on the wrong receiver — caught by the cross-asserts below. func TestOTel_PerSignalEndpoint_Override(t *testing.T) { guardOTelGlobals(t) rDefault := testutil.NewFakeOTLP(t) rTraces := testutil.NewFakeOTLP(t) rMetrics := testutil.NewFakeOTLP(t) + rLogs := testutil.NewFakeOTLP(t) shutdown, _ := initAndShutdown(t, observability.ProviderConfig{ - Endpoint: rDefault.Addr(), // both signals overridden → default sees nothing + Endpoint: rDefault.Addr(), // all three signals overridden → default sees nothing TracesEndpoint: rTraces.Addr(), MetricsEndpoint: rMetrics.Addr(), + LogsEndpoint: rLogs.Addr(), TracesEnabled: true, TracesSampleRate: 1.0, MetricsEnabled: true, + LogsEnabled: true, }) _, span := otel.Tracer("test").Start(context.Background(), "split-op") @@ -428,14 +432,25 @@ func TestOTel_PerSignalEndpoint_Override(t *testing.T) { require.NoError(t, err) counter.Add(context.Background(), 1) + lvl := &slog.LevelVar{} + lvl.Set(slog.LevelInfo) + logger := observability.NewLogger("wavehouse-test", lvl, true, 1.0) + logger.Info("split-log") + drainCtx, drainCancel := context.WithTimeout(context.Background(), 5*time.Second) defer drainCancel() require.NoError(t, shutdown(drainCtx)) assert.Equal(t, 1, rTraces.SpanCount(), "TracesEndpoint should receive the span") assert.GreaterOrEqual(t, rMetrics.MetricCount(), 1, "MetricsEndpoint should receive the metric") + assert.GreaterOrEqual(t, rLogs.LogCount(), 1, "LogsEndpoint should receive the log") assert.Zero(t, rDefault.SpanCount(), "default endpoint must NOT receive traces when overridden") assert.Zero(t, rDefault.MetricCount(), "default endpoint must NOT receive metrics when overridden") + assert.Zero(t, rDefault.LogCount(), "default endpoint must NOT receive logs when overridden") assert.Zero(t, rTraces.MetricCount(), "TracesEndpoint should NOT receive metrics (catches metrics-to-traces wiring bug)") + assert.Zero(t, rTraces.LogCount(), "TracesEndpoint should NOT receive logs (catches logs-to-traces wiring bug)") assert.Zero(t, rMetrics.SpanCount(), "MetricsEndpoint should NOT receive spans (catches traces-to-metrics wiring bug)") + assert.Zero(t, rMetrics.LogCount(), "MetricsEndpoint should NOT receive logs (catches logs-to-metrics wiring bug)") + assert.Zero(t, rLogs.SpanCount(), "LogsEndpoint should NOT receive spans (catches traces-to-logs wiring bug)") + assert.Zero(t, rLogs.MetricCount(), "LogsEndpoint should NOT receive metrics (catches metrics-to-logs wiring bug)") } From 25d68f45b0d3757216f5197a984c94363027d4b1 Mon Sep 17 00:00:00 2001 From: Jack Woods Date: Thu, 14 May 2026 15:13:43 -0400 Subject: [PATCH 05/17] test(observability): assert every header on every OTLP signal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses coderabbit Minor feedback on #136. TestOTel_Headers_AppliedToAllSignals previously checked `authorization` on traces/metrics/logs but only spot-checked the second header `x-honeycomb-team` on traces. The justification was that a single header map flows through all three exporters — true, but the test's stated purpose is catching per-exporter wiring bugs (a future refactor that built one exporter with a stale headers map would still pass silently for the un-checked signals). Fold both header assertions into one signal loop so every header is verified on every exporter. Per-signal failure messages now name the exporter that dropped the header, so a regression points straight at the broken provider.go branch. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/integration/otel_test.go | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/tests/integration/otel_test.go b/tests/integration/otel_test.go index 833e89ac..4f34c207 100644 --- a/tests/integration/otel_test.go +++ b/tests/integration/otel_test.go @@ -28,6 +28,7 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/log/global" + "google.golang.org/grpc/metadata" "github.com/Wave-RF/WaveHouse/internal/observability" "github.com/Wave-RF/WaveHouse/internal/testutil" @@ -380,23 +381,27 @@ func TestOTel_Headers_AppliedToAllSignals(t *testing.T) { require.NoError(t, shutdown(drainCtx)) // gRPC metadata keys are lowercased on the wire — assert in lowercase. - for _, sig := range []struct { + // Every configured header is checked on every signal so a per-exporter + // header-propagation bug (e.g. one exporter built with a stale headers + // map) fails loud instead of silently 401'ing on the missing signal. + signals := []struct { name string - md func() []string + md func() metadata.MD }{ - {"traces", func() []string { return r.LastTraceHeaders().Get("authorization") }}, - {"metrics", func() []string { return r.LastMetricHeaders().Get("authorization") }}, - {"logs", func() []string { return r.LastLogHeaders().Get("authorization") }}, - } { + {"traces", func() metadata.MD { return r.LastTraceHeaders() }}, + {"metrics", func() metadata.MD { return r.LastMetricHeaders() }}, + {"logs", func() metadata.MD { return r.LastLogHeaders() }}, + } + for _, sig := range signals { t.Run(sig.name, func(t *testing.T) { - vals := sig.md() - require.NotEmpty(t, vals, "%s exporter dropped the authorization header", sig.name) - assert.Equal(t, "Bearer test-token", vals[0]) + md := sig.md() + authz := md.Get("authorization") + require.NotEmpty(t, authz, "%s exporter dropped the authorization header", sig.name) + assert.Equal(t, "Bearer test-token", authz[0]) + assert.Equal(t, []string{"abc123"}, md.Get("x-honeycomb-team"), + "%s exporter dropped the x-honeycomb-team header", sig.name) }) } - // Spot-check the second header on at least one signal — same map flows - // through all three so one check confirms multi-header support. - assert.Equal(t, []string{"abc123"}, r.LastTraceHeaders().Get("x-honeycomb-team")) } // TestOTel_PerSignalEndpoint_Override verifies that TracesEndpoint, From 614207bc80789f263a9068e7564e53de6352ce77 Mon Sep 17 00:00:00 2001 From: Jack Woods Date: Thu, 14 May 2026 15:26:28 -0400 Subject: [PATCH 06/17] observability: harden config + lock in cross-parser parity + TLS-all-signals coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses claude review feedback on #136: - provider.go: unexport ProviderConfig.TLSConfig (now `tlsConfig`) and expose it only via SetTLSConfigForTesting. The doc-comment "production code leaves it nil" is now backed by the type system — IDE autocomplete doesn't surface the field for production callers, and the setter's awkward name signals test-only intent. (Defense in depth against a staging-debug `&tls.Config{InsecureSkipVerify: true}` leaking into production code.) - endpoint.go / config.go: reject empty-or-whitespace-only header values in both parsers. `authorization= ` previously passed validation, trimmed to empty, and shipped `authorization: ""` to the cloud endpoint — a silent 401 with no startup warning. Now it fails loud at boot. Doc comments on both parsers point to the parity test. - internal/config/header_parity_test.go (new): external test package that imports both parsers and asserts they agree on accept/reject for a shared corpus. config can't import observability in production (transitive OTel SDK pull), so the parsers are hand-mirrored — the runtime double-parse in main.go only catches drift that *rejects* a previously-valid input. This pins drift in either direction at CI time. - tests/integration/otel_test.go: rename TestOTel_TLSPath_Traces to TestOTel_TLSPath_AllSignals and exercise metrics + logs on the TLS path. Previously, an accidental `WithInsecure()` reintroduced on just the metrics or logs branch of provider.go would not have been caught (the existing test only enabled traces; TestOTel_Headers_AppliedToAllSignals exercises all three signals but on plaintext). Test plan: go build ./... clean; go vet ./... clean; go test on internal/config, internal/observability all green; the three integration tests we touched (TLSPath_AllSignals, PerSignalEndpoint_Override, Headers_AppliedToAllSignals) all pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/config/config.go | 15 ++++- internal/config/config_test.go | 2 + internal/config/header_parity_test.go | 77 +++++++++++++++++++++++++ internal/observability/endpoint.go | 14 ++++- internal/observability/endpoint_test.go | 3 +- internal/observability/provider.go | 27 ++++++--- tests/integration/otel_test.go | 39 ++++++++++--- 7 files changed, 153 insertions(+), 24 deletions(-) create mode 100644 internal/config/header_parity_test.go diff --git a/internal/config/config.go b/internal/config/config.go index 79c83da6..d9f4f0e8 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -170,9 +170,15 @@ type DLQ struct { // // MUST stay in sync with observability.ParseOTelHeaders in // internal/observability/endpoint.go. cmd/wavehouse/main.go's defensive -// error-check on the post-Validate parse exists to catch any drift loudly, -// but the right answer is to not drift in the first place: any rule change -// here needs the same change there, and vice versa. +// error-check on the post-Validate parse catches any drift loudly at +// startup, and the cross-parser parity test in header_parity_test.go pins +// both parsers to the same accept/reject decisions at CI time — but the +// right answer is to not drift in the first place: any rule change here +// needs the same change there, and vice versa. +// +// Empty-or-whitespace-only values are rejected (e.g. `authorization= `) +// to surface auth typos at boot rather than as silent 401s against the +// cloud gateway. func validateOTelHeaders(s string) error { s = strings.TrimSpace(s) if s == "" { @@ -190,6 +196,9 @@ func validateOTelHeaders(s string) error { if strings.TrimSpace(seg[:i]) == "" { return fmt.Errorf("header segment %q has empty key", seg) } + if strings.TrimSpace(seg[i+1:]) == "" { + return fmt.Errorf("header segment %q has empty or whitespace-only value", seg) + } } return nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 92658ccc..8c783bc5 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -324,6 +324,8 @@ func TestValidate_RejectsMalformedHeaders(t *testing.T) { }{ {name: "missing equals", headers: "not-a-pair"}, {name: "empty key", headers: "=value"}, + {name: "empty value", headers: "k="}, + {name: "whitespace-only value", headers: "authorization= "}, {name: "mixed valid and invalid", headers: "a=1,broken"}, } for _, tc := range cases { diff --git a/internal/config/header_parity_test.go b/internal/config/header_parity_test.go new file mode 100644 index 00000000..c66d2321 --- /dev/null +++ b/internal/config/header_parity_test.go @@ -0,0 +1,77 @@ +package config_test + +// External test package so the cross-parser check can import the +// observability package without dragging the OTel SDK into the +// production import graph of internal/config — see the doc on +// validateOTelHeaders for the dependency-graph rationale. + +import ( + "testing" + + "github.com/Wave-RF/WaveHouse/internal/config" + "github.com/Wave-RF/WaveHouse/internal/observability" +) + +// TestHeaderParsers_StayInSync pins config.validateOTelHeaders and +// observability.ParseOTelHeaders to the same accept/reject decisions on a +// shared corpus of inputs. The two parsers are hand-mirrored — config can't +// import observability in production without pulling the OTel SDK into every +// config consumer — and the runtime double-parse in cmd/wavehouse/main.go +// only flags drift that *rejects* a previously-valid input. Drift in the +// other direction (config silently accepts what the SDK parser rejects, or +// vice versa) reaches production as misconfigured auth. This test makes +// either kind of drift a CI failure. +func TestHeaderParsers_StayInSync(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + in string + wantErr bool + }{ + {name: "empty", in: "", wantErr: false}, + {name: "whitespace only", in: " ", wantErr: false}, + {name: "one pair", in: "k=v", wantErr: false}, + {name: "two pairs", in: "k=v,k2=v2", wantErr: false}, + {name: "whitespace around segments", in: " a = 1 , b = 2 ", wantErr: false}, + {name: "value contains equals (base64 padding)", in: "authorization=Basic dXNlcjpwYXNz==", wantErr: false}, + {name: "trailing comma tolerated", in: "k=v,", wantErr: false}, + {name: "missing equals", in: "not-a-pair", wantErr: true}, + {name: "empty key", in: "=value", wantErr: true}, + {name: "empty value", in: "k=", wantErr: true}, + {name: "whitespace-only value", in: "authorization= ", wantErr: true}, + {name: "mixed valid and invalid", in: "a=1,broken", wantErr: true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + cfg := config.Config{ + Server: config.Server{Port: 8080}, + ClickHouse: config.ClickHouse{HTTPScheme: "http"}, + Schema: config.Schema{RefreshInterval: 60}, + OTel: config.OTel{ + Enabled: true, + Addr: "127.0.0.1:4317", + Headers: tc.in, + }, + } + validateErr := cfg.Validate() + _, parseErr := observability.ParseOTelHeaders(tc.in) + + // Compare on the accept/reject boolean — error wording is allowed + // to differ between the two parsers, but their decisions must not. + if (validateErr != nil) != (parseErr != nil) { + t.Fatalf("parser drift on %q: validateOTelHeaders=%v ParseOTelHeaders=%v", + tc.in, validateErr, parseErr) + } + if tc.wantErr && validateErr == nil { + t.Fatalf("expected both parsers to reject %q, both accepted", tc.in) + } + if !tc.wantErr && validateErr != nil { + t.Fatalf("expected both parsers to accept %q, both rejected: %v", tc.in, validateErr) + } + }) + } +} diff --git a/internal/observability/endpoint.go b/internal/observability/endpoint.go index 50ee49b8..b8eeb29f 100644 --- a/internal/observability/endpoint.go +++ b/internal/observability/endpoint.go @@ -45,7 +45,10 @@ func ParseEndpoint(addr string) (host string, useTLS bool) { // (`OTEL_EXPORTER_OTLP_HEADERS`) — comma-separated `key=value` pairs — into a // map. Whitespace around the key and value is trimmed. Only the first `=` per // segment splits key from value, so base64 trailing `=` in an Authorization -// header round-trips unchanged. An empty input yields an empty map. +// header round-trips unchanged. An empty input yields an empty map. Both an +// empty key and an empty-or-whitespace-only value are rejected — the latter +// turns "authorization= " (a typo) from a silent 401 against the cloud +// gateway into a fail-loud boot error. // // Returns an error (rather than silently dropping the segment) for malformed // entries so Validate() can fail loud at boot rather than letting a typo @@ -54,8 +57,10 @@ func ParseEndpoint(addr string) (host string, useTLS bool) { // MUST stay in sync with config.validateOTelHeaders in // internal/config/config.go. config can't import observability without // transitively pulling the OTel SDK into every config consumer, so the -// parsing rules are hand-mirrored. Any rule change here needs the same -// change there, and vice versa. +// parsing rules are hand-mirrored. The cross-parser parity test in +// internal/config/header_parity_test.go pins both parsers to the same +// accept/reject decisions, so a rule change here fails CI until the +// matching change lands there (and vice versa). func ParseOTelHeaders(s string) (map[string]string, error) { s = strings.TrimSpace(s) if s == "" { @@ -76,6 +81,9 @@ func ParseOTelHeaders(s string) (map[string]string, error) { if key == "" { return nil, fmt.Errorf("header segment %q has empty key", seg) } + if val == "" { + return nil, fmt.Errorf("header segment %q has empty or whitespace-only value", seg) + } out[key] = val } return out, nil diff --git a/internal/observability/endpoint_test.go b/internal/observability/endpoint_test.go index 2a65737b..4eeaa5c0 100644 --- a/internal/observability/endpoint_test.go +++ b/internal/observability/endpoint_test.go @@ -46,7 +46,8 @@ func TestParseOTelHeaders(t *testing.T) { {name: "trailing comma tolerated", in: "a=1,", want: map[string]string{"a": "1"}}, {name: "missing equals", in: "not-a-pair", wantErr: true}, {name: "empty key", in: "=value", wantErr: true}, - {name: "empty value allowed", in: "k=", want: map[string]string{"k": ""}}, + {name: "empty value rejected", in: "k=", wantErr: true}, + {name: "whitespace-only value rejected", in: "authorization= ", wantErr: true}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/observability/provider.go b/internal/observability/provider.go index 2210735c..885a437a 100644 --- a/internal/observability/provider.go +++ b/internal/observability/provider.go @@ -53,10 +53,12 @@ var runtimeStartOnce sync.Once // Headers (set, key=value pairs already parsed by config) is applied to every // OTLP exporter — the standard knob for auth against cloud endpoints. // -// TLSConfig is a test-only escape hatch: when non-nil it overrides the default -// system-roots TLS config for `https://` endpoints. Production code leaves it -// nil; only the FakeOTLPTLS-driven integration tests populate it (with -// InsecureSkipVerify against an ephemeral self-signed cert). +// tlsConfig is unexported and reserved for the integration-test escape hatch +// (FakeOTLPTLS — InsecureSkipVerify against an ephemeral self-signed cert). +// Production code must NOT set it; the system-root TLS config picked from the +// `https://` scheme on Endpoint is what production wants. Test callers reach +// it via SetTLSConfigForTesting so production callers don't get the field +// surfaced in IDE autocomplete. type ProviderConfig struct { Endpoint string Headers map[string]string @@ -68,7 +70,16 @@ type ProviderConfig struct { PrometheusEnabled bool LogsEndpoint string LogsEnabled bool - TLSConfig *tls.Config + tlsConfig *tls.Config +} + +// SetTLSConfigForTesting injects a client *tls.Config that overrides the +// default system-roots config for `https://` endpoints. The deliberately +// awkward name signals intent: only the FakeOTLPTLS integration helpers +// should call this. Production code path keeps tlsConfig nil and falls +// through to system roots. +func (c *ProviderConfig) SetTLSConfigForTesting(cfg *tls.Config) { + c.tlsConfig = cfg } // pickEndpoint returns override if set, otherwise fallback. @@ -144,7 +155,7 @@ func InitProvider(ctx context.Context, serviceName string, cfg ProviderConfig) ( host, useTLS := ParseEndpoint(pickEndpoint(cfg.TracesEndpoint, cfg.Endpoint)) opts := []otlptracegrpc.Option{otlptracegrpc.WithEndpoint(host)} if useTLS { - opts = append(opts, otlptracegrpc.WithTLSCredentials(credentials.NewTLS(tlsConfigOrDefault(cfg.TLSConfig)))) + opts = append(opts, otlptracegrpc.WithTLSCredentials(credentials.NewTLS(tlsConfigOrDefault(cfg.tlsConfig)))) } else { opts = append(opts, otlptracegrpc.WithInsecure()) } @@ -174,7 +185,7 @@ func InitProvider(ctx context.Context, serviceName string, cfg ProviderConfig) ( host, useTLS := ParseEndpoint(pickEndpoint(cfg.MetricsEndpoint, cfg.Endpoint)) opts := []otlpmetricgrpc.Option{otlpmetricgrpc.WithEndpoint(host)} if useTLS { - opts = append(opts, otlpmetricgrpc.WithTLSCredentials(credentials.NewTLS(tlsConfigOrDefault(cfg.TLSConfig)))) + opts = append(opts, otlpmetricgrpc.WithTLSCredentials(credentials.NewTLS(tlsConfigOrDefault(cfg.tlsConfig)))) } else { opts = append(opts, otlpmetricgrpc.WithInsecure()) } @@ -238,7 +249,7 @@ func InitProvider(ctx context.Context, serviceName string, cfg ProviderConfig) ( host, useTLS := ParseEndpoint(pickEndpoint(cfg.LogsEndpoint, cfg.Endpoint)) opts := []otlploggrpc.Option{otlploggrpc.WithEndpoint(host)} if useTLS { - opts = append(opts, otlploggrpc.WithTLSCredentials(credentials.NewTLS(tlsConfigOrDefault(cfg.TLSConfig)))) + opts = append(opts, otlploggrpc.WithTLSCredentials(credentials.NewTLS(tlsConfigOrDefault(cfg.tlsConfig)))) } else { opts = append(opts, otlploggrpc.WithInsecure()) } diff --git a/tests/integration/otel_test.go b/tests/integration/otel_test.go index 4f34c207..06f57879 100644 --- a/tests/integration/otel_test.go +++ b/tests/integration/otel_test.go @@ -315,30 +315,51 @@ func TestOTel_UnreachableEndpoint_DoesNotBlockStartupOrEmits(t *testing.T) { }() } -// TestOTel_TLSPath_Traces locks in the https:// → TLS dial path. The fake -// receiver listens with an ephemeral self-signed cert; the exporter is given -// the matching client tls.Config via ProviderConfig.TLSConfig. If TLS wiring -// regresses (e.g. someone re-adds WithInsecure unconditionally), the dial -// will TLS-handshake against a plaintext server and the export drops. -func TestOTel_TLSPath_Traces(t *testing.T) { +// TestOTel_TLSPath_AllSignals locks in the https:// → TLS dial path on every +// OTLP exporter. The fake receiver listens with an ephemeral self-signed cert; +// each exporter is given the matching client tls.Config via the test-only +// SetTLSConfigForTesting setter. If TLS wiring regresses on any signal (e.g. +// someone re-adds WithInsecure unconditionally on just the metrics or logs +// branch of provider.go), that signal will TLS-handshake against a plaintext +// server and the export drops — caught here as a missing count on the right +// receiver. +// +// Headers are intentionally not asserted; TestOTel_Headers_AppliedToAllSignals +// owns the header-propagation invariant. This test owns the TLS-dial invariant +// across signals. +func TestOTel_TLSPath_AllSignals(t *testing.T) { guardOTelGlobals(t) r := testutil.NewFakeOTLPTLS(t) - shutdown, _ := initAndShutdown(t, observability.ProviderConfig{ + cfg := observability.ProviderConfig{ Endpoint: "https://" + r.Addr(), - TLSConfig: r.TLSConfig(), TracesEnabled: true, TracesSampleRate: 1.0, - }) + MetricsEnabled: true, + LogsEnabled: true, + } + cfg.SetTLSConfigForTesting(r.TLSConfig()) + shutdown, _ := initAndShutdown(t, cfg) _, span := otel.Tracer("test").Start(context.Background(), "tls-op") span.End() + counter, err := otel.GetMeterProvider().Meter("test").Int64Counter("tls_counter") + require.NoError(t, err) + counter.Add(context.Background(), 1) + + lvl := &slog.LevelVar{} + lvl.Set(slog.LevelInfo) + logger := observability.NewLogger("wavehouse-test", lvl, true, 1.0) + logger.Info("tls-log") + drainCtx, drainCancel := context.WithTimeout(context.Background(), 5*time.Second) defer drainCancel() require.NoError(t, shutdown(drainCtx)) assert.Equal(t, 1, r.SpanCount(), "TLS path must deliver the span end-to-end") + assert.GreaterOrEqual(t, r.MetricCount(), 1, "TLS path must deliver metrics end-to-end") + assert.GreaterOrEqual(t, r.LogCount(), 1, "TLS path must deliver logs end-to-end") } // TestOTel_Headers_AppliedToAllSignals verifies that ProviderConfig.Headers From 4209606ee5e8f4f4647d7b7cc0a17492422408b9 Mon Sep 17 00:00:00 2001 From: Jack Woods Date: Thu, 14 May 2026 15:39:08 -0400 Subject: [PATCH 07/17] observability: TLS 1.3 floor + RFC 7230 header-key validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses claude + coderabbit review feedback on #136. - endpoint.go: pin tlsConfigOrDefault()'s production default to MinVersion: tls.VersionTLS13. Every TLS-terminated OTLP gateway in scope (Grafana Cloud, Honeycomb) supports TLS 1.3, and the floor matches OWASP modern guidance. Test callers supplying their own *tls.Config keep full control over the version range. - Header key validation (both parsers): keys must match the RFC 7230 `token` production (the HTTP header-name grammar). Catches typos like `my key=v` (space) or `x:y=v` (colon) at boot — previously these would either fail inside the gRPC stack on first export (wrong place to fail) or get silently mutated. Mixed-case keys (`Authorization`) remain accepted; gRPC normalizes ASCII case on the wire. - Helper functions (firstNonTokenChar in observability, firstNonHeaderTokenChar in config) keep the validation DRY within each package; the cross-parser parity test pins them to the same accept/reject decisions on a shared corpus including space-in-key, colon-in-key, non-ASCII, and the legitimate mixed-case case. Test plan: go vet ./... clean; internal/config and internal/observability unit tests all green; integration TestOTel_TLSPath_AllSignals and TestOTel_Headers_AppliedToAllSignals pass (the latter uses the lowercase `authorization` form so the new key validator doesn't reject it). Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/config/config.go | 31 ++++++++++++++- internal/config/config_test.go | 3 ++ internal/config/header_parity_test.go | 4 ++ internal/observability/endpoint.go | 51 +++++++++++++++++++++---- internal/observability/endpoint_test.go | 4 ++ 5 files changed, 83 insertions(+), 10 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index d9f4f0e8..4a64b601 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -178,7 +178,9 @@ type DLQ struct { // // Empty-or-whitespace-only values are rejected (e.g. `authorization= `) // to surface auth typos at boot rather than as silent 401s against the -// cloud gateway. +// cloud gateway. Keys are validated against the RFC 7230 `token` production +// so `my key=...` (space) fails at boot rather than at the gRPC stack on +// first export. func validateOTelHeaders(s string) error { s = strings.TrimSpace(s) if s == "" { @@ -193,9 +195,13 @@ func validateOTelHeaders(s string) error { if i < 0 { return fmt.Errorf("header segment %q missing '='", seg) } - if strings.TrimSpace(seg[:i]) == "" { + key := strings.TrimSpace(seg[:i]) + if key == "" { return fmt.Errorf("header segment %q has empty key", seg) } + if bad, ok := firstNonHeaderTokenChar(key); !ok { + return fmt.Errorf("header segment %q has invalid key character %q (RFC 7230 token: letters, digits, and %s)", seg, bad, headerTokenPunctuation) + } if strings.TrimSpace(seg[i+1:]) == "" { return fmt.Errorf("header segment %q has empty or whitespace-only value", seg) } @@ -203,6 +209,27 @@ func validateOTelHeaders(s string) error { return nil } +// headerTokenPunctuation lists the punctuation characters legal in an RFC +// 7230 `token` (HTTP header field name). MUST stay in sync with +// observability.tokenPunctuationDoc. +const headerTokenPunctuation = "!#$%&'*+-.^_`|~" + +// firstNonHeaderTokenChar mirrors observability.firstNonTokenChar — see +// the doc on validateOTelHeaders for the parity rationale. +func firstNonHeaderTokenChar(s string) (rune, bool) { + for _, c := range s { + switch { + case c >= 'a' && c <= 'z': + case c >= 'A' && c <= 'Z': + case c >= '0' && c <= '9': + case strings.ContainsRune(headerTokenPunctuation, c): + default: + return c, false + } + } + return 0, true +} + // Validate checks the loaded configuration for logical consistency. func (c *Config) Validate() error { if c.ClickHouse.HTTPScheme != "http" && c.ClickHouse.HTTPScheme != "https" { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 8c783bc5..3cf2c45a 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -326,6 +326,9 @@ func TestValidate_RejectsMalformedHeaders(t *testing.T) { {name: "empty key", headers: "=value"}, {name: "empty value", headers: "k="}, {name: "whitespace-only value", headers: "authorization= "}, + {name: "space in key", headers: "my key=v"}, + {name: "colon in key", headers: "x:y=v"}, + {name: "non-ascii key", headers: "x-héader=v"}, {name: "mixed valid and invalid", headers: "a=1,broken"}, } for _, tc := range cases { diff --git a/internal/config/header_parity_test.go b/internal/config/header_parity_test.go index c66d2321..ed485883 100644 --- a/internal/config/header_parity_test.go +++ b/internal/config/header_parity_test.go @@ -40,6 +40,10 @@ func TestHeaderParsers_StayInSync(t *testing.T) { {name: "empty key", in: "=value", wantErr: true}, {name: "empty value", in: "k=", wantErr: true}, {name: "whitespace-only value", in: "authorization= ", wantErr: true}, + {name: "space in key", in: "my key=v", wantErr: true}, + {name: "colon in key", in: "x:y=v", wantErr: true}, + {name: "non-ascii key", in: "x-héader=v", wantErr: true}, + {name: "mixed-case key", in: "Authorization=Bearer x", wantErr: false}, {name: "mixed valid and invalid", in: "a=1,broken", wantErr: true}, } diff --git a/internal/observability/endpoint.go b/internal/observability/endpoint.go index b8eeb29f..13bb6c2c 100644 --- a/internal/observability/endpoint.go +++ b/internal/observability/endpoint.go @@ -7,14 +7,17 @@ import ( ) // tlsConfigOrDefault returns the supplied config when non-nil. The OTel SDK's -// credentials.NewTLS expects a non-nil *tls.Config; passing nil panics. An -// empty &tls.Config{} delegates to system defaults (system root CAs, ALPN -// negotiation), which is what production wants for cloud OTLP endpoints. +// credentials.NewTLS expects a non-nil *tls.Config; passing nil panics. The +// production default delegates to system root CAs / ALPN negotiation but +// pins MinVersion to TLS 1.3 — every TLS-terminated cloud OTLP gateway in +// scope (Grafana Cloud, Honeycomb) supports it, and the floor matches OWASP +// modern guidance. Test callers supplying their own *tls.Config keep full +// control over the version range. func tlsConfigOrDefault(c *tls.Config) *tls.Config { if c != nil { return c } - return &tls.Config{} + return &tls.Config{MinVersion: tls.VersionTLS13} } // ParseEndpoint splits an OTLP endpoint string into the gRPC dial host and a @@ -45,10 +48,15 @@ func ParseEndpoint(addr string) (host string, useTLS bool) { // (`OTEL_EXPORTER_OTLP_HEADERS`) — comma-separated `key=value` pairs — into a // map. Whitespace around the key and value is trimmed. Only the first `=` per // segment splits key from value, so base64 trailing `=` in an Authorization -// header round-trips unchanged. An empty input yields an empty map. Both an -// empty key and an empty-or-whitespace-only value are rejected — the latter -// turns "authorization= " (a typo) from a silent 401 against the cloud -// gateway into a fail-loud boot error. +// header round-trips unchanged. An empty input yields an empty map. +// +// Both empty keys and empty-or-whitespace-only values are rejected (e.g. +// `authorization= ` would otherwise ship as `authorization: ""` to the +// cloud gateway and 401 silently). Keys are also validated against the +// RFC 7230 `token` production (the HTTP header-name grammar) so a typo like +// `my key=...` fails at boot rather than blowing up inside the gRPC stack +// on first export. gRPC normalizes ASCII case on the wire, so mixed-case +// keys (`Authorization`, `X-Honeycomb-Team`) are accepted. // // Returns an error (rather than silently dropping the segment) for malformed // entries so Validate() can fail loud at boot rather than letting a typo @@ -81,6 +89,9 @@ func ParseOTelHeaders(s string) (map[string]string, error) { if key == "" { return nil, fmt.Errorf("header segment %q has empty key", seg) } + if bad, ok := firstNonTokenChar(key); !ok { + return nil, fmt.Errorf("header segment %q has invalid key character %q (RFC 7230 token: letters, digits, and %s)", seg, bad, tokenPunctuationDoc) + } if val == "" { return nil, fmt.Errorf("header segment %q has empty or whitespace-only value", seg) } @@ -88,3 +99,27 @@ func ParseOTelHeaders(s string) (map[string]string, error) { } return out, nil } + +// tokenPunctuationDoc lists the punctuation characters legal in an RFC 7230 +// `token` (HTTP header field name). Surfaced in error messages so the +// reported "what's allowed" matches what the validator actually accepts. +const tokenPunctuationDoc = "!#$%&'*+-.^_`|~" + +// firstNonTokenChar reports whether s consists entirely of RFC 7230 `token` +// characters (the HTTP header-name grammar): ALPHA / DIGIT / one of the +// punctuation chars in tokenPunctuationDoc. Returns the first offending +// rune and false on the first non-token char; returns (0, true) when s is +// fully valid. +func firstNonTokenChar(s string) (rune, bool) { + for _, c := range s { + switch { + case c >= 'a' && c <= 'z': + case c >= 'A' && c <= 'Z': + case c >= '0' && c <= '9': + case strings.ContainsRune(tokenPunctuationDoc, c): + default: + return c, false + } + } + return 0, true +} diff --git a/internal/observability/endpoint_test.go b/internal/observability/endpoint_test.go index 4eeaa5c0..094d6be0 100644 --- a/internal/observability/endpoint_test.go +++ b/internal/observability/endpoint_test.go @@ -48,6 +48,10 @@ func TestParseOTelHeaders(t *testing.T) { {name: "empty key", in: "=value", wantErr: true}, {name: "empty value rejected", in: "k=", wantErr: true}, {name: "whitespace-only value rejected", in: "authorization= ", wantErr: true}, + {name: "space in key rejected", in: "my key=v", wantErr: true}, + {name: "colon in key rejected", in: "x:y=v", wantErr: true}, + {name: "non-ascii key rejected", in: "x-héader=v", wantErr: true}, + {name: "mixed-case key accepted", in: "Authorization=Bearer x", want: map[string]string{"Authorization": "Bearer x"}}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { From 2a32db23fb69587461810f3f8a4e222784ecff1a Mon Sep 17 00:00:00 2001 From: Jack Woods Date: Thu, 14 May 2026 15:51:32 -0400 Subject: [PATCH 08/17] observability: skip OTLP header parse in prometheus-only mode + dodge gosec G101 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two CI/review fixes from claude review + CI failure on #136: 1) cmd/wavehouse/main.go: ParseOTelHeaders previously ran unconditionally inside the `cfg.OTel.Enabled || cfg.Prometheus.Enabled` block. Config.Validate() only runs validateOTelHeaders when OTel is on, so a Prometheus-only deployment with a stale `WH_OTEL_HEADERS` env var would hit the post-Validate parse, fail, and log "parse failed after Validate accepted them" — factually wrong (Validate just didn't run). Guard the parse on cfg.OTel.Enabled. Headers are inert without an OTLP exporter, so leaving them unparsed in Prometheus-only mode is safe. 2) Rename `tokenPunctuationDoc` / `headerTokenPunctuation` → `headerNamePunctuation` in both parser packages. gosec G101 was flagging the const string as a potential hardcoded credential (false positive — it's the RFC 7230 token punctuation grammar) because the variable name contained the credential-suggesting keyword "Token". The new name describes what it actually is (HTTP header-name punctuation) and dodges the matcher. Same value, same behavior, parity test still pins both packages to the same accept/ reject decisions. Test plan: golangci-lint (with gosec enabled) clean on internal/config, internal/observability, and cmd/wavehouse. go vet ./... clean. internal/config + internal/observability unit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/wavehouse/main.go | 20 ++++++++++++++++---- internal/config/config.go | 12 ++++++------ internal/observability/endpoint.go | 10 +++++----- 3 files changed, 27 insertions(+), 15 deletions(-) diff --git a/cmd/wavehouse/main.go b/cmd/wavehouse/main.go index 8d50a5dc..c7a78b76 100644 --- a/cmd/wavehouse/main.go +++ b/cmd/wavehouse/main.go @@ -108,10 +108,22 @@ func run() int { // surfaces loudly instead of silently shipping OTLP exporters with no // auth metadata (which would only show up as rejected-on-the-wire // telemetry, not as a startup error). - headers, err := observability.ParseOTelHeaders(cfg.OTel.Headers) - if err != nil { - logger.Error("otel.headers parse failed after Validate accepted them; refusing to start with bad auth config", "error", err) - return 1 + // + // Guarded on cfg.OTel.Enabled because Validate() also skips + // validateOTelHeaders when OTLP is off — in Prometheus-only mode a + // stale WH_OTEL_HEADERS env var would otherwise fail here with the + // misleading "parse failed after Validate accepted them" message + // when the parsers are in fact in sync (Validate just didn't run). + // Headers do nothing without an OTLP exporter, so leaving them + // unparsed in that mode is safe. + var headers map[string]string + if cfg.OTel.Enabled { + var err error + headers, err = observability.ParseOTelHeaders(cfg.OTel.Headers) + if err != nil { + logger.Error("otel.headers parse failed after Validate accepted them; refusing to start with bad auth config", "error", err) + return 1 + } } otelShutdown, ph, err := observability.InitProvider(ctx, serviceName, observability.ProviderConfig{ Endpoint: cfg.OTel.Addr, diff --git a/internal/config/config.go b/internal/config/config.go index 4a64b601..0ed62e4e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -200,7 +200,7 @@ func validateOTelHeaders(s string) error { return fmt.Errorf("header segment %q has empty key", seg) } if bad, ok := firstNonHeaderTokenChar(key); !ok { - return fmt.Errorf("header segment %q has invalid key character %q (RFC 7230 token: letters, digits, and %s)", seg, bad, headerTokenPunctuation) + return fmt.Errorf("header segment %q has invalid key character %q (RFC 7230 token: letters, digits, and %s)", seg, bad, headerNamePunctuation) } if strings.TrimSpace(seg[i+1:]) == "" { return fmt.Errorf("header segment %q has empty or whitespace-only value", seg) @@ -209,10 +209,10 @@ func validateOTelHeaders(s string) error { return nil } -// headerTokenPunctuation lists the punctuation characters legal in an RFC -// 7230 `token` (HTTP header field name). MUST stay in sync with -// observability.tokenPunctuationDoc. -const headerTokenPunctuation = "!#$%&'*+-.^_`|~" +// headerNamePunctuation lists the punctuation characters legal in an RFC +// 7230 `token` (HTTP header field name). MUST stay in sync with the +// const of the same name in internal/observability/endpoint.go. +const headerNamePunctuation = "!#$%&'*+-.^_`|~" // firstNonHeaderTokenChar mirrors observability.firstNonTokenChar — see // the doc on validateOTelHeaders for the parity rationale. @@ -222,7 +222,7 @@ func firstNonHeaderTokenChar(s string) (rune, bool) { case c >= 'a' && c <= 'z': case c >= 'A' && c <= 'Z': case c >= '0' && c <= '9': - case strings.ContainsRune(headerTokenPunctuation, c): + case strings.ContainsRune(headerNamePunctuation, c): default: return c, false } diff --git a/internal/observability/endpoint.go b/internal/observability/endpoint.go index 13bb6c2c..5ca778be 100644 --- a/internal/observability/endpoint.go +++ b/internal/observability/endpoint.go @@ -90,7 +90,7 @@ func ParseOTelHeaders(s string) (map[string]string, error) { return nil, fmt.Errorf("header segment %q has empty key", seg) } if bad, ok := firstNonTokenChar(key); !ok { - return nil, fmt.Errorf("header segment %q has invalid key character %q (RFC 7230 token: letters, digits, and %s)", seg, bad, tokenPunctuationDoc) + return nil, fmt.Errorf("header segment %q has invalid key character %q (RFC 7230 token: letters, digits, and %s)", seg, bad, headerNamePunctuation) } if val == "" { return nil, fmt.Errorf("header segment %q has empty or whitespace-only value", seg) @@ -100,14 +100,14 @@ func ParseOTelHeaders(s string) (map[string]string, error) { return out, nil } -// tokenPunctuationDoc lists the punctuation characters legal in an RFC 7230 +// headerNamePunctuation lists the punctuation characters legal in an RFC 7230 // `token` (HTTP header field name). Surfaced in error messages so the // reported "what's allowed" matches what the validator actually accepts. -const tokenPunctuationDoc = "!#$%&'*+-.^_`|~" +const headerNamePunctuation = "!#$%&'*+-.^_`|~" // firstNonTokenChar reports whether s consists entirely of RFC 7230 `token` // characters (the HTTP header-name grammar): ALPHA / DIGIT / one of the -// punctuation chars in tokenPunctuationDoc. Returns the first offending +// punctuation chars in headerNamePunctuation. Returns the first offending // rune and false on the first non-token char; returns (0, true) when s is // fully valid. func firstNonTokenChar(s string) (rune, bool) { @@ -116,7 +116,7 @@ func firstNonTokenChar(s string) (rune, bool) { case c >= 'a' && c <= 'z': case c >= 'A' && c <= 'Z': case c >= '0' && c <= '9': - case strings.ContainsRune(tokenPunctuationDoc, c): + case strings.ContainsRune(headerNamePunctuation, c): default: return c, false } From 11ed8be31a1e72bbc1f66e2a25d792f7edfb4a46 Mon Sep 17 00:00:00 2001 From: Jack Woods Date: Thu, 14 May 2026 16:02:36 -0400 Subject: [PATCH 09/17] deploy(compose): document OTel + Prometheus env vars in standalone.yaml MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses claude review doc-sync feedback on #136. AGENTS.md's doc-sync table requires that adding/modifying a config option also touch `deployments/compose/*` env blocks. The compose file was carrying zero OTel env vars (a pre-existing gap predating this PR), so neither the old knobs (WH_OTEL_ENABLED, WH_OTEL_ADDR, WH_OTEL_TRACES_ENABLED, etc.) nor the new ones from this PR (WH_OTEL_HEADERS, WH_OTEL_{TRACES,METRICS,LOGS}_ADDR) were visible to an operator using compose as the surface. Add the full OTel + Prometheus block commented out under the wavehouse service in standalone.yaml. Operators can uncomment the lines they need. Behaviour is unchanged — every default already matches the embedded `env-default` tag in internal/config/config.go, and observability stays off unless WH_OTEL_ENABLED is uncommented and set to true. dependencies.yaml (the make-dev stack) only runs ClickHouse, no wavehouse service, so nothing to mirror there. Co-Authored-By: Claude Opus 4.7 (1M context) --- deployments/compose/standalone.yaml | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/deployments/compose/standalone.yaml b/deployments/compose/standalone.yaml index 16c46316..a817a002 100644 --- a/deployments/compose/standalone.yaml +++ b/deployments/compose/standalone.yaml @@ -32,6 +32,27 @@ services: # read-only and uncomment WH_PIPES_DIR. The directory is a seed for # NATS KV — runtime pipe edits go through the API, not the files. # WH_PIPES_DIR: /app/pipes + # ---- Observability (all optional, all default off) --------------- + # OTLP push: uncomment WH_OTEL_ENABLED=true and point WH_OTEL_ADDR + # at the collector. https:// → TLS via system roots; bare host:port + # or http:// stays plaintext. See docs/deployment.md for worked + # Honeycomb / Grafana Cloud / DDOT-Collector examples. + # WH_OTEL_ENABLED: "true" + # WH_OTEL_ADDR: "127.0.0.1:4317" # or https://api.honeycomb.io:443 + # WH_OTEL_HEADERS: "" # "key=value,key2=value2" for cloud auth + # WH_OTEL_TRACES_ENABLED: "true" + # WH_OTEL_TRACES_SAMPLE_RATE: "1.0" + # WH_OTEL_TRACES_ADDR: "" # per-signal endpoint override; empty = inherit WH_OTEL_ADDR + # WH_OTEL_METRICS_ENABLED: "true" + # WH_OTEL_METRICS_ADDR: "" # per-signal endpoint override + # WH_OTEL_LOGS_ENABLED: "true" + # WH_OTEL_LOGS_SAMPLE_RATE: "1.0" # DEBUG/INFO OTLP rate; WARN+ always 100% + # WH_OTEL_LOGS_ADDR: "" # per-signal endpoint override + # Prometheus exposition is independent of OTLP push — pull-based + # scrapers (Alloy / Mimir) can use this alone with no collector. + # WH_PROMETHEUS_ENABLED: "true" + # WH_PROMETHEUS_PATH: "/metrics" + # WH_PROMETHEUS_PORT: "0" # 0 mounts on server.port; non-zero = sidecar listener volumes: - wavehouse-data:/app/data # - ./my-pipes:/app/pipes:ro # uncomment alongside WH_PIPES_DIR From 556d41a48e18a1bb0d8de5f0596e4c0ffd684371 Mon Sep 17 00:00:00 2001 From: Jack Woods Date: Thu, 14 May 2026 16:14:04 -0400 Subject: [PATCH 10/17] observability: stop leaking header values in parser error messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses claude review [SHOULD] MEDIUM (secrets-in-logs) on #136. Error messages from both header parsers were quoting `seg` (the full `key=value` pair). A misconfigured `WH_OTEL_HEADERS="x:y=$REAL_TOKEN"` or `WH_OTEL_HEADERS="=$REAL_TOKEN"` would then ship the live API token to every sink the structured "error" field reaches — Loki, Datadog, CloudWatch, etc. Rewrite all four error paths in both parsers to quote only the key (which is safe — header names are not secrets) or to drop the identifier entirely when the parse failed before a key was extracted: missing-`=` : "header entry missing '=' separator (format is key=value)" empty key : "header entry has empty key (format is key=value)" invalid char: "header key %q has invalid character %q (...)" empty value : "header key %q has empty or whitespace-only value" Same wording in both packages; the cross-parser parity test only compares accept/reject decisions, not error strings, so the test continues to pass. The Validate() wrapper still prefixes the returned error with "otel.headers:" so existing tests that assert on that substring stay green. golangci-lint (with gosec) clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/config/config.go | 11 +++++++---- internal/observability/endpoint.go | 12 ++++++++---- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 0ed62e4e..f852a140 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -191,19 +191,22 @@ func validateOTelHeaders(s string) error { if seg == "" { continue } + // Error messages quote only the key, never the full segment — same + // credential-exposure concern as observability.ParseOTelHeaders. + // See the doc on ParseOTelHeaders for the rationale. i := strings.IndexByte(seg, '=') if i < 0 { - return fmt.Errorf("header segment %q missing '='", seg) + return fmt.Errorf("header entry missing '=' separator (format is key=value)") } key := strings.TrimSpace(seg[:i]) if key == "" { - return fmt.Errorf("header segment %q has empty key", seg) + return fmt.Errorf("header entry has empty key (format is key=value)") } if bad, ok := firstNonHeaderTokenChar(key); !ok { - return fmt.Errorf("header segment %q has invalid key character %q (RFC 7230 token: letters, digits, and %s)", seg, bad, headerNamePunctuation) + return fmt.Errorf("header key %q has invalid character %q (RFC 7230 token: letters, digits, and %s)", key, bad, headerNamePunctuation) } if strings.TrimSpace(seg[i+1:]) == "" { - return fmt.Errorf("header segment %q has empty or whitespace-only value", seg) + return fmt.Errorf("header key %q has empty or whitespace-only value", key) } } return nil diff --git a/internal/observability/endpoint.go b/internal/observability/endpoint.go index 5ca778be..c2ce633a 100644 --- a/internal/observability/endpoint.go +++ b/internal/observability/endpoint.go @@ -80,20 +80,24 @@ func ParseOTelHeaders(s string) (map[string]string, error) { if seg == "" { continue } + // Error messages quote only the key, never the full segment. A real + // misconfiguration could be `WH_OTEL_HEADERS="x:y=$REAL_API_TOKEN"` + // — quoting `seg` would log the token to every sink the structured + // "error" field reaches (Loki, Datadog, CloudWatch, etc.). i := strings.IndexByte(seg, '=') if i < 0 { - return nil, fmt.Errorf("header segment %q missing '='", seg) + return nil, fmt.Errorf("header entry missing '=' separator (format is key=value)") } key := strings.TrimSpace(seg[:i]) val := strings.TrimSpace(seg[i+1:]) if key == "" { - return nil, fmt.Errorf("header segment %q has empty key", seg) + return nil, fmt.Errorf("header entry has empty key (format is key=value)") } if bad, ok := firstNonTokenChar(key); !ok { - return nil, fmt.Errorf("header segment %q has invalid key character %q (RFC 7230 token: letters, digits, and %s)", seg, bad, headerNamePunctuation) + return nil, fmt.Errorf("header key %q has invalid character %q (RFC 7230 token: letters, digits, and %s)", key, bad, headerNamePunctuation) } if val == "" { - return nil, fmt.Errorf("header segment %q has empty or whitespace-only value", seg) + return nil, fmt.Errorf("header key %q has empty or whitespace-only value", key) } out[key] = val } From 00bb95f7332593c314bbff2e107bc8e94875655d Mon Sep 17 00:00:00 2001 From: Jack Woods Date: Thu, 14 May 2026 16:20:41 -0400 Subject: [PATCH 11/17] test(observability): pin ParseEndpoint IPv6 behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses claude review [MAY] on #136. ParseEndpoint already handles bracketed IPv6 literals correctly — the path-strip loop uses strings.IndexByte, which doesn't touch the brackets — but TestParseEndpoint never exercised that branch. Add three IPv6 cases: bare `[::1]:4317`, `https://[::1]:4317`, and the https-with-path variant. They pass today and act as regression guards if ParseEndpoint is ever refactored to use net/url (which would require careful bracket handling). Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/observability/endpoint_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/observability/endpoint_test.go b/internal/observability/endpoint_test.go index 094d6be0..cf94bfe7 100644 --- a/internal/observability/endpoint_test.go +++ b/internal/observability/endpoint_test.go @@ -20,6 +20,13 @@ func TestParseEndpoint(t *testing.T) { {name: "https with port and path", in: "https://otlp.example.com:443/v1/traces", wantHost: "otlp.example.com:443", wantTLS: true}, {name: "empty", in: "", wantHost: ""}, {name: "ipv4 plaintext", in: "127.0.0.1:4317", wantHost: "127.0.0.1:4317"}, + // IPv6 cases pin the bracketed-literal behavior end-to-end. The + // current implementation handles them correctly via the path-strip + // loop, but a future refactor to net/url would need to preserve the + // brackets — these cases would catch any regression there. + {name: "ipv6 bare", in: "[::1]:4317", wantHost: "[::1]:4317"}, + {name: "ipv6 https", in: "https://[::1]:4317", wantHost: "[::1]:4317", wantTLS: true}, + {name: "ipv6 https with path", in: "https://[::1]:4317/otlp", wantHost: "[::1]:4317", wantTLS: true}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { From 541b5b4e671d703b0f2cede6217130cf13b69b62 Mon Sep 17 00:00:00 2001 From: Jack Woods Date: Fri, 15 May 2026 12:45:48 -0400 Subject: [PATCH 12/17] docs(observability): trim overarching code comments The OTLP/TLS/headers feature's design rationale is captured in docs/ (configuration, deployment) and AGENTS.md. The inline code comments duplicated that material; this trims the broad explainers while keeping the load-bearing at-the-line notes (credential-leak guards, partial-init rollback, runtime.Start once-only goroutine cap, etc.). Net -207 lines, no behavior change. go build / vet / test all clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/wavehouse/main.go | 28 ++---- internal/config/config.go | 83 ++++------------- internal/config/header_parity_test.go | 22 ++--- internal/observability/endpoint.go | 56 ++++-------- internal/observability/endpoint_test.go | 6 +- internal/observability/provider.go | 88 +++++------------- internal/testutil/otlp.go | 35 ++------ tests/integration/otel_test.go | 115 +++++++----------------- 8 files changed, 113 insertions(+), 320 deletions(-) diff --git a/cmd/wavehouse/main.go b/cmd/wavehouse/main.go index c7a78b76..d52d65a8 100644 --- a/cmd/wavehouse/main.go +++ b/cmd/wavehouse/main.go @@ -98,24 +98,12 @@ func run() int { slog.SetDefault(logger) var promHandler http.Handler - // Provider init runs whenever either OTLP push or Prometheus exposition is - // wanted — Prometheus-only operation (Alloy/scrape, no collector) is a - // first-class mode. The OTel SDK MeterProvider is the shared substrate. if cfg.OTel.Enabled || cfg.Prometheus.Enabled { - // Validate() ran validateOTelHeaders, which mirrors ParseOTelHeaders — - // in practice the parse always succeeds here. The defensive check - // exists so that any future drift between the two implementations - // surfaces loudly instead of silently shipping OTLP exporters with no - // auth metadata (which would only show up as rejected-on-the-wire - // telemetry, not as a startup error). - // - // Guarded on cfg.OTel.Enabled because Validate() also skips - // validateOTelHeaders when OTLP is off — in Prometheus-only mode a - // stale WH_OTEL_HEADERS env var would otherwise fail here with the - // misleading "parse failed after Validate accepted them" message - // when the parsers are in fact in sync (Validate just didn't run). - // Headers do nothing without an OTLP exporter, so leaving them - // unparsed in that mode is safe. + // Validate() already ran validateOTelHeaders; this re-parse exists + // only to catch drift between the two parsers (header_parity_test.go + // pins them at CI time, but a runtime sanity check is cheap). Skip + // in Prometheus-only mode — Validate() skips headers there too, so + // a stale WH_OTEL_HEADERS would produce a misleading error. var headers map[string]string if cfg.OTel.Enabled { var err error @@ -142,9 +130,9 @@ func run() int { } else { promHandler = ph defer func() { - // Bound shutdown so an unreachable collector doesn't hang - // process exit. The OTel SDK's batch processors don't fully - // honor the context deadline during gRPC retry/backoff. + // Bounded — OTel batch processors don't fully honor context + // deadlines during gRPC retry/backoff against an unreachable + // collector. ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() _ = otelShutdown(ctx) diff --git a/internal/config/config.go b/internal/config/config.go index f852a140..eb3ff7c8 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -30,21 +30,8 @@ type Config struct { Prometheus Prometheus `yaml:"prometheus"` } -// OTel configures the OpenTelemetry pipeline. `enabled` is the master switch; -// when false, no signals are initialized regardless of the per-signal toggles. -// -// Addr accepts plain `host:port` (plaintext gRPC, backward-compat), `http://` -// (also plaintext), or `https://` (TLS — required for direct-to-cloud OTLP). -// A URL path component is tolerated and stripped (gRPC ignores it). -// -// Headers is a comma-separated list of `key=value` pairs applied to every OTLP -// exporter — the standard knob for auth against cloud endpoints -// (`authorization=Basic `, `x-honeycomb-team=`, etc.). Values may -// contain `=` (only the first `=` per segment splits key from value). -// -// Per-signal Addr overrides (Traces.Addr, Metrics.Addr, Logs.Addr) point one -// signal at a different endpoint than the top-level Addr — useful for Grafana -// Cloud, which uses distinct gateway hosts per signal. Empty means inherit. +// OTel configures the OpenTelemetry pipeline. See docs/configuration.md for +// scheme/headers/per-signal-override semantics. type OTel struct { Enabled bool `yaml:"enabled" env:"WH_OTEL_ENABLED" env-default:"false"` Addr string `yaml:"addr" env:"WH_OTEL_ADDR" env-default:"127.0.0.1:4317"` @@ -65,32 +52,17 @@ type OTelMetrics struct { Addr string `yaml:"addr" env:"WH_OTEL_METRICS_ADDR"` } -// Prometheus controls a Prometheus exposition endpoint served alongside (or -// independently of) the OTLP push exporter. It is its own top-level block so -// operators using Prometheus scraping (Grafana Alloy / Mimir / etc.) don't -// have to hunt through `[otel]` for an output they don't otherwise care about. -// Internally the same OTel MeterProvider drives both — the Prometheus exporter -// is an additional Reader — but the user-facing config keeps the two outputs -// distinct. -// -// Works in any of three combinations: OTel only, Prometheus only, or both. If -// only Prometheus is enabled, the MeterProvider is still created so runtime + -// custom metrics flow to `/metrics`; no OTLP push is attempted. -// -// Port `0` mounts the endpoint on the existing API server router. A non-zero -// port spins up a dedicated HTTP listener — useful for firewalling metrics -// off the public API surface in production. +// Prometheus configures the Prometheus exposition endpoint. Independent of +// OTLP push — works alone or alongside. Port 0 mounts on the API router; a +// non-zero port spins a dedicated listener. See docs/configuration.md. type Prometheus struct { Enabled bool `yaml:"enabled" env:"WH_PROMETHEUS_ENABLED" env-default:"false"` Path string `yaml:"path" env:"WH_PROMETHEUS_PATH" env-default:"/metrics"` Port int `yaml:"port" env:"WH_PROMETHEUS_PORT" env-default:"0"` } -// OTelLogs sample rate applies to OTLP export of DEBUG/INFO only. -// WARN and ERROR always export at 100% — dropping them silently during -// incidents is too dangerous to expose as a knob. Stdout receives 100% of -// records regardless of this rate (sampling for scraped-log pipelines like -// Loki/Promtail belongs at the scraper, not the application). +// OTelLogs.SampleRate applies to DEBUG/INFO OTLP export only. +// WARN/ERROR always export at 100% (non-configurable); stdout is always 100%. type OTelLogs struct { Enabled bool `yaml:"enabled" env:"WH_OTEL_LOGS_ENABLED" env-default:"true"` SampleRate float64 `yaml:"sample_rate" env:"WH_OTEL_LOGS_SAMPLE_RATE" env-default:"1.0"` @@ -164,23 +136,8 @@ type DLQ struct { } // validateOTelHeaders mirrors observability.ParseOTelHeaders for boot-time -// validation. Kept here (rather than importing observability) so config stays -// at the bottom of the dependency graph — importing observability would -// transitively pull the OTel SDK into every config consumer. -// -// MUST stay in sync with observability.ParseOTelHeaders in -// internal/observability/endpoint.go. cmd/wavehouse/main.go's defensive -// error-check on the post-Validate parse catches any drift loudly at -// startup, and the cross-parser parity test in header_parity_test.go pins -// both parsers to the same accept/reject decisions at CI time — but the -// right answer is to not drift in the first place: any rule change here -// needs the same change there, and vice versa. -// -// Empty-or-whitespace-only values are rejected (e.g. `authorization= `) -// to surface auth typos at boot rather than as silent 401s against the -// cloud gateway. Keys are validated against the RFC 7230 `token` production -// so `my key=...` (space) fails at boot rather than at the gRPC stack on -// first export. +// validation. Hand-mirrored to keep config out of the OTel SDK's import graph; +// the parity is pinned by internal/config/header_parity_test.go. func validateOTelHeaders(s string) error { s = strings.TrimSpace(s) if s == "" { @@ -191,9 +148,8 @@ func validateOTelHeaders(s string) error { if seg == "" { continue } - // Error messages quote only the key, never the full segment — same - // credential-exposure concern as observability.ParseOTelHeaders. - // See the doc on ParseOTelHeaders for the rationale. + // Error messages quote only the key, never the segment — `seg` + // can contain a real API token. i := strings.IndexByte(seg, '=') if i < 0 { return fmt.Errorf("header entry missing '=' separator (format is key=value)") @@ -213,12 +169,10 @@ func validateOTelHeaders(s string) error { } // headerNamePunctuation lists the punctuation characters legal in an RFC -// 7230 `token` (HTTP header field name). MUST stay in sync with the -// const of the same name in internal/observability/endpoint.go. +// 7230 `token`. Mirrors the const of the same name in observability/endpoint.go. const headerNamePunctuation = "!#$%&'*+-.^_`|~" -// firstNonHeaderTokenChar mirrors observability.firstNonTokenChar — see -// the doc on validateOTelHeaders for the parity rationale. +// firstNonHeaderTokenChar mirrors observability.firstNonTokenChar. func firstNonHeaderTokenChar(s string) (rune, bool) { for _, c := range s { switch { @@ -279,10 +233,7 @@ func (c *Config) Validate() error { return fmt.Errorf("otel.logs.sample_rate %g out of range [0.0, 1.0]", r) } } - // Parse headers eagerly so a malformed entry fails at boot rather than - // silently dropping auth in production. The same parser runs again at - // provider init; duplicating the validation here keeps config from - // importing the observability package. + // Fail at boot rather than silently dropping auth in production. if err := validateOTelHeaders(c.OTel.Headers); err != nil { return fmt.Errorf("otel.headers: %w", err) } @@ -309,10 +260,8 @@ func (c *Config) Validate() error { return fmt.Errorf("prometheus.path %q conflicts with reserved endpoint", p.Path) } } - // Same-port mode mounts the (unauthenticated) metrics handler on the - // main router. A path inside the /v1 namespace would shadow the - // authenticated API subtree with a public handler — worse than the - // /health shadow case because it leaks at an authenticated-looking URL. + // Same-port mode would shadow the authenticated /v1 subtree with an + // unauthenticated metrics handler — leaks at an authenticated-looking URL. if p.Port == 0 && (p.Path == "/v1" || strings.HasPrefix(p.Path, "/v1/")) { return fmt.Errorf("prometheus.path %q conflicts with authenticated /v1 API namespace when prometheus.port is 0", p.Path) } diff --git a/internal/config/header_parity_test.go b/internal/config/header_parity_test.go index ed485883..3dfadb90 100644 --- a/internal/config/header_parity_test.go +++ b/internal/config/header_parity_test.go @@ -1,10 +1,8 @@ +// External test package so the cross-parser check can import observability +// without dragging the OTel SDK into the production import graph of +// internal/config (see validateOTelHeaders). package config_test -// External test package so the cross-parser check can import the -// observability package without dragging the OTel SDK into the -// production import graph of internal/config — see the doc on -// validateOTelHeaders for the dependency-graph rationale. - import ( "testing" @@ -13,14 +11,9 @@ import ( ) // TestHeaderParsers_StayInSync pins config.validateOTelHeaders and -// observability.ParseOTelHeaders to the same accept/reject decisions on a -// shared corpus of inputs. The two parsers are hand-mirrored — config can't -// import observability in production without pulling the OTel SDK into every -// config consumer — and the runtime double-parse in cmd/wavehouse/main.go -// only flags drift that *rejects* a previously-valid input. Drift in the -// other direction (config silently accepts what the SDK parser rejects, or -// vice versa) reaches production as misconfigured auth. This test makes -// either kind of drift a CI failure. +// observability.ParseOTelHeaders to identical accept/reject decisions. The +// runtime double-parse in main.go only catches one direction (config-accepts +// / SDK-rejects); this test catches the other direction at CI time. func TestHeaderParsers_StayInSync(t *testing.T) { t.Parallel() @@ -64,8 +57,7 @@ func TestHeaderParsers_StayInSync(t *testing.T) { validateErr := cfg.Validate() _, parseErr := observability.ParseOTelHeaders(tc.in) - // Compare on the accept/reject boolean — error wording is allowed - // to differ between the two parsers, but their decisions must not. + // Error wording may differ; the accept/reject decision must not. if (validateErr != nil) != (parseErr != nil) { t.Fatalf("parser drift on %q: validateOTelHeaders=%v ParseOTelHeaders=%v", tc.in, validateErr, parseErr) diff --git a/internal/observability/endpoint.go b/internal/observability/endpoint.go index c2ce633a..351c7673 100644 --- a/internal/observability/endpoint.go +++ b/internal/observability/endpoint.go @@ -6,13 +6,9 @@ import ( "strings" ) -// tlsConfigOrDefault returns the supplied config when non-nil. The OTel SDK's -// credentials.NewTLS expects a non-nil *tls.Config; passing nil panics. The -// production default delegates to system root CAs / ALPN negotiation but -// pins MinVersion to TLS 1.3 — every TLS-terminated cloud OTLP gateway in -// scope (Grafana Cloud, Honeycomb) supports it, and the floor matches OWASP -// modern guidance. Test callers supplying their own *tls.Config keep full -// control over the version range. +// tlsConfigOrDefault returns c, or a TLS 1.3 floor config when c is nil. +// credentials.NewTLS panics on a nil *tls.Config; the production default +// uses system roots with MinVersion=TLS1.3. func tlsConfigOrDefault(c *tls.Config) *tls.Config { if c != nil { return c @@ -45,30 +41,13 @@ func ParseEndpoint(addr string) (host string, useTLS bool) { } // ParseOTelHeaders parses the OpenTelemetry-spec headers env-var format -// (`OTEL_EXPORTER_OTLP_HEADERS`) — comma-separated `key=value` pairs — into a -// map. Whitespace around the key and value is trimmed. Only the first `=` per -// segment splits key from value, so base64 trailing `=` in an Authorization -// header round-trips unchanged. An empty input yields an empty map. +// (comma-separated `key=value` pairs) into a map. Only the first `=` per +// segment splits key from value, so base64 trailing `=` round-trips. Keys +// are validated against RFC 7230 `token`; empty or whitespace-only values +// are rejected. // -// Both empty keys and empty-or-whitespace-only values are rejected (e.g. -// `authorization= ` would otherwise ship as `authorization: ""` to the -// cloud gateway and 401 silently). Keys are also validated against the -// RFC 7230 `token` production (the HTTP header-name grammar) so a typo like -// `my key=...` fails at boot rather than blowing up inside the gRPC stack -// on first export. gRPC normalizes ASCII case on the wire, so mixed-case -// keys (`Authorization`, `X-Honeycomb-Team`) are accepted. -// -// Returns an error (rather than silently dropping the segment) for malformed -// entries so Validate() can fail loud at boot rather than letting a typo -// silently disable auth in production. -// -// MUST stay in sync with config.validateOTelHeaders in -// internal/config/config.go. config can't import observability without -// transitively pulling the OTel SDK into every config consumer, so the -// parsing rules are hand-mirrored. The cross-parser parity test in -// internal/config/header_parity_test.go pins both parsers to the same -// accept/reject decisions, so a rule change here fails CI until the -// matching change lands there (and vice versa). +// Kept in lockstep with config.validateOTelHeaders by the cross-parser parity +// test in internal/config/header_parity_test.go. func ParseOTelHeaders(s string) (map[string]string, error) { s = strings.TrimSpace(s) if s == "" { @@ -80,10 +59,9 @@ func ParseOTelHeaders(s string) (map[string]string, error) { if seg == "" { continue } - // Error messages quote only the key, never the full segment. A real - // misconfiguration could be `WH_OTEL_HEADERS="x:y=$REAL_API_TOKEN"` - // — quoting `seg` would log the token to every sink the structured - // "error" field reaches (Loki, Datadog, CloudWatch, etc.). + // Error messages quote only the key, never the full segment — `seg` + // can contain a real API token that would be logged to every sink + // the structured error field reaches. i := strings.IndexByte(seg, '=') if i < 0 { return nil, fmt.Errorf("header entry missing '=' separator (format is key=value)") @@ -105,15 +83,11 @@ func ParseOTelHeaders(s string) (map[string]string, error) { } // headerNamePunctuation lists the punctuation characters legal in an RFC 7230 -// `token` (HTTP header field name). Surfaced in error messages so the -// reported "what's allowed" matches what the validator actually accepts. +// `token` (HTTP header field name). const headerNamePunctuation = "!#$%&'*+-.^_`|~" -// firstNonTokenChar reports whether s consists entirely of RFC 7230 `token` -// characters (the HTTP header-name grammar): ALPHA / DIGIT / one of the -// punctuation chars in headerNamePunctuation. Returns the first offending -// rune and false on the first non-token char; returns (0, true) when s is -// fully valid. +// firstNonTokenChar returns the first non-RFC-7230-token rune in s, or +// (0, true) when s is fully valid. func firstNonTokenChar(s string) (rune, bool) { for _, c := range s { switch { diff --git a/internal/observability/endpoint_test.go b/internal/observability/endpoint_test.go index cf94bfe7..f3aac798 100644 --- a/internal/observability/endpoint_test.go +++ b/internal/observability/endpoint_test.go @@ -20,10 +20,8 @@ func TestParseEndpoint(t *testing.T) { {name: "https with port and path", in: "https://otlp.example.com:443/v1/traces", wantHost: "otlp.example.com:443", wantTLS: true}, {name: "empty", in: "", wantHost: ""}, {name: "ipv4 plaintext", in: "127.0.0.1:4317", wantHost: "127.0.0.1:4317"}, - // IPv6 cases pin the bracketed-literal behavior end-to-end. The - // current implementation handles them correctly via the path-strip - // loop, but a future refactor to net/url would need to preserve the - // brackets — these cases would catch any regression there. + // Pin IPv6 bracketed-literal behavior — a future refactor to net/url + // would need to preserve the brackets. {name: "ipv6 bare", in: "[::1]:4317", wantHost: "[::1]:4317"}, {name: "ipv6 https", in: "https://[::1]:4317", wantHost: "[::1]:4317", wantTLS: true}, {name: "ipv6 https with path", in: "https://[::1]:4317/otlp", wantHost: "[::1]:4317", wantTLS: true}, diff --git a/internal/observability/provider.go b/internal/observability/provider.go index 885a437a..2d87b03d 100644 --- a/internal/observability/provider.go +++ b/internal/observability/provider.go @@ -34,31 +34,10 @@ import ( // the Do() call site for the trade-off. var runtimeStartOnce sync.Once -// ProviderConfig wires the metrics/traces/logs pipeline. Each output is -// independently gated; SampleRate values must be in [0.0, 1.0] and only apply -// to head-based trace sampling. Log sampling is enforced inside NewLogger -// (per-level). -// -// MetricsEnabled drives the OTLP-push metric exporter; PrometheusEnabled -// drives the Prometheus exposition reader. Either, both, or neither may be -// set — the underlying OTel MeterProvider is the shared substrate. When -// PrometheusEnabled is true InitProvider returns a non-nil promHandler. -// -// Endpoint is the OTLP gRPC target consulted by every enabled OTLP exporter -// unless that signal sets its own TracesEndpoint / MetricsEndpoint / -// LogsEndpoint override (empty means inherit). Each endpoint is parsed via -// ParseEndpoint, so `https://` selects TLS while bare `host:port` or `http://` -// stays plaintext for backward compatibility. -// -// Headers (set, key=value pairs already parsed by config) is applied to every -// OTLP exporter — the standard knob for auth against cloud endpoints. -// -// tlsConfig is unexported and reserved for the integration-test escape hatch -// (FakeOTLPTLS — InsecureSkipVerify against an ephemeral self-signed cert). -// Production code must NOT set it; the system-root TLS config picked from the -// `https://` scheme on Endpoint is what production wants. Test callers reach -// it via SetTLSConfigForTesting so production callers don't get the field -// surfaced in IDE autocomplete. +// ProviderConfig wires the metrics/traces/logs pipeline. Endpoint is the +// default OTLP gRPC target; per-signal {Traces,Metrics,Logs}Endpoint values +// override it (empty inherits). MetricsEnabled and PrometheusEnabled are +// independent — either, both, or neither may be set. type ProviderConfig struct { Endpoint string Headers map[string]string @@ -73,11 +52,9 @@ type ProviderConfig struct { tlsConfig *tls.Config } -// SetTLSConfigForTesting injects a client *tls.Config that overrides the -// default system-roots config for `https://` endpoints. The deliberately -// awkward name signals intent: only the FakeOTLPTLS integration helpers -// should call this. Production code path keeps tlsConfig nil and falls -// through to system roots. +// SetTLSConfigForTesting injects a client *tls.Config for `https://` +// endpoints. Test-only — FakeOTLPTLS uses it to trust its self-signed cert. +// Production keeps tlsConfig nil (system roots). func (c *ProviderConfig) SetTLSConfigForTesting(cfg *tls.Config) { c.tlsConfig = cfg } @@ -92,17 +69,12 @@ func pickEndpoint(override, fallback string) string { // InitProvider sets up the OpenTelemetry pipeline, registering only the // signals enabled in cfg. Always installs the W3C TraceContext + Baggage -// propagator (cheap, harmless when traces are off — in-process span -// extraction still works against a no-op tracer provider). -// -// Returns a shutdown function (always non-nil) and a Prometheus HTTP handler -// (non-nil iff PrometheusEnabled). The handler reads from a private -// prometheus.Registry — global registry pollution is avoided. +// propagator. Returns a non-nil shutdown function and a Prometheus HTTP +// handler (non-nil iff PrometheusEnabled; reads from a private registry). func InitProvider(ctx context.Context, serviceName string, cfg ProviderConfig) (func(context.Context) error, http.Handler, error) { - // Snapshot the OTel globals on entry. On a partial init failure we want to - // roll them back to whatever was installed before — otherwise the caller - // (which continues on init error per main.go) keeps using shut-down - // providers as the global state, strictly worse than the no-op defaults. + // Snapshot globals so a partial-init failure can roll them back — main.go + // continues on init error, and shut-down providers as global state are + // worse than the no-op defaults. prevProp := otel.GetTextMapPropagator() prevTP := otel.GetTracerProvider() prevMP := otel.GetMeterProvider() @@ -119,12 +91,9 @@ func InitProvider(ctx context.Context, serviceName string, cfg ProviderConfig) ( return err } - // On any setup error we run partial shutdown to release whatever was - // already registered AND restore the prior globals so the caller falls back - // to clean state. The setup error itself flows back via the return value; - // the shutdown error is diagnostic-only — surface it on stdout so - // partial-init failures are debuggable in production rather than silently - // swallowed. + // On setup error, release whatever was already registered and restore + // prior globals. Shutdown errors are diagnostic-only — log them rather + // than swallowing. handleErr := func(inErr error) { otel.SetTextMapPropagator(prevProp) otel.SetTracerProvider(prevTP) @@ -201,11 +170,9 @@ func InitProvider(ctx context.Context, serviceName string, cfg ProviderConfig) ( } if cfg.PrometheusEnabled { - // Private Registry — keeps WaveHouse metrics out of the global - // prometheus.DefaultRegisterer (which the prometheus client - // library's process/Go collectors auto-register into). Tests - // and embedded use cases would otherwise see those default - // metrics leak into the /metrics output. + // Private Registry keeps WaveHouse metrics out of + // prometheus.DefaultRegisterer (which the client library's + // process/Go collectors auto-register into). reg := prometheus.NewRegistry() promExporter, err := otelprom.New(otelprom.WithRegisterer(reg)) if err != nil { @@ -224,19 +191,12 @@ func InitProvider(ctx context.Context, serviceName string, cfg ProviderConfig) ( shutdownFuncs = append(shutdownFuncs, meterProvider.Shutdown) otel.SetMeterProvider(meterProvider) - // `runtime.Start` spawns goroutines that the upstream package exposes - // no way to stop. In production InitProvider is called exactly once, - // but tests re-init the provider repeatedly — sync.Once caps the leak - // at one goroutine for the whole process. Trade-off: the runtime - // callbacks stay bound to the FIRST MeterProvider, so subsequent - // re-inits get no runtime metrics on their new MeterProvider. No test - // asserts on runtime metric presence and production never re-inits, - // so this is acceptable until upstream adds a Stop(). - // - // Errors here are intentionally non-fatal: a runtime-instrumentation - // failure shouldn't tear down a fully-initialized OTel pipeline. Log - // and continue with degraded host metrics rather than routing through - // handleErr (which would roll back the globals). + // runtime.Start spawns goroutines with no upstream Stop(); sync.Once + // caps the leak across test re-inits. Trade-off: runtime callbacks + // stay bound to the FIRST MeterProvider, so subsequent re-inits get + // no runtime metrics on their new provider. Errors here are + // non-fatal — a runtime-instrumentation failure shouldn't tear down + // a fully-initialized OTel pipeline. runtimeStartOnce.Do(func() { if err := runtime.Start(runtime.WithMinimumReadMemStatsInterval(15 * time.Second)); err != nil { slog.Warn("OTel runtime instrumentation failed to start; continuing with degraded host metrics", diff --git a/internal/testutil/otlp.go b/internal/testutil/otlp.go index 518018df..ed5cb7f9 100644 --- a/internal/testutil/otlp.go +++ b/internal/testutil/otlp.go @@ -1,23 +1,8 @@ // Package testutil — OTLP gRPC test receiver. // -// FakeOTLP is an in-process OTLP gRPC server for verifying telemetry export -// in tests. It accepts trace, metric, and log Export RPCs, records the -// received payloads, and exposes counts (and the raw payloads) for assertions. -// -// Usage: -// -// r := testutil.NewFakeOTLP(t) -// cfg := observability.ProviderConfig{Endpoint: r.Addr(), ...} -// shutdown, _ := observability.InitProvider(ctx, "svc", cfg) -// ... emit spans/metrics/logs ... -// _ = shutdown(ctx) // forces a final flush -// assert.Equal(t, expected, r.SpanCount()) -// -// Always call shutdown before asserting counts — the OTel SDK batches -// exports and only drains on shutdown (or after the batch timeout). -// -// For TLS verification, use NewFakeOTLPTLS which mints an ephemeral self-signed -// cert and exposes TLSConfig() for a matching client-side config. +// FakeOTLP captures trace/metric/log Export RPCs for test assertions. Call +// the InitProvider shutdown before asserting counts — the OTel SDK only +// drains on shutdown or batch timeout. NewFakeOTLPTLS is the TLS variant. package testutil import ( @@ -46,9 +31,8 @@ import ( tracepb "go.opentelemetry.io/proto/otlp/trace/v1" ) -// FakeOTLP is a single gRPC server that implements the trace, metric, and -// log Export RPCs and captures every received payload. Cleanup is registered -// on the *testing.T automatically. +// FakeOTLP is a gRPC server implementing trace/metric/log Export and +// capturing each request's payload + metadata. Cleanup is auto-registered. type FakeOTLP struct { addr string server *grpc.Server @@ -59,8 +43,6 @@ type FakeOTLP struct { metrics []*metricspb.ResourceMetrics logs []*logspb.ResourceLogs - // Captured gRPC request metadata, indexed by signal. Each Export call - // appends an entry. Tests assert on auth/header propagation through these. traceHeaders []metadata.MD metricHeaders []metadata.MD logHeaders []metadata.MD @@ -74,11 +56,8 @@ func NewFakeOTLP(t *testing.T) *FakeOTLP { return newFakeOTLP(t, nil) } -// NewFakeOTLPTLS is the TLS variant: an ephemeral self-signed cert (SAN -// 127.0.0.1) is generated, and the server listens with that cert. The matching -// client config is available via TLSConfig() — wire it into ProviderConfig so -// the OTel exporters trust the cert. Production code never sets ProviderConfig.TLSConfig; -// only this test path does. +// NewFakeOTLPTLS is the TLS variant: mints an ephemeral self-signed cert +// (SAN 127.0.0.1) and exposes the matching client *tls.Config via TLSConfig(). func NewFakeOTLPTLS(t *testing.T) *FakeOTLP { t.Helper() diff --git a/tests/integration/otel_test.go b/tests/integration/otel_test.go index 06f57879..062e51a9 100644 --- a/tests/integration/otel_test.go +++ b/tests/integration/otel_test.go @@ -1,17 +1,9 @@ //go:build integration -// OTel pipeline integration tests. +// OTel pipeline integration tests against testutil.FakeOTLP. // -// These tests exercise observability.InitProvider against an in-process -// OTLP gRPC receiver (testutil.FakeOTLP). They verify that sampling rates, -// per-signal gates, and unreachable-endpoint behavior all do what config -// says they do — the kind of regression that unit tests against a no-op -// global can miss. -// -// InitProvider mutates global OTel state (tracer/meter/logger providers, -// propagator). Tests in this file MUST NOT run in parallel and MUST save/ -// restore the globals on entry/exit. They share the `env(t)` infrastructure -// only incidentally — none of them touch ClickHouse or the API server. +// InitProvider mutates global OTel state; tests must NOT run in parallel +// and must save/restore globals via guardOTelGlobals. package tests @@ -34,8 +26,8 @@ import ( "github.com/Wave-RF/WaveHouse/internal/testutil" ) -// guardOTelGlobals saves and restores the package-global OTel providers so a -// test's InitProvider call doesn't leak its state into later tests. +// guardOTelGlobals saves and restores the OTel package-global providers so +// a test's InitProvider call doesn't leak into later tests. func guardOTelGlobals(t *testing.T) { t.Helper() savedProp := otel.GetTextMapPropagator() @@ -50,10 +42,9 @@ func guardOTelGlobals(t *testing.T) { }) } -// initAndShutdown installs the OTel pipeline with the given config and -// returns the shutdown func plus the Prometheus handler (non-nil only when -// cfg.PrometheusEnabled is true). Caller is responsible for calling -// shutdown to drain pending exports before asserting on the receiver. +// initAndShutdown installs the OTel pipeline and returns shutdown + the +// Prometheus handler (non-nil iff cfg.PrometheusEnabled). Caller drains via +// shutdown before asserting on the receiver. func initAndShutdown(t *testing.T, cfg observability.ProviderConfig) (func(context.Context) error, http.Handler) { t.Helper() ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) @@ -65,11 +56,8 @@ func initAndShutdown(t *testing.T, cfg observability.ProviderConfig) (func(conte } func TestOTel_TraceSampling(t *testing.T) { - // TraceIDRatioBased over random trace IDs is binomial(n, rate). For - // rate=0.5 / n=2000 the stddev is ~22; ±200 is ~9σ — flake-proof but tight - // enough to catch a sampler accidentally pinned at 25% / 75% (would land - // at 500 / 1500 and slip past a wider window). Bypass (count→n) and - // broken (count→0) failure modes still fail loud regardless of tolerance. + // Binomial(n, rate); ±200 over n=2000 is ~9σ — flake-proof and still + // catches a sampler pinned at 25%/75% (would land at 500/1500). cases := []struct { name string rate float64 @@ -124,8 +112,6 @@ func TestOTel_LogSampling_WarnFloorAlwaysExports(t *testing.T) { guardOTelGlobals(t) r := testutil.NewFakeOTLP(t) - // Logs path: enable the OTel logger pipeline first, then build the - // slog logger that fans out to (stdout, OTLP) and sample DEBUG/INFO. shutdown, _ := initAndShutdown(t, observability.ProviderConfig{ Endpoint: r.Addr(), LogsEnabled: true, @@ -133,7 +119,7 @@ func TestOTel_LogSampling_WarnFloorAlwaysExports(t *testing.T) { lvl := &slog.LevelVar{} lvl.Set(slog.LevelDebug) - // sample_rate=0.0 → DEBUG/INFO entirely dropped from OTLP, WARN+ still 100%. + // rate=0.0 → DEBUG/INFO dropped from OTLP; WARN+ still 100%. logger := observability.NewLogger("wavehouse-test", lvl, true, 0.0) const n = 50 @@ -147,9 +133,8 @@ func TestOTel_LogSampling_WarnFloorAlwaysExports(t *testing.T) { defer drainCancel() require.NoError(t, shutdown(drainCtx)) - // OTel severity numbers: DEBUG=5, INFO=9, WARN=13, ERROR=17. - // `LogCountAtLevel(13)` is "WARN and above" (testutil semantics), so the - // complement is DEBUG+INFO — the records the rate=0.0 sampler should drop. + // OTel severity: DEBUG=5, INFO=9, WARN=13, ERROR=17. LogCountAtLevel(13) + // = WARN+; complement is DEBUG+INFO (what rate=0.0 should drop). lowSevCount := r.LogCount() - r.LogCountAtLevel(13) warnAndAboveCount := r.LogCountAtLevel(13) assert.Zero(t, lowSevCount, "DEBUG+INFO records should have been dropped at sample_rate=0.0") @@ -175,11 +160,9 @@ func TestOTel_PerSignal_TracesOnly(t *testing.T) { defer drainCancel() require.NoError(t, shutdown(drainCtx)) - // Span emitted before shutdown — count should be set on return. assert.Equal(t, 1, r.SpanCount()) - // Negative assertions: poll for 100ms in case a misconfigured exporter - // would emit a stray RPC. require.Never is the testify-native primitive - // for "this must stay false for window X" — clearer than a bare sleep. + // require.Never polls for 100ms to catch any stray RPC from a + // misconfigured exporter. require.Never(t, func() bool { return r.MetricCount() > 0 }, 100*time.Millisecond, 10*time.Millisecond, "metrics disabled — no metric records should appear") require.Never(t, func() bool { return r.LogCount() > 0 }, 100*time.Millisecond, 10*time.Millisecond, @@ -202,16 +185,11 @@ func TestOTel_PrometheusScrape_ExposesMetrics(t *testing.T) { }) require.NotNil(t, promHandler, "prometheus handler should be non-nil when PrometheusEnabled=true") - // Record a known custom metric so we can assert it appears at /metrics. - // OTel→Prometheus name translation: dots/dashes become underscores, - // counter suffix `_total` is appended automatically. meter := otel.GetMeterProvider().Meter("wavehouse-test") counter, err := meter.Int64Counter("test_widget_received") require.NoError(t, err) counter.Add(context.Background(), 7) - // Scrape via httptest. We don't go over the wire; the handler is the - // same one main.go would mount on the API router or sidecar listener. server := httptest.NewServer(promHandler) t.Cleanup(server.Close) @@ -224,8 +202,7 @@ func TestOTel_PrometheusScrape_ExposesMetrics(t *testing.T) { require.NoError(t, err) bodyStr := string(body) - // Counter ending in _total is the standard Prometheus convention; the - // OTel exporter applies it automatically. + // The OTel→Prometheus exporter appends _total to counters automatically. assert.Contains(t, bodyStr, "test_widget_received_total", "/metrics must list the custom counter we recorded") assert.Contains(t, bodyStr, "# HELP", "Prometheus format includes HELP lines") assert.Contains(t, bodyStr, "# TYPE", "Prometheus format includes TYPE lines") @@ -262,10 +239,8 @@ func TestOTel_PerSignal_LogsOnly(t *testing.T) { func TestOTel_UnreachableEndpoint_DoesNotBlockStartupOrEmits(t *testing.T) { guardOTelGlobals(t) - // 127.0.0.1:1 is in the unassigned-port range — connect should never - // succeed. gRPC exporters dial lazily, so InitProvider must still - // succeed regardless. This is the critical "OTel down doesn't kill - // the binary" invariant. + // 127.0.0.1:1 never connects. Pins the "OTel down doesn't kill the + // binary" invariant — gRPC exporters dial lazily. cfg := observability.ProviderConfig{ Endpoint: "127.0.0.1:1", TracesEnabled: true, @@ -280,10 +255,8 @@ func TestOTel_UnreachableEndpoint_DoesNotBlockStartupOrEmits(t *testing.T) { require.NoError(t, err, "InitProvider must not fail on unreachable endpoint") require.NotNil(t, shutdown) - // Emit work — neither span.End nor logger.Info may block on the failed - // export. The SDK buffers in-memory and the batch processor handles - // drops asynchronously; this is the contract that lets the request hot - // path survive collector outages. + // span.End / logger.Info must never block on the failed export — the + // SDK buffers in-memory and the batch processor handles drops async. lvl := &slog.LevelVar{} logger := observability.NewLogger("wavehouse-test", lvl, true, 1.0) @@ -303,11 +276,9 @@ func TestOTel_UnreachableEndpoint_DoesNotBlockStartupOrEmits(t *testing.T) { t.Fatal("emits blocked on unreachable endpoint") } - // Best-effort shutdown in a goroutine so the test doesn't leak the - // runtime-metrics goroutine. We don't assert anything about it — the - // OTel SDK doesn't fully honor the shutdown deadline against an - // unreachable gRPC endpoint, and main.go bounds the timeout for the - // same reason. See the defer wrapping otelShutdown in cmd/wavehouse/main.go. + // Best-effort shutdown in a goroutine — the OTel SDK doesn't fully + // honor shutdown deadlines against an unreachable endpoint (same reason + // main.go bounds its shutdown context). go func() { drainCtx, drainCancel := context.WithTimeout(context.Background(), 10*time.Second) defer drainCancel() @@ -315,18 +286,10 @@ func TestOTel_UnreachableEndpoint_DoesNotBlockStartupOrEmits(t *testing.T) { }() } -// TestOTel_TLSPath_AllSignals locks in the https:// → TLS dial path on every -// OTLP exporter. The fake receiver listens with an ephemeral self-signed cert; -// each exporter is given the matching client tls.Config via the test-only -// SetTLSConfigForTesting setter. If TLS wiring regresses on any signal (e.g. -// someone re-adds WithInsecure unconditionally on just the metrics or logs -// branch of provider.go), that signal will TLS-handshake against a plaintext -// server and the export drops — caught here as a missing count on the right -// receiver. -// -// Headers are intentionally not asserted; TestOTel_Headers_AppliedToAllSignals -// owns the header-propagation invariant. This test owns the TLS-dial invariant -// across signals. +// TestOTel_TLSPath_AllSignals pins the https:// → TLS dial path on every +// OTLP exporter (traces, metrics, logs). A regression that re-adds +// WithInsecure on one branch would TLS-handshake against the plaintext side +// and surface as a missing count on the corresponding receiver. func TestOTel_TLSPath_AllSignals(t *testing.T) { guardOTelGlobals(t) r := testutil.NewFakeOTLPTLS(t) @@ -362,11 +325,9 @@ func TestOTel_TLSPath_AllSignals(t *testing.T) { assert.GreaterOrEqual(t, r.LogCount(), 1, "TLS path must deliver logs end-to-end") } -// TestOTel_Headers_AppliedToAllSignals verifies that ProviderConfig.Headers -// propagates as gRPC metadata on every OTLP exporter (traces, metrics, logs). -// Direct-to-cloud auth depends on this — Honeycomb/Grafana Cloud both -// authenticate per-RPC via a header, so a single missing exporter would 401 -// silently for that signal. +// TestOTel_Headers_AppliedToAllSignals verifies ProviderConfig.Headers +// propagates as gRPC metadata on every OTLP exporter — a single missing +// exporter would silently 401 against cloud OTLP gateways. func TestOTel_Headers_AppliedToAllSignals(t *testing.T) { guardOTelGlobals(t) r := testutil.NewFakeOTLP(t) @@ -401,10 +362,7 @@ func TestOTel_Headers_AppliedToAllSignals(t *testing.T) { defer drainCancel() require.NoError(t, shutdown(drainCtx)) - // gRPC metadata keys are lowercased on the wire — assert in lowercase. - // Every configured header is checked on every signal so a per-exporter - // header-propagation bug (e.g. one exporter built with a stale headers - // map) fails loud instead of silently 401'ing on the missing signal. + // gRPC metadata keys are lowercased on the wire. signals := []struct { name string md func() metadata.MD @@ -425,14 +383,9 @@ func TestOTel_Headers_AppliedToAllSignals(t *testing.T) { } } -// TestOTel_PerSignalEndpoint_Override verifies that TracesEndpoint, -// MetricsEndpoint, and LogsEndpoint each route their own signal to a distinct -// receiver while the default Endpoint sees none. Grafana Cloud's per-signal -// gateway hosts are the headline use case. All three signal overrides go -// through the same pickEndpoint() helper in provider.go, so a copy-paste bug -// (e.g. the metrics exporter being wired to TracesEndpoint, or the logs -// exporter inheriting TracesEndpoint) would surface here as a signal landing -// on the wrong receiver — caught by the cross-asserts below. +// TestOTel_PerSignalEndpoint_Override verifies each per-signal endpoint +// routes its own signal to a distinct receiver. The cross-asserts catch +// copy-paste wiring bugs (e.g. metrics exporter pointed at TracesEndpoint). func TestOTel_PerSignalEndpoint_Override(t *testing.T) { guardOTelGlobals(t) rDefault := testutil.NewFakeOTLP(t) From 5a611069b1630ac514b68de6be2ce3d27f8dcb9e Mon Sep 17 00:00:00 2001 From: Jack Woods Date: Fri, 15 May 2026 13:33:23 -0400 Subject: [PATCH 13/17] docs+observability: round-2 review fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move standalone.yaml's commented-out observability env-var block to deployment.md → "Observability via Compose" (CodeRabbit). The compose file kept duplicating documentation that lives in deployment.md; the new section uses Compose service-DNS (otel-collector:4317) instead of the loopback example that wouldn't have resolved from inside the wavehouse container. - Pin FakeOTLPTLS fixture to TLS 1.3 on both server and client configs (CodeRabbit). Matches the production floor in tlsConfigOrDefault — a regression below 1.3 should fail the handshake rather than negotiate to 1.2 silently. - Acquire r.mu before reading the captured-headers slices in LastTraceHeaders / LastMetricHeaders / LastLogHeaders (CodeRabbit). Return metadata.MD.Copy() so callers get an independent map. Closes a race against Reset() that today is benign in test usage but lives on a public API. Co-Authored-By: Claude Opus 4.7 (1M context) --- deployments/compose/standalone.yaml | 23 ++----------- docs/src/content/docs/deployment.md | 32 ++++++++++++++++++ internal/testutil/otlp.go | 51 ++++++++++++++++++++--------- 3 files changed, 70 insertions(+), 36 deletions(-) diff --git a/deployments/compose/standalone.yaml b/deployments/compose/standalone.yaml index a817a002..7581d92a 100644 --- a/deployments/compose/standalone.yaml +++ b/deployments/compose/standalone.yaml @@ -32,27 +32,8 @@ services: # read-only and uncomment WH_PIPES_DIR. The directory is a seed for # NATS KV — runtime pipe edits go through the API, not the files. # WH_PIPES_DIR: /app/pipes - # ---- Observability (all optional, all default off) --------------- - # OTLP push: uncomment WH_OTEL_ENABLED=true and point WH_OTEL_ADDR - # at the collector. https:// → TLS via system roots; bare host:port - # or http:// stays plaintext. See docs/deployment.md for worked - # Honeycomb / Grafana Cloud / DDOT-Collector examples. - # WH_OTEL_ENABLED: "true" - # WH_OTEL_ADDR: "127.0.0.1:4317" # or https://api.honeycomb.io:443 - # WH_OTEL_HEADERS: "" # "key=value,key2=value2" for cloud auth - # WH_OTEL_TRACES_ENABLED: "true" - # WH_OTEL_TRACES_SAMPLE_RATE: "1.0" - # WH_OTEL_TRACES_ADDR: "" # per-signal endpoint override; empty = inherit WH_OTEL_ADDR - # WH_OTEL_METRICS_ENABLED: "true" - # WH_OTEL_METRICS_ADDR: "" # per-signal endpoint override - # WH_OTEL_LOGS_ENABLED: "true" - # WH_OTEL_LOGS_SAMPLE_RATE: "1.0" # DEBUG/INFO OTLP rate; WARN+ always 100% - # WH_OTEL_LOGS_ADDR: "" # per-signal endpoint override - # Prometheus exposition is independent of OTLP push — pull-based - # scrapers (Alloy / Mimir) can use this alone with no collector. - # WH_PROMETHEUS_ENABLED: "true" - # WH_PROMETHEUS_PATH: "/metrics" - # WH_PROMETHEUS_PORT: "0" # 0 mounts on server.port; non-zero = sidecar listener + # Observability env vars (WH_OTEL_*, WH_PROMETHEUS_*) are documented in + # docs/src/content/docs/deployment.md → "Observability via Compose". volumes: - wavehouse-data:/app/data # - ./my-pipes:/app/pipes:ro # uncomment alongside WH_PIPES_DIR diff --git a/docs/src/content/docs/deployment.md b/docs/src/content/docs/deployment.md index 7c53b439..92d275c6 100644 --- a/docs/src/content/docs/deployment.md +++ b/docs/src/content/docs/deployment.md @@ -314,6 +314,38 @@ Set `otel.enabled: true` (or `WH_OTEL_ENABLED=true`) and point `otel.addr` at th WaveHouse **pushes** to an OTel collector; scraping-style pipelines (Promtail/Grafana Alloy → Loki, Vector, Fluent Bit) read stdout directly and own their own sample rates. The `otel.{traces,logs}.sample_rate` knobs apply only to the OTLP push path. Stdout always emits 100%. The logger fans out to both stdout and OTLP, so stdout output never disappears regardless of collector state. gRPC exporters are lazy, so an unreachable collector does not block startup — transient export errors are surfaced via the OTel SDK's error handler instead. +### Observability via Compose + +`deployments/compose/standalone.yaml` ships with observability disabled. To enable it, add the variables below to the `wavehouse` service's `environment:` block. All are optional and default off. + +```yaml +services: + wavehouse: + environment: + # OTLP push: scheme on WH_OTEL_ADDR selects transport — https:// → TLS + # via system roots; bare host:port or http:// stays plaintext. Use the + # Compose service name (or host.docker.internal for a host process), + # not 127.0.0.1, which won't reach a separate container. + WH_OTEL_ENABLED: "true" + WH_OTEL_ADDR: "otel-collector:4317" # or https://api.honeycomb.io:443 + WH_OTEL_HEADERS: "" # "key=value,key2=value2" for cloud auth + WH_OTEL_TRACES_ENABLED: "true" + WH_OTEL_TRACES_SAMPLE_RATE: "1.0" + WH_OTEL_TRACES_ADDR: "" # per-signal override; empty = inherit WH_OTEL_ADDR + WH_OTEL_METRICS_ENABLED: "true" + WH_OTEL_METRICS_ADDR: "" + WH_OTEL_LOGS_ENABLED: "true" + WH_OTEL_LOGS_SAMPLE_RATE: "1.0" # DEBUG/INFO OTLP rate; WARN+ always 100% + WH_OTEL_LOGS_ADDR: "" + # Prometheus exposition is independent of OTLP push — pull-based + # scrapers (Alloy / Mimir) can use this alone with no collector. + WH_PROMETHEUS_ENABLED: "true" + WH_PROMETHEUS_PATH: "/metrics" + WH_PROMETHEUS_PORT: "0" # 0 mounts on server.port; non-zero = sidecar listener +``` + +Worked end-to-end examples (Honeycomb, Grafana Cloud, Datadog DDOT) follow below. + ### Pattern: SigNoz / OTel-native backends (local collector) Point `otel.addr` at a plaintext OTLP gRPC endpoint. All three signals (traces, metrics, logs) push through the same connection. This is the default and the simplest setup. diff --git a/internal/testutil/otlp.go b/internal/testutil/otlp.go index ed5cb7f9..11ce1e88 100644 --- a/internal/testutil/otlp.go +++ b/internal/testutil/otlp.go @@ -62,7 +62,13 @@ func NewFakeOTLPTLS(t *testing.T) *FakeOTLP { t.Helper() cert, clientCfg := ephemeralTLSPair(t) - serverCfg := &tls.Config{Certificates: []tls.Certificate{cert}} + // Pinned to TLS 1.3 on both sides to match the production floor in + // observability.tlsConfigOrDefault — a regression below TLS 1.3 should + // fail the handshake here rather than negotiate to 1.2 silently. + serverCfg := &tls.Config{ + Certificates: []tls.Certificate{cert}, + MinVersion: tls.VersionTLS13, + } return newFakeOTLP(t, &fakeOTLPTLS{server: serverCfg, client: clientCfg}) } @@ -141,7 +147,11 @@ func ephemeralTLSPair(t *testing.T) (tls.Certificate, *tls.Config) { } pool := x509.NewCertPool() pool.AddCert(parsed) - return cert, &tls.Config{RootCAs: pool, ServerName: "127.0.0.1"} + return cert, &tls.Config{ + RootCAs: pool, + ServerName: "127.0.0.1", + MinVersion: tls.VersionTLS13, + } } // Addr returns the listener address (e.g. "127.0.0.1:42891") suitable for @@ -212,25 +222,36 @@ func (r *FakeOTLP) LogCountAtLevel(minSeverity int32) int { return n } -// LastTraceHeaders returns the gRPC metadata captured from the most recent -// trace Export RPC, or nil if none. -func (r *FakeOTLP) LastTraceHeaders() metadata.MD { return lastMD(&r.mu, r.traceHeaders) } +// LastTraceHeaders returns a copy of the gRPC metadata captured from the +// most recent trace Export RPC, or nil if none. +func (r *FakeOTLP) LastTraceHeaders() metadata.MD { + r.mu.Lock() + defer r.mu.Unlock() + return lastMDCopy(r.traceHeaders) +} -// LastMetricHeaders returns the gRPC metadata captured from the most recent -// metric Export RPC, or nil if none. -func (r *FakeOTLP) LastMetricHeaders() metadata.MD { return lastMD(&r.mu, r.metricHeaders) } +// LastMetricHeaders returns a copy of the gRPC metadata captured from the +// most recent metric Export RPC, or nil if none. +func (r *FakeOTLP) LastMetricHeaders() metadata.MD { + r.mu.Lock() + defer r.mu.Unlock() + return lastMDCopy(r.metricHeaders) +} -// LastLogHeaders returns the gRPC metadata captured from the most recent log -// Export RPC, or nil if none. -func (r *FakeOTLP) LastLogHeaders() metadata.MD { return lastMD(&r.mu, r.logHeaders) } +// LastLogHeaders returns a copy of the gRPC metadata captured from the most +// recent log Export RPC, or nil if none. +func (r *FakeOTLP) LastLogHeaders() metadata.MD { + r.mu.Lock() + defer r.mu.Unlock() + return lastMDCopy(r.logHeaders) +} -func lastMD(mu *sync.Mutex, slice []metadata.MD) metadata.MD { - mu.Lock() - defer mu.Unlock() +// lastMDCopy must be called with r.mu held — it reads the slice header. +func lastMDCopy(slice []metadata.MD) metadata.MD { if len(slice) == 0 { return nil } - return slice[len(slice)-1] + return slice[len(slice)-1].Copy() } // Reset clears all captured payloads. Useful between test phases. From 28ed310fb06a19a23e799220bbb37e84db8964d0 Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Mon, 18 May 2026 14:01:28 -0400 Subject: [PATCH 14/17] test(e2e): probe /ready and `wavehouse health` subcommand in orchestrator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After merging main's boot-resilience and health work (#125, #122), e2e coverage dipped to 49.9% (gate is 50%) because the Readiness handler and the cmd/wavehouse/health.go probe binary were uncovered by the SDK harness. Both are production code paths the operator-facing contract (k8s readiness, Docker HEALTHCHECK) depends on — exercising them in the e2e harness is principled, not a coverage hack. Brings e2e from 49.9% → 50.9%. Co-Authored-By: Claude Opus 4.7 (1M context) --- scripts/orchestrator/main.go | 38 ++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/scripts/orchestrator/main.go b/scripts/orchestrator/main.go index ebfa5399..25b3f781 100644 --- a/scripts/orchestrator/main.go +++ b/scripts/orchestrator/main.go @@ -188,6 +188,29 @@ func run() error { } log.Println("✓ WaveHouse healthy") + // /ready exercises the readiness probe (BootState check + ClickHouse + // ping). It's the kubelet/k8s contract — separate from /health which + // is liveness-only. CH ping may need a moment after the testcontainer + // reports listening, hence the retry budget. + if err := waitForHealth(ctx, whURL+"/ready", 10*time.Second); err != nil { + _ = whCmd.Process.Signal(syscall.SIGINT) + <-whDone + dumpLogTail(whLogPath, "wavehouse never became ready") + return fmt.Errorf("wavehouse not ready: %w", err) + } + log.Println("✓ WaveHouse ready") + + // Self-probe via the `wavehouse health` subcommand — same code path the + // distroless Dockerfile HEALTHCHECK uses. Running it here confirms the + // probe binary itself stays in sync with the in-process /health route. + if err := runSelfHealthProbe(ctx, binPath, whPort, coverDir); err != nil { + _ = whCmd.Process.Signal(syscall.SIGINT) + <-whDone + dumpLogTail(whLogPath, "wavehouse health subcommand probe failed") + return fmt.Errorf("wavehouse health subcommand: %w", err) + } + log.Println("✓ wavehouse health subcommand OK") + log.Println("→ running vitest harness...") vitest := exec.CommandContext(ctx, "pnpm", "run", "test") vitest.Dir = filepath.Join(repoRoot, "tests", "e2e", "sdk") @@ -304,6 +327,21 @@ func waitForHealth(ctx context.Context, url string, timeout time.Duration) error return fmt.Errorf("not reachable within %s", timeout) } +// runSelfHealthProbe execs `bin/wavehouse-cov health` against the running +// server. Mirrors the distroless Dockerfile HEALTHCHECK invocation. Coverage +// from the short-lived subprocess flushes into the same GOCOVERDIR as the +// long-running daemon. +func runSelfHealthProbe(ctx context.Context, binPath string, port int, coverDir string) error { + // #nosec G204 — binPath is the cover binary the orchestrator already + // launched; port/coverDir are locally constructed. + cmd := exec.CommandContext(ctx, binPath, "health") + cmd.Env = append(os.Environ(), + "GOCOVERDIR="+coverDir, + "WH_SERVER_PORT="+strconv.Itoa(port), + ) + return cmd.Run() +} + // SIGINT-shutdown of a Go program returns nil (clean exit) but the // subprocess.Wait wrapper sometimes surfaces an exit error of "signal: interrupt" // — treat that as expected. From dc4e31b2771ca9278b54cb24a95372d394f699ca Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Mon, 18 May 2026 14:14:41 -0400 Subject: [PATCH 15/17] review(observability,orchestrator): gate TLS test hook with build tag, fix env duplication - internal/observability: move SetTLSConfigForTesting into a //go:build integration file. Production binaries no longer expose a method that can inject InsecureSkipVerify at runtime; the integration suite (which already builds with -tags=integration) keeps access unchanged. - scripts/orchestrator: filter os.Environ() before appending the per-subprocess overrides. append(os.Environ(), "K=v") is silently wrong when K is already set in the parent shell (glibc/darwin getenv() takes the first match); a developer with WH_SERVER_PORT exported would have hit a confusing health-probe failure. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/observability/provider.go | 7 ---- internal/observability/tlsconfig_testhook.go | 14 ++++++++ scripts/orchestrator/main.go | 35 ++++++++++++++++++-- 3 files changed, 46 insertions(+), 10 deletions(-) create mode 100644 internal/observability/tlsconfig_testhook.go diff --git a/internal/observability/provider.go b/internal/observability/provider.go index 2d87b03d..76e37780 100644 --- a/internal/observability/provider.go +++ b/internal/observability/provider.go @@ -52,13 +52,6 @@ type ProviderConfig struct { tlsConfig *tls.Config } -// SetTLSConfigForTesting injects a client *tls.Config for `https://` -// endpoints. Test-only — FakeOTLPTLS uses it to trust its self-signed cert. -// Production keeps tlsConfig nil (system roots). -func (c *ProviderConfig) SetTLSConfigForTesting(cfg *tls.Config) { - c.tlsConfig = cfg -} - // pickEndpoint returns override if set, otherwise fallback. func pickEndpoint(override, fallback string) string { if override != "" { diff --git a/internal/observability/tlsconfig_testhook.go b/internal/observability/tlsconfig_testhook.go new file mode 100644 index 00000000..512ca5a3 --- /dev/null +++ b/internal/observability/tlsconfig_testhook.go @@ -0,0 +1,14 @@ +//go:build integration + +package observability + +import "crypto/tls" + +// SetTLSConfigForTesting injects a client *tls.Config for `https://` OTLP +// endpoints. Only compiled when built with `-tags integration` — the +// production binary doesn't see this method at all, so a TLS override hook +// that only works during tests cannot be called in prod. FakeOTLPTLS uses +// it to trust its self-signed cert. +func (c *ProviderConfig) SetTLSConfigForTesting(cfg *tls.Config) { + c.tlsConfig = cfg +} diff --git a/scripts/orchestrator/main.go b/scripts/orchestrator/main.go index 25b3f781..7673acfa 100644 --- a/scripts/orchestrator/main.go +++ b/scripts/orchestrator/main.go @@ -39,6 +39,7 @@ import ( "os/signal" "path/filepath" "strconv" + "strings" "syscall" "time" @@ -137,7 +138,13 @@ func run() error { // #nosec G204 — binPath is filepath.Join(repoRoot, "bin", "wavehouse-cov"), // not user-controlled. The test harness must launch the cover binary. whCmd := exec.CommandContext(ctx, binPath) - whCmd.Env = append(os.Environ(), + whCmd.Env = append(filterEnv(os.Environ(), + "GOCOVERDIR", "WH_SERVER_PORT", "WH_CH_ADDR", "WH_CH_HTTP_PORT", + "WH_DATA_DIR", "WH_MQ_MAX_BYTES_GB", "WH_AUTH_ENABLED", + "WH_AUTH_JWT_SECRET", "WH_AUTH_DEV_MODE", "WH_AUTH_ROLE_CLAIM", + "WH_DEDUPE_ENABLED", "WH_SCHEMA_REFRESH_INTERVAL", "WH_DLQ_ENABLED", + "WH_SERVER_CORS_ALLOWED_ORIGINS", "WH_OTEL_ENABLED", "WH_OTEL_ADDR", + ), "GOCOVERDIR="+coverDir, "WH_SERVER_PORT="+strconv.Itoa(whPort), "WH_CH_ADDR="+chAddr, @@ -214,7 +221,7 @@ func run() error { log.Println("→ running vitest harness...") vitest := exec.CommandContext(ctx, "pnpm", "run", "test") vitest.Dir = filepath.Join(repoRoot, "tests", "e2e", "sdk") - vitest.Env = append(os.Environ(), + vitest.Env = append(filterEnv(os.Environ(), "WAVEHOUSE_URL", "CLICKHOUSE_URL"), "WAVEHOUSE_URL="+whURL, "CLICKHOUSE_URL="+chHTTPURL, ) @@ -335,13 +342,35 @@ func runSelfHealthProbe(ctx context.Context, binPath string, port int, coverDir // #nosec G204 — binPath is the cover binary the orchestrator already // launched; port/coverDir are locally constructed. cmd := exec.CommandContext(ctx, binPath, "health") - cmd.Env = append(os.Environ(), + cmd.Env = append(filterEnv(os.Environ(), "GOCOVERDIR", "WH_SERVER_PORT"), "GOCOVERDIR="+coverDir, "WH_SERVER_PORT="+strconv.Itoa(port), ) return cmd.Run() } +// filterEnv returns env with any KEY=VALUE entries for the given keys +// removed. Needed because os.Exec inherits the parent's environment, and +// `append(os.Environ(), "K=v")` is silently wrong when K already exists: +// glibc/darwin getenv() returns the FIRST match, so the appended value is +// shadowed by whatever the developer's shell already had set. +func filterEnv(env []string, keys ...string) []string { + out := make([]string, 0, len(env)) + for _, e := range env { + keep := true + for _, k := range keys { + if strings.HasPrefix(e, k+"=") { + keep = false + break + } + } + if keep { + out = append(out, e) + } + } + return out +} + // SIGINT-shutdown of a Go program returns nil (clean exit) but the // subprocess.Wait wrapper sometimes surfaces an exit error of "signal: interrupt" // — treat that as expected. From d5f510d631ab4478330b4e4750b3b6b78d92f298 Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Mon, 18 May 2026 14:45:51 -0400 Subject: [PATCH 16/17] review(orchestrator): prefix-match WH_ in filterEnv instead of enumerating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review on PR #136: the explicit list was missing the four WH_OTEL_* vars introduced by this PR (HEADERS, TRACES_ADDR, METRICS_ADDR, LOGS_ADDR), and would keep silently lagging every time a new WH_ var lands. Switch filterEnv to recognise a trailing "_" as a prefix match — "WH_" now strips every wavehouse-owned env var in one go, no enumeration to keep in sync. Co-Authored-By: Claude Opus 4.7 (1M context) --- scripts/orchestrator/main.go | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/scripts/orchestrator/main.go b/scripts/orchestrator/main.go index 7673acfa..4a51b206 100644 --- a/scripts/orchestrator/main.go +++ b/scripts/orchestrator/main.go @@ -138,13 +138,7 @@ func run() error { // #nosec G204 — binPath is filepath.Join(repoRoot, "bin", "wavehouse-cov"), // not user-controlled. The test harness must launch the cover binary. whCmd := exec.CommandContext(ctx, binPath) - whCmd.Env = append(filterEnv(os.Environ(), - "GOCOVERDIR", "WH_SERVER_PORT", "WH_CH_ADDR", "WH_CH_HTTP_PORT", - "WH_DATA_DIR", "WH_MQ_MAX_BYTES_GB", "WH_AUTH_ENABLED", - "WH_AUTH_JWT_SECRET", "WH_AUTH_DEV_MODE", "WH_AUTH_ROLE_CLAIM", - "WH_DEDUPE_ENABLED", "WH_SCHEMA_REFRESH_INTERVAL", "WH_DLQ_ENABLED", - "WH_SERVER_CORS_ALLOWED_ORIGINS", "WH_OTEL_ENABLED", "WH_OTEL_ADDR", - ), + whCmd.Env = append(filterEnv(os.Environ(), "WH_", "GOCOVERDIR"), "GOCOVERDIR="+coverDir, "WH_SERVER_PORT="+strconv.Itoa(whPort), "WH_CH_ADDR="+chAddr, @@ -342,24 +336,33 @@ func runSelfHealthProbe(ctx context.Context, binPath string, port int, coverDir // #nosec G204 — binPath is the cover binary the orchestrator already // launched; port/coverDir are locally constructed. cmd := exec.CommandContext(ctx, binPath, "health") - cmd.Env = append(filterEnv(os.Environ(), "GOCOVERDIR", "WH_SERVER_PORT"), + cmd.Env = append(filterEnv(os.Environ(), "WH_", "GOCOVERDIR"), "GOCOVERDIR="+coverDir, "WH_SERVER_PORT="+strconv.Itoa(port), ) return cmd.Run() } -// filterEnv returns env with any KEY=VALUE entries for the given keys -// removed. Needed because os.Exec inherits the parent's environment, and -// `append(os.Environ(), "K=v")` is silently wrong when K already exists: -// glibc/darwin getenv() returns the FIRST match, so the appended value is -// shadowed by whatever the developer's shell already had set. -func filterEnv(env []string, keys ...string) []string { +// filterEnv returns env without entries whose KEY matches any pat. A pat +// ending in "_" is treated as a key prefix ("WH_" → drop WH_FOO, WH_BAR, +// …); otherwise it's an exact key match. +// +// Why bother: cgo paths (and some C deps of the wavehouse-cov binary) use +// libc getenv() which returns the FIRST match on duplicates, so +// append(os.Environ(), "K=v") silently shadows the override when K is +// already set in a developer's shell. Prefix-matching WH_ covers every +// current and future wavehouse-owned env var without enumeration. +func filterEnv(env []string, pats ...string) []string { out := make([]string, 0, len(env)) for _, e := range env { keep := true - for _, k := range keys { - if strings.HasPrefix(e, k+"=") { + for _, p := range pats { + if strings.HasSuffix(p, "_") { + if strings.HasPrefix(e, p) { + keep = false + break + } + } else if strings.HasPrefix(e, p+"=") { keep = false break } From 9e5423c843f81c5ad7a04024d718ea4bc6b34d55 Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Mon, 18 May 2026 15:04:33 -0400 Subject: [PATCH 17/17] review(otel): reject duplicate header keys; cover TLS + per-signal endpoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups from the latest PR #136 review pass: - ParseOTelHeaders / validateOTelHeaders now reject duplicate keys rather than silently letting the last entry win. A user who pastes `authorization=foo,authorization=bar` previously shipped only `bar` with no boot-time indication — bad failure mode in an auth-sensitive context. Parity test gets the matching case. - New TestOTel_TLSPath_PerSignalEndpoint covers the production Grafana-Cloud wiring: three TLS receivers, per-signal endpoints, and one merged client trust pool. Catches any future regression where the per-signal endpoint parsing silently drops the https:// scheme. Required exposing FakeOTLP.Cert() so the test can build the merged pool across three independently-minted self-signed certs. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/config/config.go | 5 ++ internal/config/header_parity_test.go | 1 + internal/observability/endpoint.go | 6 +++ internal/observability/endpoint_test.go | 1 + internal/testutil/otlp.go | 21 +++++--- tests/integration/otel_test.go | 71 +++++++++++++++++++++++++ 6 files changed, 99 insertions(+), 6 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index eb3ff7c8..2c9cf6a0 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -143,6 +143,7 @@ func validateOTelHeaders(s string) error { if s == "" { return nil } + seen := map[string]struct{}{} for _, seg := range strings.Split(s, ",") { seg = strings.TrimSpace(seg) if seg == "" { @@ -164,6 +165,10 @@ func validateOTelHeaders(s string) error { if strings.TrimSpace(seg[i+1:]) == "" { return fmt.Errorf("header key %q has empty or whitespace-only value", key) } + if _, exists := seen[key]; exists { + return fmt.Errorf("header key %q appears more than once", key) + } + seen[key] = struct{}{} } return nil } diff --git a/internal/config/header_parity_test.go b/internal/config/header_parity_test.go index 3dfadb90..63b9cc3e 100644 --- a/internal/config/header_parity_test.go +++ b/internal/config/header_parity_test.go @@ -38,6 +38,7 @@ func TestHeaderParsers_StayInSync(t *testing.T) { {name: "non-ascii key", in: "x-héader=v", wantErr: true}, {name: "mixed-case key", in: "Authorization=Bearer x", wantErr: false}, {name: "mixed valid and invalid", in: "a=1,broken", wantErr: true}, + {name: "duplicate key", in: "authorization=t1,authorization=t2", wantErr: true}, } for _, tc := range cases { diff --git a/internal/observability/endpoint.go b/internal/observability/endpoint.go index 351c7673..0a2b524d 100644 --- a/internal/observability/endpoint.go +++ b/internal/observability/endpoint.go @@ -77,6 +77,12 @@ func ParseOTelHeaders(s string) (map[string]string, error) { if val == "" { return nil, fmt.Errorf("header key %q has empty or whitespace-only value", key) } + // Reject duplicates rather than silently letting the last entry win + // — in an auth-sensitive context, two `authorization=…` segments + // would ship the wrong token with no boot-time indication. + if _, exists := out[key]; exists { + return nil, fmt.Errorf("header key %q appears more than once", key) + } out[key] = val } return out, nil diff --git a/internal/observability/endpoint_test.go b/internal/observability/endpoint_test.go index f3aac798..373cafe1 100644 --- a/internal/observability/endpoint_test.go +++ b/internal/observability/endpoint_test.go @@ -57,6 +57,7 @@ func TestParseOTelHeaders(t *testing.T) { {name: "colon in key rejected", in: "x:y=v", wantErr: true}, {name: "non-ascii key rejected", in: "x-héader=v", wantErr: true}, {name: "mixed-case key accepted", in: "Authorization=Bearer x", want: map[string]string{"Authorization": "Bearer x"}}, + {name: "duplicate key rejected", in: "authorization=t1,authorization=t2", wantErr: true}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/testutil/otlp.go b/internal/testutil/otlp.go index 11ce1e88..59b24596 100644 --- a/internal/testutil/otlp.go +++ b/internal/testutil/otlp.go @@ -36,7 +36,8 @@ import ( type FakeOTLP struct { addr string server *grpc.Server - tlsConfig *tls.Config // non-nil only when constructed via NewFakeOTLPTLS + tlsConfig *tls.Config // non-nil only when constructed via NewFakeOTLPTLS + cert *x509.Certificate // ditto — the receiver's self-signed leaf cert mu sync.Mutex traces []*tracepb.ResourceSpans @@ -61,7 +62,7 @@ func NewFakeOTLP(t *testing.T) *FakeOTLP { func NewFakeOTLPTLS(t *testing.T) *FakeOTLP { t.Helper() - cert, clientCfg := ephemeralTLSPair(t) + cert, parsed, clientCfg := ephemeralTLSPair(t) // Pinned to TLS 1.3 on both sides to match the production floor in // observability.tlsConfigOrDefault — a regression below TLS 1.3 should // fail the handshake here rather than negotiate to 1.2 silently. @@ -70,12 +71,13 @@ func NewFakeOTLPTLS(t *testing.T) *FakeOTLP { MinVersion: tls.VersionTLS13, } - return newFakeOTLP(t, &fakeOTLPTLS{server: serverCfg, client: clientCfg}) + return newFakeOTLP(t, &fakeOTLPTLS{server: serverCfg, client: clientCfg, cert: parsed}) } type fakeOTLPTLS struct { server *tls.Config client *tls.Config + cert *x509.Certificate } func newFakeOTLP(t *testing.T, tlsCfg *fakeOTLPTLS) *FakeOTLP { @@ -92,6 +94,7 @@ func newFakeOTLP(t *testing.T, tlsCfg *fakeOTLPTLS) *FakeOTLP { if tlsCfg != nil { serverOpts = append(serverOpts, grpc.Creds(credentials.NewTLS(tlsCfg.server))) r.tlsConfig = tlsCfg.client + r.cert = tlsCfg.cert } r.server = grpc.NewServer(serverOpts...) @@ -111,8 +114,9 @@ func newFakeOTLP(t *testing.T, tlsCfg *fakeOTLPTLS) *FakeOTLP { } // ephemeralTLSPair mints a one-shot ECDSA self-signed cert valid for 127.0.0.1 -// and returns it along with a client tls.Config that trusts only this cert. -func ephemeralTLSPair(t *testing.T) (tls.Certificate, *tls.Config) { +// and returns it along with its parsed form and a client tls.Config that +// trusts only this cert. +func ephemeralTLSPair(t *testing.T) (tls.Certificate, *x509.Certificate, *tls.Config) { t.Helper() priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) @@ -147,7 +151,7 @@ func ephemeralTLSPair(t *testing.T) (tls.Certificate, *tls.Config) { } pool := x509.NewCertPool() pool.AddCert(parsed) - return cert, &tls.Config{ + return cert, parsed, &tls.Config{ RootCAs: pool, ServerName: "127.0.0.1", MinVersion: tls.VersionTLS13, @@ -163,6 +167,11 @@ func (r *FakeOTLP) Addr() string { return r.addr } // NewFakeOTLP. func (r *FakeOTLP) TLSConfig() *tls.Config { return r.tlsConfig } +// Cert returns the receiver's parsed self-signed leaf certificate. Returns +// nil for the plaintext variant. Use to build a merged trust pool across +// multiple TLS receivers in a single test (per-signal endpoint coverage). +func (r *FakeOTLP) Cert() *x509.Certificate { return r.cert } + // SpanCount returns the total number of spans received across all RPCs. // Spans are flattened across resource and scope groupings. func (r *FakeOTLP) SpanCount() int { diff --git a/tests/integration/otel_test.go b/tests/integration/otel_test.go index 062e51a9..b2c00657 100644 --- a/tests/integration/otel_test.go +++ b/tests/integration/otel_test.go @@ -9,6 +9,8 @@ package tests import ( "context" + "crypto/tls" + "crypto/x509" "io" "log/slog" "net/http" @@ -433,3 +435,72 @@ func TestOTel_PerSignalEndpoint_Override(t *testing.T) { assert.Zero(t, rLogs.SpanCount(), "LogsEndpoint should NOT receive spans (catches traces-to-logs wiring bug)") assert.Zero(t, rLogs.MetricCount(), "LogsEndpoint should NOT receive metrics (catches metrics-to-logs wiring bug)") } + +// TestOTel_TLSPath_PerSignalEndpoint pins the production Grafana-Cloud +// wiring: distinct https:// per-signal endpoints (traces / metrics / logs +// going to separate hosts) AND TLS on every leg. Each receiver mints its +// own self-signed cert; the test merges all three certs into one client +// trust pool because ProviderConfig holds a single tlsConfig that the SDK +// applies to every exporter. A regression where per-signal endpoint parsing +// silently drops the `https://` scheme would fall back to plaintext and the +// gRPC handshake against the TLS receivers would fail. +func TestOTel_TLSPath_PerSignalEndpoint(t *testing.T) { + guardOTelGlobals(t) + rTraces := testutil.NewFakeOTLPTLS(t) + rMetrics := testutil.NewFakeOTLPTLS(t) + rLogs := testutil.NewFakeOTLPTLS(t) + + pool := x509.NewCertPool() + for _, r := range []*testutil.FakeOTLP{rTraces, rMetrics, rLogs} { + pool.AddCert(r.Cert()) + } + clientCfg := &tls.Config{ + RootCAs: pool, + ServerName: "127.0.0.1", + MinVersion: tls.VersionTLS13, + } + + cfg := observability.ProviderConfig{ + // Endpoint stays plaintext intentionally — it must never receive + // traffic because every signal is overridden. If a future regression + // causes a per-signal override to be ignored, the exporter would + // dial the plaintext default and the test's TLS-only receivers + // would record nothing. + Endpoint: "127.0.0.1:1", // unreachable; deliberately not a fake + TracesEndpoint: "https://" + rTraces.Addr(), + MetricsEndpoint: "https://" + rMetrics.Addr(), + LogsEndpoint: "https://" + rLogs.Addr(), + TracesEnabled: true, + TracesSampleRate: 1.0, + MetricsEnabled: true, + LogsEnabled: true, + } + cfg.SetTLSConfigForTesting(clientCfg) + shutdown, _ := initAndShutdown(t, cfg) + + _, span := otel.Tracer("test").Start(context.Background(), "tls-split-op") + span.End() + + counter, err := otel.GetMeterProvider().Meter("test").Int64Counter("tls_split_counter") + require.NoError(t, err) + counter.Add(context.Background(), 1) + + lvl := &slog.LevelVar{} + lvl.Set(slog.LevelInfo) + logger := observability.NewLogger("wavehouse-test", lvl, true, 1.0) + logger.Info("tls-split-log") + + drainCtx, drainCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer drainCancel() + require.NoError(t, shutdown(drainCtx)) + + assert.Equal(t, 1, rTraces.SpanCount(), "TLS TracesEndpoint should receive the span") + assert.GreaterOrEqual(t, rMetrics.MetricCount(), 1, "TLS MetricsEndpoint should receive the metric") + assert.GreaterOrEqual(t, rLogs.LogCount(), 1, "TLS LogsEndpoint should receive the log") + assert.Zero(t, rTraces.MetricCount(), "TLS TracesEndpoint should NOT receive metrics") + assert.Zero(t, rTraces.LogCount(), "TLS TracesEndpoint should NOT receive logs") + assert.Zero(t, rMetrics.SpanCount(), "TLS MetricsEndpoint should NOT receive spans") + assert.Zero(t, rMetrics.LogCount(), "TLS MetricsEndpoint should NOT receive logs") + assert.Zero(t, rLogs.SpanCount(), "TLS LogsEndpoint should NOT receive spans") + assert.Zero(t, rLogs.MetricCount(), "TLS LogsEndpoint should NOT receive metrics") +}