-
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
[ResponseOps][BUG] - UI fixes for Alerts Summary chart #137476
[ResponseOps][BUG] - UI fixes for Alerts Summary chart #137476
Conversation
@elasticmachine merge upstream |
Pinging @elastic/response-ops (Team:ResponseOps) |
Pinging @elastic/actionable-observability (Team: Actionable Observability) |
/oblt-deploy |
// If there are alerts in this day, we construct the chart data. | ||
return [ | ||
...acc, | ||
...dayAlerts.alertStatus.buckets.map((alert) => ({ |
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 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.
@XavierM, excellent point. Actually, totalDayAlert
was added to each day and calculated based on the number of alerts for that day.
But then once the 2nd of alerts kicked in. The chart flipped upside down means the totalDayAlert
(grey color) becomes at the bottom and the recovered and active at the top. I tried to fix that in many ways this morning but with no success and I don't want to spend more time on it as we all agreed.
BTW, when we hover over the bar the gray bar appears again.
I think this limitation is ok for this version, what do you think @maciejforcone?
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 think we can do it just like that -> https://github.com/fkanout/kibana/pull/3/files
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 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 failed.... 😞 but I was also looking everywhere in kibana, and it looks like that nobody is doing the fake background like we do, maybe for good reason. I am wondering if we should remove it because if we are having alerts/data, we most likely not going to see this grey background anyway. It is better to be simplistic sometime.
what do you think?
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.
@XavierM, no worries! BTW, the only way I found to make it works is by making chart reading starting from the left, which means the today bar will be at the very left bar.
To answer your question. I don't mind removing the background/reference bars.
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.
...ui/public/application/sections/rule_details/components/alert_summary/rule_alerts_summary.tsx
Show resolved
Hide resolved
...ck/plugins/triggers_actions_ui/public/application/hooks/use_load_rule_alerts_aggregations.ts
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.
#137476 (review) |
since it's a chart - it can be just flat, 30 empty bars, so it doesn't look broken. We don't need to repeat text "no alerts" because we already have three zeros. |
@elasticmachine merge upstream |
Review Idea to always get a background on the graph
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @fkanout |
* Use more efficient code/query, and add the 3rd bar (base one) * Update comment * Update comment * Add the shade color to the base bar * Update comment * Remove unused else * add tooltip and fix base bar * Update design * Add error handling message * review idea * fix no alerts + graph to get greyed out * review I * Fix the order of alertsChartData * Fix design and color scheme Co-authored-by: Xavier Mouligneau <[email protected]> (cherry picked from commit 521c2a4)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…7840) * Use more efficient code/query, and add the 3rd bar (base one) * Update comment * Update comment * Add the shade color to the base bar * Update comment * Remove unused else * add tooltip and fix base bar * Update design * Add error handling message * review idea * fix no alerts + graph to get greyed out * review I * Fix the order of alertsChartData * Fix design and color scheme Co-authored-by: Xavier Mouligneau <[email protected]> (cherry picked from commit 521c2a4) Co-authored-by: Faisal Kanout <[email protected]>
Summary
It fixes #137292 by adding:
1- Tooltip once hovering on the bar
2- Add a base/background bar that presents the total alerts in the scope of 30 days
3- Update the design and add
Last 30 days
4- Update ES query to be more efficient
It fixes #137417 by adding a proper error message