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

jobs: enable downloading execution detail files #107210

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

adityamaru
Copy link
Contributor

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

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@adityamaru
Copy link
Contributor Author

adityamaru added a commit to adityamaru/cockroach that referenced this pull request Jul 24, 2023
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
@adityamaru adityamaru force-pushed the download-files branch 2 times, most recently from 7917937 to c0c8282 Compare July 24, 2023 20:13
@adityamaru adityamaru requested review from dt, maryliag and a team July 24, 2023 20:14
@adityamaru adityamaru marked this pull request as ready for review July 24, 2023 20:14
@adityamaru adityamaru requested review from a team as code owners July 24, 2023 20:14
@adityamaru adityamaru requested a review from a team July 24, 2023 20:14
@adityamaru
Copy link
Contributor Author

first commit is from #107198 so only second commit needs a review here!

craig bot pushed a commit that referenced this pull request Jul 25, 2023
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]>
@adityamaru
Copy link
Contributor Author

@maryliag based on your offline chat I tried to add a space between Download and the icon but I can't seem to figure it out 🤦 . I looked if statement bundles do anything but they seem to also have the same button/icon layout as in the loom.

@adityamaru
Copy link
Contributor Author

oh I might have figured it out, pushing something.

@adityamaru
Copy link
Contributor Author

here we go!

Screenshot 2023-07-26 at 4 37 06 PM

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for also updating name of function to clarify is about files.
I will leave to somebody on your team to review the first commit, since I don't know enough to make sure is covering everything, so my review is focused on the ui changes only, which is the second commit

For that I added a few nits, otherwise :lgtm:

Reviewed 6 of 6 files at r1, 9 of 12 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru and @dt)


pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobProfilerView.tsx line 17 at r2 (raw file):

import { Button, InlineAlert, Icon } from "@cockroachlabs/ui-components";
import { Row, Col } from "antd";
import "antd/lib/icon/style";

curious, where you getting lint error if you didn't add those? Asking because you just need to import the style when the import was the "antd". The Button and Icon were not, so there is no need


pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobProfilerView.module.scss line 5 at r2 (raw file):

.crl-job-profiler-view {
    display: flex;
    flex-direction: column;

I don't see you using crl-job-profiler-view anywhere, just crl-job-profiler-view__actions-column, so these tow lines are not being used anywhere

@blathers-crl
Copy link

blathers-crl bot commented Jul 26, 2023

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

  • We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
  • When CI has completed, please ensure no errors have appeared.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@blathers-crl blathers-crl bot added the O-community Originated from the community label Jul 26, 2023
Copy link
Contributor Author

@adityamaru adityamaru 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 (and 1 stale) (waiting on @dt and @maryliag)


pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobProfilerView.tsx line 17 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

curious, where you getting lint error if you didn't add those? Asking because you just need to import the style when the import was the "antd". The Button and Icon were not, so there is no need

Done.


pkg/ui/workspaces/cluster-ui/src/jobs/jobDetailsPage/jobProfilerView.module.scss line 5 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

I don't see you using crl-job-profiler-view anywhere, just crl-job-profiler-view__actions-column, so these tow lines are not being used anywhere

Done.

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
@adityamaru
Copy link
Contributor Author

Now that this is rebased, I'm going to merge this UI only change.

TFTR!

bors r=maryliag

@craig
Copy link
Contributor

craig bot commented Jul 28, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jul 28, 2023

This PR was included in a batch that timed out, it will be automatically retried

@adityamaru
Copy link
Contributor Author

bors retry

@craig
Copy link
Contributor

craig bot commented Jul 28, 2023

Already running a review

@craig
Copy link
Contributor

craig bot commented Jul 28, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jul 28, 2023

Build succeeded:

@craig craig bot merged commit 167da65 into cockroachdb:master Jul 28, 2023
@adityamaru adityamaru deleted the download-files branch July 28, 2023 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-community Originated from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants