cli: add cockroach debug job-trace #65324

adityamaru · 2021-05-17T10:39:41Z

This change adds a new cli command - cockroach debug job-trace
that can be used to grab information from inflight trace spans
associated with the execution of a particular job.
The command takes a jobID and file destination where the
information will be dumped. It also requires a --url of the
node from which to pull the inflight spans.

In the future we will have a means of pulling inflight spans from
the cluster, but at the moment this command must be run against every
node to get a complete picture of job execution.

Informs: #64992

Release note (cli change): Adds a cockroach debug job-trace command
that takes 2 arguments: and file destination, along with a
--url pointing to the node on which to execute this command.
The command pulls information about inflight trace spans associated
with the job and dumps it to the file destination.

cockroach-teamcity · 2021-05-17T10:39:51Z

This change is

adityamaru · 2021-05-17T10:40:48Z

The first two commits are from #65322.

jordanlewis · 2021-05-17T12:07:15Z

Excellent.

knz

Reviewed 5 of 5 files at r1, 2 of 2 files at r2, 4 of 4 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @pbardea)

pkg/cli/debug_job_trace.go, line 34 at r3 (raw file):

	Args:  cobra.MinimumNArgs(2),
	RunE: MaybeDecorateGRPCError(
		func(command *cobra.Command, args []string) error {

nit: please move this function to a named function below.

pkg/cli/debug_job_trace.go, line 65 at r3 (raw file):

	}

	jobID, err := strconv.Atoi(jobIDStr)

This check is parameter validation, and should be done upfront in the command before starting to open connections etc.

pkg/cli/debug_job_trace.go, line 71 at r3 (raw file):

	var jobQuery = fmt.Sprintf(`SELECT trace_id FROM crdb_internal.jobs WHERE job_id='%d'`, jobID)
	rows, err := sqlConn.Query(jobQuery, nil)

please pass the query inside sqlConn.Query and use a placeholder to pass jobID.

pkg/cli/debug_job_trace.go, line 72 at r3 (raw file):

	var jobQuery = fmt.Sprintf(`SELECT trace_id FROM crdb_internal.jobs WHERE job_id='%d'`, jobID)
	rows, err := sqlConn.Query(jobQuery, nil)
	if err != nil {

I think you need to call rows.Close().
Please extract the retrieval of the traceID into a separate function, and ensure rows.Close() is called within that function.

pkg/cli/debug_job_trace.go, line 103 at r3 (raw file):

	var resRows [][]string
	spanRows, err := sqlConn.Query(query, nil)

ditto calling spanRows.Close().

pkg/cli/debug_job_trace.go, line 108 at r3 (raw file):

	}

	populateRow := func(vals []driver.Value) ([]string, error) {

I don't understand why you need this code -- what was wrong with calling runQueryAndFormatResults()?

pkg/sql/crdb_internal.go, line 623 at r2 (raw file):

	error              		STRING,
	coordinator_id     		INT,
  trace_id              INT

nit: spacing is off

adityamaru

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @knz, and @pbardea)

pkg/cli/debug_job_trace.go, line 34 at r3 (raw file):

Previously, knz (kena) wrote…

nit: please move this function to a named function below.

Done.

pkg/cli/debug_job_trace.go, line 65 at r3 (raw file):

Previously, knz (kena) wrote…

This check is parameter validation, and should be done upfront in the command before starting to open connections etc.

Done.

pkg/cli/debug_job_trace.go, line 71 at r3 (raw file):

Previously, knz (kena) wrote…

please pass the query inside sqlConn.Query and use a placeholder to pass jobID.

Done.

pkg/cli/debug_job_trace.go, line 72 at r3 (raw file):

Previously, knz (kena) wrote…

I think you need to call rows.Close().
Please extract the retrieval of the traceID into a separate function, and ensure rows.Close() is called within that function.

Done.

pkg/cli/debug_job_trace.go, line 103 at r3 (raw file):

Previously, knz (kena) wrote…

ditto calling spanRows.Close().

Deleted this code since it is no longer required.

pkg/cli/debug_job_trace.go, line 108 at r3 (raw file):

Previously, knz (kena) wrote…

I don't understand why you need this code -- what was wrong with calling runQueryAndFormatResults()?

doh, that's exactly what I want. Thanks for always pointing me to the right place 🙂

adityamaru · 2021-05-19T10:12:11Z

Rebased on #65322 that was merged, and also cherry-picked #65454. The last commit is the pertinent one!

knz

The code looks good overall, but I'm missing the test code. Can you point me to it?

Reviewed 5 of 11 files at r4, 7 of 7 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @pbardea)

pkg/cli/debug_job_trace.go, line 72 at r3 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

Done.

nice 💯

pkg/cli/debug_job_trace.go, line 35 at r5 (raw file):

