Skip to content
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 a sessions page #51903

Merged
merged 4 commits into from
Sep 4, 2020
Merged

ui: add a sessions page #51903

merged 4 commits into from
Sep 4, 2020

Conversation

jordanlewis
Copy link
Member

@jordanlewis jordanlewis commented Jul 27, 2020

This PR adds two new pages:

  1. Active Sessions, a table of all active sessions in the database.
  2. Session Details, a page with information about a single active
    session.

In addition, active sessions and queries are now terminatable from the
Active Sessions page.

Release note (admin ui change): add the session list and session detail
pages, and permit session and query termination from the UI.

Release justification: low risk, high benefit change to admin ui

Closes #52193.

Screenshots:

image

image

image

@jordanlewis jordanlewis requested a review from a team July 27, 2020 04:18
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@awoods187
Copy link
Contributor

This is amazing! I'm really excited we are tracking this information in the WebUI now.

@jordanlewis
Copy link
Member Author

Here's a short video of the page in action: https://youtu.be/xPAtTGchPfo

@jordanlewis
Copy link
Member Author

Actions menu:

image

Details page (the button location and colors need help, will need the CSS 🚁 later :)

image

image

@jordanlewis jordanlewis changed the title [dnm] ui: add a sessions page ui: add a sessions page Aug 5, 2020
@jordanlewis jordanlewis force-pushed the sessions-page branch 6 times, most recently from a71067e to ebd7f8f Compare August 10, 2020 13:53
@jordanlewis
Copy link
Member Author

Okay, this is ready for a review!

@nathanstilwell nathanstilwell self-requested a review August 10, 2020 16:53
Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this up @jordanlewis. My comments are mostly small nits. Nothing blocking except maybe the question re: pagination.

Reviewed 4 of 4 files at r1, 11 of 11 files at r2, 25 of 27 files at r3.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @jordanlewis, @nathanstilwell, and @solongordon)


pkg/ui/src/redux/sessions/sessionsSagas.tsx, line 76 at r3 (raw file):

}

export function* terminateQuerySaga(action: PayloadAction<ICancelQueryRequest>) {

I still don't fully "get" how redux-saga works but I do like how this code reads! Really clear what's intended.


pkg/ui/src/views/sessions/sessionsPage.tsx, line 62 at r3 (raw file):

        ascending: false,
      },
      pagination: {

I'm unable to glean from the code where the pagination is being applied to the data in the table...am I missing something?


pkg/ui/src/views/sessions/sessionsPage.tsx, line 160 at r3 (raw file):

              label: "Learn more",
              // TODO(jordan): point this to a more appropriate page.
              buttonHref: "https://www.cockroachlabs.com/docs/stable/show-sessions.html",

See pkg/ui/src/util/docs.ts for docs link examples. We can generate the base of the docs URL there.


pkg/ui/src/views/sessions/terminateSessionModal.tsx, line 26 at r3 (raw file):

// tslint:disable-next-line:variable-name
const TerminateSessionModal = (props: TerminateSessionModalProps, ref: React.RefObject<TerminateSessionModalRef>) => {

This whole pattern is a bit rough to read second time around. Thanks for lifting it from the diagnostics modal for now. I can't help but feel like there's a simplification that's possible here

cc @nathanstilwell, the diagnostics modal stuff was built quite quickly so I'm sure the modal pattern it implements could use a second look. Feels weird to be using these "refs" but maybe it's a valid pattern. Not sure how CC does it.


pkg/ui/src/views/statements/statementsTableContent.tsx, line 228 at r3 (raw file):

// StatementLinkTarget returns the link to the relevant statement page, given
// the input statement details.
export const StatementLinkTarget = (props: StatementLinkProps) => {

Thanks for improving this component!

Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @nathanstilwell, and @solongordon)


pkg/ui/src/views/sessions/sessionDetails.tsx, line 133 at r4 (raw file):

          <div>
            <Link className={cx("back-link")} to={ "/sessions" }>
              Back to Statements

should say Sessions here?

Copy link
Member Author

@jordanlewis jordanlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @jordanlewis, @nathanstilwell, and @solongordon)


pkg/ui/src/redux/sessions/sessionsSagas.tsx, line 76 at r3 (raw file):

Previously, dhartunian (David Hartunian) wrote…

I still don't fully "get" how redux-saga works but I do like how this code reads! Really clear what's intended.

You and me both!


pkg/ui/src/views/sessions/sessionDetails.tsx, line 133 at r4 (raw file):

Previously, dhartunian (David Hartunian) wrote…

should say Sessions here?

Nice catch, fixed.


pkg/ui/src/views/sessions/sessionsPage.tsx, line 62 at r3 (raw file):

Previously, dhartunian (David Hartunian) wrote…

I'm unable to glean from the code where the pagination is being applied to the data in the table...am I missing something?

No, I just failed at cargo culting :) Thank you for noticing, I added the remaining plumbing.


pkg/ui/src/views/sessions/sessionsPage.tsx, line 160 at r3 (raw file):

Previously, dhartunian (David Hartunian) wrote…

See pkg/ui/src/util/docs.ts for docs link examples. We can generate the base of the docs URL there.

Done.


pkg/ui/src/views/sessions/terminateSessionModal.tsx, line 26 at r3 (raw file):

Previously, dhartunian (David Hartunian) wrote…

This whole pattern is a bit rough to read second time around. Thanks for lifting it from the diagnostics modal for now. I can't help but feel like there's a simplification that's possible here

cc @nathanstilwell, the diagnostics modal stuff was built quite quickly so I'm sure the modal pattern it implements could use a second look. Feels weird to be using these "refs" but maybe it's a valid pattern. Not sure how CC does it.

