Merge ticket 049 (inline profiles into settings.json) with auto-derived-param promotion
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
This commit is contained in:
@@ -0,0 +1,84 @@
|
||||
# 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).
|
||||
Reference in New Issue
Block a user