ui: Statement Details page using new details endpoint

[maryliag]

Previously, the Statement Details page was being populated
by the value saved on cache, where we would filter the
existing statement and retrieve the correct one.
With a new endpoint _status/stmtdetails/{fingerprint_id}
we can collect more information about the statement.
This first commit changes the source of information of
the Statement Details page to the value retrieved from
the new endpoint. No changes are noticeable from the user's
perspective.
Following commits will add the new information based on the
extra information retrieved by the endpoint.

Partially addresses #72129

NoteFor Reviewers: Cluster-ui selector are still being addressed

Release note: None
Release Justification: Low risk, high benefit change

[cockroach-teamcity]

This change is 

[Azhng]

Hmm is it intentional that the db-console sagas is not added in this commit?

Reviewed 25 of 30 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)

---

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

          <Loading
            loading={_.isNil(
              this.props.completeStatementDetails?.statement?.key_data?.query,

would it be sufficient to just check this.props.completeStatementDetails is nil here?

---

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

    if (!this.props.completeStatementDetails?.statement) {
      return null;
    }

Hmm, since renderContent is wrapped in the Loading component, is this check still necessary?

---

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

              />
            </SummaryCard>
          )}

Nice! This is finally gone :D

---

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

// For tenant cases, we don't show information about node, regions and
// diagnostics.
const mapStateToProps = (state: AppState, props: RouteComponentProps) => {

Hmm, what does this change do ?

---

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

  if (
    appNames &&
    (appNames.indexOf("$ internal") >= 0 || appNames.indexOf("unset") >= 0)

nit: appNames.includes() might be clearer than indexOf() >= 0

[maryliag]

The sagas on db-console side are used for actions such as reset, create/cancel diagnostic bundle, etc. I didn't added any new action, so no new sagas were required.

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

---

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

Previously, Azhng (Archer Zhang) wrote…

would it be sufficient to just check this.props.completeStatementDetails is nil here?

Done

---

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

Previously, Azhng (Archer Zhang) wrote…

Hmm, since renderContent is wrapped in the Loading component, is this check still necessary?

Done

---

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

Previously, Azhng (Archer Zhang) wrote…

Nice! This is finally gone :D

😄

---

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

Previously, Azhng (Archer Zhang) wrote…

Hmm, what does this change do ?

I'll use things from location and match, so needed to change to route. Nothing from StatementDetailsProps needed here.

---

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

Previously, Azhng (Archer Zhang) wrote…

nit: appNames.includes() might be clearer than indexOf() >= 0

Done

[jocrl]

🎉

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

---

-- commits, line 5 at r2:
nit: The new endpoint respond is still being saved on the keyed cache, correct? Maybe s/populated by the value saved on cache/populated by the data for the statements table, to not give the impression that the cache part is changing.

Suggestion:

  Previously, the Statement Details page was being populated
  by the value saved on cache,

---

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

export interface StatementDetailsStateProps {
  completeStatementDetails: CompleteStatementDetails;

nit: s/completeStatementDetails/statementDetails? I feel like statementDetails would suffice, in that a single record from the statements table is a statement and your endpoint response is a statementDetails. Unless there is a difference between a completeStatementDetails and a statementDetails?

Code quote:

completeStatementDetails

---

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

    } = this.props.completeStatementDetails.statement.key_data;

    if (Number(stats.count) == 0) {

For my understanding, do you know what was the prior use case for this if block and error message? This block used to be for when this.props.statement is present and stats is absent, but in terms of use cases I don't know when that happens.

I know that previously, if a time range was selected that did not have the statement, this.props.statement` would be absent so we wouldn't even get here.

For adding the time picker it might help to distinguish the two cases to provide appropriate error messages. But I don't currently understand what this block is for 🤔.

---

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx, line 509 at r2 (raw file):

    const columns = makeStatementsColumns(
      statements,
      filters.app.split(","),

nit: I'm realizing that I'm late since the endpoint is already merged, but I feel like a useful future improvement would be to return the app_names as an array. The comma delimitation is very implicit; one generally wouldn't expect to need to split the string, and even then it's unclear what the delimiter is and whether a particular variable with a string type still needs to be split.

---

pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx, line 240 at r2 (raw file):

export function makeStatementFingerprintColumn(
  statType: StatisticType,
  selectedApp: string[],

nit: could these be plural-ized to s/selectedApp/selectedApps?

Code quote:

selectedApp: string[],

[Azhng]

Hmm, I think resetSQLStats's sagas on the DB Console side (probably means cluster-ui side also need update 🤔 ) need to be updated to also invalidate the keyed cache reducer storing the statement details.

Reviewed 2 of 4 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @jocrl, and @maryliag)

---

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx, line 509 at r2 (raw file):

Previously, jocrl (Josephine) wrote…

nit: I'm realizing that I'm late since the endpoint is already merged, but I feel like a useful future improvement would be to return the app_names as an array. The comma delimitation is very implicit; one generally wouldn't expect to need to split the string, and even then it's unclear what the delimiter is and whether a particular variable with a string type still needs to be split.

Hmm I think the API has the app_names returned as an array, this line looks like is coming from the filter.

[maryliag]

Invalidation during resetSQLStats added

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @jocrl)

---

-- commits, line 5 at r2:

Previously, jocrl (Josephine) wrote…

nit: The new endpoint respond is still being saved on the keyed cache, correct? Maybe s/populated by the value saved on cache/populated by the data for the statements table, to not give the impression that the cache part is changing.

Done

---

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

Previously, jocrl (Josephine) wrote…

nit: s/completeStatementDetails/statementDetails? I feel like statementDetails would suffice, in that a single record from the statements table is a statement and your endpoint response is a statementDetails. Unless there is a difference between a completeStatementDetails and a statementDetails?

Done

---

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

Previously, jocrl (Josephine) wrote…

For my understanding, do you know what was the prior use case for this if block and error message? This block used to be for when this.props.statement is present and stats is absent, but in terms of use cases I don't know when that happens.

I know that previously, if a time range was selected that did not have the statement, this.props.statement` would be absent so we wouldn't even get here.

For adding the time picker it might help to distinguish the two cases to provide appropriate error messages. But I don't currently understand what this block is for 🤔.

Imagine you're on the overview page and you see a statement there, then someone else reset the data for example, just when you're about to click on any of the statements to see the details, which means when the details page opens there isn't a statement anymore, so this place is to handle those cases, and we just show the message of we couldn't find the statement anymore.

---

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx, line 509 at r2 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Hmm I think the API has the app_names returned as an array, this line looks like is coming from the filter.

Archer is correct, this line is not about the api, is already existing code that gets the app names from the filter selected. The api uses array :)

---

pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx, line 240 at r2 (raw file):

Previously, jocrl (Josephine) wrote…

nit: could these be plural-ized to s/selectedApp/selectedApps?

Done

[jocrl]

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

---

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

Previously, maryliag (Marylia Gutierrez) wrote…

Imagine you're on the overview page and you see a statement there, then someone else reset the data for example, just when you're about to click on any of the statements to see the details, which means when the details page opens there isn't a statement anymore, so this place is to handle those cases, and we just show the message of we couldn't find the statement anymore.

Hmmm. And that would end up hitting this block, instead of this above block?

    if (!this.props.statement) {
      return null;
    }

I'm trying to test this on master, but reset sql stats (button or CLI) isn't working for me on the table page for some reason.

---

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx, line 509 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

Archer is correct, this line is not about the api, is already existing code that gets the app names from the filter selected. The api uses array :)

Ah, I see, it's this and in the code below. That's still unfortunate then, but understandably not in in the scope of this pr.

[maryliag]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @jocrl)

