Refactor Observability Overview Page #146182

CoenWarmer · 2022-11-23T16:24:01Z

Resolves #156986

Summary

This PR aims to refactor the Overview page for more maintainability and better legibility.

Functionally the page should behave exactly the same.

It is recommended to review this PR commit by commit.

Guidelines while refactoring:

Consistent use of page title components (pageTitle: string | component, rightSideItems: [<HeaderActions />])
Break up components to make them easier to read, mock, test
Separation of concerns
Place JSX defined before the return of the component in its own component
Reusable business logic into hooks
Reuse newly created hooks
Functions that are exported and only used once in the codebase don't have to be their own functions (yet), they can be defined inside the calling component

Part of refactoring effort of other AO pages:

Refactor Observability Overview Page #146182 (<-- you are here)
Refactor Rules Page #146193
Refactor Rule Detail Page #146192

formgeist · 2022-11-24T11:09:36Z

@CoenWarmer This seems out of scope for a @elastic/observability-design review in terms of code refactor. Can you seek a review from an engineer within @elastic/observability-ui to review this PR?

formgeist · 2022-11-24T11:10:27Z

@CoenWarmer Design will take a look for possible visual regressions, but we don't have the knowledge to review the code itself.

CoenWarmer · 2022-11-24T11:48:33Z

@CoenWarmer This seems out of scope for a @elastic/observability-design review in terms of code refactor. Can you seek a review from an engineer within @elastic/observability-ui to review this PR?

@formgeist Yes, certainly.

I will also look into updating the CODEOWNERS file to include either Obs-UI and/or AO so we will have a team that can review code when changes are made to the page as well. Thanks!

formgeist · 2022-11-24T12:46:56Z

I will also look into updating the CODEOWNERS file to include either Obs-UI and/or AO so we will have a team that can review code when changes are made to the page as well. Thanks!

Sounds good thanks

formgeist

Design LGTM - I did notice a regression that isn't because of this PR, but something must've changed due to updates to EUI. Wondering if you have time to take a quick look on whether this is an easy fix. The spacing in the footer elements is pretty large, so wonder if there's some messed up flexbox happening.

CoenWarmer · 2022-11-24T13:17:31Z

Design LGTM - I did notice a regression that isn't because of this PR, but something must've changed due to updates to EUI. Wondering if you have time to take a quick look on whether this is an easy fix. The spacing in the footer elements is pretty large, so wonder if there's some messed up flexbox happening.

@formgeist Good catch! Is indeed due to the changes in Eui. Will include a fix for that.

…overview-page

CoenWarmer · 2022-11-24T14:59:04Z

@formgeist now fixed!

Also tweaked the HeaderActions a bit so it will look nice on smaller viewports:

Screen.Recording.2022-11-24.at.15.58.05.mov

kibana-ci · 2022-11-24T16:23:42Z

💚 Build Succeeded

Buildkite Build
Commit: 88127fa

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`observability`	436	439	+3

Async chunks

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

id	before	after	diff
`observability`	495.7KB	495.8KB	+51.0B

Unknown metric groups

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
`osquery`	109	115	+6
`securitySolution`	443	449	+6
total			+20

Total ESLint disabled count

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

History

💚 Build #90393 succeeded 057ca84

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

kdelemme

Works fine locally!
Good job with this refactoring 👏🏻 This is a really useful endeavour you're taking on with these refactoring!

🍰 If I may suggest to try to split the changes into smaller commits and keeping the changes together.
For example, the PageHeader is extracted from the main component in one commit, but it does not show the removal from the main page component. Making it difficult to review: is it just an extract or there is also some refactoring?
Same for lifting some variables definition in the component, this could be just one commit so it is easier to review.

That being said, I know it's not always possible to do such small incremental changes so take this comment with a pinch of salt ;)

CoenWarmer · 2022-11-24T20:27:31Z

Thanks @kdelemme! Yeah, I agree it's not always easy to review this type of work. I've taken the first step (splitting up the work in many smaller commits) but it would be even better if the breaking off of components from the main component and the creation of the new file is in one commit. There's still the Alerts page to go after this round of PRs, so I'll do my best to take that approach in that PR.

