ui: add date range selector in stmts and txns pages

[xinhaoz]

Resolves #68089, #69474

This commit adds date range state
properties and a date range selector component to the DB
console's statements and transactions pages. This change
is necessary to support surfacing persisted
statistics in the DB console.

This commit adds date range state
properties and a date range selector component to the DB
console's statements and transactions pages. This change
is necessary to support surfacing persisted
statistics in the DB console.

The date range query parameters used by the api to fetch
data is stored in localSettings on db console and
localStorage in cluster-ui. The default date range
is set to 1 hour ago, and is used as the value when user's
reset the date range.

The user is able to select dates as far back as 1 year ago,
this does not mean there is data available.

Release justification: category 4 low risk, high benefit
changes to existing functionality

Release note (ui change): New date range selector component
to the DB console's statements page with the ability to show
historical data. The default date range is set to 1 hour ago,
and is used as the value when user's reset the date range.

---

Transactions page:

[cockroach-teamcity]

This change is 

[maryliag]

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

---

pkg/ui/src/views/statements/statementsPage.tsx, line 232 at r6 (raw file):

      // We currently don't have the support for determining the bounds
      // of persisted statement data, aside from issuing a get all.
      validStatementsDateRange: [

I don't we should limit one year or trying to find the oldest data. The picker itself can go back as far as possible and the user selects what makes more sense. Is common to have a selector going back more than you have data for, we simply show what we have
(make that change on the other statementsPage too, so have older data range)

[matthewtodd]

A tour de force, nice work!

I am wondering whether we want to expose this "combined" concept up through the API, or whether we see it more as an implementation detail of The Future way to view statements, in which case maybe we take over the existing /_status/statements endpoint, making the "combined" behavior opt-in with a query parameter for now, but eventually making "combined" the default?

What do you all think?

Reviewed 1 of 19 files at r3, 11 of 34 files at r5, 35 of 35 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)

[maryliag]

Reviewed 1 of 35 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @matthewtodd and @xinhaoz)

---

pkg/ui/cluster-ui/src/statementsPage/statementsPage.selectors.ts, line 49 at r7 (raw file):