Yes, these refs are definitely interesting. I thought this is how one manages state. See the way I implemented the per-row actions popovers for a more interesting example - it definitely feels like a pattern, but an odd one.

@jordanlewis jordanlewis force-pushed the sessions-page branch 3 times, most recently from ad77404 to a114c49 Compare August 14, 2020 18:34
@jordanlewis jordanlewis force-pushed the sessions-page branch 3 times, most recently from bed017a to 43106e0 Compare August 31, 2020 17:59
@jordanlewis
Copy link
Member Author

Latest revision uses UUID format for the links, includes session ID in the session page, freezes the session page from updates, and fixes the remote debugging issues.

@yuzefovich
Copy link
Member

yuzefovich commented Sep 1, 2020

Just a drive-by comment (based on the dilemma Jordan posed in his stream): re: should the webpage of a session reload itself when that session exits or should the webpage be frozen until the user initiates the reload? Not sure if it was considered, but maybe a good alternative would be to add a pop-up warning that says something like "the session has finished, click reload" to reload the page and have a button or something. (Sorry If the sessions page already does something like this.)

Never mind, this was suggested later in the video.

pkg/ui/src/views/sessions/sessionsTable.tsx Outdated Show resolved Hide resolved
pkg/ui/src/views/sessions/sessionsTable.tsx Outdated Show resolved Hide resolved
pkg/ui/src/views/sessions/sessionDetails.tsx Outdated Show resolved Hide resolved
pkg/ui/src/views/sessions/sessionDetails.tsx Outdated Show resolved Hide resolved
@jordanlewis
Copy link
Member Author

Done.

Copy link
Contributor

@solongordon solongordon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@solongordon solongordon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, I don't want this to block merging but it seems unideal that if you don't have CANCELQUERY permissions, trying to cancel a query or a session gives you a generic error when you click the link. Would be nicer to either not show the link or explain that you don't have the right privileges.

Same goes for if a non-admin tries to cancel an admin query/session.

I'd say let's just open an issue to track it.

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

Add some new information to the transaction, session and query
protobufs, and to the transactions internal tables.

Release note (sql change): add 3 new columns to the
crdb_internal.node_transactions and cluster_transactions tables:
num_stmts, num_retries, and num_auto_retries. These keep track of the
total number of statements executed on the transaction, the number of
retries encountered, and the number of automatic retries encountered.
@jordanlewis
Copy link
Member Author

Thanks! That piece of feedback is already in the acceptance testing doc as it turns out: https://docs.google.com/document/d/1obahPF5wH_GbScPFtA3VUz-UjgqR7IRMCKRuT0JFen8/edit# I'll merge this now and make issues next.

bors r+

craig bot pushed a commit that referenced this pull request Sep 4, 2020
51903: ui: add a sessions page r=jordanlewis a=jordanlewis

This PR adds two new pages:

1. Active Sessions, a table of all active sessions in the database.
2. Session Details, a page with information about a single active
   session.

In addition, active sessions and queries are now terminatable from the
Active Sessions page.

Release note (admin ui change): add the session list and session detail
pages, and permit session and query termination from the UI.

Closes #52193.

Screenshots:

![image](https://user-images.githubusercontent.com/43821/88502972-35a4da00-cf9e-11ea-953d-b3ffff7b272e.png)

![image](https://user-images.githubusercontent.com/43821/88502982-3d647e80-cf9e-11ea-8801-d41461a635eb.png)

![image](https://user-images.githubusercontent.com/43821/88502986-42293280-cf9e-11ea-91b7-2435c129f158.png)


Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
@craig
Copy link
Contributor

craig bot commented Sep 4, 2020

Build failed:

@jordanlewis
Copy link
Member Author

bors r+

@jordanlewis
Copy link
Member Author

bors r-

@craig
Copy link
Contributor

craig bot commented Sep 4, 2020

Canceled.

@jordanlewis
Copy link
Member Author

bors r+

craig bot pushed a commit that referenced this pull request Sep 4, 2020
51903: ui: add a sessions page r=jordanlewis a=jordanlewis

This PR adds two new pages:

1. Active Sessions, a table of all active sessions in the database.
2. Session Details, a page with information about a single active
   session.

In addition, active sessions and queries are now terminatable from the
Active Sessions page.

Release note (admin ui change): add the session list and session detail
pages, and permit session and query termination from the UI.

Release justification: low risk, high benefit change to admin ui

Closes #52193.

Screenshots:

![image](https://user-images.githubusercontent.com/43821/88502972-35a4da00-cf9e-11ea-953d-b3ffff7b272e.png)

![image](https://user-images.githubusercontent.com/43821/88502982-3d647e80-cf9e-11ea-8801-d41461a635eb.png)

![image](https://user-images.githubusercontent.com/43821/88502986-42293280-cf9e-11ea-91b7-2435c129f158.png)


Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
@craig
Copy link
Contributor

craig bot commented Sep 4, 2020

Build failed:

This commit adds two new pages:

1. Active Sessions, a table of all active sessions in the database.
2. Session Details, a page with information about a single active
   session.

In addition, active sessions and queries are now terminatable from the
Active Sessions page.

Release note (admin ui change): add the session list and session detail
pages, and permit session and query termination from the UI.
Previously, running the list sessions or list local sessions APIs would
require the remote debugging setting to be turned on. Now that we have
role-based access controls for viewing session activity, this is no
longer necessary.

Release note: None
Release justification: low-risk update to new functionality
@jordanlewis
Copy link
Member Author

bors r+

🤞

@craig
Copy link
Contributor

craig bot commented Sep 4, 2020

Build succeeded:

@craig craig bot merged commit 67c0817 into cockroachdb:master Sep 4, 2020
@jordanlewis jordanlewis deleted the sessions-page branch September 21, 2020 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

webui: add a SESSIONS page
6 participants