-
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] Provide troubleshooting information on the host details page #191104
[Infra] Provide troubleshooting information on the host details page #191104
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
…tion-on-the-host-details-page
/ci |
Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services) |
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.
monitoredHost: { | ||
filter: getFilterByIntegration(SYSTEM_INTEGRATION), | ||
aggs: { | ||
name: { | ||
terms: { | ||
field: HOST_NAME_FIELD, | ||
size: 1, | ||
order: { | ||
_key: 'asc', | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, |
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 endpoint is used for other asset types. We might want to run this agg only if nodeType === 'host'
.
Also, perhaps running a query like getHasDataFromSystemIntegration
parallel to this query might be faster than this term agg because it early terminates as soon as it finds a document that satisfies the criteria.
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 made the aggregation conditional so it will be set only if we have nodeType === 'host'
In this query we already filter by node name so it shouldn't be slow I think 🤔 Wdyt? I can also write an extra query again filtered by node name and use it as a boolean flag (if any data is returned or not) but I am not sure if there is a big benefit in this case as you need to query and filter again to get the results as the aggregation is added to an existing query.
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.
Hey! Thanks for adding the condition.
The fact that the query filters by host.name
helps, but elasticsearch still needs to collect documents to build the aggregation. getHasDataFromSystemIntegration
requires less computation - this function could even be reused.
Another potential downside is if we need to do something similar for other asset types. I'm not sure if adding aggregations to this query will scale nicely.
But it's up to you. We can always revisit this in the future.
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.
Got it, thanks. Yeah, when I think about it it is a good idea - it brings dependency to the infraMetricsClient
but I guess that is fine as we can use it in case we have similar queries for different assets.
|
||
return { | ||
id: nodeId, | ||
name: get(response, ['aggregations', 'nodeName', 'buckets', 0, 'key'], nodeId), | ||
hasSystemIntegration, |
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 attribute kind of makes it harder to extend the same idea to other asset types because this condition is specific to hosts.
For now, I think it's OK because we don't know if the problem we're solving here will happen to other asset types, but I would return this attribute only when nodeType === host
. wdyt?
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 changed that to be returned only for hosts, thanks for catching that!
const hostWithSystemIntegration = | ||
response.aggregations && (response.aggregations?.monitoredHost?.name?.buckets ?? []).length > 0 | ||
? response.aggregations?.monitoredHost?.name.buckets[0]?.key | ||
: null; | ||
|
||
const hasSystemIntegration = hostWithSystemIntegration === nodeId; |
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 could also be done only when nodeType === 'host'
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 👍
…tion-on-the-host-details-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.
Thanks for adding the missing (?) to the header! Left a couple more comments.
@@ -53,7 +55,7 @@ export const Flyout = ({ | |||
data-component-name={ASSET_DETAILS_FLYOUT_COMPONENT_NAME} | |||
data-asset-type={asset.type} | |||
> | |||
{loading ? ( | |||
{loading || metadataLoading ? ( |
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 will block the loading of components in the flyout that doesn't depend on metadata.
blocking_loading_spinner.mov
This is not necessarily part of this PR, but how about removing this InfraLoadingPanel
and passing loading
and metadataLoading
via props to FlyoutHeader
? This way we can have a loading spinner that doesn't block the loading rest of the flyout.
For the full page view, we do 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.
Thanks for spotting that! I changed it to use the loading spinner inside FlyoutHeader
like on the page view:
flyout_loading.mov
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.
It looks great now! thanks.
set(metricQuery, 'body.aggs', { | ||
monitoredHost: { | ||
filter: getFilterByIntegration(SYSTEM_INTEGRATION), | ||
aggs: { | ||
name: { | ||
terms: { | ||
field: HOST_NAME_FIELD, | ||
size: 1, | ||
order: { | ||
_key: 'asc', | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, |
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.
Q and nit: Do we need lodash for this?
We could do this directly on the metricQuery
set(metricQuery, 'body.aggs', { | |
monitoredHost: { | |
filter: getFilterByIntegration(SYSTEM_INTEGRATION), | |
aggs: { | |
name: { | |
terms: { | |
field: HOST_NAME_FIELD, | |
size: 1, | |
order: { | |
_key: 'asc', | |
}, | |
}, | |
}, | |
}, | |
}, | |
...(nodeType === 'host' | |
? { | |
monitoredHost: { | |
filter: getFilterByIntegration(SYSTEM_INTEGRATION), | |
aggs: { | |
name: { | |
terms: { | |
field: HOST_NAME_FIELD, | |
size: 1, | |
order: { | |
_key: 'asc', | |
}, | |
}, | |
}, | |
}, | |
}, | |
} | |
: undefined), |
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 replaced it with a getHasDataFromSystemIntegration
as discussed
…tion-on-the-host-details-page
…-host-details-page' of https://github.com/jennypavlova/kibana into 190523-infra-provide-troubleshooting-information-on-the-host-details-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.
LGTM 👏 ! Thanks for the changes!
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
…1908) Closes #191579 ### Summary When working on finding the root cause of the bug, another bug related was discovered on the host detected by APM tooltip, it was introduced by this [PR](#191104) when we check for `metadataLoading` to finish, as we don’t do it for the asset details page the loading spinner wasn’t shown there when refreshing the page, but the tooltip shows because is still waiting for the metadata request to finish in order to retrieve the integration. To fix this, we don't apply loading spinner to the asset name, but we do it for the tooltip BEFORE https://github.com/user-attachments/assets/3a946e84-1256-440c-bb65-56df30d8d3a5 AFTER https://github.com/user-attachments/assets/689a6583-c66d-4e96-8605-9cce6c9aff24
Closes #190523
Summary
This PR provides troubleshooting information in the services section on the host details page.
Testing
Go to the hosts details page / host flyout
using a host without services monitored with system integration
using a host without services not monitored with system integration ( only detected by APM )
icon next to the title (same as the table)
For hosts with services, nothing changes (the service is shown):