Update mimir-prom to v1.8.2-0.20250423083340-007de9b763aa#11292
Update mimir-prom to v1.8.2-0.20250423083340-007de9b763aa#11292
Conversation
…as dskit now has google.golang.org/grpc v1.71.1
This test has been removed upstream after the PR for duration selector math was merged. See prometheus/prometheus#16249 Signed-off-by: Nick Pillitteri <[email protected]>
Signed-off-by: Nick Pillitteri <[email protected]>
Signed-off-by: Nick Pillitteri <[email protected]>
Signed-off-by: Nick Pillitteri <[email protected]>
dimitarvdimitrov
left a comment
dimitarvdimitrov
left a comment
There was a problem hiding this comment.
do you know if we now support duration arithmetic? Do we not support it explicitly?
| expr, err = promql.PreprocessExpr(expr, timestamp.Time(r.GetStart()), timestamp.Time(r.GetEnd())) | ||
| if err != nil { | ||
| // We are being pessimistic in such cases. | ||
| level.Warn(logger).Log("msg", "failed to preprocess query, considering @ modifier as not cachable", "query", query, "err", err) |
There was a problem hiding this comment.
can we return a reason here? now we're getting double logging with this line
There was a problem hiding this comment.
Isn't that the same behaviour (different log msg) we already had in this though?
expr, err := parser.ParseExpr(query)
if err != nil {
// We are being pessimistic in such cases.
level.Warn(logger).Log("msg", "failed to parse query, considering @ modifier as not cachable", "query", query, "err", err)
return false
}
Or should we change both of those to return unique errors and remove the log line?
There was a problem hiding this comment.
ah, i didn't see this before. I'd prefer if we return a reason so we don't double log the same error. But i understand if you consider this out of scope
There was a problem hiding this comment.
Happy to fix that in a separate PR!
| }, | ||
| }, | ||
| }, | ||
| "floats: 0 length range": { |
There was a problem hiding this comment.
why are these removed?
There was a problem hiding this comment.
@charleskorn added these because he found an edge case in promql while working on MQE and added them to Prometheus upstream and MQE. The equivalent tests upstream (Prometheus) were removed when duration arithmetic support was added which we are now pulling in.
There was a problem hiding this comment.
Zero durations are no longer allowed in PromQL with prometheus/prometheus#16249 whoops Github didn't show me that Nick already replied
There's a global flag that needs to be set to enable it (same as experimental functions). We don't set it currently and I didn't want to just blindly enable it during a Prometheus update without talking with the team. I think we don't need to do anything to support it since it's entirely in the upstream PromQL parser which we use even in MQE. |
|
@krajorama asked about that in slack (not sure if I should stick the link here), it seems we're planning to enable it by default for now, but we were planning to do that in a separate PR with a proper changelog as it's cleaner. Right now it's not enabled, but some of the validation behaviour has changed anyway (returning different errors, tests have been updated accordingly). |
dimitarvdimitrov
left a comment
dimitarvdimitrov
left a comment
There was a problem hiding this comment.
thanks for explaining @56quarters @zenador
I was more concerned about Mimir crashing or panicking if its gets a request with arithmetic |
3 participants
What this PR does
This updates mimir-prom to grafana/mimir-prometheus#867
Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.