8000 PromQL: allow arithmetic operations in durations in PromQL parser by roidelapluie · Pull Request #16249 · prometheus/prometheus · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

PromQL: allow arithmetic operations in durations in PromQL parser #16249

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Apr 22, 2025
Merged

PromQL: allow arithmetic operations in durations in PromQL parser #16249

merged 6 commits into from
Apr 22, 2025

Conversation

roidelapluie
Copy link
Member
@roidelapluie roidelapluie commented Mar 20, 2025 •
edited
Loading

Updated the parser to allow calculations in PromQL durations.

This enables durations in the form of:

rate(http_requests_total[10m+2s])

The computation of the calculations is done directly at the parse level and does not hit the PromQL Engine. The lexer has also been updated and improved, in particular for subqueries.

Partial fix for #12318

@roidelapluie roidelapluie requested a review from krajorama March 20, 2025 12:34
@roidelapluie roidelapluie changed the title PromQL: allow arythmetics in durations in PromQL parser PromQL: allow arithmetic operations in durations in PromQL parser Mar 20, 2025
@roidelapluie roidelapluie force-pushed the arythmetics branch 6 times, most recently from ca60a28 to c0fb4ba Compare March 20, 2025 16:39
krajorama
krajorama reviewed Mar 21, 2025
View reviewed changes
Copy link
Member
@krajorama krajorama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly nit comments

krajorama
krajorama reviewed Mar 21, 2025
View reviewed changes
@roidelapluie roidelapluie force-pushed the arythmetics branch 3 times, most recently from bc3012f to 80b364e Compare March 24, 2025 11:11
@krajorama
Copy link
Member

From my point of view this is approved as technically ok. Not sure if this PR or my direction in #15899 would be easier to extend with supporting more expressions (functions, db access), but doesn't matter - it addresses current concerns.

Ping @beorn7 @fionaliao @juliusv as they reviewed #15899 as well.

@juliusv
Copy link
Member
juliusv commented Mar 25, 2025

Ah yeah, I was wondering what the relationship was between this PR and the alternative one at #15899.

This one here looks simpler in some ways, but it also seems to do a few surprising things like:

  • Evaluating duration expressions completely separately in the parser itself. If I understand correctly, this means that the duration expressions don't even show up in the AST (this PR doesn't touch ast.go) and the final AST does not contain the original duration expression anymore, but only an evaluated literal. That would be relevant for anyone wanting to programmatically manipulate ASTs, but also for rendering expressions out again as we do in https://github.com/prometheus/prometheus/blob/main/promql/parser/printer.go or in other tools like PromLens. Of course the resulting expression would behave the same with the time duration pre-evaluated, but it will lose some of the original intent / readability that the author put in.
  • The parse-time evaluation also necessitates custom operator evaluation code, which duplicates some of the code we have for that for normal binary operators.

Without reviewing every detail of both PRs, overall I think I'm more partial to the concept in #15899 (but applied to duration expressions in general, not just offsets) where the duration expressions are reflected in the AST, duration expressions are evaluated with the normal PromQL engine machinery, and there is only a separate validator (that I can reason about independently) that runs over duration expressions and checks whether they do anything forbidden. It also currently allows more constructs than this PR and would be more extensible in the future I think?

@roidelapluie
Copy link
Member Author

Without reviewing every detail of both PRs, overall I think I'm more partial to the concept in #15899 (but applied to duration expressions in general, not just offsets) where the duration expressions are reflected in the AST, duration expressions are evaluated with the normal PromQL engine machinery, and there is only a separate validator (that I can reason about independently) that runs over duration expressions and checks whether they do anything forbidden. It also currently allows more constructs than this PR and would be more extensible in the future I think?

I did this indeed as a simplification of such calculations done by the PromQL parser. I understand that keeping the AST might better. I am not convinced about extending it further than duration literals however as it would create very complex and unpredictable queries.

I will reflect the changes in the AST and move the operations down to the PromQL engine.

@krajorama krajorama mentioned this pull request Mar 25, 2025
Closed
@juliusv
Copy link
Member
juliusv commented Mar 25, 2025

@roidelapluie Yeah, I think reflecting those expressions in the normal AST would be a good thing already, and I also don't think we need to practically allow more operations than you are currently allowing.

@roidelapluie
Copy link
Member Author

Ah yeah, I was wondering what the relationship was between this PR and the alternative one at #15899.

