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

[Actionable Observability] Expose ObservabilityAlertSearchBar from Observability plugin #146401

Merged
merged 14 commits into from
Dec 7, 2022
Merged

[Actionable Observability] Expose ObservabilityAlertSearchBar from Observability plugin #146401

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4e96f35
Refactor ObservabilityAlertSearchBar and expose it via Observability …
maryam-saeidi Nov 28, 2022
b693b11
Implement ObservabilityAlertSearchBarKibanaProvider
maryam-saeidi Nov 29, 2022
f937182
Rename ObservabilityAlertSearchBarKibanaProvider to ObservabilityAler…
maryam-saeidi Nov 29, 2022
5489109
Merge branch 'main' into 146286-observability-alert-search-bar
maryam-saeidi Nov 29, 2022
6b2b4c1
Fix import
maryam-saeidi Nov 29, 2022
7275cc6
Fix toast undefined error and related functional test
maryam-saeidi Nov 30, 2022
307af94
Merge branch 'main' into 146286-observability-alert-search-bar
maryam-saeidi Nov 30, 2022
a86b076
Remove unnecessary act
maryam-saeidi Dec 1, 2022
66d173d
Remove useMemo
maryam-saeidi Dec 1, 2022
a52525b
Merge branch 'main' into 146286-observability-alert-search-bar
maryam-saeidi Dec 1, 2022
03598cc
Refactoring based on comments
maryam-saeidi Dec 2, 2022
cbc898e
Merge branch 'main' into 146286-observability-alert-search-bar
maryam-saeidi Dec 2, 2022
5aa3496
Merge branch 'main' into 146286-observability-alert-search-bar
maryam-saeidi Dec 5, 2022
d785a1b
Merge branch 'main' into 146286-observability-alert-search-bar
maryam-saeidi Dec 6, 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
126 changes: 84 additions & 42 deletions ...plugins/observability/public/components/shared/alert_search_bar/alert_search_bar.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,57 +5,51 @@
* 2.0.
*/

import { triggersActionsUiMock } from '@kbn/triggers-actions-ui-plugin/public/mocks';
import React from 'react';
import { act, waitFor } from '@testing-library/react';
import { AlertSearchBarProps } from './types';
import { timefilterServiceMock } from '@kbn/data-plugin/public/query/timefilter/timefilter_service.mock';
import { useServices } from './services';
import { ObservabilityAlertSearchBarProps } from './types';
import { ObservabilityAlertSearchBar } from './alert_search_bar';
import { observabilityAlertFeatureIds } from '../../../config';
import { useKibana } from '../../../utils/kibana_react';
import { kibanaStartMock } from '../../../utils/kibana_react.mock';
import { render } from '../../../utils/test_helper';

const useKibanaMock = useKibana as jest.Mock;
const useServicesMock = useServices as jest.Mock;
const getAlertsSearchBarMock = jest.fn();
const ALERT_SEARCH_BAR_DATA_TEST_SUBJ = 'alerts-search-bar';
const ACTIVE_BUTTON_DATA_TEST_SUBJ = 'alert-status-filter-active-button';

jest.mock('../../../utils/kibana_react');

