diff --git a/README.md b/README.md index d4e2fc0..fc9e2b9 100644 --- a/README.md +++ b/README.md @@ -56,8 +56,7 @@ anwesen merge --vault /path/to/vault --query 'tags=adr&__anw-order=title' > ADRs ``` anwesen serve --vault [--bind ] [--log-level ] - [--uptrace-dsn | --otlp-endpoint ] - [--otlp-header ]... [--otlp-slow-request-ms ] + [--otlp-slow-request-ms ] anwesen doctor --vault anwesen merge --vault --query anwesen version @@ -69,24 +68,55 @@ anwesen version | `--bind ` | `ANWESEN_BIND` | `127.0.0.1:8080` | Listen address for `serve`. | | `--log-level ` | `ANWESEN_LOG_LEVEL` | `info` | `error`, `warn`, `info`, `debug`, or `trace`. | | `--query ` | -- | _required for `merge`_ | A `/query` query string: frontmatter predicates plus `__anw-` controls. | -| `--uptrace-dsn ` | `ANWESEN_UPTRACE_DSN` | unset | uptrace DSN (`https://@api.uptrace.dev`). Excludes `--otlp-endpoint`. | -| `--otlp-endpoint ` | `ANWESEN_OTLP_ENDPOINT` | unset | OTLP/HTTP base URL. Excludes `--uptrace-dsn`. | -| `--otlp-header ` | `ANWESEN_OTLP_HEADERS` | none | Extra export header, repeatable. The env var takes a comma-separated list. | | `--otlp-slow-request-ms` | `ANWESEN_OTLP_SLOW_REQUEST_MS` | `500` | Requests at or over this duration, or answering 5xx, also export a span. | -Every flag has a matching `ANWESEN_` environment variable, except -`--otlp-header`, whose env var is the plural `ANWESEN_OTLP_HEADERS` because it -takes a list. CLI flags win over env vars. +Every flag has a matching `ANWESEN_` environment variable. CLI flags win +over env vars. -Telemetry is off unless `--uptrace-dsn` or `--otlp-endpoint` is set. With -neither, nothing is exported and no exporter is built. The four telemetry -flags apply to `serve` only. +`--bind` and `--otlp-slow-request-ms` apply to `serve` only. Where telemetry is +exported is configured entirely through the standard `OTEL_` variables below. - **`serve`** -- run the daemon: walk the vault, build the index, watch for changes, serve the API. - **`doctor`** -- walk the vault once and report what would stop clean ingestion: unreadable files, unparseable YAML, path collisions on the HTTP surface, and frontmatter type drift (the same key carrying incompatible types across notes). Read-only; non-zero exit if any issue is found. - **`merge`** -- one-shot local generation: walk the vault, evaluate `--query`, and write the merged markdown document to stdout. No server, no HTTP. See [Local generation](#local-generation). - **`version`** -- print version and exit. +## Telemetry + +anwesen exports OTLP metrics for every request, and a span for requests at or +over `--otlp-slow-request-ms` or answering a 5xx. Export is configured through +the standard OpenTelemetry environment variables, which the SDK reads directly: + +| Variable | Meaning | +| -------------------------------------- | ---------------------------------------------------------------- | +| `OTEL_EXPORTER_OTLP_ENDPOINT` | Base URL. The SDK appends `/v1/metrics` and `/v1/traces`. | +| `OTEL_EXPORTER_OTLP_METRICS_ENDPOINT` | Full metrics URL, used as given. Overrides the base for metrics. | +| `OTEL_EXPORTER_OTLP_TRACES_ENDPOINT` | Full traces URL, used as given. Overrides the base for traces. | +| `OTEL_EXPORTER_OTLP_HEADERS` | Export headers, `key=value` comma-separated. | +| `OTEL_EXPORTER_OTLP_PROTOCOL` | `http/protobuf` (default) or `http/json`. anwesen exports over HTTP. | +| `OTEL_SERVICE_NAME`, `OTEL_RESOURCE_ATTRIBUTES` | Override the `anwesen` service identity. | + +With none of the three endpoint variables set, telemetry is off: no exporter is +built and the request middleware is not installed. + +The base URL must be a base URL. A query or a fragment on +`OTEL_EXPORTER_OTLP_ENDPOINT` makes anwesen fail at startup, because the SDK +appends the signal path after it and the export would go nowhere in silence. + +uptrace: + +``` +OTEL_EXPORTER_OTLP_ENDPOINT=https://api.uptrace.dev +OTEL_EXPORTER_OTLP_HEADERS=uptrace-dsn=https://TOKEN@api.uptrace.dev?grpc=4317 +``` + +Paste the DSN from Project Settings into the header verbatim, tail and all -- +it is a credential there, not an address. The endpoint is the host alone. + +Removed in 0.3.0: `--uptrace-dsn`, `--otlp-endpoint`, `--otlp-header` and their +`ANWESEN_` variables. Passing any of them fails at startup with the `OTEL_` +replacement to use. + ## HTTP API All endpoints are `GET` and return JSON unless noted. diff --git a/src/cli.rs b/src/cli.rs index 155ab5b..52e65e4 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -4,18 +4,22 @@ //! //! ```text //! anwesen serve --vault [--bind ] [--log-level ] -//! [--uptrace-dsn | --otlp-endpoint ] -//! [--otlp-header ]... [--otlp-slow-request-ms ] +//! [--otlp-slow-request-ms ] //! anwesen doctor --vault [--log-level ] //! anwesen merge --vault [--query ] [--log-level ] //! anwesen version //! ``` //! -//! `--bind` and the OTLP telemetry flags are `serve`-only ([ANW-37]); +//! `--bind` and `--otlp-slow-request-ms` are `serve`-only ([ANW-37]); //! `doctor` and `merge` do not bind a port; `version` takes no flags. Each //! flag has a matching `ANWESEN_` environment variable and CLI wins -//! over env per the manual. With no `--otlp-endpoint`/`--uptrace-dsn`, -//! telemetry is off and the server behaves exactly as without these flags. +//! over env per the manual. +//! +//! Telemetry export is configured through the standard `OTEL_EXPORTER_OTLP_*` +//! environment variables ([ANW-42](https://crvrs.youtrack.cloud/issue/ANW-42)). +//! With none of them set, telemetry is off and the server behaves as it did +//! before telemetry existed. The removed flags below are still parsed so an +//! upgrade fails loudly instead of dropping export config in silence. use std::net::SocketAddr; use std::path::PathBuf; @@ -56,25 +60,22 @@ pub struct ServeArgs { #[arg(long, env = "ANWESEN_LOG_LEVEL", default_value = "info")] pub log_level: LogLevel, - /// uptrace DSN shorthand (`https://@api.uptrace.dev`), parsed - /// into the OTLP endpoint plus an `uptrace-dsn` header. Mutually - /// exclusive with --otlp-endpoint. When this and --otlp-endpoint are - /// both unset, telemetry is fully off (ANW-37). - #[arg(long, env = "ANWESEN_UPTRACE_DSN")] + /// Removed (ANW-42): use `OTEL_EXPORTER_OTLP_ENDPOINT` plus + /// `OTEL_EXPORTER_OTLP_HEADERS=uptrace-dsn=`. Still accepted so + /// startup fails with that message rather than exporting nowhere. + #[arg(long, env = "ANWESEN_UPTRACE_DSN", hide = true)] pub uptrace_dsn: Option, - /// Generic OTLP/HTTP endpoint base URL for telemetry export. The - /// per-signal path (`/v1/metrics`, `/v1/traces`) is appended by the - /// exporter. Mutually exclusive with --uptrace-dsn. - #[arg(long, env = "ANWESEN_OTLP_ENDPOINT")] + /// Removed (ANW-42): use `OTEL_EXPORTER_OTLP_ENDPOINT`. + #[arg(long, env = "ANWESEN_OTLP_ENDPOINT", hide = true)] pub otlp_endpoint: Option, - /// Extra OTLP export header as `key=value`, repeatable. On the env var - /// (`ANWESEN_OTLP_HEADERS`) pass a comma-separated `key=value` list. + /// Removed (ANW-42): use `OTEL_EXPORTER_OTLP_HEADERS`. #[arg( long = "otlp-header", env = "ANWESEN_OTLP_HEADERS", - value_delimiter = ',' + value_delimiter = ',', + hide = true )] pub otlp_headers: Vec, @@ -172,6 +173,34 @@ mod tests { } } + /// The removed telemetry flags still parse, hidden from `--help`. Clap + /// rejecting them as unknown would say nothing about the `OTEL_` + /// replacement; the migration error in `telemetry::TelemetryConfig` + /// needs the values to reach it (ANW-42). + #[test] + fn serve_still_parses_the_removed_telemetry_flags() { + let cli = parse(&[ + "serve", + "--vault", + "/tmp/v", + "--uptrace-dsn", + "https://tok@api.uptrace.dev", + "--otlp-endpoint", + "https://collector.example.com", + "--otlp-header", + "authorization=Bearer xyz", + ]) + .expect("parse"); + match cli.command { + Command::Serve(a) => { + assert!(a.uptrace_dsn.is_some()); + assert!(a.otlp_endpoint.is_some()); + assert_eq!(a.otlp_headers, vec!["authorization=Bearer xyz".to_string()]); + } + _ => panic!("expected serve"), + } + } + #[test] fn serve_rejects_malformed_bind_at_parse_time() { let err = parse(&["serve", "--vault", "/tmp/v", "--bind", "not-an-addr"]).unwrap_err(); diff --git a/src/http.rs b/src/http.rs index 48ec78e..8b02e32 100644 --- a/src/http.rs +++ b/src/http.rs @@ -819,18 +819,23 @@ mod tests { } /// With telemetry installed, a normal request is answered byte-for-byte - /// as without it. The exporter points at an unreachable local port, so - /// export fails instantly in the background and never touches the - /// response path. + /// as without it. The test process sets no `OTEL_EXPORTER_OTLP_*` + /// variables, so the exporter aims at the SDK default and fails in the + /// background without ever touching the response path. #[tokio::test] async fn telemetry_layer_does_not_alter_responses() { - use crate::telemetry::{self, RawTelemetryArgs, TelemetryConfig}; + use crate::telemetry::{self, OtelEndpointEnv, RawTelemetryArgs, TelemetryConfig}; - let cfg = TelemetryConfig::resolve(RawTelemetryArgs { - otlp_endpoint: Some("http://127.0.0.1:9".into()), - slow_request_ms: 500, - ..Default::default() - }) + let cfg = TelemetryConfig::resolve( + &RawTelemetryArgs { + slow_request_ms: 500, + ..Default::default() + }, + OtelEndpointEnv { + generic: Some("http://127.0.0.1:9".into()), + ..OtelEndpointEnv::default() + }, + ) .unwrap() .expect("telemetry on"); let tel = Arc::new(telemetry::init(cfg).expect("telemetry init")); diff --git a/src/main.rs b/src/main.rs index f573e3f..8c2cd0b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -10,7 +10,7 @@ use std::sync::Arc; use anwesen::app::Anwesen; use anwesen::doctor; use anwesen::merge; -use anwesen::telemetry::{self, RawTelemetryArgs, TelemetryConfig}; +use anwesen::telemetry::{self, OtelEndpointEnv, RawTelemetryArgs, TelemetryConfig}; use anyhow::Result; use clap::Parser; use hydra::Application; @@ -25,14 +25,18 @@ fn main() -> Result<()> { match cli.command { Command::Serve(args) => { init_logging(args.log_level); - // Resolve telemetry config before the supervisor starts; an - // unset endpoint leaves it `None` (export off, behaves as today). - let telemetry = match TelemetryConfig::resolve(RawTelemetryArgs { - uptrace_dsn: args.uptrace_dsn, - otlp_endpoint: args.otlp_endpoint, - otlp_headers: args.otlp_headers, - slow_request_ms: args.otlp_slow_request_ms, - })? { + // Resolve telemetry config before the supervisor starts; no + // OTEL_EXPORTER_OTLP_* endpoint leaves it `None` (export off, + // no middleware). A removed flag is a startup error (ANW-42). + let telemetry = match TelemetryConfig::resolve( + &RawTelemetryArgs { + uptrace_dsn: args.uptrace_dsn, + otlp_endpoint: args.otlp_endpoint, + otlp_headers: args.otlp_headers, + slow_request_ms: args.otlp_slow_request_ms, + }, + OtelEndpointEnv::from_env(), + )? { Some(cfg) => Some(Arc::new(telemetry::init(cfg)?)), None => None, }; diff --git a/src/telemetry.rs b/src/telemetry.rs index a685b2d..70fdab1 100644 --- a/src/telemetry.rs +++ b/src/telemetry.rs @@ -12,27 +12,28 @@ //! the propagated context. Every other request stays metrics-only, so the //! ~3.6k requests/min steady state does not drown the trace backend. //! -//! When no OTLP endpoint (or uptrace DSN) is configured, [`init`] returns +//! Transport configuration comes from the standard `OTEL_EXPORTER_OTLP_*` +//! environment variables only, read by the `OpenTelemetry` SDK itself +//! ([ANW-42](https://crvrs.youtrack.cloud/issue/ANW-42)). anwesen parses no +//! addresses and appends no per-signal paths; the one thing it checks is that +//! `OTEL_EXPORTER_OTLP_ENDPOINT` carries no query or fragment, because the +//! SDK's own concatenation mangles those (measured against a sink: a `?tail` +//! base sends `POST /?tail/v1/metrics`, a `#frag` base sends `POST /`). +//! +//! When no endpoint variable is set, [`TelemetryConfig::resolve`] returns //! `None`, the request middleware is not installed, and the server behaves //! exactly as it did before this module existed. External installs run //! unchanged. -//! -//! Config mirrors the gestell uptrace surface (see the gestell PDR-GES-136 -//! `[otel]` section): a `uptrace_dsn` shorthand, or a generic endpoint plus -//! headers. -use std::collections::HashMap; use std::time::{Duration, SystemTime}; -use anyhow::{Context as _, anyhow, bail}; +use anyhow::{Context as _, bail}; use axum::http::HeaderMap; use opentelemetry::KeyValue; use opentelemetry::metrics::{Counter, Histogram, MeterProvider as _}; use opentelemetry::propagation::{Extractor, TextMapPropagator}; use opentelemetry::trace::{Span, SpanKind, Tracer, TracerProvider as _}; -use opentelemetry_otlp::{ - MetricExporter, Protocol, SpanExporter, WithExportConfig, WithHttpConfig, -}; +use opentelemetry_otlp::{MetricExporter, SpanExporter}; use opentelemetry_sdk::Resource; use opentelemetry_sdk::metrics::{PeriodicReader, SdkMeterProvider}; use opentelemetry_sdk::propagation::TraceContextPropagator; @@ -40,112 +41,166 @@ use opentelemetry_sdk::trace::Sampler; use opentelemetry_sdk::trace::{SdkTracer, SdkTracerProvider}; use opentelemetry_semantic_conventions::resource::SERVICE_VERSION; -/// Raw telemetry options as parsed by clap on the `serve` command. Resolved -/// into an [`Option`] by [`TelemetryConfig::resolve`]. +/// The removed telemetry options, still parsed so their presence is an error +/// rather than a silent config drop on upgrade +/// ([ANW-42](https://crvrs.youtrack.cloud/issue/ANW-42)), plus the one +/// surviving option. Resolved into an [`Option`] by +/// [`TelemetryConfig::resolve`]. #[derive(Debug, Default)] pub struct RawTelemetryArgs { - /// `--uptrace-dsn` / `ANWESEN_UPTRACE_DSN`. + /// Removed `--uptrace-dsn` / `ANWESEN_UPTRACE_DSN`. pub uptrace_dsn: Option, - /// `--otlp-endpoint` / `ANWESEN_OTLP_ENDPOINT`. + /// Removed `--otlp-endpoint` / `ANWESEN_OTLP_ENDPOINT`. pub otlp_endpoint: Option, - /// `--otlp-header` / `ANWESEN_OTLP_HEADERS`, each `key=value`. + /// Removed `--otlp-header` / `ANWESEN_OTLP_HEADERS`. pub otlp_headers: Vec, - /// `--otlp-slow-request-ms` / `ANWESEN_OTLP_SLOW_REQUEST_MS`. + /// `--otlp-slow-request-ms` / `ANWESEN_OTLP_SLOW_REQUEST_MS`. Kept: it + /// decides when anwesen emits a span, not where the export goes, and no + /// standard variable covers it. pub slow_request_ms: u64, } -/// A resolved, telemetry-on configuration. Built only when an endpoint or a -/// DSN is present; absence is represented by `Ok(None)` from [`resolve`]. +/// The OTLP endpoint variables the SDK reads, captured once at startup so the +/// telemetry-on decision and the endpoint check stay pure functions of them. +/// Values are the raw strings; anwesen does not parse them beyond the tail +/// check in [`check_generic_endpoint`]. +#[derive(Debug, Default, Clone, PartialEq, Eq)] +pub struct OtelEndpointEnv { + /// `OTEL_EXPORTER_OTLP_ENDPOINT`: base URL, per-signal path appended by + /// the SDK. + pub generic: Option, + /// `OTEL_EXPORTER_OTLP_METRICS_ENDPOINT`: full URL, used verbatim. + pub metrics: Option, + /// `OTEL_EXPORTER_OTLP_TRACES_ENDPOINT`: full URL, used verbatim. + pub traces: Option, +} + +impl OtelEndpointEnv { + /// Read the three endpoint variables from the process environment. An + /// empty or whitespace-only value counts as unset: it would otherwise + /// turn telemetry on and export to the SDK's `localhost:4318` default. + #[must_use] + pub fn from_env() -> Self { + let var = |name: &str| { + std::env::var(name) + .ok() + .map(|v| v.trim().to_string()) + .filter(|v| !v.is_empty()) + }; + Self { + generic: var("OTEL_EXPORTER_OTLP_ENDPOINT"), + metrics: var("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT"), + traces: var("OTEL_EXPORTER_OTLP_TRACES_ENDPOINT"), + } + } + + /// The endpoint to name in the startup log: the per-signal metrics URL + /// when only that is set, otherwise the generic base. + fn describe(&self) -> &str { + self.generic + .as_deref() + .or(self.metrics.as_deref()) + .or(self.traces.as_deref()) + .unwrap_or("") + } +} + +/// A resolved, telemetry-on configuration. Built only when an endpoint +/// variable is set; absence is represented by `Ok(None)` from [`resolve`]. +/// It carries no transport settings: the exporters read those from the +/// environment themselves. /// /// [`resolve`]: TelemetryConfig::resolve #[derive(Debug, Clone, PartialEq, Eq)] pub struct TelemetryConfig { - /// OTLP/HTTP base URL. The exporter appends the per-signal path - /// (`/v1/metrics`, `/v1/traces`). - pub endpoint: String, - /// Export headers (for uptrace, the `uptrace-dsn` entry) as ordered - /// `(name, value)` pairs. - pub headers: Vec<(String, String)>, /// A request at or over this duration, or answering a 5xx, is recorded /// as a server span. pub slow_request: Duration, + /// The endpoint variables, kept for the startup log line only. + endpoints: OtelEndpointEnv, } impl TelemetryConfig { - /// Resolve raw clap options into an optional config. + /// Resolve the surviving option plus the OTLP endpoint variables into an + /// optional config. /// - /// - Both `uptrace_dsn` and `otlp_endpoint` set is an error (they are - /// two ways to name the same endpoint). - /// - Neither set means telemetry is off: `Ok(None)`. - /// - A malformed `key=value` header or an unparseable DSN is an error, - /// surfaced at startup rather than silently dropping export. + /// - A removed flag or `ANWESEN_` variable is an error naming its `OTEL_` + /// replacement: a deployment exporting today must fail one restart + /// rather than go quiet. + /// - No endpoint variable set means telemetry is off: `Ok(None)`. + /// - A query or fragment on `OTEL_EXPORTER_OTLP_ENDPOINT` is an error. /// /// # Errors - /// Returns an error when the two endpoint sources conflict, a header is - /// not `key=value`, or the uptrace DSN cannot be parsed. - pub fn resolve(raw: RawTelemetryArgs) -> anyhow::Result> { - let slow_request = Duration::from_millis(raw.slow_request_ms); - let mut headers = parse_headers(&raw.otlp_headers)?; - - match (raw.uptrace_dsn, raw.otlp_endpoint) { - (Some(_), Some(_)) => { - bail!("--uptrace-dsn and --otlp-endpoint are mutually exclusive"); - } - (Some(dsn), None) => { - let (endpoint, dsn_header) = parse_uptrace_dsn(&dsn)?; - // The DSN header leads; any explicit --otlp-header follows. - headers.insert(0, dsn_header); - Ok(Some(Self { - endpoint, - headers, - slow_request, - })) - } - (None, Some(endpoint)) => Ok(Some(Self { - endpoint, - headers, - slow_request, - })), - (None, None) => Ok(None), + /// Returns an error when a removed option is present, or when the generic + /// endpoint carries a query or a fragment. + pub fn resolve( + raw: &RawTelemetryArgs, + endpoints: OtelEndpointEnv, + ) -> anyhow::Result> { + check_removed(raw)?; + if endpoints.generic.is_none() && endpoints.metrics.is_none() && endpoints.traces.is_none() + { + return Ok(None); } + if let Some(endpoint) = &endpoints.generic { + check_generic_endpoint(endpoint)?; + } + Ok(Some(Self { + slow_request: Duration::from_millis(raw.slow_request_ms), + endpoints, + })) } } -/// Parse `key=value` header specs. Whitespace around key and value is -/// trimmed; an empty key or a spec with no `=` is an error. -fn parse_headers(specs: &[String]) -> anyhow::Result> { - let mut out = Vec::with_capacity(specs.len()); - for spec in specs { - let (k, v) = spec - .split_once('=') - .ok_or_else(|| anyhow!("OTLP header {spec:?} is not key=value"))?; - let k = k.trim(); - if k.is_empty() { - bail!("OTLP header {spec:?} has an empty key"); +/// Fail on any removed telemetry option, naming the `OTEL_` variable that +/// replaces it. Silence is the expensive failure here: an upgrade that drops +/// the export config would stop telemetry with nothing in the log. +fn check_removed(raw: &RawTelemetryArgs) -> anyhow::Result<()> { + let removed: [(&str, &str, bool); 3] = [ + ( + "--uptrace-dsn / ANWESEN_UPTRACE_DSN", + "OTEL_EXPORTER_OTLP_ENDPOINT=https://api.uptrace.dev plus \ + OTEL_EXPORTER_OTLP_HEADERS=uptrace-dsn=", + raw.uptrace_dsn.is_some(), + ), + ( + "--otlp-endpoint / ANWESEN_OTLP_ENDPOINT", + "OTEL_EXPORTER_OTLP_ENDPOINT", + raw.otlp_endpoint.is_some(), + ), + ( + "--otlp-header / ANWESEN_OTLP_HEADERS", + "OTEL_EXPORTER_OTLP_HEADERS", + !raw.otlp_headers.is_empty(), + ), + ]; + for (option, replacement, present) in removed { + if present { + bail!("{option} was removed in anwesen 0.3.0; use {replacement} instead"); } - out.push((k.to_string(), v.trim().to_string())); } - Ok(out) + Ok(()) } -/// Parse an uptrace DSN (`https://@host[:port]`) into the OTLP -/// endpoint base URL and the `uptrace-dsn` header uptrace expects. The full -/// DSN is echoed as the header value per uptrace's ingest contract. -fn parse_uptrace_dsn(dsn: &str) -> anyhow::Result<(String, (String, String))> { - let dsn = dsn.trim(); - let (scheme, rest) = dsn - .split_once("://") - .context("uptrace DSN has no scheme (expected https://@host)")?; - // Host is whatever follows the credentials `@`; a DSN with no `@` is - // treated as endpoint-only (lenient, though real uptrace DSNs carry a - // token). - let host = rest.rsplit_once('@').map_or(rest, |(_, h)| h); - let host = host.trim_end_matches('/'); - if host.is_empty() { - bail!("uptrace DSN has no host"); +/// Reject a query or fragment on `OTEL_EXPORTER_OTLP_ENDPOINT`. The SDK +/// appends the per-signal path to this value textually, so a `?grpc=4317` +/// tail sends `POST /?grpc=4317/v1/metrics` and a `#frag` tail sends +/// `POST /` -- both dead exports, neither logged. Measured against a sink on +/// opentelemetry-otlp 0.32 ([ANW-42]). A path prefix composes correctly and +/// is left alone. +/// +/// [ANW-42]: https://crvrs.youtrack.cloud/issue/ANW-42 +fn check_generic_endpoint(endpoint: &str) -> anyhow::Result<()> { + if let Some(bad) = endpoint.find(['?', '#']) { + let tail = &endpoint[bad..]; + bail!( + "OTEL_EXPORTER_OTLP_ENDPOINT {endpoint:?} has a trailing {tail:?}; \ + the exporter would append the signal path after it and export nowhere. \ + Pass the base URL alone, and put an uptrace DSN in \ + OTEL_EXPORTER_OTLP_HEADERS=uptrace-dsn=" + ); } - let endpoint = format!("{scheme}://{host}"); - Ok((endpoint, ("uptrace-dsn".to_string(), dsn.to_string()))) + Ok(()) } /// Semantic route bucket for the `http.route` label. Coarser than the axum @@ -333,27 +388,26 @@ struct TelemetryInner { impl TelemetryInner { fn new(config: TelemetryConfig) -> anyhow::Result { let TelemetryConfig { - endpoint, - headers, slow_request, + endpoints, } = config; - // `.with_endpoint()` is used verbatim by the exporter (the `/v1/*` - // suffix is only auto-appended for the generic OTEL env var), so we - // append the per-signal path ourselves. - let endpoint = endpoint.trim_end_matches('/').to_string(); - let header_map: HashMap = headers.into_iter().collect(); + // Service identity is a default only: OTEL_SERVICE_NAME and + // OTEL_RESOURCE_ATTRIBUTES override it by the SDK's own precedence. let resource = Resource::builder() .with_service_name("anwesen") .with_attribute(KeyValue::new(SERVICE_VERSION, env!("CARGO_PKG_VERSION"))) .build(); // -- metrics: thread-based periodic reader over an OTLP/HTTP exporter. + // No endpoint, headers, or protocol here: the SDK reads + // OTEL_EXPORTER_OTLP_* itself and composes the per-signal URL + // (ANW-42). `.with_http()` only picks the transport this binary is + // built with, so OTEL_EXPORTER_OTLP_PROTOCOL selects between + // http/protobuf (the default, and what anwesen sent before) and + // http/json; a `grpc` value has no effect. let metric_exporter = MetricExporter::builder() .with_http() - .with_protocol(Protocol::HttpBinary) - .with_endpoint(format!("{endpoint}/v1/metrics")) - .with_headers(header_map.clone()) .build() .context("build OTLP metric exporter")?; let reader = PeriodicReader::builder(metric_exporter) @@ -386,9 +440,6 @@ impl TelemetryInner { // -- traces: thread-based batch processor over an OTLP/HTTP exporter. let span_exporter = SpanExporter::builder() .with_http() - .with_protocol(Protocol::HttpBinary) - .with_endpoint(format!("{endpoint}/v1/traces")) - .with_headers(header_map) .build() .context("build OTLP span exporter")?; let tracer_provider = SdkTracerProvider::builder() @@ -401,7 +452,7 @@ impl TelemetryInner { let tracer = tracer_provider.tracer("anwesen"); tracing::info!( - endpoint = %endpoint, + endpoint = %endpoints.describe(), slow_request_ms = slow_request.as_millis(), "telemetry: OTLP export enabled" ); @@ -489,93 +540,136 @@ mod tests { RawTelemetryArgs::default() } - #[test] - fn resolve_off_when_nothing_set() { - let cfg = TelemetryConfig::resolve(raw()).unwrap(); - assert!(cfg.is_none(), "no endpoint/DSN means telemetry off"); + fn generic(endpoint: &str) -> OtelEndpointEnv { + OtelEndpointEnv { + generic: Some(endpoint.into()), + ..OtelEndpointEnv::default() + } } #[test] - fn resolve_generic_endpoint() { - let cfg = TelemetryConfig::resolve(RawTelemetryArgs { - otlp_endpoint: Some("https://collector.example.com".into()), - otlp_headers: vec!["authorization=Bearer xyz".into()], - slow_request_ms: 500, - ..raw() - }) - .unwrap() - .expect("telemetry on"); - assert_eq!(cfg.endpoint, "https://collector.example.com"); - assert_eq!( - cfg.headers, - vec![("authorization".to_string(), "Bearer xyz".to_string())] - ); - assert_eq!(cfg.slow_request, Duration::from_millis(500)); + fn resolve_off_when_no_endpoint_variable_is_set() { + let cfg = TelemetryConfig::resolve(&raw(), OtelEndpointEnv::default()).unwrap(); + assert!(cfg.is_none(), "no OTEL_ endpoint means telemetry off"); } #[test] - fn resolve_uptrace_dsn_splits_endpoint_and_header() { - let cfg = TelemetryConfig::resolve(RawTelemetryArgs { - uptrace_dsn: Some("https://SECRET_TOKEN@api.uptrace.dev".into()), - slow_request_ms: 250, - ..raw() - }) - .unwrap() - .expect("telemetry on"); - assert_eq!(cfg.endpoint, "https://api.uptrace.dev"); - assert_eq!( - cfg.headers, - vec![( - "uptrace-dsn".to_string(), - "https://SECRET_TOKEN@api.uptrace.dev".to_string() - )] - ); - assert_eq!(cfg.slow_request, Duration::from_millis(250)); + fn resolve_on_for_each_endpoint_variable() { + let per_signal = |field: fn(&mut OtelEndpointEnv)| { + let mut env = OtelEndpointEnv::default(); + field(&mut env); + env + }; + for env in [ + generic("https://collector.example.com"), + per_signal(|e| e.metrics = Some("https://collector.example.com/v1/metrics".into())), + per_signal(|e| e.traces = Some("https://collector.example.com/v1/traces".into())), + ] { + let cfg = TelemetryConfig::resolve( + &RawTelemetryArgs { + slow_request_ms: 250, + ..raw() + }, + env.clone(), + ) + .unwrap_or_else(|e| panic!("{env:?}: {e}")) + .expect("telemetry on"); + assert_eq!(cfg.slow_request, Duration::from_millis(250)); + } + } + + /// The tails the SDK mangles: a query lands in the query string with the + /// signal path behind it, a fragment drops the signal path entirely. + #[test] + fn resolve_rejects_a_query_or_fragment_on_the_generic_endpoint() { + for endpoint in [ + "https://api.uptrace.dev?grpc=4317", + "https://api.uptrace.dev/?grpc=4317", + "https://api.uptrace.dev:4318?grpc=4317", + "https://api.uptrace.dev#frag", + ] { + let err = TelemetryConfig::resolve(&raw(), generic(endpoint)).unwrap_err(); + assert!(err.to_string().contains("trailing"), "{endpoint}: {err}"); + } + } + + /// A path prefix composes correctly (`/otlp` -> `POST /otlp/v1/metrics`), + /// and the per-signal variables are used verbatim, tail and all. + #[test] + fn resolve_accepts_a_path_prefix_and_per_signal_tails() { + for env in [ + generic("https://collector.example.com/otlp"), + generic("https://collector.example.com/"), + OtelEndpointEnv { + metrics: Some("https://api.uptrace.dev/v1/metrics?grpc=4317".into()), + ..OtelEndpointEnv::default() + }, + ] { + TelemetryConfig::resolve(&raw(), env.clone()) + .unwrap_or_else(|e| panic!("{env:?}: {e}")) + .expect("telemetry on"); + } } #[test] - fn resolve_dsn_header_leads_explicit_headers() { - let cfg = TelemetryConfig::resolve(RawTelemetryArgs { - uptrace_dsn: Some("https://tok@api.uptrace.dev".into()), - otlp_headers: vec!["x-extra=1".into()], - ..raw() - }) - .unwrap() - .expect("telemetry on"); - assert_eq!(cfg.headers[0].0, "uptrace-dsn"); - assert_eq!(cfg.headers[1], ("x-extra".to_string(), "1".to_string())); + fn resolve_rejects_the_removed_uptrace_dsn() { + let err = TelemetryConfig::resolve( + &RawTelemetryArgs { + uptrace_dsn: Some("https://tok@api.uptrace.dev".into()), + ..raw() + }, + OtelEndpointEnv::default(), + ) + .unwrap_err() + .to_string(); + assert!(err.contains("--uptrace-dsn"), "{err}"); + assert!(err.contains("OTEL_EXPORTER_OTLP_HEADERS"), "{err}"); } #[test] - fn resolve_rejects_both_endpoint_sources() { - let err = TelemetryConfig::resolve(RawTelemetryArgs { - uptrace_dsn: Some("https://tok@api.uptrace.dev".into()), - otlp_endpoint: Some("https://collector.example.com".into()), - ..raw() - }) + fn resolve_rejects_the_removed_otlp_endpoint() { + let err = TelemetryConfig::resolve( + &RawTelemetryArgs { + otlp_endpoint: Some("https://collector.example.com".into()), + ..raw() + }, + OtelEndpointEnv::default(), + ) + .unwrap_err() + .to_string(); + assert!(err.contains("--otlp-endpoint"), "{err}"); + assert!(err.contains("OTEL_EXPORTER_OTLP_ENDPOINT"), "{err}"); + } + + #[test] + fn resolve_rejects_the_removed_otlp_header() { + let err = TelemetryConfig::resolve( + &RawTelemetryArgs { + otlp_headers: vec!["authorization=Bearer xyz".into()], + ..raw() + }, + OtelEndpointEnv::default(), + ) + .unwrap_err() + .to_string(); + assert!(err.contains("--otlp-header"), "{err}"); + assert!(err.contains("OTEL_EXPORTER_OTLP_HEADERS"), "{err}"); + } + + /// A removed option fails even with a correct `OTEL_` endpoint alongside + /// it: the operator's intent is in the flag, and half-applied config is + /// the silent failure this issue removes. + #[test] + fn resolve_rejects_a_removed_option_alongside_a_good_endpoint() { + let err = TelemetryConfig::resolve( + &RawTelemetryArgs { + uptrace_dsn: Some("https://tok@api.uptrace.dev".into()), + ..raw() + }, + generic("https://api.uptrace.dev"), + ) .unwrap_err(); - assert!(err.to_string().contains("mutually exclusive")); - } - - #[test] - fn resolve_rejects_malformed_header() { - let err = TelemetryConfig::resolve(RawTelemetryArgs { - otlp_endpoint: Some("https://collector.example.com".into()), - otlp_headers: vec!["no-equals-sign".into()], - ..raw() - }) - .unwrap_err(); - assert!(err.to_string().contains("key=value")); - } - - #[test] - fn resolve_rejects_dsn_without_scheme() { - let err = TelemetryConfig::resolve(RawTelemetryArgs { - uptrace_dsn: Some("tok@api.uptrace.dev".into()), - ..raw() - }) - .unwrap_err(); - assert!(err.to_string().contains("scheme")); + assert!(err.to_string().contains("removed"), "{err}"); } #[test]