ui: add manual refresh option to the Active Executions in SQL Activity #103786

gtr · 2023-05-23T15:13:18Z

Previously, the Active Executions views in the SQL Activity pages only
only supported automatic refresh at a rate of every 10 seconds. This
is a papercut which may lead to some queries disappearing before a user
is able to investigate it. This commit adds adds support for manual
refresh in the Active Executions page as well as a toggle to switch
between automatic and manual refresh.

A summary of changes:

A new component RefreshControl which contains an auto-refresh
toggle, a manual refresh button, and a timestamp indicating when the
last refresh was performed..
ActiveStatementsView and ActiveTransactionsView make use of this
component to control when their data gets refreshed. The toggle
defaults to ON for both pages.
Analytics for the auto refresh toggle and the manual refresh button.
A inline alert if the data hasn't been refreshed in over 10 minutes.

Loom (DB): https://www.loom.com/share/5bdab5a62a0c4bc694c9c5d7f936759a
Loom (CC): https://www.loom.com/share/de399300225540059bc7c4f156bc8270

Release note (ui change): the active executions views in the SQL
Activity pages now support toggling between automatic and manual
refresh. A manual refresh button is also added along with a timestamp
indicating when the last refresh was performed.

cockroach-teamcity · 2023-05-23T15:13:29Z

This change is

gtr · 2023-05-26T23:31:09Z

Note on the progress of this PR: Currently, the behavior of the manual / automatic refresh toggle is so that the state of the toggle is shared across the two views. See this loom demo for an example. However, recent discussions have led us to opt for a non-shared toggle state between the active statements view and the active transactions view. This means that a user can set the active statements view to be auto-refreshing while the active transactions can be set to manually-refreshing, and vice versa. This came about when we noticed that when a user triggered a manual refresh in the active statements view, the data in the active transactions view would also get updated, which is misleading as each view has its own toggle option. Based on this, we decided that we should not share the toggle state between the two views.

This is currently not possible because the data for active statements and active transactions are both served by the same endpoint, ListSessions, and are updated at the same time. To accomplish this, we will need to create two new endpoints: ListActiveStatements and ListActiveTransactions so that each view can have different fetching behavior (manual, automatic).

Alternatively, we can also keep using the ListSessions endpoint and update the Redux store selectively based on which view we want to update. I would prefer the first option of two new endpoints as it gives us more fine-tuned control over what kind of data we want to fetch. Either way, we need some backend and Redux changes before this PR is ready to be merged.

CC: @dongniwang, @kevin-v-ngo

gtr · 2023-05-31T21:49:23Z

In the spirit of moving fast, we decided to keep the toggle sharing behavior between the active statements and active transactions pages. See loom demo.

xinhaoz · 2023-06-01T20:23:22Z

I think you included the file logg by accident

gtr · 2023-06-01T21:05:33Z

I think you included the file logg by accident

😅 oops, removed it

xinhaoz

Nice! Just some comments on the last refresh timestamp.

xinhaoz · 2023-06-01T21:12:56Z

pkg/ui/workspaces/cluster-ui/src/icon/refreshIcon.tsx

