ui: add proper checks and message for insights

[maryliag]

Previously, no checks were done on insights when user was not admin, making the page show empty results, even when there was an error retrieving the data.
This commit passes on the proper error message when there is one. It also checks for admin privilege to show option to apply a recommendation.

https://www.loom.com/share/33d002cc9b704a4fad2f9851109b04ee

Epic: CRDB-20388

Release note (ui change): Hide apply option for index recommendation when user is not admin.

[cockroach-teamcity]

This change is 

[j82w]

Reviewed 27 of 28 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

---

pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 233 at r2 (raw file):

    );
  if (contentionResults.error) {
    throw new Error(contentionResults.error.message);

Should we add some additional text to the error message to help identify which query failed?

Code snippet:

throw new Error("Get contention results failed: " + contentionResults.error.message);

---

pkg/ui/workspaces/cluster-ui/src/api/sqlApi.ts line 127 at r2 (raw file):

 * removing information not useful for the user.
 * e.g. "$executing stmt 1: run-query-via-api: only users with either MODIFYCLUSTERSETTING
 * or VIEWCLUSTERSETTING privileges are allowed to show cluster settings"

Should there be a link to more info?

---

pkg/ui/workspaces/cluster-ui/src/insights/insightsErrorComponent.tsx line 21 at r2 (raw file):

  const message = errMsg
    ? sqlApiErrorMessage(errMsg)
    : "This page had an unexpected error while loading insights.";

If the issue is max result size exceeded should the reload button be removed? Reloading won't help in that scenario.

[ericharmeling]

Nice!

Reviewed 27 of 28 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

[gtr]

LGTM! Just a few questions.

Reviewed 23 of 28 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

---

pkg/ui/workspaces/cluster-ui/src/insights/types.ts line 44 at r2 (raw file):

};

export type TxnContentionInsightEventResult = {

Why was this type added? I don't see it used or imported anywhere; is it for a future change?

---

pkg/ui/workspaces/cluster-ui/src/insights/schemaInsights/schemaInsights.fixture.ts line 73 at r2 (raw file):

    schemaInsightType: "",
  },
  hasAdminRole: true,

Should there be a fixture where hasAdminRole is false?

[maryliag]

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

---

pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 233 at r2 (raw file):

Previously, j82w (Jake) wrote…

Should we add some additional text to the error message to help identify which query failed?

Done

---

pkg/ui/workspaces/cluster-ui/src/api/sqlApi.ts line 127 at r2 (raw file):

Previously, j82w (Jake) wrote…

Should there be a link to more info?

It would be hard to decide what link to show, because this could be any message.

---

pkg/ui/workspaces/cluster-ui/src/insights/insightsErrorComponent.tsx line 21 at r2 (raw file):

Previously, j82w (Jake) wrote…

If the issue is max result size exceeded should the reload button be removed? Reloading won't help in that scenario.

Good point. I made changes to hide on that case

---

pkg/ui/workspaces/cluster-ui/src/insights/types.ts line 44 at r2 (raw file):

Previously, gtr (Gerardo Torres Castro) wrote…

Why was this type added? I don't see it used or imported anywhere; is it for a future change?

I was trying something different on one of my iterations and forgot to delete this.
Nice catch!

---

pkg/ui/workspaces/cluster-ui/src/insights/schemaInsights/schemaInsights.fixture.ts line 73 at r2 (raw file):

Previously, gtr (Gerardo Torres Castro) wrote…

Should there be a fixture where hasAdminRole is false?

Currently we're not using this for testing (yet). I added so the lint would pass, but when we create proper tests for this area we should add variations indeed

[maryliag]

TFTR!
bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 0bdc210 to blathers/backport-release-22.2-95516: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}