export const statementsSelector = createSelector(
  adminUISelector,
  adminUiState => adminUiState.combinedStatements,

you may need to change back to .statements, otherwise it may not work as expected on the cases you're calling the statements api

---

pkg/ui/cluster-ui/src/statementsPage/statementsPageConnected.tsx, line 59 at r7 (raw file):

    }),
    (dispatch: Dispatch) => ({
      refreshStatements: () => dispatch(combinedStatementActions.refresh()),

similar to previous comments, we won't be refreshing combined on all cases, so might need to handle here too

---

pkg/ui/src/redux/statements/statementsSelectors.ts, line 18 at r7 (raw file):

export const selectStatementByFingerprint = createSelector(
  (state: AdminUIState) => state.cachedData.combinedStatements.data?.statements,

similar to the other comments

---

pkg/ui/src/views/statements/statementDetails.tsx, line 153 at r7 (raw file):

export const selectStatement = createSelector(
  (state: AdminUIState) => state.cachedData.combinedStatements,

same comment as other cases

---

pkg/ui/src/views/statements/statementsPage.tsx, line 67 at r7 (raw file):

// StatementsPage, based on if the appAttr route parameter is set.
export const selectStatements = createSelector(
  (state: AdminUIState) => state.cachedData.combinedStatements,

same as others

[Azhng]

Nice. Great work on this. Just some quick drive-by comments.

Also, do you mind put some screenshots into the PR description ?

Reviewed 1 of 19 files at r3, 11 of 34 files at r5, 4 of 35 files at r6, 3 of 14 files at r7.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @matthewtodd and @xinhaoz)

---

-- commits, line 42 at r6:
release justification?

---

pkg/ui/cluster-ui/src/api/fetchData.ts, line 38 at r6 (raw file):

// - non-string values will be toString'd
export function propsToQueryString(props: { [k: string]: any }) {
  return _.compact(

in other pages we are trying to move from using lodash to using vanilla js array functions (#68820). Do we really need lodash here?

---

pkg/ui/cluster-ui/src/store/localStorage/localStorage.reducer.ts, line 42 at r7 (raw file):

const initialState: LocalStorageState = {
  "adminUi/showDiagnosticsModal":
    Boolean(JSON.parse(localStorage.getItem("adminUi/showDiagnosticsModal"))) ||

This is interesting. Why do we need to JSON.parse it ?

[maryliag]

the phrasing sounds a little weird, can you change to “Date interval not valid” if the before happens after the end, and “Select a start and end date” when one of them is not selected

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

[xinhaoz]

Message is from figma, cc @Annebirzin.
Will update it to the suggested for now!

[maryliag]

Reviewed 1 of 67 files at r8, 4 of 5 files at r15, 1 of 1 files at r16, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, @matthewtodd, and @xinhaoz)

[maryliag]

Reviewed 41 of 47 files at r17.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @Azhng, @maryliag, @matthewtodd, and @xinhaoz)

---

-- commits, line 28 at r17:
nit: and transaction page

---

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx, line 168 at r17 (raw file):

  if (props.dateRange == null) return null;
  return new cockroach.server.serverpb.StatementsRequest({
    combined: true,

you need to use this value from the props too, since it will be false for tenants
I added the props value on a PR currently being merged, you can rebase yours with that one and use the value from there
#69444

---

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPageConnected.tsx, line 50 at r17 (raw file):

    (state: AppState, props: StatementsPageProps) => ({
      statements: selectStatements(state, props),
      dateRange: state.adminUI.uiConfig.isTenant

once you merge with the PR I mention, change this to
dateRange: selectIsTenant(state) ? null : selectDateRange(state)
just to be on the safe side, so we have one single place of source for that value

---

pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx, line 90 at r17 (raw file):

  if (props.dateRange == null) return null;
  return new protos.cockroach.server.serverpb.StatementsRequest({
    combined: true,

similar to the other case, the combined value must check the isTenant value

---

pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPageConnected.tsx, line 43 at r17 (raw file):

      nodeRegions: nodeRegionsByIDSelector(state),
      error: selectTransactionsLastError(state),
      dateRange: state.adminUI.uiConfig.isTenant

similar to the other message, use the selector from the other PR

---

pkg/ui/workspaces/db-console/src/redux/statements/statementsSelectors.ts, line 22 at r17 (raw file):

  (statements, statementFingerprint) => {
    if (statements == null) {
      return null;

why you want to return null here? using an empty array is safer. If you're using this value to decide on using statements vs combined statements, you should be using the value of isTenant

---

pkg/ui/workspaces/db-console/src/util/api.ts, line 679 at r17 (raw file):

): Promise<StatementsResponseMessage> {
  const queryStr = propsToQueryString({
    combined: true,

this value should be based on value of isTenant

[xinhaoz]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @Azhng, @maryliag, @matthewtodd, and @xinhaoz)

---

pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx, line 90 at r17 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

similar to the other case, the combined value must check the isTenant value

Done.

---

pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPageConnected.tsx, line 43 at r17 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

similar to the other message, use the selector from the other PR

Done.

---

pkg/ui/workspaces/db-console/src/redux/statements/statementsSelectors.ts, line 22 at r17 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

why you want to return null here? using an empty array is safer. If you're using this value to decide on using statements vs combined statements, you should be using the value of isTenant

Done.

---

pkg/ui/workspaces/db-console/src/util/api.ts, line 679 at r17 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

this value should be based on value of isTenant

This is only used for db console, so we'll always be using combined api.

[maryliag]

Reviewed 4 of 11 files at r18, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @Azhng, @maryliag, @matthewtodd, and @xinhaoz)

---

pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx, line 90 at r18 (raw file):

  props: TransactionsPageProps,
): protos.cockroach.server.serverpb.StatementsRequest | null {
  if (props.isTenant || props.dateRange == null) return null;

on the previous refresh the in-memory was being updated, now your new refresh call this function and this only updates the combined data, so when it's a tenant or the user didn't make any date selection is not doing any refreshes. We still want tenant values to be refreshed, so you still return a request when is tenant or there is not data range, the difference is that you will make the call without passing any of those values.

[maryliag]

Thank you for adding changes to transaction too. I had just 3 small nits and then

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

---

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 123 at r18 (raw file):

  (statementsState, props) => {
    const statements = statementsState.data?.statements;

nit: remove blank line

---

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetailsConnected.ts, line 46 at r18 (raw file):

    statement,
    statementsError: state.adminUI.statements.lastError,
    dateRange: state.adminUI.uiConfig.isTenant ? null : selectDateRange(state),

nit: can you change this one too to use the selector instead of the parameter directly?

---

pkg/ui/workspaces/db-console/src/redux/statements/statementsSelectors.ts, line 25 at r18 (raw file):

    );
  },
);

you can remove the return now, since you retracted the other changes you made here

[xinhaoz]

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

---

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 123 at r18 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: remove blank line

Done.

---

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetailsConnected.ts, line 46 at r18 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: can you change this one too to use the selector instead of the parameter directly?

Done.

---

pkg/ui/workspaces/db-console/src/redux/statements/statementsSelectors.ts, line 25 at r18 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

you can remove the return now, since you retracted the other changes you made here

Done.

[xinhaoz]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)