diff --git a/.github/workflows/cicd.yml b/.github/workflows/cicd.yml index 2364f89..7c463f5 100644 --- a/.github/workflows/cicd.yml +++ b/.github/workflows/cicd.yml @@ -1,5 +1,8 @@ name: CI/CD Pipeline +permissions: + contents: read + on: push: tags: diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 79143df..8fba9ae 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1,5 +1,8 @@ name: Run Tests +permissions: + contents: read + on: pull_request: branches: diff --git a/docs/METRICS_RECOMMENDATIONS.md b/docs/METRICS_RECOMMENDATIONS.md index 2ce365b..c085e06 100644 --- a/docs/METRICS_RECOMMENDATIONS.md +++ b/docs/METRICS_RECOMMENDATIONS.md @@ -10,6 +10,10 @@ This document captures the current state of Newt metrics, prioritized fixes, and - Tunnel/Traffic: newt_tunnel_sessions, newt_tunnel_bytes_total, newt_tunnel_latency_seconds, newt_tunnel_reconnects_total - Connection lifecycle: newt_connection_attempts_total, newt_connection_errors_total - Operations: newt_config_reloads_total, newt_restart_count_total, newt_build_info + - Operations: newt_config_reloads_total, newt_restart_count_total, newt_config_apply_seconds, newt_cert_rotation_total + - Build metadata: newt_build_info + - Control plane: newt_websocket_connect_latency_seconds, newt_websocket_messages_total + - Proxy: newt_proxy_active_connections, newt_proxy_buffer_bytes, newt_proxy_async_backlog_bytes, newt_proxy_drops_total - Go runtime: GC, heap, goroutines via runtime instrumentation 2) Main issues addressed now @@ -27,6 +31,10 @@ This document captures the current state of Newt metrics, prioritized fixes, and - Some call sites still need initiator label on reconnect outcomes (client vs server). This is planned. - WebSocket and Proxy metrics (connect latency, messages, active connections, buffer/drops, async backlog) are planned additions. - Config apply duration and cert rotation counters are planned. + - Registration and config reload failures are not yet emitted; add failure code paths so result labels expose churn. + - Restart counter increments only when build metadata is provided; consider decoupling to count all boots. + - Metric helpers often use `context.Background()`. Where lightweight contexts exist (e.g., HTTP handlers), propagate them to ease future correlation. + - Tracing coverage is limited to admin HTTP and WebSocket connect spans; extend to blueprint fetches, proxy accept loops, and WireGuard updates when OTLP is enabled. 4) Roadmap (phased) @@ -40,6 +48,10 @@ This document captures the current state of Newt metrics, prioritized fixes, and - Proxy: newt_proxy_active_connections, newt_proxy_buffer_bytes, newt_proxy_drops_total, newt_proxy_async_backlog_bytes - Reconnect: add initiator label (client/server) - Config & PKI: newt_config_apply_seconds{phase,result}; newt_cert_rotation_total{result} + - WebSocket disconnect and keepalive failure counters + - Proxy connection lifecycle metrics (accept totals, duration histogram) + - Pangolin blueprint/config fetch latency and status metrics + - Certificate rotation duration histogram to complement success/failure counter 5) Operational guidance @@ -64,9 +76,3 @@ This document captures the current state of Newt metrics, prioritized fixes, and - Direct scrape variant requires no attribute promotion since site_id is already a metric label. - Transform/promote variant remains optional for environments that rely on resource-to-label promotion. - -8) Testing - -- curl :2112/metrics | grep ^newt_ -- Verify presence of site_id across series; region appears when set. -- Ensure disallowed attributes are filtered; allowed (site_id) retained. diff --git a/docs/observability.md b/docs/observability.md index 6f71ecb..ba19aac 100644 --- a/docs/observability.md +++ b/docs/observability.md @@ -34,18 +34,30 @@ Runtime behavior - When OTLP is enabled, metrics and traces are exported to OTLP gRPC endpoint - Go runtime metrics (goroutines, GC, memory) are exported automatically -Metric catalog (initial) +Metric catalog (current) -- newt_build_info (gauge) labels: version, commit, site_id[, region]; value is always 1 -- newt_site_registrations_total (counter) labels: result, site_id[, region] -- newt_site_online (observable gauge) labels: site_id (0/1) -- newt_site_last_heartbeat_seconds (observable gauge) labels: site_id -- newt_tunnel_sessions (observable gauge) labels: site_id, tunnel_id [transport optional when known] -- newt_tunnel_bytes_total (counter) labels: site_id, tunnel_id, protocol (tcp|udp), direction (ingress|egress) -- newt_tunnel_latency_seconds (histogram) labels: site_id, tunnel_id, transport (e.g., wireguard) -- newt_tunnel_reconnects_total (counter) labels: site_id, tunnel_id, initiator (client|server), reason -- newt_connection_attempts_total (counter) labels: site_id, transport, result -- newt_connection_errors_total (counter) labels: site_id, transport, error_type (dial_timeout|tls_handshake|auth_failed|io_error) +| Metric | Instrument | Key attributes | Purpose | Example | +| --- | --- | --- | --- | --- | +| `newt_build_info` | Observable gauge (Int64) | `version`, `commit`, `site_id`, `region` (optional) | Emits build metadata with value `1` for scrape-time verification. | `newt_build_info{version="1.5.0",site_id="acme-edge-1"} 1` | +| `newt_site_registrations_total` | Counter (Int64) | `result` (`success`/`failure`), `site_id`, `region` (optional) | Counts Pangolin registration attempts. | `newt_site_registrations_total{result="success",site_id="acme-edge-1"} 1` | +| `newt_site_online` | Observable gauge (Int64) | `site_id` | Reports whether the site is currently connected (`1`) or offline (`0`). | `newt_site_online{site_id="acme-edge-1"} 1` | +| `newt_site_last_heartbeat_seconds` | Observable gauge (Float64) | `site_id` | Time since the most recent Pangolin heartbeat. | `newt_site_last_heartbeat_seconds{site_id="acme-edge-1"} 2.4` | +| `newt_tunnel_sessions` | Observable gauge (Int64) | `site_id`, `tunnel_id` (when enabled) | Counts active tunnel sessions per peer; collapses to per-site when tunnel IDs are disabled. | `newt_tunnel_sessions{site_id="acme-edge-1",tunnel_id="wgpub..."} 3` | +| `newt_tunnel_bytes_total` | Counter (Int64) | `direction` (`ingress`/`egress`), `protocol` (`tcp`/`udp`), `tunnel_id` (optional), `site_id`, `region` (optional) | Measures proxied traffic volume across tunnels. | `newt_tunnel_bytes_total{direction="ingress",protocol="tcp",site_id="acme-edge-1"} 4096` | +| `newt_tunnel_latency_seconds` | Histogram (Float64) | `transport` (e.g., `wireguard`), `tunnel_id` (optional), `site_id`, `region` (optional) | Captures RTT or configuration-driven latency samples. | `newt_tunnel_latency_seconds_bucket{transport="wireguard",le="0.5"} 42` | +| `newt_tunnel_reconnects_total` | Counter (Int64) | `initiator` (`client`/`server`), `reason` (enumerated), `tunnel_id` (optional), `site_id`, `region` (optional) | Tracks reconnect causes for troubleshooting flaps. | `newt_tunnel_reconnects_total{initiator="client",reason="timeout",site_id="acme-edge-1"} 5` | +| `newt_connection_attempts_total` | Counter (Int64) | `transport` (`auth`/`websocket`), `result`, `site_id`, `region` (optional) | Measures control-plane dial attempts and their outcomes. | `newt_connection_attempts_total{transport="websocket",result="success",site_id="acme-edge-1"} 8` | +| `newt_connection_errors_total` | Counter (Int64) | `transport`, `error_type`, `site_id`, `region` (optional) | Buckets connection failures by normalized error class. | `newt_connection_errors_total{transport="websocket",error_type="tls_handshake",site_id="acme-edge-1"} 1` | +| `newt_config_reloads_total` | Counter (Int64) | `result`, `site_id`, `region` (optional) | Counts remote blueprint/config reloads. | `newt_config_reloads_total{result="success",site_id="acme-edge-1"} 3` | +| `newt_restart_count_total` | Counter (Int64) | `site_id`, `region` (optional) | Increments once per process boot to detect restarts. | `newt_restart_count_total{site_id="acme-edge-1"} 1` | +| `newt_config_apply_seconds` | Histogram (Float64) | `phase` (`interface`/`peer`), `result`, `site_id`, `region` (optional) | Measures time spent applying WireGuard configuration phases. | `newt_config_apply_seconds_sum{phase="peer",result="success",site_id="acme-edge-1"} 0.48` | +| `newt_cert_rotation_total` | Counter (Int64) | `result`, `site_id`, `region` (optional) | Tracks client certificate rotation attempts. | `newt_cert_rotation_total{result="success",site_id="acme-edge-1"} 2` | +| `newt_websocket_connect_latency_seconds` | Histogram (Float64) | `transport="websocket"`, `result`, `error_type` (on failure), `site_id`, `region` (optional) | Measures WebSocket dial latency and exposes failure buckets. | `newt_websocket_connect_latency_seconds_bucket{result="success",le="0.5",site_id="acme-edge-1"} 9` | +| `newt_websocket_messages_total` | Counter (Int64) | `direction` (`in`/`out`), `msg_type` (`text`/`ping`/`pong`), `site_id`, `region` (optional) | Accounts for control WebSocket traffic volume by type. | `newt_websocket_messages_total{direction="out",msg_type="ping",site_id="acme-edge-1"} 12` | +| `newt_proxy_active_connections` | Observable gauge (Int64) | `protocol` (`tcp`/`udp`), `direction` (`ingress`/`egress`), `tunnel_id` (optional), `site_id`, `region` (optional) | Current proxy connections per tunnel and protocol. | `newt_proxy_active_connections{protocol="tcp",direction="egress",site_id="acme-edge-1"} 4` | +| `newt_proxy_buffer_bytes` | Observable gauge (Int64) | `protocol`, `direction`, `tunnel_id` (optional), `site_id`, `region` (optional) | Volume of buffered data awaiting flush in proxy queues. | `newt_proxy_buffer_bytes{protocol="udp",direction="egress",site_id="acme-edge-1"} 2048` | +| `newt_proxy_async_backlog_bytes` | Observable gauge (Int64) | `protocol`, `direction`, `tunnel_id` (optional), `site_id`, `region` (optional) | Tracks async write backlog when deferred flushing is enabled. | `newt_proxy_async_backlog_bytes{protocol="tcp",direction="egress",site_id="acme-edge-1"} 512` | +| `newt_proxy_drops_total` | Counter (Int64) | `protocol`, `tunnel_id` (optional), `site_id`, `region` (optional) | Counts proxy drop events caused by downstream write errors. | `newt_proxy_drops_total{protocol="udp",site_id="acme-edge-1"} 1` | Conventions diff --git a/docs/otel-review.md b/docs/otel-review.md index 14cfb53..ba824e7 100644 --- a/docs/otel-review.md +++ b/docs/otel-review.md @@ -1,64 +1,126 @@ -# OpenTelemetry Review +# Newt OpenTelemetry Review -## Metric inventory -The table below lists every instrument registered by `internal/telemetry/metrics.go`, the helper that emits it, and an example time-series. Attribute sets automatically add `site_id` (and optionally `region`) via `attrsWithSite` unless the observable callback overrides them. 【F:internal/telemetry/metrics.go†L23-L205】【F:internal/telemetry/metrics.go†L289-L403】 +## Overview -| Metric | Instrument & unit | Purpose | Emission path | Example series | -| --- | --- | --- | --- | --- | -| `newt_site_registrations_total` | Counter | Counts Pangolin registration attempts keyed by result (`success`, `failure`). | `telemetry.IncSiteRegistration` (called after registration completes). | `newt_site_registrations_total{result="success",site_id="abc"} 1` | -| `newt_site_online` | Observable gauge | 0/1 heartbeat for the active site, driven by the registered `StateView`. | `telemetry.SetObservableCallback` via `state.TelemetryView`. | `newt_site_online{site_id="self"} 1` | -| `newt_site_last_heartbeat_seconds` | Observable gauge | Seconds since the last Pangolin heartbeat. | Same callback as above using `state.TelemetryView.TouchHeartbeat`. | `newt_site_last_heartbeat_seconds{site_id="self"} 3.2` | -| `newt_tunnel_sessions` | Observable gauge | Active sessions per tunnel; collapses to site total when `tunnel_id` emission is disabled. | `state.TelemetryView.SessionsByTunnel` via `RegisterStateView`. | `newt_tunnel_sessions{site_id="self",tunnel_id="wgpub"} 2` | -| `newt_tunnel_bytes_total` | Counter (`By`) | Traffic accounting per tunnel, direction (`ingress`/`egress`), protocol (`tcp`/`udp`). | Proxy manager counting writers (`AddTunnelBytes`/`AddTunnelBytesSet`). | `newt_tunnel_bytes_total{direction="egress",protocol="tcp",site_id="self",tunnel_id="wgpub"} 8192` | -| `newt_tunnel_latency_seconds` | Histogram (`s`) | RTT samples from WireGuard stack and health pings per tunnel/transport. | `telemetry.ObserveTunnelLatency` from tunnel health checks. | `newt_tunnel_latency_seconds_bucket{transport="wireguard",le="0.05",tunnel_id="wgpub"} 4` | -| `newt_tunnel_reconnects_total` | Counter | Reconnect attempts bucketed by initiator (`client`/`server`) and reason enums. | `telemetry.IncReconnect` across websocket, WG, and utility flows. | `newt_tunnel_reconnects_total{initiator="client",reason="timeout",tunnel_id="wgpub"} 3` | -| `newt_connection_attempts_total` | Counter | Auth and WebSocket attempt counts by transport (`auth`, `websocket`) and result (`success`/`failure`). | `telemetry.IncConnAttempt` in auth/token and dial paths. | `newt_connection_attempts_total{transport="websocket",result="failure",site_id="self"} 2` | -| `newt_connection_errors_total` | Counter | Connection error tally keyed by transport and canonical error type (`dial_timeout`, `tls_handshake`, `auth_failed`, `io_error`). | `telemetry.IncConnError` in auth/websocket flows. | `newt_connection_errors_total{transport="auth",error_type="auth_failed",site_id="self"} 1` | -| `newt_config_reloads_total` | Counter | Successful/failed config reload attempts. | `telemetry.IncConfigReload` during WireGuard config reloads. | `newt_config_reloads_total{result="success",site_id="self"} 1` | -| `newt_restart_count_total` | Counter | Bumps to 1 at process boot for build info scrapers. | `telemetry.RegisterBuildInfo` called from `Init`. | `newt_restart_count_total{site_id="self"} 1` | -| `newt_config_apply_seconds` | Histogram (`s`) | Measures interface/peer apply duration per phase and result. | `telemetry.ObserveConfigApply` around config updates. | `newt_config_apply_seconds_bucket{phase="peer",result="success",le="0.1"} 5` | -| `newt_cert_rotation_total` | Counter | Certificate rotation events tagged by result. | `telemetry.IncCertRotation` during PKI updates. | `newt_cert_rotation_total{result="success",site_id="self"} 1` | -| `newt_build_info` | Observable gauge | Constant 1 with `version`/`commit` attributes to expose build metadata. | Callback registered in `registerBuildWSProxyInstruments`. | `newt_build_info{version="1.2.3",commit="abc123",site_id="self"} 1` | -| `newt_websocket_connect_latency_seconds` | Histogram (`s`) | Dial latency for Pangolin WebSocket connects annotated with result/error_type. | `telemetry.ObserveWSConnectLatency` inside `Client.establishConnection`. | `newt_websocket_connect_latency_seconds_bucket{result="success",transport="websocket",le="0.5"} 1` | -| `newt_websocket_messages_total` | Counter | Counts inbound/outbound WebSocket messages by direction and logical message type. | `telemetry.IncWSMessage` for ping/pong/text events. | `newt_websocket_messages_total{direction="out",msg_type="ping",site_id="self"} 4` | -| `newt_websocket_disconnects_total` | Counter | Tracks WebSocket disconnects grouped by `reason` (`shutdown`, `unexpected_close`, etc.) and `result`. | Emitted from `Client.readPumpWithDisconnectDetection` defer block. | `newt_websocket_disconnects_total{reason="unexpected_close",result="error",site_id="self"} 1` | -| `newt_websocket_keepalive_failures_total` | Counter | Failed WebSocket ping/pong keepalive attempts by reason. | Incremented in `Client.pingMonitor` when `WriteControl` fails. | `newt_websocket_keepalive_failures_total{reason="ping_write",site_id="self"} 1` | -| `newt_websocket_session_duration_seconds` | Histogram (`s`) | Duration of WebSocket sessions by outcome (`result`). | Observed when the read pump exits. | `newt_websocket_session_duration_seconds_sum{result="success",site_id="self"} 120` | -| `newt_proxy_active_connections` | Observable gauge | Active TCP/UDP proxy connections per tunnel and protocol. | Proxy manager callback via `SetProxyObservableCallback`. | `newt_proxy_active_connections{protocol="tcp",tunnel_id="wgpub"} 3` | -| `newt_proxy_buffer_bytes` | Observable gauge (`By`) | Size of proxy buffer pools (synchronous path) per tunnel/protocol. | Same proxy callback as above. | `newt_proxy_buffer_bytes{protocol="tcp",tunnel_id="wgpub"} 10240` | -| `newt_proxy_async_backlog_bytes` | Observable gauge (`By`) | Unflushed async byte backlog when deferred accounting is enabled. | Proxy callback when async accounting is turned on. | `newt_proxy_async_backlog_bytes{protocol="udp",tunnel_id="wgpub"} 4096` | -| `newt_proxy_drops_total` | Counter | Proxy write-drop events per protocol/tunnel. | `telemetry.IncProxyDrops` on UDP drop paths. | `newt_proxy_drops_total{protocol="udp",tunnel_id="wgpub"} 2` | -| `newt_proxy_accept_total` | Counter | Proxy accept attempts labelled by protocol, result, and reason. | `telemetry.IncProxyAccept` in TCP accept loop and UDP dial paths. | `newt_proxy_accept_total{protocol="tcp",result="failure",reason="timeout",site_id="self"} 1` | -| `newt_proxy_connection_duration_seconds` | Histogram (`s`) | Lifecycle duration for proxied TCP/UDP connections by result. | `telemetry.ObserveProxyConnectionDuration` when TCP/UDP handlers complete. | `newt_proxy_connection_duration_seconds_sum{protocol="udp",result="success",site_id="self"} 30` | +This document summarises the current OpenTelemetry (OTel) instrumentation in Newt, assesses +compliance with OTel guidelines, and lists concrete improvements to pursue before release. +It is based on the implementation in `internal/telemetry` and the call-sites that emit +metrics and traces across the code base. -In addition, Go runtime metrics are automatically exported when telemetry is initialised. 【F:internal/telemetry/telemetry.go†L147-L155】 +## Current metric instrumentation -## Tracing footprint -* Tracing is enabled only when OTLP export is turned on; `telemetry.Init` wires a batch `TracerProvider` and sets it globally. 【F:internal/telemetry/telemetry.go†L135-L155】 -* The admin HTTP mux (`/metrics`, `/healthz`) is wrapped with `otelhttp.NewHandler`, so any inbound admin requests produce spans. 【F:main.go†L373-L387】 -* WebSocket dials create a `ws.connect` span around the outbound handshake, but subsequent control-plane HTTP requests (token fetch, blueprint sync) use plain `http.Client` without propagation. 【F:websocket/client.go†L417-L459】 +All instruments are registered in `internal/telemetry/metrics.go`. They are grouped +into site, tunnel, connection, configuration, build, WebSocket, and proxy domains. +A global attribute filter (see `buildMeterProvider`) constrains exposed label keys to +`site_id`, `region`, and a curated list of low-cardinality dimensions so that Prometheus +exports stay bounded. -Overall span coverage is limited to the WebSocket connect loop and admin server; tunnel setup, Docker discovery, config application, and health pings currently emit only metrics. +- **Site lifecycle**: `newt_site_registrations_total`, `newt_site_online`, and + `newt_site_last_heartbeat_seconds` capture registration attempts and liveness. They + are fed either manually (`IncSiteRegistration`) or via the `TelemetryView` state + callback that publishes observable gauges for the active site. +- **Tunnel health and usage**: Counters and histograms track bytes, latency, reconnects, + and active sessions per tunnel (`newt_tunnel_*` family). Attribute helpers respect + the `NEWT_METRICS_INCLUDE_TUNNEL_ID` toggle to keep cardinality manageable on larger + fleets. +- **Connection attempts**: `newt_connection_attempts_total` and + `newt_connection_errors_total` are emitted throughout the WebSocket client to classify + authentication, dial, and transport failures. +- **Operations/configuration**: `newt_config_reloads_total`, + `newt_restart_count_total`, `newt_config_apply_seconds`, and + `newt_cert_rotation_total` provide visibility into blueprint reloads, process boots, + configuration timings, and certificate rotation outcomes. +- **Build metadata**: `newt_build_info` records the binary version/commit together + with a monotonic restart counter when build information is supplied at startup. +- **WebSocket control-plane**: `newt_websocket_connect_latency_seconds` and + `newt_websocket_messages_total` report connect latency and ping/pong/text activity. +- **Proxy data-plane**: Observable gauges (`newt_proxy_active_connections`, + `newt_proxy_buffer_bytes`, `newt_proxy_async_backlog_bytes`) and the + `newt_proxy_drops_total` counter are fed from the proxy manager to monitor backlog + and drop behaviour alongside per-protocol byte counters. -## Guideline & best-practice adherence -* **Resource & exporter configuration:** `telemetry.FromEnv` honours OTEL env-vars, sets service name/version, and promotes `site_id`/`region` resource attributes before building the provider. Exporters default to Prometheus with optional OTLP, aligning with OTel deployment guidance. 【F:internal/telemetry/telemetry.go†L56-L206】 -* **Low-cardinality enforcement:** A view-level attribute allow-list retains only approved keys (`tunnel_id`, `transport`, `protocol`, etc.), protecting Prometheus cardinality while still surfacing `site_id`/`region`. 【F:internal/telemetry/telemetry.go†L209-L231】 -* **Units and naming:** Instrument helpers enforce `_total` suffixes for counters, `_seconds` for durations, and attach `metric.WithUnit("By"|"s")` for size/time metrics, matching OTel semantic conventions. 【F:internal/telemetry/metrics.go†L23-L192】 -* **Runtime metrics & shutdown:** The runtime instrumentation is enabled, and `Setup.Shutdown` drains exporters in reverse order to avoid data loss. 【F:internal/telemetry/telemetry.go†L147-L261】 -* **Site-aware observables:** `state.TelemetryView` provides thread-safe snapshots to feed `newt_site_online`/`_last_heartbeat_seconds`/`_tunnel_sessions`, ensuring gauges report cohesive per-site data even when `tunnel_id` labels are disabled. 【F:internal/state/telemetry_view.go†L11-L79】 +Refer to `docs/observability.md` for a tabular catalogue with instrument types, +attributes, and sample exposition lines. -## Gaps & recommended improvements -1. **Tracing coverage:** Instrument the Pangolin REST calls (`getToken`, blueprint downloads) with `otelhttp.NewTransport` or explicit spans, and consider spans for WireGuard handshake/config apply to enable end-to-end traces when OTLP is on. 【F:websocket/client.go†L240-L360】 -2. **Histogram coverage:** Introduce `newt_site_registration_latency_seconds` (bootstrap) and `newt_ping_roundtrip_seconds` (heartbeat) to capture SLO-critical latencies before release. Existing latency buckets (`0.005s` → `30s`) can be reused. 【F:internal/telemetry/telemetry.go†L209-L218】 -3. **Control-plane throughput:** Add `newt_websocket_payload_bytes_total` (direction/msg_type) or reuse the tunnel counter with a `transport="websocket"` label to quantify command traffic volume and detect back-pressure. -4. **Docker discovery metrics:** If Docker auto-discovery is enabled, expose counters for container add/remove events and failures so operators can trace missing backends to discovery issues. +## Tracing coverage -## Pre-release metric backlog -Prior to GA, we recommend landing the following high-value instruments: -* **Bootstrap latency:** `newt_site_registration_latency_seconds` histogram emitted around the initial Pangolin registration HTTP call to detect slow control-plane responses. -* **Session duration:** `newt_websocket_session_duration_seconds` histogram recorded when a WebSocket closes (result + reason) to quantify stability. -* **Heartbeat lag:** `newt_ping_roundtrip_seconds` histogram from ping/pong monitors to capture tunnel health, complementing the heartbeat gauge. -* **Proxy accept errors:** `newt_proxy_accept_errors_total` counter keyed by protocol/reason to surface listener pressure distinct from data-plane drops. -* **Discovery events:** `newt_discovery_events_total` counter with `action` (`add`, `remove`, `error`) and `source` (`docker`, `file`) to audit service inventory churn. +Tracing is optional and enabled only when OTLP export is configured. When active: -Implementing the above will round out visibility into control-plane responsiveness, connection stability, and discovery health while preserving the existing low-cardinality discipline. +- The admin HTTP mux is wrapped with `otelhttp.NewHandler`, producing spans for + `/metrics` and `/healthz` requests. +- The WebSocket dial path creates a `ws.connect` span around the gRPC-based handshake. + +No other subsystems currently create spans, so data-plane operations, blueprint fetches, +Docker discovery, and WireGuard reconfiguration happen without trace context. + +## Guideline & best-practice alignment + +The implementation adheres to most OTel Go recommendations: + +- **Naming & units** – Every instrument follows the `newt_*` prefix with `_total` + suffixes for counters and `_seconds`/`_bytes` unit conventions. Histograms are + registered with explicit second-based buckets. +- **Resource attributes** – Service name/version and optional `site_id`/`region` + populate the `resource.Resource` and are also injected as metric attributes for + compatibility with Prometheus queries. +- **Attribute hygiene** – A single attribute filter (`sdkmetric.WithView`) enforces + the allow-list of label keys to prevent accidental high-cardinality emission. +- **Runtime metrics** – Go runtime instrumentation is enabled automatically through + `runtime.Start`. +- **Configuration via environment** – `telemetry.FromEnv` honours `OTEL_*` variables + alongside `NEWT_*` overrides so operators can configure exporters without code + changes. +- **Shutdown handling** – `Setup.Shutdown` iterates exporters in reverse order to + flush buffers before process exit. + +## Adjustments & improvements + +The review identified a few actionable adjustments: + +1. **Record registration failures** – `newt_site_registrations_total` is currently + incremented only on success. Emit `result="failure"` samples whenever Pangolin + rejects a registration or credential exchange so operators can alert on churn. +2. **Surface config reload failures** – `telemetry.IncConfigReload` is invoked with + `result="success"` only. Callers should record a failure result when blueprint + parsing or application aborts before success counters are incremented. +3. **Harmonise restart count behaviour** – `newt_restart_count_total` increments only + when build metadata is provided. Consider moving the increment out of + `RegisterBuildInfo` so the counter advances even for ad-hoc builds without version + strings. +4. **Propagate contexts where available** – Many emitters call metric helpers with + `context.Background()`. Passing real contexts (when inexpensive) would allow future + exporters to correlate spans and metrics. +5. **Extend tracing coverage** – Instrument critical flows such as blueprint fetches, + WireGuard reconfiguration, proxy accept loops, and Docker discovery to provide end + to end visibility when OTLP tracing is enabled. + +## Metrics to add before release + +Prioritised additions that would close visibility gaps: + +1. **WebSocket disconnect outcomes** – A counter (e.g., `newt_websocket_disconnects_total`) + partitioned by `reason` would complement the existing connect latency histogram and + explain reconnect storms. +2. **Keepalive/heartbeat failures** – Counting ping timeouts or heartbeat misses would + make `newt_site_last_heartbeat_seconds` actionable by providing discrete events. +3. **Proxy connection lifecycle** – Add counters/histograms for proxy accept events and + connection durations to correlate drops with load and backlog metrics. +4. **Blueprint/config pull latency** – Measuring Pangolin blueprint fetch durations and + HTTP status distribution would expose slow control-plane operations. +5. **Certificate rotation attempts** – Complement `newt_cert_rotation_total` with a + duration histogram to observe slow PKI updates and detect stuck rotations. + +These metrics rely on data that is already available in the code paths mentioned +above and would round out operational dashboards. + +## Tracing wishlist + +To benefit from tracing when OTLP is active, add spans around: + +- Pangolin REST calls (wrap the HTTP client with `otelhttp.NewTransport`). +- Docker discovery cycles and target registration callbacks. +- WireGuard reconfiguration (interface bring-up, peer updates). +- Proxy dial/accept loops for both TCP and UDP targets. + +Capturing these stages will let operators correlate latency spikes with reconnects +and proxy drops using distributed traces in addition to the metric signals.