+
+interface IconProps {
+  className?: string;
+  onClick?: () => void;


Looks like you can remove this since you put onClick into the container div instead (which IMO is preferable over putting it on the svg component).

xinhaoz · 2023-06-01T21:13:57Z

pkg/ui/workspaces/cluster-ui/src/statementsPage/activeStatementsPage.selectors.ts

+  onAutoRefreshToggle: (isEnabled: boolean) => {
+    dispatch(
+      localStorageActions.update({
+        key: "isAutoRefreshEnabled/ActiveExecutions",


Can you make this key a const in the localStorage file?

xinhaoz · 2023-06-01T21:20:29Z

pkg/ui/workspaces/db-console/src/views/statements/activeStatementsSelectors.tsx

+
+// lastRefreshTimestampLocalSetting is shared between the Active Statements and
+// Active Transactions components.
+const lastRefreshTimestampLocalSetting = new LocalSetting<AdminUIState, string>(


You don't need to create a new value for this. I think you can pull this timestamp from the sessions redux state (last updated) so that they can see the latest time the info was returned.

gtr

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

pkg/ui/workspaces/cluster-ui/src/icon/refreshIcon.tsx line 15 at r3 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Looks like you can remove this since you put onClick into the container div instead (which IMO is preferable over putting it on the svg component).

Done.

pkg/ui/workspaces/cluster-ui/src/statementsPage/activeStatementsPage.selectors.ts line 99 at r3 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Can you make this key a const in the localStorage file?

Done.

pkg/ui/workspaces/db-console/src/views/statements/activeStatementsSelectors.tsx line 65 at r3 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

You don't need to create a new value for this. I think you can pull this timestamp from the sessions redux state (last updated) so that they can see the latest time the info was returned.

Makes sense! Removed it as a local setting and used the lastUpdated from the sessions redux state.

maryliag

Reviewed 3 of 13 files at r2, 50 of 50 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dongniwang, @gtr, and @xinhaoz)

-- commits line 11 at r4:
nit: clarify this is under SQL Activity page

pkg/ui/workspaces/cluster-ui/src/activeExecutions/refreshControl/refreshControl.tsx line 38 at r4 (raw file):

}

// RefreshButton consists of the RefreshIcon and the text "Refresh"

nit: period

pkg/ui/workspaces/cluster-ui/src/activeExecutions/refreshControl/refreshControl.tsx line 58 at r4 (raw file):

        <span>Active {capitalize(execType)} Executions As Of: </span>
        {lastRefreshTimestamp && lastRefreshTimestamp.isValid() ? (
          <Timestamp time={lastRefreshTimestamp} format={DATE_FORMAT_24_TZ} />

because the refresh is every 10s I think is better to show the seconds here too

pkg/ui/workspaces/cluster-ui/src/activeExecutions/refreshControl/refreshControl.tsx line 66 at r4 (raw file):

      <span className={cx("refresh-divider")}>
        <span className={cx("auto-refresh-label")}>Auto Refresh: </span>
        <Switch

can you change the style here to use the same tone of blue we use elsewhere?

pkg/ui/workspaces/cluster-ui/src/activeExecutions/refreshControl/refreshControl.module.scss line 24 at r4 (raw file):

.refresh-text {
  color: #0055FF;

you should use the variables we have for colors, in this case $colors--primary-blue-3

pkg/ui/workspaces/cluster-ui/src/activeExecutions/refreshControl/refreshControl.module.scss line 35 at r4 (raw file):

.refresh-divider {
  border-left: 1px solid #c0c6d9;

same thing here, use $colors--neutral-4

gtr

Added analytics in activeStatementsPage.selectors.ts and activeTransactionsPage.selectors.tsx!

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

-- commits line 11 at r4:

Previously, maryliag (Marylia Gutierrez) wrote…

nit: clarify this is under SQL Activity page

Done.

pkg/ui/workspaces/cluster-ui/src/activeExecutions/refreshControl/refreshControl.tsx line 58 at r4 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

because the refresh is every 10s I think is better to show the seconds here too

Makes sense! Done.

pkg/ui/workspaces/cluster-ui/src/activeExecutions/refreshControl/refreshControl.tsx line 66 at r4 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

can you change the style here to use the same tone of blue we use elsewhere?

Done.

pkg/ui/workspaces/cluster-ui/src/activeExecutions/refreshControl/refreshControl.module.scss line 24 at r4 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

you should use the variables we have for colors, in this case $colors--primary-blue-3

Done.

pkg/ui/workspaces/cluster-ui/src/activeExecutions/refreshControl/refreshControl.module.scss line 35 at r4 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

same thing here, use $colors--neutral-4

Done.

maryliag

Reviewed 13 of 13 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dongniwang, @gtr, and @xinhaoz)

pkg/ui/workspaces/cluster-ui/src/activeExecutions/refreshControl/refreshControl.tsx line 70 at r5 (raw file):

        <span className={cx("auto-refresh-label")}>Auto Refresh: </span>
        <Switch
          className={cx(`ant-switch-${isAutoRefreshEnabled ? "checked" : ""}`)}

can you add a pic to show how it looks like with the new color?

pkg/ui/workspaces/cluster-ui/src/statementsPage/activeStatementsPage.selectors.ts line 122 at r5 (raw file):

      analyticsActions.track({
        name: "Manual Refresh",
        page: "Statements",

nice! thanks for adding those!

gtr

Here's a demo of the two new feedback items I got on Tuesday, which were to default the toggle as ON and to add a warning banner after x minutes. For now I set it to 10 minutes but I'm open to changing this.

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

pkg/ui/workspaces/cluster-ui/src/activeExecutions/refreshControl/refreshControl.tsx line 70 at r5 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

can you add a pic to show how it looks like with the new color?

Sure! The new demo I linked above should have this changes but it may be hard to see from the demo so here's an additional screenshot:

pkg/ui/workspaces/cluster-ui/src/statementsPage/activeStatementsView.tsx line 152 at r6 (raw file):

    checkTimeDifference();
    const intervalId = setInterval(checkTimeDifference, 10 * 1000);

I'm checking the time difference described in the above comment every 10 seconds. This was arbitrary and I was wondering if there's a preferred interval for this kind of thing.

maryliag

a few ui nits:

looks like the warning is cut off a little on the top part
the "refresh" and toggle are not quite aligned with the text, they both should be a little more up to centralize

Reviewed 2 of 8 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dongniwang, @gtr, and @xinhaoz)

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

  DB_DETAILS_VIEW_MODE = "viewMode/DatabasesDetailsPage",
  ACTIVE_EXECUTIONS_IS_AUTOREFRESH_ENABLED = "isAutoRefreshEnabled/ActiveExecutions",
  ACTIVE_EXECUTIONS_DISPLAY_REFRESH_ALERT = "displayRefreshAlert/ActiveExecutions",

you don't need a value on localstorage for this, because the page already have all the info you need to know if it should be displayed or not.

pkg/ui/workspaces/cluster-ui/src/statementsPage/activeStatementsView.tsx line 152 at r6 (raw file):

Previously, gtr (Gerardo Torres Castro) wrote…

I'm checking the time difference described in the above comment every 10 seconds. This was arbitrary and I was wondering if there's a preferred interval for this kind of thing.

I think 10 seconds is good

maryliag

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

pkg/ui/workspaces/cluster-ui/src/transactionsPage/activeTransactionsView.tsx line 288 at r7 (raw file):

        </PageConfigItem>
      </PageConfig>
      {displayRefreshAlert && (

this one here too, you don't need a a value on local storage for it, you can just compare if has been 10min since last refresh (a data you already have here)

dongniwang · 2023-06-21T15:27:31Z

Thanks @gtr for the PR, looks good to me!

Re

in the spirit of moving fast, we decided to keep the toggle sharing behavior between the active statements and active transactions pages.
Do we have a plan for implementing the not sharing behavior? From a UI perspective, we can also look into moving the control above the tabs so it's more clear that the refresh behavior is shared between statements and transactions.

The other thing is, can you share a screenshot of the inline statement you mentioned? Thank you!

A inline alert if the data hasn't been refreshed in over 10 minutes.

gtr

looks like the warning is cut off a little on the top part

Fixed.

the "refresh" and toggle are not quite aligned with the text, they both should be a little more up to centralize

Also fixed, I had previously had an extra vertical-align: middle which caused that issue.

Do we have a plan for implementing the not sharing behavior? From a UI perspective, we can also look into moving the control above the tabs so it's more clear that the refresh behavior is shared between statements and transactions.

Switching to a non-shared behavior would require creating a new endpoint for each txns and stmts because they are currently refreshed by the same endpoint. This is feasible. From a UI perspective, we definitely could move the control above the tabs but that could also disrupt Statement Fingerprints view since that view doesn't do any auto refresh.

The other thing is, can you share a screenshot of the inline statement you mentioned? Thank you!

Sure thing! it's in the new CC loom demo but here is an additional screenshot too:

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

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

Previously, maryliag (Marylia Gutierrez) wrote…

you don't need a value on localstorage for this, because the page already have all the info you need to know if it should be displayed or not.

Makes sense, removed thanks!

pkg/ui/workspaces/cluster-ui/src/statementsPage/activeStatementsPage.selectors.ts line 122 at r5 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nice! thanks for adding those!

Done.

pkg/ui/workspaces/cluster-ui/src/transactionsPage/activeTransactionsView.tsx line 288 at r7 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

this one here too, you don't need a a value on local storage for it, you can just compare if has been 10min since last refresh (a data you already have here)

Done.

maryliag

Reviewed 11 of 11 files at r8, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dongniwang, @gtr, and @xinhaoz)

pkg/ui/workspaces/cluster-ui/src/store/localStorage/localStorage.reducer.ts line 157 at r8 (raw file):

const defaultIsAutoRefreshEnabledSetting = true;
const defaultDisplayRefreshAlertSetting = false;

this is not being used anywhere

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.module.scss line 63 at r8 (raw file):

  padding-top: 5px;
  display: flex;
  // flex: 0 0 auto;

remove the comment line

Part of: #100663. Previously, the Active Executions views in the SQL Activity pages only only supported automatic refresh at a rate of every 10 seconds. This is a papercut which may lead to some queries disappearing before a user is able to investigate it. This commit adds adds support for manual refresh in the Active Executions page as well as a toggle to switch between automatic and manual refresh. A summary of changes: - A new component `RefreshControl` which contains an auto-refresh toggle, a manual refresh button, and a timestamp indicating when the last refresh was performed.. - `ActiveStatementsView` and `ActiveTransactionsView` make use of this component to control when their data gets refreshed. The toggle defaults to ON for both pages. - Analytics for the auto refresh toggle and the manual refresh button. - A inline alert if the data hasn't been refreshed in over 10 minutes. Release note (ui change): the active executions views in the SQL Activity pages now support toggling between automatic and manual refresh. A manual refresh button is also added along with a timestamp indicating when the last refresh was performed.

gtr

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

pkg/ui/workspaces/cluster-ui/src/store/localStorage/localStorage.reducer.ts line 157 at r8 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

this is not being used anywhere

Done.

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.module.scss line 63 at r8 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

remove the comment line

Done.

maryliag

Nice work!

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

gtr · 2023-07-06T11:37:09Z

TFTR!
bors r+

craig · 2023-07-06T12:13:53Z

Build succeeded:

Bazel Essential CI (Cockroach)

gtr force-pushed the manual-refresh branch from 797d0da to 607eb98 Compare May 26, 2023 23:23

gtr force-pushed the manual-refresh branch from 607eb98 to 52a0421 Compare May 31, 2023 21:47

gtr force-pushed the manual-refresh branch from 52a0421 to b6c8a88 Compare June 1, 2023 14:14

gtr marked this pull request as ready for review June 1, 2023 14:25

gtr requested review from a team June 1, 2023 14:25

gtr changed the title ~~[WIP] ui: add manual refresh option to the Active Executions in SQL Activity~~ ui: add manual refresh option to the Active Executions in SQL Activity Jun 1, 2023

gtr force-pushed the manual-refresh branch from b6c8a88 to 6cccef9 Compare June 1, 2023 21:04

xinhaoz reviewed Jun 1, 2023

View reviewed changes

gtr force-pushed the manual-refresh branch from 6cccef9 to 583952a Compare June 2, 2023 21:13

gtr commented Jun 2, 2023

View reviewed changes

maryliag requested a review from dongniwang June 2, 2023 21:31

maryliag reviewed Jun 2, 2023

View reviewed changes

gtr force-pushed the manual-refresh branch 2 times, most recently from fa9dcd0 to f734b3e Compare June 6, 2023 01:11

gtr commented Jun 6, 2023

View reviewed changes

maryliag reviewed Jun 7, 2023

View reviewed changes

gtr force-pushed the manual-refresh branch from f734b3e to bd78636 Compare June 8, 2023 22:31

gtr commented Jun 8, 2023

View reviewed changes

gtr force-pushed the manual-refresh branch from bd78636 to 2c297f1 Compare June 8, 2023 22:47

maryliag reviewed Jun 20, 2023

View reviewed changes

gtr force-pushed the manual-refresh branch from 2c297f1 to 3485019 Compare June 28, 2023 01:10

gtr commented Jun 28, 2023

View reviewed changes

maryliag reviewed Jul 4, 2023

View reviewed changes

gtr force-pushed the manual-refresh branch from 3485019 to fde54d4 Compare July 5, 2023 12:54

gtr commented Jul 5, 2023

View reviewed changes

maryliag approved these changes Jul 5, 2023

View reviewed changes

craig bot merged commit e38486c into master Jul 6, 2023

craig bot deleted the manual-refresh branch July 6, 2023 12:13

cockroach-teamcity mentioned this pull request Jul 7, 2023

PR #103786 - ui: add manual refresh option to the Active Executions in SQL Activity cockroachdb/docs#17415

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: add manual refresh option to the Active Executions in SQL Activity #103786

ui: add manual refresh option to the Active Executions in SQL Activity #103786

gtr commented May 23, 2023 •

edited

Loading

cockroach-teamcity commented May 23, 2023

gtr commented May 26, 2023 •

edited

Loading

gtr commented May 31, 2023

xinhaoz commented Jun 1, 2023

gtr commented Jun 1, 2023

xinhaoz left a comment

xinhaoz Jun 1, 2023

xinhaoz Jun 1, 2023

xinhaoz Jun 1, 2023

gtr left a comment

maryliag left a comment

gtr left a comment

maryliag left a comment

gtr left a comment

maryliag left a comment

maryliag left a comment

dongniwang commented Jun 21, 2023

gtr left a comment •

edited

Loading

maryliag left a comment

gtr left a comment

maryliag left a comment

gtr commented Jul 6, 2023

craig bot commented Jul 6, 2023

ui: add manual refresh option to the Active Executions in SQL Activity #103786

ui: add manual refresh option to the Active Executions in SQL Activity #103786

Conversation

gtr commented May 23, 2023 • edited Loading

cockroach-teamcity commented May 23, 2023

gtr commented May 26, 2023 • edited Loading

gtr commented May 31, 2023

xinhaoz commented Jun 1, 2023

gtr commented Jun 1, 2023

xinhaoz left a comment

Choose a reason for hiding this comment

xinhaoz Jun 1, 2023

Choose a reason for hiding this comment

xinhaoz Jun 1, 2023

Choose a reason for hiding this comment

xinhaoz Jun 1, 2023

Choose a reason for hiding this comment

gtr left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

gtr left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

gtr left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

dongniwang commented Jun 21, 2023

gtr left a comment • edited Loading

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

gtr left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

gtr commented Jul 6, 2023

craig bot commented Jul 6, 2023

gtr commented May 23, 2023 •

edited

Loading

gtr commented May 26, 2023 •

edited

Loading

gtr left a comment •

edited

Loading