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.

Sign up for GitHub

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

Support camelCase CSV output argument for file_name and save_interval. #2150

Merged
merged 7 commits into from
Oct 6, 2021
Merged

Support camelCase CSV output argument for file_name and save_interval. #2150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
518226a
Support camelCase csv output argument for file_name and save_interval.
josephwoodward Oct 1, 2021
10d78a5
Address linting error
josephwoodward Oct 2, 2021
4ed547f
Update output/csv/config.go
josephwoodward Oct 5, 2021
0e93149
Correct csv output warning to indicate field is already deprecated
josephwoodward Oct 5, 2021
ac736b0
Remove nolint comment in favour of formatting long function accordingly
josephwoodward Oct 5, 2021
199248a
Add test over csv output deprecation log warning
josephwoodward Oct 5, 2021
8307029
Add assertion to log level
josephwoodward Oct 5, 2021
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
15 changes: 12 additions & 3 deletions output/csv/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"gopkg.in/guregu/null.v3"

"github.com/kelseyhightower/envconfig"
"github.com/sirupsen/logrus"
"go.k6.io/k6/lib/types"
)

Expand Down Expand Up @@ -59,7 +60,7 @@ func (c Config) Apply(cfg Config) Config {
}

// ParseArg takes an arg string and converts it to a config
func ParseArg(arg string) (Config, error) {
func ParseArg(arg string, logger *logrus.Logger) (Config, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have any thoughts on how we could avoid such a breaking change to the package's pubic API? Other options I was considering was:

  1. Use Config to indicate a soon to be deprecated argument was used, and write the log message further up the call stack where the logger is initialised (in output.go)
  2. Move the checking of the soon to be deprecated arguments to output.go

Copy link
Contributor

@imiric imiric Oct 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hi, thanks for the PR!

Hhmm yeah, this is not ideal, but I think I prefer leaving it as is more than the alternatives, as currently it's clear that the warning happens where the configuration is assembled, and doing that validation outside of config.go would muddy things a bit.

Other than that a package like warnings might help, though it's not very idiomatic and we wouldn't want to add another dependency because of this. Maybe a custom error type that we can return and check for would be better.

So I'd say to leave it as is, unless someone from @grafana/k6-devs has a better idea.

Copy link
Contributor Author

@josephwoodward josephwoodward Oct 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, thanks @imiric

Hhmm yeah, this is not ideal, but I think I prefer leaving it as is more than the alternatives, as currently it's clear that the warning happens where the configuration is assembled, and doing that validation outside of config.go would muddy things a bit.

Yeah, I agree - this was my rational, it keeps the logic local. Interested to see what @grafana/k6-devs think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps, it makes sense to add this case (logging during config parsing) to #883 as "nice-to-have" to figure out how best to approach this if it comes up again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yorugac Good idea, I've just added this as a nice to have to that issue. 👍

c := Config{}

if !strings.Contains(arg, "=") {
Expand All @@ -76,11 +77,17 @@ func ParseArg(arg string) (Config, error) {
}
switch r[0] {
case "save_interval":
logger.Warnf("CSV output argument '%s' is deprecated, please use 'saveInterval' instead.", r[0])
fallthrough
case "saveInterval":
err := c.SaveInterval.UnmarshalText([]byte(r[1]))
if err != nil {
return c, err
}
case "file_name":
logger.Warnf("CSV output argument '%s' is deprecated, please use 'fileName' instead.", r[0])
fallthrough
case "fileName":
c.FileName = null.StringFrom(r[1])
default:
return c, fmt.Errorf("unknown key %q as argument for csv output", r[0])
Expand All @@ -92,7 +99,9 @@ func ParseArg(arg string) (Config, error) {

// GetConsolidatedConfig combines {default config values + JSON config +
// environment vars + arg config values}, and returns the final result.
func GetConsolidatedConfig(jsonRawConf json.RawMessage, env map[string]string, arg string) (Config, error) {
func GetConsolidatedConfig(
jsonRawConf json.RawMessage, env map[string]string, arg string, logger *logrus.Logger,
) (Config, error) {
result := NewConfig()
if jsonRawConf != nil {
jsonConf := Config{}
Expand All @@ -110,7 +119,7 @@ func GetConsolidatedConfig(jsonRawConf json.RawMessage, env map[string]string, a
result = result.Apply(envConfig)

if arg != "" {
urlConf, err := ParseArg(arg)
urlConf, err := ParseArg(arg, logger)
if err != nil {
return result, err
}
Expand Down
42 changes: 39 additions & 3 deletions output/csv/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,14 @@ import (
"testing"
"time"

"github.com/sirupsen/logrus"

"gopkg.in/guregu/null.v3"

"github.com/sirupsen/logrus/hooks/test"
"github.com/stretchr/testify/assert"

"go.k6.io/k6/lib/testutils"
"go.k6.io/k6/lib/types"
)

Expand Down Expand Up @@ -77,8 +81,9 @@ func TestApply(t *testing.T) {

func TestParseArg(t *testing.T) {
cases := map[string]struct {
config Config
expectedErr bool
config Config
expectedLogEntries []string
expectedErr bool
}{
"test_file.csv": {
config: Config{
Expand All @@ -90,12 +95,33 @@ func TestParseArg(t *testing.T) {
config: Config{
SaveInterval: types.NullDurationFrom(5 * time.Second),
},
expectedLogEntries: []string{
"CSV output argument 'save_interval' is deprecated, please use 'saveInterval' instead.",
},
},
"saveInterval=5s": {
config: Config{
SaveInterval: types.NullDurationFrom(5 * time.Second),
},
},
"file_name=test.csv,save_interval=5s": {
config: Config{
FileName: null.StringFrom("test.csv"),
SaveInterval: types.NullDurationFrom(5 * time.Second),
},
expectedLogEntries: []string{
"CSV output argument 'file_name' is deprecated, please use 'fileName' instead.",
"CSV output argument 'save_interval' is deprecated, please use 'saveInterval' instead.",
},
},
"fileName=test.csv,save_interval=5s": {
config: Config{
FileName: null.StringFrom("test.csv"),
SaveInterval: types.NullDurationFrom(5 * time.Second),
},
expectedLogEntries: []string{
"CSV output argument 'save_interval' is deprecated, please use 'saveInterval' instead.",
},
},
"filename=test.csv,save_interval=5s": {
expectedErr: true,
Expand All @@ -106,8 +132,11 @@ func TestParseArg(t *testing.T) {
arg := arg
testCase := testCase

testLogger, hook := test.NewNullLogger()
testLogger.SetOutput(testutils.NewTestOutput(t))

t.Run(arg, func(t *testing.T) {
config, err := ParseArg(arg)
config, err := ParseArg(arg, testLogger)

if testCase.expectedErr {
assert.Error(t, err)
Expand All @@ -116,6 +145,13 @@ func TestParseArg(t *testing.T) {
}
assert.Equal(t, testCase.config.FileName.String, config.FileName.String)
assert.Equal(t, testCase.config.SaveInterval.String(), config.SaveInterval.String())

var entries []string
for _, v := range hook.AllEntries() {
assert.Equal(t, v.Level, logrus.WarnLevel)
entries = append(entries, v.Message)
}
assert.ElementsMatch(t, entries, testCase.expectedLogEntries)
})
}
}
10 changes: 5 additions & 5 deletions output/csv/output.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,18 +76,18 @@ func newOutput(params output.Params) (*Output, error) {
sort.Strings(resTags)
sort.Strings(ignoredTags)

config, err := GetConsolidatedConfig(params.JSONConfig, params.Environment, params.ConfigArgument)
logger := params.Logger.WithFields(logrus.Fields{
"output": "csv",
"filename": params.ConfigArgument,
})
config, err := GetConsolidatedConfig(params.JSONConfig, params.Environment, params.ConfigArgument, logger.Logger)
if err != nil {
return nil, err
}

saveInterval := time.Duration(config.SaveInterval.Duration)
fname := config.FileName.String

logger := params.Logger.WithFields(logrus.Fields{
"output": "csv",
"filename": params.ConfigArgument,
})
if fname == "" || fname == "-" {
stdoutWriter := csv.NewWriter(os.Stdout)
return &Output{
Expand Down