This one here looks simpler in some ways, but it also seems to do a few surprising things like:

  • Evaluating duration expressions completely separately in the parser itself. If I understand correctly, this means that the duration expressions don't even show up in the AST (this PR doesn't touch ast.go) and the final AST does not contain the original duration expression anymore, but only an evaluated literal. That would be relevant for anyone wanting to programmatically manipulate ASTs, but also for rendering expressions out again as we do in https://github.com/prometheus/prometheus/blob/main/promql/parser/printer.go or in other tools like PromLens. Of course the resulting expression would behave the same with the time duration pre-evaluated, but it will lose some of the original intent / readability that the author put in.

Can you have a new look?

krajorama
krajorama reviewed Apr 2, 2025
View reviewed changes
Copy link
Member
@krajorama krajorama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good, I've added some comments

@roidelapluie
PromQL: allow arithmetic in durations in PromQL parser
7370d62 
Updated the parser to allow calculations in PromQL durations.

This enables durations in the form of:

  rate(http_requests_total[10m+2s])

The computation of the calculations is done directly at the parse level and does not hit the PromQL Engine.
The lexer has also been updated and improved, in particular for subqueries.

Buxfix: rate(http_requests_total[0]) is no longer allowed.

Signed-off-by: Julien Pivotto <291750+roidelapluie@users.noreply.github.com>
@roidelapluie
PromQL duration expr: Add a check for durations our of range
9cfd6f7 
Signed-off-by: Julien Pivotto <291750+roidelapluie@users.noreply.github.com>
@roidelapluie
Update duration expr documentation
2e0a4ef 
Signed-off-by: Julien Pivotto <291750+roidelapluie@users.noreply.github.com>
@roidelapluie
Move calculation to the engine
2f6ad79 
Signed-off-by: Julien Pivotto <291750+roidelapluie@users.noreply.github.com>
@roidelapluie roidelapluie requested a review from krajorama April 3, 2025 11:17
@roidelapluie
Copy link
Member Author

I implemented tests in a different way, not testing the parser,can you have a look?

@roidelapluie
Address feedback
2cf2bf5 
Signed-off-by: Julien Pivotto <291750+roidelapluie@users.noreply.github.com>
krajorama
krajorama approved these changes Apr 4, 2025
View reviewed changes
Copy link
Member
@krajorama krajorama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with nit comment

@roidelapluie
Address feedback
e57512a 
Signed-off-by: Julien Pivotto <291750+roidelapluie@users.noreply.github.com>
@roidelapluie
Copy link
Member Author

cc @juliusv can you have a look?

@roidelapluie roidelapluie requested review from juliusv and removed request for juliusv April 14, 2025 07:48
@roidelapluie roidelapluie merged commit f4ca136 into prometheus:main Apr 22, 2025
27 checks passed
@roidelapluie
Copy link
Member Author

Merging because no input for > 1w and approved by krajo.

@zenador zenador mentioned this pull request Apr 22, 2025
Merged
56quarters added a commit to grafana/mimir that referenced this pull request Apr 23, 2025
@56quarters
Remove streamingpromql engine test with 0 range
2976973 
This test has been removed upstream after the PR for duration selector
math was merged.

See prometheus/prometheus#16249

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@zenador zenador mentioned this pull request Apr 24, 2025
Merged
4 tasks
zenador added a commit to grafana/mimir that referenced this pull request Apr 25, 2025
@zenador @56quarters
Update mimir-prom to v1.8.2-0.20250423083340-007de9b763aa (#11292)
bd39677 
* Update mimir-prometheus to v1.8.2-0.20250423083340-007de9b763aa

* Fix build

* Fix build problems from vendor due to pinning that is now irrelevant as dskit now has google.golang.org/grpc v1.71.1

* Fix test

* Remove streamingpromql engine test with 0 range

This test has been removed upstream after the PR for duration selector
math was merged.

See prometheus/prometheus#16249

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

* Remove more 0 length range selectors and sync tests

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

* Disable duration expression testing in MQE until we enable it

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

* Remove comment from test since it was breaking the unit test

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

* Add @56quarters 's changes to go.mod

---------

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
Co-authored-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@zenador zenador mentioned this pull request Apr 25, 2025
Closed
4 tasks
@krajorama krajorama mentioned this pull request Apr 30, 2025
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krajorama krajorama krajorama approved these changes

@juliusv juliusv Awaiting requested review from juliusv

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
3660 @roidelapluie @krajorama @juliusv @bboreham
0