func runDebugJobTrace(_ *cobra.Command, args []string) error {
	jobID, err := strconv.Atoi(args[0])

Maybe strconv.ParseInt here and pass it as int64 below. This avoids a cast.

pkg/sql/crdb_internal.go, line 623 at r2 (raw file):

Previously, knz (kena) wrote…

nit: spacing is off

still off

pbardea

+1 to tests

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru)

pkg/cli/debug_job_trace.go, line 104 at r5 (raw file):

	}

	var inflightSpanQuery = `WITH spans AS(

nit: formatting of the query looks a bit of

adityamaru · 2021-05-25T15:12:00Z

I'm actually struggling a little while writing a test for this. I've added a WIP commit for the general structure but I'm running into two main issues:

When running the command through the CLI test, the outputted file is of a different format, as compared to testing it locally.

Test is outputting:

Local testing is outputting:

The latter is what I would expect, any ideas why there is a difference in behavior @knz?

I'm not too sure what to really test the output against. I was thinking of grepping the outputted file for the payload and matching that column against what we expect to see. We can't compare it against a static file in testdata because of the nature of the output. Do you all have any other ideas?

knz · 2021-05-25T15:17:11Z

you need to use --format to choose a specific output format and make your output the same in every case

(The default value of --format is different depending on how you invoke the command)

knz · 2021-05-25T15:17:50Z

Do you all have any other ideas?

We have other tests (e.g. for node status) that expect specific outputs based on regular rexpressions.

adityamaru

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @pbardea)

pkg/cli/debug_job_trace.go, line 35 at r5 (raw file):

Previously, knz (kena) wrote…

Maybe strconv.ParseInt here and pass it as int64 below. This avoids a cast.

Done.

pkg/sql/crdb_internal.go, line 623 at r2 (raw file):

Previously, knz (kena) wrote…

still off

fixed the trace_id prefix indent. the col type indentation appears fine in my editor, but not in reviewable.

knz

Code structure/style LGTM, but I'll let your team chime in on correctness.

Reviewed 2 of 3 files at r6, 5 of 5 files at r7.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @knz)

pkg/sql/crdb_internal.go, line 621 at r6 (raw file):

	fraction_completed 		FLOAT,
	high_water_timestamp	DECIMAL,
	error              		STRING,

While you're at it, perhaps you could put that STRING and the INT just below it on the same column as the rest.

adityamaru · 2021-05-26T16:54:52Z

While you're at it, perhaps you could put that STRING and the INT just below it on the same column as the rest.

These only appear shifted in reviewable for some reason. Github and in my IDE the spacing is identical for all the columns.

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @knz, and @pbardea)

pkg/sql/crdb_internal.go, line 621 at r6 (raw file):

Previously, knz (kena) wrote…

While you're at it, perhaps you could put that STRING and the INT just below it on the same column as the rest.

I'm realizing what the issue is: this vtable definition uses tabs, not spaces, in its schema.
all the other vtables uses spaces - this one is the exception.

So the improvement would be to convert this to use spaces like everything else: two spaces at the beginning, then spaces to align the columns.

adityamaru

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @pbardea)

pkg/sql/crdb_internal.go, line 621 at r6 (raw file):

Previously, knz (kena) wrote…

I'm realizing what the issue is: this vtable definition uses tabs, not spaces, in its schema.
all the other vtables uses spaces - this one is the exception.

So the improvement would be to convert this to use spaces like everything else: two spaces at the beginning, then spaces to align the columns.

nice! all done 🙂

knz · 2021-05-26T19:45:02Z

my previous review stands

Code structure/style LGTM, but I'll let your team chime in on correctness.

pbardea

LGTM

adityamaru · 2021-05-31T13:36:37Z

Had to fix up a few things to get the test passing:

debug job-trace no longer creates a "job-trace" directory, but just creates whatever path is passed in as the CLI argument.
test cleans up directory after itself.
we would occasionally see a "contention event" in the trace for a job. We are only really interested in the header and the trace we injected, so the test only checks for those now.

This change adds a new cli command - `cockroach debug job-trace` that can be used to grab information from inflight trace spans associated with a particular jobs execution. The command takes a `jobID` and file destination where the information will be dumped. It also requires a `--url` of the node from which to pull the inflight spans. In the future we will have a means of pulling inflight spans from the cluster, but at the moment this command must be run against every node to get a complete picture of job execution. Release note (cli change): Adds a `cockroach debug job-trace` command that takes 2 arguments: <jobID> and file destination, alongwith a `--url` pointing to the node on which to execute this command against. The command pulls information about inflight trace spans associated with the job and dumps it to the file destination.

adityamaru · 2021-05-31T22:59:37Z

Seeing an Execution timeout in CI. Not sure what is causing this yet.

adityamaru · 2021-06-01T03:19:15Z

Going ahead and merging this. The bazel failure is unrelated and seems to have been occurring on most builds since ~3-4 days ago - pkg/sql/sql_test_/sql_test

TFTR!
bors r=pbardea,knz

craig · 2021-06-01T04:24:33Z

Build succeeded:

GitHub CI (Cockroach)

adityamaru requested a review from dt May 17, 2021 10:39

adityamaru requested a review from a team as a code owner May 17, 2021 10:39

adityamaru requested a review from a team May 17, 2021 10:39

adityamaru requested review from pbardea and removed request for a team and dt May 17, 2021 10:40

knz reviewed May 17, 2021

View reviewed changes

adityamaru force-pushed the debug-job-tracing branch from 7a0e53c to d1b11e6 Compare May 19, 2021 10:08

adityamaru commented May 19, 2021

View reviewed changes

adityamaru requested a review from knz May 19, 2021 10:12

knz reviewed May 19, 2021

View reviewed changes

pbardea reviewed May 20, 2021

View reviewed changes

adityamaru force-pushed the debug-job-tracing branch 2 times, most recently from 628654a to faef8c3 Compare May 25, 2021 14:12

adityamaru requested a review from pbardea May 25, 2021 15:12

adityamaru force-pushed the debug-job-tracing branch from faef8c3 to 1df2401 Compare May 26, 2021 09:07

adityamaru commented May 26, 2021

View reviewed changes

adityamaru force-pushed the debug-job-tracing branch from 1df2401 to 076db55 Compare May 26, 2021 09:14

adityamaru requested a review from knz May 26, 2021 09:15

knz approved these changes May 26, 2021

View reviewed changes

adityamaru force-pushed the debug-job-tracing branch from 076db55 to e4dbca5 Compare May 26, 2021 16:54

knz reviewed May 26, 2021

View reviewed changes

adityamaru force-pushed the debug-job-tracing branch from e4dbca5 to 89170c9 Compare May 26, 2021 17:58

adityamaru commented May 26, 2021

View reviewed changes

pbardea approved these changes May 27, 2021

View reviewed changes

adityamaru force-pushed the debug-job-tracing branch 3 times, most recently from 8d78069 to 77cb68e Compare May 31, 2021 13:34

adityamaru force-pushed the debug-job-tracing branch from 77cb68e to 2871d62 Compare May 31, 2021 13:51

adityamaru force-pushed the debug-job-tracing branch from 2871d62 to a7a8da0 Compare May 31, 2021 22:59

craig bot merged commit 7762f00 into cockroachdb:master Jun 1, 2021

adityamaru added the backport-21.1.x label Jun 1, 2021

jseldess mentioned this pull request Sep 8, 2021

cli: add cockroach debug job-trace cockroachdb/docs#11514

Closed

kathancox mentioned this pull request Oct 6, 2021

Document CLI for job debug trace cockroachdb/docs#10943

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: add cockroach debug job-trace #65324

cli: add cockroach debug job-trace #65324

adityamaru commented May 17, 2021 •

edited

Loading

cockroach-teamcity commented May 17, 2021

adityamaru commented May 17, 2021

jordanlewis commented May 17, 2021

knz left a comment

adityamaru left a comment

adityamaru commented May 19, 2021

knz left a comment

pbardea left a comment

adityamaru commented May 25, 2021

knz commented May 25, 2021

knz commented May 25, 2021

adityamaru left a comment

knz left a comment

adityamaru commented May 26, 2021

knz left a comment

adityamaru left a comment

knz commented May 26, 2021

pbardea left a comment

adityamaru commented May 31, 2021

adityamaru commented May 31, 2021

adityamaru commented Jun 1, 2021

craig bot commented Jun 1, 2021

cli: add cockroach debug job-trace #65324

cli: add cockroach debug job-trace #65324

Conversation

adityamaru commented May 17, 2021 • edited Loading

cockroach-teamcity commented May 17, 2021

adityamaru commented May 17, 2021

jordanlewis commented May 17, 2021

knz left a comment

Choose a reason for hiding this comment

adityamaru left a comment

Choose a reason for hiding this comment

adityamaru commented May 19, 2021

knz left a comment

Choose a reason for hiding this comment

pbardea left a comment

Choose a reason for hiding this comment

adityamaru commented May 25, 2021

knz commented May 25, 2021

knz commented May 25, 2021

adityamaru left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

adityamaru commented May 26, 2021

knz left a comment

Choose a reason for hiding this comment

adityamaru left a comment

Choose a reason for hiding this comment

knz commented May 26, 2021

pbardea left a comment

Choose a reason for hiding this comment

adityamaru commented May 31, 2021

adityamaru commented May 31, 2021

adityamaru commented Jun 1, 2021

craig bot commented Jun 1, 2021

adityamaru commented May 17, 2021 •

edited

Loading