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

cli: Include collected goroutine dumps in debug zip #36813

Merged
merged 2 commits into from Apr 16, 2019

Conversation

Projects
None yet
3 participants
@giorgosp
Copy link
Collaborator

commented Apr 12, 2019

This PR makes the debug zip command include the goroutine dump files in the final zip, if the
GoroutineDumper has run until the command is triggered.

Also enhances the testing of the GetFiles endpoint by testing the names and the contents of the downloaded files.

Fixes #33318

cc @tbg

server: Test heap profile files retrieval
This commit adds tests for the names and contents of the heap profile files
returned by the GetFiles endpoint.

Release note: None

@giorgosp giorgosp requested review from cockroachdb/cli-prs as code owners Apr 12, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

This change is Reviewable

@tbg
Copy link
Member

left a comment

Nice, thanks @giorgosp! Only minor comments.

Reviewed 2 of 2 files at r1, 6 of 6 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @giorgosp)


pkg/cli/zip.go, line 403 at r2 (raw file):

				} else {
					for _, file := range goroutinesResp.Files {
						name := prefix + "/goroutines/" + file.Name + ".pprof"

Use .txt here and code like

cockroach/pkg/cli/zip.go

Lines 346 to 348 in a17d619

if err := z.createRawOrError(prefix+"/stacks.txt", stacksData, err); err != nil {
return err
}


pkg/server/status_test.go, line 325 at r2 (raw file):

// TestStatusLocalGoroutineDumpsRetrieval tests the retrieval of the goroutine dumps.
func TestStatusLocalGoroutineDumpsRetrieval(t *testing.T) {

Fold this into the previous test. First, you set up both the heap files and the goroutine files. Then, you run two subtest

t.Run("heap", func(t *testing.T) {
 // fetch the heap files and verify
})

t.Run("heap", func(t *testing.T) {
 // fetch the goroutine files and verify
})

That way we're not setting up two servers. You can rename the test to TestStatusGetFiles

@giorgosp giorgosp force-pushed the giorgosp:goroutine-dumps-debug-zip branch 2 times, most recently from 78859ca to 6106f05 Apr 15, 2019

@giorgosp

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 15, 2019

Thank you @tbg, force-pushed some adjustments.

@giorgosp giorgosp force-pushed the giorgosp:goroutine-dumps-debug-zip branch from 6106f05 to 9efc6c0 Apr 15, 2019

@tbg

tbg approved these changes Apr 15, 2019

Copy link
Member

left a comment

Only minor nits. Btw, if you're not using reviewable yet, follow the button and ack the comments you've addressed, that makes it easier for me to follow what you've done. (Reviewable is also overall a bit more powerful which you'll appreciate once you end up contributing some more, which I hope you will!)

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @giorgosp)


pkg/cli/zip.go, line 398 at r2 (raw file):

						return err
					}); err != nil {
					if err := z.createError(prefix+"/goroutinefiles", err); err != nil {

wanna give the same treatment to /heapfiles above which should be /heapprof on error?


pkg/server/status_test.go, line 309 at r3 (raw file):

	})

	// Test fetching goroutine files

nit: style guide asks for punctuation for comments on their own line. (i.e. add trailing dot).

https://github.com/cockroachdb/cockroach/blob/master/docs/style.md


pkg/server/status_test.go, line 357 at r3 (raw file):

			Type: -1, Patterns: []string{"*"}}
		_, err = client.GetFiles(context.Background(), &request)
		if err == nil {

nit: we usually try to match the error more precisely, using testutils.IsError. (Same applies above).

@tbg
Copy link
Member

left a comment

PS here's another issue that'd be a good next one for you to tackle: #36846

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

cli: Include collected goroutine dumps in debug zip.
This commit implements another request to the GetFiles endpoint in order
to fetch the goroutine dump files for the debug zip command.
However, if the GoroutineDumper has not taken any dumps yet, no files
will be returned from the server.

Release note (cli change): Include collected goroutine dumps in `debug zip`.

@giorgosp giorgosp force-pushed the giorgosp:goroutine-dumps-debug-zip branch from 9efc6c0 to cb5bcd0 Apr 16, 2019

@giorgosp
Copy link
Collaborator Author

left a comment

Thank you Tobias, please take another look.

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


pkg/cli/zip.go, line 398 at r2 (raw file):

Previously, tbg (Tobias Grieger) wrote…

wanna give the same treatment to /heapfiles above which should be /heapprof on error?

Done.


pkg/cli/zip.go, line 403 at r2 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Use .txt here and code like

cockroach/pkg/cli/zip.go

Lines 346 to 348 in a17d619

if err := z.createRawOrError(prefix+"/stacks.txt", stacksData, err); err != nil {
return err
}

Done.


pkg/server/status_test.go, line 325 at r2 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Fold this into the previous test. First, you set up both the heap files and the goroutine files. Then, you run two subtest

t.Run("heap", func(t *testing.T) {
 // fetch the heap files and verify
})

t.Run("heap", func(t *testing.T) {
 // fetch the goroutine files and verify
})

That way we're not setting up two servers. You can rename the test to TestStatusGetFiles

Done.

@tbg

tbg approved these changes Apr 16, 2019

Copy link
Member

left a comment

Wonderful, thank you!

bors r+

Reviewed 2 of 2 files at r4.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

craig bot pushed a commit that referenced this pull request Apr 16, 2019

Merge #36813
36813: cli: Include collected goroutine dumps in debug zip r=tbg a=giorgosp

This PR makes the debug zip command include the goroutine dump files in the final zip, if the
GoroutineDumper has run until the command is triggered.

Also enhances the testing of the GetFiles endpoint by testing the names and the contents of the downloaded files.

Fixes #33318 

cc @tbg 

Co-authored-by: George Papadrosou <gpapadrosou@gmail.com>
@craig

This comment has been minimized.

Copy link

commented Apr 16, 2019

Build succeeded

@craig craig bot merged commit cb5bcd0 into cockroachdb:master Apr 16, 2019

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@giorgosp giorgosp deleted the giorgosp:goroutine-dumps-debug-zip branch Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.