From 65330d37e0cb0b4a0ceb970e77df9d8ebbf38c09 Mon Sep 17 00:00:00 2001 From: Hugh Kaznowski Date: Sun, 6 Sep 2026 20:45:33 +0100 Subject: [PATCH] Add tickets, ideas and samply ignore --- .gitignore | 1 + IDEAS.md | 12 ++++ ...l-index-instead-of-seeking-by-key-range.md | 72 +++++++++++++++++++ ...ilter-scans-all-buckets-for-every-query.md | 56 +++++++++++++++ ...filter-has-no-index-full-scan-per-query.md | 56 +++++++++++++++ ...filter-reparses-json-per-line-per-query.md | 52 ++++++++++++++ ...build-full-series-from-scratch-per-poll.md | 52 ++++++++++++++ 7 files changed, 301 insertions(+) create mode 100644 IDEAS.md create mode 100644 tickets/077-log-queries-scan-full-index-instead-of-seeking-by-key-range.md create mode 100644 tickets/078-metric-comparison-filter-scans-all-buckets-for-every-query.md create mode 100644 tickets/079-free-text-regex-filter-has-no-index-full-scan-per-query.md create mode 100644 tickets/080-json-field-filter-reparses-json-per-line-per-query.md create mode 100644 tickets/081-promql-dashboard-queries-rebuild-full-series-from-scratch-per-poll.md diff --git a/.gitignore b/.gitignore index c1c4e31..7e50c68 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ /target target/ .claude/worktrees/ +profile.json.gz diff --git a/IDEAS.md b/IDEAS.md new file mode 100644 index 0000000..02219ac --- /dev/null +++ b/IDEAS.md @@ -0,0 +1,12 @@ +Under a normal log ingestion params are usually auto-detected. +Sometimes, however it's necessary to extract a param - that can be done with a regex: If a message has a non empty or missing capture for the target group, then it has a param value. +On top of that, it may be easier to capture params dependent on these other params - say you have a string of a capture group you are interested in, and you want to extract values out of it. That should be possible as well as a regex patten on top of messages with a string param. +Sometimes a string param (or even message) may be json. In which case we may want to extract params with `jq` syntax. + +There is lots of test-helper code that is simply a function stub with ignored parameters and a different scope. We don't need any of those ever. You can change the scope of some functions for the tests. Preferably you change the tests instead. + +The timeline still does not reflect the dropdown. Either use a toast with a reason why it can't while setting the dropdown to what it currently was, or have some kind of loading indicator if it's a loading issue. + +JSON formatting on frontend should auto-collapsed not open + + diff --git a/tickets/077-log-queries-scan-full-index-instead-of-seeking-by-key-range.md b/tickets/077-log-queries-scan-full-index-instead-of-seeking-by-key-range.md new file mode 100644 index 0000000..799217b --- /dev/null +++ b/tickets/077-log-queries-scan-full-index-instead-of-seeking-by-key-range.md @@ -0,0 +1,72 @@ +# 077 — Log queries scan the full index instead of seeking by key range + +## Summary + +Every log read — including polling reads that only want logs newer +than `since_sequence` — walks the *entire* retained log range and +filters with a closure, rather than seeking the `BTreeMap` directly to +the relevant key range. This is the read-path optimization the +`left-right`/sort-helper-index work never got around to: iterators +should narrow the underlying key range as far as the caller's bounds +allow, and accept range bounds as a parameter instead of scanning from +`Start`/`MetricBucketSplit` and filtering after the fact. + +## Where + +- `EngineSnapshot::logs_range` (`src/engine/snapshot/mod.rs:159-168`) + always ranges `IndexKey::Start..IndexKey::LogParamSplit` — i.e. every + ingested log ever retained — with no way for a caller to pass a + narrower bound. +- `EngineSnapshot::logs_since` (`src/engine/snapshot/mod.rs:170-181`) + calls `logs_range()` and then filters by `sequence < before_sequence` + and a timestamp window in a closure — it has all the information + needed to seek instead (`before_sequence` bounds `sequence` + directly), but throws that away and scans linearly regardless of how + small the actual window is. +- The unsorted log-query path in `handle_query_inner` + (`src/engine/snapshot/query.rs:59-65`) does the same thing: + `snapshot.logs_range().filter(|(sequence, timestamp, _)| { since ... + start_ms ... end_ms ... })`. On every poll (frontend passes + `since_sequence` — see `webapp/src/lib/api.ts:263,343`), this walks + every previously-seen log line again just to throw it away in the + filter, before even getting to the new ones. +- `IndexKey::IngestedLog { sequence, timestamp }` + (`src/engine/index_key.rs:10-13`) derives `Ord` field-order-first on + `sequence`, so a lower bound of + `IndexKey::IngestedLog { sequence: since + 1, timestamp: Timestamp::MIN }` + (or equivalent) is a valid, correct seek — sequence is assigned + monotonically at ingest (`src/engine/handle.rs:118-119`), so this + isn't a heuristic, it's exact. + +## Scope + +- Change `logs_range` to accept a key range (e.g. + `RangeBounds` or a `(Bound, Bound)` over + sequence) instead of hardcoding `Start..LogParamSplit`, so callers + can seek. +- Rework `logs_since` and the `EngineQuery::Logs` handler in + `query.rs:59-65` to build a lower bound from `since_sequence` (and, + where possible, from `start_ms`/note below) and pass it through, + instead of filtering post-hoc from the very start of the index. +- Note: because `IngestedLog` keys sort by `(sequence, timestamp)` and + not `(timestamp, sequence)`, a `start_ms`/`end_ms`-only query (no + `since_sequence`) can't be turned into an exact key-range seek from + this index alone even though ingestion is roughly time-ordered by + sequence — call this out explicitly rather than seeking incorrectly; + it may be acceptable to leave that case as a scan, or to binary-search + for the first sequence at/after `start_ms` since timestamps are + non-decreasing with sequence in practice. +- The sorted branch (`query.rs:82-128`) iterates `sort_helper_index` + directly rather than `logs_range`, so it's less affected, but still + does a per-candidate `index.get()` (`query.rs:106-107`) with no early + bound — check whether the helper index itself can be range-bounded + once `since_sequence`/`start_ms` are known. +- Add/extend a test asserting that a `since_sequence` poll on a large + log store does not touch (or does not compare against) log entries + at or before `since_sequence` — a range-based seek should make this + observably true, not just faster. + +## Depends on + +None. Related to ticket 060/061 (sort helper indexes) — same +underlying principle (avoid full scans by indexing/seeking). diff --git a/tickets/078-metric-comparison-filter-scans-all-buckets-for-every-query.md b/tickets/078-metric-comparison-filter-scans-all-buckets-for-every-query.md new file mode 100644 index 0000000..a96a8a0 --- /dev/null +++ b/tickets/078-metric-comparison-filter-scans-all-buckets-for-every-query.md @@ -0,0 +1,56 @@ +# 078 — Metric-comparison filters scan every metric's buckets on every query + +## Summary + +Same underlying issue as ticket 077 (iterators should narrow the key +range as far as possible, and take a range as a parameter), applied to +metrics: a metric-comparison filter (e.g. `latency_ms[value]<200` in +the filter bar) rebuilds its matching-sequence set from scratch on +every single query by scanning *all* buckets for *all* metrics, then +discarding everything that doesn't match the requested metric name — +instead of seeking directly to that metric's key range. + +## Where + +- `EngineSnapshot::bucket_values` + (`src/engine/snapshot/metrics.rs:295-312`) ranges + `IndexKey::MetricBucketSplit..IndexKey::End` — every bucket for every + declared metric — and filters by `name == metric_name` inside the + `filter_map` closure. `IndexKey::Bucket { metric_name, labels }` + (`src/engine/index_key.rs:23-26`) derives `Ord` on `metric_name` + first, so this is seekable: a lower bound of + `IndexKey::Bucket { metric_name: metric_name.into(), labels: + BTreeMap::new() }` and an upper bound one metric name past it would + narrow the range to just that metric's buckets. +- The compare-filter compiler, + `compile_metric_comparison` (`src/engine/snapshot/filter.rs:83-104`), + calls `self.bucket_values(metric, None)` and builds a fresh + `HashSet` of matching sequences on every call — this runs once + per filter compile, and filters are compiled fresh per query + (`compile_filter_query`, called from + `src/engine/snapshot/query.rs:46`), so a dashboard or search bar + polling with a metric-comparison filter re-scans all bucket values + every poll interval. Unlike the log sort-helper indexes + (`src/engine/snapshot/sort.rs`), there is no incremental cache here + keyed by `(metric, param, op, value)`. + +## Scope + +- Narrow `bucket_values` to accept/derive a key range for the + requested `metric_name` instead of ranging + `MetricBucketSplit..End` and filtering by name in the closure. This + alone removes the "every other metric's buckets" part of the scan + regardless of caching. +- Separately (bigger win, more invasive — may warrant its own + follow-up ticket): consider whether metric-comparison results can be + incrementally maintained similarly to `sort_helper_indexes` + (`src/engine/snapshot/sort.rs`) rather than recomputed per query, if + profiling shows compare-filtered dashboard polling is a hot path. + Keep this ticket scoped to the range-narrowing fix; note the caching + idea here so it isn't lost, but split it out if it turns out to need + design work (cache invalidation on new bucket values, per unique + filter expression). + +## Depends on + +None. Same principle as ticket 077. diff --git a/tickets/079-free-text-regex-filter-has-no-index-full-scan-per-query.md b/tickets/079-free-text-regex-filter-has-no-index-full-scan-per-query.md new file mode 100644 index 0000000..3b94082 --- /dev/null +++ b/tickets/079-free-text-regex-filter-has-no-index-full-scan-per-query.md @@ -0,0 +1,56 @@ +# 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. diff --git a/tickets/080-json-field-filter-reparses-json-per-line-per-query.md b/tickets/080-json-field-filter-reparses-json-per-line-per-query.md new file mode 100644 index 0000000..a8f4902 --- /dev/null +++ b/tickets/080-json-field-filter-reparses-json-per-line-per-query.md @@ -0,0 +1,52 @@ +# 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::(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. diff --git a/tickets/081-promql-dashboard-queries-rebuild-full-series-from-scratch-per-poll.md b/tickets/081-promql-dashboard-queries-rebuild-full-series-from-scratch-per-poll.md new file mode 100644 index 0000000..5820c06 --- /dev/null +++ b/tickets/081-promql-dashboard-queries-rebuild-full-series-from-scratch-per-poll.md @@ -0,0 +1,52 @@ +# 081 — PromQL dashboard queries rebuild full label→points maps from scratch every poll + +## Summary + +A dashboard panel backed by a PromQL-style query re-evaluates its +entire series (all labels, all points) from scratch on every polling +interval, with no reuse of the previous evaluation's work and no +incremental narrowing by the time range the panel actually needs — +same "narrow the range, don't rescan everything" principle as the +other read-path tickets, applied to the dashboard/metrics query path +this time. + +## Where + +- `evaluate_promql` and its helpers + (`src/engine/snapshot/query.rs:412-548`) build label→points maps for + the query's underlying metric(s) fresh on every call — there is no + reuse across repeated calls with the same query string and an + overlapping time window, which is exactly the pattern dashboard + polling produces (`webapp/src/lib/polling.ts` re-requests the same + panel query on an interval). +- This sits on top of `bucket_values` (ticket 078), so once that's + narrowed to a metric's own key range, `evaluate_promql` still + recomputes the full aggregation/label-grouping logic over whatever + `bucket_values` returns, every single call, even for the portion of + the time range it already computed on the previous poll. + +## Scope + +- Start by checking whether panels pass a time range/step down to + `evaluate_promql` at all — if they do, apply the same key-range + narrowing as ticket 077/078 first (bound the underlying + `bucket_values` range by the panel's requested window) before + considering caching, since that alone may remove most of the + redundant work. +- If polling the *same* query with an advancing time window is common + (sliding dashboard windows), investigate caching prior evaluation + results keyed by `(query string, label set)` and only computing the + delta for the newly-advanced portion of the window — this is more + involved (needs a cache invalidation story tied to new bucket + ingestion, similar to `sort_helper_indexes`' + `purge_stale_sort_helper_indexes`, + `src/engine/snapshot/sort.rs`) — do not build this speculatively + without first confirming via profiling that repeated PromQL + evaluation (not `bucket_values` scanning, addressed by ticket 078) + is the actual bottleneck for dashboards with multiple panels. + +## Depends on + +Should land after ticket 078 (metric bucket range narrowing), since +that changes what `evaluate_promql` is scanning underneath and may +reduce the size of this problem before any caching work is designed.