---

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

Previously, jocrl (Josephine) wrote…

Hmmm. And that would end up hitting this block, instead of this above block?

    if (!this.props.statement) {
      return null;
    }

I'm trying to test this on master, but reset sql stats (button or CLI) isn't working for me on the table page for some reason.

I remove the block you're mentioning.

[jocrl]

---

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

Previously, maryliag (Marylia Gutierrez) wrote…

I remove the block you're mentioning.

Right, I'm aware; my previous comment was referring to old behavior before the changes in this PR. What I'm saying is that there used to be two separate paths, and I had the uncertain impression that the first now-deleted block was what used to be hit in the scenario that you described. And if so, that I don't know when the second block is hit.

Not sure if you know the distinction in prior code behavior?

If not, don't worry and I'll figure this out after fixing reset sql stats.

[maryliag]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @jocrl)

---

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

Previously, jocrl (Josephine) wrote…

Right, I'm aware; my previous comment was referring to old behavior before the changes in this PR. What I'm saying is that there used to be two separate paths, and I had the uncertain impression that the first now-deleted block was what used to be hit in the scenario that you described. And if so, that I don't know when the second block is hit.

Not sure if you know the distinction in prior code behavior?

If not, don't worry and I'll figure this out after fixing reset sql stats.

Previously, we would pass a statement on the props to the details page (statement here means the query itself). From that we would search for the exec stats on the cache.
Since we were always passing the query we could display it on the black box, and search for the exec to show below the box.

