MQE: label each source of memory consumption to make it easier to identify where something is returned to the pool multiple times #11654
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… where something is returned to the pool multiple times
tcard
approved these changes
Jun 6, 2025
There was a problem hiding this comment.
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
approved these changes
Jun 6, 2025
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
This was referenced Jun 12, 2025
Merged
charleskorn
added a commit
that referenced
this pull request
Jun 17, 2025
… 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.