* main: (30 commits) [Cloud Posture] test latest findings table sort (elastic#144668) [api-docs] 2022-11-28 Daily api_docs build (elastic#146359) [api-docs] 2022-11-27 Daily api_docs build (elastic#146353) [api-docs] 2022-11-26 Daily api_docs build (elastic#146350) [DataViews] Fix form validation UX when the same data view name already exists (elastic#146126) [Discover] Prevent agg based visualizations of Discover saved objects with adhoc data views (elastic#145583) [Health Gateway] Update response aggregation (elastic#145761) [api-docs] 2022-11-25 Daily api_docs build (elastic#146341) [Metric threshold rule] Adds new context variable for group by keys (elastic#145654) [Controls] [Portable Dashboards] Add control group renderer example plugin (elastic#146189) Refactor Observability Overview Page (elastic#146182) Send complete test data to xMatters, so it can create an alert (elastic#145431) [Dashboard] [Controls] Allow options list suggestions to be sorted (elastic#144867) Add open API specification for list connector types (elastic#145951) skip flaky suite (elastic#146086) [ML] Removing duplicate tooltip text (elastic#146308) Refactor Rules Page (elastic#146193) [DOCS] Alert limit for cases (elastic#145950) Extend session index fields mapping with a session creation timestamp. (elastic#145997) [Files] Move <Image /> component to `@kbn/shared-ux` package (elastic#145995) ...

CoenWarmer added 6 commits November 23, 2022 17:18

Break out data assistant flyout into its own component

936ce3a

Break out header actions into its own component

a9b04fe

Create a useMetrics hook

041d862

Export all components from index.ts

52dd01d

Add prop to allow setting of width for date picker

4790978

Refactor Overview page

057ca84

CoenWarmer requested review from a team as code owners November 23, 2022 16:24

CoenWarmer added the release_note:skip Skip the PR/issue when compiling release notes label Nov 23, 2022

This was referenced Nov 23, 2022

Refactor Rules Page #146193

Merged

Refactor Rule Detail Page #146192

Closed

formgeist approved these changes Nov 24, 2022

View reviewed changes

CoenWarmer added 4 commits November 24, 2022 14:41

Merge branch 'main' of github.com:elastic/kibana into chore/refactor-…

8e187bc

…overview-page

Fix styling as result of Eui changes

d1e3c96

Make HeaderActions responsive

6a53f0e

Cleanup

3d5da08

Reuse alert consumers variable

88127fa

CoenWarmer force-pushed the chore/refactor-overview-page branch from c71d5c1 to 88127fa Compare November 24, 2022 15:24

kdelemme approved these changes Nov 24, 2022

View reviewed changes

CoenWarmer merged commit dd86c7f into elastic:main Nov 24, 2022

CoenWarmer deleted the chore/refactor-overview-page branch November 24, 2022 20:27

kibanamachine added the v8.7.0 label Nov 24, 2022

kibanamachine added the backport:skip This commit does not require backporting label Nov 24, 2022

CoenWarmer mentioned this pull request May 8, 2023

Refactor Overview page #156986

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Observability Overview Page #146182

Refactor Observability Overview Page #146182

CoenWarmer commented Nov 23, 2022 •

edited

Loading

formgeist commented Nov 24, 2022

formgeist commented Nov 24, 2022

CoenWarmer commented Nov 24, 2022

formgeist commented Nov 24, 2022

formgeist left a comment

CoenWarmer commented Nov 24, 2022 •

edited

Loading

CoenWarmer commented Nov 24, 2022 •

edited

Loading

kibana-ci commented Nov 24, 2022

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

kdelemme left a comment

CoenWarmer commented Nov 24, 2022

Refactor Observability Overview Page #146182

Refactor Observability Overview Page #146182

Conversation

CoenWarmer commented Nov 23, 2022 • edited Loading

Summary

Part of refactoring effort of other AO pages:

formgeist commented Nov 24, 2022

formgeist commented Nov 24, 2022

CoenWarmer commented Nov 24, 2022

formgeist commented Nov 24, 2022

formgeist left a comment

Choose a reason for hiding this comment

CoenWarmer commented Nov 24, 2022 • edited Loading

CoenWarmer commented Nov 24, 2022 • edited Loading

kibana-ci commented Nov 24, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

kdelemme left a comment

Choose a reason for hiding this comment

CoenWarmer commented Nov 24, 2022

CoenWarmer commented Nov 23, 2022 •

edited

Loading

CoenWarmer commented Nov 24, 2022 •

edited

Loading

CoenWarmer commented Nov 24, 2022 •

edited

Loading