diff --git a/cmd/run.go b/cmd/run.go index 958857248097..f06a8f00cc8c 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -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 diff --git a/core/engine_test.go b/core/engine_test.go index 8b9224ab7d17..685faa94d18e 100644 --- a/core/engine_test.go +++ b/core/engine_test.go @@ -263,8 +263,9 @@ 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`}) + gotParseErr := ths.Parse() + require.NoError(t, gotParseErr) e, _, wait := newTestEngine(t, nil, nil, nil, lib.Options{ Thresholds: map[string]stats.Thresholds{ @@ -294,8 +295,9 @@ 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"}) + gotParseErr := ths.Parse() + require.NoError(t, gotParseErr) ths.Thresholds[0].AbortOnFail = true thresholds := map[string]stats.Thresholds{metric.Name: ths} @@ -317,8 +319,9 @@ 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"}) + gotParseErr := ths.Parse() + require.NoError(t, gotParseErr) ths.Thresholds[0].AbortOnFail = true thresholds := map[string]stats.Thresholds{metric.Name: ths} @@ -373,8 +376,9 @@ 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) + gotParseErr := ths.Parse() + require.NoError(t, gotParseErr) ths.Thresholds[0].AbortOnFail = data.abort thresholds[m] = ths } diff --git a/output/json/json_test.go b/output/json/json_test.go index c788006d8b3d..e38a3ebe47b2 100644 --- a/output/json/json_test.go +++ b/output/json/json_test.go @@ -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}) } diff --git a/stats/thresholds.go b/stats/thresholds.go index 1b7cbaea53fa..3b513dc57b04 100644 --- a/stats/thresholds.go +++ b/stats/thresholds.go @@ -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) { @@ -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 @@ -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) { @@ -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 } @@ -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{} +) diff --git a/stats/thresholds_test.go b/stats/thresholds_test.go index 9e381bf75fb8..91ff5cf89c4f 100644 --- a/stats/thresholds_test.go +++ b/stats/thresholds_test.go @@ -37,29 +37,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) { @@ -219,8 +204,10 @@ 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) + parsed, parseErr := parseThresholdExpression("rate<0.01") + require.NoError(t, parseErr) + threshold := newThreshold(`rate<0.01`, false, types.NullDuration{}) + threshold.parsed = parsed t.Run("no taint", func(t *testing.T) { b, err := threshold.runNoTaint(sinks) @@ -243,8 +230,10 @@ 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) + parsed, parseErr := parseThresholdExpression("rate<0.01") + require.NoError(t, parseErr) + threshold := newThreshold(`rate<0.01`, false, types.NullDuration{}) + threshold.parsed = parsed t.Run("no taint", func(t *testing.T) { b, err := threshold.runNoTaint(sinks) @@ -262,22 +251,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) @@ -293,8 +363,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) { @@ -304,8 +373,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) @@ -342,15 +410,15 @@ func TestThresholdsRunAll(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() - thresholds, err := NewThresholds(data.sources) + thresholds := NewThresholds(data.sources) + gotParseErr := thresholds.Parse() + require.NoError(t, gotParseErr) 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 { @@ -411,8 +479,9 @@ 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"}) + gotParseErr := thresholds.Parse() + require.NoError(t, gotParseErr) 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) @@ -536,7 +605,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) })