If the query itself was empty (I can't imagine the scenario, but I imagine it was added because of some bug), the search for the exec would probably break, so that check was added for "if you don't have the value to search, don't even try, go back". Then the second block is "okay, I have the statement, let's show on the box, but look for exec stats" and if that failed we would show the message that the stats were not found.

The difference now is that we don't pass the query itself anymore, just the fingerprint id on the url, so or we will have the query and the exec stats or nothing.

[jocrl]

---

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

statement on the props to the details page (statement here means the query itself)

The this.props.statement is the statement object, not the query itself. It's then destructured to stats, so that stats is actually this.props.statement. In the old code, the query itself is this.props.statement.statement.

I'm unclear about the use cases in the old code where this.props.statement is null, vs when this.props.statement is present but this.props.statement.stats is not.

[Azhng]

Reviewed 1 of 30 files at r1, 1 of 4 files at r2, 7 of 10 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @jocrl, and @maryliag)

---

pkg/ui/workspaces/cluster-ui/src/store/statementDetails/statementDetails.reducer.ts, line 54 at r3 (raw file):

    },
    invalidated: (state, action: PayloadAction<{ key: string }>) => {
      if (action.payload.key) {

Hmm, I presume that adding the if check here is to handle two cases:

1. invalidate specifically for a key, (where a key is specified)
2. invalidate when reset SQL Stats is clicked, (where the key is not specified

Correct me if I misunderstood this, but it seems like when it's been invalidated in the second case, the state actually doesn't get reset?

[maryliag]

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

---

pkg/ui/workspaces/cluster-ui/src/store/statementDetails/statementDetails.reducer.ts, line 54 at r3 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Hmm, I presume that adding the if check here is to handle two cases:

1. invalidate specifically for a key, (where a key is specified)
2. invalidate when reset SQL Stats is clicked, (where the key is not specified

Correct me if I misunderstood this, but it seems like when it's been invalidated in the second case, the state actually doesn't get reset?

I wasn't able to test the reset of the code since the reset wasn't working, so I didn't wanted to add to this PR until I had tested. I added the if part, just so it wouldn't crash for the invalidate. I added the remaining code now.

[Azhng]

Reviewed 1 of 10 files at r3, 1 of 2 files at r4, 7 of 7 files at r5, 1 of 1 files at r6.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng, @jocrl, and @maryliag)

---

pkg/ui/workspaces/cluster-ui/src/store/statementDetails/statementDetails.reducer.ts, line 67 at r6 (raw file):

            valid: false,
            lastError: null,
          };

Hmm, should we just delete the content of the key here instead e.g. delete state[key] ?

[maryliag]

Working on DB and CC Console 🎉

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

---

pkg/ui/workspaces/cluster-ui/src/store/statementDetails/statementDetails.reducer.ts, line 67 at r6 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Hmm, should we just delete the content of the key here instead e.g. delete state[key] ?

Done

[maryliag]

TFTR!
bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)