Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Actionable Observability] Fix alerts' blank page in case of invalid query string #145067

Merged
merged 5 commits into from
Nov 14, 2022
Merged

[Actionable Observability] Fix alerts' blank page in case of invalid query string #145067

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d032a85
Validate alert query before saving in state and allow setting url sto…
maryam-saeidi Nov 13, 2022
726a7ec
Merge branch 'main' into 143641-fix-alert-search-bar
maryam-saeidi Nov 13, 2022
4f84167
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Nov 13, 2022
59007a7
Fix types
maryam-saeidi Nov 14, 2022
f7929aa
Merge branch '143641-fix-alert-search-bar' of https://github.com/mary…
maryam-saeidi Nov 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 22 additions & 10 deletions x-pack/plugins/observability/public/components/shared/alert_search_bar/alert_search_bar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,13 @@
import { EuiFlexGroup, EuiFlexItem } from '@elastic/eui';

import React, { useCallback, useEffect } from 'react';
import { i18n } from '@kbn/i18n';
import { Query } from '@kbn/es-query';
import { useKibana } from '@kbn/kibana-react-plugin/public';
import { observabilityAlertFeatureIds } from '../../../config';
import { ObservabilityAppServices } from '../../../application/types';
import { AlertsStatusFilter } from './components';
import { ALERT_STATUS_QUERY, DEFAULT_QUERIES } from './constants';
import { ALERT_STATUS_QUERY, DEFAULT_QUERIES, DEFAULT_QUERY_STRING } from './constants';
import { AlertSearchBarProps } from './types';
import { buildEsQuery } from '../../../utils/build_es_query';
import { AlertStatus } from '../../../../common/typings';
Expand All @@ -22,7 +23,7 @@ const getAlertStatusQuery = (status: string): Query[] => {
return status ? [{ query: ALERT_STATUS_QUERY[status], language: 'kuery' }] : [];
};

