[Logs UI] Refactor log entry data fetching to hooks #51526

Zacqary · 2019-11-22T23:00:44Z

Summary

Refactors the log entry data fetching to use React hooks without Redux. Creates temporary bridges to Apollo and Redux as we continue to work on removing them from other parts of the data layer.

Update: Refactored to use encapsulated hooks similar to how other features are already implemented

Data store hooks are moved into a folder called /store/v2, which includes several different hooks for different parts of the application state, and a context provider component to make them accessible to components.

~~It's ultimately a similar structure to the Redux data model but with significantly reduced complexity.~~

This is a departure from the way other data-fetching hooks have been implemented so far. I went with this approach because of how tightly-coupled the Log Entries, Log Position, and Log Filter states are. Since we plan to refactor all three of them in rapid succession, it didn't seem like I'd be able to make the Log Entries state self-contained in the same way Highlights, Summary, and Analysis are. The Highlights state also tightly depends on these three states, so I'm wondering if we want to reevaluate its implementation.

~~Details:~~

Reducers are replaced by some useState hooks. Data fetching uses a useReducer hook with a Redux-like pattern to track its loading/loaded/error logic, but action types are placed in a single enum and don't need to be exported across multiple files.
A change in part of the app state updates the context, triggering re-renders of any components that include the useStore hook. We can further optimize this to only re-render components that depend on updated parts of the state.
~~Epics are replaced by useEffects which watch for dependency changes rather than observable streams of actions.~~
Data fetching is triggered by changes to the logPosition and logFilter states, so the top-level Store component also connects these bits of the state to the logEntries state. This uses a selector to only pull certain parts of the other states that it needs. We can still make use of the reselect library to build selectors, as it can select from any plain object and not just a Redux store.

I worry that the last item may be creating a circular dependency structure, but that might not actually be all that different from epics depending on action types from other sections of the state and vice-versa.

Known Issues

sourceId can only be default in this PR. The sourceId is determined through a lot of weird layers of context and I feel like it requires its own issue to refactor. I may need some help figuring that one out.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
~~[ ] Unit or functional tests were updated or added to match the most common scenarios~~
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

…g-entries

elasticmachine · 2019-11-22T23:00:46Z

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

elasticmachine · 2019-11-22T23:49:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 9b203b7

elasticmachine · 2019-11-25T18:08:45Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: ccee4f2

elasticmachine · 2019-11-25T22:25:25Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 1017ae5

elasticmachine · 2019-11-26T18:27:03Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 8c597a2

…g-entries

elasticmachine · 2019-11-27T00:24:27Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 388120d

elasticmachine · 2019-11-27T00:49:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: d61e53e

Zacqary · 2019-11-27T16:35:02Z

retest

elasticmachine · 2019-11-27T17:49:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: d61e53e

afgomez

This is a quick first pass. I'll download the branch and work my way through the code to understand everything better.

Looking good!

afgomez · 2019-11-28T09:30:32Z

x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/redux_bridges.tsx

 // Bridges Redux container state with Hooks state. Once state is moved fully from
 // Redux to Hooks this can be removed.


I guess this can go away now 🎉

Not quite yet, after #50397 and #50398 these can all go away

x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/log_highlights.tsx

x-pack/legacy/plugins/infra/public/store/v2/log_entries/gql_queries.ts

afgomez · 2019-11-28T13:21:26Z

x-pack/legacy/plugins/infra/public/store/v2/log_entries/index.ts

