[Actionable Observability] Expose ObservabilityAlertSearchBar from Observability plugin #146401

maryam-saeidi · 2022-11-28T14:26:55Z

📝 Summary

In this PR, I exposed ObservabilityAlertSearchBar from the Observability plugin to be used in other plugins such as APM.
I've added ObservabilityAlertSearchBarProvider in order for other plugins to provide Kibana dependencies to the shared component.

🧪 How to test

For testing the implementation, I imported this component in the APM plugin and used it in the alerts tab, you can do the same locally by following these steps:

Import ObservabilityAlertSearchBar in APM and define related hook:

import {
  ObservabilityAlertSearchBar,
  ObservabilityAlertSearchBarProvider,
} from '@kbn/observability-plugin/public';

export const useToasts = () =>
  useKibana<ApmPluginStartDeps>().services.notifications!.toasts;

Replace AlertsTableStatusFilter with the ObservabilityAlertSearchBar component:

<ObservabilityAlertSearchBarProvider
  {...services}
  useToasts={useToasts}
>
  <ObservabilityAlertSearchBar
    appName={'apmApp'}
    kuery={''}
    onRangeFromChange={(input) => console.log(input)}
    onRangeToChange={(input) => console.log(input)}
    onKueryChange={(input) => console.log(input)}
    onStatusChange={(input) => console.log(input)}
    onEsQueryChange={(input) => console.log(input)}
    rangeTo={'now'}
    rangeFrom={'now-15m'}
    status={'all'}
  />
</ObservabilityAlertSearchBarProvider>

You should see the new search bar in APM alerts tab:

…plugin

maryam-saeidi · 2022-11-28T14:27:50Z

x-pack/plugins/observability/public/components/shared/alert_search_bar/alert_search_bar.tsx

