diff --git a/CHANGELOG.md b/CHANGELOG.md index feca92cbd..59b07f8c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **A zero divisor of `/` and `%` is NaN, like Loki.** `sum(rate({app="api"}[5m])) / 0`, + `5 / `, `-1 / 0` and `1 / 0` answered `+Inf` or `-Inf` + (IEEE division), and the fallback sample arithmetic used when a query is not + parsed as a binary expression answered `0` for `/` and `%`. Loki routes every binary operation through + `syntax.MergeBinOp`, which returns NaN for a zero right operand of `/` and `%` + whatever the numerator is, for a literal fold (`1 / 0`), a vector against a + literal and two vectors alike. The proxy now returns NaN in every sample + arithmetic path; comparisons (with and without `bool`), `^`, NaN and infinite + operands already matched Loki and are pinned against a direct port of + `MergeBinOp` by an operator by operand edge table. Aggregations over a + division by zero are not changed here. (#745) + ## [2.11.2] - 2026-10-12 ### CI diff --git a/conformance/registry/cases/semantics/binary-division-by-zero-nan.yaml b/conformance/registry/cases/semantics/binary-division-by-zero-nan.yaml new file mode 100644 index 000000000..dc29230ba --- /dev/null +++ b/conformance/registry/cases/semantics/binary-division-by-zero-nan.yaml @@ -0,0 +1,59 @@ +id: semantics/binary-division-by-zero-nan +title: A zero divisor of `/` and `%` is NaN, like Loki (fixed) +proves: + - loki_api_v1_query_range + - loki_api_v1_query +consumer: explore +why: | + Loki guards a zero right operand of `/` and `%` with NaN in the one function + every binary operation goes through, syntax.MergeBinOp + (pkg/logql/syntax/ast.go:1978-2005): `if right.F == 0 { res.F = math.NaN() }`. + It applies to a literal fold (reduceBinOp, ast.go:1918-1935, so `1 / 0` is NaN), + to a vector against a literal (LiteralStepEvaluator.Next, + pkg/logql/evaluator.go:1179) and to two vectors (vectorBinop, evaluator.go:1020), + whatever the numerator is: `sum(rate(..)) / 0`, `-1 / 0` and `Inf / 0` are all + NaN, never +Inf or -Inf. The proxy computed IEEE a/b, so a nonzero numerator + over a zero divisor came back as "+Inf" or "-Inf" (only 0/0 was NaN), and the + legacy sample arithmetic used when a query is not parsed as a binary + expression answered 0 for both operators. `%` by zero was already NaN through + math.Mod on the main path. The comparison operators, `^`, NaN operands and + infinite operands already matched MergeBinOp; the edge table in + TestBinarySampleValueMatchesLokiMergeBinOp pins every operator against a + direct port of it, with and without bool. + + min and max across series skip a NaN unless every value is NaN: Loki's + VectorAggEvaluator replaces a NaN accumulator (pkg/logql/evaluator.go:525-535), + so `max(x / 0)` over mixed values is the largest real value. Residual, not + fixed here: the one proxy fold that lets NaN win is withoutMerge.combine + (math.Min / math.Max in internal/proxy/vector_matching.go), fixed by #734 + (semantics/non-sum-vector-aggregation-unreduced). With this change no other + min/max fold receives a NaN: on origin/main an aggregation over a binary + expression drops the binary operand (`max(v / 2)` and `max(v / 0)` send VL the + same query without the division), which #741 fixes; the folds #741 adds must + apply Loki's NaN rule. +request: + method: GET + path: /loki/api/v1/query_range + params: + query: 'sum(count_over_time({app="payment-service"}[5m])) / 0' + start: 'now-1h' + end: 'now' + step: '60' + headers: + X-Scope-OrgID: "0" +compare: + mode: observed + fields: [series, values] + expect: | + One unlabelled series whose every value is "NaN". +gap: + status: fixed + fixed_by: "#745" + impact: api-only + effort: S + area: 'internal/proxy binary_matching.go (binarySampleValue), metric_binary.go (applyOp)' + loki: 'pkg/logql/syntax/ast.go:1944-2125 MergeBinOp (the / and % guards at :1978-2005), reduceBinOp :1918; pkg/logql/evaluator.go:1020 vectorBinop, :1179 LiteralStepEvaluator.Next' + proxy: 'a zero divisor of / and % is NaN in every sample-arithmetic path' + planned_test: 'TestCompat_BinaryDivisionByZeroIsNaN (test/e2e-compat, group core); TestBinarySampleValueMatchesLokiMergeBinOp, TestBinaryScalarFoldMatchesLokiReduceBinOp, TestBinaryMatchersMatchLokiMergeBinOp, TestApplyOpDivisionAndModuloByZeroAreNaN, TestBinaryVectorDivisionByZeroHandlers and TestBinaryConstantVectorDivisionByZeroInstant (internal/proxy)' + note: 'Measured on origin/main b20a550f. The e2e is written and not yet run. min/max NaN folding: residual in #734 (withoutMerge.combine) and #741 (aggregation over binary).' + clusters: [] diff --git a/conformance/registry/generated/wiring.json b/conformance/registry/generated/wiring.json index 84533f221..6fa95505d 100644 --- a/conformance/registry/generated/wiring.json +++ b/conformance/registry/generated/wiring.json @@ -186,6 +186,7 @@ "internal/proxy/tenant_query_limits_test.go", "internal/proxy/tumbling_axis_parity_test.go", "internal/proxy/unwrap_conversion_error_test.go", + "test/e2e-compat/binary_division_zero_test.go", "test/e2e-compat/extracted_suffix_collision_test.go", "test/e2e-compat/parenthesized_range_or_filters_test.go", "test/e2e-compat/parser_capture_identity_test.go", @@ -210,6 +211,7 @@ "internal/proxy/unwrap_conversion_error_test.go", "internal/translator/extracted_suffix_test.go", "internal/translator/regexp_escaping_test.go", + "test/e2e-compat/binary_division_zero_test.go", "test/e2e-compat/drilldown_field_breakdown_test.go", "test/e2e-compat/extracted_suffix_collision_test.go", "test/e2e-compat/heavy_query_protection_test.go", @@ -361,6 +363,10 @@ "semantics/backend-timeout-argument": [ "internal/proxy/backend_heavy_test.go" ], + "semantics/binary-division-by-zero-nan": [ + "internal/proxy/binary_division_zero_test.go", + "test/e2e-compat/binary_division_zero_test.go" + ], "semantics/client-cancellation-propagates": [ "internal/proxy/backend_heavy_test.go" ], @@ -541,7 +547,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": [ @@ -592,6 +598,9 @@ "operator-configurable-limits", "semantics/backend-aborted-response" ], + "internal/proxy/binary_division_zero_test.go": [ + "semantics/binary-division-by-zero-nan" + ], "internal/proxy/compat_cache_freshness_test.go": [ "semantics/metadata-answers-include-last-24h-like-loki" ], @@ -895,6 +904,11 @@ "test/e2e-compat/bad_request_status_parity_test.go": [ "loki-compatible-profile" ], + "test/e2e-compat/binary_division_zero_test.go": [ + "loki_api_v1_query", + "loki_api_v1_query_range", + "semantics/binary-division-by-zero-nan" + ], "test/e2e-compat/compat_option_matrix_test.go": [ "loki-compatible-profile", "profiles/detected-fields-dotted-json-keys", @@ -1064,7 +1078,7 @@ "profiles/structured-metadata-keys-per-profile" ] }, - "tests_with_markers": 96, + "tests_with_markers": 98, "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..2db0726dc 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/binary-division-by-zero-nan` — A zero divisor of `/` and `%` is NaN, like Loki (fixed) | #745 | 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 | diff --git a/internal/proxy/binary_division_zero_test.go b/internal/proxy/binary_division_zero_test.go new file mode 100644 index 000000000..ef75d459f --- /dev/null +++ b/internal/proxy/binary_division_zero_test.go @@ -0,0 +1,310 @@ +package proxy + +import ( + "context" + "encoding/json" + "fmt" + "math" + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "strings" + "testing" +) + +// lokiMergeBinOp is a direct port of Loki v3.7.7 syntax.MergeBinOp +// (pkg/logql/syntax/ast.go:1944-2125) reduced to the sample value: it returns +// the resulting value and whether the sample is kept. left is the vector side +// when swap is set the way the Loki evaluator passes it; here left/right are the +// operands in query order, so a filtered comparison keeps the left value +// (ast.go:2100-2118, retSample = left unless swap). +func lokiMergeBinOp(op string, left, right float64, filter bool) (float64, bool) { + switch op { + case "+": + return left + right, true + case "-": + return left - right, true + case "*": + return left * right, true + case "/": + // guard against divide by zero (ast.go:1978-1991) + if right == 0 { + return math.NaN(), true + } + return left / right, true + case "%": + if right == 0 { + return math.NaN(), true + } + return math.Mod(left, right), true + case "^": + return math.Pow(left, right), true + } + var truth bool + switch op { + case "==": + truth = left == right + case "!=": + truth = left != right + case ">": + truth = left > right + case ">=": + truth = left >= right + case "<": + truth = left < right + case "<=": + truth = left <= right + default: + panic("unexpected operation " + op) + } + if filter { + // a filtered vector comparison returns the vector side's own value + return left, truth + } + if truth { + return 1, true + } + return 0, true +} + +func sameFloat(a, b float64) bool { + if math.IsNaN(a) || math.IsNaN(b) { + return math.IsNaN(a) && math.IsNaN(b) + } + return a == b +} + +var binaryEdgeOperands = []float64{0, math.Copysign(0, -1), 1, -1, 2.5, -7, math.NaN(), math.Inf(1), math.Inf(-1)} + +var binaryEdgeOps = []string{"+", "-", "*", "/", "%", "^", "==", "!=", ">", ">=", "<", "<="} + +// Every operator against zero, NaN and infinite operands gives what Loki's +// MergeBinOp gives, with and without bool: a zero divisor of / and % is NaN +// (Inf/0 and -1/0 included), not +Inf or -Inf. +// conformance: semantics/binary-division-by-zero-nan +func TestBinarySampleValueMatchesLokiMergeBinOp(t *testing.T) { + for _, op := range binaryEdgeOps { + for _, a := range binaryEdgeOperands { + for _, b := range binaryEdgeOperands { + for _, returnBool := range []bool{false, true} { + want, wantKeep := lokiMergeBinOp(op, a, b, !returnBool && strings.ContainsAny(op, "=<>")) + got, keep := binarySampleValue(a, b, op, returnBool) + if keep != wantKeep || (keep && !sameFloat(got, want)) { + t.Errorf("%v %s %v (bool=%v): got %v keep=%v, Loki %v keep=%v", a, op, b, returnBool, got, keep, want, wantKeep) + } + } + } + } + } +} + +// The scalar-only literal path folds like Loki's reduceBinOp +// (ast.go:1918-1935: MergeBinOp with filter=false). +// conformance: semantics/binary-division-by-zero-nan +func TestBinaryScalarFoldMatchesLokiReduceBinOp(t *testing.T) { + for _, op := range binaryEdgeOps { + for _, a := range binaryEdgeOperands { + for _, b := range binaryEdgeOperands { + want, _ := lokiMergeBinOp(op, a, b, false) + got, _ := binarySampleValue(a, b, op, true) + if !sameFloat(got, want) { + t.Errorf("%v %s %v: got %v, Loki %v", a, op, b, got, want) + } + } + } + } +} + +// The fallback arithmetic used when the query cannot be parsed as a binary +// expression has the same division rule (it returned 0). +// conformance: semantics/binary-division-by-zero-nan +func TestApplyOpDivisionAndModuloByZeroAreNaN(t *testing.T) { + for _, op := range []string{"+", "-", "*", "/", "%", "^", "==", "!=", ">", ">=", "<", "<="} { + for _, a := range binaryEdgeOperands { + for _, b := range binaryEdgeOperands { + want, _ := lokiMergeBinOp(op, a, b, false) + if got := applyOp(a, b, op); !sameFloat(got, want) { + t.Errorf("applyOp(%v, %v, %q) = %v, Loki %v", a, b, op, got, want) + } + } + } + } +} + +// vector / 0 on query_range and query: every sample is NaN, as Loki's +// LiteralStepEvaluator and vectorBinop evaluate it (evaluator.go:1020,1179). +// conformance: semantics/binary-division-by-zero-nan +func TestBinaryVectorDivisionByZeroHandlers(t *testing.T) { + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/health" { + return + } + _ = r.ParseForm() + value := "60" + if strings.Contains(r.FormValue("query"), `"zero"`) { + value = "0" + } + switch r.URL.Path { + case "/select/logsql/stats_query_range": + fmt.Fprintf(w, `{"status":"success","data":{"resultType":"matrix","result":[{"metric":{},"values":[[1700000040,%q],[1700000100,%q]]}]}}`, value, value) + case "/select/logsql/stats_query": + fmt.Fprintf(w, `{"status":"success","data":{"resultType":"vector","result":[{"metric":{},"value":[1700000100,%q]}]}}`, value) + default: + http.Error(w, "unexpected endpoint "+r.URL.Path, http.StatusInternalServerError) + } + })) + defer backend.Close() + p := newGapTestProxy(t, backend.URL) + + for _, tc := range []struct { + name, query, want string + }{ + {"vector_div_scalar_zero", `sum(rate({app="api"}[1m])) / 0`, "NaN"}, + {"vector_mod_scalar_zero", `sum(rate({app="api"}[1m])) % 0`, "NaN"}, + {"scalar_div_zero_vector", `5 / sum(rate({app="zero"}[1m]))`, "NaN"}, + {"vector_div_zero_vector", `sum(rate({app="api"}[1m])) / sum(rate({app="zero"}[1m]))`, "NaN"}, + {"zero_vector_div_zero_vector", `sum(rate({app="zero"}[1m])) / sum(rate({app="zero"}[1m]))`, "NaN"}, + {"nonzero_divisor_unchanged", `sum(rate({app="api"}[1m])) / 4`, "15"}, + {"zero_numerator_unchanged", `0 / sum(rate({app="api"}[1m]))`, "0"}, + } { + for _, mode := range []string{"query_range", "query"} { + t.Run(tc.name+"/"+mode, func(t *testing.T) { + params := url.Values{"query": {tc.query}} + if mode == "query_range" { + params.Set("start", "1700000040") + params.Set("end", "1700000100") + params.Set("step", "60") + } else { + params.Set("time", "1700000100") + } + rec := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, "/loki/api/v1/"+mode+"?"+params.Encode(), nil) + if mode == "query_range" { + p.handleQueryRange(rec, r) + } else { + p.handleQuery(rec, r) + } + if rec.Code != http.StatusOK { + t.Fatalf("status=%d body=%s", rec.Code, rec.Body.String()) + } + var response struct { + Data struct { + Result []struct { + Value []any `json:"value"` + Values [][]any `json:"values"` + } `json:"result"` + } `json:"data"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &response); err != nil || len(response.Data.Result) != 1 { + t.Fatalf("want one series: %v %s", err, rec.Body.String()) + } + points := response.Data.Result[0].Values + if mode == "query" { + points = [][]any{response.Data.Result[0].Value} + } + if len(points) == 0 { + t.Fatalf("no points: %s", rec.Body.String()) + } + for _, point := range points { + if got, _ := point[1].(string); got != tc.want { + t.Fatalf("value %v, want %s: %s", point, tc.want, rec.Body.String()) + } + } + }) + } + } +} + +// The matchers that wrap binarySampleValue (scalar on either side, vector with +// vector) keep Loki's value for every operator and operand: a filtered +// comparison returns the vector side's own value and drops a false comparison, +// bool returns 1 or 0, and / and % by zero are NaN. +// conformance: semantics/binary-division-by-zero-nan +func TestBinaryMatchersMatchLokiMergeBinOp(t *testing.T) { + ctx := binaryEvaluationContext(context.Background()) + decode := func(t *testing.T, body []byte) (string, bool) { + t.Helper() + var response struct { + Data struct { + Result []struct{ Values [][]any } `json:"result"` + } `json:"data"` + } + if err := json.Unmarshal(body, &response); err != nil { + t.Fatalf("decode %s: %v", body, err) + } + if len(response.Data.Result) == 0 { + return "", false + } + return response.Data.Result[0].Values[0][1].(string), true + } + check := func(t *testing.T, what string, body []byte, want float64, wantKeep bool) { + t.Helper() + got, ok := decode(t, body) + if ok != wantKeep { + t.Errorf("%s: kept=%v, Loki kept=%v (%s)", what, ok, wantKeep, body) + return + } + if !ok { + return + } + value, err := strconv.ParseFloat(got, 64) + if err != nil || !sameFloat(value, want) { + t.Errorf("%s: got %s, Loki %v", what, got, want) + } + } + for _, op := range binaryEdgeOps { + for _, a := range binaryEdgeOperands { + for _, b := range binaryEdgeOperands { + for _, returnBool := range []bool{false, true} { + filter := !returnBool && strings.ContainsAny(op, "=<>") + vecA := binaryTestBody([]map[string]string{{"app": "x"}}, [][]any{{1700000040, strconv.FormatFloat(a, 'f', -1, 64)}}) + vecB := binaryTestBody([]map[string]string{{"app": "x"}}, [][]any{{1700000040, strconv.FormatFloat(b, 'f', -1, 64)}}) + what := fmt.Sprintf("%v %s %v bool=%v", a, op, b, returnBool) + + // vector op scalar: the vector is the left operand. + want, keep := lokiMergeBinOp(op, a, b, filter) + body, err := matchBinaryScalarContext(ctx, vecA, b, false, op, "matrix", returnBool) + if err != nil { + t.Fatal(err) + } + check(t, "vector-scalar "+what, body, want, keep) + + // scalar op vector: Loki returns the vector's value for a filtered + // comparison (swap=true), so the value kept is b's side here. + want, keep = lokiMergeBinOp(op, a, b, filter) + if filter && keep { + want = b + } + body, err = matchBinaryScalarContext(ctx, vecB, a, true, op, "matrix", returnBool) + if err != nil { + t.Fatal(err) + } + check(t, "scalar-vector "+what, body, want, keep) + + // vector op vector, one-to-one. + want, keep = lokiMergeBinOp(op, a, b, filter) + body, err = matchBinaryMetricResultsContext(ctx, vecA, vecB, op, "matrix", nil, returnBool) + if err != nil { + t.Fatal(err) + } + check(t, "vector-vector "+what, body, want, keep) + } + } + } + } +} + +// A constant vector divided by zero is NaN on the instant endpoint, like Loki's +// vectorBinop over two vector() selectors. +// conformance: semantics/binary-division-by-zero-nan +func TestBinaryConstantVectorDivisionByZeroInstant(t *testing.T) { + p := newGapTestProxy(t, "http://127.0.0.1:1") + for _, query := range []string{`vector(1)/vector(0)`, `vector(1) % vector(0)`} { + rec := httptest.NewRecorder() + p.handleQuery(rec, httptest.NewRequest(http.MethodGet, "/loki/api/v1/query?"+url.Values{"query": {query}, "time": {"1700000100"}}.Encode(), nil)) + if rec.Code != http.StatusOK || !strings.Contains(rec.Body.String(), `"NaN"`) { + t.Errorf("%s: want a NaN sample: %d %s", query, rec.Code, rec.Body.String()) + } + } +} diff --git a/internal/proxy/binary_matching.go b/internal/proxy/binary_matching.go index f5aa4eb19..91aaf7f33 100644 --- a/internal/proxy/binary_matching.go +++ b/internal/proxy/binary_matching.go @@ -123,9 +123,18 @@ func binarySampleValue(a, b float64, op string, returnBool bool) (float64, bool) return a - b, true case "*": return a * b, true + // Loki guards a zero right operand of / and % with NaN, whatever the left + // operand is (syntax.MergeBinOp, pkg/logql/syntax/ast.go:1978-2005), where + // IEEE division gives +Inf or -Inf. case "/": + if b == 0 { + return math.NaN(), true + } return a / b, true case "%": + if b == 0 { + return math.NaN(), true + } return math.Mod(a, b), true case "^": return math.Pow(a, b), true diff --git a/internal/proxy/binary_matching_test.go b/internal/proxy/binary_matching_test.go index 67ab0ff45..7de204adf 100644 --- a/internal/proxy/binary_matching_test.go +++ b/internal/proxy/binary_matching_test.go @@ -228,7 +228,8 @@ func TestBinaryInstantUsesSingleDefaultTimestamp(t *testing.T) { func TestBinaryScalarSubtreesAndIEEEValues(t *testing.T) { p := newTestProxy(t, "http://127.0.0.1:1") for _, tc := range []struct{ query, want string }{ - {"1 + 2 * 3", "7"}, {"2 ^ 3 ^ 2", "512"}, {"1 / 0", "+Inf"}, {"0 / 0", "NaN"}, {"1 < bool 2", "1"}, {"1 > 2", "0"}, + {"1 + 2 * 3", "7"}, {"2 ^ 3 ^ 2", "512"}, {"1 / 0", "NaN"}, {"-1 / 0", "NaN"}, {"1 % 0", "NaN"}, {"0 / 0", "NaN"}, {"1 < bool 2", "1"}, {"1 > 2", "0"}, + // A zero divisor of / and % is NaN in Loki (syntax.MergeBinOp), not +Inf. // Loki renders sample values with model.SampleValue.String ('f', -1): // never exponent notation for large or small magnitudes. {"1234 * 1000", "1234000"}, {"1 / 60000", "0.000016666666666666667"}, diff --git a/internal/proxy/critical_fixes_test.go b/internal/proxy/critical_fixes_test.go index ae4a9c2cd..1d32784f3 100644 --- a/internal/proxy/critical_fixes_test.go +++ b/internal/proxy/critical_fixes_test.go @@ -114,7 +114,7 @@ func TestCritical_ApplyOp_AllOperators(t *testing.T) { }{ // Existing operators {10, 3, "/", 10.0 / 3.0}, - {10, 0, "/", 0}, // division by zero + // a zero divisor is NaN (TestApplyOpDivisionAndModuloByZeroAreNaN) {6, 7, "*", 42}, {10, 3, "+", 13}, {10, 3, "-", 7}, diff --git a/internal/proxy/metric_binary.go b/internal/proxy/metric_binary.go index 880ce3a39..a39410cc7 100644 --- a/internal/proxy/metric_binary.go +++ b/internal/proxy/metric_binary.go @@ -2254,7 +2254,7 @@ func applyOp(a, b float64, op string) float64 { switch op { case "/": if b == 0 { - return 0 // avoid division by zero, return 0 like Prometheus + return math.NaN() // Loki: a zero divisor is NaN (syntax.MergeBinOp) } return a / b case "*": @@ -2265,7 +2265,7 @@ func applyOp(a, b float64, op string) float64 { return a - b case "%": if b == 0 { - return 0 + return math.NaN() } return math.Mod(a, b) case "^": diff --git a/internal/proxy/scalar_matrix_test.go b/internal/proxy/scalar_matrix_test.go index 6a62edcde..89345ac1b 100644 --- a/internal/proxy/scalar_matrix_test.go +++ b/internal/proxy/scalar_matrix_test.go @@ -70,21 +70,22 @@ func TestScalarMatrix_ApplyScalarOp_DivisionAndModuloByZero(t *testing.T) { t.Run("right_scalar_zero", func(t *testing.T) { body := scalarMatrixBody(4) + // Loki: a zero divisor of / and % is NaN (syntax.MergeBinOp). outDiv := applyScalarOp(body, "/", 0, "matrix") - assertApproxEqual(t, firstMatrixValue(t, outDiv), 0) + assertNaN(t, firstMatrixValue(t, outDiv)) outMod := applyScalarOp(body, "%", 0, "matrix") - assertApproxEqual(t, firstMatrixValue(t, outMod), 0) + assertNaN(t, firstMatrixValue(t, outMod)) }) t.Run("reverse_zero_denominator", func(t *testing.T) { body := scalarMatrixBody(0) outDiv := applyScalarOpReverse(body, "/", 2, "matrix") - assertApproxEqual(t, firstMatrixValue(t, outDiv), 0) + assertNaN(t, firstMatrixValue(t, outDiv)) outMod := applyScalarOpReverse(body, "%", 2, "matrix") - assertApproxEqual(t, firstMatrixValue(t, outMod), 0) + assertNaN(t, firstMatrixValue(t, outMod)) }) } @@ -221,6 +222,13 @@ func firstMatrixValue(t *testing.T, body []byte) float64 { } } +func assertNaN(t *testing.T, got float64) { + t.Helper() + if !math.IsNaN(got) { + t.Fatalf("unexpected value: got=%v want=NaN", got) + } +} + func assertApproxEqual(t *testing.T, got, want float64) { t.Helper() if math.Abs(got-want) > 1e-9 { diff --git a/test/e2e-compat/binary_division_zero_test.go b/test/e2e-compat/binary_division_zero_test.go new file mode 100644 index 000000000..aed4e906e --- /dev/null +++ b/test/e2e-compat/binary_division_zero_test.go @@ -0,0 +1,146 @@ +//go:build e2e + +package e2e_compat + +import ( + "encoding/json" + "fmt" + "net/http" + "net/url" + "sort" + "strconv" + "strings" + "testing" + "time" +) + +// Loki guards a zero right operand of `/` and `%` with NaN, for a literal +// fold (pkg/logql/syntax/ast.go reduceBinOp), a vector against a literal +// (evaluator.go LiteralStepEvaluator) and a vector against a vector +// (evaluator.go vectorBinop), all through syntax.MergeBinOp (ast.go:1978-2005). +// The proxy computed IEEE a/b, so a nonzero numerator over zero was +Inf or -Inf. +// conformance: semantics/binary-division-by-zero-nan, loki_api_v1_query, loki_api_v1_query_range +func TestCompat_BinaryDivisionByZeroIsNaN(t *testing.T) { + app := fmt.Sprintf("div-zero-%d", time.Now().UnixNano()) + fixtureEnd := time.Now().Add(-3 * time.Hour).Truncate(time.Minute) + const lines = 6 + var vlRows strings.Builder + lokiValues := map[string][][]string{} + for i := 0; i < lines; i++ { + stamp := fixtureEnd.Add(time.Duration(-240+40*i) * time.Second) + pod := "p" + strconv.Itoa(i%2) + message := fmt.Sprintf("division fixture line %d pod=%s", i, pod) + row, _ := json.Marshal(map[string]string{"_time": stamp.UTC().Format(time.RFC3339Nano), "_msg": message, "app": app, "pod": pod}) + vlRows.Write(row) + vlRows.WriteByte('\n') + lokiValues[pod] = append(lokiValues[pod], []string{strconv.FormatInt(stamp.UnixNano(), 10), message}) + } + status, body := hardeningRequest(t, http.MethodPost, vlURL+"/insert/jsonline?_stream_fields=app,pod", vlRows.String(), map[string]string{"Content-Type": "application/stream+json"}) + if status != http.StatusOK { + t.Fatalf("VL ingest: %d %s", status, body) + } + var streams []any + for pod, values := range lokiValues { + streams = append(streams, map[string]any{"stream": map[string]string{"app": app, "pod": pod}, "values": values}) + } + payload, _ := json.Marshal(map[string]any{"streams": streams}) + status, body = hardeningRequest(t, http.MethodPost, lokiURL+"/loki/api/v1/push", string(payload), map[string]string{"Content-Type": "application/json"}) + if status != http.StatusNoContent { + t.Fatalf("Loki ingest: %d %s", status, body) + } + forceVLFlush(t) + if status, body = hardeningRequest(t, http.MethodPost, lokiURL+"/flush", "", nil); status >= 300 { + t.Fatalf("Loki flush: %d %s", status, body) + } + selector := fmt.Sprintf(`{app=%q}`, app) + waitForFixtureOnBothBackends(t, fmt.Sprintf("app:=%q", app), selector, fixtureEnd.Add(-5*time.Minute), fixtureEnd, lines) + + total := fmt.Sprintf(`sum(count_over_time(%s[5m]))`, selector) + byPod := fmt.Sprintf(`sum by (pod) (count_over_time(%s[5m]))`, selector) + zero := "(" + total + " * 0)" + for _, tc := range []struct { + name, query string + nan bool + }{ + {"vector_div_zero", total + ` / 0`, true}, + {"vector_mod_zero", total + ` % 0`, true}, + {"grouped_vector_div_zero", byPod + ` / 0`, true}, + {"zero_over_vector", `0 / ` + total, false}, + {"literal_over_zero_vector", `5 / ` + zero, true}, + {"vector_over_zero_vector", total + ` / ` + zero, true}, + {"vector_mod_zero_vector", total + ` % ` + zero, true}, + {"nonzero_divisor", total + ` / 2`, false}, + // Literal-only queries (`1 / 0`) are not compared live: the compose Loki did + // not answer them within the client timeout in CI. Their values are pinned + // against a port of Loki's MergeBinOp/reduceBinOp in the unit tests + // (TestBinaryScalarFoldMatchesLokiReduceBinOp). + } { + for _, mode := range []string{"query", "query_range"} { + t.Run(tc.name+"/"+mode, func(t *testing.T) { + params := url.Values{"query": {tc.query}} + if mode == "query" { + params.Set("time", strconv.FormatInt(fixtureEnd.Unix(), 10)) + } else { + params.Set("start", strconv.FormatInt(fixtureEnd.Add(-time.Minute).Unix(), 10)) + params.Set("end", strconv.FormatInt(fixtureEnd.Unix(), 10)) + params.Set("step", "60") + } + loki := divisionZeroValues(t, lokiURL, mode, params) + proxy := divisionZeroValues(t, proxyURL, mode, params) + if loki != proxy { + t.Fatalf("%s %s\n loki: %s\n proxy: %s", mode, tc.query, loki, proxy) + } + if got := strings.Contains(loki, `"NaN"`); got != tc.nan { + t.Fatalf("Loki sanity: NaN=%v, want %v: %s", got, tc.nan, loki) + } + }) + } + } +} + +// divisionZeroValues renders the type, the labels and the sample values of a +// response, in a stable order, so two backends can be compared as strings. +func divisionZeroValues(t *testing.T, base, mode string, params url.Values) string { + t.Helper() + status, body := hardeningRequest(t, http.MethodGet, base+"/loki/api/v1/"+mode+"?"+params.Encode(), "", map[string]string{"X-Scope-OrgID": "0"}) + var response struct { + Status string `json:"status"` + Data struct { + ResultType string `json:"resultType"` + Result json.RawMessage `json:"result"` + } `json:"data"` + } + if status != http.StatusOK || json.Unmarshal(body, &response) != nil || response.Status != "success" { + t.Fatalf("%s: %d %s", base, status, body) + } + if response.Data.ResultType == "scalar" { + var point []json.RawMessage + if json.Unmarshal(response.Data.Result, &point) != nil || len(point) != 2 { + t.Fatalf("%s: bad scalar %s", base, body) + } + return "scalar " + string(point[1]) + } + var series []struct { + Metric map[string]string `json:"metric"` + Value []json.RawMessage `json:"value"` + Values [][]json.RawMessage `json:"values"` + } + if json.Unmarshal(response.Data.Result, &series) != nil { + t.Fatalf("%s: bad result %s", base, body) + } + rendered := make([]string, 0, len(series)) + for _, s := range series { + labels, _ := json.Marshal(s.Metric) + points := s.Values + if len(s.Value) == 2 { + points = append(points, s.Value) + } + values := make([]string, 0, len(points)) + for _, point := range points { + values = append(values, string(point[1])) + } + rendered = append(rendered, string(labels)+"="+strings.Join(values, ",")) + } + sort.Strings(rendered) + return response.Data.ResultType + " " + strings.Join(rendered, ";") +}