-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Metrics UI] Fixes for editing alerts in alert management #64597
[Metrics UI] Fixes for editing alerts in alert management #64597
Conversation
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.
LGTM
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.
Oops
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.
Can't clear the alert per selection on the edit screen.
compressed | ||
> | ||
<MetricsExplorerGroupBy | ||
onChange={onGroupByChange} |
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.
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); | |
}, |
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': { |
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.
My bad, my comment wasn't clear enough.
@@ -211,14 +212,24 @@ export const Expressions: React.FC<Props> = props => { | |||
setAlertParams('filterQuery', filter); | |||
} | |||
|
|||
setAlertParams('groupBy', md.currentOptions.groupBy); | |||
setAlertParams('groupBy', md.currentOptions.groupBy || ''); |
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.
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] | |
); |
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.
LGTM
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
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.