+
+  const [prevParams, cachePrevParams] = useState(params);
+
+  const runFetchNewEntriesRequest = async () => {


Bear in mind that I'm a hooks newbie.

I see the body of the useFetchEntriesEffect defines a bunch of functions like this one. Shouldn't these be wrapped in a useCallback hook?

I think this seemed like an unnecessary optimization because I believe the effect only ever re-runs when params change, and all of these functions depend on params. I can wrap them in useCallbacks just in case there's something I'm missing, though.

x-pack/legacy/plugins/infra/public/store/v2/log_entries/index.ts

afgomez

Please take a look at my comments above.

Thanks for tackling this <3

elasticmachine · 2019-12-02T21:46:45Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 3b5227d

…g-entries

elasticmachine · 2019-12-03T17:52:19Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 756fc64

elasticmachine · 2019-12-03T18:15:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 1c3e0c7

elasticmachine · 2019-12-03T23:35:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9a84a8d

x-pack/legacy/plugins/infra/public/containers/logs/log_entries/index.ts

elasticmachine · 2019-12-04T18:20:38Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8e9d9ee

afgomez

Code wise I don't have any more comments. Thanks for addressing the ones I left!
If @weltenwort doesn't have any more comments we can go ahead

elasticmachine · 2019-12-05T20:59:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6e544f3

…g-entries

elasticmachine · 2019-12-09T20:47:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 550e485

History

💔 Build #14305 failed 030cd5f
💚 Build #14256 succeeded 34e8afa

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

* Get initialinitial log fetch working with v2 store * Replicate shouldLoadAroundPosition logic within hooks * Reload entries on filter change * Add scroll to load additional entries functionality * Cleanup types types and remove state/remote folder * Typescript cleanup * Remove extraneous console.log * Fix typecheck * Add action to load new entries manually * Typecheck fix * Move v2 store stuff into logs containers * Typecheck fix * More typecheck fix * Remove filterQuery from log highlights redux bridge * Rename LogEntriesDependencies to LogEntriesFetchParams * Fix endless reloading bug * Fix duplicate entry rendering * Make sourceId into a dynamic parameter * Fix bug in pagesAfterEnd not being reported causing endless reload * Fix bugs with live streaming

Zacqary added 7 commits November 20, 2019 11:40

Get initialinitial log fetch working with v2 store

80b892d

Replicate shouldLoadAroundPosition logic within hooks

79bd94f

Reload entries on filter change

3d2ecae

Add scroll to load additional entries functionality

12ea512

Cleanup types types and remove state/remote folder

1bbd8c1

Typescript cleanup

97e6496

Merge remote-tracking branch 'upstream/master' into 50395-refactor-lo…

9b203b7

…g-entries

Zacqary added v8.0.0 Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Nov 22, 2019

Zacqary requested a review from a team November 22, 2019 23:00

Remove extraneous console.log

ccee4f2

Zacqary added 2 commits November 25, 2019 15:11

Fix typecheck

1017ae5

Add action to load new entries manually

8c597a2

Typecheck fix

388120d

Zacqary marked this pull request as ready for review November 26, 2019 23:33

Merge remote-tracking branch 'upstream/master' into 50395-refactor-lo…

d61e53e

…g-entries

afgomez self-requested a review November 28, 2019 09:29

afgomez reviewed Nov 28, 2019

View reviewed changes

afgomez suggested changes Nov 28, 2019

View reviewed changes

Move v2 store stuff into logs containers

3b5227d

Zacqary added 5 commits December 3, 2019 10:36

Typecheck fix

49134a2

Merge remote-tracking branch 'upstream/master' into 50395-refactor-lo…

756fc64

…g-entries

More typecheck fix

4b6bb64

Remove filterQuery from log highlights redux bridge

24bfe9e

Rename LogEntriesDependencies to LogEntriesFetchParams

1c3e0c7

Zacqary added 2 commits December 3, 2019 15:47

Fix endless reloading bug

ce1b88f

Fix duplicate entry rendering

9a84a8d

afgomez reviewed Dec 4, 2019

View reviewed changes

x-pack/legacy/plugins/infra/public/containers/logs/log_entries/index.ts Outdated Show resolved Hide resolved

Make sourceId into a dynamic parameter

8e9d9ee

afgomez approved these changes Dec 5, 2019

View reviewed changes

Fix bug in pagesAfterEnd not being reported causing endless reload

6e544f3

Zacqary added 3 commits December 9, 2019 09:44

Merge remote-tracking branch 'upstream/master' into 50395-refactor-lo…

34e8afa

…g-entries

Fix bugs with live streaming

030cd5f

Merge remote-tracking branch 'upstream/master' into 50395-refactor-lo…

550e485

…g-entries

Zacqary merged commit 21f9ab2 into elastic:master Dec 9, 2019

Zacqary deleted the 50395-refactor-log-entries branch December 9, 2019 23:25

Zacqary mentioned this pull request Dec 9, 2019

[7.x] [Logs UI] Refactor log entry data fetching to hooks (#51526) #52582

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Logs UI] Refactor log entry data fetching to hooks #51526

[Logs UI] Refactor log entry data fetching to hooks #51526

Zacqary commented Nov 22, 2019 •

edited

Loading

elasticmachine commented Nov 22, 2019

elasticmachine commented Nov 22, 2019

elasticmachine commented Nov 25, 2019

elasticmachine commented Nov 25, 2019

elasticmachine commented Nov 26, 2019

elasticmachine commented Nov 27, 2019

elasticmachine commented Nov 27, 2019

Zacqary commented Nov 27, 2019

elasticmachine commented Nov 27, 2019

afgomez left a comment

afgomez Nov 28, 2019

Zacqary Dec 3, 2019

afgomez Nov 28, 2019

Zacqary Dec 3, 2019

afgomez left a comment

elasticmachine commented Dec 2, 2019

elasticmachine commented Dec 3, 2019

elasticmachine commented Dec 3, 2019

elasticmachine commented Dec 3, 2019

elasticmachine commented Dec 4, 2019

afgomez left a comment

elasticmachine commented Dec 5, 2019

elasticmachine commented Dec 9, 2019

		// Bridges Redux container state with Hooks state. Once state is moved fully from
		// Redux to Hooks this can be removed.


		const [prevParams, cachePrevParams] = useState(params);

		const runFetchNewEntriesRequest = async () => {

[Logs UI] Refactor log entry data fetching to hooks #51526

[Logs UI] Refactor log entry data fetching to hooks #51526

Conversation

Zacqary commented Nov 22, 2019 • edited Loading

Summary

Known Issues

Checklist

For maintainers

elasticmachine commented Nov 22, 2019

elasticmachine commented Nov 22, 2019

💔 Build Failed

elasticmachine commented Nov 25, 2019

💔 Build Failed

elasticmachine commented Nov 25, 2019

💔 Build Failed

elasticmachine commented Nov 26, 2019

💔 Build Failed

elasticmachine commented Nov 27, 2019

💔 Build Failed

elasticmachine commented Nov 27, 2019

💔 Build Failed

Zacqary commented Nov 27, 2019

elasticmachine commented Nov 27, 2019

💔 Build Failed

afgomez left a comment

Choose a reason for hiding this comment

afgomez Nov 28, 2019

Choose a reason for hiding this comment

Zacqary Dec 3, 2019

Choose a reason for hiding this comment

afgomez Nov 28, 2019

Choose a reason for hiding this comment

Zacqary Dec 3, 2019

Choose a reason for hiding this comment

afgomez left a comment

Choose a reason for hiding this comment

elasticmachine commented Dec 2, 2019

💔 Build Failed

elasticmachine commented Dec 3, 2019

💔 Build Failed

elasticmachine commented Dec 3, 2019

💔 Build Failed

elasticmachine commented Dec 3, 2019

💚 Build Succeeded

elasticmachine commented Dec 4, 2019

💚 Build Succeeded

afgomez left a comment

Choose a reason for hiding this comment

elasticmachine commented Dec 5, 2019

💚 Build Succeeded

elasticmachine commented Dec 9, 2019

💚 Build Succeeded

History

Zacqary commented Nov 22, 2019 •

edited

Loading