Break ticket 102's actor split into reviewable sub-tickets (103-108)
Ticket 102 (splitting the engine into reader/primary/profile actors) was a single multi-thousand-line ticket covering a new actor topology, an EngineHandle API redesign, a query fan-out layer, moving alerts, and a full test-harness rewrite, with some design decisions deliberately left open. Splitting it into six sequenced tickets (profile actor skeleton + signaling, move metric computation, move alert evaluation, split the handle API, query fan-out, test harness rework) lets each land as its own reviewable commit instead of one large speculative rewrite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TeyhRfMYjMditek6geDRoQ
This commit is contained in:
Hugh Kaznowski
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
756e054527
commit
2c1aaa6e9c
@@ -1,107 +0,0 @@
|
||||
# 102 — Split the single engine actor into reader, primary, and profile actors
|
||||
|
||||
## Summary
|
||||
|
||||
Today, one actor (`run()` in `src/engine/handle.rs`) does everything:
|
||||
tailing/ingest bookkeeping, raw log storage, param extraction, *every*
|
||||
declared metric's regex evaluation, sort-helper indexing, eviction, and
|
||||
alert evaluation, all serialized through one `mpsc` channel. This is
|
||||
migration step 3 (the actual split) of [[0001-actor-split]]'s resolved
|
||||
plan, and depends on [[100-arc-ify-stored-log-and-metric-strings]] and
|
||||
[[101-extract-metric-computation-into-separate-stage]] landing first —
|
||||
this ticket is the wiring/topology change those two steps prepare for,
|
||||
not a from-scratch redesign. See [[0001-actor-split]]'s "Resolved
|
||||
decisions" section for the full rationale and answers to the open
|
||||
questions below; this ticket's scope is that design, made concrete.
|
||||
|
||||
## Where
|
||||
|
||||
- `src/engine/handle.rs:130-365` (`run()`) — the single actor loop to
|
||||
be split. The `EngineMessage::Ingest` arm (:143-219) is the part that
|
||||
becomes the reader→primary boundary; `apply_all_metrics` /
|
||||
`apply_metric_to_log` (`src/engine/snapshot/mod.rs:616`, :499-586)
|
||||
and alert evaluation (`src/engine/alerts.rs:211`, called from
|
||||
`handle.rs:199-219`) are what moves to the new profile actor.
|
||||
- `src/cli/mod.rs:95` (`spawn_engine`) and `:105-124`
|
||||
(`load_active_profile`) — the sole production call site of
|
||||
`engine::spawn`; `load_active_profile`'s existing comment ("there's
|
||||
no way to reprocess earlier lines later, so this must happen before
|
||||
`spawn_ingest`", `:104-106`) documents a constraint the new topology
|
||||
must preserve for the primary actor's startup ordering.
|
||||
- `src/engine/ingest.rs` (stdin/file-tailing + Prometheus-polling
|
||||
tasks, spawned at `src/cli/mod.rs:136-145`) — today's only
|
||||
multi-task precedent, producer-side only (multiple producers into one
|
||||
actor's channel); the new reader actor sits upstream of this or
|
||||
subsumes part of it, per [[0001-actor-split]]'s three-actor design.
|
||||
- `src/api/mod.rs:712-768` (`activate_profile` /
|
||||
`reload_engine_declarations`) — the runtime profile-switch path that
|
||||
currently removes-then-redeclares against the single actor; needs to
|
||||
target the new profile actor instead. See [[0004-profile-switch-reindexing-cost]]
|
||||
for the existing cost analysis of this path, which doesn't disappear
|
||||
with this split but moves and stops blocking the primary.
|
||||
- `src/engine/tests.rs` (2348 lines) and the `test_handle` pattern
|
||||
(see ticket 099's removal of the duplicate `test_handle` stub) —
|
||||
built around one actor with one channel; will need real rework, not
|
||||
just extension, to cover primary alone, profile actor alone, and
|
||||
primary+profile interaction.
|
||||
|
||||
## Scope
|
||||
|
||||
Implement the three-actor topology exactly as resolved in
|
||||
[[0001-actor-split]]:
|
||||
|
||||
- **Reader actor**: tails stdin/file, splits into lines, timestamps
|
||||
them, wraps each line in an `Arc` (per [[100-arc-ify-stored-log-and-metric-strings]]),
|
||||
sends downstream.
|
||||
- **Primary actor**: ingests timestamped `Arc` lines, extracts declared
|
||||
+ auto params only (no metrics, no profile logic), maintains the
|
||||
`param=value` index. Owns retention/eviction (`SetMaxLogs`) on its
|
||||
own schedule — a lagging profile actor does not block primary
|
||||
eviction; it silently misses data for the evicted range (accepted
|
||||
tradeoff, not a bug to fix here).
|
||||
- **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. Owns alert evaluation
|
||||
(moved from the primary per the resolved decision that alerts are
|
||||
profile-scoped). Still exactly one active profile at a time — no
|
||||
concurrent multi-profile indexing; this changes *how* the active
|
||||
profile's data is computed, not the single-active-profile user model.
|
||||
- **Primary→profile signaling**: a `tokio::sync::watch` (or equivalent)
|
||||
that the primary notifies right after each `left_right` sync
|
||||
completes; the profile actor wakes on that signal and re-reads
|
||||
through its own `left_right` reader — no busy-polling, no second data
|
||||
copy in transit.
|
||||
- **Query fan-out**: a layer for requests that need both raw-log and
|
||||
profile-derived data in one response (e.g. `Logs`/`Timeline`
|
||||
queries, which today read both from a single `EngineSnapshot` in one
|
||||
pass — see `query.rs`'s `log_entry_params` call against
|
||||
`compiled_params`) — this becomes a real join across two
|
||||
snapshots/actors.
|
||||
- **`EngineHandle` API**: split into two handle types (primary vs.
|
||||
profile), or one combined handle that internally fans out — decide
|
||||
during implementation which `api.rs`'s `ApiState` should hold; either
|
||||
is acceptable per [[0001-actor-split]], this ticket doesn't mandate
|
||||
one.
|
||||
- Rework `src/engine/tests.rs` and the `test_handle` pattern to cover
|
||||
the new topology (primary alone, profile actor alone,
|
||||
primary+profile interaction) — explicitly in scope, not deferred,
|
||||
since the existing single-actor test harness cannot exercise the new
|
||||
split at all.
|
||||
- Update `src/api/mod.rs:712-768`'s `activate_profile` /
|
||||
`reload_engine_declarations` to address the profile actor.
|
||||
|
||||
Out of scope (per [[0001-actor-split]]'s resolved decisions —
|
||||
explicitly deferred, not silently dropped):
|
||||
|
||||
- System param suppressions — dropped from scope entirely; revisit as
|
||||
a separate idea if it becomes a real requirement.
|
||||
- Full-text/regex indexing on the primary — a materially different
|
||||
investment ([[0005-additional-read-indexes]]), doesn't depend on
|
||||
actor topology.
|
||||
- Profile-actor crash/restart checkpointing beyond "restart and
|
||||
rebuild from whatever the primary currently retains" — no persisted
|
||||
checkpoint required for this ticket.
|
||||
- Diff-based or async profile-switch reload
|
||||
([[0004-profile-switch-reindexing-cost]]'s options) — this ticket
|
||||
only needs `activate_profile` to target the right actor, not to
|
||||
change the switch's synchronous-rebuild cost model.
|
||||
@@ -0,0 +1,67 @@
|
||||
# 103 — Stand up the profile actor skeleton and primary→profile signaling, with no behavior moved yet
|
||||
|
||||
## Summary
|
||||
|
||||
First slice of [[0001-actor-split]]'s step 3 (splitting the single
|
||||
engine actor into reader/primary/profile actors). This ticket is pure
|
||||
plumbing: a second actor task with its own `left_right` write side for
|
||||
profile-owned state, a way for it to read the primary's index
|
||||
(`EngineHandle`'s `IndexReader` is already `Clone`, so no primary-side
|
||||
change is needed to hand out a read-only view), and a
|
||||
`tokio::sync::watch` the primary signals on after each `left_right`
|
||||
sync so the profile actor wakes without busy-polling. No metric
|
||||
computation, alert evaluation, or query routing moves in this ticket —
|
||||
that's [[104-move-metric-computation-to-profile-actor]] and
|
||||
[[105-move-alert-evaluation-to-profile-actor]]. Depends on
|
||||
[[101-extract-metric-computation-into-separate-stage]] (done) for the
|
||||
`ingest_params`/`apply_all_metrics` seam this split will eventually
|
||||
route across two actors.
|
||||
|
||||
## Where
|
||||
|
||||
- `src/engine/handle.rs:33-47` (`IndexWriter`) — `mutate()` calls
|
||||
`self.handle.append(...)` then `self.handle.publish()`; this is
|
||||
where the new watch-sender notification hooks in, right after
|
||||
`publish()`.
|
||||
- `src/engine/handle.rs:48-73` (`IndexReadHandle`/`IndexReader`) — the
|
||||
existing read-side pattern to mirror for the profile actor's own
|
||||
derived state; `IndexReader` is already `Clone`
|
||||
(`handle.rs:61-67` is a template, not something to reuse directly —
|
||||
the profile actor needs its own `left_right` pair for its own
|
||||
state, plus a cloned primary `IndexReader` for read access).
|
||||
- `src/engine/handle.rs:74-78` (`EngineHandle`) and `:119-124`
|
||||
(`spawn`) — the shape to parallel for a new profile-actor spawn
|
||||
function.
|
||||
- `src/engine/snapshot/mod.rs:208-255` (`logs_range`/
|
||||
`logs_in_sequence_range`) — the read-side primary API the profile
|
||||
actor will use (via its cloned `IndexReader`) to pull newly-visible
|
||||
lines after waking on the watch signal.
|
||||
|
||||
## Scope
|
||||
|
||||
- Add a `ProfileSnapshot` type (new module, e.g.
|
||||
`src/engine/profile.rs`) with its own `left_right` write/read pair,
|
||||
parallel to `EngineSnapshot`'s pattern in `handle.rs` — for now it
|
||||
can be effectively empty (a placeholder struct), since no state
|
||||
moves into it yet.
|
||||
- Add a `tokio::sync::watch<u64>` (carrying, e.g., the primary's
|
||||
latest processed sequence) that `IndexWriter::mutate` sends on after
|
||||
`publish()` succeeds.
|
||||
- Add a profile actor task: holds a cloned primary `IndexReader` plus
|
||||
its own `ProfileSnapshot` write side, wakes on the watch channel,
|
||||
and — for this ticket only — reads whatever new lines
|
||||
`logs_in_sequence_range` exposes since its last-seen sequence and
|
||||
does nothing with them (e.g. just advances its own watermark). The
|
||||
goal is proving the wake/read loop works end-to-end, not doing real
|
||||
work with the data yet.
|
||||
- Wire `spawn` (`src/cli/mod.rs:95`) to also spawn the profile actor
|
||||
alongside the primary, but do not yet change what `ApiState` holds
|
||||
or what any API handler talks to — `EngineHandle` and its public API
|
||||
stay exactly as they are today. That's
|
||||
[[106-split-enginehandle-api-for-primary-and-profile]]'s concern.
|
||||
- Out of scope: moving `apply_all_metrics`, metric declarations, or
|
||||
alert evaluation; changing `EngineHandle`'s public surface; eviction
|
||||
coordination beyond what already exists (primary still evicts on its
|
||||
own schedule per [[0001-actor-split]]'s resolved decision — this
|
||||
ticket doesn't need to handle the profile actor falling behind
|
||||
eviction yet, since it isn't consuming anything meaningful).
|
||||
@@ -0,0 +1,73 @@
|
||||
# 104 — Move metric declarations and bucket computation into the profile actor
|
||||
|
||||
## Summary
|
||||
|
||||
Second slice of the split: metric declarations, `apply_all_metrics`,
|
||||
and the declare/update/remove/backfill machinery move off the primary
|
||||
`EngineSnapshot` and onto the profile actor's `ProfileSnapshot`
|
||||
(stood up in [[103-profile-actor-skeleton-and-primary-signaling]]).
|
||||
The primary keeps raw log storage and param declarations only, per
|
||||
[[0001-actor-split]]'s resolved decision that the primary "extracts
|
||||
declared + auto params only... no metrics, no profile logic." The
|
||||
profile actor consumes newly-visible primary lines (via the
|
||||
watch-wake loop from ticket 103) and runs metric regexes against them
|
||||
itself, building its own bucket data independent of the primary.
|
||||
|
||||
## Where
|
||||
|
||||
- `src/engine/snapshot/mod.rs:364-434` (`validate_metric_request`,
|
||||
`declare_metric`, `resolve_backfill_and_store_metric`) and
|
||||
`:434-487` (`validate_update_metric`, `update_metric`,
|
||||
`remove_metric_declaration`) — the declare/update/remove machinery
|
||||
to move onto `ProfileSnapshot`.
|
||||
- `src/engine/snapshot/mod.rs:587-617` (`backfill_metric`,
|
||||
`apply_all_metrics`) — backfill replays `self.logs_range()` (a
|
||||
primary-side accessor) through `apply_metric_to_log`; once metrics
|
||||
live on the profile actor, backfill needs to read logs through the
|
||||
profile actor's cloned primary `IndexReader` instead of `self`.
|
||||
- `src/engine/handle.rs` — `EngineMessage::DeclareMetric`/
|
||||
`UpdateMetric`/`RemoveMetricDeclaration` arms (see the message list
|
||||
at `src/engine/message.rs:30-63`) currently route through the
|
||||
single actor's `write.mutate` against `EngineSnapshot`; these need
|
||||
to route to the profile actor's `ProfileSnapshot` instead.
|
||||
- `src/engine/handle.rs`'s `Ingest` arm — `snap.apply_all_metrics(...)`
|
||||
is called directly after `ingest_params` today
|
||||
(see ticket 101's change); this call moves out of the primary's
|
||||
ingest closure entirely once the profile actor computes metrics
|
||||
itself from the lines it observes via the watch/read loop, rather
|
||||
than the primary computing them inline.
|
||||
|
||||
## Scope
|
||||
|
||||
- Move `MetricDeclaration`/bucket-related `IndexKey`/`DataValue`
|
||||
variants' *usage* (not necessarily their type definitions, which are
|
||||
shared) onto `ProfileSnapshot`'s own index/map, separate from the
|
||||
primary's.
|
||||
- Move `declare_metric`/`update_metric`/`remove_metric_declaration`/
|
||||
`backfill_metric`/`apply_all_metrics` (and their validation
|
||||
counterparts) to operate on `ProfileSnapshot`, reading raw log text
|
||||
via the profile actor's cloned primary `IndexReader` wherever they
|
||||
need `logs_range`-style access, instead of `self.index` on the
|
||||
primary.
|
||||
- Remove the `apply_all_metrics` call from the primary's `Ingest` arm
|
||||
in `handle.rs` — the primary no longer computes metrics inline; the
|
||||
profile actor's watch-wake loop (from ticket 103) now calls
|
||||
`apply_all_metrics`-equivalent logic itself per newly-visible line.
|
||||
- `DeclareMetric`/`UpdateMetric`/`RemoveMetricDeclaration` messages
|
||||
need a new route to the profile actor (a second `mpsc` channel, or
|
||||
folding into the profile actor's own message enum) — decide the
|
||||
concrete mechanism during implementation; `EngineHandle`'s public
|
||||
API doesn't need to change yet if this ticket keeps routing internal
|
||||
to `handle.rs` (full API surface changes are
|
||||
[[106-split-enginehandle-api-for-primary-and-profile]]'s scope).
|
||||
- Out of scope: alert evaluation (still reads the primary's snapshot
|
||||
directly today — [[105-move-alert-evaluation-to-profile-actor]]
|
||||
moves it), and the profile-switch (`activate_profile`) reload path
|
||||
(still targets whatever this ticket lands as the metric-owning
|
||||
actor, but retargeting `src/api/mod.rs:812-852` explicitly is
|
||||
[[106-split-enginehandle-api-for-primary-and-profile]]'s scope).
|
||||
- Backfill cost and blocking behavior: per [[0001-actor-split]]'s
|
||||
performance rationale, this move is what stops a slow backfill from
|
||||
blocking the primary's ingest/query-serving — confirm with a test
|
||||
that ingesting new lines on the primary keeps working while the
|
||||
profile actor is mid-backfill on a newly declared metric.
|
||||
@@ -0,0 +1,52 @@
|
||||
# 105 — Move alert evaluation onto the profile actor
|
||||
|
||||
## Summary
|
||||
|
||||
Alert rules are profile-scoped and read metric buckets
|
||||
(`alert_states: HashMap<String, AlertRuntimeState>` lives on the
|
||||
single actor's stack frame today), so per [[0001-actor-split]]'s
|
||||
resolved decision, alert evaluation moves with metric computation.
|
||||
Depends on [[104-move-metric-computation-to-profile-actor]] landing
|
||||
first — alert evaluation reads the metric buckets that ticket moves
|
||||
onto `ProfileSnapshot`.
|
||||
|
||||
## Where
|
||||
|
||||
- `src/engine/handle.rs:141` — `let mut alert_states:
|
||||
HashMap<String, AlertRuntimeState> = HashMap::new();`, currently a
|
||||
local in the single actor's `run()` loop.
|
||||
- `src/engine/handle.rs`'s `Ingest` arm — today, right after
|
||||
`write.mutate(...)` publishes, the actor does `write.peek()` and
|
||||
calls `alerts::evaluate_alerts(&rules, &mut alert_states, &guard,
|
||||
sequence, timestamp, &text)`, then spawns
|
||||
`alerts::deliver_for_rule` for each fired alert. This whole block
|
||||
moves to wherever the profile actor's per-line processing lives
|
||||
(the watch-wake loop from ticket 103/104).
|
||||
- `src/engine/alerts.rs:139` (`AlertRuntimeState`), `:241`
|
||||
(`evaluate_alerts`), `:384` (`deliver_for_rule`) — signatures take
|
||||
the rules list and a `&EngineSnapshot`-shaped guard; check whether
|
||||
`evaluate_alerts` needs to change its snapshot parameter type to
|
||||
`ProfileSnapshot` (likely yes, since it reads bucket data that now
|
||||
lives there) or whether it can stay snapshot-shape-agnostic via
|
||||
whatever trait/accessor bucket-reading already uses.
|
||||
- Settings access for alert rules/ntfy config
|
||||
(`src/engine/handle.rs:170-177`, `settings.lock().unwrap()`) — this
|
||||
stays available to the profile actor the same way (it's a
|
||||
`SettingsHandle` clone, not primary-actor state), so no primary
|
||||
dependency here.
|
||||
|
||||
## Scope
|
||||
|
||||
- Move `alert_states` ownership to the profile actor's task-local
|
||||
state.
|
||||
- Move the post-publish `evaluate_alerts` call and `deliver_for_rule`
|
||||
spawn to fire from the profile actor's per-line processing, reading
|
||||
`ProfileSnapshot` (post-metric-computation) instead of the primary's
|
||||
`EngineSnapshot`.
|
||||
- Update `evaluate_alerts`'s signature in `src/engine/alerts.rs` if it
|
||||
needs to accept a different snapshot type; keep its actual
|
||||
evaluation logic unchanged — this ticket is a relocation, not a
|
||||
rewrite of alert semantics.
|
||||
- Out of scope: any change to alert rule semantics, ntfy delivery, or
|
||||
the `AlertRuntimeState` model itself — purely which actor owns and
|
||||
drives evaluation.
|
||||
@@ -0,0 +1,62 @@
|
||||
# 106 — Give API handlers a real way to address the primary vs. the profile actor
|
||||
|
||||
## Summary
|
||||
|
||||
By the time [[104-move-metric-computation-to-profile-actor]] and
|
||||
[[105-move-alert-evaluation-to-profile-actor]] land, metric/alert
|
||||
messages need to route to the profile actor while param/log messages
|
||||
stay on the primary — today's single `EngineHandle` (one `mpsc::Sender`
|
||||
+ one `IndexReader`) can't express that. This ticket gives `api.rs`'s
|
||||
handlers, and the runtime profile-switch path, a real API surface for
|
||||
both actors, per [[0001-actor-split]]'s resolved decision to decide
|
||||
during implementation whether that's two handle types or one
|
||||
internally-fanning-out handle.
|
||||
|
||||
## Where
|
||||
|
||||
- `src/engine/handle.rs:74-78` (`EngineHandle { tx, reader }`) — the
|
||||
single handle type `ApiState` holds today.
|
||||
- `src/api/mod.rs` — `ApiState`'s definition (wherever it stores the
|
||||
`EngineHandle`) and every handler that sends an `EngineMessage`;
|
||||
metric/alert-related routes need to target the profile actor's
|
||||
channel instead of the primary's.
|
||||
- `src/api/mod.rs:812-852` (`reload_engine_declarations`,
|
||||
`activate_profile`) — today removes/redeclares params *and* metrics
|
||||
against one engine. After this split, param removal/redeclare stays
|
||||
primary-side, metric removal/redeclare targets the profile actor —
|
||||
this function needs to send to both.
|
||||
- `src/config/profile.rs`'s `load_into_engine` (referenced in
|
||||
[[0004-profile-switch-reindexing-cost]] at `:332-384`) — same
|
||||
dual-target concern at startup load time as well as runtime
|
||||
reactivation.
|
||||
- `src/api/mcp_server.rs` — check whether the MCP server's query
|
||||
surface holds its own `EngineHandle` reference that also needs the
|
||||
updated shape.
|
||||
|
||||
## Scope
|
||||
|
||||
- Decide and implement the concrete shape: either (a) `ApiState` holds
|
||||
two handles (`primary: EngineHandle`, `profile: ProfileHandle`) and
|
||||
each handler picks the right one, or (b) one combined handle type
|
||||
that internally holds both channels/readers and routes by message
|
||||
kind. Document the choice in the PR description since
|
||||
[[0001-actor-split]] deliberately left it open.
|
||||
- Update every `api.rs` handler that currently sends a
|
||||
`DeclareMetric`/`UpdateMetric`/`RemoveMetricDeclaration` or alert-
|
||||
related message to target the profile actor; param-related messages
|
||||
(`DeclareParam`/`RemoveParamDeclaration`/`ClearParams`/
|
||||
`SetMaxLogs`/`EnsureSortHelperIndex`) stay on the primary.
|
||||
- Update `reload_engine_declarations`/`activate_profile` and
|
||||
`Profile::load_into_engine` to split their remove-then-redeclare
|
||||
sequence across both actors correctly, preserving today's behavior
|
||||
(still fully synchronous from the caller's perspective — this
|
||||
ticket doesn't change the blocking nature of profile switching,
|
||||
only which actor(s) the messages go to; see
|
||||
[[0004-profile-switch-reindexing-cost]] for the separate question of
|
||||
making that async).
|
||||
- Out of scope: the query fan-out layer for requests needing both raw
|
||||
logs and profile-derived data in one response — that's
|
||||
[[107-query-fan-out-across-primary-and-profile]]. Read-only query
|
||||
handlers can be touched here only insofar as they need updating to
|
||||
compile against the new handle shape, not to actually merge two data
|
||||
sources yet.
|
||||
@@ -0,0 +1,58 @@
|
||||
# 107 — Fan out and merge queries that need both raw logs and profile-derived data
|
||||
|
||||
## Summary
|
||||
|
||||
Today, `Logs`/`Timeline` queries read raw log text *and*
|
||||
profile-derived params/metrics from a single `EngineSnapshot` in one
|
||||
pass — e.g. `query.rs`'s log-entry building calls `log_entry_params`
|
||||
against `compiled_params` from the same snapshot the raw text came
|
||||
from. Once params/logs live on the primary and metrics live on the
|
||||
profile actor (per tickets 103-106), any query needing both becomes a
|
||||
real join across two snapshots instead of one borrow. This is the
|
||||
piece of [[0001-actor-split]]'s design explicitly called out as "a
|
||||
query-fan-out/merge layer is still needed."
|
||||
|
||||
## Where
|
||||
|
||||
- `src/engine/snapshot/query.rs` (`handle_query_inner` and the log-
|
||||
entry-building helpers around line 70-117 per prior research) — the
|
||||
read path that currently assumes one `EngineSnapshot` has everything
|
||||
a `Logs`/`Timeline` response needs.
|
||||
- `src/engine/message.rs:12-27` (`EngineQuery`) — currently answered
|
||||
entirely off the primary's `left_right` reader
|
||||
(`EngineHandle::ask`/`handle_query_ro`, per
|
||||
[[0001-actor-split]]'s "queries already bypass this actor" note);
|
||||
after the split, some query variants need data from both actors'
|
||||
read sides.
|
||||
- `src/api/mod.rs` — wherever `EngineQuery` results are turned into
|
||||
HTTP responses; the merge/join logic likely belongs here or in a new
|
||||
helper alongside `query.rs`, not duplicated per handler.
|
||||
- Dashboard/PromQL panel queries (`src/engine/promql.rs`) — confirm
|
||||
whether these are profile-only reads (metrics live entirely on the
|
||||
profile actor) and therefore need no fan-out, versus `Logs`/
|
||||
`Timeline` which do.
|
||||
|
||||
## Scope
|
||||
|
||||
- Identify every `EngineQuery` variant that currently reads both raw
|
||||
log data and profile-derived data (params, metrics) from one
|
||||
snapshot, and design the two-snapshot join for each — likely: read
|
||||
matching log entries from the primary by sequence range, then look
|
||||
up each entry's profile-derived fields (declared param values,
|
||||
metric-driven fields) from the profile actor's snapshot by the same
|
||||
sequence, filtering/merging results.
|
||||
- Confirm ordering/consistency expectations: the profile actor can lag
|
||||
the primary (per [[0001-actor-split]]'s accepted eviction/lag
|
||||
tradeoff), so a joined query may find a primary-side log entry with
|
||||
no corresponding profile-side data yet — decide and document the
|
||||
behavior (omit those fields vs. error) rather than leaving it
|
||||
implicit.
|
||||
- Update PromQL/dashboard panel query handling only if it's confirmed
|
||||
to need primary-side data too (e.g. `ottyl_seq`/`ottyl_ts` bare
|
||||
queries per prior ticket history) — otherwise leave it reading the
|
||||
profile actor alone.
|
||||
- Out of scope: query result caching or performance optimization
|
||||
beyond what's needed for correctness — this ticket is about
|
||||
producing correct joined results, not making them fast (see
|
||||
[[0005-additional-read-indexes]] for read-path performance work,
|
||||
which should be sequenced after this settles).
|
||||
@@ -0,0 +1,57 @@
|
||||
# 108 — Rework the engine test harness for the primary/profile actor split
|
||||
|
||||
## Summary
|
||||
|
||||
`src/engine/tests.rs` (2300+ lines) and the `test_handle` pattern are
|
||||
built entirely around one actor with one channel and one snapshot.
|
||||
After tickets 103-107 land, tests need to exercise three shapes that
|
||||
don't exist today: the primary actor alone, the profile actor alone,
|
||||
and primary+profile interaction (including lag/backfill scenarios).
|
||||
This is explicitly called out as a gap in [[0001-actor-split]]'s "Not
|
||||
yet considered" section — real rework, not just extension.
|
||||
|
||||
## Where
|
||||
|
||||
- `src/engine/tests.rs` — the whole file; most existing tests
|
||||
construct an `EngineSnapshot` directly or drive it through a single
|
||||
`test_handle`-obtained channel and assert against one snapshot's
|
||||
query results. Every test that declares a metric, ingests a
|
||||
matching line, and asserts on bucket/query output now spans two
|
||||
actors.
|
||||
- `src/engine/handle.rs` `test_handle` (see ticket 099's history for
|
||||
the existing single-actor test-visibility pattern) — needs an
|
||||
equivalent for the profile actor, and likely a combined test harness
|
||||
helper that spins up both and wires them together the way `spawn`
|
||||
does in production.
|
||||
- `src/api/mod.rs`'s test module (`ingest_lines`/similar helpers, e.g.
|
||||
around `:1817` per prior research) and `src/config/profile.rs`'s
|
||||
test module — both currently assume one engine handle; they need
|
||||
updating wherever they exercise metric/alert behavior that's now
|
||||
profile-actor-owned.
|
||||
|
||||
## Scope
|
||||
|
||||
- Add primary-only test helpers (construct a primary actor, ingest
|
||||
lines, assert on raw storage/param extraction only — no metrics).
|
||||
- Add profile-only test helpers (construct a profile actor against a
|
||||
fake/minimal primary read-side, declare metrics, assert on bucket
|
||||
computation and alert evaluation in isolation).
|
||||
- Add combined-harness helpers for tests that need real end-to-end
|
||||
behavior (ingest via primary, wait for the profile actor to catch up
|
||||
via the watch signal — needs a deterministic "wait until profile
|
||||
actor has processed up to sequence N" helper rather than a sleep, to
|
||||
avoid flaky tests) — a `barrier`-equivalent for the profile actor,
|
||||
mirroring `EngineHandle::barrier` (`src/engine/handle.rs:89-101` per
|
||||
prior research).
|
||||
- Add explicit tests for the accepted lag/eviction tradeoff from
|
||||
[[0001-actor-split]]'s resolved decisions: primary evicts on
|
||||
schedule regardless of profile-actor progress, and a lagging profile
|
||||
actor silently misses data for the evicted range — this behavior
|
||||
should be asserted, not just assumed.
|
||||
- Migrate existing tests in `src/engine/tests.rs` to the new harness
|
||||
incrementally; tests that only exercise primary-side behavior (param
|
||||
declaration, log storage, eviction) need minimal changes, while
|
||||
tests exercising metrics/alerts need the combined harness.
|
||||
- Out of scope: adding new test *coverage* beyond what's needed to
|
||||
preserve today's behavior guarantees across the split — this is a
|
||||
harness migration, not a coverage expansion project.
|
||||
Reference in New Issue
Block a user