-
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
[Infra UI] Add alerts to asset details flyout #161677
[Infra UI] Add alerts to asset details flyout #161677
Conversation
…-add-alerts-to-asset-details-flyout
…-add-alerts-to-asset-details-flyout
…-add-alerts-to-asset-details-flyout
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
…-ref HEAD~1..HEAD --fix'
…-add-alerts-to-asset-details-flyout
…ttps://github.com/jennypavlova/kibana into 160371-infra-ui-add-alerts-to-asset-details-flyout
@@ -39,7 +38,7 @@ export const AlertFlyout = ({ options, nodeType, filter, visible, setVisible }: | |||
options, | |||
nodeType, | |||
filter, | |||
customMetrics, | |||
customMetrics: inventoryPrefill?.customMetrics ?? [], |
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 is needed for the story (otherwise the storybook shows an error). Not sure if we can do something better there to avoid this change.
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.
Would it work if you did this instead?
const { inventoryPrefill } = useAlertPrefillContext();
const { customMetrics = [] } = inventoryPrefill ?? {};
Does the problem happen because the customMetrics
is null? I think your approach is ok, btw.
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.
The problem is that the inventoryPrefill
is undefined
when the component is used in the storybook context- that's why I used inventoryPrefill?.customMetrics
. Your idea is good I will try it out
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.
Done ✅
import { calculateTimeRangeBucketSize, getAlertSummaryTimeRange, useTimeBuckets } from '..'; | ||
import { DEFAULT_INTERVAL, DEFAULT_DATE_FORMAT } from '../constants'; | ||
|
||
export const useSummaryTimeRange = (unifiedSearchDateRange: TimeRange) => { |
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 we use this abstraction in other usages of AlertSummaryWidget? It would be nice to improve this part also for other AlertSummaryWidgets if it is possible :)
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 you think it can help on the alerts and overview page feel free to open an issue to replace it there after this PR is merged 🙂 I am not super familiar with the implementation there so I would say that that's a bit out of the scope of this PR (already getting big and I see it's hard to get a review already 😅 )
…-add-alerts-to-asset-details-flyout
@maryam-saeidi I am not sure if I can do it on the outside as I don't know at this moment if there is a new alert for the host so I need to refresh after creating a rule and if the alert is triggered (at this point I only know if the create rule flyout is open or not. I was thinking about having something like a refresh button appearing after the create rule flyout is closed but that's definitely not a good UX and I don't know when exactly I need to refresh because I don't control the request to get the alerts at this moment). So you can't somehow subscribe to the alerts and refresh the component if there are new alerts coming inside Alerts Summary? I get the idea to have a simple way to show the alerts but this can be an optional extra functionality where it's needed ( Also other places where we use the component can benefit from it ) Regarding the loading spinner:
Where can I create an issue/feature request so you can prioritize this enhancement? |
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! Thanks for the changes.
@jennypavlova AlertSummaryWidget is owned by @elastic/response-ops but originally developed by @elastic/actionable-observability. @emma-raffenne @katrin-freihofner Any suggestion about where the enhancement tickets should be created and which team needs to prioritize it? |
I would recommend to create them with label |
@emma-raffenne Thank you for your reply - I added the issues: #162167 and #162171 cc: @crespocarlos thank you for mentioning the issues! |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
Closes elastic#160371 ## Summary This PR adds alerts section to the overview tab inside the asset details flyout component. Notes: A lot of changes are extracting common components from the alerts tab to a common folder. The flyout version is not showing the chart so it's not exactly the same component but a big part of the logic is reused there. The tooltip content can be found in a [Figma comment ](https://www.figma.com/file/XBVpHX6pOBaTPoGHWhEQJH?node-id=843:435665&mode=design#492130894) <img width="1616" alt="alerts_section" src="https://github.com/elastic/kibana/assets/14139027/399dd1ea-e1cb-4e7f-9ed5-917ced7cc490"> ## Alerts summary widget changes: After introducing the `hideChart` prop [here](elastic#161263) in this PR I change the spinner type and size in case of no chart we want to have a smaller section with a smaller spinner: ![image](https://github.com/elastic/kibana/assets/14139027/43a3c611-0404-4c21-a503-22f1a79dc1de) ![image](https://github.com/elastic/kibana/assets/14139027/a870fa9b-5367-4303-9b7d-4da9ff2eae2b) ## Storybook I added some changes to make the alerts widget show in the storybook [[Workaround for storybook](elastic@d97a2b1)] <img width="1905" alt="image" src="https://github.com/elastic/kibana/assets/14139027/539c9443-f977-4301-8d2b-d24f1d01b44e"> ## Testing - Go to Hosts view and open the single host flyout - alerts section should be visible - Alerts title icon should open a tooltip with links to alerts and alerts documentation - Alerts links: - The Create rule link will open a flyout (on top, not closing the existing flyout) to create an inventory rule, when closed/saved rule the single host flyout should remain open - The Show All link should navigate to alerts and apply time range / host.name filter selected in the hosts view https://github.com/elastic/kibana/assets/14139027/b362042a-b9de-460c-86ae-282154b586ff --------- Co-authored-by: kibanamachine <[email protected]>
Closes #160371
Summary
This PR adds alerts section to the overview tab inside the asset details flyout component.
Notes: A lot of changes are extracting common components from the alerts tab to a common folder. The flyout version is not showing the chart so it's not exactly the same component but a big part of the logic is reused there. The tooltip content can be found in a Figma comment
Alerts summary widget changes:
After introducing the
hideChart
prop here in this PR I change the spinner type and size in case of no chart we want to have a smaller section with a smaller spinner:Storybook
I added some changes to make the alerts widget show in the storybook [Workaround for storybook]
Testing
alerts_section.mov