-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Alerting UI] Display a banner to users when some alerts have failures, added alert statuses column and filters #79038
[Alerting UI] Display a banner to users when some alerts have failures, added alert statuses column and filters #79038
Conversation
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
…ner-management-ui
…anner-management-ui # Conflicts: # x-pack/plugins/alerts/common/alert.ts # x-pack/plugins/triggers_actions_ui/public/application/sections/alert_details/components/alert_details.test.tsx
...ugins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.tsx
Show resolved
Hide resolved
...ugins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.tsx
Show resolved
Hide resolved
...ugins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.tsx
Outdated
Show resolved
Hide resolved
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 finally got to see the new status functionality and it is really amazing! Great work @YulNaumenko and @pmuellr ❤️
I finished my review, a bunch of nits, some UX questions and a few bugs. If I don't get to re-review before vacation, you can defer to someone else to verify the feedback is fixed 👍
{ | ||
field: 'executionStatus', | ||
name: i18n.translate( | ||
'xpack.triggersActionsUI.sections.alertsList.alertsListTable.columns.statusTitle', | ||
{ defaultMessage: 'Status' } | ||
), | ||
sortable: false, | ||
truncateText: false, | ||
'data-test-subj': 'alertsTableCell-status', | ||
render: (executionStatus: AlertExecutionStatus) => { | ||
const healthColor = getHealthColor(executionStatus.status); | ||
return <EuiHealth color={healthColor}>{executionStatus.status}</EuiHealth>; | ||
}, | ||
}, |
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 recall some of @mdefazio's mock ups had the column a bit further to the right (right of tags?). I'm just asking the question if the first column is where we want this?
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.
We have gone back and forth a bit on this. I like it on the left since I think users will want to see this information first. Perhaps we will find out more after it's used more, but this feels right to me.
...s/triggers_actions_ui/public/application/sections/alert_details/components/alert_details.tsx
Outdated
Show resolved
Hide resolved
<EuiButton color="danger" onClick={() => setDissmissAlertErrors(true)}> | ||
<FormattedMessage | ||
id="xpack.triggersActionsUI.sections.alertDetails.alertInstances.dismissButtonTitle" | ||
defaultMessage="Dismiss" |
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.
@mdefazio did we decide to keep or remove the dismiss button? When dismissing, the banner returns next time I refresh the page.
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.
If it's coming back after each refresh, then I don't think we include the dismiss option. It would be more annoying to dismiss it and it keep coming back as opposed to always seeing and not being able to do anything with it (until the errors are fixed)
...ugins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.tsx
Show resolved
Hide resolved
...s/triggers_actions_ui/public/application/sections/alert_details/components/alert_details.tsx
Outdated
Show resolved
Hide resolved
...s/triggers_actions_ui/public/application/sections/alert_details/components/alert_details.tsx
Outdated
Show resolved
Hide resolved
alertStatusesFilter: [status], | ||
}); | ||
setAlertsStatusesTotal({ ...alertsStatuses, [status]: alertsTotalResponse.total }); | ||
alertsStatuses = { ...alertsStatuses, [status]: alertsTotalResponse.total }; |
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.
nit: the usage of alertsStatuses
is a bit hard to understand. Would it be possible to use a mix of await Promise.all([...]);
and AlertExecutionStatusValues.map(async (status: string) => { ... });
and then call setAlertsStatusesTotal
once all the calls are finished?
<EuiButtonEmpty color="danger" onClick={() => setDissmissAlertErrors(true)}> | ||
<FormattedMessage | ||
id="xpack.triggersActionsUI.sections.alertsList.dismissBunnerButtonLabel" | ||
defaultMessage="Dismiss" |
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 question in regards to dismiss. cc @mdefazio
...ugins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.tsx
Outdated
Show resolved
Hide resolved
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.
Looking excellent! Just saw Mike's review, he noted some of the same things I did.
We'll need some e2e tests here - you can look at the functional tests I added for executionStatus on the server for how to generate almost all of the possible combinations of things, with our fixture alerts. Given FF, I'm fine merging this now and adding the tests next week or so ...
@@ -97,6 +99,9 @@ export async function loadAlerts({ | |||
].join('') | |||
); | |||
} | |||
if (alertStatusesFilter && alertStatusesFilter.length) { | |||
filters.push(`alert.attributes.executionStatus.status:(${alertStatusesFilter.join(' or ')})`); |
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.
TIL! Didn't realize you could do the ORs like this in KQL! Of course, I don't know KQL very well :-)
Actually, I thought KQL was sensitive to the case, and it had to be OR
and not or
, but maybe the rules are different outside of parenthesis ... or I'm just wrong on the case sensitivity.
...s/triggers_actions_ui/public/application/sections/alert_details/components/alert_details.tsx
Outdated
Show resolved
Hide resolved
...s/triggers_actions_ui/public/application/sections/alert_details/components/alert_details.tsx
Outdated
Show resolved
Hide resolved
...iggers_actions_ui/public/application/sections/alerts_list/components/alert_status_filter.tsx
Outdated
Show resolved
Hide resolved
...ugins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.tsx
Show resolved
Hide resolved
...ugins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.tsx
Outdated
Show resolved
Hide resolved
...ugins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.tsx
Outdated
Show resolved
Hide resolved
.../triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.test.tsx
Show resolved
Hide resolved
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.
Banner 1
Error found in 1 alert
Error found in 2 alerts
View | Dismiss
Banner 2
Title:
An error occurred when reading the alert
An error occurred when decrypting the alert
An error occurred when running the alert
An error occurred for unknown reasons
Description:
Include the error message. The text "Error Message" is not needed.
Dismiss
Note that the ci error "api integration spaces only Alerting executionStatus should eventually be "ok" for no-op alert" is my fault - I believe that test has been skipped as it's flaky, so a merge master should make it go away ... |
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.
LGTM, just a few missed i18n, great work!
...s/triggers_actions_ui/public/application/sections/alert_details/components/alert_details.tsx
Outdated
Show resolved
Hide resolved
...s/triggers_actions_ui/public/application/sections/alert_details/components/alert_details.tsx
Outdated
Show resolved
Hide resolved
...ugins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.tsx
Outdated
Show resolved
Hide resolved
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.
Looks great!
💚 Build SucceededMetrics [docs]@kbn/optimizer bundle module count
async chunks size
History
To update your PR or re-run it, just comment with: |
…s, added alert statuses column and filters (elastic#79038) * Added ui for alert failures banner * Added UI for alerts statuses * Adjusted form * Added banned on the details page * Fixed failing intern. check and type checks * Added unit test for displaying alert error banner * Fixed type check * Fixed due to comments * Changes due to comments * Fixed due to comments * Fixed text on banners * Added i18n translations
…s, added alert statuses column and filters (#79038) (#79403) * Added ui for alert failures banner * Added UI for alerts statuses * Adjusted form * Added banned on the details page * Fixed failing intern. check and type checks * Added unit test for displaying alert error banner * Fixed type check * Fixed due to comments * Changes due to comments * Fixed due to comments * Fixed text on banners * Added i18n translations
* master: (128 commits) add core-js production dependency (elastic#79395) Add support for sharing saved objects to all spaces (elastic#76132) [Alerting UI] Display a banner to users when some alerts have failures, added alert statuses column and filters (elastic#79038) load js-yaml lazily (elastic#79092) skip flaky suite (elastic#77278) Fix agentPolicyUpdateEventHandler() to use app context soClient for creation of actions (elastic#79341) [Security Solution] Untitled Timeline created when first action is to add note (elastic#78988) [Security Solutions][Detection Engine] Updates the edit rules page to:wq! only have what is selected for editing (elastic#79233) Cleanup yarn.lock from duplicates (elastic#66617) [kbn/optimizer] implement more efficient auto transpilation for node (elastic#79052) [Ingest Manager] Rename Fleet setup and requirement, Fleet => Central… (elastic#79291) [core/server/plugins] don't run discovery in dev server parent process (take 2) (elastic#79358) [babel/register] remove from build (take 2) (elastic#79379) [Security Solution] Changes rules table tag display (elastic#77102) define integrationTestRoot in config file and use to define screensho… (elastic#79247) Revert "[babel/register] remove from build (elastic#79176)" skip flaky suite (elastic#75241) [Uptime] Synthetics UI (elastic#77960) [Security Solution] [Detections] Only display actions options if user has "read" privileges (elastic#78812) [babel/register] remove from build (elastic#79176) ...
* master: (288 commits) add core-js production dependency (elastic#79395) Add support for sharing saved objects to all spaces (elastic#76132) [Alerting UI] Display a banner to users when some alerts have failures, added alert statuses column and filters (elastic#79038) load js-yaml lazily (elastic#79092) skip flaky suite (elastic#77278) Fix agentPolicyUpdateEventHandler() to use app context soClient for creation of actions (elastic#79341) [Security Solution] Untitled Timeline created when first action is to add note (elastic#78988) [Security Solutions][Detection Engine] Updates the edit rules page to only have what is selected for editing (elastic#79233) Cleanup yarn.lock from duplicates (elastic#66617) [kbn/optimizer] implement more efficient auto transpilation for node (elastic#79052) [Ingest Manager] Rename Fleet setup and requirement, Fleet => Central… (elastic#79291) [core/server/plugins] don't run discovery in dev server parent process (take 2) (elastic#79358) [babel/register] remove from build (take 2) (elastic#79379) [Security Solution] Changes rules table tag display (elastic#77102) define integrationTestRoot in config file and use to define screensho… (elastic#79247) Revert "[babel/register] remove from build (elastic#79176)" skip flaky suite (elastic#75241) [Uptime] Synthetics UI (elastic#77960) [Security Solution] [Detections] Only display actions options if user has "read" privileges (elastic#78812) [babel/register] remove from build (elastic#79176) ...
Extended Alerting Management UI with alert statuses info, added errors banner and filtering by Status.
Details page with error banner:
Resolve #74778