const mockKibana = () => {
useKibanaMock.mockReturnValue({
services: {
...kibanaStartMock.startContract().services,
triggersActionsUi: {
...triggersActionsUiMock.createStart(),
getAlertsSearchBar: getAlertsSearchBarMock.mockReturnValue(
<div data-test-subj={ALERT_SEARCH_BAR_DATA_TEST_SUBJ} />
),
},
},

jest.mock('./services');

const mockServices = () => {
useServicesMock.mockReturnValue({
timeFilterService: timefilterServiceMock,
AlertsSearchBar: getAlertsSearchBarMock.mockReturnValue(
Copy link
Contributor

Choose a reason for hiding this comment

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

I added getAlertsSearchBar to the TriggerActionsUI mock over here. So you might not need to reintroduce this here.

Copy link
Member Author

Choose a reason for hiding this comment

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

But this one is mocking useServices not useKibana, and I also want to check the related props, so I defined the mock here.

<div data-test-subj={ALERT_SEARCH_BAR_DATA_TEST_SUBJ} />
),
errorToast: jest.fn(),
});
};

describe('ObservabilityAlertSearchBar', () => {
const renderComponent = (props: Partial<AlertSearchBarProps> = {}) => {
const alertSearchBarProps: AlertSearchBarProps = {
const renderComponent = (props: Partial<ObservabilityAlertSearchBarProps> = {}) => {
const observabilityAlertSearchBarProps: ObservabilityAlertSearchBarProps = {
appName: 'testAppName',
rangeFrom: 'now-15m',
setRangeFrom: jest.fn(),
rangeTo: 'now',
setRangeTo: jest.fn(),
kuery: '',
setKuery: jest.fn(),
status: 'active',
setStatus: jest.fn(),
setEsQuery: jest.fn(),
onRangeFromChange: jest.fn(),
onRangeToChange: jest.fn(),
onKueryChange: jest.fn(),
onStatusChange: jest.fn(),
onEsQueryChange: jest.fn(),
rangeTo: 'now',
rangeFrom: 'now-15m',
status: 'all',
...props,
};
return render(<ObservabilityAlertSearchBar {...alertSearchBarProps} />);
return render(<ObservabilityAlertSearchBar {...observabilityAlertSearchBarProps} />);
};

beforeAll(() => {
mockKibana();
mockServices();
});

beforeEach(() => {
Expand Down Expand Up @@ -88,21 +82,20 @@ describe('ObservabilityAlertSearchBar', () => {
});

it('should filter active alerts', async () => {
const mockedSetEsQuery = jest.fn();
const mockedOnEsQueryChange = jest.fn();
const mockedFrom = '2022-11-15T09:38:13.604Z';
const mockedTo = '2022-11-15T09:53:13.604Z';
const { getByTestId } = renderComponent({
setEsQuery: mockedSetEsQuery,
rangeFrom: mockedFrom,
rangeTo: mockedTo,
});

await act(async () => {
const activeButton = getByTestId(ACTIVE_BUTTON_DATA_TEST_SUBJ);
activeButton.click();
act(() => {
maryam-saeidi marked this conversation as resolved.
Show resolved Hide resolved
renderComponent({
onEsQueryChange: mockedOnEsQueryChange,
rangeFrom: mockedFrom,
rangeTo: mockedTo,
status: 'active',
});
});

expect(mockedSetEsQuery).toHaveBeenCalledWith({
expect(mockedOnEsQueryChange).toHaveBeenCalledWith({
bool: {
filter: [
{
Expand All @@ -127,4 +120,53 @@ describe('ObservabilityAlertSearchBar', () => {
},
});
});

it('should include defaultSearchQueries in es query', async () => {
const mockedOnEsQueryChange = jest.fn();
const mockedFrom = '2022-11-15T09:38:13.604Z';
const mockedTo = '2022-11-15T09:53:13.604Z';
const defaultSearchQueries = [
{
query: 'kibana.alert.rule.uuid: 413a9631-1a29-4344-a8b4-9a1dc23421ee',
language: 'kuery',
},
];

act(() => {
renderComponent({
onEsQueryChange: mockedOnEsQueryChange,
rangeFrom: mockedFrom,
rangeTo: mockedTo,
defaultSearchQueries,
status: 'all',
});
});

expect(mockedOnEsQueryChange).toHaveBeenCalledWith({
bool: {
filter: [
{
bool: {
minimum_should_match: 1,
should: [
{ match: { 'kibana.alert.rule.uuid': '413a9631-1a29-4344-a8b4-9a1dc23421ee' } },
],
},
},
{
range: {
'@timestamp': expect.objectContaining({
format: 'strict_date_optional_time',
gte: mockedFrom,
lte: mockedTo,
}),
},
},
],
must: [],
must_not: [],
should: [],
},
});
});
});
93 changes: 44 additions & 49 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 @@ -6,16 +6,15 @@
*/

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 '../../../utils/kibana_react';
import { observabilityAlertFeatureIds } from '../../../config';
import { ObservabilityAppServices } from '../../../application/types';
import { useServices } from './services';
Copy link
Contributor

Choose a reason for hiding this comment

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

What benefit does it have to introduce a new useServices which just exports useKibana().services ?

Copy link
Member Author

@maryam-saeidi maryam-saeidi Dec 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Since this component will be used in another plugin, we don't know if they have all the related dependencies defined in their plugin as this shared component needs.
So I used a similar pattern as mentioned in Clint's presentation to make these dependencies explicit and the consumer should make sure to pass the related services to the ObservabilityAlertSearchBarProvider that I defined here.
useServices is a hook that will access the services passed to the provider.

import { AlertsStatusFilter } from './components';
import { observabilityAlertFeatureIds } from '../../../config';
import { ALERT_STATUS_QUERY, DEFAULT_QUERIES, DEFAULT_QUERY_STRING } from './constants';
import { AlertSearchBarProps } from './types';
import { ObservabilityAlertSearchBarProps } from './types';
import { buildEsQuery } from '../../../utils/build_es_query';
import { AlertStatus } from '../../../../common/typings';

Expand All @@ -27,84 +26,79 @@ const getAlertStatusQuery = (status: string): Query[] => {

export function ObservabilityAlertSearchBar({
appName,
defaultSearchQueries = DEFAULT_QUERIES,
onEsQueryChange,
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 for readability

onKueryChange,
onRangeFromChange,
onRangeToChange,
onStatusChange,
kuery,
rangeFrom,
setRangeFrom,
rangeTo,
setRangeTo,
kuery,
setKuery,
status,
setStatus,
setEsQuery,
queries = DEFAULT_QUERIES,
}: AlertSearchBarProps) {
const {
data: {
query: {
timefilter: { timefilter: timeFilterService },
},
},
notifications: { toasts },
triggersActionsUi: { getAlertsSearchBar: AlertsSearchBar },
} = useKibana<ObservabilityAppServices>().services;
}: ObservabilityAlertSearchBarProps) {
const { AlertsSearchBar, errorToast, timeFilterService } = useServices();

const onStatusChange = useCallback(
const onAlertStatusChange = useCallback(
(alertStatus: AlertStatus) => {
setEsQuery(
onEsQueryChange(
buildEsQuery(
{
to: rangeTo,
from: rangeFrom,
},
kuery,
[...getAlertStatusQuery(alertStatus), ...queries]
[...getAlertStatusQuery(alertStatus), ...defaultSearchQueries]
)
);
},
[kuery, queries, rangeFrom, rangeTo, setEsQuery]
[kuery, defaultSearchQueries, rangeFrom, rangeTo, onEsQueryChange]
);

useEffect(() => {
onStatusChange(status);
}, [onStatusChange, status]);
onAlertStatusChange(status);
}, [onAlertStatusChange, status]);

const onSearchBarParamsChange = useCallback(
const onSearchBarParamsChange = useCallback<
(query: {
dateRange: { from: string; to: string; mode?: 'absolute' | 'relative' };
query: string;
}) => void
>(
({ dateRange, query }) => {
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,
to: dateRange.to,
from: dateRange.from,
},
query,
[...getAlertStatusQuery(status), ...queries]
[...getAlertStatusQuery(status), ...defaultSearchQueries]
);
setKuery(query);
onKueryChange(query);
timeFilterService.setTime(dateRange);
setRangeFrom(dateRange.from);
setRangeTo(dateRange.to);
setEsQuery(esQuery);
onRangeFromChange(dateRange.from);
onRangeToChange(dateRange.to);
maryam-saeidi marked this conversation as resolved.
Show resolved Hide resolved
onEsQueryChange(esQuery);
} catch (error) {
toasts.addError(error, {
errorToast(error, {
title: i18n.translate('xpack.observability.alerts.searchBar.invalidQueryTitle', {
defaultMessage: 'Invalid query string',
}),
});
setKuery(DEFAULT_QUERY_STRING);
onKueryChange(DEFAULT_QUERY_STRING);
clintandrewhall marked this conversation as resolved.
Show resolved Hide resolved
}
},
[
defaultSearchQueries,
timeFilterService,
setRangeFrom,
setRangeTo,
setKuery,
setEsQuery,
rangeTo,
rangeFrom,
onRangeFromChange,
onRangeToChange,
onKueryChange,
onEsQueryChange,
status,
queries,
toasts,
errorToast,
]
);

Expand All @@ -126,13 +120,14 @@ export function ObservabilityAlertSearchBar({
<EuiFlexItem grow={false}>
<AlertsStatusFilter
status={status}
onChange={(id) => {
setStatus(id as AlertStatus);
}}
onChange={(id) => onStatusChange(id as AlertStatus)}
Copy link
Contributor

Choose a reason for hiding this comment

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

always avoid inline creation of functions and objects... they will trigger a re-render due to reference inequality.

Copy link
Contributor

Choose a reason for hiding this comment

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

For example: https://amnsingh.medium.com/purecomponent-caveats-3a42b6e50e76

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've read different articles about this topic and there are opposite views about it, such as this one:
https://reacttraining.com/blog/react-inline-functions-and-performance/
How do we check if re-rendering actually has a negative performance impact? (I assume the concern is performance)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is indeed a widely contested point.

AFAIK the performance hit is negligible (defining functions is considered cheap)

@maryam-saeidi if you want to know for sure, you could add performance.now()'s to the component and compare the different implementations. Testing in dev mode might give a skewed result, you want to test this in a production build.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if the hit is "negligible", I haven't encountered an argument that defining inline is preferable, other than that's how it was written when the PR was created.

I personally pull them out into const for readability and to avoid waffling/considering it every time.

Copy link
Contributor

Choose a reason for hiding this comment

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

@maryam-saeidi I just looked at the article you referenced... it's from 2017, and reflects class components, (e.g. PureComponent.

AFAIK, the strict equality check and re-render is always an issue with inline objects and props. @CoenWarmer it's not about defining functions, it's about the function causing a sub-component to re-render... which you can't really know how a re-render will affect children.

The rule of thumb still applies here. I don't see an argument where defining inline gains anything, where I see plenty of arguments why not to-- along with the cheapness of it, namely creating a const.

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 just looked at the article you referenced... it's from 2017, and reflects class components, (e.g. PureComponent.

Yes, since we were talking about the concepts (the article that you shared was from 2018 and also related to PureComponents) I didn't look for a newer article.

When I read react documents, I don't see a focus on avoiding re-rendering as much as possible but rather on using tools to improve performance when it is needed. Actually, I saw more focus on making sure our components work as expected even if it is slow and it re-render unnecessarily and then optimizing it.

Copy link
Contributor

@CoenWarmer CoenWarmer Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@clintandrewhall Using inline functions is considered an acceptable practice both in the old and the new React docs. In this case, the component that was passed the inline function is not memoized, so unsure as to what value not passing an arrow function would bring apart from perhaps aesthetic reasons.

I'm however not against defining functions in a const as a rule of thumb. Perhaps the component which receives the function could become a memoized component in the future, in which case you don't have to touch this component anymore which saves time and effort.

/>
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlexItem>
</EuiFlexGroup>
);
}

// eslint-disable-next-line import/no-default-export
export default ObservabilityAlertSearchBar;
10 changes: 9 additions & 1 deletion ...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 @@ -12,13 +12,21 @@ import {
useAlertSearchBarStateContainer,
} from './containers';
import { ObservabilityAlertSearchBar } from './alert_search_bar';
import { ObservabilityAlertSearchBarProvider } from './services';
import { AlertSearchBarWithUrlSyncProps } from './types';
import { useKibana } from '../../../utils/kibana_react';
import { ObservabilityAppServices } from '../../../application/types';

function AlertSearchbarWithUrlSync(props: AlertSearchBarWithUrlSyncProps) {
const { urlStorageKey, ...searchBarProps } = props;
const stateProps = useAlertSearchBarStateContainer(urlStorageKey);
const services = useKibana<ObservabilityAppServices>().services;
Copy link
Contributor

Choose a reason for hiding this comment

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

Take every opportunity to eliminate useKibana. If you move the Provider to your React root, these lines get deleted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wondering about the reasoning for it, AlertSearchbarWithUrlSync is an internal component (only used within the Observability plugin) and I put the provider here to make AlertSearchbarWithUrlSync stand alone without a need to always wrap the component with this provider on every usage. Will it cause an issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

To make a component shareable and portable, we need to prioritize decoupling it from Kibana. The useKibana hook is incredibly troublesome, as it literally allows a plugin to slam entire start contracts into context, all without any checks except at runtime.

In short: you have no idea if ObservabilityAppServices is actually in context when this component is consumed elsewhere. It's an implementation detail a consumer must know, and due to the nature of useKibana, they won't know until runtime. It's a constant problem.

This is why creating a provider for the shared component and putting it in the root is so helpful-- it allows us to provide and enforce dependency injection.

Copy link
Member Author

Choose a reason for hiding this comment

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

you have no idea if ObservabilityAppServices is actually in context when this component is consumed elsewhere.

Since this component is meant to be used inside of the Observability plugin, it can assume that ObservabilityAppServices is provided through useKibana. I understand using these shared components between plugins can cause an issue, hence the creation of the related provider, but I don't see the issue that you mentioned in sharing components inside of a plugin. Am I missing something?

This is why creating a provider for the shared component and putting it in the root is so helpful-- it allows us to provide and enforce dependency injection.

How do we do that at the root level?


return <ObservabilityAlertSearchBar {...stateProps} {...searchBarProps} />;
return (
<ObservabilityAlertSearchBarProvider {...services}>
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move this up to your React root.

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the benefit of that?

Copy link
Contributor

Choose a reason for hiding this comment

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

see: #146401 (comment)

<ObservabilityAlertSearchBar {...stateProps} {...searchBarProps} />
</ObservabilityAlertSearchBarProvider>
);
}

export function ObservabilityAlertSearchbarWithUrlSync(props: AlertSearchBarWithUrlSyncProps) {
Expand Down
5 changes: 4 additions & 1 deletion ...servability/public/components/shared/alert_search_bar/components/alerts_status_filter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import { EuiButtonGroup, EuiButtonGroupOptionProps } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import React from 'react';
import { ALL_ALERTS, ACTIVE_ALERTS, RECOVERED_ALERTS } from '../constants';
import { AlertStatusFilterProps } from '../types';
Expand Down Expand Up @@ -34,7 +35,9 @@ const options: EuiButtonGroupOptionProps[] = [
export function AlertsStatusFilter({ status, onChange }: AlertStatusFilterProps) {
return (
<EuiButtonGroup
legend="Filter by"
legend={i18n.translate('xpack.observability.alerts.alertStatusFilter.legend', {
defaultMessage: 'Filter by',
})}
color="primary"
options={options}
idSelected={status}
Expand Down
10 changes: 5 additions & 5 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 @@ -52,14 +52,14 @@ export function useAlertSearchBarStateContainer(urlStorageKey: string) {
);

return {
kuery,
onKueryChange: setKuery,
onRangeFromChange: setRangeFrom,
onRangeToChange: setRangeTo,
onStatusChange: setStatus,
maryam-saeidi marked this conversation as resolved.
Show resolved Hide resolved
rangeFrom,
setRangeFrom,
rangeTo,
setRangeTo,
kuery,
setKuery,
status,
setStatus,
};
}

Expand Down
Loading