Skip to content

feat(proxy): add -align-queries-with-step for Loki's evaluation grid - #574

Closed
clain23 wants to merge 1 commit into
ReliablyObserve:mainfrom
maestra-io:fix/align-range-to-step-grid
Closed

clain23 wants to merge 1 commit into
ReliablyObserve:mainfrom
maestra-io:fix/align-range-to-step-grid

Conversation

@clain23

@clain23 clain23 commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Summary

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 (production/helm/loki/values.yaml at v3.7.1). It is visible from the outside: 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:

step requested start Loki's first point proxy's first point shift
137s 1700000000 1699999983 1700000000 17s
97s 1700000000 1699999981 1700000000 19s
60s 1700000000 1700000000 1700000000 0

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. AlignQueriesWithStep is a field on the queryrange Config struct (pkg/querier/queryrange/queryrangebase/roundtrip.go), read once when the middleware chain is built, and pkg/validation/limits.go carries no per-tenant override for it. -align-queries-with-step is off by default, matching Loki's binary default; set it to true to match a chart-deployed Loki.

Scope

Alignment happens once, at the query_range entry, 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 without start or end is 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_length is 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 to step-1ns, so a query exactly at the limit was rejected where Loki accepts it.
  • The 11,000-point resolution check, the alignment and the range evaluators now read the step with one parser. The raw nanosecond integer Grafana sometimes sends is a proxy extension (Loki reads a bare integer as seconds); it was exempt from the resolution check while being evaluated on that step, and would have been aligned on it too. Now all three agree.

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"):

  • A request with no step. Loki derives max(floor(range/250), 1s) at decode time and aligns to that; the proxy's range evaluator falls back to 1m, 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 is range/250 against this proxy's range/120.
  • Binary joins of two vector operands keep aligning both bounds whatever the flag says, as before. A tumbling operand comes from epoch-aligned VictoriaLogs buckets and a sliding one is evaluated at 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 ### Added entry for the flag and two ### Fixed entries (max_query_length ordering, resolution-check step parser) under ## [Unreleased].

Validation

  • The e2e stack exercises the flag. Every proxy variant in test/e2e-compat/docker-compose.yml now passes -align-queries-with-step=true, matching the stack's Loki (align_queries_with_step: true).
  • TestCompat_StepAlignedGridMatchesLoki (new, TestCompat_* CI shard) queries sum(count_over_time({app="api-gateway"}[5m])) at step=137s with 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.
  • TestQueryRangeAlignsBoundsBeforeQueryingTheBackend runs the handler both ways against a recording backend: with the flag on the read starts 17s earlier, with it off it is unchanged.
  • TestAlignRangeRequestToStepGrid covers the truncation, the already-aligned no-op, the log-query exemption and the rewritten bound reaching r.URL; TestAlignRangeRequestToStepGrid_DerivesLokiDefaultBounds (new) covers omitted start/end and since.
  • TestResolutionLimitReadsStepLikeTheEvaluator (new) pins the shared step parser: 1000s and 1000000000000 are limited alike.
  • TestAlignRangeRequestAcceptsNanosecondStep is kept, reworded as the proxy-extension agreement it is rather than Loki behaviour.
  • TestHelmExtraArgsCoverage requires every Go flag to appear in values.yaml; the flag is listed there and in docs/configuration.md.
  • Local: gofmt -s -l empty, go build ./..., go vet ./... (including -tags=e2e), golangci-lint run (v2.13.2, 0 issues), scripts/ci/check_changelog_pr.py and scripts/ci/check_conformance.py ok, go test ./... -count=1 green across all packages, on top of current main (v1.86.0).

Merge order: after #570, which fixes the window edges this alignment makes systematic on second-precision ingest.

@szibis

szibis commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

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 align_queries_with_step: true it truncates both bounds down (queryrangebase.stepAlign). Measured on the stack with step=137, start%137=18, end%137=56: first point start-18, last point end-56. This PR matches that.

Live, same query, step=137, unaligned start:

axis
Loki n=22, first 1790057344, last 1790061043
main n=21, first 1790058148 (shifted 26s), last 1790061025
this branch, flag on n=22, first 1790057344, last 1790061043

Grid matched Loki exactly at steps 7s/37s/137s over 5m/1h/6h for grouped counts, rate(… | json …) and topk. Correct scoping: /index/volume_range is not step-aligned by Loki and the proxy matches; instant queries unaffected; POST is aligned.

