diff --git a/docs/telemetry.md b/docs/telemetry.md index ac3e6c7..98fcf58 100644 --- a/docs/telemetry.md +++ b/docs/telemetry.md @@ -88,8 +88,13 @@ Explicit filters must be valid and nonempty; changing them requires a restart. Without explicit filters, `RUST_LOG=warn` suppresses informational spans and logs. Use `wakey_agent=info,wakey=info,wakey_core=info,opentelemetry=warn` for the agent and `wakey_control_plane=info,wakey_core=info,opentelemetry=warn` for CC. Exporter warnings -remain local to prevent export-failure feedback loops. Increasing verbosity exports the -existing debug events too; inspect them before enabling it broadly. +remain local to prevent export-failure feedback loops. Both OTLP logs and traces +exclude OpenTelemetry SDK targets and the `h2`, `hyper`, `hyper_util`, `tonic` +and `tower` crate targets, even under broad debug filters. These transport +crates also serve application traffic: their diagnostics stay console-only, +while Wakey's command/storage/relay spans still export. Console filters remain +independent. Increasing verbosity exports application debug events too; inspect +them before enabling it broadly. ## Signals diff --git a/wakey-control-plane/src/api/control/devices.rs b/wakey-control-plane/src/api/control/devices.rs index 4fbb504..d89a9c9 100644 --- a/wakey-control-plane/src/api/control/devices.rs +++ b/wakey-control-plane/src/api/control/devices.rs @@ -35,6 +35,15 @@ pub async fn rename_known_device( AxumPath(device_id): AxumPath, Json(req): Json, ) -> Result { + // The store validates too, but checking here distinguishes bad input from + // storage failures without interpreting anyhow error messages. + if req.display_name.trim().is_empty() { + return Err(ApiError::new( + StatusCode::BAD_REQUEST, + "rename_known_device_failed", + "display_name must not be empty", + )); + } match state .store .rename_known_device(&device_id, &req.display_name) @@ -49,7 +58,7 @@ pub async fn rename_known_device( Err(err) => { warn!(error = %err, "failed to rename known device"); Err(ApiError::new( - StatusCode::BAD_REQUEST, + StatusCode::INTERNAL_SERVER_ERROR, "rename_known_device_failed", err.to_string(), )) @@ -294,3 +303,112 @@ fn known_device_response(device: crate::state::KnownDevice) -> KnownDeviceRespon .collect(), } } + +#[cfg(test)] +mod tests { + use super::*; + use std::collections::HashMap; + use std::sync::Arc; + use std::time::Duration; + use tokio::sync::{Mutex, RwLock}; + + #[tokio::test] + async fn rename_distinguishes_invalid_names_missing_devices_and_storage_failures() { + let dir = std::env::temp_dir().join(format!("wakey-rename-api-{}", uuid::Uuid::new_v4())); + let store = Arc::new( + crate::state::Store::load_or_init( + &dir.join("state.sqlite3"), + vec![], + Duration::from_secs(60), + ) + .await + .unwrap(), + ); + let state = AppState { + store: store.clone(), + sessions: Arc::new(RwLock::new(HashMap::new())), + pending: Arc::new(Mutex::new(HashMap::new())), + public_url: "http://localhost".into(), + command_timeout: Duration::from_secs(30), + enroll_token_ttl: Duration::from_secs(60), + terminals: crate::runtime::terminals::TerminalRegistry::new(), + alerts: crate::api::AlertsHub::new(), + fleet: Arc::new(crate::api::FleetCache::default()), + }; + let device = store + .create_known_device(KnownDeviceInput { + display_name: "before".into(), + pinned: false, + notes: None, + identifiers: vec![], + }) + .await + .unwrap(); + let rename = |id: String, name: &str| { + rename_known_device( + State(state.clone()), + AxumPath(id), + Json(RenameKnownDeviceRequest { + display_name: name.into(), + }), + ) + }; + let response = rename(device.device_id.clone(), " after ") + .await + .unwrap() + .into_response(); + assert_eq!(response.status(), StatusCode::OK); + assert_eq!( + store + .get_known_device(&device.device_id) + .await + .unwrap() + .unwrap() + .display_name, + "after" + ); + assert_eq!( + rename("missing".into(), "valid") + .await + .err() + .unwrap() + .status, + StatusCode::NOT_FOUND + ); + assert_eq!( + rename(device.device_id.clone(), " \t\n ") + .await + .err() + .unwrap() + .status, + StatusCode::BAD_REQUEST + ); + // A deterministic SQLite failure, not a clock-sensitive lock timeout. + let mut tx = store.begin_write().await.unwrap(); + sqlx::query("CREATE TRIGGER reject_rename BEFORE UPDATE ON known_devices BEGIN SELECT RAISE(ABORT, 'test rename failure'); END") + .execute(&mut *tx).await.unwrap(); + tx.commit().await.unwrap(); + let failure = rename(device.device_id.clone(), "valid") + .await + .err() + .unwrap(); + assert_eq!(failure.status, StatusCode::INTERNAL_SERVER_ERROR); + assert_eq!(failure.code, "rename_known_device_failed"); + assert_eq!( + store + .get_known_device(&device.device_id) + .await + .unwrap() + .unwrap() + .display_name, + "after" + ); + assert_eq!( + rename(device.device_id, " ").await.err().unwrap().status, + StatusCode::BAD_REQUEST + ); + drop(state); + drop(store); + std::fs::remove_dir_all(dir).unwrap(); + } +} diff --git a/wakey-core/src/telemetry.rs b/wakey-core/src/telemetry.rs index ef566df..61a1216 100644 --- a/wakey-core/src/telemetry.rs +++ b/wakey-core/src/telemetry.rs @@ -181,14 +181,14 @@ pub fn init( let _ = METRICS.set(Instruments::new()); let otel = tracing_opentelemetry::layer() .with_tracer(traces.tracer(service.to_owned())) - .with_filter(trace_filter); - // Exporter failures must remain local, never feed back into the failing exporter. + .with_filter(trace_filter) + .with_filter(tracing_subscriber::filter::filter_fn(not_exporter_internal)); + // Apply the same exclusion to both OTLP layers. Console filtering stays + // independent so exporter and transport diagnostics remain available. let logs_layer = opentelemetry_appender_tracing::layer::OpenTelemetryTracingBridge::new(&logs) .with_filter(log_filter) - .with_filter(tracing_subscriber::filter::filter_fn(|metadata| { - !metadata.target().starts_with("opentelemetry") - })); + .with_filter(tracing_subscriber::filter::filter_fn(not_exporter_internal)); let subscriber = tracing_subscriber::registry().with(otel).with(logs_layer); if config.json_logs { subscriber @@ -235,6 +235,17 @@ pub fn init( Ok(guard) } +/// Exporting the exporter's own diagnostics can create a perpetual feedback +/// loop. Transport targets are shared with application traffic, so exclude +/// them from OTLP altogether while retaining console diagnostics. +fn not_exporter_internal(metadata: &tracing::Metadata<'_>) -> bool { + let target = metadata.target(); + let root = target.split("::").next().unwrap_or(target); + !(root == "opentelemetry" + || root.starts_with("opentelemetry_") + || matches!(root, "h2" | "hyper" | "hyper_util" | "tonic" | "tower")) +} + fn signal_filter( explicit: Option<&str>, rust_log: Option<&str>, diff --git a/wakey-core/tests/telemetry.rs b/wakey-core/tests/telemetry.rs index 9f95112..fc0ec85 100644 --- a/wakey-core/tests/telemetry.rs +++ b/wakey-core/tests/telemetry.rs @@ -238,7 +238,7 @@ async fn exports_all_signals_and_preserves_remote_command_parent() { json_logs: false, console_filter: Some("info".into()), otlp_log_filter: Some("debug".into()), - trace_filter: Some("info".into()), + trace_filter: Some("info,opentelemetry_sdk=debug,h2=debug,hyper=debug,hyper_util=debug,tonic=debug,tower=debug".into()), }, "unused", "info", @@ -295,6 +295,20 @@ async fn exports_all_signals_and_preserves_remote_command_parent() { let hidden = tracing::debug_span!("debug_span_not_exported"); drop(hidden); tracing::warn!(target: "opentelemetry_sdk", "synthetic exporter failure remains local"); + macro_rules! transport_activity { + ($target:literal) => {{ + let span = tracing::debug_span!(target: $target, "exporter_transport_span"); + span.in_scope(|| tracing::debug!(target: $target, "synthetic exporter transport event")); + }}; + } + transport_activity!("opentelemetry_sdk"); + transport_activity!("h2::proto"); + transport_activity!("hyper::client"); + transport_activity!("hyper_util::client"); + transport_activity!("tonic::transport"); + transport_activity!("tower::buffer"); + // A similarly named application target is not transport infrastructure. + tracing::info!(target: "tower_device", "application target remains exported"); let invalid_parent = tracing::info_span!("invalid_remote_parent"); telemetry::set_remote_parent( &invalid_parent, @@ -332,6 +346,12 @@ async fn exports_all_signals_and_preserves_remote_command_parent() { assert!(invalid_parent.parent_span_id.is_empty()); assert_ne!(parent.trace_id, invalid_parent.trace_id); let logs = collector.logs.lock().unwrap(); + assert!( + !spans + .iter() + .any(|span| span.name == "exporter_transport_span"), + "exporter transport spans must not feed back into OTLP" + ); assert!( !spans .iter() @@ -343,6 +363,17 @@ async fn exports_all_signals_and_preserves_remote_command_parent() { .flat_map(|r| &r.scope_logs) .flat_map(|s| &s.log_records) .collect(); + assert!( + !records + .iter() + .any(|r| format!("{:?}", r.body).contains("synthetic exporter transport event")), + "exporter transport events must not feed back into OTLP" + ); + assert!( + records + .iter() + .any(|r| format!("{:?}", r.body).contains("application target remains exported")) + ); assert!( records .iter()