From bf410213baffb1888f3de2685405e64baf8c246c Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Fri, 19 Aug 2022 11:21:28 +0200 Subject: [PATCH 1/5] Fix topk and bottomk operations with int <= 0 --- pkg/logql/rangemapper_test.go | 2 ++ pkg/logql/syntax/ast.go | 6 +++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/logql/rangemapper_test.go b/pkg/logql/rangemapper_test.go index 33d27d6cb6c5..d6157bcd42b4 100644 --- a/pkg/logql/rangemapper_test.go +++ b/pkg/logql/rangemapper_test.go @@ -1737,4 +1737,6 @@ func Test_FailQuery(t *testing.T) { require.NoError(t, err) _, _, err = rvm.Parse(`{app="foo"} |= "err"`) require.Error(t, err) + _, _, err = rvm.Parse(`topk(0, sum by (geoip_country_code) (count_over_time({app="foo"} | json | geoip_country_code != "" and __error__="" [15m])))`) + require.Error(t, err) } diff --git a/pkg/logql/syntax/ast.go b/pkg/logql/syntax/ast.go index a68dcf029cc5..541e34d40221 100644 --- a/pkg/logql/syntax/ast.go +++ b/pkg/logql/syntax/ast.go @@ -869,9 +869,13 @@ func mustNewVectorAggregationExpr(left SampleExpr, operation string, gr *Groupin if params == nil { panic(logqlmodel.NewParseError(fmt.Sprintf("parameter required for operation %s", operation), 0, 0)) } - if p, err = strconv.Atoi(*params); err != nil { + p, err = strconv.Atoi(*params) + if err != nil { panic(logqlmodel.NewParseError(fmt.Sprintf("invalid parameter %s(%s,", operation, *params), 0, 0)) } + if p <= 0 { + panic(logqlmodel.NewParseError(fmt.Sprintf("invalid parameter (must be greater than 0) %s(%s", operation, *params), 0, 0)) + } default: if params != nil { From 62f3d754795bc2ef119bfee5fec6bba2984053ae Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Fri, 19 Aug 2022 11:55:10 +0200 Subject: [PATCH 2/5] Fix VectorAggregationExpr String method --- pkg/logql/syntax/ast.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/pkg/logql/syntax/ast.go b/pkg/logql/syntax/ast.go index 541e34d40221..8f561c97b80c 100644 --- a/pkg/logql/syntax/ast.go +++ b/pkg/logql/syntax/ast.go @@ -928,10 +928,16 @@ func canInjectVectorGrouping(vecOp, rangeOp string) bool { func (e *VectorAggregationExpr) String() string { var params []string - if e.Params != 0 { + switch e.Operation { + // bottomK and topk can have first parameter as 0 + case OpTypeBottomK, OpTypeTopK: params = []string{fmt.Sprintf("%d", e.Params), e.Left.String()} - } else { - params = []string{e.Left.String()} + default: + if e.Params != 0 { + params = []string{fmt.Sprintf("%d", e.Params), e.Left.String()} + } else { + params = []string{e.Left.String()} + } } return formatOperation(e.Operation, e.Grouping, params...) } From 39930cb5c9afe1decb6cd6f0177c041b6983c16c Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Fri, 19 Aug 2022 11:57:38 +0200 Subject: [PATCH 3/5] Update rangemapper_test expression --- pkg/logql/rangemapper_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/logql/rangemapper_test.go b/pkg/logql/rangemapper_test.go index d6157bcd42b4..d2ab1d84e3b2 100644 --- a/pkg/logql/rangemapper_test.go +++ b/pkg/logql/rangemapper_test.go @@ -1737,6 +1737,6 @@ func Test_FailQuery(t *testing.T) { require.NoError(t, err) _, _, err = rvm.Parse(`{app="foo"} |= "err"`) require.Error(t, err) - _, _, err = rvm.Parse(`topk(0, sum by (geoip_country_code) (count_over_time({app="foo"} | json | geoip_country_code != "" and __error__="" [15m])))`) + _, _, err = rvm.Parse(`topk(0, sum(count_over_time({app="foo"} | json | __error__="" [15m])))`) require.Error(t, err) } From 10e347a44b07c950d69f697ebdaf294b3387b782 Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Fri, 19 Aug 2022 12:12:57 +0200 Subject: [PATCH 4/5] Add tests --- pkg/logql/syntax/ast_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/pkg/logql/syntax/ast_test.go b/pkg/logql/syntax/ast_test.go index 4c3c11234522..b7938e546ab1 100644 --- a/pkg/logql/syntax/ast_test.go +++ b/pkg/logql/syntax/ast_test.go @@ -180,6 +180,21 @@ func Test_SampleExpr_String(t *testing.T) { } } +func Test_SampleExpr_String_Fail(t *testing.T) { + t.Parallel() + for _, tc := range []string{ + `topk(0, sum(rate({region="us-east1"}[5m])) by (name))`, + `topk by (name)(0,sum(rate({region="us-east1"}[5m])))`, + `bottomk(0, sum(rate({region="us-east1"}[5m])) by (name))`, + `bottomk by (name)(0,sum(rate({region="us-east1"}[5m])))`, + } { + t.Run(tc, func(t *testing.T) { + _, err := ParseExpr(tc) + require.ErrorContains(t, err, "parse error : invalid parameter (must be greater than 0)") + }) + } +} + func TestMatcherGroups(t *testing.T) { for i, tc := range []struct { query string From 814fc646d7ad3c310dce0a973f01c859a56b299f Mon Sep 17 00:00:00 2001 From: Susana Ferreira Date: Fri, 19 Aug 2022 12:42:52 +0200 Subject: [PATCH 5/5] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 41c445661fe8..10ec83818f5a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ * [5406](https://github.com/grafana/loki/pull/5406) **ctovena**: Revise the configuration parameters that configure the usage report to grafana.com. ##### Fixes +* [6937](https://github.com/grafana/loki/pull/6937) **ssncferreira**: Fix topk and bottomk expressions with parameter <= 0. * [6358](https://github.com/grafana/loki/pull/6358) **taharah**: Fixes sigv4 authentication for the Ruler's remote write configuration by allowing both a global and per tenant configuration. * [6375](https://github.com/grafana/loki/pull/6375) **dannykopping**: Fix bug that prevented users from using the `json` parser after a `line_format` pipeline stage. ##### Changes