From 00c21c80322b7b131ba662e972aaff5a8b3c57bb Mon Sep 17 00:00:00 2001 From: Hugh Kaznowski Date: Sat, 5 Sep 2026 17:02:33 +0100 Subject: [PATCH] Trim comments across Rust and webapp source to direct one-liners (ticket 030) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pass over src/*.rs, webapp/src/**, and the in-app "?" docs prose: condensed multi-paragraph doc comments to single lines that state the non-obvious why, deleted comments that only restated adjacent code, and tightened the Docs.tsx help-panel wording. No logic, structure, or code formatting changed — verified via cargo test (143 passed) and tsc -b && vite build. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01XFUNZwCvz3kcod3352ZR2o --- src/alerts.rs | 88 +---- src/api.rs | 188 +---------- src/data_value.rs | 26 +- src/dto.rs | 117 ++----- src/engine.rs | 333 +++---------------- src/filter_datetime.rs | 59 +--- src/filter_parser.rs | 51 +-- src/index_key.rs | 14 +- src/ingest.rs | 25 +- src/main.rs | 16 +- src/metric_primitive.rs | 14 +- src/profile.rs | 253 +++----------- src/promql.rs | 76 +---- src/settings.rs | 125 ++----- src/timestamp.rs | 6 +- tickets/030-comment-cleanup.md | 35 -- webapp/src/App.tsx | 36 +- webapp/src/components/Alerts.tsx | 6 +- webapp/src/components/CollapsibleSection.tsx | 18 +- webapp/src/components/Dashboards.tsx | 39 +-- webapp/src/components/Docs.tsx | 142 ++++---- webapp/src/components/FilterBar.tsx | 12 +- webapp/src/components/HelpIcon.tsx | 6 +- webapp/src/components/LogPreview.tsx | 12 +- webapp/src/components/LogTimeline.tsx | 13 +- webapp/src/components/Metrics.tsx | 14 +- webapp/src/components/NavMenu.tsx | 8 +- webapp/src/components/PanelChart.tsx | 23 +- webapp/src/lib/api.ts | 46 +-- webapp/src/lib/docsNav.tsx | 2 +- webapp/src/lib/themes.ts | 40 +-- webapp/src/lib/useSettingsPanel.ts | 54 +-- 32 files changed, 347 insertions(+), 1550 deletions(-) delete mode 100644 tickets/030-comment-cleanup.md diff --git a/src/alerts.rs b/src/alerts.rs index 9ca9eea..ea8e370 100644 --- a/src/alerts.rs +++ b/src/alerts.rs @@ -1,21 +1,6 @@ -//! Alert rules: definitions (ticket 017), trigger evaluation (ticket 018), -//! and ntfy delivery (ticket 019). +//! Alert rules: definitions, trigger evaluation, and ntfy delivery. //! -//! ## Design: why alert queries only support regex/sequence/timestamp -//! -//! `Immediate`/`Threshold`/`Ratio` conditions reuse FilterBar query strings -//! (see `crate::filter_parser`), evaluated against a *single newly-ingested -//! line* on `EngineMessage::Ingest` (see `evaluate_alerts`, called from -//! `engine::run`). `engine.rs`'s own `compile_filter_query` resolves -//! `metric[param]` comparisons by scanning that metric's *entire* bucket -//! history up front (`compile_metric_comparison`) — reusing that per alert -//! check, per ingested line, would make alert evaluation cost proportional to -//! total log volume rather than to new logs ingested, which this ticket -//! requires. So alert queries here only support the subset of FilterBar that -//! evaluates against a single line's own fields — `regex(...)`, `sequence`, -//! `timestamp`, and `and`/`or` — via `eval_alert_expr` below. A `metric[param]` -//! comparison in an alert query is rejected with a clear error (logged, the -//! rule just never fires) rather than silently misbehaving. +//! Alert queries are restricted to regex/sequence/timestamp — a metric[param] comparison would require a full bucket rescan per line, breaking the proportional-cost guarantee. use std::collections::hash_map::DefaultHasher; use std::collections::{HashMap, HashSet, VecDeque}; @@ -34,7 +19,6 @@ use crate::filter_parser::{self, CompareOp, FilterExpr, FilterValue}; use crate::settings::Settings; use crate::timestamp::Timestamp; -/// A single alert rule definition (ticket 017). #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct AlertRule { pub id: String, @@ -44,36 +28,23 @@ pub struct AlertRule { pub sink: AlertSink, } -/// What triggers an alert rule (ticket 018). #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(tag = "kind", rename_all = "lowercase")] pub enum AlertCondition { - /// Fires once per log line matching `query`. Immediate { query: String }, - /// Fires when `query`'s match count within a rolling `window_ms` crosses - /// `threshold` (via `op`). + /// Fires when `query`'s match count within a rolling `window_ms` crosses `threshold` (via `op`). Threshold { query: String, op: CompareOp, threshold: f64, window_ms: i64, }, - /// Fires the first time `field_regex` matches a line and `capture_group` - /// captures a value not seen before *for this rule*. Seen values live - /// only in memory (`AlertRuntimeState::seen_values`) for the process's - /// lifetime — a restart forgets them, so previously-seen values can fire - /// again. This is a deliberate simplicity tradeoff (matches the - /// codebase's general "no restart persistence for in-memory engine - /// state" pattern), not an oversight. + /// Seen values live only in memory — a restart forgets them, so previously-seen values can fire again; a deliberate simplicity tradeoff, not an oversight. NewValue { field_regex: String, capture_group: String, }, - /// Fires when `count(query_a) / count(query_b)` (rolling over - /// `window_ms`) crosses `ratio` (via `op`). A zero `count_b` skips - /// evaluation entirely for that line (rather than treating the ratio as - /// infinite) — there's no meaningful "actual vs threshold" comparison to - /// report yet. + /// A zero `count_b` skips evaluation for that line rather than treating the ratio as infinite. Ratio { query_a: String, query_b: String, @@ -83,9 +54,7 @@ pub enum AlertCondition { }, } -/// Where a fired alert is delivered (ticket 019). Modeled as an enum (rather -/// than `AlertRule` holding a bare topic/server) so more sink kinds can be -/// added later without reshaping `AlertRule`. +/// Modeled as an enum (rather than bare fields on `AlertRule`) so more sink kinds can be added later without reshaping it. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(tag = "kind", rename_all = "lowercase")] pub enum AlertSink { @@ -96,7 +65,6 @@ pub enum AlertSink { }, } -/// Error returned by alert-rule mutations/lookups. Mirrors `CssError`'s shape. #[derive(Debug)] pub enum AlertError { NotFound, @@ -120,9 +88,7 @@ impl From for AlertError { } } -/// Generates an id for a newly-created alert rule. Same approach as -/// `settings::generate_imported_css_id`: hash the name and current time -/// rather than pulling in a `uuid` dependency. +/// Hashes the name and current time rather than pulling in a `uuid` dependency. fn generate_alert_rule_id(name: &str) -> String { let mut hasher = DefaultHasher::new(); name.hash(&mut hasher); @@ -135,7 +101,6 @@ fn generate_alert_rule_id(name: &str) -> String { } impl Settings { - /// Creates a new alert rule, persisting settings to `settings_path`. pub fn create_alert_rule( &mut self, settings_path: &Path, @@ -150,9 +115,6 @@ impl Settings { Ok(rule) } - /// Replaces an existing alert rule's fields (id stays fixed), persisting - /// settings to `settings_path`. Returns `AlertError::NotFound` if no rule - /// with `id` exists. pub fn update_alert_rule( &mut self, settings_path: &Path, @@ -176,8 +138,6 @@ impl Settings { Ok(updated) } - /// Removes an alert rule by id, persisting settings to `settings_path`. - /// Returns `AlertError::NotFound` if no rule with `id` exists. pub fn remove_alert_rule(&mut self, settings_path: &Path, id: &str) -> Result<(), AlertError> { let index = self .alert_rules @@ -190,11 +150,7 @@ impl Settings { } } -/// Per-rule in-memory evaluation state (ticket 018): rolling match-timestamp -/// windows for `Threshold`/`Ratio`, and seen-values for `NewValue`. Lives -/// only for the engine process's lifetime (see `AlertCondition::NewValue`'s -/// doc comment) — not persisted, not backfilled against already-ingested -/// logs. +/// Per-rule in-memory evaluation state; lives only for the process's lifetime — not persisted, not backfilled against already-ingested logs. #[derive(Debug, Default)] pub struct AlertRuntimeState { threshold_matches: VecDeque, @@ -203,9 +159,7 @@ pub struct AlertRuntimeState { seen_values: HashSet, } -/// Drops every entry older than `window_ms` relative to `now_ms` from the -/// front of a rolling window. `timestamps` is push_back-ordered, so once a -/// front entry is within the window every entry behind it is too. +/// `timestamps` is push_back-ordered, so once the front entry is within the window, every entry behind it is too. fn prune_window(timestamps: &mut VecDeque, now_ms: i64, window_ms: i64) { while let Some(&front) = timestamps.front() { if now_ms - front > window_ms { @@ -216,9 +170,7 @@ fn prune_window(timestamps: &mut VecDeque, now_ms: i64, window_ms: i64) { } } -/// Evaluates a FilterBar query string against a single log line's own fields -/// only — see this module's doc comment for why `metric[param]` comparisons -/// aren't supported here. +/// Evaluates a FilterBar query against a single line's own fields only (metric[param] comparisons aren't supported — see module doc). fn eval_alert_query(query: &str, sequence: u64, timestamp: Timestamp, text: &str) -> Result { let expr = filter_parser::parse(query).map_err(|err| err.to_string())?; eval_alert_expr(&expr, sequence, timestamp, text) @@ -261,12 +213,7 @@ fn eval_alert_expr(expr: &FilterExpr, sequence: u64, timestamp: Timestamp, text: } } -/// Evaluates every enabled rule's condition against one newly-ingested line, -/// mutating each rule's runtime state (rolling windows / seen-values) along -/// the way. Returns `(rule, message)` for every rule that fired — the caller -/// (`engine::run`) hands each off to `deliver_for_rule`. A malformed query -/// (bad regex, unsupported metric comparison) is logged to stderr and simply -/// never matches, rather than panicking or propagating. +/// A malformed query (bad regex, unsupported metric comparison) is logged and simply never matches, rather than panicking. pub fn evaluate_alerts( rules: &[AlertRule], states: &mut HashMap, @@ -362,9 +309,7 @@ fn evaluate_condition( let count_a = state.ratio_a_matches.len() as f64; let count_b = state.ratio_b_matches.len() as f64; if count_b == 0.0 { - // Zero denominator: skip evaluation rather than treat the - // ratio as infinite — there's nothing meaningful to compare - // against `ratio` yet. + // Zero denominator: skip rather than treat the ratio as infinite. return None; } let observed = count_a / count_b; @@ -377,10 +322,7 @@ fn evaluate_condition( } } -/// Delivers one fired alert to its rule's configured sink. Errors are logged -/// to stderr and dropped — no retry queue, matching `remove_css`'s -/// best-effort style. Called via `tokio::spawn` from the ingest hot path -/// (`engine::run`) so a slow/unreachable sink never blocks ingestion. +/// Errors are only logged (no retry queue); spawned off the ingest hot path so a slow/unreachable sink never blocks ingestion. pub async fn deliver_for_rule(rule: AlertRule, message: String) { match rule.sink { AlertSink::Ntfy { topic, server } => { @@ -392,9 +334,7 @@ pub async fn deliver_for_rule(rule: AlertRule, message: String) { } } -/// Publishes `message` to `{server}/{topic}` via ntfy's plain-text publish -/// API (no auth, raw body). Standalone and `AlertRule`-free so a future -/// "send test notification" endpoint can call it directly. +/// Standalone and `AlertRule`-free so a "send test notification" endpoint can call it directly. pub async fn deliver_ntfy(server: &str, topic: &str, message: &str) -> Result<(), String> { let url = format!("{}/{}", server.trim_end_matches('/'), topic); let response = reqwest::Client::new() diff --git a/src/api.rs b/src/api.rs index 116fbd8..c3e4c1f 100644 --- a/src/api.rs +++ b/src/api.rs @@ -29,8 +29,7 @@ static WEBAPP_DIST: EmbedDir = fs_embed::fs_embed!("webapp/dist"); struct ApiState { engine: EngineHandle, settings: SettingsHandle, - /// Path to the settings JSON file on disk. Imported CSS files are stored - /// alongside it, in its parent directory (see `ApiState::css_dir`). + /// Imported CSS files are stored alongside this path's parent directory (see `ApiState::css_dir`). settings_path: Arc, } @@ -42,13 +41,7 @@ impl ApiState { .unwrap_or_else(|| PathBuf::from(".")) } - /// Persists a newly-declared param into the active profile, if any (see - /// `ProfileStore::record_param_declaration`), so it survives a restart. - /// A no-op (not an error) when no profile is currently active — the - /// engine declaration above has already succeeded regardless. Failures - /// (e.g. I/O) are logged to stderr rather than surfaced to the caller: the - /// engine declaration is the source of truth for the running process, and - /// this is a best-effort persistence step layered on top. + /// 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 { return; @@ -76,7 +69,6 @@ impl ApiState { } } - /// The active profile's store path and name, if a profile is currently active. fn active_profile_location(&self) -> Option<(PathBuf, String)> { let settings = self.settings.lock().unwrap(); let profile_name = settings.profiles.active_profile.clone()?; @@ -126,8 +118,7 @@ pub fn router(engine: EngineHandle, settings: SettingsHandle, settings_path: Pat .fallback(serve_webapp) } -/// Serves the webapp embedded in the binary (see `WEBAPP_DIST`), falling -/// back to `index.html` for any unmatched path so client-side routing works. +/// Falls back to `index.html` for unmatched paths so client-side routing works. async fn serve_webapp(uri: axum::http::Uri) -> Response { let path = uri.path().trim_start_matches('/'); let file = WEBAPP_DIST @@ -167,11 +158,6 @@ async fn ask(engine: &EngineHandle, query: EngineQuery) -> Result, Query(params): Query, @@ -186,8 +172,6 @@ async fn get_logs( } } -/// `GET /api/timeline` — same `filter` query param and error mapping as -/// `get_logs` above. async fn get_timeline( State(state): State, Query(params): Query, @@ -210,10 +194,6 @@ async fn get_stats(State(state): State) -> impl IntoResponse { } } -/// `GET /api/params` — lists all declared params (name, regex, capture -/// group, kind) as `Vec`. Mirrors `list_metrics` below -/// (ticket 015 needed this for LogPreview's column picker; ticket 004 only -/// exposed the equivalent for metrics). async fn list_params(State(state): State) -> impl IntoResponse { match ask(&state.engine, EngineQuery::ParamDeclarations).await { Ok(EngineResponse::ParamDeclarations(declarations)) => Json(declarations).into_response(), @@ -222,14 +202,6 @@ async fn list_params(State(state): State) -> impl IntoResponse { } } -/// `POST /api/params` — declares a param. Body mirrors `DeclareParamRequest`: -/// `{ "name": "...", "message_match_regex": "...", "capture_group": "...", -/// "kind": "string" | "number" | "timestamp" }`. On success, also persists -/// the declaration into the active profile (if any — see -/// `ApiState::persist_param_declaration`) and returns `201 Created` with the -/// declaration as `ParamDeclarationDto`. On a validation failure (bad regex, -/// unknown capture group), returns `400 Bad Request` with -/// `{ "error": "..." }`. async fn declare_param( State(state): State, Json(body): Json, @@ -260,8 +232,6 @@ async fn declare_param( } } -/// `GET /api/metrics` — lists all declared metrics (name, regex, params) as -/// `Vec`. async fn list_metrics(State(state): State) -> impl IntoResponse { match ask(&state.engine, EngineQuery::MetricDeclarations).await { Ok(EngineResponse::MetricDeclarations(declarations)) => Json(declarations).into_response(), @@ -270,11 +240,6 @@ async fn list_metrics(State(state): State) -> impl IntoResponse { } } -/// `POST /api/metrics` — declares a metric. Body mirrors -/// `DeclareMetricRequest`: `{ "name": "...", "message_match_regex": "...", -/// "params": [{ "param_name": "...", "capture_group": "...", "kind": "..." }] }`. -/// Same success/failure shape as `declare_param` above, persisting into the -/// active profile via `ApiState::persist_metric_declaration`. async fn declare_metric( State(state): State, Json(body): Json, @@ -306,9 +271,7 @@ async fn declare_metric( } } -/// Parses the `labels` query param (`"foo=bar,baz=qux"`) into a -/// `key -> expected value` map. Malformed pairs (no `=`) are skipped rather -/// than rejected — see `BucketValuesParams` doc comment for the format. +/// Malformed pairs (no `=`) are skipped rather than rejected. fn parse_labels_filter(raw: &str) -> BTreeMap { raw.split(',') .filter_map(|pair| pair.split_once('=')) @@ -316,9 +279,6 @@ fn parse_labels_filter(raw: &str) -> BTreeMap { .collect() } -/// `GET /api/metrics/:name` — queries bucket values for one metric. Query -/// params: `labels` (see `parse_labels_filter`), `start_ms`, `end_ms`, -/// `since_sequence`, `limit` (mirroring `/api/logs`/`/api/timeline`). async fn get_metric_bucket_values( State(state): State, AxumPath(metric_name): AxumPath, @@ -340,12 +300,6 @@ async fn get_metric_bucket_values( } } -/// `GET /api/query` — evaluates a PromQL query (ticket 013; see `crate::promql` for -/// the supported grammar) against declared metrics. Query params: `promql` (the -/// query string), `start_ms`, `end_ms` (bound which points are included, mirroring -/// `/api/logs`/`/api/metrics/:name`). Returns `Vec` — chart-ready -/// `labels` + `(timestamp_ms, value)` series — on success (`200 OK`), or -/// `400 Bad Request` with `{ "error": "..." }` for a malformed query. async fn run_promql_query( State(state): State, Query(params): Query, @@ -365,10 +319,6 @@ async fn run_promql_query( } } -/// Maps a `CssError` from a settings mutation/lookup to a response status -/// and message: `NotFound` -> 404, `NotImported` -> 400 (the id is valid but -/// refers to a linked entry, which this operation doesn't apply to), `Io` -> -/// 500. fn css_error_response(err: CssError) -> axum::response::Response { match err { CssError::NotFound => (StatusCode::NOT_FOUND, err.to_string()).into_response(), @@ -377,18 +327,12 @@ fn css_error_response(err: CssError) -> axum::response::Response { } } -/// `GET /api/settings/css` — lists all configured CSS entries (imported and -/// linked) as `Vec`. async fn list_css(State(state): State) -> impl IntoResponse { let settings = state.settings.lock().unwrap(); let entries: Vec = settings.css_entries.iter().map(CssEntryDto::from).collect(); Json(entries).into_response() } -/// `POST /api/settings/css` — imports raw CSS content. Body: -/// `{ "filename": "theme.css", "content": "body { ... }" }`. Stores the -/// content as a file alongside the settings file, adds a `CssEntry::Imported` -/// to settings, and returns the new entry as `CssEntryDto` (201 Created). async fn import_css( State(state): State, Json(body): Json, @@ -414,11 +358,7 @@ async fn import_css( } } -/// `POST /api/settings/css/link` — registers a reference to an external CSS -/// path/URL. Body: `{ "path_or_url": "https://example.com/style.css" }`. -/// Content is never copied; the entry is never deletable via -/// `DELETE /api/settings/css/:id`. Returns the new entry as `CssEntryDto` -/// (201 Created). +/// Content is never copied, and the entry can never be deleted via `DELETE /api/settings/css/:id`. async fn link_css( State(state): State, Json(body): Json, @@ -436,10 +376,6 @@ async fn link_css( } } -/// `DELETE /api/settings/css/:id` — removes an imported CSS entry (file + -/// settings reference). Rejects with 400 if `id` refers to a `Linked` entry -/// (never deletable this way) and 404 if `id` is unknown. On success returns -/// 204 No Content. async fn remove_css( State(state): State, AxumPath(id): AxumPath, @@ -452,10 +388,7 @@ async fn remove_css( } } -/// `GET /api/settings/css/:id/content` — serves back the raw CSS content of -/// an imported entry (`content-type: text/css`). Rejects with 400 for a -/// linked entry (there's no local content to serve — the frontend should use -/// `path_or_url` directly) and 404 for an unknown id. +/// Rejects a linked entry with 400 — there's no local content; the frontend should use `path_or_url` directly. async fn get_css_content( State(state): State, AxumPath(id): AxumPath, @@ -473,8 +406,6 @@ async fn get_css_content( } } -/// Maps an `AlertError` to a response status/message: `NotFound` -> 404, -/// `Io` -> 500. Mirrors `css_error_response` above. fn alert_error_response(err: AlertError) -> axum::response::Response { match err { AlertError::NotFound => (StatusCode::NOT_FOUND, err.to_string()).into_response(), @@ -482,18 +413,11 @@ fn alert_error_response(err: AlertError) -> axum::response::Response { } } -/// `GET /api/settings/alerts` — lists all configured alert rules as -/// `Vec`. async fn list_alert_rules(State(state): State) -> impl IntoResponse { let settings = state.settings.lock().unwrap(); Json(settings.alert_rules.clone()).into_response() } -/// `POST /api/settings/alerts` — creates an alert rule. Body: -/// `{ "name": "...", "enabled": true, "condition": { "kind": "immediate" | \ -/// "threshold" | "newvalue" | "ratio", ... }, "sink": { "kind": "ntfy", \ -/// "topic": "...", "server": "..." | null } }`. Returns `201 Created` with -/// the created rule (including its generated `id`). async fn create_alert_rule( State(state): State, Json(body): Json, @@ -505,9 +429,6 @@ async fn create_alert_rule( } } -/// `PUT /api/settings/alerts/:id` — replaces an alert rule's fields (id in -/// the URL is authoritative). Returns the updated rule, or `404 Not Found` -/// if no rule with `id` exists. async fn update_alert_rule( State(state): State, AxumPath(id): AxumPath, @@ -520,8 +441,6 @@ async fn update_alert_rule( } } -/// `DELETE /api/settings/alerts/:id` — removes an alert rule. Returns -/// `204 No Content`, or `404 Not Found` if no rule with `id` exists. async fn delete_alert_rule(State(state): State, AxumPath(id): AxumPath) -> impl IntoResponse { let mut settings = state.settings.lock().unwrap(); match settings.remove_alert_rule(&state.settings_path, &id) { @@ -530,11 +449,7 @@ async fn delete_alert_rule(State(state): State, AxumPath(id): AxumPath } } -/// `POST /api/settings/alerts/test` — delivers a test notification to an -/// ntfy topic/server without requiring a saved rule, so the sink can be -/// verified before saving. Body: `{ "topic": "...", "server": "..." | null }`. -/// Returns `204 No Content` on success, `502 Bad Gateway` if ntfy rejects -/// or is unreachable. +/// Delivers a test notification without a saved rule so the sink can be verified before saving. async fn test_alert_notification(Json(body): Json) -> impl IntoResponse { let server = body.server.unwrap_or_else(|| "https://ntfy.sh".to_string()); match deliver_ntfy(&server, &body.topic, "Test notification from ottyl").await { @@ -543,20 +458,13 @@ async fn test_alert_notification(Json(body): Json) } } -/// `GET /api/settings` — returns the subset of `Settings` the NavMenu UI -/// needs (colour scheme, active profile name) as `SettingsDto`. CSS entries -/// have their own dedicated list endpoint (`GET /api/settings/css`). +/// CSS entries have their own endpoint (`GET /api/settings/css`). async fn get_settings(State(state): State) -> impl IntoResponse { let settings = state.settings.lock().unwrap(); Json(SettingsDto::from(&*settings)).into_response() } -/// `PATCH /api/settings` — updates process-wide settings. Body: -/// `{ "color_scheme": "light" | "dark" | "system", "max_logs": number | -/// null }` (omit a field to leave it unchanged; `max_logs: null` clears the -/// cap back to unlimited). Persists to `settings_path`, pushes a changed -/// `max_logs` to the engine so it takes effect on the next ingest without a -/// restart, and returns the updated settings as `SettingsDto`. +/// `max_logs: null` clears the cap; a changed value is pushed to the engine so it applies without a restart. async fn update_settings( State(state): State, Json(body): Json, @@ -588,9 +496,6 @@ async fn update_settings( Json(new_max_logs.0).into_response() } -/// Maps a `ProfileError` to a response status/message: `NotFound` -> 404, -/// `DuplicateName`/`CannotDeleteDefault` -> 400, `Io` -> 500. Mirrors -/// `css_error_response` above. fn profile_error_response(err: ProfileError) -> axum::response::Response { match err { ProfileError::NotFound => (StatusCode::NOT_FOUND, err.to_string()).into_response(), @@ -600,10 +505,6 @@ fn profile_error_response(err: ProfileError) -> axum::response::Response { } } -/// `GET /api/settings/profiles` — lists all known profiles with their -/// param/metric declaration and dashboard counts, as `Vec`. -/// Use `GET /api/settings`'s `active_profile` field to know which one (if -/// any) is currently active. 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); @@ -612,10 +513,6 @@ async fn list_profiles(State(state): State) -> impl IntoResponse { Json(summaries).into_response() } -/// `POST /api/settings/profiles` — creates a new, empty profile. Body: -/// `{ "name": "..." }`. Returns `201 Created` with the new profile as -/// `ProfileSummaryDto`, or `400 Bad Request` if a profile with that name -/// already exists. async fn create_profile( State(state): State, Json(body): Json, @@ -633,11 +530,7 @@ async fn create_profile( } } -/// `DELETE /api/settings/profiles/:name` — deletes a profile. If it was the -/// active profile, `active_profile` is cleared (but the engine's currently -/// declared params/metrics are left as-is — they simply won't be reloaded on -/// the next restart). Returns `204 No Content`, or `404 Not Found` if no -/// profile with that name exists. +/// If it was the active profile, `active_profile` is cleared, but the engine's already-declared params/metrics keep running until the next restart. async fn delete_profile( State(state): State, AxumPath(name): AxumPath, @@ -652,12 +545,7 @@ async fn delete_profile( } } -/// Removes every currently-declared param/metric from the running engine -/// (via `EngineMessage::RemoveParamDeclaration`/`RemoveMetricDeclaration`), -/// then replays `profile`'s stored declarations into it (see -/// `profile::load_into_engine`). Used by `activate_profile` so switching the -/// active profile takes effect on the running engine immediately, without -/// requiring a process restart. +/// Removes every currently-declared param/metric from the engine, then replays `profile`'s declarations into it. async fn reload_engine_declarations(engine: &EngineHandle, profile: &Profile) { if let Ok(EngineResponse::ParamDeclarations(params)) = ask(engine, EngineQuery::ParamDeclarations).await @@ -690,12 +578,6 @@ async fn reload_engine_declarations(engine: &EngineHandle, profile: &Profile) { profile::load_into_engine(engine, profile).await; } -/// `POST /api/settings/profiles/:name/activate` — sets `name` as the active -/// profile (persisted, see `ProfileStore::set_active`) and hot-swaps the -/// running engine's declared params/metrics to match it (see -/// `reload_engine_declarations`), so the switch is visible without a -/// restart. Returns the updated settings as `SettingsDto`, or `404 Not -/// Found` if no profile with that name exists. async fn activate_profile( State(state): State, AxumPath(name): AxumPath, @@ -723,9 +605,6 @@ async fn activate_profile( } } -/// `GET /api/settings/profiles/:name/dashboards` — lists all dashboards saved -/// on the profile `name`, as `Vec`. Returns `404 Not Found` if -/// no profile with that name exists. async fn list_dashboards( State(state): State, AxumPath(name): AxumPath, @@ -742,10 +621,6 @@ async fn list_dashboards( } } -/// `POST /api/settings/profiles/:name/dashboards` — creates a new, empty -/// dashboard on profile `name`. Body: `{ "name": "..." }`. Returns -/// `201 Created` with the new dashboard as `DashboardDto`, or `404 Not -/// Found` if no profile with that name exists. async fn create_dashboard( State(state): State, AxumPath(name): AxumPath, @@ -761,9 +636,6 @@ async fn create_dashboard( } } -/// `GET /api/settings/profiles/:name/dashboards/:dashboard_id` — fetches one -/// dashboard by id. Returns `404 Not Found` if the profile or dashboard -/// doesn't exist. async fn get_dashboard( State(state): State, AxumPath((name, dashboard_id)): AxumPath<(String, String)>, @@ -780,11 +652,6 @@ async fn get_dashboard( } } -/// `PUT /api/settings/profiles/:name/dashboards/:dashboard_id` — replaces the -/// dashboard's name/panels. Body: `{ "name": "...", "panels": [{ "id": "...", -/// "title": "...", "promql": "...", "chart_type": "bar" | "line" | "pie" }] }`. -/// Returns the updated dashboard as `DashboardDto`, or `404 Not Found` if the -/// profile or dashboard doesn't exist. async fn save_dashboard( State(state): State, AxumPath((name, dashboard_id)): AxumPath<(String, String)>, @@ -805,9 +672,6 @@ async fn save_dashboard( } } -/// `DELETE /api/settings/profiles/:name/dashboards/:dashboard_id` — deletes a -/// dashboard. Returns `204 No Content`, or `404 Not Found` if the profile or -/// dashboard doesn't exist. async fn delete_dashboard( State(state): State, AxumPath((name, dashboard_id)): AxumPath<(String, String)>, @@ -836,10 +700,7 @@ mod tests { use tokio::sync::mpsc; use tower::ServiceExt; - /// Spins up a fresh engine + router pair, backed by a temp settings file, - /// for one test. Returns the router, the engine handle (so a test can - /// ingest lines directly without an HTTP route for it), and the - /// `TempDir` guard (kept alive for the test's duration). + /// Spins up a fresh engine + router pair backed by a temp settings file. fn test_app() -> (Router, EngineHandle, SettingsHandle, tempfile::TempDir) { let (engine_tx, engine_rx) = mpsc::channel(128); @@ -937,9 +798,7 @@ mod tests { let status = delete_request(&app, "/api/settings/alerts/does-not-exist").await; assert_eq!(status, StatusCode::NOT_FOUND); - // `update_alert_rule`'s `AlertError::NotFound` maps to a plain-text - // 404 body (mirrors `css_error_response`), not JSON, so this checks - // the status directly rather than via `send_json`'s JSON parsing. + // AlertError::NotFound maps to a plain-text (not JSON) 404 body, so check status directly instead of via send_json. let request = Request::builder() .method("PUT") .uri("/api/settings/alerts/does-not-exist") @@ -1085,16 +944,11 @@ mod tests { let values = body.as_array().unwrap(); assert_eq!(values.len(), 2); for value in values { - // `MetricPrimitive` is a Rust enum with derived `Serialize`, so its - // JSON form is externally tagged: `{"String": "/a"}`. + // MetricPrimitive derives Serialize as an externally-tagged enum, so JSON is {"String": "/a"}. assert_eq!(value["labels"]["path"]["String"], "/a"); } - // Bucket values are grouped by label combination (ordered by the - // labels' `BTreeMap` key, i.e. "/a" before "/b"), not globally by - // sequence — so filtering to sequence > 0 leaves sequence 2 (from the - // "/a" bucket) ahead of sequence 1 (from the "/b" bucket), and a - // limit of 1 picks the former. + // Bucket values group by label (BTreeMap-ordered "/a" before "/b"), not by sequence, so limit=1 picks sequence 2 from "/a" ahead of sequence 1 from "/b". let (status, body) = get_json(&app, "/api/metrics/requests?since_sequence=0&limit=1").await; assert_eq!(status, StatusCode::OK); let values = body.as_array().unwrap(); @@ -1159,10 +1013,7 @@ mod tests { async fn declaring_param_persists_into_active_profile() { let (app, _engine, settings_handle, dir) = test_app(); - // Set up an active profile against the *same* shared `settings` - // handle the router was built with — mutating a separate `Settings` - // loaded fresh from disk wouldn't be visible to the running app's - // in-memory settings. + // 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(); @@ -1229,12 +1080,10 @@ mod tests { assert!(body["panels"].as_array().unwrap().is_empty()); let dashboard_id = body["id"].as_str().unwrap().to_string(); - // Listing shows the new (empty) dashboard. let (status, body) = get_json(&app, "/api/settings/profiles/web/dashboards").await; assert_eq!(status, StatusCode::OK); assert_eq!(body.as_array().unwrap().len(), 1); - // Save adds a panel. let (status, body) = send_json( &app, "PUT", @@ -1253,13 +1102,11 @@ mod tests { assert_eq!(panels.len(), 1); assert_eq!(panels[0]["chart_type"], "line"); - // Get by id reflects the saved panel. let (status, body) = get_json(&app, &format!("/api/settings/profiles/web/dashboards/{dashboard_id}")).await; assert_eq!(status, StatusCode::OK); assert_eq!(body["panels"].as_array().unwrap().len(), 1); - // Delete removes it. let status = delete_request(&app, &format!("/api/settings/profiles/web/dashboards/{dashboard_id}")).await; assert_eq!(status, StatusCode::NO_CONTENT); let (status, body) = get_json(&app, "/api/settings/profiles/web/dashboards").await; @@ -1308,8 +1155,7 @@ mod tests { ) .await; - // Simulate a restart: reload the profile store from disk directly, - // independent of the running app/engine. + // Simulate a restart by reloading the profile store 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(); diff --git a/src/data_value.rs b/src/data_value.rs index 4fa157b..731ecfb 100644 --- a/src/data_value.rs +++ b/src/data_value.rs @@ -3,19 +3,13 @@ use crate::timestamp::Timestamp; use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; -/// A single parsed metric primitive extracted from a regex capture group. #[allow(dead_code)] #[derive(Debug, Clone, Serialize, Deserialize)] pub struct RegexToMetricPrimitiveMatcher { pub param_name: String, pub capture_group: String, pub kind: MetricPrimitiveKind, - /// Ticket 022: whether this param contributes to the bucket's `labels` - /// (the default) or is the bucket's per-event captured `value` (at most - /// one `Value`-role param per metric — `declare_metric` rejects more - /// than one). `#[serde(default)]` so profile files saved before this - /// field existed still load, with every param defaulting to `Label` — - /// i.e. unchanged behavior (the fixed occurrence-count-of-1 value). + /// Whether this param is a bucket label or its captured value (at most one Value-role param per metric); `#[serde(default)]` so profile files saved before this field existed still load as `Label`. #[serde(default)] pub role: ParamRole, } @@ -29,10 +23,7 @@ pub enum ParamRole { Value, } -/// `rename_all = "lowercase"` matches every other `kind`-style enum exposed -/// over the API/settings JSON (`ColorScheme`, `CssEntryKind`, -/// `ProfileMetricPrimitiveKind`), so `POST /api/params`/`POST /api/metrics` -/// bodies use `"string"`/`"number"`/`"timestamp"`. +/// `rename_all = "lowercase"` matches other `kind`-style enums, so JSON bodies use "string"/"number"/"timestamp". #[allow(dead_code)] #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] @@ -40,12 +31,7 @@ pub enum MetricPrimitiveKind { String, Number, Timestamp, - /// Ticket 023: milliseconds. On a metric's `Value`-role param, this - /// doesn't parse the captured text at all — the value is instead derived - /// as the elapsed time since the previous counter event with the same - /// label set (see `crate::engine::apply_metric_to_log`). On any other - /// param (a `Label`-role metric param, or a standalone `ParamDeclaration`), - /// it parses the captured text as a literal `i64` millisecond count. + /// On a Value-role param this is derived as elapsed time since the previous event (not parsed from text); otherwise it's a literal i64 millisecond count. Duration, } @@ -65,16 +51,16 @@ pub enum DataValue { MetricDeclaration { message_match_regex: String, params: Vec, - /// See `crate::engine::DeclareMetricRequest::matched_labels` (ticket 025). + /// See `crate::engine::DeclareMetricRequest::matched_labels`. matched_labels: Option>, - /// See `crate::engine::DeclareMetricRequest::unmatched_labels` (ticket 025). + /// See `crate::engine::DeclareMetricRequest::unmatched_labels`. unmatched_labels: Option>, }, /// Paired with `IndexKey::Bucket`. BucketValue { values: Vec<(u64, Timestamp, MetricPrimitive)>, }, - /// Paired with `IndexKey::LastMetricEvent` (ticket 023). + /// Paired with `IndexKey::LastMetricEvent`. LastEventTimestamp { timestamp: Timestamp, }, diff --git a/src/dto.rs b/src/dto.rs index c0b122c..915d227 100644 --- a/src/dto.rs +++ b/src/dto.rs @@ -11,12 +11,7 @@ pub struct LogEntryDto { pub sequence: u64, pub timestamp_ms: i64, pub text: String, - /// Derived param values (ticket 001's `ParamDeclaration`s) for this line, - /// keyed by param name. Only params whose regex actually matched this - /// line's text (and whose captured group parsed as the declared `kind`) - /// are present — a non-matching param is omitted rather than sent as - /// null, so the frontend can render a blank/dash for anything missing - /// from the map. + /// Only params whose regex matched (and parsed as the declared kind) are present — omitted rather than sent as null, so the frontend can render a blank for anything missing. pub params: BTreeMap, } @@ -33,9 +28,9 @@ pub struct MetricDeclarationDto { pub name: String, pub message_match_regex: String, pub params: Vec, - /// See `crate::engine::DeclareMetricRequest::matched_labels` (ticket 025). + /// See `crate::engine::DeclareMetricRequest::matched_labels`. pub matched_labels: Option>, - /// See `crate::engine::DeclareMetricRequest::unmatched_labels` (ticket 025). + /// See `crate::engine::DeclareMetricRequest::unmatched_labels`. pub unmatched_labels: Option>, } @@ -60,9 +55,7 @@ pub struct LogsParams { pub start_ms: Option, pub end_ms: Option, pub limit: Option, - /// Raw FilterBar query string (ticket 007), e.g. `regex("error") and - /// sequence<5`. Parsed with `filter_parser::parse` and evaluated in the - /// engine; a malformed string surfaces as `EngineResponse::Error`. + /// Raw FilterBar query string, e.g. `regex("error") and sequence<5`. pub filter: Option, } @@ -71,14 +64,11 @@ pub struct TimelineParams { pub start_ms: Option, pub end_ms: Option, pub bucket_ms: Option, - /// Same FilterBar query string as `LogsParams::filter`: only logs - /// matching it are counted into timeline buckets. + /// Same FilterBar query string as `LogsParams::filter`; only matching logs are counted into timeline buckets. pub filter: Option, } -/// One entry from a metric's derived bucket timeseries: the label combination -/// that bucket represents, plus one `(sequence, timestamp, value)` occurrence -/// within it. +/// One (sequence, timestamp, value) occurrence within a metric's bucket for a given label combination. #[derive(Debug, Clone, Serialize)] pub struct BucketValueDto { pub labels: BTreeMap, @@ -87,13 +77,7 @@ pub struct BucketValueDto { pub value: MetricPrimitive, } -/// Query params for `GET /api/metrics/:name`. `labels` is a single -/// comma-separated `key=value` list (e.g. `?labels=path=/a,duration=5`) -/// rather than repeated `label.foo=bar` params, since axum's `Query` -/// extractor needs a fixed/known set of field names — an arbitrary label set -/// doesn't fit that without a custom deserializer. Every listed label must -/// match (compared against each label value's `Display` form, see -/// `MetricPrimitive`); unlisted labels on a bucket are ignored. +/// `labels` is a comma-separated `key=value` list (not repeated params) since axum's `Query` extractor needs a fixed set of field names. #[derive(Debug, Deserialize)] pub struct BucketValuesParams { pub labels: Option, @@ -103,24 +87,13 @@ pub struct BucketValuesParams { pub limit: Option, } -/// One chart-ready time series returned by `GET /api/query` (ticket 013's PromQL -/// endpoint): a label combination plus its `(timestamp_ms, value)` points, sorted by -/// timestamp. `labels` uses plain `String` values (not `MetricPrimitive`) since every -/// value has already been through `MetricPrimitive`'s `Display` form — chart code -/// doesn't need the typed value, just something to show in a legend/tooltip. -/// -/// `value` is always `f64` (see `crate::engine::coerce_to_f64` for how a bucket's -/// `MetricPrimitive` is coerced into one), since a chart needs a plottable number -/// regardless of the underlying metric primitive's kind. +/// `labels` uses plain `String` (already Display-formatted) since charts just need something to show; `value` is always `f64` for plottability. #[derive(Debug, Clone, Serialize, PartialEq)] pub struct PromqlSeriesDto { pub labels: BTreeMap, pub points: Vec<(i64, f64)>, } -/// Query params for `GET /api/query`. `promql` is the query string (see -/// `crate::promql`); `start_ms`/`end_ms` bound which points are included, mirroring -/// `/api/logs`/`/api/metrics/:name`. #[derive(Debug, Deserialize)] pub struct PromqlParams { pub promql: String, @@ -128,17 +101,13 @@ pub struct PromqlParams { pub end_ms: Option, } -/// JSON error body returned by API handlers on a 4xx/5xx response, e.g. -/// `{ "error": "invalid regex: ..." }`. +/// JSON error body returned by API handlers on a 4xx/5xx response. #[derive(Debug, Serialize)] pub struct ErrorDto { pub error: String, } -/// Request body for `POST /api/settings/css`: raw CSS content to import and -/// store alongside the settings file. `is_theme` (ticket 033) marks this as -/// a selectable colour-scheme theme rather than arbitrary CSS; defaults to -/// `false` (arbitrary CSS) when omitted. +/// `is_theme` marks this as a selectable colour-scheme theme rather than arbitrary CSS; defaults to false. #[derive(Debug, Deserialize)] pub struct ImportCssRequest { pub filename: String, @@ -147,9 +116,7 @@ pub struct ImportCssRequest { pub is_theme: bool, } -/// Request body for `POST /api/settings/css/link`: a reference to an -/// external CSS path/URL that ottyl doesn't copy or own. `is_theme` (ticket -/// 033) marks this as a selectable theme rather than arbitrary CSS. +/// A reference to an external CSS path/URL that ottyl doesn't copy or own. #[derive(Debug, Deserialize)] pub struct LinkCssRequest { pub path_or_url: String, @@ -157,14 +124,7 @@ pub struct LinkCssRequest { pub is_theme: bool, } -/// Response shape for a single CSS entry, used both by the list endpoint and -/// by mutation endpoints that return the affected entry. `id` is the entry's -/// real id for `Imported` entries, or a stable id derived from the -/// linked path/URL for `Linked` entries (see `settings::css_entry_id`) — -/// this lets the frontend key/reference both kinds of entries uniformly, -/// even though only `Imported` entries are deletable/content-fetchable. -/// `is_theme` (ticket 033) distinguishes a theme entry (selectable from the -/// theme picker) from arbitrary imported/linked CSS. +/// `id` is the real id for `Imported` entries or a stable id derived from the path/URL for `Linked` ones, so the frontend can key both kinds uniformly (only `Imported` entries are deletable/content-fetchable). #[derive(Debug, Clone, Serialize)] pub struct CssEntryDto { pub id: String, @@ -202,19 +162,14 @@ impl From<&CssEntry> for CssEntryDto { } } -/// Response shape for `GET /api/settings` and `PATCH /api/settings` (ticket -/// 011): the subset of `Settings` the NavMenu UI needs to render a colour -/// scheme picker and know which profile is active. `css_entries` is -/// deliberately omitted here since it already has its own dedicated -/// `GET /api/settings/css` endpoint (ticket 010). +/// `css_entries` is omitted here since it has its own `GET /api/settings/css` endpoint. #[derive(Debug, Clone, Serialize)] pub struct SettingsDto { pub color_scheme: ColorScheme, pub active_profile: Option, - /// `None` means unlimited (ticket 026). + /// `None` means unlimited. pub max_logs: Option, - /// Id of the selected custom theme entry, if any (ticket 033). `None` - /// means `color_scheme` alone determines the active palette. + /// Id of the selected custom theme entry, if any. `None` means `color_scheme` alone determines the active palette. pub custom_theme_id: Option, } @@ -229,11 +184,7 @@ impl From<&Settings> for SettingsDto { } } -/// Request body for `PATCH /api/settings`. `color_scheme`: `None` leaves the -/// current value untouched. `max_logs`/`custom_theme_id`: each distinguishes -/// "field omitted" (leave unchanged, outer `None`) from "field present" -/// (inner `Option` applied verbatim — `null` clears it, a value sets it), via -/// `deserialize_double_option`. +/// `max_logs`/`custom_theme_id` distinguish "omitted" (leave unchanged) from "present" (null clears, value sets), via `deserialize_double_option`. #[derive(Debug, Deserialize)] pub struct UpdateSettingsRequest { pub color_scheme: Option, @@ -243,10 +194,7 @@ pub struct UpdateSettingsRequest { pub custom_theme_id: Option>, } -/// Distinguishes a missing JSON field (outer `None`, via `#[serde(default)]` -/// short-circuiting before this runs) from a present `null` (`Some(None)`) -/// or present value (`Some(Some(value))`) — plain `Option>` can't -/// tell "absent" and "explicit null" apart on its own. +/// Distinguishes a missing field (outer `None`) from present `null` (`Some(None)`) or present value (`Some(Some(value))`), which plain `Option>` can't do alone. fn deserialize_double_option<'de, D, T>(deserializer: D) -> Result>, D::Error> where D: serde::Deserializer<'de>, @@ -255,10 +203,6 @@ where Option::::deserialize(deserializer).map(Some) } -/// Request body for `POST /api/settings/alerts` (ticket 017): creates a new -/// alert rule (see `crate::alerts::Settings::create_alert_rule`). `AlertRule` -/// itself is returned directly as the response body (it already derives -/// `Serialize`), so there's no separate response DTO here. #[derive(Debug, Deserialize)] pub struct CreateAlertRuleRequest { pub name: String, @@ -267,8 +211,6 @@ pub struct CreateAlertRuleRequest { pub sink: AlertSink, } -/// Request body for `PUT /api/settings/alerts/:id`: replaces every field -/// except `id` (taken from the URL, see `Settings::update_alert_rule`). #[derive(Debug, Deserialize)] pub struct UpdateAlertRuleRequest { pub name: String, @@ -277,19 +219,13 @@ pub struct UpdateAlertRuleRequest { pub sink: AlertSink, } -/// Request body for `POST /api/settings/alerts/test` (ticket 020): delivers -/// a test notification to an ntfy topic/server without requiring a saved -/// rule. `server` defaults to `https://ntfy.sh` when absent, same as -/// `AlertSink::Ntfy`. +/// `server` defaults to `https://ntfy.sh` when absent. #[derive(Debug, Deserialize)] pub struct TestAlertNotificationRequest { pub topic: String, pub server: Option, } -/// Summary of one profile for `GET /api/settings/profiles` (ticket 011): -/// just the name and declaration/dashboard counts the Presets panel needs to -/// render a list, rather than the full param/metric declaration bodies. #[derive(Debug, Clone, Serialize)] pub struct ProfileSummaryDto { pub name: String, @@ -309,15 +245,12 @@ impl From<&Profile> for ProfileSummaryDto { } } -/// Request body for `POST /api/settings/profiles`: creates a new, empty -/// profile with this name (see `ProfileStore::create`). #[derive(Debug, Deserialize)] pub struct CreateProfileRequest { pub name: String, } -/// One panel within a dashboard (ticket 014): a PromQL query plus how to -/// chart its result. Mirrors `profile::Panel`. +/// Mirrors `profile::Panel`. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct PanelDto { pub id: String, @@ -348,9 +281,7 @@ impl From<&PanelDto> for Panel { } } -/// A saved dashboard (ticket 014), as returned by -/// `GET /api/settings/profiles/:name/dashboards` and friends. Mirrors -/// `profile::Dashboard`. +/// Mirrors `profile::Dashboard`. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct DashboardDto { pub id: String, @@ -378,18 +309,12 @@ impl From<&DashboardDto> for Dashboard { } } -/// Request body for `POST /api/settings/profiles/:name/dashboards`: creates -/// a new, empty dashboard with this name (see `ProfileStore::create_dashboard`). #[derive(Debug, Deserialize)] pub struct CreateDashboardRequest { pub name: String, } -/// Request body for -/// `PUT /api/settings/profiles/:name/dashboards/:dashboard_id`: replaces the -/// dashboard's `name`/`panels` (see `ProfileStore::save_dashboard`). The -/// dashboard `id` in the URL is authoritative — any `id` in the body is -/// ignored. +/// The dashboard `id` in the URL is authoritative; any `id` in the body is ignored. #[derive(Debug, Deserialize)] pub struct SaveDashboardRequest { pub name: String, diff --git a/src/engine.rs b/src/engine.rs index e5359cf..cfd61ff 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -23,9 +23,7 @@ pub struct IngestedLine { pub text: String, } -/// Also the request body for `POST /api/params` (ticket 004): `Deserialize` is -/// derived directly on this engine type rather than adding a separate DTO, -/// since its shape is already exactly what the API needs. +/// Also the request body for `POST /api/params` — `Deserialize` is derived directly here rather than adding a separate DTO, since the shape already matches. #[derive(Debug, Clone, Deserialize)] pub struct DeclareParamRequest { pub name: String, @@ -34,43 +32,16 @@ pub struct DeclareParamRequest { pub kind: MetricPrimitiveKind, } -/// v1 mechanism: a metric declaration's params are derived purely from inline named -/// capture groups on the metric's own `message_match_regex` (each -/// `RegexToMetricPrimitiveMatcher::capture_group` must name a capture group on that -/// regex). Referencing an already-declared `ParamDeclaration` (001) by name is not -/// supported yet — that's left for a future ticket. -/// -/// Also the request body for `POST /api/metrics` (ticket 004) — see -/// `DeclareParamRequest`'s doc comment for why `Deserialize` lives here rather -/// than on a separate DTO. +/// Params are derived purely from inline named capture groups on this metric's own regex — referencing an already-declared `ParamDeclaration` by name isn't supported yet. Also the request body for `POST /api/metrics` (hence `Deserialize`). #[derive(Debug, Clone, Deserialize)] pub struct DeclareMetricRequest { pub name: String, pub message_match_regex: String, pub params: Vec, - /// Ticket 025: fixed labels added on top of whatever `params` captured - /// when the line *does* match `message_match_regex`. Together with - /// `unmatched_labels` below, this is what lets a single metric+query - /// produce a "matched X" vs "didn't match X" pair of series (e.g. the - /// seeded default dashboard's error/non-error split) with clean, - /// consistent label values, rather than whatever case/substring the - /// regex happened to capture (a case-insensitive `error` match could - /// capture `"ERROR"`, `"Error"`, etc., fragmenting what should be one - /// series into several). `#[serde(default)]` so existing declarations - /// without this field are unaffected. + /// Fixed labels layered on top of what `params` captured, so e.g. a case-insensitive match doesn't fragment into "ERROR"/"Error" series; `#[serde(default)]` so existing declarations are unaffected. #[serde(default)] pub matched_labels: Option>, - /// When a line does *not* match `message_match_regex`, and this is - /// `Some`, the metric still registers one occurrence — labeled with - /// these fixed values rather than anything captured (there's nothing to - /// capture on a non-match) — instead of the line being invisible to this - /// metric. This is the other half of the "matched X vs didn't match X" - /// pair described above: plain regex alternation can't express the - /// "didn't match" side without lookaround (unsupported by the `regex` - /// crate), since an alternation branch that's a catch-all always wins at - /// the search's leftmost position, so it can never tell "contains X - /// later in the line" apart from "X is absent entirely". `#[serde(default)]` - /// so existing declarations without this field keep matching only. + /// When a line doesn't match, registers one occurrence labeled with these fixed values instead of being invisible to this metric — plain regex alternation can't express "didn't match" without lookaround; `#[serde(default)]` so existing declarations keep matching-only behavior. #[serde(default)] pub unmatched_labels: Option>, } @@ -82,11 +53,7 @@ pub enum EngineQuery { Stats, ParamDeclarations, MetricDeclarations, - /// Bucket values for one metric (ticket 004's `GET /api/metrics/:name`). - /// `labels_filter`, when present, keeps only buckets where every listed - /// label matches — compared against each label value's `Display` form - /// (see `MetricPrimitive`) rather than a typed value, since the filter - /// arrives as plain strings off the query string. + /// `labels_filter` compares against each label's `Display` form (not a typed value), since the filter arrives as plain strings off the query string. BucketValues { metric_name: String, labels_filter: Option>, @@ -95,12 +62,7 @@ pub enum EngineQuery { since_sequence: Option, limit: Option, }, - /// PromQL query (ticket 013's `GET /api/query`). `query` is parsed here (rather - /// than by the API handler) so parsing and evaluation both happen inside the - /// single-owner engine loop, matching every other `EngineQuery` variant's - /// pattern of evaluating in place against `index` with no copying. A malformed - /// query is therefore only discovered once this reaches `handle_query`, hence - /// `EngineResponse::Promql` carrying a `Result` rather than a bare `Vec`. + /// `query` is parsed inside the engine (not the API handler) so it stays in the single-owner loop with no copying; a malformed query is only discovered here, hence `EngineResponse::Promql` carries a `Result`. Promql { query: String, start_ms: Option, end_ms: Option }, } @@ -123,10 +85,7 @@ pub enum EngineMessage { name: String, respond_to: oneshot::Sender, }, - /// Sets the cap on ingested logs kept in memory (ticket 026). `None` - /// means unlimited. Sent on startup (from `Settings::max_logs`) and - /// whenever the setting changes at runtime, so the cap takes effect on - /// the next ingest without a restart. + /// `None` means unlimited; sent on startup and whenever the setting changes so the cap applies without a restart. SetMaxLogs(Option), Query { query: EngineQuery, @@ -142,30 +101,20 @@ pub enum EngineResponse { ParamDeclarations(Vec), MetricDeclarations(Vec), BucketValues(Vec), - /// `Err` holds a human-readable message for a malformed query (see - /// `crate::promql::parse_query`), which the API layer maps to a 400 response. + /// `Err` holds a human-readable message for a malformed query, which the API layer maps to a 400 response. Promql(Result, String>), - /// A query-level error, e.g. a malformed `filter` string on - /// `EngineQuery::Logs`/`Timeline` (ticket 007). Kept as a variant on the - /// existing response enum, rather than making `handle_query` return a - /// `Result`, so every query path still funnels through the same - /// `oneshot::Sender` — callers (`api.rs`) just add one - /// more match arm to turn it into a 4xx. + /// Kept as a variant (rather than making `handle_query` return a `Result`) so every query path still funnels through the same `oneshot::Sender`. Error(String), } pub type EngineHandle = mpsc::Sender; -/// Sole owner of the primary index. Runs single-threaded so the map never needs a -/// lock, and does its own response serialization/DTO construction so callers never -/// have to clone the index to answer a query. +/// Sole owner of the primary index; runs single-threaded so the map never needs a lock, and builds DTOs itself so callers never clone the index. pub async fn run(mut rx: mpsc::Receiver, settings: SettingsHandle) { let mut index: BTreeMap = BTreeMap::new(); let mut max_logs: Option = None; let mut log_count: usize = 0; - // Ticket 018: per-rule rolling windows / seen-value sets, keyed by rule - // id. Lives only as long as this loop (no restart persistence — see - // `alerts::AlertCondition::NewValue`'s doc comment). + // Per-rule rolling windows/seen-value sets, keyed by rule id; lives only as long as this loop (no restart persistence). let mut alert_states: HashMap = HashMap::new(); while let Some(message) = rx.recv().await { @@ -180,9 +129,7 @@ pub async fn run(mut rx: mpsc::Receiver, settings: SettingsHandle apply_all_metrics(&mut index, sequence, timestamp, &text); evict_excess_logs(&mut index, &mut log_count, max_logs); - // Ticket 018: evaluate alert rules against just this line - // (not the whole log range), then hand any fired rule off to - // ticket 019's delivery, off the hot path via `tokio::spawn`. + // Evaluate alert rules against just this line, then hand any fired rule off to delivery via tokio::spawn so it stays off the hot path. let rules = settings.lock().unwrap().alert_rules.clone(); let fired = alerts::evaluate_alerts(&rules, &mut alert_states, sequence, timestamp, &text); for (rule, message) in fired { @@ -217,9 +164,7 @@ pub async fn run(mut rx: mpsc::Receiver, settings: SettingsHandle } } -/// Validates the regex and capture group before storing, so a bad declaration is -/// rejected up front rather than silently stored (or discovered later at derivation -/// time). +/// Validates the regex and capture group before storing, so a bad declaration is rejected up front rather than discovered later. fn declare_param( index: &mut BTreeMap, request: DeclareParamRequest, @@ -244,9 +189,7 @@ fn declare_param( Ok(()) } -/// Validates the regex and every param matcher's capture-group reference before -/// storing. All matchers are checked (not just the first bad one) so a rejection can -/// report every problem at once. +/// Validates every param matcher's capture-group reference (not just the first bad one) so a rejection reports every problem at once. fn declare_metric( index: &mut BTreeMap, request: DeclareMetricRequest, @@ -298,9 +241,7 @@ fn declare_metric( Ok(()) } -/// Parses a single captured substring into the `MetricPrimitive` its param declares. -/// Returns `None` on a parse failure (e.g. a `Number` param whose capture isn't -/// numeric), which the caller treats as "this match doesn't apply". +/// Returns `None` on a parse failure, which the caller treats as "this match doesn't apply". fn parse_metric_primitive(raw: &str, kind: MetricPrimitiveKind) -> Option { match kind { MetricPrimitiveKind::String => Some(MetricPrimitive::String(raw.to_string())), @@ -315,52 +256,12 @@ fn parse_metric_primitive(raw: &str, kind: MetricPrimitiveKind) -> Option raw.parse::().ok().map(MetricPrimitive::Duration), } } -/// Runs one metric's regex against a single log line and, on a match, appends a -/// bucket value entry: this is the "counter increment" (ticket 022) — a match is -/// an occurrence at a point in time, optionally labeled. -/// -/// `Bucket.labels` holds every `Label`-role param (the default role — see -/// [`ParamRole`]); at most one param may instead be `Value`-role (enforced at -/// declare time), and its parsed primitive becomes the pushed event's value. -/// With no `Value`-role param, the pushed value is a fixed presence marker -/// (`MetricPrimitive::Number(Number::Int(1))`), matching the original v1 -/// behavior where a bucket's `values` was purely the timeseries of *when that -/// label combination occurred* — histogram()/gauge() derived queries -/// (`crate::promql::AggOp`) read this same `value`, so a `Value`-role param is -/// what makes those meaningful (bucketing/reading an occurrence-count-of-1 -/// series degenerates to "everything in one bucket"/"always 1"). If a param's -/// capture group doesn't match, or its captured text fails to parse as its -/// declared `kind`, the whole match is skipped for this line rather than -/// storing a partially-labeled bucket. -/// -/// A `Value`-role param of kind `Duration` (ticket 023) is a special case: its -/// value isn't parsed from captured text at all. It's derived as the elapsed -/// time (using this event's ingestion `timestamp`) since the *previous* -/// counter event with this exact label set, tracked independently via -/// `IndexKey::LastMetricEvent` (not `Bucket.values` itself, since — see -/// below — a `Bucket` entry isn't always pushed). On the first occurrence of -/// a given label set there is no previous event to diff against, so — per the -/// ticket's "omit rather than emit a bogus zero" requirement — no `Bucket` -/// entry is pushed for that occurrence at all, rather than a zero/placeholder -/// duration; the occurrence is still recorded in `LastMetricEvent` so the -/// *next* occurrence of that label set has something to diff against. -/// -/// When `regex` does *not* match and `unmatched_labels` is `Some` (ticket -/// 025), a fixed occurrence is registered anyway, labeled with those literal -/// values — see `DeclareMetricRequest::unmatched_labels` for why this exists -/// instead of a plain regex alternation. When it *does* match, `matched_labels` -/// (if `Some`) is layered on top of whatever `params` captured, so the -/// "matched" side can use the same clean literal values as the "unmatched" -/// side — see `DeclareMetricRequest::matched_labels`. +/// At most one param may be `Value`-role (its primitive becomes the event's value, otherwise a fixed `1` marker is used); any param failing to match/parse skips the whole match rather than storing a partial one. A `Value`-role `Duration` param is elapsed time since the previous same-labeled event, so the first occurrence pushes no bucket (nothing to diff against) rather than a bogus zero. When `regex` doesn't match and `unmatched_labels` is `Some`, an occurrence is still registered with those fixed labels. fn apply_metric_to_log( index: &mut BTreeMap, metric_name: &str, @@ -413,12 +314,7 @@ fn apply_metric_to_log( } let value = if is_duration { - // Bookkeeping key is separate from the `Bucket` key below: a - // `Duration` value's first occurrence for this label set pushes no - // `Bucket` entry (nothing to diff against), so the "last event" - // timestamp can't live inside `Bucket.values` — it has to be tracked - // independently so the *next* occurrence still has something to - // diff against. + // Tracked separately from Bucket: a Duration's first occurrence pushes no Bucket entry, so the last-event timestamp can't live in Bucket.values. let last_event_key = IndexKey::LastMetricEvent { metric_name: metric_name.to_string(), labels: labels.clone() }; let previous_timestamp = match index.get(&last_event_key) { @@ -456,9 +352,7 @@ fn push_bucket_value( } } -/// Applies a newly-declared metric to every already-ingested log, so a metric -/// declared after logs have already arrived still gets populated buckets for them -/// (backfill), rather than only reflecting logs ingested from that point on. +/// Backfills buckets for a newly-declared metric against every already-ingested log, not just future ones. fn backfill_metric( index: &mut BTreeMap, metric_name: &str, @@ -485,9 +379,6 @@ fn backfill_metric( } } -/// Runs every currently-declared metric's regex against one newly-ingested log line, -/// updating whichever buckets match. A line matching zero metrics leaves the bucket -/// range untouched. fn apply_all_metrics( index: &mut BTreeMap, sequence: u64, @@ -529,11 +420,7 @@ fn apply_all_metrics( } } -/// Bucket values for one metric, in insertion order, optionally filtered to a single -/// exact label combination. Used directly by tests (exact-match filtering) and, via -/// `EngineQuery::BucketValues` in `handle_query` below, by the metrics API (ticket -/// 004) — which layers its own textual label-filter and time/sequence-range -/// filtering on top rather than using this function's `labels_filter` parameter. +/// The metrics API layers its own textual label-filter and range filtering on top rather than using this function's `labels_filter` param (used directly by tests). fn bucket_values<'a>( index: &'a BTreeMap, metric_name: &'a str, @@ -553,42 +440,7 @@ fn bucket_values<'a>( .flatten() } -/// A [`FilterExpr`] compiled against a *snapshot* of the current index, ready -/// to be evaluated per-log-line without touching the index again. -/// -/// ## Design: how metric comparisons resolve against buckets -/// -/// A FilterBar comparison like `my_metric[level]==5` can't be evaluated by -/// re-running `my_metric`'s regex against each log line at query time — that -/// would duplicate work the engine already did at ingest/backfill time -/// (ticket 003), and would require re-parsing the metric's declaration for -/// every query. Instead, since ticket 003 already derived, for every metric, -/// one `Bucket` per distinct label combination holding the `(sequence, -/// timestamp, _)` occurrences that produced it, resolving a metric -/// comparison is a **one-time scan of that metric's buckets per query** -/// (via [`bucket_values`]), rather than a per-log-line regex re-run: -/// -/// 1. For `metric[param] value`, scan every bucket for `metric` (this -/// reuses the same ticket-003 range-scan `bucket_values` already uses for -/// `GET /api/metrics/:name`), and keep the buckets whose `labels[param]` -/// satisfies ` value`. -/// 2. Every `(sequence, ..)` occurrence recorded in a kept bucket is a log -/// line the comparison matches. Those sequences are collected into a -/// `HashSet` once, up front — [`CompiledFilter::MetricMatch`]. -/// 3. Evaluating the comparison for a given log line is then just an `O(1)` -/// `HashSet::contains(&sequence)` check, reusing the fact that a line's -/// bucket membership (which buckets a line matched) was already computed -/// when the line was ingested/backfilled, rather than recomputed here. -/// -/// `sequence`/`timestamp` comparisons skip buckets entirely: they compare -/// straight against the log's own `IndexKey::IngestedLog` fields, so they're -/// compiled into their own lightweight variants ([`CompiledFilter::Sequence`] -/// / [`CompiledFilter::Timestamp`]) instead of [`CompiledFilter::MetricMatch`]. -/// -/// A metric comparison with no `[param]` isn't meaningful under this model — -/// v1's buckets have no single "the metric's value", only a per-label-combo -/// occurrence marker (see `apply_metric_to_log`'s doc comment) — so -/// `compile_filter` rejects it with a clear error instead of guessing. +/// A [`FilterExpr`] compiled against a snapshot of the index, so evaluating per log line touches the index only once: a `metric[param]` comparison is pre-resolved into a `HashSet` of matching sequences ([`CompiledFilter::MetricMatch`]) for O(1) per-line lookup instead of a regex re-run; a comparison with no `[param]` is rejected since v1 buckets have no single "the metric's value", only a per-label-combo occurrence marker. enum CompiledFilter { Regex(Regex), Sequence { op: CompareOp, target: f64 }, @@ -598,11 +450,7 @@ enum CompiledFilter { Or(Box, Box), } -/// Parses and compiles a FilterBar query string against `index`, resolving -/// every regex/metric-comparison/datetime-literal up front so evaluating it -/// per log line (see [`eval_compiled`]) never touches the index or re-parses -/// anything. `now` anchors relative datetime literals (e.g. `-5m`) so every -/// comparison in one query is resolved against the same instant. +/// Resolves every regex/metric-comparison/datetime-literal up front so per-line evaluation ([`eval_compiled`]) never touches the index; `now` anchors relative literals so every comparison in one query resolves against the same instant. fn compile_filter_query( index: &BTreeMap, filter: &str, @@ -637,12 +485,7 @@ fn compile_filter( } } -/// `sequence value` — compares straight against the log's own sequence -/// number. `value` is normally `FilterValue::Number` (the grammar parses a -/// bare integer as a number); a `Raw` value is accepted too as long as it -/// still parses as a number (defensive - the grammar shouldn't produce this -/// for `sequence`, but nothing stops a caller from writing `sequence<"5"`-ish -/// input that the value token still captures as text). +/// A `Raw` value is accepted defensively (parsed as a number) even though the grammar shouldn't produce one for `sequence`. fn compile_sequence_comparison(op: CompareOp, value: &FilterValue) -> Result { let target = match value { FilterValue::Number(n) => *n, @@ -653,11 +496,7 @@ fn compile_sequence_comparison(op: CompareOp, value: &FilterValue) -> Result value` — compares straight against the log's own -/// timestamp. A `FilterValue::Number` is treated as raw milliseconds (no -/// datetime parsing needed); a `FilterValue::Raw` token (e.g. `12:00`, -/// `-5m`, `2024-01-15`) is resolved via `filter_datetime::parse_datetime` -/// against `now`. +/// A `Number` is treated as raw milliseconds; a `Raw` token is resolved via `filter_datetime::parse_datetime` against `now`. fn compile_timestamp_comparison( op: CompareOp, value: &FilterValue, @@ -672,9 +511,7 @@ fn compile_timestamp_comparison( Ok(CompiledFilter::Timestamp { op, target_ms }) } -/// `metric[param] value` for any metric name other than the reserved -/// `sequence`/`timestamp` — see [`CompiledFilter`]'s doc comment for the -/// overall approach. +/// `metric[param] value` for any metric name other than `sequence`/`timestamp` — see [`CompiledFilter`]'s doc comment. fn compile_metric_comparison( index: &BTreeMap, metric: &str, @@ -710,14 +547,7 @@ fn compile_metric_comparison( Ok(CompiledFilter::MetricMatch(matching)) } -/// Compares one bucket label's `MetricPrimitive` against a filter's -/// right-hand side. Numbers compare numerically (a `Raw` value is parsed as -/// a number); timestamps compare against a resolved datetime (a `Number` -/// value is treated as raw milliseconds, a `Raw` value goes through -/// `filter_datetime::parse_datetime`); strings compare lexicographically -/// against the raw token (or a number's textual form). Mismatched kinds that -/// can't be meaningfully compared (e.g. a string label against a bare -/// number) simply never match, rather than erroring. +/// Mismatched kinds that can't be meaningfully compared (e.g. a string label against a bare number) simply never match, rather than erroring. fn compare_metric_primitive( primitive: &MetricPrimitive, op: CompareOp, @@ -754,8 +584,7 @@ fn compare_metric_primitive( Ok(ordering.is_some_and(|ordering| apply_compare_op(ordering, op))) } -/// `pub(crate)` so `crate::alerts` can reuse it for `Threshold`/`Ratio` -/// comparisons instead of duplicating the match. +/// `pub(crate)` so `crate::alerts` can reuse it for `Threshold`/`Ratio` comparisons instead of duplicating the match. pub(crate) fn apply_compare_op(ordering: Ordering, op: CompareOp) -> bool { match op { CompareOp::Lt => ordering == Ordering::Less, @@ -767,10 +596,7 @@ pub(crate) fn apply_compare_op(ordering: Ordering, op: CompareOp) -> bool { } } -/// Evaluates a compiled filter against one log line. Cheap: every regex, -/// datetime literal, and bucket scan was already resolved by -/// [`compile_filter_query`], so this is just pattern matching, numeric -/// comparisons, and `HashSet` lookups — no index access. +/// Cheap: everything was already resolved by [`compile_filter_query`], so this is just pattern matching and `HashSet` lookups — no index access. fn eval_compiled(compiled: &CompiledFilter, sequence: u64, timestamp: Timestamp, text: &str) -> bool { match compiled { CompiledFilter::Regex(re) => re.is_match(text), @@ -832,11 +658,7 @@ fn param_declarations_range( }) } -/// Compiles every currently-declared param's regex once, so `derive_log_params` -/// can be applied to many lines without recompiling each param's regex per -/// line. A param whose stored regex somehow fails to compile (shouldn't -/// happen — `declare_param` validates it before storing) is skipped rather -/// than panicking. +/// A param whose stored regex fails to compile (shouldn't happen — `declare_param` validates it) is skipped rather than panicking. fn compiled_param_declarations( index: &BTreeMap, ) -> Vec<(String, Regex, String, MetricPrimitiveKind)> { @@ -849,12 +671,7 @@ fn compiled_param_declarations( .collect() } -/// Runs every declared param's regex against one log line's text, returning -/// the derived `MetricPrimitive` values keyed by param name. A param is -/// omitted from the result (rather than included as null/empty) when its -/// regex doesn't match the line, its capture group doesn't match, or the -/// captured text fails to parse as the param's declared `kind` — see -/// `LogEntryDto::params`. +/// A param is omitted (not included as null/empty) when its regex/capture group doesn't match or the captured text fails to parse — see `LogEntryDto::params`. fn derive_log_params( text: &str, declarations: &[(String, Regex, String, MetricPrimitiveKind)], @@ -875,11 +692,7 @@ fn derive_log_params( params } -/// Evicts the oldest (lowest-`sequence`) ingested logs until `log_count` is -/// back at or under `max_logs`. `sequence` numbering is not renumbered on -/// eviction, and any `Bucket` values already derived from an evicted log are -/// left in place — buckets are aggregate data, not raw logs, so they're not -/// retroactively pruned. +/// Bucket values already derived from an evicted log are left in place — buckets are aggregate data, not raw logs, so they're not retroactively pruned. fn evict_excess_logs( index: &mut BTreeMap, log_count: &mut usize, @@ -921,16 +734,7 @@ fn handle_query(index: &BTreeMap, query: EngineQuery) -> En None => None, }; let limit = params.limit.unwrap_or(500).min(10_000); - // Compile each currently-declared param's regex once up front, then - // apply the compiled set to every *returned* line below (ticket 015). - // Perf note: this is a straightforward per-returned-line, - // per-declared-param regex scan — O(lines_returned * declared_params) - // — rather than precomputing/caching derived params for the whole log - // range. That's fine at this codebase's current scale (v1), but if - // either the log volume or number of declared params grows a lot, - // this should move to incremental derivation at ingest time (mirroring - // how `apply_all_metrics` already does for metric buckets) instead of - // recomputing on every `/api/logs` query. + // O(lines_returned * declared_params) per query — fine at v1 scale; if volume/params grow, move to incremental derivation at ingest time like apply_all_metrics does for buckets. let compiled_params = compiled_param_declarations(index); let entries: Vec = logs_range(index) .filter(|(sequence, timestamp, _)| { @@ -1068,18 +872,7 @@ fn handle_query(index: &BTreeMap, query: EngineQuery) -> En } } -/// Coerces a bucket's stored `MetricPrimitive` into a plottable `f64` for -/// `PromqlSeriesDto`: -/// - `Number` uses its underlying numeric value. -/// - `Timestamp` uses its millisecond value (arbitrary but at least meaningful/ -/// orderable, since there's no other sensible single number for a point in time). -/// - `String` has no numeric reading at all, so it falls back to `1.0` — i.e. it is -/// treated the same as the presence/occurrence marker `apply_metric_to_log` already -/// stores for every bucket value today (ticket 003), meaning a PromQL series over a -/// `String`-valued bucket becomes an occurrence-count series rather than being -/// skipped. This keeps every declared metric queryable (matching the ticket's -/// "documented fallback" requirement) rather than silently omitting non-numeric -/// series. +/// Coerces a bucket's `MetricPrimitive` into a plottable `f64`: Number/Timestamp/Duration use their natural numeric value; `String` has no numeric reading, so it falls back to `1.0` (matching the presence/occurrence marker used for every bucket) rather than being skipped, keeping every declared metric queryable. fn coerce_to_f64(value: &MetricPrimitive) -> f64 { match value { MetricPrimitive::Number(n) => match n { @@ -1092,10 +885,7 @@ fn coerce_to_f64(value: &MetricPrimitive) -> f64 { } } -/// True when every matcher in `matchers` is satisfied by `labels` (compared against -/// each label value's `Display` form, same convention as `EngineQuery::BucketValues`'s -/// `labels_filter`). Unmatched/extra labels on the bucket are ignored — a matcher list -/// only needs to be a subset. +/// Compared against each label's `Display` form; extra labels on the bucket are ignored — the matcher list only needs to be a subset. fn matches_label_matchers( labels: &BTreeMap, matchers: &[LabelMatcher], @@ -1105,26 +895,7 @@ fn matches_label_matchers( }) } -/// Evaluates a parsed PromQL query against `index`, producing chart-ready series. -/// -/// Baseline (no aggregation wrapper): one output series per distinct label -/// combination that matches `parsed.metric_name` + `parsed.matchers`, with that -/// combination's bucket values as `(timestamp_ms, value)` points (see -/// `coerce_to_f64`), sorted by timestamp. -/// -/// `sum()`/`avg()`: merges every matching series' points by exact millisecond -/// timestamp (summing, or averaging, the values that land on the same timestamp -/// across series) into a single output series with empty labels — the aggregation -/// collapses the label dimension, same as PromQL's `sum by ()`/`avg by ()` default. -/// -/// `rate()`: requires (and, per `promql::parse_query`, is only ever given) a range -/// selector. This is a simplified, single-point-per-series rate rather than PromQL's -/// true sliding-window-per-evaluation-step semantics: for each matching series, it -/// takes the window `(last_point_ts - range, last_point_ts]` and divides the number -/// of points falling in that window by the range's duration in seconds, producing one -/// `(last_point_ts, rate)` point per series. Since bucket values are occurrence -/// markers rather than cumulative counters (ticket 003), an occurrence-count-per- -/// second reading is the meaningful analogue of PromQL's counter-increase rate here. +/// Evaluates a parsed PromQL query against `index`, producing chart-ready series: baseline is one series per label combination; `sum()`/`avg()` merge series by timestamp into one label-less series; `rate()` is simplified to a single point over `(last_ts - range, last_ts]` (an occurrence-count-per-second reading, not PromQL's true sliding-window-per-step) since bucket values are occurrence markers, not cumulative counters. fn evaluate_promql( index: &BTreeMap, parsed: &PromqlQuery, @@ -1190,10 +961,7 @@ fn evaluate_promql( .collect() } Some(AggOp::Gauge) => { - // Ticket 022: collapse each label combination's counter events - // down to their most-recent value in the queried time range — - // "value at time T" derived from the last matching event, rather - // than a distinct stored gauge kind. + // Collapse each label combination's events down to their most-recent value in range — "value at time T", not a distinct stored gauge kind. series_points .into_iter() .filter_map(|(labels, points)| { @@ -1212,12 +980,7 @@ fn evaluate_promql( .flat_map(|points| points.iter().map(|(ts, _)| *ts)) .max() .unwrap_or(0); - // Ticket 022: bucket every matching counter event's value - // (across all label combinations — histogram collapses the - // label dimension the same way sum()/avg() do) into fixed-width - // buckets, one output series per bucket labeled `le` (Prometheus's - // "less-than-or-equal" convention) holding a single - // `(last_ts, count)` point. + // Buckets every value (across all label combinations, collapsing the label dimension like sum()/avg()) into fixed-width buckets, labeled `le` (Prometheus's less-than-or-equal convention). let mut counts: BTreeMap = BTreeMap::new(); for points in series_points.values() { for (_, value) in points { @@ -1667,8 +1430,7 @@ mod tests { assert_eq!(series.len(), 1); assert_eq!(series[0].labels.get("path").map(String::as_str), Some("/a")); - // Occurrence markers coerce to 1.0 (see `coerce_to_f64`); timestamps come from - // `ingest_log`'s `sequence as millis` helper. + // Occurrence markers coerce to 1.0 (see coerce_to_f64); timestamps come from ingest_log's sequence-as-millis helper. assert_eq!(series[0].points, vec![(0, 1.0), (2, 1.0)]); } @@ -1835,8 +1597,7 @@ mod tests { let mut series = evaluate_promql(&index, &parsed, None, None); series.sort_by(|a, b| a.labels.get("le").cmp(&b.labels.get("le"))); - // durations 2,4 -> bucket [0,10) le=10 (count 2); 12,15 -> [10,20) le=20 (count 2); - // 21 -> [20,30) le=30 (count 1). + // durations 2,4 -> le=10 (count 2); 12,15 -> le=20 (count 2); 21 -> le=30 (count 1). assert_eq!(series.len(), 3); assert_eq!(series[0].labels.get("le").map(String::as_str), Some("10")); assert_eq!(series[0].points[0].1, 2.0); @@ -1871,9 +1632,7 @@ mod tests { ingest_log(&mut index, 2, "user=alice logged in"); let values: Vec<_> = bucket_values(&index, "login_gap", None).collect(); - // First occurrences (alice@0, bob@1) produce no duration value at all — - // only alice's second occurrence (sequence 2) has a previous event to - // diff against. + // First occurrences (alice@0, bob@1) produce no duration value — only alice's second occurrence (seq 2) has something to diff against. assert_eq!(values.len(), 1); let (labels, (sequence, timestamp, value)) = &values[0]; assert_eq!(labels.get("user").map(|v| v.to_string()), Some("alice".to_string())); @@ -1912,10 +1671,7 @@ mod tests { assert_eq!(series[0].points, vec![(10, 10.0)]); } - // Fixed matched_labels/unmatched_labels (rather than a captured "category" - // param) so a case-insensitive match like "ERROR" vs "Error" still - // collapses into one clean "error" series instead of fragmenting by - // whatever case the source text happened to use. + // Fixed labels (not a captured param) so a case-insensitive match collapses into one clean "error" series regardless of source-text case. fn declare_error_category_metric(index: &mut BTreeMap) { declare_metric( index, @@ -2016,7 +1772,7 @@ mod tests { } } - // -- ticket 007: FilterBar query integration ----------------------- + // -- FilterBar query integration -- fn logs_query(filter: Option<&str>) -> EngineQuery { EngineQuery::Logs(LogsParams { @@ -2107,10 +1863,7 @@ mod tests { ingest_log(&mut index, 1, "level=info duration=2 boom"); ingest_log(&mut index, 2, "level=error duration=3 all good"); - // Only sequence 0 has both the "boom" regex match *and* level=error. - // Note: comparison values are bare tokens (see `value` in the - // grammar), not quoted string literals like `regex(...)`'s argument - // — so `level==error`, not `level=="error"`. + // Only sequence 0 has both; note comparison values are bare tokens (level==error), not quoted string literals like regex()'s argument. match handle_query( &index, logs_query(Some(r#"regex("boom") and requests[level]==error"#)), diff --git a/src/filter_datetime.rs b/src/filter_datetime.rs index 9b67a9b..af07993 100644 --- a/src/filter_datetime.rs +++ b/src/filter_datetime.rs @@ -1,24 +1,9 @@ -//! Datetime parsing for the filter query language's timestamp comparisons. -//! -//! This module is intentionally standalone: it has no dependency on the engine/index -//! types or the filter grammar module, so it can be developed, tested, and reused -//! independently of the rest of the filter machinery. -//! -//! Three input forms are supported, all normalized to a single [`Timestamp`]: -//! -//! 1. Relative humantime, e.g. `5h`, `-3m` — resolved against a supplied "now" at -//! parse/eval time (parsed with the `humantime` crate). -//! 2. Absolute time-of-day, e.g. `12:00` — resolves to that time on the current day. -//! **This module treats "current day" as the UTC calendar day of `now`, and resolves -//! the time-of-day in UTC.** (We use `chrono` for date/time parsing since no other -//! datetime crate was already a dependency.) -//! 3. Full date/datetime literals, e.g. `2024-01-15` or `2024-01-15T12:00:00Z` -//! (RFC3339-ish). A bare date is interpreted as midnight UTC on that date. +//! Datetime parsing for the filter query language's timestamp comparisons: relative +//! humantime, time-of-day (resolved against the UTC calendar day of `now`), or a +//! full date/datetime literal. -// `engine.rs` (ticket 007) calls `parse_datetime` to resolve `FilterValue::Raw` -// tokens (e.g. `-5m`, `12:00`, a full date) in `timestamp`/metric-comparison -// filter clauses; `#![allow(dead_code)]` stays since most of this module's -// surface is still only exercised by its own unit tests below. +// `#![allow(dead_code)]` stays since most of this module's surface is still only +// exercised by its own unit tests below. #![allow(dead_code)] use chrono::{DateTime, NaiveDate, NaiveTime, TimeZone, Utc}; @@ -28,9 +13,7 @@ use crate::timestamp::Timestamp; /// Error returned when a datetime filter literal can't be resolved to a [`Timestamp`]. #[derive(Debug, Clone, PartialEq, Eq)] pub enum DateTimeParseError { - /// The input matched the shape of a supported format (relative duration, - /// time-of-day, or date/datetime) but its value was invalid or ambiguous, - /// e.g. an out-of-range time like `25:99`. + /// The input matched a supported format's shape but its value was invalid, e.g. `25:99`. Invalid { input: String, reason: String }, /// The input didn't match any supported format at all. Unparseable(String), @@ -54,11 +37,8 @@ impl std::fmt::Display for DateTimeParseError { impl std::error::Error for DateTimeParseError {} -/// Parses a filter datetime literal into an absolute [`Timestamp`], resolving any -/// relative or day-relative forms against `now`. -/// -/// `now` is taken as a parameter (rather than this function calling `Timestamp::now()` -/// internally) so callers can pass a fixed value in tests. +/// Parses a filter datetime literal into an absolute [`Timestamp`]; `now` is a parameter +/// (rather than calling `Timestamp::now()` internally) so callers can pass a fixed value in tests. pub fn parse_datetime(input: &str, now: Timestamp) -> Result { let trimmed = input.trim(); if trimmed.is_empty() { @@ -80,11 +60,8 @@ pub fn parse_datetime(input: &str, now: Timestamp) -> Result Option> { let (sign, rest) = match input.strip_prefix('-') { Some(rest) => (-1i128, rest), @@ -94,11 +71,7 @@ fn try_relative(input: &str, now: Timestamp) -> Option Option Option> { - // Only attempt this path for inputs shaped like a date (contain a '-' separating - // digits, e.g. "2024-01-15"); otherwise let other parsers have a shot. + // Only attempt this path for inputs shaped like a date, e.g. "2024-01-15". let looks_like_date = input.len() >= 8 && input.as_bytes()[4] == b'-'; if !looks_like_date { return None; @@ -151,8 +123,7 @@ fn try_full_datetime(input: &str) -> Option) -> Timestamp { mod tests { use super::*; - /// A fixed "now": 2024-06-15T12:34:56Z. fn fixed_now() -> Timestamp { let dt = Utc.with_ymd_and_hms(2024, 6, 15, 12, 34, 56).unwrap(); Timestamp::from_millis(dt.timestamp_millis()) diff --git a/src/filter_parser.rs b/src/filter_parser.rs index c653d5d..7289bad 100644 --- a/src/filter_parser.rs +++ b/src/filter_parser.rs @@ -1,26 +1,5 @@ -//! FilterBar query language: grammar, AST and parser. -//! -//! This module is intentionally standalone and has no dependency on the -//! engine/index types (`data_value`, `index_key`, `engine`, ...). It only -//! knows how to turn a filter string into an [`FilterExpr`] AST. Evaluating -//! that AST against real log data, and resolving [`FilterValue::Raw`] tokens -//! into concrete datetimes, is left to later integration tickets. -//! -//! Grammar (see `src/filter_parser.pest`): -//! -//! - `regex("...")` — raw-text regex search over log lines. -//! - `metric_name[param] value` — metric comparisons, with `` one -//! of `<`, `>`, `<=`, `>=`, `==`, `!=`. -//! - `sequence value` and `timestamp value` — reserved metric -//! names usable the same way, without a `[param]`. -//! - `and` / `or` boolean combinators, with `and` binding tighter than `or`. -//! - Parenthesized sub-expressions. -//! -//! `engine.rs` (ticket 007) is what actually calls into this module: it -//! compiles the returned [`FilterExpr`] against the index (resolving -//! [`FilterValue::Raw`] tokens via `filter_datetime` and metric comparisons -//! via bucket data) and evaluates it per log line while answering -//! `EngineQuery::Logs`/`Timeline`. +//! FilterBar query language: grammar (see `src/filter_parser.pest`), AST and parser. +//! Evaluating the AST against real log data is left to `engine.rs`. #![allow(dead_code)] use pest::Parser; @@ -48,9 +27,8 @@ pub enum FilterExpr { Or(Box, Box), } -/// Comparison operators supported by metric comparisons. Also reused by -/// `crate::alerts::AlertCondition::Threshold`/`Ratio` for their own -/// threshold/ratio comparisons (hence `Serialize`/`Deserialize`). +/// Comparison operators for metric comparisons; also reused by `crate::alerts::AlertCondition` +/// for threshold/ratio comparisons (hence `Serialize`/`Deserialize`). #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] pub enum CompareOp { @@ -62,10 +40,8 @@ pub enum CompareOp { Ne, } -/// The right-hand side of a comparison. Values that parse as a number -/// become [`FilterValue::Number`]; everything else (e.g. datetime-like -/// tokens such as `12:00`) is kept as an opaque [`FilterValue::Raw`] string -/// to be resolved by a later ticket. +/// The right-hand side of a comparison: values that parse as a number become +/// [`FilterValue::Number`], everything else is kept as an opaque [`FilterValue::Raw`] string. #[derive(Debug, Clone, PartialEq)] pub enum FilterValue { Number(f64), @@ -96,9 +72,7 @@ impl std::error::Error for FilterParseError {} impl From> for FilterParseError { fn from(err: pest::error::Error) -> Self { - // pest's own Display already renders a located, human-readable - // message (line/column + a pointer into the offending input), so we - // use it as the basis and add a short prefix for context. + // pest's Display already renders a located message, so use it and add a short prefix. FilterParseError::new(format!("failed to parse filter expression: {err}")) } } @@ -121,9 +95,7 @@ pub fn parse(input: &str) -> Result { fn build_or_expr(pair: Pair) -> Result { debug_assert_eq!(pair.as_rule(), Rule::or_expr); - // `or_kw` is atomic (see the grammar file) so it is *not* silent: it - // shows up as a sibling pair between the `and_expr` operands. Skip it - // rather than assuming a fixed alternating structure. + // `or_kw` is atomic so it's not silent, showing up as a sibling pair between `and_expr` operands — skip it rather than assuming a fixed alternating structure. let mut and_exprs = pair .into_inner() .filter(|p| p.as_rule() != Rule::or_kw); @@ -140,7 +112,7 @@ fn build_or_expr(pair: Pair) -> Result { fn build_and_expr(pair: Pair) -> Result { debug_assert_eq!(pair.as_rule(), Rule::and_expr); - // Same reasoning as above for `and_kw`. + // Same reasoning as `build_or_expr` for `and_kw`. let mut terms = pair .into_inner() .filter(|p| p.as_rule() != Rule::and_kw); @@ -236,8 +208,7 @@ fn parse_value(s: &str) -> FilterValue { } } -/// Unescape a regex string literal's inner contents (`\"` -> `"`, `\\` -> `\`, -/// any other `\x` -> `x`). +/// Unescape a regex string literal's inner contents (`\"` -> `"`, `\\` -> `\`, `\x` -> `x`). fn unescape(s: &str) -> String { let mut out = String::with_capacity(s.len()); let mut chars = s.chars(); @@ -368,8 +339,6 @@ mod tests { let result = parse("regex(\"unterminated and sequence<"); assert!(result.is_err()); let err = result.unwrap_err(); - // Ensure the error message is human-readable and carries location - // info (pest errors always mention line/column context). let msg = err.to_string(); assert!(!msg.is_empty()); } diff --git a/src/index_key.rs b/src/index_key.rs index 2215d13..1490b6e 100644 --- a/src/index_key.rs +++ b/src/index_key.rs @@ -3,11 +3,7 @@ use crate::timestamp::Timestamp; use serde::Serialize; use std::collections::BTreeMap; -/// Key type for the engine's primary index. -/// -/// Variants are ordered top to bottom as declared (derived `Ord` compares the -/// discriminant first), which is what carves the map into contiguous ranges: -/// logs, then params, then metric declarations, then buckets. +/// Key type for the engine's primary index. Variant order matters: derived `Ord` compares the discriminant first, carving the map into contiguous ranges in declaration order (logs, params, metric declarations, buckets). #[allow(dead_code)] #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize)] pub enum IndexKey { @@ -33,13 +29,7 @@ pub enum IndexKey { metric_name: String, labels: BTreeMap, }, - /// Ticket 023: internal bookkeeping (never returned by a query) tracking - /// the timestamp of the most recent counter event for one metric+label - /// combination, independent of whether that event actually pushed a - /// `Bucket` value — a `Duration`-kind `Value` param's *first* occurrence - /// of a label set has no previous event to diff against and pushes no - /// `Bucket` entry, but must still be recorded here so the *next* - /// occurrence can compute its elapsed time. + /// Internal bookkeeping tracking the last event timestamp per metric+label, so a `Duration` param's first occurrence (which pushes no `Bucket`) still lets the next occurrence compute elapsed time. LastMetricEvent { metric_name: String, labels: BTreeMap, diff --git a/src/ingest.rs b/src/ingest.rs index bdd461f..c152651 100644 --- a/src/ingest.rs +++ b/src/ingest.rs @@ -30,9 +30,7 @@ pub async fn run(engine: mpsc::Sender) { } } -/// Identifies a specific file instance on disk, so a recreated path (e.g. a -/// tailed process restarting and reopening its log file) can be told apart from -/// the same file simply growing. +/// Identifies a specific file instance on disk, so a recreated path can be told apart from the same file simply growing. #[derive(PartialEq, Eq, Clone, Copy)] struct FileId(#[cfg(unix)] u64, #[cfg(not(unix))] ()); @@ -55,13 +53,9 @@ async fn open(path: &Path) -> Option<(File, FileId)> { Some((file, file_id(&meta))) } -/// Tails `path` like `tail -f`: reads existing content, then keeps reading as -/// new lines are appended. If the file is deleted and recreated at the same -/// path (or truncated in place) — e.g. the tailed process restarted and -/// reopened its log — tailing resumes against the new file from its start, -/// rather than exiting or getting stuck. `sequence` keeps incrementing across -/// a recreation event; it's ottyl's own line counter, not tied to the source -/// file. +/// Tails `path` like `tail -f`, resuming from the start if the file is deleted/recreated or +/// truncated in place rather than exiting or getting stuck; `sequence` keeps incrementing +/// across a recreation since it's ottyl's own line counter, not tied to the source file. pub async fn run_file(engine: mpsc::Sender, path: PathBuf) { let mut sequence: u64 = 0; @@ -81,9 +75,7 @@ pub async fn run_file(engine: mpsc::Sender, path: PathBuf) { let mut chunk = String::new(); match reader.read_line(&mut chunk).await { Ok(0) => { - // EOF: decide whether the file was truncated/recreated - // (tail the new instance from scratch) or just hasn't - // grown yet (keep polling). + // EOF: decide whether the file was truncated/recreated or just hasn't grown yet. match tokio::fs::metadata(&path).await { Ok(meta) => { let new_id = file_id(&meta); @@ -93,8 +85,7 @@ pub async fn run_file(engine: mpsc::Sender, path: PathBuf) { id = new_id; } Err(_) => { - // Path gone (mid-recreation); wait for it to - // reappear and reopen fresh. + // Path gone (mid-recreation); wait for it to reappear and reopen fresh. } } tokio::time::sleep(POLL_INTERVAL).await; @@ -119,8 +110,7 @@ pub async fn run_file(engine: mpsc::Sender, path: PathBuf) { return; } } - // else: partial line at EOF (write in progress) — keep - // buffering and read again without treating it as final. + // else: partial line at EOF (write in progress) — keep buffering. } Err(_) => continue 'reopen, } @@ -189,7 +179,6 @@ mod tests { assert_eq!(recv_text(&mut rx).await, "long line before truncation"); - // Truncate in place (same inode, shorter content) then write new data. let mut f = std::fs::OpenOptions::new() .write(true) .truncate(true) diff --git a/src/main.rs b/src/main.rs index 6886699..811d166 100644 --- a/src/main.rs +++ b/src/main.rs @@ -31,14 +31,11 @@ struct Args { #[arg(long, default_value_t = 0)] port: u16, - /// Path to the settings file. Defaults to a per-OS config location - /// (e.g. `~/.config/ottyl/settings.json` on Linux). + /// Path to the settings file. Defaults to a per-OS config location. #[arg(long)] settings_path: Option, - /// File to tail for log lines, instead of reading from stdin. Behaves like - /// `tail -f`: reads existing content then keeps following appended lines, - /// and keeps tailing if the file is deleted/recreated at the same path. + /// File to tail for log lines, instead of reading from stdin. Behaves like `tail -f`. #[arg(long)] file: Option, } @@ -60,13 +57,8 @@ async fn main() { let _ = engine_tx.send(engine::EngineMessage::SetMaxLogs(max_logs)).await; } - // Ticket 009: reload the active profile's param/metric declarations into the - // engine BEFORE starting ingest below. Ordering matters: declarations must be - // in place before any line is ingested so that backfill/live-derivation - // semantics (ticket 003) apply uniformly to every line, including ones that - // arrive immediately once ingest starts (there's no "declare after the fact - // and go back to reprocess early lines" step, so doing it after ingest starts - // could silently miss lines that raced ahead of the declarations). + // 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 mut settings_guard = settings.lock().unwrap(); diff --git a/src/metric_primitive.rs b/src/metric_primitive.rs index ab4b060..267651c 100644 --- a/src/metric_primitive.rs +++ b/src/metric_primitive.rs @@ -10,9 +10,7 @@ pub enum MetricPrimitive { String(String), Number(Number), Timestamp(Timestamp), - /// Milliseconds elapsed (ticket 023). Distinct from `Number` so a - /// duration reads unambiguously in JSON/label form, even though it's - /// stored the same way (a plain `i64`). + /// Milliseconds elapsed; distinct from `Number` so it reads unambiguously in JSON/label form. Duration(i64), } @@ -49,10 +47,7 @@ impl Ord for MetricPrimitive { } } -/// Textual form used for label-filter comparisons over the metrics API -/// (`GET /api/metrics/:name?labels=foo=bar`): query-string values are plain -/// strings, so a label filter is compared against this formatted form rather -/// than trying to parse the filter value into a typed `MetricPrimitive`. +/// Textual form used to compare label filters (query-string values) against a formatted `MetricPrimitive` rather than parsing them into one. impl fmt::Display for MetricPrimitive { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { @@ -74,10 +69,7 @@ pub enum Number { } impl Number { - /// Widens to `f64` for cross-kind numeric comparisons (e.g. a filter - /// query's `FilterValue::Number` against a declared metric's captured - /// `Number`). `pub(crate)` rather than private since ticket 007's filter - /// evaluator (in `engine.rs`) needs it and lives in a different module. + /// Widens to `f64` for cross-kind numeric comparisons; `pub(crate)` since the filter evaluator in `engine.rs` needs it. pub(crate) fn as_f64(&self) -> f64 { match self { Number::Int(v) => *v as f64, diff --git a/src/profile.rs b/src/profile.rs index 5f72346..f8fb82c 100644 --- a/src/profile.rs +++ b/src/profile.rs @@ -1,33 +1,5 @@ -//! Named, persistable bundles of parsing/indexing rules ("profiles" / "presets"), -//! ticket 009. -//! -//! A `Profile` bundles the param declarations (ticket 001) and metric -//! declarations (ticket 002) needed to derive a given CLI app's structure from -//! its raw log output, plus any saved dashboards for that app. Profiles are -//! collected in a `ProfileStore`, persisted as a single `profiles.json` file -//! under `Settings.profiles.profiles_dir` (resolved relative to the settings -//! file's directory — the same convention ticket 010 uses for CSS storage). A -//! single file (rather than one file per profile) is simplest and avoids -//! filename-sanitization concerns for profile names. -//! -//! `Settings.profiles` (see `settings.rs`) only tracks lightweight pointers — -//! known profile names and which one is active — while this module owns the -//! actual declaration content. That mirrors how `Settings.css_entries` holds -//! lightweight `CssEntry` records while the CSS content itself lives in -//! separate files (ticket 010). -//! -//! On startup, `main.rs` loads the active profile's declarations into a fresh -//! engine by replaying `EngineMessage::DeclareParam`/`DeclareMetric` for each -//! stored declaration (see `load_into_engine` below) — this reproduces the -//! same parsing/indexing rules the profile had when last saved, without -//! requiring the user to re-declare anything by hand after a restart. -//! -//! Several public methods here (e.g. `ProfileStore::delete`, -//! `record_param_declaration`) have no caller yet within this worktree — CRUD -//! over profiles isn't exposed over HTTP until ticket 004 lands the API layer -//! and calls into this module. `#![allow(dead_code)]` mirrors how -//! `engine.rs`/`data_value.rs` handle the same not-yet-wired situation for -//! tickets 001/002. +//! 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. #![allow(dead_code)] use std::collections::hash_map::DefaultHasher; @@ -46,22 +18,17 @@ 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 `ProfileStore::ensure_default`). pub const DEFAULT_PROFILE_NAME: &str = "Default"; -/// Mirrors `data_value::MetricPrimitiveKind` with `Serialize`/`Deserialize` so -/// profiles can round-trip through JSON. `MetricPrimitiveKind` itself only -/// derives `Serialize` (it's an engine-internal type owned by tickets -/// 001/002/003, out of scope for this ticket to modify), so this module keeps -/// its own mirror and converts at the boundary. +/// Mirrors `data_value::MetricPrimitiveKind` with `Serialize`/`Deserialize` so profiles can round-trip through JSON, since the engine-internal type only derives `Serialize`. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] pub enum ProfileMetricPrimitiveKind { String, Number, Timestamp, - /// Mirrors `MetricPrimitiveKind::Duration` (ticket 023). + /// Mirrors `MetricPrimitiveKind::Duration`. Duration, } @@ -87,7 +54,7 @@ impl From for MetricPrimitiveKind { } } -/// A stored param declaration, matching `DeclareParamRequest`'s shape (ticket 001). +/// A stored param declaration, matching `DeclareParamRequest`'s shape. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct ParamDeclarationRecord { pub name: String, @@ -124,26 +91,21 @@ pub struct MetricParamRecord { pub param_name: String, pub capture_group: String, pub kind: ProfileMetricPrimitiveKind, - /// See `ParamRole`. `#[serde(default)]` so profiles saved before ticket - /// 022 still load, with every param defaulting to `Label`. + /// See `ParamRole`; `#[serde(default)]` so profiles saved before this field existed still load, defaulting to `Label`. #[serde(default)] pub role: ParamRole, } -/// A stored metric declaration, matching `DeclareMetricRequest`'s shape (ticket 002). +/// A stored metric declaration, matching `DeclareMetricRequest`'s shape. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct MetricDeclarationRecord { pub name: String, pub message_match_regex: String, pub params: Vec, - /// See `crate::engine::DeclareMetricRequest::matched_labels` (ticket - /// 025). `#[serde(default)]` so profiles saved before that ticket still - /// load. + /// See `crate::engine::DeclareMetricRequest::matched_labels`; `#[serde(default)]` so older profiles still load. #[serde(default)] pub matched_labels: Option>, - /// See `crate::engine::DeclareMetricRequest::unmatched_labels` (ticket - /// 025). `#[serde(default)]` so profiles saved before that ticket still - /// load. + /// See `crate::engine::DeclareMetricRequest::unmatched_labels`; `#[serde(default)]` so older profiles still load. #[serde(default)] pub unmatched_labels: Option>, } @@ -190,8 +152,7 @@ impl From<&MetricDeclarationRecord> for DeclareMetricRequest { } } -/// A chart type a dashboard panel can render (ticket 014). Kept in sync with -/// the frontend's `'bar' | 'line' | 'pie'` union (see `webapp/src/lib/api.ts`). +/// A chart type a dashboard panel can render; kept in sync with the frontend's `'bar' | 'line' | 'pie'` union (see `webapp/src/lib/api.ts`). #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] pub enum ChartType { @@ -200,8 +161,7 @@ pub enum ChartType { Pie, } -/// One panel within a dashboard: a PromQL query (ticket 013) plus how to -/// chart its result. +/// One panel within a dashboard: a PromQL query plus how to chart its result. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct Panel { pub id: String, @@ -210,7 +170,7 @@ pub struct Panel { pub chart_type: ChartType, } -/// A named set of panels, saved as part of a profile (ticket 014). +/// A named set of panels, saved as part of a profile. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct Dashboard { pub id: String, @@ -219,10 +179,7 @@ pub struct Dashboard { pub panels: Vec, } -/// Generates an id for a newly-created dashboard/panel. Mirrors -/// `settings::generate_imported_css_id`: hashes a discriminating seed -/// (dashboard/panel name) together with the current time, avoiding a new -/// dependency (e.g. `uuid`) for what's otherwise a random-ish opaque string. +/// Generates an id by hashing a seed (dashboard/panel name) with the current time, avoiding a `uuid` dependency for what's otherwise an opaque string. fn generate_id(seed: &str) -> String { let mut hasher = DefaultHasher::new(); seed.hash(&mut hasher); @@ -235,28 +192,17 @@ fn generate_id(seed: &str) -> String { } impl Dashboard { - /// A new dashboard named `name`, with no panels yet. pub fn new(name: String) -> Dashboard { let id = generate_id(&name); Dashboard { id, name, panels: Vec::new() } } } -/// Ticket 025: name of the metric seeded into a brand-new `Default` profile, -/// used to drive its starter dashboard's two panels. +/// Name of the metric seeded into a brand-new `Default` profile, used to drive its starter dashboard's two panels. const SEEDED_METRIC_NAME: &str = "log_category"; const SEEDED_METRIC_LABEL: &str = "category"; -/// A counter (ticket 022) that classifies every ingested line into exactly -/// two buckets — `category="error"` or `category="other"` — without -/// capturing any text: `matched_labels`/`unmatched_labels` (ticket 025) apply -/// those fixed values directly, rather than a param capturing the literal -/// matched substring, so a case-insensitive match like `"ERROR"` vs `"Error"` -/// collapses into one clean series instead of fragmenting by source casing. -/// A plain regex alternation can't express the "didn't match" half of this -/// without lookaround, which the `regex` crate doesn't support (see -/// `crate::engine::DeclareMetricRequest::unmatched_labels`) — hence going -/// through the metric-declaration mechanism instead of the PromQL surface. +/// A counter that splits every line into `category="error"`/`"other"` via `matched_labels`/`unmatched_labels` rather than a captured param, so case variants like "ERROR" collapse into one series; the `regex` crate lacks the lookaround needed to express "didn't match" any other way. fn seeded_error_split_metric() -> MetricDeclarationRecord { MetricDeclarationRecord { name: SEEDED_METRIC_NAME.to_string(), @@ -267,10 +213,7 @@ fn seeded_error_split_metric() -> MetricDeclarationRecord { } } -/// The starter dashboard seeded into a brand-new `Default` profile (ticket -/// 025): a line chart and a pie chart, both querying the bare -/// `SEEDED_METRIC_NAME` counter so each renders both the "error" and "other" -/// series/slices from a single PromQL query, same as any user-created panel. +/// The starter dashboard seeded into a brand-new `Default` profile: a line chart and a pie chart, both querying the seeded error-split counter. fn seeded_default_dashboard() -> Dashboard { let mut dashboard = Dashboard::new("Overview".to_string()); dashboard.panels = vec![ @@ -290,9 +233,7 @@ fn seeded_default_dashboard() -> Dashboard { dashboard } -/// A named bundle of parsing/indexing rules for a particular CLI app: which -/// params and metrics to derive from its log output, plus any saved -/// dashboards. +/// A named bundle of parsing/indexing rules for a particular CLI app, plus any saved dashboards. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct Profile { pub name: String, @@ -300,13 +241,11 @@ pub struct Profile { pub params: Vec, #[serde(default)] pub metrics: Vec, - /// Saved dashboards (ticket 014), each a named set of PromQL-backed panels. #[serde(default)] pub dashboards: Vec, } impl Profile { - /// A new, empty profile with the given name. pub fn new(name: String) -> Profile { Profile { name, @@ -317,18 +256,7 @@ impl Profile { } } -/// Sends every param/metric declaration stored in `profile` to the engine at -/// `engine`, as `EngineMessage::DeclareParam`/`DeclareMetric`. Used on startup -/// to reload the active profile's rules into a fresh engine (see `main.rs`), -/// and in tests to simulate that reload. -/// -/// A stored declaration should generally already be valid — it round-tripped -/// through the engine's own validation when originally declared (see -/// `record_param_declaration`/`record_metric_declaration` below) — but this -/// doesn't assume that: any declaration the engine rejects (or any send/recv -/// failure, e.g. the engine channel closing) is logged to stderr and skipped -/// rather than panicking, so one bad stored declaration can't take the whole -/// process down. +/// Replays every param/metric declaration stored in `profile` into `engine` (used on startup and in tests); any rejected declaration or send/recv failure is logged to stderr and skipped rather than panicking. pub async fn load_into_engine(engine: &EngineHandle, profile: &Profile) { for record in &profile.params { let (respond_to, response) = oneshot::channel(); @@ -388,8 +316,7 @@ pub enum ProfileError { NotFound, /// A profile with the given name already exists. DuplicateName, - /// The `Default` profile can't be deleted — there must always be at - /// least one profile to fall back to. + /// The `Default` profile can't be deleted — there must always be at least one profile to fall back to. CannotDeleteDefault, /// Underlying I/O failure reading/writing the profile store or settings file. Io(io::Error), @@ -414,9 +341,7 @@ impl From for ProfileError { } } -/// Resolves `settings.profiles.profiles_dir` relative to `settings_path`'s -/// parent directory (or treats it as absolute if it already is one) — the -/// same directory convention ticket 010 uses for CSS storage. +/// 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() { @@ -429,25 +354,19 @@ pub fn resolve_profiles_dir(settings: &Settings, settings_path: &Path) -> PathBu } } -/// Path to the single file holding all profiles, within `profiles_dir` (see -/// `resolve_profiles_dir`). +/// 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") } -/// On-disk collection of all known profiles, persisted as a single JSON file -/// (see module docs for why one file rather than one-per-profile). +/// 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 file is treated as an - /// empty store (not an error) — there's no reason to fail startup just - /// because no profiles have been created yet. Any I/O or parse error - /// while reading an existing file is logged to stderr and treated as an - /// empty store, mirroring `Settings::load`'s non-fatal-error handling. + /// 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 { match fs::read_to_string(store_path) { Ok(contents) => match serde_json::from_str::>(&contents) { @@ -471,8 +390,7 @@ impl ProfileStore { } } - /// Persists the store to `store_path` as pretty-printed JSON, creating - /// any missing parent directories first. + /// 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)?; @@ -482,26 +400,15 @@ impl ProfileStore { fs::write(store_path, contents) } - /// All known profiles. pub fn list(&self) -> &[Profile] { &self.profiles } - /// Looks up a profile by name. pub fn get(&self, name: &str) -> Option<&Profile> { self.profiles.iter().find(|p| p.name == name) } - /// Creates a new profile named `name`, persists the store to - /// `store_path`, and records the name in `settings.profiles.names` - /// (persisting `settings` to `settings_path` too, keeping the two files in - /// sync). Fails with `ProfileError::DuplicateName` if a profile with that - /// name already exists. - /// - /// The new profile starts as a copy of the currently active profile's - /// params/metrics/dashboards (or empty, if none is active) — a preset is - /// most useful as a variation on the one you were just using, not a - /// blank slate. + /// 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, @@ -528,13 +435,7 @@ impl ProfileStore { Ok(()) } - /// Deletes the profile named `name`, persists the store to `store_path`, - /// and drops it from `settings.profiles.names` (persisting `settings` to - /// `settings_path` too). If the deleted profile was active, clears - /// `settings.profiles.active_profile`. Fails with `ProfileError::NotFound` - /// if no such profile exists, or `ProfileError::CannotDeleteDefault` for - /// the `Default` profile — there must always be at least one profile to - /// fall back to. + /// 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, @@ -557,21 +458,7 @@ impl ProfileStore { Ok(()) } - /// Guarantees a `Default` profile exists in this store, that it's tracked - /// in `settings.profiles.names`, and that `settings.profiles.active_profile` - /// falls back to it when nothing else is active — so the app always has - /// at least one usable profile, even on a first run with no settings file - /// yet. Persists both the store and `settings` if either changed. - /// - /// A newly-created `Default` profile (not one that already existed) is - /// seeded with a starter dashboard (ticket 025): a line chart and a pie - /// chart of "error" vs. non-"error" log volume, both querying - /// `seeded_error_split_metric`'s counter — editable/deletable afterward - /// like any other dashboard/metric, and never re-seeded or duplicated on - /// a later call once the profile exists. - /// - /// Called once on startup (see `main.rs`) before anything else touches - /// profiles. + /// 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, @@ -595,14 +482,7 @@ impl ProfileStore { Ok(()) } - /// Sets (or clears, with `name: None`) the active profile: updates - /// `settings.profiles.active_profile` and persists `settings` to - /// `settings_path`. Fails with `ProfileError::NotFound` if `name` is - /// `Some` and no such profile exists in this store. - /// - /// This only updates `Settings`; it does not itself reload the newly - /// active profile's declarations into a running engine (that happens on - /// the next startup — see `load_into_engine` and `main.rs`). + /// 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, @@ -619,22 +499,7 @@ impl ProfileStore { Ok(()) } - /// Records a newly-declared param into `profile_name`'s stored - /// declarations (replacing any existing record with the same param name, - /// so re-declaring updates it in place), persisting the store to - /// `store_path`. - /// - /// **Integration hook for ticket 004 (HTTP API)**: when the HTTP API - /// declares a param against the engine (sending - /// `EngineMessage::DeclareParam`), it should also call this — with the - /// same `DeclareParamRequest` — against the currently-active profile, so - /// the declaration survives a restart (see `load_into_engine`). This - /// ticket (009) cannot wire that call itself: there's no declare-param - /// HTTP route yet for it to hook into. Call shape: - /// - /// ```ignore - /// profile_store.record_param_declaration(&store_path, &active_profile_name, &request)?; - /// ``` + /// 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. pub fn record_param_declaration( &mut self, store_path: &Path, @@ -655,13 +520,7 @@ impl ProfileStore { Ok(()) } - /// Records a newly-declared metric into `profile_name`'s stored - /// declarations (replacing any existing record with the same metric name), - /// persisting the store to `store_path`. - /// - /// **Integration hook for ticket 004 (HTTP API)**: same as - /// `record_param_declaration` above, but for - /// `EngineMessage::DeclareMetric`/`DeclareMetricRequest`. + /// 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`. pub fn record_metric_declaration( &mut self, store_path: &Path, @@ -682,16 +541,13 @@ impl ProfileStore { Ok(()) } - /// Lists the dashboards saved on `profile_name`. Fails with - /// `ProfileError::NotFound` if no such profile exists. + /// 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)?; Ok(&profile.dashboards) } - /// Creates a new, empty dashboard named `name` on `profile_name`, - /// persisting the store to `store_path`. Returns the new dashboard. - /// Fails with `ProfileError::NotFound` if no such profile exists. + /// Creates a new, empty dashboard named `name` on `profile_name`, persisting the store; fails with `NotFound` if the profile doesn't exist. pub fn create_dashboard( &mut self, store_path: &Path, @@ -709,12 +565,7 @@ impl ProfileStore { Ok(dashboard) } - /// Replaces the dashboard identified by `dashboard_id` on `profile_name` - /// with `dashboard` (e.g. after adding/removing/editing panels), - /// persisting the store to `store_path`. `dashboard.id` is forced to - /// `dashboard_id` so a caller can't accidentally change a dashboard's - /// identity through this call. Fails with `ProfileError::NotFound` if - /// either the profile or the dashboard doesn't exist. + /// 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. pub fn save_dashboard( &mut self, store_path: &Path, @@ -738,10 +589,7 @@ impl ProfileStore { Ok(dashboard) } - /// Deletes the dashboard identified by `dashboard_id` from - /// `profile_name`, persisting the store to `store_path`. Fails with - /// `ProfileError::NotFound` if either the profile or the dashboard - /// doesn't exist. + /// Deletes the dashboard identified by `dashboard_id` from `profile_name`, persisting the store; fails with `NotFound` if either doesn't exist. pub fn delete_dashboard( &mut self, store_path: &Path, @@ -838,7 +686,6 @@ mod tests { assert!(store.get("web").is_some()); assert_eq!(settings.profiles.names, vec!["web".to_string(), "worker".to_string()]); - // Duplicate creation is rejected. let dup = store.create(&store_path, &mut settings, &settings_path, "web".to_string()); assert!(matches!(dup, Err(ProfileError::DuplicateName))); @@ -870,8 +717,7 @@ mod tests { assert!(settings.profiles.names.contains(&DEFAULT_PROFILE_NAME.to_string())); assert_eq!(settings.profiles.active_profile, Some(DEFAULT_PROFILE_NAME.to_string())); - // Idempotent: doesn't duplicate the profile, and doesn't clobber an - // already-active (non-Default) profile. + // 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(); @@ -908,8 +754,7 @@ mod tests { assert!(dashboard.panels.iter().any(|p| p.chart_type == ChartType::Line)); assert!(dashboard.panels.iter().any(|p| p.chart_type == ChartType::Pie)); - // Reload (simulating a restart) and ensure_default again: must not - // re-seed or duplicate the dashboard/metric. + // 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(); @@ -1076,19 +921,7 @@ mod tests { assert_eq!(profile.metrics.len(), 1); } - /// Acceptance criterion: declaring params/metrics into a profile, "restarting" - /// (dropping and reloading the store from disk, rather than a literal process - /// restart), and replaying the saved profile against a fresh engine reproduces - /// the same declarations and still ingests lines correctly — without - /// re-declaring anything by hand. - /// - /// Note: ticket 003 (metric bucket derivation) isn't implemented yet in this - /// worktree, so there's no bucket index to compare directly. What this test - /// verifies instead is the part ticket 009 owns: that a profile's declarations - /// round-trip through disk byte-for-byte and, once replayed via - /// `load_into_engine`, produce identical engine-visible declarations (which is - /// exactly the state ticket 003's derivation logic will read from) — and that - /// the engine keeps ingesting lines normally afterwards. + /// 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. #[tokio::test] async fn profile_round_trip_reproduces_declarations_in_fresh_engine() { let dir = tempfile::tempdir().unwrap(); @@ -1096,8 +929,7 @@ mod tests { let settings_path = dir.path().join("settings.json"); let mut settings = Settings::default(); - // Declare directly into a profile (as ticket 004's HTTP API will do via the - // `record_*_declaration` hooks once it exists). + // 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 @@ -1135,8 +967,7 @@ mod tests { tokio::spawn(crate::engine::run(engine_rx, settings_handle)); load_into_engine(&engine_tx, &profile).await; - // Ingest a line and confirm the engine is functioning with the replayed - // declarations in place (no re-declaration needed). + // Ingest a line and confirm the engine works with the replayed declarations (no re-declaration needed). engine_tx .send(EngineMessage::Ingest(IngestedLine { sequence: 0, @@ -1185,9 +1016,7 @@ mod tests { assert_eq!(logs[0].text, "level=info request duration=42"); } - /// Acceptance criterion (ticket 014): creating a dashboard (with a panel), - /// then reloading the profile store from disk (simulating a restart), - /// still has the dashboard and its panel. + /// Acceptance criterion: a dashboard (with a panel) survives reloading the profile store from disk. #[test] fn dashboard_create_save_delete_round_trip_and_persists_across_reload() { let dir = tempfile::tempdir().unwrap(); diff --git a/src/promql.rs b/src/promql.rs index 8b876e1..0e3bfc9 100644 --- a/src/promql.rs +++ b/src/promql.rs @@ -1,47 +1,5 @@ -//! Hand-rolled parser for a small PromQL subset (ticket 013). -//! -//! Design decision: a crates.io PromQL parser (e.g. `promql-parser`) was considered -//! but rejected — the required subset here is tiny (metric name + label matchers, -//! plus an optional `[5m]` range and a single top-level `sum()`/`avg()`/`rate()` -//! wrapper), while a full PromQL parser brings a large, largely-unused AST plus an -//! extra maintenance dependency. Mirroring `filter_parser.rs`'s approach (a small, -//! engine-independent parser module) is simpler and easier to control/extend, and -//! keeps parsing fully decoupled from the engine's index — this module knows nothing -//! about `IndexKey`/`DataValue`, only `PromqlQuery`'s own types below. -//! -//! ## Supported grammar -//! -//! ```text -//! query := aggregation | vector_selector -//! aggregation := agg_op "(" vector_selector ")" -//! agg_op := "sum" | "avg" | "rate" | "histogram" | "gauge" -//! vector_selector := metric_name label_matchers? range_selector? -//! metric_name := [a-zA-Z_][a-zA-Z0-9_]* -//! label_matchers := "{" label_matcher ("," label_matcher)* "}" -//! label_matcher := label_name "=" quoted_string -//! label_name := [a-zA-Z_][a-zA-Z0-9_]* -//! quoted_string := '"' (any char except '"' or '\')* '"' -//! range_selector := "[" digits duration_unit? "]" -//! duration_unit := "s" | "m" | "h" | "d" -//! ``` -//! -//! Only exact-match label matchers (`=`) are supported — PromQL's `!=`, `=~`, `!~` -//! are out of scope for this ticket's required subset. `rate()` requires its inner -//! vector selector to carry a range selector with a duration unit (matching real -//! PromQL), and is rejected at parse time otherwise. -//! -//! Ticket 022 (metrics-as-counters rethink) adds two derived forms on top of the -//! same grammar, both consumed as regular `PromqlSeriesDto` series like every -//! other query shape: -//! - `histogram(metric_name[width])`: buckets every matching counter event's -//! (coerced-to-f64) value into fixed-width buckets of size `width` (a bare -//! number, no duration unit — reusing the `[...]` range-selector slot for a -//! different purpose than `rate()`'s duration), and emits one series per -//! bucket, labeled `le` (Prometheus's "less-than-or-equal" convention) with -//! that bucket's upper bound, holding a single `(timestamp_ms, count)` point. -//! - `gauge(metric_name)`: takes no range selector; emits the most-recent -//! matching counter event's value per label combination as a single point, -//! i.e. the counter's series collapsed down to its last value. +//! Hand-rolled parser for a small PromQL subset: metric name + label matchers, an optional `[5m]` range, and a top-level `sum()`/`avg()`/`rate()`/`histogram()`/`gauge()` wrapper. +//! Only exact-match (`=`) label matchers are supported; `rate()` requires a duration range selector, while `histogram(metric[width])` reuses that `[...]` slot for a bare bucket width instead. use std::time::Duration; @@ -50,10 +8,9 @@ pub enum AggOp { Sum, Avg, Rate, - /// Ticket 022: buckets counter values into fixed-width buckets. Requires - /// `bucket_width`. + /// Buckets counter values into fixed-width buckets; requires `bucket_width`. Histogram, - /// Ticket 022: the most-recent counter value per label combination. + /// The most-recent counter value per label combination. Gauge, } @@ -68,23 +25,19 @@ pub struct PromqlQuery { pub agg: Option, pub metric_name: String, pub matchers: Vec, - /// Duration range selector, e.g. `[5m]`. Only meaningful for `rate()`. + /// Duration range selector, e.g. `[5m]`; only meaningful for `rate()`. pub range: Option, - /// Bare-number range selector, e.g. `[10]`. Only meaningful for - /// `histogram()`, where it's the bucket width. + /// Bare-number range selector, e.g. `[10]`; only meaningful for `histogram()`, where it's the bucket width. pub bucket_width: Option, } -/// What a `[...]` range selector turned out to hold, disambiguated by whether -/// a duration unit followed the digits. +/// What a `[...]` range selector turned out to hold: a duration, or a bare width. enum RangeSelector { Duration(Duration), BareWidth(f64), } -/// Parses a PromQL query string per the grammar documented above. Returns a -/// human-readable error message (suitable for surfacing directly to an API caller) -/// on any malformed input. +/// Parses a PromQL query string, returning a human-readable error message on malformed input. pub fn parse_query(input: &str) -> Result { let mut parser = Parser::new(input); let query = parser.parse_query()?; @@ -143,7 +96,6 @@ impl<'a> Parser<'a> { } } - /// `[a-zA-Z_][a-zA-Z0-9_]*`, used for both metric names and label names. fn parse_identifier(&mut self) -> Result { self.skip_whitespace(); let rest = self.remaining(); @@ -214,9 +166,7 @@ impl<'a> Parser<'a> { Ok(matchers) } - /// `[]` (e.g. `[5m]`, a duration — used by `rate()`) or a - /// bare `[]` with no unit (a plain number — used by `histogram()` - /// as a bucket width). + /// Parses `[]` (a duration, e.g. `[5m]`) or a bare `[]` (a bucket width). fn parse_range_selector(&mut self) -> Result, String> { self.skip_whitespace(); if self.peek() != Some('[') { @@ -290,10 +240,7 @@ impl<'a> Parser<'a> { inner.agg = agg; Ok(inner) } else { - // Not (or not followed by) an aggregation call — rewind and parse the - // whole thing as a plain vector selector (covers metric names that - // happen to collide with an agg_op keyword, e.g. a metric named `rate` - // used without a call). + // Rewind and parse as a plain vector selector, covering metric names that collide with an agg_op keyword. self.pos = checkpoint; self.parse_vector_selector() } @@ -387,8 +334,7 @@ mod tests { #[test] fn rejects_unknown_aggregation() { - // `count` isn't a supported agg_op, so this parses as a bare metric named - // `count`, then trips on the unexpected trailing `(...)`. + // `count` isn't a supported agg_op, so this parses as a bare metric name and trips on the trailing `(...)`. let err = parse_query("count(requests)").unwrap_err(); assert!(err.contains("unexpected trailing input")); } diff --git a/src/settings.rs b/src/settings.rs index e17b671..c157f55 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -1,9 +1,4 @@ -//! Process-wide settings model: colour scheme, CSS entries, and profile -//! pointers. There is exactly one `Settings` instance at runtime, loaded once -//! on startup and persisted to disk whenever it changes (see `main.rs`). -//! -//! Persistence format is JSON (via `serde_json`, already a dependency) rather -//! than TOML, to keep the dependency list minimal. +//! Process-wide settings model: colour scheme, CSS entries, and profile pointers, persisted as JSON. use std::collections::hash_map::DefaultHasher; use std::fmt; @@ -19,8 +14,7 @@ use serde::{Deserialize, Serialize}; use crate::alerts::AlertRule; -/// Shared, process-wide settings handle. Cheap to clone; lock is only held -/// briefly since settings changes are rare (unlike the engine's hot path). +/// Shared, process-wide settings handle. Cheap to clone. pub type SettingsHandle = Arc>; /// User's preferred colour scheme for the webapp. @@ -40,17 +34,7 @@ pub enum ColorScheme { System, } -/// A single CSS entry configured for the webapp: either content that ottyl -/// stores itself ("imported") or a reference to an external path/URL that -/// the webapp links to directly ("linked", and therefore not deletable via -/// the settings API since ottyl doesn't own it). -/// -/// Ticket 010 builds the CSS import/link HTTP API around this shape. -/// -/// `is_theme` (ticket 033) marks an entry as a full colour-scheme theme (a -/// `:root { --c-*: ...; }` variable set, selectable from the theme picker) -/// rather than arbitrary CSS that's always injected. `#[serde(default)]` so -/// existing settings.json files without the field still load as `false`. +/// A single CSS entry: content ottyl stores itself ("imported") or a reference to an external path/URL ("linked", not deletable via the API). `is_theme` marks it as a selectable colour-scheme theme rather than always-injected CSS. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(tag = "kind", rename_all = "lowercase")] pub enum CssEntry { @@ -67,14 +51,10 @@ pub enum CssEntry { }, } -/// Placeholder shape for profile management (ticket 009 builds CRUD on top -/// of this). Profiles are expected to live as individual files under a -/// dedicated directory; this struct only tracks known profile names and -/// which one is currently active. +/// 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. Ticket 009 owns creating/loading the actual - /// profile files. + /// Names of known profiles. pub names: Vec, /// Name of the currently active profile, if any. pub active_profile: Option, @@ -83,27 +63,19 @@ pub struct Profiles { pub profiles_dir: String, } -/// The single, process-wide settings instance. -/// -/// Only `PartialEq` (not `Eq`): `alert_rules` carries `f64` fields -/// (`AlertCondition::Threshold`/`Ratio`), which aren't `Eq`. +/// 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)] pub struct Settings { pub color_scheme: ColorScheme, pub css_entries: Vec, - /// Id of the selected custom theme entry (a `css_entries` entry with - /// `is_theme: true`), if any (ticket 033). `None` means `color_scheme` - /// alone determines the active palette. `#[serde(default)]` so existing - /// settings.json files without this field still load. + /// 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, - /// Maximum number of ingested logs kept in memory. `None` (the default) - /// means unlimited — matches today's unbounded retention. + /// Maximum number of ingested logs kept in memory; `None` means unlimited. #[serde(default)] pub max_logs: Option, - /// Alert rule definitions (ticket 017). `#[serde(default)]` so existing - /// settings.json files without this field still load. + /// Alert rule definitions. #[serde(default)] pub alert_rules: Vec, } @@ -125,23 +97,13 @@ impl Default for Settings { } } -/// Computes the default settings file path using OS-conventional config -/// directories: `/settings.json`, where `config_dir` comes from -/// `directories::ProjectDirs::from("", "", "ottyl")`. -/// -/// Returns `None` if no valid config directory could be determined for the -/// current OS/user (this mirrors `ProjectDirs::from`'s own fallibility). +/// 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`. If the file doesn't exist yet, a default - /// `Settings` is created, persisted to `path`, and returned. - /// - /// Any I/O or parse error while reading an existing file is treated as - /// non-fatal: it is logged to stderr and a default `Settings` is - /// returned (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. pub fn load(path: &Path) -> Settings { match fs::read_to_string(path) { Ok(contents) => match serde_json::from_str(&contents) { @@ -174,8 +136,7 @@ impl Settings { } } - /// Persists these settings to `path` as pretty-printed JSON, creating - /// any missing parent directories first. + /// Persists these settings to `path` as pretty-printed JSON, creating any missing parent directories first. pub fn save(&self, path: &Path) -> io::Result<()> { if let Some(parent) = path.parent() { fs::create_dir_all(parent)?; @@ -186,18 +147,14 @@ impl Settings { } } -/// Error returned by CSS-entry mutations and lookups (ticket 010). +/// Error returned by CSS-entry mutations and lookups. #[derive(Debug)] pub enum CssError { /// No entry with the given id exists. NotFound, - /// The entry exists but is a `Linked` entry, not an `Imported` one, so - /// the requested operation (delete / read-content) doesn't apply: ottyl - /// never owns a linked entry's content, so there's nothing to delete or - /// serve back. + /// The entry is a `Linked` entry, not `Imported` — ottyl doesn't own a linked entry's content, so there's nothing to delete or serve back. NotImported, - /// Underlying I/O failure: writing/reading/removing the CSS file on - /// disk, or persisting `settings.json`. + /// Underlying I/O failure: writing/reading/removing the CSS file, or persisting `settings.json`. Io(io::Error), } @@ -219,14 +176,7 @@ impl From for CssError { } } -/// Computes the id used to reference a `CssEntry` over the API. -/// -/// For `Imported` entries this is just the stored id. `Linked` entries have -/// no stored id (ottyl doesn't own them), so one is derived deterministically -/// by hashing the `path_or_url`; this lets the frontend key/reference linked -/// entries the same way as imported ones (e.g. for list rendering) without -/// ottyl needing to persist an id it has no other use for. Because it's -/// derived from content, the same `path_or_url` always maps to the same id. +/// Computes the id used to reference a `CssEntry` over the API: the stored id for `Imported`, or a hash of `path_or_url` for `Linked` (which has no stored id), so the frontend can key both the same way. pub fn css_entry_id(entry: &CssEntry) -> String { match entry { CssEntry::Imported { id, .. } => id.clone(), @@ -240,10 +190,7 @@ fn linked_entry_id(path_or_url: &str) -> String { format!("linked-{:016x}", hasher.finish()) } -/// Generates an id for a newly-imported CSS entry. Not cryptographically -/// meaningful — just needs to be unique enough in practice: it hashes the -/// filename, content, and current time together, avoiding a new dependency -/// (e.g. `uuid`) for what's otherwise a random-ish opaque string. +/// Generates an id for a newly-imported CSS entry by hashing filename, content, and time, avoiding a `uuid` dependency for what's otherwise an opaque string. fn generate_imported_css_id(filename: &str, content: &str) -> String { let mut hasher = DefaultHasher::new(); filename.hash(&mut hasher); @@ -261,15 +208,7 @@ fn imported_css_file_path(css_dir: &Path, id: &str) -> PathBuf { } impl Settings { - /// Imports raw CSS `content`: writes it to `/.css` (id - /// generated from the filename/content/timestamp, see - /// `generate_imported_css_id`), records a `CssEntry::Imported { id, - /// filename, is_theme }` in `css_entries`, and persists settings to - /// `settings_path`. Returns the generated id. - /// - /// `css_dir` is expected to be the settings file's parent directory - /// (i.e. CSS files live alongside `settings.json`). `is_theme` (ticket - /// 033) marks this entry as a selectable theme rather than arbitrary CSS. + /// Imports raw CSS `content`: writes it to `/.css`, records a `CssEntry::Imported` in `css_entries`, and persists settings. Returns the generated id. `css_dir` is expected to be the settings file's parent directory. pub fn import_css( &mut self, settings_path: &Path, @@ -290,9 +229,7 @@ impl Settings { Ok(id) } - /// Registers a reference to an external CSS path/URL without copying - /// its content. Persists settings to `settings_path`. `is_theme` (ticket - /// 033) marks this entry as a selectable theme rather than arbitrary CSS. + /// Registers a reference to an external CSS path/URL without copying its content, and persists settings. pub fn link_css( &mut self, settings_path: &Path, @@ -307,14 +244,7 @@ impl Settings { Ok(()) } - /// Removes an imported CSS entry identified by `id` (see - /// `css_entry_id`): deletes its file under `css_dir` and drops it from - /// `css_entries`, then persists settings to `settings_path`. - /// - /// Returns `CssError::NotFound` if no entry matches `id`, and - /// `CssError::NotImported` if `id` refers to a `Linked` entry — linked - /// entries are never deletable this way since ottyl doesn't own their - /// content. + /// Removes an imported CSS entry identified by `id`: deletes its file under `css_dir`, drops it from `css_entries`, and persists settings. Fails with `NotFound` or `NotImported` (linked entries aren't deletable this way). pub fn remove_css( &mut self, settings_path: &Path, @@ -331,8 +261,7 @@ impl Settings { Some(CssEntry::Imported { .. }) => { let idx = index.unwrap(); self.css_entries.remove(idx); - // Best-effort: if the file is already gone, the settings - // reference is still what matters and has been removed above. + // Best-effort: the settings entry is already removed even if the file is gone. let _ = fs::remove_file(imported_css_file_path(css_dir, id)); self.save(settings_path)?; Ok(()) @@ -340,13 +269,7 @@ impl Settings { } } - /// Reads back the raw content of an imported CSS entry identified by - /// `id` (see `css_entry_id`). - /// - /// Returns `CssError::NotFound` if no entry matches `id`, and - /// `CssError::NotImported` if `id` refers to a `Linked` entry (there's no - /// local content to serve for those — the frontend should link/fetch the - /// original `path_or_url` directly instead). + /// Reads back the raw content of an imported CSS entry identified by `id`. Fails with `NotFound` or `NotImported` (linked entries have no local content — the frontend fetches `path_or_url` directly). pub fn css_content(&self, css_dir: &Path, id: &str) -> Result { match self.css_entries.iter().find(|entry| css_entry_id(entry) == id) { None => Err(CssError::NotFound), @@ -364,9 +287,7 @@ mod tests { #[test] fn default_settings_path_ends_with_settings_json() { - // We can't assert the exact OS path here without touching real - // config dirs, but we can verify the path-computation logic shapes - // it as expected. + // Can't assert the exact OS path without touching real config dirs, so just check the shape. if let Some(path) = default_settings_path() { assert_eq!(path.file_name().unwrap(), "settings.json"); assert!(path.parent().is_some()); diff --git a/src/timestamp.rs b/src/timestamp.rs index a43f866..3681f24 100644 --- a/src/timestamp.rs +++ b/src/timestamp.rs @@ -1,11 +1,7 @@ use serde::Serialize; use std::time::{SystemTime, UNIX_EPOCH}; -/// Wall-clock instant stored as nanoseconds since the Unix epoch. -/// -/// The README's data model sketches this field as `Instant`, but `Instant` has no -/// epoch and can't be serialized to the frontend, so wall-clock time is stored here -/// instead. +/// Wall-clock instant stored as nanoseconds since the Unix epoch (not `std::time::Instant`, which has no epoch and can't be serialized). #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize)] pub struct Timestamp(i64); diff --git a/tickets/030-comment-cleanup.md b/tickets/030-comment-cleanup.md deleted file mode 100644 index a870e93..0000000 --- a/tickets/030-comment-cleanup.md +++ /dev/null @@ -1,35 +0,0 @@ -# 030 — Cut comments down to clear, direct one-liners - -## Summary - -Pass over the Rust codebase, the webapp source, and the webapp's in-app docs, -trimming every comment/doc-comment to a single clear, direct line where one is -needed at all, and removing it outright where it isn't. - -## Scope - -- `src/*.rs`: many modules currently carry multi-paragraph `///`/`//!` blocks - (e.g. `src/settings.rs`'s module doc and per-method docs). Keep only what - explains a non-obvious *why* (a constraint, an invariant, a workaround); - drop restatements of what the code already says via naming. -- `webapp/src/**`: same treatment for inline comments and any JSDoc-style - blocks. -- The webapp's in-app documentation content (surfaced via the "?" help icons - from [021](021-docs-contextual-help.md)) — reduce prose there the same way: - clear and direct, not verbose. -- This is a mechanical cleanup pass, not a refactor — don't change behavior or - restructure code while doing it; keep it reviewable as a comments-only diff - where possible. - -## Acceptance criteria - -- No multi-line comment blocks remain outside of license/generated headers - (if any). -- Every remaining comment states a non-obvious reason, not a restatement of - the adjacent code. -- `cargo test` and the webapp build/tests still pass (confirms nothing beyond - comments changed). - -## Depends on - -None. diff --git a/webapp/src/App.tsx b/webapp/src/App.tsx index f87ca53..eb7c0de 100644 --- a/webapp/src/App.tsx +++ b/webapp/src/App.tsx @@ -22,11 +22,7 @@ import { DocsNavProvider } from './lib/docsNav' const POLL_MS = 2000 -/// Reads the standalone-page routing params written by `PopOutButton` -/// (ticket 036) — a `section` id selects one of the four primary sections to -/// render full-page with no nav/sibling chrome, and `startMs`/`endMs` carry -/// over the range that was active at pop-out time for sections that have no -/// `LogTimeline` of their own to pick one. +/// Reads the routing params `PopOutButton` writes: `section` picks which primary section to render full-page, and `startMs`/`endMs` carry over the range for sections with no `LogTimeline` of their own to pick one. function parsePopOutQuery(): { section: PopOutSectionId | null; range: TimeRange | null } { const query = new URLSearchParams(window.location.search) const section = query.get('section') as PopOutSectionId | null @@ -46,9 +42,7 @@ function StandalonePage({ children }: { children: ReactNode }) { ) } -/// Params has no lifted-state sibling to share `params`/`loadParams` with -/// when popped out on its own, so it owns a minimal fetch of its own here -/// rather than App's full `loadParams` machinery. +/// Popped out on its own, Params has no lifted-state sibling to share `params`/`loadParams` with, so it owns a minimal fetch here instead. function StandaloneParams() { const [params, setParams] = useState([]) @@ -71,15 +65,12 @@ function StandaloneParams() { ) } -/// Metrics/Dashboards already own their own data fetching — they only need -/// the range that was active at pop-out time, carried over via query params. +/// Metrics/Dashboards own their own data fetching — they only need the range carried over via query params. function StandaloneRanged({ section, range }: { section: 'metrics' | 'dashboards'; range: TimeRange | null }) { return {section === 'metrics' ? : } } -/// The log viewer has its own `LogTimeline` to pick a range, so it doesn't -/// need one carried over — it just needs its own copy of App's log/timeline -/// polling state, since none of that is shared with any sibling here. +/// The log viewer has its own `LogTimeline`, so it needs its own copy of App's log/timeline polling state instead of a carried-over range. function StandaloneLog() { const [logs, setLogs] = useState([]) const [params, setParams] = useState([]) @@ -194,11 +185,7 @@ function FullApp() { const [filterError, setFilterError] = useState(null) const [logViewerCollapsed, setLogViewerCollapsed] = useSectionCollapsed('log-viewer') - // Read from inside the poll loop below without making it a dependency — - // a rolling preset's own re-resolution (ticket 024) updates `range`, and - // depending on `range` directly would restart the effect (and its - // interval) every tick, turning "poll every POLL_MS" into "poll as fast as - // React can re-render." + // Read via ref inside the poll loop instead of depending on `range` directly, which would restart the interval every rolling-preset tick. const rangeRef = useRef(range) useEffect(() => { rangeRef.current = range @@ -221,9 +208,7 @@ function FullApp() { setRange(computePresetRange(id)) } - // A drag on LogTimeline always yields a fixed (non-rolling) range, - // overriding whatever preset was active — same as picking a preset - // overrides a previous drag. + // A drag always yields a fixed (non-rolling) range, overriding any active preset. function handleManualRangeSelect(next: TimeRange | null) { if (next === null) { selectPreset('all') @@ -237,8 +222,7 @@ function FullApp() { let cancelled = false async function poll() { - // Rolling presets "move with time": re-resolve on this same polling - // cadence rather than freezing the window at selection time. + // Rolling presets re-resolve on this same polling cadence instead of freezing the window at selection time. const effectiveRange = activePreset && activePreset !== 'all' ? computePresetRange(activePreset) : rangeRef.current if (activePreset && activePreset !== 'all') { @@ -273,11 +257,7 @@ function FullApp() { cancelled = true clearInterval(id) } - // Intentionally excludes `range`: a manual drag (which does change - // `activePreset` to null) still gets an immediate poll; only a second - // drag made while already in fixed-range mode waits for the next - // interval tick, which is an acceptable trade-off for not restarting - // this effect (and its interval) every rolling-preset tick. + // Intentionally excludes `range` to avoid restarting this interval on every rolling-preset tick. // eslint-disable-next-line react-hooks/exhaustive-deps }, [filter, activePreset]) diff --git a/webapp/src/components/Alerts.tsx b/webapp/src/components/Alerts.tsx index 7765f16..382fbeb 100644 --- a/webapp/src/components/Alerts.tsx +++ b/webapp/src/components/Alerts.tsx @@ -24,9 +24,7 @@ const OPS: { value: CompareOp; label: string }[] = [ { value: 'ne', label: '!=' }, ] -/// Parses a plain millisecond number or a humantime-style duration like -/// `30s`/`5m`/`1h` into milliseconds. Mirrors the backend's `window_ms` -/// field without pulling in a full humantime parser for the webapp. +/// Parses a plain millisecond number or a humantime-style duration (`30s`/`5m`/`1h`) to mirror the backend's `window_ms` without a full humantime parser here. function parseWindowMs(input: string): number | null { const trimmed = input.trim() if (!trimmed) return null @@ -119,8 +117,6 @@ function formFromRule(rule: AlertRule): FormState { return base } -/// Validates the form's condition-specific fields, returning an error -/// message (or null if valid) plus the built `AlertCondition` when valid. function buildCondition(form: FormState): { error: string | null; condition: AlertCondition | null } { switch (form.kind) { case 'immediate': diff --git a/webapp/src/components/CollapsibleSection.tsx b/webapp/src/components/CollapsibleSection.tsx index 5b8920d..e700cc0 100644 --- a/webapp/src/components/CollapsibleSection.tsx +++ b/webapp/src/components/CollapsibleSection.tsx @@ -1,10 +1,7 @@ import { useState } from 'react' import type { TimeRange } from './LogTimeline' -/// localStorage key for which of the four primary sections (log viewer, -/// params, metrics, dashboards) are collapsed (ticket 035). Mirrors -/// LogPreview's VISIBLE_PARAM_COLUMNS_STORAGE_KEY pattern — one key holding -/// a record of section id -> collapsed. +/// localStorage key holding a record of section id -> collapsed, mirroring LogPreview's VISIBLE_PARAM_COLUMNS_STORAGE_KEY pattern. const COLLAPSED_SECTIONS_STORAGE_KEY = 'ottyl.collapsedSections' function loadCollapsedSections(): Record { @@ -27,10 +24,7 @@ function saveCollapsedSections(sections: Record) { } } -/// Persists a section's collapsed state across reloads. Deliberately -/// returns only the boolean + setter — callers keep rendering their -/// subtree, just hide it with CSS (`hidden`), so collapsing never unmounts -/// in-progress form state. +/// Returns only the boolean + setter, not a conditional-render helper — callers keep rendering their subtree and hide it with CSS (`hidden`), so collapsing never unmounts in-progress form state. export function useSectionCollapsed(sectionId: string): [boolean, (next: boolean) => void] { const [collapsed, setCollapsedState] = useState(() => loadCollapsedSections()[sectionId] ?? false) @@ -59,8 +53,7 @@ export function CollapseToggle({ collapsed, onToggle }: { collapsed: boolean; on ) } -/// The four primary sections that can be popped out into their own tab -/// (ticket 036) — matches the `?section=` query param a standalone page reads. +/// The four primary sections poppable into their own tab; matches the `?section=` query param a standalone page reads. export type PopOutSectionId = 'log' | 'params' | 'metrics' | 'dashboards' function popOutUrl(section: PopOutSectionId, range?: TimeRange | null): string { @@ -72,10 +65,7 @@ function popOutUrl(section: PopOutSectionId, range?: TimeRange | null): string { return `?${query.toString()}` } -/// Opens this section, full-page and with no surrounding chrome, in a new -/// tab. `range` is only needed for sections without their own LogTimeline — -/// the popped-out page has no way to pick a range itself, so the current one -/// is carried over via query params (see `App.tsx`'s standalone rendering). +/// `range` is only needed for sections without their own LogTimeline — the popped-out page can't pick a range itself, so the current one is carried over via query params. export function PopOutButton({ section, range }: { section: PopOutSectionId; range?: TimeRange | null }) { return ( ))} - {/* A dragged (custom, non-preset) selection — presets already show - their own active state above, so this only appears for the - "fixed range from a drag" case. */} + {/* Only shown for a dragged (non-preset) selection — presets already show their own active state above. */} {range && activePreset === null && (