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

Unify duration configuration values #1738

Merged
merged 1 commit into from
Dec 1, 2020
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
2 changes: 2 additions & 0 deletions cmd/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ func getNullInt64(flags *pflag.FlagSet, key string) null.Int {
}

func getNullDuration(flags *pflag.FlagSet, key string) types.NullDuration {
// TODO: use types.ParseExtendedDuration? not sure we should support
// unitless durations (i.e. milliseconds) here...
v, err := flags.GetDuration(key)
if err != nil {
panic(err)
Expand Down
29 changes: 28 additions & 1 deletion cmd/config_consolidation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,9 +204,11 @@ func getConfigConsolidationTestCases() []configConsolidationTestCase {
// Verify some CLI errors
{opts{cli: []string{"--blah", "blah"}}, exp{cliParseError: true}, nil},
{opts{cli: []string{"--duration", "blah"}}, exp{cliParseError: true}, nil},
{opts{cli: []string{"--duration", "1000"}}, exp{cliParseError: true}, nil}, // intentionally unsupported
{opts{cli: []string{"--iterations", "blah"}}, exp{cliParseError: true}, nil},
{opts{cli: []string{"--execution", ""}}, exp{cliParseError: true}, nil},
{opts{cli: []string{"--stage", "10:20s"}}, exp{cliReadError: true}, nil},
{opts{cli: []string{"--stage", "1000:20"}}, exp{cliReadError: true}, nil}, // intentionally unsupported
// Check if CLI shortcuts generate correct execution values
{opts{cli: []string{"--vus", "1", "--iterations", "5"}}, exp{}, verifySharedIters(I(1), I(5))},
{opts{cli: []string{"-u", "2", "-i", "6"}}, exp{}, verifySharedIters(I(2), I(6))},
Expand Down Expand Up @@ -249,6 +251,7 @@ func getConfigConsolidationTestCases() []configConsolidationTestCase {
// Test if environment variable shortcuts are working as expected
{opts{env: []string{"K6_VUS=5", "K6_ITERATIONS=15"}}, exp{}, verifySharedIters(I(5), I(15))},
{opts{env: []string{"K6_VUS=10", "K6_DURATION=20s"}}, exp{}, verifyConstLoopingVUs(I(10), 20*time.Second)},
{opts{env: []string{"K6_VUS=10", "K6_DURATION=10000"}}, exp{}, verifyConstLoopingVUs(I(10), 10*time.Second)},
{
opts{env: []string{"K6_STAGES=2m30s:11,1h1m:100"}},
exp{},
Expand All @@ -259,6 +262,7 @@ func getConfigConsolidationTestCases() []configConsolidationTestCase {
exp{},
verifyRampingVUs(null.NewInt(0, true), buildStages(100, 100, 30, 0)),
},
{opts{env: []string{"K6_STAGES=1000:100"}}, exp{consolidationError: true}, nil}, // intentionally unsupported
// Test if JSON configs work as expected
{opts{fs: defaultConfig(`{"iterations": 77, "vus": 7}`)}, exp{}, verifySharedIters(I(7), I(77))},
{opts{fs: defaultConfig(`wrong-json`)}, exp{consolidationError: true}, nil},
Expand All @@ -273,6 +277,12 @@ func getConfigConsolidationTestCases() []configConsolidationTestCase {
cli: []string{"--config", "/my/config.file"},
}, exp{}, verifyConstLoopingVUs(I(8), 120*time.Second),
},
{
opts{
fs: getFS([]file{{"/my/config.file", `{"duration": 20000}`}}),
cli: []string{"--config", "/my/config.file"},
}, exp{}, verifyConstLoopingVUs(null.NewInt(1, false), 20*time.Second),
},
{
opts{
fs: defaultConfig(`{"stages": [{"duration": "20s", "target": 20}], "vus": 10}`),
Expand Down Expand Up @@ -332,7 +342,24 @@ func getConfigConsolidationTestCases() []configConsolidationTestCase {
exp{},
verifySharedIters(I(12), I(25)),
},

{
opts{
fs: defaultConfig(`{"scenarios": { "foo": {
"executor": "constant-vus", "vus": 2, "duration": "1d",
"gracefulStop": "10000", "startTime": 1000.5
}}}`),
}, exp{}, func(t *testing.T, c Config) {
exec := c.Scenarios["foo"]
require.NotEmpty(t, exec)
require.IsType(t, executor.ConstantVUsConfig{}, exec)
clvc, ok := exec.(executor.ConstantVUsConfig)
require.True(t, ok)
assert.Equal(t, null.IntFrom(2), clvc.VUs)
assert.Equal(t, types.NullDurationFrom(24*time.Hour), clvc.Duration)
assert.Equal(t, types.NullDurationFrom(time.Second+500*time.Microsecond), clvc.StartTime)
assert.Equal(t, types.NullDurationFrom(10*time.Second), clvc.GracefulStop)
},
},
// TODO: test the externally controlled executor
// TODO: test execution-segment

Expand Down
29 changes: 7 additions & 22 deletions js/modules/k6/grpc/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import (
"github.com/loadimpact/k6/js/common"
"github.com/loadimpact/k6/lib"
"github.com/loadimpact/k6/lib/metrics"
"github.com/loadimpact/k6/lib/types"
"github.com/loadimpact/k6/stats"
)

Expand Down Expand Up @@ -220,17 +221,9 @@ func (c *Client) Connect(ctxPtr *context.Context, addr string, params map[string
isPlaintext, _ = v.(bool)
case "timeout":
var err error
switch t := v.(type) {
case string:
if timeout, err = time.ParseDuration(t); err != nil {
return false, fmt.Errorf("unable to parse %q: %v", k, err)
}
case int64:
timeout = time.Duration(t) * time.Millisecond
case float64:
timeout = time.Duration(t * float64(time.Millisecond))
default:
return false, fmt.Errorf("unable to use type %T as a timeout value", v)
timeout, err = types.GetDurationValue(v)
if err != nil {
return false, fmt.Errorf("invalid timeout value: %w", err)
}
default:
return false, fmt.Errorf("unknown connect param: %q", k)
Expand Down Expand Up @@ -359,17 +352,9 @@ func (c *Client) Invoke(ctxPtr *context.Context,
}
case "timeout":
var err error
switch t := v.(type) {
case string:
if timeout, err = time.ParseDuration(t); err != nil {
return nil, fmt.Errorf("unable to parse %q: %v", k, err)
}
case int64:
timeout = time.Duration(t) * time.Millisecond
case float64:
timeout = time.Duration(t * float64(time.Millisecond))
default:
return nil, fmt.Errorf("unable to use type %T as a timeout value", v)
timeout, err = types.GetDurationValue(v)
if err != nil {
return nil, fmt.Errorf("invalid timeout value: %w", err)
}
default:
return nil, fmt.Errorf("unknown param: %q", k)
Expand Down
6 changes: 3 additions & 3 deletions js/modules/k6/grpc/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ func TestClient(t *testing.T) {
if !assert.Error(t, err) {
return
}
assert.Contains(t, err.Error(), "unable to parse \"timeout\"")
assert.Contains(t, err.Error(), "invalid duration")
})

t.Run("ConnectStringTimeout", func(t *testing.T) {
Expand Down Expand Up @@ -258,7 +258,7 @@ func TestClient(t *testing.T) {
if !assert.Error(t, err) {
return
}
assert.Contains(t, err.Error(), "unable to use type bool as a timeout value")
assert.Contains(t, err.Error(), "invalid timeout value: unable to use type bool as a duration value")
})

t.Run("InvokeInvalidTimeout", func(t *testing.T) {
Expand All @@ -268,7 +268,7 @@ func TestClient(t *testing.T) {
if !assert.Error(t, err) {
return
}
assert.Contains(t, err.Error(), " unable to parse \"timeout\"")
assert.Contains(t, err.Error(), "invalid duration")
})

t.Run("InvokeStringTimeout", func(t *testing.T) {
Expand Down
7 changes: 6 additions & 1 deletion js/modules/k6/http/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
"github.com/loadimpact/k6/js/common"
"github.com/loadimpact/k6/lib"
"github.com/loadimpact/k6/lib/netext/httpext"
"github.com/loadimpact/k6/lib/types"
)

// ErrHTTPForbiddenInInitContext is used when a http requests was made in the init context
Expand Down Expand Up @@ -335,7 +336,11 @@ func (h *HTTP) parseRequest(
case "auth":
result.Auth = params.Get(k).String()
case "timeout":
result.Timeout = time.Duration(params.Get(k).ToFloat() * float64(time.Millisecond))
t, err := types.GetDurationValue(params.Get(k).Export())
if err != nil {
return nil, fmt.Errorf("invalid timeout value: %w", err)
}
result.Timeout = t
case "throw":
result.Throw = params.Get(k).ToBoolean()
case "responseType":
Expand Down
18 changes: 18 additions & 0 deletions js/modules/k6/http/request_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,24 @@ func TestRequestAndBatch(t *testing.T) {
assert.Contains(t, err.Error(), "context deadline exceeded")
assert.WithinDuration(t, startTime.Add(1*time.Second), endTime, 2*time.Second)

logEntry := hook.LastEntry()
assert.Nil(t, logEntry)
})
t.Run("10s", func(t *testing.T) {
hook := logtest.NewLocal(state.Logger)
defer hook.Reset()

startTime := time.Now()
_, err := common.RunString(rt, sr(`
http.get("HTTPBIN_URL/delay/10", {
timeout: "1s",
})
`))
endTime := time.Now()
require.Error(t, err)
assert.Contains(t, err.Error(), "context deadline exceeded")
assert.WithinDuration(t, startTime.Add(1*time.Second), endTime, 2*time.Second)

logEntry := hook.LastEntry()
assert.Nil(t, logEntry)
})
Expand Down
10 changes: 8 additions & 2 deletions js/modules/k6/ws/ws.go
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,10 @@ func (s *Socket) trackPong(pingID string) {
// timeout, which is in ms, has elapsed
func (s *Socket) SetTimeout(fn goja.Callable, timeoutMs float64) error {
// Starts a goroutine, blocks once on the timeout and pushes the callable
// back to the main loop through the scheduled channel
// back to the main loop through the scheduled channel.
//
// Intentionally not using the generic GetDurationValue() helper, since this
// API is meant to use ms, similar to the original SetTimeout() JS API.
d := time.Duration(timeoutMs * float64(time.Millisecond))
if d <= 0 {
return fmt.Errorf("setTimeout requires a >0 timeout parameter, received %.2f", timeoutMs)
Expand All @@ -416,7 +419,10 @@ func (s *Socket) SetTimeout(fn goja.Callable, timeoutMs float64) error {
// in ms
func (s *Socket) SetInterval(fn goja.Callable, intervalMs float64) error {
// Starts a goroutine, blocks forever on the ticker and pushes the callable
// back to the main loop through the scheduled channel
// back to the main loop through the scheduled channel.
//
// Intentionally not using the generic GetDurationValue() helper, since this
// API is meant to use ms, similar to the original SetInterval() JS API.
d := time.Duration(intervalMs * float64(time.Millisecond))
if d <= 0 {
return fmt.Errorf("setInterval requires a >0 timeout parameter, received %.2f", intervalMs)
Expand Down
2 changes: 1 addition & 1 deletion js/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ func parseTTL(ttlS string) (time.Duration, error) {
fallthrough
default:
var err error
ttl, err = types.ParseExtendedDurationMs(ttlS)
ttl, err = types.ParseExtendedDuration(ttlS)
if ttl < 0 || err != nil {
return ttl, fmt.Errorf("invalid DNS TTL: %s", ttlS)
}
Expand Down
77 changes: 62 additions & 15 deletions lib/types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"bytes"
"encoding/json"
"fmt"
"math"
"reflect"
"strconv"
"strings"
Expand Down Expand Up @@ -97,6 +98,11 @@ func (d Duration) String() string {
// ParseExtendedDuration is a helper function that allows for string duration
// values containing days.
func ParseExtendedDuration(data string) (result time.Duration, err error) {
// Assume millisecond values if data is provided with no units
if t, errp := strconv.ParseFloat(data, 64); errp == nil {
return time.Duration(t * float64(time.Millisecond)), nil
}

dPos := strings.IndexByte(data, 'd')
if dPos < 0 {
return time.ParseDuration(data)
Expand All @@ -123,16 +129,6 @@ func ParseExtendedDuration(data string) (result time.Duration, err error) {
return time.Duration(days)*24*time.Hour + hours, nil
}

// ParseExtendedDurationMs wraps ParseExtendedDuration while assuming
// millisecond values if data is provided with no units.
// TODO: Merge this into ParseExtendedDuration once it's safe to do so globally.
func ParseExtendedDurationMs(data string) (result time.Duration, err error) {
if t, errp := strconv.ParseFloat(data, 32); errp == nil {
data = fmt.Sprintf("%.2fms", t)
}
return ParseExtendedDuration(data)
}

// UnmarshalText converts text data to Duration
func (d *Duration) UnmarshalText(data []byte) error {
v, err := ParseExtendedDuration(string(data))
Expand All @@ -157,12 +153,10 @@ func (d *Duration) UnmarshalJSON(data []byte) error {
}

*d = Duration(v)
} else if t, errp := strconv.ParseFloat(string(data), 64); errp == nil {
*d = Duration(t * float64(time.Millisecond))
} else {
var v time.Duration
if err := json.Unmarshal(data, &v); err != nil {
return err
}
*d = Duration(v)
return fmt.Errorf("'%s' is not a valid duration value", string(data))
}

return nil
Expand Down Expand Up @@ -233,3 +227,56 @@ func (d NullDuration) ValueOrZero() Duration {

return d.Duration
}

func getInt64(v interface{}) (int64, error) {
switch n := v.(type) {
case int:
return int64(n), nil
case int8:
return int64(n), nil
case int16:
return int64(n), nil
case int32:
return int64(n), nil
Comment on lines +233 to +240
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 still baffled that multiple cases don't work for type switches 😕:

This could be much more succinct:

	case int, int8, int16, int32:
	  return int64(n), nil

But I guess there's a good reason for it.

Copy link
Member Author

@na-- na-- Nov 27, 2020

Choose a reason for hiding this comment

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

You can do that, but then n will be of an interface{} type, and you can't directly cast that to int64... You can do it via the reflect package, but that'd be much slower and somewhat type unsafe.

case int64:
return n, nil
case uint:
return int64(n), nil
case uint8:
return int64(n), nil
case uint16:
return int64(n), nil
case uint32:
return int64(n), nil
case uint64:
if n > math.MaxInt64 {
return 0, fmt.Errorf("%d is too big", n)
}
return int64(n), nil
default:
return 0, fmt.Errorf("unable to use type %T as a duration value", v)
}
}

// GetDurationValue is a helper function that can convert a lot of different
// types to time.Duration.
//
// TODO: move to a separate package and check for integer overflows?
func GetDurationValue(v interface{}) (time.Duration, error) {
switch d := v.(type) {
case time.Duration:
return d, nil
case string:
return ParseExtendedDuration(d)
case float32:
return time.Duration(float64(d) * float64(time.Millisecond)), nil
case float64:
return time.Duration(d * float64(time.Millisecond)), nil
default:
n, err := getInt64(v)
Comment on lines +267 to +276
Copy link
Contributor

Choose a reason for hiding this comment

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

Are any beside time.Duration, string, float64 and int64 ... even possible? Because the previous code didn't handle the rest, and I have the feeling that was because we will never actually get one ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added them out of an abundance of caution... I have no idea if some weird webpack minification magic or something like that won't try to encode plain integer values as something that goja will export as uint8 or whatever... Stranger things have happened... 😅 And given that this helper function is intentionally outside of js/, I also think it makes some sense to handle the other Go int native types. This way the function is more complete and can basically convert every possible value we can recognize as a time.Duration to that, no need to second-guess.

if err != nil {
return 0, err
}
return time.Duration(n) * time.Millisecond, nil
}
}
Loading