roachprod-microbench: add clean command #126810

sambhav-jain-16 · 2024-07-08T10:34:41Z

Epic: none

Release note: None

This change adds a new utility command to roachprod-microbench as part of fixing #106661.

cockroach-teamcity · 2024-07-08T10:34:50Z

This change is

herkolategan

Looks good, just a few minor comments.

pkg/cmd/roachprod-microbench/clean.go

herkolategan · 2024-07-08T10:58:27Z

Side note: You can add the description into the commit This change adds a new utility command to roachprod-microbench as part of fixing https://github.com/cockroachdb/cockroach/issues/106661. for easier reference. Usually GitHub copies the description in the first commit automatically to the PR desc.

pkg/cmd/roachprod-microbench/clean.go

sambhav-jain-16 · 2024-07-08T14:06:08Z

TFTR !!

bors r=@herkolategan

herkolategan · 2024-07-08T14:07:05Z

bors cancel

craig · 2024-07-08T14:07:09Z

Canceled.

Epic: none Release note: None Previously cleaning benchmark output was only internally exposed in roachprod-microbench, but we now need it as part of a CI job, hence a new utility command "clean" has been added to roachprod-microbench See: cockroachdb#106661

sambhav-jain-16 · 2024-07-08T14:49:49Z

TFTR !!

bors r=@herkolategan

renatolabs

Minor comments!

For those of us with less context, it would have been helpful for the commit to be more substantial in the explanation provided (and also for future understandability).

In addition, it's not entirely clear to me how this is "part of fixing #106661" -- that issue refers to roachperf/macro benchmarks, and this is a change related to our microbenchmark infrastructure?

renatolabs · 2024-07-08T14:55:19Z

pkg/cmd/roachprod-microbench/main.go


 	return command
 }

