storage: introduce search interface with scoring and filtering#18497
Conversation
f3f783f to
48c2b3f
Compare
48c2b3f to
1324caa
Compare
1324caa to
d87f4a3
Compare
aknuds1
left a comment
There was a problem hiding this comment.
Directionally good, but my review uncovered some issues and refactoring opportunities.
aknuds1
left a comment
There was a problem hiding this comment.
Having looked it over again, I'd mostly like to suggest making storage.mergeSearchSets stream results when hints.CompareFunc is nil. This is to avoid full materialization when possible, with high-cardinality performance in mind.
Please take a look at my suggestions :)
Not requesting changes any longer, but not done reviewing either :)
There was a problem hiding this comment.
In addition to the comments, I think that noopQuerier should also be made to implement Searcher for completeness:
// Compile-time assertion that noopQuerier implements Searcher.
var _ Searcher = noopQuerier{}Otherwise, I think it looks good after an in-depth review.
a6c1d32 to
0a0d0a2
Compare
aknuds1
left a comment
There was a problem hiding this comment.
Mostly LGTM, but there's a new change in this revision that seems highly unusual to me, i.e. to return nil after the first Warnings() call to iterators.
Signed-off-by: Julien Pivotto <291750+roidelapluie@users.noreply.github.com>
0a0d0a2 to
f69db5b
Compare
aknuds1
left a comment
There was a problem hiding this comment.
LGTM, thanks for applying my feedback!
#### What this PR does This is the first PR for the implementation of a new streaming labels/values search API. See prometheus/proposals#74. This PR leverages Prometheus vendored storage interface changes and search/filter utilities. For reference - noting not all of these are merged as yet. * prometheus/prometheus#18405 * prometheus/prometheus#18402 * prometheus/prometheus#18576 * prometheus/prometheus#18497 * prometheus/prometheus#18573 This PR focuses on the Searcher implementation which will run on the Ingester and Store-Gateways. The changes are at the gRPC boundary into these components. Once this PR is merged, the next PR will focus on the Querier and fan-out of the search requests to the changes made in this PR. TBD - should a change log be recorded for non-user facing change. None of the changes in this PR will be inline to any user operation at this time. #### Which issue(s) this PR fixes or relates to Fixes #<issue number> #### Checklist - [x] Tests updated. - [ ] Documentation added. - [ ] `CHANGELOG.md` updated - the order of entries should be `[CHANGE]`, `[FEATURE]`, `[ENHANCEMENT]`, `[BUGFIX]`. If changelog entry is not needed, please add the `changelog-not-needed` label to the PR. - [ ] [`about-versioning.md`](https://github.com/grafana/mimir/blob/main/docs/sources/mimir/configure/about-versioning.md) updated with experimental features. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Introduces new gRPC streaming APIs and non-trivial merge/ordering logic on the read path; while largely additive, it touches core ingester/store-gateway query surfaces and could impact correctness or performance under load. > > **Overview** > Adds a new *streaming labels/values search API* across ingester and store-gateway, including new gRPC methods `SearchLabelNames` / `SearchLabelValues`, request/response protos (`SearchFilter`, `SearchOrdering`, `SearchResultBatch`), and regenerated client/server stubs. > > Implements the ingester-side `storage.Searcher` integration with batched streaming, input validation mapped to `InvalidArgument`, warning propagation, and context-cancellation checks; wires the new RPCs through activity tracking and profiling wrappers. > > Implements the store-gateway search path by running per-block searches concurrently, applying per-block filter/order/limit, and streaming a deduplicated k-way merge via a new `PairwiseMergeSearchSets` utility (ported from Prometheus), plus wrappers for tracing, activity tracking, and client error wrapping; adds comprehensive tests for batching, ordering, limits, warnings, errors, and cancellation. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 3e5862c. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
Which issue(s) does the PR fix:
Release notes for end users (ALL commits must be considered).
Reviewers should verify clarity and quality.