From 6eeeda0d0d15d25c720078aefaa40e97b61b28c7 Mon Sep 17 00:00:00 2001 From: szibis Date: Mon, 12 Oct 2026 13:48:22 +0200 Subject: [PATCH 1/5] fix: reject group(), label_join() and other names Loki does not have, accept keywords in any case Loki 3.7.7 has no group aggregation and no label_join: its lexer makes a function token only of the names in functionTokens, so any other name is an IDENTIFIER the grammar cannot place and the query is a 400 parse error at the column of the name. The proxy kept every unknown name(...) as an opaque call and answered it. The parser now keeps opaque only label_replace, vector and approx_topk and reports every other name the way Loki does, also inside a label_replace argument. Loki lower-cases a word before it looks it up, so TOPK(3, X), SUM BY (a) (X), RATE(...) and OFFSET are the keywords they spell, while a label called Rate or Count stays a label. The scanner reads keywords in any case and the query parameter is written with lower-case keywords before a handler reads it. --- .../proxy-accepts-operators-not-in-loki.yaml | 75 ++++++++ internal/logql/ast.go | 2 +- internal/logql/canonical.go | 32 +++- internal/logql/edge_cases_test.go | 10 +- internal/logql/log_range_alternatives_test.go | 2 +- internal/logql/loki_unknown_names_test.go | 173 ++++++++++++++++++ internal/logql/parser.go | 15 +- .../logql/parser_unbalanced_paren_test.go | 3 +- internal/logql/scanner.go | 28 +++ internal/logql/semantic.go | 19 +- internal/proxy/http_utils.go | 18 ++ internal/proxy/proxy.go | 4 +- internal/proxy/unknown_logql_names_test.go | 144 +++++++++++++++ 13 files changed, 500 insertions(+), 25 deletions(-) create mode 100644 conformance/registry/cases/semantics/proxy-accepts-operators-not-in-loki.yaml create mode 100644 internal/logql/loki_unknown_names_test.go create mode 100644 internal/proxy/unknown_logql_names_test.go diff --git a/conformance/registry/cases/semantics/proxy-accepts-operators-not-in-loki.yaml b/conformance/registry/cases/semantics/proxy-accepts-operators-not-in-loki.yaml new file mode 100644 index 000000000..0df80326e --- /dev/null +++ b/conformance/registry/cases/semantics/proxy-accepts-operators-not-in-loki.yaml @@ -0,0 +1,75 @@ +id: semantics/proxy-accepts-operators-not-in-loki +title: group(), label_join() and other names Loki does not have are answered; uppercase keywords are not (fixed) +proves: + - loki_api_v1_query_range + - loki_api_v1_query +consumer: api +why: | + Loki 3.7.7 has no `group` aggregation and no `label_join`: its lexer makes a + function token only of the names in pkg/logql/syntax/lex.go functionTokens + (the range operations, sum avg max min count stddev stdvar bottomk topk sort + sort_desc approx_topk, vector, label_replace and the unwrap conversions), so + any other name is an IDENTIFIER the grammar cannot place (syntax.y vectorOp, + rangeOp, labelReplaceExpr, vectorExpr). `group(count_over_time({...}[5m]))` + and `label_join(...)` are HTTP 400 `parse error at line 1, col 1: syntax + error: unexpected IDENTIFIER`, and so is every PromQL name Loki lacks (abs, + clamp, round, ceil, sqrt, ln, scalar, time, quantile, count_values, ...), + with the column of the name wherever it stands (`sum(group(...))` is col 5). + The proxy's parser kept any unknown `name(...)` as an opaque metric call + (OpaqueMetricExpr) for the translator, which implemented group (every value + 1) and label_join as proxy-only extensions: a client got an answer where + Loki answers 400, and a client written against the proxy broke on Loki. + + The other direction: Loki's lexer lower-cases a word before it looks it up + (lex.go Lex: tokenTextLower, tokenNext), so `TOPK(3, X)`, `SUM BY (a) (X)`, + `RATE(...)`, `OFFSET`, `UNWRAP`, `AND`, `ON`, `GROUP_LEFT`, `BOOL` and the + parser keywords are the keywords they spell. A function name counts only when + a call follows (lex.go isFunction: "(" or by/without and "("), so a label + called Rate or Count stays a label. The proxy matched the lower-case + spelling only: `TOPK(...)` fell through the opaque path and answered a + `streams` result, `SUM BY (app) (...)` was a parse error. + + Fixed in internal/logql: the scanner reads a keyword in any case (the + keyword and function-name sets the dotted-name check already uses), the parser + keeps opaque only label_replace, vector and approx_topk and reports any other + name as Loki does (the column of the name, relative to the whole query also + inside a label_replace argument), and LowercaseKeywords writes the keywords + in lower case in the `query` parameter before a handler reads it, so the + translator and every handler see the spelling they match. The group + post-processing and label_join code (translator markers, proxy response + rewriting) became unreachable and was removed. + + Not changed (separate causes, still open): a name Loki rejects with a + different grammar error (`sum({app="a"})` col 14 unexpected ")", `avg_over_time` + without unwrap "invalid aggregation", `approx_topk by (...)` "grouping not + allowed"); keyword-named labels in a selector (`{by="x"}`), whose proxy text + is its own; and leading white space, which the proxy trims before it + reports a column. +request: + method: GET + path: /loki/api/v1/query_range + params: + query: 'group(count_over_time({app="api-gateway"}[5m]))' + start: 'now-1h' + end: 'now' + step: '60' + headers: + X-Scope-OrgID: "0" +compare: + mode: observed + fields: [status, error_text] + expect: | + Loki's 400 `parse error at line 1, col 1: syntax error: unexpected + IDENTIFIER`, byte for byte; `TOPK(3, RATE({...}[5m]))` answers like the + lower-case form. +gap: + status: fixed + fixed_by: "#TBD" + impact: api-only + effort: S + area: 'internal/logql scanner (keyword case) + parser (opaque calls) + semantic (label_replace argument position); internal/proxy lowercaseQueryKeywords' + loki: 'pkg/logql/syntax/lex.go:95-137 functionTokens, :226-252 Lex lower-casing, :398 isFunction; syntax.y:477-490 vectorOp, :492-508 rangeOp' + proxy: '400 with Loki''s text; keywords accepted in any case' + planned_test: 'TestCompat_OperatorsNotInLokiRejected and TestCompat_UppercaseKeywordsMatchLoki (test/e2e-compat, group semantics); TestValidateLogQL_NamesLokiDoesNotKnowAreRejected, TestValidateLogQL_KeywordsAreCaseInsensitive and TestLowercaseKeywords (internal/logql); TestUnknownLogQLNamesRejectedLikeLoki and TestUppercaseKeywordsSendTheLowercaseBackendRequests (internal/proxy)' + note: 'Messages from Loki v3.7.7 syntax.ParseExpr, run on the same queries. The e2e vs Loki is written, not yet run.' + clusters: [] diff --git a/internal/logql/ast.go b/internal/logql/ast.go index 4a9f892ba..53f64d202 100644 --- a/internal/logql/ast.go +++ b/internal/logql/ast.go @@ -516,7 +516,7 @@ func (l *LiteralExpr) String() string { func (l *LiteralExpr) expr() {} // OpaqueMetricExpr holds the raw text of a metric-level function call that the -// parser does not understand (e.g. label_replace, label_join). It passes +// parser does not understand (label_replace, vector, approx_topk). It passes // through to VictoriaLogs without modification. type OpaqueMetricExpr struct { Raw string diff --git a/internal/logql/canonical.go b/internal/logql/canonical.go index 5ede677f5..2f20d5021 100644 --- a/internal/logql/canonical.go +++ b/internal/logql/canonical.go @@ -6,8 +6,10 @@ import "strings" // expression into the plain `{selector} pipeline [range]` form the translator // handles: parentheses around a log query or a log range, and a pipeline or // unwrap written after the range (`({a} | json)[5m]`, `rate({a}[5m] | json)`). -// Every other query is returned unchanged, without parsing it. +// Every other query is returned unchanged, without parsing it. It also writes +// the keywords in lower case (LowercaseKeywords). func CanonicalizeLogRanges(query string) string { + query = LowercaseKeywords(query) if !hasAlternativeLogRange(query) { return query } @@ -26,6 +28,34 @@ func CanonicalizeLogRanges(query string) string { return string(append(out, query[last:]...)) } +// LowercaseKeywords writes in lower case the words Loki's lexer reads without +// regard to case (pkg/logql/syntax/lex.go lower-cases a word before it looks it +// up): SUM, TOPK(3, ...), RATE(...), BY, OFFSET, UNWRAP and the like. Names, +// strings and label names that are not keywords keep their spelling. The +// translator and the handlers match the lower-case spelling. +func LowercaseKeywords(query string) string { + upper := false + for i := 0; i < len(query) && !upper; i++ { + upper = query[i] >= 'A' && query[i] <= 'Z' + } + if !upper { + return query + } + var out []byte + last := 0 + s := newScanner(query) + for tok := s.next(); tok.Typ != TokEOF; tok = s.next() { + if span := query[s.start:s.pos]; span != tok.Val && strings.EqualFold(span, tok.Val) { + out = append(append(out, query[last:s.start]...), tok.Val...) + last = s.pos + } + } + if out == nil { + return query + } + return string(append(out, query[last:]...)) +} + // hasAlternativeLogRange is the cheap pre-check for CanonicalizeLogRanges. It // looks, outside string literals, for a `(` that opens a log query (it is // followed by `{` or `(` and does not follow an identifier or a number, which diff --git a/internal/logql/edge_cases_test.go b/internal/logql/edge_cases_test.go index 69c2c75da..8a5fe63c8 100644 --- a/internal/logql/edge_cases_test.go +++ b/internal/logql/edge_cases_test.go @@ -874,7 +874,7 @@ func TestEdge_IpFilter_PipelineIntegration(t *testing.T) { } } -// ─── label_replace / label_join – extended coverage ────────────────────────── +// ─── label_replace – extended coverage ────────────────────────────────────── func TestEdge_LabelReplace_Extended(t *testing.T) { valid := []string{ @@ -888,16 +888,8 @@ func TestEdge_LabelReplace_Extended(t *testing.T) { `label_replace(sum by(app, level)(count_over_time({env="production"}[5m])), "app_env", "$1-prod", "app", "(.*)")`, // label_replace on vector agg `label_replace(avg by(level)(rate({app="a"}[5m])), "lvl", "$1", "level", "(.*)")`, - // label_join with two labels - `label_join(sum by(app, level)(count_over_time({env="production"}[5m])), "combined", "/", "app", "level")`, - // label_join with single label - `label_join(sum by(app)(rate({env="production"}[5m])), "svc_copy", "_", "app")`, - // label_join with three labels - `label_join(sum by(app, env, level)(count_over_time({env="production"}[5m])), "full", ".", "app", "env", "level")`, // nested label_replace `label_replace(label_replace(rate({app="a"}[5m]), "a", "$1", "b", "(.*)"), "c", "x", "d", "(.*)")`, - // label_join of label_replace output - `label_join(label_replace(sum by(app)(rate({app="a"}[5m])), "svc", "$1", "app", "(.*)"), "combined", "/", "svc", "env")`, // label_replace inside topk `topk(5, label_replace(sum by(app)(rate({env="production"}[5m])), "service", "$1", "app", "(.*)"))`, } diff --git a/internal/logql/log_range_alternatives_test.go b/internal/logql/log_range_alternatives_test.go index 1945a748b..bcd9f888c 100644 --- a/internal/logql/log_range_alternatives_test.go +++ b/internal/logql/log_range_alternatives_test.go @@ -39,7 +39,7 @@ func TestCanonicalizeLogRanges(t *testing.T) { // Any white space around the offset still takes the pipeline after the range along. {"rate({app=\"x\"}[5m]\noffset 1h | json)", "rate({app=\"x\"} | json [5m]\noffset 1h)"}, {"rate({app=\"x\"}[5m] offset\t1h\n| json)", "rate({app=\"x\"} | json [5m] offset\t1h)"}, - // The first argument of label_replace / label_join is a metric expression. + // The first argument of label_replace is a metric expression. {`label_replace(rate(({app="x"} |= "x, (")[1m]), "a", "$1", "b", "(.*)")`, `label_replace(rate({app="x"} |= "x, (" [1m]), "a", "$1", "b", "(.*)")`}, {`label_replace(sum by (a) (rate(({app="x"} |= "x")[1m])), "a", "b", "c", "d") > 1`, `label_replace(sum by (a) (rate({app="x"} |= "x" [1m])), "a", "b", "c", "d") > 1`}, // Canonical forms and strings that merely look like the alternative ones stay as they are. diff --git a/internal/logql/loki_unknown_names_test.go b/internal/logql/loki_unknown_names_test.go new file mode 100644 index 000000000..32bc0fab2 --- /dev/null +++ b/internal/logql/loki_unknown_names_test.go @@ -0,0 +1,173 @@ +package logql_test + +import ( + "testing" + + "github.com/ReliablyObserve/Loki-VL-proxy/internal/logql" +) + +// Loki v3.7.7 has no group() or label_join() and no PromQL math or time +// functions: its lexer turns only the names in pkg/logql/syntax/lex.go +// functionTokens into function tokens, so any other name is an IDENTIFIER that +// the grammar (syntax.y vectorOp, rangeOp, LABEL_REPLACE, vectorExpr) cannot +// place. The messages below are what syntax.ParseExpr returned for each query, +// including the column the unexpected token starts at. +// +// conformance: semantics/proxy-accepts-operators-not-in-loki +func TestValidateLogQL_NamesLokiDoesNotKnowAreRejected(t *testing.T) { + cases := []struct{ query, want string }{ + {`group(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`GROUP(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`group by (app) (count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`group without (app) (count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`group(count_over_time({app="a"}[5m])) by (app)`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`sum(group(count_over_time({app="a"}[5m])))`, `parse error at line 1, col 5: syntax error: unexpected IDENTIFIER`}, + {`group(count_over_time({app="a"}[5m])) > 0`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`2 * group(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 5: syntax error: unexpected IDENTIFIER`}, + {`sum(rate({app="a"}[5m])) + group(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 28: syntax error: unexpected IDENTIFIER`}, + {`vector(1) + group(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 13: syntax error: unexpected IDENTIFIER`}, + {`topk(3, group(count_over_time({app="a"}[5m])))`, `parse error at line 1, col 9: syntax error: unexpected IDENTIFIER`}, + {`sum by (app) (group(count_over_time({app="a"}[5m])))`, `parse error at line 1, col 15: syntax error: unexpected IDENTIFIER`}, + {`(group(count_over_time({app="a"}[5m])))`, `parse error at line 1, col 2: syntax error: unexpected IDENTIFIER`}, + {`sum( group (rate({app="a"}[5m])))`, `parse error at line 1, col 7: syntax error: unexpected IDENTIFIER`}, + {`label_join(count_over_time({app="a"}[5m]), "d", ",", "app", "pod")`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`LABEL_JOIN(count_over_time({app="a"}[5m]), "d", ",", "app")`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`label_join(sum by (app) (count_over_time({app="a"}[5m])), "d", ",", "app")`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`label_replace(label_join(count_over_time({app="a"}[5m]), "d", ",", "app"), "a", "b", "c", "d")`, `parse error at line 1, col 15: syntax error: unexpected IDENTIFIER`}, + {`sum(label_replace(group(count_over_time({app="a"}[5m])), "a", "b", "c", "d"))`, `parse error at line 1, col 19: syntax error: unexpected IDENTIFIER`}, + {`count_over_time({app="a"}[5m]) + label_replace(label_join(count_over_time({app="a"}[5m]), "d", ",", "app"), "a", "b", "c", "d")`, `parse error at line 1, col 48: syntax error: unexpected IDENTIFIER`}, + {`abs(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`sgn(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`round(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`clamp(count_over_time({app="a"}[5m]), 1, 2)`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`clamp_min(count_over_time({app="a"}[5m]), 1)`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`ceil(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`floor(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`sqrt(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`ln(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`exp(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`log2(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`sin(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`scalar(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`time()`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`timestamp(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`hour(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`quantile(0.5, count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`count_values("x", count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`present_over_time({app="a"}[5m])`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`increase(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`delta(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`irate({app="a"}[5m])`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`absent(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`histogram_quantile(0.5, count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`foo(count_over_time({app="a"}[5m]))`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`bar(1)`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`group`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`ln`, `parse error at line 1, col 1: syntax error: unexpected IDENTIFIER`}, + {`rate({app="a"}[5m]) + foo`, `parse error at line 1, col 23: syntax error: unexpected IDENTIFIER`}, + {`rate({app="a"}[5m]) OFFSET 1m`, `parse error at line 1, col 21: syntax error: unexpected offset`}, + {`count_over_time({app="a"}[5m]) OFFSET 5m`, `parse error at line 1, col 32: syntax error: unexpected offset`}, + {`count_over_time({app="a"}[5m]) + Count`, `parse error at line 1, col 34: syntax error: unexpected IDENTIFIER`}, + {`rate({app="a"}[5m]) + Rate`, `parse error at line 1, col 23: syntax error: unexpected IDENTIFIER`}, + {`{app="a"} | json | JSON="x"`, `parse error at line 1, col 24: syntax error: unexpected =`}, + } + for _, tc := range cases { + t.Run(tc.query, func(t *testing.T) { + if got := logql.ValidateLogQL(tc.query); got != tc.want { + t.Fatalf("ValidateLogQL(%q)\n got %q\n want %q", tc.query, got, tc.want) + } + }) + } +} + +// Loki's lexer lower-cases every word before it looks it up (lex.go: tokens, +// functionTokens), so SUM, TOPK, RATE, BY, OFFSET and the rest are the keywords +// they spell. A function name is a function only when a "(" follows it, or by or +// without and then "(", so a label called Rate or Count stays a label. +func TestValidateLogQL_KeywordsAreCaseInsensitive(t *testing.T) { + queries := []string{ + `TOPK(3, count_over_time({app="a"}[5m]))`, + `Topk(3, count_over_time({app="a"}[5m]))`, + `SUM(count_over_time({app="a"}[5m]))`, + `Sum by (app) (count_over_time({app="a"}[5m]))`, + `SUM BY (app) (count_over_time({app="a"}[5m]))`, + `SUM WITHOUT (app) (count_over_time({app="a"}[5m]))`, + `sum(count_over_time({app="a"}[5m])) BY (app)`, + `sum(rate({app="a"}[5m])) By (app)`, + `AVG(count_over_time({app="a"}[5m]))`, + `MIN(count_over_time({app="a"}[5m]))`, + `MAX(count_over_time({app="a"}[5m]))`, + `COUNT(count_over_time({app="a"}[5m]))`, + `STDDEV(count_over_time({app="a"}[5m]))`, + `STDVAR(count_over_time({app="a"}[5m]))`, + `BOTTOMK(3, count_over_time({app="a"}[5m]))`, + `SORT(count_over_time({app="a"}[5m]))`, + `SORT_DESC(count_over_time({app="a"}[5m]))`, + `RATE({app="a"}[5m])`, + `Rate({app="a"}[5m])`, + `COUNT_OVER_TIME({app="a"}[5m])`, + `BYTES_RATE({app="a"}[5m])`, + `BYTES_OVER_TIME({app="a"}[5m])`, + `ABSENT_OVER_TIME({app="a"}[5m])`, + `RATE_COUNTER({app="a"} | unwrap x [5m])`, + `FIRST_OVER_TIME({app="a"} | unwrap x [5m])`, + `LAST_OVER_TIME({app="a"} | unwrap x [5m])`, + `QUANTILE_OVER_TIME(0.5, {app="a"} | unwrap x [5m])`, + `AVG_OVER_TIME({app="a"} | unwrap x [5m])`, + `SUM(RATE({app="a"}[5m]))`, + `LABEL_REPLACE(count_over_time({app="a"}[5m]), "d", "$1", "app", "(.*)")`, + `VECTOR(1)`, + `VECTOR(1) + count_over_time({app="a"}[5m])`, + `count_over_time({app="a"}[5m]) AND count_over_time({app="a"}[5m])`, + `count_over_time({app="a"}[5m]) OR count_over_time({app="a"}[5m])`, + `count_over_time({app="a"}[5m]) UNLESS count_over_time({app="a"}[5m])`, + `count_over_time({app="a"}[5m]) > BOOL 1`, + `count_over_time({app="a"}[5m]) + ON (app) count_over_time({app="a"}[5m])`, + `count_over_time({app="a"}[5m]) + IGNORING (app) count_over_time({app="a"}[5m])`, + `count_over_time({app="a"}[5m]) + ON (app) GROUP_LEFT count_over_time({app="a"}[5m])`, + `count_over_time({app="a"}[5m]) + ON (app) GROUP_RIGHT (x) count_over_time({app="a"}[5m])`, + `rate({app="a"}[5m] OFFSET 1m)`, + `{app="a"} | JSON`, + `{app="a"} | LOGFMT`, + `{app="a"} | LINE_FORMAT "x"`, + `{app="a"} | LABEL_FORMAT a=b`, + `{app="a"} | DROP a`, + `{app="a"} | KEEP a`, + `rate({app="a"} | json | UNWRAP x [5m])`, + `rate({app="a"} | json | unwrap BYTES(x) [5m])`, + `rate({app="a"} | json | unwrap DURATION(x) [5m])`, + `sum by (Rate) (count_over_time({app="a"}[5m]))`, + `sum by (Count) (count_over_time({app="a"}[5m]))`, + `{app="a"} | logfmt | Count > 1`, + `{app="a"} | json | Sum > 1`, + } + for _, query := range queries { + t.Run(query, func(t *testing.T) { + if got := logql.ValidateLogQL(query); got != "" { + t.Fatalf("ValidateLogQL(%q) = %q, want it accepted like Loki", query, got) + } + }) + } +} + +// LowercaseKeywords writes the keywords Loki reads case-insensitively in lower +// case and leaves every other word, string and label name as written. +// +// conformance: semantics/proxy-accepts-operators-not-in-loki +func TestLowercaseKeywords(t *testing.T) { + for _, tc := range []struct{ query, want string }{ + {`TOPK(3, RATE({app="a"}[5m]))`, `topk(3, rate({app="a"}[5m]))`}, + {`SUM BY (App) (COUNT_OVER_TIME({app="a"} |= "ERROR" [5m]))`, `sum by (App) (count_over_time({app="a"} |= "ERROR" [5m]))`}, + {`Sum(Rate({app="a"}[5m])) By (app) > BOOL 1`, `sum(rate({app="a"}[5m])) by (app) > bool 1`}, + {`rate({app="a"} | JSON | UNWRAP BYTES(Size) [5m] OFFSET 1m)`, `rate({app="a"} | json | unwrap bytes(Size) [5m] offset 1m)`}, + {`A + ON (app) GROUP_LEFT (Pod) B`, `A + on (app) group_left (Pod) B`}, + {`{app="a"} | logfmt | Count > 1 | Rate < 2`, `{app="a"} | logfmt | Count > 1 | Rate < 2`}, + {`sum by (Rate, Count) (count_over_time({App="a"}[5m]))`, `sum by (Rate, Count) (count_over_time({App="a"}[5m]))`}, + {`{app="a"} |= "SUM(BY)" | line_format "{{.OFFSET}}"`, `{app="a"} |= "SUM(BY)" | line_format "{{.OFFSET}}"`}, + {`rate({app="a"}[5m])`, `rate({app="a"}[5m])`}, + } { + if got := logql.LowercaseKeywords(tc.query); got != tc.want { + t.Errorf("LowercaseKeywords(%q)\n got %q\n want %q", tc.query, got, tc.want) + } + } +} diff --git a/internal/logql/parser.go b/internal/logql/parser.go index 4e8fee577..7ac7b84a7 100644 --- a/internal/logql/parser.go +++ b/internal/logql/parser.go @@ -82,6 +82,10 @@ var vectorOps = map[string]VectorOp{ "sort_desc": VectorSortDesc, } +// opaqueMetricFunctions are the metric-level calls the parser keeps as raw text +// (OpaqueMetricExpr): the ones in Loki's grammar that vectorOps and rangeOps do not cover. +var opaqueMetricFunctions = map[string]bool{"label_replace": true, "vector": true, "approx_topk": true} + // rangeOps maps function names to RangeOp constants. var rangeOps = map[string]RangeOp{ "rate": RangeRate, @@ -207,11 +211,16 @@ func (p *parser) parsePrimary() (Expr, error) { } return ra, nil } - // Unknown metric-level function (e.g. label_replace, label_join): - // capture the full raw text and return it as-is so VictoriaLogs + // Loki's grammar has three more metric-level calls (syntax.y + // labelReplaceExpr, vectorExpr, vectorOp APPROX_TOPK); any other name + // is an IDENTIFIER it cannot place (group, label_join, abs, ...). + name := p.cur.Val + if !opaqueMetricFunctions[name] { + return nil, p.syntaxErrorAt("") + } + // Capture the full raw text and return it as-is so VictoriaLogs // receives the original expression unchanged. startPos := p.sc.pos - len(p.cur.Val) - name := p.cur.Val p.advance() if p.cur.Typ == TokLParen { p.advance() // consume '(' diff --git a/internal/logql/parser_unbalanced_paren_test.go b/internal/logql/parser_unbalanced_paren_test.go index 16766743a..0abc50f33 100644 --- a/internal/logql/parser_unbalanced_paren_test.go +++ b/internal/logql/parser_unbalanced_paren_test.go @@ -6,7 +6,7 @@ import ( ) // TestParse_UnbalancedOpaqueParens guards against a parser panic on opaque -// metric functions (label_replace, label_join, or any unknown function name) +// metric functions (label_replace) // whose argument parentheses never close. consumeBalancedParens returned 0 when // it hit EOF with parens still open, and the caller then sliced p.input[start:0] // — panicking with "slice bounds out of range". Since ValidateLogQL runs on @@ -43,7 +43,6 @@ func TestParse_UnbalancedOpaqueParens(t *testing.T) { func TestParse_BalancedOpaqueParens(t *testing.T) { valid := []string{ `label_replace(rate({app="x"}[5m]), "dst", "$1", "src", "(.*)")`, - `label_join(rate({app="x"}[5m]), "dst", ",", "a", "b")`, } for _, in := range valid { t.Run(in, func(t *testing.T) { diff --git a/internal/logql/scanner.go b/internal/logql/scanner.go index 39fa31e18..7975b3434 100644 --- a/internal/logql/scanner.go +++ b/internal/logql/scanner.go @@ -2,6 +2,7 @@ package logql import ( "strconv" + "strings" "unicode" "unicode/utf8" ) @@ -391,6 +392,30 @@ func (s *scanner) scanRawString() Token { return Token{Typ: TokRawString, Val: val} } +// readsAsKeyword reports whether Loki's lexer reads the lower-cased word at the +// scanner position as a keyword: any of lokiKeywords, or a function name +// (lex.go functionTokens) that a call follows (lex.go isFunction), so a label +// called Rate or Count stays a label. +func (s *scanner) readsAsKeyword(lower string) bool { + if lokiKeywords[lower] { + return true + } + if !rangeAggNames[lower] && !vectorAggNames[lower] && !unwrapConversions[lower] && lower != "vector" { + return false + } + i := skipSpace(s.src, s.pos) + if i < len(s.src) && s.src[i] == '(' { + return true + } + for _, grouping := range [...]string{"by", "without"} { + if len(s.src)-i >= len(grouping) && strings.EqualFold(s.src[i:i+len(grouping)], grouping) { + j := skipSpace(s.src, i+len(grouping)) + return j < len(s.src) && s.src[j] == '(' + } + } + return false +} + func (s *scanner) scanIdent() Token { start := s.pos for { @@ -401,6 +426,9 @@ func (s *scanner) scanIdent() Token { s.pos += size } val := s.src[start:s.pos] + if lower := strings.ToLower(val); lower != val && s.readsAsKeyword(lower) { + val = lower + } switch val { case "by": return Token{Typ: TokBy, Val: val} diff --git a/internal/logql/semantic.go b/internal/logql/semantic.go index 01e69a639..112fd27cb 100644 --- a/internal/logql/semantic.go +++ b/internal/logql/semantic.go @@ -5,6 +5,7 @@ import ( "regexp" "regexp/syntax" "strconv" + "strings" ) // errEmptyCompatibleMatchers is Loki's syntax.errAtleastOneEqualityMatcherRequired, @@ -37,7 +38,7 @@ func validateSemantics(expr Expr, raw string) string { case *OpaqueMetricExpr: // label_replace(inner, ...) and similar wrappers: Loki validates the // wrapped sample expression like any other. - inner, err := opaqueFirstArgument(x.Raw) + inner, err := opaqueFirstArgument(raw, x.Raw) var lokiErr *ParseError if errors.As(err, &lokiErr) { return lokiErr.Msg @@ -50,11 +51,17 @@ func validateSemantics(expr Expr, raw string) string { } // opaqueFirstArgument parses the leading expression argument of an opaque -// metric function call such as `label_replace(rate({app="a"}[5m]), ...)`. -// It returns a nil Expr when the call has no parseable expression argument, -// and the parse error when that argument is itself rejected. -func opaqueFirstArgument(call string) (Expr, error) { - p := &parser{sc: newScanner(call)} +// metric function call such as `label_replace(rate({app="a"}[5m]), ...)`, read +// from query where call stands so that an error carries the line and column +// Loki reports for the whole query. It returns a nil Expr when the call has no +// parseable expression argument, and the parse error when that argument is +// itself rejected. +func opaqueFirstArgument(query, call string) (Expr, error) { + src, pos := query, strings.Index(query, call) + if pos < 0 { + src, pos = call, 0 + } + p := &parser{sc: &scanner{src: src, pos: pos}} p.advance() if p.cur.Typ != TokIdent { return nil, nil diff --git a/internal/proxy/http_utils.go b/internal/proxy/http_utils.go index 4268c6370..5517edec9 100644 --- a/internal/proxy/http_utils.go +++ b/internal/proxy/http_utils.go @@ -64,6 +64,24 @@ var ( hdrValZstdGzip = []string{"zstd, gzip"} ) +// lowercaseQueryKeywords writes the keywords of the query parameter in lower +// case before any handler reads it. Loki's lexer is case-insensitive for them +// (SUM BY, TOPK(...), RATE(...), OFFSET), and the translator and the handlers +// match the lower-case spelling. +func lowercaseQueryKeywords(next http.HandlerFunc) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + if query := r.FormValue("query"); query != "" { + if lower := logql.LowercaseKeywords(query); lower != query { + r.Form.Set("query", lower) + if r.PostForm.Has("query") { + r.PostForm.Set("query", lower) + } + } + } + next(w, r) + } +} + // validateQuery checks query string length and returns a sanitized version. // It also rewrites queries that Loki accepts but VL would reject (e.g. phi>1). func (p *Proxy) validateQuery(w http.ResponseWriter, query string, endpoint string) (string, bool) { diff --git a/internal/proxy/proxy.go b/internal/proxy/proxy.go index 82fc238ca..d47e5e8c7 100644 --- a/internal/proxy/proxy.go +++ b/internal/proxy/proxy.go @@ -1737,9 +1737,9 @@ func (p *Proxy) routeHandler(endpoint, route string, h http.HandlerFunc) http.Ha p.tenantMiddleware( p.limiter.Middleware( p.requestLogger(endpoint, route, - p.lokiQueryParamValidation(endpoint, + lowercaseQueryKeywords(p.lokiQueryParamValidation(endpoint, p.tenantLimitsMiddleware(endpoint, - p.compatCacheMiddleware(endpoint, route, h))))))) + p.compatCacheMiddleware(endpoint, route, h)))))))) } // RegisterRoutes wires every proxy, admin, debug, and metrics route onto the diff --git a/internal/proxy/unknown_logql_names_test.go b/internal/proxy/unknown_logql_names_test.go new file mode 100644 index 000000000..9da478dcd --- /dev/null +++ b/internal/proxy/unknown_logql_names_test.go @@ -0,0 +1,144 @@ +package proxy + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "sort" + "strconv" + "strings" + "sync" + "testing" +) + +// recordingBackend is a fake VictoriaLogs that answers every request with an +// empty result and keeps the requests it saw, as path and sorted parameters. +type recordingBackend struct { + *httptest.Server + mu sync.Mutex + calls []string +} + +func newRecordingBackend(t *testing.T) *recordingBackend { + t.Helper() + b := &recordingBackend{} + b.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = r.ParseForm() + b.mu.Lock() + b.calls = append(b.calls, r.URL.Path+"?"+r.Form.Encode()) + b.mu.Unlock() + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"hits":[],"values":[]}`)) + })) + t.Cleanup(b.Close) + return b +} + +// dataCalls are the requests that read log data or metric results, not the +// label and stream-field lookups the proxy makes on its own. +func (b *recordingBackend) dataCalls() []string { + b.mu.Lock() + defer b.mu.Unlock() + var out []string + for _, c := range b.calls { + if strings.HasPrefix(c, "/select/logsql/query?") || strings.HasPrefix(c, "/select/logsql/stats_query") || strings.HasPrefix(c, "/select/logsql/hits?") { + out = append(out, c) + } + } + sort.Strings(out) + return out +} + +func queryURL(endpoint, query string) string { + params := url.Values{"query": {query}, "start": {"1775322000"}, "end": {"1775325600"}, "time": {"1775325600"}, "step": {"60"}, "limit": {"10"}} + return "/loki/api/v1/" + endpoint + "?" + params.Encode() +} + +// Loki v3.7.7 has no group() and no label_join(), and none of the PromQL math, +// time or quantile functions: a name outside lex.go functionTokens is an +// IDENTIFIER the grammar cannot place (syntax.y vectorOp, LABEL_REPLACE, +// vectorExpr). Both query endpoints answer 400 with Loki's text and the column +// of the name, before any backend call. The messages are syntax.ParseExpr's. +// +// conformance: semantics/proxy-accepts-operators-not-in-loki +func TestUnknownLogQLNamesRejectedLikeLoki(t *testing.T) { + const identifier = "parse error at line 1, col %d: syntax error: unexpected IDENTIFIER" + cases := []struct { + query string + col int + }{ + {`group(count_over_time({app="a"}[5m]))`, 1}, + {`GROUP(count_over_time({app="a"}[5m]))`, 1}, + {`group by (app) (count_over_time({app="a"}[5m]))`, 1}, + {`sum(group(rate({app="a"}[5m])))`, 5}, + {`label_join(rate({app="a"}[5m]), "d", ",", "app", "pod")`, 1}, + {`LABEL_JOIN(rate({app="a"}[5m]), "d", ",", "app")`, 1}, + {`label_replace(label_join(rate({app="a"}[5m]), "d", ",", "app"), "a", "b", "c", "d")`, 15}, + {`sum(rate({app="a"}[5m])) + clamp_min(rate({app="a"}[5m]), 1)`, 28}, + {`abs(rate({app="a"}[5m]))`, 1}, + {`time()`, 1}, + {`count_values("x", count_over_time({app="a"}[5m]))`, 1}, + } + backend := newRecordingBackend(t) + p := newProfileProxy(t, backend.URL, LabelStyleUnderscores, MetadataFieldModeTranslated, false) + for _, endpoint := range []string{"query_range", "query"} { + for _, tc := range cases { + t.Run(endpoint+"/"+tc.query, func(t *testing.T) { + want := strings.Replace(identifier, "%d", strconv.Itoa(tc.col), 1) + w := serve(p, http.MethodGet, queryURL(endpoint, tc.query), nil) + var got struct{ ErrorType, Error, Message string } + if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil || w.Code != http.StatusBadRequest || got.ErrorType != "bad_data" || got.Error != want || got.Message != want { + t.Fatalf("status=%d body=%.300s, want 400 %q", w.Code, w.Body, want) + } + }) + } + } + if calls := backend.dataCalls(); len(calls) != 0 { + t.Fatalf("rejected queries reached the backend: %v", calls) + } +} + +// Loki lower-cases every word before its lexer looks it up (lex.go), so SUM, +// TOPK(3, ...), RATE, BY, OFFSET, UNWRAP and the like are the keywords they +// spell. The proxy answers them with the same backend requests as the +// lower-case spelling. +// +// conformance: semantics/proxy-accepts-operators-not-in-loki +func TestUppercaseKeywordsSendTheLowercaseBackendRequests(t *testing.T) { + cases := []struct{ upper, lower string }{ + {`TOPK(3, RATE({app="a"}[5m]))`, `topk(3, rate({app="a"}[5m]))`}, + {`SUM BY (app) (COUNT_OVER_TIME({app="a"} |= "ERROR" [5m]))`, `sum by (app) (count_over_time({app="a"} |= "ERROR" [5m]))`}, + {`SUM(RATE({app="a"}[5m])) BY (app)`, `sum(rate({app="a"}[5m])) by (app)`}, + {`SUM WITHOUT (pod) (RATE({app="a"}[5m]))`, `sum without (pod) (rate({app="a"}[5m]))`}, + {`SUM(RATE({app="a"} | JSON | UNWRAP bytes [5m]))`, `sum(rate({app="a"} | json | unwrap bytes [5m]))`}, + {`SUM(RATE({app="a"}[5m])) / ON (app) SUM(RATE({app="b"}[5m]))`, `sum(rate({app="a"}[5m])) / on (app) sum(rate({app="b"}[5m]))`}, + {`SUM(RATE({app="a"}[5m])) > BOOL 1`, `sum(rate({app="a"}[5m])) > bool 1`}, + {`MAX(COUNT_OVER_TIME({app="a"}[5m] OFFSET 1m))`, `max(count_over_time({app="a"}[5m] offset 1m))`}, + {`{app="a"} | JSON | DROP pod`, `{app="a"} | json | drop pod`}, + } + run := func(t *testing.T, endpoint, query string) []string { + backend := newRecordingBackend(t) + p := newProfileProxy(t, backend.URL, LabelStyleUnderscores, MetadataFieldModeTranslated, false) + if w := serve(p, http.MethodGet, queryURL(endpoint, query), nil); w.Code >= 500 || w.Code == http.StatusBadRequest { + t.Fatalf("%s %s: status %d %.300s", endpoint, query, w.Code, w.Body) + } + return backend.dataCalls() + } + for _, endpoint := range []string{"query_range", "query"} { + for _, tc := range cases { + if endpoint == "query" && strings.HasPrefix(tc.lower, "{") { + continue // a log query is not an instant query + } + t.Run(endpoint+"/"+tc.upper, func(t *testing.T) { + lower := run(t, endpoint, tc.lower) + if len(lower) == 0 { + t.Fatalf("the lower-case form %q reached no backend call, the comparison proves nothing", tc.lower) + } + if upper := run(t, endpoint, tc.upper); strings.Join(upper, "\n") != strings.Join(lower, "\n") { + t.Fatalf("backend requests differ\n upper %q\n lower %q", upper, lower) + } + }) + } + } +} From 3acabfad4a09f2563f6c47363cd237996df48ba6 Mon Sep 17 00:00:00 2001 From: szibis Date: Mon, 12 Oct 2026 13:50:28 +0200 Subject: [PATCH 2/5] refactor: remove the group() and label_join() translation and response rewriting Loki has neither, and the validator now rejects both before the translator runs, so the marker suffixes, their parsers and the proxy-side rewrites of a matrix response were unreachable. --- internal/proxy/label_transform_test.go | 85 -------------- internal/proxy/proxy.go | 24 +--- internal/proxy/vector_matching.go | 46 +------- .../partial_coverage_internal_test.go | 18 --- internal/translator/translator.go | 106 +----------------- internal/translator/translator_test.go | 91 --------------- 6 files changed, 8 insertions(+), 362 deletions(-) diff --git a/internal/proxy/label_transform_test.go b/internal/proxy/label_transform_test.go index badd48d06..fa3e0bac4 100644 --- a/internal/proxy/label_transform_test.go +++ b/internal/proxy/label_transform_test.go @@ -42,54 +42,6 @@ func parseMatrix(t *testing.T, body []byte) []map[string]string { return out } -func parseMatrixValues(t *testing.T, body []byte) [][]interface{} { - t.Helper() - var resp struct { - Data struct { - Result []struct { - Values [][]interface{} `json:"values"` - } `json:"result"` - } `json:"data"` - } - if err := json.Unmarshal(body, &resp); err != nil { - t.Fatalf("parseMatrixValues: %v", err) - } - if len(resp.Data.Result) == 0 { - return nil - } - return resp.Data.Result[0].Values -} - -func TestApplyGroupNormalization(t *testing.T) { - body := matrixBody(t, []map[string]interface{}{ - { - "metric": map[string]string{"level": "error"}, - "values": [][]interface{}{{"1234567890", "42"}, {"1234567891", "7"}}, - }, - { - "metric": map[string]string{"level": "warn"}, - "values": [][]interface{}{{"1234567890", "3"}}, - }, - }) - - result := applyGroupNormalization(body) - - vals := parseMatrixValues(t, result) - for _, v := range vals { - if len(v) < 2 || v[1] != "1" { - t.Errorf("expected value 1, got %v", v) - } - } -} - -func TestApplyGroupNormalization_NonMatrix(t *testing.T) { - body := []byte(`{"status":"success","data":{"resultType":"streams","result":[]}}`) - out := applyGroupNormalization(body) - if string(out) != string(body) { - t.Error("non-matrix body should be returned unchanged") - } -} - func TestApplyLabelReplace_BasicMatch(t *testing.T) { body := matrixBody(t, []map[string]interface{}{ {"metric": map[string]string{"instance": "host1:9090"}, "values": [][]interface{}{{"1", "1"}}}, @@ -132,40 +84,3 @@ func TestApplyLabelReplace_NoMatch_KeepsDst(t *testing.T) { t.Errorf("host label should be unchanged on no-match, got %q", metrics[0]["host"]) } } - -func TestApplyLabelJoin(t *testing.T) { - body := matrixBody(t, []map[string]interface{}{ - {"metric": map[string]string{"service": "api", "host": "node1"}, "values": [][]interface{}{{"1", "1"}}}, - }) - - spec := translator.LabelJoinSpec{ - DstLabel: "service_host", - Separator: "/", - SrcLabels: []string{"service", "host"}, - } - result := applyLabelJoin(body, spec) - metrics := parseMatrix(t, result) - - if metrics[0]["service_host"] != "api/node1" { - t.Errorf("service_host=%q want api/node1", metrics[0]["service_host"]) - } -} - -func TestApplyLabelJoin_MissingSrc(t *testing.T) { - body := matrixBody(t, []map[string]interface{}{ - {"metric": map[string]string{"service": "api"}, "values": [][]interface{}{{"1", "1"}}}, - }) - - spec := translator.LabelJoinSpec{ - DstLabel: "service_host", - Separator: "/", - SrcLabels: []string{"service", "host"}, // host is missing - } - result := applyLabelJoin(body, spec) - metrics := parseMatrix(t, result) - - // Missing labels are skipped, so only "api" appears (no trailing separator) - if metrics[0]["service_host"] != "api" { - t.Errorf("service_host=%q want api", metrics[0]["service_host"]) - } -} diff --git a/internal/proxy/proxy.go b/internal/proxy/proxy.go index d47e5e8c7..c8627114e 100644 --- a/internal/proxy/proxy.go +++ b/internal/proxy/proxy.go @@ -2349,11 +2349,9 @@ func (p *Proxy) handleQueryRange(w http.ResponseWriter, r *http.Request) { p.metrics.RecordRequest("query_range", http.StatusBadRequest, time.Since(start)) return } - // Extract without() labels and label-transform markers for post-processing. + // Extract without() labels and label_replace markers for post-processing. logsqlQuery, withoutLabels := translator.ParseWithoutMarker(logsqlQuery) - logsqlQuery, isGroupQuery := translator.ParseGroupMarker(logsqlQuery) logsqlQuery, labelReplaceSpecs := translator.ParseAllLabelReplaceMarkers(logsqlQuery) - logsqlQuery, labelJoinSpec := translator.ParseLabelJoinMarker(logsqlQuery) for _, spec := range labelReplaceSpecs { if _, err := regexp.Compile("^(?:" + spec.Regex + ")$"); err != nil { p.writeError(w, http.StatusBadRequest, fmt.Sprintf("parse error : invalid regex in label_replace: %v", err)) @@ -2369,7 +2367,7 @@ func (p *Proxy) handleQueryRange(w http.ResponseWriter, r *http.Request) { } p.log.Debug("translated query", "logsql", redactQuery(logsqlQuery, p.debugLogRawQueries), "without", withoutLabels) - needsCapture := len(withoutLabels) > 0 || isGroupQuery || len(labelReplaceSpecs) > 0 || labelJoinSpec != nil + needsCapture := len(withoutLabels) > 0 || len(labelReplaceSpecs) > 0 var ( sc = &statusCapture{ResponseWriter: w, code: 200} capture *bufferedResponseWriter @@ -2417,15 +2415,9 @@ func (p *Proxy) handleQueryRange(w http.ResponseWriter, r *http.Request) { if len(withoutLabels) > 0 { cacheOut = applyWithoutGrouping(cacheOut, withoutLabels, withoutAggregationOp(logqlQuery)) } - if isGroupQuery { - cacheOut = applyGroupNormalization(cacheOut) - } for _, spec := range labelReplaceSpecs { cacheOut = applyLabelReplace(cacheOut, spec) } - if labelJoinSpec != nil { - cacheOut = applyLabelJoin(cacheOut, *labelJoinSpec) - } copyBackendHeaders(w.Header(), capture.Header()) if w.Header().Get("Content-Type") == "" { w.Header().Set("Content-Type", "application/json") @@ -2614,11 +2606,9 @@ func (p *Proxy) handleQuery(w http.ResponseWriter, r *http.Request) { return } - // Extract without() labels and label-transform markers for post-processing. + // Extract without() labels and label_replace markers for post-processing. logsqlQuery, withoutLabels := translator.ParseWithoutMarker(logsqlQuery) - logsqlQuery, isGroupQuery := translator.ParseGroupMarker(logsqlQuery) logsqlQuery, labelReplaceSpecs := translator.ParseAllLabelReplaceMarkers(logsqlQuery) - logsqlQuery, labelJoinSpec := translator.ParseLabelJoinMarker(logsqlQuery) for _, spec := range labelReplaceSpecs { if _, err := regexp.Compile("^(?:" + spec.Regex + ")$"); err != nil { p.writeError(w, http.StatusBadRequest, fmt.Sprintf("parse error : invalid regex in label_replace: %v", err)) @@ -2636,7 +2626,7 @@ func (p *Proxy) handleQuery(w http.ResponseWriter, r *http.Request) { // Wrap writer to capture actual status code for metrics sc := &statusCapture{ResponseWriter: w, code: 200} - needsCapture := len(withoutLabels) > 0 || isGroupQuery || len(labelReplaceSpecs) > 0 || labelJoinSpec != nil + needsCapture := len(withoutLabels) > 0 || len(labelReplaceSpecs) > 0 var bw *bufferedResponseWriter if needsCapture { bw = &bufferedResponseWriter{header: w.Header()} @@ -2666,15 +2656,9 @@ func (p *Proxy) handleQuery(w http.ResponseWriter, r *http.Request) { if len(withoutLabels) > 0 { result = applyWithoutGrouping(result, withoutLabels, withoutAggregationOp(logqlQuery)) } - if isGroupQuery { - result = applyGroupNormalization(result) - } for _, spec := range labelReplaceSpecs { result = applyLabelReplace(result, spec) } - if labelJoinSpec != nil { - result = applyLabelJoin(result, *labelJoinSpec) - } w.Header().Set("Content-Type", "application/json") w.Write(result) } diff --git a/internal/proxy/vector_matching.go b/internal/proxy/vector_matching.go index c3b2ee72a..ad75a1c00 100644 --- a/internal/proxy/vector_matching.go +++ b/internal/proxy/vector_matching.go @@ -624,7 +624,7 @@ func applyArithmeticOp(left, right float64, op string) float64 { } } -// --- group() / label_replace() / label_join() post-processing --- +// --- label_replace() post-processing --- // matrixResponseRW is a minimal struct for parsing and re-encoding a Loki matrix response. type matrixResponseRW struct { @@ -638,27 +638,6 @@ type matrixResponseRW struct { } `json:"data"` } -// applyGroupNormalization sets every sample value in a matrix response to "1", -// implementing group() semantics (return 1 for each series that has data). -func applyGroupNormalization(body []byte) []byte { - var resp matrixResponseRW - if err := json.Unmarshal(body, &resp); err != nil || resp.Data.ResultType != "matrix" { - return body - } - for i := range resp.Data.Result { - for j := range resp.Data.Result[i].Values { - if len(resp.Data.Result[i].Values[j]) >= 2 { - resp.Data.Result[i].Values[j][1] = "1" - } - } - } - out, err := json.Marshal(resp) - if err != nil { - return body - } - return out -} - // applyLabelReplace applies label_replace semantics to all series in a matrix response. // For each series: new_value = regexp.ReplaceAll(src_label_value, replacement). // If new_value is non-empty, dst_label is set; otherwise dst_label is deleted. @@ -690,29 +669,6 @@ func applyLabelReplace(body []byte, spec translator.LabelReplaceSpec) []byte { return out } -// applyLabelJoin applies label_join semantics to all series in a matrix response. -// dst_label is set to the src_labels values joined by separator. -func applyLabelJoin(body []byte, spec translator.LabelJoinSpec) []byte { - var resp matrixResponseRW - if err := json.Unmarshal(body, &resp); err != nil || resp.Data.ResultType != "matrix" { - return body - } - for i, s := range resp.Data.Result { - parts := make([]string, 0, len(spec.SrcLabels)) - for _, src := range spec.SrcLabels { - if v := s.Metric[src]; v != "" { - parts = append(parts, v) - } - } - resp.Data.Result[i].Metric[spec.DstLabel] = strings.Join(parts, spec.Separator) - } - out, err := json.Marshal(resp) - if err != nil { - return body - } - return out -} - // sampleTimeSeconds reads a sample time of a decoded response: a JSON number, or // its text. func sampleTimeSeconds(v interface{}) (float64, bool) { diff --git a/internal/translator/partial_coverage_internal_test.go b/internal/translator/partial_coverage_internal_test.go index 18e5565cd..9ca7fdabb 100644 --- a/internal/translator/partial_coverage_internal_test.go +++ b/internal/translator/partial_coverage_internal_test.go @@ -5,24 +5,6 @@ import ( "testing" ) -func TestParseGroupMarker(t *testing.T) { - t.Run("with_marker_strips_and_returns_true", func(t *testing.T) { - // The marker is appended as a suffix; reconstruct one by piggybacking - // on a known query that includes it. - base := `{app="x"} | stats count()` - // Round-trip via GroupMarker — assert via output of helpers. - // We can't reach unexported groupMarker, but we can call ParseGroupMarker - // on a string built to include the suffix. - q, ok := ParseGroupMarker(base) - if ok { - t.Fatalf("plain string should not match marker: ok=%v stripped=%q", ok, q) - } - if q != base { - t.Errorf("plain string should be returned unchanged: %q", q) - } - }) -} - func TestDurationSeconds(t *testing.T) { tests := []struct { in string diff --git a/internal/translator/translator.go b/internal/translator/translator.go index 8d0baaeb1..d8c222f62 100644 --- a/internal/translator/translator.go +++ b/internal/translator/translator.go @@ -30,9 +30,7 @@ const emptyByGrouping = "__lvp_by_empty__" // Marker suffixes embedded in translated queries for proxy post-processing. // The proxy strips them before forwarding to VL and applies the corresponding transform. const ( - groupMarker = "__lvp_group__" labelReplacePrefix = "__lvp_lr:" - labelJoinPrefix = "__lvp_lj:" ) // LabelReplaceSpec holds the parsed arguments of a label_replace() expression. @@ -43,22 +41,6 @@ type LabelReplaceSpec struct { Regex string } -// LabelJoinSpec holds the parsed arguments of a label_join() expression. -type LabelJoinSpec struct { - DstLabel string - Separator string - SrcLabels []string -} - -// ParseGroupMarker reports whether the translated query carries a group-normalization -// marker, and returns the clean query (marker stripped). -func ParseGroupMarker(query string) (string, bool) { - if strings.HasSuffix(query, groupMarker) { - return query[:len(query)-len(groupMarker)], true - } - return query, false -} - // ParseLabelReplaceMarker extracts a label_replace spec embedded in a translated query. // Returns the clean query (marker stripped) and the spec, or nil if absent. func ParseLabelReplaceMarker(query string) (string, *LabelReplaceSpec) { @@ -123,38 +105,6 @@ func ParseAllLabelReplaceMarkers(query string) (string, []LabelReplaceSpec) { return query, specs } -// ParseLabelJoinMarker extracts a label_join spec embedded in a translated query. -// Returns the clean query (marker stripped) and the spec, or nil if absent. -func ParseLabelJoinMarker(query string) (string, *LabelJoinSpec) { - idx := strings.LastIndex(query, labelJoinPrefix) - if idx < 0 { - return query, nil - } - payload := strings.TrimSuffix(query[idx+len(labelJoinPrefix):], "__") - b, err := base64.StdEncoding.DecodeString(payload) - if err != nil { - return query, nil - } - s := string(b) - n1 := strings.IndexByte(s, 0) - if n1 < 0 { - return query, nil - } - dst := s[:n1] - rest := s[n1+1:] - n2 := strings.IndexByte(rest, 0) - if n2 < 0 { - return query, nil - } - sep := rest[:n2] - srcsRaw := rest[n2+1:] - var srcs []string - if srcsRaw != "" { - srcs = strings.Split(srcsRaw, "\x01") - } - return query[:idx], &LabelJoinSpec{DstLabel: dst, Separator: sep, SrcLabels: srcs} -} - // DropCondition represents a matcher-form conditional drop from `| drop field=value`. // Only entries where the field value satisfies the condition have the field removed. type DropCondition struct { @@ -456,15 +406,12 @@ func translateLogQLFull(logql string, labelFn LabelTranslateFunc, streamFields m return "*", nil } - // label_replace and label_join are transform wrappers around a complete metric + // label_replace is a transform wrapper around a complete metric // expression. Handle them before the without/binary/metric path so the inner // expression is translated correctly and the marker is appended last. if result, ok := tryTranslateLabelReplace(logql, labelFn); ok { return applyDerivedGroupings(result, caps, streamFields), nil } - if result, ok := tryTranslateLabelJoin(logql, labelFn); ok { - return applyDerivedGroupings(result, caps, streamFields), nil - } // Extract without() labels before translation. // The proxy will post-process VL results to remove these labels from metric grouping. @@ -1810,31 +1757,6 @@ func tryTranslateLabelReplace(logql string, labelFn LabelTranslateFunc) (string, return translated + labelReplacePrefix + payload + "__", true } -// tryTranslateLabelJoin handles label_join(v, "dst", "sep", "src1", ...). -// It translates the inner expression and embeds a marker for proxy post-processing. -func tryTranslateLabelJoin(logql string, labelFn LabelTranslateFunc) (string, bool) { - const prefix = "label_join(" - if !strings.HasPrefix(logql, prefix) { - return "", false - } - inner, rest, ok := splitFuncFirstArg(logql[len(prefix):]) - if !ok { - return "", false - } - args := parseAllStringArgs(rest) - if len(args) < 3 { // dst + sep + at least one src - return "", false - } - translated, err := translateLogQLFull(inner, labelFn, nil, logsql.Capabilities{}) - if err != nil { - return "", false - } - payload := base64.StdEncoding.EncodeToString([]byte( - args[0] + "\x00" + args[1] + "\x00" + strings.Join(args[2:], "\x01"), - )) - return translated + labelJoinPrefix + payload + "__", true -} - // tryTranslateMetricQuery attempts to translate a metric/aggregation query. func tryTranslateMetricQuery(logql string, labelFn LabelTranslateFunc) (string, bool) { return tryTranslateMetricQueryWithout(logql, labelFn, false) @@ -1869,13 +1791,6 @@ func tryTranslateMetricQueryWithout(logql string, labelFn LabelTranslateFunc, ha // Try to match outer aggregation: sum(...) by (labels) outerAgg, innerExpr, byLabels := extractOuterAggregation(logql) - // group() returns 1 for every series that has data. Translate the inner metric - // normally (for grouping/presence), then the proxy normalises all values to 1. - isGroup := outerAgg == "group" - if isGroup { - outerAgg = "" - } - // count_values() groups by the VALUES of the inner metric — VL has no equivalent. if outerAgg == "count_values" { return "", false // caught by the count_values guard in translateLogQLFull @@ -1888,9 +1803,6 @@ func tryTranslateMetricQueryWithout(logql string, labelFn LabelTranslateFunc, ha // Special handling for quantile_over_time(phi, {query} | unwrap field [duration]) if result, ok := tryTranslateQuantileOverTime(innerExpr, outerAgg, byLabels, labelFn); ok { - if isGroup { - return result + groupMarker, true - } return result, true } @@ -1934,9 +1846,6 @@ func tryTranslateMetricQueryWithout(logql string, labelFn LabelTranslateFunc, ha } } if rateResult, ok := buildRateLikeQuery(logsqlQuery, query, rateExpr, duration, outerAgg, byLabels, labelFn, hasWithout, rateGate, rateConv); ok { - if isGroup { - return rateResult + groupMarker, true - } return rateResult, true } } @@ -2006,9 +1915,6 @@ func tryTranslateMetricQueryWithout(logql string, labelFn LabelTranslateFunc, ha // aggregates the rows of each remaining label set. result = addByClauseLast(result, innerBy, nil) } - if isGroup { - return result + groupMarker, true - } return result, true } else { result = logsqlQuery + " " + logsql.PipeStats{Funcs: []logsql.StatsFuncAlias{{Func: logsql.DeferredExpr{Raw: logsqlFunc}}}}.String() @@ -2022,9 +1928,6 @@ func tryTranslateMetricQueryWithout(logql string, labelFn LabelTranslateFunc, ha } } - if isGroup { - return result + groupMarker, true - } return result, true } @@ -2036,9 +1939,6 @@ func tryTranslateMetricQueryWithout(logql string, labelFn LabelTranslateFunc, ha // Alias the last stats result so the outer aggregation can reference it. innerAliased := translatedInner + " as __lvp_inner" if outerResult, ok := applyOuterAggregation(innerAliased, outerAgg, "__lvp_inner"); ok { - if isGroup { - return outerResult + groupMarker, true - } return outerResult, true } } @@ -2265,8 +2165,8 @@ var ( withoutMarkerRE = regexp.MustCompile(`\bwithout\s*\(([^)]+)\)`) goTemplateRE = regexp.MustCompile(`\{\{\s*\.([\w.]+)\s*\}\}`) vectorMatchRE = regexp.MustCompile(`\s+(on|ignoring|group_left|group_right)\s*\(([^)]*)\)`) - aggByBeforeRE = regexp.MustCompile(`^(sum|avg|max|min|count|topk|bottomk|stddev|stdvar|sort|sort_desc|group|count_values)\s+(?:by|without)\s*\(([^)]*)\)\s*\(`) - aggFuncRE = regexp.MustCompile(`^(sum|avg|max|min|count|topk|bottomk|stddev|stdvar|sort|sort_desc|group|count_values)\s*\(`) + aggByBeforeRE = regexp.MustCompile(`^(sum|avg|max|min|count|topk|bottomk|stddev|stdvar|sort|sort_desc|count_values)\s+(?:by|without)\s*\(([^)]*)\)\s*\(`) + aggFuncRE = regexp.MustCompile(`^(sum|avg|max|min|count|topk|bottomk|stddev|stdvar|sort|sort_desc|count_values)\s*\(`) aggByAfterRE = regexp.MustCompile(`^(?:by|without)\s*\(([^)]+)\)`) durationPartRE = regexp.MustCompile(`([0-9]*\.?[0-9]+)(ns|us|µs|ms|s|m|h|d|w|y)`) ) diff --git a/internal/translator/translator_test.go b/internal/translator/translator_test.go index 7e19eb15f..2aeead906 100644 --- a/internal/translator/translator_test.go +++ b/internal/translator/translator_test.go @@ -766,52 +766,6 @@ func TestTopkTranslation(t *testing.T) { } } -func TestGroupTranslation(t *testing.T) { - cases := []struct { - in string - wantHas string - wantErr string - }{ - { - in: `group(rate({app="api"}[5m])) by (level)`, - wantHas: "__lvp_group__", - }, - { - in: `group(count_over_time({app="api"}[5m])) by (status)`, - wantHas: "__lvp_group__", - }, - { - // group result must still contain a by-label clause - in: `group(rate({app="api"}[5m])) by (level)`, - wantHas: "level", - }, - } - for _, tc := range cases { - result, err := TranslateLogQL(tc.in) - if tc.wantErr != "" { - if err == nil || !strings.Contains(err.Error(), tc.wantErr) { - t.Errorf("query %q: want error %q, got result=%q err=%v", tc.in, tc.wantErr, result, err) - } - continue - } - if err != nil { - t.Errorf("query %q: unexpected error: %v", tc.in, err) - continue - } - if !strings.Contains(result, tc.wantHas) { - t.Errorf("query %q:\n got: %q\n want substring: %q", tc.in, result, tc.wantHas) - } - // Parsed group marker must round-trip cleanly - clean, ok := ParseGroupMarker(result) - if !ok { - t.Errorf("query %q: ParseGroupMarker found no marker in %q", tc.in, result) - } - if strings.Contains(clean, "__lvp_group__") { - t.Errorf("query %q: marker not fully stripped: %q", tc.in, clean) - } - } -} - func TestCountValuesError(t *testing.T) { cases := []string{ `count_values("status", count_over_time({app="api"}[5m]))`, @@ -928,51 +882,6 @@ func TestParseAllLabelReplaceMarkers(t *testing.T) { }) } -func TestLabelJoinTranslation(t *testing.T) { - cases := []struct { - in string - wantHas string - specDst string - specSep string - specSrc []string - }{ - { - in: `label_join(rate({app="api"}[5m]), "service_host", "/", "service", "host")`, - wantHas: `app:="api"`, - specDst: "service_host", - specSep: "/", - specSrc: []string{"service", "host"}, - }, - } - for _, tc := range cases { - result, err := TranslateLogQL(tc.in) - if err != nil { - t.Errorf("query %q: unexpected error: %v", tc.in, err) - continue - } - if !strings.Contains(result, tc.wantHas) { - t.Errorf("query %q: VL part missing %q in %q", tc.in, tc.wantHas, result) - } - clean, spec := ParseLabelJoinMarker(result) - if spec == nil { - t.Errorf("query %q: ParseLabelJoinMarker returned nil in %q", tc.in, result) - continue - } - if spec.DstLabel != tc.specDst { - t.Errorf("query %q: DstLabel=%q want %q", tc.in, spec.DstLabel, tc.specDst) - } - if spec.Separator != tc.specSep { - t.Errorf("query %q: Separator=%q want %q", tc.in, spec.Separator, tc.specSep) - } - if len(spec.SrcLabels) != len(tc.specSrc) { - t.Errorf("query %q: SrcLabels=%v want %v", tc.in, spec.SrcLabels, tc.specSrc) - } - if strings.Contains(clean, "__lvp_lj:") { - t.Errorf("query %q: marker not stripped from clean query %q", tc.in, clean) - } - } -} - func TestParseBareDropFields(t *testing.T) { tests := []struct { name string From a5aaaa31f3710e408ff1834cfd8908bfbd9925d8 Mon Sep 17 00:00:00 2001 From: szibis Date: Mon, 12 Oct 2026 13:54:19 +0200 Subject: [PATCH 3/5] test: compare group(), label_join() and uppercase keywords with Loki, and document the change The e2e suite runs the queries Loki rejects against Loki and every Loki-compatible proxy on both query endpoints and compares status and text, and runs uppercase keyword queries and compares the series. group() and label_join() move from the parity and known-gap lists to the error parity list. --- CHANGELOG.md | 29 ++++ docs/KNOWN_ISSUES.md | 3 +- docs/logql-parser.md | 8 +- docs/roadmap.md | 4 +- docs/translation-reference.md | 7 +- test/e2e-compat/logql_exhaustive_test.go | 15 +- test/e2e-compat/operators_not_in_loki_test.go | 150 ++++++++++++++++++ 7 files changed, 193 insertions(+), 23 deletions(-) create mode 100644 test/e2e-compat/operators_not_in_loki_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index feca92cbd..0a2b5b3a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,35 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **`group()`, `label_join()` and every other name Loki does not have are a 400 + parse error, and keywords are accepted in any case (#TBD).** Loki 3.7.7 has no + `group` aggregation and no `label_join`: its lexer makes a function token only + of the names in `functionTokens` (the range operations, `sum` `avg` `max` + `min` `count` `stddev` `stdvar` `bottomk` `topk` `sort` `sort_desc` + `approx_topk`, `vector`, `label_replace` and the unwrap conversions), so any + other name is an `IDENTIFIER` the grammar cannot place and the query fails + with `parse error at line 1, col N: syntax error: unexpected IDENTIFIER`, N + being the column of the name. The proxy kept any unknown `name(...)` as an + opaque call: `group(count_over_time({...}[5m]))` answered a series valued 1 + per stream on `query_range` and a line count on `query`, `label_join(...)` + joined labels in the response, and `abs`, `clamp_min`, `round`, `ln`, + `scalar`, `quantile`, `time()` and `count_values()` reached the translator. + Both endpoints now answer Loki's status and text, byte for byte, with the + column of the name wherever it stands (`sum(group(...))` is col 5, and inside + a `label_replace` argument it is relative to the whole query); only + `label_replace`, `vector` and `approx_topk` stay opaque calls. Loki's lexer + also lower-cases a word before it looks it up, so `TOPK(3, ...)`, `SUM BY + (a) (...)`, `RATE(...)`, `OFFSET`, `UNWRAP`, `BOOL`, `ON` and `GROUP_LEFT` are + the keywords they spell, while a label called `Rate` or `Count` stays a + label. The proxy matched the lower-case spelling only: `TOPK(...)` fell + through the opaque path and answered a `streams` result, and `SUM BY (app) + (...)` was a parse error. The scanner now reads keywords in any case, and + the `query` parameter is written with lower-case keywords before a handler + reads it. The group-normalisation and `label_join` post-processing of the + response, now unreachable, is removed. + ## [2.11.2] - 2026-10-12 ### CI diff --git a/docs/KNOWN_ISSUES.md b/docs/KNOWN_ISSUES.md index 1f1a927a9..7ab2d0ced 100644 --- a/docs/KNOWN_ISSUES.md +++ b/docs/KNOWN_ISSUES.md @@ -102,8 +102,7 @@ These are not current open issues in this codebase: - route-aware proxy, cache, and upstream request telemetry - prefixed app metrics under `loki_vl_proxy_*` with CI guard coverage - `label_replace()` — fully implemented in translator with proxy-side post-processing (v1.21.0) -- `label_join()` — fully implemented in translator with proxy-side post-processing (v1.21.0) -- `group()` — implemented: inner metric translated normally, proxy normalises all matrix values to `1` (v1.21.0) +- `label_join()` and `group()` — not in Loki 3.7.7's LogQL; both are a 400 parse error like Loki's (they were proxy-only extensions from v1.21.0) - bare label matcher malformed VL output — queries like `app="value"` (missing braces) now return a descriptive HTTP 400 instead of silently producing double-quoted VL syntax (v1.20.0) - `detected_level` on log responses — derived per row from stored fields and the line with Loki's rules, including `unknown`, normalised values and structured metadata placement under `categorize-labels` (Unreleased; replaces the v1.20.0 JSON/logfmt inference) - circuit breaker sliding window — failure counting uses a 30-second sliding window; sporadic slow-query resets no longer open the breaker (v1.18.0) diff --git a/docs/logql-parser.md b/docs/logql-parser.md index 9a6322170..4096f0728 100644 --- a/docs/logql-parser.md +++ b/docs/logql-parser.md @@ -46,7 +46,7 @@ flowchart TD LQ["*LogQuery\n(stream + pipeline)"] RA["*RangeAggregation\n(rate, max_over_time…)"] BE["*BinOpExpr\n(left op right)"] - OM["*OpaqueMetricExpr\n(label_replace, label_join…)"] + OM["*OpaqueMetricExpr\n(label_replace, vector…)"] VA["*VectorAggregation\n(sum by, topk…)"] end @@ -128,7 +128,7 @@ Expr (interface) ├── *VectorAggregation sum by (label) (rate(...)) / topk(5, ...) ├── *BinOpExpr left op right, optional VectorMatching ├── *LiteralExpr scalar 3.14 -└── *OpaqueMetricExpr label_replace(...), label_join(...) — raw pass-through +└── *OpaqueMetricExpr label_replace(...), vector(...) — raw pass-through ``` ### Opaque pass-through nodes @@ -137,7 +137,7 @@ Two node types capture content without parsing it: - **`LabelFilterStage`** / **`LabelFormatStage`** — the inner expression grammar for `| level > 1` or `| label_format dst=src` is complex and context-dependent. These stages capture the raw text after the keyword so the translator sees exactly what Loki would see. -- **`OpaqueMetricExpr`** — metric-level functions that aren't yet expressible in the AST (e.g. `label_replace`, `label_join`). The parser captures the full raw text including balanced parentheses and returns it verbatim so VictoriaLogs receives the original expression unchanged. +- **`OpaqueMetricExpr`** — metric-level functions that aren't yet expressible in the AST (`label_replace`, `vector`, `approx_topk`; any other name is a Loki parse error). The parser captures the full raw text including balanced parentheses and returns it verbatim so VictoriaLogs receives the original expression unchanged. ## Scanner @@ -262,7 +262,7 @@ The AST-to-AST translator (`logql.Translate`) maps LogQL pipeline stages to `log | `*RangeAggregation` | any | `errFallthrough` | Metric path via string translator | | `*VectorAggregation` | any | `errFallthrough` | Metric path via string translator | | `*BinOpExpr` | any | `errFallthrough` | Binary metric path | -| `*OpaqueMetricExpr` | any | raw pass-through | `label_replace`, `label_join` etc. | +| `*OpaqueMetricExpr` | any | raw pass-through | `label_replace`, `vector`, `approx_topk` | `errFallthrough` is a package-private sentinel — callers route to `TranslateLogQLWithCapabilities` unchanged. diff --git a/docs/roadmap.md b/docs/roadmap.md index 0374a06ea..b7a501963 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -51,9 +51,9 @@ description: Shipped milestones and committed, already-decided work for loki-vl- - [x] `IsScalar` supports negative and scientific notation - [x] Circuit breaker half-open metrics fix - [x] Tenant map reload race condition fix -- [x] `group()` outer aggregation — inner metric translated normally, proxy normalises all values to `1` (v1.21.x) +- [x] ~~`group()` outer aggregation~~ — removed: Loki has no `group()`, the proxy answers its 400 parse error - [x] `label_replace()` — proxy post-processing via marker suffix; Prometheus no-match semantics (v1.21.x) -- [x] `label_join()` — proxy post-processing via marker suffix; missing src labels skipped (v1.21.x) +- [x] ~~`label_join()`~~ — removed: Loki has no `label_join()`, the proxy answers its 400 parse error - [x] `count_values()` — returns descriptive error (not translatable to VL; VL has no group-by-value primitive) (v1.21.x) - [x] Circuit breaker sliding-window failure counting — 30s window replaces consecutive-failure model; sporadic slow-query resets no longer open the breaker (v1.18.0) - [x] Bare label matcher error — `app="value"` (missing braces) returns HTTP 400 with descriptive Loki-style parse error instead of silently emitting malformed VL syntax (v1.20.0) diff --git a/docs/translation-reference.md b/docs/translation-reference.md index 334188c34..7c07d2060 100644 --- a/docs/translation-reference.md +++ b/docs/translation-reference.md @@ -148,11 +148,11 @@ The rejected rows form groups of their own, so every other group aggregates exac | `avg(rate({...}[5m])) by (x)` | same normalized-rate path grouped by `(x)` | | `topk(10, rate({...}[5m]))` | normalized-rate path with stream grouping; on range queries the proxy ranks series independently at each timestamp, so a range can return more than `k` distinct series, each with only its winning samples | -Supported: `sum`, `avg`, `max`, `min`, `count`, `topk`, `bottomk`, `stddev`, `stdvar`, `sort`, `sort_desc`, `group`, `label_replace`, `label_join`. +Supported: `sum`, `avg`, `max`, `min`, `count`, `topk`, `bottomk`, `stddev`, `stdvar`, `sort`, `sort_desc`, `label_replace`. -`group()` returns `1` for every series that has data. The inner metric is translated normally for grouping/presence detection; the proxy normalises all result values to `1` in post-processing. +Loki has no `group()` and no `label_join()` (nor the PromQL math and time functions): any name outside Loki's function tokens is a 400 `parse error at line 1, col N: syntax error: unexpected IDENTIFIER` on both query endpoints. Keywords are read in any case, as Loki's lexer does (`TOPK(3, ...)`, `SUM BY (a) (...)`). -`label_replace(v, dst, repl, src, regex)` and `label_join(v, dst, sep, src1, ...)` are handled via a marker-suffix pattern: the inner expression is translated to VL, the spec is base64-encoded and embedded as a query suffix, the proxy strips the marker before sending to VL and applies the label operation to the matrix response. +`label_replace(v, dst, repl, src, regex)` is handled via a marker-suffix pattern: the inner expression is translated to VL, the spec is base64-encoded and embedded as a query suffix, the proxy strips the marker before sending to VL and applies the label operation to the matrix response. `count_values(label, v)` is not translatable — VictoriaLogs has no primitive that groups by metric values. Queries using `count_values` return a descriptive error. @@ -186,7 +186,6 @@ The following Loki semantics are implemented in the proxy to bridge gaps where V | Subquery `max_over_time(rate(...)[1h:5m])` | Not LogQL: Loki's grammar has no `[range:step]` form. Rejected with Loki's HTTP 400 parse error (`syntax error: unexpected RATE, expecting NUMBER or { or (`) before any backend call | | Range-vector metric windows (`*_over_time`, `rate`, `count_over_time`, `bytes_*`, `rate_counter`) | Proxy applies Loki-compatible sliding-window evaluation over step-aligned timestamps and emits matrix/vector responses | | `label_replace(expr, dst, repl, src, regex)` | Proxy post-processing: inner expr translated to VL, spec embedded as marker, applied to matrix response (Prometheus semantics: no-match leaves dst unchanged) | -| `label_join(v, dst, sep, src1, ...)` | Proxy post-processing: same marker pattern as `label_replace`; missing src labels are skipped | ### Parser-Stage Metric Compatibility Path diff --git a/test/e2e-compat/logql_exhaustive_test.go b/test/e2e-compat/logql_exhaustive_test.go index 70f1f9226..cb143b27c 100644 --- a/test/e2e-compat/logql_exhaustive_test.go +++ b/test/e2e-compat/logql_exhaustive_test.go @@ -82,8 +82,9 @@ func TestLogQL_Exhaustive_ErrorParity(t *testing.T) { // ── count_values (not translatable to VL) ── {"count_values_metric", `count_values("app", count_over_time({app="api-gateway"}[5m]))`, "count_values"}, - // ── label_replace / label_join applied to a log stream ── - // Note: proxy accepts these (proxy extension); Loki rejects → tracked in TestLogQL_Exhaustive_KnownGaps + // ── group() and label_join() do not exist in Loki 3.7.7 LogQL ── + {"group_aggregation", `group(sum by (app)(count_over_time({env="production"}[5m])))`, "unknown_function"}, + {"label_join_function", `label_join(sum by (app)(count_over_time({env="production"}[5m])), "app_copy", "", "app")`, "unknown_function"}, // ── absent_over_time without range ── {"absent_over_time_no_range", `absent_over_time({app="api-gateway"})`, "malformed"}, @@ -379,15 +380,13 @@ func TestLogQL_Exhaustive_QueryParity(t *testing.T) { {"wildcard_regex", `{app=~".+",env="production"}`, "basic"}, {"multiple_not_equal", `{app!="nginx-ingress",env="production",level!="debug"}`, "basic"}, - // ── label_replace / label_join / group() ────────────────────────────── + // ── label_replace / aggregations that regroup ───────────────────────── {"label_replace_basic", `label_replace(sum by (level)(count_over_time({app="api-gateway"}[5m])), "level_alias", "$1", "level", "(.*)")`, "label_transform"}, {"label_replace_rewrite", `label_replace(sum by (app)(rate({env="production"}[5m])), "service", "$1", "app", "(.*)")`, "label_transform"}, {"label_replace_no_match", `label_replace(sum by (level)(count_over_time({app="api-gateway"}[5m])), "new_label", "default", "level", "^nonexistent$")`, "label_transform"}, {"label_replace_multi_result", `label_replace(sum by (app, level)(count_over_time({env="production"}[5m])), "app_env", "$1-prod", "app", "(.*)")`, "label_transform"}, {"avg_without_app", `avg without(app)(rate({env="production"}[5m]))`, "label_transform"}, {"count_by_level_agg", `count by(level)(count_over_time({env="production"}[5m]))`, "label_transform"}, - {"group_outer_agg", `group(sum by (app)(count_over_time({env="production"}[5m])))`, "label_transform"}, - {"group_without", `group(sum without (level)(count_over_time({env="production"}[5m])))`, "label_transform"}, // ── Subqueries ──────────────────────────────────────────────────────── // Loki 3.7.1 and the proxy both reject every subquery form (matching 400); @@ -956,12 +955,6 @@ func TestLogQL_Exhaustive_KnownGaps(t *testing.T) { "proxy_extension", 400, 200, "proxy supports @ start() modifier; Loki 3.7.1 parse error", }, - { - "label_join_function", - `label_join(sum by (app)(count_over_time({env="production"}[5m])), "app_copy", "", "app")`, - "proxy_extension", 400, 200, - "label_join() is a proxy post-processing extension; not in Loki 3.7.1 LogQL", - }, // stddev_outer_aggregation, stdvar_outer_aggregation, quantile_neg_error_code were // proxy_bug/code_mismatch gaps but are now fixed — moved to QueryParity/ErrorParity. // Subquery extensions were removed: every [range:step] form is now rejected diff --git a/test/e2e-compat/operators_not_in_loki_test.go b/test/e2e-compat/operators_not_in_loki_test.go new file mode 100644 index 000000000..8a7e8fde8 --- /dev/null +++ b/test/e2e-compat/operators_not_in_loki_test.go @@ -0,0 +1,150 @@ +//go:build e2e + +package e2e_compat + +import ( + "encoding/json" + "net/http" + "net/url" + "sort" + "strconv" + "strings" + "testing" + "time" +) + +// Loki has no group(), no label_join() and none of the PromQL math, time or +// quantile functions: a name outside the lexer's function tokens is an +// IDENTIFIER, and the query is a 400 parse error at the column of that name. +// Every Loki-compatible proxy answers the same status and the same bytes, on +// the range and the instant endpoint, wherever the name stands in the query. +// +// conformance: semantics/proxy-accepts-operators-not-in-loki +func TestCompat_OperatorsNotInLokiRejected(t *testing.T) { + ensureDataIngested(t) + end := time.Now() + window := func(params url.Values) url.Values { + out := cloneQueryValues(params) + out.Set("start", strconv.FormatInt(end.Add(-time.Hour).UnixNano(), 10)) + out.Set("end", strconv.FormatInt(end.UnixNano(), 10)) + out.Set("time", strconv.FormatInt(end.UnixNano(), 10)) + out.Set("step", "60") + return out + } + queries := []string{ + `group(count_over_time({app="api-gateway"}[5m]))`, + `GROUP(count_over_time({app="api-gateway"}[5m]))`, + `group by (app) (count_over_time({app="api-gateway"}[5m]))`, + `group(sum by (app) (count_over_time({env="production"}[5m])))`, + `sum(group(rate({app="api-gateway"}[5m])))`, + `label_join(rate({app="api-gateway"}[5m]), "d", ",", "app", "level")`, + `LABEL_JOIN(rate({app="api-gateway"}[5m]), "d", ",", "app")`, + `label_join(sum by (app) (count_over_time({env="production"}[5m])), "app_copy", "", "app")`, + `label_replace(label_join(rate({app="api-gateway"}[5m]), "d", ",", "app"), "a", "b", "c", "d")`, + `sum(rate({app="api-gateway"}[5m])) + clamp_min(rate({app="api-gateway"}[5m]), 1)`, + `abs(rate({app="api-gateway"}[5m]))`, + `round(rate({app="api-gateway"}[5m]))`, + `ln(rate({app="api-gateway"}[5m]))`, + `scalar(rate({app="api-gateway"}[5m]))`, + `quantile(0.5, rate({app="api-gateway"}[5m]))`, + `count_values("v", count_over_time({app="api-gateway"}[5m]))`, + `time()`, + } + for _, path := range []string{"/loki/api/v1/query_range", "/loki/api/v1/query"} { + for _, query := range queries { + t.Run(path+" "+query, func(t *testing.T) { + params := window(url.Values{"query": {query}}) + lokiStatus, lokiBody := rejectedQueryGet(t, lokiURL, path, params, "0", nil) + lokiMsg := strings.TrimSpace(string(lokiBody)) + if lokiStatus != http.StatusBadRequest || !strings.HasPrefix(lokiMsg, "parse error at line ") { + t.Fatalf("Loki fixture drifted: want a 400 parse error, got %d %s", lokiStatus, lokiBody) + } + for name, base := range lokiProfileProxies() { + status, body := rejectedQueryGet(t, base, path, params, "0", nil) + var envelope struct { + Status, ErrorType, Error string + } + if status != http.StatusBadRequest || json.Unmarshal(body, &envelope) != nil || + envelope.Status != "error" || envelope.ErrorType != "bad_data" || envelope.Error != lokiMsg { + t.Errorf("%s proxy: %d %s, want 400 bad_data %q", name, status, body, lokiMsg) + } + } + }) + } + } +} + +// Loki lower-cases a word before it looks it up, so SUM, TOPK(3, ...), RATE, +// BY, OFFSET, UNWRAP, BOOL, ON and GROUP_LEFT are the keywords they spell. The +// proxy answers 200 with the series Loki answers. +// +// conformance: semantics/proxy-accepts-operators-not-in-loki +func TestCompat_UppercaseKeywordsMatchLoki(t *testing.T) { + ensureDataIngested(t) + end := time.Now() + queries := []string{ + `TOPK(3, SUM BY (app) (RATE({env="production"}[5m])))`, + `SUM BY (app) (COUNT_OVER_TIME({env="production"}[5m]))`, + `SUM(RATE({env="production"}[5m])) BY (app)`, + `SUM WITHOUT (level) (COUNT_OVER_TIME({env="production"}[5m]))`, + `MAX BY (app) (BYTES_OVER_TIME({env="production"}[5m]))`, + `SUM BY (app) (COUNT_OVER_TIME({env="production"}[5m])) / ON (app) SUM BY (app) (COUNT_OVER_TIME({env="production"}[5m]))`, + `SUM BY (app) (COUNT_OVER_TIME({env="production"}[5m])) > BOOL 0`, + `SUM BY (app) (COUNT_OVER_TIME({env="production"}[5m] OFFSET 1m))`, + `SUM BY (app) (SUM_OVER_TIME({env="production"} | JSON | UNWRAP duration_ms [5m]))`, + } + type result struct { + status int + resultType string + series []string + } + run := func(base, path string, params url.Values) result { + status, body := rejectedQueryGet(t, base, path, params, "0", nil) + out := result{status: status} + var resp struct { + Data struct { + ResultType string `json:"resultType"` + Result []struct { + Metric map[string]string `json:"metric"` + } `json:"result"` + } `json:"data"` + } + if status == http.StatusOK && json.Unmarshal(body, &resp) == nil { + out.resultType = resp.Data.ResultType + for _, s := range resp.Data.Result { + keys := make([]string, 0, len(s.Metric)) + for k, v := range s.Metric { + keys = append(keys, k+"="+v) + } + sort.Strings(keys) + out.series = append(out.series, strings.Join(keys, ",")) + } + sort.Strings(out.series) + } + return out + } + for _, path := range []string{"/loki/api/v1/query_range", "/loki/api/v1/query"} { + for _, query := range queries { + t.Run(path+" "+query, func(t *testing.T) { + params := url.Values{ + "query": {query}, + "start": {strconv.FormatInt(end.Add(-time.Hour).UnixNano(), 10)}, + "end": {strconv.FormatInt(end.UnixNano(), 10)}, + "time": {strconv.FormatInt(end.UnixNano(), 10)}, + "step": {"60"}, + } + loki := run(lokiURL, path, params) + if loki.status != http.StatusOK { + t.Fatalf("Loki fixture drifted: want 200, got %d", loki.status) + } + for name, base := range lokiProfileProxies() { + got := run(base, path, params) + if got.status != loki.status || got.resultType != loki.resultType || strings.Join(got.series, "|") != strings.Join(loki.series, "|") { + t.Errorf("%s proxy: status %d %s series %q, Loki %d %s series %q", + name, got.status, got.resultType, got.series, loki.status, loki.resultType, loki.series) + } + } + }) + } + } +} From c5f96c1c471726be4412e92bbdd3ec03b9135648 Mon Sep 17 00:00:00 2001 From: szibis Date: Mon, 12 Oct 2026 14:00:31 +0200 Subject: [PATCH 4/5] fix: carry the lower-cased query to the multi-tenant fan-out, and compare the proxy with itself in the e2e The multi-tenant fan-out re-reads the query from the URL, so the lower-case rewrite covers the URL query as well as the parsed form. The proxy tests now send each uppercase query as a GET, a form-encoded POST and a multi-tenant GET. The e2e compares the upper-case answer of the proxy with its own lower-case answer; Loki only has to answer 200 for both spellings. --- CHANGELOG.md | 39 +++++------ internal/proxy/http_utils.go | 4 ++ internal/proxy/unknown_logql_names_test.go | 51 ++++++++++---- test/e2e-compat/operators_not_in_loki_test.go | 68 +++++++++++-------- 4 files changed, 98 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a2b5b3a2..d39f481b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,31 +10,32 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **`group()`, `label_join()` and every other name Loki does not have are a 400 - parse error, and keywords are accepted in any case (#TBD).** Loki 3.7.7 has no - `group` aggregation and no `label_join`: its lexer makes a function token only - of the names in `functionTokens` (the range operations, `sum` `avg` `max` - `min` `count` `stddev` `stdvar` `bottomk` `topk` `sort` `sort_desc` + parse error, and keywords are accepted in any case (#TBD).** Loki 3.7.7 has + no `group` aggregation and no `label_join`: its lexer makes a function token + only of the names in `functionTokens` (the range operations, `sum` `avg` + `max` `min` `count` `stddev` `stdvar` `bottomk` `topk` `sort` `sort_desc` `approx_topk`, `vector`, `label_replace` and the unwrap conversions), so any other name is an `IDENTIFIER` the grammar cannot place and the query fails with `parse error at line 1, col N: syntax error: unexpected IDENTIFIER`, N being the column of the name. The proxy kept any unknown `name(...)` as an opaque call: `group(count_over_time({...}[5m]))` answered a series valued 1 per stream on `query_range` and a line count on `query`, `label_join(...)` - joined labels in the response, and `abs`, `clamp_min`, `round`, `ln`, - `scalar`, `quantile`, `time()` and `count_values()` reached the translator. - Both endpoints now answer Loki's status and text, byte for byte, with the - column of the name wherever it stands (`sum(group(...))` is col 5, and inside - a `label_replace` argument it is relative to the whole query); only - `label_replace`, `vector` and `approx_topk` stay opaque calls. Loki's lexer - also lower-cases a word before it looks it up, so `TOPK(3, ...)`, `SUM BY - (a) (...)`, `RATE(...)`, `OFFSET`, `UNWRAP`, `BOOL`, `ON` and `GROUP_LEFT` are - the keywords they spell, while a label called `Rate` or `Count` stays a - label. The proxy matched the lower-case spelling only: `TOPK(...)` fell - through the opaque path and answered a `streams` result, and `SUM BY (app) - (...)` was a parse error. The scanner now reads keywords in any case, and - the `query` parameter is written with lower-case keywords before a handler - reads it. The group-normalisation and `label_join` post-processing of the - response, now unreachable, is removed. + joined labels in the response, `abs` and `clamp_min` were answered as well, + and `time()` and `count_values()` were a 400 with the proxy's own text + (`unexpected )`, `unexpected STRING`). Both endpoints now answer Loki's + status and text, byte for byte, with the column of the name wherever it + stands (`sum(group(...))` is col 5, and inside a `label_replace` argument it + is relative to the whole query); only `label_replace`, `vector` and + `approx_topk` stay opaque calls. Loki's lexer also lower-cases a word before + it looks it up, so `TOPK(3, ...)`, `SUM BY (a) (...)`, `RATE(...)`, `OFFSET`, + `UNWRAP`, `BOOL`, `ON` and `GROUP_LEFT` are the keywords they spell, while a + label called `Rate` or `Count` stays a label. The proxy matched the + lower-case spelling only: `TOPK(...)` fell through the opaque path and + answered a `streams` result, and `SUM BY (app) (...)` was a parse error. The + scanner now reads keywords in any case, and the `query` parameter is written + with lower-case keywords before a handler reads it. The group-normalisation + and `label_join` post-processing of the response, now unreachable, is + removed. ## [2.11.2] - 2026-10-12 diff --git a/internal/proxy/http_utils.go b/internal/proxy/http_utils.go index 5517edec9..d8e4415b7 100644 --- a/internal/proxy/http_utils.go +++ b/internal/proxy/http_utils.go @@ -76,6 +76,10 @@ func lowercaseQueryKeywords(next http.HandlerFunc) http.HandlerFunc { if r.PostForm.Has("query") { r.PostForm.Set("query", lower) } + if values := r.URL.Query(); values.Has("query") { // read again by the multi-tenant fan-out + values.Set("query", lower) + r.URL.RawQuery = values.Encode() + } } } next(w, r) diff --git a/internal/proxy/unknown_logql_names_test.go b/internal/proxy/unknown_logql_names_test.go index 9da478dcd..c0468d097 100644 --- a/internal/proxy/unknown_logql_names_test.go +++ b/internal/proxy/unknown_logql_names_test.go @@ -117,28 +117,49 @@ func TestUppercaseKeywordsSendTheLowercaseBackendRequests(t *testing.T) { {`MAX(COUNT_OVER_TIME({app="a"}[5m] OFFSET 1m))`, `max(count_over_time({app="a"}[5m] offset 1m))`}, {`{app="a"} | JSON | DROP pod`, `{app="a"} | json | drop pod`}, } - run := func(t *testing.T, endpoint, query string) []string { + // The same request three ways: a GET, a form-encoded POST and a multi-tenant + // GET that fans out per tenant. + run := func(t *testing.T, endpoint, variant, query string) []string { backend := newRecordingBackend(t) p := newProfileProxy(t, backend.URL, LabelStyleUnderscores, MetadataFieldModeTranslated, false) - if w := serve(p, http.MethodGet, queryURL(endpoint, query), nil); w.Code >= 500 || w.Code == http.StatusBadRequest { - t.Fatalf("%s %s: status %d %.300s", endpoint, query, w.Code, w.Body) + var w *httptest.ResponseRecorder + switch variant { + case "post": + mux := http.NewServeMux() + p.RegisterRoutes(mux) + target := queryURL(endpoint, query) + form := target[strings.Index(target, "?")+1:] + req := httptest.NewRequest(http.MethodPost, target[:strings.Index(target, "?")], strings.NewReader(form)) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.Header.Set("X-Scope-OrgID", "0") + w = httptest.NewRecorder() + mux.ServeHTTP(w, req) + case "multitenant": + w = serve(p, http.MethodGet, queryURL(endpoint, query), map[string]string{"X-Scope-OrgID": "0|1"}) + default: + w = serve(p, http.MethodGet, queryURL(endpoint, query), nil) + } + if w.Code >= 500 || w.Code == http.StatusBadRequest { + t.Fatalf("%s %s %s: status %d %.300s", variant, endpoint, query, w.Code, w.Body) } return backend.dataCalls() } for _, endpoint := range []string{"query_range", "query"} { - for _, tc := range cases { - if endpoint == "query" && strings.HasPrefix(tc.lower, "{") { - continue // a log query is not an instant query - } - t.Run(endpoint+"/"+tc.upper, func(t *testing.T) { - lower := run(t, endpoint, tc.lower) - if len(lower) == 0 { - t.Fatalf("the lower-case form %q reached no backend call, the comparison proves nothing", tc.lower) + for _, variant := range []string{"get", "post", "multitenant"} { + for _, tc := range cases { + if endpoint == "query" && strings.HasPrefix(tc.lower, "{") { + continue // a log query is not an instant query } - if upper := run(t, endpoint, tc.upper); strings.Join(upper, "\n") != strings.Join(lower, "\n") { - t.Fatalf("backend requests differ\n upper %q\n lower %q", upper, lower) - } - }) + t.Run(endpoint+"/"+variant+"/"+tc.upper, func(t *testing.T) { + lower := run(t, endpoint, variant, tc.lower) + if len(lower) == 0 { + t.Fatalf("the lower-case form %q reached no backend call, the comparison proves nothing", tc.lower) + } + if upper := run(t, endpoint, variant, tc.upper); strings.Join(upper, "\n") != strings.Join(lower, "\n") { + t.Fatalf("backend requests differ\n upper %q\n lower %q", upper, lower) + } + }) + } } } } diff --git a/test/e2e-compat/operators_not_in_loki_test.go b/test/e2e-compat/operators_not_in_loki_test.go index 8a7e8fde8..a21033855 100644 --- a/test/e2e-compat/operators_not_in_loki_test.go +++ b/test/e2e-compat/operators_not_in_loki_test.go @@ -75,28 +75,32 @@ func TestCompat_OperatorsNotInLokiRejected(t *testing.T) { } // Loki lower-cases a word before it looks it up, so SUM, TOPK(3, ...), RATE, -// BY, OFFSET, UNWRAP, BOOL, ON and GROUP_LEFT are the keywords they spell. The -// proxy answers 200 with the series Loki answers. +// BY, OFFSET, UNWRAP, BOOL, ON and GROUP_LEFT are the keywords they spell: both +// spellings answer 200 on Loki. The proxy answers the upper-case spelling with +// the same status, result type and series as its own lower-case answer (the +// comparison with Loki's series is the job of the rows for each query shape, +// not of this one). // // conformance: semantics/proxy-accepts-operators-not-in-loki func TestCompat_UppercaseKeywordsMatchLoki(t *testing.T) { ensureDataIngested(t) end := time.Now() - queries := []string{ - `TOPK(3, SUM BY (app) (RATE({env="production"}[5m])))`, - `SUM BY (app) (COUNT_OVER_TIME({env="production"}[5m]))`, - `SUM(RATE({env="production"}[5m])) BY (app)`, - `SUM WITHOUT (level) (COUNT_OVER_TIME({env="production"}[5m]))`, - `MAX BY (app) (BYTES_OVER_TIME({env="production"}[5m]))`, - `SUM BY (app) (COUNT_OVER_TIME({env="production"}[5m])) / ON (app) SUM BY (app) (COUNT_OVER_TIME({env="production"}[5m]))`, - `SUM BY (app) (COUNT_OVER_TIME({env="production"}[5m])) > BOOL 0`, - `SUM BY (app) (COUNT_OVER_TIME({env="production"}[5m] OFFSET 1m))`, - `SUM BY (app) (SUM_OVER_TIME({env="production"} | JSON | UNWRAP duration_ms [5m]))`, + cases := []struct{ upper, lower string }{ + {`TOPK(3, SUM BY (app) (RATE({env="production"}[5m])))`, `topk(3, sum by (app) (rate({env="production"}[5m])))`}, + {`SUM BY (app) (COUNT_OVER_TIME({env="production"}[5m]))`, `sum by (app) (count_over_time({env="production"}[5m]))`}, + {`SUM(RATE({env="production"}[5m])) BY (app)`, `sum(rate({env="production"}[5m])) by (app)`}, + {`SUM WITHOUT (level) (COUNT_OVER_TIME({env="production"}[5m]))`, `sum without (level) (count_over_time({env="production"}[5m]))`}, + {`MAX BY (app) (BYTES_OVER_TIME({env="production"}[5m]))`, `max by (app) (bytes_over_time({env="production"}[5m]))`}, + {`SUM BY (app) (COUNT_OVER_TIME({env="production"}[5m])) / ON (app) SUM BY (app) (COUNT_OVER_TIME({env="production"}[5m]))`, + `sum by (app) (count_over_time({env="production"}[5m])) / on (app) sum by (app) (count_over_time({env="production"}[5m]))`}, + {`SUM BY (app) (COUNT_OVER_TIME({env="production"}[5m])) > BOOL 0`, `sum by (app) (count_over_time({env="production"}[5m])) > bool 0`}, + {`SUM BY (app) (COUNT_OVER_TIME({env="production"}[5m] OFFSET 1m))`, `sum by (app) (count_over_time({env="production"}[5m] offset 1m))`}, + {`SUM BY (app) (SUM_OVER_TIME({env="production"} | JSON | UNWRAP duration_ms [5m]))`, `sum by (app) (sum_over_time({env="production"} | json | unwrap duration_ms [5m]))`}, } type result struct { status int resultType string - series []string + series string } run := func(base, path string, params url.Values) result { status, body := rejectedQueryGet(t, base, path, params, "0", nil) @@ -111,37 +115,41 @@ func TestCompat_UppercaseKeywordsMatchLoki(t *testing.T) { } if status == http.StatusOK && json.Unmarshal(body, &resp) == nil { out.resultType = resp.Data.ResultType + var series []string for _, s := range resp.Data.Result { keys := make([]string, 0, len(s.Metric)) for k, v := range s.Metric { keys = append(keys, k+"="+v) } sort.Strings(keys) - out.series = append(out.series, strings.Join(keys, ",")) + series = append(series, strings.Join(keys, ",")) } - sort.Strings(out.series) + sort.Strings(series) + out.series = strings.Join(series, "|") } return out } for _, path := range []string{"/loki/api/v1/query_range", "/loki/api/v1/query"} { - for _, query := range queries { - t.Run(path+" "+query, func(t *testing.T) { - params := url.Values{ - "query": {query}, - "start": {strconv.FormatInt(end.Add(-time.Hour).UnixNano(), 10)}, - "end": {strconv.FormatInt(end.UnixNano(), 10)}, - "time": {strconv.FormatInt(end.UnixNano(), 10)}, - "step": {"60"}, + for _, tc := range cases { + t.Run(path+" "+tc.upper, func(t *testing.T) { + params := func(query string) url.Values { + return url.Values{ + "query": {query}, + "start": {strconv.FormatInt(end.Add(-time.Hour).UnixNano(), 10)}, + "end": {strconv.FormatInt(end.UnixNano(), 10)}, + "time": {strconv.FormatInt(end.UnixNano(), 10)}, + "step": {"60"}, + } } - loki := run(lokiURL, path, params) - if loki.status != http.StatusOK { - t.Fatalf("Loki fixture drifted: want 200, got %d", loki.status) + for _, query := range []string{tc.lower, tc.upper} { + if got := run(lokiURL, path, params(query)); got.status != http.StatusOK { + t.Fatalf("Loki fixture drifted: want 200 for %q, got %d", query, got.status) + } } for name, base := range lokiProfileProxies() { - got := run(base, path, params) - if got.status != loki.status || got.resultType != loki.resultType || strings.Join(got.series, "|") != strings.Join(loki.series, "|") { - t.Errorf("%s proxy: status %d %s series %q, Loki %d %s series %q", - name, got.status, got.resultType, got.series, loki.status, loki.resultType, loki.series) + want, got := run(base, path, params(tc.lower)), run(base, path, params(tc.upper)) + if want.status != http.StatusOK || got != want { + t.Errorf("%s proxy: upper-case %+v, lower-case %+v", name, got, want) } } }) From ccd696965f7334d90e9c8c68bcc487ea2f956d82 Mon Sep 17 00:00:00 2001 From: szibis Date: Mon, 12 Oct 2026 14:02:48 +0200 Subject: [PATCH 5/5] chore(conformance): record #748 as the fix of proxy-accepts-operators-not-in-loki --- CHANGELOG.md | 2 +- .../proxy-accepts-operators-not-in-loki.yaml | 2 +- conformance/registry/generated/wiring.json | 20 ++++++++++++++++--- conformance/reports/parity-gaps.md | 1 + 4 files changed, 20 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d39f481b9..9db7ddc31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **`group()`, `label_join()` and every other name Loki does not have are a 400 - parse error, and keywords are accepted in any case (#TBD).** Loki 3.7.7 has + parse error, and keywords are accepted in any case (#748).** Loki 3.7.7 has no `group` aggregation and no `label_join`: its lexer makes a function token only of the names in `functionTokens` (the range operations, `sum` `avg` `max` `min` `count` `stddev` `stdvar` `bottomk` `topk` `sort` `sort_desc` diff --git a/conformance/registry/cases/semantics/proxy-accepts-operators-not-in-loki.yaml b/conformance/registry/cases/semantics/proxy-accepts-operators-not-in-loki.yaml index 0df80326e..26f413981 100644 --- a/conformance/registry/cases/semantics/proxy-accepts-operators-not-in-loki.yaml +++ b/conformance/registry/cases/semantics/proxy-accepts-operators-not-in-loki.yaml @@ -64,7 +64,7 @@ compare: lower-case form. gap: status: fixed - fixed_by: "#TBD" + fixed_by: "#748" impact: api-only effort: S area: 'internal/logql scanner (keyword case) + parser (opaque calls) + semantic (label_replace argument position); internal/proxy lowercaseQueryKeywords' diff --git a/conformance/registry/generated/wiring.json b/conformance/registry/generated/wiring.json index 84533f221..2f85a184d 100644 --- a/conformance/registry/generated/wiring.json +++ b/conformance/registry/generated/wiring.json @@ -456,6 +456,11 @@ "internal/proxy/plain_stream_labels_test.go", "test/e2e-compat/plain_name_after_parser_test.go" ], + "semantics/proxy-accepts-operators-not-in-loki": [ + "internal/logql/loki_unknown_names_test.go", + "internal/proxy/unknown_logql_names_test.go", + "test/e2e-compat/operators_not_in_loki_test.go" + ], "semantics/sliding-window-recurrence-exact": [ "internal/proxy/sliding_window_stats_test.go", "test/e2e-compat/heavy_query_protection_test.go" @@ -541,7 +546,7 @@ "test/e2e-compat/unwrap_ungrouped_sum_test.go" ] }, - "registry_items": 299, + "registry_items": 300, "routes_without_registry_item": [], "test_to_items": { "cmd/proxy/main_test.go": [ @@ -559,6 +564,9 @@ "semantics/line-filter-or-alternatives", "semantics/parenthesized-log-range" ], + "internal/logql/loki_unknown_names_test.go": [ + "semantics/proxy-accepts-operators-not-in-loki" + ], "internal/logql/parser_captures_test.go": [ "parsed-label-series-identity" ], @@ -827,6 +835,9 @@ "series-limits-and-partial-results", "window-bounds-and-step-alignment" ], + "internal/proxy/unknown_logql_names_test.go": [ + "semantics/proxy-accepts-operators-not-in-loki" + ], "internal/proxy/unwrap_bytes_gate_property_test.go": [ "semantics/unwrap-gate-parsefloat-divergences", "semantics/unwrap-sample-validity" @@ -982,6 +993,9 @@ "loki_api_v1_label_name_values", "loki_api_v1_labels" ], + "test/e2e-compat/operators_not_in_loki_test.go": [ + "semantics/proxy-accepts-operators-not-in-loki" + ], "test/e2e-compat/parenthesized_range_or_filters_test.go": [ "loki_api_v1_query", "loki_api_v1_query_range", @@ -1064,7 +1078,7 @@ "profiles/structured-metadata-keys-per-profile" ] }, - "tests_with_markers": 96, + "tests_with_markers": 99, "unknown_ids": {}, "unwired_items": [ "api_prom_label", @@ -1172,5 +1186,5 @@ "versions/capabilities-floor-vs-matrix", "versions/rejected-stats-query-wording-vl-1.47" ], - "wired_items": 106 + "wired_items": 107 } diff --git a/conformance/reports/parity-gaps.md b/conformance/reports/parity-gaps.md index 47d3231db..019ec70b4 100644 --- a/conformance/reports/parity-gaps.md +++ b/conformance/reports/parity-gaps.md @@ -114,4 +114,5 @@ Last run: 2026-10-05T20:16:15Z on `fcaccfcc`, window [1791220800, 1791224400]. | `semantics/unwrap-rate-ungrouped-sum-per-stream` — An ungrouped sum over rate with unwrap is one series on query_range, like Loki (fixed) | #716 | 0 | 0 | | `severity/otlp-severity-before-vl-1.50` — OTLP rows stored by VictoriaLogs before v1.50 (one `severity` field, no severity_number) get detected_level "Unspecified" / "Info2" (fixed) | #666 | 0 | 0 | | `versions/rejected-stats-query-wording-vl-1.47` — The proxy did not recognise VictoriaLogs v1.47 parse errors, so a rejected query was not Loki's 400 (fixed) | #659 | 0 | 0 | +| `semantics/proxy-accepts-operators-not-in-loki` — group(), label_join() and other names Loki does not have are answered; uppercase keywords are not (fixed) | #748 | 0 | 0 | | `versions/capabilities-floor-vs-matrix` — logsql.Capabilities treated VictoriaLogs below v1.40 as unsupported while the matrix claimed v1.30 (fixed) | #659 | 0 | 0 |