ui/server: add ability to create conditional statement diagnostics

[lindseyjin]

Resolves #57634

Previously, we did not have the ability to create conditional statement
diagnostics in the frontend. This commit adds in the ability to specify
a minimum execution latency and an expiry time when creating a statement
diagnostics request. These changes apply to both DB and CC console.

Since expired requests are not surfaced at all in the frontend, we have
also modified the statement diagnostics API response to not return
already expired and incomplete requests.

Lastly, this commit also deletes some unused code related to statement
diagnostics modals.

Release note (ui change): Add ability to create conditional statement
diagnostics by adding two new fields 1) minimum execution latency, which
specifies the limit for when a statement should be tracked and 2) expiry
time, which specifies when a diagnostics request should expire.

[cockroach-teamcity]

This change is 

[lindseyjin]

CC Console:
https://www.loom.com/share/32056d2af2f4429a805bd07b1dbf11f6

DB Console:
https://www.loom.com/share/c053b2ae5bec42f58610aac2ecf40bc2

@Annebirzin Let me know if the styling looks good to you!
I think there's a few minor details that are off eg) I'm missing the alert left blue border, because I couldn't find a way to override the alert styling on both CC and DB console. Hope that's still ok, but let me know if you think otherwise!

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/server/statement_diagnostics_requests.go, line 184 at r1 (raw file):

				req.ExpiresAt = expiresAt.Time
				// Don't return already expired requests.
				if req.ExpiresAt.Before(time.Now()) {

I decided to modify the backend API response, since we're currently not surfacing expired requests at all in the frontend.

If we have a use for them in the future (e.g. showing cancelled/expired requests), we can probably include them back in the response and add a selector in the frontend instead.

---

pkg/ui/workspaces/cluster-ui/src/statementsDiagnostics/activateStatementDiagnosticsModal.tsx, line 167 at r1 (raw file):

                    </Select>
                  </div>
                  <Divider type="horizontal" style={{ marginBottom: 0 }} />

Inline styling, since including a class on the antd divider won't override default styling

---

pkg/ui/workspaces/cluster-ui/src/statementsDiagnostics/activateStatementDiagnosticsModal.scss, line 4 at r1 (raw file):

.modal-body {
  .ant-modal-body{

CC console has some global styling rules that overrides certain styling rules, but that doesn't exist on DB console. Not sure if it's good practice to override antd class names like this, but don't think there's a better way around this?

[lindseyjin]

Modal with updated description text:

Reviewable status: complete! 0 of 0 LGTMs obtained

[maryliag]

Reviewed 19 of 24 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lindseyjin and @maryliag)

---

-- commits, line 15 at r3:
nit: add the issue number

---

pkg/ui/workspaces/cluster-ui/src/store/statementDiagnostics/statementDiagnostics.sagas.spec.ts, line 33 at r3 (raw file):

describe("statementsDiagnostics sagas", () => {
  describe("createDiagnosticsReportSaga", () => {

can you also add new tests checking if after the expire date the request is no longer there?

[jocrl]

Woo! Commented a few things; the main one is to avoid using refs and imperative patterns in react. Happy to chat if it would help 🙂

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

---

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

  ActivateStatementDiagnosticsModal,
} from "../statementsDiagnostics";
type IDuration = google.protobuf.IDuration;

I'm kinda suspicious about the introduction and use of this google.protobuf.IDuration type here. It seems like it wasn't present in the codebase prior to this PR, and I feel like the codebase surely has encountered some sort of this same "duration" type before.

I see in status.proto that min_execution_latency has the type google.protobuf.Duration. Searching for that type in .proto files, another field with this type is aggregation_interval, which in the front endaggregationInterval uses type number.

Would that work for this?

---

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

  StatementDetailsState
> {
  activateDiagnosticsRef: React.RefObject<ActivateDiagnosticsModalRef>;

In the other comment I mention that I think using a ref here is a bad idea. But if you were to keep the ref, the name here of the interface ActivateDiagnosticsModalRef is kinda weird. As your property name activateDiagnosticsRef implies, the entire thing of type React.RefObject<ActivateDiagnosticsModalRef>; is a ref.
However, the point of the RefObject<T> generic typing is that activateDiagnosticsRef is a ref of an object with the type T. That is, T itself is not a ref, it is what the ref is pointing to.

I might suggest the name ActivateDiagnosticsModal, but that's already used for the component 😛. Maybe ActivateDiagnosticsModalElement, in keeping with the HTMLElement type name ((code example)[https://stackoverflow.com/questions/61102101/cannot-assign-refobjecthtmldivelement-to-refobjecthtmlelement-instance]).

Code quote:

ActivateDiagnosticsModalRef

---

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx, line 346 at r2 (raw file):
@lindseyjin, is there a particular reason you're "connecting/linking" the <ActivateStatementDiagnosticsModal/> and <DiagnosticsView/> components with a ref, instead of local state on <StatementDetails/>?

I'm curious what other people think here, but I'm inclined to say that the modal should be opened not by calling showModalFor on the ref, but by passing as props to <DiagnosticsView/> a function that sets local state on <StatementDetails/> for whether or not the modal is visible. In <ActivateStatementDiagnosticsModal/>, instead of the useImperativeHandle that sets state, you would have a useEffect.

Refs in general are an anti-pattern in React, and in general there should be a deliberate reason of solving something you cannot otherwise do if you're going to use one. See these docs, and to quote:

Your first inclination may be to use refs to “make things happen” in your app. If this is the case, take a moment and think more critically about where state should be owned in the component hierarchy. Often, it becomes clear that the proper place to “own” that state is at a higher level in the hierarchy. See the Lifting State Up guide for examples of this.

In general with react, you want data and props to just "flow", instead of "telling" another component to do something. It's telling that the docs for a ref start with warnings to not use it 😉

In this case it seems like only these two components are concerned with whether the modal is open, so local state is a good idea. If more components were concerned, that might be a reason to hold that state in redux.

---

pkg/ui/workspaces/cluster-ui/src/statementsDiagnostics/activateStatementDiagnosticsModal.scss, line 4 at r1 (raw file):

Previously, lindseyjin (Lindsey Jin) wrote…

CC console has some global styling rules that overrides certain styling rules, but that doesn't exist on DB console. Not sure if it's good practice to override antd class names like this, but don't think there's a better way around this?

Yeah, if I'm understanding that ant-modal-body is a class from the 3rd party library, I do agree this is kinda weird 😛. I'm assuming the problem is that just applying modal-body doesn't take precedence over the padding specified in the library on modal-body?

I see in your other comment that you apply an inline style to the ant Divider. Could you do something similar, and instead of passing className, pass inline styles to style the modal? Since presumably other use cases that want to style the modal would also run into this problem.

[maryliag]

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

---

pkg/ui/workspaces/cluster-ui/src/statementsDiagnostics/activateStatementDiagnosticsModal.scss, line 4 at r1 (raw file):

Previously, jocrl (Josephine) wrote…

Yeah, if I'm understanding that ant-modal-body is a class from the 3rd party library, I do agree this is kinda weird 😛. I'm assuming the problem is that just applying modal-body doesn't take precedence over the padding specified in the library on modal-body?

I see in your other comment that you apply an inline style to the ant Divider. Could you do something similar, and instead of passing className, pass inline styles to style the modal? Since presumably other use cases that want to style the modal would also run into this problem.

I prefer not using inline styles because is hard to keep track and modify all at once. We do have some other instances where we change the ant styles, like here:

cockroach/pkg/ui/workspaces/db-console/src/views/app/components/modal/styles.styl

Line 19 in b832320

.ant-modal-close-x

so I think is okay to override the rules here

[jocrl]

Also, just wanted to add a congratulations on the number of moving parts for this feature 😁!

And I can't speak for anyone else's expectations, but given that you're also leaving tomorrow on my end I would 10/10 understand if you left things for Thomas to deal with 😉. Which would also be a good learning experience for him! I think the first section of https://pl.reactjs.org/docs/refs-and-the-dom.html (up to and including "Don’t Overuse Refs") is worth really reading and understanding, if you plan to write more react in the future.

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

[maryliag]

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

---

pkg/server/statement_diagnostics_requests.go, line 184 at r1 (raw file):

Previously, lindseyjin (Lindsey Jin) wrote…

I decided to modify the backend API response, since we're currently not surfacing expired requests at all in the frontend.

If we have a use for them in the future (e.g. showing cancelled/expired requests), we can probably include them back in the response and add a selector in the frontend instead.

Make sense to update the code here 😄
Just one thing, is that you will get a lint error using time here (error: forbidden; use 'timeutil' instead)

[jocrl]

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

---

pkg/ui/workspaces/cluster-ui/src/statementsDiagnostics/activateStatementDiagnosticsModal.scss, line 4 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

I prefer not using inline styles because is hard to keep track and modify all at once. We do have some other instances where we change the ant styles, like here:

cockroach/pkg/ui/workspaces/db-console/src/views/app/components/modal/styles.styl

Line 19 in b832320

.ant-modal-close-x

so I think is okay to override the rules here

👍

[jocrl]

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

---

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx, line 346 at r2 (raw file):
Actually, one edit to my suggestion:

Above, I suggested

instead of the useImperativeHandle that sets state, you would have a useEffect.

I think instead of a useEffect, you want to lift the visible state out of <ActivateStatementDiagnosticsModal/>, and hold it as a single source of truth in <DiagnosticsView/>, as opposed to duplicating the visible state in both components. So you would just pass setVisible as props to <ActivateStatementDiagnosticsModal/>.

The companion docs to "don't use refs" is lifting state up (which, the docs sadly are still using class components). I think this docs page is also pretty core react that's worth taking time to understand 🙂.

[Annebirzin]

@lindseyjin Looking great! A couple of updates:

• I chatted with @kevin-v-ngo today about making it so the blue alert message does not show if the first collection method is selected and the expiration is deselected (only shows when the 2nd collection method is selected) - I've added that state to Figma here for reference.
• In the Loom videos it looked like the blue used in the controls and links was off. Can we use our core blue (#0055FF) for radio buttons, check boxes and links? Also the background box for the alert message blue looked a little off (the stroke and the fill)

Let me know any questions

[jocrl]

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

---

pkg/ui/workspaces/cluster-ui/src/statementsDiagnostics/activateStatementDiagnosticsModal.tsx, line 49 at r3 (raw file):

    ref: React.RefObject<ActivateDiagnosticsModalRef>,
  ) => {
    const [visible, setVisible] = useState(false);

This state is the one that I think would be a good candidate to lift up, now that the parent component also needs to use it 🙂

---

pkg/ui/workspaces/cluster-ui/src/statementsDiagnostics/activateStatementDiagnosticsModal.tsx, line 109 at r3 (raw file):

    return (
      <Modal
        visible={visible}

Here it looks like you are passing visible to some reusable Modal component, that presumably will follow whatever value gets passed for this?

[jocrl]

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

---

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

Previously, jocrl (Josephine) wrote…

In the other comment I mention that I think using a ref here is a bad idea. But if you were to keep the ref, the name here of the interface ActivateDiagnosticsModalRef is kinda weird. As your property name activateDiagnosticsRef implies, the entire thing of type React.RefObject<ActivateDiagnosticsModalRef>; is a ref.
However, the point of the RefObject<T> generic typing is that activateDiagnosticsRef is a ref of an object with the type T. That is, T itself is not a ref, it is what the ref is pointing to.

I might suggest the name ActivateDiagnosticsModal, but that's already used for the component 😛. Maybe ActivateDiagnosticsModalElement, in keeping with the HTMLElement type name ((code example)[https://stackoverflow.com/questions/61102101/cannot-assign-refobjecthtmldivelement-to-refobjecthtmlelement-instance]).

Retracting since there is another usage that uses the same naming.

---

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

Previously, jocrl (Josephine) wrote…

Actually, one edit to my suggestion:

Above, I suggested

instead of the useImperativeHandle that sets state, you would have a useEffect.

I think instead of a useEffect, you want to lift the visible state out of <ActivateStatementDiagnosticsModal/>, and hold it as a single source of truth in <DiagnosticsView/>, as opposed to duplicating the visible state in both components. So you would just pass setVisible as props to <ActivateStatementDiagnosticsModal/>.

The companion docs to "don't use refs" is lifting state up (which, the docs sadly are still using class components). I think this docs page is also pretty core react that's worth taking time to understand 🙂.

Lindsey brought up that this imperative pattern already existed and was not introduced in this PR. She made this issue #74221 to refactor out the refs.

[lindseyjin]

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

---

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

Previously, jocrl (Josephine) wrote…

I'm kinda suspicious about the introduction and use of this google.protobuf.IDuration type here. It seems like it wasn't present in the codebase prior to this PR, and I feel like the codebase surely has encountered some sort of this same "duration" type before.

I see in status.proto that min_execution_latency has the type google.protobuf.Duration. Searching for that type in .proto files, another field with this type is aggregation_interval, which in the front endaggregationInterval uses type number.

Would that work for this?

I know that for aggregationInterval, it's passed in as a Duration but we do convert it to a number later on.

However, minExecLatency and expiresAfter need to be converted from numbers to Durations to be passed to the backend. I could do this at a point closer to the API request and pass around numbers for the most part, if you think that would be cleaner? However, not sure if there's a huge difference since we'll have to convert it at some point anyways.

---

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

Previously, jocrl (Josephine) wrote…

Lindsey brought up that this imperative pattern already existed and was not introduced in this PR. She made this issue #74221 to refactor out the refs.

Thanks for linking the issue!

---

pkg/ui/workspaces/cluster-ui/src/statementsDiagnostics/activateStatementDiagnosticsModal.tsx, line 49 at r3 (raw file):

Previously, jocrl (Josephine) wrote…

This state is the one that I think would be a good candidate to lift up, now that the parent component also needs to use it 🙂

hi! wondering if this comment still applies, or if this is part of the followup issue to refactor out refs?

if so, how do I lift up the state? 🤔

---

pkg/ui/workspaces/cluster-ui/src/statementsDiagnostics/activateStatementDiagnosticsModal.tsx, line 109 at r3 (raw file):

Previously, jocrl (Josephine) wrote…

Here it looks like you are passing visible to some reusable Modal component, that presumably will follow whatever value gets passed for this?

From what I understand, we call the showModalFor function on the ref from other components in order to set visible to true, which allows the modal to show up

---

pkg/ui/workspaces/cluster-ui/src/store/statementDiagnostics/statementDiagnostics.sagas.spec.ts, line 33 at r3 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

can you also add new tests checking if after the expire date the request is no longer there?

Actually, there isn't any logic in the frontend right now to check if reports are expired, since I've only modified the backend API response to not return expired reports.

I've created a quick test in the backend to check that expired reports shouldn't be returned in the API response instead!

[lindseyjin]

Updated the alert to only show when the second collection method is selected without an expiry date

I've also updated the radio buttons to be the core blue colour. Otherwise, I checked the code and the links/checkbox are already core blue (might've just rendered strangely on my monitor?) 🤔

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

[maryliag]

Can you just update the images to show the new versions after the changes requested from Anne?

Reviewed 2 of 24 files at r1, 2 of 4 files at r4, 3 of 3 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jocrl and @maryliag)

---

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

Previously, lindseyjin (Lindsey Jin) wrote…

I know that for aggregationInterval, it's passed in as a Duration but we do convert it to a number later on.

However, minExecLatency and expiresAfter need to be converted from numbers to Durations to be passed to the backend. I could do this at a point closer to the API request and pass around numbers for the most part, if you think that would be cleaner? However, not sure if there's a huge difference since we'll have to convert it at some point anyways.

I was able to find some usage of IDuration on the codebase, so you can keep like this :)

---

pkg/ui/workspaces/cluster-ui/src/statementsDiagnostics/activateStatementDiagnosticsModal.tsx, line 49 at r3 (raw file):

Previously, lindseyjin (Lindsey Jin) wrote…

hi! wondering if this comment still applies, or if this is part of the followup issue to refactor out refs?

if so, how do I lift up the state? 🤔

This is not related to the refactor of refs, but I think we can keep it like this, and work on improvements when working on the refs refactor

[lindseyjin]

New Loom recording with the new changes:
https://www.loom.com/share/afb17abb48a2400aa9cdd69fc8363552

Screenshot:

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

---

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

Previously, maryliag (Marylia Gutierrez) wrote…

I was able to find some usage of IDuration on the codebase, so you can keep like this :)

Got it, thanks!

[lindseyjin]

Updated again with slightly different text: s/duration thresholds/latency threshold

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

[lindseyjin]

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

---

pkg/ui/workspaces/cluster-ui/src/util/convert.ts, line 14 at r7 (raw file):

import * as protos from "@cockroachlabs/crdb-protobuf-client";
import { fromNumber } from "long";

Workaround because there are some files in db_console that call functions in this file, and importing Long directly introduces errors in the pipeline since the Long versions are incompatible

[maryliag]

great work! 🎉

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jocrl and @THardy98)

[lindseyjin]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)