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

[Metrics UI] Fixes for editing alerts in alert management #64597

Merged
merged 4 commits into from
Apr 28, 2020
Merged

[Metrics UI] Fixes for editing alerts in alert management #64597

merged 4 commits into from
Apr 28, 2020

Conversation

simianhacker
Copy link
Member

Summary

This PR closes #64012 by setting the sourceId when it's not properly set and this also fixes the missing filter and group-by fields.

@simianhacker simianhacker added release_note:fix Feature:Metrics UI Metrics UI feature v8.0.0 Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.8.0 v7.7.1 labels Apr 27, 2020
@simianhacker simianhacker requested a review from a team as a code owner April 27, 2020 22:55
phillipb
phillipb approved these changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@phillipb phillipb left a comment

Choose a reason for hiding this comment

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

LGTM

phillipb
phillipb suggested changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@phillipb phillipb left a comment

Choose a reason for hiding this comment

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

Oops

phillipb
phillipb suggested changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@phillipb phillipb left a comment

Choose a reason for hiding this comment

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

Can't clear the alert per selection on the edit screen.

x-pack/plugins/infra/public/components/alerting/metrics/expression.tsx
compressed
>
<MetricsExplorerGroupBy
onChange={onGroupByChange}
Copy link
Contributor

Choose a reason for hiding this comment

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

There's actually a bug in the underlying alert library that won't let us setAlertParams('groupBy', undefined), so this is causing a bug with clearing out the group by field in edit. We should update:

kibana/x-pack/plugins/infra/public/components/alerting/metrics/expression.tsx

Lines 151 to 152 in 0db04a7

setAlertParams('groupBy', group || undefined);
},
to be setAlertParams('groupBy', group || '');.

The bug in alerting is here:

kibana/x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_reducer.ts

Lines 78 to 95 in daf6226

case 'setAlertParams': {
const { key, value } = payload;
if (isEqual(alert.params[key], value)) {
return state;
} else {
return {
...state,
alert: {
...alert,
params: {
...alert.params,
[key]: value,
},
},
};
}
}
case 'setAlertActionParams': {

phillipb
phillipb suggested changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@phillipb phillipb left a comment

Choose a reason for hiding this comment

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

My bad, my comment wasn't clear enough.

x-pack/plugins/infra/public/components/alerting/metrics/expression.tsx Outdated
@@ -211,14 +212,24 @@ export const Expressions: React.FC<Props> = props => {
setAlertParams('filterQuery', filter);
}

setAlertParams('groupBy', md.currentOptions.groupBy);
setAlertParams('groupBy', md.currentOptions.groupBy || '');
Copy link
Contributor

Choose a reason for hiding this comment

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

This call was actually not the problem. The one that needs to be updated is the onGroupByChange here:

kibana/x-pack/plugins/infra/public/components/alerting/metrics/expression.tsx

Lines 148 to 153 in 12aae67

const onGroupByChange = useCallback(
(group: string | null) => {
setAlertParams('groupBy', group || undefined);
},
[setAlertParams]
);

phillipb
phillipb approved these changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@phillipb phillipb left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@simianhacker simianhacker merged commit 4a04adf into elastic:master Apr 28, 2020
@simianhacker simianhacker mentioned this pull request Apr 28, 2020
Merged
simianhacker added a commit to simianhacker/kibana that referenced this pull request Apr 28, 2020
@simianhacker
[Metrics UI] Fixes for editing alerts in alert management (elastic#64597
7421d1d 
)

* [Metrics UI] Fixes for editing alerts in alert management

* Change EuiFieldSearch to use onChange instead of onSearch

* Fixing groupBy

* Fixing the correct groupBy
simianhacker added a commit to simianhacker/kibana that referenced this pull request Apr 28, 2020
@simianhacker
[Metrics UI] Fixes for editing alerts in alert management (elastic#64597
fc35a36 
)

* [Metrics UI] Fixes for editing alerts in alert management

* Change EuiFieldSearch to use onChange instead of onSearch

* Fixing groupBy

* Fixing the correct groupBy
@simianhacker simianhacker mentioned this pull request Apr 28, 2020
Merged
simianhacker added a commit that referenced this pull request Apr 28, 2020
@simianhacker
[Metrics UI] Fixes for editing alerts in alert management (#64597) (#…
acbc8da 
…64671)

* [Metrics UI] Fixes for editing alerts in alert management

* Change EuiFieldSearch to use onChange instead of onSearch

* Fixing groupBy

* Fixing the correct groupBy
simianhacker added a commit that referenced this pull request Apr 28, 2020
@simianhacker
[Metrics UI] Fixes for editing alerts in alert management (#64597) (#…
f7893a7 
…64672)

* [Metrics UI] Fixes for editing alerts in alert management

* Change EuiFieldSearch to use onChange instead of onSearch

* Fixing groupBy

* Fixing the correct groupBy
@simianhacker simianhacker deleted the issue-64012-fix-alert-edit branch April 17, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phillipb phillipb phillipb approved these changes

Assignees
No one assigned
Labels
Feature:Metrics UI Metrics UI feature release_note:fix Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.7.1 v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
3 participants
@simianhacker @kibanamachine @phillipb