-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
fix: update prom rule to use range query #4461
Conversation
WalkthroughThe recent changes focus on enhancing support for PromQL based alerts, specifically by introducing more flexible evaluation windows and improving the logic for handling alerts. This includes changes to how alert queries are run, the introduction of more sophisticated comparison and match types, and improvements to the clarity and functionality of the code. These updates aim to address specific issues related to PromQL alert evaluation times and discrepancies in lookback deltas between different types of queries. Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
…o use-range-query-promql
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (5)
- frontend/src/container/FormAlertRules/RuleOptions.tsx (4 hunks)
- pkg/query-service/pqlEngine/engine.go (3 hunks)
- pkg/query-service/rules/promRule.go (7 hunks)
- pkg/query-service/rules/promrule_test.go (1 hunks)
- pkg/query-service/rules/thresholdRule.go (1 hunks)
Files skipped from review due to trivial changes (1)
- pkg/query-service/rules/thresholdRule.go
Additional comments: 3
frontend/src/container/FormAlertRules/RuleOptions.tsx (1)
- 174-175: The use of
renderPromEvalWindows
inrenderPromRuleOptions
correctly implements the PR's objective to introduce Prometheus-specific evaluation windows. However, as mentioned earlier, the current implementation does not differentiate the options from the standard evaluation windows. Ensure the options provided byrenderPromEvalWindows
are tailored to Prometheus-specific needs or clarify the need for this separation.pkg/query-service/rules/promRule.go (2)
- 119-121: The use of a unit converter in the
targetVal
method to handle different units of measurement is a good enhancement, ensuring that alert thresholds are correctly interpreted regardless of the unit specified. This change supports the PR's objective to enhance alert evaluation logic.- 336-348: The introduction of
matchType
andcompareOp
methods inpromRule.go
improves code readability and maintainability by encapsulating the logic for retrieving the match type and comparison operation from the rule condition. This is a positive change that aligns with best practices for code organization.
You mean |
Yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- pkg/query-service/rules/thresholdRule.go (1 hunks)
Files skipped from review due to trivial changes (1)
- pkg/query-service/rules/thresholdRule.go
The comments are non-blocking. I will merge this for now and address them in follow up PR. |
Summary
Fixes #4038
Fixes #3917
What do we support today:
Instant query with only
At least once
comparison condition.Issues with the current promql alerts
On average
,In total
andAll the times
are not supportedWhat users see in the chart UI when working with promql comes from the Range query API. They expect the same when creating alerts. For instance, there is no way to set an alert if the latency is greater than 800ms for the last 10 minutes. It's common to have occasional spikes and triggering alerts with
At least once
can be noisy.This PR moves to RangeQuery instead of InstantQuery.
Summary by CodeRabbit
Summary by CodeRabbit