Repository navigation
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
Merged
Conversation
github-actions
Bot
added
size/XL
Contributor
❌ Visual smoke: failedBase Fix proofCaptures of the exact cases this pull request adds or fixes (registry case, base | PR | Loki). The verdict compares the PR with Loki where the base did not match. The gate fails a case unless one capture is fixed or improved, or, beyond the Loki data window, the base failed where Loki and the PR both answered with data (the label does not excuse it; only
explore-unwrap-first-last-under-sum-15m: still differs
explore-unwrap-first-last-under-sum-1h: still differs
explore-unwrap-first-last-under-sum-6h: undecided
Regression check
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 explore-unwrap-first-last-under-sum-15m: data differs base vs PR and neither build matches Loki; label the pull request `visual-change-expected` if intended (1):
explore-unwrap-first-last-under-sum-1h: data differs base vs PR and neither build matches Loki; label the pull request `visual-change-expected` if intended (1):
explore-unwrap-first-last-under-sum-6h: data differs base vs PR and no Loki data for this range; label the pull request `visual-change-expected` if intended (1)
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: |
Contributor
⚪ Performance A/B: no change beyond noise119 shape×range — 119 ⚪ same · results vs Loki: 97 same, 0 new difference(s), 22 pre-existing Every shape is within noise of the base build (±30% and 100 ms). 119 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
|
szibis
added a commit
that referenced
this pull request
Oct 12, 2026
…-aggregation, with its visual fix proof page
github-actions
Bot
added
size/XL
szibis
added a commit
that referenced
this pull request
Oct 12, 2026
…-aggregation, with its visual fix proof page
szibis
force-pushed
the
fix/unwrap-first-last-aggregation
branch
from
October 12, 2026 11:01
b8d8594 to
09e9dcf
Compare
github-actions
Bot
added
size/XL
szibis
added a commit
that referenced
this pull request
Oct 19, 2026
…-aggregation, with its visual fix proof page
szibis
force-pushed
the
fix/unwrap-first-last-aggregation
branch
from
October 19, 2026 10:19
09e9dcf to
d6b937e
Compare
github-actions
Bot
added
size/XL
szibis
marked this pull request as ready for review
October 19, 2026 10:20
szibis
added a commit
that referenced
this pull request
Oct 22, 2026
…-aggregation, with its visual fix proof page
szibis
force-pushed
the
fix/unwrap-first-last-aggregation
branch
from
October 22, 2026 14:26
d6b937e to
8b338f0
Compare
github-actions
Bot
added
size/XL
…ion read each series' sample from VictoriaLogs row_min and row_max instead of a stats function it lacks
…y, the topk, sort and no-by branches are pinned, and the open shapes are registered
…-aggregation, with its visual fix proof page
szibis
force-pushed
the
fix/unwrap-first-last-aggregation
branch
from
October 22, 2026 19:42
8b338f0 to
e070299
Compare
github-actions
Bot
added
size/XL
szibis
added a commit
that referenced
this pull request
Oct 22, 2026
Chart version and README badge for v2.30.0, and the CHANGELOG sections matched to the published releases: the first_over_time/last_over_time entry (#728) moves from [2.29.0] to [2.30.0]. #728 merged before the v2.29.0 metadata pull request, whose added heading then filed the entry under [2.29.0] although v2.29.0 was cut before #728.
szibis
added a commit
that referenced
this pull request
Oct 22, 2026
Chart version and README badge for v2.30.0, and the CHANGELOG sections matched to the published releases: the first_over_time/last_over_time entry (#728) moves from [2.29.0] to [2.30.0]. #728 merged before the v2.29.0 metadata pull request, whose added heading then filed the entry under [2.29.0] although v2.29.0 was cut before #728. Co-authored-by: szibis <szibis@users.noreply.github.com>
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-first-last-under-aggregation. Loki 3.7.7 answers one series forsum(first_over_time({app="payment-service"} | logfmt | unwrap amount [5m])), and likewise forlast_over_time. The proxy sent VictoriaLogs astats first(...)/last(...); VictoriaLogs has neither function, so the proxy answered 422 on query_range and query. The same 422 happened under avg/min/max/count, withby (...), as a topk/bottomk inner, as a binary operand, and for the bare form without a parser.Fix (VictoriaLogs does all of it, in one stats query). The query keeps each series' sample with
row_min/row_maxon_time, unpacks it to a number, then applies the outer aggregation:pkg/logql/range_vector.go:818-843), folded by the vector aggregation (pkg/logql/evaluator.go).unpack_jsonpipe, so a non-numeric value still answers Loki's 400.offsetis honoured.stdvar,topkandbottomkstill fail loudly rather than answering a wrong 200.Remaining differences (already registered):
semantics/unwrap-over-time-range-windows).(_stream, _msg)(semantics/unwrap-per-series-identity-raw-rows).row_maxkeeps the first row where Loki keeps the later one.Tests
TestFirstLastOverTimeUnderAggregationTranslatesToRowStats(12 shapes),TestFirstLastOverTimeBareIsOneValuePerSeries,TestFirstLastOverTimeUnsupportedGroupedAggregationFailsLoudly.TestUnwrapCountingQueryandTestUnwrapConversionErrorAnswersLokiText.TestCompat_UnwrapFirstLastUnderAggregation,TestCompat_UnwrapFirstLastTopkBottomk.Rebased on main
Rebased onto main after the VictoriaLogs v1.49.0 floor (#770) and the
sum withoutfold (#739); the open question aboutrow_min(_time, …)before v1.43 no longer applies, since every supported version has it. #739's pinned tests pass unchanged. On main the new tests fail (406 subtests: the proxy sendslast(...), which VictoriaLogs lacks, and answers 502); on this branch they pass.sum|max|min|avg|count without (pod) (first_over_time|last_over_time(... | unwrap ...))now goes through thewithoutfold where main answered 422; after a bare parser it carries the series identity of the open rowsemantics/unwrap-per-series-identity-raw-rows.semantics/first-last-over-time-translationstays open: both of its requests now translate to therow_maxchain, but its offset and|~shapes have no committed test and its comparison against Loki has not run.Proofs in progress
Rebased onto main (2026-10-22)
Base moved from
8d57747b(v2.19.0) to304b5549(v2.25.1 plus #827). This PR is not stacked on #727 (its branch had main as the merge base), so it is rebased onorigin/mainalone. Head8b338f04, three commits, all signed, subjects unchanged.CHANGELOG.md## [Unreleased]/### Fixed.bench/visual/spec.json(same append point as #731'sexplore-unwrap-quantile-over-time)conformance/registry/generated/wiring.json,conformance/reports/{gaps,parity-gaps,roadmap}.mdtranslator.go,unwrap_conversion_error.goand tests)unwrapconversion-error counter and the translator changes of main since v2.19.0 compose with theunpack_jsonallowance.Gates (phase 1, no Docker, capped
GOMEMLIMIT=3GiB GOMAXPROCS=4)gofmt -l,go build ./...,go vet ./...andgo vet ./test/e2e-compat/...andgo vet -tags=e2e ./test/e2e-compat/...(the e2e files carry//go:build e2e; compile only, nothing run): clean.go test -short -count=1 -p 1 ./...(all packages, 98 s, peak RSS 1.70 GB with/usr/bin/time -l, not compared with main in this phase) and the./internal/... -jsonrun that feeds the conformance gate: all ok.go test -count=1 -run 'TestSlidingWindowStats_|TestOrderedJSONExtractedMetricDifferential' ./internal/proxywithout-short(the tests that skip under-shortand declare the registry items near this change; CI's evidence step runs without-short): ok.-raceon the touched tests:TestFirstLastOverTime*,TestUnwrapCountingQuery,TestUnwrapConversionErrorAnswersLokiText,TestUngroupedSumUnwrapDifferential,TestMatrix_MetricFunctions_AllSupportedok (478 tests,internal/proxyandinternal/translator).python3 scripts/ci/check_conformance.py --go-results <unit json>: ok (422 tests declare 138 items, 134 with a passing run, 0 failing).python3 -m unittest discover -s scripts/ci/tests -p 'test_*.py': 181 tests OK. Harness tests (bench/ab,bench/visual,bench/parity): OK.python3 scripts/ci/check_changelog_pr.py --base origin/main --head HEAD: ok.go run ./cmd/configdoc -check: ok.Overlaps. With #727 and #730 the conflicts are textual only:
CHANGELOG.md,bench/visual/spec.json, generated files, andinternal/proxy/unwrap_ungrouped_sum_differential_test.goagainst #730 (both add a pipe branch to the VictoriaLogs emulator at the same spot; keep both). Merged together in a scratch tree (727+728+730) the only failing test is the golden table of #727/#730.Not in this PR. On
query_range,first_over_timeandlast_over_timestill answer every point one step early and omit the sample atend(the bucket labelling that #727 fixes for sum, avg, min and max). It is outside this PR's scope and stays a follow-up.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-728on 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:
explore-unwrap-first-last-under-sumat 15m/1h/6h (the fix proof,fixes: semantics/unwrap-first-last-under-aggregation), the core set and the changed-file entries (37 captures; the plan trims the over-budget ones and dropsexplore-sum-without-count,explore-topk-sum-without— raise--max-capturesor run those two by hand if wanted).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:
sum by (service_name) (last_over_time({service_name="payment-service"} | logfmt | unwrap amount [$__auto])), the same withfirst_over_time, withavg/min/max/count,by (app), as a topk/bottomk inner and under a binary operation.