Remove tickets 079/080 (no profiling evidence to justify the fix)
Both tickets explicitly scope themselves to "confirm via profiling before implementing" (079: a trigram/token pre-filter for free-text regex search; 080: caching parsed JSON per log line for JSON-field filters). Neither has profiling data showing it's the dominant cost once ticket 077's range-narrowing lands, and building either speculatively risks unbounded memory (079) or penalizing the more common non-JSON case (080) for no measured benefit. Revisit if `samply` profiling against a realistic log volume shows these are actually hot. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TeyhRfMYjMditek6geDRoQ
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
946fd50d53
commit
7df6026a21
@@ -1,56 +0,0 @@
|
|||||||
# 079 — Free-text regex filters have no index; full scan + regex eval per query
|
|
||||||
|
|
||||||
## Summary
|
|
||||||
|
|
||||||
A free-text search-bar filter (a bare regex, not a
|
|
||||||
`param[value]op`/`ottyl_seq`/`ottyl_ts` comparison) is evaluated
|
|
||||||
line-by-line against every retained log on every query — there's no
|
|
||||||
narrower key range to seek into here since the match isn't tied to a
|
|
||||||
declared param or timestamp, so this is the one case in the read path
|
|
||||||
where "narrow the key range" doesn't directly apply and an actual
|
|
||||||
secondary index would be needed instead.
|
|
||||||
|
|
||||||
## Where
|
|
||||||
|
|
||||||
- `CompiledFilter::Regex` (compiled in
|
|
||||||
`src/engine/snapshot/filter.rs`, evaluated via `.eval(...)` per log
|
|
||||||
line) is checked against the full text of every candidate log line
|
|
||||||
returned by `logs_range()`/the sort-helper iteration in
|
|
||||||
`handle_query_inner` (`src/engine/snapshot/query.rs:59-70` and
|
|
||||||
`111-116`) — i.e. once ticket 077 narrows the *sequence*/time range,
|
|
||||||
a regex-text filter still forces a full per-line regex match over
|
|
||||||
everything left in that range, and if there's no `since_sequence`
|
|
||||||
bound at all (e.g. a fresh search over the whole retained log
|
|
||||||
window), this is a full linear scan with a regex eval per line.
|
|
||||||
- There is no cached/precomputed text index (trigram, token, or
|
|
||||||
otherwise) — every query recompiles nothing (the regex itself is
|
|
||||||
cached fine, see param/metric regex caches in
|
|
||||||
`src/engine/snapshot/mod.rs:26-34`), but re-*evaluates* the regex
|
|
||||||
against the full text of every line in range, every time.
|
|
||||||
|
|
||||||
## Scope
|
|
||||||
|
|
||||||
- This is the most invasive of the read-path tickets — flag as such.
|
|
||||||
Do not implement speculatively; first confirm via profiling
|
|
||||||
(`samply record` against a realistic log volume with an active
|
|
||||||
free-text filter) that this is actually the dominant cost before
|
|
||||||
building an index, since regex eval over a bounded/paginated result
|
|
||||||
set may already be fast enough once ticket 077's range-narrowing
|
|
||||||
lands.
|
|
||||||
- If profiling confirms it's worth it: investigate a coarse
|
|
||||||
pre-filter (e.g. a trigram index or per-line token set) built
|
|
||||||
incrementally at ingest time (alongside
|
|
||||||
`update_sort_helper_indexes_on_ingest`,
|
|
||||||
`src/engine/handle.rs:151`) that can cheaply reject lines that
|
|
||||||
can't possibly match a given regex's required literal substrings,
|
|
||||||
before paying for the actual regex match — not a replacement for
|
|
||||||
the regex eval, a way to skip most of it.
|
|
||||||
- Keep the index bounded in size / evictable in step with
|
|
||||||
`evict_excess_logs` (`src/engine/handle.rs:153`) so it doesn't grow
|
|
||||||
unbounded relative to `max logs in memory`.
|
|
||||||
|
|
||||||
## Depends on
|
|
||||||
|
|
||||||
Should land after ticket 077 (range narrowing) so profiling reflects
|
|
||||||
the scan cost that's actually left, not the one about to be fixed
|
|
||||||
anyway.
|
|
||||||
@@ -1,52 +0,0 @@
|
|||||||
# 080 — JSON field filters re-parse every candidate line's JSON on every query
|
|
||||||
|
|
||||||
## Summary
|
|
||||||
|
|
||||||
A filter comparing a JSON field (e.g. a log line shaped as JSON,
|
|
||||||
filtered by a field inside it) re-parses the full JSON body of every
|
|
||||||
candidate log line from scratch on every query that uses such a
|
|
||||||
filter — there's no memoization of the parse, even though the same log
|
|
||||||
line's JSON gets re-parsed identically on every subsequent poll until
|
|
||||||
the line ages out.
|
|
||||||
|
|
||||||
## Where
|
|
||||||
|
|
||||||
- `eval_json_comparison` (`src/engine/snapshot/filter.rs:177-185`)
|
|
||||||
calls `serde_json::from_str::<serde_json::Value>(text)` on the raw
|
|
||||||
log text every time it's invoked, and it's invoked once per
|
|
||||||
candidate line per query (called from the per-line filter
|
|
||||||
evaluation path used in `query.rs:59-70`/`111-116` via
|
|
||||||
`CompiledFilter::eval`).
|
|
||||||
- Contrast with `log_entry_params` (used a few lines below in
|
|
||||||
`query.rs:79`/`121`), which at least only runs against lines that
|
|
||||||
survive the filter, not before — the JSON parse here runs as part of
|
|
||||||
filtering itself, so it can't be skipped even for lines that get
|
|
||||||
discarded downstream for other reasons if this filter is combined
|
|
||||||
with others.
|
|
||||||
|
|
||||||
## Scope
|
|
||||||
|
|
||||||
- Lowest-effort option: nothing to cache at the snapshot level today
|
|
||||||
since `DataValue::LogLiteral` only stores raw text
|
|
||||||
(`src/engine/snapshot/mod.rs` / `data_value.rs`) — re-parsing per
|
|
||||||
query is currently unavoidable without changing storage. Confirm
|
|
||||||
this is worth doing before storing pre-parsed JSON: check whether
|
|
||||||
most ingested lines are actually JSON-shaped in typical usage, since
|
|
||||||
adding a parsed-JSON cache to every ingested line penalizes the
|
|
||||||
(likely more common) non-JSON case.
|
|
||||||
- If justified, consider parsing once at ingest time only for lines
|
|
||||||
that look JSON-shaped (reuse the cheap detection already implied by
|
|
||||||
`eval_json_comparison`'s `Ok(...) else return false` short-circuit)
|
|
||||||
and storing the parsed `serde_json::Value` (or just the needed field
|
|
||||||
extractions) alongside `LogLiteral`, invalidated/dropped the same
|
|
||||||
way the line itself is evicted (`evict_excess_logs`,
|
|
||||||
`src/engine/handle.rs:153`).
|
|
||||||
- Alternative, cheaper fix: memoize the parse per-query-invocation
|
|
||||||
rather than per-line-per-filter-eval, i.e. if a single query
|
|
||||||
applies the JSON filter against a line more than once (it currently
|
|
||||||
doesn't appear to, but double-check `CompiledFilter` composition —
|
|
||||||
AND/OR combinators) avoid re-parsing within that single pass.
|
|
||||||
|
|
||||||
## Depends on
|
|
||||||
|
|
||||||
None.
|
|
||||||
Reference in New Issue
Block a user