Yay! 🙌

A few nits, but consider this an if you want to merge first and shelve any nits for later.

nit: Whether in this PR or next time, could you modify the statement details story to show multiple plans? So that the next dev doesn't need to figure out how to cause two different plans to make UI changes.

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

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

    } = this.props;
    const { currentTab } = this.state;
    const { statements_per_plan_hash } = this.props.statementDetails;

nit: I'm guessing this decision was made in the endpoint PR, but would it make more sense as s/statements_per_plan_hash/plan_hashes_per_statement?
The current name sounds like there are multiple statements for one plan hash. But as I'm understanding this, it's that there are multiple plan hashes for one statement fingerprint (or am I misunderstanding?).

Code quote:

statements_per_plan_hash

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

export function PlanDetails({ plans }: PlanDetailsProps): React.ReactElement {
  const [plan, setPlan] = useState(null);

nit: s/useState/useState<PlanHashStats|null> would be helpful typing

Code quote:

useState

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

  return [
    {
      name: "planID",

nit/side thought: I think this is interesting in that there have been a lot of PRs with tables and column identifiers and column names, and a question of how to restrict typing on those.

I'm assuming that these string names need to match type PlanDetailsTableColumnKeys. But there is no current type enforcement of that.

The suggestion I gave in Matthew and Xin Hao's PR was to use string enums; essentially that PlanDetailsTableColumnKey would be an enum type and one would do PlanDetailsTableColumnKey.planID here.

But this PR also presents a way of enforcing types on column configurations that is different from the strategy in Matthew and Xin Hao's recent PRs. So perhaps it would be helpful to step back and figure out we want to do.

Code quote:

ColumnDescriptor

Added two plans to stories

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

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

My idea here was more of a "statement statistics per plan hash", where this is a list with objects where the key is the hash and the value are the stats

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

Done

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

I'm assuming that these string names need to match type PlanDetailsTableColumnKeys
they don't need to match, they can be anything, we just use the same name to make it easier to identify

not sure how different it is, this strategy used here is the same used for the statements table columns (the one difference from that one is that the statements depends on a variable because the same table can exist on statement and transaction details page, so we need to know what to show on the tooltip, but that doesn't happen here)

Nice.

Reviewed 4 of 6 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @Annebirzin, @jocrl, and @maryliag)

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

  avg_exec_time: "Average Execution Time",
  exec_count: "Execution Count",
  avg_rows_read: "Average Rows Read",

nit: feels a bit that we are mixing snake case and camel case here. should we stick with camel case here?

Great, thanks!

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

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

Ooooo. I get it now 🙂.
For the future, maybe statements_stats_per_plan_hash? Or just plan_hash_stats, since it's a list and not a dictionary.

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

Right... The databases page does the equivalent of s/"planID"/PlanDetailsTableColumnKey.planID (see this comment, and the diff between r1 and r2).

It's a minor difference, but could be helpful with stricter typing since we have this pattern a lot. Unless you feel particularly inspired to do things now, I'm just thinking of this as food for thought.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @Annebirzin)

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

Done

LGTM!

I did have one minor piece of feedback that's not a blocker. Across all detail pages, can we decrease the fixed height of all SQL text boxes to around 200px (not sure what the current height is), and then increase the fixed height of the Explain plan text box to around 450 px (since explain plans tend to be longer)?