[CRDB-5534] ui: hot ranges table page #75073

Santamaura · 2022-01-18T16:08:49Z

This PR adds the hot ranges page in a sortable table format. Values in the range id, nodes, and database columns link to their respective pages within crdb for more info.

Resolves: CC-5804 (duplicate)

Notes:

Tooltip text still needs to be finalized
This branch was rebased off of Andrii's endpoint pr and thus includes his changes to the backend and debug page which can be ignored when reviewing.

cockroach-teamcity · 2022-01-18T16:09:11Z

This change is

dhartunian

I know this is marked as do-not-merge but just a heads up that this should not have a CC link in the title, that's for CC PRs only. All Jira tickets for CRDB should have a corresponding Github issue that you can reference in the PR/commit description like "Resolves #xxxxx".

Additionally, I strongly suggest working on this as a single commit with amended revisions. Maybe squash once you take the do-not-merge tag off and it's ready for review.

Also, make sure this contains a release note once it's revised.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh and @zachlite)

koorosh

Reviewed 6 of 6 files at r1, 5 of 5 files at r2, 3 of 3 files at r3, 1 of 1 files at r4, 5 of 5 files at r5, 5 of 5 files at r6, 8 of 8 files at r7, 1 of 8 files at r8, 1 of 6 files at r12, 1 of 3 files at r14, 1 of 5 files at r15, 2 of 4 files at r16.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Santamaura and @zachlite)

pkg/ui/workspaces/db-console/src/redux/hotRanges/hotRangesReducer.ts, line 32 at r17 (raw file):

};

export default function(state = INITIAL_STATE, action: any): HotRangesState {

In CRDB, there's CachedDataReducer (pkg/ui/workspaces/db-console/src/redux/cachedDataReducer.ts) to manage data that can be fetched periodically. This infrastructure establishes generic way to handle errors/loading in flight states, etc, but it is less flexible to manage. It is also a question to CRUX team (cc @nathanstilwell ) to agree if we should use CachedDataReducer for new features or redux-toolkit or as default reducer as in current implementation?

pkg/ui/workspaces/db-console/src/redux/hotRanges/hotRangesReducer.ts, line 42 at r17 (raw file):

      return {
        loading: false,
        lastUpdate: getCurrentDateTime(),

Reducer should produce new state by exclusively depending on received payload and previous state. Calling getCurrentDateTime() inside reducer should be avoided. Instead, you need to call getCurrentDateTime() in some saga that dispatches an action with payload that contains current date.

Santamaura

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh, @nathanstilwell, and @zachlite)

pkg/ui/workspaces/db-console/src/redux/hotRanges/hotRangesReducer.ts, line 32 at r17 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

In CRDB, there's CachedDataReducer (pkg/ui/workspaces/db-console/src/redux/cachedDataReducer.ts) to manage data that can be fetched periodically. This infrastructure establishes generic way to handle errors/loading in flight states, etc, but it is less flexible to manage. It is also a question to CRUX team (cc @nathanstilwell ) to agree if we should use CachedDataReducer for new features or redux-toolkit or as default reducer as in current implementation?

I am not sure either could use some other opinion on this..

pkg/ui/workspaces/db-console/src/redux/hotRanges/hotRangesReducer.ts, line 42 at r17 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

Reducer should produce new state by exclusively depending on received payload and previous state. Calling getCurrentDateTime() inside reducer should be avoided. Instead, you need to call getCurrentDateTime() in some saga that dispatches an action with payload that contains current date.

Done, it's moved to the saga.

koorosh

Reviewed 1 of 5 files at r18.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh, @nathanstilwell, @Santamaura, and @zachlite)

pkg/ui/workspaces/db-console/src/redux/hotRanges/hotRangesReducer.ts, line 32 at r17 (raw file):

Previously, Santamaura (Alex Santamaura) wrote…

I am not sure either could use some other opinion on this..

