diff --git a/src/api.rs b/src/api.rs index 89d7aea..21596c0 100644 --- a/src/api.rs +++ b/src/api.rs @@ -13,7 +13,7 @@ use crate::engine::{ EngineResponse, }; use crate::alerts::{deliver_ntfy, AlertError}; -use crate::profile::{self, ParamDeclarationRecord, Profile, ProfileError, ProfileStore}; +use crate::profile::{self, Dashboard, ParamDeclarationRecord, Profile, ProfileError}; use crate::settings::{CssError, SettingsHandle}; use axum::extract::{Path as AxumPath, Query, State}; use axum::http::{header, StatusCode}; @@ -43,11 +43,11 @@ impl ApiState { /// Best-effort: a no-op if no profile is active, and I/O failures are only logged, since the engine declaration is already the source of truth. fn persist_param_declaration(&self, request: &DeclareParamRequest) { - let Some((store_path, profile_name)) = self.active_profile_location() else { + let mut settings = self.settings.lock().unwrap(); + let Some(profile_name) = settings.active_profile.clone() else { return; }; - let mut store = ProfileStore::load(&store_path); - if let Err(err) = store.record_param_declaration(&store_path, &profile_name, request) { + if let Err(err) = settings.record_param_declaration(&self.settings_path, &profile_name, request) { eprintln!( "warning: failed to persist param declaration '{}' into profile '{profile_name}': {err}", request.name @@ -61,15 +61,25 @@ impl ApiState { /// auto-detected logfmt key survives a restart instead of being rejected as "undeclared" once /// declarations replay before any log line re-derives it. async fn persist_metric_declaration(&self, request: &DeclareMetricRequest) { - let Some((store_path, profile_name)) = self.active_profile_location() else { - return; + let profile_name = { + let settings = self.settings.lock().unwrap(); + let Some(profile_name) = settings.active_profile.clone() else { + return; + }; + profile_name + }; + let existing_params = { + let settings = self.settings.lock().unwrap(); + settings.profile(&profile_name).map(|p| p.params.clone()).unwrap_or_default() }; - let mut store = ProfileStore::load(&store_path); - let existing_params = store.get(&profile_name).map(|p| p.params.clone()).unwrap_or_default(); let promoted = promoted_param_records(&self.engine, &existing_params, request).await; - if let Err(err) = - store.record_metric_declaration(&store_path, &profile_name, request, &promoted) - { + let mut settings = self.settings.lock().unwrap(); + if let Err(err) = settings.record_metric_declaration( + &self.settings_path, + &profile_name, + request, + &promoted, + ) { eprintln!( "warning: failed to persist metric declaration '{}' into profile '{profile_name}': {err}", request.name @@ -79,14 +89,21 @@ impl ApiState { /// Same as `persist_metric_declaration`, for an in-place edit (`PUT /api/metrics/{name}`) — also drops the persisted record under `old_name` when the edit renames the metric, so a rename doesn't leave a stale duplicate declaration in the profile file. async fn persist_metric_declaration_update(&self, old_name: &str, request: &DeclareMetricRequest) { - let Some((store_path, profile_name)) = self.active_profile_location() else { - return; + let profile_name = { + let settings = self.settings.lock().unwrap(); + let Some(profile_name) = settings.active_profile.clone() else { + return; + }; + profile_name + }; + let existing_params = { + let settings = self.settings.lock().unwrap(); + settings.profile(&profile_name).map(|p| p.params.clone()).unwrap_or_default() }; - let mut store = ProfileStore::load(&store_path); - let existing_params = store.get(&profile_name).map(|p| p.params.clone()).unwrap_or_default(); let promoted = promoted_param_records(&self.engine, &existing_params, request).await; - if let Err(err) = store.record_metric_declaration_update( - &store_path, + let mut settings = self.settings.lock().unwrap(); + if let Err(err) = settings.record_metric_declaration_update( + &self.settings_path, &profile_name, old_name, request, @@ -98,13 +115,6 @@ impl ApiState { ); } } - - fn active_profile_location(&self) -> Option<(PathBuf, String)> { - let settings = self.settings.lock().unwrap(); - let profile_name = settings.profiles.active_profile.clone()?; - let profiles_dir = profile::resolve_profiles_dir(&settings, &self.settings_path); - Some((profile::profiles_store_path(&profiles_dir), profile_name)) - } } pub fn router(engine: EngineHandle, settings: SettingsHandle, settings_path: PathBuf) -> Router { @@ -621,9 +631,7 @@ fn profile_error_response(err: ProfileError) -> axum::response::Response { async fn list_profiles(State(state): State) -> impl IntoResponse { let settings = state.settings.lock().unwrap(); - let profiles_dir = profile::resolve_profiles_dir(&settings, &state.settings_path); - let store = ProfileStore::load(&profile::profiles_store_path(&profiles_dir)); - let summaries: Vec = store.list().iter().map(ProfileSummaryDto::from).collect(); + let summaries: Vec = settings.profiles.iter().map(ProfileSummaryDto::from).collect(); Json(summaries).into_response() } @@ -632,12 +640,9 @@ async fn create_profile( Json(body): Json, ) -> impl IntoResponse { let mut settings = state.settings.lock().unwrap(); - let profiles_dir = profile::resolve_profiles_dir(&settings, &state.settings_path); - let store_path = profile::profiles_store_path(&profiles_dir); - let mut store = ProfileStore::load(&store_path); - match store.create(&store_path, &mut settings, &state.settings_path, body.name.clone()) { + match settings.create_profile(&state.settings_path, body.name.clone()) { Ok(()) => { - let profile = store.get(&body.name).expect("profile was just created by store.create"); + let profile = settings.profile(&body.name).expect("profile was just created by create_profile"); (StatusCode::CREATED, Json(ProfileSummaryDto::from(profile))).into_response() } Err(err) => profile_error_response(err), @@ -650,10 +655,7 @@ async fn delete_profile( AxumPath(name): AxumPath, ) -> impl IntoResponse { let mut settings = state.settings.lock().unwrap(); - let profiles_dir = profile::resolve_profiles_dir(&settings, &state.settings_path); - let store_path = profile::profiles_store_path(&profiles_dir); - let mut store = ProfileStore::load(&store_path); - match store.delete(&store_path, &mut settings, &state.settings_path, &name) { + match settings.delete_profile(&state.settings_path, &name) { Ok(()) => StatusCode::NO_CONTENT.into_response(), Err(err) => profile_error_response(err), } @@ -701,11 +703,8 @@ async fn activate_profile( ) -> impl IntoResponse { let activated_profile = { let mut settings = state.settings.lock().unwrap(); - let profiles_dir = profile::resolve_profiles_dir(&settings, &state.settings_path); - let store_path = profile::profiles_store_path(&profiles_dir); - let store = ProfileStore::load(&store_path); - match store.set_active(&mut settings, &state.settings_path, Some(name.clone())) { - Ok(()) => Ok(store.get(&name).cloned()), + match settings.set_active_profile(&state.settings_path, Some(name.clone())) { + Ok(()) => Ok(settings.profile(&name).cloned()), Err(err) => Err(err), } }; @@ -727,9 +726,7 @@ async fn list_dashboards( AxumPath(name): AxumPath, ) -> impl IntoResponse { let settings = state.settings.lock().unwrap(); - let profiles_dir = profile::resolve_profiles_dir(&settings, &state.settings_path); - let store = ProfileStore::load(&profile::profiles_store_path(&profiles_dir)); - match store.list_dashboards(&name) { + match settings.list_dashboards(&name) { Ok(dashboards) => { let dtos: Vec = dashboards.iter().map(DashboardDto::from).collect(); Json(dtos).into_response() @@ -743,11 +740,8 @@ async fn create_dashboard( AxumPath(name): AxumPath, Json(body): Json, ) -> impl IntoResponse { - let settings = state.settings.lock().unwrap(); - let profiles_dir = profile::resolve_profiles_dir(&settings, &state.settings_path); - let store_path = profile::profiles_store_path(&profiles_dir); - let mut store = ProfileStore::load(&store_path); - match store.create_dashboard(&store_path, &name, body.name) { + let mut settings = state.settings.lock().unwrap(); + match settings.create_dashboard(&state.settings_path, &name, body.name) { Ok(dashboard) => (StatusCode::CREATED, Json(DashboardDto::from(&dashboard))).into_response(), Err(err) => profile_error_response(err), } @@ -758,9 +752,7 @@ async fn get_dashboard( AxumPath((name, dashboard_id)): AxumPath<(String, String)>, ) -> impl IntoResponse { let settings = state.settings.lock().unwrap(); - let profiles_dir = profile::resolve_profiles_dir(&settings, &state.settings_path); - let store = ProfileStore::load(&profile::profiles_store_path(&profiles_dir)); - match store.list_dashboards(&name) { + match settings.list_dashboards(&name) { Ok(dashboards) => match dashboards.iter().find(|d| d.id == dashboard_id) { Some(dashboard) => Json(DashboardDto::from(dashboard)).into_response(), None => profile_error_response(ProfileError::NotFound), @@ -774,16 +766,13 @@ async fn save_dashboard( AxumPath((name, dashboard_id)): AxumPath<(String, String)>, Json(body): Json, ) -> impl IntoResponse { - let settings = state.settings.lock().unwrap(); - let profiles_dir = profile::resolve_profiles_dir(&settings, &state.settings_path); - let store_path = profile::profiles_store_path(&profiles_dir); - let mut store = ProfileStore::load(&store_path); - let dashboard = crate::profile::Dashboard { + let mut settings = state.settings.lock().unwrap(); + let dashboard = Dashboard { id: dashboard_id.clone(), name: body.name, panels: body.panels.iter().map(|p| p.into()).collect(), }; - match store.save_dashboard(&store_path, &name, &dashboard_id, dashboard) { + match settings.save_dashboard(&state.settings_path, &name, &dashboard_id, dashboard) { Ok(dashboard) => Json(DashboardDto::from(&dashboard)).into_response(), Err(err) => profile_error_response(err), } @@ -793,11 +782,8 @@ async fn delete_dashboard( State(state): State, AxumPath((name, dashboard_id)): AxumPath<(String, String)>, ) -> impl IntoResponse { - let settings = state.settings.lock().unwrap(); - let profiles_dir = profile::resolve_profiles_dir(&settings, &state.settings_path); - let store_path = profile::profiles_store_path(&profiles_dir); - let mut store = ProfileStore::load(&store_path); - match store.delete_dashboard(&store_path, &name, &dashboard_id) { + let mut settings = state.settings.lock().unwrap(); + match settings.delete_dashboard(&state.settings_path, &name, &dashboard_id) { Ok(()) => StatusCode::NO_CONTENT.into_response(), Err(err) => profile_error_response(err), } @@ -1231,20 +1217,10 @@ mod tests { // Use the same shared settings handle the router was built with — a separately-loaded Settings wouldn't be visible to the running app. let settings_path = dir.path().join("settings.json"); - let profiles_dir = { - let settings = settings_handle.lock().unwrap(); - profile::resolve_profiles_dir(&settings, &settings_path) - }; - let store_path = profile::profiles_store_path(&profiles_dir); - let mut store = ProfileStore::load(&store_path); { let mut settings = settings_handle.lock().unwrap(); - store - .create(&store_path, &mut settings, &settings_path, "web".to_string()) - .unwrap(); - store - .set_active(&mut settings, &settings_path, Some("web".to_string())) - .unwrap(); + settings.create_profile(&settings_path, "web".to_string()).unwrap(); + settings.set_active_profile(&settings_path, Some("web".to_string())).unwrap(); } let (status, _body) = send_json( @@ -1261,8 +1237,8 @@ mod tests { .await; assert_eq!(status, StatusCode::CREATED); - let reloaded_store = ProfileStore::load(&store_path); - let profile = reloaded_store.get("web").unwrap(); + let reloaded_settings = Settings::load(&settings_path); + let profile = reloaded_settings.profile("web").unwrap(); assert_eq!(profile.params.len(), 1); assert_eq!(profile.params[0].name, "level"); } @@ -1277,20 +1253,10 @@ mod tests { let (app, engine, settings_handle, dir) = test_app(); let settings_path = dir.path().join("settings.json"); - let profiles_dir = { - let settings = settings_handle.lock().unwrap(); - profile::resolve_profiles_dir(&settings, &settings_path) - }; - let store_path = profile::profiles_store_path(&profiles_dir); - let mut store = ProfileStore::load(&store_path); { let mut settings = settings_handle.lock().unwrap(); - store - .create(&store_path, &mut settings, &settings_path, "web".to_string()) - .unwrap(); - store - .set_active(&mut settings, &settings_path, Some("web".to_string())) - .unwrap(); + settings.create_profile(&settings_path, "web".to_string()).unwrap(); + settings.set_active_profile(&settings_path, Some("web".to_string())).unwrap(); } // Ingest a line carrying a logfmt key before any param is manually declared, so "dx" only @@ -1310,8 +1276,8 @@ mod tests { .await; assert_eq!(status, StatusCode::CREATED); - let reloaded_store = ProfileStore::load(&store_path); - let profile = reloaded_store.get("web").unwrap(); + let reloaded_settings = Settings::load(&settings_path); + let profile = reloaded_settings.profile("web").unwrap(); assert_eq!(profile.metrics.len(), 1); assert_eq!(profile.metrics[0].name, "direction_x"); // The auto-derived "dx" param got promoted into the profile alongside the metric. @@ -1413,7 +1379,7 @@ mod tests { #[tokio::test] async fn saved_dashboard_persists_across_simulated_restart() { - let (app, _engine, settings_handle, dir) = test_app(); + let (app, _engine, _settings_handle, dir) = test_app(); let (status, _body) = send_json(&app, "POST", "/api/settings/profiles", json!({ "name": "web" })).await; assert_eq!(status, StatusCode::CREATED); @@ -1440,15 +1406,10 @@ mod tests { ) .await; - // Simulate a restart by reloading the profile store fresh from disk, independent of the running app/engine. + // Simulate a restart by reloading settings fresh from disk, independent of the running app/engine. let settings_path = dir.path().join("settings.json"); - let profiles_dir = { - let settings = settings_handle.lock().unwrap(); - profile::resolve_profiles_dir(&settings, &settings_path) - }; - let store_path = profile::profiles_store_path(&profiles_dir); - let reloaded_store = ProfileStore::load(&store_path); - let dashboards = reloaded_store.list_dashboards("web").unwrap(); + let reloaded_settings = Settings::load(&settings_path); + let dashboards = reloaded_settings.list_dashboards("web").unwrap(); assert_eq!(dashboards.len(), 1); assert_eq!(dashboards[0].name, "Overview"); assert_eq!(dashboards[0].panels.len(), 1); diff --git a/src/dto.rs b/src/dto.rs index 3f010f8..8e07754 100644 --- a/src/dto.rs +++ b/src/dto.rs @@ -198,7 +198,7 @@ impl From<&Settings> for SettingsDto { fn from(settings: &Settings) -> Self { SettingsDto { color_scheme: settings.color_scheme, - active_profile: settings.profiles.active_profile.clone(), + active_profile: settings.active_profile.clone(), max_logs: settings.max_logs, custom_theme_id: settings.custom_theme_id.clone(), ntfy_topic: settings.ntfy_topic.clone(), diff --git a/src/main.rs b/src/main.rs index 8823fba..aa762ce 100644 --- a/src/main.rs +++ b/src/main.rs @@ -22,7 +22,6 @@ use clap::Parser; use tokio::net::TcpListener; use tokio::sync::mpsc; -use profile::ProfileStore; use settings::Settings; /// The resolved default settings path, rendered into `--help` so `--settings-path`'s help text @@ -76,28 +75,28 @@ async fn main() { // Reload the active profile's declarations before ingest starts: there's no way to // reprocess early lines later, so declaring after ingest starts could miss lines that raced ahead. { - let (active_profile_name, profiles_dir) = { + let (active_profile_name, active_profile) = { let mut settings_guard = settings.lock().unwrap(); - let profiles_dir = profile::resolve_profiles_dir(&settings_guard, &settings_path); - let mut store = ProfileStore::load(&profile::profiles_store_path(&profiles_dir)); - store - .ensure_default(&profile::profiles_store_path(&profiles_dir), &mut settings_guard, &settings_path) + settings_guard + .ensure_default_profile(&settings_path) .expect("failed to ensure the Default profile exists"); - (settings_guard.profiles.active_profile.clone(), profiles_dir) + let active_profile_name = settings_guard.active_profile.clone(); + let active_profile = active_profile_name + .as_deref() + .and_then(|name| settings_guard.profile(name).cloned()); + (active_profile_name, active_profile) }; - if let Some(profile_name) = active_profile_name { - let store = ProfileStore::load(&profile::profiles_store_path(&profiles_dir)); - match store.get(&profile_name) { - Some(active_profile) => { - profile::load_into_engine(&engine_tx, active_profile).await; - } - None => { - eprintln!( - "warning: active profile '{profile_name}' not found under {}; skipping", - profiles_dir.display() - ); - } + match (active_profile_name, active_profile) { + (Some(_), Some(active_profile)) => { + profile::load_into_engine(&engine_tx, &active_profile).await; } + (Some(profile_name), None) => { + eprintln!( + "warning: active profile '{profile_name}' not found in {}; skipping", + settings_path.display() + ); + } + (None, _) => {} } } diff --git a/src/profile.rs b/src/profile.rs index 6096014..ce00db3 100644 --- a/src/profile.rs +++ b/src/profile.rs @@ -1,5 +1,8 @@ -//! Named, persistable bundles of parsing/indexing rules and dashboards for a CLI app, persisted as a single `profiles.json`. -//! `Settings.profiles` only tracks lightweight name/active pointers; this module owns the actual content. +//! Named, persistable bundles of parsing/indexing rules and dashboards for a CLI app. +//! Ticket 049 folded profile storage inline into `Settings` (`settings.json`) instead of a separate +//! `profiles.json`; this module still owns the `Profile`/`Dashboard`/declaration data model, plus the +//! CRUD methods on `Settings` (`impl Settings` below `ProfileError`) that used to live on the +//! now-removed `ProfileStore`. #![allow(dead_code)] use std::collections::hash_map::DefaultHasher; @@ -8,7 +11,7 @@ use std::fmt; use std::fs; use std::hash::{Hash, Hasher}; use std::io; -use std::path::{Path, PathBuf}; +use std::path::Path; use std::time::{SystemTime, UNIX_EPOCH}; use serde::{Deserialize, Serialize}; @@ -18,7 +21,7 @@ use crate::engine::{DeclareMetricRequest, DeclareParamRequest, EngineHandle, Eng use crate::settings::Settings; use tokio::sync::oneshot; -/// Name of the profile that always exists and can't be deleted (see `ProfileStore::ensure_default`). +/// Name of the profile that always exists and can't be deleted (see `Settings::ensure_default_profile`). pub const DEFAULT_PROFILE_NAME: &str = "Default"; /// Mirrors `data_value::MetricPrimitiveKind` with `Serialize`/`Deserialize` so profiles can round-trip through JSON, since the engine-internal type only derives `Serialize`. @@ -442,7 +445,7 @@ pub async fn load_into_engine(engine: &EngineHandle, profile: &Profile) { } } -/// Error returned by `ProfileStore` mutations and lookups. +/// Error returned by `Settings`'s profile-related mutations and lookups. #[derive(Debug)] pub enum ProfileError { /// No profile with the given name exists. @@ -474,198 +477,135 @@ impl From for ProfileError { } } -/// Resolves `settings.profiles.profiles_dir` relative to `settings_path`'s parent directory (or as absolute if it already is one). -pub fn resolve_profiles_dir(settings: &Settings, settings_path: &Path) -> PathBuf { - let profiles_dir = Path::new(&settings.profiles.profiles_dir); - if profiles_dir.is_absolute() { - profiles_dir.to_path_buf() - } else { - settings_path - .parent() - .map(|parent| parent.join(profiles_dir)) - .unwrap_or_else(|| profiles_dir.to_path_buf()) +/// Reads a legacy pre-049 `profiles.json` file (back when profiles lived in a separate file rather +/// than inline in `settings.json`) for `Settings::migrate_legacy_profiles_file`. Returns `None` if +/// the file doesn't exist or can't be read/parsed — treated as "nothing to migrate" rather than +/// failing settings load. +pub(crate) fn load_legacy_profile_store(path: &Path) -> Option> { + let contents = fs::read_to_string(path).ok()?; + match serde_json::from_str::>(&contents) { + Ok(profiles) => Some(profiles), + Err(err) => { + eprintln!( + "warning: failed to parse legacy profile store at {}: {err}; skipping migration", + path.display() + ); + None + } } } -/// Path to the single file holding all profiles, within `profiles_dir`. -pub fn profiles_store_path(profiles_dir: &Path) -> PathBuf { - profiles_dir.join("profiles.json") +/// Turns any pre-unification/pre-explicit-duration metric or param shapes still present in +/// `profiles` into their current form; used by `Settings::load` on every startup (previously done +/// by the now-removed `ProfileStore::load`, before ticket 049 folded profile storage into +/// `settings.json`). Returns whether anything changed, so the caller can decide whether to +/// re-persist. +/// +/// Ticket 037: turn any pre-unification inline metric matchers into standalone param references. +/// Deterministic naming makes this idempotent, so it's safe to run on every load; persisting the +/// result isn't required for correctness (migration re-runs identically next time), but it's done +/// here anyway so the on-disk file only carries the legacy `"params"` shape once, on the very next +/// save. +/// +/// Ticket 038: follows on immediately after — once `migrate_legacy_metric_params` has resolved +/// every metric's `value_param`, convert any `Duration`-kind param still referenced that way into +/// the explicit `elapsed_since_last` extractor, so a profile saved either before 037 (inline +/// matchers) or between 037 and 038 (bare `value_param` reference) ends up behaving identically +/// under the new, explicit shape. +pub(crate) fn migrate_legacy_profile_shapes(profiles: &mut [Profile]) -> bool { + let mut any_migrated = false; + for profile in profiles { + let legacy_migrated = migrate_legacy_metric_params(profile); + let duration_migrated = migrate_duration_value_params(profile); + if legacy_migrated || duration_migrated { + any_migrated = true; + } + } + any_migrated } -/// On-disk collection of all known profiles, persisted as a single JSON file. -#[derive(Debug, Clone, Default)] -pub struct ProfileStore { - profiles: Vec, -} - -impl ProfileStore { - /// Loads the store from `store_path`; a missing or unreadable/unparsable file is treated as an empty store rather than failing startup. - pub fn load(store_path: &Path) -> ProfileStore { - let mut store = match fs::read_to_string(store_path) { - Ok(contents) => match serde_json::from_str::>(&contents) { - Ok(profiles) => ProfileStore { profiles }, - Err(err) => { - eprintln!( - "warning: failed to parse profile store at {}: {err}; using empty store", - store_path.display() - ); - ProfileStore::default() - } - }, - Err(err) if err.kind() == io::ErrorKind::NotFound => ProfileStore::default(), - Err(err) => { - eprintln!( - "warning: failed to read profile store at {}: {err}; using empty store", - store_path.display() - ); - ProfileStore::default() - } - }; - - // Ticket 037: turn any pre-unification inline metric matchers into standalone param - // references. Deterministic naming makes this idempotent, so it's safe to run on every - // load; persisting the result isn't required for correctness (migration re-runs - // identically next time), but it's done here anyway so the on-disk file only carries the - // legacy `"params"` shape once, on the very next save. - // - // Ticket 038: follows on immediately after — once `migrate_legacy_metric_params` has - // resolved every metric's `value_param`, convert any `Duration`-kind param still referenced - // that way into the explicit `elapsed_since_last` extractor, so a profile saved either before - // 037 (inline matchers) or between 037 and 038 (bare `value_param` reference) ends up - // behaving identically under the new, explicit shape. - let mut any_migrated = false; - for profile in &mut store.profiles { - let legacy_migrated = migrate_legacy_metric_params(profile); - let duration_migrated = migrate_duration_value_params(profile); - if legacy_migrated || duration_migrated { - any_migrated = true; - } - } - if any_migrated { - if let Err(err) = store.save(store_path) { - eprintln!( - "warning: failed to persist migrated profile store at {}: {err}", - store_path.display() - ); - } - } - - store - } - - /// Persists the store to `store_path` as pretty-printed JSON, creating any missing parent directories first. - pub fn save(&self, store_path: &Path) -> io::Result<()> { - if let Some(parent) = store_path.parent() { - fs::create_dir_all(parent)?; - } - let contents = serde_json::to_string_pretty(&self.profiles) - .map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err))?; - fs::write(store_path, contents) - } - - pub fn list(&self) -> &[Profile] { - &self.profiles - } - - pub fn get(&self, name: &str) -> Option<&Profile> { +/// Profile CRUD, previously on the standalone `ProfileStore` (removed by ticket 049): every mutation +/// here reads/writes `self.profiles` directly and persists via `Settings::save`, so `settings.json` is +/// the single source of truth instead of a separate file kept in sync alongside it. +impl Settings { + pub fn profile(&self, name: &str) -> Option<&Profile> { self.profiles.iter().find(|p| p.name == name) } - /// Creates a new profile named `name` (persisting store and settings), copying the active profile's params/metrics/dashboards as a starting point rather than starting blank. Fails with `DuplicateName` if the name exists. - pub fn create( - &mut self, - store_path: &Path, - settings: &mut Settings, - settings_path: &Path, - name: String, - ) -> Result<(), ProfileError> { - if self.get(&name).is_some() { + /// Creates a new profile named `name`, persisting settings, copying the active profile's + /// params/metrics/dashboards as a starting point rather than starting blank. Fails with + /// `DuplicateName` if the name exists. + pub fn create_profile(&mut self, settings_path: &Path, name: String) -> Result<(), ProfileError> { + if self.profile(&name).is_some() { return Err(ProfileError::DuplicateName); } let mut profile = Profile::new(name.clone()); - if let Some(active) = settings.profiles.active_profile.as_deref().and_then(|n| self.get(n)) { + if let Some(active) = self.active_profile.as_deref().and_then(|n| self.profile(n)) { profile.params = active.params.clone(); profile.metrics = active.metrics.clone(); profile.dashboards = active.dashboards.clone(); } self.profiles.push(profile); - self.save(store_path)?; - - if !settings.profiles.names.contains(&name) { - settings.profiles.names.push(name); - } - settings.save(settings_path)?; + self.save(settings_path)?; Ok(()) } - /// Deletes the profile named `name` (persisting store and settings), clearing `active_profile` if it was active. Fails with `NotFound`, or `CannotDeleteDefault` for the `Default` profile. - pub fn delete( - &mut self, - store_path: &Path, - settings: &mut Settings, - settings_path: &Path, - name: &str, - ) -> Result<(), ProfileError> { + /// Deletes the profile named `name`, persisting settings, clearing `active_profile` if it was + /// active. Fails with `NotFound`, or `CannotDeleteDefault` for the `Default` profile. + pub fn delete_profile(&mut self, settings_path: &Path, name: &str) -> Result<(), ProfileError> { if name == DEFAULT_PROFILE_NAME { return Err(ProfileError::CannotDeleteDefault); } let index = self.profiles.iter().position(|p| p.name == name).ok_or(ProfileError::NotFound)?; self.profiles.remove(index); - self.save(store_path)?; - - settings.profiles.names.retain(|n| n != name); - if settings.profiles.active_profile.as_deref() == Some(name) { - settings.profiles.active_profile = None; + if self.active_profile.as_deref() == Some(name) { + self.active_profile = None; } - settings.save(settings_path)?; + self.save(settings_path)?; Ok(()) } - /// Ensures a `Default` profile exists (seeding a starter dashboard/metric only on first creation), is tracked in `settings.profiles.names`, and falls back to active if nothing else is. Called once on startup before anything else touches profiles. - pub fn ensure_default( - &mut self, - store_path: &Path, - settings: &mut Settings, - settings_path: &Path, - ) -> Result<(), ProfileError> { - if self.get(DEFAULT_PROFILE_NAME).is_none() { + /// Ensures a `Default` profile exists (seeding a starter dashboard/metric only on first + /// creation), and falls back to active if nothing else is. Called once on startup before anything + /// else touches profiles. + pub fn ensure_default_profile(&mut self, settings_path: &Path) -> Result<(), ProfileError> { + if self.profile(DEFAULT_PROFILE_NAME).is_none() { let mut profile = Profile::new(DEFAULT_PROFILE_NAME.to_string()); profile.metrics.push(seeded_error_split_metric()); profile.dashboards.push(seeded_default_dashboard()); self.profiles.push(profile); - self.save(store_path)?; } - if !settings.profiles.names.iter().any(|n| n == DEFAULT_PROFILE_NAME) { - settings.profiles.names.push(DEFAULT_PROFILE_NAME.to_string()); + if self.active_profile.is_none() { + self.active_profile = Some(DEFAULT_PROFILE_NAME.to_string()); } - if settings.profiles.active_profile.is_none() { - settings.profiles.active_profile = Some(DEFAULT_PROFILE_NAME.to_string()); - } - settings.save(settings_path)?; + self.save(settings_path)?; Ok(()) } - /// Sets (or clears, with `None`) the active profile in `settings`. Fails with `NotFound` if `name` is `Some` and no such profile exists. Doesn't itself reload declarations into a running engine — that happens on next startup via `load_into_engine`. - pub fn set_active( - &self, - settings: &mut Settings, + /// Sets (or clears, with `None`) the active profile. Fails with `NotFound` if `name` is `Some` + /// and no such profile exists. Doesn't itself reload declarations into a running engine — that + /// happens on next startup via `load_into_engine`. + pub fn set_active_profile( + &mut self, settings_path: &Path, name: Option, ) -> Result<(), ProfileError> { if let Some(name) = &name { - if self.get(name).is_none() { + if self.profile(name).is_none() { return Err(ProfileError::NotFound); } } - settings.profiles.active_profile = name; - settings.save(settings_path)?; + self.active_profile = name; + self.save(settings_path)?; Ok(()) } - /// Records/updates a declared param into `profile_name`'s stored declarations (matched by param name), persisting the store. Intended to be called by the HTTP API alongside declaring against the engine, so declarations survive a restart. + /// Records/updates a declared param into `profile_name`'s stored declarations (matched by param + /// name), persisting settings. Intended to be called by the HTTP API alongside declaring against + /// the engine, so declarations survive a restart. pub fn record_param_declaration( &mut self, - store_path: &Path, + settings_path: &Path, profile_name: &str, request: &DeclareParamRequest, ) -> Result<(), ProfileError> { @@ -679,11 +619,12 @@ impl ProfileStore { Some(existing) => *existing = record, None => profile.params.push(record), } - self.save(store_path)?; + self.save(settings_path)?; Ok(()) } - /// Records/updates a declared metric into `profile_name`'s stored declarations (matched by metric name), persisting the store; same integration intent as `record_param_declaration`. + /// Records/updates a declared metric into `profile_name`'s stored declarations (matched by metric + /// name), persisting settings; same integration intent as `record_param_declaration`. /// /// `promoted_params` is any of the metric's referenced `label_params`/`value_param` that live /// only as auto-derived engine state (see `crate::engine::auto_declare_params`) rather than a @@ -695,7 +636,7 @@ impl ProfileStore { /// param is never clobbered. pub fn record_metric_declaration( &mut self, - store_path: &Path, + settings_path: &Path, profile_name: &str, request: &DeclareMetricRequest, promoted_params: &[ParamDeclarationRecord], @@ -715,14 +656,17 @@ impl ProfileStore { Some(existing) => *existing = record, None => profile.metrics.push(record), } - self.save(store_path)?; + self.save(settings_path)?; Ok(()) } - /// Same as `record_metric_declaration`, for an in-place edit that may rename the metric: drops any persisted record under `old_name` first (when it differs from the new name) so a rename doesn't leave a stale duplicate declaration behind, then upserts by the new name as usual. `promoted_params` has the same meaning as in `record_metric_declaration`. + /// Same as `record_metric_declaration`, for an in-place edit that may rename the metric: drops + /// any persisted record under `old_name` first (when it differs from the new name) so a rename + /// doesn't leave a stale duplicate declaration behind, then upserts by the new name as usual. + /// `promoted_params` has the same meaning as in `record_metric_declaration`. pub fn record_metric_declaration_update( &mut self, - store_path: &Path, + settings_path: &Path, profile_name: &str, old_name: &str, request: &DeclareMetricRequest, @@ -746,20 +690,21 @@ impl ProfileStore { Some(existing) => *existing = record, None => profile.metrics.push(record), } - self.save(store_path)?; + self.save(settings_path)?; Ok(()) } /// Lists the dashboards saved on `profile_name`; fails with `NotFound` if it doesn't exist. pub fn list_dashboards(&self, profile_name: &str) -> Result<&[Dashboard], ProfileError> { - let profile = self.get(profile_name).ok_or(ProfileError::NotFound)?; + let profile = self.profile(profile_name).ok_or(ProfileError::NotFound)?; Ok(&profile.dashboards) } - /// Creates a new, empty dashboard named `name` on `profile_name`, persisting the store; fails with `NotFound` if the profile doesn't exist. + /// Creates a new, empty dashboard named `name` on `profile_name`, persisting settings; fails + /// with `NotFound` if the profile doesn't exist. pub fn create_dashboard( &mut self, - store_path: &Path, + settings_path: &Path, profile_name: &str, name: String, ) -> Result { @@ -770,14 +715,16 @@ impl ProfileStore { .ok_or(ProfileError::NotFound)?; let dashboard = Dashboard::new(name); profile.dashboards.push(dashboard.clone()); - self.save(store_path)?; + self.save(settings_path)?; Ok(dashboard) } - /// Replaces the dashboard identified by `dashboard_id` with `dashboard`, persisting the store; `dashboard.id` is forced to `dashboard_id` so a caller can't change a dashboard's identity through this call. + /// Replaces the dashboard identified by `dashboard_id` with `dashboard`, persisting settings; + /// `dashboard.id` is forced to `dashboard_id` so a caller can't change a dashboard's identity + /// through this call. pub fn save_dashboard( &mut self, - store_path: &Path, + settings_path: &Path, profile_name: &str, dashboard_id: &str, mut dashboard: Dashboard, @@ -794,14 +741,15 @@ impl ProfileStore { .ok_or(ProfileError::NotFound)?; dashboard.id = dashboard_id.to_string(); *existing = dashboard.clone(); - self.save(store_path)?; + self.save(settings_path)?; Ok(dashboard) } - /// Deletes the dashboard identified by `dashboard_id` from `profile_name`, persisting the store; fails with `NotFound` if either doesn't exist. + /// Deletes the dashboard identified by `dashboard_id` from `profile_name`, persisting settings; + /// fails with `NotFound` if either doesn't exist. pub fn delete_dashboard( &mut self, - store_path: &Path, + settings_path: &Path, profile_name: &str, dashboard_id: &str, ) -> Result<(), ProfileError> { @@ -816,7 +764,7 @@ impl ProfileStore { .position(|d| d.id == dashboard_id) .ok_or(ProfileError::NotFound)?; profile.dashboards.remove(index); - self.save(store_path)?; + self.save(settings_path)?; Ok(()) } } @@ -859,92 +807,64 @@ mod tests { response.await.unwrap() } - #[test] - fn resolve_profiles_dir_is_relative_to_settings_parent() { - let settings_path = Path::new("/config/ottyl/settings.json"); - let mut settings = Settings::default(); - settings.profiles.profiles_dir = "profiles".to_string(); - - let dir = resolve_profiles_dir(&settings, settings_path); - assert_eq!(dir, Path::new("/config/ottyl/profiles")); - } - - #[test] - fn resolve_profiles_dir_respects_absolute_path() { - let settings_path = Path::new("/config/ottyl/settings.json"); - let mut settings = Settings::default(); - settings.profiles.profiles_dir = "/elsewhere/profiles".to_string(); - - let dir = resolve_profiles_dir(&settings, settings_path); - assert_eq!(dir, Path::new("/elsewhere/profiles")); - } - #[test] fn create_list_get_delete_round_trip() { let dir = tempfile::tempdir().unwrap(); - let store_path = dir.path().join("profiles.json"); let settings_path = dir.path().join("settings.json"); let mut settings = Settings::default(); - let mut store = ProfileStore::load(&store_path); - store.create(&store_path, &mut settings, &settings_path, "web".to_string()).unwrap(); - store.create(&store_path, &mut settings, &settings_path, "worker".to_string()).unwrap(); + settings.create_profile(&settings_path, "web".to_string()).unwrap(); + settings.create_profile(&settings_path, "worker".to_string()).unwrap(); - assert_eq!(store.list().len(), 2); - assert!(store.get("web").is_some()); - assert_eq!(settings.profiles.names, vec!["web".to_string(), "worker".to_string()]); + assert_eq!(settings.profiles.len(), 2); + assert!(settings.profile("web").is_some()); - let dup = store.create(&store_path, &mut settings, &settings_path, "web".to_string()); + let dup = settings.create_profile(&settings_path, "web".to_string()); assert!(matches!(dup, Err(ProfileError::DuplicateName))); - // Reload from disk: profiles and settings both persisted. - let reloaded = ProfileStore::load(&store_path); - assert_eq!(reloaded.list().len(), 2); + // Reload from disk: profiles persisted. let reloaded_settings = Settings::load(&settings_path); - assert_eq!(reloaded_settings.profiles.names, settings.profiles.names); + assert_eq!(reloaded_settings.profiles.len(), 2); + assert!(reloaded_settings.profile("web").is_some()); + assert!(reloaded_settings.profile("worker").is_some()); - store.delete(&store_path, &mut settings, &settings_path, "worker").unwrap(); - assert!(store.get("worker").is_none()); - assert_eq!(settings.profiles.names, vec!["web".to_string()]); + settings.delete_profile(&settings_path, "worker").unwrap(); + assert!(settings.profile("worker").is_none()); + assert_eq!(settings.profiles.len(), 1); - let missing = store.delete(&store_path, &mut settings, &settings_path, "worker"); + let missing = settings.delete_profile(&settings_path, "worker"); assert!(matches!(missing, Err(ProfileError::NotFound))); } #[test] fn ensure_default_creates_default_and_activates_it_once() { let dir = tempfile::tempdir().unwrap(); - let store_path = dir.path().join("profiles.json"); let settings_path = dir.path().join("settings.json"); let mut settings = Settings::default(); - let mut store = ProfileStore::load(&store_path); - store.ensure_default(&store_path, &mut settings, &settings_path).unwrap(); + settings.ensure_default_profile(&settings_path).unwrap(); - assert!(store.get(DEFAULT_PROFILE_NAME).is_some()); - assert!(settings.profiles.names.contains(&DEFAULT_PROFILE_NAME.to_string())); - assert_eq!(settings.profiles.active_profile, Some(DEFAULT_PROFILE_NAME.to_string())); + assert!(settings.profile(DEFAULT_PROFILE_NAME).is_some()); + assert_eq!(settings.active_profile, Some(DEFAULT_PROFILE_NAME.to_string())); // Idempotent: doesn't duplicate the profile or clobber an already-active (non-Default) profile. - store.create(&store_path, &mut settings, &settings_path, "web".to_string()).unwrap(); - store.set_active(&mut settings, &settings_path, Some("web".to_string())).unwrap(); - store.ensure_default(&store_path, &mut settings, &settings_path).unwrap(); + settings.create_profile(&settings_path, "web".to_string()).unwrap(); + settings.set_active_profile(&settings_path, Some("web".to_string())).unwrap(); + settings.ensure_default_profile(&settings_path).unwrap(); - assert_eq!(store.list().iter().filter(|p| p.name == DEFAULT_PROFILE_NAME).count(), 1); - assert_eq!(settings.profiles.active_profile, Some("web".to_string())); + assert_eq!(settings.profiles.iter().filter(|p| p.name == DEFAULT_PROFILE_NAME).count(), 1); + assert_eq!(settings.active_profile, Some("web".to_string())); } #[test] fn ensure_default_seeds_starter_dashboard_only_on_creation() { let dir = tempfile::tempdir().unwrap(); - let store_path = dir.path().join("profiles.json"); let settings_path = dir.path().join("settings.json"); let mut settings = Settings::default(); - let mut store = ProfileStore::load(&store_path); - store.ensure_default(&store_path, &mut settings, &settings_path).unwrap(); + settings.ensure_default_profile(&settings_path).unwrap(); - let default_profile = store.get(DEFAULT_PROFILE_NAME).unwrap(); + let default_profile = settings.profile(DEFAULT_PROFILE_NAME).unwrap(); assert_eq!(default_profile.metrics.len(), 1); assert_eq!(default_profile.metrics[0].name, SEEDED_METRIC_NAME); assert_eq!( @@ -965,9 +885,9 @@ mod tests { assert!(dashboard.panels.iter().any(|p| p.chart_type == ChartType::Pie && p.promql == SEEDED_METRIC_NAME)); // Reload (simulating a restart) and ensure_default again: must not re-seed or duplicate. - let mut store = ProfileStore::load(&store_path); - store.ensure_default(&store_path, &mut settings, &settings_path).unwrap(); - let default_profile = store.get(DEFAULT_PROFILE_NAME).unwrap(); + let mut reloaded = Settings::load(&settings_path); + reloaded.ensure_default_profile(&settings_path).unwrap(); + let default_profile = reloaded.profile(DEFAULT_PROFILE_NAME).unwrap(); assert_eq!(default_profile.metrics.len(), 1); assert_eq!(default_profile.dashboards.len(), 1); } @@ -979,12 +899,10 @@ mod tests { use tokio::sync::mpsc; let dir = tempfile::tempdir().unwrap(); - let store_path = dir.path().join("profiles.json"); let settings_path = dir.path().join("settings.json"); let mut settings = Settings::default(); - let mut store = ProfileStore::load(&store_path); - store.ensure_default(&store_path, &mut settings, &settings_path).unwrap(); - let default_profile = store.get(DEFAULT_PROFILE_NAME).unwrap().clone(); + settings.ensure_default_profile(&settings_path).unwrap(); + let default_profile = settings.profile(DEFAULT_PROFILE_NAME).unwrap().clone(); let (engine_tx, engine_rx) = mpsc::channel(128); let settings_handle = std::sync::Arc::new(std::sync::Mutex::new(settings.clone())); @@ -1022,41 +940,37 @@ mod tests { #[test] fn default_profile_cannot_be_deleted() { let dir = tempfile::tempdir().unwrap(); - let store_path = dir.path().join("profiles.json"); let settings_path = dir.path().join("settings.json"); let mut settings = Settings::default(); - let mut store = ProfileStore::load(&store_path); - store.ensure_default(&store_path, &mut settings, &settings_path).unwrap(); + settings.ensure_default_profile(&settings_path).unwrap(); - let err = store.delete(&store_path, &mut settings, &settings_path, DEFAULT_PROFILE_NAME); + let err = settings.delete_profile(&settings_path, DEFAULT_PROFILE_NAME); assert!(matches!(err, Err(ProfileError::CannotDeleteDefault))); - assert!(store.get(DEFAULT_PROFILE_NAME).is_some()); + assert!(settings.profile(DEFAULT_PROFILE_NAME).is_some()); } #[test] fn create_copies_the_active_profile() { let dir = tempfile::tempdir().unwrap(); - let store_path = dir.path().join("profiles.json"); let settings_path = dir.path().join("settings.json"); let mut settings = Settings::default(); - let mut store = ProfileStore::load(&store_path); - store.create(&store_path, &mut settings, &settings_path, "web".to_string()).unwrap(); + settings.create_profile(&settings_path, "web".to_string()).unwrap(); let param_req = param_request("level", r"level=(?P\w+)", "level"); - store.record_param_declaration(&store_path, "web", ¶m_req).unwrap(); - store.create_dashboard(&store_path, "web", "Overview".to_string()).unwrap(); - store.set_active(&mut settings, &settings_path, Some("web".to_string())).unwrap(); + settings.record_param_declaration(&settings_path, "web", ¶m_req).unwrap(); + settings.create_dashboard(&settings_path, "web", "Overview".to_string()).unwrap(); + settings.set_active_profile(&settings_path, Some("web".to_string())).unwrap(); - store.create(&store_path, &mut settings, &settings_path, "web-copy".to_string()).unwrap(); - let copy = store.get("web-copy").unwrap(); - assert_eq!(copy.params, store.get("web").unwrap().params); - assert_eq!(copy.dashboards, store.get("web").unwrap().dashboards); + settings.create_profile(&settings_path, "web-copy".to_string()).unwrap(); + let copy = settings.profile("web-copy").unwrap().clone(); + assert_eq!(copy.params, settings.profile("web").unwrap().params); + assert_eq!(copy.dashboards, settings.profile("web").unwrap().dashboards); // With no active profile, new profiles start empty as before. - store.set_active(&mut settings, &settings_path, None).unwrap(); - store.create(&store_path, &mut settings, &settings_path, "blank".to_string()).unwrap(); - let blank = store.get("blank").unwrap(); + settings.set_active_profile(&settings_path, None).unwrap(); + settings.create_profile(&settings_path, "blank".to_string()).unwrap(); + let blank = settings.profile("blank").unwrap(); assert!(blank.params.is_empty()); assert!(blank.dashboards.is_empty()); } @@ -1064,49 +978,45 @@ mod tests { #[test] fn set_active_updates_and_persists_settings() { let dir = tempfile::tempdir().unwrap(); - let store_path = dir.path().join("profiles.json"); let settings_path = dir.path().join("settings.json"); let mut settings = Settings::default(); - let mut store = ProfileStore::load(&store_path); - store.create(&store_path, &mut settings, &settings_path, "web".to_string()).unwrap(); - store.create(&store_path, &mut settings, &settings_path, "worker".to_string()).unwrap(); + settings.create_profile(&settings_path, "web".to_string()).unwrap(); + settings.create_profile(&settings_path, "worker".to_string()).unwrap(); - store.set_active(&mut settings, &settings_path, Some("web".to_string())).unwrap(); - assert_eq!(settings.profiles.active_profile, Some("web".to_string())); + settings.set_active_profile(&settings_path, Some("web".to_string())).unwrap(); + assert_eq!(settings.active_profile, Some("web".to_string())); // Switching to another known profile works and persists. - store.set_active(&mut settings, &settings_path, Some("worker".to_string())).unwrap(); + settings.set_active_profile(&settings_path, Some("worker".to_string())).unwrap(); let reloaded = Settings::load(&settings_path); - assert_eq!(reloaded.profiles.active_profile, Some("worker".to_string())); + assert_eq!(reloaded.active_profile, Some("worker".to_string())); // Setting an unknown profile active is rejected, leaving the previous value. - let err = store.set_active(&mut settings, &settings_path, Some("ghost".to_string())); + let err = settings.set_active_profile(&settings_path, Some("ghost".to_string())); assert!(matches!(err, Err(ProfileError::NotFound))); - assert_eq!(settings.profiles.active_profile, Some("worker".to_string())); + assert_eq!(settings.active_profile, Some("worker".to_string())); // Clearing the active profile works. - store.set_active(&mut settings, &settings_path, None).unwrap(); - assert_eq!(settings.profiles.active_profile, None); + settings.set_active_profile(&settings_path, None).unwrap(); + assert_eq!(settings.active_profile, None); } #[test] fn record_param_and_metric_declarations_persist_into_profile() { let dir = tempfile::tempdir().unwrap(); - let store_path = dir.path().join("profiles.json"); let settings_path = dir.path().join("settings.json"); let mut settings = Settings::default(); - let mut store = ProfileStore::load(&store_path); - store.create(&store_path, &mut settings, &settings_path, "web".to_string()).unwrap(); + settings.create_profile(&settings_path, "web".to_string()).unwrap(); let param_req = param_request("level", r"level=(?P\w+)", "level"); - store.record_param_declaration(&store_path, "web", ¶m_req).unwrap(); + settings.record_param_declaration(&settings_path, "web", ¶m_req).unwrap(); let metric_req = metric_request("requests", r"duration=(?P\d+)", "duration"); - store.record_metric_declaration(&store_path, "web", &metric_req, &[]).unwrap(); + settings.record_metric_declaration(&settings_path, "web", &metric_req, &[]).unwrap(); - let profile = store.get("web").unwrap(); + let profile = settings.profile("web").unwrap(); assert_eq!(profile.params.len(), 1); assert_eq!(profile.params[0].name, "level"); assert_eq!(profile.metrics.len(), 1); @@ -1114,62 +1024,58 @@ mod tests { // Re-declaring the same name updates in place rather than duplicating. let param_req_v2 = param_request("level", r"lvl=(?P\w+)", "level"); - store.record_param_declaration(&store_path, "web", ¶m_req_v2).unwrap(); - let profile = store.get("web").unwrap(); + settings.record_param_declaration(&settings_path, "web", ¶m_req_v2).unwrap(); + let profile = settings.profile("web").unwrap(); assert_eq!(profile.params.len(), 1); assert_eq!(profile.params[0].message_match_regex, r"lvl=(?P\w+)"); // Reload from disk: recorded declarations survive a restart. - let reloaded = ProfileStore::load(&store_path); - let profile = reloaded.get("web").unwrap(); + let reloaded = Settings::load(&settings_path); + let profile = reloaded.profile("web").unwrap(); assert_eq!(profile.params.len(), 1); assert_eq!(profile.metrics.len(), 1); } - /// Acceptance criterion: declaring params/metrics, "restarting" (reloading the store from disk), and replaying into a fresh engine reproduces the same declarations and still ingests lines, without re-declaring by hand. + /// Acceptance criterion: declaring params/metrics, "restarting" (reloading settings from disk), and replaying into a fresh engine reproduces the same declarations and still ingests lines, without re-declaring by hand. #[tokio::test] async fn profile_round_trip_reproduces_declarations_in_fresh_engine() { let dir = tempfile::tempdir().unwrap(); - let store_path = dir.path().join("profiles.json"); let settings_path = dir.path().join("settings.json"); let mut settings = Settings::default(); // Declare directly into a profile, as the HTTP API will do via the `record_*_declaration` hooks. - let mut store = ProfileStore::load(&store_path); - store.create(&store_path, &mut settings, &settings_path, "web".to_string()).unwrap(); - store + settings.create_profile(&settings_path, "web".to_string()).unwrap(); + settings .record_param_declaration( - &store_path, + &settings_path, "web", ¶m_request("level", r"level=(?P\w+)", "level"), ) .unwrap(); // A metric now references params by name (ticket 037), so "duration" must be declared as a // standalone param before the metric below can reference it as a label param. - store + settings .record_param_declaration( - &store_path, + &settings_path, "web", ¶m_request("duration", r"request duration=(?P\d+)", "duration"), ) .unwrap(); - store + settings .record_metric_declaration( - &store_path, + &settings_path, "web", &metric_request("requests", r"request duration=(?P\d+)", "duration"), &[], ) .unwrap(); - store.set_active(&mut settings, &settings_path, Some("web".to_string())).unwrap(); + settings.set_active_profile(&settings_path, Some("web".to_string())).unwrap(); // Simulate a restart: drop everything in memory, reload from disk. - drop(store); drop(settings); let settings = Settings::load(&settings_path); - let active_name = settings.profiles.active_profile.clone().unwrap(); - let reloaded_store = ProfileStore::load(&store_path); - let profile = reloaded_store.get(&active_name).unwrap().clone(); + let active_name = settings.active_profile.clone().unwrap(); + let profile = settings.profile(&active_name).unwrap().clone(); // Spawn a fresh engine and replay the profile's declarations into it. let (engine_tx, engine_rx) = mpsc::channel(16); @@ -1225,18 +1131,16 @@ mod tests { assert_eq!(logs[0].text, "level=info request duration=42"); } - /// Acceptance criterion: a dashboard (with a panel) survives reloading the profile store from disk. + /// Acceptance criterion: a dashboard (with a panel) survives reloading settings from disk. #[test] fn dashboard_create_save_delete_round_trip_and_persists_across_reload() { let dir = tempfile::tempdir().unwrap(); - let store_path = dir.path().join("profiles.json"); let settings_path = dir.path().join("settings.json"); let mut settings = Settings::default(); - let mut store = ProfileStore::load(&store_path); - store.create(&store_path, &mut settings, &settings_path, "web".to_string()).unwrap(); + settings.create_profile(&settings_path, "web".to_string()).unwrap(); - let dashboard = store.create_dashboard(&store_path, "web", "Overview".to_string()).unwrap(); + let dashboard = settings.create_dashboard(&settings_path, "web", "Overview".to_string()).unwrap(); assert_eq!(dashboard.name, "Overview"); assert!(dashboard.panels.is_empty()); @@ -1247,15 +1151,15 @@ mod tests { promql: r#"requests{path="/a"}"#.to_string(), chart_type: ChartType::Line, }); - let saved = store - .save_dashboard(&store_path, "web", &dashboard.id, with_panel) + let saved = settings + .save_dashboard(&settings_path, "web", &dashboard.id, with_panel) .unwrap(); assert_eq!(saved.panels.len(), 1); assert_eq!(saved.id, dashboard.id); // Simulate a restart: drop and reload from disk. - drop(store); - let reloaded = ProfileStore::load(&store_path); + drop(settings); + let mut reloaded = Settings::load(&settings_path); let dashboards = reloaded.list_dashboards("web").unwrap(); assert_eq!(dashboards.len(), 1); assert_eq!(dashboards[0].name, "Overview"); @@ -1263,10 +1167,9 @@ mod tests { assert_eq!(dashboards[0].panels[0].promql, r#"requests{path="/a"}"#); // Deleting removes it, and that too persists. - let mut reloaded = reloaded; - reloaded.delete_dashboard(&store_path, "web", &dashboard.id).unwrap(); + reloaded.delete_dashboard(&settings_path, "web", &dashboard.id).unwrap(); assert!(reloaded.list_dashboards("web").unwrap().is_empty()); - let reloaded_again = ProfileStore::load(&store_path); + let reloaded_again = Settings::load(&settings_path); assert!(reloaded_again.list_dashboards("web").unwrap().is_empty()); // Unknown profile/dashboard both fail with NotFound. @@ -1276,12 +1179,16 @@ mod tests { )); } - /// Writes a pre-037 `profiles.json` (metrics carrying inline `"params"` matchers rather than - /// `label_params`/`value_param`) directly to disk, bypassing `ProfileStore`/`Profile` entirely - /// so this doesn't accidentally exercise the current (post-migration) shape. - fn write_legacy_profiles_json(store_path: &Path, contents: &str) { - fs::create_dir_all(store_path.parent().unwrap()).unwrap(); - fs::write(store_path, contents).unwrap(); + /// Writes a pre-037 `settings.json` (with an inline `profiles` array whose metrics carry legacy + /// `"params"` matchers rather than `label_params`/`value_param`) directly to disk, bypassing + /// `Settings`/`Profile` entirely so this doesn't accidentally exercise the current + /// (post-migration) shape. `profiles_json` is the raw JSON array of profiles. + fn write_legacy_settings_json(settings_path: &Path, profiles_json: &str) { + fs::create_dir_all(settings_path.parent().unwrap()).unwrap(); + let contents = format!( + r#"{{"color_scheme":"system","css_entries":[],"custom_theme_id":null,"profiles":{profiles_json},"active_profile":null,"max_logs":null,"alert_rules":[],"ntfy_topic":null,"ntfy_server":null}}"# + ); + fs::write(settings_path, contents).unwrap(); } /// Acceptance criterion: a profile saved before ticket 037 (inline `param_name`/`capture_group`/ @@ -1290,9 +1197,9 @@ mod tests { #[test] fn migrates_legacy_inline_metric_params_into_standalone_declarations() { let dir = tempfile::tempdir().unwrap(); - let store_path = dir.path().join("profiles.json"); - write_legacy_profiles_json( - &store_path, + let settings_path = dir.path().join("settings.json"); + write_legacy_settings_json( + &settings_path, r#"[ { "name": "legacy", @@ -1312,8 +1219,8 @@ mod tests { ]"#, ); - let store = ProfileStore::load(&store_path); - let profile = store.get("legacy").unwrap(); + let settings = Settings::load(&settings_path); + let profile = settings.profile("legacy").unwrap(); // No pre-existing bare "path"/"duration" param, so implicit ones were created, named after the metric. assert_eq!(profile.params.len(), 2); @@ -1329,15 +1236,16 @@ mod tests { assert_eq!(profile.metrics[0].value_param, Some("requests.duration".to_string())); // The migration is persisted, so the on-disk file no longer carries the legacy "params" shape. - let reloaded_raw = fs::read_to_string(&store_path).unwrap(); + let reloaded_raw = fs::read_to_string(&settings_path).unwrap(); assert!(!reloaded_raw.contains("param_name")); // Replaying into a fresh engine behaves identically to before migration: the metric still ingests and buckets. + let profile = profile.clone(); let (engine_tx, engine_rx) = mpsc::channel(16); let settings_handle = std::sync::Arc::new(std::sync::Mutex::new(Settings::default())); tokio::runtime::Runtime::new().unwrap().block_on(async { tokio::spawn(crate::engine::run(engine_rx, settings_handle)); - load_into_engine(&engine_tx, profile).await; + load_into_engine(&engine_tx, &profile).await; engine_tx .send(EngineMessage::Ingest(IngestedLine { sequence: 0, @@ -1364,9 +1272,9 @@ mod tests { #[test] fn migrates_legacy_value_role_duration_matcher_into_elapsed_since_last_param() { let dir = tempfile::tempdir().unwrap(); - let store_path = dir.path().join("profiles.json"); - write_legacy_profiles_json( - &store_path, + let settings_path = dir.path().join("settings.json"); + write_legacy_settings_json( + &settings_path, r#"[ { "name": "legacy", @@ -1386,8 +1294,8 @@ mod tests { ]"#, ); - let store = ProfileStore::load(&store_path); - let profile = store.get("legacy").unwrap(); + let settings = Settings::load(&settings_path); + let profile = settings.profile("legacy").unwrap(); let gap_param = profile.params.iter().find(|p| p.name == "login_gap.gap").unwrap(); assert!(matches!(gap_param.kind, ProfileMetricPrimitiveKind::Duration)); @@ -1403,9 +1311,9 @@ mod tests { #[test] fn migrates_post_037_bare_duration_value_param_into_elapsed_since_last() { let dir = tempfile::tempdir().unwrap(); - let store_path = dir.path().join("profiles.json"); - write_legacy_profiles_json( - &store_path, + let settings_path = dir.path().join("settings.json"); + write_legacy_settings_json( + &settings_path, r#"[ { "name": "legacy", @@ -1426,8 +1334,8 @@ mod tests { ]"#, ); - let store = ProfileStore::load(&store_path); - let profile = store.get("legacy").unwrap(); + let settings = Settings::load(&settings_path); + let profile = settings.profile("legacy").unwrap(); let gap_param = profile.params.iter().find(|p| p.name == "gap").unwrap(); assert!(gap_param.elapsed_since_last); @@ -1441,9 +1349,9 @@ mod tests { #[test] fn migration_reuses_an_existing_matching_bare_named_param() { let dir = tempfile::tempdir().unwrap(); - let store_path = dir.path().join("profiles.json"); - write_legacy_profiles_json( - &store_path, + let settings_path = dir.path().join("settings.json"); + write_legacy_settings_json( + &settings_path, r#"[ { "name": "legacy", @@ -1464,8 +1372,8 @@ mod tests { ]"#, ); - let store = ProfileStore::load(&store_path); - let profile = store.get("legacy").unwrap(); + let settings = Settings::load(&settings_path); + let profile = settings.profile("legacy").unwrap(); // Reused the existing "path" declaration rather than creating "requests.path". assert_eq!(profile.params.len(), 1); @@ -1478,24 +1386,23 @@ mod tests { #[test] fn migration_is_a_noop_on_an_already_migrated_profile() { let dir = tempfile::tempdir().unwrap(); - let store_path = dir.path().join("profiles.json"); + let settings_path = dir.path().join("settings.json"); let mut settings = Settings::default(); - let mut store = ProfileStore::load(&store_path); - store.create(&store_path, &mut settings, &dir.path().join("settings.json"), "web".to_string()).unwrap(); - store + settings.create_profile(&settings_path, "web".to_string()).unwrap(); + settings .record_param_declaration( - &store_path, + &settings_path, "web", ¶m_request("path", r"path=(?P\S+)", "path"), ) .unwrap(); - store - .record_metric_declaration(&store_path, "web", &metric_request("requests", r"path=(?P\S+)", "path"), &[]) + settings + .record_metric_declaration(&settings_path, "web", &metric_request("requests", r"path=(?P\S+)", "path"), &[]) .unwrap(); - let before = store.get("web").unwrap().clone(); - let reloaded = ProfileStore::load(&store_path); - let after = reloaded.get("web").unwrap().clone(); + let before = settings.profile("web").unwrap().clone(); + let reloaded = Settings::load(&settings_path); + let after = reloaded.profile("web").unwrap().clone(); assert_eq!(before, after); } @@ -1507,13 +1414,11 @@ mod tests { #[tokio::test] async fn metric_referencing_auto_derived_param_survives_restart_via_promotion() { let dir = tempfile::tempdir().unwrap(); - let store_path = dir.path().join("profiles.json"); let settings_path = dir.path().join("settings.json"); let mut settings = Settings::default(); - let mut store = ProfileStore::load(&store_path); - store.create(&store_path, &mut settings, &settings_path, "web".to_string()).unwrap(); - store.set_active(&mut settings, &settings_path, Some("web".to_string())).unwrap(); + settings.create_profile(&settings_path, "web".to_string()).unwrap(); + settings.set_active_profile(&settings_path, Some("web".to_string())).unwrap(); // Simulate the auto-derive path: ingest a line carrying a logfmt key before any param is // manually declared, so `dx` only exists as engine-internal auto-derived state. @@ -1555,12 +1460,12 @@ mod tests { kind: dx.kind.into(), elapsed_since_last: dx.elapsed_since_last, }; - store.record_metric_declaration(&store_path, "web", &metric_req, &[promoted]).unwrap(); + settings.record_metric_declaration(&settings_path, "web", &metric_req, &[promoted]).unwrap(); // Simulate a restart: drop everything, reload from disk into a *fresh* engine with no prior ingestion. - drop(store); - let reloaded_store = ProfileStore::load(&store_path); - let profile = reloaded_store.get("web").unwrap().clone(); + drop(settings); + let reloaded_settings = Settings::load(&settings_path); + let profile = reloaded_settings.profile("web").unwrap().clone(); assert_eq!(profile.params.len(), 1); assert_eq!(profile.params[0].name, "dx"); diff --git a/src/settings.rs b/src/settings.rs index 7e1eee9..36e8169 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -1,4 +1,5 @@ -//! Process-wide settings model: colour scheme, CSS entries, and profile pointers, persisted as JSON. +//! Process-wide settings model: colour scheme, CSS entries, and profiles (ticket 049 folded +//! profile storage inline here instead of a separate `profiles.json`), persisted as JSON. use std::collections::hash_map::DefaultHasher; use std::fmt; @@ -13,6 +14,7 @@ use directories::ProjectDirs; use serde::{Deserialize, Serialize}; use crate::alerts::AlertRule; +use crate::profile::Profile; /// Shared, process-wide settings handle. Cheap to clone. pub type SettingsHandle = Arc>; @@ -51,27 +53,101 @@ pub enum CssEntry { Linked { path_or_url: String }, } -/// Tracks known profile names and which one is active; the profile files themselves live elsewhere. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default)] -pub struct Profiles { - /// Names of known profiles. - pub names: Vec, - /// Name of the currently active profile, if any. - pub active_profile: Option, - /// Directory (relative to the settings file's directory, or absolute) - /// where individual profile files are expected to live. - pub profiles_dir: String, +/// Legacy on-disk shape of the (pre-049) `Profiles` pointer struct: only names/active-profile/dir +/// pointers, with the actual profile content living in a separate `profiles.json`. Kept only so an +/// old `settings.json` still parses; see `ProfilesOnDisk` and `Settings::migrate_legacy_profiles_file`. +#[derive(Debug, Clone, Deserialize, Default)] +struct LegacyProfilesPointer { + // `names` is intentionally not captured: it was always redundant with the legacy + // `profiles.json`'s actual profile list, which is what migration reads instead. + #[serde(default)] + active_profile: Option, + /// Directory (relative to the settings file's directory, or absolute) the legacy `profiles.json` + /// lived under. Every version of this codebase ever wrote `"profiles"` here (no API ever let a + /// user change it), so migration doesn't bother re-deriving a custom value from this field. + #[serde(default)] + #[allow(dead_code)] + profiles_dir: Option, +} + +/// On-disk shape of the `profiles` field: either the new inline `Vec` (ticket 049) or the +/// old pointer-struct shape, so a `settings.json` written before this ticket still deserializes. +#[derive(Debug, Clone, Deserialize)] +#[serde(untagged)] +enum ProfilesOnDisk { + Inline(Vec), + Legacy(LegacyProfilesPointer), +} + +impl Default for ProfilesOnDisk { + fn default() -> Self { + ProfilesOnDisk::Inline(Vec::new()) + } +} + +/// Raw on-disk shape of `Settings`, used only to support deserializing an old `settings.json` whose +/// `profiles` field was the pointer struct and had no top-level `active_profile` — see `Settings`'s +/// `#[serde(from = ...)]`. +#[derive(Debug, Clone, Deserialize)] +struct SettingsOnDisk { + color_scheme: ColorScheme, + #[serde(default)] + css_entries: Vec, + #[serde(default)] + custom_theme_id: Option, + #[serde(default)] + profiles: ProfilesOnDisk, + #[serde(default)] + active_profile: Option, + #[serde(default)] + max_logs: Option, + #[serde(default)] + alert_rules: Vec, + #[serde(default)] + ntfy_topic: Option, + #[serde(default)] + ntfy_server: Option, +} + +impl From for Settings { + fn from(on_disk: SettingsOnDisk) -> Self { + let (profiles, legacy_active_profile) = match on_disk.profiles { + ProfilesOnDisk::Inline(profiles) => (profiles, None), + ProfilesOnDisk::Legacy(pointer) => (Vec::new(), pointer.active_profile), + }; + Settings { + color_scheme: on_disk.color_scheme, + css_entries: on_disk.css_entries, + custom_theme_id: on_disk.custom_theme_id, + profiles, + active_profile: on_disk.active_profile.or(legacy_active_profile), + max_logs: on_disk.max_logs, + alert_rules: on_disk.alert_rules, + ntfy_topic: on_disk.ntfy_topic, + ntfy_server: on_disk.ntfy_server, + } + } } /// The single, process-wide settings instance. Only `PartialEq` (not `Eq`): `alert_rules` carries `f64` fields, which aren't `Eq`. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +/// +/// `profiles`/`active_profile` used to be a separate `Profiles` pointer struct referencing a +/// standalone `profiles.json` (see `crate::profile::ProfileStore`, removed by ticket 049); profile +/// data now lives inline here, as the single source of truth. `Deserialize` goes through +/// `SettingsOnDisk` so a `settings.json` written before this ticket still loads (see +/// `migrate_legacy_profiles_file`, which then imports the old `profiles.json`, if any, on `load`). +#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] +#[serde(from = "SettingsOnDisk")] pub struct Settings { pub color_scheme: ColorScheme, pub css_entries: Vec, /// Id of the selected custom theme entry, if any; `None` means `color_scheme` alone determines the active palette. #[serde(default)] pub custom_theme_id: Option, - pub profiles: Profiles, + /// Every known profile, inline (ticket 049). + pub profiles: Vec, + /// Name of the currently active profile, if any. + pub active_profile: Option, /// Maximum number of ingested logs kept in memory; `None` means unlimited. #[serde(default)] pub max_logs: Option, @@ -86,36 +162,21 @@ pub struct Settings { pub ntfy_server: Option, } -impl Default for Settings { - fn default() -> Self { - Settings { - color_scheme: ColorScheme::default(), - css_entries: Vec::new(), - custom_theme_id: None, - profiles: Profiles { - names: Vec::new(), - active_profile: None, - profiles_dir: "profiles".to_string(), - }, - max_logs: None, - alert_rules: Vec::new(), - ntfy_topic: None, - ntfy_server: None, - } - } -} - /// Computes the default settings file path (`/settings.json`) from OS-conventional config directories; `None` if none could be determined. pub fn default_settings_path() -> Option { ProjectDirs::from("", "", "ottyl").map(|dirs| dirs.config_dir().join("settings.json")) } impl Settings { - /// Loads settings from `path`, creating and persisting defaults if missing. Any I/O or parse error on an existing file is logged and treated as defaults, without overwriting the unreadable file. + /// Loads settings from `path`, creating and persisting defaults if missing. Any I/O or parse error on an existing file is logged and treated as defaults, without overwriting the unreadable file. Also runs the one-time legacy `profiles.json` migration (see `migrate_legacy_profiles_file`). pub fn load(path: &Path) -> Settings { match fs::read_to_string(path) { - Ok(contents) => match serde_json::from_str(&contents) { - Ok(settings) => settings, + Ok(contents) => match serde_json::from_str::(&contents) { + Ok(mut settings) => { + settings.migrate_legacy_profiles_file(path); + settings.migrate_legacy_profile_shapes(path); + settings + } Err(err) => { eprintln!( "warning: failed to parse settings file at {}: {err}; using defaults", @@ -125,7 +186,8 @@ impl Settings { } }, Err(err) if err.kind() == io::ErrorKind::NotFound => { - let settings = Settings::default(); + let mut settings = Settings::default(); + settings.migrate_legacy_profiles_file(path); if let Err(save_err) = settings.save(path) { eprintln!( "warning: failed to create default settings file at {}: {save_err}", @@ -153,6 +215,60 @@ impl Settings { .map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err))?; fs::write(path, contents) } + + /// One-time migration (ticket 049): if `self.profiles` is empty (a fresh `Settings`, or one + /// parsed from a pre-049 `settings.json` whose profiles lived in a separate file) and a legacy + /// `/profiles/profiles.json` exists, reads it in, merges it into + /// `self.profiles`, persists `self`, and — only once that save succeeds — renames the old file + /// (`.migrated` suffix) so it isn't re-imported on a later load. Every prior version of this + /// codebase always used the fixed `"profiles"` subdirectory name (no API ever let it be + /// reconfigured), so that's the only location checked. Best-effort: any failure is logged and + /// left for a future load to retry rather than panicking. + fn migrate_legacy_profiles_file(&mut self, settings_path: &Path) { + if !self.profiles.is_empty() { + return; + } + let legacy_path = settings_path + .parent() + .map(|parent| parent.join("profiles")) + .unwrap_or_else(|| PathBuf::from("profiles")) + .join("profiles.json"); + let Some(profiles) = crate::profile::load_legacy_profile_store(&legacy_path) else { + return; + }; + self.profiles = profiles; + if let Err(err) = self.save(settings_path) { + eprintln!( + "warning: failed to persist profiles migrated from {}: {err}", + legacy_path.display() + ); + return; + } + let migrated_path = legacy_path.with_extension("json.migrated"); + if let Err(err) = fs::rename(&legacy_path, &migrated_path) { + eprintln!( + "warning: migrated profiles from {} into settings.json, but failed to rename the old file: {err}", + legacy_path.display() + ); + } + } + + /// Migrates any pre-037/pre-038 metric or param shapes still present in `self.profiles` into + /// their current form (see `crate::profile::migrate_legacy_profile_shapes`), persisting `self` + /// if anything changed. Runs on every load — previously done inside the now-removed + /// `ProfileStore::load` (ticket 049 folded profile storage into `settings.json`) — so a profile + /// saved by an older version of ottyl, whether loaded inline here or just merged in by + /// `migrate_legacy_profiles_file`, still gets migrated forward. + fn migrate_legacy_profile_shapes(&mut self, settings_path: &Path) { + if crate::profile::migrate_legacy_profile_shapes(&mut self.profiles) + && let Err(err) = self.save(settings_path) + { + eprintln!( + "warning: failed to persist migrated profile shapes to {}: {err}", + settings_path.display() + ); + } + } } /// Error returned by CSS-entry mutations and lookups. @@ -315,6 +431,20 @@ mod tests { assert_eq!(settings.color_scheme, ColorScheme::Dark); } + /// A `settings.json` written before ticket 049 (nested `Profiles` pointer struct, no top-level + /// `active_profile`) still parses: the pointer's `active_profile` carries over, and `profiles` + /// starts empty (the real content lived in a separate `profiles.json`, migrated separately by + /// `Settings::load`, not by plain deserialization). + #[test] + fn legacy_profiles_pointer_shape_deserializes_with_empty_inline_profiles() { + let settings: Settings = serde_json::from_str( + r#"{"color_scheme":"dark","css_entries":[],"custom_theme_id":null,"profiles":{"names":["web"],"active_profile":"web","profiles_dir":"profiles"},"max_logs":null,"alert_rules":[],"ntfy_topic":null,"ntfy_server":null}"#, + ) + .unwrap(); + assert!(settings.profiles.is_empty()); + assert_eq!(settings.active_profile, Some("web".to_string())); + } + #[test] fn round_trips_through_save_and_load() { let dir = tempfile::tempdir().unwrap(); @@ -329,8 +459,8 @@ mod tests { settings.css_entries.push(CssEntry::Linked { path_or_url: "https://example.com/style.css".to_string(), }); - settings.profiles.names.push("default".to_string()); - settings.profiles.active_profile = Some("default".to_string()); + settings.profiles.push(crate::profile::Profile::new("default".to_string())); + settings.active_profile = Some("default".to_string()); settings.alert_rules.push(crate::alerts::AlertRule { id: "rule1".to_string(), name: "error spike".to_string(), @@ -458,4 +588,55 @@ mod tests { let result = settings.css_content(&css_dir, &linked_id); assert!(matches!(result, Err(CssError::NotImported))); } + + /// Acceptance criterion (ticket 049): a pre-existing legacy `profiles_dir/profiles.json` is + /// merged into `settings.json` on the next load, and the old file is renamed (not re-imported on + /// a subsequent load). + #[test] + fn load_migrates_legacy_profiles_file_into_inline_settings() { + let dir = tempfile::tempdir().unwrap(); + let settings_path = dir.path().join("settings.json"); + let profiles_dir = dir.path().join("profiles"); + fs::create_dir_all(&profiles_dir).unwrap(); + let legacy_path = profiles_dir.join("profiles.json"); + fs::write( + &legacy_path, + r#"[{"name":"web","params":[],"metrics":[],"dashboards":[]}]"#, + ) + .unwrap(); + // No settings.json yet: simulates upgrading a pre-049 install that never wrote one either, + // or one that did but whose nested pointer already carried an active_profile name. + fs::write( + &settings_path, + r#"{"color_scheme":"system","css_entries":[],"custom_theme_id":null,"profiles":{"names":["web"],"active_profile":"web","profiles_dir":"profiles"},"max_logs":null,"alert_rules":[],"ntfy_topic":null,"ntfy_server":null}"#, + ) + .unwrap(); + + let settings = Settings::load(&settings_path); + assert_eq!(settings.profiles.len(), 1); + assert_eq!(settings.profiles[0].name, "web"); + assert_eq!(settings.active_profile, Some("web".to_string())); + + // The legacy file is gone (renamed), so it isn't re-imported. + assert!(!legacy_path.exists()); + let migrated_path = legacy_path.with_extension("json.migrated"); + assert!(migrated_path.exists()); + + // Reload from disk: the migrated settings.json now carries profiles inline directly. + let reloaded = Settings::load(&settings_path); + assert_eq!(reloaded.profiles.len(), 1); + assert_eq!(reloaded.profiles[0].name, "web"); + } + + /// A fresh install (no legacy file at all) never creates `profiles.json` — it's not even looked + /// for beyond the one migration check, and `profiles` simply stays empty. + #[test] + fn load_without_legacy_file_leaves_profiles_empty() { + let dir = tempfile::tempdir().unwrap(); + let settings_path = dir.path().join("settings.json"); + + let settings = Settings::load(&settings_path); + assert!(settings.profiles.is_empty()); + assert!(!dir.path().join("profiles").exists()); + } } diff --git a/tickets/050-structured-header-extraction-and-bounded-logfmt-overfit.md b/tickets/050-structured-header-extraction-and-bounded-logfmt-overfit.md new file mode 100644 index 0000000..f40d972 --- /dev/null +++ b/tickets/050-structured-header-extraction-and-bounded-logfmt-overfit.md @@ -0,0 +1,84 @@ +# 050 — Bound logfmt-overfit param cardinality; derive params from `log stream`-style structured headers + +## Summary + +A real-world `log stream` (macOS unified log) dump (`huig_dump.txt`, ~700 +lines) exposes two related problems in the auto-param-derivation path +(`auto_declare_params` in `src/engine.rs`, backed by `extract_pairs` in +`src/logfmt.rs`, run on *every* ingested line at `src/engine.rs:155`): + +1. **`=`-overfitting produces runaway, low-value param cardinality.** + `extract_pairs` scans anywhere in the line for `key=value`/`key="value"` + and treats every match as a declarable param. On the sample dump this + discovers **220 distinct auto-declared param names** from under 700 + lines — one-off WiFi/radio stat dumps alone contribute lines with + 20-27 `key=value` pairs each (e.g. `txRTSFrm=169 txRTSFail=0 + rxCTSUcast=168 ...`), most of which are internal counters that will + never recur meaningfully. Some matches are outright false positives: + base64-encoded blobs ending in padding (e.g. + `rKTEtG7JmnThhoKPbZiVrA=='>,`) get parsed as a `key=value` pair because + `=` precedes non-whitespace. Since `auto_declare_params` "never + overwrites an existing declaration" and there is no cap or pruning + anywhere in the ingest path, every distinct key seen ever becomes a + permanent declaration — this is almost certainly the source of the + reported ingest lag on this input (per-line nom scanning cost is + presumably fine; the unbounded, ever-growing declaration/index set is + the more likely culprit and should be measured either way) and it + clutters the params UI with noise no user asked for. + +2. **The input's real structure is being ignored.** `log stream` output is + not logfmt at all — it's a fixed-column header (`Timestamp`, `Thread`, + `Type`, `Activity`, `PID`, `TTL`) followed by a free-text + `process: message` tail (see header row and body of `huig_dump.txt`). + `Type` (`Default`/`Error`/`Activity`), the process name (`kernel`, + `airportd`, `bluetoothd`, `WindowServer`, ...), and `PID` are + low-cardinality, genuinely useful params that could be derived + directly and cheaply from column position — with no `=`-scanning, no + ambiguity, and no risk of false positives — instead of (or in addition + to) whatever generic `key=value` pairs happen to appear later in the + line. + +## Scope + +- Add a bound/safeguard on `auto_declare_params` so a single burst of + unusual lines can't permanently inflate the declared-param set: + options include (pick and justify one, or combine) a cap on the total + number of auto-derived declarations, a per-line cap on how many + `key=value` pairs from one line are eligible for auto-declaration, and/or + a heuristic to reject candidate pairs that look like base64 fragments + (trailing `=`/`==` with a base64-alphabet value and no realistic bareword + shape) in `src/logfmt.rs`'s `extract_pairs`/`pair`. +- Add an optional structured-header extractor for `log stream`-shaped + input: detect the fixed-column header/row shape (timestamp, thread id, + log type, activity id, pid, ttl, `process: message`) and surface + `Type`/process-name/`PID` as derivable params, analogous to how + `ottyl_fmt` (ticket 044) records which format a line matched. Decide + whether this is a new `ottyl_fmt` variant (e.g. `"log_stream"`) or a + separate always-attempted extraction that runs alongside + logfmt/JSON detection, and record the decision in the PR. +- Use `huig_dump.txt` (already in the repo root) as the seed fixture for + tests — copy the relevant lines into a test fixture under wherever + existing ingest fixtures live, rather than reading the file from the + repo root at test time. +- Re-measure ingest throughput on this fixture before/after to confirm + the lag is actually addressed, and note the before/after numbers in the + PR description. + +## Acceptance criteria + +- Ingesting `huig_dump.txt`'s lines no longer produces hundreds of + permanent auto-declared params from one-off stat-dump lines; the bound + chosen is verified by a test asserting the declared-param count stays + under a stated threshold after ingesting the fixture. +- Base64-padding false positives (e.g. the `==`-terminated blobs in the + fixture) are not auto-declared as params. +- `Type`, process name, and `PID` are available as derivable/queryable + params for `log stream`-shaped lines, verified against the fixture. +- Existing logfmt/JSON auto-declaration behavior for non-`log + stream`-shaped lines (existing tests in `src/logfmt.rs`/`src/engine.rs`) + is unchanged. + +## Depends on + +044 (reserved param naming / `ottyl_fmt` — this ticket likely extends the +same format-detection mechanism).