feat(search): add disk cache metrics and disable fast-field RAM cache conditionally - #6709
Conversation
Omit fast_field_cache while split_range_disk_cache is set now means capacity 0, and SearcherContext skips the long-lived .fast cache in that case. Existing configs without Foyer still default to 1 GiB. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop SearcherConfigDeser. Keep omitted fast_field_cache as None and apply the 1 GiB vs empty default from SearcherConfig. Co-authored-by: Cursor <cursoragent@cursor.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f05c375c2c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let storage_long_term_cache = if fast_field_cache.capacity().as_u64() == 0 { | ||
| Arc::new(QuickwitCache::empty()) | ||
| } else { |
There was a problem hiding this comment.
When an explicit fast-field configuration has capacity 0 but includes virtual_caches, this shortcut removes the .fast route entirely. Previously QuickwitCache::new still constructed MemorySizedCache, whose get/put paths feed every virtual cache even though the real cache retains nothing, allowing operators to measure hypothetical policies and capacities. With QuickwitCache::empty(), those configured probes receive no accesses and their metrics become misleading; only bypass construction when virtual_caches is also empty.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
If we don't need the fast field cache then we don't need virtual_caches for fast field cache as well.
Co-authored-by: Cursor <cursoragent@cursor.com>
Match the documented searcher example to a realistic large-node layout and drop write_policy so the default write-on-eviction is obvious.
Record request outcomes, admission bypasses, and Foyer fail-open on the decorator path, and forward Foyer's mixtrics to the process metrics recorder.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/quickwit-oss/quickwit/blob/530a96289528aac7fc11f9576b5e9af229e9d0ea/quickwit-storage/src/split_range_cache/metrics.rs#L171-L173
When split_range_disk_cache is enabled, this overload silently discards every bucket set supplied by Foyer. Quickwit's Prometheus recorder only configures custom buckets from quickwit_metrics::histogram_buckets() when build_recorder runs, while metrics::describe_histogram! cannot convey bucket boundaries, so these Foyer histograms are exported with generic defaults instead of their intended latency/size boundaries. This makes bucket distributions and derived quantiles misleading; register the known Foyer bucket sets with the Prometheus builder/inventory rather than dropping them here.
https://github.com/quickwit-oss/quickwit/blob/530a96289528aac7fc11f9576b5e9af229e9d0ea/quickwit-storage/src/split_range_cache/storage.rs#L218-L220
When a Foyer failure is followed by an object-storage failure, this increments split_range_disk_cache_fail_open_total before the fallback read completes, even though the request was not served from lower storage as the documented metric description states. This makes the counter misleading during compound cache and object-store outages; increment it only after a successful fallback, or explicitly define and document it as an attempted fail-open counter.
https://github.com/quickwit-oss/quickwit/blob/530a96289528aac7fc11f9576b5e9af229e9d0ea/quickwit-storage/src/split_range_cache/metrics.rs#L279-L282
When operators set QW_METRICS_LABELS, this adapter builds Foyer metric keys solely from Foyer's own label names and values, bypassing the global labels that Quickwit's metric macros append via quickwit_metrics::__labels_env_var(). Consequently the new Foyer series lack labels such as environment or region even though docs/reference/cli.md promises those labels on every Quickwit metric, so dashboard filters can omit these series or aggregate them across environments. Append the configured global labels when constructing these metric keys.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Use 15G memory, 2G buffer pool, and 3G submit queue in the searcher example so it matches a realistic node layout.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c06357093
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _buckets: Vec<f64>, | ||
| ) -> BoxedHistogramVec { | ||
| self.register_histogram_vec(name, desc, label_names) |
There was a problem hiding this comment.
When Foyer registers a histogram through this method, the supplied bucket boundaries are discarded. Both the Prometheus and OTLP recorders configure custom boundaries only from quickwit_metrics::histogram_buckets() during exporter initialization, so the later describe_histogram! call cannot restore Foyer's boundaries; Foyer latency histograms will therefore use exporter defaults and report a misleading distribution. Register the supplied boundaries with the exporters instead of ignoring them.
Useful? React with 👍 / 👎.
congx4
changed the title
congx4
changed the title
congx4
changed the title
congx4
changed the title
1 participant
Summary
split_range_disk_cacheis set andfast_field_cache/fast_field_cache_capacityis omitted, the long-lived.fastRAM cache now defaults to capacity 0.fast_field_cacheto 1 GiB. An explicit capacity (including0or1G) is always honored.Stacked on #6707.
Test plan
cargo test -p quickwit-config --lib node_configcargo test -p quickwit-search test_zero_capacity_fast_fieldcargo test -p quickwit-search test_nonzero_fast_fieldsplit_range_disk_cacheleaves the.fastRAM cache empty, and addingfast_field_cache_capacity: 1Gkeeps bothMade with Cursor