diff --git a/.gitignore b/.gitignore index 7e50c68..59a4203 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,5 @@ target/ .claude/worktrees/ profile.json.gz +.codex + diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..78c3a62 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,65 @@ +# AGENTS.md + + + + +## Codebase Intelligence for ottyl (Repowise) + +Indexed by [Repowise](https://repowise.dev). Last indexed: 2026-09-07 (commit 0c41048). Confidence: 98%. +### How to work in this repo + +- **Trust the index.** `verified: true` means the bytes were checked against the live tree, so never re-read those lines. Re-read only on `bounds: "approximate"`, `_meta.stale_warning`, `search_method: "bm25"` or `confidence: "low"`; `index_behind: true` alone is informational. +- **Pre-edit, not instead-of-edit.** These tools decide *which* files to read and edit. Reading a file before you edit it is correct and expected. +- **Noisy commands** (tests, builds, `git log`/`diff`, searches, listings): prefer `repowise distill `, the same command with its exit code preserved and errors-first output. A `[repowise#: N lines omitted]` marker is recoverable via `repowise expand ` (add `-q ` to filter); never re-run the command to see omitted output. +- **Recording a decision** you had to reason out: `repowise decision add --title T --decision D` records it without prompting and prints the id (`--format json` to parse it back). It lands `proposed`, for a person to confirm. + +### Tools + +| Tool | When and why | +|------|--------------| +| `get_answer(question)` | First call for any how/where/why question. Cite `confidence: "high"` or `grounding: "extracted"` directly; `degraded` means judge by `retrieval_quality`. `symbol_bodies` has live bodies. | +| `get_context(targets=[...])` | Triage card for files/modules/symbols: docs, signatures, hotspot, fix history. No source bytes — `include=["skeleton"]` for the whole file verified, `["callers"|"decisions"]` for depth. Batch targets. | +| `get_symbol(id)` | **Follow-up, not an entry point** — one verified body for an id a prior response named (`path.py::Name`, `path.py:140-180`, `repowise#`). Never walk a file symbol by symbol; Read it. | +| `search_codebase(query)` | Hybrid search, auto-routed by query shape; force with `mode=symbol|path|concept|hybrid`. A hit whose `sources` are `[fts]` only has no semantic agreement, so verify it. | +| `get_why(query, targets?)` | Why the code is shaped this way: decision records, git archaeology, rationale comments. Call before a refactor or a pattern divergence. | +| `get_risk(targets, changed_files?)` | What history says about touching these files. PR mode (`changed_files`) leads with a `directive`: read `will_break` / `missing_cochanges` / `missing_tests` / `tests_to_run` first. | +| `get_change_risk(revspec, extensions?, exclude_patterns?)` | Defect score for a whole commit or `base..head` range, from its diff on the live checkout. Lead with `risk_percentile`. Scores a range; `get_risk` scores paths. | +| `get_health(targets?, include?)` | Defect / maintainability / performance scores and findings. Self-check the files you touched before finishing. | +| `get_dead_code()` | Confidence-tiered unreachable files / unused exports / zombie packages. For cleanup sweeps, not targeted fixes. | +| `get_overview()` | Architecture map. Call once, first, in an unfamiliar repo; skip it after that. | + +### Architecture +ottyl ingests log & metric streams -> processes via Rust engine (parsing, filtering, PromQL) -> outputs visualized data via TypeScript web UI. + +### Key modules +- `root` — What the prose above calls things, the identifier to search for, and where it lives + +### Entry points +- `src/main.rs` +- `webapp/src/main.tsx` +- `webapp/src/App.tsx` + +### Files that need care (bug-fix history first, then churn — check `get_risk` before editing) +- `webapp/src/App.tsx` — 3 bug fixes, last fix today; 22 commits/90d +- `webapp/src/components/Dashboards.tsx` — 3 bug fixes, last fix today; 17 commits/90d +- `webapp/src/lib/api.ts` — 1 bug fix, last fix today; 41 commits/90d +- `webapp/src/components/NavMenu.tsx` — 1 bug fix, last fix today; 22 commits/90d +- `webapp/src/components/Docs.tsx` — 1 bug fix, last fix today; 16 commits/90d + +### Code health +Three co-equal signals: defect risk 7.53/10 avg, hotspot health 7.15/10 (stable), worst `src/engine/snapshot/mod.rs` at 5.5/10 · maintainability 7.91/10 · performance risk 21 open static I/O-in-loop / N+1 findings. Detail: `get_health()`. + +Critical files: +- `webapp/src/components/NavMenu.tsx` — churn risk — impact −2.4 +- `src/engine/tests.rs` — churn risk — impact −2.4 +- `src/engine/mod.rs` — churn risk — impact −2.4 +- `src/config/mod.rs` — churn risk — impact −2.4 +- `src/api/dto.rs` — churn risk — impact −2.4 + +### Commands +- Build: `make build` +- Test: `make test` +- Lint: `make lint` +- Dev: `make dev` + + diff --git a/IDEAS.md b/IDEAS.md deleted file mode 100644 index 02219ac..0000000 --- a/IDEAS.md +++ /dev/null @@ -1,12 +0,0 @@ -Under a normal log ingestion params are usually auto-detected. -Sometimes, however it's necessary to extract a param - that can be done with a regex: If a message has a non empty or missing capture for the target group, then it has a param value. -On top of that, it may be easier to capture params dependent on these other params - say you have a string of a capture group you are interested in, and you want to extract values out of it. That should be possible as well as a regex patten on top of messages with a string param. -Sometimes a string param (or even message) may be json. In which case we may want to extract params with `jq` syntax. - -There is lots of test-helper code that is simply a function stub with ignored parameters and a different scope. We don't need any of those ever. You can change the scope of some functions for the tests. Preferably you change the tests instead. - -The timeline still does not reflect the dropdown. Either use a toast with a reason why it can't while setting the dropdown to what it currently was, or have some kind of loading indicator if it's a loading issue. - -JSON formatting on frontend should auto-collapsed not open - - diff --git a/ideas/0001-actor-split.md b/ideas/0001-actor-split.md new file mode 100644 index 0000000..ddc6119 --- /dev/null +++ b/ideas/0001-actor-split.md @@ -0,0 +1,296 @@ +# Idea: split the engine's single actor into a log-ingestion worker + per-profile worker(s) + +## Original note + +> This needs to be fleshed out with ai. +> +> The general idea is that the primary engine ingesting worker is +> doing too much. It should only parse logs to extract params. +> +> Since profiles carry additional (non-derived) params, metrics, +> dashboards, and panels - those should be stored separately and can +> be handled by a different worker. The worker would have read access +> to the primary log sequence and derived params index, but would +> construct it's own profile perspective. It has write access and +> then +> +> The profiles need to be informed of new messages to index. +> +> What if message needs to be different format? Thats per-profile. +> +> System param suppressions? +> +> Regex indexing? We probably want text indexing on the primary while +> also +> +> Values in the primary index should be arcs, to prevent cloning +> allocations. + +## Current architecture (confirmed in code) + +- **One process, one actor, today.** `spawn_engine` + (`src/cli/mod.rs:95`) calls `engine::spawn(...)` exactly once per + server run. `load_active_profile` (`src/cli/mod.rs:105-124`) loads + *one* active profile's param/metric declarations into that single + engine at startup, before ingest starts, with an explicit comment + explaining why: "there's no way to reprocess earlier lines later, so + this must happen before `spawn_ingest`" (`src/cli/mod.rs:104-106`). + All other `engine::spawn` call sites (`src/api/mod.rs:853,1459`, + several in `src/config/profile.rs`) are test harnesses, not a second + production engine instance. +- The actor loop is `run()`, `src/engine/handle.rs:130-365` — a single + `while let Some(message) = rx.recv().await` over one + `mpsc::Receiver` (`:141`). Every mutation (`Ingest`, + `IngestMetricSample`, `DeclareParam`/`RemoveParamDeclaration`, + `ClearParams`, `DeclareMetric`/`UpdateMetric`/ + `RemoveMetricDeclaration`, `SetMaxLogs`, `EnsureSortHelperIndex`, + `Barrier` — full list `src/engine/message.rs:30-63`) funnels through + this one channel and one task. +- **Queries already bypass this actor.** `EngineQuery` + (`src/engine/message.rs:12-27`) doesn't go through the mpsc channel + — `EngineHandle::ask` reads directly off a `left_right` + `IndexReader`/`ReadHandle` (`src/engine/handle.rs:49-73`, entry point + `src/api/mod.rs:218-219`, evaluated by `handle_query_ro`, + `src/engine/snapshot/mod.rs:856-858`). So read-side scaling is + already decoupled from the actor loop today — this idea is entirely + about the **write/mutation** path. +- Per-line ingest (`EngineMessage::Ingest` arm, + `src/engine/handle.rs:143-219`) does, synchronously, in one + `left_right` write op, per line: ingest-lag/watermark bookkeeping, + log-stream header detection, storing the line as + `DataValue::LogLiteral`, auto-param detection + (`auto_declare_params`), running **every declared metric's regex** + against the line (`apply_all_metrics` → + `apply_metric_to_log`/`resolve_param_value`, + `src/engine/snapshot/mod.rs:409-460,821-851`), sort-helper-index + maintenance, log eviction, and then — after publish — + `alerts::evaluate_alerts` against the resulting snapshot + (`handle.rs:199-219`), which reads metric buckets that were just + computed in the same message. +- Module size: `src/engine/` is ~5.5k lines excluding tests (`snapshot/ + mod.rs` alone is 1100 lines — the whole `EngineSnapshot` impl per its + own header comment). +- **No `Arc`, everywhere owned data.** The index is one flat + `BTreeMap` (`snapshot/mod.rs:43`) of owned + `String`s (`DataValue::LogLiteral { text: String }`, + `src/engine/data_value.rs:81`). Per ingested line, the raw text is + cloned at least twice for storage alone (`handle.rs:175,184-186`), + and every metric that matches the line clones its own copy of the + label/value strings it captures (`apply_metric_to_log`, + `mod.rs:426,439,444`) — with N metrics referencing overlapping + params, one line produces N independent owned-String allocations of + essentially the same substrings. +- **`left_right`'s reconciliation deep-clones everything.** After a + batch of writes, `left_right::Absorb::sync_with` clones the *entire* + `EngineSnapshot` (`*self = first.clone()`, `handle.rs:29-31`) to + reconcile the read-side copy — i.e. the "values should be Arc" note + isn't just about avoiding per-line clones, it's also about avoiding + a full-`BTreeMap`-of-Strings deep clone on every reconciliation. +- **No text/regex index exists.** `CompiledFilter::Regex(re) => + re.is_match(text)` (`snapshot/filter.rs:159`) runs per line at query + time; `IndexKey` (`src/engine/index_key.rs:8-32`) has no + inverted/full-text structure. Every filter-bar query re-scans and + re-matches the whole retained range. +- **Backfill is a full synchronous history replay today, in the same + actor.** `declare_metric`/`update_metric` + (`snapshot/mod.rs:330-391`) → `backfill_metric` (`:481-509`) collects + **all** retained lines into a `Vec` and replays + `apply_metric_to_log` over every one, inside the single actor's + message handling — blocking ingest/alerts/other declarations for the + duration. `load_into_engine` (`src/config/profile.rs:332-384`) does + this once per metric, awaiting each response, so loading a profile + with N metrics against L retained lines costs O(N·L) synchronous + scan time. This is also the mechanism [[0004-profile-switch-reindexing-cost]] + describes for *runtime* profile switching via the `activate_profile` + API handler (`src/api/mod.rs:712-768`, which removes then + re-declares via the same messages) — worth reconciling that this + research pass separately characterized profile activation at + startup as one-shot ("no way to reprocess earlier lines later"), + while the earlier research for ticket 099/idea 0004 confirmed a + *runtime* reload path exists via `activate_profile`. Both are true: + startup load is one-shot by necessity (ingest hasn't started yet), + and the runtime reload path reuses the same declare/backfill + machinery after the fact — but this is worth double-checking before + design work starts, since the two research passes framed it + differently. +- **Alerts are coupled to the single actor's local state.** Alert + evaluation runs directly in the ingest arm against the just-mutated + snapshot (`handle.rs:199-211`, `alerts::evaluate_alerts`, + `src/engine/alerts.rs:211`), reading `alert_states: + HashMap` that lives on the single actor + task's own stack frame (`handle.rs:137`) — there's no mechanism today + for one actor to read another actor's in-memory state. +- **Existing multi-task precedent is producer-side only.** + `src/engine/ingest.rs`'s stdin/file-tailing and Prometheus-polling + tasks each hold a cloned `EngineHandle` and push into the *same* + actor's mpsc channel (spawned at `src/cli/mod.rs:136-145`) — a + plausible pattern for "many producers, one consumer," but there's no + existing precedent for multiple *consuming* actors each owning their + own `left_right` write side that need to stay coherent with each + other or with a shared upstream. + +## Implications and things to consider + +### Performance + +- The stated goal — "the primary worker should only parse logs to + extract params" — would remove per-line metric-regex evaluation + (`apply_all_metrics`) from the primary's critical path, which is + real work today (N metrics × regex match per line). That's a + genuine win *if* profile workers can consume the primary's output + without re-paying an equivalent cost themselves — but since a + profile worker still has to run its own metrics' regexes against + each line to build its own buckets, the total CPU work done across + the process doesn't shrink; what changes is that it's no longer all + serialized on one task, so it can run concurrently with the primary + (a real win on a multi-core box) and a slow/backfilling profile + worker no longer blocks raw ingestion. +- The "values in the primary index should be Arcs" change is + independently valuable regardless of whether the actor split + happens — it directly addresses the confirmed double/triple string + cloning per line (storage clone + per-metric label/value clones) and + the full-snapshot deep-clone on every `left_right` reconciliation. + Worth sequencing as a prerequisite: switching to `Arc` (or + `Arc`) for stored text means a profile worker reading "the + primary log sequence" can cheaply hold references to the same + underlying bytes instead of needing its own copies — this is likely + a load-bearing precondition for the split being worth it at all, not + a nice-to-have alongside it. +- A profile worker needing "read access to the primary log sequence + and derived params index" implies either (a) a second `left_right` + reader over the primary's structures (cheap, if values are Arc'd — + see above) or (b) a message-replay feed (the primary notifies + profile workers of new lines, per the note's "profiles need to be + informed of new messages to index"). Option (b) reintroduces a + second data copy per profile worker unless the message itself only + carries an `Arc`/index reference rather than owned data — otherwise + this recreates the same N-copies-per-line problem this idea is + trying to remove, just distributed across processes/tasks instead of + within one. +- Backfill cost (O(N metrics × L retained lines), see [[0004-profile-switch-reindexing-cost]]) + doesn't disappear with a split — it moves to whichever worker owns + the profile. The upside is it no longer blocks the *primary's* + ingest/query-serving while it happens, which is the main perf + argument for doing this at all, per that idea's "Performance" + section. +- Full-text/regex indexing on the primary (per the note's "Regex + indexing?") is a materially different performance investment from + the Arc/split work — an inverted or trigram index trades ingest-time + CPU and memory for query-time speed on filter-bar searches, and is + worth scoping as its own effort (see [[0005-additional-read-indexes]]) + rather than folded into this split, since it doesn't depend on the + actor topology. + +### Code structure + +- This is the single largest structural change on the idea list — + `src/engine/` (~5.5k lines) currently assumes one writer owning one + `left_right` pair. Splitting introduces: a defined "primary + perspective" (raw logs + derived/auto params only, no + profile-specific metrics) as its own bounded module, a + "profile perspective" abstraction that can be instantiated per + active profile, a notification/subscription mechanism from primary + to profile workers (the note's "profiles need to be informed of new + messages to index" — doesn't exist in any form today; the closest + analogue, `ingest.rs`'s producer tasks feeding one consumer, runs the + opposite direction), and a query-fan-out/merge layer for any request + that needs both raw logs and profile-derived data in one response + (today `Logs`/`Timeline` queries read both from a single + `EngineSnapshot` in one pass, e.g. `query.rs:154`'s + `log_entry_params` call against `compiled_params` from the same + snapshot — splitting the data sources means this becomes a real join + across two snapshots/workers instead of one borrow). +- Alert evaluation (`handle.rs:199-211`) currently depends on running + immediately after metric bucketing in the same message, against the + same snapshot. If metrics move to per-profile workers, alerts need to + either move with them (probably correct, since alert rules are + profile-scoped conceptually) or gain a defined way to read + cross-worker metric state that doesn't exist today. +- `EngineHandle`'s current API (`handle.rs:49-73` for reads, + the mpsc `EngineMessage` send for writes) would need to become two + APIs, or one handle type parameterized by "primary" vs. "profile" — + worth deciding whether `api.rs`'s handlers (which currently hold one + `EngineHandle` in `ApiState`) address a single combined handle that + internally fans out, or explicitly juggle a primary handle plus an + active-profile handle. +- Given `CLAUDE.md`'s existing note that `engine.rs` is "the file to + read first" and already the largest/highest-churn file in the repo + (13 commits/90d per the repowise summary), this split is also a + legibility opportunity, not just a performance one — today a reader + has to understand ingest, metrics, alerts, and backfill as one + intertwined concern to understand any one of them. + +### Functionality + +- **Per-profile message format** (the note's "What if message needs to + be different format? Thats per-profile.") — today, log-stream header + detection and logfmt/JSON auto-param extraction happen once, in the + primary, against the raw line (`derive_log_stream_params_from_row`, + `auto_declare_params`, both in the current `Ingest` arm). If + different profiles can expect genuinely different message shapes + from the *same* piped process (unclear from the note whether this + means "different profiles parse the same stream differently" or + "each profile only ever sees its own stream"), that's a real design + fork: either the primary's parsing must become profile-aware (which + undercuts "primary only extracts params" as a clean, profile-agnostic + boundary), or each profile worker does its own message-format + parsing independently on top of the shared raw text — the latter fits + the stated architecture better but means format detection isn't a + primary-side concern at all, just param extraction of an + already-classified line is. +- **System param suppressions** (the note's fragment) is undeveloped — + unclear whether this means "some auto-derived params should never be + promoted to profile-visible params" (a filtering rule between primary + and profile perspectives) or "a way to suppress a param a profile + inherited but doesn't want" (a per-profile override). This needs a + sentence or two from you before it can be scoped at all; flagging + here rather than guessing. +- Multiple simultaneously-active profile workers (implied by "a + different worker" being per-profile, plural) raises a question the + current single-profile-at-a-time model never had to answer: can two + profiles be "active" and indexing concurrently, or is it still + strictly one active profile with the split only changing *how* that + one profile's data is computed relative to the primary? The + perf/functionality tradeoffs are very different depending on the + answer (concurrent multi-profile indexing multiplies the backfill + and per-line cost discussed above by the number of concurrently + active profiles). + +### Not yet considered + +- **Failure/restart semantics**: if a profile worker crashes or lags + behind the primary, what's the recovery story? Today there's one + actor with one failure mode (the whole engine). A split needs an + answer for "profile worker is behind — does it catch up from the + primary's retained history (bounded by the same retention limit the + primary enforces), or is data permanently lost for the gap?" +- **Retention/eviction coordination**: the primary already evicts old + logs (`evict_excess_logs`, `handle.rs:195`, `SetMaxLogs` message). + If a profile worker is reading "the primary log sequence" lazily or + asynchronously, eviction on the primary could remove a line before a + lagging profile worker has processed it — there's no coordination + primitive today (like a per-consumer low-water-mark) to prevent this + silently dropping data for a slow profile worker. +- **Testing story**: `src/engine/tests.rs` (2348 lines) and the + `test_handle` pattern (see ticket 099) are built entirely around one + actor with one channel — a split roughly doubles the kinds of + integration tests needed (primary alone, profile worker alone, + primary+profile interaction) and the existing test harness would + need real rework, not just extension. +- **Migration path**: this is a rewrite of the highest-risk, largest, + most-churned file in the codebase (per `CLAUDE.md`'s repowise + summary) — no plan yet for whether this ships as one big change or + incrementally (e.g. Arc-ing values first as an independent, + lower-risk step; then extracting metric computation into a + separately-callable-but-still-single-actor stage; then finally + splitting into separate tasks) — given the size, an incremental path + is very likely necessary rather than optional. +- **Whether this is worth it at all without profiling evidence**: per + this project's own established norm (tickets 079/080 were removed + for "no profiling evidence to justify the fix"), this is a large + structural bet motivated by the note's stated concern ("primary + worker is doing too much") rather than a measured bottleneck — worth + a `samply` profile of realistic ingest load *before* committing to + this, to confirm metric-bucketing-on-the-primary is actually a + measurable cost relative to param extraction, storage, and alert + evaluation, rather than assuming it from code-reading alone. diff --git a/ideas/0002-nested-and-jq-param-extraction.md b/ideas/0002-nested-and-jq-param-extraction.md new file mode 100644 index 0000000..69e8ed5 --- /dev/null +++ b/ideas/0002-nested-and-jq-param-extraction.md @@ -0,0 +1,158 @@ +# Idea: dependent/nested param extraction and jq-style JSON extraction + +## Original note + +1. A param currently auto-detects or is declared via a regex against + the raw log line — that's the baseline, already true today. +2. Semantics for "has a value": if the message has a non-empty or + missing capture for the target group, treat that as the param + having a value (as opposed to today's behavior — needs + confirmation, see Open questions). +3. Extract a param from *another param's already-captured string*, + applying a second regex on top of it, rather than always + re-matching the raw line. +4. When a string param (or the whole message) is JSON, extract fields + via `jq` syntax rather than the current plain dot-path. + +## Current state (confirmed in code) + +- `ParamDeclaration` (`src/engine/data_value.rs:6-20`) supports exactly + one regex + one named capture group per param, or one JSON + dot-path, or the computed `elapsed_since_last` — these three modes + are mutually exclusive (validated in + `src/engine/snapshot/mod.rs:618-633`). +- `extract_param_raw` (`src/engine/snapshot/params.rs:85-99`) *always* + operates on the raw ingested line's text — there is no "source + param" concept anywhere in `ParamDeclaration`/ + `ParamDeclarationRecord`/`ParamDeclarationDto`. Every param + independently re-parses the same raw line; none can chain off + another param's captured value. +- JSON extraction (`extract_json_field`, `src/engine/data_value.rs:22-39`) + requires the whole line to parse as a JSON object, and walks a plain + dot-separated path through nested objects only — no array indexing, + no jq filters/pipes/functions. Frontend surface: + `webapp/src/components/Params.tsx:24-33` (gates the "JSON field" + option on `looksLikeJsonObject`), free-text path input, no + path-builder/autocomplete. +- No `jq` crate or jq-like dependency exists in the codebase today. + +## Why this is an idea, not a ticket yet + +This is a real feature addition with several open design questions +that change its shape and scope significantly — it's not a +well-bounded bug fix or mechanical change. + +## Open questions + +- **Chaining model**: should a param declaration gain an optional + "source param" field (extract from another param's value instead of + the raw line)? If so, declarations form a small dependency graph — + what happens on a cycle, or when the source param has no value for a + given line (does the dependent param also have no value, silently)? + Does extraction order need to become topological instead of the + current independent-per-declaration model? +- **Missing/empty capture semantics**: what should currently happen + when a regex matches but the named group didn't participate (e.g. an + optional group in an alternation)? Worth checking + `extract_param_raw`'s exact behavior before assuming this needs a + change — it may already do the right thing. +- **jq scope**: is full jq syntax the actual goal, or would extending + the existing dot-path extractor with array indexing (`items[0].id`) + cover most real cases far more cheaply than pulling in a jq + implementation (e.g. the `jaq` crate) as a new dependency? jq's + filter language (pipes, `select()`, `map()`) is a much bigger surface + to expose safely in a declaration UI than a path string. +- **UI shape**: `Params.tsx`'s declaration form is built around one + regex/one capture group or one JSON path; a chained/jq model needs a + materially different UI (e.g. "extract from: [raw message ▾] or + [param: session_id ▾]" plus a jq expression box with live preview + against a sample line). + +## Suggested next step + +Worth a short design pass (e.g. `ce-brainstorm` or a quick +conversation) to settle the chaining model and jq-vs-extended-dot-path +question before this gets broken into tickets — the two features (2/3 +and 4) could also ship independently rather than as one big change. + +## Implications and things to consider + +### Performance + +- Chained extraction means a line can now do N regex/JSON passes + instead of 1 per param, and a dependent param's cost is paid *after* + its source param already ran — worst case, a profile with a long + dependency chain multiplies per-line ingest cost by chain depth. + This lands directly in the single actor's per-line ingest path (see + [[0001-actor-split]] — if that split happens first, param extraction + is exactly the workload it isolates, so it's worth sequencing this + after that decision rather than before). +- A `jq`-style engine (even a Rust one like `jaq`) is meaningfully + more expensive per invocation than a compiled `regex::Regex` capture + — evaluating a jq filter against a parsed `serde_json::Value` for + *every* ingested line for *every* JSON-extraction param is a + different cost profile than today's simple dot-path walk. Worth + benchmarking before committing to full jq rather than assuming it's + free. +- Topological ordering of dependent params (if declarations form a + DAG) adds a per-declaration-change cost (recompute order) but not a + per-line cost, as long as the order is cached after each + add/remove/edit rather than recomputed per line. + +### Code structure + +- `extract_param_raw` (`src/engine/snapshot/params.rs:85-99`) currently + has a flat signature `(text: &str, declaration) -> Option`. + Chaining requires either passing in the full set of already-extracted + param values for the current line (so a declaration can look up its + source param's value) or restructuring extraction as a fold over + declarations in dependency order — a real shape change to this + function and its caller, not a param-by-param tweak. +- The "three mutually exclusive modes" validation in + `src/engine/snapshot/mod.rs:618-633` needs a fourth mode (or an + orthogonal "source" field crossed with the existing regex/JSON + modes) — worth deciding whether source-param and JSON-field are + independent axes (e.g. "JSON-extract from param X" as well as from + the raw line) since that combination is implied by note #3. +- This is exactly the shape of change flagged in this repo's + `CLAUDE.md` as needing three coordinated updates (engine message + + handler, settings/profile persistence, api.rs route) plus a frontend + affordance — touches more surface area than the "Current state" + section alone suggests. + +### Functionality + +- Cycle detection isn't optional once chaining exists — a user could + declare param B from param A and (accidentally, via UI) param A from + param B. Needs either a hard validation error at declare-time or a + documented "first N declarations in insertion order" fallback. +- Auto-derived params (JSON auto-flattening, + `flatten_json_object`/`flatten_json_object_into`, + `src/engine/data_value.rs:41-60+`) aren't user declarations — decide + whether a chained/jq param can source from an *auto-derived* param, + which would tie this feature to auto-detection internals that + weren't designed to be a stable "source" API. +- Backfill: declaring a new dependent param triggers the same + `backfill_metric`-style history replay as any param declaration + (see [[0004-profile-switch-reindexing-cost]] for the general + backfill-cost concern) — a chain of N dependent params means N + passes over retained history on declare, not just N passes per new + line going forward. + +### Not yet considered + +- Error/partial-failure UX: what does the UI show for a param whose + jq expression is syntactically valid but fails on a particular + line's JSON shape (e.g. missing key, type mismatch) — silent + no-value, or a visible per-line extraction error surfaced somewhere? +- Whether jq's more powerful operations (`map`, `select`, arithmetic) + should be allowed at all in a param *value* context, where the + result is expected to be a single scalar — most of jq's power is + about reshaping/filtering collections, which doesn't map cleanly + onto "this param has one value per log line." +- No investigation yet into how this interacts with `sort_by` + (`SortHelperKey`) or PromQL — do chained/jq-derived params get the + same downstream treatment (sortable, usable in metric declarations, + usable in dashboard queries) as directly-extracted params, or are + there gaps once the extraction pipeline is no longer "one regex + against the raw line"? diff --git a/ideas/0003-timeline-ordering-invariant.md b/ideas/0003-timeline-ordering-invariant.md new file mode 100644 index 0000000..7fe8cec --- /dev/null +++ b/ideas/0003-timeline-ordering-invariant.md @@ -0,0 +1,117 @@ +# Idea: timeline ordering — confirm/harden the sequence-is-primary invariant + +## Original note + +"The timeline is time ordered. That is always derived from the +sequence ordering. That means the primary sequence is simply sequence +ordered." — reads as a statement of intent/invariant rather than a bug +report, so it's unclear whether this needs a code change or is just +confirming existing behavior. + +## Current state (confirmed in code) + +- `IndexKey::IngestedLog { sequence, timestamp }` + (`src/engine/index_key.rs:10-13`) derives `Ord` with `sequence` as + the primary field and `timestamp` only as a (practically unused) + tie-breaker, since `sequence` is already unique. +- `sequence` is a strictly monotonic counter incremented per ingested + line (`src/engine/ingest.rs:13,52`); `timestamp` is a separate + wall-clock `Timestamp::now()` read at the same point in the ingest + loop (`src/engine/ingest.rs:18,93`) — not derived from `sequence`. +- The default log listing path (`logs_in_sequence_range`, + `src/engine/snapshot/mod.rs:180-208`, used by the `Logs` query in + `src/engine/snapshot/query.rs:118-172`) is explicitly sequence-order, + with an existing comment documenting this + (`src/engine/snapshot/mod.rs:176-179`). +- The only other ordering mode is `sort_by` (sort by a declared param's + value, sequence as tie-break, via `SortHelperKey`, + `src/engine/snapshot/sort.rs`) — there's no "sort by timestamp" mode + distinct from sequence order. +- **The one place ordering is genuinely timestamp-driven**: the + `Timeline` query (`src/engine/snapshot/query.rs:173-198`) buckets + logs by `timestamp.as_millis() / bucket_ms` + (`:191`), not by sequence. + +## Why this is an idea, not a ticket + +The invariant the note describes already appears to hold for the +default log listing — sequence is the primary and effectively only +ordering key there. The one place `timestamp` (not `sequence`) drives +grouping is the `Timeline` bucketing, and `sequence`/`timestamp` are +captured independently rather than one being derived from the other. +In the current single-threaded ingest loop they're expected to +co-vary, but nothing guards against a backward wall-clock step (NTP +correction, clock skew) between two `Timestamp::now()` calls — if that +ever happened, sequence-ordered views and timestamp-bucketed views +could disagree on relative order for the affected lines. This has not +been observed as an actual bug; it's a latent gap worth a decision. + +## Open question + +Is the ask just "confirm this holds" (in which case: it does, for the +default listing — no code change needed), or do you want the +`Timeline` bucketing hardened so it can never diverge from sequence +order even under clock skew (e.g. clamp each line's timestamp to be +`>= previous line's timestamp`, or derive bucket assignment from +sequence position rather than raw wall-clock read)? The fix is small +either way, but scoping it as a ticket needs that decision first. + +## Implications and things to consider + +### Performance + +- Clamping `timestamp` to be monotonic (`max(this_line_ts, + previous_line_ts)`) is O(1) per line — negligible cost, just one + extra comparison in the existing ingest loop + (`src/engine/ingest.rs:13-18,52,93`). Deriving bucket assignment from + sequence instead of wall-clock would be a larger change (see below) + but isn't inherently more expensive at ingest time. +- No performance concern either way — this is a correctness/consistency + question, not a hot-path cost question. + +### Code structure + +- If bucket assignment moves from wall-clock `timestamp` to something + sequence-derived, `Timeline`'s bucketing + (`src/engine/snapshot/query.rs:173-198`) still fundamentally needs a + *time value* per bucket (users read timeline buckets as "logs between + 10:32:00 and 10:32:05", not "logs 100-150") — sequence can't replace + timestamp as the bucketing axis, only guard it. The realistic fix is + the monotonic-clamp approach, not a switch to sequence-based + bucketing. +- Clamping at ingest time (`ingest.rs`) is the natural single place to + fix this, rather than patching every read path that consumes + `timestamp` — confirm there's exactly one ingest entry point today + (there appear to be two: stdin/file tailing and Prometheus metrics + polling, per `src/engine/ingest.rs` — a clock-skew guard needs to + apply to both, and they'd need to share one "last observed timestamp" + reference point to clamp against consistently). + +### Functionality + +- A clamped timestamp is no longer "true" wall-clock time for that + line — if any UI surface displays a log's literal ingest timestamp + (as opposed to just using it for bucketing), clamping would make a + displayed timestamp lie slightly during a clock step-back event. Low + probability, but worth deciding whether clamping is silent or + surfaced (e.g. a flag on the log entry noting its timestamp was + adjusted). +- This only matters at all during an actual backward clock step, which + in most deployments (a piped CLI process, not a long-running server + through NTP corrections) may never occur in practice — worth weighing + whether this is worth any code change versus documenting it as an + accepted limitation. + +### Not yet considered + +- No investigation into whether the same two independently-captured + timestamp/sequence pattern exists anywhere else besides ingest (e.g. + alert evaluation timestamps, or the Prometheus-polling ingest path + specifically) — the research so far only confirmed the stdin/file + ingest loop. +- No check on how a forward clock jump (not just backward) would + interact with `Timeline` bucketing — a large forward jump would + create a sparse/empty bucket range, which is a different (probably + more common) failure mode than the backward-step case this idea + focuses on, and might be worth addressing in the same pass if one + happens. diff --git a/ideas/0004-profile-switch-reindexing-cost.md b/ideas/0004-profile-switch-reindexing-cost.md new file mode 100644 index 0000000..487cc62 --- /dev/null +++ b/ideas/0004-profile-switch-reindexing-cost.md @@ -0,0 +1,142 @@ +# Idea: profile switching triggers a full synchronous re-index per metric + +## Original note + +"Metrics and dashboards are tied with the profile. However, they are +indexed on the engine. Switching profiles means re-indexing." — a +description of a real coupling; unclear whether it's being flagged as +a performance problem to fix or just an architectural note to be aware +of. + +## Current state (confirmed in code) + +- `Profile` (`src/config/profile.rs:309-318`) is pure config — params, + metrics, dashboards — with no live engine state of its own. + `Settings::set_active_profile` (`src/config/profile.rs:489-502`) + only flips which profile is active and persists to disk; it doesn't + touch the engine. +- The actual switch side effect lives in the API layer: + `activate_profile` (`src/api/mod.rs:746-768`) calls + `reload_engine_declarations` (`src/api/mod.rs:712-744`), which: + 1. Removes every current param/metric declaration from the engine + (`EngineMessage::RemoveParamDeclaration`/`RemoveMetricDeclaration`, + skipping auto-derived params). + 2. Calls `profile.load_into_engine(engine)` + (`src/config/profile.rs:332-384`), re-declaring every param/metric + in the newly-activated profile. +- There is no dedicated `EngineMessage` for "switch profile" — it's + entirely remove-everything-then-redeclare-everything via the + existing declare/remove messages + (`src/engine/message.rs:30-63` has no such variant). +- Each metric redeclare triggers a full rebuild: `declare_metric` + (`src/engine/snapshot/mod.rs:330-335`) → + `resolve_backfill_and_store_metric` → `backfill_metric` + (`:481-509`), which replays **all retained log history** + (`self.logs_range()`) through `apply_metric_to_log` for that metric. + Raw ingested lines themselves are never cleared on switch — only + metric/param declarations and their derived buckets are dropped and + rebuilt. +- This all happens synchronously, one metric at a time, inside the + engine's single-writer actor loop + (`src/engine/handle.rs:303-338`) — so switching profiles blocks that + loop for a duration that scales with retained log volume × number of + metrics in the newly-activated profile. + +## Why this is an idea, not a ticket + +Whether this is worth fixing depends on facts not available from +reading code alone: typical retained log volume in real usage, typical +metric count per profile, and how often users actually switch profiles +mid-session. Without knowing the current switch latency is actually +noticeable, a fix would be speculative optimization. + +## Options if this turns out to matter + +- Diff-based reload: only remove/re-add metrics whose *definition* + actually changed between the old and new profile, instead of + removing everything and redeclaring everything (a metric present + unchanged in both profiles currently still gets a full remove + + backfill for no reason). +- Make the removal/backfill sequence async relative to the HTTP + response, with the UI showing a "reindexing…" state rather than + blocking the request (and the engine loop) until complete. +- Lazily backfill only the time range currently being viewed, deferring + the rest — a bigger change, and only worth it if the switch cost is + dominated by backfill work the user doesn't immediately look at + anyway. + +## Suggested next step + +Before scoping a ticket: confirm with a profile that has a realistic +number of metrics and log volume whether switch latency is actually +perceptible. If it is, the diff-based reload (first option) is the +cheapest fix and a reasonable first ticket. + +## Implications and things to consider + +### Performance + +- Cost today is O(retained lines × metrics in new profile), done + synchronously in the single-writer engine actor + (`src/engine/handle.rs:303-338`) — every other engine message + (ingest, queries, alert evaluation) queues up behind a profile switch + until it finishes, so the blast radius isn't just "the switch feels + slow", it's "the whole app appears frozen" for that duration, + including live ingestion of new lines arriving mid-switch. +- The diff-based reload option only helps when profiles share metrics + — if switching to a profile with a mostly-disjoint metric set (the + common case for genuinely different apps/profiles, which is the + whole point of profiles), it doesn't reduce the backfill cost much. + It's a bigger win for "tweak one metric in the currently active + profile and reactivate" workflows than for switching between + unrelated profiles. +- If [[0001-actor-split]] happens, this problem's shape changes + significantly — a per-profile worker rebuilding its own perspective + from a shared primary index could do that rebuild without blocking + ingestion or query-serving on the primary at all, which addresses the + "whole app freezes" part of this problem as a side effect, even if + the total backfill work stays the same. Worth reading that idea + alongside this one. + +### Code structure + +- The remove-then-readd flow lives entirely in the API layer + (`src/api/mod.rs:712-768`), not the engine — a diff-based reload + would need `reload_engine_declarations` to compute a diff (metrics + present in old profile but not new → remove; new but not old → + declare; present in both — compare full declaration equality, not + just name, to decide if a changed regex/threshold needs a rebuild) + before sending any engine messages. That diff logic is new code, not + a variant of existing code. +- Async/non-blocking reload would need a new `EngineMessage` variant + (today there is none for "switch profile" — it's just existing + declare/remove messages) if the goal is to let the engine + acknowledge the switch request immediately and do backfill work + in the background while still serving other queries — this is a + bigger structural change than diff-based reload, since the engine's + single-actor model currently has no notion of "background work + interleaved with message handling." + +### Functionality + +- If reload becomes async/background, the UI needs a defined + "reindexing in progress" state — what does a dashboard panel show + for a metric that's declared but not yet backfilled? An empty chart + is misleading (looks like "no data") versus a genuinely no-data + metric. +- Partial-failure semantics: if backfill for metric 3 of 5 fails or is + interrupted (e.g. app restart mid-switch), what state is left behind? + Today's synchronous approach has no partial-state problem because it + either completes or the whole process is still running; async reload + introduces a new failure mode that doesn't exist today. + +### Not yet considered + +- No data gathered yet on real profile sizes/log volumes in actual + usage (flagged already in "Why this is an idea") — this entire idea + is speculative until that's known. +- No consideration of whether profile switching happens automatically + (e.g. auto-detected based on the piped command) or is always a + manual user action — if it's always manual and rare, the UX cost of + a blocking-but-rare operation may be perfectly acceptable and not + worth any of the above complexity. diff --git a/ideas/0005-additional-read-indexes.md b/ideas/0005-additional-read-indexes.md new file mode 100644 index 0000000..138e9bd --- /dev/null +++ b/ideas/0005-additional-read-indexes.md @@ -0,0 +1,128 @@ +# Idea: additional indexes to speed up reads + +## Original note + +"Ask AI to come up with additional indexes that will help speed up +reads." — an open-ended research request rather than a scoped +change; this note captures what's already known so a future deep-dive +doesn't start from zero. + +## Current index surface (confirmed in code) + +- `IndexKey` (`src/engine/index_key.rs`) — the engine's indexing key + space includes at least `IngestedLog { sequence, timestamp }`, + `Bucket` (metric bucket data), and `LastMetricEvent`. Ordering is + primary-`sequence` for logs (see + [[0003-timeline-ordering-invariant]] for detail). +- `SortHelperKey` (`src/engine/snapshot/sort.rs`) — a secondary index + keyed by a declared param's value (plus sequence as tie-break), + built to support `sort_by` queries; has explicit staleness handling + (`purge_stale_sort_helper_indexes`, referenced from ticket 081's + history) tied to new bucket ingestion. +- `.claude/CLAUDE.md`'s repowise health summary already flags "19 open + static I/O-in-loop / N+1 findings" and calls out `src/engine.rs`'s + `handle_query` as a function/brain-method hotspot — likely overlaps + with whatever a fresh performance pass on read paths would surface, + worth checking those findings first (`get_health()` / + `get_risk(['src/engine.rs'])` via the repowise MCP tools, once the + repo's index is initialized — it wasn't available this session, see + note below) rather than starting from a blank slate. +- Related, already-ticketed read-path work referenced in prior ticket + history: ticket 077/078 (metric bucket range narrowing — "narrow the + range, don't rescan everything") and ticket 081 (PromQL dashboard + queries rebuild full series from scratch per poll) — both were + framed around the same "avoid full rescans" principle this idea is + chasing generally. + +## Why this is an idea, not a ticket + +This is explicitly a "come up with ideas" ask, not a pre-scoped +change — it needs a dedicated investigation (likely with actual +profiling under a representative workload, per this project's +`samply`-based profiling convention) rather than guesses from reading +code structure alone. Ticket 081's history is a useful precedent here: +079/080 (also performance tickets) were removed for lacking "profiling +evidence to justify the fix" — so a fix in this space should follow +`samply record` evidence, not speculation. + +## Note + +The repowise MCP tools reported no index for this repo this session +("This repository has no repowise index yet") despite +`.claude/CLAUDE.md` describing one as current — worth running `repowise +init --yes` (or checking why the existing index isn't being picked up) +before the next performance-focused session, since `get_risk`/ +`get_health`/`get_change_risk` would materially speed up this kind of +investigation. + +## Suggested next step + +Profile a representative workload with `samply` against the read paths +(`Logs`, `Timeline`, dashboard PromQL panel queries) to find actual +bottlenecks, then propose specific new index structures (or narrowings +of existing ones) grounded in what the flamegraph shows — rather than +guessing at indexes with no evidence they'd matter. + +## Implications and things to consider + +### Performance + +- Any new index is a write-time cost paid on every ingested line in + exchange for a read-time win — worth stating explicitly per proposed + index, since the engine's ingest path is already doing param + extraction + metric bucketing + alert evaluation per line + (see [[0001-actor-split]]'s findings on how much work already happens + synchronously per line); an expensive new index (e.g. a full-text + inverted index for regex/text search) could turn ingest into the new + bottleneck instead of read. +- A text/regex index in particular is a different kind of structure + than the existing `IndexKey`/`SortHelperKey` model — those are + ordered key→value indexes; full-text search typically wants an + inverted index (token → line positions) or a trigram index, which is + a new subsystem, not a new `IndexKey` variant. Worth scoping as its + own investigation rather than bundling into "additional indexes" + generally. +- Memory cost matters as much as CPU cost here — this is a + for-the-life-of-the-process in-memory tool (per `CLAUDE.md`'s + description), so any new index's memory footprint competes directly + with how much log history a user can retain before running out of + RAM. A `samply` CPU profile alone won't surface this — a memory + profile of retained-line-count vs. RSS would be needed too. + +### Code structure + +- New indexes need to hook into the same lifecycle the existing ones + do: built incrementally at ingest, invalidated/pruned when retention + drops old lines, and rebuilt on backfill (declare/profile-switch) + the same way `SortHelperKey`'s `purge_stale_sort_helper_indexes` + does. A proposal that only covers "how to build it" without covering + eviction/backfill parity with existing indexes is incomplete. +- If [[0001-actor-split]] happens first, new indexes would likely be + scoped to whichever worker owns the data they index (e.g. a + profile-specific index lives with that profile's worker, not the + primary log worker) — worth sequencing this investigation after that + architectural decision rather than before, so new indexes aren't + designed against a data-ownership model that's about to change. + +### Functionality + +- Read-path speedups are invisible to users unless something is + currently slow enough to notice — without a concrete complaint or + profiling evidence of an actual slow query, this risks becoming + effort spent on indexes nobody was blocked on. The `samply`-first + approach in "Suggested next step" is meant to guard against exactly + this. + +### Not yet considered + +- No investigation yet into what "reads" actually means for the + intended scope — dashboard panel polling (already covered by ticket + 081's history), the log timeline's default view, ad-hoc filter-bar + queries, and the MCP server's query surface (`src/api/mcp_server.rs`) + are all "reads" with potentially different bottlenecks; a single + "additional indexes" effort may need to be split once profiling + shows which of these is actually slow. +- No check on whether retention limits already bound the problem — + if there's a hard cap on retained log lines, some read-path costs may + already be bounded in practice regardless of indexing, which changes + how much this is worth investing in. diff --git a/tickets/093-json-viewer-collapsed-by-default.md b/tickets/093-json-viewer-collapsed-by-default.md new file mode 100644 index 0000000..dcc5091 --- /dev/null +++ b/tickets/093-json-viewer-collapsed-by-default.md @@ -0,0 +1,35 @@ +# 093 — Log-line JSON viewer should default to collapsed, not expanded + +## Summary + +When a log line's text parses as a JSON object, the frontend renders it +via a collapsible tree view — but every node starts expanded, so a +line with a large JSON payload dumps its entire structure into the +timeline by default instead of showing a collapsed summary the user +opts into expanding. + +## Where + +- `webapp/src/components/LogPreview.tsx:97-107` — `JsonValueView` + recursively renders JSON values, delegating objects/arrays to + `JsonNode`. +- `webapp/src/components/LogPreview.tsx:109-156` — `JsonNode` renders + a collapsible subtree with a ▾/▸ disclosure toggle. Default state is + set at line 116: `useState(true)`, i.e. expanded. Each nested + object/array has its own independent `expanded` state. +- `webapp/src/components/LogPreview.tsx:160-164` — `LogMessage` is the + per-row entry point: parses the line via `tryParseJsonObject` + (`:82-94`) and renders through `JsonValueView` when it succeeds. +- Empty objects/arrays already short-circuit to plain `{}`/`[]` with + no toggle (`:118-125`) — unaffected by this change. + +## Scope + +- Flip the default in `JsonNode` (`LogPreview.tsx:116`) from + `useState(true)` to `useState(false)` so JSON subtrees render + collapsed until the user clicks to expand. +- Since expand state is per-node (not shared across the tree), this + changes every node in a JSON log line's tree independently — confirm + that's the intended look (top-level object collapsed with `{...}` + rather than only nested children collapsing) rather than needing a + "expand root, collapse children" split default. diff --git a/tickets/094-adopt-rustfmt.md b/tickets/094-adopt-rustfmt.md new file mode 100644 index 0000000..60f0b5b --- /dev/null +++ b/tickets/094-adopt-rustfmt.md @@ -0,0 +1,34 @@ +# 094 — Adopt rustfmt and format the codebase + +## Summary + +The Rust backend has no `rustfmt.toml` and isn't currently formatted +to rustfmt's defaults: `cargo fmt -- --check` exits non-zero, reporting +diffs across 24 files (352 diff hunks total). + +## Where + +- No `rustfmt.toml` / `.rustfmt.toml` exists at the repo root. +- `cargo fmt -- --check` reports diffs in `src/api/{dto,mcp_server,mod}.rs`, + `src/cli/{mod,sessions}.rs`, `src/config/{mod,profile}.rs`, and most + of `src/engine/**` (`alerts.rs`, `data_value.rs`, `filter_datetime.rs`, + `filter_parser.rs`, `ingest.rs`, `logfmt.rs`, `message.rs`, `mod.rs`, + `promql.rs`, `rate.rs`, `snapshot/{filter,metrics,mod,params,query,sort}.rs`, + `tests.rs`). Heaviest: `src/engine/snapshot/mod.rs` (~65 hunks), + `src/config/profile.rs` (~38), `src/api/mod.rs` (~35), + `src/engine/alerts.rs` (~28). +- `Makefile:26` (`lint-be`) currently only runs `cargo clippy + --all-targets` — no `cargo fmt --check` step. + +## Scope + +- Add a `rustfmt.toml` if any defaults should be overridden (otherwise + an empty/absent file is fine and rustfmt defaults apply — decide + whether to keep the file empty for documentation purposes or omit + it). +- Run `cargo fmt` across the whole `src/` tree as a single, + non-functional formatting commit (don't mix with logic changes, to + keep the diff reviewable and `git blame`-friendly). +- Add a `cargo fmt --check` step to `make lint` (or a new `make + fmt-check` wired into CI) so future changes can't drift out of + format again. diff --git a/tickets/095-enforce-clippy-clean.md b/tickets/095-enforce-clippy-clean.md new file mode 100644 index 0000000..82b3374 --- /dev/null +++ b/tickets/095-enforce-clippy-clean.md @@ -0,0 +1,41 @@ +# 095 — Resolve clippy warnings and make lint enforce a clean run + +## Summary + +`make lint`'s backend target runs `cargo clippy --all-targets` with no +`-D warnings` (or equivalent), so clippy warnings accumulate silently +instead of failing the lint step. There are currently 23 unique +warnings across 5 lint categories. + +## Where + +- `Makefile:26` (`lint-be:`) — `cargo clippy --all-targets`, no denial + flag. +- `cargo clippy --all-targets` currently reports, by lint: + - `clippy::cloned_ref_to_slice_refs` — 10 occurrences, mostly in + `src/engine/alerts.rs` test code (e.g. around lines 469, 472, 493, + 534). + - `clippy::collapsible_if` — 8 occurrences, concentrated in + `src/engine/snapshot/mod.rs` (lines 608, 940, 961) and + `src/engine/alerts.rs` (around lines 510, 512). + - `clippy::type_complexity` — 3 occurrences. + - `clippy::single_match` — 1 occurrence. + - `clippy::unnecessary_get_then_check` — 1 occurrence, + `src/engine/tests.rs:2234` + (`entries[1].params.get("level").is_none()`). + +## Scope + +- Fix all 23 current warnings (mechanical for most: `from_ref` for the + slice-ref clone cases, collapsing nested `if`s, `is_none()` → + `!contains_key`/`get().is_none()` equivalent simplification per + clippy's suggestion, breaking up the complex types clippy flags or + adding a type alias, converting the single-arm `match` to `if let`). +- Once clean, change `Makefile:26` to deny warnings (e.g. `cargo + clippy --all-targets -- -D warnings`) so the target fails on any + future regression instead of just printing them. +- Don't add blanket `#[allow(...)]`s to silence these — fix the + underlying code per clippy's suggestion, matching the existing + `#[allow(dead_code)]` uses elsewhere in the codebase which are for + genuinely-unused-for-now items, not a precedent for suppressing + active lints. diff --git a/tickets/096-clap-short-flags.md b/tickets/096-clap-short-flags.md new file mode 100644 index 0000000..4fbb6a1 --- /dev/null +++ b/tickets/096-clap-short-flags.md @@ -0,0 +1,34 @@ +# 096 — Add single-character short flags to clap CLI args + +## Summary + +None of the CLI's clap-defined flags have a `short` form — every +option is long-only, including ones that are typed often +(`--file`, `--metrics-url`). + +## Where + +- `src/cli/mod.rs:20-39` — top-level `Args` struct, all fields + `#[arg(long, ...)]` with no `short`: + - `port: u16` — `:25-26`, `default_value_t = 0` + - `settings_path: Option` — `:28-29` + - `file: Option` — `:31-32` + - `metrics_url: Option` — `:34-35` + - `name: Option` — `:37-38` +- `src/cli/mod.rs:41-50` — `Command::Mcp` subcommand, also long-only: + - `session: Option` — `:44-45` + - `port: Option` — `:46-47` + +## Scope + +- Add `short` to each flag where the initial letter is unambiguous + within its own `Args`/subcommand struct: `-p` (port), `-f` (file), + `-m` (metrics-url), `-n` (name), `-s` (settings-path or session, + whichever reads more naturally per subcommand — they're in separate + structs so both can use `-s` without conflict). +- `Mcp`'s `port` can reuse `-p` since it's a distinct struct from the + top-level `Args`. +- Verify with `cargo run -- --help` (and the `mcp` subcommand's + `--help`) that clap doesn't reject any of these for collision, and + update `README.md`'s CLI usage section if it documents flags + explicitly. diff --git a/tickets/097-metric-gauge-counter-semantics.md b/tickets/097-metric-gauge-counter-semantics.md new file mode 100644 index 0000000..5b4b57f --- /dev/null +++ b/tickets/097-metric-gauge-counter-semantics.md @@ -0,0 +1,50 @@ +# 097 — Metrics have no gauge/counter distinction, so `rate()`/`sum()`/`avg()` can be applied without a semantics guard + +## Summary + +`MetricPrimitiveKind` only distinguishes data *type* +(`String`/`Number`/`Timestamp`/`Duration`/`Boolean`), not metric +*semantics* (counter vs. gauge). PromQL's `gauge()` op is a bespoke +query-time "take the last point" function rather than something tied +to how a metric was declared, and nothing stops calling `rate()` on a +metric that's conceptually a gauge, or `sum()`/`avg()` on a +monotonically-increasing counter without rating it first. + +## Where + +- `src/engine/data_value.rs:70-76` — `MetricPrimitiveKind`: + `String, Number, Timestamp, Duration, Boolean`. No `Counter`/`Gauge` + variant. +- `src/engine/metric_primitive.rs:8-14` — `MetricPrimitive` mirrors + the same type-only distinction (`String, Number(Number), Timestamp, + Duration(i64), Boolean`). +- `src/engine/promql.rs:9-16` — `AggOp` includes `Gauge` as one of six + query-time aggregation ops (`Sum, Avg, Count, Rate, Histogram, + Gauge`), evaluated in `src/engine/snapshot/mod.rs:993-1001` by + taking only `points.last()` per series — a query-side convenience, + not a property of the declared metric. +- The only type guard on aggregation today is + `src/engine/snapshot/mod.rs:888-896`, which rejects `sum`/`avg` on + `MetricPrimitiveKind::String` — there's no equivalent guard for + applying `rate()` to a non-monotonic value or `sum()`/`avg()` to an + un-rated counter. + +## Scope + +- Add a semantics field to metric declarations (e.g. extend + `MetricPrimitiveKind` with `Counter`/`Gauge` variants, or add a + separate `MetricSemantics` enum alongside the existing type field — + decide which based on whether type and semantics should ever vary + independently for the same declaration). +- Thread that through `DeclareMetricRequest`/`MetricDeclarationRecord`/ + `MetricDeclarationDto` (the same three-places pattern noted in this + repo's `CLAUDE.md` for declaration features: `engine.rs` message + + handler, `settings.rs`/`profile.rs` persistence, `api.rs` route, + plus a webapp UI affordance in `Params.tsx`/`Metrics`'s declaration + form). +- Use it to validate PromQL queries at evaluation time (e.g. warn or + reject `rate()` on a declared gauge, or `sum()`/`avg()` directly on a + declared counter without an inner `rate()`), and consider whether + the current ad hoc `gauge()` PromQL wrapper becomes redundant once + the declaration itself carries this information (a bare selector on + a declared gauge could just work the way `gauge()` does today). diff --git a/tickets/098-histogram-quantile-support.md b/tickets/098-histogram-quantile-support.md new file mode 100644 index 0000000..2ce3d43 --- /dev/null +++ b/tickets/098-histogram-quantile-support.md @@ -0,0 +1,41 @@ +# 098 — No histogram_quantile()-style percentile query + +## Summary + +The PromQL layer's `histogram()` op buckets raw sample values by a +fixed width and returns per-bucket counts — it's unrelated to +Prometheus's classic histogram bucket/quantile model, and there's no +way to ask for p50/p95/p99 out of a metric today. + +## Where + +- `src/engine/promql.rs:9-16` — `AggOp::Histogram` is one of the six + supported ops. +- `src/engine/snapshot/mod.rs:1002` onward — `Histogram` evaluation: + buckets values by `bucket_width` into counts, grouped by + `by_labels`. This answers "how many samples fell in each bucket", + not "what value is the Nth percentile". +- No `histogram_quantile()` function or equivalent exists anywhere in + `promql.rs`'s parser (`parse_query`, `:261-324` recognizes only + `sum`, `avg`, `count`, `rate`, `histogram`, `gauge`). +- Depends on ticket 097 (gauge/counter semantics) for the broader + question of whether a metric needs a declared "this holds raw + observations for quantile purposes" flag, but quantile computation + itself doesn't strictly require that — it can operate on whatever + raw values are already stored in a metric's buckets. + +## Scope + +- Decide the quantile computation model: interpolate over the raw + stored sample values in a bucket/window (simplest, exact for the + data actually retained) vs. building a genuine bucketed-histogram + representation with configurable bucket boundaries (closer to + Prometheus's classic histograms, more work, needed only if retained + raw values are later dropped in favor of pre-aggregated buckets). +- Add a `histogram_quantile(p, metric[range])`-shaped PromQL construct + (or extend `histogram()` with a quantile argument) to + `promql.rs`'s parser and `AggOp`, plus an evaluator in + `snapshot/mod.rs` alongside the existing `Histogram` handling. +- Add dashboard/panel UI support for selecting a quantile once the + query-language side exists (`webapp/src/components/Dashboards.tsx` + or wherever panel query construction lives). diff --git a/tickets/099-remove-test-handle-stub.md b/tickets/099-remove-test-handle-stub.md new file mode 100644 index 0000000..cdda94e --- /dev/null +++ b/tickets/099-remove-test-handle-stub.md @@ -0,0 +1,45 @@ +# 099 — Remove `test_handle`'s duplicate-of-`spawn` test stub + +## Summary + +`test_handle` is a `#[cfg(test)]` production-file function that exists +only to give tests access to the engine's internal `mpsc::Receiver` — +it duplicates `spawn()` almost line-for-line rather than exposing that +receiver through `spawn()`/`EngineHandle` itself. Per this repo's +`IDEAS.md` guidance: prefer changing a function's scope/visibility (or +the tests) over maintaining a parallel stub. + +## Where + +- `src/engine/handle.rs:113-128` — `#[cfg(test)] pub(crate) fn + test_handle(channel_capacity: usize) -> (EngineHandle, + mpsc::Receiver)`, duplicating `spawn()` + (`src/engine/handle.rs:103-111`) except that `spawn()` discards the + receiver after wiring the actor loop, while `test_handle` returns it + raw. +- Re-exported at `src/engine/mod.rs:21`: `pub(crate) use + handle::test_handle;`. +- Consumed only by tests in `src/engine/ingest.rs:228` (`use + crate::engine::test_handle;`) and called at lines 248, 264, 281, 302 + inside `#[cfg(test)] mod tests`. + +## Scope + +- Remove `test_handle` and instead give tests access via `spawn()` + itself — e.g. have `spawn()` take an optional channel-capacity + override and always hand back the receiver alongside the handle (or + the actor join handle), with production call sites (`main.rs`) + simply ignoring what they don't need, rather than keeping a second + constructor whose only difference is what it returns. +- Update the four call sites in `src/engine/ingest.rs`'s test module + to use the adjusted `spawn()` signature. +- Out of scope: the other test-fixture helpers in `src/engine/tests.rs` + (`metric_request`, `declare_test_param`, `ingest_log`, `logs_query`, + `logs_query_sorted`, etc.) are ordinary test-data builders, not + visibility workarounds — leave those alone. Also leave the + `#[allow(dead_code)]` items (`src/engine/message.rs:11,29`, + `src/api/mcp_server.rs:254`, `src/config/mod.rs:27`, and similar in + `metric_primitive.rs`/`data_value.rs`/`timestamp.rs`/`index_key.rs`) + untouched — they weren't confirmed to be test-visibility stubs and + need individual inspection before deciding whether they're even + related to this ticket's pattern.