export function AlertSearchBar({
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Renamed it to make it easier to find the usage of this component.

export function ObservabilityAlertSearchBar({
appName,
rangeFrom,
setRangeFrom,
Expand All @@ -41,6 +42,7 @@ export function AlertSearchBar({
timefilter: { timefilter: timeFilterService },
},
},
notifications: { toasts },
triggersActionsUi: { getAlertsSearchBar: AlertsSearchBar },
} = useKibana<ObservabilityAppServices>().services;

Expand All @@ -66,20 +68,29 @@ export function AlertSearchBar({

const onSearchBarParamsChange = useCallback(
({ dateRange, query }) => {
timeFilterService.setTime(dateRange);
setRangeFrom(dateRange.from);
setRangeTo(dateRange.to);
setKuery(query);
setEsQuery(
buildEsQuery(
try {
// First try to create es query to make sure query is valid, then save it in state
const esQuery = buildEsQuery(
{
to: rangeTo,
from: rangeFrom,
},
query,
[...getAlertStatusQuery(status), ...queries]
)
);
);
setKuery(query);
timeFilterService.setTime(dateRange);
setRangeFrom(dateRange.from);
setRangeTo(dateRange.to);
setEsQuery(esQuery);
} catch (error) {
toasts.addError(error, {
title: i18n.translate('xpack.observability.alerts.searchBar.invalidQueryTitle', {
defaultMessage: 'Invalid query string',
}),
});
setKuery(DEFAULT_QUERY_STRING);
}
},
[
timeFilterService,
Expand All @@ -91,6 +102,7 @@ export function AlertSearchBar({
rangeFrom,
status,
queries,
toasts,
]
);

Expand Down
13 changes: 7 additions & 6 deletions ...bservability/public/components/shared/alert_search_bar/alert_search_bar_with_url_sync.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,20 @@ import {
Provider,
useAlertSearchBarStateContainer,
} from './containers';
import { AlertSearchBar } from './alert_search_bar';
import { ObservabilityAlertSearchBar } from './alert_search_bar';
import { AlertSearchBarWithUrlSyncProps } from './types';

function InternalAlertSearchbarWithUrlSync(props: AlertSearchBarWithUrlSyncProps) {
const stateProps = useAlertSearchBarStateContainer();
function AlertSearchbarWithUrlSync(props: AlertSearchBarWithUrlSyncProps) {
const { urlStorageKey, ...searchBarProps } = props;
const stateProps = useAlertSearchBarStateContainer(urlStorageKey);

return <AlertSearchBar {...props} {...stateProps} />;
return <ObservabilityAlertSearchBar {...stateProps} {...searchBarProps} />;
}

export function AlertSearchbarWithUrlSync(props: AlertSearchBarWithUrlSyncProps) {
export function ObservabilityAlertSearchbarWithUrlSync(props: AlertSearchBarWithUrlSyncProps) {
return (
<Provider value={alertSearchBarStateContainer}>
<InternalAlertSearchbarWithUrlSync {...props} />
<AlertSearchbarWithUrlSync {...props} />
</Provider>
);
}
1 change: 1 addition & 0 deletions x-pack/plugins/observability/public/components/shared/alert_search_bar/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { ALERT_STATUS_ACTIVE, ALERT_STATUS_RECOVERED, ALERT_STATUS } from '@kbn/
import { AlertStatusFilter } from '../../../../common/typings';

export const DEFAULT_QUERIES: Query[] = [];
export const DEFAULT_QUERY_STRING = '';

export const ALL_ALERTS: AlertStatusFilter = {
status: '',
Expand Down
32 changes: 21 additions & 11 deletions ...ic/components/shared/alert_search_bar/containers/use_alert_search_bar_state_container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ import {
AlertSearchBarContainerState,
} from './state_container';

export function useAlertSearchBarStateContainer() {
export function useAlertSearchBarStateContainer(urlStorageKey: string) {
const stateContainer = useContainer();

useUrlStateSyncEffect(stateContainer);
useUrlStateSyncEffect(stateContainer, urlStorageKey);

const { setRangeFrom, setRangeTo, setKuery, setStatus } = stateContainer.transitions;
const { rangeFrom, rangeTo, kuery, status } = useContainerSelector(
Expand All @@ -47,7 +47,10 @@ export function useAlertSearchBarStateContainer() {
};
}

function useUrlStateSyncEffect(stateContainer: AlertSearchBarStateContainer) {
function useUrlStateSyncEffect(
stateContainer: AlertSearchBarStateContainer,
urlStorageKey: string
) {
const history = useHistory();
const timefilterService = useTimefilterService();

Expand All @@ -57,27 +60,33 @@ function useUrlStateSyncEffect(stateContainer: AlertSearchBarStateContainer) {
useHash: false,
useHashQuery: false,
});
const { start, stop } = setupUrlStateSync(stateContainer, urlStateStorage);
const { start, stop } = setupUrlStateSync(stateContainer, urlStateStorage, urlStorageKey);

start();

syncUrlStateWithInitialContainerState(timefilterService, stateContainer, urlStateStorage);
syncUrlStateWithInitialContainerState(
timefilterService,
stateContainer,
urlStateStorage,
urlStorageKey
);

return stop;
}, [stateContainer, history, timefilterService]);
}, [stateContainer, history, timefilterService, urlStorageKey]);
}

function setupUrlStateSync(
stateContainer: AlertSearchBarStateContainer,
stateStorage: IKbnUrlStateStorage
stateStorage: IKbnUrlStateStorage,
urlStorageKey: string
) {
// This handles filling the state when an incomplete URL set is provided
const setWithDefaults = (changedState: Partial<AlertSearchBarContainerState> | null) => {
stateContainer.set({ ...defaultState, ...changedState });
};

return syncState({
storageKey: '_a',
storageKey: urlStorageKey,
stateContainer: {
...stateContainer,
set: setWithDefaults,
Expand All @@ -89,9 +98,10 @@ function setupUrlStateSync(
function syncUrlStateWithInitialContainerState(
timefilterService: TimefilterContract,
stateContainer: AlertSearchBarStateContainer,
urlStateStorage: IKbnUrlStateStorage
urlStateStorage: IKbnUrlStateStorage,
urlStorageKey: string
) {
const urlState = urlStateStorage.get<Partial<AlertSearchBarContainerState>>('_a');
const urlState = urlStateStorage.get<Partial<AlertSearchBarContainerState>>(urlStorageKey);

if (urlState) {
const newState = {
Expand All @@ -115,5 +125,5 @@ function syncUrlStateWithInitialContainerState(
stateContainer.set(defaultState);
}

urlStateStorage.set('_a', stateContainer.get());
urlStateStorage.set(urlStorageKey, stateContainer.get());
}
4 changes: 2 additions & 2 deletions x-pack/plugins/observability/public/components/shared/alert_search_bar/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@
* 2.0.
*/

export { AlertSearchBar as ObservabilityAlertSearchBar } from './alert_search_bar';
export { AlertSearchbarWithUrlSync as ObservabilityAlertSearchbarWithUrlSync } from './alert_search_bar_with_url_sync';
export { ObservabilityAlertSearchBar } from './alert_search_bar';
export { ObservabilityAlertSearchbarWithUrlSync } from './alert_search_bar_with_url_sync';
8 changes: 6 additions & 2 deletions x-pack/plugins/observability/public/components/shared/alert_search_bar/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,17 @@ interface AlertSearchBarStateTransitions {
setStatus: (status: AlertStatus) => AlertSearchBarContainerState;
}

export interface AlertSearchBarWithUrlSyncProps {
export interface CommonAlertSearchBarProps {
appName: string;
setEsQuery: (query: { bool: BoolQuery }) => void;
queries?: Query[];
}

export interface AlertSearchBarWithUrlSyncProps extends CommonAlertSearchBarProps {
urlStorageKey: string;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Allowing consumers to set the storage key.

}

export interface AlertSearchBarProps
extends AlertSearchBarContainerState,
AlertSearchBarStateTransitions,
AlertSearchBarWithUrlSyncProps {}
CommonAlertSearchBarProps {}
8 changes: 7 additions & 1 deletion x-pack/plugins/observability/public/pages/alerts/containers/alerts_page/alerts_page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,12 @@ import { LoadingObservability } from '../../../overview';
import './styles.scss';
import { renderRuleStats } from '../../components/rule_stats';
import { ObservabilityAppServices } from '../../../../application/types';
import { ALERTS_PER_PAGE, ALERTS_SEARCH_BAR_ID, ALERTS_TABLE_ID } from './constants';
import {
ALERTS_PER_PAGE,
ALERTS_SEARCH_BAR_ID,
ALERTS_TABLE_ID,
URL_STORAGE_KEY,
} from './constants';
import { RuleStatsState } from './types';

export function AlertsPage() {
Expand Down Expand Up @@ -132,6 +137,7 @@ export function AlertsPage() {
<ObservabilityAlertSearchbarWithUrlSync
appName={ALERTS_SEARCH_BAR_ID}
setEsQuery={setEsQuery}
urlStorageKey={URL_STORAGE_KEY}
/>
</EuiFlexItem>

Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/observability/public/pages/alerts/containers/alerts_page/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ export const ALERTS_PAGE_ID = 'alerts-o11y';
export const ALERTS_SEARCH_BAR_ID = 'alerts-search-bar-o11y';
export const ALERTS_PER_PAGE = 50;
export const ALERTS_TABLE_ID = 'xpack.observability.alerts.alert.table';
export const URL_STORAGE_KEY = '_a';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to keep URL_STORAGE_KEY as _a here and searchBarParams in rule_details/constants.ts? The namings do not match.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to use a more meaningful name for this storage rather than _a, so I changed it on the rule details page.
But on the alerts page, I didn't change it since I wasn't sure if changing the URL structure will break anything. We can discuss this use-case within our team and I can update it in the next PR if that makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! 👍

1 change: 1 addition & 0 deletions x-pack/plugins/observability/public/pages/rule_details/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

export const EXECUTION_TAB = 'execution';
export const ALERTS_TAB = 'alerts';
export const URL_STORAGE_KEY = 'searchBarParams';
export const EVENT_ERROR_LOG_TAB = 'rule_error_log_list';
export const RULE_DETAILS_PAGE_ID = 'rule-details-alerts-o11y';
export const RULE_DETAILS_ALERTS_SEARCH_BAR_ID = 'rule-details-alerts-search-bar-o11y';
2 changes: 2 additions & 0 deletions x-pack/plugins/observability/public/pages/rule_details/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import {
ALERTS_TAB,
RULE_DETAILS_PAGE_ID,
RULE_DETAILS_ALERTS_SEARCH_BAR_ID,
URL_STORAGE_KEY,
} from './constants';
import { RuleDetailsPathParams, TabId } from './types';
import { useBreadcrumbs } from '../../hooks/use_breadcrumbs';
Expand Down Expand Up @@ -216,6 +217,7 @@ export function RuleDetailsPage() {
<ObservabilityAlertSearchbarWithUrlSync
appName={RULE_DETAILS_ALERTS_SEARCH_BAR_ID}
setEsQuery={setEsQuery}
urlStorageKey={URL_STORAGE_KEY}
queries={ruleQuery.current}
/>
<EuiSpacer size="s" />
Expand Down