Update actor split idea file

This commit is contained in:
Hugh Kaznowski
2026-09-07 13:14:19 +01:00
parent c7173cf9dc
commit 5fee6909f2
3 changed files with 99 additions and 76 deletions
+2 -2
View File
@@ -5,7 +5,7 @@
<!-- REPOWISE_AGENTS:START — Do not edit below this line. Auto-generated by Repowise. --> <!-- REPOWISE_AGENTS:START — Do not edit below this line. Auto-generated by Repowise. -->
## Codebase Intelligence for ottyl (Repowise) ## Codebase Intelligence for ottyl (Repowise)
Indexed by [Repowise](https://repowise.dev). Last indexed: 2026-09-07 (commit dbe2ea9). Confidence: 100%. Indexed by [Repowise](https://repowise.dev). Last indexed: 2026-09-07 (commit c7173cf). Confidence: 100%.
### How to work in this repo ### How to work in this repo
- **Trust the index.** `verified: true` and `_meta.complete` mean the bytes were checked against the live tree, so never re-read them. Re-read only what `bounds: "approximate"` or `_meta.stale_warning` names. `confidence` rates the prose, not the evidence: on `low` read the `fallback_targets` or `best_guesses` the reply names, and run `repowise update` and ask again if `_meta.hint` says the index is behind HEAD. `index_behind: true` alone is informational. - **Trust the index.** `verified: true` and `_meta.complete` mean the bytes were checked against the live tree, so never re-read them. Re-read only what `bounds: "approximate"` or `_meta.stale_warning` names. `confidence` rates the prose, not the evidence: on `low` read the `fallback_targets` or `best_guesses` the reply names, and run `repowise update` and ask again if `_meta.hint` says the index is behind HEAD. `index_behind: true` alone is informational.
@@ -70,7 +70,7 @@ Covers the 3 source files in 2 directories under…
- `webapp/src/components/Docs.tsx` — 1 bug fix, last fix today; 17 commits/90d - `webapp/src/components/Docs.tsx` — 1 bug fix, last fix today; 17 commits/90d
### Code health ### Code health
Three co-equal signals: defect risk 5.95/10 avg, hotspot health 4.94/10 (stable), worst `src/engine/snapshot/mod.rs` at 2.0/10 · maintainability 7.48/10 · performance risk 16 open static I/O-in-loop / N+1 findings. Detail: `get_health()`. Three co-equal signals: defect risk 5.95/10 avg, hotspot health 4.94/10 (declining), worst `src/engine/snapshot/mod.rs` at 2.0/10 · maintainability 7.48/10 · performance risk 16 open static I/O-in-loop / N+1 findings. Detail: `get_health()`.
Critical files: Critical files:
- `webapp/src/components/LogPreview.tsx` — untested hotspot — impact −2.0 - `webapp/src/components/LogPreview.tsx` — untested hotspot — impact −2.0
+2
View File
@@ -43,3 +43,5 @@ Frontend structure: `webapp/src/App.tsx` is the shell; `webapp/src/components/`
- **`tickets/`** holds scoped, written-up work items (problem statement, pointers to exact call sites with `file.rs:line` references, and an explicit scope section) — check here for planned/in-flight work before starting something that might overlap. Tickets are removed from this directory once implemented (see recent commit history — e.g. "Remove ticket 047 (implemented: ...)"). - **`tickets/`** holds scoped, written-up work items (problem statement, pointers to exact call sites with `file.rs:line` references, and an explicit scope section) — check here for planned/in-flight work before starting something that might overlap. Tickets are removed from this directory once implemented (see recent commit history — e.g. "Remove ticket 047 (implemented: ...)").
- Declaration-related features (params, metrics) consistently need three coordinated changes: an `EngineMessage` variant + handler in `engine.rs`, a persistence method in `settings.rs`/`profile.rs`, and an `api.rs` route wiring them together (plus a webapp UI affordance). Grep for an existing similar declaration (e.g. how metric declaration is wired) as the template before adding a new one. - Declaration-related features (params, metrics) consistently need three coordinated changes: an `EngineMessage` variant + handler in `engine.rs`, a persistence method in `settings.rs`/`profile.rs`, and an `api.rs` route wiring them together (plus a webapp UI affordance). Grep for an existing similar declaration (e.g. how metric declaration is wired) as the template before adding a new one.
- Doc comments in this codebase tend to explain *why* a non-obvious choice was made (e.g. ordering constraints, a workaround, a past ticket's rationale) rather than restating what the code does — match that style rather than adding descriptive comments. - Doc comments in this codebase tend to explain *why* a non-obvious choice was made (e.g. ordering constraints, a workaround, a past ticket's rationale) rather than restating what the code does — match that style rather than adding descriptive comments.
- All markdown files (`*.md`) in this repo should follow one-sentence-per-line: each sentence starts on its own line, even within the same paragraph.
+95 -74
View File
@@ -26,6 +26,54 @@
> Values in the primary index should be arcs, to prevent cloning > Values in the primary index should be arcs, to prevent cloning
> allocations. > allocations.
## Resolved decisions (2026-09-07)
Clarified interactively; supersedes the open questions raised below where
they conflict.
- **No profiling gate.** Proceeding on the code-reading evidence already in
this doc (double/triple string clones per line, full-`EngineSnapshot`
deep-clone on every `left_right` reconciliation) rather than waiting on a
`samply` profile first.
- **Three actors, not two.** The split is: **reader actor** (tails
stdin/file, splits into lines, timestamps, wraps each line in an `Arc`,
sends downstream) → **primary actor** (ingests timestamped `Arc` lines,
extracts declared + auto params only, maintains the `param=value` index —
no metrics, no profile logic) → **profile actor** (holds its own
`left_right` reader over the primary's index; builds metrics, custom
params, timeseries, and PromQL evaluation for the one active profile).
This resolves the "per-profile message format" question below: there is
no per-profile parsing fork on the primary — format/shape parsing of an
already-classified line is entirely the profile actor's concern, on top
of the shared raw text.
- **Still exactly one active profile at a time.** No concurrent
multi-profile indexing; the split changes *how* the one active profile's
data is computed relative to the primary, not the user-facing
single-active-profile model.
- **Alerts move to the profile actor.** Alert rules are profile-scoped and
read metric buckets, so they move with metric computation rather than
staying on the primary.
- **Primary→profile signaling:** the primary actor notifies subscribers
(e.g. a `tokio::sync::watch` carrying the latest processed
position/index) right after each `left_right` sync completes. The
profile actor wakes on that signal and re-reads whatever's newly visible
through its own `left_right` reader — no busy-polling, no second copy of
the data in transit.
- **Eviction wins over profile-actor lag.** If the profile actor is
behind (e.g. mid-backfill on a newly declared metric) when the primary
evicts old lines (`SetMaxLogs`), the primary evicts on schedule
regardless — the profile actor silently misses data for the evicted
range. Simplest option, matches today's single-actor retention model.
- **System param suppressions: dropped from scope.** Not needed right now;
revisit as a separate idea if it becomes a real requirement.
- **Migration path: incremental**, in this order:
1. Arc-ify stored values (`Arc<str>`/`Arc<String>` for log text and
metric-captured strings) — independent win, and the load-bearing
precondition for cheap cross-actor reads in steps 2–3.
2. Extract metric computation into a separately-callable stage, still
within the single actor.
3. Split into the reader/primary/profile actors described above.
## Current architecture (confirmed in code) ## Current architecture (confirmed in code)
- **One process, one actor, today.** `spawn_engine` - **One process, one actor, today.** `spawn_engine`
@@ -188,12 +236,16 @@
`left_right` pair. Splitting introduces: a defined "primary `left_right` pair. Splitting introduces: a defined "primary
perspective" (raw logs + derived/auto params only, no perspective" (raw logs + derived/auto params only, no
profile-specific metrics) as its own bounded module, a profile-specific metrics) as its own bounded module, a
"profile perspective" abstraction that can be instantiated per "profile perspective" abstraction for the one active profile, a
active profile, a notification/subscription mechanism from primary notification/subscription mechanism from primary to the profile actor
to profile workers (the note's "profiles need to be informed of new (the note's "profiles need to be informed of new messages to index" —
messages to index" — doesn't exist in any form today; the closest doesn't exist in any form today; the closest analogue, `ingest.rs`'s
analogue, `ingest.rs`'s producer tasks feeding one consumer, runs the producer tasks feeding one consumer, runs the opposite direction).
opposite direction), and a query-fan-out/merge layer for any request **Resolved** (see "Resolved decisions"): the primary signals via e.g. a
`tokio::sync::watch` after each `left_right` sync completes, and the
profile actor re-reads through its own `left_right` reader rather than
the primary pushing data. A query-fan-out/merge layer is still needed
for any request
that needs both raw logs and profile-derived data in one response that needs both raw logs and profile-derived data in one response
(today `Logs`/`Timeline` queries read both from a single (today `Logs`/`Timeline` queries read both from a single
`EngineSnapshot` in one pass, e.g. `query.rs:154`'s `EngineSnapshot` in one pass, e.g. `query.rs:154`'s
@@ -202,10 +254,9 @@
across two snapshots/workers instead of one borrow). across two snapshots/workers instead of one borrow).
- Alert evaluation (`handle.rs:199-211`) currently depends on running - Alert evaluation (`handle.rs:199-211`) currently depends on running
immediately after metric bucketing in the same message, against the immediately after metric bucketing in the same message, against the
same snapshot. If metrics move to per-profile workers, alerts need to same snapshot. **Resolved** (see "Resolved decisions"): alerts move
either move with them (probably correct, since alert rules are with metric computation into the profile actor, since alert rules are
profile-scoped conceptually) or gain a defined way to read profile-scoped and read the metric buckets computed there.
cross-worker metric state that doesn't exist today.
- `EngineHandle`'s current API (`handle.rs:49-73` for reads, - `EngineHandle`'s current API (`handle.rs:49-73` for reads,
the mpsc `EngineMessage` send for writes) would need to become two the mpsc `EngineMessage` send for writes) would need to become two
APIs, or one handle type parameterized by "primary" vs. "profile" — APIs, or one handle type parameterized by "primary" vs. "profile" —
@@ -222,75 +273,45 @@
### Functionality ### Functionality
- **Per-profile message format** (the note's "What if message needs to - **Per-profile message format** — **resolved**, see "Resolved decisions"
be different format? Thats per-profile.") — today, log-stream header above: there's no per-profile parsing fork on the primary. Log-stream
detection and logfmt/JSON auto-param extraction happen once, in the header detection and auto-param extraction stay a one-time, primary-side,
primary, against the raw line (`derive_log_stream_params_from_row`, profile-agnostic concern (`derive_log_stream_params_from_row`,
`auto_declare_params`, both in the current `Ingest` arm). If `auto_declare_params`); any further message-shape interpretation the
different profiles can expect genuinely different message shapes profile actor needs happens independently, on top of the shared raw
from the *same* piped process (unclear from the note whether this `Arc` text, in the profile actor itself.
means "different profiles parse the same stream differently" or - **System param suppressions** — **dropped from scope** (see "Resolved
"each profile only ever sees its own stream"), that's a real design decisions"). Revisit as a separate idea if it becomes a real need.
fork: either the primary's parsing must become profile-aware (which - **Concurrency** — **resolved**: still strictly one active profile at a
undercuts "primary only extracts params" as a clean, profile-agnostic time. The split changes *how* that one profile's data is computed
boundary), or each profile worker does its own message-format relative to the primary, not the user-facing model — no multi-profile
parsing independently on top of the shared raw text — the latter fits concurrent indexing.
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 ### Not yet considered
- **Failure/restart semantics**: if a profile worker crashes or lags - **Failure/restart semantics** — partially resolved: the profile actor
behind the primary, what's the recovery story? Today there's one falling behind the primary during a backfill is expected and accepted
actor with one failure mode (the whole engine). A split needs an (see "Resolved decisions" — eviction wins over lag, silent gaps for the
answer for "profile worker is behind — does it catch up from the evicted range). Still open: what happens if the profile actor *crashes*
primary's retained history (bounded by the same retention limit the outright (not just lags) — does it restart and replay from whatever the
primary enforces), or is data permanently lost for the gap?" primary currently retains, or is there a persisted checkpoint? Not yet
- **Retention/eviction coordination**: the primary already evicts old decided; likely fine to punt until the split is actually implemented,
logs (`evict_excess_logs`, `handle.rs:195`, `SetMaxLogs` message). since "restart and rebuild from currently-retained history" is a
If a profile worker is reading "the primary log sequence" lazily or reasonable default consistent with the accepted-gaps decision above.
asynchronously, eviction on the primary could remove a line before a - **Retention/eviction coordination** — **resolved**, see "Resolved
lagging profile worker has processed it — there's no coordination decisions": no low-water-mark. The primary evicts on its own schedule
primitive today (like a per-consumer low-water-mark) to prevent this (`evict_excess_logs`, `handle.rs:195`, `SetMaxLogs`) regardless of the
silently dropping data for a slow profile worker. profile actor's progress; a lagging profile actor silently misses data
for the evicted range rather than blocking primary eviction.
- **Testing story**: `src/engine/tests.rs` (2348 lines) and the - **Testing story**: `src/engine/tests.rs` (2348 lines) and the
`test_handle` pattern (see ticket 099) are built entirely around one `test_handle` pattern (see ticket 099) are built entirely around one
actor with one channel — a split roughly doubles the kinds of actor with one channel — a split roughly doubles the kinds of
integration tests needed (primary alone, profile worker alone, integration tests needed (primary alone, profile worker alone,
primary+profile interaction) and the existing test harness would primary+profile interaction) and the existing test harness would
need real rework, not just extension. need real rework, not just extension. Still open — no resolution yet.
- **Migration path**: this is a rewrite of the highest-risk, largest, - **Migration path** — **resolved**, see "Resolved decisions": incremental,
most-churned file in the codebase (per `CLAUDE.md`'s repowise in three steps (Arc-ify stored values → extract metric computation into
summary) — no plan yet for whether this ships as one big change or a still-single-actor stage → split into reader/primary/profile actors).
incrementally (e.g. Arc-ing values first as an independent, - **Whether this is worth it at all without profiling evidence** —
lower-risk step; then extracting metric computation into a **resolved**, see "Resolved decisions": proceeding without a `samply`
separately-callable-but-still-single-actor stage; then finally profile, on the code-reading evidence already documented in this file.
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.