ui: send request state to component, process data in components

[xinhaoz]

We should not decouple the data from a fetch request and fields
describing the request/response like inFlight, valid,
lastUpdated which are stored in redux, when passing them as props
into the component.

In traditional redux apps we we would be able to select the request
state as a whole unit, getting fields like isLoading, error and
etc with the data portion of the payload. Unfortunately, with the
our split redux stores in db-console and CC, we were not able to
select directly from the stores. Instead, we have been following a
pattern of creating selectors for each required part of the request
field (e.g. selectData, selectError, selectDataIsValid) for
each cached request, having to create a copy of each selector in both
db-console and cluster-ui. These selectors are then used to pass each
field independently as a prop. One can see how this boilerplate code
can quickly multiply with the number of new requests being created.

This pattern was probably a mistake -- we use selectors to memoize
computations when we want to transform data from the redux store.
For modern React we have built-in utilities like useMemo that can
accomplish this within the component. For many fields currently using
createSelector, like isLoading, the logic is extremely simple, where
the field is directly read and returned with no computation. This means
caching with createSelectorin most cases is unnecessary, so we have
a lot of boilerplate with no useful effects. In addition, the processing
of some of the data in each prop selectors (db-console and cluster-ui)
means we have code that gets duplicated. This is hard to maintain and
can become confusing as the prop may even go through more processing
when inside the component.

We should adopt a pattern of sending the entire request state for cached
data fetches to the components reading them. This commit introduces the
RequestState<T>> type which going forward should be used as the redux
field type for all cached data fetches in cluster-ui, and be prop type
used by components that use the response.

This commit shows an example of what can be done with the statements page
and sql stats response when we adopt this pattern. Note that for class
components, where useMemo is not available, we can just use redux's
selectors/reselect to help memoize when necessary.

Release note: None

---

DB-Console: https://www.loom.com/share/2dbc66751fce49aea25b5f341d26e76a
CC: https://www.loom.com/share/3cd57711da2441eaab9fbf593607c03d

Epic: CRDB-25476

[cockroach-teamcity]

This change is 

[maryliag]

Nice changes!
I would like to see a loom with the page working on DB and CC

Reviewed 14 of 14 files at r1, 11 of 12 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)

---

pkg/ui/workspaces/db-console/src/redux/apiReducers.ts line 650 at r1 (raw file):

// This mapped type assigns keys in object type T where the key value
// is of type V to the key value. Otherwise, it assigns it 'never'.
// THis enables one to etract keys in an object T of type V.

nit: this
nit: extract?

---

pkg/ui/workspaces/db-console/src/views/statements/statements.spec.tsx line 58 at r2 (raw file):

  fixedWindowEnd: moment(1646334815),
};

why all these tests got removed?

[j82w]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @xinhaoz)

---

pkg/ui/workspaces/cluster-ui/src/sqlActivity/util.tsx line 47 at r1 (raw file):

  const regions = filters.regions?.length > 0 ? filters.regions.split(",") : [];
  const nodes = filters.nodes?.length > 0 ? filters.nodes.split(",") : [];
  const appNames = filters.app

I don't see this being used anywhere. Am I missing something?

---

pkg/ui/workspaces/cluster-ui/src/sqlActivity/util.tsx line 144 at r1 (raw file):

      if (
        data.internal_app_name_prefix &&
        app.startsWith(data.internal_app_name_prefix)

Should there be a check if the app value exists? It checks below: apps.add(app ? app : unset);

[xinhaoz]

Loom added!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w and @maryliag)

---

pkg/ui/workspaces/cluster-ui/src/sqlActivity/util.tsx line 47 at r1 (raw file):

Previously, j82w (Jake) wrote…

I don't see this being used anywhere. Am I missing something?

Ah I think that was deleted by a merge conflict by accident (it's used in the next commit) - put it back for this commit as well.

---

pkg/ui/workspaces/cluster-ui/src/sqlActivity/util.tsx line 144 at r1 (raw file):

Previously, j82w (Jake) wrote…

Should there be a check if the app value exists? It checks below: apps.add(app ? app : unset);

Ah yes it might be a good idea to add that in. It's true we don't guarantee app is not null in key_data. I also added some tests for this case.

---

pkg/ui/workspaces/db-console/src/redux/apiReducers.ts line 650 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: this
nit: extract?

Done.

---

pkg/ui/workspaces/db-console/src/views/statements/statements.spec.tsx line 58 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

why all these tests got removed?

We don't use these selectors anymore. The selectors were just wrappers around the functions in sqlActivity/util.tsx, which are now being tested in the corresponding test file (and moved to cluster-ui).

[maryliag]

a few nits, otherwise

Reviewed 12 of 12 files at r3, 12 of 12 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @j82w and @xinhaoz)

---

pkg/ui/workspaces/cluster-ui/src/sqlActivity/util.spec.tsx line 231 at r4 (raw file):

    expect(
      filteredStmts.map(stmt => Number(stmt.aggregatedFingerprintID)).sort(),
    ).toEqual(expectedIDs);

all tests are using this very same code block (or some cases almost the same), so maybe you can create a function that receives: expectedCount, expectedIDs, stmtsRaw and the filters, so each test only have to call that function that will do all these checks

Code quote:

    const expectedCount = 3;
    const expectedIDs = [1, 2, 6];

    const statements = convertRawStmtsToAggregateStatistics(stmtsRaw);

    const filteredStmts = filteredStatementsData(
      filters,
      null,
      statements,
      false,
    );

    expect(filteredStmts.length).toEqual(expectedCount);

    expect(
      filteredStmts.map(stmt => Number(stmt.aggregatedFingerprintID)).sort(),
    ).toEqual(expectedIDs);

---

pkg/ui/workspaces/cluster-ui/src/sqlActivity/util.spec.tsx line 291 at r4 (raw file):

  it("should filter out statements with svc_lat less than filter time value", () => {
    // Create stmts with ids matching service_lat in seceonds.

nit: seconds

[xinhaoz]

Addressed feedback! TFTR!!
bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)