I thought about this a bit more and would stick with old reducers implementation (with CachedDataReducer) because:

current implementation doesn't allow to reuse it in CC Console and follows different pattern than other reducers;
current implementation doesn't handle continuous page refresh and state invalidation (that's already implemented in CachedDataReducer);
it allows to avoid additional saga implementations per page.

pkg/ui/workspaces/db-console/src/redux/hotRanges/hotRangesReducer.ts, line 24 at r18 (raw file):

};

export default function(state = INITIAL_STATE, action: any): HotRangesState {

Avoid using any type for action argument. It is possible to use union of actions types (defined in hotRangesActions.ts).

pkg/ui/workspaces/db-console/src/redux/hotRanges/hotRangesSagas.ts, line 45 at r18 (raw file):

export function* hotRangesSaga() {
  yield all([takeEvery(GET_HOT_RANGES, getHotRangesSaga)]);

It might be more convenient to use takeLatest instead of takeEvery as far as we're interested in most recent request.

Santamaura

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh, @nathanstilwell, and @zachlite)

pkg/ui/workspaces/db-console/src/redux/hotRanges/hotRangesReducer.ts, line 32 at r17 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

I thought about this a bit more and would stick with old reducers implementation (with CachedDataReducer) because:

current implementation doesn't allow to reuse it in CC Console and follows different pattern than other reducers;

current implementation doesn't handle continuous page refresh and state invalidation (that's already implemented in CachedDataReducer);

it allows to avoid additional saga implementations per page.

Agreed. I have changed to using CachedDateReducer and it has simplified the redux flow and it will now poll to update the data.

pkg/ui/workspaces/db-console/src/redux/hotRanges/hotRangesReducer.ts, line 24 at r18 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

Avoid using any type for action argument. It is possible to use union of actions types (defined in hotRangesActions.ts).

👍 file has been removed as it is not needed when using CachedDataReducer

pkg/ui/workspaces/db-console/src/redux/hotRanges/hotRangesSagas.ts, line 45 at r18 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

It might be more convenient to use takeLatest instead of takeEvery as far as we're interested in most recent request.

👍 file has been removed.

koorosh

Reviewed 1 of 9 files at r19.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh, @nathanstilwell, @Santamaura, and @zachlite)

