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

Jump to bottom

Add TimeFormat option for output/csv with 'unix' (default) and 'rfc3399' options #2274

Merged
merged 2 commits into from
Jun 8, 2022
Merged

Add TimeFormat option for output/csv with 'unix' (default) and 'rfc3399' options #2274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d0c7e6
output/csv: add ISO8601 time format flag
rpocklin Mar 8, 2022
d630eff
Code review feedback
rpocklin May 26, 2022
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
10 changes: 10 additions & 0 deletions output/csv/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,15 @@ type Config struct {
// Samples.
FileName null.String `json:"file_name" envconfig:"K6_CSV_FILENAME"`
SaveInterval types.NullDuration `json:"save_interval" envconfig:"K6_CSV_SAVE_INTERVAL"`
TimeFormat TimeFormat `json:"time_format" envconfig:"K6_CSV_TIME_FORMAT"`
}

// NewConfig creates a new Config instance with default values for some fields.
func NewConfig() Config {
return Config{
FileName: null.StringFrom("file.csv"),
SaveInterval: types.NullDurationFrom(1 * time.Second),
TimeFormat: Unix,
}
}

Expand All @@ -56,6 +58,7 @@ func (c Config) Apply(cfg Config) Config {
if cfg.SaveInterval.Valid {
c.SaveInterval = cfg.SaveInterval
}
c.TimeFormat = cfg.TimeFormat
return c
}

Expand All @@ -66,6 +69,7 @@ func ParseArg(arg string, logger *logrus.Logger) (Config, error) {
if !strings.Contains(arg, "=") {
c.FileName = null.StringFrom(arg)
c.SaveInterval = types.NullDurationFrom(1 * time.Second)
c.TimeFormat = Unix
return c, nil
}

Expand All @@ -89,6 +93,12 @@ func ParseArg(arg string, logger *logrus.Logger) (Config, error) {
fallthrough
case "fileName":
c.FileName = null.StringFrom(r[1])
case "timeFormat":
c.TimeFormat = TimeFormat(r[1])
if !c.TimeFormat.IsValid() {
return c, fmt.Errorf("unknown value %q as argument for csv output timeFormat, expected 'unix' or 'rfc3399'", arg)
}

default:
return c, fmt.Errorf("unknown key %q as argument for csv output", r[0])
}
Expand Down
13 changes: 13 additions & 0 deletions output/csv/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,30 +39,36 @@ func TestNewConfig(t *testing.T) {
config := NewConfig()
assert.Equal(t, "file.csv", config.FileName.String)
assert.Equal(t, "1s", config.SaveInterval.String())
assert.Equal(t, TimeFormat("unix"), config.TimeFormat)
}

func TestApply(t *testing.T) {
configs := []Config{
{
FileName: null.StringFrom(""),
SaveInterval: types.NullDurationFrom(2 * time.Second),
TimeFormat: "unix",
},
{
FileName: null.StringFrom("newPath"),
SaveInterval: types.NewNullDuration(time.Duration(1), false),
TimeFormat: "unix",
},
}
expected := []struct {
FileName string
SaveInterval string
TimeFormat TimeFormat
}{
{
FileName: "",
SaveInterval: "2s",
TimeFormat: TimeFormat("unix"),
},
{
FileName: "newPath",
SaveInterval: "1s",
TimeFormat: TimeFormat("unix"),
},
}

Expand All @@ -75,6 +81,7 @@ func TestApply(t *testing.T) {

assert.Equal(t, expected.FileName, baseConfig.FileName.String)
assert.Equal(t, expected.SaveInterval, baseConfig.SaveInterval.String())
assert.Equal(t, expected.TimeFormat, baseConfig.TimeFormat)
})
}
}
Expand Down Expand Up @@ -126,6 +133,12 @@ func TestParseArg(t *testing.T) {
"filename=test.csv,save_interval=5s": {
expectedErr: true,
},
"fileName=test.csv,timeFormat=rfc3399": {
config: Config{
FileName: null.StringFrom("test.csv"),
TimeFormat: "rfc3399",
},
},
}

