From b6401c1c3c14da7ebfa03717c3b42f59809b6928 Mon Sep 17 00:00:00 2001 From: Hugh Kaznowski Date: Sun, 6 Sep 2026 16:34:42 +0100 Subject: [PATCH] Strip all comments outside src/consts.rs Removes doc comments, module comments, and inline comments across the backend so the only comments left in the codebase are the brief ones on the constants they document. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01A1ZJ3SrayzLzVndhVq9BmS --- src/alerts.rs | 40 --- src/api.rs | 128 +------- src/data_value.rs | 68 ----- src/dto.rs | 59 ---- src/engine.rs | 648 +--------------------------------------- src/filter_datetime.rs | 18 -- src/filter_parser.pest | 19 -- src/filter_parser.rs | 17 -- src/index_key.rs | 7 - src/ingest.rs | 21 +- src/logfmt.rs | 153 ---------- src/main.rs | 21 -- src/mcp_server.rs | 40 --- src/metric_primitive.rs | 6 - src/profile.rs | 205 ------------- src/promql.rs | 24 -- src/rate.rs | 35 --- src/sessions.rs | 30 -- src/settings.rs | 92 ------ src/timestamp.rs | 1 - 20 files changed, 10 insertions(+), 1622 deletions(-) diff --git a/src/alerts.rs b/src/alerts.rs index 3071b51..e9f3f19 100644 --- a/src/alerts.rs +++ b/src/alerts.rs @@ -1,14 +1,3 @@ -//! Alert rules: definitions, trigger evaluation, and ntfy delivery. -//! -//! Alert queries are restricted to regex/ottyl_seq/ottyl_ts/ottyl_fmt/json[field] — a metric[param] comparison would require a full bucket rescan per line, breaking the proportional-cost guarantee. json[field] is fine here since, like regex/ottyl_seq/ottyl_ts/ottyl_fmt, it only inspects the line's own text/fields, no index lookup. -//! -//! `Threshold`/`Ratio` windows are maintained via `crate::rate::RateWindow` — the same windowed-rate -//! implementation PromQL's `rate()` uses (`crate::engine::evaluate_promql`) — rather than a -//! bespoke `VecDeque`+prune loop, so the two agree on what "windowed rate" means (ticket 040). A -//! window is also backfilled once, on first evaluation, from already-ingested logs within its own -//! trailing window (see `backfill_window`): a rule created mid-session (or a process that replays -//! existing log content, e.g. `--file` tailing) no longer starts with a silently empty window. - use std::collections::hash_map::DefaultHasher; use std::collections::{BTreeMap, HashMap, HashSet}; use std::fmt; @@ -41,19 +30,16 @@ pub struct AlertRule { #[serde(tag = "kind", rename_all = "lowercase")] pub enum AlertCondition { Immediate { query: String }, - /// 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, }, - /// 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, }, - /// A zero `count_b` skips evaluation for that line rather than treating the ratio as infinite. Ratio { query_a: String, query_b: String, @@ -86,7 +72,6 @@ impl From for AlertError { } } -/// 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); @@ -145,13 +130,6 @@ impl Settings { } } -/// Per-rule in-memory evaluation state; lives only for the process's lifetime — not persisted -/// across a restart with no replayed log history. Each `RateWindow` is backfilled once (see -/// `backfill_window`) from whatever matching history is already in the index at first evaluation, -/// so — unlike before ticket 040 — a rule doesn't start blind to matches ingested before it was -/// declared, or before this evaluation loop last started, as long as that history is still in the -/// in-memory log index (e.g. `--file` tailing replays existing file content from the start on -/// every process start). #[derive(Debug, Default)] pub struct AlertRuntimeState { threshold_window: RateWindow, @@ -163,10 +141,6 @@ pub struct AlertRuntimeState { seen_values: HashSet, } -/// Builds a `RateWindow` seeded from already-ingested logs matching `query`, bounded to -/// `window_ms` before `now` (not the full log history) so the cost stays proportional to the -/// window rather than everything ever ingested. `before_sequence` excludes the line currently -/// being evaluated, which the caller pushes separately if it matches. fn backfill_window( index: &BTreeMap, query: &str, @@ -184,7 +158,6 @@ fn backfill_window( RateWindow::backfill(matching_timestamps, now, window_ms) } -/// Evaluates a FilterBar query against a single line's own fields only (metric[param] comparisons aren't supported — see module doc; json[field] is supported since it's also line-local). 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) @@ -237,7 +210,6 @@ fn eval_alert_expr(expr: &FilterExpr, sequence: u64, timestamp: Timestamp, text: } } -/// 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, @@ -344,7 +316,6 @@ fn evaluate_condition( let count_a = state.ratio_a_window.count() as f64; let count_b = state.ratio_b_window.count() as f64; if count_b == 0.0 { - // Zero denominator: skip rather than treat the ratio as infinite. return None; } let observed = count_a / count_b; @@ -357,9 +328,6 @@ fn evaluate_condition( } } -/// Delivers to the single, process-wide ntfy sink configured in Settings. Errors are only logged -/// (no retry queue); spawned off the ingest hot path so a slow/unreachable sink never blocks ingestion. -/// A silently no-op'd delivery (no topic configured) is expected — a rule can be defined before the sink is. pub async fn deliver_for_rule(rule_name: String, message: String, topic: Option, server: Option) { let Some(topic) = topic else { return; @@ -370,7 +338,6 @@ pub async fn deliver_for_rule(rule_name: String, message: String, topic: Option< } } -/// 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() @@ -506,7 +473,6 @@ mod tests { } let fired = evaluate_alerts(&[rule.clone()], &mut states, &index, 2, Timestamp::from_millis(2000), "error"); assert_eq!(fired.len(), 1); - // Stays quiet for a non-matching line even though the window is full. let fired = evaluate_alerts(&[rule], &mut states, &index, 3, Timestamp::from_millis(2100), "fine"); assert!(fired.is_empty()); } @@ -527,7 +493,6 @@ mod tests { let mut states = HashMap::new(); let index: BTreeMap = BTreeMap::new(); assert!(evaluate_alerts(&[rule.clone()], &mut states, &index, 0, Timestamp::from_millis(0), "error").is_empty()); - // Second match arrives well after the first has aged out of the window. assert!(evaluate_alerts(&[rule], &mut states, &index, 1, Timestamp::from_millis(5_000), "error").is_empty()); } @@ -568,15 +533,11 @@ mod tests { }; let mut states = HashMap::new(); let index: BTreeMap = BTreeMap::new(); - // Only an "error" so far; no "request" yet, so denominator is 0 -> skip. assert!(evaluate_alerts(&[rule.clone()], &mut states, &index, 0, Timestamp::from_millis(0), "error").is_empty()); - // One request: ratio 1/1 = 1.0 >= 0.5 -> fires. let fired = evaluate_alerts(&[rule.clone()], &mut states, &index, 1, Timestamp::from_millis(1), "request"); assert_eq!(fired.len(), 1); - // A second request: ratio 1/2 = 0.5 still >= 0.5 -> fires again. let fired = evaluate_alerts(&[rule.clone()], &mut states, &index, 2, Timestamp::from_millis(2), "request"); assert_eq!(fired.len(), 1); - // A third request with no more errors: ratio drops to 1/3 -> no longer fires. let fired = evaluate_alerts(&[rule], &mut states, &index, 3, Timestamp::from_millis(3), "request"); assert!(fired.is_empty()); } @@ -606,7 +567,6 @@ mod tests { let rule = immediate_rule("some_metric[param]==5"); let mut states = HashMap::new(); let index: BTreeMap = BTreeMap::new(); - // Should log a warning and simply not fire, never panic. let fired = evaluate_alerts(&[rule], &mut states, &index, 0, Timestamp::from_millis(0), "anything"); assert!(fired.is_empty()); } diff --git a/src/api.rs b/src/api.rs index c3e83c5..45c61bd 100644 --- a/src/api.rs +++ b/src/api.rs @@ -26,15 +26,12 @@ use tokio::sync::oneshot; static WEBAPP_DIST: EmbedDir = fs_embed::fs_embed!("webapp/dist"); -/// Embedded so an agent that has only discovered a port (no filesystem access into the ottyl -/// repo/binary) can still fetch it — see `SKILL.md` at the repo root and ticket 069. const SKILL_MD: &str = include_str!("../SKILL.md"); #[derive(Clone)] struct ApiState { engine: EngineHandle, settings: SettingsHandle, - /// Imported CSS files are stored alongside this path's parent directory (see `ApiState::css_dir`). settings_path: Arc, } @@ -46,7 +43,6 @@ impl ApiState { .unwrap_or_else(|| PathBuf::from(".")) } - /// 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 mut settings = self.settings.lock().unwrap(); let Some(profile_name) = settings.active_profile.clone() else { @@ -60,11 +56,6 @@ impl ApiState { } } - /// Counterpart to `persist_param_declaration` for `DELETE /api/params` (ticket 052): empties the - /// active profile's stored params so the just-cleared set doesn't reappear from `settings.json` - /// on next load. Same best-effort semantics — a no-op if no profile is active, I/O failures only - /// logged, since the engine-side clear (already applied by the time this runs) is the source of - /// truth for the running process. fn persist_clear_params(&self) { let mut settings = self.settings.lock().unwrap(); let Some(profile_name) = settings.active_profile.clone() else { @@ -77,11 +68,6 @@ impl ApiState { } } - /// Same as `persist_param_declaration`, for metric declarations. Also promotes any of the - /// metric's referenced params that only exist as auto-derived engine state (see - /// `promoted_param_records`) into persisted profile params, so a metric built against an - /// auto-detected logfmt key survives a restart instead of being rejected as "undeclared" once - /// declarations replay before any log line re-derives it. async fn persist_metric_declaration(&self, request: &DeclareMetricRequest) { let profile_name = { let settings = self.settings.lock().unwrap(); @@ -109,7 +95,6 @@ impl ApiState { } } - /// Same as `persist_metric_declaration`, for an in-place edit (`PUT /api/metrics/{name}`) — also drops the persisted record under `old_name` when the edit renames the metric, so a rename doesn't leave a stale duplicate declaration in the profile file. async fn persist_metric_declaration_update(&self, old_name: &str, request: &DeclareMetricRequest) { let profile_name = { let settings = self.settings.lock().unwrap(); @@ -138,9 +123,6 @@ impl ApiState { } } - /// Best-effort, matching `persist_param_declaration`'s intent — a no-op if no profile is - /// active, and I/O failures are only logged, since the engine's removal is already the source - /// of truth for what the live process shows. fn persist_metric_declaration_removal(&self, name: &str) { let mut settings = self.settings.lock().unwrap(); let Some(profile_name) = settings.active_profile.clone() else { @@ -203,7 +185,6 @@ async fn get_skill_md() -> impl IntoResponse { ([(header::CONTENT_TYPE, "text/markdown; charset=utf-8")], SKILL_MD) } -/// 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 @@ -234,13 +215,6 @@ fn mime_for_extension(extension: &str) -> &'static str { } } -/// Ticket 075: every read query is served straight off a `left_right::ReadHandle` obtained from -/// `engine.reader_factory` — wait-free, and never funneled through the actor's mpsc queue, so a slow -/// query here can no longer block an unrelated cheap one. The one exception is a sorted `Logs` query -/// the first time its `sort_by` is used: no helper index exists yet for it, and only the actor (the -/// sole owner of the write side) can build one, so this sends `EnsureSortHelperIndex`, awaits the -/// ack, then re-enters a fresh `ReadHandle` (now caught up) before proceeding — every other read, -/// and every sorted query once the helper already exists, never reaches the actor at all. async fn ask(engine: &EngineHandle, query: EngineQuery) -> Result { let read_handle = engine.reader_factory.handle(); if let Some(sort_by) = crate::engine::query_sort_by(&query) { @@ -262,13 +236,6 @@ async fn ask(engine: &EngineHandle, query: EngineQuery) -> Result) -> impl IntoResponse { async fn list_params(State(state): State) -> impl IntoResponse { match ask(&state.engine, EngineQuery::ParamDeclarations).await { Ok(EngineResponse::ParamDeclarations(mut declarations)) => { - // `ottyl_seq`/`ottyl_ts`/`ottyl_fmt` (ticket 055) are computed line-locally rather than - // stored as index `ParamDeclaration` entries (see `crate::engine::declare_param`'s - // rejection of these names), so they're merged in here purely for discoverability - // rather than via `param_declarations_range`. This is deliberately a merge only in this - // one HTTP-facing listing, not in `EngineQuery::ParamDeclarations` itself — other - // internal callers of that query (promoting a metric's referenced params, replaying - // declarations on profile switch) must keep seeing only real, persistable entries. declarations.extend(reserved_param_declaration_dtos()); Json(declarations).into_response() } @@ -357,10 +317,6 @@ async fn list_params(State(state): State) -> impl IntoResponse { } } -/// `GET /api/params`-only synthesized entries for the three reserved names -/// (`crate::engine::OTTYL_RESERVED_PARAM_NAMES`) — see `list_params`. `message_match_regex`/ -/// `capture_group` are empty since these are never text-extracted; `reserved: true` marks them as -/// non-deletable/non-editable, distinct from `auto_derived`. fn reserved_param_declaration_dtos() -> [ParamDeclarationDto; 3] { [ ParamDeclarationDto { @@ -421,8 +377,6 @@ async fn declare_param( json_field_path: request.json_field_path, kind: request.kind, elapsed_since_last: request.elapsed_since_last, - // A request that reached here was accepted by `declare_param`, which always stores - // it as non-auto (see `engine::declare_param`). auto_derived: false, reserved: false, }; @@ -433,11 +387,6 @@ async fn declare_param( } } -/// `DELETE /api/params` (ticket 052): clears every declared param (manual and auto-derived) and -/// immediately re-derives whatever `auto_declare_params` can still find in log text the engine -/// still retains. The response body's `logs_considered` tells the caller how many retained lines -/// were re-scanned, so the UI can make clear that lines evicted by `max_logs` before this ran -/// can't regain their params — this is expected behavior, not a partial failure. async fn clear_params(State(state): State) -> impl IntoResponse { let (respond_to, response) = oneshot::channel(); if state.engine.tx.send(EngineMessage::ClearParams { respond_to }).await.is_err() { @@ -493,7 +442,6 @@ async fn declare_metric( } } -/// Updates the metric declaration named by the URL path in place, reusing `declare_metric`'s validation (via `engine::update_metric`) rather than creating a second declaration or erroring on a duplicate-name conflict. `body.name` may differ from the path segment to rename the metric, so long as no other declaration already uses the new name. async fn update_metric( State(state): State, AxumPath(name): AxumPath, @@ -528,12 +476,6 @@ async fn update_metric( } } -/// Deletes the metric declaration named by the URL path — both from the live engine index and, -/// if a profile is active, from persisted settings (ticket 051), so it doesn't reappear on -/// restart. Its recorded bucket-value history is purged along with it (see the comment on -/// `EngineMessage::RemoveMetricDeclaration`'s handler in `engine.rs`), rather than left orphaned, -/// so a dashboard panel or PromQL query still referencing the metric by name fails with a clear -/// "unknown metric" error instead of silently keeping stale data alive. async fn delete_metric( State(state): State, AxumPath(name): AxumPath, @@ -558,7 +500,6 @@ async fn delete_metric( } } -/// Malformed pairs (no `=`) are skipped rather than rejected. fn parse_labels_filter(raw: &str) -> BTreeMap { raw.split(',') .filter_map(|pair| pair.split_once('=')) @@ -639,7 +580,6 @@ async fn import_css( } } -/// 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, @@ -669,7 +609,6 @@ async fn remove_css( } } -/// 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, @@ -730,7 +669,6 @@ async fn delete_alert_rule(State(state): State, AxumPath(id): AxumPath } } -/// 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 { @@ -739,13 +677,11 @@ async fn test_alert_notification(Json(body): Json) } } -/// 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() } -/// `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, @@ -780,9 +716,6 @@ async fn update_settings( .tx .send(EngineMessage::SetMaxLogs(new_max_logs.1)) .await; - // `SetMaxLogs` has no response to await on its own (ticket 075: it's fire-and-forget, same as - // `Ingest`), but this endpoint's caller reasonably expects a subsequent read to already - // reflect the new cap's eviction — barrier so it does. state.engine.barrier().await; } Json(new_max_logs.0).into_response() @@ -817,7 +750,6 @@ async fn create_profile( } } -/// 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, @@ -829,10 +761,6 @@ async fn delete_profile( } } -/// Removes every currently-declared param/metric from the engine, then replays `profile`'s declarations into it. -/// Auto-derived params (`ParamDeclarationDto::auto_derived`) are left alone — they aren't part of -/// any profile, just facts discovered from the log stream, so a profile switch shouldn't discard -/// them (they'd otherwise only reappear once a new line re-introduces that key). async fn reload_engine_declarations(engine: &EngineHandle, profile: &Profile) { if let Ok(EngineResponse::ParamDeclarations(params)) = ask(engine, EngineQuery::ParamDeclarations).await @@ -972,7 +900,6 @@ mod tests { use std::sync::Mutex; use tower::ServiceExt; - /// Spins up a fresh engine + router pair backed by a temp settings file. fn test_app() -> (Router, EngineHandle, SettingsHandle, tempfile::TempDir) { let dir = tempfile::tempdir().unwrap(); let settings_path = dir.path().join("settings.json"); @@ -1066,7 +993,6 @@ mod tests { let status = delete_request(&app, "/api/settings/alerts/does-not-exist").await; assert_eq!(status, StatusCode::NOT_FOUND); - // 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") @@ -1104,10 +1030,6 @@ mod tests { assert_eq!(body["capture_group"], "level"); } - /// Ticket 052's core acceptance criteria: `DELETE /api/params` removes every declared param - /// (manual and auto-derived), that removal survives a reload of persisted settings, and — - /// without any new line being ingested — the retained `IngestedLog` entries immediately - /// re-derive a fresh (auto-derived) param set. #[tokio::test] async fn clearing_params_persists_and_reextracts_from_retained_logs() { let (app, engine, settings_handle, dir) = test_app(); @@ -1119,14 +1041,10 @@ mod tests { settings.set_active_profile(&settings_path, Some("web".to_string())).unwrap(); } - // A manual declaration plus an auto-derived one (from ingesting a logfmt line before "path" is declared). declare_test_param(&app, "path", r"request path=(?P\S+)", "path", "string").await; ingest_lines(&engine, 0, &["request path=/a", "level=info x=1"]).await; let (_, before) = get_json(&app, "/api/params").await; - // Ticket 055 merges the three non-deletable `ottyl_*` reserved names into this listing - // too; `DELETE /api/params` (ticket 052) only ever clears real declarations, so the - // reserved ones must be excluded from the "everything got removed" count below. let before_declared: Vec<&str> = before .as_array() .unwrap() @@ -1141,7 +1059,6 @@ mod tests { let (status, result) = send_json(&app, "DELETE", "/api/params", json!({})).await; assert_eq!(status, StatusCode::OK); assert_eq!(result["removed"], before_declared.len() as u64); - // Re-derived without any new ingest — same three keys come back, now all auto-derived. assert_eq!(result["reextracted"], 3); assert_eq!(result["logs_considered"], 2); @@ -1151,8 +1068,6 @@ mod tests { assert_eq!(after_declared.len(), 3); assert!(after_declared.iter().all(|p| p["auto_derived"] == true)); - // The clear persisted: "path" (the only manually-declared, profile-stored param) doesn't - // reappear from settings.json on a fresh load. let reloaded_settings = Settings::load(&settings_path); let profile = reloaded_settings.profile("web").unwrap(); assert!(profile.params.is_empty()); @@ -1177,9 +1092,6 @@ mod tests { assert!(body["error"].as_str().unwrap().contains("invalid regex")); } - /// Declares a standalone param via `POST /api/params` — most metric-declaration tests need one - /// or more already-declared params to reference by name (ticket 037), rather than embedding a - /// regex/capture-group/kind inline in the metric request. async fn declare_test_param(app: &Router, name: &str, pattern: &str, capture_group: &str, kind: &str) { let (status, _body) = send_json( app, @@ -1271,7 +1183,6 @@ mod tests { assert_eq!(body["name"], "requests"); assert_eq!(body["label_params"].as_array().unwrap().len(), 2); - // Still exactly one declaration named "requests", now with the updated regex/params — not a second declaration alongside the original. let (status, body) = get_json(&app, "/api/metrics").await; assert_eq!(status, StatusCode::OK); let metrics = body.as_array().unwrap(); @@ -1357,8 +1268,6 @@ mod tests { assert_eq!(status, StatusCode::NOT_FOUND); } - /// Acceptance criterion: deleting a metric persists the removal into the active profile too, so - /// it doesn't reappear from `settings.json` on the next restart. #[tokio::test] async fn deleting_metric_persists_removal_and_it_does_not_reappear_after_restart() { let (app, _engine, settings_handle, dir) = test_app(); @@ -1389,10 +1298,6 @@ mod tests { assert!(Settings::load(&settings_path).profile("web").unwrap().metrics.is_empty()); } - /// Acceptance criterion: a dashboard panel (PromQL query) that still references a metric by - /// name after it's been deleted fails with a readable "unknown metric" error, rather than a - /// panic or a silently-empty series — seeded here by declaring, ingesting into, deleting, then - /// re-querying the same metric name. #[tokio::test] async fn promql_query_against_deleted_metric_fails_with_readable_error() { let (app, engine, _settings, _dir) = test_app(); @@ -1420,7 +1325,6 @@ mod tests { .unwrap(); engine.barrier().await; - // Sanity check: the dashboard-panel-style query works before deletion. let (status, _body) = get_json(&app, "/api/query?promql=requests").await; assert_eq!(status, StatusCode::OK); @@ -1478,11 +1382,9 @@ mod tests { let values = body.as_array().unwrap(); assert_eq!(values.len(), 2); for value in values { - // MetricPrimitive derives Serialize as an externally-tagged enum, so JSON is {"String": "/a"}. assert_eq!(value["labels"]["path"]["String"], "/a"); } - // 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(); @@ -1548,7 +1450,6 @@ mod tests { async fn declaring_param_persists_into_active_profile() { let (app, _engine, settings_handle, dir) = test_app(); - // 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 mut settings = settings_handle.lock().unwrap(); @@ -1576,11 +1477,6 @@ mod tests { assert_eq!(profile.params[0].name, "level"); } - /// Regression test: declaring a metric against a param that only exists as auto-derived engine - /// state (discovered from an ingested logfmt key, never manually declared via `POST - /// /api/params`) must persist that param into the active profile too, not just the metric — - /// otherwise a restart replays the metric before any log line re-derives the auto param, and the - /// engine rejects it as referencing an undeclared param (see `promoted_param_records`). #[tokio::test] async fn declaring_metric_against_auto_derived_param_promotes_it_into_the_profile() { let (app, engine, settings_handle, dir) = test_app(); @@ -1592,8 +1488,6 @@ mod tests { settings.set_active_profile(&settings_path, Some("web".to_string())).unwrap(); } - // Ingest a line carrying a logfmt key before any param is manually declared, so "dx" only - // exists as engine-internal auto-derived state (never persisted anywhere). ingest_lines(&engine, 0, &["dx=1 moved"]).await; let (status, _body) = send_json( @@ -1613,11 +1507,9 @@ mod tests { let profile = reloaded_settings.profile("web").unwrap(); assert_eq!(profile.metrics.len(), 1); assert_eq!(profile.metrics[0].name, "direction_x"); - // The auto-derived "dx" param got promoted into the profile alongside the metric. assert_eq!(profile.params.len(), 1); assert_eq!(profile.params[0].name, "dx"); - // Simulate a restart: replay the persisted profile into a fresh engine with no prior ingestion. let fresh_settings_handle = std::sync::Arc::new(std::sync::Mutex::new(Settings::default())); let fresh_engine_tx = crate::engine::spawn(fresh_settings_handle, 16); profile::load_into_engine(&fresh_engine_tx, profile).await; @@ -1734,7 +1626,6 @@ mod tests { ) .await; - // Simulate a restart by reloading settings fresh from disk, independent of the running app/engine. let settings_path = dir.path().join("settings.json"); let reloaded_settings = Settings::load(&settings_path); let dashboards = reloaded_settings.list_dashboards("web").unwrap(); @@ -1744,9 +1635,6 @@ mod tests { assert_eq!(dashboards[0].panels[0].promql, "requests"); } - /// Ticket 075: `Ingest` has no response to await on its own, and reads no longer share a queue - /// with it — every caller here immediately reads back what it just ingested, so this barriers - /// once at the end rather than making every call site remember to. async fn ingest_lines(engine: &EngineHandle, start_sequence: u64, texts: &[&str]) { for (offset, text) in texts.iter().enumerate() { let sequence = start_sequence + offset as u64; @@ -1776,8 +1664,6 @@ mod tests { assert_eq!(status, StatusCode::OK); assert_eq!(body["max_logs"], 2); - // Existing over-cap logs are evicted as soon as the setting change - // reaches the engine, not just on the next ingest. let (_, body) = get_json(&app, "/api/logs").await; assert_eq!(body.as_array().unwrap().len(), 2); @@ -1822,8 +1708,6 @@ mod tests { assert_eq!(body.as_array().unwrap().len(), 3); } - /// Ticket 045: seeded fixture mixing lines that carry the sort param with lines that don't, - /// exercised through the actual HTTP endpoint rather than just `handle_query` directly. #[tokio::test] async fn get_logs_sort_by_orders_by_param_and_toggles_missing_param_handling() { let (app, engine, _settings, _dir) = test_app(); @@ -1834,28 +1718,24 @@ mod tests { &engine, 0, &[ - "priority=3 boom", // sequence 0, has param - "no param here", // sequence 1, missing - "priority=1 boom", // sequence 2, has param - "also missing", // sequence 3, missing + "priority=3 boom", + "no param here", + "priority=1 boom", + "also missing", ], ) .await; - // Default order (no sort_by): unchanged, plain ingest/ottyl_seq order. let (status, body) = get_json(&app, "/api/logs").await; assert_eq!(status, StatusCode::OK); let sequences: Vec = body.as_array().unwrap().iter().map(|e| e["sequence"].as_u64().unwrap()).collect(); assert_eq!(sequences, vec![0, 1, 2, 3]); - // sort_by=priority with the fallback toggle on (the default): entries carrying the param - // sorted ascending by its value, then the missing-param entries ordered by ottyl_seq. let (status, body) = get_json(&app, "/api/logs?sort_by=priority").await; assert_eq!(status, StatusCode::OK); let sequences: Vec = body.as_array().unwrap().iter().map(|e| e["sequence"].as_u64().unwrap()).collect(); assert_eq!(sequences, vec![2, 0, 1, 3]); - // Same sort_by, but with the fallback toggle explicitly off: missing-param lines are dropped. let (status, body) = get_json(&app, "/api/logs?sort_by=priority&sort_missing_fallback=false").await; assert_eq!(status, StatusCode::OK); let sequences: Vec = body.as_array().unwrap().iter().map(|e| e["sequence"].as_u64().unwrap()).collect(); diff --git a/src/data_value.rs b/src/data_value.rs index 2d66651..07c5ece 100644 --- a/src/data_value.rs +++ b/src/data_value.rs @@ -3,61 +3,22 @@ use crate::timestamp::Timestamp; use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; -/// A named, standalone parameter: how to pull a value out of a matching log line, and what kind it -/// parses as. Ticket 037 unified this with the metric-declaration-embedded matcher that used to -/// exist alongside it (`RegexToMetricPrimitiveMatcher`) — a metric now references params by name -/// (`MetricDeclaration::label_params`/`value_param`) instead of redeclaring its own extractor per -/// param, so a param declared once (e.g. via the Params panel) can be reused as a label by one -/// metric and as the value by another without re-entering its regex/capture-group/kind. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct ParamDeclaration { pub name: String, pub message_match_regex: String, pub capture_group: String, - /// Alternative extraction source to `capture_group`: a dot-separated JSON field path (e.g. - /// `foo.bar`), read from lines that parse as a JSON object instead of via the regex capture - /// group. When present and non-empty, this takes precedence over `capture_group`. - /// `#[serde(default)]` so profile files/requests saved before this field existed still load - /// as plain regex-capture-group params. #[serde(default)] pub json_field_path: Option, pub kind: MetricPrimitiveKind, - /// A third extraction source, alongside `capture_group`/`json_field_path`: when set, this - /// param's value is never parsed from text at all — it's computed as elapsed time since the - /// previous event that produced the same label set (ticket 038). Takes precedence over - /// `json_field_path`/`capture_group` when set, and `message_match_regex`/`capture_group`/ - /// `json_field_path` are simply unused in that case (`declare_param` doesn't validate them for - /// this shape). Only meaningful paired with `kind == Duration`, since elapsed time is the only - /// primitive this computation can produce — `declare_param` enforces that pairing. - /// `#[serde(default)]` so params declared before this field existed keep parsing as plain - /// regex/JSON-field extraction. #[serde(default)] pub elapsed_since_last: bool, - /// Set when this param was discovered by `crate::engine::auto_declare_params` (a nom-based - /// `key=value` scan run on every ingested line) rather than declared through `POST /api/params`. - /// `declare_param` refuses to overwrite an auto-derived param by name, and it's never persisted - /// to a profile — see `crate::engine::declare_param` and `crate::api::reload_engine_declarations`. - /// `#[serde(default)]` so profiles/requests saved before this field existed still load as - /// ordinary (non-auto) params. #[serde(default)] pub auto_derived: bool, - /// Set for a `log stream` column/message param declared by - /// `crate::engine::auto_declare_log_stream_params`. Those params' `message_match_regex` is a - /// pure fixed character-offset slice with no content anchor of its own (ticket 076) — it'll - /// happily "match" and extract garbage from any line at least that long, including a wrapped - /// multi-line dump's continuation line or the header row itself. When set, extraction ( - /// `extract_param_raw`/`derive_log_params`) additionally requires - /// `crate::logfmt::is_log_stream_line(text)` before trusting the regex at all. - /// `#[serde(default)]` so params declared before this field existed keep extracting as before. #[serde(default)] pub log_stream_gated: bool, } -/// Reads a value out of a JSON object by dot-separated field path (e.g. `"foo.bar"`), for -/// [`ParamDeclaration::json_field_path`]. Returns `None` when `text` doesn't parse as a JSON -/// object, the path doesn't resolve to a value, or the resolved value isn't a scalar -/// (string/number/bool) — there's no sensible flat text form of a nested object/array to feed into -/// `parse_metric_primitive`. pub fn extract_json_field(text: &str, field_path: &str) -> Option { let root: serde_json::Value = serde_json::from_str(text).ok()?; let mut current = root.as_object()?; @@ -77,14 +38,6 @@ pub fn extract_json_field(text: &str, field_path: &str) -> Option { } } -/// Flattens a JSON object into dot-separated field paths mapped to each leaf scalar's raw text form -/// (string/number/bool), for [`crate::engine::auto_declare_params`] when a line is JSON -/// (`crate::logfmt::detect_format`) rather than logfmt — the JSON equivalent of -/// `crate::logfmt::extract_pairs`'s `key=value` scan. Nested objects recurse, dot-joining the path -/// (`"foo": {"bar": 1}` yields `"foo.bar"`); arrays and null are skipped since there's no single leaf -/// scalar to declare a param against, matching `extract_json_field`'s own restriction to scalars. -/// Returns an empty vec for anything that isn't a JSON object, same as `extract_pairs` returns empty -/// for a line with no `key=value` pairs. pub fn flatten_json_object(text: &str) -> Vec<(String, String)> { let Ok(serde_json::Value::Object(root)) = serde_json::from_str::(text) else { return Vec::new(); @@ -111,7 +64,6 @@ fn flatten_json_object_into( } } -/// `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")] @@ -119,45 +71,25 @@ pub enum MetricPrimitiveKind { String, Number, Timestamp, - /// A literal i64 millisecond count. Note this is unrelated to - /// `ParamDeclaration::elapsed_since_last`, which is a separate, independent extraction source — - /// a param using that source always parses/produces `Duration`, but not every `Duration`-kind - /// param is computed that way (one can also just parse a literal duration out of captured text, - /// like any other kind). Duration, - /// A standalone param declared with this kind doesn't require (or use) a capture group: its value is - /// simply whether `message_match_regex` matches the line, case-insensitively or per the regex's own - /// flags — see `crate::engine::derive_log_params`. Unlike other kinds it's present as `true`/`false` - /// on every line rather than omitted when unmatched. Boolean, } -/// Value type paired with [`crate::index_key::IndexKey`] in the engine's primary index. #[allow(dead_code)] #[derive(Debug, Clone, Serialize)] pub enum DataValue { - /// Paired with `IndexKey::IngestedLog`. LogLiteral { text: String }, - /// Paired with `IndexKey::ParamDeclaration`. ParamDeclaration(ParamDeclaration), - /// Paired with `IndexKey::MetricDeclaration`. `label_params`/`value_param` reference standalone - /// `ParamDeclaration`s by name (resolved against the engine's param store in - /// `crate::engine::apply_metric_to_log`) rather than embedding their own extractor. MetricDeclaration { message_match_regex: String, label_params: Vec, - /// At most one value param per metric; its primitive becomes the event's bucket value instead of a label. value_param: Option, - /// See `crate::engine::DeclareMetricRequest::matched_labels`. matched_labels: Option>, - /// See `crate::engine::DeclareMetricRequest::unmatched_labels`. unmatched_labels: Option>, }, - /// Paired with `IndexKey::Bucket`. BucketValue { values: Vec<(u64, Timestamp, MetricPrimitive)>, }, - /// Paired with `IndexKey::LastMetricEvent`. LastEventTimestamp { timestamp: Timestamp, }, diff --git a/src/dto.rs b/src/dto.rs index 6675a5b..a45a7d8 100644 --- a/src/dto.rs +++ b/src/dto.rs @@ -11,7 +11,6 @@ pub struct LogEntryDto { pub sequence: u64, pub timestamp_ms: i64, pub text: String, - /// 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, } @@ -20,20 +19,10 @@ pub struct ParamDeclarationDto { pub name: String, pub message_match_regex: String, pub capture_group: String, - /// See `crate::data_value::ParamDeclaration::json_field_path`. pub json_field_path: Option, pub kind: MetricPrimitiveKind, - /// See `crate::data_value::ParamDeclaration::elapsed_since_last`. pub elapsed_since_last: bool, - /// See `crate::data_value::ParamDeclaration::auto_derived`. The frontend uses this to disable - /// editing — `POST /api/params` also rejects a request naming an auto-derived param. pub auto_derived: bool, - /// Set only for the built-in `ottyl_seq`/`ottyl_ts`/`ottyl_fmt` names - /// (`crate::engine::OTTYL_RESERVED_PARAM_NAMES`), synthesized into the `GET /api/params` - /// response by `crate::api::list_params` rather than backed by any index entry — distinct from - /// `auto_derived` (which *is* a real, if machine-discovered, `ParamDeclaration`). Always - /// `false` for every other entry. There is no removal path for these at all (not just a - /// disabled one), so the frontend should treat `reserved` as non-deletable/non-editable. #[serde(default)] pub reserved: bool, } @@ -42,13 +31,9 @@ pub struct ParamDeclarationDto { pub struct MetricDeclarationDto { pub name: String, pub message_match_regex: String, - /// Names of standalone `ParamDeclaration`s used as this metric's bucket labels. pub label_params: Vec, - /// Name of the standalone `ParamDeclaration` (if any) whose primitive becomes this metric's per-event bucket value instead of a label. pub value_param: Option, - /// See `crate::engine::DeclareMetricRequest::matched_labels`. pub matched_labels: Option>, - /// See `crate::engine::DeclareMetricRequest::unmatched_labels`. pub unmatched_labels: Option>, } @@ -65,24 +50,7 @@ pub struct StatsDto { pub last_sequence: Option, pub first_timestamp_ms: Option, pub last_timestamp_ms: Option, - /// Ticket 011: a running, *approximate* estimate of the in-memory heap - /// held by currently-ingested logs, in bytes. "Approximate" because it - /// only counts each `IngestedLine::text`'s UTF-8 byte length — it does - /// not account for `String`'s allocator overhead/capacity slack, the - /// `BTreeMap`/`IndexKey` node overhead the text is stored under, or any - /// derived data (bucket values, params) the line produces elsewhere in - /// the index. That's judged the best cheap proxy for "how much memory is - /// this log's text costing us": it's O(1) to compute per log (`text.len()`, - /// already an interned byte count for UTF-8 `String`), moves in lockstep - /// with the dominant cost (raw log text, which is usually much larger - /// than the fixed per-entry bookkeeping), and needs no per-entry - /// bookkeeping beyond what eviction already tracks. pub approx_bytes_total: u64, - /// Ticket 073: the highest ingestion-to-engine-processing lag (`ottyl_ts` at read time vs. when - /// the engine actor actually got around to processing that line) seen within the trailing 5 - /// seconds, in milliseconds — see `crate::engine::record_ingest_lag`'s rolling high-watermark. - /// Stays near zero under normal load; rises under actor backpressure (a slow query in flight, a - /// large backlog) and decays back out of the window once the stall clears. pub ingest_lag_high_watermark_ms: i64, } @@ -92,15 +60,8 @@ pub struct LogsParams { pub start_ms: Option, pub end_ms: Option, pub limit: Option, - /// Raw FilterBar query string, e.g. `regex("error") and ottyl_seq<5`. pub filter: Option, - /// Ticket 045: an already-declared param name to order the returned entries by, in place of the - /// default ingest order (which tracks `ottyl_ts`). `None` (the default) leaves ordering - /// unchanged. pub sort_by: Option, - /// Ticket 045: only meaningful alongside `sort_by`. `true` (the default) orders lines missing - /// that param among themselves by `ottyl_seq` and appends them after the sorted lines; `false` - /// drops those lines from the response entirely. pub sort_missing_fallback: Option, } @@ -109,11 +70,9 @@ pub struct TimelineParams { pub start_ms: Option, pub end_ms: Option, pub bucket_ms: Option, - /// Same FilterBar query string as `LogsParams::filter`; only matching logs are counted into timeline buckets. pub filter: Option, } -/// 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, @@ -122,7 +81,6 @@ pub struct BucketValueDto { pub value: MetricPrimitive, } -/// `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, @@ -132,7 +90,6 @@ pub struct BucketValuesParams { pub limit: Option, } -/// `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, @@ -146,27 +103,22 @@ pub struct PromqlParams { pub end_ms: Option, } -/// JSON error body returned by API handlers on a 4xx/5xx response. #[derive(Debug, Serialize)] pub struct ErrorDto { pub error: String, } -/// Every imported CSS entry is a selectable theme (ticket 020) — there's no separate -/// always-on-overlay mode to opt into. #[derive(Debug, Deserialize)] pub struct ImportCssRequest { pub filename: String, pub content: String, } -/// 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, } -/// `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, @@ -201,18 +153,13 @@ impl From<&CssEntry> for CssEntryDto { } } -/// `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. pub max_logs: Option, - /// Id of the selected custom theme entry, if any. `None` means `color_scheme` alone determines the active palette. pub custom_theme_id: Option, - /// Ntfy topic every alert rule delivers to; `None` until configured. pub ntfy_topic: Option, - /// Ntfy server override; `None` means `https://ntfy.sh`. pub ntfy_server: Option, } @@ -229,7 +176,6 @@ impl From<&Settings> for SettingsDto { } } -/// `max_logs`/`custom_theme_id`/`ntfy_topic`/`ntfy_server` 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,7 +189,6 @@ pub struct UpdateSettingsRequest { pub ntfy_server: Option>, } -/// 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>, @@ -266,7 +211,6 @@ pub struct UpdateAlertRuleRequest { pub condition: AlertCondition, } -/// `server` defaults to `https://ntfy.sh` when absent. #[derive(Debug, Deserialize)] pub struct TestAlertNotificationRequest { pub topic: String, @@ -297,7 +241,6 @@ pub struct CreateProfileRequest { pub name: String, } -/// Mirrors `profile::Panel`. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct PanelDto { pub id: String, @@ -328,7 +271,6 @@ impl From<&PanelDto> for Panel { } } -/// Mirrors `profile::Dashboard`. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct DashboardDto { pub id: String, @@ -361,7 +303,6 @@ pub struct CreateDashboardRequest { pub name: String, } -/// 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 5525b75..2486fae 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -30,8 +30,6 @@ pub struct IngestedLine { pub text: String, } -/// One sample scraped from a Prometheus/OpenMetrics endpoint, pushed straight into a metric's bucket -/// without going through log-line regex matching — see `crate::ingest::run_metrics_poll`. pub struct MetricSample { pub metric_name: String, pub labels: BTreeMap, @@ -39,40 +37,25 @@ pub struct MetricSample { pub timestamp: Timestamp, } -/// Also the request body for `POST /api/params` — `Deserialize` is derived directly here rather than adding a separate DTO, since the shape already matches `ParamDeclaration`. pub type DeclareParamRequest = ParamDeclaration; -/// Response to `EngineMessage::ClearParams` (ticket 052) — also the JSON body for `DELETE /api/params`. #[derive(Debug, Clone, serde::Serialize)] pub struct ClearParamsResult { - /// How many param declarations (manual + auto-derived) were removed. pub removed: usize, - /// How many params `auto_declare_params` re-derived immediately afterward from retained log text. pub reextracted: usize, - /// How many `IndexKey::IngestedLog` entries were still retained (not yet evicted by `max_logs`) - /// and so were re-scanned — surfaced so a caller can tell the user re-extraction only covers - /// logs still held in memory, not lines evicted before the clear. pub logs_considered: usize, } -/// A metric references its params by name (resolved against the engine's standalone -/// `ParamDeclaration` store in `apply_metric_to_log`) rather than embedding its own -/// regex/capture-group/kind per param — see ticket 037. 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, - /// Names of already-declared params whose values become this metric's bucket labels; `#[serde(default)]` so a label-less metric (e.g. matched/unmatched-labels-only) doesn't need an empty array. #[serde(default)] pub label_params: Vec, - /// Name of an already-declared param (if any) whose value becomes this metric's per-event bucket value instead of a label. #[serde(default)] pub value_param: Option, - /// Fixed labels layered on top of what `label_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 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>, } @@ -84,7 +67,6 @@ pub enum EngineQuery { Stats, ParamDeclarations, MetricDeclarations, - /// `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>, @@ -93,7 +75,6 @@ pub enum EngineQuery { since_sequence: Option, limit: Option, }, - /// `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 }, } @@ -109,13 +90,6 @@ pub enum EngineMessage { name: String, respond_to: oneshot::Sender, }, - /// Ticket 052: wipes the entire standalone-param set (manual *and* auto-derived — unlike - /// `reload_engine_declarations`'s profile-switch cleanup, this is an explicit user-driven reset, - /// not a "keep engine-discovered facts across profiles" cleanup), then immediately re-runs - /// `auto_declare_params` over every `IndexKey::IngestedLog` entry still retained in the index, - /// oldest to newest, so declaration-order semantics match a fresh ingest. Lines already evicted - /// by `max_logs` can't regain their params — `ClearParamsResult::logs_considered` reports how - /// many retained lines were re-scanned, so a caller can surface that bound to the user. ClearParams { respond_to: oneshot::Sender, }, @@ -123,7 +97,6 @@ pub enum EngineMessage { request: DeclareMetricRequest, respond_to: oneshot::Sender>, }, - /// Updates an existing metric declaration named `name` in place — same validation as `DeclareMetric`, but fails if no declaration named `name` exists (rather than silently creating one), and re-backfills bucket data from scratch so edited params/regex don't leave stale buckets from the old declaration mixed in. UpdateMetric { name: String, request: DeclareMetricRequest, @@ -133,24 +106,11 @@ pub enum EngineMessage { name: String, respond_to: oneshot::Sender, }, - /// `None` means unlimited; sent on startup and whenever the setting changes so the cap applies without a restart. SetMaxLogs(Option), - /// Ticket 075: reads no longer funnel through this actor loop (see `EngineHandle::reader_factory`) - /// — a sorted `Logs` query is the one read that still needs the actor, and only when no helper - /// index exists yet for `param_name`: the API layer sends this first, awaits the ack, then re-enters - /// its `ReadHandle` and proceeds with the query against the now-built (and now-cached) index. Every - /// other read, and every sorted query once the helper already exists, never reaches this loop at all. EnsureSortHelperIndex { param_name: String, respond_to: oneshot::Sender<()>, }, - /// Ticket 075: acks immediately, with no mutation — a read via `EngineHandle::reader_factory` no - /// longer shares one FIFO queue with `EngineMessage::Ingest`/declaration sends, so nothing - /// guarantees a read right after a `tx.send(...).await` observes that send's effects. Sending - /// this and awaiting its ack is a barrier: since the actor drains messages strictly in order and - /// fully finishes one (including its `publish()`) before starting the next, every message sent - /// before this one is guaranteed visible to readers by the time this ack arrives. See - /// `EngineHandle::barrier`. Barrier(oneshot::Sender<()>), } @@ -162,41 +122,19 @@ pub enum EngineResponse { ParamDeclarations(Vec), MetricDeclarations(Vec), BucketValues(Vec), - /// `Err` holds a human-readable message for a malformed query, which the API layer maps to a 400 response. Promql(Result, String>), - /// 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), } -/// Ticket 075: state a read query (`handle_query`) touches — split out of `run`'s own locals so it -/// can live behind a `left_right` pair instead of the actor's exclusive ownership. Everything else -/// `run` owns (`alert_states`, `ingest_lag_samples`, `metric_sample_sequence`, `log_stream_header`, -/// `max_logs`) is write-only bookkeeping no query ever reads, so it stays a plain local instead. #[derive(Debug, Clone, Default)] pub(crate) struct EngineSnapshot { index: BTreeMap, sort_helper_indexes: HashMap, log_count: usize, approx_bytes_total: u64, - /// Recomputed on every ingest from `ingest_lag_samples` (a run-loop-local, not part of this - /// snapshot) and stashed here as a plain value — purely informational, so per-request freshness - /// down to the millisecond doesn't matter, and stashing it avoids threading `ingest_lag_samples` - /// itself (and `Timestamp::now()`) into a snapshot that must stay deterministic under replay. ingest_lag_high_watermark_ms: i64, } -/// A single deferred mutation to `EngineSnapshot`, applied once per copy (see `Absorb` below). Modeled -/// as a boxed closure rather than an enum mirroring every `EngineMessage` mutation shape — the -/// pragmatic choice given how many distinct mutations `run` already performs. Each closure must be -/// self-contained and deterministic: every value it needs (including anything that would otherwise -/// require `Timestamp::now()` or a mutable run-loop-local) is resolved to a plain value *before* the -/// closure is constructed, since left-right may invoke it once now and once later (when catching the -/// stale copy up), and it must produce the same effect both times. -/// Wrapped in a named struct (rather than a bare `Box` type -/// alias) to sidestep a known rustc limitation ("implementation of `Absorb` is not general enough") -/// where a trait impl'd directly over a boxed `dyn FnMut(&mut T)` trait object confuses the borrow -/// checker's higher-ranked-lifetime check when that type also needs to prove `Send` through -/// `tokio::spawn` — hiding the trait object behind a concrete field sidesteps it. struct EngineOp(Box); impl Absorb for EngineSnapshot { @@ -206,18 +144,11 @@ impl Absorb for EngineSnapshot { fn drop_first(self: Box) {} - /// Only runs once, when a fresh `ReadHandle` is created from the factory — cloning the whole - /// snapshot here (rather than replaying the oplog) is the simplest correct way to bring a new - /// reader copy up to date with the write side, and the cost is paid once per new handle, not per - /// read. fn sync_with(&mut self, first: &Self) { *self = first.clone(); } } -/// Sole owner of `EngineMessage::Ingest`/mutation handling; a `left_right::WriteHandle` (see -/// `EngineSnapshot`) still gives every read query wait-free, lock-free access to a consistent -/// snapshot without going through this channel at all. pub struct EngineHandle { pub tx: mpsc::Sender, pub reader_factory: left_right::ReadHandleFactory, @@ -230,10 +161,6 @@ impl Clone for EngineHandle { } impl EngineHandle { - /// Waits until every `EngineMessage` sent before this call has been fully applied and published - /// — see `EngineMessage::Barrier`'s doc comment for why this is needed at all now that reads - /// bypass the actor's queue. Mainly for tests that ingest/declare then immediately read and - /// expect to see it; production reads generally accept the small, bounded staleness instead. pub async fn barrier(&self) { let (respond_to, response) = oneshot::channel(); if self.tx.send(EngineMessage::Barrier(respond_to)).await.is_ok() { @@ -242,11 +169,6 @@ impl EngineHandle { } } -/// Spawns the actor and returns the `EngineHandle` callers use to send mutations and (via -/// `reader_factory`) read from — the left-right pair has to be constructed here, before the task -/// starts, since `run` itself has no way to hand its internals back out once spawned. -/// `channel_capacity` is exposed mainly so tests can keep using the small capacities they always -/// have; production always passes the same value main.rs used pre-ticket-075. pub fn spawn(settings: SettingsHandle, channel_capacity: usize) -> EngineHandle { let (tx, rx) = mpsc::channel(channel_capacity); let (write, read) = left_right::new::(); @@ -255,9 +177,6 @@ pub fn spawn(settings: SettingsHandle, channel_capacity: usize) -> EngineHandle EngineHandle { tx, reader_factory } } -/// Test-only constructor for callers (`ingest.rs`'s own tests) that want to intercept -/// `EngineMessage`s on a plain `mpsc::Receiver` themselves rather than driving a real `run` loop — -/// they never read through `reader_factory`, so the paired `WriteHandle` is simply dropped. #[cfg(test)] pub(crate) fn test_handle(channel_capacity: usize) -> (EngineHandle, mpsc::Receiver) { let (tx, rx) = mpsc::channel(channel_capacity); @@ -265,31 +184,15 @@ pub(crate) fn test_handle(channel_capacity: usize) -> (EngineHandle, mpsc::Recei (EngineHandle { tx, reader_factory: read.factory() }, rx) } -/// Sole owner of `EngineSnapshot`'s `left_right::WriteHandle`; runs single-threaded so appending an -/// op and publishing it never needs a lock. Every read query instead goes straight to a -/// `left_right::ReadHandle` obtained from `EngineHandle::reader_factory`, wait-free and off this -/// channel entirely (ticket 075) — the one exception is a sorted `Logs` query the first time a given -/// `sort_by` is used, which still needs this loop to build (and cache) its helper index; see -/// `EngineMessage::EnsureSortHelperIndex`. async fn run( mut rx: mpsc::Receiver, settings: SettingsHandle, mut write: left_right::WriteHandle, ) { let mut max_logs: Option = None; - // Its own counter, independent of log `sequence` — scraped samples aren't tied to any ingested line. let mut metric_sample_sequence: u64 = 0; - // 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(); - // Ticket 059: the most recently seen `log stream` header line's column names, cached across - // ingested lines since a `log stream` pipe only prints its header once, before any data line. - // Kept as a plain run-loop local (not part of `EngineSnapshot`) since it's derived purely from - // `text` (see `log_stream_header_transition`) and never read by a query — moving it into the - // snapshot's left-right op closures would mean re-deriving it non-deterministically across two - // separately-timed closure invocations instead of resolving it once, synchronously, right here. let mut log_stream_header: Option = None; - // Ticket 073: rolling high-watermark of ingestion-to-engine-processing lag samples — see - // `record_ingest_lag`/`ingest_lag_high_watermark_ms`. let mut ingest_lag_samples: Vec<(Timestamp, i64)> = Vec::new(); while let Some(message) = rx.recv().await { @@ -305,10 +208,6 @@ async fn run( let ingest_lag_high_watermark_ms = ingest_lag_high_watermark_ms(&ingest_lag_samples, processed_at); - // The header transition is a pure function of `text` alone, so it's resolved here, - // synchronously, against this loop's own `log_stream_header` — not inside the op - // closure below, which left-right may invoke a second time (to catch the stale - // snapshot copy up) and so must never reach back into run-loop-local state. let is_new_header = crate::logfmt::detect_log_stream_header(&text).is_some(); log_stream_header = log_stream_header_transition(&log_stream_header, &text); let row_header = if is_new_header { None } else { log_stream_header.clone() }; @@ -344,9 +243,6 @@ async fn run( }))); write.publish(); - // Alert evaluation reads the index for context (e.g. windowed rate lookups) — peek - // the snapshot just published rather than threading `&index` through the closure - // above, since firing an alert spawns delivery off the hot path regardless. let fired = write .enter() .map(|guard| { @@ -381,10 +277,6 @@ async fn run( write.publish(); } EngineMessage::DeclareParam { request, respond_to } => { - // Peeked against the write side's currently-published state (always fully caught up, - // since every message here appends then immediately publishes) rather than computed - // inside the op closure below — a closure must not itself decide whether to mutate - // from a `Result` it computes, since a failed validation must append no op at all. let result = write .enter() .map(|guard| validate_param_declaration(&guard.index, &request)) @@ -393,9 +285,6 @@ async fn run( let name = request.name.clone(); let request_for_op = request.clone(); write.append(EngineOp(Box::new(move |snap: &mut EngineSnapshot| { - // A manual re-declaration can change the regex/kind under the same name — drop - // any helper index built under the old declaration so a sorted query rebuilds it - // fresh rather than serving values derived from the now-stale regex. snap.sort_helper_indexes.remove(&name); if succeeded { insert_param_declaration(&mut snap.index, request_for_op.clone()); @@ -411,21 +300,12 @@ async fn run( let name_for_op = name.clone(); write.append(EngineOp(Box::new(move |snap: &mut EngineSnapshot| { snap.index.remove(&IndexKey::ParamDeclaration { name: name_for_op.clone() }); - // A helper index built under the old declaration (or a same-named one re-declared - // with a different regex/kind later) would otherwise keep serving stale derived - // values until ticket 061's sweep happens to purge it — dropping it here instead - // means the next sorted query for this param rebuilds it fresh. snap.sort_helper_indexes.remove(&name_for_op); }))); write.publish(); let _ = respond_to.send(existed); } EngineMessage::ClearParams { respond_to } => { - // `clear_all_param_declarations`/`reextract_params_from_retained_logs`'s returned - // counts depend on the exact current param/log state, so they're computed once here - // against a scratch clone of the write side's current index (cheap relative to how - // rarely a user triggers a full param clear) and reused for the response; the closure - // below re-runs the same two calls "for real" against each `EngineSnapshot` copy. let mut scratch = write.enter().map(|guard| guard.index.clone()).unwrap_or_default(); let removed = clear_all_param_declarations(&mut scratch); let (reextracted, logs_considered) = reextract_params_from_retained_logs(&mut scratch); @@ -493,12 +373,6 @@ async fn run( } } -/// Validates the regex and capture group before storing, so a bad declaration is rejected up front rather than discovered later. -/// Read-only half of `declare_param` (ticket 075's left-right conversion): validates without -/// mutating so the actor loop can compute `EngineMessage::DeclareParam`'s response by peeking the -/// write-side snapshot (`WriteHandle::enter`) *before* appending the actual insert as a left-right -/// op — an op closure must not itself decide whether to mutate based on a `Result` it computes -/// internally, since a failed validation must produce no oplog entry at all. fn validate_param_declaration( index: &BTreeMap, request: &DeclareParamRequest, @@ -520,16 +394,11 @@ fn validate_param_declaration( )); } } - // An `elapsed_since_last` param is never text-extracted at all, so `message_match_regex` is - // irrelevant to it — but the field always compiles as a regex regardless of extractor (an empty - // pattern is valid and matches trivially), so it's harmless to still require it to parse. let re = Regex::new(&request.message_match_regex) .map_err(|err| format!("invalid regex: {err}"))?; if request.elapsed_since_last && request.kind != MetricPrimitiveKind::Duration { return Err("elapsed_since_last params must declare kind 'duration'".to_string()); } - // A Boolean-kind param never uses a capture group — its value is just whether the regex matched. - // Neither does an `elapsed_since_last` param — its value is computed, not captured. let uses_json_field = request.json_field_path.as_deref().is_some_and(|path| !path.is_empty()); if request.kind != MetricPrimitiveKind::Boolean && !request.elapsed_since_last @@ -544,16 +413,13 @@ fn validate_param_declaration( Ok(()) } -/// Write-only half of `declare_param` — assumes `validate_param_declaration` already passed. fn insert_param_declaration(index: &mut BTreeMap, request: DeclareParamRequest) { index.insert( IndexKey::ParamDeclaration { name: request.name.clone() }, - // `auto_derived` is never trusted from the request — only `auto_declare_params` may set it. DataValue::ParamDeclaration(ParamDeclaration { auto_derived: false, ..request }), ); } -/// Validates the regex and capture group before storing, so a bad declaration is rejected up front rather than discovered later. Thin wrapper over `validate_param_declaration`/`insert_param_declaration` kept for callers (tests) that don't need the two halves split across a left-right op boundary — see `validate_param_declaration`'s doc comment. #[cfg(test)] fn declare_param( index: &mut BTreeMap, @@ -564,11 +430,6 @@ fn declare_param( Ok(()) } -/// Turns an arbitrary log-line key into a valid regex named-capture-group identifier (ASCII -/// alphanumeric/underscore, not starting with a digit) — the param's own `name` (shown to the user, -/// used as the index key) keeps the raw key text; only the capture group embedded in its generated -/// regex needs sanitizing. Two different keys sanitizing to the same group name is harmless since -/// each param's regex/capture-group pair is private to that one param. fn sanitize_group_name(key: &str) -> String { let mut group: String = key.chars().map(|c| if c.is_ascii_alphanumeric() || c == '_' { c } else { '_' }).collect(); @@ -578,10 +439,6 @@ fn sanitize_group_name(key: &str) -> String { group } -/// Conservative kind inference for an auto-derived param's first sample: only `Number` is guessed -/// (int, falling back to float), everything else stays `String` — `Boolean`/`Timestamp`/`Duration` -/// carry extraction semantics beyond "parses as", so guessing them wrong would silently drop values -/// that don't fit (see `parse_metric_primitive`). fn infer_kind(sample: &str) -> MetricPrimitiveKind { if sample.parse::().is_ok() || sample.parse::().is_ok() { MetricPrimitiveKind::Number @@ -590,9 +447,6 @@ fn infer_kind(sample: &str) -> MetricPrimitiveKind { } } -/// Builds the regex an auto-derived param uses to re-extract its value from any line on read -/// (`derive_log_params`) — quoted keys always look for a `"..."` value, unquoted keys a -/// whitespace-delimited bareword, matching how `crate::logfmt::extract_pairs` found the sample. fn build_auto_param_pattern(key: &str, group: &str, quoted: bool) -> String { let escaped_key = regex::escape(key); if quoted { @@ -602,30 +456,12 @@ fn build_auto_param_pattern(key: &str, group: &str, quoted: bool) -> String { } } -/// Hard ceiling on how many params `auto_declare_params` will ever create automatically over the -/// life of the engine. Without this, one unusual input can permanently inflate the declared-param -/// set forever — `auto_declare_params` never overwrites or prunes a declaration, so every distinct -/// key ever seen sticks around. Ticket 050's real `log stream` dump discovered 220 distinct keys from -/// under 700 lines, mostly one-off WiFi/radio stat counters (e.g. `txRTSFrm`/`rxCTSUcast`/...) that -/// will never recur meaningfully. Chosen alongside `MAX_NEW_AUTO_PARAMS_PER_LINE` (below) rather than -/// on its own: a per-line cap alone still lets an unbounded *variety* of lines each contribute a few -/// new keys forever, so this total ceiling is the actual backstop. -/// Per-line ceiling on how many *new* params a single ingested line may contribute to the auto-derived -/// set. Without this, one unusually wide line (the fixture's WiFi stat dump has 20+ `key=value` pairs -/// in a single line) could burn most of `MAX_AUTO_DECLARED_PARAMS` in one shot, crowding out keys from -/// more representative lines ingested later. Pairs beyond the cap are simply never declared (from this -/// line or any other, since a later line with fewer pairs would still need to reach them within its own -/// per-line budget) — acceptable since they're exactly the low-value, one-off counters ticket 050 flags. fn auto_derived_param_count(index: &BTreeMap) -> usize { param_declarations_range(index).filter(|declaration| declaration.6).count() } -/// One param worth auto-declaring, found on an ingested line by either extraction path -/// `auto_declare_params` supports: a logfmt `key=value` pair (name, and a regex/capture-group built -/// from it) or a JSON leaf field (name being its dot-separated path, extracted via `json_field_path` -/// instead of a regex). Lets `auto_declare_params` share one insert-and-budget loop across both. struct AutoParamCandidate { name: String, value: String, @@ -634,18 +470,6 @@ struct AutoParamCandidate { json_field_path: Option, } -/// Scans `text` for auto-derivable params and declares a standalone param for any name not already -/// declared — auto or manual. A manual declaration for a name always wins and is never touched here; -/// this only fills in names nobody has declared yet. JSON lines (`crate::logfmt::detect_format`) are -/// scanned via `crate::data_value::flatten_json_object` (dot-path leaf fields, extracted by -/// `json_field_path` rather than regex, since a `key=value` scan finds nothing useful in JSON's -/// `"key":value` shape); anything else falls back to the original `key=value` pair scan -/// (`crate::logfmt::extract_pairs`). Run on every ingested line (`EngineMessage::Ingest`), same as -/// `apply_all_metrics`, but note param *values* are still recomputed from raw text at read time -/// (`derive_log_params`), not stored here — so a key discovered on line N also back-fills the column -/// for lines before N on the next read. Bounded by `MAX_AUTO_DECLARED_PARAMS`/ -/// `MAX_NEW_AUTO_PARAMS_PER_LINE` (see their doc comments) so a single burst of unusual lines can't -/// permanently inflate the declared-param set (ticket 050). fn auto_declare_params(index: &mut BTreeMap, text: &str) { let mut total = auto_derived_param_count(index); if total >= MAX_AUTO_DECLARED_PARAMS { @@ -705,25 +529,6 @@ fn auto_declare_params(index: &mut BTreeMap, text: &str) { } } -/// Declares one param per column of the most-recently-seen `log stream` header line, plus a trailing -/// unlabelled `message` param for whatever text follows the last named column (ticket 059) — replacing -/// the old fixed three (`log_type`/`process`/`pid`) with whatever columns the header actually names. -/// `header` is threaded through by the caller (`run`'s ingest loop) rather than owned here: a -/// `log stream` pipe prints its header once, before any data line, and this function only ever sees -/// one ingested line at a time, so the header has to be cached across calls somewhere. A later header -/// line (e.g. a fresh `log stream` invocation piped into the same run) simply replaces the cached one. -/// Unlike `auto_declare_params`, adding these is never *skipped* on account of -/// `MAX_AUTO_DECLARED_PARAMS` — though since they're stored as `auto_derived` too, once present they do -/// count against that shared ceiling, tightening (slightly) the budget left for generic `key=value` -/// pairs. Like `auto_declare_params`, never overwrites an existing declaration (manual or auto) for -/// any of these names. -/// Pure header-value transition for one ingested line (ticket 075's left-right conversion): a -/// header-detecting line replaces the current header outright; anything else leaves it unchanged. -/// Split out from the old combined `auto_declare_log_stream_params` so `EngineMessage::Ingest` can -/// resolve this synchronously against the run loop's own `log_stream_header` var (kept as a plain -/// local, not part of `EngineSnapshot`), then hand the *resulting* header value to the left-right op -/// closure by value — the closure itself must never reach back into run-loop-local state, since it -/// may be replayed a second time (once per `EngineSnapshot` copy). fn log_stream_header_transition( current: &Option, text: &str, @@ -731,10 +536,6 @@ fn log_stream_header_transition( crate::logfmt::detect_log_stream_header(text).or_else(|| current.clone()) } -/// Derives and inserts any not-yet-declared `log stream` column params for one row, given the -/// header already in effect for it (see `log_stream_header_transition`) — the index-mutating half of -/// the old combined `auto_declare_log_stream_params`, kept separate so it can run inside a left-right -/// op closure that only ever touches `EngineSnapshot::index`. fn derive_log_stream_params_from_row( index: &mut BTreeMap, header: &crate::logfmt::LogStreamHeader, @@ -782,10 +583,6 @@ fn derive_log_stream_params_from_row( } } -/// Convenience wrapper combining `log_stream_header_transition` and `derive_log_stream_params_from_row` -/// for callers (tests) that don't need to split them across a left-right op boundary. -/// `EngineMessage::Ingest`'s handler calls the two halves directly instead — see -/// `log_stream_header_transition`'s doc comment for why. #[cfg(test)] fn auto_declare_log_stream_params( index: &mut BTreeMap, @@ -802,12 +599,6 @@ fn auto_declare_log_stream_params( } } -/// Ticket 052: removes every `IndexKey::ParamDeclaration` entry — manual and auto-derived alike. -/// Returns how many were removed. Unlike `RemoveParamDeclaration`'s single-name removal (used by -/// `reload_engine_declarations` during a profile switch, which deliberately keeps auto-derived -/// params since they're engine-discovered facts rather than profile state), this is a full reset: -/// the caller (`EngineMessage::ClearParams`) immediately re-runs `auto_declare_params` afterward, so -/// wiping the auto-derived ones too just means they come back fresh instead of stale. fn clear_all_param_declarations(index: &mut BTreeMap) -> usize { let names: Vec = param_declarations_range(index).map(|(name, ..)| name.to_string()).collect(); @@ -818,16 +609,6 @@ fn clear_all_param_declarations(index: &mut BTreeMap) -> us removed } -/// Ticket 052: re-runs `auto_declare_params` against every `IndexKey::IngestedLog` entry still held -/// in the index, oldest to newest (matching normal ingest order, so declaration-order semantics — -/// e.g. which key "wins" a naming collision — come out the same as if the lines were freshly -/// ingested). Only ever called right after `clear_all_param_declarations`, so every param produced -/// here is fresh, not a re-declaration. -/// -/// Returns `(reextracted, logs_considered)`: how many params came back, and how many retained log -/// lines were re-scanned. Lines already dropped by `evict_excess_logs` before this ran aren't in the -/// index anymore and so can't contribute — `logs_considered` is how the caller surfaces that bound -/// to the user, rather than the re-extraction silently looking complete when it isn't. fn reextract_params_from_retained_logs(index: &mut BTreeMap) -> (usize, usize) { let texts: Vec = logs_range(index).map(|(_, _, text)| text.to_string()).collect(); let logs_considered = texts.len(); @@ -838,10 +619,6 @@ fn reextract_params_from_retained_logs(index: &mut BTreeMap (reextracted, logs_considered) } -/// Validates the metric's own regex plus that every name in `label_params`/`value_param` refers to -/// an already-declared standalone param (not just the first bad one, so a rejection reports every -/// problem at once). Shared by `declare_metric` and `update_metric` so both reject a bad -/// declaration the same way. fn validate_metric_request( index: &BTreeMap, request: &DeclareMetricRequest, @@ -899,10 +676,6 @@ fn declare_metric( Ok(()) } -/// Updates the metric declaration named `existing_name` in place, reusing `declare_metric`'s validation so an edit is rejected the same way a bad new declaration would be. Fails (rather than creating a fresh declaration) if no metric named `existing_name` exists yet. Existing bucket/last-event data for this metric is cleared first and rebuilt from the full log history under the new regex/params, so edited matchers don't leave stale buckets from the previous declaration mixed in with fresh ones. Supports renaming (when `request.name` differs from `existing_name`) as long as the new name isn't already taken by another declaration. -/// Read-only checks `update_metric` performs before mutating anything — split out (ticket 075) so -/// the actor loop can peek the write-side snapshot for `EngineMessage::UpdateMetric`'s `Result` -/// before deciding whether to append a mutating op at all. fn validate_update_metric( index: &BTreeMap, existing_name: &str, @@ -955,7 +728,6 @@ fn update_metric( Ok(()) } -/// Removes every `Bucket` and `LastMetricEvent` entry belonging to `metric_name`, ahead of a full re-backfill in `update_metric` — otherwise the old declaration's buckets would sit alongside freshly backfilled ones under the same metric name. fn clear_metric_data(index: &mut BTreeMap, metric_name: &str) { index.retain(|key, _| match key { IndexKey::Bucket { metric_name: name, .. } => name != metric_name, @@ -964,19 +736,12 @@ fn clear_metric_data(index: &mut BTreeMap, metric_name: &st }); } -/// Removes `name`'s declaration and, per ticket 051, purges its bucket history along with it -/// (reusing `clear_metric_data`, same as `update_metric`'s re-backfill does for the old -/// declaration) rather than leaving it orphaned — an orphaned bucket would let a stale dashboard -/// panel or PromQL query keep silently returning old data by name instead of failing with a clear -/// "unknown metric" error (see the declared-or-has-data check in `evaluate_promql`). Returns -/// whether a declaration existed, which the caller maps to 404 vs success at the API layer. fn remove_metric_declaration(index: &mut BTreeMap, name: &str) -> bool { let existed = index.remove(&IndexKey::MetricDeclaration { name: name.to_string() }).is_some(); clear_metric_data(index, name); existed } -/// 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())), @@ -991,31 +756,17 @@ fn parse_metric_primitive(raw: &str, kind: MetricPrimitiveKind) -> Option raw.parse::().ok().map(MetricPrimitive::Duration), - // A standalone Boolean param never reaches this branch (see derive_log_params, which derives its - // value from whether the regex matched rather than parsing captured text); reached only if a metric - // matcher references a Boolean-kind param, where it's parsed as a literal "true"/"false" capture. MetricPrimitiveKind::Boolean => raw.parse::().ok().map(MetricPrimitive::Boolean), } } -/// A `ParamDeclaration` with its regex pre-compiled, keyed by name — resolved once per ingest/backfill -/// pass (not once per metric) so metrics sharing a param don't each recompile its regex. A param -/// whose stored regex fails to compile (shouldn't happen — `declare_param` validates it) is omitted -/// rather than panicking, same as `compiled_param_declarations`. struct ResolvedParam { regex: Regex, capture_group: String, json_field_path: Option, kind: MetricPrimitiveKind, - /// See `crate::data_value::ParamDeclaration::elapsed_since_last`. When set, `regex`/ - /// `capture_group`/`json_field_path` above are unused — resolution goes through - /// `resolve_param_value`'s computed path instead of `extract_param_raw`. elapsed_since_last: bool, - /// See `crate::data_value::ParamDeclaration::log_stream_gated`. log_stream_gated: bool, } @@ -1039,13 +790,6 @@ fn resolved_param_declarations(index: &BTreeMap) -> HashMap .collect() } -/// Extracts a param's raw captured text from `text` via its own regex (JSON field path taking -/// precedence when set) — shared by `apply_metric_to_log` (resolving a metric's `label_params`/ -/// `value_param`) and `derive_log_params` (a standalone param declaration shown as a logs-table -/// column). Returns `None` when the regex doesn't match, or (for a JSON field path) the field can't -/// be resolved. `log_stream_gated` (ticket 076) additionally requires `text` to pass -/// `crate::logfmt::is_log_stream_line` before the regex is trusted at all — see -/// `crate::data_value::ParamDeclaration::log_stream_gated`. fn extract_param_raw( text: &str, regex: &Regex, @@ -1062,15 +806,6 @@ fn extract_param_raw( } } -/// Resolves one param's value against `text`/`timestamp`, dispatching purely on what the param -/// itself declares (`ResolvedParam::elapsed_since_last`) rather than on whether it's being resolved -/// as a label or as the value — the same resolution is used for both in `apply_metric_to_log`, so -/// an `elapsed_since_last` param behaves identically regardless of which list references it. An -/// elapsed-since-last param's value is elapsed time since the previous event that produced -/// `labels_so_far` under `metric_name`, tracked via a `LastMetricEvent` index entry; its first -/// occurrence returns `None` (nothing to diff against yet) rather than a bogus zero. Every other -/// param falls through to ordinary text extraction (regex capture group or JSON field path) parsed -/// as its declared kind. fn resolve_param_value( index: &mut BTreeMap, metric_name: &str, @@ -1080,8 +815,6 @@ fn resolve_param_value( timestamp: Timestamp, ) -> Option { if resolved.elapsed_since_last { - // Tracked separately from Bucket: the first occurrence for a given label set 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_so_far.clone(), @@ -1105,13 +838,6 @@ fn resolve_param_value( parse_metric_primitive(&raw, resolved.kind) } -/// Resolves `label_params`/`value_param` by name against `resolved_params` — a dangling reference -/// (a param removed after this metric was declared) skips the whole match, same as any other -/// extraction failure. At most one param is the value param (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. When `regex` (the metric's own message-match regex) -/// doesn't match and `unmatched_labels` is `Some`, an occurrence is still registered with those -/// fixed labels. #[allow(clippy::too_many_arguments)] fn apply_metric_to_log( index: &mut BTreeMap, @@ -1183,7 +909,6 @@ fn push_bucket_value( } } -/// Backfills buckets for a newly-declared metric against every already-ingested log, not just future ones. #[allow(clippy::too_many_arguments)] fn backfill_metric( index: &mut BTreeMap, @@ -1241,7 +966,6 @@ fn apply_all_metrics( ) }) .collect(); - // Resolved once per ingested line (shared across every metric that runs against it), not once per metric. let resolved_params = resolved_param_declarations(index); for (name, message_match_regex, label_params, value_param, matched_labels, unmatched_labels) in metrics { if let Ok(re) = Regex::new(&message_match_regex) { @@ -1262,7 +986,6 @@ fn apply_all_metrics( } } -/// 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, @@ -1282,24 +1005,17 @@ fn bucket_values<'a>( .flatten() } -/// 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 }, Timestamp { op: CompareOp, target_ms: i64 }, - /// `ottyl_fmt value` — like `Json`, this is line-local and re-detected fresh per line in - /// `eval_compiled` (`crate::logfmt::detect_format`) rather than pre-resolved against the index. Fmt { op: CompareOp, target: String }, MetricMatch(HashSet), - /// `json[field] value` — unlike `MetricMatch`, this can't be pre-resolved against the - /// index (the JSON lives in the log line itself, not a declared metric/param), so it's - /// evaluated per line in `eval_compiled` by parsing `text` as JSON. Json { field: String, op: CompareOp, value: FilterValue }, And(Box, Box), Or(Box, Box), } -/// 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, @@ -1340,7 +1056,6 @@ fn compile_filter( } } -/// A `Raw` value is accepted defensively (parsed as a number) even though the grammar shouldn't produce one for `ottyl_seq`. fn compile_ottyl_seq_comparison(op: CompareOp, value: &FilterValue) -> Result { let target = match value { FilterValue::Number(n) => *n, @@ -1351,7 +1066,6 @@ fn compile_ottyl_seq_comparison(op: CompareOp, value: &FilterValue) -> Result CompiledFilter { let target = match value { FilterValue::Number(n) => n.to_string(), @@ -1378,7 +1088,6 @@ fn compile_ottyl_fmt_comparison(op: CompareOp, value: &FilterValue) -> CompiledF CompiledFilter::Fmt { op, target } } -/// `metric[param] value` for any metric name other than `ottyl_seq`/`ottyl_ts`/`ottyl_fmt` — see [`CompiledFilter`]'s doc comment. fn compile_metric_comparison( index: &BTreeMap, metric: &str, @@ -1402,12 +1111,6 @@ fn compile_metric_comparison( ) })?; - // Validate `param` against the metric's actual declared `label_params` (ticket 037's unified - // `MetricDeclaration`) rather than just reading whatever key happens to be present in a - // bucket's `labels` map — a typo or a param that was never declared as a label on this metric - // would otherwise silently match nothing instead of being rejected up front. A metric name - // with no declaration at all is left alone here (`bucket_values` below simply yields nothing - // for it, matching prior behavior) since there's no label_params list to validate against. if let Some(DataValue::MetricDeclaration { label_params, .. }) = index.get(&IndexKey::MetricDeclaration { name: metric.to_string() }) { @@ -1431,7 +1134,6 @@ fn compile_metric_comparison( Ok(CompiledFilter::MetricMatch(matching)) } -/// 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, @@ -1475,9 +1177,6 @@ fn compare_metric_primitive( Ok(ordering.is_some_and(|ordering| apply_compare_op(ordering, op))) } -/// `json[field] value`: the log line is parsed as JSON fresh on every call (no per-line -/// cache), since this doesn't go through the index-backed pre-resolution `MetricMatch` uses. -/// Any line that isn't a JSON object, or lacks `field`, simply doesn't match — no error. pub(crate) fn eval_json_comparison(text: &str, field: &str, op: CompareOp, value: &FilterValue) -> bool { let Ok(parsed) = serde_json::from_str::(text) else { return false; @@ -1488,8 +1187,6 @@ pub(crate) fn eval_json_comparison(text: &str, field: &str, op: CompareOp, value compare_json_value(field_value, op, value) } -/// Mirrors `compare_metric_primitive`'s "mismatched kinds never match" behavior. Only `Eq`/`Ne` -/// are meaningful for booleans, and `null`/array/object fields aren't scalar-comparable at all. fn compare_json_value(field_value: &serde_json::Value, op: CompareOp, value: &FilterValue) -> bool { match field_value { serde_json::Value::Number(n) => { @@ -1528,7 +1225,6 @@ fn compare_json_value(field_value: &serde_json::Value, op: CompareOp, value: &Fi } } -/// `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, @@ -1540,7 +1236,6 @@ pub(crate) fn apply_compare_op(ordering: Ordering, op: CompareOp) -> bool { } } -/// 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), @@ -1632,45 +1327,19 @@ fn param_declarations_range( }) } -/// A param whose stored regex fails to compile (shouldn't happen — `declare_param` validates it) is -/// skipped rather than panicking. An `elapsed_since_last` param is skipped outright — it has no -/// meaningful reading as a standalone logs-table column (its value depends on which metric's label -/// set it's grouped under, which only exists once it's referenced by a metric's `label_params`/ -/// `value_param`). -/// Ticket 060: key type for a sort helper index — one such index exists per param name that's ever -/// been used as a `sort_by`. Declaration order matters for the derived `Ord`: every `WithParam` key -/// sorts before every `WithoutParam` key, and within each variant ordering falls out of the tuple -/// fields (value then sequence; sequence alone), which is exactly `sort_missing_fallback`'s "sorted -/// lines first, then lines missing the param ordered by `ottyl_seq`" semantics — expressed as part of -/// the index's own ordering instead of a separate partition-and-append step per query. #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] enum SortHelperKey { WithParam(MetricPrimitive, u64), WithoutParam(u64), } -/// One param's helper index plus when a query last actually read from it — ticket 061 sweeps entries -/// whose `last_accessed` is stale. The map's value is that line's `Timestamp`, not `()`, purely so a -/// query can reconstruct the exact `IndexKey::IngestedLog { sequence, timestamp }` primary-index key -/// and look its text up in O(log n) instead of re-scanning `logs_range` to find it by sequence alone. -/// `Clone`/`Debug` so `EngineSnapshot` (which holds these, ticket 075) can derive both — `sync_with` -/// clones the whole snapshot once per new `left_right::ReadHandle`, not per read. #[derive(Debug, Clone)] struct SortHelperIndexEntry { index: BTreeMap, last_accessed: std::time::Instant, } -/// How long a sort helper index may sit unread before ticket 061's sweep drops it. Not exposed via -/// `Settings` (ticket 061 explicitly scopes that out) — a dropped index is always transparently -/// rebuildable on the next query for that sort key, so this is purely a memory-bound knob, not a -/// user-visible behavior change. -/// Derives a single named param's value from `text`, for helper-index maintenance where deriving -/// every declared param (`derive_log_params`) per ingested line would be wasted work when only one -/// sort key's helper index actually needs updating. -/// A compiled param declaration: name, regex, capture group, JSON field path, kind, and whether -/// `crate::data_value::ParamDeclaration::log_stream_gated` was set. type CompiledParamDeclaration = (String, Regex, String, Option, MetricPrimitiveKind, bool); fn derive_single_param( @@ -1694,9 +1363,6 @@ fn derive_single_param( parse_metric_primitive(&raw, *kind) } -/// One-time full scan building a sort helper index for `sort_by` from every currently-ingested log — -/// used both the first time a param is sorted by (no helper index exists yet) and to transparently -/// rebuild one ticket 061's sweep purged. fn build_sort_helper_index(index: &BTreeMap, sort_by: &str) -> BTreeMap { let compiled_params = compiled_param_declarations(index); logs_range(index) @@ -1710,11 +1376,6 @@ fn build_sort_helper_index(index: &BTreeMap, sort_by: &str) .collect() } -/// Incrementally extends every currently-live helper index with one freshly-ingested line, called -/// from `EngineMessage::Ingest` right alongside the primary index insert (ticket 060) — this is what -/// lets a sorted query avoid re-deriving params for the whole timeline on every call once a helper -/// index for that sort key exists. A no-op when no helper index has been built yet (the common case -/// for a param nobody has sorted by), so ingest pays nothing for sort keys that are never queried. fn update_sort_helper_indexes_on_ingest( sort_helper_indexes: &mut HashMap, index: &BTreeMap, @@ -1735,20 +1396,10 @@ fn update_sort_helper_indexes_on_ingest( } } -/// Ticket 061: drops any helper index whose `last_accessed` exceeds `SORT_HELPER_INDEX_PURGE_AFTER`. -/// Cheap to call opportunistically on every ingest (`HashMap::retain` is a no-op scan when nothing's -/// stale) rather than needing a dedicated timer/task — a purged index rebuilds transparently via -/// `build_sort_helper_index` the next time something sorts by it. fn purge_stale_sort_helper_indexes(sort_helper_indexes: &mut HashMap) { sort_helper_indexes.retain(|_, entry| entry.last_accessed.elapsed() < SORT_HELPER_INDEX_PURGE_AFTER); } -/// Builds (if missing) or touches (if present) the helper index for `sort_by`, and bumps its -/// `last_accessed` either way. Ticket 075: this is now the *only* place a sort helper index gets -/// built or access-time-refreshed — a plain read query can no longer do so itself, since reads go -/// straight through a `left_right::ReadHandle` and can't mutate `EngineSnapshot`. A sorted `Logs` -/// query whose helper index is missing has the API layer send `EngineMessage::EnsureSortHelperIndex` -/// (handled by calling this) before it re-enters its `ReadHandle` and proceeds. fn ensure_sort_helper_index_built( sort_helper_indexes: &mut HashMap, index: &BTreeMap, @@ -1779,11 +1430,6 @@ fn compiled_param_declarations(index: &BTreeMap) -> Vec BTreeMap { let mut params = BTreeMap::new(); for (name, regex, capture_group, json_field_path, kind, log_stream_gated) in declarations { @@ -1815,37 +1461,17 @@ fn derive_log_params(text: &str, declarations: &[CompiledParamDeclaration]) -> B params } -/// `LogEntryDto::params` for one line: `derive_log_params`'s declared-param values, plus the -/// reserved `ottyl_fmt` pseudo-param (ticket 072) — computed line-locally the same way filter-bar -/// `ottyl_fmt` comparisons already do (`compile_ottyl_fmt_comparison`), not stored as a -/// `ParamDeclaration`, so it's merged in here rather than inside `derive_log_params` itself (whose -/// other callers/tests want the plain declared-param set only). fn log_entry_params(text: &str, declarations: &[CompiledParamDeclaration]) -> BTreeMap { let mut params = derive_log_params(text, declarations); params.insert("ottyl_fmt".to_string(), MetricPrimitive::String(crate::logfmt::detect_format(text).to_string())); params } -/// Ticket 011's approximate per-log byte size: see `StatsDto::approx_bytes_total`'s -/// doc comment for why the raw UTF-8 text length is the chosen proxy rather than a -/// full struct-size estimate. fn approx_log_bytes(text: &str) -> u64 { text.len() as u64 } -/// Ticket 073: how long a rolling ingestion-to-engine-processing lag sample stays eligible to be -/// the reported high watermark. -/// Ticket 073: at most this many lag samples are held at once — a fresh sample only displaces the -/// current *lowest*-lag held sample (never appended past this cap), so the collection always -/// reflects the highest few lags seen recently rather than every sample. -/// Records one `(engine processing time) - (ottyl_ts)` lag sample into the rolling high-watermark -/// collection the wishlist asked for (ticket 073): up to `INGEST_LAG_WATERMARK_MAX_ENTRIES` samples, -/// each valid only within the trailing `INGEST_LAG_WATERMARK_WINDOW_MS` — a sample older than that -/// window is dropped outright (an ingest burst that happened seconds ago shouldn't keep the reported -/// watermark artificially high forever), and once the collection is at capacity a new sample only -/// replaces the current *lowest*-lag entry, and only if it's actually larger. Called on every -/// `EngineMessage::Ingest`, so this stays O(`INGEST_LAG_WATERMARK_MAX_ENTRIES`) per ingested line. fn record_ingest_lag(samples: &mut Vec<(Timestamp, i64)>, now: Timestamp, lag_ms: i64) { samples.retain(|(recorded_at, _)| now.as_millis() - recorded_at.as_millis() < INGEST_LAG_WATERMARK_WINDOW_MS); if samples.len() < INGEST_LAG_WATERMARK_MAX_ENTRIES { @@ -1860,9 +1486,6 @@ fn record_ingest_lag(samples: &mut Vec<(Timestamp, i64)>, now: Timestamp, lag_ms } } -/// The current high watermark to report (`StatsDto::ingest_lag_high_watermark_ms`): the highest -/// still-in-window sample, or `0` when nothing's been recorded (or everything's aged out) — a -/// missing signal reads as "no lag", not as an error. fn ingest_lag_high_watermark_ms(samples: &[(Timestamp, i64)], now: Timestamp) -> i64 { samples .iter() @@ -1872,12 +1495,6 @@ fn ingest_lag_high_watermark_ms(samples: &[(Timestamp, i64)], now: Timestamp) -> .unwrap_or(0) } -/// 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. `approx_bytes_total` (ticket 011) is decremented by -/// each evicted log's approximate size, mirroring the increment on ingest. fn evict_excess_logs( index: &mut BTreeMap, log_count: &mut usize, @@ -1912,11 +1529,6 @@ fn logs_range(index: &BTreeMap) -> impl Iterator, before_sequence: u64, @@ -1930,14 +1542,6 @@ pub(crate) fn logs_since( }) } -/// How `handle_query_inner`'s `Logs`/`sort_by` branch gets at a sort helper index — split into two -/// variants (ticket 075) because the two callers hold fundamentally different kinds of access to -/// `sort_helper_indexes`: a direct caller (tests, and formerly the actor) has `&mut` access and can -/// lazily build-or-touch a missing helper right there; the read-only API path only ever has a -/// `left_right::ReadGuard`'s `&EngineSnapshot`, which can't mutate anything — it relies on -/// `EngineMessage::EnsureSortHelperIndex` having built the helper through the actor beforehand, and -/// simply looks it up. `resolve` takes `self` by value (rather than `&mut self`) specifically so the -/// returned reference can carry the enum's own `'a` lifetime instead of a shorter reborrowed one. enum SortHelperAccess<'a> { #[cfg(test)] Mutable(&'a mut HashMap), @@ -1945,7 +1549,6 @@ enum SortHelperAccess<'a> { } impl<'a> SortHelperAccess<'a> { - // `index` is only read by the `#[cfg(test)]` `Mutable` arm below — unused in a non-test build. #[cfg_attr(not(test), allow(unused_variables))] fn resolve( self, @@ -1963,8 +1566,6 @@ impl<'a> SortHelperAccess<'a> { } } -/// Test-facing entry point: lazily builds a missing sort helper index the same way ticket 060/061 -/// always has. `EngineMessage`'s handlers no longer call this (see `handle_query_ro`). #[cfg(test)] fn handle_query( index: &BTreeMap, @@ -1982,10 +1583,6 @@ fn handle_query( ) } -/// Read-only entry point for the API layer's real query path (ticket 075): reads straight off a -/// `left_right::ReadGuard`'s `&EngineSnapshot`, no actor round-trip, no mutation. Callers must check -/// `has_sort_helper_index` and send `EngineMessage::EnsureSortHelperIndex` first when a sorted `Logs` -/// query's helper index doesn't exist yet — see `api.rs`'s `ask`. pub(crate) fn handle_query_ro(snapshot: &EngineSnapshot, query: EngineQuery) -> EngineResponse { handle_query_inner( &snapshot.index, @@ -1996,9 +1593,6 @@ pub(crate) fn handle_query_ro(snapshot: &EngineSnapshot, query: EngineQuery) -> ) } -/// What `sort_by` a query needs a sort helper index for, if any — what the API layer checks -/// (alongside `has_sort_helper_index`) to decide whether `EngineMessage::EnsureSortHelperIndex` needs -/// sending before `handle_query_ro` can proceed. pub(crate) fn query_sort_by(query: &EngineQuery) -> Option<&str> { match query { EngineQuery::Logs(params) => params.sort_by.as_deref(), @@ -2006,7 +1600,6 @@ pub(crate) fn query_sort_by(query: &EngineQuery) -> Option<&str> { } } -/// Whether a sort helper index for `sort_by` already exists in `snapshot` — see `query_sort_by`. pub(crate) fn has_sort_helper_index(snapshot: &EngineSnapshot, sort_by: &str) -> bool { snapshot.sort_helper_indexes.contains_key(sort_by) } @@ -2027,15 +1620,12 @@ fn handle_query_inner( }, None => None, }; - // Ticket 045: `sort_by` must name an already-declared param — reject anything else up - // front rather than silently falling through to "every line is missing it". if let Some(sort_by) = params.sort_by.as_deref() { if !param_declarations_range(index).any(|(name, ..)| name == sort_by) { return EngineResponse::Error(format!("unknown param '{sort_by}' for sort_by")); } } let limit = params.limit.unwrap_or(500).min(10_000); - // 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 filtered = logs_range(index) .filter(|(sequence, timestamp, _)| { @@ -2050,8 +1640,6 @@ fn handle_query_inner( }); let entries: Vec = match params.sort_by.as_deref() { - // Default path: unchanged from pre-045 behavior — take(limit) before mapping, so - // only the entries actually returned pay the per-line param-derivation cost. None => filtered .take(limit) .map(|(sequence, timestamp, text)| LogEntryDto { @@ -2061,11 +1649,6 @@ fn handle_query_inner( params: log_entry_params(text, &compiled_params), }) .collect(), - // Ticket 060: walk the (lazily built-or-reused) helper index for this sort key in - // key order instead of materializing and sorting the full filtered set — the index's - // own key ordering already expresses "sorted-by-value lines, then lines missing the - // param ordered by ottyl_seq" (`sort_missing_fallback`'s semantics), so this just - // filters/limits as it walks rather than partition-sort-concat afterward. Some(sort_by) => { static EMPTY_HELPER: std::sync::LazyLock> = std::sync::LazyLock::new(BTreeMap::new); @@ -2090,9 +1673,6 @@ fn handle_query_inner( { continue; } - // A line evicted (by `max_logs`) since this key was recorded is simply - // skipped, same as buckets/other derived data aren't retroactively pruned - // on eviction elsewhere in this file. let Some(DataValue::LogLiteral { text }) = index.get(&IndexKey::IngestedLog { sequence, timestamp: *timestamp }) else { @@ -2246,7 +1826,6 @@ fn handle_query_inner( } } -/// 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 as a bare series or under `rate()`/`gauge()`, which only ever count or sample points rather than summing this value. `sum()`/`avg()` do read it, so `evaluate_promql` rejects those aggregations up front for a `String`-kind metric (see `declared_value_kind`) rather than letting this 1.0 stand in for a sum. fn coerce_to_f64(value: &MetricPrimitive) -> f64 { match value { MetricPrimitive::Number(n) => match n { @@ -2266,7 +1845,6 @@ fn coerce_to_f64(value: &MetricPrimitive) -> f64 { } } -/// 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], @@ -2276,8 +1854,6 @@ fn matches_label_matchers( }) } -/// Projects a full label map down to just the names in `by_labels`; an empty `by_labels` collapses -/// everything to the empty map, i.e. "one group total" (the historical `sum()`/`avg()` behavior). fn project_labels( labels: &BTreeMap, by_labels: &[String], @@ -2288,20 +1864,6 @@ fn project_labels( .collect() } -/// Thin adapter over the single shared windowed-rate implementation (`crate::rate`, ticket 040): -/// PromQL's series here are plain millisecond `i64` timestamps rather than `Timestamp`, so this -/// just converts at the boundary and delegates — the actual window-scan logic lives only in -/// `crate::rate::windowed_rate_series`, also used (via `crate::rate::RateWindow`) by alert -/// `Threshold`/`Ratio` conditions in `crate::alerts`, so both agree on what "windowed rate" means. -/// -/// Ticket 057: `rate::windowed_rate_series` only evaluates *at each occurrence's own timestamp*, -/// so a metric whose occurrences stop (or start) well inside a much wider queried range produces a -/// series that just ends (or begins) at the last (or first) real occurrence — nothing says "the -/// rate is 0 here" for the rest of the range. When `start_ms`/`end_ms` are given (a dashboard panel -/// always supplies both), this appends a synthetic boundary point at each edge — computed via -/// `rate::rate_at` against the original occurrence points, so a boundary that falls inside a -/// still-warm window reads its true partial value rather than being forced to 0 — whenever the -/// real series doesn't already reach that edge. fn windowed_rate_series( points: &[(i64, f64)], range_ms: i64, @@ -2330,20 +1892,7 @@ fn windowed_rate_series( series } -/// Fixed window width (ms) used by `merge_series_windowed` to combine multiple raw-occurrence -/// sub-series into one time series — the same fixed-width bucketing style `histogram()`/the -/// `Timeline` query already use elsewhere in this file, just applied along the time axis instead of -/// the value axis. Occurrence timestamps across different label groups (e.g. different hosts serving -/// the same path) essentially never land on the same millisecond, so summing/averaging only requires -/// matching *this* window to combine into "total over time" rather than going sparse (ticket 042). -/// Merges several series into one combined time series (ticket 042). A single sub-series is returned -/// untouched — there's only one contributor, so there's nothing to merge and no reason to lose -/// resolution. With two or more sub-series, points are bucketed into fixed `MERGE_WINDOW_MS`-wide -/// windows (rather than requiring an exact-millisecond timestamp match) before summing/averaging -/// within each window, so raw occurrence markers from different label groups combine into a -/// meaningful "total over time" instead of each window reflecting only whichever sub-series happened -/// to report at that exact millisecond. fn merge_series_windowed(sub_series: &[Vec<(i64, f64)>], is_avg: bool) -> Vec<(i64, f64)> { if sub_series.len() <= 1 { return sub_series.first().cloned().unwrap_or_default(); @@ -2365,9 +1914,6 @@ fn merge_series_windowed(sub_series: &[Vec<(i64, f64)>], is_avg: bool) -> Vec<(i .collect() } -/// Merges several already-windowed rate series by sweeping the union of their timestamps, carrying each -/// sub-series' last-known value forward (0.0 before its first point) so every step reflects every -/// sub-series' most recent reading — unlike `merge_series_windowed`, which buckets by fixed time windows instead. fn merge_series_locf(sub_series: &[Vec<(i64, f64)>], is_avg: bool) -> Vec<(i64, f64)> { let mut all_timestamps: std::collections::BTreeSet = std::collections::BTreeSet::new(); for points in sub_series { @@ -2391,10 +1937,6 @@ fn merge_series_locf(sub_series: &[Vec<(i64, f64)>], is_avg: bool) -> Vec<(i64, .collect() } -/// The value kind a metric's buckets carry: the kind of its `Value`-role param, or `Number` when -/// it has none (every occurrence then falls back to a fixed `Number::Int(1)` marker — see -/// `apply_metric_to_log`). Returns `None` when `metric_name` has no declaration at all, in which -/// case there's nothing to validate against and the query simply yields no series. fn declared_value_kind( index: &BTreeMap, metric_name: &str, @@ -2416,15 +1958,7 @@ fn declared_value_kind( } } -/// The three reserved param names usable without declaring anything — see `compile_ottyl_seq_comparison`/ -/// `compile_ottyl_ts_comparison`/`compile_ottyl_fmt_comparison` for the filter-bar equivalent, and -/// `webapp/src/components/Docs.tsx` for the user-facing docs. Shared by `evaluate_promql` (dashboard -/// panels, this file) and `crate::api::list_params` (`GET /api/params` discoverability). -/// For `ottyl_seq`/`ottyl_ts` only (not `ottyl_fmt`, which `evaluate_promql` rejects outright before -/// this is consulted): returns a `(sequence, timestamp_ms) -> value` mapping used to synthesize a -/// virtual PromQL series straight from the ingested-log index, since neither is backed by a -/// `MetricDeclaration`/bucket the way every other metric name is. fn ottyl_reserved_numeric_value_fn(metric_name: &str) -> Option f64> { match metric_name { "ottyl_seq" => Some(|sequence, _timestamp_ms| sequence as f64), @@ -2433,23 +1967,12 @@ fn ottyl_reserved_numeric_value_fn(metric_name: &str) -> Option } } -/// Evaluates a parsed PromQL query against `index`, producing chart-ready series: baseline is one series per label combination; `sum()`/`avg()` merge series into one series per `by_labels` group (the whole result collapses to a single label-less series when `by_labels` is empty); `rate()` — bare or wrapped in `sum()`/`avg()` — turns each series into a windowed occurrences-per-second reading via `windowed_rate_series`, so it plots as a real line rather than one trailing point; `histogram()` groups by `by_labels` the same way sum()/avg() do, then buckets each group's values independently, so `histogram(m) by (label)` yields one histogram (a family of `le`-labeled series carrying the group's own labels) per label group instead of collapsing all groups together. -/// `sum()`/`avg()` over a `String`-kind metric are rejected up front — `coerce_to_f64` has no -/// numeric reading for a string, so silently treating every occurrence as `1.0` would let a sum -/// pass for a count with no signal to the caller. `sum(rate(...))`/`avg(rate(...))` are exempt: -/// the inner `rate()` only counts occurrences per window and never reads the coerced value, so a -/// string-kind metric under `rate()` is already the count semantic the ticket asks for. fn evaluate_promql( index: &BTreeMap, parsed: &PromqlQuery, start_ms: Option, end_ms: Option, ) -> Result, String> { - // `ottyl_fmt` is categorical ("json"/"logfmt"/"none", `crate::logfmt::detect_format`) with no - // sensible numeric reading, unlike `ottyl_seq`/`ottyl_ts` below — rather than silently - // returning an empty series (the failure mode ticket 055 calls out), a dashboard query over it - // is rejected up front with a message pointing back at the filter/query bar, where it already - // works (see `compile_ottyl_fmt_comparison`). if parsed.metric_name == "ottyl_fmt" { return Err( "'ottyl_fmt' is a categorical value (json/logfmt/none), not a number, so it can't be \ @@ -2459,14 +1982,6 @@ fn evaluate_promql( ); } - // Ticket 051: a metric with neither a declaration nor any recorded bucket data is genuinely - // unknown (as opposed to a declared-but-empty-so-far metric, or an undeclared scraped - // Prometheus metric ingested via `--metrics-url`'s `IngestMetricSample`, which never gets a - // `MetricDeclaration` but still has bucket data) — most commonly because a dashboard panel - // still references a metric that has since been deleted. Failing here with a clear error beats - // a panic or a silently-empty result. `ottyl_seq`/`ottyl_ts` (ticket 055) are exempt: they're - // synthesized straight from the ingested-log index below and never have a `MetricDeclaration` - // or bucket entries of their own. if ottyl_reserved_numeric_value_fn(&parsed.metric_name).is_none() && !index.contains_key(&IndexKey::MetricDeclaration { name: parsed.metric_name.clone() }) && bucket_values(index, &parsed.metric_name, None).next().is_none() @@ -2488,11 +2003,6 @@ fn evaluate_promql( let mut series_points: BTreeMap, Vec<(i64, f64)>> = BTreeMap::new(); if let Some(reserved_value) = ottyl_reserved_numeric_value_fn(&parsed.metric_name) { - // `ottyl_seq`/`ottyl_ts` are computed line-locally (see `compile_ottyl_seq_comparison`/ - // `compile_ottyl_ts_comparison`) rather than backed by a `MetricDeclaration` and bucket - // entries, so they're synthesized here straight from the ingested-log index instead of - // going through `bucket_values`. Neither carries any labels, so a non-empty matcher list - // simply matches nothing, same as an unmatched label filter would against a real metric. let no_labels: BTreeMap = BTreeMap::new(); if matches_label_matchers(&no_labels, &parsed.matchers) { for (sequence, timestamp, _text) in logs_range(index) { @@ -2533,9 +2043,6 @@ fn evaluate_promql( .map(|(labels, points)| PromqlSeriesDto { labels, points }) .collect(), Some(AggOp::Count) => { - // Occurrence count per group, ignoring each point's coerced value entirely — the - // explicit opt-in for the count-shaped result `sum()`/`avg()` used to give a - // `String`-kind metric by accident (see `coerce_to_f64`). let mut groups: BTreeMap, Vec>> = BTreeMap::new(); for (labels, points) in series_points { let count_points: Vec<(i64, f64)> = @@ -2553,7 +2060,6 @@ fn evaluate_promql( } Some(AggOp::Sum) | Some(AggOp::Avg) => { let is_avg = parsed.agg == Some(AggOp::Avg); - // Validated by `promql::parse_query`: an inner rate() always carries a range. let rate_range_ms = parsed .inner_rate .then(|| parsed.range.expect("inner rate() always carries a range").as_millis() as i64); @@ -2581,7 +2087,6 @@ fn evaluate_promql( .collect() } Some(AggOp::Rate) => { - // Validated by `promql::parse_query`: `rate()` never parses without one. let range_ms = parsed.range.expect("rate() always carries a range").as_millis() as i64; series_points .into_iter() @@ -2592,7 +2097,6 @@ fn evaluate_promql( .collect() } Some(AggOp::Gauge) => { - // 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)| { @@ -2602,8 +2106,6 @@ fn evaluate_promql( .collect() } Some(AggOp::Histogram) => { - // Validated by `promql::parse_query`: `histogram()` never parses - // without one. let bucket_width = parsed.bucket_width.expect("histogram() always carries a bucket width"); let last_ts = series_points @@ -2611,10 +2113,6 @@ fn evaluate_promql( .flat_map(|points| points.iter().map(|(ts, _)| *ts)) .max() .unwrap_or(0); - // Groups series by `by_labels` first (empty `by_labels` collapses to a single group, - // matching sum()/avg()'s behavior), then buckets each group's values independently into - // fixed-width buckets, labeled `le` (Prometheus's less-than-or-equal convention) alongside - // the group's own by_labels values. let mut groups: BTreeMap, Vec<&(i64, f64)>> = BTreeMap::new(); for (labels, points) in &series_points { let group_key = project_labels(labels, &parsed.by_labels); @@ -2739,7 +2237,6 @@ mod tests { const LOG_STREAM_FIXTURE: &str = include_str!("fixtures/log_stream_sample.txt"); - /// Runs both auto-declaration passes over every fixture line, same order as `EngineMessage::Ingest`. fn ingest_log_stream_fixture(index: &mut BTreeMap) { let mut header = None; for line in LOG_STREAM_FIXTURE.lines() { @@ -2750,16 +2247,10 @@ mod tests { #[test] fn ingesting_log_stream_fixture_does_not_blow_up_declared_param_count() { - // Ticket 050: unbounded auto-declaration on this real `log stream` dump produced 220 distinct - // params. The bound (MAX_AUTO_DECLARED_PARAMS + the header-derived log_stream columns) must - // keep the total in the tens, nowhere near "hundreds". let mut index = BTreeMap::new(); ingest_log_stream_fixture(&mut index); let declared = param_declarations_range(&index).count(); - // The header-derived columns (ticket 059) are stored as `auto_derived` too, so the shared - // `MAX_AUTO_DECLARED_PARAMS` ceiling covers all of them together (see - // `auto_declare_log_stream_params`'s doc comment). assert!( declared <= MAX_AUTO_DECLARED_PARAMS, "expected at most {MAX_AUTO_DECLARED_PARAMS} declared params, got {declared}" @@ -2773,8 +2264,6 @@ mod tests { ingest_log_stream_fixture(&mut index); let names: HashSet<&str> = param_declarations_range(&index).map(|d| d.0).collect(); - // The mDNSResponder lines' `` blobs would otherwise surface as params - // named after the tail of the base64 payload (see `looks_like_base64_padding_artifact`). assert!(!names.contains("rKTEtG7JmnThhoKPbZiVrA")); assert!(!names.contains("gEdA8N4g")); } @@ -2785,7 +2274,6 @@ mod tests { ingest_log_stream_fixture(&mut index); let declared: HashSet<&str> = param_declarations_range(&index).map(|d| d.0).collect(); - // Derived generically from the fixture's own header row (ticket 059), not a fixed set of three. for name in ["Timestamp", "Thread", "Type", "Activity", "PID", "TTL", "message"] { assert!(declared.contains(name), "expected {name} to be declared, got {declared:?}"); } @@ -2806,8 +2294,6 @@ mod tests { #[test] fn log_stream_columns_are_not_contaminated_by_non_columnar_lines() { - // Ticket 076: a wrapped multi-line dump's continuation line, and the header row itself, must - // not pick up garbage values for the fixed-offset columns once they're declared. let mut index = BTreeMap::new(); ingest_log_stream_fixture(&mut index); let compiled = compiled_param_declarations(&index); @@ -2825,11 +2311,6 @@ mod tests { assert!(!header_params.contains_key("Thread"), "header line got a Thread: {header_params:?}"); } - /// Ticket 074/076 end-to-end: sorting a real `log stream` capture by PID (a non-monotonic, - /// declared-via-`auto_declare_log_stream_params` column, exactly what the wishlist reported as - /// "does not work") must actually reorder the response — this is the scenario ticket 076's - /// contamination fix was expected to unblock, verified here via the real `EngineQuery::Logs` - /// sorted path (`handle_query`) rather than just the lower-level param-derivation helpers. #[test] fn logs_query_sorted_by_a_log_stream_column_actually_reorders_real_log_stream_output() { let mut index = BTreeMap::new(); @@ -2862,7 +2343,6 @@ mod tests { sorted_pids.sort_unstable(); assert_eq!(pids, sorted_pids, "entries were not actually reordered by PID"); - // Sanity check this isn't a no-op: the fixture's own ingest order is not already PID-sorted. let original_order: Vec = entries.iter().map(|e| e.sequence).collect(); let mut sorted_by_sequence = original_order.clone(); sorted_by_sequence.sort_unstable(); @@ -2880,7 +2360,7 @@ mod tests { } #[test] - #[ignore] // manual throughput measurement, not part of the regular suite + #[ignore] fn bench_ingest_fixture_throughput() { let repeats = 50; let mut index = BTreeMap::new(); @@ -2957,8 +2437,6 @@ mod tests { &mut index, DeclareParamRequest { name: "level".to_string(), - // No named capture group needed for JSON-field extraction, just something that - // matches the lines this param applies to. message_match_regex: r".".to_string(), capture_group: String::new(), json_field_path: Some("foo.bar".to_string()), @@ -2997,7 +2475,6 @@ mod tests { let params = derive_log_params(r#"{"foo":{"bar":42}}"#, &declarations); assert_eq!(params.get("bar"), Some(&MetricPrimitive::Number(Number::Int(42)))); - // A line that doesn't parse as JSON, or is missing the field, yields no param. assert!(derive_log_params("not json", &declarations).is_empty()); assert!(derive_log_params(r#"{"foo":{}}"#, &declarations).is_empty()); } @@ -3050,7 +2527,6 @@ mod tests { assert_eq!(approx_bytes_total, 3); } - // -- Ticket 073: ingest lag high watermark -- #[test] fn ingest_lag_watermark_reports_the_highest_sample_seen() { @@ -3072,13 +2548,10 @@ mod tests { assert_eq!(samples.len(), 5); assert_eq!(ingest_lag_high_watermark_ms(&samples, t0), 50); - // Smaller than every held sample -- nothing changes, so the max is unaffected. record_ingest_lag(&mut samples, t0, 5); assert_eq!(samples.len(), 5); assert_eq!(ingest_lag_high_watermark_ms(&samples, t0), 50); - // Bigger than the current minimum (10) -- replaces it, so the new min becomes 20 and the max - // stays 50 (this sample didn't beat it). record_ingest_lag(&mut samples, t0, 25); assert_eq!(samples.len(), 5); assert!(samples.iter().all(|(_, lag)| *lag != 10), "the old minimum (10) should have been replaced"); @@ -3092,16 +2565,12 @@ mod tests { record_ingest_lag(&mut samples, t0, 500); assert_eq!(ingest_lag_high_watermark_ms(&samples, t0), 500); - // Still within the 5s window just before it closes. let almost_expired = Timestamp::from_millis(INGEST_LAG_WATERMARK_WINDOW_MS - 1); assert_eq!(ingest_lag_high_watermark_ms(&samples, almost_expired), 500); - // Past the 5s window -- the stale spike no longer counts, reads back to the "no lag" default. let expired = Timestamp::from_millis(INGEST_LAG_WATERMARK_WINDOW_MS); assert_eq!(ingest_lag_high_watermark_ms(&samples, expired), 0); - // A fresh low-lag sample recorded at that point evicts the stale one outright rather than - // just being masked by the window filter. record_ingest_lag(&mut samples, expired, 5); assert_eq!(samples.len(), 1); assert_eq!(ingest_lag_high_watermark_ms(&samples, expired), 5); @@ -3139,8 +2608,6 @@ mod tests { clear_all_param_declarations(&mut index); assert_eq!(param_declarations_range(&index).count(), 0); - // No new lines ingested — re-extraction works purely off the `IngestedLog` entries already - // retained in the index (ticket 052's core acceptance criterion). let (reextracted, logs_considered) = reextract_params_from_retained_logs(&mut index); assert_eq!(logs_considered, 2); @@ -3156,9 +2623,6 @@ mod tests { #[test] fn reextract_params_from_retained_logs_only_covers_lines_still_in_the_index() { - // Simulates a line evicted (by `max_logs`) before the clear+re-extract runs: its key/value - // pair is simply gone from the index, same as `evict_excess_logs` leaves things, so - // `auto_declare_params` never sees it again. let mut index = BTreeMap::new(); insert_ingested_log(&mut index, 1, "x1=-4"); auto_declare_params(&mut index, "evicted_only_key=1"); @@ -3210,9 +2674,6 @@ mod tests { assert!(param_declarations_range(&index).next().is_none()); } - /// Declares a standalone param whose extractor points at `pattern`'s named `capture_group` — the - /// shape most of these tests need so a metric can then reference it by name via `label_params`/ - /// `value_param` instead of embedding its own extractor (ticket 037). fn declare_test_param( index: &mut BTreeMap, name: &str, @@ -3236,10 +2697,6 @@ mod tests { .unwrap(); } - /// Declares a standalone `elapsed_since_last` param (ticket 038) — computed as elapsed time - /// since the previous event sharing whatever label set it's grouped under, never text-extracted. - /// `message_match_regex`/`capture_group` are unused for this extractor but still need to satisfy - /// `declare_param`'s baseline "is this a valid regex" check, so an always-matching `"."` is fine. fn declare_elapsed_since_last_param(index: &mut BTreeMap, name: &str) { declare_param( index, @@ -3308,7 +2765,6 @@ mod tests { let pattern = r"request path=(?P\S+)"; declare_test_param(&mut index, "path", pattern, "path", MetricPrimitiveKind::String); - // "duration" is never declared as a standalone param. let result = declare_metric(&mut index, metric_request("requests", pattern, vec!["path", "duration"])); assert!(result.is_err()); assert!(result.unwrap_err().contains("duration")); @@ -3351,15 +2807,10 @@ mod tests { let mut index = BTreeMap::new(); declare_requests_metric(&mut index); assert!(remove_metric_declaration(&mut index, "requests")); - // Removing an already-gone (or never-declared) metric is a clean `false`, not a panic — - // this is what the API layer's 404 branch relies on. assert!(!remove_metric_declaration(&mut index, "requests")); assert!(!remove_metric_declaration(&mut index, "never-declared")); } - /// Ticket 051: deleting a metric purges its bucket history too, so a stale PromQL query - /// (e.g. a dashboard panel that still names the deleted metric) fails with a readable - /// "unknown metric" error instead of silently returning orphaned data. #[test] fn remove_metric_declaration_purges_bucket_history_so_promql_reports_unknown_metric() { let mut index = BTreeMap::new(); @@ -3376,10 +2827,6 @@ mod tests { assert!(err.contains("unknown metric"), "unexpected error message: {err}"); } - /// A metric that was ingested via `--metrics-url` scraping (`IngestMetricSample`) never gets a - /// `MetricDeclaration` at all, so the "unknown metric" check must key off having *either* a - /// declaration *or* recorded data — not declaration alone — or every scraped metric would be - /// rejected as unknown. #[test] fn undeclared_metric_with_bucket_data_is_still_queryable_via_promql() { let mut index = BTreeMap::new(); @@ -3421,7 +2868,6 @@ mod tests { let mut index = BTreeMap::new(); declare_metric(&mut index, metric_request("requests", r"request path=(?P\S+)", vec![])).unwrap(); - // "duration" is never declared as a standalone param. let result = update_metric( &mut index, "requests", @@ -3442,7 +2888,6 @@ mod tests { let result = update_metric(&mut index, "requests", metric_request("requests", pattern, vec!["path", "duration"])); assert!(result.is_ok()); - // Still one declaration named "requests", now with the updated regex/params. let declarations: Vec<_> = metric_declarations_range(&index).collect(); assert_eq!(declarations.len(), 1); assert_eq!(declarations[0].0, "requests"); @@ -3458,7 +2903,6 @@ mod tests { declare_metric(&mut index, metric_request("requests", r"request path=(?P\S+)", vec!["path"])).unwrap(); assert_eq!(bucket_values(&index, "requests", None).count(), 2); - // Editing to a narrower regex should leave exactly the buckets matching the *new* declaration — not the old ones layered underneath; the "path" param's own regex is unaffected, so it still extracts fine. update_metric(&mut index, "requests", metric_request("requests", r"request path=(?P/a)", vec!["path"])).unwrap(); assert_eq!(bucket_values(&index, "requests", None).count(), 1); } @@ -3469,7 +2913,6 @@ mod tests { declare_metric(&mut index, metric_request("requests", r"request path=(?P\S+)", vec![])).unwrap(); declare_metric(&mut index, metric_request("errors", r"error code=(?P\d+)", vec![])).unwrap(); - // Renaming onto an already-taken name is rejected. let result = update_metric( &mut index, "requests", @@ -3477,7 +2920,6 @@ mod tests { ); assert!(result.is_err()); - // Renaming onto a fresh name succeeds and drops the old declaration. update_metric( &mut index, "requests", @@ -3517,9 +2959,7 @@ mod tests { fn metric_param_extracts_from_json_field_path() { let mut index = BTreeMap::new(); ingest_log(&mut index, 0, r#"{"request":{"path":"/a"}}"#); - // Doesn't parse as JSON at all — should be skipped, not panic. ingest_log(&mut index, 1, "not json"); - // Parses as JSON but is missing the field — also skipped. ingest_log(&mut index, 2, r#"{"request":{}}"#); declare_param( @@ -3646,7 +3086,6 @@ 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. assert_eq!(series[0].points, vec![(0, 1.0), (2, 1.0)]); } @@ -3695,8 +3134,6 @@ mod tests { assert_eq!(series[1].points, vec![(0, 1.0)]); } - /// Declares a metric whose captured `status` param is `String`-kind and holds the `Value` role, - /// so every bucket entry for it is a `MetricPrimitive::String` rather than the default occurrence marker. fn declare_string_valued_metric(index: &mut BTreeMap) { declare_test_param( index, @@ -3824,11 +3261,6 @@ mod tests { #[test] fn sum_by_merges_staggered_raw_timestamps_across_label_groups_instead_of_going_sparse() { - // Regression test for ticket 042: `sum(requests) by (path)` groups together several - // underlying (path, host) series. Their raw occurrence timestamps are staggered (as real - // occurrence timestamps essentially always are), so a merge that only combines exact- - // timestamp matches (the old `merge_series_exact` behavior) would leave each point - // reflecting just one host instead of the combined total across hosts. let mut index = BTreeMap::new(); declare_requests_by_host_metric(&mut index); ingest_log(&mut index, 0, "request path=/a host=web1"); @@ -3840,8 +3272,6 @@ mod tests { assert_eq!(series.len(), 1); assert_eq!(series[0].labels.get("path").map(String::as_str), Some("/a")); - // The three staggered per-host occurrences merge into one combined-total point rather than - // three separate points each showing a lone host's count of 1. let total: f64 = series[0].points.iter().map(|(_, v)| v).sum(); assert_eq!(total, 3.0, "expected all three staggered occurrences to sum to 3.0, got {:?}", series[0].points); assert!( @@ -3863,15 +3293,11 @@ mod tests { assert_eq!(series.len(), 1); assert_eq!(series[0].labels.get("path").map(String::as_str), Some("/a")); - // Both occurrences (value 1.0 each) land in the same merged bucket, so the average stays 1.0 - // rather than reporting two separate points that each only saw one host. assert_eq!(series[0].points, vec![(0, 1.0)]); } #[test] fn sum_by_still_gives_a_single_label_group_its_own_line_at_full_resolution() { - // Existing single-label-group behavior (only one underlying series feeds each output group) - // must stay untouched by the multi-series windowing fix. let mut index = BTreeMap::new(); declare_requests_metric(&mut index); ingest_log(&mut index, 0, "request path=/a"); @@ -3917,10 +3343,6 @@ mod tests { assert_eq!(series[1].points, vec![(0, 0.5)]); } - /// Ticket 057: reproduces the reported `sum by (code) (rate(KeyPressed[5m]))` chart — a burst - /// of occurrences confined to the very start of a much wider queried range. Without a - /// synthetic trailing point, the series would just stop at the last occurrence's timestamp - /// (`1`), giving no indication the rate has decayed to 0 for the rest of `end_ms`. #[test] fn rate_series_gets_a_trailing_zero_point_once_occurrences_stop_early_in_a_wider_range() { let mut index = BTreeMap::new(); @@ -3939,8 +3361,6 @@ mod tests { ); } - /// Mirror of the trailing case: occurrences starting well after `start_ms` shouldn't make the - /// line appear to begin mid-range with no indication the rate was 0 beforehand. #[test] fn rate_series_gets_a_leading_zero_point_when_occurrences_start_late_in_a_wider_range() { let mut index = BTreeMap::new(); @@ -3953,9 +3373,6 @@ mod tests { assert_eq!(series[0].points, vec![(0, 0.0), (50_000, 0.5)]); } - /// Same shape as the two tests above, but through `sum(rate(...)) by (label)` — the exact - /// query form from the ticket's report — so each label's line independently reflects - /// rate-zero for the remainder of the range instead of just its own group stopping early. #[test] fn sum_by_rate_gets_boundary_points_per_group_across_a_wider_range() { let mut index = BTreeMap::new(); @@ -3973,8 +3390,6 @@ mod tests { assert_eq!(series[1].points, vec![(0, 0.5), (100_000, 0.0)]); } - /// A boundary point is only appended when the real series doesn't already reach that edge — - /// continuously-occurring metrics (the common case) must render exactly as before. #[test] fn rate_series_skips_boundary_points_already_covered_by_real_data() { let mut index = BTreeMap::new(); @@ -3988,17 +3403,11 @@ mod tests { assert_eq!(series[0].points, vec![(0, 0.5), (1000, 1.0)]); } - /// Ticket 040 acceptance criterion: a dashboard `rate()` chart and an alert `Threshold` - /// condition over the same metric/window agree on the computed rate for the same historical - /// data, since both now go through the single shared implementation in `crate::rate` rather - /// than each scanning/windowing independently. #[test] fn dashboard_rate_and_alert_threshold_agree_on_same_windowed_data() { let mut index = BTreeMap::new(); declare_requests_metric(&mut index); - // Five matching lines 500ms apart; `ingest_log`'s `sequence` doubles as the millisecond - // timestamp (see its definition above), so these are also the log timestamps. let lines: Vec<(u64, &str)> = vec![ (0, "request path=/a"), (500, "request path=/a"), @@ -4010,15 +3419,11 @@ mod tests { ingest_log(&mut index, *ms, text); } - // Dashboard side: rate() over a 2s window, read at the series' own last timestamp. let parsed = promql::parse_query("rate(requests[2s])").unwrap(); let series = evaluate_promql(&index, &parsed, None, None).unwrap(); assert_eq!(series.len(), 1); let (_, dashboard_rate) = *series[0].points.last().unwrap(); - // Alert side: a Threshold condition over the same query/window, replayed over the same - // lines in the same order. threshold=0/op=Ge always fires, so every fired message's - // reported match count is available to compare against the dashboard's rate. let rule = alerts::AlertRule { id: "r1".to_string(), name: "spike".to_string(), @@ -4094,9 +3499,6 @@ mod tests { assert!(result.is_ok()); } - // A metric may declare at most one value param — enforced at the type level now - // (`DeclareMetricRequest::value_param` is a single `Option`, not a list of - // role-tagged params), so there's no longer a runtime "too many value params" rejection to test. #[test] fn value_param_becomes_the_bucket_value_not_a_label() { @@ -4177,7 +3579,6 @@ mod tests { let mut series = evaluate_promql(&index, &parsed, None, None).unwrap(); series.sort_by(|a, b| a.labels.get("le").cmp(&b.labels.get("le"))); - // 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); @@ -4191,8 +3592,6 @@ mod tests { fn histogram_with_by_clause_buckets_each_label_group_separately() { let mut index = BTreeMap::new(); declare_durations_metric(&mut index); - // path=/a: 2,4 -> le=10 (count 2); 12 -> le=20 (count 1). - // path=/b: 21 -> le=30 (count 1). for (seq, path, duration) in [ (0, "/a", 2), (1, "/a", 4), @@ -4240,12 +3639,10 @@ 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 — 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())); assert_eq!(*sequence, 2); - // `ingest_log`'s helper timestamps by `sequence as millis`. assert_eq!(timestamp.as_millis(), 2); assert_eq!(*value, MetricPrimitive::Duration(2)); } @@ -4279,7 +3676,6 @@ mod tests { assert_eq!(series[0].points, vec![(10, 10.0)]); } - // 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, @@ -4489,7 +3885,6 @@ mod tests { } } - // -- FilterBar query integration -- fn logs_query(filter: Option<&str>) -> EngineQuery { EngineQuery::Logs(LogsParams { @@ -4503,7 +3898,6 @@ mod tests { }) } - /// Ticket 045: like `logs_query`, but with an alternate sort key and its missing-param handling. fn logs_query_sorted(sort_by: &str, sort_missing_fallback: Option) -> EngineQuery { EngineQuery::Logs(LogsParams { since_sequence: None, @@ -4525,9 +3919,6 @@ mod tests { }) } - /// Ticket 072: `ottyl_fmt` is filterable in the search bar (`compile_ottyl_fmt_comparison`) but - /// wasn't previously attached to `LogEntryDto::params` at all, so Log Preview's column picker - /// could offer it as a column yet every row showed blank. #[test] fn logs_query_response_includes_ottyl_fmt_per_entry() { let mut index = BTreeMap::new(); @@ -4599,7 +3990,6 @@ mod tests { } } - // -- Ticket 045: sort_by / sort_missing_fallback -- #[test] fn logs_query_sort_by_orders_lines_by_declared_param_value() { @@ -4611,8 +4001,6 @@ mod tests { "priority", MetricPrimitiveKind::Number, ); - // Ingested (and therefore default-ordered) as sequence 0, 1, 2, with priority values - // deliberately out of that order so a passing test can only mean the sort actually ran. ingest_log(&mut index, 0, "priority=3 boom"); ingest_log(&mut index, 1, "priority=1 boom"); ingest_log(&mut index, 2, "priority=2 boom"); @@ -4626,10 +4014,6 @@ mod tests { } } - /// Seeded fixture mixing lines that carry the sort param with lines that don't, per the ticket - /// 045 acceptance criteria: with the fallback toggle on, lines missing the param are ordered by - /// `ottyl_seq` relative to each other (and appended after the sorted lines); with it off, they're - /// excluded from the response entirely. #[test] fn logs_query_sort_by_missing_param_fallback_orders_by_sequence_and_appends() { let mut index = BTreeMap::new(); @@ -4640,16 +4024,14 @@ mod tests { "priority", MetricPrimitiveKind::Number, ); - ingest_log(&mut index, 0, "priority=3 boom"); // has param - ingest_log(&mut index, 1, "no param here"); // missing - ingest_log(&mut index, 2, "priority=1 boom"); // has param - ingest_log(&mut index, 3, "also missing"); // missing + ingest_log(&mut index, 0, "priority=3 boom"); + ingest_log(&mut index, 1, "no param here"); + ingest_log(&mut index, 2, "priority=1 boom"); + ingest_log(&mut index, 3, "also missing"); match handle_query(&index, logs_query_sorted("priority", Some(true)), 0, 0, &mut HashMap::new()) { EngineResponse::Logs(entries) => { let sequences: Vec = entries.iter().map(|e| e.sequence).collect(); - // Lines with the param sorted ascending by value (2 then 0), followed by the - // missing-param lines in ottyl_seq order (1 then 3). assert_eq!(sequences, vec![2, 0, 1, 3]); } _ => panic!("expected Logs response"), @@ -4691,9 +4073,6 @@ mod tests { } } - /// Explicit regression check for the ticket 045 acceptance criterion that default ordering - /// (no `sort_by`) is unchanged: same fixture as the fallback/drop tests above, but with - /// `sort_by: None` the response stays in plain ingest (`ottyl_seq`) order. #[test] fn logs_query_without_sort_by_keeps_default_ottyl_ts_order() { let mut index = BTreeMap::new(); @@ -4718,7 +4097,6 @@ mod tests { } } - // -- Ticket 060: sort helper index -- #[test] fn sort_helper_index_is_built_lazily_and_reused_across_queries() { @@ -4737,8 +4115,6 @@ mod tests { let mut sort_helper_indexes = HashMap::new(); assert!(!sort_helper_indexes.contains_key("priority")); - // First query has no helper index yet — must lazily build one rather than erroring or - // silently returning unsorted results. match handle_query(&index, logs_query_sorted("priority", None), 0, 0, &mut sort_helper_indexes) { EngineResponse::Logs(entries) => { let sequences: Vec = entries.iter().map(|e| e.sequence).collect(); @@ -4749,11 +4125,6 @@ mod tests { assert!(sort_helper_indexes.contains_key("priority")); assert_eq!(sort_helper_indexes["priority"].index.len(), 3); - // Ingest more lines directly into the already-built helper index (bypassing the full - // `EngineMessage::Ingest` actor loop, same as every other test in this file exercises - // `handle_query` directly) via the same incremental-update path `run()` calls on ingest, - // then requery — the new lines must appear correctly ordered without the helper index - // having been thrown away and rebuilt from scratch. ingest_log(&mut index, 3, "priority=0 boom"); update_sort_helper_indexes_on_ingest( &mut sort_helper_indexes, @@ -4775,8 +4146,6 @@ mod tests { match handle_query(&index, logs_query_sorted("priority", None), 0, 0, &mut sort_helper_indexes) { EngineResponse::Logs(entries) => { let sequences: Vec = entries.iter().map(|e| e.sequence).collect(); - // 3 (priority=0) sorts before the original three; 4 (missing the param) falls back - // to ottyl_seq order, appended after every line that has the param. assert_eq!(sequences, vec![3, 1, 2, 0, 4]); } _ => panic!("expected Logs response"), @@ -4785,15 +4154,12 @@ mod tests { #[test] fn sort_helper_index_incremental_update_is_a_noop_when_no_index_is_live() { - // Guards `update_sort_helper_indexes_on_ingest`'s early return: ingest must not pay to - // derive a param nobody has ever sorted by. let mut sort_helper_indexes: HashMap = HashMap::new(); let index = BTreeMap::new(); update_sort_helper_indexes_on_ingest(&mut sort_helper_indexes, &index, 0, Timestamp::from_millis(0), "hello"); assert!(sort_helper_indexes.is_empty()); } - // -- Ticket 061: purging stale sort helper indexes -- #[test] fn purge_stale_sort_helper_indexes_drops_only_entries_past_the_purge_duration() { @@ -4835,7 +4201,6 @@ mod tests { let _ = handle_query(&index, logs_query_sorted("priority", None), 0, 0, &mut sort_helper_indexes); assert!(sort_helper_indexes.contains_key("priority")); - // Simulate ticket 061's sweep having purged this entry for staleness. sort_helper_indexes.remove("priority"); match handle_query(&index, logs_query_sorted("priority", None), 0, 0, &mut sort_helper_indexes) { @@ -4851,7 +4216,6 @@ mod tests { #[test] fn logs_query_with_ottyl_fmt_comparison_filters_by_detected_format_for_mixed_format_lines() { let mut index = BTreeMap::new(); - // Seeded fixture mixing all three detectable formats on purpose (ticket 044). ingest_log(&mut index, 0, r#"{"level":"error","code":500}"#); ingest_log(&mut index, 1, r#"ts=1788648105773 msg="entity_moved" x1=-4"#); ingest_log(&mut index, 2, "just some plain prose here"); @@ -4917,7 +4281,6 @@ 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; 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"#)), @@ -4932,7 +4295,6 @@ mod tests { _ => panic!("expected Logs response"), } - // With `or`, sequences 0 (boom) and 2 (level=error) both match. match handle_query( &index, logs_query(Some(r#"regex("boom") or requests[level]==error"#)), diff --git a/src/filter_datetime.rs b/src/filter_datetime.rs index af07993..2c9c706 100644 --- a/src/filter_datetime.rs +++ b/src/filter_datetime.rs @@ -1,21 +1,12 @@ -//! 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. - -// `#![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}; 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 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), } @@ -37,8 +28,6 @@ impl std::fmt::Display for DateTimeParseError { impl std::error::Error for DateTimeParseError {} -/// 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() { @@ -60,8 +49,6 @@ pub fn parse_datetime(input: &str, now: Timestamp) -> Result Option> { let (sign, rest) = match input.strip_prefix('-') { Some(rest) => (-1i128, rest), @@ -71,7 +58,6 @@ fn try_relative(input: &str, now: Timestamp) -> Option Option Option> { - // 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; @@ -123,7 +107,6 @@ fn try_full_datetime(input: &str) -> Option value metric comparisons -// - json[field] value JSON field comparisons (line parsed as JSON) -// - ottyl_seq value reserved metric name (no [param]) -// - ottyl_ts value reserved metric name (no [param]) -// - ottyl_fmt value reserved metric name (no [param]) -// - `and` / `or` boolean combinators, with `and` binding tighter than `or` -// - parenthesized sub-expressions - WHITESPACE = _{ " " | "\t" | "\n" | "\r" } file = { SOI ~ or_expr ~ EOI } @@ -19,11 +7,6 @@ and_expr = { term ~ (and_kw ~ term)* } term = _{ regex_expr | comparison | "(" ~ or_expr ~ ")" } -// These must be atomic (`@`): a non-atomic rule would let pest's implicit -// whitespace insertion slip in between the keyword and its own boundary -// check below, which breaks the lookahead (it would see the first -// non-whitespace character *after* skipping the separating space, instead -// of the character immediately following the keyword). and_kw = @{ "and" ~ !(ASCII_ALPHANUMERIC | "_") } or_kw = @{ "or" ~ !(ASCII_ALPHANUMERIC | "_") } @@ -35,8 +18,6 @@ escaped_char = @{ "\\" ~ ANY } comparison = _{ json_comparison | metric_comparison } -// `json[field] value` must be tried before `metric_comparison` since "json" is -// otherwise a syntactically valid (if never declared) metric name. json_comparison = { "json" ~ "[" ~ param ~ "]" ~ op ~ value } metric_comparison = { metric ~ ("[" ~ param ~ "]")? ~ op ~ value } diff --git a/src/filter_parser.rs b/src/filter_parser.rs index 430591f..9c7cf7f 100644 --- a/src/filter_parser.rs +++ b/src/filter_parser.rs @@ -1,5 +1,3 @@ -//! 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; @@ -11,20 +9,15 @@ use serde::{Deserialize, Serialize}; #[grammar = "filter_parser.pest"] struct FilterGrammar; -/// The parsed AST for a FilterBar query. #[derive(Debug, Clone, PartialEq)] pub enum FilterExpr { - /// `regex("...")` — raw-text regex search over log lines. Regex(String), - /// `metric[param] value` — a single metric comparison. Comparison { metric: String, param: Option, op: CompareOp, value: FilterValue, }, - /// `json[field] value` — a comparison against a field of the line's parsed JSON. - /// Lines that aren't valid JSON (or lack the field) simply don't match. JsonComparison { field: String, op: CompareOp, @@ -34,8 +27,6 @@ pub enum FilterExpr { Or(Box, Box), } -/// 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 { @@ -47,15 +38,12 @@ pub enum CompareOp { Ne, } -/// 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), Raw(String), } -/// A located, human-readable parse error. #[derive(Debug, Clone, PartialEq)] pub struct FilterParseError { message: String, @@ -79,12 +67,10 @@ impl std::error::Error for FilterParseError {} impl From> for FilterParseError { fn from(err: pest::error::Error) -> Self { - // 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}")) } } -/// Parse a FilterBar query string into a [`FilterExpr`] AST. pub fn parse(input: &str) -> Result { let mut pairs = FilterGrammar::parse(Rule::file, input)?; @@ -102,7 +88,6 @@ 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 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); @@ -119,7 +104,6 @@ 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 `build_or_expr` for `and_kw`. let mut terms = pair .into_inner() .filter(|p| p.as_rule() != Rule::and_kw); @@ -237,7 +221,6 @@ fn parse_value(s: &str) -> FilterValue { } } -/// 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(); diff --git a/src/index_key.rs b/src/index_key.rs index 1490b6e..6a228cf 100644 --- a/src/index_key.rs +++ b/src/index_key.rs @@ -3,37 +3,30 @@ use crate::timestamp::Timestamp; use serde::Serialize; use std::collections::BTreeMap; -/// 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 { - /// Marker for the start of the whole range. Never inserted, only navigated to. Start, IngestedLog { sequence: u64, timestamp: Timestamp, }, - /// Marker between the log range and the param range. LogParamSplit, ParamDeclaration { name: String, }, - /// Marker between the param range and the metric declaration range. ParamMetricSplit, MetricDeclaration { name: String, }, - /// Marker between metric declarations and their buckets. MetricBucketSplit, Bucket { metric_name: String, labels: BTreeMap, }, - /// 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, }, - /// Marker for the end of the whole range. Nothing sorts after this. End, } diff --git a/src/ingest.rs b/src/ingest.rs index 5da5d26..ad31fc8 100644 --- a/src/ingest.rs +++ b/src/ingest.rs @@ -7,8 +7,6 @@ use crate::timestamp::Timestamp; use tokio::fs::File; use tokio::io::{AsyncBufReadExt, BufReader}; -/// Reads stdin line by line, sequences and timestamps each line, and forwards it -/// to the engine task. pub async fn run(engine: EngineHandle) { let stdin = tokio::io::stdin(); let mut lines = BufReader::new(stdin).lines(); @@ -28,7 +26,6 @@ pub async fn run(engine: EngineHandle) { } } -/// 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))] ()); @@ -51,9 +48,6 @@ async fn open(path: &Path) -> Option<(File, FileId)> { Some((file, file_id(&meta))) } -/// 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: EngineHandle, path: PathBuf) { let mut sequence: u64 = 0; @@ -73,7 +67,6 @@ pub async fn run_file(engine: EngineHandle, path: PathBuf) { let mut chunk = String::new(); match reader.read_line(&mut chunk).await { Ok(0) => { - // 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); @@ -82,9 +75,7 @@ pub async fn run_file(engine: EngineHandle, path: PathBuf) { } id = new_id; } - Err(_) => { - // Path gone (mid-recreation); wait for it to reappear and reopen fresh. - } + Err(_) => {} } tokio::time::sleep(POLL_INTERVAL).await; } @@ -108,7 +99,6 @@ pub async fn run_file(engine: EngineHandle, path: PathBuf) { return; } } - // else: partial line at EOF (write in progress) — keep buffering. } Err(_) => continue 'reopen, } @@ -116,9 +106,6 @@ pub async fn run_file(engine: EngineHandle, path: PathBuf) { } } -/// Polls `url` on a fixed interval for a Prometheus/OpenMetrics-format response, pushing every parsed -/// sample straight into the engine. Errors (unreachable endpoint, non-2xx response) are only logged — -/// polling keeps retrying on the next tick rather than exiting. pub async fn run_metrics_poll(engine: EngineHandle, url: String) { let client = reqwest::Client::new(); loop { @@ -143,10 +130,6 @@ pub async fn run_metrics_poll(engine: EngineHandle, url: String) { } } -/// Parses the Prometheus text exposition / OpenMetrics format: one sample per non-comment, -/// non-empty line, as `metric_name{label="value",...} value [timestamp_ms]` (labels and the -/// timestamp are both optional). A line that doesn't parse is skipped rather than aborting the -/// whole scrape. fn parse_openmetrics(body: &str, now: Timestamp) -> Vec { body.lines().filter_map(|line| parse_openmetrics_line(line, now)).collect() } @@ -180,8 +163,6 @@ fn parse_openmetrics_line(line: &str, now: Timestamp) -> Option { Some(MetricSample { metric_name, labels, value, timestamp }) } -/// Splits on commas without handling an escaped comma inside a quoted label value — fine for the -/// common case this targets, not a full OpenMetrics-grammar parser. fn parse_openmetrics_labels(raw: &str) -> BTreeMap { raw.split(',') .filter_map(|pair| pair.split_once('=')) diff --git a/src/logfmt.rs b/src/logfmt.rs index 4361147..57bf137 100644 --- a/src/logfmt.rs +++ b/src/logfmt.rs @@ -1,8 +1,3 @@ -//! Fast, tolerant `key=value`/`key="value"` tokenizer (logfmt-ish), used by -//! `crate::engine::auto_declare_params` to discover param names as lines are ingested. Scans -//! anywhere in the line rather than requiring the whole line to be logfmt, so it works on lines -//! with an arbitrary non-logfmt prefix/suffix (e.g. `ts=... msg="..." x1=-4 y1=34`). - use nom::branch::alt; use nom::bytes::complete::{escaped, tag, take_until, take_while1}; use nom::character::complete::{char, none_of, one_of}; @@ -30,20 +25,10 @@ fn bareword_value(input: &str) -> IResult<&str, &str> { take_while1(|c: char| !c.is_whitespace())(input) } -/// A bareword value starting with a bare `=` means the raw text actually had `key==...`/`key===...` — -/// never a real logfmt pair. This is exactly what `pair` would otherwise produce when it lands -/// mid-way through a base64/hash blob whose padding (`=`/`==`) happens to sit right after a run of -/// alnum chars that `key()` greedily consumed — e.g. the `huig_dump.txt` fixture's -/// `,`: `key()` eats `rKTEtG7JmnThhoKPbZiVrA`, the first `=` is -/// consumed as the pair's `=`, leaving a bareword value of `='>,` (ticket 050). Rejected here rather -/// than post-filtered by a caller so `extract_pairs` never reports the false positive to anyone. fn looks_like_base64_padding_artifact(value: &str) -> bool { value.starts_with('=') } -/// One `key=value` pair found on a line: the key, the raw captured value (unescaped as-is, quotes -/// stripped but any inner `\"`/`\\` left literal since callers only need it for numeric-kind -/// inference), and whether the value was quoted. fn pair(input: &str) -> IResult<&str, (&str, &str, bool)> { let (input, k) = key(input)?; let (input, _) = char('=')(input)?; @@ -58,9 +43,6 @@ fn pair(input: &str) -> IResult<&str, (&str, &str, bool)> { } } -/// Finds every `key=value`/`key="value"` pair anywhere in `line`. A position that doesn't start a -/// pair is skipped one char at a time, so this never fails outright — a line with no pairs at all -/// (plain prose, a JSON blob) just returns an empty vec. pub fn extract_pairs(line: &str) -> Vec<(&str, &str, bool)> { let mut results = Vec::new(); let mut rest = line; @@ -80,25 +62,6 @@ pub fn extract_pairs(line: &str) -> Vec<(&str, &str, bool)> { results } -/// Fixed-column shape of macOS `log stream` output (ticket 050/054): a line starts with -/// `Timestamp Thread Type Activity PID TTL` (date, time+tz, a hex thread id, a bareword type, a hex -/// activity id, a decimal pid, a decimal ttl) followed by a free-text `process: message` tail — see -/// `huig_dump.txt`'s header row and body. Anchored at the very start of the line and requires the -/// hex/decimal shape of each column, so it won't accidentally fire on ordinary logfmt/JSON/prose that -/// merely resembles it in one column. Checked ahead of JSON/logfmt in `detect_format` since it's the -/// most specific of the three (an all-columns-plus-hex-ids match is a much stronger signal than an -/// incidental `key=value` substring), and is deliberately a *format label* rather than a dedicated -/// `ottyl_fmt` value change to JSON/logfmt detection — a `log stream` line's message tail can still -/// separately contain real `key=value` pairs (see the WiFi/radio stat lines in the fixture), which -/// `crate::engine::auto_declare_params` keeps discovering via `extract_pairs` regardless of this -/// line's `ottyl_fmt` label. -/// `crate::data_value::ParamDeclaration::log_stream_gated` params (the columns/`message` -/// `auto_declare_log_stream_params` declares) require this to hold before their own fixed-offset -/// `message_match_regex` is trusted at all (ticket 076) — that regex is pure character-offset -/// arithmetic with no content anchor of its own, so it happily "matches" (and extracts garbage -/// from) any unrelated line at least that long, e.g. a wrapped multi-line dump's continuation line -/// or the header line itself. This can't be baked into `message_match_regex` as a leading lookahead -/// instead: the `regex` crate deliberately doesn't support look-around at all. static LOG_STREAM_RE: LazyLock = LazyLock::new(|| { Regex::new( r"^\d{4}-\d{2}-\d{2}\s+\S+\s+0x[0-9a-fA-F]+\s+\S+\s+0x[0-9a-fA-F]+\s+\d+\s+\d+\s+[^\s:]+:\s", @@ -110,21 +73,6 @@ pub fn is_log_stream_line(line: &str) -> bool { LOG_STREAM_RE.is_match(line) } -/// Regex fragment for one column separator under the generic whitespace-columnar rule ticket 059 -/// asks for: a run of 2+ whitespace chars, or a lone tab, separates columns; a single plain space does -/// not (so a value that legitimately contains one -- e.g. `log stream`'s `date time+tz` timestamp -- -/// stays intact within its own column). `is_separator_run` is the same rule applied at split time; -/// this is that rule re-expressed as a regex fragment for the trailing message field's -/// `message_match_regex`, which re-derives its value the same way whenever a caller queries by it -/// later (declarations don't cache extracted values -- see `crate::engine::extract_param_raw`). -/// Same idea as `COLUMN_VALUE_PATTERN`, but bounded to a known column `width` (in chars) -- needed for -/// every column except the last, since two adjacent `log stream` values can legitimately be separated -/// by only a single space when the first one nearly fills its column's on-screen width (e.g. the -/// `Timestamp` column: its own `date time+tz` value already contains one internal space, so a -/// content-only pattern can't tell that boundary apart from the one-space gap before the next column -/// starts -- seed this from `LogStreamColumn::start` positions instead). Greedily matches up to -/// `width` chars while requiring the match to end on a non-whitespace char, which is what excludes the -/// column's own trailing padding spaces from the capture. fn bounded_column_value_pattern(width: usize) -> String { if width <= 1 { r"\S".to_string() @@ -139,12 +87,6 @@ fn is_separator_run(run: &str) -> bool { static WHITESPACE_RUN: LazyLock = LazyLock::new(|| Regex::new(r"[ \t]+").expect("static regex must compile")); -/// Splits `line` at whitespace runs that qualify as a column separator (`is_separator_run`), returning -/// each piece alongside its byte offset in `line`; a lone plain space never splits. Consumes at most -/// `max_columns.map(|n| n - 1)` separators -- the final piece returned is the untouched remainder of -/// the line, including any further whitespace runs inside it. That's what lets a trailing unlabelled -/// `log stream` message tail (whose own internal spacing can be arbitrary) come back as one field -/// instead of being chopped up by it. fn split_columns_with_offsets(line: &str, max_columns: Option) -> Vec<(usize, &str)> { let mut columns = Vec::new(); let mut start = 0usize; @@ -168,41 +110,21 @@ fn split_columns(line: &str, max_columns: Option) -> Vec<&str> { split_columns_with_offsets(line, max_columns).into_iter().map(|(_, s)| s).collect() } -/// A column-name identifier a generated `message_match_regex`'s named capture group can use as-is -/// (the regex crate requires `[A-Za-z_][0-9A-Za-z_]*`). static COLUMN_NAME: LazyLock = LazyLock::new(|| Regex::new(r"^[A-Za-z][A-Za-z0-9_]*$").expect("static regex must compile")); -/// One column named by a detected `log stream` header (ticket 059). `start` is that column's position -/// in `char`s (not bytes -- see `char_slice`) within the header line -- meaningful only when -/// `LogStreamHeader::tab_delimited` is `false` (see that field's doc comment). -/// `Clone` so the engine's ingest path (`crate::engine::run`) can snapshot the current header value -/// by-value before handing it to a left-right op closure — the closure must be replayable against -/// two separate `EngineSnapshot` copies without touching the run loop's own `log_stream_header` var. #[derive(Clone)] pub struct LogStreamColumn { pub name: String, start: usize, } -/// A detected `log stream` header: its named columns, plus whether they were tab- or space-separated. -/// `log stream`'s real output right-pads each header name to a fixed on-screen width using runs of -/// 2+ spaces, and every data line's values land in that same fixed alignment regardless of how much -/// of that width they individually fill -- so for that shape, a data line's column boundaries are -/// best found at the header's own fixed character positions, not by re-splitting the data line's own -/// whitespace (see `split_log_stream_row`'s doc comment for why that alone isn't reliable there). -/// A tab-separated header carries no such alignment guarantee (values aren't padded to a shared -/// width), but a lone tab is itself an unambiguous separator, so that case instead re-splits each data -/// line directly. #[derive(Clone)] pub struct LogStreamHeader { pub columns: Vec, tab_delimited: bool, } -/// Byte-indexed `str` slicing doesn't work directly with `LogStreamColumn::start` (a *char* offset), -/// since `log stream` output is expected to stay ASCII in practice but callers shouldn't panic if a -/// message contains multi-byte chars before the slice point. `end` of `None` slices to end of string. fn char_slice(s: &str, start: usize, end: Option) -> &str { if end.is_some_and(|end| end <= start) { return ""; @@ -216,13 +138,6 @@ fn char_slice(s: &str, start: usize, end: Option) -> &str { &s[byte_start..byte_end.max(byte_start)] } -/// Recognizes a `log stream`-style header row generically (ticket 059), instead of assuming the fixed -/// `Timestamp Thread Type Activity PID TTL` names: a short run of bareword column names separated by -/// qualifying whitespace runs (`split_columns_with_offsets`) that is, critically, *not itself* a -/// `log stream` data line (`is_log_stream_line`) -- so a header is never mistaken for the data line -/// immediately following it. `crate::engine::auto_declare_log_stream_params` caches the result across -/// ingested lines, since `log stream` prints its header once, before any data line, and this only ever -/// sees one line at a time. pub fn detect_log_stream_header(line: &str) -> Option { if is_log_stream_line(line) { return None; @@ -244,21 +159,6 @@ pub fn detect_log_stream_header(line: &str) -> Option { Some(LogStreamHeader { columns, tab_delimited }) } -/// Splits a `log stream` data line into `header`'s named columns plus, when present, one trailing -/// unlabelled tail -- `crate::engine::auto_declare_log_stream_params` maps the first -/// `header.columns.len()` pieces onto the header's names and the optional last piece onto the -/// `message` param. -/// -/// For a space-padded fixed-width header, every column but the last is sliced at the header's own -/// fixed character positions (`LogStreamColumn::start`) rather than by re-splitting the data line's -/// own whitespace, since two adjacent values can legitimately be joined by only a single space when -/// the first nearly fills its column's width (e.g. `log stream`'s `Timestamp` column, whose value -/// already contains one internal space). Only the last named column's end is whitespace-run-detected -/// (`split_columns`), since that's what distinguishes its own value from the free-text message tail -/// `log stream` appends after it. -/// -/// For a tab-delimited header, there's no shared on-screen width to align against, but a lone tab is -/// itself unambiguous, so this instead re-splits the data line directly on qualifying separators. pub fn split_log_stream_row<'a>(line: &'a str, header: &LogStreamHeader) -> Vec<&'a str> { if header.tab_delimited { return split_columns(line, Some(header.columns.len() + 1)); @@ -280,17 +180,6 @@ pub fn split_log_stream_row<'a>(line: &'a str, header: &LogStreamHeader) -> Vec< result } -/// Builds the `message_match_regex` for the `index`-th (0-based) of `header`'s named columns. -/// -/// Fixed-width case: skips exactly `header.columns[index].start` chars, then captures this column's -/// own value bounded to its known width (`bounded_column_value_pattern`) -- unbounded for the last -/// column, which has no next column to bound against. -/// -/// Tab-delimited case: skips `index` preceding columns' own value+separator pairs instead, since -/// there's no shared fixed width to anchor a character-position skip against. -/// -/// Anchored at the start of the line since a `log stream` line never carries this fixed-per-run -/// column prefix anywhere but the very start. pub fn log_stream_column_pattern(header: &LogStreamHeader, index: usize, capture_group: &str) -> String { if header.tab_delimited { let mut pattern = String::from("^"); @@ -309,9 +198,6 @@ pub fn log_stream_column_pattern(header: &LogStreamHeader, index: usize, capture format!("^.{{{start}}}(?P<{capture_group}>{value_pattern})") } -/// Builds the `message_match_regex` for the trailing unlabelled tail after `header`'s last named -/// column: everything from the end of that column's own separator to end of line, verbatim -- its own -/// internal whitespace, however irregular, is never treated as a further separator. pub fn log_stream_message_pattern(header: &LogStreamHeader, capture_group: &str) -> String { if header.tab_delimited { let mut pattern = String::from("^"); @@ -326,14 +212,6 @@ pub fn log_stream_message_pattern(header: &LogStreamHeader, capture_group: &str) format!("^.{{{start}}}{COLUMN_VALUE_PATTERN}{COLUMN_SEPARATOR_PATTERN}(?P<{capture_group}>.*)$") } -/// Go `log/slog` `NewJSONHandler` output (the `go-log` label): a single-line JSON object whose first -/// three fields are always `time`, `level`, `msg` in that fixed order (slog's `JSONHandler` writes the -/// built-in attrs before any caller-supplied ones), followed by zero or more arbitrary attrs and a -/// closing `}`. Parsed with nom rather than a full JSON decode since only that fixed prefix shape -/// needs verifying; the variable tail is deliberately not parsed field-by-field the way -/// `crate::engine::auto_declare_params` will separately pick up JSON leaf fields from the line. -/// Checked ahead of the generic JSON object check in `detect_format` since it's the more specific of -/// the two (see that function's doc comment for the general ordering rationale). fn go_log_prefix(input: &str) -> IResult<&str, (&str, &str, &str)> { let (input, _) = tag("{\"time\":\"")(input)?; let (input, time) = take_until("\"")(input)?; @@ -352,14 +230,6 @@ pub fn is_go_log_line(text: &str) -> bool { } } -/// Python `logging` module output in the shape `"%(asctime)s %(levelname)s %(name)s %(message)s"` -/// (the `py-log` label): a `YYYY-MM-DD HH:MM:SS,mmm` timestamp, an uppercase level word, a logger -/// name, then a message tail that (for `delete-py-log`, and any caller using `logger.info("event=%s -/// %s=%s", ...)`-style calls) is itself logfmt `key=value` pairs. Only the fixed prefix is parsed with -/// nom; the tail is handed to `extract_pairs` both to confirm the shape (a py-log line with no pairs -/// at all wouldn't be distinguishable from plain prose after this prefix) and because that's the same -/// extraction `crate::engine::auto_declare_params` needs anyway. Checked ahead of the generic logfmt -/// check in `detect_format` since it's the more specific of the two. fn py_log_prefix(input: &str) -> IResult<&str, (&str, &str, &str, &str)> { let (input, date) = take_while1(|c: char| c.is_ascii_digit() || c == '-')(input)?; let (input, _) = char(' ')(input)?; @@ -380,16 +250,6 @@ pub fn is_py_log_line(text: &str) -> bool { } } -/// The value of the `ottyl_fmt` reserved param (`crate::engine::compile_filter`/`eval_alert_expr`): -/// which format a line was recognized as when parsed. Checked most-specific-first: `log stream`'s -/// fixed-column shape (see `LOG_STREAM_RE`'s doc comment for why it's the most specific), then the two -/// named-tool shapes `go-log` (a `time`/`level`/`msg`-prefixed JSON object) and `py-log` (a -/// timestamp/level/logger prefix over logfmt pairs) — each is a strict subset of the generic JSON/logfmt -/// case checked right after it, so they'd otherwise never be reached. JSON is checked next since a JSON -/// object can incidentally contain `key=value`-shaped substrings that `extract_pairs` would otherwise -/// pick up; only a top-level object counts (arrays/scalars aren't what `crate::engine::eval_json_comparison` -/// treats as JSON either). Anything else falls back to logfmt-style pair extraction, and a line with -/// none of the above is `"none"`. pub fn detect_format(text: &str) -> &'static str { if is_log_stream_line(text) { "log_stream" @@ -471,15 +331,12 @@ mod tests { #[test] fn a_bare_json_array_or_scalar_is_not_treated_as_json() { - // Not an object, so falls through to logfmt/none like `eval_json_comparison` would treat it. assert_eq!(detect_format("[1,2,3]"), "none"); assert_eq!(detect_format("42"), "none"); } #[test] fn rejects_base64_padding_fragment_as_a_pair() { - // The exact false-positive shape from ticket 050's fixture: `key()` greedily consumes the - // tail of the base64 blob as if it were a key, landing right before the `==` padding. let pairs = extract_pairs( "qname: , qtype: A, client pid: 12260 (firefox)", ); @@ -541,7 +398,6 @@ mod tests { assert_eq!(row[3], "0x0"); assert_eq!(row[4], "606"); assert_eq!(row[5], "0"); - // The message tail is never chopped up by its own internal single/double spacing. assert_eq!(row[6], "WindowServer: (HSTouchHIDService) something happened"); } @@ -551,8 +407,6 @@ mod tests { assert_eq!(column_names(&header), vec!["Timestamp", "Thread", "Type"]); assert!(header.tab_delimited); - // Two tabs in a row still qualify as a single separator (like a run of 2+ spaces), so this - // still cleanly separates Timestamp/Thread/Type despite the doubled tab before "Default". let spaced = split_log_stream_row("2026-09-06 01:48:54\t0x4dfed2\t\tDefault trailing message", &header); let tabbed = split_log_stream_row("2026-09-06 01:48:54\t0x4dfed2\tDefault trailing message", &header); assert_eq!(spaced[0], "2026-09-06 01:48:54"); @@ -581,8 +435,6 @@ mod tests { "WindowServer: (HSTouchHIDService) something happened" ); - // Also exercise the Timestamp column specifically -- its own value contains one internal - // space, so a naive content-only pattern would previously over-consume into the next column. let timestamp_re = Regex::new(&log_stream_column_pattern(&header, 0, "Timestamp")).unwrap(); assert_eq!( timestamp_re.captures(line).unwrap().name("Timestamp").unwrap().as_str(), @@ -592,10 +444,6 @@ mod tests { #[test] fn is_log_stream_line_rejects_non_columnar_lines_that_a_fixed_offset_column_regex_would_still_match() { - // Ticket 076: this is exactly why `ParamDeclaration::log_stream_gated` params must check - // `is_log_stream_line` before trusting their own offset-based capture at all -- the raw - // column regex has no content anchor and matches plenty on lines that clearly aren't - // columnar `log stream` output. let header_line = LOG_STREAM_FIXTURE.lines().next().unwrap(); let header = detect_log_stream_header(header_line).unwrap(); let timestamp_re = Regex::new(&log_stream_column_pattern(&header, 0, "Timestamp")).unwrap(); @@ -614,7 +462,6 @@ mod tests { .find(|line| line.contains("txRTSFrm=")) .expect("fixture must contain the WiFi stat dump line"); assert_eq!(detect_format(line), "log_stream"); - // The embedded key=value pairs are still there for auto_declare_params to see. assert!(!extract_pairs(line).is_empty()); } diff --git a/src/main.rs b/src/main.rs index 3d5659a..1f0fedd 100644 --- a/src/main.rs +++ b/src/main.rs @@ -26,8 +26,6 @@ use tokio::net::TcpListener; use settings::Settings; -/// The resolved default settings path, rendered into `--help` so `--settings-path`'s help text -/// names a real, concrete location instead of only describing where to look for one. static SETTINGS_PATH_HELP: LazyLock = LazyLock::new(|| { format!( "Path to the settings file. Defaults to {}.", @@ -37,50 +35,35 @@ static SETTINGS_PATH_HELP: LazyLock = LazyLock::new(|| { ) }); -/// Observability dashboard from terminal output. #[derive(Parser, Debug)] struct Args { #[command(subcommand)] command: Option, - /// Port to serve the API and webapp on. Defaults to 0, letting the OS assign - /// an ephemeral free port. #[arg(long, default_value_t = 0)] port: u16, #[arg(long, help = SETTINGS_PATH_HELP.as_str())] settings_path: Option, - /// File to tail for log lines, instead of reading from stdin. Behaves like `tail -f`. #[arg(long)] file: Option, - /// URL of a Prometheus/OpenMetrics-compatible endpoint to poll for metrics, in addition to log ingestion. #[arg(long)] metrics_url: Option, - /// Name to register this session under, so `ottyl mcp --session ` or `ottyl sessions` - /// can find it later. Defaults to a random human-readable name (e.g. "comic-gazelle"). - /// Starting a second instance under the same name as one still running overwrites its - /// registration (the older process keeps running, just without a discoverable session entry). #[arg(long)] name: Option, } #[derive(Subcommand, Debug)] enum Command { - /// Start an MCP (Model Context Protocol) server over stdio, proxying tool calls to a running - /// ottyl instance's HTTP API. Register the printed command with an MCP client (e.g. Claude - /// Code's `.mcp.json`). Mcp { - /// Name of a running ottyl session to connect to (see `ottyl sessions`). #[arg(long)] session: Option, - /// Port of a running ottyl instance to connect to, instead of resolving `--session` by name. #[arg(long)] port: Option, }, - /// List known ottyl sessions and the ports they're serving on. Sessions, } @@ -113,8 +96,6 @@ async fn main() { let _ = engine_tx.tx.send(engine::EngineMessage::SetMaxLogs(max_logs)).await; } - // 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, active_profile) = { let mut settings_guard = settings.lock().unwrap(); @@ -184,8 +165,6 @@ async fn main() { .expect("server error"); } -/// Resolves `--session`/`--port` into a target base URL and runs the MCP server over stdio until -/// the client disconnects. async fn run_mcp(settings_path: &std::path::Path, session: Option, port: Option) { use rmcp::ServiceExt; diff --git a/src/mcp_server.rs b/src/mcp_server.rs index 065921c..d8a6b80 100644 --- a/src/mcp_server.rs +++ b/src/mcp_server.rs @@ -1,13 +1,3 @@ -//! MCP (Model Context Protocol) server for agents (ticket 067): a short-lived process, started -//! via `ottyl mcp --session ` (see `crate::sessions`), that speaks MCP over stdio and -//! proxies each tool call to a *running* ottyl instance's existing HTTP API. Every tool here is a -//! thin translation onto `api.rs` routes/DTOs — no query logic is duplicated outside `engine.rs`. -//! -//! Uses the official `rmcp` SDK (https://github.com/modelcontextprotocol/rust-sdk) over the -//! `transport-io` (stdio) transport rather than hand-rolling the JSON-RPC/MCP handshake, since a -//! maintained implementation of the protocol's framing and lifecycle is worth the extra -//! dependency. - use std::collections::BTreeMap; use rmcp::handler::server::router::tool::ToolRouter; @@ -21,13 +11,10 @@ use serde_json::Value; #[derive(Debug, Deserialize, schemars::JsonSchema)] pub struct QueryLogsParams { - /// FilterBar query string, e.g. `regex("error") and ottyl_seq<5`. Omitted matches every log. #[serde(default)] pub filter: Option, - /// If true, return only how many logs matched instead of the log lines themselves. #[serde(default)] pub count_only: bool, - /// Maximum number of log lines to return. Ignored when `count_only` is set. #[serde(default)] pub limit: Option, } @@ -35,15 +22,11 @@ pub struct QueryLogsParams { #[derive(Debug, Serialize, Deserialize, schemars::JsonSchema)] pub struct DeclareParamParams { pub name: String, - /// Regex matched against each log line; `capture_group` extracts this param's value from it. pub message_match_regex: String, pub capture_group: String, - /// Dotted path into a JSON-structured log line to extract from instead of `capture_group`. #[serde(default)] pub json_field_path: Option, - /// One of: "string", "number", "timestamp", "duration", "boolean". pub kind: String, - /// Extract the elapsed time since this param was last seen, instead of parsing captured text. #[serde(default)] pub elapsed_since_last: bool, } @@ -52,23 +35,18 @@ pub struct DeclareParamParams { pub struct DeclareMetricParams { pub name: String, pub message_match_regex: String, - /// Names of already-declared params whose values become this metric's bucket labels. #[serde(default)] pub label_params: Vec, - /// Name of an already-declared param whose value becomes this metric's per-event bucket value. #[serde(default)] pub value_param: Option, - /// Fixed labels layered on top of `label_params` for matching lines. #[serde(default)] pub matched_labels: Option>, - /// Fixed labels recorded once for a non-matching line, instead of it being invisible to this metric. #[serde(default)] pub unmatched_labels: Option>, } #[derive(Debug, Deserialize, schemars::JsonSchema)] pub struct CreateDashboardParams { - /// Profile to create the dashboard in. Defaults to the currently active profile. #[serde(default)] pub profile: Option, pub name: String, @@ -76,17 +54,13 @@ pub struct CreateDashboardParams { #[derive(Debug, Deserialize, schemars::JsonSchema)] pub struct AddPanelParams { - /// Profile the dashboard lives in. Defaults to the currently active profile. #[serde(default)] pub profile: Option, pub dashboard_id: String, - /// Id of an existing panel to update in place; omit to add a new panel. #[serde(default)] pub panel_id: Option, pub title: String, - /// PromQL-style query backing this panel (see `crate::promql`). pub promql: String, - /// One of: "bar", "line", "pie". Defaults to "line". #[serde(default = "default_chart_type")] pub chart_type: String, } @@ -97,7 +71,6 @@ fn default_chart_type() -> String { #[derive(Debug, Deserialize, schemars::JsonSchema)] pub struct GetDashboardParams { - /// Profile the dashboard lives in. Defaults to the currently active profile. #[serde(default)] pub profile: Option, pub dashboard_id: String, @@ -107,9 +80,6 @@ pub struct GetDashboardParams { pub struct OttylMcp { client: reqwest::Client, base_url: String, - // Constructed fresh per call by the `#[tool_handler]`-generated `ServerHandler` methods - // (`Self::tool_router()`), not read through this field — kept only because `#[tool_router]` - // requires a field of this name/type to exist on the struct. #[allow(dead_code)] tool_router: ToolRouter, } @@ -138,8 +108,6 @@ impl OttylMcp { format!("{}{path}", self.base_url) } - /// Resolves the profile a dashboard tool call should operate on: the explicit `profile` - /// argument if given, otherwise the target instance's currently active profile. async fn resolve_profile(&self, explicit: Option) -> Result { if let Some(profile) = explicit { return Ok(profile); @@ -282,10 +250,6 @@ impl OttylMcp { json_result(body) } - // Read-modify-write over HTTP (no dashboard-level locking): two concurrent `add_panel` calls - // against the same dashboard can race and one's addition can be lost, same as two browser tabs - // saving the same dashboard through the webapp today. Not addressed here — out of scope for a - // thin proxy over the existing `save_dashboard` endpoint. #[tool(description = "Add a new panel to a dashboard, or update an existing one by panel_id.")] async fn add_panel( &self, @@ -359,8 +323,6 @@ impl OttylMcp { } } -/// Percent-encodes a single path segment (profile/dashboard names/ids can contain characters that -/// aren't valid unescaped in a URL path). fn urlencoding_path(segment: &str) -> String { segment .bytes() @@ -373,8 +335,6 @@ fn urlencoding_path(segment: &str) -> String { .collect() } -/// Generates a new panel id by hashing title/promql/time, avoiding a `uuid` dependency (matches -/// `settings::generate_imported_css_id`'s approach for the same reason). fn generate_panel_id(title: &str, promql: &str) -> String { use std::collections::hash_map::DefaultHasher; use std::hash::{Hash, Hasher}; diff --git a/src/metric_primitive.rs b/src/metric_primitive.rs index 8acedf9..86fba1e 100644 --- a/src/metric_primitive.rs +++ b/src/metric_primitive.rs @@ -3,16 +3,13 @@ use serde::Serialize; use std::cmp::Ordering; use std::fmt; -/// A simple, self-describing value used by metrics/labels without pulling in JSON. #[allow(dead_code)] #[derive(Debug, Clone, Serialize)] pub enum MetricPrimitive { String(String), Number(Number), Timestamp(Timestamp), - /// Milliseconds elapsed; distinct from `Number` so it reads unambiguously in JSON/label form. Duration(i64), - /// A `Boolean`-kind param's value: whether `message_match_regex` matched the line. See `MetricPrimitiveKind::Boolean`. Boolean(bool), } @@ -51,7 +48,6 @@ impl Ord for 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 { @@ -65,7 +61,6 @@ impl fmt::Display for MetricPrimitive { } } -/// Numbers are kept as a tagged int/float pair so exact integers stay exact. #[allow(dead_code)] #[derive(Debug, Clone, Copy, Serialize)] pub enum Number { @@ -74,7 +69,6 @@ pub enum Number { } impl Number { - /// 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 0165978..cffe0af 100644 --- a/src/profile.rs +++ b/src/profile.rs @@ -1,8 +1,3 @@ -//! Named, persistable bundles of parsing/indexing rules and dashboards for a CLI app. -//! Ticket 049 folded profile storage inline into `Settings` (`settings.json`) instead of a separate -//! `profiles.json`; this module still owns the `Profile`/`Dashboard`/declaration data model, plus the -//! CRUD methods on `Settings` (`impl Settings` below `ProfileError`) that used to live on the -//! now-removed `ProfileStore`. #![allow(dead_code)] use std::collections::hash_map::DefaultHasher; @@ -22,16 +17,13 @@ use crate::engine::{DeclareMetricRequest, DeclareParamRequest, EngineHandle, Eng use crate::settings::Settings; use tokio::sync::oneshot; -/// 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`. Duration, - /// Mirrors `MetricPrimitiveKind::Boolean`. Boolean, } @@ -59,20 +51,14 @@ impl From for MetricPrimitiveKind { } } -/// A stored param declaration, matching `DeclareParamRequest`'s shape (== `data_value::ParamDeclaration`). #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct ParamDeclarationRecord { pub name: String, pub message_match_regex: String, pub capture_group: String, - /// See `crate::data_value::ParamDeclaration::json_field_path`; `#[serde(default)]` - /// so profiles saved before this field existed still load as plain regex-capture-group params. #[serde(default)] pub json_field_path: Option, pub kind: ProfileMetricPrimitiveKind, - /// See `crate::data_value::ParamDeclaration::elapsed_since_last`; `#[serde(default)]` so - /// profiles saved before ticket 038 (where a `Duration`-kind value param implicitly meant this) - /// still load — `migrate_duration_value_params` below converts those. #[serde(default)] pub elapsed_since_last: bool, } @@ -99,17 +85,12 @@ impl From<&ParamDeclarationRecord> for DeclareParamRequest { json_field_path: record.json_field_path.clone(), kind: record.kind.into(), elapsed_since_last: record.elapsed_since_last, - // A profile only ever stores manually-declared params (see `record_param_declaration`). auto_derived: false, log_stream_gated: false, } } } -/// Pre-037 role of an inline metric param matcher — a label identifying a bucket, or the (at most -/// one) value that becomes the event's captured value. Kept only so `LegacyMetricParamRecord` can -/// still deserialize a profile saved before params were unified into standalone `ParamDeclaration`s -/// referenced by name; see `migrate_legacy_metric_params`. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] #[serde(rename_all = "lowercase")] enum LegacyParamRole { @@ -118,10 +99,6 @@ enum LegacyParamRole { Value, } -/// Pre-037 inline metric param matcher shape (formerly `data_value::RegexToMetricPrimitiveMatcher`), -/// kept only to support migrating profiles saved before ticket 037 unified params — see -/// `migrate_legacy_metric_params`. Never produced when serializing a record built from a -/// `DeclareMetricRequest`. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] struct LegacyMetricParamRecord { param_name: String, @@ -133,25 +110,16 @@ struct LegacyMetricParamRecord { role: LegacyParamRole, } -/// A stored metric declaration, matching `DeclareMetricRequest`'s shape. `legacy_params` is only -/// ever populated by deserializing a pre-037 profile file (old key name `"params"`) — see -/// `migrate_legacy_metric_params`, which converts it into `label_params`/`value_param` plus any -/// implicitly-created standalone params, and empties it back out so a subsequent save never writes -/// the old shape again. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct MetricDeclarationRecord { pub name: String, pub message_match_regex: String, - /// Names of standalone `ParamDeclarationRecord`s used as this metric's bucket labels. #[serde(default)] pub label_params: Vec, - /// Name of the standalone `ParamDeclarationRecord` (if any) whose value becomes this metric's bucket value. #[serde(default)] pub value_param: Option, - /// 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`; `#[serde(default)]` so older profiles still load. #[serde(default)] pub unmatched_labels: Option>, #[serde(default, rename = "params", skip_serializing_if = "Vec::is_empty")] @@ -185,15 +153,6 @@ impl From<&MetricDeclarationRecord> for DeclareMetricRequest { } } -/// Migrates every metric in `profile` still carrying pre-037 inline matchers -/// (`MetricDeclarationRecord::legacy_params`, deserialized from an old profile file's `"params"` -/// key) into the unified shape: each matcher becomes a reference to a standalone -/// `ParamDeclarationRecord`, resolved by reusing an existing declared param of the exact same name -/// and shape (regex/capture-group-or-json-field-path/kind) when one exists, or else creating a new -/// one implicitly named `.` (added to `profile.params`). Deterministic -/// naming means re-running this on an already-migrated profile is a no-op, and running it twice on -/// the same legacy data produces the same result. Returns whether anything changed, so the caller -/// can decide whether to persist the migration. fn migrate_legacy_metric_params(profile: &mut Profile) -> bool { let mut migrated = false; for metric_index in 0..profile.metrics.len() { @@ -221,8 +180,6 @@ fn migrate_legacy_metric_params(profile: &mut Profile) -> bool { migrated } -/// Resolves one legacy matcher to a standalone param name, reusing an existing declared param of -/// the same bare name and shape, or else creating `.` on `profile.params`. fn resolve_or_create_legacy_param( profile: &mut Profile, metric_name: &str, @@ -247,9 +204,6 @@ fn resolve_or_create_legacy_param( capture_group: matcher.capture_group.clone(), json_field_path: matcher.json_field_path.clone(), kind: matcher.kind, - // Pre-037, a Value-role Duration matcher implicitly meant "elapsed time since the - // previous same-labeled event" (ticket 038's now-explicit `elapsed_since_last`) — carry - // that meaning forward rather than dropping it into a meaningless literal-parse matcher. elapsed_since_last: matcher.role == LegacyParamRole::Value && matcher.kind == ProfileMetricPrimitiveKind::Duration, }); @@ -257,18 +211,6 @@ fn resolve_or_create_legacy_param( implicit_name } -/// Migrates every already-declared `Duration`-kind param that's referenced as some metric's -/// `value_param` into the explicit `elapsed_since_last` extractor (ticket 038). Before ticket 038, a -/// `Duration`-kind value param implicitly meant "elapsed time since the previous same-labeled -/// event" (see `crate::engine::apply_metric_to_log`'s pre-038 history) — this migration makes that -/// meaning explicit so `Duration` kind no longer has a role-dependent special case. Only params -/// actually referenced as *some* metric's value param are converted: a `Duration`-kind param used -/// purely as a label param already meant a literal parsed millisecond count, and keeps meaning that. -/// Runs after `migrate_legacy_metric_params` (which populates `value_param` from pre-037 -/// `legacy_params` and already sets `elapsed_since_last` correctly for those via -/// `resolve_or_create_legacy_param`), so this only has new work to do for profiles saved between -/// ticket 037 and ticket 038 landing, where `value_param` already pointed at a bare `Duration`-kind -/// param straight from the unified shape. Returns whether anything changed. fn migrate_duration_value_params(profile: &mut Profile) -> bool { let value_param_names: std::collections::HashSet<&str> = profile.metrics.iter().filter_map(|metric| metric.value_param.as_deref()).collect(); @@ -285,7 +227,6 @@ fn migrate_duration_value_params(profile: &mut Profile) -> bool { migrated } -/// 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 { @@ -294,7 +235,6 @@ pub enum ChartType { Pie, } -/// 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, @@ -303,7 +243,6 @@ pub struct Panel { pub chart_type: ChartType, } -/// A named set of panels, saved as part of a profile. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct Dashboard { pub id: String, @@ -312,7 +251,6 @@ pub struct Dashboard { pub panels: Vec, } -/// 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); @@ -331,7 +269,6 @@ impl Dashboard { } } -/// 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(), @@ -344,7 +281,6 @@ fn seeded_error_split_metric() -> MetricDeclarationRecord { } } -/// 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![ @@ -364,7 +300,6 @@ fn seeded_default_dashboard() -> Dashboard { dashboard } -/// 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, @@ -387,7 +322,6 @@ impl Profile { } } -/// 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(); @@ -442,16 +376,11 @@ pub async fn load_into_engine(engine: &EngineHandle, profile: &Profile) { } } -/// Error returned by `Settings`'s profile-related mutations and lookups. #[derive(Debug)] pub enum ProfileError { - /// No profile with the given name exists. 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. CannotDeleteDefault, - /// Underlying I/O failure reading/writing the profile store or settings file. Io(io::Error), } @@ -474,10 +403,6 @@ impl From for ProfileError { } } -/// Reads a legacy pre-049 `profiles.json` file (back when profiles lived in a separate file rather -/// than inline in `settings.json`) for `Settings::migrate_legacy_profiles_file`. Returns `None` if -/// the file doesn't exist or can't be read/parsed — treated as "nothing to migrate" rather than -/// failing settings load. pub(crate) fn load_legacy_profile_store(path: &Path) -> Option> { let contents = fs::read_to_string(path).ok()?; match serde_json::from_str::>(&contents) { @@ -492,23 +417,6 @@ pub(crate) fn load_legacy_profile_store(path: &Path) -> Option> { } } -/// Turns any pre-unification/pre-explicit-duration metric or param shapes still present in -/// `profiles` into their current form; used by `Settings::load` on every startup (previously done -/// by the now-removed `ProfileStore::load`, before ticket 049 folded profile storage into -/// `settings.json`). Returns whether anything changed, so the caller can decide whether to -/// re-persist. -/// -/// Ticket 037: turn any pre-unification inline metric matchers into standalone param references. -/// Deterministic naming makes this idempotent, so it's safe to run on every load; persisting the -/// result isn't required for correctness (migration re-runs identically next time), but it's done -/// here anyway so the on-disk file only carries the legacy `"params"` shape once, on the very next -/// save. -/// -/// Ticket 038: follows on immediately after — once `migrate_legacy_metric_params` has resolved -/// every metric's `value_param`, convert any `Duration`-kind param still referenced that way into -/// the explicit `elapsed_since_last` extractor, so a profile saved either before 037 (inline -/// matchers) or between 037 and 038 (bare `value_param` reference) ends up behaving identically -/// under the new, explicit shape. pub(crate) fn migrate_legacy_profile_shapes(profiles: &mut [Profile]) -> bool { let mut any_migrated = false; for profile in profiles { @@ -521,17 +429,11 @@ pub(crate) fn migrate_legacy_profile_shapes(profiles: &mut [Profile]) -> bool { any_migrated } -/// Profile CRUD, previously on the standalone `ProfileStore` (removed by ticket 049): every mutation -/// here reads/writes `self.profiles` directly and persists via `Settings::save`, so `settings.json` is -/// the single source of truth instead of a separate file kept in sync alongside it. impl Settings { pub fn profile(&self, name: &str) -> Option<&Profile> { self.profiles.iter().find(|p| p.name == name) } - /// Creates a new profile named `name`, persisting settings, copying the active profile's - /// params/metrics/dashboards as a starting point rather than starting blank. Fails with - /// `DuplicateName` if the name exists. pub fn create_profile(&mut self, settings_path: &Path, name: String) -> Result<(), ProfileError> { if self.profile(&name).is_some() { return Err(ProfileError::DuplicateName); @@ -547,8 +449,6 @@ impl Settings { Ok(()) } - /// Deletes the profile named `name`, persisting settings, clearing `active_profile` if it was - /// active. Fails with `NotFound`, or `CannotDeleteDefault` for the `Default` profile. pub fn delete_profile(&mut self, settings_path: &Path, name: &str) -> Result<(), ProfileError> { if name == DEFAULT_PROFILE_NAME { return Err(ProfileError::CannotDeleteDefault); @@ -562,9 +462,6 @@ impl Settings { Ok(()) } - /// Ensures a `Default` profile exists (seeding a starter dashboard/metric only on first - /// creation), and falls back to active if nothing else is. Called once on startup before anything - /// else touches profiles. pub fn ensure_default_profile(&mut self, settings_path: &Path) -> Result<(), ProfileError> { if self.profile(DEFAULT_PROFILE_NAME).is_none() { let mut profile = Profile::new(DEFAULT_PROFILE_NAME.to_string()); @@ -579,9 +476,6 @@ impl Settings { Ok(()) } - /// Sets (or clears, with `None`) the active profile. Fails with `NotFound` if `name` is `Some` - /// and no such profile exists. Doesn't itself reload declarations into a running engine — that - /// happens on next startup via `load_into_engine`. pub fn set_active_profile( &mut self, settings_path: &Path, @@ -597,11 +491,6 @@ impl Settings { Ok(()) } - /// Empties `profile_name`'s stored param declarations, persisting settings — the counterpart to - /// `record_param_declaration` for `DELETE /api/params` (ticket 052). A profile only ever stores - /// manually-declared params (auto-derived ones live only in the engine, never persisted here), so - /// this is enough to keep the whole set from reappearing on next load; the engine-side clear - /// re-derives whatever auto params the retained log text still supports. pub fn clear_param_declarations( &mut self, settings_path: &Path, @@ -617,9 +506,6 @@ impl Settings { Ok(()) } - /// Records/updates a declared param into `profile_name`'s stored declarations (matched by param - /// name), persisting settings. Intended to be called by the HTTP API alongside declaring against - /// the engine, so declarations survive a restart. pub fn record_param_declaration( &mut self, settings_path: &Path, @@ -640,17 +526,6 @@ impl Settings { Ok(()) } - /// Records/updates a declared metric into `profile_name`'s stored declarations (matched by metric - /// name), persisting settings; same integration intent as `record_param_declaration`. - /// - /// `promoted_params` is any of the metric's referenced `label_params`/`value_param` that live - /// only as auto-derived engine state (see `crate::engine::auto_declare_params`) rather than a - /// persisted profile param — the caller (`ApiState::persist_metric_declaration`) resolves these - /// against the live engine before calling in. Without persisting them alongside the metric, a - /// restart would replay the metric before any log line re-derives its auto params, and the - /// engine would reject it as referencing an undeclared param. Each is added only if `profile - /// .params` doesn't already have an entry of that name, so an existing (e.g. manually-declared) - /// param is never clobbered. pub fn record_metric_declaration( &mut self, settings_path: &Path, @@ -677,10 +552,6 @@ impl Settings { Ok(()) } - /// Same as `record_metric_declaration`, for an in-place edit that may rename the metric: drops - /// any persisted record under `old_name` first (when it differs from the new name) so a rename - /// doesn't leave a stale duplicate declaration behind, then upserts by the new name as usual. - /// `promoted_params` has the same meaning as in `record_metric_declaration`. pub fn record_metric_declaration_update( &mut self, settings_path: &Path, @@ -711,11 +582,6 @@ impl Settings { Ok(()) } - /// Removes `name`'s persisted metric declaration from `profile_name` (ticket 051), so it doesn't - /// reappear from `settings.json` on the next restart. A no-op (not an error) if the profile has - /// no record under that name — matches `Settings::remove_css`'s sibling calls in spirit, but - /// deleting an already-gone declaration is fine here since the caller (the API layer) has - /// already confirmed against the live engine index whether the metric existed at all. pub fn remove_metric_declaration( &mut self, settings_path: &Path, @@ -732,14 +598,11 @@ impl Settings { Ok(()) } - /// Lists the dashboards saved on `profile_name`; fails with `NotFound` if it doesn't exist. pub fn list_dashboards(&self, profile_name: &str) -> Result<&[Dashboard], ProfileError> { let profile = self.profile(profile_name).ok_or(ProfileError::NotFound)?; Ok(&profile.dashboards) } - /// Creates a new, empty dashboard named `name` on `profile_name`, persisting settings; fails - /// with `NotFound` if the profile doesn't exist. pub fn create_dashboard( &mut self, settings_path: &Path, @@ -757,9 +620,6 @@ impl Settings { Ok(dashboard) } - /// Replaces the dashboard identified by `dashboard_id` with `dashboard`, persisting settings; - /// `dashboard.id` is forced to `dashboard_id` so a caller can't change a dashboard's identity - /// through this call. pub fn save_dashboard( &mut self, settings_path: &Path, @@ -783,8 +643,6 @@ impl Settings { Ok(dashboard) } - /// Deletes the dashboard identified by `dashboard_id` from `profile_name`, persisting settings; - /// fails with `NotFound` if either doesn't exist. pub fn delete_dashboard( &mut self, settings_path: &Path, @@ -839,10 +697,6 @@ mod tests { } } - /// Ticket 075: reads no longer go through `EngineHandle::tx` — this reads straight off - /// `EngineHandle::reader_factory`, same as the real API layer. Only used here for - /// `ParamDeclarations`/`MetricDeclarations` lookups, neither of which ever needs the - /// `EnsureSortHelperIndex` round-trip a sorted `Logs` query would (see `api.rs`'s own `ask`). async fn ask(engine: &EngineHandle, query: EngineQuery) -> EngineResponse { let read_handle = engine.reader_factory.handle(); let guard = read_handle.enter().expect("engine unavailable"); @@ -864,7 +718,6 @@ mod tests { let dup = settings.create_profile(&settings_path, "web".to_string()); assert!(matches!(dup, Err(ProfileError::DuplicateName))); - // Reload from disk: profiles persisted. let reloaded_settings = Settings::load(&settings_path); assert_eq!(reloaded_settings.profiles.len(), 2); assert!(reloaded_settings.profile("web").is_some()); @@ -889,7 +742,6 @@ mod tests { assert!(settings.profile(DEFAULT_PROFILE_NAME).is_some()); assert_eq!(settings.active_profile, Some(DEFAULT_PROFILE_NAME.to_string())); - // Idempotent: doesn't duplicate the profile or clobber an already-active (non-Default) profile. settings.create_profile(&settings_path, "web".to_string()).unwrap(); settings.set_active_profile(&settings_path, Some("web".to_string())).unwrap(); settings.ensure_default_profile(&settings_path).unwrap(); @@ -926,7 +778,6 @@ mod tests { .any(|p| p.chart_type == ChartType::Line && p.promql.contains(SEEDED_METRIC_NAME))); assert!(dashboard.panels.iter().any(|p| p.chart_type == ChartType::Pie && p.promql == SEEDED_METRIC_NAME)); - // Reload (simulating a restart) and ensure_default again: must not re-seed or duplicate. let mut reloaded = Settings::load(&settings_path); reloaded.ensure_default_profile(&settings_path).unwrap(); let default_profile = reloaded.profile(DEFAULT_PROFILE_NAME).unwrap(); @@ -1009,7 +860,6 @@ mod tests { assert_eq!(copy.params, settings.profile("web").unwrap().params); assert_eq!(copy.dashboards, settings.profile("web").unwrap().dashboards); - // With no active profile, new profiles start empty as before. settings.set_active_profile(&settings_path, None).unwrap(); settings.create_profile(&settings_path, "blank".to_string()).unwrap(); let blank = settings.profile("blank").unwrap(); @@ -1029,17 +879,14 @@ mod tests { settings.set_active_profile(&settings_path, Some("web".to_string())).unwrap(); assert_eq!(settings.active_profile, Some("web".to_string())); - // Switching to another known profile works and persists. settings.set_active_profile(&settings_path, Some("worker".to_string())).unwrap(); let reloaded = Settings::load(&settings_path); assert_eq!(reloaded.active_profile, Some("worker".to_string())); - // Setting an unknown profile active is rejected, leaving the previous value. let err = settings.set_active_profile(&settings_path, Some("ghost".to_string())); assert!(matches!(err, Err(ProfileError::NotFound))); assert_eq!(settings.active_profile, Some("worker".to_string())); - // Clearing the active profile works. settings.set_active_profile(&settings_path, None).unwrap(); assert_eq!(settings.active_profile, None); } @@ -1064,14 +911,12 @@ mod tests { assert_eq!(profile.metrics.len(), 1); assert_eq!(profile.metrics[0].name, "requests"); - // Re-declaring the same name updates in place rather than duplicating. let param_req_v2 = param_request("level", r"lvl=(?P\w+)", "level"); settings.record_param_declaration(&settings_path, "web", ¶m_req_v2).unwrap(); let profile = settings.profile("web").unwrap(); assert_eq!(profile.params.len(), 1); assert_eq!(profile.params[0].message_match_regex, r"lvl=(?P\w+)"); - // Reload from disk: recorded declarations survive a restart. let reloaded = Settings::load(&settings_path); let profile = reloaded.profile("web").unwrap(); assert_eq!(profile.params.len(), 1); @@ -1092,22 +937,18 @@ mod tests { settings.remove_metric_declaration(&settings_path, "web", "requests").unwrap(); assert!(settings.profile("web").unwrap().metrics.is_empty()); - // Removing an unknown metric name is a harmless no-op, not an error. settings.remove_metric_declaration(&settings_path, "web", "never-declared").unwrap(); - // Reload from disk: the removal itself survives a restart (ticket 051's acceptance criterion). let reloaded = Settings::load(&settings_path); assert!(reloaded.profile("web").unwrap().metrics.is_empty()); } - /// Acceptance criterion: declaring params/metrics, "restarting" (reloading settings from disk), and replaying into a fresh engine reproduces the same declarations and still ingests lines, without re-declaring by hand. #[tokio::test] async fn profile_round_trip_reproduces_declarations_in_fresh_engine() { let dir = tempfile::tempdir().unwrap(); let settings_path = dir.path().join("settings.json"); let mut settings = Settings::default(); - // Declare directly into a profile, as the HTTP API will do via the `record_*_declaration` hooks. settings.create_profile(&settings_path, "web".to_string()).unwrap(); settings .record_param_declaration( @@ -1116,8 +957,6 @@ mod tests { ¶m_request("level", r"level=(?P\w+)", "level"), ) .unwrap(); - // A metric now references params by name (ticket 037), so "duration" must be declared as a - // standalone param before the metric below can reference it as a label param. settings .record_param_declaration( &settings_path, @@ -1135,18 +974,15 @@ mod tests { .unwrap(); settings.set_active_profile(&settings_path, Some("web".to_string())).unwrap(); - // Simulate a restart: drop everything in memory, reload from disk. drop(settings); let settings = Settings::load(&settings_path); let active_name = settings.active_profile.clone().unwrap(); let profile = settings.profile(&active_name).unwrap().clone(); - // Spawn a fresh engine and replay the profile's declarations into it. let settings_handle = std::sync::Arc::new(std::sync::Mutex::new(settings.clone())); let engine_tx = crate::engine::spawn(settings_handle, 16); load_into_engine(&engine_tx, &profile).await; - // Ingest a line and confirm the engine works with the replayed declarations (no re-declaration needed). engine_tx .tx .send(EngineMessage::Ingest(IngestedLine { @@ -1198,7 +1034,6 @@ mod tests { assert_eq!(logs[0].text, "level=info request duration=42"); } - /// Acceptance criterion: a dashboard (with a panel) survives reloading settings from disk. #[test] fn dashboard_create_save_delete_round_trip_and_persists_across_reload() { let dir = tempfile::tempdir().unwrap(); @@ -1224,7 +1059,6 @@ mod tests { assert_eq!(saved.panels.len(), 1); assert_eq!(saved.id, dashboard.id); - // Simulate a restart: drop and reload from disk. drop(settings); let mut reloaded = Settings::load(&settings_path); let dashboards = reloaded.list_dashboards("web").unwrap(); @@ -1233,23 +1067,17 @@ mod tests { assert_eq!(dashboards[0].panels.len(), 1); assert_eq!(dashboards[0].panels[0].promql, r#"requests{path="/a"}"#); - // Deleting removes it, and that too persists. reloaded.delete_dashboard(&settings_path, "web", &dashboard.id).unwrap(); assert!(reloaded.list_dashboards("web").unwrap().is_empty()); let reloaded_again = Settings::load(&settings_path); assert!(reloaded_again.list_dashboards("web").unwrap().is_empty()); - // Unknown profile/dashboard both fail with NotFound. assert!(matches!( reloaded_again.list_dashboards("ghost"), Err(ProfileError::NotFound) )); } - /// Writes a pre-037 `settings.json` (with an inline `profiles` array whose metrics carry legacy - /// `"params"` matchers rather than `label_params`/`value_param`) directly to disk, bypassing - /// `Settings`/`Profile` entirely so this doesn't accidentally exercise the current - /// (post-migration) shape. `profiles_json` is the raw JSON array of profiles. fn write_legacy_settings_json(settings_path: &Path, profiles_json: &str) { fs::create_dir_all(settings_path.parent().unwrap()).unwrap(); let contents = format!( @@ -1258,9 +1086,6 @@ mod tests { fs::write(settings_path, contents).unwrap(); } - /// Acceptance criterion: a profile saved before ticket 037 (inline `param_name`/`capture_group`/ - /// `kind`/`role` matchers embedded in the metric) loads with those matchers turned into - /// standalone param declarations referenced by name, and the metric still ingests identically. #[test] fn migrates_legacy_inline_metric_params_into_standalone_declarations() { let dir = tempfile::tempdir().unwrap(); @@ -1289,7 +1114,6 @@ mod tests { let settings = Settings::load(&settings_path); let profile = settings.profile("legacy").unwrap(); - // No pre-existing bare "path"/"duration" param, so implicit ones were created, named after the metric. assert_eq!(profile.params.len(), 2); let path_param = profile.params.iter().find(|p| p.name == "requests.path").unwrap(); assert_eq!(path_param.message_match_regex, profile.metrics[0].message_match_regex); @@ -1302,11 +1126,9 @@ mod tests { assert_eq!(profile.metrics[0].label_params, vec!["requests.path".to_string()]); assert_eq!(profile.metrics[0].value_param, Some("requests.duration".to_string())); - // The migration is persisted, so the on-disk file no longer carries the legacy "params" shape. let reloaded_raw = fs::read_to_string(&settings_path).unwrap(); assert!(!reloaded_raw.contains("param_name")); - // Replaying into a fresh engine behaves identically to before migration: the metric still ingests and buckets. let profile = profile.clone(); let settings_handle = std::sync::Arc::new(std::sync::Mutex::new(Settings::default())); tokio::runtime::Runtime::new().unwrap().block_on(async { @@ -1332,10 +1154,6 @@ mod tests { }); } - /// Acceptance criterion (ticket 038): a pre-037 legacy profile whose inline `Value`-role matcher - /// used `kind: "duration"` (the old "elapsed time since previous event" special case) migrates - /// into a standalone param with the new explicit `elapsed_since_last: true` extractor, not just a - /// bare `Duration`-kind param that would now (post-038) mean a literal parsed millisecond count. #[test] fn migrates_legacy_value_role_duration_matcher_into_elapsed_since_last_param() { let dir = tempfile::tempdir().unwrap(); @@ -1370,11 +1188,6 @@ mod tests { assert_eq!(profile.metrics[0].value_param, Some("login_gap.gap".to_string())); } - /// Acceptance criterion (ticket 038): a profile saved after ticket 037 landed but before ticket - /// 038 — so it already carries the unified `label_params`/`value_param` shape rather than legacy - /// inline matchers, with a bare `Duration`-kind param referenced as a metric's `value_param` — is - /// migrated the same way: the param gains `elapsed_since_last: true` so its behavior is unchanged - /// (elapsed time since the previous same-labeled event) under the new explicit extractor. #[test] fn migrates_post_037_bare_duration_value_param_into_elapsed_since_last() { let dir = tempfile::tempdir().unwrap(); @@ -1406,13 +1219,9 @@ mod tests { let gap_param = profile.params.iter().find(|p| p.name == "gap").unwrap(); assert!(gap_param.elapsed_since_last); - // A Duration-kind param not referenced as any value_param is left untouched (still a literal parse). assert!(!profile.params.iter().find(|p| p.name == "user").unwrap().elapsed_since_last); } - /// When a bare-named param already declared on the profile matches a legacy matcher's shape - /// exactly (same regex/capture-group/kind), migration reuses it by name instead of creating a - /// redundant `.` declaration. #[test] fn migration_reuses_an_existing_matching_bare_named_param() { let dir = tempfile::tempdir().unwrap(); @@ -1442,14 +1251,11 @@ mod tests { let settings = Settings::load(&settings_path); let profile = settings.profile("legacy").unwrap(); - // Reused the existing "path" declaration rather than creating "requests.path". assert_eq!(profile.params.len(), 1); assert_eq!(profile.params[0].name, "path"); assert_eq!(profile.metrics[0].label_params, vec!["path".to_string()]); } - /// Migration is idempotent: loading an already-migrated (or never-legacy) profile a second time - /// doesn't change anything or re-trigger a save. #[test] fn migration_is_a_noop_on_an_already_migrated_profile() { let dir = tempfile::tempdir().unwrap(); @@ -1473,11 +1279,6 @@ mod tests { assert_eq!(before, after); } - /// Acceptance criterion: a metric declared against a param that only exists as auto-derived - /// engine state (not yet a persisted profile param) still ingests correctly after a simulated - /// restart, because `record_metric_declaration`'s `promoted_params` persists that param into the - /// profile alongside the metric. Without this, `load_into_engine` would reject the metric on - /// restart since declarations replay before any log line re-derives the auto param. #[tokio::test] async fn metric_referencing_auto_derived_param_survives_restart_via_promotion() { let dir = tempfile::tempdir().unwrap(); @@ -1487,8 +1288,6 @@ mod tests { settings.create_profile(&settings_path, "web".to_string()).unwrap(); settings.set_active_profile(&settings_path, Some("web".to_string())).unwrap(); - // Simulate the auto-derive path: ingest a line carrying a logfmt key before any param is - // manually declared, so `dx` only exists as engine-internal auto-derived state. let settings_handle = std::sync::Arc::new(std::sync::Mutex::new(settings.clone())); let engine_tx = crate::engine::spawn(settings_handle, 16); engine_tx @@ -1501,8 +1300,6 @@ mod tests { .await .unwrap(); - // Declare a metric referencing the auto-derived "dx" param — this succeeds live (the param - // exists in the engine), mirroring the interactive-declare flow that hit this bug. let metric_req = metric_request("direction_x", r"dx=(?P\S+) moved", "dx"); let (respond_to, response) = oneshot::channel(); engine_tx @@ -1512,7 +1309,6 @@ mod tests { .unwrap(); response.await.unwrap().unwrap(); - // Promote "dx" from its live (auto-derived) engine state, as `ApiState::persist_metric_declaration` does. let EngineResponse::ParamDeclarations(live_params) = ask(&engine_tx, EngineQuery::ParamDeclarations).await else { @@ -1530,7 +1326,6 @@ mod tests { }; settings.record_metric_declaration(&settings_path, "web", &metric_req, &[promoted]).unwrap(); - // Simulate a restart: drop everything, reload from disk into a *fresh* engine with no prior ingestion. drop(settings); let reloaded_settings = Settings::load(&settings_path); let profile = reloaded_settings.profile("web").unwrap().clone(); diff --git a/src/promql.rs b/src/promql.rs index 5027113..227ef33 100644 --- a/src/promql.rs +++ b/src/promql.rs @@ -1,21 +1,12 @@ -//! Hand-rolled parser for a small PromQL subset: metric name + label matchers, an optional `[5m]` range, and a top-level `sum()`/`avg()`/`count()`/`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. -//! `sum()`/`avg()` may additionally wrap a `rate(...)` call directly (e.g. `sum(rate(requests[5m]))`), and either may be followed by a `by (label, ...)` clause to preserve one series per distinct value of the named labels instead of collapsing to a single series. -//! `count()` is a plain vector selector wrapper only (no `rate()` nesting) — it counts occurrences per group regardless of the metric's value, which is the deliberate opt-in for occurrence counting that `sum()`/`avg()` no longer fall back to for a `String`-kind metric (see `crate::engine::evaluate_promql`). - use std::time::Duration; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum AggOp { Sum, Avg, - /// Occurrence count per group, ignoring the metric's stored value entirely — the explicit, - /// opt-in-by-name alternative to summing a `String`-kind metric's coerced-to-1.0 values. Count, Rate, - /// Buckets counter values into fixed-width buckets; requires `bucket_width`. Histogram, - /// The most-recent counter value per label combination. Gauge, } @@ -30,25 +21,17 @@ pub struct PromqlQuery { pub agg: Option, pub metric_name: String, pub matchers: Vec, - /// 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. pub bucket_width: Option, - /// Set when `agg` is `Sum`/`Avg` and it wraps an inner `rate(metric[range])` rather than a bare vector - /// selector, e.g. `sum(rate(requests[5m]))`. `range` above carries the inner `rate()`'s duration in that case. pub inner_rate: bool, - /// Labels named in a trailing `by (...)` clause; empty means "collapse to a single series" (the historical - /// behavior of `sum()`/`avg()`). When non-empty, one series is kept per distinct combination of these labels. pub by_labels: Vec, } -/// 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, 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()?; @@ -177,7 +160,6 @@ impl<'a> Parser<'a> { Ok(matchers) } - /// 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('[') { @@ -239,7 +221,6 @@ impl<'a> Parser<'a> { }) } - /// Parses a trailing `by (label, ...)` clause, if present; returns an empty vec otherwise. fn parse_by_clause(&mut self) -> Result, String> { self.skip_whitespace(); let checkpoint = self.pos; @@ -289,14 +270,11 @@ impl<'a> Parser<'a> { _ => None, }; self.skip_whitespace(); - // `sum by (label) (metric)`: a `by (...)` clause may also precede the aggregation's - // parenthesized argument, not just follow it. let prefix_by_labels = if agg.is_some() { self.parse_by_clause()? } else { Vec::new() }; self.skip_whitespace(); if agg.is_some() && self.peek() == Some('(') { self.expect_char('(')?; self.skip_whitespace(); - // `sum()`/`avg()` may wrap `rate(metric[range])` directly instead of a bare vector selector. let mut inner = if matches!(agg, Some(AggOp::Sum) | Some(AggOp::Avg)) { let inner_checkpoint = self.pos; let took_inner_rate = self.parse_identifier().is_ok_and(|id| id == "rate"); @@ -338,7 +316,6 @@ impl<'a> Parser<'a> { if prefix_by_labels.is_empty() { trailing_by_labels } else { prefix_by_labels }; Ok(inner) } else { - // 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() } @@ -432,7 +409,6 @@ mod tests { #[test] fn rejects_unknown_aggregation() { - // `median` isn't a supported agg_op, so this parses as a bare metric name and trips on the trailing `(...)`. let err = parse_query("median(requests)").unwrap_err(); assert!(err.contains("unexpected trailing input")); } diff --git a/src/rate.rs b/src/rate.rs index 71ace08..42fde8f 100644 --- a/src/rate.rs +++ b/src/rate.rs @@ -1,24 +1,7 @@ -//! Ticket 040: the single windowed-rate/count implementation in the codebase, shared by -//! PromQL's `rate()` (`crate::engine::evaluate_promql`, via the `windowed_rate_series` adapter -//! there) and alert `Threshold`/`Ratio` conditions (`crate::alerts`). Previously each computed -//! its own trailing window independently — PromQL scanning indexed bucket data, alerts scanning -//! raw log-line matches with a hand-rolled `VecDeque` — and could disagree for equivalent-looking -//! rules simply because the window boundary was implemented twice. -//! -//! `count_in_window` is the one place that decides what "falls in the trailing window" means; -//! everything else in this module is built on top of it, either by batch-rescanning a full point -//! series (`rate_at`/`windowed_rate_series`, fine for PromQL's historical/one-shot evaluation) or -//! incrementally (`RateWindow`, for alerts, which see one point at a time and need cost -//! proportional to the window rather than a full rescan per ingested line). - use std::collections::VecDeque; use crate::timestamp::Timestamp; -/// How many `points` fall in the trailing `(now - window_ms, now]` window. Points are occurrence -/// markers rather than a continuous reading, so only their timestamps matter — the boundary is -/// exclusive on the old end and inclusive of `now` itself, matching the original -/// `windowed_rate_series` behavior this replaces. fn count_in_window(points: &[(Timestamp, f64)], now: Timestamp, window_ms: i64) -> usize { let now_ms = now.as_millis(); let window_start = now_ms - window_ms; @@ -31,33 +14,21 @@ fn count_in_window(points: &[(Timestamp, f64)], now: Timestamp, window_ms: i64) .count() } -/// Trailing-window occurrence rate (points per second) ending at `now`: how many `points` fall in -/// `(now - window_ms, now]`, divided by the window width in seconds. pub fn rate_at(points: &[(Timestamp, f64)], now: Timestamp, window_ms: i64) -> f64 { let window_seconds = window_ms as f64 / 1000.0; count_in_window(points, now, window_ms) as f64 / window_seconds } -/// Evaluates `rate_at` at every one of `points`' own timestamps, turning an occurrence series -/// into a windowed rate series — used by PromQL's `rate()` so it plots as a real line rather than -/// one trailing point. pub fn windowed_rate_series(points: &[(Timestamp, f64)], window_ms: i64) -> Vec<(Timestamp, f64)> { points.iter().map(|(ts, _)| (*ts, rate_at(points, *ts, window_ms))).collect() } -/// Incrementally maintained trailing window: for a caller that sees points arrive one at a time -/// (alert `Threshold`/`Ratio` conditions, evaluated per ingested line) and needs cost proportional -/// to the window rather than `count_in_window`'s full rescan per call. Assumes points are pushed -/// in non-decreasing timestamp order, true for ingested log lines. #[derive(Debug, Default, Clone)] pub struct RateWindow { timestamps: VecDeque, } impl RateWindow { - /// Builds a window pre-populated from already-seen history (e.g. currently-ingested logs - /// matching an alert's query), so a freshly-created window starts with whatever of that - /// history already falls in the trailing window instead of empty. pub fn backfill(timestamps: impl IntoIterator, now: Timestamp, window_ms: i64) -> Self { let mut window = Self::default(); for ts in timestamps { @@ -67,14 +38,11 @@ impl RateWindow { window } - /// Records one occurrence at `timestamp` and drops anything that has aged out of the - /// resulting trailing window. pub fn push(&mut self, timestamp: Timestamp, window_ms: i64) { self.timestamps.push_back(timestamp); self.prune(timestamp, window_ms); } - /// Same boundary as `count_in_window`: drops entries at or before `now - window_ms`. fn prune(&mut self, now: Timestamp, window_ms: i64) { let window_start = now.as_millis() - window_ms; while let Some(&front) = self.timestamps.front() { @@ -102,9 +70,7 @@ mod tests { (Timestamp::from_millis(1000), 1.0), (Timestamp::from_millis(2000), 1.0), ]; - // (1000, 2000]: only the point at 2000. assert_eq!(rate_at(&points, Timestamp::from_millis(2000), 1000), 1.0); - // (0, 2000]: points at 1000 and 2000 -> 2 points / 2s. assert_eq!(rate_at(&points, Timestamp::from_millis(2000), 2000), 1.0); } @@ -136,7 +102,6 @@ mod tests { #[test] fn rate_window_backfill_seeds_from_history_within_the_window() { let history = [0, 500, 2000, 2500].map(Timestamp::from_millis); - // now=2500ms, window=1000ms -> window (1500, 2500]: only 2000 and 2500 qualify. let window = RateWindow::backfill(history, Timestamp::from_millis(2500), 1000); assert_eq!(window.count(), 2); } diff --git a/src/sessions.rs b/src/sessions.rs index 335bfcf..657b232 100644 --- a/src/sessions.rs +++ b/src/sessions.rs @@ -1,8 +1,3 @@ -//! Named session registry (ticket 068): each running `ottyl` process registers itself under a -//! name by writing a small file to `/sessions/.json`, so a second CLI -//! invocation (e.g. `ottyl mcp --session `, ticket 067) can resolve a name to a port without -//! the user copying one around by hand. - use std::fs; use std::io; use std::path::{Path, PathBuf}; @@ -10,7 +5,6 @@ use std::process; use serde::{Deserialize, Serialize}; -/// On-disk shape of a `sessions/.json` file. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct SessionInfo { pub name: String, @@ -18,8 +12,6 @@ pub struct SessionInfo { pub pid: u32, } -/// The `sessions/` directory for a given settings file, following the same -/// sibling-directory convention as `profiles/` (see `settings::migrate_legacy_profiles_file`). pub fn sessions_dir(settings_path: &Path) -> PathBuf { settings_path .parent() @@ -31,10 +23,6 @@ fn session_file_path(dir: &Path, name: &str) -> PathBuf { dir.join(format!("{name}.json")) } -/// Best-effort liveness check for a PID. Shells out to `kill -0` on unix (no signal is delivered, -/// only permission/existence is checked); assumed alive on other platforms, which just means a -/// stale session file there waits for an explicit name collision to be overwritten instead of -/// being proactively cleaned up. #[cfg(unix)] fn is_pid_alive(pid: u32) -> bool { process::Command::new("kill") @@ -51,9 +39,6 @@ fn is_pid_alive(_pid: u32) -> bool { true } -/// Removes session files whose owning process is no longer running. Best-effort: there's no -/// shutdown hook to unregister a session on graceful exit, so this runs on every startup instead. -/// Any I/O error reading a single entry is ignored — a future call gets another chance. pub fn cleanup_stale(dir: &Path) { let Ok(entries) = fs::read_dir(dir) else { return; @@ -75,16 +60,10 @@ pub fn cleanup_stale(dir: &Path) { } } -/// Generates a random human-readable session name (e.g. `comic-gazelle`) for when `--name` isn't -/// given explicitly. pub fn generate_name() -> String { petname::petname(2, "-").unwrap_or_else(|| format!("session-{}", process::id())) } -/// Registers this process as session `name` on `port`: cleans up stale entries, then writes -/// `sessions/.json`, overwriting any existing file for that name (documented behavior — a -/// second instance started under the same explicit `--name` as a still-running one simply -/// replaces its registration; the older process keeps running, just without a discoverable entry). pub fn register_session(settings_path: &Path, name: &str, port: u16) -> io::Result { let dir = sessions_dir(settings_path); fs::create_dir_all(&dir)?; @@ -99,9 +78,6 @@ pub fn register_session(settings_path: &Path, name: &str, port: u16) -> io::Resu Ok(path) } -/// Looks up a session by name. Returns `None` if no such file exists, it fails to parse, or its -/// process is no longer alive (a stale entry left by a process that didn't exit through the normal -/// next-`register_session` cleanup path). pub fn resolve_session(settings_path: &Path, name: &str) -> Option { let dir = sessions_dir(settings_path); let contents = fs::read_to_string(session_file_path(&dir, name)).ok()?; @@ -109,8 +85,6 @@ pub fn resolve_session(settings_path: &Path, name: &str) -> Option is_pid_alive(info.pid).then_some(info) } -/// Lists every session file under `dir`, live or stale (callers that only want live sessions -/// should filter on `is_pid_alive`-backed `resolve_session` instead). Used by `ottyl sessions`. pub fn list_sessions(dir: &Path) -> Vec { let Ok(entries) = fs::read_dir(dir) else { return Vec::new(); @@ -157,8 +131,6 @@ mod tests { let sessions_dir_path = sessions_dir(&settings_path); fs::create_dir_all(&sessions_dir_path).unwrap(); - // A PID essentially guaranteed not to be alive (PID 1 exists but a huge, unlikely-assigned - // PID stands in as "not alive" without depending on system PID-reuse specifics). let stale = SessionInfo { name: "foo".to_string(), port: 1111, pid: 999_999 }; fs::write( session_file_path(&sessions_dir_path, "foo"), @@ -166,8 +138,6 @@ mod tests { ) .unwrap(); - // Registering a live session under the same name succeeds and the stale cleanup pass (run - // as part of registration) doesn't error out on the bogus pre-existing entry. register_session(&settings_path, "foo", 5555).unwrap(); let info = resolve_session(&settings_path, "foo").unwrap(); assert_eq!(info.port, 5555); diff --git a/src/settings.rs b/src/settings.rs index 9cbf85b..1828919 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -1,6 +1,3 @@ -//! Process-wide settings model: colour scheme, CSS entries, and profiles (ticket 049 folded -//! profile storage inline here instead of a separate `profiles.json`), persisted as JSON. - use std::collections::hash_map::DefaultHasher; use std::fmt; use std::fs; @@ -16,15 +13,8 @@ use serde::{Deserialize, Serialize}; use crate::alerts::AlertRule; use crate::profile::Profile; -/// Shared, process-wide settings handle. Cheap to clone. pub type SettingsHandle = Arc>; -/// User's preferred colour scheme for the webapp. -/// -/// `Dark` accepts the legacy `"grey"` serde value (ticket 004: "grey" was a -/// near-duplicate of "dark" and has been removed as a selectable option) so a -/// settings file saved before that change still loads cleanly instead of -/// falling back to `System`. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] #[serde(rename_all = "lowercase")] pub enum ColorScheme { @@ -37,33 +27,22 @@ pub enum ColorScheme { Yellow, Pink, Orange, - /// Brighter, light-surfaced counterpart to `Green` (ticket 048). #[serde(rename = "light_green")] LightGreen, - /// Brighter, light-surfaced counterpart to `Red` (ticket 056). #[serde(rename = "light_red")] LightRed, - /// Brighter, light-surfaced counterpart to `Blue` (ticket 056). #[serde(rename = "light_blue")] LightBlue, - /// Brighter, light-surfaced counterpart to `Yellow` (ticket 056). #[serde(rename = "light_yellow")] LightYellow, - /// Brighter, light-surfaced counterpart to `Pink` (ticket 056). #[serde(rename = "light_pink")] LightPink, - /// Brighter, light-surfaced counterpart to `Orange` (ticket 056). #[serde(rename = "light_orange")] LightOrange, #[default] System, } -/// A single CSS entry: content ottyl stores itself ("imported") or a reference to an external -/// path/URL ("linked", not deletable via the API). Every entry is always a selectable -/// colour-scheme theme (ticket 020 removed the "use as theme"/always-on-overlay distinction); -/// an entry persisted before that change with a stored `is_theme: false` still loads fine (the -/// field is simply ignored) and becomes selectable like any other entry. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(tag = "kind", rename_all = "lowercase")] pub enum CssEntry { @@ -71,25 +50,15 @@ pub enum CssEntry { Linked { path_or_url: String }, } -/// Legacy on-disk shape of the (pre-049) `Profiles` pointer struct: only names/active-profile/dir -/// pointers, with the actual profile content living in a separate `profiles.json`. Kept only so an -/// old `settings.json` still parses; see `ProfilesOnDisk` and `Settings::migrate_legacy_profiles_file`. #[derive(Debug, Clone, Deserialize, Default)] struct LegacyProfilesPointer { - // `names` is intentionally not captured: it was always redundant with the legacy - // `profiles.json`'s actual profile list, which is what migration reads instead. #[serde(default)] active_profile: Option, - /// Directory (relative to the settings file's directory, or absolute) the legacy `profiles.json` - /// lived under. Every version of this codebase ever wrote `"profiles"` here (no API ever let a - /// user change it), so migration doesn't bother re-deriving a custom value from this field. #[serde(default)] #[allow(dead_code)] profiles_dir: Option, } -/// On-disk shape of the `profiles` field: either the new inline `Vec` (ticket 049) or the -/// old pointer-struct shape, so a `settings.json` written before this ticket still deserializes. #[derive(Debug, Clone, Deserialize)] #[serde(untagged)] enum ProfilesOnDisk { @@ -103,9 +72,6 @@ impl Default for ProfilesOnDisk { } } -/// Raw on-disk shape of `Settings`, used only to support deserializing an old `settings.json` whose -/// `profiles` field was the pointer struct and had no top-level `active_profile` — see `Settings`'s -/// `#[serde(from = ...)]`. #[derive(Debug, Clone, Deserialize)] struct SettingsOnDisk { color_scheme: ColorScheme, @@ -147,40 +113,25 @@ impl From for Settings { } } -/// The single, process-wide settings instance. Only `PartialEq` (not `Eq`): `alert_rules` carries `f64` fields, which aren't `Eq`. -/// -/// `profiles`/`active_profile` used to be a separate `Profiles` pointer struct referencing a -/// standalone `profiles.json` (see `crate::profile::ProfileStore`, removed by ticket 049); profile -/// data now lives inline here, as the single source of truth. `Deserialize` goes through -/// `SettingsOnDisk` so a `settings.json` written before this ticket still loads (see -/// `migrate_legacy_profiles_file`, which then imports the old `profiles.json`, if any, on `load`). #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] #[serde(from = "SettingsOnDisk")] pub struct Settings { pub color_scheme: ColorScheme, pub css_entries: Vec, - /// Id of the selected custom theme entry, if any; `None` means `color_scheme` alone determines the active palette. #[serde(default)] pub custom_theme_id: Option, - /// Every known profile, inline (ticket 049). pub profiles: Vec, - /// Name of the currently active profile, if any. pub active_profile: Option, - /// Maximum number of ingested logs kept in memory; `None` means unlimited. #[serde(default)] pub max_logs: Option, - /// Alert rule definitions. #[serde(default)] pub alert_rules: Vec, - /// Ntfy topic every alert rule delivers to; `None` until configured in Settings. #[serde(default)] pub ntfy_topic: Option, - /// Ntfy server override; `None` means `https://ntfy.sh`. #[serde(default)] pub ntfy_server: Option, } -/// 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( crate::consts::APP_QUALIFIER, @@ -191,7 +142,6 @@ pub fn default_settings_path() -> Option { } impl Settings { - /// Loads settings from `path`, creating and persisting defaults if missing. Any I/O or parse error on an existing file is logged and treated as defaults, without overwriting the unreadable file. Also runs the one-time legacy `profiles.json` migration (see `migrate_legacy_profiles_file`). pub fn load(path: &Path) -> Settings { match fs::read_to_string(path) { Ok(contents) => match serde_json::from_str::(&contents) { @@ -229,7 +179,6 @@ impl Settings { } } - /// 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)?; @@ -239,14 +188,6 @@ impl Settings { fs::write(path, contents) } - /// One-time migration (ticket 049): if `self.profiles` is empty (a fresh `Settings`, or one - /// parsed from a pre-049 `settings.json` whose profiles lived in a separate file) and a legacy - /// `/profiles/profiles.json` exists, reads it in, merges it into - /// `self.profiles`, persists `self`, and — only once that save succeeds — renames the old file - /// (`.migrated` suffix) so it isn't re-imported on a later load. Every prior version of this - /// codebase always used the fixed `"profiles"` subdirectory name (no API ever let it be - /// reconfigured), so that's the only location checked. Best-effort: any failure is logged and - /// left for a future load to retry rather than panicking. fn migrate_legacy_profiles_file(&mut self, settings_path: &Path) { if !self.profiles.is_empty() { return; @@ -276,12 +217,6 @@ impl Settings { } } - /// Migrates any pre-037/pre-038 metric or param shapes still present in `self.profiles` into - /// their current form (see `crate::profile::migrate_legacy_profile_shapes`), persisting `self` - /// if anything changed. Runs on every load — previously done inside the now-removed - /// `ProfileStore::load` (ticket 049 folded profile storage into `settings.json`) — so a profile - /// saved by an older version of ottyl, whether loaded inline here or just merged in by - /// `migrate_legacy_profiles_file`, still gets migrated forward. fn migrate_legacy_profile_shapes(&mut self, settings_path: &Path) { if crate::profile::migrate_legacy_profile_shapes(&mut self.profiles) && let Err(err) = self.save(settings_path) @@ -294,14 +229,10 @@ impl Settings { } } -/// Error returned by CSS-entry mutations and lookups. #[derive(Debug)] pub enum CssError { - /// No entry with the given id exists. NotFound, - /// 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, or persisting `settings.json`. Io(io::Error), } @@ -323,7 +254,6 @@ impl From for CssError { } } -/// 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(), @@ -337,7 +267,6 @@ fn linked_entry_id(path_or_url: &str) -> String { format!("linked-{:016x}", hasher.finish()) } -/// 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); @@ -355,7 +284,6 @@ fn imported_css_file_path(css_dir: &Path, id: &str) -> PathBuf { } impl Settings { - /// 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, @@ -374,14 +302,12 @@ impl Settings { Ok(id) } - /// 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, path_or_url: String) -> Result<(), CssError> { self.css_entries.push(CssEntry::Linked { path_or_url }); self.save(settings_path)?; Ok(()) } - /// 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, @@ -398,7 +324,6 @@ impl Settings { Some(CssEntry::Imported { .. }) => { let idx = index.unwrap(); self.css_entries.remove(idx); - // 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(()) @@ -406,7 +331,6 @@ impl Settings { } } - /// 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), @@ -424,7 +348,6 @@ mod tests { #[test] fn default_settings_path_ends_with_settings_json() { - // 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()); @@ -471,10 +394,6 @@ mod tests { assert_eq!(settings.color_scheme, ColorScheme::Dark); } - /// A `settings.json` written before ticket 049 (nested `Profiles` pointer struct, no top-level - /// `active_profile`) still parses: the pointer's `active_profile` carries over, and `profiles` - /// starts empty (the real content lived in a separate `profiles.json`, migrated separately by - /// `Settings::load`, not by plain deserialization). #[test] fn legacy_profiles_pointer_shape_deserializes_with_empty_inline_profiles() { let settings: Settings = serde_json::from_str( @@ -548,7 +467,6 @@ mod tests { assert!(css_file.exists()); assert_eq!(fs::read_to_string(&css_file).unwrap(), "body { color: red; }"); - // Drop and reload settings from disk (simulates a restart). drop(settings); let reloaded = Settings::load(&settings_path); assert!(reloaded.css_entries.iter().any(|entry| matches!( @@ -598,7 +516,6 @@ mod tests { let result = settings.remove_css(&settings_path, &css_dir, &linked_id); assert!(matches!(result, Err(CssError::NotImported))); - // Entry must still be there — rejection shouldn't silently drop it. assert_eq!(settings.css_entries.len(), 1); } @@ -629,9 +546,6 @@ mod tests { assert!(matches!(result, Err(CssError::NotImported))); } - /// Acceptance criterion (ticket 049): a pre-existing legacy `profiles_dir/profiles.json` is - /// merged into `settings.json` on the next load, and the old file is renamed (not re-imported on - /// a subsequent load). #[test] fn load_migrates_legacy_profiles_file_into_inline_settings() { let dir = tempfile::tempdir().unwrap(); @@ -644,8 +558,6 @@ mod tests { r#"[{"name":"web","params":[],"metrics":[],"dashboards":[]}]"#, ) .unwrap(); - // No settings.json yet: simulates upgrading a pre-049 install that never wrote one either, - // or one that did but whose nested pointer already carried an active_profile name. fs::write( &settings_path, r#"{"color_scheme":"system","css_entries":[],"custom_theme_id":null,"profiles":{"names":["web"],"active_profile":"web","profiles_dir":"profiles"},"max_logs":null,"alert_rules":[],"ntfy_topic":null,"ntfy_server":null}"#, @@ -657,19 +569,15 @@ mod tests { assert_eq!(settings.profiles[0].name, "web"); assert_eq!(settings.active_profile, Some("web".to_string())); - // The legacy file is gone (renamed), so it isn't re-imported. assert!(!legacy_path.exists()); let migrated_path = legacy_path.with_extension("json.migrated"); assert!(migrated_path.exists()); - // Reload from disk: the migrated settings.json now carries profiles inline directly. let reloaded = Settings::load(&settings_path); assert_eq!(reloaded.profiles.len(), 1); assert_eq!(reloaded.profiles[0].name, "web"); } - /// A fresh install (no legacy file at all) never creates `profiles.json` — it's not even looked - /// for beyond the one migration check, and `profiles` simply stays empty. #[test] fn load_without_legacy_file_leaves_profiles_empty() { let dir = tempfile::tempdir().unwrap(); diff --git a/src/timestamp.rs b/src/timestamp.rs index 3681f24..3adfce4 100644 --- a/src/timestamp.rs +++ b/src/timestamp.rs @@ -1,7 +1,6 @@ use serde::Serialize; use std::time::{SystemTime, UNIX_EPOCH}; -/// 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);