Repository navigation
Conversation
ecb56cc to
f88b8e8
Compare
|
Reviewed end to end. Mechanically correct and exact against the live Loki; the gap is that it ships dark. One correction for the record: Loki does not round start down and end up. With Live, same query,
Grid matched Loki exactly at steps 7s/37s/137s over 5m/1h/6h for grouped counts, Before merge:
Merge order: land #570 first. This PR places evaluation points on whole-second grid positions, which turns the boundary case #570 fixes from rare into systematic on second-precision ingest. |
szibis
left a comment
There was a problem hiding this comment.
Decision: accept, merge third (after #570), once it stops shipping dark and two ordering issues are fixed.
The alignment itself is exact against a live Loki: at step=137 with an unaligned start, Loki and this branch both return 22 points from 1790057344 to 1790061043, where main returns 21 starting 26s late.
Why we cannot merge it as it stands:
- Nothing turns the flag on. The e2e stack's Loki has
align_queries_with_step: true, and no proxy variant intest/e2e-compat/docker-compose.ymlpasses-align-queries-with-step. So every parity test still compares an aligned Loki against an unaligned proxy, and the fix is never exercised. Set the flag on the compose variants and add a parity case with an odd step (for example 137s). max_query_lengthis checked on the aligned range; Loki checks the original. In Loki the limits middleware runs before step alignment. Alignment can grow the range by up tostep-1ns, so a query exactly at the limit is rejected here and accepted by Loki. Move the check before alignment. (The 11,000-point check is already correctly placed.)- Binary expressions are aligned unconditionally, flag or not (
alignBinaryRangeRequest), so with the default-off flagsum(rate(a[5m]))/sum(rate(b[5m]))is aligned whilesum(rate(a[5m]))is not: two grids in one Grafana panel, and the binop case deviates from a default Loki. Gate it on the same flag, or record why not. It also lacks the negative-remainder guard the new code has. TestAlignRangeRequestAcceptsNanosecondSteppins non-Loki behaviour: Loki reads137000000000as 137 billion seconds. That nanosecond spelling also skips the 11,000-point check while now being aligned; the two step parsers should agree.- Minor: no alignment when
startorendis omitted (Loki aligns its derived bounds too), and the two deliberate non-alignments (no step,/patterns) belong in the documented-deviations doc, not only a code comment.
One correction for the record: Loki does not round start down and end up, it truncates both down, which is what this PR does. Our own docs had that wrong and we are fixing them.
Also remove Co-Authored-By: Claude … from the commit and the "Generated with Claude Code" line from the body: no AI attribution in this repo.
f88b8e8 to
727a04c
Compare
|
Thanks for the correction on the record; the code truncates both bounds down and the body now says so without the round-up wording. Rebased onto current
Attribution removed from the commit and the body. Local: gofmt, build, vet (incl. |
Loki evaluates a metric range query on the grid of `k * step`, not on the client's `start`: the queryrange step-align middleware truncates BOTH bounds down to a multiple of the step when `query_range.align_queries_with_step` is set, which the Loki Helm chart does. On 3.7.1 with step=137s over a start that is not a multiple of 137, every returned timestamp satisfies `ts % 137 == 0` and the first point lands 17 seconds before the requested start. The proxy always started its grid at the request's own `start`, so a side-by-side panel against such a Loki shows the whole series shifted by `start mod step`, while a step that happens to divide the start looks fine. That is why the mismatch only surfaces on odd steps. -align-queries-with-step is off by default, matching Loki's binary default; set it to true to match a chart-deployed Loki. Alignment happens once, at the query_range entry, so every downstream path builds its request and its grid from Loki's bounds. Log queries are left alone. A request without start or end is aligned on the bounds Loki derives for it (end=now, start=end-since). max_query_length is now checked before alignment, on the range the client asked for, as Loki's limits middleware runs before its step-align middleware; alignment can grow the range by up to step-1ns. The 11,000-point resolution check, the alignment and the range evaluators read the step with one parser, so the raw nanosecond integer Grafana sometimes sends (a proxy extension) is limited, aligned and evaluated on the same step. The binary-join alignment stays unconditional and says why: with the flag off a join of a sliding and a tumbling operand would otherwise have no common timestamps. It shares the truncation helper, including the negative-remainder guard. Two paths Loki aligns and this does not, a request with no step and /loki/api/v1/patterns, are recorded in docs/compatibility-loki.md along with the binary-join and nanosecond-step deviations. Every proxy variant in test/e2e-compat/docker-compose.yml now runs with the flag, matching the stack's Loki, and TestCompat_StepAlignedGridMatchesLoki compares the evaluation grid at step=137s with an unaligned start against Loki.
|
Fix: the test aligns Also rebased onto current |
727a04c to
d7521cc
Compare
|
Thanks @clain23 for this work. Aligning range queries to the step grid, as Loki's This branch is far behind main, and the alignment now has to be wired into the stats bucket cache and the folded-result cache that landed since. We are re-implementing it as part of that wider change, building on your approach and crediting you as co-author. Closing this PR in favour of that one; we will link it here once it is open. |
Summary
Loki evaluates a metric range query on the grid of
k * step, not on the client'sstart: the queryrange step-align middleware truncates both bounds down to a multiple of the step whenquery_range.align_queries_with_stepis set, which the Loki Helm chart does (production/helm/loki/values.yamlat v3.7.1). It is visible from the outside: on 3.7.1 withstep=137sover a start that is not a multiple of 137, every returned timestamp satisfiests % 137 == 0and the first point lands 17 seconds before the requested start.The proxy always started its grid at the request's own
start, so a side-by-side panel against such a Loki shows the whole series shifted bystart mod step:The last row is why this is easy to miss: a step that happens to divide the start looks fine, so the mismatch only surfaces on odd steps.
Why a global flag
Loki's own knob is global.
AlignQueriesWithStepis a field on the queryrangeConfigstruct (pkg/querier/queryrange/queryrangebase/roundtrip.go), read once when the middleware chain is built, andpkg/validation/limits.gocarries no per-tenant override for it.-align-queries-with-stepis off by default, matching Loki's binary default; set it totrueto match a chart-deployed Loki.Scope
Alignment happens once, at the
query_rangeentry, so every downstream path builds its request and its own grid from Loki's bounds. Log queries are left alone (no evaluation grid), so the rewrite is gated on the parsed expression producing a matrix. A request withoutstartorendis aligned on the bounds Loki derives for it (end=now,start=end-since), which are written into the request.Ordering against the limits, as in Loki:
max_query_lengthis checked before alignment, on the range the client asked for. Loki's limits middleware runs before its step-align middleware, and alignment can grow the range by up tostep-1ns, so a query exactly at the limit was rejected where Loki accepts it.Two paths Loki aligns and this deliberately does not, plus one it aligns regardless of the flag, are recorded in
docs/compatibility-loki.md("What Stays Outside Loki Parity"):step. Loki derivesmax(floor(range/250), 1s)at decode time and aligns to that; the proxy's range evaluator falls back to1m, so aligning to Loki's derived step would move the bounds off the grid the response is built on./loki/api/v1/patterns. Same middleware in Loki, but its default step isrange/250against this proxy'srange/120.start+k*step, so with the flag off and an unaligned start the join would otherwise have no common timestamps, and an empty join is worse than one shifted by less than a step against a default Loki. The reason is in the code comment; with the flag on the request arrives already aligned and the call is a no-op. It shares the truncation helper, including the negative-remainder guard.Changelog
One
### Addedentry for the flag and two### Fixedentries (max_query_lengthordering, resolution-check step parser) under## [Unreleased].Validation
test/e2e-compat/docker-compose.ymlnow passes-align-queries-with-step=true, matching the stack's Loki (align_queries_with_step: true).TestCompat_StepAlignedGridMatchesLoki(new,TestCompat_*CI shard) queriessum(count_over_time({app="api-gateway"}[5m]))atstep=137swith an unaligned start on both sides, checks Loki's timestamps are on the grid, and requires the proxy's timestamp set to equal Loki's.TestQueryRangeAlignsBoundsBeforeQueryingTheBackendruns the handler both ways against a recording backend: with the flag on the read starts 17s earlier, with it off it is unchanged.TestAlignRangeRequestToStepGridcovers the truncation, the already-aligned no-op, the log-query exemption and the rewritten bound reachingr.URL;TestAlignRangeRequestToStepGrid_DerivesLokiDefaultBounds(new) covers omittedstart/endandsince.TestResolutionLimitReadsStepLikeTheEvaluator(new) pins the shared step parser:1000sand1000000000000are limited alike.TestAlignRangeRequestAcceptsNanosecondStepis kept, reworded as the proxy-extension agreement it is rather than Loki behaviour.TestHelmExtraArgsCoveragerequires every Go flag to appear invalues.yaml; the flag is listed there and indocs/configuration.md.gofmt -s -lempty,go build ./...,go vet ./...(including-tags=e2e),golangci-lint run(v2.13.2, 0 issues),scripts/ci/check_changelog_pr.pyandscripts/ci/check_conformance.pyok,go test ./... -count=1green across all packages, on top of currentmain(v1.86.0).Merge order: after #570, which fixes the window edges this alignment makes systematic on second-precision ingest.