Skip to content
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

OU-206: Merge monitoring alerts with alerts from other sources in the dev console #12940

Merged
merged 1 commit into from Aug 17, 2023
Merged

OU-206: Merge monitoring alerts with alerts from other sources in the dev console #12940

merged 1 commit into from Aug 17, 2023

Conversation

jgbernalp
Copy link
Contributor

@jgbernalp jgbernalp commented Jun 26, 2023 •
edited

This PR allows plugins to contribute extensions to add new alerting rules sources in the dev console

https://issues.redhat.com/browse/OU-206

Screen.Recording.2023-06-29.at.12.10.23.mov

@openshift-ci openshift-ci bot added component/core Related to console core functionality component/dev-console Related to dev-console component/monitoring Related to monitoring component/sdk Related to console-plugin-sdk labels Jun 26, 2023
@jgbernalp jgbernalp force-pushed the add-dev-console-log-based-alerts branch from 15d031a to 4b098e8 Compare June 27, 2023 12:24
kyoto
kyoto suggested changes Jun 28, 2023
View reviewed changes
...d/packages/dev-console/src/components/monitoring/alerts/MonitoringAlertsRulesDetailsPage.tsx Outdated
Comment on lines 7 to 11
import { useResolvedExtensions } from '@console/dynamic-plugin-sdk';
import {
AlertingRulesSourceExtension,
isAlertingRulesSource,
} from '@console/dynamic-plugin-sdk/src/extensions/alerts';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { useResolvedExtensions } from '@console/dynamic-plugin-sdk';
import {
AlertingRulesSourceExtension,
isAlertingRulesSource,
} from '@console/dynamic-plugin-sdk/src/extensions/alerts';
import {
AlertingRulesSourceExtension,
isAlertingRulesSource,
useResolvedExtensions,
} from '@console/dynamic-plugin-sdk';

Maybe combine the dynamic-plugin-sdk imports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not all packages are exported in the main @console/dynamic-plugin-sdk so a full path is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the alerts to the main package export in the SDK

...d/packages/dev-console/src/components/monitoring/alerts/MonitoringAlertsRulesDetailsPage.tsx
const alertsSource = React.useMemo(
() =>
customExtensions
.filter((extension) => extension.properties.contextId === 'dev-observe-alerting')
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment explaining where dev-observe-alerting comes from?

...d/packages/dev-console/src/components/monitoring/alerts/MonitoringAlertsRulesDetailsPage.tsx Outdated
});
dispatch(alertingLoading('alerts', 'dev'));

const poller = (): void => {
Copy link
Member

Choose a reason for hiding this comment

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

I notice that this code is almost the same as the poller we have in /components/monitoring/alerting.tsx. I think it would be worth extracting this into a single helper function.

frontend/packages/dev-console/src/components/monitoring/alerts/MonitoringAlerts.tsx Outdated
});
dispatch(alertingLoading('alerts', 'dev'));

