-
Notifications
You must be signed in to change notification settings - Fork 686
MQE: label each source of memory consumption to make it easier to identify where something is returned to the pool multiple times #11654
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… where something is returned to the pool multiple times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the MemoryConsumptionTracker to attribute memory usage to specific sources, making negative-memory panics more informative and aiding debugging in pedantic mode. It also propagates these source annotations through pool operations and updates all related tests.
- Introduce
MemoryConsumptionSourceenum and per-source tracking inMemoryConsumptionTracker. - Change
IncreaseMemoryConsumption/DecreaseMemoryConsumptionsignatures to accept a source, and update all call sites. - Add
TestMemoryConsumptionSourceNamesto verify source-name mapping and adjust numerous tests/pool definitions.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/limiter/memory_consumption.go | Added MemoryConsumptionSource, per-source counters, updated methods |
| pkg/util/limiter/memory_consumption_test.go | Updated tests to pass source param; added verification test |
| pkg/streamingpromql/types/limiting_pool.go | Updated NewLimitingBucketedPool signature and Get/Put to use sources |
| pkg/streamingpromql/types/limiting_pool_test.go | Updated pool tests to supply source enum |
| pkg/streamingpromql/operators/operator_buffer_test.go | Updated memory-tracker calls to specify FPointSlices source |
| pkg/streamingpromql/operators/functions/histogram_function.go | Supplied BucketSlices source to histogram pools |
| pkg/streamingpromql/operators/functions/common_test.go | Split combined IncreaseMemoryConsumption into per-source calls |
| pkg/streamingpromql/operators/binops/one_to_one_vector_vector_binary_operation_test.go | Separated float/histogram memory tracking into distinct calls |
| pkg/streamingpromql/operators/aggregations/topkbottomk/range_query.go | Added TopKBottomKRangeQuerySeriesSlices and IntSliceSlice sources |
| pkg/streamingpromql/operators/aggregations/topkbottomk/instant_query.go | Added TopKBottomKInstantQuerySeriesSlices source |
| pkg/streamingpromql/operators/aggregations/quantile.go | Added QuantileGroupSlices source |
| pkg/querier/block_streaming.go | Updated tracker interface and calls to use StoreGatewayChunks |
| pkg/ingester/client/streaming.go | Updated tracker interface and calls to use IngesterChunks |
Comments suppressed due to low confidence (3)
pkg/util/limiter/memory_consumption.go:109
- The comment for
IncreaseMemoryConsumptionshould mention the newsourceparameter and explain its purpose.
// IncreaseMemoryConsumption attempts to increase the current memory consumption by b bytes.
pkg/util/limiter/memory_consumption.go:132
- The comment for
DecreaseMemoryConsumptionshould mention the newsourceparameter and its role in per-source tracking.
// DecreaseMemoryConsumption decreases the current memory consumption by b bytes.
pkg/util/limiter/memory_consumption_test.go:253
- You cannot range over an integer. Change this to iterate from 0 to memoryConsumptionSourceCount, e.g.:
for i := MemoryConsumptionSource(0); i < memoryConsumptionSourceCount; i++ {}.
for i := range memoryConsumptionSourceCount {
aknuds1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, except I think implementing a String method on MemoryConsumptionSource is preferable.
Signed-off-by: Nick Pillitteri <[email protected]>
aknuds1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
… becomes negative (#11713) #### What this PR does This PR adds the trace ID (if any) to the panic message used if the estimated memory consumption of a query becomes negative. #### Which issue(s) this PR fixes or relates to #11615, #11654 #### Checklist - [x] Tests updated. - [n/a] Documentation added. - [covered by #10067] `CHANGELOG.md` updated - the order of entries should be `[CHANGE]`, `[FEATURE]`, `[ENHANCEMENT]`, `[BUGFIX]`. - [n/a] [`about-versioning.md`](https://github.com/grafana/mimir/blob/main/docs/sources/mimir/configure/about-versioning.md) updated with experimental features.
What this PR does
This PR extends
limiting.MemoryConsumptionTrackerto track memory consumption by source.This allows us to include more information in
Estimated memory consumption of this query is negativepanics, and also makes it easier to debug failures when MQE is running in pedantic mode during tests.Which issue(s) this PR fixes or relates to
(none)
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.