LogQL: the derived step is whole seconds, matching the reference (#425 Part B) - #439
Merged
Conversation
…es it (#425 Part B) A `query_range` that omits `step` derived `span_ns / 250` NANOSECONDS, which pinned the answer at 251 points on every window and gave arbitrary spacing. The reference derives a whole number of SECONDS — `int(math.Max(math.Floor(end.Sub(start).Seconds()/250), 1))`, multiplied by `time.Second` (`pkg/loghttp/params.go:140-142`, reached from `step()` at `:122-126` @ grafana/loki v3.7.4 `b318f282`). Measured on the pinned image: a 499 s window is 500 points at 1 s there against our 251 at 1.996 s; a 1 h window derives 14 s (the reference's own `params_test.go:34-38` expectation) against our 14.4 s. `derive_step_ns` now computes Go's expression rather than an approximation of it. Go's `Duration.Seconds()` is `float64(sec) + float64(nsec)/1e9`, so past ~2^52 ns of span the sum rounds up and the float floors one second above integer division — a widening interval just below each multiple of 250e9, first non-empty at k = 67_109 and 953 ns wide at k = 36_893_488. Measured on the pinned image with `max_query_length: 0s`, with controls either side. `/patterns` derives its step through the same shared helper at the reference (`patterns.go:20`), so it moves too, and it moves TOWARD the reference: at k = 67_110 we served 67_100 s and the reference answers 67_110 s. It moves for 3_682_638 values of k, all ≡ 0 (mod 10). No SQL, no index, no route and no round-trip changes: `metric_plan` forces the client path for every range query, so the rollup arm the step could select is unreachable, and `metric_raw_samples_sliding` takes no step. `explain_indexes` passes with zero edits. Part A closes as an accepted divergence per the owner ruling of 2026-08-12 (#301 stands): our range grid stays anchored on the request `start`, so no point escapes `[start, end]`. Ledger row `range-step-grid-start-anchored`, a matching `docs/api.md` paragraph, and `deploy/e2e/loki.yaml`'s `split_queries_by_interval: 0` comment rewritten to cite the ruling rather than read as a pending workaround. Claude-Session: https://claude.ai/code/session_01B1xu1frZL9oF8MYRhkiMhv
`docs/api.md` §2.1 said the count "varies between 250 and 500". The upper half is right and exhaustively gated (`derived_step_never_trips_the_11000_ point_fence`, maximum 500 at k = 1); the lower half is false. Any window under 250 s derives `1s`, so a 10 s window is 11 points and a zero-length one is a single point. Says "at most 500" and gives the small-window shape instead of a bound it cannot support. Sweep for the same claim elsewhere: `git grep -n "250 and 500\|between 250\|varies between"` and `git grep -n "point count" -- docs/` find no other instance; §2.6.4 defers to §2.1 for the rule and states no count, the ledger row quotes only measured per-window numbers, and `derive_step_ns`'s doc comment states the old fixed 251 and the measured 499 s case, neither a range. Claude-Session: https://claude.ai/code/session_01B1xu1frZL9oF8MYRhkiMhv
`live_port_uniqueness` reddened on the PR head after #277 merged (`ce6e54c`): `logs_variants_warnings_live.rs` and `logs_api_live.rs` each took 31200/31201, each branch picking the next free pair without the other being visible. A collision between two branches, not a defect in either; #277 is merged, so these move. The values come from the guard's own free list printed by the failure (`31_204, 31_205, 31_206, …`), re-read after the edit rather than assumed. Neither number is spelled anywhere else: `git grep -nE "31_?20[45]|31204|31205" -- crates/ e2e/ xtask/ docs/ .github/` returns exactly the two `let port =` lines, so no URL literal, hex form or range-claiming comment needs to follow. Claude-Session: https://claude.ai/code/session_01B1xu1frZL9oF8MYRhkiMhv
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
1 participant
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.
Part B of #425. Part A does not proceed (#301 stands — see the ruling on the issue).
When a caller omits
step, we derivedspan_ns / 250nanoseconds, which pinned every response at 251 points and spaced them at arbitrary fractions of a second. The rule is whole seconds:max(floor((end-start) in seconds / 250), 1).Measured on
grafana/loki:3.7.4and now matching point for point:Go computes this in floating point rather than integer division, and the two disagree for spans above ~194 days — first at
k = 67_109, widening with thef64ULP. That was reproduced against a limits-lifted reference container (span16_777_249_999_999_999ns derives 67,109 s there, where integer division gives 67,108) and the float form is implemented.Accepted divergence: a 501 s window returns one point past the requested
endat the reference; we stop atend. Recorded indocs/api.mdand the differential ledger.Reviewer
VERDICT: PASSon7b7a52b0: #425 (comment)Plan review reached PASS at round 5 after three separate figures were computed correctly and written up wrongly; the shipped test now asserts the full 31-row distribution, so the numbers are checked rather than believed.
https://claude.ai/code/session_01B1xu1frZL9oF8MYRhkiMhv