Add support for extended range selector modifiers smoothed & anchored #13398
Conversation
| clear | ||
| eval instant at 1m deriv(foo[3m] smoothed) | ||
| expect fail msg: smoothed modifier can only be used with: delta, increase, rate - not with deriv | ||
| # Unsupported by streaming engine. |
There was a problem hiding this comment.
Note to reviewers - because the this error is emitted from the MQE planner the prometheus test.go execEval() does not handle this
There was a problem hiding this comment.
Could we change the behaviour of the test framework to support this? I don't think it matters if the error is returned from NewInstantQuery / NewRangeQuery or from Exec, either way the query fails, and it'd be nice to be able to use all of Prometheus' tests as-is.
There was a problem hiding this comment.
I'll have a look at this as a separate PR
There was a problem hiding this comment.
charleskorn
left a comment
charleskorn
left a comment
There was a problem hiding this comment.
Thanks for working on this!
One more thing: we should make it possible to enable this feature only for some tenants - extending the existing experimentalFunctionsMiddleware likely makes the most sense here given it fulfils a similar purpose for experimental functions.
pkg/streamingpromql/operators/selectors/instant_vector_selector.go
Outdated
Show resolved
Hide resolved
| clear | ||
| eval instant at 1m deriv(foo[3m] smoothed) | ||
| expect fail msg: smoothed modifier can only be used with: delta, increase, rate - not with deriv | ||
| # Unsupported by streaming engine. |
There was a problem hiding this comment.
Could we change the behaviour of the test framework to support this? I don't think it matters if the error is returned from NewInstantQuery / NewRangeQuery or from Exec, either way the query fails, and it'd be nice to be able to use all of Prometheus' tests as-is.
pkg/streamingpromql/operators/selectors/range_vector_selector.go
Outdated
Show resolved
Hide resolved
|
💻 Deploy preview available (Add support for extended range selector modifiers smoothed & anchored): |
…com/grafana/mimir into support_anchored_smoothed_selectors
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
tacole02
left a comment
tacole02
left a comment
There was a problem hiding this comment.
Docs look good! Thank you!
…electors and wired into experimental functions middleware
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| return apierror.New(apierror.TypeBadData, DecorateWithParamName(err, "query").Error()) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Bug: Silent Error Bypass Allows Invalid Queries
The raiseError function returns nil when the type parameter doesn't match any of the expected constants (isFunction, isAggregator, or isExtendedRangeSelector). This means if an unexpected type value is passed, the function silently returns nil instead of an error, causing the middleware to incorrectly allow the query to proceed rather than rejecting it.
| m.extendedRangeFloats.Reset() | ||
| // Release the temporary ring buffer and initialise it off our given buff. | ||
| // The ring buffer will release the buff back to the slice pool. | ||
| m.extendedRangeFloats.Release() |
There was a problem hiding this comment.
We should move this before extendRangeVectorPoints so there's a high likelihood it'll use the slice that Release will return to the pool.
| if ok && matrixSelector.Anchored { | ||
| _, supported := promql.AnchoredSafeFunctions[expr.Func.Name] | ||
| if !supported { | ||
| return nil, fmt.Errorf("anchored modifier can only be used with: changes, delta, increase, rate, resets - not with %s", expr.Func.Name) |
There was a problem hiding this comment.
Can we generate the list of functions from promql.AnchoredSafeFunctions rather than hardcoding it?
(same applies below with SmoothedSafeFunctions)
| # This query will have a nil smoothedHead and smoothed tail | ||
| eval instant at 1s increase(metric[1s] smoothed) | ||
| {} 0 | ||
|
|
||
| # This query will have a nil smoothedHead, but a value for the smoothedTail | ||
| eval instant at 5s increase(metric[1s] smoothed) | ||
| {} 1 | ||
|
|
||
| # This query will have a value for smoothedHead, but a nil for the smoothedTail | ||
| eval instant at 6s increase(metric[1s] smoothed) | ||
| {} 1 |
There was a problem hiding this comment.
Could you please add equivalent tests for rate as well?
| lastPoint = fHead[len(fHead)-1] | ||
| } | ||
|
|
||
| if smoothedOrAnchored && smoothedHead != nil && smoothedTail != nil { |
There was a problem hiding this comment.
Why do we not use smoothedHead if smoothedTail is nil (and vice-versa)?
| // There is no values within the range of 1m-2m (left-open / right-closed). | ||
| // Because of that no back-filling from the look-back is used |
There was a problem hiding this comment.
I realise this isn't behaviour we defined, but this seems weird to me, as it differs to how instant vector selectors behave with the lookback - might be worth raising with the Prometheus maintainers to understand why this has been done
| if i.lazyLen == 0 && (len(i.head) > 0 || len(i.tail) > 0) { | ||
| i.lazyLen = len(i.head) + len(i.tail) | ||
| } |
There was a problem hiding this comment.
I wouldn't bother with this, unless we can see it causes a significant performance problem.
Alternatively, if we retain a reference to the view, then we can get the size from there cheaply.
| return i.at(i.idx) | ||
| } | ||
|
|
||
| func (i *headTailIterator) inc() { |
There was a problem hiding this comment.
[nit] Maybe "advance" is a better name?
| return i.idx < i.len() | ||
| } | ||
|
|
||
| func (i *headTailIterator) next() *promql.FPoint { |
There was a problem hiding this comment.
The pointers generated by next, prev etc. are going to be a significant source of garbage, and as far as I can tell we don't make use of the fact they're pointers - is there a reason why they're pointers?
| head, tail []promql.FPoint | ||
| idx int // combined iterator position across both | ||
| lazyLen int // combined length, lazy initialized in len() |
There was a problem hiding this comment.
Is there a reason to use head and tail directly here rather than calling At() on the view? That might help simplify this logic slightly as well.
| } | ||
|
|
||
| // accumulateTo will accumulate all points <= time into the given buff. | ||
| // The first point which is >= time is returned, and if not found the last point < time is returned. |
There was a problem hiding this comment.
Returning the next point from a method named accumulateTo seems out of place - perhaps we could not return it, and instead rely on the caller calling at to get it?
3 participants
What this PR does
This PR adds support for the
anchoredandsmoothedrange selector modifiers into the Mimir query engine.ie
eval instant at 60s metric[1m] anchoredoreval instant at 60s metric[1m] smoothedThis is the equivalent implementation to prometheus/pull/16457 and the original proposal for this extension can be found here.
A new CLI argument has been added to enable these extensions;
-query-frontend.enabled-promql-extended-range-selectors=smoothed,anchoredThe
experimental_functions.gomiddleware has been extended to also manage the tenant access to use these extensions. As part of adding support for this, the error messages when a function/aggregate/modifier is not enabled has been updated to correctly identify if the denied keyword is a function, aggregate or extended range modifier.Which issue(s) this PR fixes or relates to
Fixes #3260
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.Notes to reviewers;
The other note is to call attention the oddity in the use of
smoothedranges in rate/increase functions.Prometheus calculates the range boundary values differently when the range is used in one of these functions. In the prometheus implementation, the entire matrix is re-walked to do these different calculations. See https://github.com/prometheus/prometheus/blob/main/promql/functions.go#L128-L173
To avoid this, you will see that I pre-calculate these alternate values on the range boundary and pass these along with the step data. These are interpolated calculations and it would seem more efficient to pre-calculate these and not use them rather then to have to later re-walk the data.
I also did not attempt to determine if the parent of the range vector was a rate/increase function and use this to decide which boundary point calculation to use. The reason for this was in case this range vector result was ever cached / re-used then this implementation may be confusing.
Note
Adds experimental support for PromQL extended range selector modifiers
smoothedandanchored, with per-tenant enablement and full MQE planning/execution support.smoothedandanchoredmodifiers support across MQE parsing, AST cloning, planning (coreproto, plan versions), and execution (range/instant selectors, extended range handling, rate/increase/delta logic).-query-frontend.enabled-promql-extended-range-selectors(enabled_promql_extended_range_selectors) with defaults wiring and help text.about-versioning.md, configuration parameters) and CHANGELOG entries.Written by Cursor Bugbot for commit 6889720. This will update automatically on new commits. Configure here.