Query Frontend: add cortex_query_samples_processed_cache_adjusted_total metric #11164
+1,139
−132
Conversation
This file contains 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
Konstantinov-Innokentii
changed the title
Konstantinov-Innokentii
changed the title
pracucci
approved these changes
Apr 10, 2025
pracucci
reviewed
Apr 16, 2025
| @@ -67,6 +67,13 @@ message Extent { | |||
| // If the response is combination of multiple queries over time, all of which had timestamp set, this is the timestamp of oldest query. | |||
| // When merging extents and some of them have 0 query timestamp, we keep non-zero timestamp, if possible. | |||
| int64 query_timestamp_ms = 6; | |||
| // Number of samples processed per step to create this Extent. | |||
There was a problem hiding this comment.
Can we sort it by timestamp by contract? If so, I would document it here, given it's a design decision.
| return result | ||
| } | ||
|
|
||
| func mergeSamplesProcessedPerStep(steps ...[]StepStat) []StepStat { |
There was a problem hiding this comment.
If we simply define that StepStat must be ordered by timestamp, then you don't need to use a map here. You can just go building the merged slice in a single pass. Keep it simple, just take 2 inputs. Don't need to be variadic.
Comment on lines
537
to
539
| if details := QueryDetailsFromContext(ctx); details != nil { | ||
| details.SamplesProcessedFromCache = cachedSamplesProcessed | ||
| } |
There was a problem hiding this comment.
Design-wise, I don't think it's partitionCacheExtents() responsibility taking the context in input and updating it.
There was a problem hiding this comment.
Fixed, thanks. Lifted stats updates up.
| cachedSamplesProcessed += uint64(stepStat.Value) | ||
| } | ||
| if details := QueryDetailsFromContext(ctx); details != nil { | ||
| details.SamplesProcessedFromCache = cachedSamplesProcessed |
There was a problem hiding this comment.
I'm a bit confused. partitionCacheExtents() can be called multiple times for a single input query. Here we overwrite SamplesProcessedFromCache each time 🤔
There was a problem hiding this comment.
Yep, my bad; thanks for catching it. Pushed a fix
It tracks oriignal number of samples used to populate the cache. The value is approximate, as cache extents might be merged or truncated. Since MQE does not expose per-step sample statistics, a heuristic is applied: each extent stores the total number of ProcessedSamples, and when only a sub-range of an extent is used in a query, the number of samples is estimated proportionally to the overlap between the requested range and the extent's time range.
…nSubrange more clear - it receives range borders, not he the range itself.
1. TestMergeCacheExtentsForRequest now expectsExtents, not samples and timeranges separately 2. Style fixes in the TestFilterRecentCacheExtents
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Co-authored-by: Marco Pracucci <marco@pracucci.com>
1. Introduce new approximation algoritm. Instead of operating with SamplesProcessed count per whole extent, use approximate perStepStats, to avoid cumulative error when resizing extent 2. Move SamplesProcessedFromCache stat to query-frontend stats from the Querier stats, since they are tracked & reported in frontend.
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Sum samples processed from cache, rather that overwrite. Also, mergePerStepStats in one pass, under the assumption that they are alwats sorted by timestamp
split_and_cache_middleware doesn't update it concurrently.
Konstantinov-Innokentii
force-pushed
the
add_cortex_query_samples_processed_from_cache_total_metric
branch
from
71a4c29 to
2706b22
Compare
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
The metric tracks the number of samples processed to execute a query if no query results cache was enabled.
How does it work: (Updated)
The extent now has a SamplesProcessedPerStep field. splitAndCacheMiddleware tracks partial stats of each downstream response it made and when downstreamRequest is converted to an extent. perStepStats are approximated from totalSamplesProcessed for this request. When extents are used to respond to a query in the partitionCacheExtents, the corresponding perStepStats are extracted and merged.
The same happens when extents are merged in or truncated when filtering recent cache extents filterRecentCacheExtents.
This number of samples processed in Extent is approximate since MQE does not expose per-step sample statistics. When extracting only part of the Extent's range, perStepStats are estimated based on TotalSamplesProcessed and number of step in the request range.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.