for arg, testCase := range cases {
Expand Down
19 changes: 19 additions & 0 deletions output/csv/consts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package csv

// TimeFormat custom enum type
type TimeFormat string

// valid defined values for TimeFormat
const (
Unix TimeFormat = "unix"
RFC3399 TimeFormat = "rfc3399"
)

// IsValid validates TimeFormat
func (timeFormat TimeFormat) IsValid() bool {
switch timeFormat {
case Unix, RFC3399:
return true
}
return false
}
Comment on lines +13 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we need this, it could be directly on the parser.

Copy link
Contributor Author

@rpocklin rpocklin May 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This seemed to be idiomatic for Go to have isValid() methods on types, and allows for a clean extension of TimeFormat types in the future.

Copy link
Contributor

@codebien codebien May 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If a timeFormat exists it must be valid, for that I prefer to maintain this logic on the parsing side so we can keep it centralized. @mstoykov opinion?

I think we should move this check in the output constructor so we can check the consolidated value and we can validate also values from json and env.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you can make a suggestion that might help illustrate what you mean?

Copy link
Contributor

@codebien codebien May 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I did it directly on the right line so it should be easier to explain #2274 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

For enums in general we use enumer as for example with

k6/lib/runtime_options.go

Lines 32 to 33 in 1679c68

//go:generate enumer -type=CompatibilityMode -transform=snake -trimprefix CompatibilityMode -output compatibility_mode_gen.go
type CompatibilityMode uint8

Which generates https://github.com/grafana/k6/blob/1679c6843c7a5578b25aaba228d98476926467c2/lib/compatibility_mode_gen.go and is then used

k6/lib/runtime_options.go

Lines 71 to 78 in 1679c68

if cm, err = CompatibilityModeString(val); err != nil {
var compatValues []string
for _, v := range CompatibilityModeValues() {
compatValues = append(compatValues, v.String())
}
err = fmt.Errorf(`invalid compatibility mode "%s". Use: "%s"`,
val, strings.Join(compatValues, `", "`))
}

Copy link
Contributor Author

@rpocklin rpocklin Jun 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

For enums in general we use enumer as for example with

k6/lib/runtime_options.go

Lines 32 to 33 in 1679c68

//go:generate enumer -type=CompatibilityMode -transform=snake -trimprefix CompatibilityMode -output compatibility_mode_gen.go
type CompatibilityMode uint8

Which generates https://github.com/grafana/k6/blob/1679c6843c7a5578b25aaba228d98476926467c2/lib/compatibility_mode_gen.go and is then used

k6/lib/runtime_options.go

Lines 71 to 78 in 1679c68

if cm, err = CompatibilityModeString(val); err != nil {
var compatValues []string
for _, v := range CompatibilityModeValues() {
compatValues = append(compatValues, v.String())
}
err = fmt.Errorf(`invalid compatibility mode "%s". Use: "%s"`,
val, strings.Join(compatValues, `", "`))
}

@mstoykov I spent a long time learning enumer and how it works for Go. I ended up with the follow error:
enumer: can't handle non-integer constant type TimeFormat

From what I can understand, this would only work on integer-based enums, whereas in this case we need to preserve the string literals as they are passed in by the user (via the command line) and compared to the matching TimeFormat const.

Let me know if this is still possible with your suggestion, otherwise i'm at a loss how to implement what you are suggesting.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rpocklin yes, as you noticed, enumer is integer-based but it shouldn't be an issue. As you can see from the code posted above, enumer generates a function for mapping the type from a string. In the linked code is if cm, err = CompatibilityModeString(val); err != nil { where in case of error it means that a value hasn't been found.

Let me know if it helps.

4 changes: 4 additions & 0 deletions output/csv/doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/*
Package csv implements an output writing metrics in csv format
*/
package csv
18 changes: 15 additions & 3 deletions output/csv/output.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"fmt"
"os"
"sort"
"strconv"
"strings"
"sync"
"time"
Expand Down Expand Up @@ -54,6 +55,7 @@ type Output struct {
ignoredTags []string
row []string
saveInterval time.Duration
timeFormat TimeFormat
}

// New Creates new instance of CSV output
Expand Down Expand Up @@ -97,6 +99,7 @@ func newOutput(params output.Params) (*Output, error) {
csvWriter: stdoutWriter,
row: make([]string, 3+len(resTags)+1),
saveInterval: saveInterval,
timeFormat: config.TimeFormat,
closeFn: func() error { return nil },
logger: logger,
params: params,
Expand All @@ -114,6 +117,7 @@ func newOutput(params output.Params) (*Output, error) {
ignoredTags: ignoredTags,
row: make([]string, 3+len(resTags)+1),
saveInterval: saveInterval,
timeFormat: config.TimeFormat,
logger: logger,
params: params,
}
Expand Down Expand Up @@ -182,7 +186,7 @@ func (o *Output) flushMetrics() {
for _, sc := range samples {
for _, sample := range sc.GetSamples() {
sample := sample
row := SampleToRow(&sample, o.resTags, o.ignoredTags, o.row)
row := SampleToRow(&sample, o.resTags, o.ignoredTags, o.row, o.timeFormat)
err := o.csvWriter.Write(row)
if err != nil {
o.logger.WithField("filename", o.fname).Error("CSV: Error writing to file")
Expand All @@ -200,9 +204,17 @@ func MakeHeader(tags []string) []string {
}

// SampleToRow converts sample into array of strings
func SampleToRow(sample *metrics.Sample, resTags []string, ignoredTags []string, row []string) []string {
func SampleToRow(sample *metrics.Sample, resTags []string, ignoredTags []string, row []string,
timeFormat TimeFormat,
) []string {
row[0] = sample.Metric.Name
row[1] = fmt.Sprintf("%d", sample.Time.Unix())

if timeFormat == RFC3399 {
row[1] = sample.Time.Format(time.RFC3339)
} else {
row[1] = strconv.FormatInt(sample.Time.Unix(), 10)
}

row[2] = fmt.Sprintf("%f", sample.Value)
sampleTags := sample.Tags.CloneTags()

Expand Down
46 changes: 43 additions & 3 deletions output/csv/output_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ func TestSampleToRow(t *testing.T) {
sample *metrics.Sample
resTags []string
ignoredTags []string
timeFormat string
}{
{
testname: "One res tag, one ignored tag, one extra tag",
Expand All @@ -88,6 +89,7 @@ func TestSampleToRow(t *testing.T) {
},
resTags: []string{"tag1"},
ignoredTags: []string{"tag2"},
timeFormat: "",
},
{
testname: "Two res tags, three extra tags",
Expand All @@ -105,9 +107,10 @@ func TestSampleToRow(t *testing.T) {
},
resTags: []string{"tag1", "tag2"},
ignoredTags: []string{},
timeFormat: "",
},
{
testname: "Two res tags, two ignored",
testname: "Two res tags, two ignored, with RFC3399 timestamp",
sample: &metrics.Sample{
Time: time.Unix(1562324644, 0),
Metric: testMetric,
Expand All @@ -123,6 +126,7 @@ func TestSampleToRow(t *testing.T) {
},
resTags: []string{"tag1", "tag3"},
ignoredTags: []string{"tag4", "tag6"},
timeFormat: "rfc3399",
},
}

Expand Down Expand Up @@ -158,7 +162,7 @@ func TestSampleToRow(t *testing.T) {
{
baseRow: []string{
"my_metric",
"1562324644",
"2019-07-05T11:04:04Z",
"1.000000",
"val1",
"val3",
Expand All @@ -173,10 +177,11 @@ func TestSampleToRow(t *testing.T) {
for i := range testData {
testname, sample := testData[i].testname, testData[i].sample
resTags, ignoredTags := testData[i].resTags, testData[i].ignoredTags
timeFormat := TimeFormat(testData[i].timeFormat)
expectedRow := expected[i]

t.Run(testname, func(t *testing.T) {
row := SampleToRow(sample, resTags, ignoredTags, make([]string, 3+len(resTags)+1))
row := SampleToRow(sample, resTags, ignoredTags, make([]string, 3+len(resTags)+1), timeFormat)
for ind, cell := range expectedRow.baseRow {
assert.Equal(t, cell, row[ind])
}
Expand Down Expand Up @@ -225,6 +230,7 @@ func TestRun(t *testing.T) {
samples []metrics.SampleContainer
fileName string
fileReaderFunc func(fileName string, fs afero.Fs) string
timeFormat string
outputContent string
}{
{
Expand Down Expand Up @@ -253,6 +259,7 @@ func TestRun(t *testing.T) {
},
fileName: "test",
fileReaderFunc: readUnCompressedFile,
timeFormat: "",
outputContent: "metric_name,timestamp,metric_value,check,error,extra_tags\n" + "my_metric,1562324643,1.000000,val1,val3,url=val2\n" + "my_metric,1562324644,1.000000,val1,val3,tag4=val4&url=val2\n",
},
{
Expand Down Expand Up @@ -281,8 +288,38 @@ func TestRun(t *testing.T) {
},
fileName: "test.gz",
fileReaderFunc: readCompressedFile,
timeFormat: "unix",
outputContent: "metric_name,timestamp,metric_value,check,error,extra_tags\n" + "my_metric,1562324643,1.000000,val1,val3,url=val2\n" + "my_metric,1562324644,1.000000,val1,val3,name=val4&url=val2\n",
},
{
samples: []metrics.SampleContainer{
metrics.Sample{
Time: time.Unix(1562324644, 0),
Metric: testMetric,
Value: 1,
Tags: metrics.NewSampleTags(map[string]string{
"check": "val1",
"url": "val2",
"error": "val3",
}),
},
metrics.Sample{
Time: time.Unix(1562324644, 0),
Metric: testMetric,
Value: 1,
Tags: metrics.NewSampleTags(map[string]string{
"check": "val1",
"url": "val2",
"error": "val3",
"name": "val4",
}),
},
},
fileName: "test",
fileReaderFunc: readUnCompressedFile,
timeFormat: "rfc3399",
outputContent: "metric_name,timestamp,metric_value,check,error,extra_tags\n" + "my_metric,2019-07-05T11:04:04Z,1.000000,val1,val3,url=val2\n" + "my_metric,2019-07-05T11:04:04Z,1.000000,val1,val3,name=val4&url=val2\n",
},
}

for _, data := range testData {
Expand All @@ -295,6 +332,9 @@ func TestRun(t *testing.T) {
SystemTags: metrics.NewSystemTagSet(metrics.TagError | metrics.TagCheck),
},
})

output.timeFormat = TimeFormat(data.timeFormat)

require.NoError(t, err)
require.NotNil(t, output)

Expand Down