Repository navigation
fix: an unwrapped value Loki converts makes its sample and one that overflows fails the query like Loki - #730
Merged
Merged
Conversation
github-actions
Bot
added
size/XL
Contributor
⚪ Performance A/B: no change beyond noise130 shape×range — 130 ⚪ same · results vs Loki: 104 same, 0 new difference(s), 26 pre-existing Every shape is within noise of the base build (±30% and 100 ms). 130 unchanged shape×range (within noise)
Cold (first-run) timings — what a user opening a panel sees
↺ Loki cold taken from the first set that ran the same query; Loki's cache answered this one. Why these shapes
base |
Contributor
PR Quality ReportCompared against base branch Coverage and tests
Compatibility
Performance smokeLower CPU cost (
State
|
Contributor
✅ Visual smoke: passedBase
Gate: a base-vs-PR data difference passes when the PR is closer to Loki, and fails when the PR diverges from Loki or Loki cannot decide it (at a range Loki does not hold, a difference passes when the same page proved it closer to Loki at a shorter range; otherwise label the pull request Core set montages (5): base | PR | Lokidd-fields-1h
dd-landing-1h
dd-service-logs-1h
explore-logs-plain-1h
explore-metric-plain-1h
All montages of this run: |
github-actions
Bot
added
size/XL
szibis
force-pushed
the
fix/unwrap-gate-parsefloat
branch
from
October 12, 2026 11:01
2d738b7 to
4269be1
Compare
github-actions
Bot
added
size/XL
szibis
marked this pull request as ready for review
October 12, 2026 13:23
This was referenced Oct 22, 2026
szibis
force-pushed
the
fix/unwrap-gate-parsefloat
branch
from
October 22, 2026 14:26
53041df to
0584a2d
Compare
github-actions
Bot
added
size/XL
szibis
force-pushed
the
fix/unwrap-gate-parsefloat
branch
from
October 22, 2026 20:35
0584a2d to
7c0d042
Compare
github-actions
Bot
added
size/XL
…verflows fails the query like Loki The unwrap gate now has the syntax of strconv.ParseFloat (hexadecimal floats, digit separators in exponents, exponent 308, infinity), time.ParseDuration (leading zeros) and humanize.ParseBytes (commas anywhere in the number, any Unicode space around the unit). A range filter after the math pipe drops the values whose conversion overflows, the counting gate flags them so the query fails with SampleExtractionErr, and nan values are applied to the answer as NaN where every function of the query poisons its result.
…n-only check shares the backend call The flagged rows' NaN is read as 0 with the math pipe's default operator instead of a format pipe, which rewrote the converted column as text that the stats read only up to 27 characters. A check that only counts nan values no longer separates identical concurrent stats calls. Adds infinity and instant nan answers to the handler tests.
… grammar refused The counting gate tests the grammar once per row and tells infinity (flag 3) and nan (flag 2) apart only on the rows it refused; the plain gate selects infinity with a case-insensitive phrase filter instead of a regular expression per row. The duration limit is written without an exponent, go-humanize v1.0.1 checks the bytes port in the property test, the value-forms e2e matrix is trimmed for the shared group, a probe-less check is covered by a coalescing test, and the conversion-error row reports overflow.
…ouped instants The over-time functions answer query_range one step early or with the step's bucket instead of the window (semantics/unwrap-over-time-range-windows) and an ungrouped series lacks the parsed labels Loki keeps (semantics/unwrap-per-series-identity-raw-rows), so comparing their range answers said nothing about the value gate: every spelling failed on main and on this branch alike. The rate stays on query_range; sum, avg, max and min over time are compared on instant queries grouped by the service, which match Loki. A window that holds only a nan under max or min stays a registered residual. The ungrouped instant sum over sum_over_time or avg_over_time of a nan series still differs from Loki (8 subtests, identical on main) and is added to the registry row.
Twelve shapes (plain unwrap, with an error filter, sum by over rate, duration() and bytes(), max, every production stream, instant) over 1h with Loki and 6h, 24h and 7d against main, 5 runs, plus a 7-run re-measure of the duration, bytes and plain shapes and the control set at 1h and 24h. Every verdict is within the harness noise; a duration() unwrap is 8 to 16 ms slower on a 60 to 80 ms query. The performance evidence and gap reports are regenerated.
The registry row and the changelog entry carry the measured cost of the gate (A/B against main), the ungrouped instant sum over a nan series that still differs from Loki, and the live run on VictoriaLogs v1.40.0, v1.47.0 and v1.52.0.
…arity run An instant sum without grouping over sum_over_time or avg_over_time of a series that holds a nan is a registered residual of the unwrap gate (semantics/unwrap-gate-parsefloat-divergences): Loki answers NaN and the proxy drops the series. The parity test compared it and failed on the stack, so it skips that outer for those two functions, like the other residuals it already skips.
szibis
force-pushed
the
fix/unwrap-gate-parsefloat
branch
from
October 22, 2026 21:10
3b1958e to
7e4575e
Compare
github-actions
Bot
added
size/XL
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Gap
semantics/unwrap-gate-parsefloat-divergences. The unwrap gate stood in forstrconv.ParseFloat,time.ParseDurationand humanizeParseBytes(Lokipkg/logql/log/metrics_extraction.go:316-338) with regular expressions. Those capped the exponent and did not know hexadecimal floats, exponent digit separators,infinity, comma runs or Unicode space folding. Values Loki converts were dropped, and values that overflow were kept where Loki fails the query.VictoriaLogs facts, checked in its v1.43.1 and v1.50.0 source and by running its query code in-process:
mathreads hex floats,1e308and1e1_0as Go does.mathgives NaN forinfinity,nanand every overflow.sumrestarts at a NaN.1e308,1e1_0, any exponentmathalready reads theminfinityinfon a copy by a conditionalreplace_regexp1e309,1000e307,0x1p2000,2562048h,19EB)mathdrops the row; the counting gate flags it, and the proxy re-reads it to answer Loki's error,5,1.5,0, no-break and other Unicode spaces, Kelvin signnansumcannot carry itAlso fixed:
01m) were silently kept as NaN.infrows.+nanand-nanare reported.Remaining (the registry row stays open with this shorter list):
nanunder max/min windows.nan.sort/limitafter stats, two gates).Tests
TestUnwrapCountedGateFlagsOverflow,TestUnwrapNaNReachesTheLokiAnswerandTestUnwrapInfinityAndInstantNaNReachTheAnswer.TestUnwrapConversionErrorDifferential.TestCompat_UnwrapValueFormsParity,TestCompat_UnwrapOverflowParity.Pending before ready (proofs that need the compose stack)
x:<=inf,-(a b)insideformat if,filter A or (B),math x default 0, exponent literals.bench/ab/results/2026-10-12-unwrapgate-*.jsonpredate the current base.Rebased onto main (2026-10-22, third update)
Base
4e4f3e8a(v2.30.0 plus #841, #842 and #843), head7e4575e8: eight commits, all signed. The seven original subjects are kept in order ("chore(conformance): regenerate the parity reports for the unwrap gate rows" holds the regenerated files), plus one new last commit, "test: leave the registered ungrouped nan sum out of the value forms parity run" (see below). Previous head0584a2db(base304b5549). Main gained, among others, #727 (sum/avg/max/min_over_time unwrap sample atend), #728 (first/last_over_time unwrap under an aggregation) and the release metadata commits for v2.26.1 to v2.30.0; this PR no longer overlaps any open PR.CHANGELOG.mdorigin/mainon every rebase: #842's### Securityentry stays first under## [Unreleased]and this PR's### Fixedentry follows it, above## [2.30.0]; no released section is edited (git diff origin/main HEAD --numstat -- CHANGELOG.mdis55 0). The entry is added in the first commit, as before.internal/proxy/unwrap_ungrouped_sum_differential_test.go(the emulator's pipe switch: main addedmathcopy andunpack_jsonbranches, this PR thefilter <field>:*branch)internal/proxy/extraction_list_outside_without_test.go(the golden table, see below)bench/ab/shapes.json(both appended a set: mainjson-error-split, this PRunwrap-gate)unwrap-gateset unchanged; checked by JSON-diffing every set of the result against main (21 sets equal) andunwrap-gateagainst the PR (equal); 22 sets,rangesunchanged. File stays in the repo'sindent=1format.conformance/registry/generated/{wiring,perf-evidence}.json,conformance/reports/{parity-gaps,performance}.mdwire.py,gaps.py,roadmap.py,parity_gaps.pyandperf_evidence.py; all five pass--checkon the final tree (gaps.mdandroadmap.mdare unchanged by this PR).bench/visual/spec.jsonis not touched by this PR.CI finding fixed in this update
TestCompat_UnwrapValueFormsParitycomparedsum (sum_over_time(...))andsum (avg_over_time(...))over the nan forms (an ungrouped instant sum). That is a registered residual of this PR (semantics/unwrap-gate-parsefloat-divergences: Loki answers NaN, the proxy drops the series), so the e2e groupcorefailed on 8 subtests. The test now skips that outer for those two functions, like the other residuals it already skips. The residual itself is unchanged and stays registered.Golden table: what changed and why it is correct
The table is
extractionListGoldensininternal/proxy/extraction_list_outside_without_test.go(69 entries: modeinstant,tumblingorslidingx query). Each entry pins the LogsQL sent to VictoriaLogs and the answer the proxy gives for a shape that goes through an extraction list (| logfmt amount, route). #727 changed 24 answers (the end sample) and this PR changes the text sent. There is no update flag: the test printsgotandwanton a mismatch. The table was regenerated by taking main's table and rewriting each mismatching entry from thegotthe test printed, with a script that refuses to report success if an entry'sAnswerdiffers from the one on main.Answers: 0 of 69 changed. They are main's, so they carry fix: sum, avg, max and min over time with unwrap answer the sample at end on query_range, like Loki #727's extra sample at
start(the old values one step later, the last one onend,semantics/unwrap-range-last-point) and fix: first_over_time and last_over_time with unwrap under an aggregation read each series' sample from VictoriaLogs row_min and row_max instead of a stats function it lacks #728's first/last handling. The fixture holds plain decimalamountvalues only (no hexadecimal floats,inf/infinity,nan, separators or overflow), and on those Loki'sstrconv.ParseFloatconverts exactly the rows the old gate kept. The gate change therefore cannot move an answer, and the unchanged answers are the check that it does not.Sent text: 50 entries changed, 19 did not. The 19 are the shapes without an unwrap gate (count/rate/bytes over an extraction list) and the instant raw
max_over_time, which sendslimit 1000001. The 50 are the unwrap shapes (sum,avg,min,max,count,topk,bottomkovermax/min/avg_over_time, grouped and ungrouped,without, in the three modes) and twosum(rate(... | unwrap ...))shapes. What changed in the text:ParseFloatgrammar (hexadecimal floats,1e308, exponent digit separators) instead of the capped regular expression;__lvp_bad0/1/2 (and 3 forinfinityin the counting gate), sonanrows are grouped apart and the proxy can answer NaN like Loki;infinityis shortened toinfon a copy (copy, conditionalreplace_regexp) because VictoriaLogs'mathreadsinfbut notinfinity;-(__lvp_v:<=inf)aftermathmarks a value that overflowed (Loki fails such a query, so the proxy must see it) andmath __lvp_v default 0keeps the column numeric;math, the presence filterfilter __lvp_rate:*the translator adds because of the copy.Loki v3.7.7 (
pkg/logql/log/metrics_extraction.go) converts withstrconv.ParseFloatand fails the query on a conversion error, which is the behaviour all of these pipes implement; none of them changes what is returned for a decimal value.Gates on the final tree (Go sources identical to the tested tree apart from #842's one-line change and the e2e test skip) (no Docker,
GOMEMLIMIT=3GiB GOMAXPROCS=4; Go 1.27.3)gofmt -s -l: clean.go build ./...,go vet ./...,go vet -tags e2e ./test/e2e-compat/: clean.golangci-lint run(v2.14.0, the version inci.yaml, built with Go 1.27.3): 0 issues.go test -short -p 1 -count=1 ./...: all 13 packages ok. Peak RSS with/usr/bin/time -l(measured on the previous base88edc57b; the commits since change no Go code): head 1,824,292,864 B (1.70 GiB), 133 s;origin/mainat the same base 1,778,909,184 B (1.66 GiB), 118 s (+2.5 percent).-race -shorton the tests this PR adds or changes (TestUnwrap*,TestSeriesLimit_*,TestExtractionListOutsideWithout*,TestStripUnwrapCounter,internal/proxyandinternal/translator): 425 passed.go test ./internal/... -count=1 -jsonthenpython3 scripts/ci/check_conformance.py --go-results: conformance gate ok (478 tests declare 140 items, 136 with a passing run, 0 failing; nofailevent in the run).wire,gaps,roadmap,parity_gaps,perf_evidence) with--check: pass.ci.yaml(python3 -m unittest scripts.ci.tests.test_check_quality_gate ... test_scan_release_image): 203 tests OK.scripts/ci/sync_observability_assets.sh --check: in sync.python3 scripts/ci/check_changelog_pr.py --base origin/main --head HEAD: ok.Translator cost (re-measured on the new base).
go test -bench 'BenchmarkTranslate_Unwrap' -benchmem,origin/mainvs head, benchstat, n=12 and n=14. The machine was heavily loaded by other jobs while this ran (a main-versus-main control drifted by 80 percent between two runs), so the times are only indicative; the allocation figures are exact.Interleaved run (main and head test binaries alternated 14 times, so a load drift hits both):
Translate_UnwrapSumByJSONTranslate_UnwrapMaxPlainSequential
-count=6x 2 runs (quieter moment):UnwrapSumByJSON27.28 us -> 26.28 us (~, p=0.20);UnwrapMaxPlain5.46 us -> 6.66 us (+22%, p<0.001), with the same B/op and allocs/op. So the gate costs +2 allocations and about 1.5 KiB per unwrap translation, and between about 9 and 22 percent of the translation time of the smallest query (about 1.2 us on an idle machine, against 5.5 us); the larger query is within noise. The VictoriaLogs-side cost per row is the A/B below. The earlier text of this PR said the absolute cost stays under a microsecond; on this base the small query is a little above it.go.mod.
github.com/dustin/go-humanize v1.0.1is added (test-only: the property tests compare the bytes pattern and its portparseHumanByteswithhumanize.ParseBytes); main only mentions it in comments.go mod tidy -diffis clean.Proofs to be refreshed (phase 2, need the stack)
Supported VictoriaLogs for the e2e runs: the floor v1.49.0 and the newest v1.53.0, with the CI pin v1.52.0 (the PR text from before the floor moved named older lines). Commands run from the worktree
$SCRATCH/wt-730on the PR head; the Colima VMcolima-lvpis the Docker host (DOCKER_CONTEXT=colima-lvp), and the shared e2e stack must not be running while a fresh stack is started on the same ports.1. e2e vs Loki (one fresh stack per group and per VictoriaLogs version)
Then the two CI groups that cover the touched code, each on its own fresh stack on v1.52.0 and v1.49.0 (same up/down lines,
-runpattern copied from.github/workflows/ci.yaml):2. A/B proof (previous release / base vs PR vs Loki, own compose project and ports)
Plus
go test -run '^$' -bench <touched benchmarks> -benchmem -count=6 ./internal/translator ./internal/proxyonorigin/mainand the head, compared withbenchstat, and the backend-safety load test (many 24h/7d queries concurrently, VictoriaLogs RestartCount before and after,/labelsprobe answering 200).3. Visual proof (isolated stack, base | PR | Loki)
Plan: no fix-proof case is registered (
fix_casesis empty), so add afixesvisual entry for the gate if it is visual-coverable; the plan selects the core set and the unwrap entries (explore-unwrap-sum,explore-unwrap-unit-string,explore-unwrap-missing-label,explore-unwrap-conversion-error,explore-unwrap-duration-seconds) at 1h, 31 captures.4. Real Grafana range matrix: the shapes below through the Loki-type datasources of the proxy and of Loki direct, over 5m, 15m, 1h, 6h, 24h and 7d where the data exists, compared for status, errors, series counts and totals.
Matrix shapes: unwrap over
latency_ms,amount,duration_ms,duration(ttl),bytes(size_bytes)with and without| __error__="",rate,max_over_time, instant and range, plus value forms with hexadecimal floats,1e308,infinity,nan, and an overflowing value (1e309,19EB) which must answer Loki's 400 text.The committed
bench/ab/results/2026-10-12-unwrapgate-*.jsonare measured on a base before the v1.49.0 floor and against v2.11.2: replace them with the new run (--save bench/ab/results/), re-runconformance/scripts/perf_evidence.pyandgaps.py, and refresh the cost numbers in the registry rowsemantics/unwrap-gate-parsefloat-divergencesand in the changelog entry.