-
Notifications
You must be signed in to change notification settings - Fork 38
OU-632: Split ALERTS query_range into several requests for Incidents #678
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
base: main
Are you sure you want to change the base?
Conversation
|
@rioloc: This pull request references OU-632 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
1b594e8 to
7ca9eb0
Compare
|
/retest |
756ba1e to
a1bef57
Compare
|
@rioloc: This pull request references OU-632 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@rioloc: This pull request references OU-632 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
falox
left a comment
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.
a couple of minor points
/lgtm
@DavidRajnoha @rioloc make sure to test what happens when one of the subqueries fails (expected: the parent call fails). Also, make sure that the splitting + merging doesn't affect the ordering of the data points (probably covered in the unit tests, but I comment anyway).
If I well understood the point, that is guaranteed by the |
Also for this the order should be guaranteed to be deterministic, so the one of the results of |
|
/label qe-approved |
|
@rioloc: This pull request references OU-632 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Assisted-By: Claude Code
6a724e5 to
b9ce5f8
Compare
Assisted-By: Claude Code
6dc32da to
d0a451c
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: falox, PeterYurkovich, rioloc The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: falox, PeterYurkovich, rioloc The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/override ci/prow/okd-scos-images |
|
@PeterYurkovich: Overrode contexts on behalf of PeterYurkovich: ci/prow/okd-scos-images DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Currently okd image tests are down across the whole openshift org, overriding the test |
|
/hold Revision d0a451c was retested 3 times: holding |
|
/test e2e-monitoring |
|
@jgbernalp: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override ci/prow/okd-scos-images |
|
@jgbernalp: Overrode contexts on behalf of jgbernalp: ci/prow/okd-scos-images DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test e2e-monitoring |
|
@rioloc: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This PR introduces the following changes:
ALERTS expression simplified
ALERTS{}. The second half of the query,+ on () group_left (component, layer) (absent(meta{layer="${query.layer}", component="${query.component}"}))), was removed becausecomponentandlayerlabels are already present in the incident entity.Split of
query_rangerequest into multiple ones for long queriesALERTS{}joined viaorcondition, exceeds aMAX_URL_LENGTHlimit, then several queries will be created instead.fetchDataForIncidentsAndAlertswas updated to support both single and multiple queries. In the case of multiple queries then multiple requests will be triggered to the Prometheus API.skip alert if no incident is matched
matchinIncidentis found. This should be an edge case that could happen if an alert is triggered and a new incident metric was not produced yet.Assisted-By: Claude Code