-
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
[Infrastructure UI] Replace Lens table with EUI table and own api #142871
Conversation
@@ -15,8 +15,7 @@ | |||
"triggersActionsUi", | |||
"observability", | |||
"ruleRegistry", | |||
"unifiedSearch", | |||
"lens" |
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 might want to leave this for now as we are thinking to use Lens Embeddables for other things. or we can remove it and just add it back later.
@@ -75,7 +74,6 @@ export interface InfraClientStartDeps { | |||
embeddable?: EmbeddableStart; | |||
osquery?: unknown; // OsqueryPluginStart; | |||
share: SharePluginStart; | |||
lens: LensPublicStart; |
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 might want to leave this for now as we are thinking to use Lens Embeddables for other things. or we can remove it and just add it back later.
bbebaec
to
5a212ca
Compare
ef705fe
to
392e035
Compare
x-pack/plugins/infra/common/inventory_models/host/metrics/snapshot/memory_total.ts
Show resolved
Hide resolved
"xpack.infra.hostsTable.nameColumnHeader": "Nom", | ||
"xpack.infra.hostsTable.operatingSystemColumnHeader": "Système d'exploitation", | ||
"xpack.infra.hostsTable.numberOfCpusColumnHeader": "Nombre de processeurs", | ||
"xpack.infra.hostsTable.diskLatencyColumnHeader": "", |
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.
Note: Those translations are missing (couldn't find them in the project so I guess they should be added)
'', | ||
'', | ||
true, | ||
{ |
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.
Regarding the dynamic time range, we discussed with @crespocarlos that it will be better to be added as part of the filter issue and maybe after we have the url state implemented so we will have a clear scope of this issue (replacement of the lens table with eui table) and it will unblock the other issues where we need the table to be changed. 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.
It makes more sense to address this on the unified search ticket. Everything related to filter is connected to that.
x-pack/plugins/infra/public/pages/metrics/hosts/components/hosts_table.tsx
Show resolved
Hide resolved
Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI) |
} as TypedLensByValueInput['attributes']); | ||
import React from 'react'; | ||
import { EuiBasicTable } from '@elastic/eui'; | ||
import { SnapshotNode } from '../../../../../common/http_api'; |
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.
How about
import { SnapshotNode } from '../../../../../common/http_api'; | |
import type { SnapshotNode } from '../../../../../common/http_api'; |
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.
Cool stuff! Just left a few minor comments.
I wonder whether we should prompt users to enable the add_host_metadata
in case the OS info is missing. But his would be another ticket anyways
import { i18n } from '@kbn/i18n'; | ||
import { EuiText } from '@elastic/eui'; | ||
import { scaleUpPercentage } from '../../../../components/infrastructure_node_metrics_tables/shared/hooks'; | ||
import { SnapshotNodeMetric } from '../../../../../common/http_api/snapshot_api'; |
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 (we're just importing a type here):
import { SnapshotNodeMetric } from '../../../../../common/http_api/snapshot_api'; | |
import type { SnapshotNodeMetric } from '../../../../../common/http_api/snapshot_api'; |
export const useHostTable = (nodes: SnapshotNode[]) => { | ||
const items: MappedMetrics[] = useMemo(() => { | ||
return nodes.map(({ metrics, path }) => ({ | ||
...path[0], |
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.
What happens if path
has more than 1 item? I don't think path
could ever be null
, right?
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.
Good point, I don't think that's the case here - Based on what I see in applyMetadataToLastPath
there should be only one document with metadata, there we use the last document to add the os - So we can do it similar to the idea there using last
where we are setting the os we need here and even if there are other elements before we will get the correct one (the last one we used to add the os to).
import { useSourceContext } from '../../../containers/metrics_source'; | ||
import { useSnapshot } from '../inventory_view/hooks/use_snaphot'; | ||
import { SnapshotMetricType } from '../../../../common/inventory_models/types'; |
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 (just type import):
import { SnapshotMetricType } from '../../../../common/inventory_models/types'; | |
import type { SnapshotMetricType } from '../../../../common/inventory_models/types'; |
@@ -56,6 +56,10 @@ export const applyMetadataToLastPath = ( | |||
lastPath.ip = ipAddresses; | |||
} | |||
} | |||
if (inventoryFields.os) { | |||
const inventoryFieldsOs = get(firstMetaDoc, inventoryFields.os) as string; |
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: we could maybe shorten this?
const inventoryFieldsOs = get(firstMetaDoc, inventoryFields.os) as string; | |
lastPath.os = get(firstMetaDoc, inventoryFields.os) as string; |
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.
Good idea, thanks 🙂 Done ✅
@@ -0,0 +1,126 @@ | |||
/* |
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.
Nice! :)
Maybe it would be good to validate when path
has more than 1 item (I'm not sure when this case would actually happen in real life though)
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.
As I checked the in applyMetadataToLastPath
we will use the last element so I will update one of the items to have 2 elements in the path
and check that we use only the last.
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
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!
* main: (57 commits) [Files] Filepicker (elastic#143111) [Infrastructure UI] Replace Lens table with EUI table and own api (elastic#142871) [api-docs] Daily api_docs build (elastic#143829) [api-docs] Daily api_docs build (elastic#143825) [api-docs] Daily api_docs build (elastic#143823) [Security Solution] Restructuring folders of Detection Engine + refactoring Rule Management (elastic#142950) [Dev tools] Fix performance issue with autocomplete suggestions (elastic#143428) [Security Solution] Disable ML rule's edit button link under basic license (elastic#143260) [Lens] Use the language-documentation package for formula (elastic#143649) [api-docs] Daily api_docs build (elastic#143811) [Security Solution] Fix missing title on inspect pop-up (elastic#143601) fix incorrect filters being passed to events table causing duplicate entries in our inpsect tool request tab (elastic#143239) [Security Solution][Endpoint] `get-file` response action kibana download file API (elastic#143708) Rely on refresh context to update stats independently of overview cards. (elastic#143308) [RAM] Rule event log - Fix incorrect results when filtering by message and outcome simultaneously (elastic#143119) [ML] Display link to create data view from error cases in data frame analytics results pages (elastic#143596) Update links in README :) (elastic#143675) Add more tests for ml_inference_logic (elastic#143764) skip failing test suite (elastic#143717) [DOCS] Add assignees to case APIs (elastic#143610) ...
Closes #142797
This PR replaces the lens component with eui table using snapshot API. The time range implementation will be added as part of the filtering implementation (It's hardcoded for now).
Services on Host
andDisk Latency
will be implemented in separate issues.Testing
Metricbeats - if you want to test the table using metricbeat add :
to your metricbeat config to get the
Operating System
valueSlingshot - The OS value won't appear in the table because slingshot is not adding
os.name
value to the generated hostsThe time range inside
hosts_content.tsx
should be adjusted based on the test data ( I went to the Inventory page and coppied the time range values from the request there to make sure I have the same data )I tested with both (Metricbeats and Slingshot) locally and was able to see the data in the table: