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

roachprod-microbench: add clean command #126810

Merged
merged 1 commit into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/cmd/roachprod-microbench/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library", "go_test")
go_library(
name = "roachprod-microbench_lib",
srcs = [
"clean.go",
"compare.go",
"executor.go",
"export.go",
Expand Down
85 changes: 85 additions & 0 deletions pkg/cmd/roachprod-microbench/clean.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// Copyright 2024 The Cockroach Authors.
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package main

import (
"fmt"
"io"
"os"
"path/filepath"
"strings"

"github.com/cockroachdb/errors"
)

type cleanConfig struct {
inputFilePath string
outputFilePath string
}

type clean struct {
cleanConfig
inputFile *os.File
}

func defaultCleanConfig() cleanConfig {
return cleanConfig{
inputFilePath: "",
outputFilePath: "",
}
}

func newClean(config cleanConfig) (*clean, error) {
file, err := os.Open(config.inputFilePath)
if err != nil {
return nil, err
}

return &clean{cleanConfig: config, inputFile: file}, nil
}

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@sambhav-jain-16 sambhav-jain-16 Jul 9, 2024

Choose a reason for hiding this comment

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

@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.

return err
}

outputFile, err := os.Create(c.outputFilePath)
if err != nil {
return err
}
defer outputFile.Close()

for _, benchmarkResult := range cleanedBenchmarkOutputLog.results {
if _, writeErr := outputFile.WriteString(
fmt.Sprintf("%s\n", strings.Join(benchmarkResult, " "))); writeErr != nil {
return errors.Wrap(writeErr, "failed to write benchmark result to file")
}
}

return nil
}

func (c *clean) cleanBenchmarkOutputLog() error {
defer c.inputFile.Close()

rawBenchmarkLogs, err := io.ReadAll(c.inputFile)
if err != nil {
return err
}

cleanedBenchmarkOutputLog := extractBenchmarkResults(string(rawBenchmarkLogs))
if err = c.writeCleanOutputToFile(cleanedBenchmarkOutputLog); err != nil {
return err
}

return nil
}
25 changes: 25 additions & 0 deletions pkg/cmd/roachprod-microbench/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,35 @@ Typical usage:
command.AddCommand(makeCompareCommand())
command.AddCommand(makeRunCommand())
command.AddCommand(makeExportCommand())
command.AddCommand(makeCleanCommand())

return command
}

func makeCleanCommand() *cobra.Command {
config := defaultCleanConfig()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Footnotes

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

runCmdFunc := func(cmd *cobra.Command, commandLine []string) error {
args, _ := splitArgsAtDash(cmd, commandLine)
Copy link
Contributor

Choose a reason for hiding this comment

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

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


config.inputFilePath = args[0]
config.outputFilePath = args[1]
c, err := newClean(config)
if err != nil {
return err
}

return c.cleanBenchmarkOutputLog()
}
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`,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, will update the documentation.

Args: cobra.ExactArgs(2),
RunE: runCmdFunc,
}
return command
}

func makeRunCommand() *cobra.Command {
config := defaultExecutorConfig()
runCmdFunc := func(cmd *cobra.Command, commandLine []string) error {
Expand Down
Loading