-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Infra] Infra Inventory refactors #229980
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] Infra Inventory refactors #229980
Conversation
…eshold rule params
| const { onViewChange } = useWaffleViewState(); | ||
|
|
||
| useEffect(() => { | ||
| if (currentView) { | ||
| onViewChange(currentView); | ||
| } | ||
| }, [currentView, onViewChange]); | ||
|
|
||
| useEffect(() => { | ||
| // load snapshot data after default view loaded, unless we're not loading a view | ||
| if (currentView != null) { | ||
| reload(); | ||
| } | ||
| }, [currentView, reload]); |
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.
Srlsy, why?
| @@ -1,101 +0,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.
This thing had become a second source of truth, causing all the shenanigans around race conditions between saved view state and URL state
| }); | ||
| }); | ||
|
|
||
| it('should work with filterQuery as Lucene expressions', () => { |
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.
kqlQuery takes care of this
| const filterQueryAsJson = useMemo( | ||
| () => convertKueryToElasticSearchQuery(urlState.expression, metricsView?.dataViewReference), | ||
| [metricsView?.dataViewReference, urlState.expression] | ||
| ); |
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 are 0 reasons to send a DSL query object to APIs
| rt.partial({ | ||
| accountId: rt.string, | ||
| region: rt.string, | ||
| filterQuery: rt.union([rt.string, rt.null]), |
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 API can handle KQL filters internally
| export const METRICS_EXPLORER_API_MAX_METRICS = 20; | ||
|
|
||
| export const TIMESTAMP_FIELD = '@timestamp'; | ||
| export const HOST_FIELD = 'host.name'; | ||
| export const CONTAINER_FIELD = 'container.id'; | ||
| export const POD_FIELD = 'kubernetes.pod.uid'; | ||
| export const HOST_NAME_FIELD = 'host.name'; | ||
| export const CONTAINER_ID_FIELD = 'container.id'; | ||
| export const KUBERNETES_POD_UID_FIELD = 'kubernetes.pod.uid'; | ||
|
|
||
| export const METRICS_EXPLORER_API_MAX_METRICS = 20; | ||
| export const EVENT_MODULE = 'event.module'; | ||
| export const METRICSET_MODULE = 'metricset.module'; | ||
| export const METRICSET_NAME = 'metricset.name'; | ||
| export const DATASTREAM_DATASET = 'data_stream.dataset'; | ||
|
|
||
| // integrations | ||
| export const SYSTEM_INTEGRATION = 'system'; | ||
| export const HOST_METRICS_RECEIVER_OTEL = 'hostmetricsreceiver.otel'; |
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 has to be temporary and we need a centralized place to define these constants, but this will do for this PR
| urlStateKey: 'waffleFilter', | ||
| }); | ||
|
|
||
| const [state, setState] = useState<InventoryFiltersState>(urlState); |
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.
second source of truth
| urlStateKey: 'waffleTime', | ||
| }); | ||
|
|
||
| const [state, setState] = useState<WaffleTimeState>(urlState); |
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.
second source of truth
| urlStateKey: 'waffleOptions', | ||
| }); | ||
|
|
||
| const [state, setState] = useState<WaffleOptionsState>(urlState); |
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.
second source of truth
| history.replace(newLocation); | ||
| } | ||
| }, | ||
| [decodeUrlState, defaultState, encodeUrlState, history, urlStateKey] |
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.
defaultState was causing setState function to be redefined during re-renders
|
Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services) |
| <EuiSelect | ||
| aria-label={i18n.translate('xpack.infra.nodeTypeExpression.select.ariaLabel', { | ||
| defaultMessage: 'Select a node type', | ||
| })} |
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.
A11y ❤️
| schema: config.featureFlags.hostOtelEnabled | ||
| ? DataSchemaFormat.SEMCONV | ||
| : DataSchemaFormat.ECS, | ||
| schema: config.featureFlags.hostOtelEnabled ? DataSchemaFormat.SEMCONV : 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.
Why we change this to undefined instead of 'ecs'?
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'll change this soon when we add the schema selector to the page
| const [state, setState] = useState<WaffleOptionsState>(urlState); | ||
|
|
||
| useEffect(() => setUrlState(state), [setUrlState, state]); | ||
| const previousViewId = useRef<string | undefined>(currentView?.id); |
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!
...ck/solutions/observability/plugins/metrics_data_access/common/inventory_models/host/index.ts
Show resolved
Hide resolved
MiriamAparicio
left a comment
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 making our infra live better, just added a couple of comments and a question
|
@elasticmachine merge upstream |
tonyghiani
left a comment
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.
Unblocking for shared ownership of use_url_state, change LGTM
| export const PrefilledInventoryAlertFlyout = ({ onClose }: { onClose(): void }) => { | ||
| const { inventoryPrefill } = useAlertPrefillContext(); | ||
| const { nodeType, metric, filterQuery } = inventoryPrefill; | ||
| const { nodeType, metric, kuery } = inventoryPrefill; |
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.
Instead of a query DSL object, inventoryPrefill will store a kql filter
| nodeType, | ||
| }: Props) => { | ||
| const [popoverOpen, setPopoverOpen] = useState(false); | ||
| const [popoverOpen, { toggle: togglePopover }] = useBoolean(false); |
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.
| export const useInventoryAlertPrefill = () => { | ||
| const [nodeType, setNodeType] = useState<InventoryItemType>('host'); | ||
| const [filterQuery, setFilterQuery] = useState<string | undefined>(); | ||
| const [kuery, setKuery] = useState<string | 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.
Change to store kql instead of query DSL
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.
|
Same error if I try creating the rule from |
I think this was introduced by another PR. I'll check it. Thanks for flagging it. |
fixed: beaacdc
|
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
|
part of elastic#226336 ## Summary This PR fixes and simplifies some things in the Infra Inventory UI **Payload simplified** The Snapshop API currently accepts query DSL in the request body. This object is complex, and there are no apparent reason for it to remain like this. | Before | After | |--------|-----------------| | `filterQuery: "{\"bool\":{\"should\":[{\"term\":{\"host.name\":{\"value\":\"advertService\"}}}],\"minimum_should_match\":1}}"` | `kuery: "host.name:\"metricbeat-host\""` | **Race condition fix** An old issue with a race condition between Saved Views and URL state has been fixed | Before | After | |--------|-----------------| ||| **Misc** - Centralize entity-specific code in the inventory models - Fix caching issues with the `useUrlState` --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
part of elastic#226336 ## Summary This PR fixes and simplifies some things in the Infra Inventory UI **Payload simplified** The Snapshop API currently accepts query DSL in the request body. This object is complex, and there are no apparent reason for it to remain like this. | Before | After | |--------|-----------------| | `filterQuery: "{\"bool\":{\"should\":[{\"term\":{\"host.name\":{\"value\":\"advertService\"}}}],\"minimum_should_match\":1}}"` | `kuery: "host.name:\"metricbeat-host\""` | **Race condition fix** An old issue with a race condition between Saved Views and URL state has been fixed | Before | After | |--------|-----------------| ||| **Misc** - Centralize entity-specific code in the inventory models - Fix caching issues with the `useUrlState` --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>



part of #226336
Summary
This PR fixes and simplifies some things in the Infra Inventory UI
Payload simplified
The Snapshop API currently accepts query DSL in the request body. This object is complex, and there are no apparent reason for it to remain like this.
filterQuery: "{\"bool\":{\"should\":[{\"term\":{\"host.name\":{\"value\":\"advertService\"}}}],\"minimum_should_match\":1}}"kuery: "host.name:\"metricbeat-host\""Race condition fix
An old issue with a race condition between Saved Views and URL state has been fixed
Misc
useUrlState