Streaming labels values PR#6 - #15426
Conversation
8853ab6 to
e6982ad
Compare
5b2fa36 to
217c905
Compare
e6982ad to
1b2c64f
Compare
Re-introduces the dropped optimizations on top of the new PR-5 base: - search_handler: per-(endpoint × score) sync.Pool for batch envelopes; *WithScore record variants with non-pointer Score float64 to avoid the per-record *float64 heap allocation; defaultSuccessTrailer fast path; streamSearchNDJSON takes a pre-built envelope. - distributor_search: replace cancel closure with dual context.CancelCauseFunc fields on ingesterSearchResultSet. - tenantfederation/merge_queryable: tenantJobsForSearch fast path (zero-alloc when no id-label matcher) + single-job bypass in SearchLabelNames/SearchLabelValues. - storegateway/bucket_search: single-set fast path skipping MergeSearchResultSets wrapper alloc. Benchmarks re-added: handler Encoding / MetadataEncoding / OverHTTP + DefaultSuccessTrailer pinning test; distributor SearchLabelValues / SearchLabelNames; ingester SearchLabelValues / SearchLabelNames / LegacyVsSearchLabelValues; storegateway SearchLabelValues / SearchLabelNames / SearchLabelValuesVsLabelValues; tenantfederation SearchLabelValues.
0c26c56 to
191e343
Compare
| ┌──────────────────────────────────────────────────────────────────────┐ | ||
| │ Tenant federation │ | ||
| │ per-tenant fan-out; k-way merge across tenants │ | ||
| │ single-tenant fast paths skip the merge wrapper │ |
There was a problem hiding this comment.
Maybe not that important for an overview.
| // newIngesterSearchResultSet stores the two cancel funcs as struct | ||
| // fields so the constructor does not need to allocate a closure to | ||
| // bundle them — Close calls both directly. |
There was a problem hiding this comment.
I don't understand why this is relevant? Is it about changing the type of cancel? Anyway, this comment probably can go.
|
@tcp13equals2 Would you like me to review the README? I don't always review internal docs as close, but I'm happy to if you would like. |
hey @knylander-grafana - sure that would be great. Thank you! |
| // a real httptest.NewServer + http.Client so chunked-encoding cost shows up | ||
| // honestly. Gated by testing.Short(): the in-memory Encoding benchmark above | ||
| // covers the encoding cost without paying the network round-trip tax. | ||
| func BenchmarkSearchLabelNamesHandler_OverHTTP(b *testing.B) { |
There was a problem hiding this comment.
I don't know if this benchmark shows what is interesting about this new API. Just for fun, I benchmarked exactly the same test and it has slightly worst performance than the legacy API /api/v1/labels. That's not surprising, the way it's benchmarked.
It's maybe fine, but why do you want this benchmark?
Imo, if we include benchmark we could for instance show time to first byte (maybe with filter or not), but that's really what is interesting here.
Also that's arguably what matters the most, because if we mess-up something on the server where the querier waits for all results before streaming it back to the client, we should see a huge regression.
ldufr
left a comment
There was a problem hiding this comment.
Overall good, I didn't review too thoroughly the README, since @knylander-grafana
offered to have a look.
|
I have merged this one and any feedback on the docs I will include in a subsequent PR. Thanks all for your reviews! |
What this PR does
This is the 6th PR in the streaming label/value search API implementation.
See also #15233, #15301, #15347, #15349 and #15364.
This PR adds a README.md to assist in documenting this implementation, benchmark test files and some enhancements found whilst running the newly added benchmarks.
No changelog has been recorded since these are internal implementation, testing and documentation changes with no user facing modifications.
Benchmarks: legacy
LabelValuesvs newSearchLabelValuesIngester (
pkg/ingester)Observations
The allocation reduction is due to the batch buffer reuse and synchronous-send which make skipping the clone safe.
The increase in B/op is due to the wrapping of each record in a
SearchResult{Value, Score}.Store-gateway (
pkg/storegateway)Observations
Although the same SearchResult wrapper is present it is lost in the other SG B/op noise.
Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]. If changelog entry is not needed, please add thechangelog-not-neededlabel to the PR.about-versioning.mdupdated with experimental features.Note
Low Risk
No user-facing API or behavior changes beyond optional micro-optimizations on an experimental, flag-gated search path; risk is mainly benchmark/doc churn and regression in edge fast paths (single tenant/block, cancel lifecycle).
Overview
Adds
docs/internal/streaming-label-value-search/README.md, an internal guide for the experimental streaming label/value search stack (NDJSON HTTP endpoints, data flow, merge layers, wire batching, benchmarks).Benchmark coverage is added across ingester, distributor, store-gateway, tenant federation, and HTTP handler tests (including legacy vs new parity benches and distributor merge/dedup scenarios).
Performance tweaks found while benchmarking: HTTP
search_handlerpools NDJSON batch envelopes, splits score vs no-score record types to avoid*float64allocs, and writes a prebuilt success trailer; distributor stores stream cancel funcs oningesterSearchResultSetinstead of per-ingester closures; tenant federation usestenantJobsForSearch(skip map work when no id-label matcher) and a single-tenant job bypass; store-gateway skipsMergeSearchResultSetswhen only one block matches. Ingester/store-gateway sources link to the new README.Reviewed by Cursor Bugbot for commit 3990db9. Bugbot is set up for automated code reviews on this repo. Configure here.