297 lines
17 KiB
Markdown
297 lines
17 KiB
Markdown
# 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.
|