[Enterprise Search] Fixes Search Index page to go blank when connection lost

[efegurkan]

Summary

Previously when we lose connection to backend, Search Index page was going blank. This PR prevents page to go blank, but rather show a flash message.

After fix:

Screen.Recording.2022-10-26.at.13.22.27.mov

Previously:

Screen.Recording.2022-10-26.at.13.23.34.mov

Checklist

• Unit or functional tests were updated or added to match the most common scenarios

[efegurkan]

This is eventually data duplication. We have the "index" selector already, but it is connected directly to the FetchIndexApiLogic.data. I would like to fix the connector state as well, just wanted to check with you before what would you expect on connector side when connection lost.

I think it should still hold information that is available on the UI. I am adding changes, and will push them here. Sound good? @sphilipse @byronhulcher

Screen.Recording.2022-10-26.at.13.32.01.mov

[sphilipse]

I am honestly not a fan of this change. There's two reasons why I designed it this way:

1. We should at all times have easy access to the original API result.
2. By moving the index transformation to a selector, we take advantage of selector memoization and only transform that data if something has actually changed.

The way you've done it now creates two problems:

1. The view index logic always needs to be loaded before the API logic, because you're hooking into those actions. If they are loaded in the reverse order (which is definitely possible, and can create very hard to diagnose bugs later on), your view index logic just breaks because it won't be triggered.

2. You're breaking any kind of memoization we may have in place there. That's not a huge deal because that memoization is probably broken anyway for that specific selector, but it's just not great practice in general.

Redux works best when you have the original data in a single place, and then use selectors to transform that data in places where you need it. That guarantees that you won't have out-of-sync data issues, and it takes advantage of memoization to make things a little more efficient.

Relying too heavily on actions is the easiest way to create very obscure and hard-to-diagnose bugs.

[sphilipse]

What I do like is that we should probably stop importing indexData directly and instead rely on the viewIndex. But we should do that without hooking into actions to start setting data. That's an anti-pattern.

[efegurkan]

I agree with your points. The current issue is we rely heavily on the API logic instead of the view index logic, which causes issues when the polling triggers and API data is lost. We need to cache this data somewhere so there is something to show still even if there is an error from the endpoint.
So there are a few ways to do it I believe,

• one is to have a logic file to handle the state management and memoization - in this case it is index view logic -, and use API logic files to only get the data.
• Another way is to make the API logic keep the data until the next reset or detach happens.
• And maybe the third way of doing it would be to split the parts where we heavily need the raw api data and the parts that does not require. i.e. splitting the data where we need to keep polling and the data that we know that wouldn't change in between polls i.e. index name etc. Which may help us while fixing re-renders on each poll.

The view index logic always needs to be loaded before the API logic

I am not sure if I follow up on this as all of the API logic is bound here with a connect, and they have to be mounted before anyways?

The decision we do here could affect another bug fix that I plan to work on, so I think we should decide how to approach this without losing memoization and relying on actions to set data, so I will kindly ask more feedback on what to do next :D

[sphilipse]

We need to cache this data somewhere so there is something to show still even if there is an error from the endpoint.

The best way to do this is probably to have a wrapper logic around the API logic that handles both the initial call and the polling, and only ever interact with that wrapper logic, never with the API logic. That way you still have a single point of contact with those actions and you guarantee that the API logic is always loaded after the wrapper logic.

It would also allow us to take advantage of memoization by manually checking if the new data differs from the old data, and only replacing it if there's a difference. That way all the derived data will still have the same in-memory id and memoize correctly. So you'd have a logic that looks a little something like:

{
  actions: {
    loadInitialData: ({ indexName: string}) => // makes initial fetch index call,
    pollData: () =>  // polls data
  },
  connect: { 
    actions: [
      FetchIndexAPILogic, [
        'apiReset as resetFetchIndexApi',
        'apiSuccess as fetchIndexApiSuccess',
        'makeRequest as makeFetchIndexRequest',
      ],
    values: [
      FetchIndexAPILogic, [
        'status'
    ],
  },
  reducers: [
indexData: [
      null,
      {
        fetchIndexApiSuccess: (currentIndexData, newIndexData) => {
          return isDeepEqual(currentIndexData, newIndexData) ? currentIndexData: newIndexData
        },
        resetFetchIndexApi: null,
      },
    ],
]
}

Then you import the indexData from this wrapper into the IndexViewLogic, and call loadInitialData and pollData from here, and the rest should just work as is. You could even move the polling logic into this logic file instead.

There's a second advantage in that this limits the bloat we've created in this logic file and splits it up a little more.

I am not sure if I follow up on this as all of the API logic is bound here with a connect, and they have to be mounted before anyways?

If the API logic is loaded first and then loads data and then the view index logic is loaded later, the view index logic will not contain any data despite it being in the API logic.

[efegurkan]

That's neat, and solves three problems at once 😄

I will update this and see how it looks. If we agree, add it to the guidelines as well.

[sphilipse]

question: I'm a bit confused by this change. What is this accomplishing?

[efegurkan]

TS was complaining about this, as I was passing ElasticsearchIndexWithIngestion typed index, where it was not compatible with first two overloads.

After talking with @TattdCodeMonkey offline, I think better solution is to change index to ElasticsearchIndex rather.

[TattdCodeMonkey]

lots of clean-up and improvements here 👍

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: eb14665

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
enterpriseSearch	1842	1843	+1

Async chunks

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

id	before	after	diff
enterpriseSearch	2.0MB	2.0MB	+795.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	108	113	+5
securitySolution	440	446	+6
total			+19

Total ESLint disabled count

id	before	after	diff
enterpriseSearch	20	22	+2
fleet	67	73	+6
osquery	109	115	+6
securitySolution	517	523	+6
total			+20

History

• 💚 Build #86338 succeeded feffbbf
• 💚 Build #85852 succeeded 5767b19
• 💚 Build #82858 succeeded a4759f3
• 💚 Build #82788 succeeded 9b903c0

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