Add tickets and ideas
This commit is contained in:
@@ -2,3 +2,5 @@
|
|||||||
target/
|
target/
|
||||||
.claude/worktrees/
|
.claude/worktrees/
|
||||||
profile.json.gz
|
profile.json.gz
|
||||||
|
.codex
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,65 @@
|
|||||||
|
# AGENTS.md
|
||||||
|
|
||||||
|
<!-- Add your project instructions above or below the Repowise section. Repowise only updates the managed section between markers. -->
|
||||||
|
|
||||||
|
<!-- REPOWISE_AGENTS:START — Do not edit below this line. Auto-generated by Repowise. -->
|
||||||
|
## 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 <cmd>`, the same command with its exit code preserved and errors-first output. A `[repowise#<ref>: N lines omitted]` marker is recoverable via `repowise expand <ref>` (add `-q <regex>` 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#<hex>`). 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`
|
||||||
|
|
||||||
|
<!-- REPOWISE_AGENTS:END -->
|
||||||
@@ -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
|
|
||||||
|
|
||||||
|
|
||||||
@@ -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<EngineMessage>` (`: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<IndexKey, DataValue>` (`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<String, AlertRuntimeState>` 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<str>` (or
|
||||||
|
`Arc<String>`) 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.
|
||||||
@@ -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<String>`.
|
||||||
|
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"?
|
||||||
@@ -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.
|
||||||
@@ -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.
|
||||||
@@ -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.
|
||||||
@@ -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.
|
||||||
@@ -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.
|
||||||
@@ -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.
|
||||||
@@ -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<PathBuf>` — `:28-29`
|
||||||
|
- `file: Option<PathBuf>` — `:31-32`
|
||||||
|
- `metrics_url: Option<String>` — `:34-35`
|
||||||
|
- `name: Option<String>` — `:37-38`
|
||||||
|
- `src/cli/mod.rs:41-50` — `Command::Mcp` subcommand, also long-only:
|
||||||
|
- `session: Option<String>` — `:44-45`
|
||||||
|
- `port: Option<u16>` — `: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.
|
||||||
@@ -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).
|
||||||
@@ -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).
|
||||||
@@ -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<EngineMessage>)`, 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.
|
||||||
Reference in New Issue
Block a user