-
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
[Monitoring] Some progress on making alerts better in the UI #81569
[Monitoring] Some progress on making alerts better in the UI #81569
Conversation
x-pack/plugins/monitoring/public/components/elasticsearch/node/advanced.js
Outdated
Show resolved
Hide resolved
anchorPosition="downLeft" | ||
> | ||
<EuiContextMenu | ||
key={`${showByNode ? 'byNode' : 'byType'}_${panels.length}`} |
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 don't need a key
here since since it's not iterated (at its nested scope).
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.
Sure, I can try to remove it. I originally added it because I saw strange behavior when toggling between the modes and React refusing to render any changes.
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 ended up adding this back in because I saw the same behavior. I'd be happy to solve the issue another way if you have ideas.
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.
@chrisronline Looking good 👍 Thank you for getting the changes in so quickly!
A couple more things:
- If there are multiple alert types on the node’s detail page and you disabling one of them (via the call out panel), it switches and expands the next alert type below it (might take a few seconds) with the same disabled state (even though it’s not disabled):
This is after I disabled the the CPU Usage alert above it
I think we should "Group by alert type" by default, and the switch should always be labeled "Group by node" which could be in either on or off state
This seems to be how the
Nice find!
I'm sorry, I don't know what you mean. It seems to work fine for me.
I feel like the off state isn't obvious to users. I like the idea of labeling each state explicitly. Perhaps we can change the UI toggle - it doesn't have to be a
Yup, i'll fix this up.
Fixed up too |
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.
Great job! I love this new alerting experience 😍
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Distributable file count
Page load bundle
History
To update your PR or re-run it, just comment with: |
…#81569) * Some progress on making alerts better in the UI * Handle edge case * Updates * More updates * Show kibana instances alerts better * Stop showing missing nodes and improve the detail alert UI * WIP * Fix the badge display * Okay I think this is finally working * Fix type issues * Fix tests * Fix tests * Fix alert counts * Fix setup mode listing * Better detail page view of alerts * Feedback * Sorting * Fix a couple small issues * Start of unit tests * I don't think we need this Mock type * Fix types * More tests * Improve tests and fix sorting * Make this test more resilient * Updates after merging master * Fix tests * Fix types, and improve tests * PR comments * Remove nextStep logic * PR feedback * PR feedback * Removing unnecessary changes * Fixing bad merge issues * Remove unused imports * Add tooltip to alerts grouped by node * Fix up stateFilter usage * Code clean up * PR feedback * Fix state filtering in the category list * Fix types * Fix test * Fix types * Update snapshots Co-authored-by: Kibana Machine <[email protected]>
…#85719) * Some progress on making alerts better in the UI * Handle edge case * Updates * More updates * Show kibana instances alerts better * Stop showing missing nodes and improve the detail alert UI * WIP * Fix the badge display * Okay I think this is finally working * Fix type issues * Fix tests * Fix tests * Fix alert counts * Fix setup mode listing * Better detail page view of alerts * Feedback * Sorting * Fix a couple small issues * Start of unit tests * I don't think we need this Mock type * Fix types * More tests * Improve tests and fix sorting * Make this test more resilient * Updates after merging master * Fix tests * Fix types, and improve tests * PR comments * Remove nextStep logic * PR feedback * PR feedback * Removing unnecessary changes * Fixing bad merge issues * Remove unused imports * Add tooltip to alerts grouped by node * Fix up stateFilter usage * Code clean up * PR feedback * Fix state filtering in the category list * Fix types * Fix test * Fix types * Update snapshots Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
* master: (116 commits) Fix UX E2E tests (elastic#85722) Increasing default api key removalDelay to 1h (elastic#85576) align cors settings names with elasticsearch (elastic#85738) unskip tests and make sure submit is not triggered too quickly (elastic#85567) Row trigger 2 (elastic#83167) Add session id to audit log (elastic#85451) [TSVB] Fields lists do not populate all the times (elastic#85530) [Visualize] Removes the external link icon from OSS badges (elastic#85580) fixes EQL tests (elastic#85712) [APM] enable 'log_level' for Go (elastic#85511) ini `1.3.5` -> `1.3.7` (elastic#85707) Fix fleet route protections (elastic#85626) [Monitoring] Some progress on making alerts better in the UI (elastic#81569) [Security Solution] Refactor Timeline Notes to use EuiCommentList (elastic#85256) [Security Solution][Detections][Threshold Rules] Threshold rule exceptions (elastic#85103) [Security Solution] Alerts details (elastic#83963) skip flaky suite (elastic#62060) skip flaky suite (elastic#85098) skip flaky suite (elastic#84020) skip flaky suite (elastic#85671) ...
Relates to #80397
This PR does a few things to improve how we surface alerts in the Stack Monitoring UI.
We currently do not handle a bunch of alerts firing at the same time well. Per @ravikesarwani's suggestion and @igoristic's suggestion, we should group this better and offer the user a way to change between the grouping.
In addition, alerts presence on the individual node/instance listing pages can be improved as well, per a suggestion.
Screenshots