pkg/ui/workspaces/db-console/src/views/hotRanges/hotRangesTable.tsx, line 31 at r19 (raw file):

  const [sortSetting, setSortSetting] = useState({
    ascending: true,
    columnTitle: null,

By default, it has to be sorted by QPS column in descending order?

Code quote:

    ascending: true,
    columnTitle: null,

pkg/ui/workspaces/db-console/src/views/hotRanges/hotRangesTable.tsx, line 68 at r19 (raw file):

      ),
      cell: (val: HotRange) => (
        <Link to={`/node/${val.replica_node_ids[0]}`}>

Can you map over replica ids and create multiple links for every replica? I don't think we should navigate to first replica only.

pkg/ui/workspaces/db-console/src/views/hotRanges/hotRangesTable.tsx, line 102 at r19 (raw file):

      ),
      cell: (val: HotRange) => (
        <Link to={`/database/${val.database_name}/table/${val.table_name}`}>

Some ranges can contain empty strings for database, and table names. Can we somehow validate such cases to avoid malformed links?
Also, as discussed in figma (https://www.figma.com/file/vgQDWxqSFzjCJdMBV4Po3u?node-id=2729:9311#137465754) table name should represent fully qualified name (schema name with table name) ie. system.ranges where system is a schema name that is also provided within response. Note, that it's possible that schema can be empty and non empty table name.

pkg/ui/workspaces/db-console/src/views/hotRanges/index.tsx, line 48 at r19 (raw file):

      <Helmet title="Hot Ranges" />
      <h1 className="base-heading">Hot ranges</h1>
      <Text className={cx("hotranges-description")}>The hot ranges table shows ranges receiving a high number of reads or writes. By default the table is sorted by <br /> ranges with the highest QPS (Queries Per Second). Use this information to... <a href="" target="_blank">Learn more</a></Text>

Can you add TODO or/and file another issue to add proper links on documentation so we don't forget about it later?
Prefer to use <Anchor> component (pkg/ui/workspaces/cluster-ui/src/anchor/anchor.tsx) instead of <a> to use proper styles.

Santamaura

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh, @nathanstilwell, and @zachlite)

pkg/ui/workspaces/db-console/src/views/hotRanges/hotRangesTable.tsx, line 31 at r19 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

By default, it has to be sorted by QPS column in descending order?

Correct, done.

pkg/ui/workspaces/db-console/src/views/hotRanges/hotRangesTable.tsx, line 68 at r19 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

Can you map over replica ids and create multiple links for every replica? I don't think we should navigate to first replica only.

Good catch thanks.

pkg/ui/workspaces/db-console/src/views/hotRanges/hotRangesTable.tsx, line 102 at r19 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

Some ranges can contain empty strings for database, and table names. Can we somehow validate such cases to avoid malformed links?
Also, as discussed in figma (https://www.figma.com/file/vgQDWxqSFzjCJdMBV4Po3u?node-id=2729:9311#137465754) table name should represent fully qualified name (schema name with table name) ie. system.ranges where system is a schema name that is also provided within response. Note, that it's possible that schema can be empty and non empty table name.

Good catch on the malformed links. I didn't see that discussion in figma, thanks for pointing it out. Both have been done.

pkg/ui/workspaces/db-console/src/views/hotRanges/index.tsx, line 48 at r19 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

Can you add TODO or/and file another issue to add proper links on documentation so we don't forget about it later?

Prefer to use <Anchor> component (pkg/ui/workspaces/cluster-ui/src/anchor/anchor.tsx) instead of <a> to use proper styles.

Added a TODO and switched to using <Anchor>

Santamaura

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh, @nathanstilwell, and @zachlite)

pkg/ui/workspaces/db-console/src/views/hotRanges/index.tsx, line 48 at r19 (raw file):

Previously, Santamaura (Alex Santamaura) wrote…

Added a TODO and switched to using <Anchor>

Github issue: #75502

koorosh

LGTM! I think it is ready to be merged as soon as #74377 PR is merged

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh, @nathanstilwell, and @zachlite)

Santamaura

Won't some changes need to be made to switch to using server side pagination?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh, @nathanstilwell, and @zachlite)

koorosh

Definitely, you're right!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh, @nathanstilwell, and @zachlite)

dhartunian

Reviewed 1 of 5 files at r2, 1 of 3 files at r3, 3 of 5 files at r6, 6 of 8 files at r7, 1 of 8 files at r8, 1 of 3 files at r14, 1 of 5 files at r18, 2 of 9 files at r19, 2 of 2 files at r20, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh, @nathanstilwell, @Santamaura, and @zachlite)

pkg/ui/workspaces/db-console/src/views/reports/containers/debug/index.tsx, line 421 at r20 (raw file):

            name="All Nodes"
            url="#/debug/hotranges"
            note="#/debug/hotranges"

Some folks might still rely on the older hotrange pages. Are we keeping them around? If so, might be nice to keep the old links as "Hot Ranges (legacy)" perhaps.

pkg/ui/workspaces/db-console/src/views/reports/containers/hotranges/index.tsx, line 36 at r20 (raw file):

    });
  }, [nodeId]);
  // eslint-disable-next-line react-hooks/exhaustive-deps

nit: can you add a comment explaining why this is necessary?

pkg/ui/workspaces/db-console/src/views/hotRanges/index.tsx, line 43 at r20 (raw file):

    );
  };
  // TODO: Alex S add url to anchor once it's available

nit: please use // TODO(santamaura) format for TODOs (you can use your github or slack username in there but just something that clearly identifies. Unfortunately, we have multiple Alex S devs already).

