Add tickets, ideas and samply ignore
This commit is contained in:
@@ -1,3 +1,4 @@
|
|||||||
/target
|
/target
|
||||||
target/
|
target/
|
||||||
.claude/worktrees/
|
.claude/worktrees/
|
||||||
|
profile.json.gz
|
||||||
|
|||||||
@@ -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
|
||||||
|
|
||||||
|
|
||||||
@@ -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<IndexKey>` or a `(Bound<u64>, Bound<u64>)` 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).
|
||||||
@@ -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<u64>` 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.
|
||||||
@@ -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.
|
||||||
@@ -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::<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.
|
||||||
@@ -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.
|
||||||
Reference in New Issue
Block a user