@@ -27,17 +27,17 @@ const getAlertStatusQuery = (status: string): Query[] => {

 export function ObservabilityAlertSearchBar({
  appName,
+  defaultSearchQueries = DEFAULT_QUERIES,
+  onEsQueryChange,


Renamed for readability

maryam-saeidi · 2022-11-28T14:30:41Z

x-pack/plugins/triggers_actions_ui/public/application/sections/alerts_search_bar/types.ts

    dateRange: { from: string; to: string; mode?: 'absolute' | 'relative' };
-    query?: string;
+    query: string;


In the implementation, we always pass a string to this function and in the observability plugin, we rely on getting a string for the query (if it is undefined, '' will be returned).

…tSearchBarProvider

clintandrewhall

Some initial feedback. Happy to chat more any time!

x-pack/plugins/observability/public/components/shared/alert_search_bar/alert_search_bar.tsx

clintandrewhall · 2022-11-29T16:34:06Z

x-pack/plugins/observability/public/components/shared/alert_search_bar/alert_search_bar.tsx

-              onChange={(id) => {
-                setStatus(id as AlertStatus);
-              }}
+              onChange={(id) => onStatusChange(id as AlertStatus)}


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

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

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)

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.

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.

@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.

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.

@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.

clintandrewhall · 2022-11-29T16:34:41Z

...s/observability/public/components/shared/alert_search_bar/alert_search_bar_with_url_sync.tsx


-  return <ObservabilityAlertSearchBar {...stateProps} {...searchBarProps} />;
+  return (
+    <ObservabilityAlertSearchBarProvider {...services}>


I would move this up to your React root.

What is the benefit of that?

see: #146401 (comment)

clintandrewhall · 2022-11-29T16:35:12Z

...s/observability/public/components/shared/alert_search_bar/alert_search_bar_with_url_sync.tsx


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


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

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?

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.

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?

...ublic/components/shared/alert_search_bar/containers/use_alert_search_bar_state_container.tsx

clintandrewhall · 2022-11-29T16:37:10Z

x-pack/plugins/observability/public/components/shared/alert_search_bar/services.tsx

+      value={{
+        timeFilterService: services.data.query.timefilter.timefilter,
+        errorToast: services.notifications.toasts.addError,
+        AlertsSearchBar: services.triggersActionsUi.getAlertsSearchBar,


Should this be the result of getAlertsSearchBar...?

It can be, but we can also use it as a functional component without calling it first.

clintandrewhall · 2022-11-29T16:46:19Z

x-pack/plugins/observability/public/components/shared/alert_search_bar/services.tsx

+  return (
+    <ObservabilityAlertSearchBarContext.Provider
+      value={{
+        timeFilterService: services.data.query.timefilter.timefilter,


nit: I would suggest destructuring your incoming services and make this a bit easier to read, and avoid referential inequality:

const { data, notifications, triggersActionsUi } = services; // You could avoid this part, I just did this to make it clearer what is being pulled from where. const timeFilterService = data.query.timefilter.timefilter; const errorToast = notifications.toasts.addError; const AlertsSearchBar = triggersActionsUi.getAlertsSearchBar(); // You can optionally `useMemo` this, depending on the volatility of the services you consume. const services = { timeFilterService, errorToast, AlertsSearchBar, }; /* const services = useMemo({ timeFilterService, errorToast, AlertsSearchBar, }), [timeFilterService, errorToast, AlertsSearchBar]); */ return ( <ObservabilityAlertSearchBarContext.Provider value={services}> ...

clintandrewhall · 2022-11-29T16:48:43Z

x-pack/plugins/observability/public/components/shared/alert_search_bar/types.ts

+
+export interface Services {
+  timeFilterService: TimefilterContract;
+  AlertsSearchBar: (props: AlertsSearchBarProps) => ReactElement<AlertsSearchBarProps>;


I don't think this level of specificity is necessary for your use case. Unless you're interacting directly with those props, you can just say, "this is a React component I'm rendering":

AlertsSearchBar: () => ReactNode;

Since I am passing the related props to this component, I expect the component to get AlertsSearchBarProps.

x-pack/plugins/observability/public/components/shared/alert_search_bar/types.ts

elasticmachine · 2022-11-30T13:49:34Z

Pinging @elastic/actionable-observability (Team: Actionable Observability)

maryam-saeidi · 2022-11-30T13:59:40Z

x-pack/plugins/observability/public/components/shared/alert_search_bar/services.tsx

+      timefilter: { timefilter: timeFilterService },
+    },
+  },
+  useToasts,


I've tried to pass toast directly similar to other dependencies but it will cause an issue regarding this in the toasts API. So I passed useToasts to call useKibana when we need to show the toast. I saw useToasts in other plugins such as cases too.

kdelemme

Tested locally and works as expected. LGTM

kdelemme · 2022-11-30T16:41:07Z

x-pack/plugins/observability/public/components/shared/alert_search_bar/services.tsx

+  useToasts,
+  triggersActionsUi: { getAlertsSearchBar: AlertsSearchBar },
+}) => {
+  const services = useMemo<Services>(


I noticed we use useMemo / useCallback almost everywhere in kibana.
I wonder if we really need a useMemo here?

Indeed, in this case, it made sense to me conceptually but maybe in reality it is not needed. I will try to gather more information about this topic in order to have a better process in making those decisions.

This is a really well written article about useMemo and useCallback. https://www.developerway.com/posts/how-to-use-memo-use-callback

tl;dr: there is a high chance it is not leading to performance improvements, and it certainly leads to less readable components.

The React team has been experimenting with a compiler that can optimize JSX, applying useMemo / useCallback where it can really improve performance called React Forget: https://www.youtube.com/watch?v=lGEMwh32soc.

This doesn't mean that this particular compiler will ever make it. It does serve to illustrate that the React team is aware that useCallback / useMemo APIs don't provide easy enough tools to decide whether or not it actually makes sense to use them.

@CoenWarmer Thanks for the links, I will check them and apply the result accordingly 👍🏻

CoenWarmer · 2022-12-01T10:08:07Z

...ck/plugins/observability/public/components/shared/alert_search_bar/alert_search_bar.test.tsx

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


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

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

...ck/plugins/observability/public/components/shared/alert_search_bar/alert_search_bar.test.tsx

CoenWarmer · 2022-12-01T10:20:26Z

x-pack/plugins/observability/public/components/shared/alert_search_bar/alert_search_bar.tsx

-import { useKibana } from '../../../utils/kibana_react';
-import { observabilityAlertFeatureIds } from '../../../config';
-import { ObservabilityAppServices } from '../../../application/types';
+import { useServices } from './services';


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

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.

CoenWarmer

On the whole looks good. I have some concerns about introducing a new context and useServices hook making things more complex for which I've left some comments.

I can't fully judge why this component needs to live in the observability and not triggerActionsUi plugin. Do you know?

maryam-saeidi · 2022-12-01T11:28:43Z

I can't fully judge why this component needs to live in the observability and not triggerActionsUi plugin. Do you know?

Yes, I've decided on that based on the discussion with @XavierM. The extra filtering for active/recovered alerts is the design we want to use in Observability-related apps and not in other solutions such as Security, so I implemented it in the Observability plugin to be used everywhere. (In future, we will change this filter to use controls instead)

kibana-ci · 2022-12-06T15:12:15Z

💚 Build Succeeded

Buildkite Build
Commit: d785a1b

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`observability`	449	451	+2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`observability`	567	571	+4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`observability`	497.9KB	510.7KB	+12.7KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`observability`	31	33	+2

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`observability`	73.1KB	74.0KB	+924.0B

Unknown metric groups

API count

id	before	after	diff
`observability`	571	575	+4

async chunk count

id	before	after	diff
`observability`	17	18	+1

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	59	65	+6
`observability`	45	46	+1
`osquery`	109	115	+6
`securitySolution`	443	449	+6
total			+21

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	68	74	+6
`observability`	52	53	+1
`osquery`	110	117	+7
`securitySolution`	520	526	+6
total			+22

History

💚 Build #92583 succeeded 5aa3496
💛 Build #92348 was flaky cbc898e
💚 Build #92138 succeeded a52525b
💚 Build #91773 succeeded 307af94
💔 Build #91405 failed 6b2b4c1
💔 Build #91391 failed 5489109

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @maryam-saeidi

## 📝 Summary In the previous [PR](#146401), I've added a provider to pass Kibana dependencies to the observability alert search bar component. Since then, I had a lot of discussions about whether it makes sense to have a context for the dependencies of this component or not. Since we haven't decided yet whether this pattern should be adopted or not and there are suggestions about having one provider for the alert-related components, I refactored it to pass the dependency via props instead. ## 🧪 How to test Add this component to [APM](https://github.com/elastic/kibana/blob/main/x-pack/plugins/apm/public/components/app/alerts_overview/index.tsx): ``` // Add import { ObservabilityAlertSearchBar } from '@kbn/observability-plugin/public'; export const useToasts = () => useKibana<ApmPluginStartDeps>().services.notifications!.toasts; // Update const { triggersActionsUi: { getAlertsStateTable: AlertsStateTable, getAlertsSearchBar: AlertsSearchBar, alertsTableConfigurationRegistry, }, data: { query: { timefilter: { timefilter: timeFilterService }, }, }, } = services; // Replace // <AlertsTableStatusFilter // status={alertStatusFilter} // onChange={setAlertStatusFilter} // /> <ObservabilityAlertSearchBar appName={'apmApp'} kuery={''} onRangeFromChange={(input) => console.log(input)} onRangeToChange={(input) => console.log(input)} onKueryChange={(input) => console.log(input)} onStatusChange={(input) => console.log(input)} onEsQueryChange={(input) => console.log(input)} rangeTo={'now'} rangeFrom={'now-15m'} status={'all'} services={{ timeFilterService, AlertsSearchBar, useToasts }} /> ```

Refactor ObservabilityAlertSearchBar and expose it via Observability …

4e96f35

…plugin

maryam-saeidi commented Nov 28, 2022

View reviewed changes

maryam-saeidi added 4 commits November 29, 2022 14:23

Implement ObservabilityAlertSearchBarKibanaProvider

b693b11

Rename ObservabilityAlertSearchBarKibanaProvider to ObservabilityAler…

f937182

…tSearchBarProvider

Merge branch 'main' into 146286-observability-alert-search-bar

5489109

Fix import

6b2b4c1

clintandrewhall reviewed Nov 29, 2022

View reviewed changes

maryam-saeidi added 2 commits November 30, 2022 13:41

Fix toast undefined error and related functional test

7275cc6

Merge branch 'main' into 146286-observability-alert-search-bar

307af94

maryam-saeidi marked this pull request as ready for review November 30, 2022 13:48

maryam-saeidi requested review from a team as code owners November 30, 2022 13:48

maryam-saeidi self-assigned this Nov 30, 2022

maryam-saeidi added release_note:skip Skip the PR/issue when compiling release notes Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" labels Nov 30, 2022

maryam-saeidi commented Nov 30, 2022

View reviewed changes

kdelemme approved these changes Nov 30, 2022

View reviewed changes

CoenWarmer reviewed Dec 1, 2022

View reviewed changes

...ck/plugins/observability/public/components/shared/alert_search_bar/alert_search_bar.test.tsx Outdated Show resolved Hide resolved

CoenWarmer reviewed Dec 1, 2022

View reviewed changes

maryam-saeidi added 3 commits December 1, 2022 17:13

Remove unnecessary act

a86b076

Remove useMemo

66d173d

Merge branch 'main' into 146286-observability-alert-search-bar

a52525b

maryam-saeidi mentioned this pull request Dec 1, 2022

Integrate Observability Alert Search Bar in APM Alerts tab #146290

Closed

1 task

maryam-saeidi added 2 commits December 2, 2022 16:31

Refactoring based on comments

03598cc

Merge branch 'main' into 146286-observability-alert-search-bar

cbc898e

maryam-saeidi added 2 commits December 5, 2022 11:10

Merge branch 'main' into 146286-observability-alert-search-bar

5aa3496

Merge branch 'main' into 146286-observability-alert-search-bar

d785a1b

jcger approved these changes Dec 7, 2022

View reviewed changes

maryam-saeidi merged commit 9b828e3 into elastic:main Dec 7, 2022

kibanamachine added v8.7.0 backport:skip This commit does not require backporting labels Dec 7, 2022

maryam-saeidi mentioned this pull request Dec 13, 2022

[AO] Refactor observability alert search bar #147461

Merged

maryam-saeidi deleted the 146286-observability-alert-search-bar branch December 19, 2022 10:29

[Actionable Observability] Expose ObservabilityAlertSearchBar from Observability plugin #146401

[Actionable Observability] Expose ObservabilityAlertSearchBar from Observability plugin #146401

Conversation

maryam-saeidi commented Nov 28, 2022 • edited Loading

📝 Summary

🧪 How to test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintandrewhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoenWarmer Dec 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintandrewhall Nov 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Nov 30, 2022

Choose a reason for hiding this comment

kdelemme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoenWarmer Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryam-saeidi Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

CoenWarmer left a comment • edited Loading

Choose a reason for hiding this comment

maryam-saeidi commented Dec 1, 2022 • edited Loading

kibana-ci commented Dec 6, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

async chunk count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

maryam-saeidi commented Nov 28, 2022 •

edited

Loading

CoenWarmer Dec 5, 2022 •

edited

Loading

clintandrewhall Nov 29, 2022 •

edited

Loading

CoenWarmer Dec 1, 2022 •

edited

Loading

maryam-saeidi Dec 1, 2022 •

edited

Loading

CoenWarmer left a comment •

edited

Loading

maryam-saeidi commented Dec 1, 2022 •

edited

Loading