const poller = (): void => {
Copy link
Member

Choose a reason for hiding this comment

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

The two pollers in this PR and the poller in /components/monitoring/alerting.tsx are very similar, so I think it would be good to combine them all.

frontend/packages/dev-console/src/components/monitoring/alerts/MonitoringAlerts.tsx
@@ -123,10 +175,10 @@ export const MonitoringAlerts: React.FC<Props> = ({ match, rules, filters, listS
);

const Content = React.useMemo(() => {
if (loading && !loadError) {
if (!alerts?.loaded && !alerts?.loadError) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we also need to handle the case where the alerts are loaded, but the rules are pending here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alerts are extracted from rules so they are loaded at the same time

@jgbernalp jgbernalp force-pushed the add-dev-console-log-based-alerts branch from 4b098e8 to 552fdae Compare June 28, 2023 06:19
@kyoto
Copy link
Member

kyoto commented Jun 29, 2023

/lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 29, 2023
@jgbernalp
Copy link
Contributor Author

/assign @yapei @opayne1 @RickJWagner
for approvals

@jgbernalp
Copy link
Contributor Author

/jira refresh

@openshift-ci-robot
Copy link
Contributor

@jgbernalp: No Jira issue is referenced in the title of this pull request.
To reference a jira issue, add 'XYZ-NNN:' to the title of this pull request and request another refresh with /jira refresh.

In response to this:

/jira refresh

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.

@jgbernalp jgbernalp changed the title Merge monitoring alerts with alerts from other sources in the dev console OU-206: Merge monitoring alerts with alerts from other sources in the dev console Jun 29, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jun 29, 2023 •
edited by openshift-ci bot

@jgbernalp: This pull request references OU-206 which is a valid jira issue.

In response to this:

This PR allows plugins to contribute extensions to add new alerting rules sources in the dev console

https://issues.redhat.com/browse/OU-206

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.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 29, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jun 29, 2023 •
edited by openshift-ci bot

@jgbernalp: This pull request references OU-206 which is a valid jira issue.

In response to this:

This PR allows plugins to contribute extensions to add new alerting rules sources in the dev console

https://issues.redhat.com/browse/OU-206

Screen.Recording.2023-06-29.at.12.10.23.mov

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.

RickJWagner
RickJWagner reviewed Jun 29, 2023
View reviewed changes
Copy link

@RickJWagner RickJWagner left a comment

Choose a reason for hiding this comment

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

Looks good.

@RickJWagner
Copy link

/label px-approved

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Jun 29, 2023
@RickJWagner RickJWagner removed their assignment Jun 29, 2023
@yapei
Copy link
Contributor

yapei commented Jun 30, 2023

@kabirbhartiRH from logging team will test the PR

@jgbernalp
Copy link
Contributor Author

/assign @kabirbhartiRH

@kabirbhartiRH
Copy link

@jgbernalp @yapei We have a blocker bug https://issues.redhat.com/browse/LOG-4240 on Logging 5.8.0
I am not sure the PR can be tested at this time?

@opayne1
Copy link
Contributor

opayne1 commented Jul 20, 2023

/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Jul 20, 2023
@jgbernalp jgbernalp force-pushed the add-dev-console-log-based-alerts branch from 552fdae to d20929a Compare August 16, 2023 07:52
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2023
@kyoto
Copy link
Member

kyoto commented Aug 16, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2023
@jgbernalp jgbernalp force-pushed the add-dev-console-log-based-alerts branch from d20929a to f841126 Compare August 16, 2023 09:37
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2023
@kyoto
Copy link
Member

kyoto commented Aug 16, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 16, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jgbernalp, kyoto

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kabirbhartiRH
Copy link

/qe-approved

@jgbernalp
Copy link
Contributor Author

/retest

1 similar comment
@jgbernalp
Copy link
Contributor Author

/retest

@jgbernalp
Copy link
Contributor Author

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Aug 16, 2023
@jgbernalp
Copy link
Contributor Author

/retest

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 6143838 and 2 for PR HEAD f841126 in total

@jgbernalp
Copy link
Contributor Author

/retest

1 similar comment
@jgbernalp
Copy link
Contributor Author

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 16, 2023

@jgbernalp: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 8c2c7b4 into openshift:master Aug 17, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RickJWagner RickJWagner RickJWagner left review comments

@kyoto kyoto kyoto requested changes

@christianvogt christianvogt Awaiting requested review from christianvogt

Assignees

@kyoto kyoto

@yapei yapei

@kabirbhartiRH kabirbhartiRH

@opayne1 opayne1

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dev-console Related to dev-console component/monitoring Related to monitoring component/sdk Related to console-plugin-sdk docs-approved Signifies that Docs has signed off on this PR jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@jgbernalp @kyoto @openshift-ci-robot @RickJWagner @yapei @kabirbhartiRH @opayne1 @openshift-merge-robot