+func makeCleanCommand() *cobra.Command {
+	config := defaultCleanConfig()


FYI: this function is unnecessary; variables in Go are always initialized to their zero value¹. This is equivalent to var config cleanConfig.

Footnotes

https://go.dev/ref/spec#The_zero_value ↩

renatolabs · 2024-07-08T14:58:12Z

pkg/cmd/roachprod-microbench/main.go

+func makeCleanCommand() *cobra.Command {
+	config := defaultCleanConfig()
+	runCmdFunc := func(cmd *cobra.Command, commandLine []string) error {
+		args, _ := splitArgsAtDash(cmd, commandLine)


We don't need splitArgsAtDash. This command doesn't seem to require or support any other args other than the two expected file paths.

renatolabs · 2024-07-08T15:02:44Z

pkg/cmd/roachprod-microbench/clean.go

+
+func (c *clean) writeCleanOutputToFile(cleanedBenchmarkOutputLog benchmarkExtractionResult) error {
+
+	if err := os.MkdirAll(filepath.Dir(c.outputFilePath), os.ModePerm); err != nil {


FWIW, I find this behaviour quite unusual for a command line utility: if I pass foo/bar/baz/path.txt to a command, I wouldn't expect it to create the entire directory tree to write to path.txt. It's generally better to have the caller be responsible for passing a valid path to begin with.

It makes handling these paths more explicit and also makes errors more evident, as writing to the path would fail if the path passed is invalid.

@renatolabs Good point! Since this is meant to be added as a CI step, I was thinking it is fine we can create the directory in the code itself.

renatolabs · 2024-07-08T15:04:55Z

pkg/cmd/roachprod-microbench/main.go

+	command := &cobra.Command{
+		Use:   "clean <inputFilePath> <outputFilePath>",
+		Short: "remove noisy logs from the benchmark output and dump it to a file",
+		Long:  `remove noisy logs from the benchmark output and dump it to a file`,


If I understand things right, this is kind of summarizing benchmark results and writing the summary to a file. Using remove here sounds like it's performing a destructive operation, which is not the case.

It's also unclear what "it" refers to in this sentence.

Understood, will update the documentation.

craig · 2024-07-08T15:16:13Z

Build succeeded:

sambhav-jain-16 · 2024-07-09T06:10:50Z

In addition, it's not entirely clear to me how this is "part of fixing #106661" -- that issue refers to roachperf/macro benchmarks, and this is a change related to our microbenchmark infrastructure?

This PR doesn't fully fix the issue but adds a utility command in roachprod-microbench that will be used in the CI step that will be added in further PRs.

For those of us with less context, it would have been helpful for the commit to be more substantial in the explanation provided (and also for future understandability).

Noted. Thanks for pointing out !

There were post-review comments in cockroachdb#126810. This change aims to address those. Epic: none Release note: None

Epic: none Release note: None As part of fixing cockroachdb#106661, we need to run microbenchmarks in a CI job and compare the microbenchmarks with that of the latest master branch commit. For this purpose we want to re-use `roachprod-microbench` compare command. However, as of current implementation of the compare command, it doesn't do threshold comparison and this PR aims to add that capability using a flag. It is a follow up change to cockroachdb#126810 which exposed `roachprod-microbench` capability of cleaning the benchmark output.

126884: roachprod-microbench: address review comments in clean command r=renatolabs a=sambhav-jain-16 There were post-review comments in #126810. This change aims to address those. Epic: none Release note: None Co-authored-by: Sambhav Jain <[email protected]>

Epic: none Release note: None As part of fixing cockroachdb#106661, we need to run microbenchmarks in a CI job and compare the microbenchmarks with that of the latest master branch commit. For this purpose we want to re-use `roachprod-microbench` compare command. However, as of current implementation of the compare command, it doesn't do threshold comparison and this PR aims to add that capability using a flag. It is a follow up change to cockroachdb#126810 which exposed `roachprod-microbench` capability of cleaning the benchmark output.

Epic: none Release note: None As part of fixing cockroachdb#106661, we need to run microbenchmarks in a CI job and compare the microbenchmarks with that of the latest master branch commit. For this purpose we want to re-use `roachprod-microbench` compare command. However, as of current implementation of the compare command, it doesn't do threshold comparison and this PR aims to add that capability using a flag. This change also adds a flag `--publish-sheets` to optionally allow publishing google sheets or not. It is a follow up change to cockroachdb#126810 which exposed `roachprod-microbench` capability of cleaning the benchmark output.

Epic: none Release note: None As part of fixing cockroachdb#106661, we need to run microbenchmarks in a CI job and compare the microbenchmarks with that of the latest master branch commit. For this purpose we want to re-use `roachprod-microbench` compare command. However, as of current implementation of the compare command, it doesn't do threshold comparison and this PR aims to add that capability using a flag. This change also adds a flag `--publish-sheets` to optionally allow publishing google sheets. It is a follow up change to cockroachdb#126810 which exposed `roachprod-microbench` capability of cleaning the benchmark output.

126885: roachprod-microbench: add threshold flag to compare command r=herkolategan a=sambhav-jain-16 Epic: none Release note: None As part of fixing #106661, we need to run microbenchmarks in a CI job and compare the microbenchmarks with that of the latest master branch commit. For this purpose we want to re-use `roachprod-microbench` compare command. However, as of current implementation of the compare command, it doesn't do threshold comparison and this PR aims to add that capability using a flag. This change also adds a flag `--publish-sheets` to optionally allow publishing google sheets or not. It is a follow up change to #126810 which exposed `roachprod-microbench` capability of cleaning the benchmark output. Co-authored-by: Sambhav Jain <[email protected]>

sambhav-jain-16 requested a review from a team as a code owner July 8, 2024 10:34

sambhav-jain-16 requested review from srosenberg and renatolabs and removed request for a team July 8, 2024 10:34

sambhav-jain-16 requested a review from herkolategan July 8, 2024 10:34

herkolategan reviewed Jul 8, 2024

View reviewed changes

pkg/cmd/roachprod-microbench/clean.go Outdated Show resolved Hide resolved

pkg/cmd/roachprod-microbench/clean.go Outdated Show resolved Hide resolved

herkolategan reviewed Jul 8, 2024

View reviewed changes

pkg/cmd/roachprod-microbench/clean.go Outdated Show resolved Hide resolved

sambhav-jain-16 force-pushed the rm-clean-command branch from 3ddc89b to 12b8c2d Compare July 8, 2024 11:45

sambhav-jain-16 requested a review from herkolategan July 8, 2024 11:54

sambhav-jain-16 force-pushed the rm-clean-command branch 2 times, most recently from 7faacb2 to bd19424 Compare July 8, 2024 12:23

herkolategan approved these changes Jul 8, 2024

View reviewed changes

sambhav-jain-16 force-pushed the rm-clean-command branch from bd19424 to 9a58f51 Compare July 8, 2024 14:08

sambhav-jain-16 force-pushed the rm-clean-command branch from 9a58f51 to 1399985 Compare July 8, 2024 14:09

renatolabs reviewed Jul 8, 2024

View reviewed changes

craig bot merged commit 61a6b81 into cockroachdb:master Jul 8, 2024
21 of 22 checks passed

sambhav-jain-16 mentioned this pull request Jul 9, 2024

roachprod-microbench: address review comments in clean command #126884

Merged

sambhav-jain-16 mentioned this pull request Jul 9, 2024

roachprod-microbench: add threshold flag to compare command #126885

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roachprod-microbench: add clean command #126810

roachprod-microbench: add clean command #126810

sambhav-jain-16 commented Jul 8, 2024

cockroach-teamcity commented Jul 8, 2024

herkolategan left a comment

herkolategan commented Jul 8, 2024

sambhav-jain-16 commented Jul 8, 2024

herkolategan commented Jul 8, 2024

craig bot commented Jul 8, 2024

sambhav-jain-16 commented Jul 8, 2024

renatolabs left a comment

renatolabs Jul 8, 2024

renatolabs Jul 8, 2024

renatolabs Jul 8, 2024

sambhav-jain-16 Jul 9, 2024 •

edited

Loading

renatolabs Jul 8, 2024

sambhav-jain-16 Jul 9, 2024

craig bot commented Jul 8, 2024

sambhav-jain-16 commented Jul 9, 2024


		func (c *clean) writeCleanOutputToFile(cleanedBenchmarkOutputLog benchmarkExtractionResult) error {

		if err := os.MkdirAll(filepath.Dir(c.outputFilePath), os.ModePerm); err != nil {

roachprod-microbench: add clean command #126810

roachprod-microbench: add clean command #126810

Conversation

sambhav-jain-16 commented Jul 8, 2024

cockroach-teamcity commented Jul 8, 2024

herkolategan left a comment

Choose a reason for hiding this comment

herkolategan commented Jul 8, 2024

sambhav-jain-16 commented Jul 8, 2024

herkolategan commented Jul 8, 2024

craig bot commented Jul 8, 2024

sambhav-jain-16 commented Jul 8, 2024

renatolabs left a comment

Choose a reason for hiding this comment

renatolabs Jul 8, 2024

Choose a reason for hiding this comment

Footnotes

renatolabs Jul 8, 2024

Choose a reason for hiding this comment

renatolabs Jul 8, 2024

Choose a reason for hiding this comment

sambhav-jain-16 Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

renatolabs Jul 8, 2024

Choose a reason for hiding this comment

sambhav-jain-16 Jul 9, 2024

Choose a reason for hiding this comment

craig bot commented Jul 8, 2024

sambhav-jain-16 commented Jul 9, 2024

sambhav-jain-16 Jul 9, 2024 •

edited

Loading