pkg/ui/workspaces/db-console/src/views/hotRanges/index.tsx, line 50 at r20 (raw file):

      <Text className={cx("hotranges-description")}>
        The hot ranges table shows ranges receiving a high number of reads or writes. By default the table is sorted by 
        <br />

nit: I see trailing spaces here. Does eslint not take care of this?

Santamaura

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @koorosh, @nathanstilwell, and @zachlite)

pkg/ui/workspaces/db-console/src/views/reports/containers/debug/index.tsx, line 421 at r20 (raw file):

Previously, dhartunian (David Hartunian) wrote…

Some folks might still rely on the older hotrange pages. Are we keeping them around? If so, might be nice to keep the old links as "Hot Ranges (legacy)" perhaps.

Sorry David, these changes are a part of #74377 which this PR is rebased on. cc @koorosh

pkg/ui/workspaces/db-console/src/views/reports/containers/hotranges/index.tsx, line 36 at r20 (raw file):

Previously, dhartunian (David Hartunian) wrote…

nit: can you add a comment explaining why this is necessary?

Same as above comment cc @koorosh

pkg/ui/workspaces/db-console/src/views/hotRanges/index.tsx, line 43 at r20 (raw file):

Previously, dhartunian (David Hartunian) wrote…

nit: please use // TODO(santamaura) format for TODOs (you can use your github or slack username in there but just something that clearly identifies. Unfortunately, we have multiple Alex S devs already).

Makes sense, done.

pkg/ui/workspaces/db-console/src/views/hotRanges/index.tsx, line 50 at r20 (raw file):

Previously, dhartunian (David Hartunian) wrote…

nit: I see trailing spaces here. Does eslint not take care of this?

Ah odd eslint wasn't on when I committed this, should be ok now.

koorosh

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @koorosh, @nathanstilwell, @Santamaura, and @zachlite)

pkg/ui/workspaces/db-console/src/views/reports/containers/debug/index.tsx, line 421 at r20 (raw file):

Previously, Santamaura (Alex Santamaura) wrote…

Sorry David, these changes are a part of #74377 which this PR is rebased on. cc @koorosh

Agree, we can keep both pages for now. Will update in original PR.

pkg/ui/workspaces/db-console/src/views/reports/containers/hotranges/index.tsx, line 36 at r20 (raw file):

Previously, Santamaura (Alex Santamaura) wrote…

Same as above comment cc @koorosh

Ack! Will update in original PR.

