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
Bug 1883200: fix silence toggle in devconsole monitoring #6756
Conversation
@vikram-raj: This pull request references Bugzilla bug 1883200, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In 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/test-infra repository. |
frontend/packages/dev-console/src/components/monitoring/alerts/monitoring-alerts-utils.tsx
Outdated
Show resolved
Hide resolved
/retest |
8f94aa9
to
61142c2
Compare
@vikram-raj: This pull request references Bugzilla bug 1883200, which is valid. 3 validation(s) were run on this bug
In 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/test-infra repository. |
/cc @invincibleJai |
/retest |
// eslint-disable-next-line promise/no-nesting | ||
coFetchJSON(`${ALERT_MANAGER_TENANCY_BASE_PATH}/api/v2/silences`) | ||
.then((silences) => { | ||
rule.silencedBy = _.filter( |
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.
rule
is a prop received by the component. ideally we should avoid modification of props. we could use a deep copy of the prop instead.
frontend/packages/dev-console/src/components/monitoring/alerts/SilenceDurationDropdown.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/dev-console/src/components/monitoring/alerts/SilenceDurationDropdown.scss
Outdated
Show resolved
Hide resolved
frontend/packages/dev-console/src/components/monitoring/alerts/SilenceDurationDropdown.tsx
Outdated
Show resolved
Hide resolved
4abdec3
to
5a9f1ad
Compare
@vikram-raj: This pull request references Bugzilla bug 1883200, which is valid. 3 validation(s) were run on this bug
In 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/test-infra repository. |
coFetchJSON.delete(`${alertManagerBaseURL}/api/v2/silence/${silence.id}`); | ||
coFetchJSON | ||
.delete(`${alertManagerBaseURL}/api/v2/silence/${silence.id}`) | ||
.then(() => { |
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.
In the admin Console, after deleting a silence, we call refreshNotificationPollers
, which immediately fetches the latest silences from Alertmanager and then triggers the evaluation of which alerts are silenced. I think that would achieve the same thing you are doing here in a simpler way.
frontend/packages/dev-console/src/components/monitoring/alerts/SilenceAlert.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/dev-console/src/components/monitoring/alerts/SilenceAlert.tsx
Outdated
Show resolved
Hide resolved
}; | ||
setSilencing(true); | ||
coFetchJSON | ||
.post(`${ALERT_MANAGER_TENANCY_BASE_PATH}/api/v2/silences`, payload) |
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.
Same comment as for the delete
. In the admin Console, after creating a silence, we call refreshNotificationPollers
to trigger a refresh of the alert's state
, so I think we could do something similar here.
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.
Our scenario is a little different. In our case, we need to trigger the refresh for rules and silences. But in refreshNotificationPollers
it is refreshing only silences.
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.
I looked into this some more and I still think we can simplify the code here by using refreshNotificationPollers
or a similar approach. However, I'm OK with addressing that with a follow up PR.
/retest |
frontend/packages/dev-console/src/components/monitoring/alerts/SilenceAlert.tsx
Outdated
Show resolved
Hide resolved
@vikram-raj there is improvement with handling toggle component based on issue description, but i see some delay still in silences getting updated and user can toggle again meanwhile |
@vikram-raj: This pull request references Bugzilla bug 1883200, which is valid. 3 validation(s) were run on this bug
In 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/test-infra repository. |
@invincibleJai I tested it locally and in the cluster as well. And did not notice any delay. |
1dd7da5
to
c03f2fd
Compare
/approve Thanks @vikram-raj looks good now after the changes, verified it again. cc @kyoto |
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.
Tested it locally, works as expected.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: invincibleJai, jerolimov, vikram-raj The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@vikram-raj: All pull requests linked via external trackers have merged: Bugzilla bug 1883200 has been moved to the MODIFIED state. In 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/test-infra repository. |
Fixes:
https://issues.redhat.com/browse/ODC-4423
GIF
Test Setup
oc apply -f https://gist.githubusercontent.com/vikram-raj/d801e9ef6400cc1bd5af5a336bbedfa0/raw/61d90b705617ba8070da80f8c97ac16376a9ca58/prometheus-example-app.yaml
oc apply -f https://gist.githubusercontent.com/vikram-raj/86300d923a7a3a3a3348e359327e2307/raw/535d889923141a3f3e26e355d6bccec4650a0fcd/user-workload-config.yaml
ns1
namespace. But in local setup all alerts and rules will be visible so, filter by nameCriticalAlert
and to test this PR.