Resolve tickets 071-074, 076: log-stream contamination, sort, ottyl_fmt column, ingest lag
- 076: gate auto-declared log-stream column/message params (`ParamDeclaration::log_stream_gated`) behind `is_log_stream_line` at extraction time, since their fixed-offset regex has no content anchor and previously extracted garbage from continuation lines and the header row itself (the regex crate doesn't support lookaround, so this couldn't be baked into the regex text). - 074: end-to-end regression test confirms sorting a real `log stream` capture by a log-stream column (e.g. PID) now actually reorders results, since 076's fix removes the garbage values that were corrupting the sort helper index; also add a "Building sort index…" indicator in the timeline UI for a slow first-time sort. - 072: attach the reserved `ottyl_fmt` pseudo-param to each `LogEntryDto`'s params so it can be picked as a Log Preview column (previously listed as pickable but always blank). - 073: track a rolling 5s high-watermark of ingestion-to-engine-processing lag and surface it in the nav stats popover next to approx_bytes_total. - 071: group the regex(...) help button back with the filter bar's other controls, and restructure the Filtering docs section into scannable per-feature subsections (also fixes stale log_type/process/pid mention). Tickets 070 (CLI scoping) and 075 (API unresponsiveness investigation) are left in place per their own scope — both are explicitly conversation/investigation tasks, not implementation tickets. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A1ZJ3SrayzLzVndhVq9BmS
This commit is contained in:
Hugh Kaznowski
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
f34dbba6c7
commit
9fc7540a5b
@@ -1,49 +0,0 @@
|
||||
# 071 — Filter bar's syntax button/popup is ugly and incoherent
|
||||
|
||||
## Summary
|
||||
|
||||
Wishlist feedback, verbatim: "UI filter ugly syntax. The button moved
|
||||
to the right, away from the existing button cluster. The popup is a
|
||||
incoherent mess and should be clearer."
|
||||
|
||||
Two distinct complaints bundled together:
|
||||
|
||||
1. Layout regression — some button (the filter-help/syntax button,
|
||||
presumably introduced or moved by ticket 065's filter-bar help
|
||||
rewrite) now sits away from the rest of the filter bar's button
|
||||
cluster in `FilterBar.tsx` (`webapp/src/components/FilterBar.tsx`,
|
||||
currently 67 lines), instead of grouped with the others.
|
||||
2. Content/clarity — whatever popup that button opens (the filter
|
||||
syntax help introduced/reworked in ticket 065,
|
||||
"filter-bar-help-should-describe-filter-syntax-not-regex") reads as
|
||||
an incoherent mess rather than a clear reference.
|
||||
|
||||
## Scope
|
||||
|
||||
- Reproduce in the running app: open the filter bar, locate the
|
||||
syntax/help button, and confirm which button has drifted from the
|
||||
cluster and what "incoherent" means concretely (wall of text?
|
||||
disorganized sections? unclear examples?) — the wishlist entry
|
||||
doesn't specify, so this needs eyes-on before a fix.
|
||||
- Fix the button's position so it's grouped with the filter bar's other
|
||||
controls again.
|
||||
- Rework the popup's content/layout for clarity — likely restructuring
|
||||
into clearly separated sections (e.g. by filter feature: regex match,
|
||||
metric comparison, datetime, sort) with short examples per section,
|
||||
rather than a single undifferentiated block.
|
||||
- Out of scope: changing the underlying filter grammar itself
|
||||
(`filter_parser.pest`/`filter_parser.rs`) — this is presentation only.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- The filter syntax button sits back with the filter bar's other
|
||||
buttons, not isolated to one side.
|
||||
- The help popup reads as a structured, scannable reference rather than
|
||||
a single dense block — verified visually in the running app (`make
|
||||
dev`), not just by inspecting JSX.
|
||||
|
||||
## Depends on
|
||||
|
||||
None. Touches the same area as ticket 065 and ticket 066
|
||||
("filter-bar-value-doesnt-reflect-active-preset-or-sort") — check those
|
||||
haven't already reshuffled this layout before starting.
|
||||
@@ -1,44 +0,0 @@
|
||||
# 072 — Log Preview needs a format column (`ottyl_fmt`)
|
||||
|
||||
## Summary
|
||||
|
||||
Wishlist feedback: "Log preview needs format column." `ottyl_fmt`
|
||||
(json/logfmt/none, `crate::logfmt::detect_format`) already exists as a
|
||||
reserved, filterable pseudo-param (`OTTYL_RESERVED_PARAM_NAMES`,
|
||||
`src/engine.rs:1960`; filter-bar support via
|
||||
`compile_ottyl_fmt_comparison`, `src/engine.rs:1067`) — you can already
|
||||
filter `ottyl_fmt==json` in the search bar. But it isn't surfaced as a
|
||||
visible column in the Log Preview table
|
||||
(`webapp/src/components/LogPreview.tsx`), and it can't be, because
|
||||
`LogEntryDto` (`src/dto.rs:10-16`) doesn't include it: `params` only
|
||||
holds declared `ParamDeclaration` matches, and format detection is
|
||||
computed line-locally elsewhere, never attached to the returned log
|
||||
entry.
|
||||
|
||||
## Scope
|
||||
|
||||
- Add the per-line detected format to `LogEntryDto` (`src/dto.rs:10`),
|
||||
populated wherever log entries are assembled for the `Logs` query
|
||||
response in `engine.rs` (reuse `crate::logfmt::detect_format`, the
|
||||
same detection `ottyl_fmt` filtering already calls).
|
||||
- Add `ottyl_fmt` as a column option in Log Preview's `ColumnPicker`
|
||||
(`webapp/src/components/LogPreview.tsx:28-56`) alongside declared
|
||||
params — it's a reserved pseudo-param like `ottyl_seq`/`ottyl_ts`, so
|
||||
treat it the way the params list already flags `reserved`
|
||||
(`dto.rs:38`, `ParamDeclarationDto.reserved`) rather than requiring
|
||||
it be "declared" to show up as a pickable column.
|
||||
- Render the column value as the short format label (`json`/`logfmt`/
|
||||
`none`) matching what `ottyl_fmt` filter comparisons already produce.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- A "Format" (or `ottyl_fmt`) column is available in the Log Preview's
|
||||
column picker and, once toggled on, shows the correct detected format
|
||||
per row — json lines show `json`, logfmt lines show `logfmt`, neither
|
||||
shows `none`.
|
||||
- No change to filter-bar `ottyl_fmt` comparison behavior (still works
|
||||
as before).
|
||||
|
||||
## Depends on
|
||||
|
||||
None.
|
||||
@@ -1,64 +0,0 @@
|
||||
# 073 — Track and surface ingestion-to-engine-processing lag (high watermark)
|
||||
|
||||
## Summary
|
||||
|
||||
Wishlist feedback, verbatim:
|
||||
|
||||
> There needs to be a ingstion to parsing latency on frontend.
|
||||
> Basically when a log is read, it gets what would be the `ottyl_ts`
|
||||
> value. However, the engine can lag on ingestion. That lag needs to
|
||||
> be tracked as a high watermark. We can have a collection that stores
|
||||
> say 5 highest values within the last 5 seconds. If a value in that
|
||||
> collection is older than the 5s window, it gets replaced. If a value
|
||||
> is higher than the minimum current, it gets replaced. The frontend
|
||||
> will display the highest single value (humantime duration lag, or ms
|
||||
> lag) alongside where it displays the logs and allocation size.
|
||||
|
||||
Confirmed the mechanism this is about: `ingest.rs` stamps
|
||||
`timestamp: Timestamp::now()` at read time (this becomes `ottyl_ts`,
|
||||
e.g. `ingest.rs:25`/`:107`) *before* sending `EngineMessage::Ingest`
|
||||
over the mpsc channel to the engine actor task. The engine only
|
||||
processes it whenever the actor's message loop gets scheduled to it —
|
||||
under load (large backlog, slow query in flight, etc.) that gap can
|
||||
grow. Today nothing measures `(engine processing time) - (ottyl_ts)`,
|
||||
so there's no visibility into ingest backpressure.
|
||||
|
||||
The "allocation size" reference point is `approx_bytes_total`
|
||||
(`src/dto.rs:80`, ticket 011), currently shown in
|
||||
`webapp/src/components/NavMenu.tsx:104` in the stats popover — that's
|
||||
the existing display location the lag figure should sit alongside.
|
||||
|
||||
## Scope
|
||||
|
||||
- In the engine actor's ingest handling (where `EngineMessage::Ingest`
|
||||
is received, `engine.rs`), compute `lag = Timestamp::now() -
|
||||
message.timestamp` at the moment of processing.
|
||||
- Maintain a small rolling high-watermark collection per the spec
|
||||
above: keep up to 5 entries, each valid only within the last 5
|
||||
seconds; a new lag value replaces the current minimum-held entry if
|
||||
it's larger, and any entry older than the 5s window is evicted
|
||||
(replaced) regardless of magnitude. Expose the current max of that
|
||||
collection.
|
||||
- Add this figure to whatever stats DTO backs the popover
|
||||
`approx_bytes_total` comes from, and to the corresponding stats
|
||||
query/response path in `api.rs`.
|
||||
- Display it in `NavMenu.tsx` next to `approx_bytes_total` (`:104`),
|
||||
formatted as a humantime-style duration for larger lag and plain ms
|
||||
for small values — match whatever precedent this codebase already
|
||||
uses for duration formatting (check `webapp/src/lib` for an existing
|
||||
humanize helper before writing a new one).
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- Under normal (non-lagged) ingestion, the displayed value stays near
|
||||
zero.
|
||||
- Artificially delaying engine processing (e.g. a slow query
|
||||
saturating the actor's message loop, or a synthetic test) causes the
|
||||
displayed high-watermark to rise and correctly decays out of the
|
||||
rolling 5s window afterward.
|
||||
- The value is visible in the same UI location as
|
||||
`approx_bytes_total`.
|
||||
|
||||
## Depends on
|
||||
|
||||
None.
|
||||
@@ -1,68 +0,0 @@
|
||||
# 074 — Timeline sort-by-property still doesn't work after tickets 059-061
|
||||
|
||||
## Summary
|
||||
|
||||
Wishlist feedback, verbatim: "The sorting on the timeline still does
|
||||
not work. I have tried sorting things that I know are sorted (logs
|
||||
param timestamp) and things that arent (thread name or pid). It does
|
||||
not work (change view, index etc). If it's taking time to index, that
|
||||
needs to be visible."
|
||||
|
||||
This is a regression/bug report against functionality that should
|
||||
already exist: sort-by-param was added in ticket 045, and tickets
|
||||
060/061 (resolved in commit `de9ba6f`, "Resolve tickets 059-061:
|
||||
generic log-stream columns + sort helper indexes") replaced the naive
|
||||
full-materialize-then-sort with an incremental helper-index scheme
|
||||
(`SortHelperIndexEntry`, `build_sort_helper_index`,
|
||||
`update_sort_helper_indexes`, `purge_stale_sort_helper_indexes` in
|
||||
`engine.rs:1332-1431`), wired into the `EngineQuery::Logs` sorted
|
||||
branch around `engine.rs:1598-1634`. On the frontend, `sortBy`/
|
||||
`sortMissingFallback` are threaded from `LogTimeline.tsx`'s settings
|
||||
through `App.tsx` (`:349-350`, `:505-506`) into `fetchLogs`
|
||||
(`webapp/src/lib/api.ts:256-266`). Nothing found in a static read of
|
||||
this wiring looks obviously broken, so this needs live reproduction,
|
||||
not just a code read — treat the below as an investigation, not an
|
||||
assumed root cause.
|
||||
|
||||
## Scope
|
||||
|
||||
- Reproduce with `make dev` against real piped input: declare a param
|
||||
covering thread name or PID, set the timeline's sort dropdown to it,
|
||||
and confirm whether results actually reorder. Also test sorting by an
|
||||
already-monotonic value (`ottyl_ts` or a declared timestamp param) to
|
||||
isolate whether the bug is "sort does nothing" vs. "sort scrambles an
|
||||
already-correct order."
|
||||
- Check whether the helper-index rebuild triggers correctly on first
|
||||
use per sort key (`engine.rs:1604`, `.entry(sort_by...).or_insert_with`)
|
||||
— a stale/empty helper index reused across a param rename or
|
||||
re-declaration (see the ticket-061-era comments at `engine.rs:225-233`
|
||||
about helper indexes needing invalidation when a declaration changes)
|
||||
is one plausible cause worth ruling in or out.
|
||||
- Check whether the frontend is actually sending `sort_by` on every
|
||||
poll/refresh, not just the initial query — a poll path that omits it
|
||||
(`webapp/src/lib/polling.ts`) would look exactly like "sorting
|
||||
doesn't work" despite correct backend logic.
|
||||
- Add visibility for the "still indexing" case the wishlist explicitly
|
||||
calls out: if a helper index is being built (or rebuilt) for a
|
||||
newly-selected sort key and that's slow enough to be user-visible,
|
||||
surface a loading/building state in the timeline UI rather than
|
||||
silently returning unsorted or partial results while it builds.
|
||||
- Write a regression test once the actual defect is identified —
|
||||
ticket 060's existing tests apparently didn't catch this, so identify
|
||||
what scenario they miss.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- Sorting the timeline by a declared param (including thread
|
||||
name/PID-style non-monotonic params) actually reorders displayed log
|
||||
entries, verified live in the browser.
|
||||
- Sorting by a param that's already effectively time-ordered doesn't
|
||||
regress (still displays in the expected order).
|
||||
- If a helper index build takes long enough to matter, the UI shows
|
||||
that it's building rather than looking broken or stalled.
|
||||
|
||||
## Depends on
|
||||
|
||||
Builds on tickets 045, 060, 061 — read those and their resolving
|
||||
commits (`de9ba6f` and its predecessor implementing 045) before
|
||||
forming a hypothesis.
|
||||
@@ -1,104 +0,0 @@
|
||||
# 076 — `log stream` declared columns get contaminated with garbage on non-columnar lines
|
||||
|
||||
## Summary
|
||||
|
||||
Wishlist feedback, verbatim: "The `log stream` type whitespace-separated
|
||||
formatting is not working at all. I have a session `suited-gyrfalcon`
|
||||
running, please investigate."
|
||||
|
||||
Investigated directly against the live `suited-gyrfalcon` instance
|
||||
(`ottyl sessions` → port 62059 → `/api/params`, `/api/logs`,
|
||||
`/api/stats`; 236,069 ingested log lines, real `log stream` output).
|
||||
Root cause confirmed — this is not a gap in header/column detection
|
||||
(ticket 059's detector itself works correctly on genuine `log stream`
|
||||
rows), it's that the *declared params* it produces get applied far too
|
||||
broadly once they exist.
|
||||
|
||||
**What's actually happening:** `auto_declare_log_stream_params`
|
||||
(`engine.rs:472-497`) correctly gates *declaration* on
|
||||
`is_log_stream_line(text)` (`:484`) — so it only fires column
|
||||
declarations the first time it sees a genuine columnar data row. But
|
||||
the resulting `ParamDeclaration.message_match_regex` for each column is
|
||||
a pure **fixed character-offset** pattern with no content anchor at all
|
||||
— e.g. observed live: `Activity` → `^.{55}(?P<Activity>\S(?:.{0,19}\S)?)`,
|
||||
`PID` → `^.{76}(?P<PID>\S(?:.{0,5}\S)?)`, `Thread` →
|
||||
`^.{32}(?P<Thread>\S(?:.{0,9}\S)?)` (from `log_stream_column_pattern`,
|
||||
`logfmt.rs:287-303`). Once declared, these are ordinary
|
||||
`ParamDeclaration`s: the *general* per-line param-derivation path
|
||||
(`auto_declare_params`/`extract_param_raw`) applies every declared
|
||||
param's regex to **every** ingested line, with no re-check that the
|
||||
line is actually log-stream-shaped — there's no gate equivalent to
|
||||
`is_log_stream_line` at derivation time, only at first-declaration time.
|
||||
|
||||
Confirmed impact from the live instance:
|
||||
- Sampling ~1000-line windows at several points across the 236k-line
|
||||
stream, roughly **1-2% of lines get a garbage `Timestamp` value**
|
||||
that isn't a real timestamp — e.g. line 306:
|
||||
`"so_gencnt: 4042707 so_state: 0x0"` (a continuation line from a
|
||||
wrapped multi-line network-stats dump, no relation to the columnar
|
||||
format at all) got `Timestamp: "so_gencnt: 4042707 so_state: 0x0"`,
|
||||
`Activity`/`PID`/etc sliced from arbitrary character offsets of that
|
||||
unrelated text. Same for bracket/brace-only continuation lines
|
||||
(`"}"`, `"]"`) inside pretty-printed dumps.
|
||||
- Even the header line itself (sequence 0,
|
||||
`"Timestamp Thread Type Activity PID TTL"`) ends up with
|
||||
`Activity: "Activity"`, `Thread: "Thread"`, `Timestamp: "Timestamp"`,
|
||||
`Type: "Type"` as its own param values — because ticket 052's
|
||||
retroactive re-derivation (`engine.rs:540`, re-running
|
||||
`auto_declare_params` over every retained line whenever a new param
|
||||
is declared) re-applies the newly-declared fixed-offset regexes back
|
||||
over already-ingested lines, including the header line, with no
|
||||
exemption for it.
|
||||
- This directly undermines ticket 074's "sorting is broken" complaint
|
||||
too: sorting by `Timestamp` mixes ~1-2% garbage string values in among
|
||||
otherwise lexically-sortable ISO timestamps, which would visibly
|
||||
scramble ordering — worth re-checking ticket 074 once this is fixed,
|
||||
the two may partially be the same underlying issue.
|
||||
|
||||
This explains "not working at all" better than a full parse failure
|
||||
would: the common case (genuine columnar rows) parses fine — it's that
|
||||
the contamination on the ~1-2% of non-columnar lines interleaved
|
||||
throughout every real `log stream` capture (multi-line dumps are
|
||||
common) makes every declared log-stream column look untrustworthy.
|
||||
|
||||
## Scope
|
||||
|
||||
- Make the per-column `message_match_regex` self-verifying, not purely
|
||||
positional: either (a) prefix each generated column/message pattern
|
||||
(`log_stream_column_pattern`/`log_stream_message_pattern`,
|
||||
`logfmt.rs:287-320`) with the same shape check `is_log_stream_line`
|
||||
uses so the regex only matches on a genuinely columnar line, or (b)
|
||||
gate the general param-derivation path itself so a param declared via
|
||||
`auto_declare_log_stream_params` is only evaluated against lines
|
||||
where `is_log_stream_line` holds — (a) is likely simpler since it
|
||||
keeps the gate local to the declaration rather than adding a
|
||||
special case to the generic derivation path.
|
||||
- Fix ticket 052's retroactive re-derivation (`engine.rs:540`) to
|
||||
respect that same gate, so the header line (and any other
|
||||
non-columnar retained line) doesn't retroactively pick up columnar
|
||||
param values.
|
||||
- Add a regression test using the real contaminated lines captured
|
||||
above (continuation lines like `"so_gencnt: ... so_state: ..."` and
|
||||
the header line itself) alongside the existing
|
||||
`log_stream_sample.txt` fixture — the current fixture and its tests
|
||||
don't include a multi-line-dump continuation line, which is why
|
||||
ticket 059's tests passed despite this.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- Ingesting a `log stream` capture that includes multi-line
|
||||
continuation content (network stat dumps, bracketed/braced
|
||||
pretty-printed blocks) does not populate `Timestamp`/`Thread`/`Type`/
|
||||
`Activity`/`PID`/`TTL` params on those continuation lines.
|
||||
- The header line itself never ends up with its own column names as
|
||||
values.
|
||||
- Existing correctly-parsed columnar rows are unaffected.
|
||||
- A regression test using real captured contamination examples (not
|
||||
just the original fixture) passes.
|
||||
|
||||
## Depends on
|
||||
|
||||
Builds on ticket 059 (commit `de9ba6f`) and ticket 052's retroactive
|
||||
re-derivation. Related to ticket 074 (timeline sort) — re-verify that
|
||||
complaint after this fix lands, since garbage `Timestamp` values are a
|
||||
plausible contributor to it.
|
||||
Reference in New Issue
Block a user