This change implements second version of hot ranges api that's required for UI to represent enhanced Hot Ranges page. Before, Hot ranges (under Advanced debugging page) used former version of HotRanges api that provided information about hot ranges with internal/ sensitive data (see: cockroachdb#53212) that should not be exposed to users. Now, in addition to existing endpoint, additional one is implemented that is based on a former version and provides hot ranges information that is only needed for Hot Ranges page (range id, qps, table, db, and index names for particular range). The list of hot ranges and their QPS is provided by `HotRanges` service and then information like DB, table and index names are retrieved from range's `StartKey` that might include this info or not (in case if it's meta range, or range that stores index itself for instance). `HotRange` and `HotRangeV2` services expect the same request type as an argument but return different responses. `HotRangeV2` service returns a flat list of hot ranges instead of grouped ranges per node/store. Release note: None

Current change extends `statuspb.HotRangesResponse` to include `LeaseholderNodeID` field to indicate the node id that contains leaseholder replica for current hot range. This change was made in `localHotRanges` function (that is used by `HotRanges` that in turn used by `HotRangeV2` service) to reuse existing logic of iteration over the stores and querying hot ranges. It extends its response by `LeaseholderNodeID` field. Otherwise, the same logic should be implemented in `HotRangeV2` service by calling `VisitStores` iterator. Release note: None

Before, `listHotRanges` request handler of `apiV2Server` relied on `HotRanges` service that is now should be replaced by new `HotRangesV2` implementation. Current change reuses HotRangeV2 service in `api/v2/ranges/hot` api. It allows to share the same logic between REST and gRPC endpoints and gradually migrate to new version of API. Release note: None

Before, different hot ranges info was kept in different maps and with this change, `hotRangeReportMeta` struct is defined to group hot range data for every range. Release note: None

…o messages Provide more detailed descriptions on implemented functions and proto messages related to Hot Ranges functionality. Release note: None

Before, hot ranges response included `table_name` field without specific information about schema. Current change adds `schema_name` field (in case it is user's table), and returns empty string for system tables that don't provide information about schema. Release note: None

Initially, `HotRangesV2` service in status server was defined to use GET method to handle HTTP request. It was convenient way to display response data in Advanced debugging page since it allowed to render response body right on to page. But now, hot ranges will be used on user facing page and request dispatching for hot ranges api should follow generic workflow: initialize `HotRangesRequest` protobuf message and dispatch request with `src/util/api` service. This restriction forces to use POST method (since GET method doesn't allow to provide request body). In addition, Hot Ranges debugging page is refactored to use `api` service. Release note: None

This PR adds in the hot ranges page in a table format. Release note(ui change): add hot ranges page Release note (<category, see below>): <what> <show> <why>

After connecting to the new hot ranges endpoint there were a few finishing changes to make the ui smooth. Release note (ui change): polish hot ranges table implementation

Release note (<category, see below>): <what> <show> <why>

koorosh · 2022-03-03T15:43:44Z

This PR can be closed in favor of #77330

77330: ui: add hot ranges page in Db Console r=koorosh a=koorosh This change adds a new page in Db Console - Hot Ranges that display list of hot ranges in the cluster and some useful information related to each range. The main goal of this page is to help eliminate root cause of hot ranges. This page uses already implemented hot ranges API (V2) that provides all necessary data to show. Release note (ui change): add Hot Ranges page and link to it on the sidebar Release justification: bug fixes and low-risk updates to new functionality This PR is mostly based on #75073 with additional changes to apply server side pagination. Resolves: #40536 <img width="1398" alt="Screen Shot 2022-03-03 at 17 19 31" src="https://user-images.githubusercontent.com/3106437/156594563-32a50245-0572-4b0c-b7cd-368e6ba1910c.png"> Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net> Co-authored-by: Andrii Vorobiov <and.vorobiov@gmail.com>

koorosh · 2022-03-07T13:26:36Z

@Santamaura , can you close this PR because these changes have been already added in PR #77330

Santamaura force-pushed the hot-ranges-page branch 2 times, most recently from 5a42d76 to 1a56d2e Compare January 18, 2022 22:15

Santamaura changed the title ~~WIP: Hot ranges page~~ [CC-5804] ui: hot ranges table page Jan 19, 2022

Santamaura added the do-not-merge bors won't merge a PR with this label. label Jan 19, 2022

Santamaura marked this pull request as ready for review January 19, 2022 20:53

Santamaura requested a review from a team as a code owner January 19, 2022 20:53

Santamaura requested review from koorosh and zachlite January 19, 2022 20:53

dhartunian reviewed Jan 20, 2022

View reviewed changes

Santamaura changed the title ~~[CC-5804] ui: hot ranges table page~~ [CRDB-5534] ui: hot ranges table page Jan 20, 2022

koorosh requested changes Jan 21, 2022

View reviewed changes

Santamaura commented Jan 21, 2022

View reviewed changes

koorosh requested changes Jan 24, 2022

View reviewed changes

Santamaura commented Jan 24, 2022

View reviewed changes

koorosh requested changes Jan 24, 2022

View reviewed changes

Santamaura commented Jan 25, 2022

View reviewed changes

koorosh reviewed Jan 26, 2022

View reviewed changes

Santamaura commented Jan 26, 2022

View reviewed changes

koorosh reviewed Jan 27, 2022

View reviewed changes

dhartunian reviewed Jan 31, 2022

View reviewed changes

Santamaura commented Jan 31, 2022

View reviewed changes

koorosh reviewed Feb 1, 2022

View reviewed changes

koorosh mentioned this pull request Feb 16, 2022

ui: use paginated hot ranges data #76707

Closed

koorosh added 5 commits February 23, 2022 11:40

server: nit. use struct to group hot ranges data

b6748ad

Before, different hot ranges info was kept in different maps and with this change, `hotRangeReportMeta` struct is defined to group hot range data for every range. Release note: None

server: nit. add details comments for hot range V2 functions and prot…

6312414

…o messages Provide more detailed descriptions on implemented functions and proto messages related to Hot Ranges functionality. Release note: None

koorosh and others added 16 commits February 23, 2022 11:40

wip setup basic redux flow to old hot ranges rpc

7b4a042

triaging babel compile failure

086aab3

complete hot ranges table presentational component

5d71072

update link paths for hot ranges

e71804a

move hot range files to match new dir structure

f3330e0

styling for table and datetime info

5451808

ui: add hot ranges page

8a12c40

This PR adds in the hot ranges page in a table format. Release note(ui change): add hot ranges page Release note (<category, see below>): <what> <show> <why>

update component + redux flow to be compatible with new endpoint

7888e85

ui: polish hot ranges table implementation

5b758f3

After connecting to the new hot ranges endpoint there were a few finishing changes to make the ui smooth. Release note (ui change): polish hot ranges table implementation

change qps to show 2 decimals + minor css fix

0c6b0bb

move update time to saga + tooltips

7b809d8

update hotranges redux flow to use cacheddatareducer

50c99e6

address Andrii CR 2

ed2bd8d

update TODO to use github and linting

767257c

Release note (<category, see below>): <what> <show> <why>

Santamaura force-pushed the hot-ranges-page branch from 90229e6 to 767257c Compare February 23, 2022 17:08

koorosh mentioned this pull request Mar 3, 2022

ui: add hot ranges page in Db Console #77330

Merged

Santamaura closed this Mar 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CRDB-5534] ui: hot ranges table page #75073

[CRDB-5534] ui: hot ranges table page #75073

Santamaura commented Jan 18, 2022 •

edited

Loading

cockroach-teamcity commented Jan 18, 2022

dhartunian left a comment

koorosh left a comment

Santamaura left a comment

koorosh left a comment

Santamaura left a comment

koorosh left a comment

Santamaura left a comment

Santamaura left a comment

koorosh left a comment

Santamaura left a comment

koorosh left a comment

dhartunian left a comment

Santamaura left a comment

koorosh left a comment

koorosh commented Mar 3, 2022

koorosh commented Mar 7, 2022

[CRDB-5534] ui: hot ranges table page #75073

[CRDB-5534] ui: hot ranges table page #75073

Conversation

Santamaura commented Jan 18, 2022 • edited Loading

cockroach-teamcity commented Jan 18, 2022

dhartunian left a comment

Choose a reason for hiding this comment

koorosh left a comment

Choose a reason for hiding this comment

Santamaura left a comment

Choose a reason for hiding this comment

koorosh left a comment

Choose a reason for hiding this comment

Santamaura left a comment

Choose a reason for hiding this comment

koorosh left a comment

Choose a reason for hiding this comment

Santamaura left a comment

Choose a reason for hiding this comment

Santamaura left a comment

Choose a reason for hiding this comment

koorosh left a comment

Choose a reason for hiding this comment

Santamaura left a comment

Choose a reason for hiding this comment

koorosh left a comment

Choose a reason for hiding this comment

dhartunian left a comment

Choose a reason for hiding this comment

Santamaura left a comment

Choose a reason for hiding this comment

koorosh left a comment

Choose a reason for hiding this comment

koorosh commented Mar 3, 2022

koorosh commented Mar 7, 2022

Santamaura commented Jan 18, 2022 •

edited

Loading