feat: sumologic timeslice validation [PC-7427] (#238)

Tightens sumo logic log query validation to allow timeslice operator to be used with 15,30 and 60 sec as an argument --------- Co-authored-by: Mateusz Hawrus <[email protected]>
nobl9 · Feb 2, 2024 · ec30bda · ec30bda
1 parent 74d4207
commit ec30bda
Show file tree

Hide file tree

Showing 5 changed files with 175 additions and 46 deletions.
diff --git a/cspell.yaml b/cspell.yaml
@@ -111,6 +111,7 @@ words:
   - stimeslice
   - strconv
   - structs
+  - submatches
   - sumologic
   - testutils
   - timeseries

diff --git a/manifest/v1alpha/slo/metrics_sumo_logic.go b/manifest/v1alpha/slo/metrics_sumo_logic.go
@@ -3,6 +3,7 @@ package slo
 import (
 	"fmt"
 	"regexp"
+	"strings"
 	"time"
 
 	"github.com/pkg/errors"
@@ -44,15 +45,15 @@ var sumoLogicCountMetricsLevelValidation = validation.New[CountMetricsSpec](
 				if *good.Type != "logs" || *total.Type != "logs" {
 					return nil
 				}
-				goodTS, err := getTimeSliceFromSumoLogicQuery(*good.Query)
+				goodTimeSlice, err := getTimeSliceFromSumoLogicQuery(*good.Query)
 				if err != nil {
 					return nil
 				}
-				totalTS, err := getTimeSliceFromSumoLogicQuery(*total.Query)
+				totalTimeSlice, err := getTimeSliceFromSumoLogicQuery(*total.Query)
 				if err != nil {
 					return nil
 				}
-				if goodTS != totalTS {
+				if goodTimeSlice.duration != totalTimeSlice.duration {
 					return errors.Errorf(
 						"'sumologic.query' with segment 'timeslice ${duration}', " +
 							"${duration} must be the same for both 'good' and 'total' metrics")
@@ -107,21 +108,9 @@ var sumoLogicLogsTypeValidation = validation.New[SumoLogicMetric](
 		WithName("query").
 		Required().
 		Rules(
-			validation.NewSingleRule(func(s string) error {
-				const minTimeSliceSeconds = 15
-				timeslice, err := getTimeSliceFromSumoLogicQuery(s)
-				if err != nil {
-					return err
-				}
-				if timeslice.Seconds() < minTimeSliceSeconds {
-					return errors.Errorf("minimum timeslice value is [%ds], got: [%s]", minTimeSliceSeconds, timeslice)
-				}
-				return nil
-			}),
+			validation.NewSingleRule(validateSumoLogicTimeslice),
 			validation.StringMatchRegexp(regexp.MustCompile(`(?m)\bn9_value\b`)).
 				WithDetails("n9_value is required"),
-			validation.StringMatchRegexp(regexp.MustCompile(`(?m)\bn9_time\b`)).
-				WithDetails("n9_time is required"),
 			validation.StringMatchRegexp(regexp.MustCompile(`(?m)\bby\b`)).
 				WithDetails("aggregation function is required"),
 		),
@@ -136,21 +125,54 @@ var sumoLogicLogsTypeValidation = validation.New[SumoLogicMetric](
 		return m.Type != nil && *m.Type == sumoLogicTypeLogs
 	})
 
-var sumoLogicTimeSliceRegexp = regexp.MustCompile(`(?m)\stimeslice\s(\d+\w+)\s`)
+func validateSumoLogicTimeslice(query string) error {
+	timeSlice, err := getTimeSliceFromSumoLogicQuery(query)
+	if err != nil {
+		return err
+	}
 
-func getTimeSliceFromSumoLogicQuery(query string) (time.Duration, error) {
-	subMatches := sumoLogicTimeSliceRegexp.FindAllStringSubmatch(query, 2)
-	if len(subMatches) != 1 {
-		return 0, fmt.Errorf("exactly one timeslice declaration is required in the query")
+	if seconds := int(timeSlice.duration.Seconds()); seconds != 15 && seconds != 30 && seconds != 60 ||
+		strings.HasPrefix(timeSlice.durationStr, "0") || // Sumo Logic doesn't support leading zeros in query body
+		strings.HasPrefix(timeSlice.durationStr, "+") ||
+		strings.HasPrefix(timeSlice.durationStr, "-") ||
+		strings.HasSuffix(timeSlice.durationStr, "ms") {
+		return errors.Errorf("timeslice value must be 15, 30, or 60 seconds, got: [%s]", timeSlice.durationStr)
 	}
-	subMatch := subMatches[0]
-	if len(subMatch) != 2 {
-		return 0, fmt.Errorf("timeslice declaration must match regular expression: %s", sumoLogicTimeSliceRegexp)
+
+	if !timeSlice.containsAlias {
+		return errors.New("timeslice operator requires an n9_time alias")
 	}
+	return nil
+}
+
+type parsedSumoLogicSlice struct {
+	containsAlias bool
+	duration      time.Duration
+	durationStr   string
+}
+
+func getTimeSliceFromSumoLogicQuery(query string) (parsedSumoLogicSlice, error) {
+	r := regexp.MustCompile(`\stimeslice\s([-+]?(\d+[a-z]+\s?)+)\s(?:as n9_time)?`)
+	matchResults := r.FindAllStringSubmatch(query, 2)
+	if len(matchResults) == 0 {
+		return parsedSumoLogicSlice{}, errors.New("query must contain a 'timeslice' operator")
+	}
+	if len(matchResults) > 1 {
+		return parsedSumoLogicSlice{}, errors.New("exactly one 'timeslice' usage is required in the query")
+	}
+	submatches := matchResults[0]
+
+	if submatches[1] != submatches[2] {
+		return parsedSumoLogicSlice{}, errors.New("timeslice interval must be in a NumberUnit form - for example '30s'")
+	}
+
 	// https://help.sumologic.com/05Search/Search-Query-Language/Search-Operators/timeslice#syntax
-	timeslice, err := time.ParseDuration(subMatch[1])
+	durationString := strings.TrimSpace(submatches[1])
+	containsAlias := strings.Contains(submatches[0][1:], "as n9_time")
+	tsDuration, err := time.ParseDuration(durationString)
 	if err != nil {
-		return 0, fmt.Errorf("error parsing timeslice duration: %s", err.Error())
+		return parsedSumoLogicSlice{}, fmt.Errorf("error parsing timeslice duration: %s", err.Error())
 	}
-	return timeslice, nil
+
+	return parsedSumoLogicSlice{duration: tsDuration, durationStr: durationString, containsAlias: containsAlias}, nil
 }
diff --git a/manifest/v1alpha/slo/metrics_sumo_logic_test.go b/manifest/v1alpha/slo/metrics_sumo_logic_test.go
@@ -36,7 +36,7 @@ func TestSumoLogic_CountMetricsLevel(t *testing.T) {
 			Query: ptr(`
 _collector="n9-dev-tooling-cluster" _source="logs"
   | json "log"
-  | timeslice 20s as n9_time
+  | timeslice 15s as n9_time
   | parse "level=* *" as (log_level, tail)
   | if (log_level matches "error" ,0,1) as log_level_not_error
   | sum(log_level_not_error) as n9_value by n9_time
@@ -47,7 +47,7 @@ _collector="n9-dev-tooling-cluster" _source="logs"
 			Query: ptr(`
 _collector="n9-dev-tooling-cluster" _source="logs"
   | json "log"
-  | timeslice 25s as n9_time
+  | timeslice 30s as n9_time
   | parse "level=* *" as (log_level, tail)
   | if (log_level matches "error" ,0,1) as log_level_not_error
   | sum(log_level_not_error) as n9_value by n9_time
@@ -153,7 +153,7 @@ func TestSumoLogic_LogsType(t *testing.T) {
 			Query: ptr(`
 _collector="n9-dev-tooling-cluster" _source="logs"
   | json "log"
-  | timeslice 20s as n9_time
+  | timeslice 15s as n9_time
   | parse "level=* *" as (log_level, tail)
   | if (log_level matches "error" ,0,1) as log_level_not_error
   | sum(log_level_not_error) as n9_value by n9_time
@@ -173,7 +173,7 @@ _collector="n9-dev-tooling-cluster" _source="logs"
 			},
 		)
 	})
-	tests := map[string]struct {
+	invalidCases := map[string]struct {
 		Query string
 		Error testutils.ExpectedError
 	}{
@@ -187,22 +187,78 @@ _collector="n9-dev-tooling-cluster" _source="logs"
   | sort by n9_time asc`,
 			Error: testutils.ExpectedError{
 				Prop:    "spec.objectives[0].rawMetric.query.sumoLogic.query",
-				Message: "exactly one timeslice declaration is required in the query",
+				Message: "query must contain a 'timeslice' operator",
 			},
 		},
 		"two timeslice segments": {
 			Query: `
 _collector="n9-dev-tooling-cluster" _source="logs"
   | json "log"
-  | timeslice 30s
-  | timeslice 20s as n9_time
+  | timeslice 30s as n9_time
+  | timeslice 15s as n9_time
+  | parse "level=* *" as (log_level, tail)
+  | if (log_level matches "error" ,0,1) as log_level_not_error
+  | sum(log_level_not_error) as n9_value by n9_time
+  | sort by n9_time asc`,
+			Error: testutils.ExpectedError{
+				Prop:    "spec.objectives[0].rawMetric.query.sumoLogic.query",
+				Message: "exactly one 'timeslice' usage is required in the query",
+			},
+		},
+		"leading zeros in timeslice value": {
+			Query: `
+_collector="n9-dev-tooling-cluster" _source="logs"
+  | json "log"
+  | timeslice 015s as n9_time
+  | parse "level=* *" as (log_level, tail)
+  | if (log_level matches "error" ,0,1) as log_level_not_error
+  | sum(log_level_not_error) as n9_value by n9_time
+  | sort by n9_time asc`,
+			Error: testutils.ExpectedError{
+				Prop:    "spec.objectives[0].rawMetric.query.sumoLogic.query",
+				Message: "timeslice value must be 15, 30, or 60 seconds, got: [015s]",
+			},
+		},
+		"+ sign in timeslice value": {
+			Query: `
+_collector="n9-dev-tooling-cluster" _source="logs"
+  | json "log"
+  | timeslice +15s as n9_time
+  | parse "level=* *" as (log_level, tail)
+  | if (log_level matches "error" ,0,1) as log_level_not_error
+  | sum(log_level_not_error) as n9_value by n9_time
+  | sort by n9_time asc`,
+			Error: testutils.ExpectedError{
+				Prop:    "spec.objectives[0].rawMetric.query.sumoLogic.query",
+				Message: "timeslice interval must be in a NumberUnit form - for example '30s'",
+			},
+		},
+		"- sign in timeslice value": {
+			Query: `
+_collector="n9-dev-tooling-cluster" _source="logs"
+  | json "log"
+  | timeslice -15s as n9_time
+  | parse "level=* *" as (log_level, tail)
+  | if (log_level matches "error" ,0,1) as log_level_not_error
+  | sum(log_level_not_error) as n9_value by n9_time
+  | sort by n9_time asc`,
+			Error: testutils.ExpectedError{
+				Prop:    "spec.objectives[0].rawMetric.query.sumoLogic.query",
+				Message: "timeslice interval must be in a NumberUnit form - for example '30s'",
+			},
+		},
+		"milliseconds in timeslice value": {
+			Query: `
+_collector="n9-dev-tooling-cluster" _source="logs"
+  | json "log"
+  | timeslice 15000ms as n9_time
   | parse "level=* *" as (log_level, tail)
   | if (log_level matches "error" ,0,1) as log_level_not_error
   | sum(log_level_not_error) as n9_value by n9_time
   | sort by n9_time asc`,
 			Error: testutils.ExpectedError{
 				Prop:    "spec.objectives[0].rawMetric.query.sumoLogic.query",
-				Message: "exactly one timeslice declaration is required in the query",
+				Message: "timeslice value must be 15, 30, or 60 seconds, got: [15000ms]",
 			},
 		},
 		"invalid timeslice segment": {
@@ -219,7 +275,7 @@ _collector="n9-dev-tooling-cluster" _source="logs"
 				Message: `error parsing timeslice duration: time: unknown unit "x" in duration "20x"`,
 			},
 		},
-		"minimum timeslice value": {
+		"unsupported timeslice value": {
 			Query: `
 _collector="n9-dev-tooling-cluster" _source="logs"
   | json "log"
@@ -230,14 +286,14 @@ _collector="n9-dev-tooling-cluster" _source="logs"
   | sort by n9_time asc`,
 			Error: testutils.ExpectedError{
 				Prop:    "spec.objectives[0].rawMetric.query.sumoLogic.query",
-				Message: `minimum timeslice value is [15s], got: [14s]`,
+				Message: `timeslice value must be 15, 30, or 60 seconds, got: [14s]`,
 			},
 		},
 		"missing n9_value": {
 			Query: `
 _collector="n9-dev-tooling-cluster" _source="logs"
   | json "log"
-  | timeslice 20s as n9_time
+  | timeslice 15s as n9_time
   | parse "level=* *" as (log_level, tail)
   | if (log_level matches "error" ,0,1) as log_level_not_error
   | sum(log_level_not_error) by n9_time
@@ -247,25 +303,25 @@ _collector="n9-dev-tooling-cluster" _source="logs"
 				ContainsMessage: "n9_value is required",
 			},
 		},
-		"missing n9_time": {
+		"missing n9_time alias": {
 			Query: `
 _collector="n9-dev-tooling-cluster" _source="logs"
   | json "log"
-  | timeslice 20s
+  | timeslice 30s
   | parse "level=* *" as (log_level, tail)
   | if (log_level matches "error" ,0,1) as log_level_not_error
   | sum(log_level_not_error) as n9_value by time
   | sort by time asc`,
 			Error: testutils.ExpectedError{
 				Prop:            "spec.objectives[0].rawMetric.query.sumoLogic.query",
-				ContainsMessage: "n9_time is required",
+				ContainsMessage: "timeslice operator requires an n9_time alias",
 			},
 		},
 		"missing aggregation function": {
 			Query: `
 _collector="n9-dev-tooling-cluster" _source="logs"
   | json "log"
-  | timeslice 20s as n9_time
+  | timeslice 15s as n9_time
   | parse "level=* *" as (log_level, tail)
   | if (log_level matches "error" ,0,1) as log_level_not_error
   | sum(log_level_not_error) as n9_value`,
@@ -275,7 +331,7 @@ _collector="n9-dev-tooling-cluster" _source="logs"
 			},
 		},
 	}
-	for name, test := range tests {
+	for name, test := range invalidCases {
 		t.Run(name, func(t *testing.T) {
 			slo := validRawMetricSLO(v1alpha.SumoLogic)
 			slo.Spec.Objectives[0].RawMetric.MetricQuery.SumoLogic = &SumoLogicMetric{
@@ -286,4 +342,50 @@ _collector="n9-dev-tooling-cluster" _source="logs"
 			testutils.AssertContainsErrors(t, slo, err, 1, test.Error)
 		})
 	}
+	validCases := map[string]struct {
+		Query string
+		Error testutils.ExpectedError
+	}{
+		"valid timeslice [15s]": {
+			Query: `
+_collector="n9-dev-tooling-cluster" _source="logs"
+  | json "log"
+  | timeslice 15s as n9_time
+  | parse "level=* *" as (log_level, tail)
+  | if (log_level matches "error" ,0,1) as log_level_not_error
+  | sum(log_level_not_error) as n9_value by n9_time
+  | sort by n9_time asc`,
+		},
+		"valid timeslice [30s]": {
+			Query: `
+_collector="n9-dev-tooling-cluster" _source="logs"
+  | json "log"
+  | timeslice 30s as n9_time
+  | parse "level=* *" as (log_level, tail)
+  | if (log_level matches "error" ,0,1) as log_level_not_error
+  | sum(log_level_not_error) as n9_value by n9_time
+  | sort by n9_time asc`,
+		},
+		"valid timeslice [60s]": {
+			Query: `
+_collector="n9-dev-tooling-cluster" _source="logs"
+  | json "log"
+  | timeslice 60s as n9_time
+  | parse "level=* *" as (log_level, tail)
+  | if (log_level matches "error" ,0,1) as log_level_not_error
+  | sum(log_level_not_error) as n9_value by n9_time
+  | sort by n9_time asc`,
+		},
+	}
+	for name, test := range validCases {
+		t.Run(name, func(t *testing.T) {
+			slo := validRawMetricSLO(v1alpha.SumoLogic)
+			slo.Spec.Objectives[0].RawMetric.MetricQuery.SumoLogic = &SumoLogicMetric{
+				Type:  ptr(sumoLogicTypeLogs),
+				Query: ptr(test.Query),
+			}
+			err := validate(slo)
+			testutils.AssertNoError(t, slo, err)
+		})
+	}
 }
diff --git a/sdk/test_data/client/simple_module/go.mod b/sdk/test_data/client/simple_module/go.mod
@@ -8,7 +8,7 @@ require github.com/nobl9/nobl9-go v0.73.0
 
 require (
 	github.com/BurntSushi/toml v1.3.2 // indirect
-	github.com/aws/aws-sdk-go v1.49.16 // indirect
+	github.com/aws/aws-sdk-go v1.50.3 // indirect
 	github.com/bmatcuk/doublestar/v4 v4.6.1 // indirect
 	github.com/decred/dcrd/dcrec/secp256k1/v4 v4.2.0 // indirect
 	github.com/fatih/color v1.15.0 // indirect
@@ -27,7 +27,7 @@ require (
 	github.com/lestrrat-go/blackmagic v1.0.2 // indirect
 	github.com/lestrrat-go/httpcc v1.0.1 // indirect
 	github.com/lestrrat-go/iter v1.0.2 // indirect
-	github.com/lestrrat-go/jwx v1.2.27 // indirect
+	github.com/lestrrat-go/jwx v1.2.28 // indirect
 	github.com/lestrrat-go/option v1.0.1 // indirect
 	github.com/mattn/go-colorable v0.1.13 // indirect
 	github.com/mattn/go-isatty v0.0.19 // indirect
@@ -36,7 +36,7 @@ require (
 	github.com/pkg/errors v0.9.1 // indirect
 	github.com/rs/zerolog v1.31.0 // indirect
 	golang.org/x/crypto v0.17.0 // indirect
-	golang.org/x/exp v0.0.0-20240103183307-be819d1f06fc // indirect
+	golang.org/x/exp v0.0.0-20240112132812-db7319d0e0e3 // indirect
 	golang.org/x/net v0.17.0 // indirect
 	golang.org/x/sys v0.15.0 // indirect
 	golang.org/x/text v0.14.0 // indirect