Before merge:

  1. The flag is off and nothing turns it on, so no parity test exercises it. The stack's Loki sets align_queries_with_step: true, and no proxy variant in the compose file passes -align-queries-with-step. Every proxy-vs-Loki comparison therefore keeps an aligned Loki against an unaligned proxy. Please set it on the compose variants and add a parity case with an odd step.
  2. The existing binary-expression alignment is not brought under the flag. alignBinaryRangeRequest already truncates both bounds for every binop regardless. With the flag off, sum(rate(a[5m]))/sum(rate(b[5m])) is aligned while sum(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 say why not. It also lacks the negative-remainder guard the new code has, and two docs describe the old behaviour.
  3. max_query_length is now checked on the aligned range; Loki checks the original. In Loki the limits middleware precedes step alignment. Alignment can grow the range by up to step-1ns, so a query exactly at the limit is rejected here and accepted by Loki. (The 11,000-point check is correctly placed before alignment.)
  4. TestAlignRangeRequestAcceptsNanosecondStep pins non-Loki behaviour: Loki reads 137000000000 as 137 billion seconds. That nanosecond spelling is a pre-existing proxy extension, and it also skips the 11,000-point check while now being aligned — the two step parsers should agree.
  5. Minor: no alignment when start or end is omitted (Loki aligns its derived bounds too); setForm writes the URL only when the key is already in the query string, which works today but is fragile; a Parse failure silently skips alignment, so one query in a panel can stay on the old grid.
  6. Good: sane default matching Loki's, Helm value, docs row, changelog accurate. The two deliberate non-alignments (no step, /patterns) belong in the documented-deviations doc, not only a code comment.
  7. Attribution: Co-Authored-By: Claude … in the commits and a "Generated with Claude Code" line plus session link in the body. No AI attribution is allowed in this repo; we will strip it at squash time, but please remove it at the source.

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 szibis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Nothing turns the flag on. The e2e stack's Loki has align_queries_with_step: true, and no proxy variant in test/e2e-compat/docker-compose.yml passes -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).
  2. max_query_length is 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 to step-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.)
  3. Binary expressions are aligned unconditionally, flag or not (alignBinaryRangeRequest), so with the default-off flag sum(rate(a[5m]))/sum(rate(b[5m])) is aligned while sum(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.
  4. TestAlignRangeRequestAcceptsNanosecondStep pins non-Loki behaviour: Loki reads 137000000000 as 137 billion seconds. That nanosecond spelling also skips the 11,000-point check while now being aligned; the two step parsers should agree.
  5. Minor: no alignment when start or end is 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.

@clain23

clain23 commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

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 main (v1.86.0):

  1. No longer dark. Every proxy variant in test/e2e-compat/docker-compose.yml passes -align-queries-with-step=true, matching the stack's Loki. TestCompat_StepAlignedGridMatchesLoki (new, in the TestCompat_* shard) runs sum(count_over_time({app="api-gateway"}[5m])) at step=137s with an unaligned start on both sides, asserts Loki's timestamps are on the grid, and requires the proxy's timestamp set to equal Loki's.
  2. max_query_length before alignment. The check moved above the alignment call, on the client's range, with a comment on Loki's middleware order. It was previously after the offset shift, which does not change the length, so nothing else moves.
  3. Binary alignment. Kept unconditional, and the reason is recorded in the code comment, the CHANGELOG and docs/compatibility-loki.md: with the flag off, a join of a sliding operand (start+k*step) and a tumbling one (epoch-aligned VictoriaLogs buckets) would otherwise have no common timestamps, and an empty join is worse than one shifted by less than a step against a default Loki. It now shares alignDownToStep with the entry alignment, so it has the negative-remainder guard. If you would rather gate it and accept the empty join, that is a one-line change; I did not want to make that call for you.
  4. Step parsers agree. exceedsLokiRangeResolution reads the step with parseStepToNanos, the parser the evaluators and the alignment use, so the nanosecond-integer spelling is limited, aligned and evaluated on one step. TestResolutionLimitReadsStepLikeTheEvaluator pins it; TestAlignRangeRequestAcceptsNanosecondStep is reworded as the proxy-extension agreement it is, not Loki behaviour. The extension itself is listed as a deviation in docs/compatibility-loki.md.
  5. Minor. A request without start/end is aligned on Loki's derived bounds (end=now, start=end-since) via a shared lokiRangeBounds, also used by the resolution check; TestAlignRangeRequestToStepGrid_DerivesLokiDefaultBounds covers it. setForm writes the URL unconditionally. A Parse failure cannot reach the alignment because validateQuery has already run (noted in the comment). The no-step and /patterns non-alignments are in docs/compatibility-loki.md under "What Stays Outside Loki Parity", and the two docs describing binary alignment now say it applies whatever the flag says.
  6. docs/reference/configuration-reference.md regenerated with go run ./cmd/configdoc (the flag was missing there, which the new generated-docs gate catches).

Attribution removed from the commit and the body. Local: gofmt, build, vet (incl. -tags=e2e), golangci-lint (0 issues), changelog, conformance and generated-docs gates, go test ./... -count=1 green.

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.
@clain23

clain23 commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

e2e-ui (explore-regression) failed on the previous head in raw per-stream series honour the documented proxy cap (proxy 46 series, Loki index 62, then 67 on retry). That is this PR working as intended, and the test's reference not following it:

  • The stack's proxy now runs with -align-queries-with-step, so rate({app="api-gateway"}[5m]) at step=60 is evaluated on the window truncated down to the 60s grid, as the stack's Loki does.
  • The reference is Loki's /series over the unaligned [end-15m, end]. The generator creates about 2 streams/s with rotating pod names, so streams created in the last partial step are listed by the index but no longer covered by the aligned query. The reference grew between the two attempts while the proxy's answer stayed at 46, which fits.

Fix: the test aligns end down to the step before querying both sides, with a comment on why. No proxy code changed.

Also rebased onto current main (#606): the compose variants keep your new -max-stats-query-series=1000000 next to -align-queries-with-step=true, the CHANGELOG entries sit under [Unreleased] with the max_query_length and resolution-check bullets moved under ### Fixed, and conformance/registry/generated/wiring.json is regenerated. Local: build, vet (incl. -tags=e2e), golangci-lint (0 issues), configdoc, changelog and conformance gates ok, go test ./... green apart from TestOptimization_ConnectionPool_HighConcurrency, which fails the same way on clean main on macOS (kern.ipc.somaxconn=128 against 200 concurrent connections) and passed in CI on the previous head.

@clain23
clain23 force-pushed the fix/align-range-to-step-grid branch from 727a04c to d7521cc Compare September 23, 2026 09:42
@szibis

szibis commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

Thanks @clain23 for this work. Aligning range queries to the step grid, as Loki's align_queries_with_step does, turned out to be the key to caching Grafana's auto-refresh: a refresh that moves the window by less than one step currently lands on a new bucket grid and recomputes the whole range, while an aligned query reuses its cached buckets and folded results.

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.

@szibis szibis closed this Oct 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants