Hugh KaznowskiandClaude Sonnet 5
7a38d008fd
worktree-agent-a19b9ef959dcda6b2 (ticket 049) was branched from before "Auto
derived format" (1a481d3) landed on main, so its synchronous
persist_metric_declaration(_update) and ProfileStore::record_metric_declaration(_update)
didn't know about promoting auto-derived engine params into the persisted
profile. This merge reconciles the two:
- src/api.rs: kept the async persist_metric_declaration/_update methods and
promoted_param_records helper from "Auto derived format", but switched their
persistence from ProfileStore::load/record_metric_declaration to the new
Settings-based settings.record_metric_declaration(_update), matching ticket
049's single-settings.json model. Dropped the now-unused
active_profile_location helper in favor of locking self.settings directly.
- src/profile.rs: removed the standalone ProfileStore struct/impl entirely
(049 folds profiles into Settings), keeping load_legacy_profile_store for
migrating a pre-049 profiles.json. Extended record_metric_declaration and
record_metric_declaration_update (now methods on Settings) to keep taking
promoted_params: &[ParamDeclarationRecord], persisting promoted auto-derived
params alongside the metric via Settings::save. Moved the ticket 037/038
legacy-shape migration (migrate_legacy_metric_params /
migrate_duration_value_params) out of the removed ProfileStore::load into a
new pub(crate) migrate_legacy_profile_shapes(profiles), invoked from
Settings::load (via a new Settings::migrate_legacy_profile_shapes) so
profiles inlined in settings.json still get migrated forward on every load.
Rewrote the ProfileStore-based tests (legacy-shape migration tests, the
no-op-migration test, and the auto-derived-param promotion round-trip test)
to exercise Settings directly, including a write_legacy_settings_json helper
that replaces the old write_legacy_profiles_json.
- src/api.rs tests: rewrote declaring_metric_against_auto_derived_param_promotes_it_into_the_profile
to create/activate the profile via settings_handle.lock() and reload via
Settings::load instead of ProfileStore.
Also fixed one clippy::collapsible_if warning introduced by the new
Settings::migrate_legacy_profile_shapes.
cargo build, cargo test (222 passed, 0 failed, 0 ignored), and
cargo clippy --all-targets all pass; no new clippy warnings beyond
pre-existing ones in files untouched by this merge.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XFUNZwCvz3kcod3352ZR2o
85 lines
4.6 KiB
Markdown
85 lines
4.6 KiB
Markdown
# 050 — Bound logfmt-overfit param cardinality; derive params from `log stream`-style structured headers
|
|
|
|
## Summary
|
|
|
|
A real-world `log stream` (macOS unified log) dump (`huig_dump.txt`, ~700
|
|
lines) exposes two related problems in the auto-param-derivation path
|
|
(`auto_declare_params` in `src/engine.rs`, backed by `extract_pairs` in
|
|
`src/logfmt.rs`, run on *every* ingested line at `src/engine.rs:155`):
|
|
|
|
1. **`=`-overfitting produces runaway, low-value param cardinality.**
|
|
`extract_pairs` scans anywhere in the line for `key=value`/`key="value"`
|
|
and treats every match as a declarable param. On the sample dump this
|
|
discovers **220 distinct auto-declared param names** from under 700
|
|
lines — one-off WiFi/radio stat dumps alone contribute lines with
|
|
20-27 `key=value` pairs each (e.g. `txRTSFrm=169 txRTSFail=0
|
|
rxCTSUcast=168 ...`), most of which are internal counters that will
|
|
never recur meaningfully. Some matches are outright false positives:
|
|
base64-encoded blobs ending in padding (e.g.
|
|
`rKTEtG7JmnThhoKPbZiVrA=='>,`) get parsed as a `key=value` pair because
|
|
`=` precedes non-whitespace. Since `auto_declare_params` "never
|
|
overwrites an existing declaration" and there is no cap or pruning
|
|
anywhere in the ingest path, every distinct key seen ever becomes a
|
|
permanent declaration — this is almost certainly the source of the
|
|
reported ingest lag on this input (per-line nom scanning cost is
|
|
presumably fine; the unbounded, ever-growing declaration/index set is
|
|
the more likely culprit and should be measured either way) and it
|
|
clutters the params UI with noise no user asked for.
|
|
|
|
2. **The input's real structure is being ignored.** `log stream` output is
|
|
not logfmt at all — it's a fixed-column header (`Timestamp`, `Thread`,
|
|
`Type`, `Activity`, `PID`, `TTL`) followed by a free-text
|
|
`process: message` tail (see header row and body of `huig_dump.txt`).
|
|
`Type` (`Default`/`Error`/`Activity`), the process name (`kernel`,
|
|
`airportd`, `bluetoothd`, `WindowServer`, ...), and `PID` are
|
|
low-cardinality, genuinely useful params that could be derived
|
|
directly and cheaply from column position — with no `=`-scanning, no
|
|
ambiguity, and no risk of false positives — instead of (or in addition
|
|
to) whatever generic `key=value` pairs happen to appear later in the
|
|
line.
|
|
|
|
## Scope
|
|
|
|
- Add a bound/safeguard on `auto_declare_params` so a single burst of
|
|
unusual lines can't permanently inflate the declared-param set:
|
|
options include (pick and justify one, or combine) a cap on the total
|
|
number of auto-derived declarations, a per-line cap on how many
|
|
`key=value` pairs from one line are eligible for auto-declaration, and/or
|
|
a heuristic to reject candidate pairs that look like base64 fragments
|
|
(trailing `=`/`==` with a base64-alphabet value and no realistic bareword
|
|
shape) in `src/logfmt.rs`'s `extract_pairs`/`pair`.
|
|
- Add an optional structured-header extractor for `log stream`-shaped
|
|
input: detect the fixed-column header/row shape (timestamp, thread id,
|
|
log type, activity id, pid, ttl, `process: message`) and surface
|
|
`Type`/process-name/`PID` as derivable params, analogous to how
|
|
`ottyl_fmt` (ticket 044) records which format a line matched. Decide
|
|
whether this is a new `ottyl_fmt` variant (e.g. `"log_stream"`) or a
|
|
separate always-attempted extraction that runs alongside
|
|
logfmt/JSON detection, and record the decision in the PR.
|
|
- Use `huig_dump.txt` (already in the repo root) as the seed fixture for
|
|
tests — copy the relevant lines into a test fixture under wherever
|
|
existing ingest fixtures live, rather than reading the file from the
|
|
repo root at test time.
|
|
- Re-measure ingest throughput on this fixture before/after to confirm
|
|
the lag is actually addressed, and note the before/after numbers in the
|
|
PR description.
|
|
|
|
## Acceptance criteria
|
|
|
|
- Ingesting `huig_dump.txt`'s lines no longer produces hundreds of
|
|
permanent auto-declared params from one-off stat-dump lines; the bound
|
|
chosen is verified by a test asserting the declared-param count stays
|
|
under a stated threshold after ingesting the fixture.
|
|
- Base64-padding false positives (e.g. the `==`-terminated blobs in the
|
|
fixture) are not auto-declared as params.
|
|
- `Type`, process name, and `PID` are available as derivable/queryable
|
|
params for `log stream`-shaped lines, verified against the fixture.
|
|
- Existing logfmt/JSON auto-declaration behavior for non-`log
|
|
stream`-shaped lines (existing tests in `src/logfmt.rs`/`src/engine.rs`)
|
|
is unchanged.
|
|
|
|
## Depends on
|
|
|
|
044 (reserved param naming / `ottyl_fmt` — this ticket likely extends the
|
|
same format-detection mechanism).
|