-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
jobs: add table to display execution details #106879
Conversation
9407f6e
to
4b11656
Compare
In this loom we request the execution details for a job twice and see it populated in the list: https://www.loom.com/share/04764a4d62ea4e55827b149a4f5e1741?sid=30e99ca6-b9eb-4a15-aa42-819e2701219e |
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 10 of 13 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @dt)
-- commits
line 2 at r1:
thinking if the name "execution details" is the more clear name here. Since this seems to be listing the files, maybe a name that make it more clear? Execution Files
or Execution Details Files
. And then updating both the table title and the message of when is empty.
wdyt @dongniwang ?
-- commits
line 7 at r1:
nit: observability
-- commits
line 14 at r1:
how future is this "future"? Asking because if is still coming for 23.2, then it's okay. Otherwise I would question if the current feature is helpful the way it is now, since the user can't really click/download/see the files.
If is coming soon, then this PR can keep as is, otherwise I would just add a hidden style on the new section for now, until the completed feature is added.
pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobDetails.tsx
line 128 at r1 (raw file):
): React.ReactElement => { const id = job?.id; console.log(this.props.jobProfilerResponse);
remove this line
pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobProfilerView.tsx
line 13 at r1 (raw file):
import { cockroach } from "@cockroachlabs/crdb-protobuf-client"; import "antd/lib/pagination/style"; import "antd/lib/dropdown/style";
you only need to import the style of components you're importing from antd
, so you only need the row
and col
and can remove the pagination
, dropdown
and anchor
(below)
pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobProfilerView.tsx
line 14 at r1 (raw file):
import "antd/lib/pagination/style"; import "antd/lib/dropdown/style"; import moment from "moment";
use import moment from "moment-timezone";
pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobProfilerView.tsx
line 155 at r1 (raw file):
<Row gutter={24}> <Col className="gutter-row" span={24}> <SortedTable
is it possible to have a large number of files (more than 20?). If so, I'm not seeing any pagination on this table.
pkg/ui/workspaces/db-console/src/views/jobs/jobDetails.tsx
line 23 at r1 (raw file):
} from "src/redux/apiReducers"; import { AdminUIState } from "src/redux/state"; import { ListJobProfilerExecutionDetailsResponseMessage } from "oss/src/util/api";
remove the oss
, start with src/...
pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobProfilerView.module.scss
line 3 at r1 (raw file):
@import "src/core/index.module"; .anchor {
doesn't look like you use anchor
in this page, so there is no need for style for it.
Thanks for the ping @maryliag. @adityamaru Would it be possible to walk through the page structure with me in details?
|
Thanks for the review @maryliag! Re: @dongniwang's questions I want to preface it with the fact that for this release we are only thinking about support engineers and CRDB developers as our target audience. This
Each file in this list contains some information about the related job. These files will be requested by Support Engineers/CRDB Engineers to better understand the execution characteristics of a job. These files will initially contain:
Yes, a follow up change will add the ability to request the collecting of these execution detail files, and the ability to download each file (or all files). This will be worked on immediately after the structure of this PR has been vetted by the cluster obs folks.
There will be a button on this page to request execution details. This was left out of this change to reduce the scope of the review and keep the PR small while I iterate on the overall structure. |
I wonder if we should rename the tab "Advanced Debugging" instead of "Profiler", at least until 24.1+ / whenever we do more work around making these consumable by end-users vs crl engineers on support calls |
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dongniwang, @dt, and @maryliag)
Previously, maryliag (Marylia Gutierrez) wrote…
thinking if the name "execution details" is the more clear name here. Since this seems to be listing the files, maybe a name that make it more clear?
Execution Files
orExecution Details Files
. And then updating both the table title and the message of when is empty.
wdyt @dongniwang ?
Execution Detail Files
is a good idea! I'll wait for Dongni's comments
Previously, maryliag (Marylia Gutierrez) wrote…
how future is this "future"? Asking because if is still coming for 23.2, then it's okay. Otherwise I would question if the current feature is helpful the way it is now, since the user can't really click/download/see the files.
If is coming soon, then this PR can keep as is, otherwise I would just add a hidden style on the new section for now, until the completed feature is added.
As soon as you're happy with the structure of this PR I will work on adding the "request" and "download" functionality. Very much a goal for this release!
pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobDetails.tsx
line 128 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
remove this line
Done.
pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobProfilerView.tsx
line 13 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
you only need to import the style of components you're importing from
antd
, so you only need therow
andcol
and can remove thepagination
,dropdown
andanchor
(below)
Done.
pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobProfilerView.tsx
line 14 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
use
import moment from "moment-timezone";
Done.
pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobProfilerView.tsx
line 155 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
is it possible to have a large number of files (more than 20?). If so, I'm not seeing any pagination on this table.
For this release (and for the foreseeable future) it is very unlikely there will be >10 files on this page. For this reason we also didn't add a limit
and an offset
to the underlying RPC as we thought it'd be overkill for now. I think we can punt on pagination until we start seeing increased usage of this page.
pkg/ui/workspaces/db-console/src/views/jobs/jobDetails.tsx
line 23 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
remove the
oss
, start withsrc/...
Done.
pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobProfilerView.module.scss
line 3 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
doesn't look like you use
anchor
in this page, so there is no need for style for it.
Done.
4b11656
to
a398cb1
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 4 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @dongniwang, and @dt)
pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobDetails.tsx
line 58 at r2 (raw file):
enum TabKeysEnum { OVERVIEW = "Overview", PROFILER = "Advanced Debugging",
btw, it might be helpful to add a screenshot on the PR, because it helps the Docs team when they're creating stuff. Specially since you loom has the Profiler name, so you can always keep the PR description with the most up to date UI version from this PR.
pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobProfilerView.tsx
line 53 at r2 (raw file):
} export class JobProfilerView extends React.Component<
oh I completely missed that, sorry. Prefer using functional components instead of class components. Creating as a class was the old way, but React made several performance improvements and recommends functional components instead. We still have several places on the code that use classes, this is probably where you used as an example, but we want to get rid of them and eventually refactor.
We have places with the new way that you can look for example, such as Insights page
A few differences:
- Although props are created the same way, the state is different, now you can create each one with the
useState
function, that has it's own set and default value ( example ) - Functional components don't have the functions such as
component{DidUpdate|DidMount|WillUnmount}
- there is no
render
function, is just a straight up return with the html code
I see you are refreshing the data more frequently, so you can also use this part of code as an example
a398cb1
to
29096a3
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dongniwang, @dt, and @maryliag)
pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobDetails.tsx
line 58 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
btw, it might be helpful to add a screenshot on the PR, because it helps the Docs team when they're creating stuff. Specially since you loom has the Profiler name, so you can always keep the PR description with the most up to date UI version from this PR.
added!
pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobProfilerView.tsx
line 53 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
oh I completely missed that, sorry. Prefer using functional components instead of class components. Creating as a class was the old way, but React made several performance improvements and recommends functional components instead. We still have several places on the code that use classes, this is probably where you used as an example, but we want to get rid of them and eventually refactor.
We have places with the new way that you can look for example, such as Insights pageA few differences:
- Although props are created the same way, the state is different, now you can create each one with the
useState
function, that has it's own set and default value ( example )- Functional components don't have the functions such as
component{DidUpdate|DidMount|WillUnmount}
- there is no
render
function, is just a straight up return with the html codeI see you are refreshing the data more frequently, so you can also use this part of code as an example
Thanks for the code pointers they were very helpful. Let me know if I ported it correctly!
29096a3
to
ee063c7
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.
just a few nits. Also, now that you upgrade from class to function, can you create a new loom confirm that is working?
Reviewed 1 of 4 files at r2, 7 of 9 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @dt)
pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobProfilerView.tsx
line 62 at r4 (raw file):
title: "Execution Detail Files", hideTitleUnderline: true, cell: (executionDetails: string) => <div>{executionDetails}</div>,
nit: you don't need to add <div>
here, just executionDetails
is enough. We usually add the html when is a custom component, if is just a string you can use it directly
pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobProfilerView.tsx
line 128 at r4 (raw file):
data={executionDetailsResponse.data?.files} columns={columns} className={cx("execution-details-table")}
it doesn't look like you create this class, so this line is not doing anything and you can remove it
In cockroachdb#105384 and cockroachdb#106629 we added support to collect and list files that had been collected as part of a job's execution details. These files are meant to provide improved observability into the state of a job. This change is the first of a few that exposes these endpoints on the DBConsole job details page. This change only adds support for listing files that have been requested as part of a job's execution details. A follow-up change will add support to request these files, sort them and download them from the job details page. This page is not available on the Cloud Console as it is meant for advanced debugging. This change also renames the `Profiler` tab to `Advanced Debugging` as the users of this tab are going to be internal CRDB support and engineering for the time being. Informs: cockroachdb#105076 Release note (ui change): add table in the Profiler job details page that lists all the available files describing a job's execution details
ee063c7
to
b825a90
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.
New looms is here! - https://www.loom.com/share/5cf8c3f733b44f52987bdc90a13f2c78?sid=e0c4e08d-cfa0-4b6c-a7b1-796032dc90ca
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @maryliag)
pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobProfilerView.tsx
line 128 at r4 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
it doesn't look like you create this class, so this line is not doing anything and you can remove it
Done.
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 2 of 2 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dt)
TFTR! bors r=maryliag |
This PR was included in a batch that was canceled, it will be automatically retried |
Build succeeded: |
This change includes the collection of cluster-wide, active tracing spans to a job's execution details. The traces are stored in a zip with a text and jaegar json file per node, that contain the active tracing spans associated with the current execution of the job. Once cockroachdb#106879 and cockroachdb#107210 merge a trace.zip will be generated everytime a user requests new execution details from the job details page. This zip will be downloadable from the job details page itself. Informs: cockroachdb#102794 Release note: None
107114: sql: collect cluster-wide traces as part of job execution details r=stevendanna a=adityamaru This change includes the collection of cluster-wide, active tracing spans to a job's execution details. The traces are stored in a zip with a text and jaegar json file per node, that contain the active tracing spans associated with the current execution of the job. Once #106879 and #107210 merge a trace.zip will be generated everytime a user requests new execution details from the job details page. This zip will be downloadable from the job details page itself. Informs: #102794 Release note: None Co-authored-by: adityamaru <[email protected]>
In cockroachdb#106879 we added a table to the `Advanced Debugging` tab of the job details page. This table lists out all the execution detail files that are available for the given job. This change is a follow up to add download functionality to each row in the table. The format of the downloaded file is determined by the prefix of the filename. A final change to allow users to generate execution details will be added in the next follow up. Informs: cockroachdb#105076 Release note: None
In cockroachdb#106879 we added a table to the `Advanced Debugging` tab of the job details page. This table lists out all the execution detail files that are available for the given job. This change is a follow up to add download functionality to each row in the table. The format of the downloaded file is determined by the prefix of the filename. A final change to allow users to generate execution details will be added in the next follow up. Informs: cockroachdb#105076 Release note: None
In cockroachdb#106879 we added a table to the `Advanced Debugging` tab of the job details page. This table lists out all the execution detail files that are available for the given job. This change is a follow up to add download functionality to each row in the table. The format of the downloaded file is determined by the prefix of the filename. A final change to allow users to generate execution details will be added in the next follow up. Informs: cockroachdb#105076 Release note: None
107210: jobs: enable downloading execution detail files r=maryliag a=adityamaru In #106879 we added a table to the `Advanced Debugging` tab of the job details page. This table lists out all the execution detail files that are available for the given job. This change is a follow up to add download functionality to each row in the table. The format of the downloaded file is determined by the prefix of the filename. A final change to allow users to generate execution details will be added in the next follow up. Informs: #105076 Release note: None 107760: spanconfigccl: fix tests under multitenancy r=yuzefovich a=rafiss fixes #106818 fixes #106821 Release note: None Co-authored-by: adityamaru <[email protected]> Co-authored-by: Rafi Shamim <[email protected]>
In #105384 and #106629 we added support to collect and list files that had been collected as part of
a job's execution details. These files are meant
to provide improved obersvability into the state
of a job.
This change is the first of a few that exposes these endpoints on the DBConsole job details page. This change only adds support for listing files that have been requested as part of a job's execution details.
A future change will add support to request these files, sort them and download them from the job details page.
This page is not available on the Cloud Console as it is meant for advanced debugging.
Informs: #105076
Release note (ui change): add table in the Profiler job details page that lists all the available files describing a job's execution details