From 5d99d46e2008f427d8b57e1d9eff2a2d66f4e443 Mon Sep 17 00:00:00 2001 From: mohamedhamed-ahmed Date: Mon, 15 May 2023 12:42:55 +0100 Subject: [PATCH] Code Refactoring and fixing comments --- .../plugins/infra/public/locators/helpers.ts | 29 ++++++----- .../infra/public/locators/locators.test.ts | 16 +++--- .../infra/public/locators/logs_locator.ts | 12 +++-- .../public/locators/node_logs_locator.ts | 14 ++--- .../src/url_state_storage_service.ts | 2 +- .../public/pages/link_to/redirect_to_logs.tsx | 16 ++++-- .../pages/link_to/redirect_to_node_logs.tsx | 51 ++++++------------- .../tabs/logs/logs_link_to_stream.tsx | 18 +++---- x-pack/plugins/infra/public/plugin.ts | 4 +- 9 files changed, 77 insertions(+), 85 deletions(-) diff --git a/x-pack/plugins/infra/public/locators/helpers.ts b/x-pack/plugins/infra/public/locators/helpers.ts index e1d4407bcb8fe..5c52903e259eb 100644 --- a/x-pack/plugins/infra/public/locators/helpers.ts +++ b/x-pack/plugins/infra/public/locators/helpers.ts @@ -34,7 +34,7 @@ interface LocationToDiscoverParams { logView?: LogViewReference; } -export const parseSearchString = ({ +export const createSearchString = ({ time, timeRange, filter = '', @@ -88,6 +88,8 @@ export const getLocationToDiscover = async ({ state.matches('checkingStatusFailed') ); + service.stop(); + if ('resolvedLogView' in doneState.context) { discoverLocation = await constructDiscoverLocation( discover, @@ -98,8 +100,6 @@ export const getLocationToDiscover = async ({ discoverLocation = await constructDiscoverLocation(discover, discoverParams); } - service.stop(); - if (!discoverLocation) { throw new Error('Discover location not found'); } @@ -112,16 +112,19 @@ const constructDiscoverLocation = async ( discoverParams: DiscoverAppLocatorParams, resolvedLogView?: ResolvedLogView ) => { - const locationParams = !resolvedLogView - ? discoverParams - : { - ...discoverParams, - columns: parseColumns(resolvedLogView.columns), - dataViewId: resolvedLogView.dataViewReference.toSpec().id, - dataViewSpec: resolvedLogView.dataViewReference.toSpec(), - }; - - return await discover.locator?.getLocation(locationParams); + if (!resolvedLogView) { + return await discover.locator?.getLocation(discoverParams); + } + + const columns = parseColumns(resolvedLogView.columns); + const dataViewSpec = resolvedLogView.dataViewReference.toSpec(); + + return await discover.locator?.getLocation({ + ...discoverParams, + columns, + dataViewId: dataViewSpec.id, + dataViewSpec, + }); }; const parseColumns = (columns: ResolvedLogView['columns']) => { diff --git a/x-pack/plugins/infra/public/locators/locators.test.ts b/x-pack/plugins/infra/public/locators/locators.test.ts index 9e6f1107c9e39..c7c993b2484af 100644 --- a/x-pack/plugins/infra/public/locators/locators.test.ts +++ b/x-pack/plugins/infra/public/locators/locators.test.ts @@ -15,11 +15,12 @@ import { coreMock } from '@kbn/core/public/mocks'; import { findInventoryFields } from '../../common/inventory_models'; import moment from 'moment'; import { DEFAULT_LOG_VIEW } from '../observability_logs/log_view_state'; +import type { LogViewReference } from '../../common/log_views'; const setupLogsLocator = async (appTarget: string = LOGS_APP_TARGET) => { const deps: LogsLocatorDependencies = { core: coreMock.createSetup(), - appTarget, + config: { appTarget }, }; const logsLocator = new LogsLocatorDefinition(deps); const nodeLogsLocator = new NodeLogsLocatorDefinition(deps); @@ -167,7 +168,7 @@ describe('Infra Locators', () => { nodeId, nodeType, time, - logView: DEFAULT_LOG_VIEW, + logView: { ...DEFAULT_LOG_VIEW, logViewId: 'test' }, }; const { nodeLogsLocator } = await setupLogsLocator(); const { path } = await nodeLogsLocator.getLocation(params); @@ -217,15 +218,18 @@ describe('Infra Locators', () => { */ export const constructUrlSearchString = (params: Partial) => { - const { time = 1550671089404 } = params; + const { time = 1550671089404, logView } = params; - return `/stream?logView=${constructLogView()}&logPosition=${constructLogPosition( + return `/stream?logView=${constructLogView(logView)}&logPosition=${constructLogPosition( time )}&logFilter=${constructLogFilter(params)}`; }; -const constructLogView = () => { - return `(logViewId:default,type:log-view-reference)`; +const constructLogView = (logView?: LogViewReference) => { + const logViewId = + logView && 'logViewId' in logView ? logView.logViewId : DEFAULT_LOG_VIEW.logViewId; + + return `(logViewId:${logViewId},type:log-view-reference)`; }; const constructLogPosition = (time: number = 1550671089404) => { diff --git a/x-pack/plugins/infra/public/locators/logs_locator.ts b/x-pack/plugins/infra/public/locators/logs_locator.ts index f3d26095e6100..2cd088463e7ee 100644 --- a/x-pack/plugins/infra/public/locators/logs_locator.ts +++ b/x-pack/plugins/infra/public/locators/logs_locator.ts @@ -27,9 +27,13 @@ export interface LogsLocatorParams extends SerializableRecord { export type LogsLocator = LocatorPublic; +interface LocatorConfig { + appTarget: string; +} + export interface LogsLocatorDependencies { core: InfraClientCoreSetup; - appTarget: string; + config: LocatorConfig; } export class LogsLocatorDefinition implements LocatorDefinition { @@ -38,13 +42,13 @@ export class LogsLocatorDefinition implements LocatorDefinition { - const { parseSearchString, getLocationToDiscover } = await import('./helpers'); + const { createSearchString, getLocationToDiscover } = await import('./helpers'); - if (this.deps.appTarget === DISCOVER_APP_TARGET) { + if (this.deps.config.appTarget === DISCOVER_APP_TARGET) { return await getLocationToDiscover({ core: this.deps.core, ...params }); } - const searchString = parseSearchString(params); + const searchString = createSearchString(params); return { app: 'logs', diff --git a/x-pack/plugins/infra/public/locators/node_logs_locator.ts b/x-pack/plugins/infra/public/locators/node_logs_locator.ts index 7661c22166321..9d932c6425375 100644 --- a/x-pack/plugins/infra/public/locators/node_logs_locator.ts +++ b/x-pack/plugins/infra/public/locators/node_logs_locator.ts @@ -7,8 +7,7 @@ import { LocatorDefinition, LocatorPublic } from '@kbn/share-plugin/public'; import type { InventoryItemType } from '../../common/inventory_models/types'; -import type { InfraClientCoreSetup } from '../types'; -import type { LogsLocatorParams } from './logs_locator'; +import type { LogsLocatorDependencies, LogsLocatorParams } from './logs_locator'; import { DISCOVER_APP_TARGET } from '../../common/constants'; const NODE_LOGS_LOCATOR_ID = 'NODE_LOGS_LOCATOR'; @@ -20,10 +19,7 @@ export interface NodeLogsLocatorParams extends LogsLocatorParams { export type NodeLogsLocator = LocatorPublic; -export interface NodeLogsLocatorDependencies { - core: InfraClientCoreSetup; - appTarget: string; -} +export type NodeLogsLocatorDependencies = LogsLocatorDependencies; export class NodeLogsLocatorDefinition implements LocatorDefinition { public readonly id = NODE_LOGS_LOCATOR_ID; @@ -31,18 +27,18 @@ export class NodeLogsLocatorDefinition implements LocatorDefinition { - const { parseSearchString, getLocationToDiscover } = await import('./helpers'); + const { createSearchString, getLocationToDiscover } = await import('./helpers'); const { findInventoryFields } = await import('../../common/inventory_models'); const { nodeType, nodeId, filter, timeRange, logView } = params; const nodeFilter = `${findInventoryFields(nodeType).id}: ${nodeId}`; const query = filter ? `(${nodeFilter}) and (${filter})` : nodeFilter; - if (this.deps.appTarget === DISCOVER_APP_TARGET) { + if (this.deps.config.appTarget === DISCOVER_APP_TARGET) { return await getLocationToDiscover({ core: this.deps.core, timeRange, filter, logView }); } - const searchString = parseSearchString({ ...params, filter: query }); + const searchString = createSearchString({ ...params, filter: query }); return { app: 'logs', diff --git a/x-pack/plugins/infra/public/observability_logs/log_stream_position_state/src/url_state_storage_service.ts b/x-pack/plugins/infra/public/observability_logs/log_stream_position_state/src/url_state_storage_service.ts index 882a52a86a8f1..5b8e3e56c989c 100644 --- a/x-pack/plugins/infra/public/observability_logs/log_stream_position_state/src/url_state_storage_service.ts +++ b/x-pack/plugins/infra/public/observability_logs/log_stream_position_state/src/url_state_storage_service.ts @@ -102,7 +102,7 @@ const decodePositionQueryValueFromUrl = (queryValueFromUrl: unknown) => { // Used by linkTo components export const replaceLogPositionInQueryString = (time?: number) => - Number.isNaN(time) + Number.isNaN(time) || time == null ? (value: string) => value : replaceStateKeyInQueryString(defaultPositionStateKey, { position: { diff --git a/x-pack/plugins/infra/public/pages/link_to/redirect_to_logs.tsx b/x-pack/plugins/infra/public/pages/link_to/redirect_to_logs.tsx index 8ef37c90af710..80b9ff09180e0 100644 --- a/x-pack/plugins/infra/public/pages/link_to/redirect_to_logs.tsx +++ b/x-pack/plugins/infra/public/pages/link_to/redirect_to_logs.tsx @@ -5,6 +5,7 @@ * 2.0. */ +import { useEffect } from 'react'; import { useLocation, useParams } from 'react-router-dom'; import { getFilterFromLocation, getTimeFromLocation } from './query_params'; import { useKibanaContextForPlugin } from '../../hooks/use_kibana'; @@ -21,11 +22,16 @@ export const RedirectToLogs = () => { const filter = getFilterFromLocation(location); const time = getTimeFromLocation(location); - locators.logsLocator.navigate({ - time, - filter, - logView: { ...DEFAULT_LOG_VIEW, logViewId: logViewId || DEFAULT_LOG_VIEW.logViewId }, - }); + useEffect(() => { + locators.logsLocator.navigate( + { + time, + filter, + logView: { ...DEFAULT_LOG_VIEW, logViewId: logViewId || DEFAULT_LOG_VIEW.logViewId }, + }, + { replace: true } + ); + }, [filter, locators.logsLocator, logViewId, time]); return null; }; diff --git a/x-pack/plugins/infra/public/pages/link_to/redirect_to_node_logs.tsx b/x-pack/plugins/infra/public/pages/link_to/redirect_to_node_logs.tsx index ed25dd16fa208..483e8ccf01e78 100644 --- a/x-pack/plugins/infra/public/pages/link_to/redirect_to_node_logs.tsx +++ b/x-pack/plugins/infra/public/pages/link_to/redirect_to_node_logs.tsx @@ -5,15 +5,11 @@ * 2.0. */ -import { i18n } from '@kbn/i18n'; import { LinkDescriptor } from '@kbn/observability-plugin/public'; -import React from 'react'; +import { useEffect } from 'react'; import { RouteComponentProps } from 'react-router-dom'; -import useMount from 'react-use/lib/useMount'; import { InventoryItemType } from '../../../common/inventory_models/types'; -import { LoadingPage } from '../../components/loading_page'; import { useKibanaContextForPlugin } from '../../hooks/use_kibana'; -import { useLogView } from '../../hooks/use_log_view'; import { DEFAULT_LOG_VIEW_ID } from '../../observability_logs/log_view_state'; import { getFilterFromLocation, getTimeFromLocation } from './query_params'; @@ -29,40 +25,25 @@ export const RedirectToNodeLogs = ({ }, location, }: RedirectToNodeLogsType) => { - const { services } = useKibanaContextForPlugin(); - const { isLoading, load } = useLogView({ - initialLogViewReference: { type: 'log-view-reference', logViewId }, - logViews: services.logViews.client, - }); - - useMount(() => { - load(); - }); - - if (isLoading) { - return ( - - ); - } + const { + services: { locators }, + } = useKibanaContextForPlugin(); const filter = getFilterFromLocation(location); const time = getTimeFromLocation(location); - services.locators.nodeLogsLocator.navigate({ - nodeId, - nodeType, - time, - filter, - logView: { type: 'log-view-reference', logViewId }, - }); + useEffect(() => { + locators.nodeLogsLocator.navigate( + { + nodeId, + nodeType, + time, + filter, + logView: { type: 'log-view-reference', logViewId }, + }, + { replace: true } + ); + }, [filter, locators.nodeLogsLocator, logViewId, nodeId, nodeType, time]); return null; }; diff --git a/x-pack/plugins/infra/public/pages/metrics/hosts/components/tabs/logs/logs_link_to_stream.tsx b/x-pack/plugins/infra/public/pages/metrics/hosts/components/tabs/logs/logs_link_to_stream.tsx index c9a1669a1c2d7..f5c2101317f01 100644 --- a/x-pack/plugins/infra/public/pages/metrics/hosts/components/tabs/logs/logs_link_to_stream.tsx +++ b/x-pack/plugins/infra/public/pages/metrics/hosts/components/tabs/logs/logs_link_to_stream.tsx @@ -23,16 +23,14 @@ export const LogsLinkToStream = ({ startTime, endTime, query }: LogsLinkToStream return ( - locators.logsLocator?.navigate({ - time: endTime, - timeRange: { - startTime, - endTime, - }, - filter: query, - }) - } + href={locators.logsLocator?.getRedirectUrl({ + time: endTime, + timeRange: { + startTime, + endTime, + }, + filter: query, + })} data-test-subj="hostsView-logs-link-to-stream-button" iconType="popout" flush="both" diff --git a/x-pack/plugins/infra/public/plugin.ts b/x-pack/plugins/infra/public/plugin.ts index c09e8ea720b07..639f83c798474 100644 --- a/x-pack/plugins/infra/public/plugin.ts +++ b/x-pack/plugins/infra/public/plugin.ts @@ -298,10 +298,10 @@ export class Plugin implements InfraClientPluginClass { // Register Locators const logsLocator = pluginsSetup.share.url.locators.create( - new LogsLocatorDefinition({ appTarget: this.appTarget, core }) + new LogsLocatorDefinition({ config: { appTarget: this.appTarget }, core }) ); const nodeLogsLocator = pluginsSetup.share.url.locators.create( - new NodeLogsLocatorDefinition({ appTarget: this.appTarget, core }) + new NodeLogsLocatorDefinition({ config: { appTarget: this.appTarget }, core }) ); this.locators = {