From f5cc1cbd7f8ebb8490f025060c0621f2e47acddf Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Sun, 20 Feb 2022 17:40:39 +0200 Subject: [PATCH 1/6] Add support for Prometheus like duration units in logql --- pkg/logql/ast_test.go | 4 ++-- pkg/logql/lex.go | 14 ++++++++++---- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/pkg/logql/ast_test.go b/pkg/logql/ast_test.go index e49648fb0772..1f38522e4249 100644 --- a/pkg/logql/ast_test.go +++ b/pkg/logql/ast_test.go @@ -66,7 +66,7 @@ func Test_SampleExpr_String(t *testing.T) { for _, tc := range []string{ `rate( ( {job="mysql"} |="error" !="timeout" ) [10s] )`, `absent_over_time( ( {job="mysql"} |="error" !="timeout" ) [10s] )`, - `absent_over_time( ( {job="mysql"} |="error" !="timeout" ) [10s] offset 10m )`, + `absent_over_time( ( {job="mysql"} |="error" !="timeout" ) [10s] offset 10d )`, `sum without(a) ( rate ( ( {job="mysql"} |="error" !="timeout" ) [10s] ) )`, `sum by(a) (rate( ( {job="mysql"} |="error" !="timeout" ) [10s] ) )`, `sum(count_over_time({job="mysql"}[5m]))`, @@ -78,7 +78,7 @@ func Test_SampleExpr_String(t *testing.T) { `sum(count_over_time({job="mysql"} | pattern " bar " | json [5m]))`, `sum(count_over_time({job="mysql"} | unpack | json [5m]))`, `sum(count_over_time({job="mysql"} | regexp "(?Pfoo|bar)" [5m]))`, - `sum(count_over_time({job="mysql"} | regexp "(?Pfoo|bar)" [5m] offset 10m))`, + `sum(count_over_time({job="mysql"} | regexp "(?Pfoo|bar)" [5m] offset 10y))`, `topk(10,sum(rate({region="us-east1"}[5m])) by (name))`, `topk by (name)(10,sum(rate({region="us-east1"}[5m])))`, `avg( rate( ( {job="nginx"} |= "GET" ) [10s] ) ) by (region)`, diff --git a/pkg/logql/lex.go b/pkg/logql/lex.go index 9002ac478c5f..4afe4a21f5d7 100644 --- a/pkg/logql/lex.go +++ b/pkg/logql/lex.go @@ -240,21 +240,27 @@ func tryScanDuration(number string, l *scanner.Scanner) (time.Duration, bool) { return 0, false } // we've found more characters before a whitespace or the end - d, err := time.ParseDuration(sb.String()) + var duration time.Duration + prometheusDuration, err := model.ParseDuration(sb.String()) if err != nil { - return 0, false + duration, err = time.ParseDuration(sb.String()) + if err != nil { + return 0, false + } + } else { + duration = time.Duration(prometheusDuration) } // we need to consume the scanner, now that we know this is a duration. for i := 0; i < consumed; i++ { _ = l.Next() } - return d, true + return time.Duration(duration), true } func isDurationRune(r rune) bool { // "ns", "us" (or "µs"), "ms", "s", "m", "h". switch r { - case 'n', 's', 'u', 'm', 'h', 'µ': + case 'n', 's', 'u', 'm', 'h', 'µ', 'y', 'w', 'd': return true default: return false From e6d347aed3f6044b617f9d9112309cb4e46e99fb Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Sun, 20 Feb 2022 17:46:55 +0200 Subject: [PATCH 2/6] Remove redundant type conversion --- pkg/logql/lex.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/logql/lex.go b/pkg/logql/lex.go index 4afe4a21f5d7..c10fe953e59c 100644 --- a/pkg/logql/lex.go +++ b/pkg/logql/lex.go @@ -254,7 +254,7 @@ func tryScanDuration(number string, l *scanner.Scanner) (time.Duration, bool) { for i := 0; i < consumed; i++ { _ = l.Next() } - return time.Duration(duration), true + return duration, true } func isDurationRune(r rune) bool { From 5c8b58c50b5655bacd7e93310f831a11c6576c19 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Sun, 20 Feb 2022 18:52:57 +0200 Subject: [PATCH 3/6] deduplicate call to sb.String Add a comment to explain why both promql and time's ParseDuration methods are used --- pkg/logql/lex.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/logql/lex.go b/pkg/logql/lex.go index c10fe953e59c..e20d9800f122 100644 --- a/pkg/logql/lex.go +++ b/pkg/logql/lex.go @@ -240,10 +240,15 @@ func tryScanDuration(number string, l *scanner.Scanner) (time.Duration, bool) { return 0, false } // we've found more characters before a whitespace or the end + durationString := sb.String() var duration time.Duration - prometheusDuration, err := model.ParseDuration(sb.String()) + // Try to parse promql style durations first, to ensure that we support the same duration + // units as promql + prometheusDuration, err := model.ParseDuration(durationString) if err != nil { - duration, err = time.ParseDuration(sb.String()) + // Fall back to standard library's time.ParseDuration if a promql style + // duration couldn't be parsed. + duration, err = time.ParseDuration(durationString) if err != nil { return 0, false } From 19f57e34154e0869e02e568f1d451a044859e3c0 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Sun, 20 Feb 2022 19:52:00 +0200 Subject: [PATCH 4/6] extract duration parsing to separate function. --- pkg/logql/lex.go | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/pkg/logql/lex.go b/pkg/logql/lex.go index e20d9800f122..36a129b4d787 100644 --- a/pkg/logql/lex.go +++ b/pkg/logql/lex.go @@ -241,25 +241,36 @@ func tryScanDuration(number string, l *scanner.Scanner) (time.Duration, bool) { } // we've found more characters before a whitespace or the end durationString := sb.String() + duration, err := parseDuration(durationString) + if err != nil { + return 0, false + } + + // we need to consume the scanner, now that we know this is a duration. + for i := 0; i < consumed; i++ { + _ = l.Next() + } + + return duration, true +} + +func parseDuration(d string) (time.Duration, error) { var duration time.Duration // Try to parse promql style durations first, to ensure that we support the same duration // units as promql - prometheusDuration, err := model.ParseDuration(durationString) + prometheusDuration, err := model.ParseDuration(d) if err != nil { // Fall back to standard library's time.ParseDuration if a promql style // duration couldn't be parsed. - duration, err = time.ParseDuration(durationString) + duration, err = time.ParseDuration(d) if err != nil { - return 0, false + return 0, err } } else { duration = time.Duration(prometheusDuration) } - // we need to consume the scanner, now that we know this is a duration. - for i := 0; i < consumed; i++ { - _ = l.Next() - } - return duration, true + + return duration, nil } func isDurationRune(r rune) bool { From 938ab976b5d45d86d24a858a708d4a63c8124a22 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Sun, 20 Feb 2022 20:35:19 +0200 Subject: [PATCH 5/6] Add tests to ensure promql duration units are supported --- pkg/logql/lex_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/pkg/logql/lex_test.go b/pkg/logql/lex_test.go index b57af7969f1b..a87b4e5c6422 100644 --- a/pkg/logql/lex_test.go +++ b/pkg/logql/lex_test.go @@ -4,6 +4,7 @@ import ( "strings" "testing" "text/scanner" + "time" "github.com/stretchr/testify/require" ) @@ -126,3 +127,31 @@ func Test_isFunction(t *testing.T) { }) } } + +func Test_parseDuration(t *testing.T) { + const MICROSECOND = 1000 * time.Nanosecond + const DAY = 24 * time.Hour + const WEEK = 7 * DAY + const YEAR = 365 * DAY + + for _, tc := range []struct { + input string + expected time.Duration + }{ + {"1ns", time.Nanosecond}, + {"1s", time.Second}, + {"1us", MICROSECOND}, + {"1m", time.Minute}, + {"1h", time.Hour}, + {"1µs", MICROSECOND}, + {"1y", YEAR}, + {"1w", WEEK}, + {"1d", DAY}, + {"1h15m30.918273645s", time.Hour + 15*time.Minute + 30*time.Second + 918273645*time.Nanosecond}, + } { + actual, err := parseDuration(tc.input) + + require.Equal(t, err, nil) + require.Equal(t, tc.expected, actual) + } +} From 70efae18ab83840a6cb184d09cb010ae0b5fd492 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 21 Feb 2022 07:49:56 +0200 Subject: [PATCH 6/6] Sort duration rune comment and list of runes in ascending order of length. --- pkg/logql/lex.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/logql/lex.go b/pkg/logql/lex.go index 36a129b4d787..00d1a85d850f 100644 --- a/pkg/logql/lex.go +++ b/pkg/logql/lex.go @@ -274,9 +274,9 @@ func parseDuration(d string) (time.Duration, error) { } func isDurationRune(r rune) bool { - // "ns", "us" (or "µs"), "ms", "s", "m", "h". + // "ns", "us" (or "µs"), "ms", "s", "m", "h", "d", "w", "y". switch r { - case 'n', 's', 'u', 'm', 'h', 'µ', 'y', 'w', 'd': + case 'n', 'u', 'µ', 'm', 's', 'h', 'd', 'w', 'y': return true default: return false