Skip to content

Commit

Permalink
Ensure thresholds parsing is skipped on --no-thresholds
Browse files Browse the repository at this point in the history
This commit ensures that thresholds parsing is a separate step, done
only when the `--no-threshold` flag is not set.

Note that this commit deactivates tests that assumed
`Thresholds.UnmarshalJSON` to effectively parse the thresholds.
  • Loading branch information
oleiade committed Feb 25, 2022
1 parent 7a9d184 commit 985d033
Show file tree
Hide file tree
Showing 5 changed files with 137 additions and 68 deletions.
10 changes: 10 additions & 0 deletions cmd/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,16 @@ a commandline interface for interacting with it.`,
return err
}

// Parse the thresholds, only if the --no-threshold flag is not set.
if !runtimeOptions.NoThresholds.Bool {
for _, thresholds := range conf.Options.Thresholds {
err := thresholds.Parse()
if err != nil {
return errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig)
}
}
}

conf, err = deriveAndValidateConfig(conf, initRunner.IsExecutable, logger)
if err != nil {
return err
Expand Down
12 changes: 4 additions & 8 deletions core/engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,7 @@ func TestEngine_processSamples(t *testing.T) {
})
t.Run("submetric", func(t *testing.T) {
t.Parallel()
ths, err := stats.NewThresholds([]string{`value<2`})
assert.NoError(t, err)
ths := stats.NewThresholds([]string{`value<2`})

e, _, wait := newTestEngine(t, nil, nil, nil, lib.Options{
Thresholds: map[string]stats.Thresholds{
Expand Down Expand Up @@ -294,8 +293,7 @@ func TestEngineThresholdsWillAbort(t *testing.T) {
// The incoming samples for the metric set it to 1.25. Considering
// the metric is of type Gauge, value > 1.25 should always fail, and
// trigger an abort.
ths, err := stats.NewThresholds([]string{"value>1.25"})
assert.NoError(t, err)
ths := stats.NewThresholds([]string{"value>1.25"})
ths.Thresholds[0].AbortOnFail = true

thresholds := map[string]stats.Thresholds{metric.Name: ths}
Expand All @@ -317,8 +315,7 @@ func TestEngineAbortedByThresholds(t *testing.T) {
// the metric is of type Gauge, value > 1.25 should always fail, and
// trigger an abort.
// **N.B**: a threshold returning an error, won't trigger an abort.
ths, err := stats.NewThresholds([]string{"value>1.25"})
assert.NoError(t, err)
ths := stats.NewThresholds([]string{"value>1.25"})
ths.Thresholds[0].AbortOnFail = true

thresholds := map[string]stats.Thresholds{metric.Name: ths}
Expand Down Expand Up @@ -373,8 +370,7 @@ func TestEngine_processThresholds(t *testing.T) {
t.Parallel()
thresholds := make(map[string]stats.Thresholds, len(data.ths))
for m, srcs := range data.ths {
ths, err := stats.NewThresholds(srcs)
assert.NoError(t, err)
ths := stats.NewThresholds(srcs)
ths.Thresholds[0].AbortOnFail = data.abort
thresholds[m] = ths
}
Expand Down
3 changes: 1 addition & 2 deletions output/json/json_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,7 @@ func setThresholds(t *testing.T, out output.Output) {
jout, ok := out.(*Output)
require.True(t, ok)

ts, err := stats.NewThresholds([]string{"rate<0.01", "p(99)<250"})
require.NoError(t, err)
ts := stats.NewThresholds([]string{"rate<0.01", "p(99)<250"})

jout.SetThresholds(map[string]stats.Thresholds{"my_metric1": ts})
}
51 changes: 29 additions & 22 deletions stats/thresholds.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,18 +44,13 @@ type Threshold struct {
parsed *thresholdExpression
}

func newThreshold(src string, abortOnFail bool, gracePeriod types.NullDuration) (*Threshold, error) {
parsedExpression, err := parseThresholdExpression(src)
if err != nil {
return nil, err
}

func newThreshold(src string, abortOnFail bool, gracePeriod types.NullDuration) *Threshold {
return &Threshold{
Source: src,
AbortOnFail: abortOnFail,
AbortGracePeriod: gracePeriod,
parsed: parsedExpression,
}, nil
parsed: nil,
}
}

func (t *Threshold) runNoTaint(sinks map[string]float64) (bool, error) {
Expand Down Expand Up @@ -143,7 +138,7 @@ type Thresholds struct {
}

// NewThresholds returns Thresholds objects representing the provided source strings
func NewThresholds(sources []string) (Thresholds, error) {
func NewThresholds(sources []string) Thresholds {
tcs := make([]thresholdConfig, len(sources))
for i, source := range sources {
tcs[i].Threshold = source
Expand All @@ -152,19 +147,16 @@ func NewThresholds(sources []string) (Thresholds, error) {
return newThresholdsWithConfig(tcs)
}

func newThresholdsWithConfig(configs []thresholdConfig) (Thresholds, error) {
func newThresholdsWithConfig(configs []thresholdConfig) Thresholds {
thresholds := make([]*Threshold, len(configs))
sinked := make(map[string]float64)

for i, config := range configs {
t, err := newThreshold(config.Threshold, config.AbortOnFail, config.AbortGracePeriod)
if err != nil {
return Thresholds{}, fmt.Errorf("threshold %d error: %w", i, err)
}
t := newThreshold(config.Threshold, config.AbortOnFail, config.AbortGracePeriod)
thresholds[i] = t
}

return Thresholds{thresholds, false, sinked}, nil
return Thresholds{thresholds, false, sinked}
}

func (ts *Thresholds) runAll(timeSpentInTest time.Duration) (bool, error) {
Expand Down Expand Up @@ -237,17 +229,30 @@ func (ts *Thresholds) Run(sink Sink, duration time.Duration) (bool, error) {
return ts.runAll(duration)
}

// Parse parses the Thresholds and fills each Threshold.parsed field with the result.
// It effectively asserts they are syntaxically correct.
func (ts *Thresholds) Parse() error {
for _, t := range ts.Thresholds {
parsed, err := parseThresholdExpression(t.Source)
if err != nil {
return err
}

t.parsed = parsed
}

return nil
}

// UnmarshalJSON is implementation of json.Unmarshaler
func (ts *Thresholds) UnmarshalJSON(data []byte) error {
var configs []thresholdConfig
if err := json.Unmarshal(data, &configs); err != nil {
return err
}
newts, err := newThresholdsWithConfig(configs)
if err != nil {
return err
}
*ts = newts

*ts = newThresholdsWithConfig(configs)

return nil
}

Expand Down Expand Up @@ -279,5 +284,7 @@ func MarshalJSONWithoutHTMLEscape(t interface{}) ([]byte, error) {
return bytes, err
}

var _ json.Unmarshaler = &Thresholds{}
var _ json.Marshaler = &Thresholds{}
var (
_ json.Unmarshaler = &Thresholds{}
_ json.Marshaler = &Thresholds{}
)
129 changes: 93 additions & 36 deletions stats/thresholds_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.k6.io/k6/lib/types"
"gopkg.in/guregu/null.v3"
)
Expand All @@ -37,29 +36,14 @@ func TestNewThreshold(t *testing.T) {
src := `rate<0.01`
abortOnFail := false
gracePeriod := types.NullDurationFrom(2 * time.Second)
wantParsed := &thresholdExpression{tokenRate, null.Float{}, tokenLess, 0.01}

gotThreshold, err := newThreshold(src, abortOnFail, gracePeriod)
gotThreshold := newThreshold(src, abortOnFail, gracePeriod)

assert.NoError(t, err)
assert.Equal(t, src, gotThreshold.Source)
assert.False(t, gotThreshold.LastFailed)
assert.Equal(t, abortOnFail, gotThreshold.AbortOnFail)
assert.Equal(t, gracePeriod, gotThreshold.AbortGracePeriod)
assert.Equal(t, wantParsed, gotThreshold.parsed)
}

func TestNewThreshold_InvalidThresholdConditionExpression(t *testing.T) {
t.Parallel()

src := "1+1==2"
abortOnFail := false
gracePeriod := types.NullDurationFrom(2 * time.Second)

gotThreshold, err := newThreshold(src, abortOnFail, gracePeriod)

assert.Error(t, err, "instantiating a threshold with an invalid expression should fail")
assert.Nil(t, gotThreshold, "instantiating a threshold with an invalid expression should return a nil Threshold")
assert.Nil(t, gotThreshold.parsed)
}

func TestThreshold_runNoTaint(t *testing.T) {
Expand Down Expand Up @@ -219,8 +203,7 @@ func TestThresholdRun(t *testing.T) {
t.Parallel()

sinks := map[string]float64{"rate": 0.0001}
threshold, err := newThreshold(`rate<0.01`, false, types.NullDuration{})
assert.NoError(t, err)
threshold := newThreshold(`rate<0.01`, false, types.NullDuration{})

t.Run("no taint", func(t *testing.T) {
b, err := threshold.runNoTaint(sinks)
Expand All @@ -243,8 +226,7 @@ func TestThresholdRun(t *testing.T) {
t.Parallel()

sinks := map[string]float64{"rate": 1}
threshold, err := newThreshold(`rate<0.01`, false, types.NullDuration{})
assert.NoError(t, err)
threshold := newThreshold(`rate<0.01`, false, types.NullDuration{})

t.Run("no taint", func(t *testing.T) {
b, err := threshold.runNoTaint(sinks)
Expand All @@ -262,22 +244,103 @@ func TestThresholdRun(t *testing.T) {
})
}

func TestThresholdsParse(t *testing.T) {
t.Parallel()

t.Run("valid threshold expressions", func(t *testing.T) {
t.Parallel()

// Prepare a Thresholds collection containing syntaxically
// correct thresholds
ts := Thresholds{
Thresholds: []*Threshold{
newThreshold("rate<1", false, types.NullDuration{}),
},
}

// Collect the result of the parsing operation
gotErr := ts.Parse()

assert.NoError(t, gotErr, "Parse shouldn't fail parsing valid expressions")
assert.Condition(t, func() bool {
for _, threshold := range ts.Thresholds {
if threshold.parsed == nil {
return false
}
}

return true
}, "Parse did not fail, but some Thresholds' parsed field is left empty")
})

t.Run("invalid threshold expressions", func(t *testing.T) {
t.Parallel()

// Prepare a Thresholds collection containing syntaxically
// correct thresholds
ts := Thresholds{
Thresholds: []*Threshold{
newThreshold("foo&1", false, types.NullDuration{}),
},
}

// Collect the result of the parsing operation
gotErr := ts.Parse()

assert.Error(t, gotErr, "Parse should fail parsing invalid expressions")
assert.Condition(t, func() bool {
for _, threshold := range ts.Thresholds {
if threshold.parsed == nil {
return true
}
}

return false
}, "Parse failed, but some Thresholds' parsed field was not empty")
})

t.Run("mixed valid/invalid threshold expressions", func(t *testing.T) {
t.Parallel()

// Prepare a Thresholds collection containing syntaxically
// correct thresholds
ts := Thresholds{
Thresholds: []*Threshold{
newThreshold("rate<1", false, types.NullDuration{}),
newThreshold("foo&1", false, types.NullDuration{}),
},
}

// Collect the result of the parsing operation
gotErr := ts.Parse()

assert.Error(t, gotErr, "Parse should fail parsing invalid expressions")
assert.Condition(t, func() bool {
for _, threshold := range ts.Thresholds {
if threshold.parsed == nil {
return true
}
}

return false
}, "Parse failed, but some Thresholds' parsed field was not empty")
})
}

func TestNewThresholds(t *testing.T) {
t.Parallel()

t.Run("empty", func(t *testing.T) {
t.Parallel()

ts, err := NewThresholds([]string{})
assert.NoError(t, err)
ts := NewThresholds([]string{})
assert.Len(t, ts.Thresholds, 0)
})
t.Run("two", func(t *testing.T) {
t.Parallel()

sources := []string{`rate<0.01`, `p(95)<200`}
ts, err := NewThresholds(sources)
assert.NoError(t, err)
ts := NewThresholds(sources)
assert.Len(t, ts.Thresholds, 2)
for i, th := range ts.Thresholds {
assert.Equal(t, sources[i], th.Source)
Expand All @@ -293,8 +356,7 @@ func TestNewThresholdsWithConfig(t *testing.T) {
t.Run("empty", func(t *testing.T) {
t.Parallel()

ts, err := NewThresholds([]string{})
assert.NoError(t, err)
ts := NewThresholds([]string{})
assert.Len(t, ts.Thresholds, 0)
})
t.Run("two", func(t *testing.T) {
Expand All @@ -304,8 +366,7 @@ func TestNewThresholdsWithConfig(t *testing.T) {
{`rate<0.01`, false, types.NullDuration{}},
{`p(95)<200`, true, types.NullDuration{}},
}
ts, err := newThresholdsWithConfig(configs)
assert.NoError(t, err)
ts := newThresholdsWithConfig(configs)
assert.Len(t, ts.Thresholds, 2)
for i, th := range ts.Thresholds {
assert.Equal(t, configs[i].Threshold, th.Source)
Expand Down Expand Up @@ -342,15 +403,13 @@ func TestThresholdsRunAll(t *testing.T) {
t.Run(name, func(t *testing.T) {
t.Parallel()

thresholds, err := NewThresholds(data.sources)
thresholds := NewThresholds(data.sources)
thresholds.sinked = map[string]float64{"rate": 0.0001, "p(95)": 500}
thresholds.Thresholds[0].AbortOnFail = data.abort
thresholds.Thresholds[0].AbortGracePeriod = data.grace

runDuration := 1500 * time.Millisecond

assert.NoError(t, err)

succeeded, err := thresholds.runAll(runDuration)

if data.err {
Expand Down Expand Up @@ -411,8 +470,7 @@ func TestThresholds_Run(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()

thresholds, err := NewThresholds([]string{"p(95)<2000"})
require.NoError(t, err, "Initializing new thresholds should not fail")
thresholds := NewThresholds([]string{"p(95)<2000"})

gotOk, gotErr := thresholds.Run(testCase.args.sink, testCase.args.duration)
assert.Equal(t, gotErr != nil, testCase.wantErr, "Thresholds.Run() error = %v, wantErr %v", gotErr, testCase.wantErr)
Expand Down Expand Up @@ -536,7 +594,6 @@ func TestThresholdsJSON(t *testing.T) {
t.Parallel()

var ts Thresholds
assert.Error(t, json.Unmarshal([]byte(`["="]`), &ts))
assert.Nil(t, ts.Thresholds)
assert.False(t, ts.Abort)
})
Expand Down

0 comments on commit 985d033

Please sign in to comment.