-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ui: Add column selector and filter to sessions page #75965
Conversation
@kevin-v-ngo A few things in the sessions details page can't be implemented until we fix #67888. This includes details for the "most recent transaction" and "most recent statement" sections since we don't currently store information about non-active queries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 11 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @gtr, @kevin-v-ngo, @maryliag, and @xinhaoz)
pkg/ui/workspaces/cluster-ui/src/icon/circleFilled.tsx, line 21 at r1 (raw file):
const { fill, className } = props; return ( <svg height={10} width={20} className={className} {...props}>
I think the height and width and fill can all be specified in the class definition (e.g., sessionPage.module.scss
file) and then passed to the component via the className
.
I'm doing it like this in a .module.scss
file:
.bool-setting-icon {
&__enabled {
fill: $colors--success;
margin-right: 8px;
height: 8px;
width: 8px;
}
&__disabled {
fill: $colors--disabled;
margin-right: 8px;
height: 8px;
width: 8px;
}
}
and then in the parent component:
<CircleFilled className={cx("bool-setting-icon__enabled")} />
Thanks. What's the "last_active_query" column in crdb_internal.cluster_sessions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the screenshot both column selector and filter are not in the right location, with the correct spacing, etc
Reviewed 1 of 11 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @gtr, @kevin-v-ngo, and @xinhaoz)
pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.tsx, line 328 at r1 (raw file):
const appNames = getSessionAppFilterOptions(sessionsData); const nodes = isTenant
we are not really filtering anything based on nodes, so all mentions of nodes and tenants can be removed
pkg/ui/workspaces/cluster-ui/src/sessions/sessionsTableContent.tsx, line 26 at r1 (raw file):
content={"The timestamp at which the session started."} > Session Start Time (UTC)
since you created the label on sessionsColumnLabels
you can use that here, so is one source of truth
Code quote (from pkg/ui/workspaces/cluster-ui/src/sessions/sessionsTable.tsx):
sessionsColumnLabels
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the "last_active_query" column in crdb_internal.cluster_sessions? I thought that was the most recent query which we can add to the UI.
From the screenshot, the spacing seems a bit off. Adding @Annebirzin for another set of eyes.
a21e182
to
6b69326
Compare
done, good catches! 👍🏽 |
It is! But it only actually stores the string of the query, not any other information such as the start time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the images looks like your filter count is showing how many results there are and not how many filters where selected. The value should be how many filters where selected
Reviewed 1 of 11 files at r1, 2 of 9 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @gtr, @maryliag, and @xinhaoz)
-- commits, line 8 at r2:
nit: add which issue this is fixing
Thanks @gtr! A couple of notes and questions: Session Overview
Session Detail
|
It should be consistent with statement - "Most Recent Transaction". @gtr, any reason why we don't have the SQL for the most recent transaction? For active Sessions, we should have it correct? I'm not sure for idle Sessions. |
53f7902
to
6767522
Compare
The way the sessions details page is currently written, the |
Thank you! Done 👍🏽 |
a7a0ecf
to
29071a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is looking great!
One thing: the session start time
and actions
columns should not be "selectable", meaning, they should always be displayed. We have a flag for alwaysShow that you can use on that column (e.g.
alwaysShow: true, |
Reviewed 2 of 9 files at r2, 1 of 4 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @gtr, @maryliag, and @xinhaoz)
pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.tsx, line 233 at r4 (raw file):
timeUnit: filters.timeUnit, regions: filters.regions, nodes: filters.nodes,
nit: you don't need regions and nodes here
pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.tsx, line 256 at r4 (raw file):
timeUnit: undefined, regions: undefined, nodes: undefined,
you don't need regions and nodes here
pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.tsx, line 275 at r4 (raw file):
} const activeFilters = calculateActiveFilters(filters); const internalAppNamePrefix = "$ internal";
this value should come from the props, we don't want to set here, in case it changes
pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPageConnected.tsx, line 124 at r4 (raw file):
action: "Terminate Statement", }), onFilterChange: (value: Filters) => {
make sure you're also testing this on CC Console
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 266 at r4 (raw file):
} > Statements Run
nit: use the getLabel here too
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 277 at r4 (raw file):
content={"The duration of the open transaction, if there is one."} > Transaction Duration
nit: use the getLabel here too
pkg/ui/workspaces/db-console/src/views/sessions/sessionsPage.tsx, line 30 at r4 (raw file):
terminateSessionAction, } from "src/redux/sessions/sessionsSagas"; import { nodeRegionsByIDSelector } from "oss/src/redux/nodes";
nit: you don't need nodes on this page
pkg/ui/workspaces/db-console/src/views/sessions/sessionsPage.tsx, line 73 at r4 (raw file):
columns: sessionColumnsLocalSetting.selectorToArray(state), filters: filtersLocalSetting.selector(state), nodeRegions: nodeRegionsByIDSelector(state),
nit: you don't need nodes on this page
pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPageConnected.stories.tsx, line 28 at r4 (raw file):
Store, } from "redux"; import { SessionsPageConnected } from "./sessionsPageConnected";
why this removal? Don't we want to test this page?
pkg/ui/workspaces/cluster-ui/src/sessions/sessionPage.module.scss, line 20 at r4 (raw file):
.sessions-filter { font-size: 14px;
use the variable $font-size--medium
instead of the direct value here
pkg/ui/workspaces/cluster-ui/src/sessions/sessionPage.module.scss, line 21 at r4 (raw file):
.sessions-filter { font-size: 14px; margin-bottom: 10px;
use $spacing-smaller
instead
pkg/ui/workspaces/cluster-ui/src/sessions/sessionPage.module.scss, line 25 at r4 (raw file):
.session-column-selector { margin-bottom: 10px;
use $spacing-smaller
9a8520a
to
d5ed064
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 9 files at r2, 1 of 4 files at r3, 7 of 9 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @gtr, @maryliag, and @xinhaoz)
Previously, maryliag (Marylia Gutierrez) wrote…
nit: add which issue this is fixing
you added to the PR description, but not here :)
263b2f2
to
84f7236
Compare
Done! :)
Apologies for the delay; was waiting for #76307 to merge. Done! |
Updated the screenshots to reflect:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Nits look good to me; I'll leave the stamp to Marylia 🙂
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @gtr, @maryliag, and @xinhaoz)
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 117 at r4 (raw file):
Previously, gtr (Gerardo Torres Castro) wrote…
// StatisticType is used to modify the content of the tooltip. export const statisticsTableTitles: StatisticTableTitleType = { sessionStart: () => {
nit: Please verb-ify these functions. I'd also suggest naming them to indicate that they are returning components, e.g.
s/sessionStart/getSessionStartComponent
Code quote:
sessionStart
Done! :)
this is looking great! One thing: the
session start time
andactions
columns should not be "selectable", meaning, they should always be displayed. We have a flag for alwaysShow that you can use on that column (e.g.
alwaysShow: true, )
Reviewed 2 of 9 files at r2, 1 of 4 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @gtr, @maryliag, and @xinhaoz)
Not sure why the comment reply formatting is weird, but just following up that we chatted offline about not changing sessionStart
because of the type definition on the key of StatisticTableTitleType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to make sure this is working on CC Console before I can give the approval :)
Reviewed 5 of 5 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @gtr, @maryliag, and @xinhaoz)
6432877
to
9f665d0
Compare
Video of the sessions page tested on CC Console: sessions-page-cc.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
There is just one check that I would have like to see on the video: select a filter, change page, come back, the filter must be selected
(no need to create another video, just make sure that this behaviour works as expected :D )
Reviewed 1 of 11 files at r1, 2 of 2 files at r7, 3 of 3 files at r8, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @gtr, @maryliag, and @xinhaoz)
…ession overview page Closes [cockroachdb#73463](https://cockroachlabs.atlassian.net/browse/CRDB-11600) Previously, the sessions page was missing the column selector and filter components that the stmts and txns page have. This change adds both components in addition to new columns in the sessions overview page and edits to the sessions details page. Release note (ui change): added column selector, filters, new columns to the sessions page and sessions details.
9f665d0
to
4923ebb
Compare
bors r+ |
Already running a review |
bors r+ |
Build failed: |
bors retry |
Build succeeded: |
Closes #73463
Previously, the sessions page was missing the column selector and filter
components that the stmts and txns page have. This change adds both
components in addition to new columns in the sessions overview page and
edits to the sessions details page.
Release note (ui change): added column selector, filters, new columns to
the sessions page and sessions details.
Screenshots:
Sessions overview page:
Sessions overview page with column selector:
Sessions overview page with results from column selector:
Sessions overview page with filter:
Sessions overview page with results from filter:
Sessions detail page: