From eaee2d7db459052323f0bb85701d7a43a827b0f8 Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Sun, 8 Sep 2024 11:24:44 +0200 Subject: [PATCH] lib/logstorage: improve error logging for incorrect queries passed to /select/logsql/stats_query and /select/logsql/stats_query_range functions --- app/vlselect/logsql/logsql.go | 10 +++---- docs/VictoriaLogs/querying/README.md | 4 +-- lib/logstorage/parser.go | 23 ++++++++------- lib/logstorage/parser_test.go | 42 +++++++++++++++++++++------- 4 files changed, 51 insertions(+), 28 deletions(-) diff --git a/app/vlselect/logsql/logsql.go b/app/vlselect/logsql/logsql.go index cb8ef56fc..e12e53ed0 100644 --- a/app/vlselect/logsql/logsql.go +++ b/app/vlselect/logsql/logsql.go @@ -593,9 +593,8 @@ func ProcessStatsQueryRangeRequest(ctx context.Context, w http.ResponseWriter, r // Obtain `by(...)` fields from the last `| stats` pipe in q. // Add `_time:step` to the `by(...)` list. - byFields, ok := q.GetStatsByFields(int64(step)) - if !ok { - err := fmt.Errorf("the query must end with '| stats ...'; got [%s]", q) + byFields, err := q.GetStatsByFieldsAddGroupingByTime(int64(step)) + if err != nil { httpserver.SendPrometheusError(w, r, err) return } @@ -706,9 +705,8 @@ func ProcessStatsQueryRequest(ctx context.Context, w http.ResponseWriter, r *htt } // Obtain `by(...)` fields from the last `| stats` pipe in q. - byFields, ok := q.GetStatsByFields(0) - if !ok { - err := fmt.Errorf("the query must end with '| stats ...'; got [%s]", q) + byFields, err := q.GetStatsByFields() + if err != nil { httpserver.SendPrometheusError(w, r, err) return } diff --git a/docs/VictoriaLogs/querying/README.md b/docs/VictoriaLogs/querying/README.md index f170d7b65..95ae6bc31 100644 --- a/docs/VictoriaLogs/querying/README.md +++ b/docs/VictoriaLogs/querying/README.md @@ -289,7 +289,7 @@ for the given [`query`](https://docs.victoriametrics.com/victorialogs/logsql/) a in the format compatible with [Prometheus querying API](https://prometheus.io/docs/prometheus/latest/querying/api/#instant-queries). The `` must contain [`stats` pipe](https://docs.victoriametrics.com/victorialogs/logsql/#stats-pipe). The calculated stats is converted into metrics -with labels enumerated in `by(...)` clause of the `| stats by(...)` pipe. +with labels from `by(...)` clause of the `| stats by(...)` pipe. The `` arg can contain values in [any supported format](https://docs.victoriametrics.com/#timestamp-formats). If `` is missing, then it equals to the current time. @@ -360,7 +360,7 @@ for the given [`query`](https://docs.victoriametrics.com/victorialogs/logsql/) o The stats is returned in the format compatible with [Prometheus querying API](https://prometheus.io/docs/prometheus/latest/querying/api/#range-queries). The `` must contain [`stats` pipe](https://docs.victoriametrics.com/victorialogs/logsql/#stats-pipe). The calculated stats is converted into metrics -with labels enumerated in `by(...)` clause of the `| stats by(...)` pipe. +with labels from `by(...)` clause of the `| stats by(...)` pipe. The `` and `` args can contain values in [any supported format](https://docs.victoriametrics.com/#timestamp-formats). If `` is missing, then it equals to the minimum timestamp across logs stored in VictoriaLogs. diff --git a/lib/logstorage/parser.go b/lib/logstorage/parser.go index 57831ca29..87ff9dc6b 100644 --- a/lib/logstorage/parser.go +++ b/lib/logstorage/parser.go @@ -455,16 +455,19 @@ func (q *Query) Optimize() { } // GetStatsByFields returns `by (...)` fields from the last `stats` pipe at q. +func (q *Query) GetStatsByFields() ([]string, error) { + return q.GetStatsByFieldsAddGroupingByTime(0) +} + +// GetStatsByFieldsAddGroupingByTime returns `by (...)` fields from the last `stats` pipe at q. // -// If step > 0, then _time:step field is added to the last `stats by(...)` pipe at q. -// -// False is returned if q doesn't contain safe `| stats ...` pipe. -func (q *Query) GetStatsByFields(step int64) ([]string, bool) { +// if step > 0, then _time:step is added to the last `stats by (...)` pipe at q. +func (q *Query) GetStatsByFieldsAddGroupingByTime(step int64) ([]string, error) { pipes := q.pipes idx := getLastPipeStatsIdx(pipes) if idx < 0 { - return nil, false + return nil, fmt.Errorf("missing `| stats ...` pipe in the query [%s]", q) } ps := pipes[idx].(*pipeStats) @@ -491,21 +494,21 @@ func (q *Query) GetStatsByFields(step int64) ([]string, bool) { // `| fields ...` pipe must contain all the by(...) fields, otherwise it breaks output. for _, f := range fields { if !slices.Contains(t.fields, f) { - return nil, false + return nil, fmt.Errorf("missing %q field at %q pipe in the query [%s]", f, p, q) } } case *pipeDelete: // Disallow deleting by(...) fields, since this breaks output. for _, f := range t.fields { if slices.Contains(fields, f) { - return nil, false + return nil, fmt.Errorf("the %q field cannot be deleted via %q in the query [%s]", f, p, q) } } case *pipeCopy: // Disallow copying by(...) fields, since this breaks output. for _, f := range t.srcFields { if slices.Contains(fields, f) { - return nil, false + return nil, fmt.Errorf("the %q field cannot be copied via %q in the query [%s]", f, p, q) } } case *pipeRename: @@ -516,11 +519,11 @@ func (q *Query) GetStatsByFields(step int64) ([]string, bool) { } } default: - return nil, false + return nil, fmt.Errorf("the %q pipe cannot be put after %q pipe in the query [%s]", p, ps, q) } } - return fields, true + return fields, nil } func getLastPipeStatsIdx(pipes []pipe) int { diff --git a/lib/logstorage/parser_test.go b/lib/logstorage/parser_test.go index 366ed9c80..453080b21 100644 --- a/lib/logstorage/parser_test.go +++ b/lib/logstorage/parser_test.go @@ -2101,7 +2101,7 @@ func TestQueryDropAllPipes(t *testing.T) { f(`foo | filter bar:baz | stats by (x) min(y)`, `foo bar:baz`) } -func TestQueryGetStatsByFields_PositiveStep(t *testing.T) { +func TestQueryGetStatsByFieldsAddGroupingByTime_Success(t *testing.T) { f := func(qStr string, step int64, fieldsExpected []string, qExpected string) { t.Helper() @@ -2109,9 +2109,9 @@ func TestQueryGetStatsByFields_PositiveStep(t *testing.T) { if err != nil { t.Fatalf("cannot parse [%s]: %s", qStr, err) } - fields, ok := q.GetStatsByFields(step) - if !ok { - t.Fatalf("cannot obtain byFields from the query [%s]", qStr) + fields, err := q.GetStatsByFieldsAddGroupingByTime(step) + if err != nil { + t.Fatalf("unexpected error in GetStatsByFieldsAddGroupingByTime(): %s", err) } if !reflect.DeepEqual(fields, fieldsExpected) { t.Fatalf("unexpected byFields;\ngot\n%q\nwant\n%q", fields, fieldsExpected) @@ -2130,6 +2130,28 @@ func TestQueryGetStatsByFields_PositiveStep(t *testing.T) { f(`* | by (_time:1m offset 30s,level) count() x, count_uniq(z) y`, nsecsPerDay, []string{"_time", "level"}, `* | stats by (_time:86400000000000, level) count(*) as x, count_uniq(z) as y`) } +func TestQueryGetStatsByFieldsAddGroupingByTime_Failure(t *testing.T) { + f := func(qStr string) { + t.Helper() + + q, err := ParseQuery(qStr) + if err != nil { + t.Fatalf("cannot parse [%s]: %s", qStr, err) + } + fields, err := q.GetStatsByFieldsAddGroupingByTime(nsecsPerHour) + if err == nil { + t.Fatalf("expecting non-nil error") + } + if fields != nil { + t.Fatalf("unexpected non-nil fields: %q", fields) + } + } + + f(`*`) + f(`_time:5m | count() | drop _time`) + f(`* | by (x) count() | keep x`) +} + func TestQueryGetStatsByFields_Success(t *testing.T) { f := func(qStr string, fieldsExpected []string) { t.Helper() @@ -2138,9 +2160,9 @@ func TestQueryGetStatsByFields_Success(t *testing.T) { if err != nil { t.Fatalf("cannot parse [%s]: %s", qStr, err) } - fields, ok := q.GetStatsByFields(0) - if !ok { - t.Fatalf("cannot obtain byFields from the query [%s]", qStr) + fields, err := q.GetStatsByFields() + if err != nil { + t.Fatalf("unexpected error in GetStatsByFields(): %s", err) } if !reflect.DeepEqual(fields, fieldsExpected) { t.Fatalf("unexpected byFields;\ngot\n%q\nwant\n%q", fields, fieldsExpected) @@ -2185,9 +2207,9 @@ func TestQueryGetStatsByFields_Failure(t *testing.T) { if err != nil { t.Fatalf("cannot parse [%s]: %s", qStr, err) } - fields, ok := q.GetStatsByFields(0) - if ok { - t.Fatalf("expecting failure to get byFields for the query [%s]", qStr) + fields, err := q.GetStatsByFields() + if err == nil { + t.Fatalf("expecting non-nil error") } if fields != nil { t.Fatalf("expectig nil fields; got %q", fields)