From 2a77054035513eabc884135d3a8c19fc76e0956b Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Sun, 18 Oct 2026 19:13:30 +0100 Subject: [PATCH] Fix filtered chart reads and verify the idle-worker SLO Stream ungrouped retained reads in the existing time-major index order, avoiding temporary metric sorts without changing tier reconciliation or chart samples. Extend query-plan and filtered-result coverage. Add an explicit strict mode for the existing 90 ms API budget and an optional fresh-worker CI proof with fixed executable identities and raw capacity evidence. Keep ordinary hosted budgets and required gates intact. Contract-Neutral: Chart payloads, retained-tier reconciliation, sampling, index layout and source/release gates are unchanged; the implementation removes an unnecessary sort and adds controlled verification of existing latency budgets. --- .github/workflows/build-and-test.yml | 80 +++++++++ .../workload-chart-latency-2026-10-18.md | 67 ++++++++ .../subsystems/performance-and-scalability.md | 19 ++- internal/api/slo_bench_test.go | 20 ++- internal/api/slo_budget_test.go | 36 +++++ internal/api/testdata/chart_latency_proof.py | 152 ++++++++++++++++++ .../api/testdata/test_chart_latency_proof.py | 39 +++++ pkg/metrics/store.go | 6 +- pkg/metrics/store_query_plan_test.go | 18 ++- pkg/metrics/store_tier_coverage_test.go | 7 + 10 files changed, 435 insertions(+), 9 deletions(-) create mode 100644 docs/release-control/v6/internal/records/workload-chart-latency-2026-10-18.md create mode 100644 internal/api/slo_budget_test.go create mode 100644 internal/api/testdata/chart_latency_proof.py create mode 100644 internal/api/testdata/test_chart_latency_proof.py diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 7e3eeef8f7..0707265940 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -758,6 +758,86 @@ jobs: - name: Build Pulse backend run: go build ./cmd/pulse + chart-latency-slo: + name: Workload chart 90ms SLO + needs: changes + if: github.event_name == 'pull_request' && needs.changes.outputs.go_compile == 'true' + runs-on: ubuntu-24.04 + timeout-minutes: 25 + env: + GOMAXPROCS: "2" + steps: + - name: Checkout repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Checkout pull request base + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + ref: ${{ github.event.pull_request.base.sha }} + path: chart-baseline + + - name: Set up Go + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: go.mod + cache: true + + - name: Check idle proof guards + run: python3 -m unittest discover -s internal/api/testdata -p test_chart_latency_proof.py -v + + - name: Compile fixed baseline and candidate executables + run: | + set -euo pipefail + proof_dir="$RUNNER_TEMP/chart-latency" + mkdir -p "$proof_dir" + for source in chart-baseline .; do + mkdir -p "$source/internal/api/frontend-modern/dist" + [ -f "$source/internal/api/frontend-modern/dist/index.html" ] || \ + printf 'ci embed stub\n' \ + > "$source/internal/api/frontend-modern/dist/index.html" + done + { + date -u +%FT%TZ + go version + go env GOOS GOARCH + lscpu + printf 'GOMAXPROCS=%s\nGITHUB_RUN_ID=%s\nGITHUB_RUN_ATTEMPT=%s\n' \ + "$GOMAXPROCS" "$GITHUB_RUN_ID" "$GITHUB_RUN_ATTEMPT" + printf 'baseline HEAD/tree:\n' + git -C chart-baseline rev-parse HEAD HEAD^{tree} + printf 'candidate HEAD/tree:\n' + git rev-parse HEAD HEAD^{tree} + printf 'candidate source changes:\n' + git diff --exit-code + } > "$proof_dir/metadata.txt" + (cd chart-baseline && go test -p 1 ./internal/api -run '^$' -c -o "$proof_dir/baseline-api.test") + go test -p 1 ./internal/api -run '^$' -c -o "$proof_dir/candidate-api.test" + sha256sum "$proof_dir/"*.test >> "$proof_dir/metadata.txt" + + # This additional evidence job keeps the ordinary hosted envelope and + # required source/release gates intact. A fresh VM must first demonstrate + # idle capacity; the candidate then enforces the existing local 90 ms SLO. + - name: Verify chart latency on observed idle capacity + run: | + python3 internal/api/testdata/chart_latency_proof.py \ + --baseline "$RUNNER_TEMP/chart-latency/baseline-api.test" \ + --candidate "$RUNNER_TEMP/chart-latency/candidate-api.test" \ + --output "$RUNNER_TEMP/chart-latency/samples" + + - name: Upload chart latency evidence + if: always() + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: chart-latency + path: | + ${{ runner.temp }}/chart-latency/metadata.txt + ${{ runner.temp }}/chart-latency/samples/ + if-no-files-found: warn + retention-days: 7 + benchmarks: name: Benchmarks needs: changes diff --git a/docs/release-control/v6/internal/records/workload-chart-latency-2026-10-18.md b/docs/release-control/v6/internal/records/workload-chart-latency-2026-10-18.md new file mode 100644 index 0000000000..1f30fcb715 --- /dev/null +++ b/docs/release-control/v6/internal/records/workload-chart-latency-2026-10-18.md @@ -0,0 +1,67 @@ +# Workload chart latency investigation, 18 October 2026 + +Scope: the local 90 ms workload-chart SLO within L10's existing +`performance-post-rc-headroom` follow-up. This work does not close L10 or +change source/release qualification gates. + +## Defect and correction + +The retained metrics identity index is time-major. Ungrouped multi-metric +reads nevertheless requested metric-major output, forcing SQLite temporary +sorts on the fresh-service workload-chart path. Ungrouped reads now use the +index order for both filtered and unfiltered queries. The result assembler +already supports interleaved metric series; streaming display buckets keep +their required grouping. No index, retention rule, chart sampling algorithm, +payload shape or response-cache boundary changes. + +The expanded query-plan guard fails against the original store with +`USE TEMP B-TREE FOR ORDER BY` and passes with the correction. Tier-coverage +tests compare multi-metric filtered reads (including duplicate requested +metrics) against unfiltered reconciliation for raw and display-bucket reads. +The full metrics package and seven SLO-budget selection cases pass. + +## Diagnostic evidence + +Baseline source: `52b40fd611112904cf7e987e0fc1e8df1c7a0cbf`, tree +`b1e58aeecb1c83ab2d17223d55bfd8353df3dbae`. Both diagnostic binaries use +Go 1.26.9 linux/amd64 and GOMAXPROCS=2 on pulse-dev (Ryzen 5 3600). + +| Fixed executable | SHA-256 | 4 h cold-service diagnostic | +| --- | --- | --- | +| Baseline | `f9afc621e05d69d7c13769fa4e162badab9f90c679909184417602b1ef2ca47d` | 167.43 ms/op, 38 iterations | +| Sort correction | `f8318a86724b775e46cbd186944f17ce49e01711f9b9029499bbc9cee0be28f8` | 188.39 ms/op, 33 iterations | + +These separate five-second CPU/allocation profiles ran on a shared, busy +worker. They are attribution evidence, not a matched speed comparison or +an idle-worker SLO result. The original profile shows SQLite sorter work; +the corrected profile leaves row reads as the dominant cost. Neither timing +establishes a speedup or satisfies 90 ms. + +The native idle sampler required 30 continuous seconds with load1 below 2, +at least 90% idle CPU, at most 1% I/O wait, 0.1% steal and 2% guest CPU, +and no swap activity. An initial wait was stopped before any SLO measurement; +a later bounded five-minute wait expired without qualifying. Both retained +their raw capacity evidence and produced zero SLO samples. No other workload +or service was stopped to obtain an idle result. + +Native raw evidence is retained under +`/opt/pulse-release-worker/task-proofs/chart-latency-slo-20261018/`. + +## Absolute SLO verification + +The optional `Workload chart 90ms SLO` job in Build and Test is the repeatable +proof surface. A fresh hosted VM compiles base and candidate API test +executables before timing, records their committed HEAD/tree and binary +hashes, and observes idle capacity before each process. CPU, guest, swap and +pressure samples remain in the `chart-latency` artifact alongside raw output +and post-measurement samples. The candidate must execute exactly one fresh +200-sample workload-chart test and report the existing 90 ms budget; skipped +tests and ordinary 550 ms hosted passes cannot satisfy it. Actual hosted +runner identity remains intact. Background contention invalidates the proof. + +The associated source-check run and retained artifact supply the measured +percentiles for that exact candidate. This record does not substitute a +diagnostic benchmark mean for those percentiles. The earlier historical +12.72% comparison remains preserved; changed harness boundaries cannot +retroactively resolve it. Broader performance headroom remains governed by +the existing L10 follow-up. diff --git a/docs/release-control/v6/internal/subsystems/performance-and-scalability.md b/docs/release-control/v6/internal/subsystems/performance-and-scalability.md index cd1c8333a9..80a587dce0 100644 --- a/docs/release-control/v6/internal/subsystems/performance-and-scalability.md +++ b/docs/release-control/v6/internal/subsystems/performance-and-scalability.md @@ -1869,6 +1869,16 @@ dashboard-specific overview, trend, or summary transport. `TestSLO_WorkloadCharts` must similarly use a fresh service per timed sample so its existing query-latency budgets cannot be satisfied solely by a response-cache hit after the sanity request. + Controlled idle-worker proofs set `PULSE_STRICT_LOCAL_API_SLOS=true` to + enforce the existing 90 ms local budget, including on hosted runners, + without changing their actual runner identity or accepting a skipped + test. Ordinary hosted execution retains its 550 ms envelope. The optional + Build and Test chart-latency job compiles fixed base/candidate executables + before measuring, requires observed idle capacity, and retains executable + hashes, raw percentiles, and CPU/guest/swap evidence before, during and + after measurement. Its base timing is context under the ordinary envelope; + only the candidate's explicit 90 ms report establishes the local SLO. + This evidence does not replace required source checks or release gates. 31. Keep the retired dashboard overview route absent from the protected hot path. `frontend-modern/src/pages/Dashboard.tsx`, `frontend-modern/src/hooks/useDashboardOverview.ts`, and @@ -2683,7 +2693,14 @@ WAL and checkpoint writes per ~60-byte sample before the change (#1966). The accepted cost is on metric-filtered reads, which now scan a resource's other metrics in the window: a single-metric 500-node query moved from 45 to 59 microseconds and the 24-hour 50-disk `smart_temp` batch from about 0.25 to 0.49 -s, while all-metric dashboard reads are unchanged. Legacy databases keep their +s, while all-metric dashboard reads are unchanged. Ungrouped retained reads, +including multi-metric filters, must consume that time-major index order +without a temporary metric sort. Interleaved metric rows must preserve every +series' timestamp order, tier precedence, values and extrema. Streaming +display-bucket aggregation retains its required series grouping. The query +plan guard covers filtered and unfiltered reads across retained windows and +single/fleet scopes; tier-coverage tests compare filtered results with the +unfiltered reconciliation contract. Legacy databases keep their existing unique index authoritative while the replacement index is built, but that O(rows) migration must run on deferred startup maintenance and swap indexes in one SQLite transaction so constructor latency, concurrent reads, uniqueness, diff --git a/internal/api/slo_bench_test.go b/internal/api/slo_bench_test.go index b6e2f332fc..537d421e50 100644 --- a/internal/api/slo_bench_test.go +++ b/internal/api/slo_bench_test.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "os" "path/filepath" "reflect" "sort" @@ -653,12 +654,23 @@ const ( ) func effectiveAPISLOTarget(localTarget, githubActionsTarget time.Duration) time.Duration { - if githubActionsTarget > 0 && latencyBudgetEnforced() { + if githubActionsTarget > 0 && latencyBudgetEnforced() && !strictLocalAPISLOs() { return githubActionsTarget } return localTarget } +// Controlled idle-worker proofs can enforce the local budgets without changing +// runner identity or falling back to the hosted envelopes. Ordinary shared +// worker runs retain their existing contention policy. +func strictLocalAPISLOs() bool { + return os.Getenv("PULSE_STRICT_LOCAL_API_SLOS") == "true" +} + +func apiLatencyBudgetEnforced() bool { + return strictLocalAPISLOs() || latencyBudgetEnforced() +} + // measureEndpointLatencies runs fn sloIterations times with a warmup phase and // returns the measured latency durations. func measureEndpointLatencies(t *testing.T, fn func()) []time.Duration { @@ -727,7 +739,9 @@ func percentile(durations []time.Duration, pct float64) time.Duration { // up to ~4x on the median during vite builds), so a local overrun cannot be // attributed to a code regression and the test skips with the full // distribution, the same way skipUnderRace treats race-detector overhead. -// A local pass still means the budget was genuinely met. +// A local pass still means the budget was genuinely met. An explicitly +// controlled proof sets PULSE_STRICT_LOCAL_API_SLOS=true to enforce the local +// budget, including on Actions, and must retain its idle-worker evidence. func assertLatencySLO(t *testing.T, label string, latencies []time.Duration, target time.Duration) { t.Helper() p50 := percentile(latencies, 0.50) @@ -736,7 +750,7 @@ func assertLatencySLO(t *testing.T, label string, latencies []time.Duration, tar if p95 <= target { return } - if latencyBudgetEnforced() { + if apiLatencyBudgetEnforced() { t.Errorf("SLO VIOLATION: p95=%v exceeds target %v", p95, target) return } diff --git a/internal/api/slo_budget_test.go b/internal/api/slo_budget_test.go new file mode 100644 index 0000000000..3af8e56efd --- /dev/null +++ b/internal/api/slo_budget_test.go @@ -0,0 +1,36 @@ +package api + +import ( + "testing" + "time" +) + +func TestAPISLOBudgetSelection(t *testing.T) { + const local = 90 * time.Millisecond + const hosted = 550 * time.Millisecond + for _, tc := range []struct { + name, actions, runID, strict string + target time.Duration + enforced bool + }{ + {"local", "", "", "", local, false}, + {"shared-worker", "true", "", "", local, false}, + {"hosted", "true", "123", "", hosted, true}, + {"strict-local", "", "", "true", local, true}, + {"strict-shared-worker", "true", "", "true", local, true}, + {"strict-hosted", "true", "123", "true", local, true}, + {"disabled-hosted", "true", "123", "false", hosted, true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("GITHUB_ACTIONS", tc.actions) + t.Setenv("GITHUB_RUN_ID", tc.runID) + t.Setenv("PULSE_STRICT_LOCAL_API_SLOS", tc.strict) + if got := effectiveAPISLOTarget(local, hosted); got != tc.target { + t.Fatalf("target=%v, want %v", got, tc.target) + } + if got := apiLatencyBudgetEnforced(); got != tc.enforced { + t.Fatalf("enforced=%v, want %v", got, tc.enforced) + } + }) + } +} diff --git a/internal/api/testdata/chart_latency_proof.py b/internal/api/testdata/chart_latency_proof.py new file mode 100644 index 0000000000..7741f2811a --- /dev/null +++ b/internal/api/testdata/chart_latency_proof.py @@ -0,0 +1,152 @@ +"""Run the workload chart SLO on observed idle capacity and retain raw evidence.""" + +import argparse +import hashlib +import json +import os +from pathlib import Path +import re +import subprocess +import time + + +def snapshot(): + cpu = list(map(int, Path('/proc/stat').read_text().splitlines()[0].split()[1:])) + swaps = {} + for line in Path('/proc/vmstat').read_text().splitlines(): + key, value = line.split() + if key in ('pswpin', 'pswpout'): + swaps[key] = int(value) + pressure = {} + for kind in ('cpu', 'memory', 'io'): + path = Path('/proc/pressure') / kind + if path.exists(): + pressure[kind] = path.read_text().strip() + return dict(time=time.time(), cpu=cpu, swaps=swaps, load=os.getloadavg(), pressure=pressure) + + +def interval(before, after): + delta = [b - a for a, b in zip(before['cpu'], after['cpu'])] + total = sum(delta[:8]) # Guest time is already included in user/nice time. + scale = 100 / total if total > 0 else 0 + return dict(time=after['time'], seconds=after['time'] - before['time'], + total_ticks=total, idle_pct=delta[3] * scale, iowait_pct=delta[4] * scale, + steal_pct=delta[7] * scale, guest_pct=sum(delta[8:10]) * scale, + busy_pct=(total - delta[3] - delta[4] - delta[7]) * scale, + swap_pages=sum(after['swaps'][k] - before['swaps'][k] for k in after['swaps']), + load=after['load'], pressure=after['pressure']) + + +def quiet(row): + return (row['total_ticks'] > 0 and row['load'][0] < 2 and row['idle_pct'] >= 90 + and row['iowait_pct'] <= 1 and row['steal_pct'] <= 0.1 + and row['guest_pct'] <= 2 and row['swap_pages'] == 0) + + +def wait_idle(prefix): + deadline = time.monotonic() + 300 + streak = 0 + before = snapshot() + with prefix.with_suffix('.idle.jsonl').open('w') as receipt: + while streak < 30: + time.sleep(5) + after = snapshot() + row = interval(before, after) + streak = streak + row['seconds'] if quiet(row) else 0 + row['quiet_streak_seconds'] = streak + receipt.write(json.dumps(row) + '\n') + receipt.flush() + before = after + if time.monotonic() > deadline: + raise RuntimeError('No qualifying idle window. No measurement executed.') + + +def ticks(pid): + try: + fields = Path(f'/proc/{pid}/stat').read_text().rsplit(')', 1)[1].split() + return int(fields[11]) + int(fields[12]) + except (FileNotFoundError, ProcessLookupError): + return None + + +def digest(path): + with path.open('rb') as source: + return hashlib.file_digest(source, 'sha256').hexdigest() + + +def validate_output(text, returncode, strict): + if returncode != 0: + raise RuntimeError(f'SLO process exited {returncode}') + passes = re.findall(r'^--- PASS: TestSLO_WorkloadCharts \(', text, re.MULTILINE) + if len(passes) != 1 or re.search(r'^--- SKIP: TestSLO_WorkloadCharts ', text, re.MULTILINE): + raise RuntimeError('SLO did not pass, including a skipped or missing test') + reports = re.findall(r'charts/workloads p50=\S+ p95=\S+ p99=\S+ SLO=(\S+)', text) + if len(reports) != 1 or (strict and reports[0] != '90ms'): + raise RuntimeError(f'Expected one explicit SLO report at the selected budget, got {reports}') + + +def measure(binary, prefix, strict): + identity = digest(binary) + wait_idle(prefix) + # Preserve the real Actions identity, without forwarding runner credentials. + env = {key: os.environ[key] for key in ( + 'PATH', 'HOME', 'TMPDIR', 'TZ', 'GOMAXPROCS', 'CI', + 'GITHUB_ACTIONS', 'GITHUB_RUN_ID', 'GITHUB_RUN_ATTEMPT') if key in os.environ} + if strict: + env['PULSE_STRICT_LOCAL_API_SLOS'] = 'true' + command = [str(binary), '-test.run=^TestSLO_WorkloadCharts$', '-test.v', + '-test.count=1', '-test.cpu=2', '-test.timeout=5m'] + capacity_errors = [] + with prefix.with_suffix('.log').open('w') as log, prefix.with_suffix('.cpu.jsonl').open('w') as receipt: + before = snapshot() + process = subprocess.Popen(command, env=env, stdout=log, stderr=subprocess.STDOUT) + own_before = 0 + while process.poll() is None: + time.sleep(1) + after = snapshot() + own_after = ticks(process.pid) + row = interval(before, after) + if own_before is not None and own_after is not None and row['total_ticks'] > 0: + row['measurement_cpu_pct'] = 100 * (own_after - own_before) / row['total_ticks'] + row['background_busy_pct'] = max(0, row['busy_pct'] - row['measurement_cpu_pct']) + if (row['background_busy_pct'] > 10 or row['guest_pct'] > 2 + or row['steal_pct'] > 0.1 or row['iowait_pct'] > 1 or row['swap_pages']): + capacity_errors.append(row) + receipt.write(json.dumps(row) + '\n') + receipt.flush() + before, own_before = after, own_after + returncode = process.wait() + with prefix.with_suffix('.after.jsonl').open('w') as receipt: + before = snapshot() + for _ in range(3): + time.sleep(5) + after = snapshot() + receipt.write(json.dumps(interval(before, after)) + '\n') + before = after + result = dict(command=command, binary_sha256=identity, environment=env, + pid=process.pid, returncode=returncode, capacity_errors=capacity_errors) + prefix.with_suffix('.result.json').write_text(json.dumps(result, indent=2) + '\n') + text = prefix.with_suffix('.log').read_text() + print(text, flush=True) + if digest(binary) != identity: + raise RuntimeError('Measured executable changed') + validate_output(text, returncode, strict) + if capacity_errors: + raise RuntimeError('Background CPU or guest/steal activity invalidated the idle measurement') + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument('--baseline', type=Path, required=True) + parser.add_argument('--candidate', type=Path, required=True) + parser.add_argument('--output', type=Path, required=True) + args = parser.parse_args() + args.output.mkdir(parents=True, exist_ok=False) + # Baseline uses its ordinary hosted envelope only as timing context. The + # candidate must pass the existing local 90 ms budget without any skip. + measure(args.baseline.resolve(), args.output / 'baseline', strict=False) + measure(args.candidate.resolve(), args.output / 'candidate', strict=True) + + +if __name__ == '__main__': + main() diff --git a/internal/api/testdata/test_chart_latency_proof.py b/internal/api/testdata/test_chart_latency_proof.py new file mode 100644 index 0000000000..677e82c56e --- /dev/null +++ b/internal/api/testdata/test_chart_latency_proof.py @@ -0,0 +1,39 @@ +import unittest + +import chart_latency_proof as proof + + +class ChartLatencyProofTests(unittest.TestCase): + def test_guest_time_is_not_counted_twice(self): + before = dict(time=0, cpu=[0] * 10, swaps={'pswpin': 0, 'pswpout': 0}) + after = dict(time=1, cpu=[20, 0, 0, 80, 0, 0, 0, 0, 10, 0], + swaps=before['swaps'], load=[0.5, 0.5, 0.5], pressure={}) + row = proof.interval(before, after) + self.assertEqual(row['total_ticks'], 100) + self.assertEqual(row['busy_pct'], 20) + self.assertEqual(row['guest_pct'], 10) + + def test_idle_requires_cpu_io_and_swap_quiet(self): + row = dict(total_ticks=100, load=[1, 1, 1], idle_pct=95, iowait_pct=0, + steal_pct=0, guest_pct=0, swap_pages=0) + self.assertTrue(proof.quiet(row)) + for field, value in [('total_ticks', 0), ('load', [2, 1, 1]), ('idle_pct', 89), + ('iowait_pct', 2), ('steal_pct', 1), ('guest_pct', 3), ('swap_pages', 1)]: + with self.subTest(field=field): + self.assertFalse(proof.quiet(dict(row, **{field: value}))) + + def test_skip_or_hosted_pass_cannot_satisfy_local_slo(self): + report = 'charts/workloads p50=40ms p95=50ms p99=60ms SLO=90ms\n' + passed = report + '--- PASS: TestSLO_WorkloadCharts (1.0s)\n' + proof.validate_output(passed, 0, strict=True) + for text, status in [(report + '--- SKIP: TestSLO_WorkloadCharts (1.0s)\n', 0), + (passed.replace('90ms', '550ms'), 0), (passed, 1), ('PASS\n', 0), + (passed + report, 0), (passed + passed, 0), + (passed + '--- SKIP: TestSLO_WorkloadCharts (1.0s)\n', 0)]: + with self.subTest(text=text, status=status): + with self.assertRaises(RuntimeError): + proof.validate_output(text, status, strict=True) + + +if __name__ == '__main__': + unittest.main() diff --git a/pkg/metrics/store.go b/pkg/metrics/store.go index fdfeb29fbc..77e00a927f 100644 --- a/pkg/metrics/store.go +++ b/pkg/metrics/store.go @@ -1696,7 +1696,11 @@ func retainedQuerySQL(resourceType string, resourceIDs, metricTypes []string, st FROM (` + query + ") GROUP BY " + identityColumns + "bucket_ts ORDER BY " + identityColumns + "bucket_ts ASC" } else { orderColumns := identityColumns - if !groupSeries && len(metricTypes) == 0 { + if !groupSeries { + // The identity index is time-major within each resource. Metric + // filters do not require metric-major output: the result assembler + // handles interleaved series while preserving each series' time order. + // Only streamed display buckets need consecutive metric groups. orderColumns = "" if len(resourceIDs) != 1 { orderColumns = "resource_id, " diff --git a/pkg/metrics/store_query_plan_test.go b/pkg/metrics/store_query_plan_test.go index 4df466fe9c..4fd216b0a1 100644 --- a/pkg/metrics/store_query_plan_test.go +++ b/pkg/metrics/store_query_plan_test.go @@ -537,11 +537,21 @@ func explainQueryPlan(t *testing.T, db *sql.DB, query string, args []any) string func TestRetainedUnaggregatedQueryAvoidsMetricSort(t *testing.T) { db := newPlanTestDB(t) + store := &Store{} end := time.Unix(2000000040, 0) - query, args := retainedQuerySQL("node", []string{"a", "b"}, nil, end.Add(-time.Hour), end, 0, []Tier{TierRaw, TierMinute, TierHourly}, false) - for _, line := range strings.Split(explainQueryPlan(t, db, query, args), "\n") { - if strings.Contains(strings.ToUpper(line), "TEMP B-TREE") { - t.Fatalf("unaggregated all-metric read sorts despite time-ordered index: %s", line) + for _, window := range []time.Duration{time.Hour, 24 * time.Hour, 7 * 24 * time.Hour, 30 * 24 * time.Hour} { + for _, ids := range [][]string{{"a"}, {"a", "b"}} { + for _, metrics := range [][]string{nil, {"cpu"}, {"cpu", "memory"}} { + t.Run(fmt.Sprintf("%s/resources_%d/metrics_%d", window, len(ids), len(metrics)), func(t *testing.T) { + query, args := retainedQuerySQL("node", ids, metrics, end.Add(-window), end, 0, store.tierFallbacks(window), false) + plan := explainQueryPlan(t, db, query, args) + for _, line := range strings.Split(plan, "\n") { + if strings.Contains(strings.ToUpper(line), "TEMP B-TREE") { + t.Fatalf("unaggregated read sorts despite time-ordered index: %s", plan) + } + } + }) + } } } } diff --git a/pkg/metrics/store_tier_coverage_test.go b/pkg/metrics/store_tier_coverage_test.go index 92df758e9e..fb6e688a10 100644 --- a/pkg/metrics/store_tier_coverage_test.go +++ b/pkg/metrics/store_tier_coverage_test.go @@ -56,6 +56,10 @@ func TestStoreRetainedTierCoverage(t *testing.T) { if err != nil { t.Fatal(err) } + filteredMany, err := store.QueryMetricTypesBatch("node", []string{"a", "b"}, []string{"memory", "cpu", "cpu"}, start, end, step) + if err != nil { + t.Fatal(err) + } for _, id := range []string{"a", "b"} { all, err := store.QueryAll("node", id, start, end, step) if err != nil { @@ -64,6 +68,9 @@ func TestStoreRetainedTierCoverage(t *testing.T) { if !reflect.DeepEqual(all, batch[id]) { t.Fatalf("batch differs for %s: %v != %v", id, batch[id], all) } + if !reflect.DeepEqual(filteredMany[id], all) { + t.Fatalf("multi-metric filter changed ordering or retained values for %s: %v != %v", id, filteredMany[id], all) + } for _, metric := range []string{"cpu", "memory"} { single, err := store.Query("node", id, metric, start, end, step) if err != nil {