Skip to content

Commit a5bc412

Browse files
committed
Merge branch 'fix/344-d02-query-info-lookback' into 'main'
fix(dashboards): carry pgwatch_query_info forward so pgss legends resolve Closes #344 See merge request postgres-ai/postgresai!405
2 parents 35fb765 + a00beb9 commit a5bc412

7 files changed

Lines changed: 643 additions & 28 deletions

File tree

config/grafana/dashboards/Dashboard_2_Aggregated_query_analysis.json

Lines changed: 23 additions & 23 deletions
Large diffs are not rendered by default.

config/grafana/dashboards/README.md

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,50 @@ ignores the sort.** Use the reducer display names:
133133
| `last` | `Last` |
134134
| `lastNotNull` | `Last *` (note the asterisk) |
135135

136+
## Joining a sparsely-emitted metric
137+
138+
Some metrics are not emitted on every scrape. `pgwatch_query_info` — the
139+
queryid-to-query-text mapping the pg_stat_statements legends join against — is
140+
exported only for queryids active in the last `QUERYID_ACTIVE_MINUTES`, so a
141+
queryid that goes quiet develops gaps of hours. Joined as a bare instant vector
142+
it silently misses its ~5 min lookback, and the legend degrades to the raw label
143+
set (`{cluster="…", datname="…", queryid="…"}`) instead of the query text.
144+
145+
Join such a metric through this operand, on **both** branches of the
146+
`or … unless` pair:
147+
148+
```promql
149+
(topk by (queryid) (1, tlast_over_time(pgwatch_query_info[7d])) > bool 0)
150+
```
151+
152+
Each part earns its place:
153+
154+
| Part | Why |
155+
|------|-----|
156+
| `tlast_over_time(…[7d])` | Carries the last known mapping forward, so a stale queryid still resolves. Use the same window on both branches: widening one only would let a queryid match both, and `or` would emit it twice. |
157+
| `topk by (queryid) (1, …)` | One queryid can hold several `displayname*` series inside a window that wide — the exporter's text pick is not stable across redeploys or sources. Without it the joined series is duplicated and the stacked total inflated. `tlast_over_time` returns the sample *time*, so the newest mapping wins; on an exact timestamp tie the pick is arbitrary but there is still exactly one. |
158+
| `> bool 0` | Restores the value to `1`. The operand is multiplied into the metric being ranked, and without this the plotted rate would be scaled by a unix timestamp. |
159+
160+
`group_left(...)` must copy every label the panel's `legendFormat` renders. An
161+
operand that is otherwise perfect still produces the raw-label failure if the
162+
labels never cross the join, and when the legend is driven by a template
163+
variable (`{{$legend_label}}`) that means *every* value the variable can take.
164+
165+
`tlast_over_time` is a MetricsQL extension, not PromQL — these dashboards ship
166+
against VictoriaMetrics in both compose and Helm, alongside other MetricsQL
167+
already in use here (`default 0`). Against a strict Prometheus there is no
168+
equivalent: `last_over_time` carries the mapping forward but cannot de-duplicate
169+
it, and the join then fails outright with "duplicate series for the match group".
170+
171+
The window is a judgement call — it must comfortably exceed the observed
172+
per-series staleness, and it bounds how long a superseded query text can linger.
173+
The mechanical parts are enforced on every MR by two tests sharing one
174+
definition in `tests/grafana_dashboards/query_info_join.py`:
175+
`tests/grafana_dashboards/test_query_info_carry_forward.py` (the operand, the
176+
lookback floor, and the `group_left` label transfer, across every dashboard) and
177+
`tests/compliance_vectors/test_mr219_monitoring_guards.py` (the `or … unless`
178+
branch shape on Dashboard 02). They run in different CI jobs.
179+
136180
## Units
137181

138182
- **binBps**: Use binary bytes per second (KiB/s, MiB/s, GiB/s) for Postgres

tests/compliance_vectors/test_mr219_monitoring_guards.py

Lines changed: 53 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,10 @@
55

66
import yaml
77

8+
from tests.grafana_dashboards.query_info_join import (
9+
QUERY_INFO_JOIN_OPERAND,
10+
join_operand_problems,
11+
)
812

913
PROJECT_ROOT = Path(__file__).resolve().parents[2]
1014

@@ -312,14 +316,27 @@ def test_queryid_dedup_trigger_is_partition_safe():
312316

313317

314318
def test_dashboard_2_pgss_query_info_expressions_have_or_fallbacks():
319+
# pgwatch_query_info arrives in sparse bursts, so an instant-vector join
320+
# misses its ~5 min lookback and the legend degrades to raw labels (#344).
321+
# Both branches need the same carry-forward operand: widening one alone
322+
# makes `or` emit two series per queryid. This test owns the branch shape;
323+
# the operand itself is shared with tests/grafana_dashboards/.
315324
dashboard_paths = [
316325
PROJECT_ROOT / "config/grafana/dashboards/Dashboard_2_Aggregated_query_analysis.json",
317326
PROJECT_ROOT / "postgres_ai_helm/config/grafana/dashboards/Dashboard_2_Aggregated_query_analysis.json",
318327
]
319328
missing = []
320329
checked = 0
330+
operand = QUERY_INFO_JOIN_OPERAND.pattern
331+
group_left_pattern = re.compile(
332+
r"\*\s*on\(queryid\)\s*group_left\([^)]*\)\s*" + operand
333+
)
334+
# Deliberately not anchored to end-of-string: this guard owns the branch
335+
# shape and the operand, not whatever a panel wraps the pair in. An
336+
# end anchor blocked legitimate reshapes (an outer sum by(), a trailing
337+
# comparison) for no gain here.
321338
fallback_pattern = re.compile(
322-
r"\)\s+or\s+\(.*\s+unless\s+on\(queryid\)\s+pgwatch_query_info\)\s*$",
339+
r"\)\s+or\s+\(.*\s+unless\s+on\(queryid\)\s*" + operand,
323340
flags=re.DOTALL,
324341
)
325342

@@ -330,10 +347,41 @@ def test_dashboard_2_pgss_query_info_expressions_have_or_fallbacks():
330347
for dashboard_panel in nested_panels or [panel]:
331348
for target in dashboard_panel.get("targets", []) or []:
332349
expr = target.get("expr") or ""
333-
if "pgwatch_pg_stat_statements_" in expr and "pgwatch_query_info" in expr:
334-
checked += 1
335-
if not fallback_pattern.search(expr):
336-
missing.append((dashboard_path, dashboard_panel.get("id"), dashboard_panel.get("title")))
350+
if "pgwatch_pg_stat_statements_" not in expr:
351+
continue
352+
if "pgwatch_query_info" not in expr:
353+
continue
354+
checked += 1
355+
group_left_match = group_left_pattern.search(expr)
356+
fallback_match = fallback_pattern.search(expr)
357+
problems = join_operand_problems(expr)
358+
if not group_left_match:
359+
problems.append(
360+
"group_left branch does not use the carry-forward operand"
361+
)
362+
if not fallback_match:
363+
problems.append(
364+
"`or ... unless on(queryid)` fallback does not use "
365+
"the carry-forward operand"
366+
)
367+
if (
368+
group_left_match
369+
and fallback_match
370+
and group_left_match.group(1) != fallback_match.group(1)
371+
):
372+
problems.append(
373+
f"branch lookbacks differ: {group_left_match.group(1)} "
374+
f"vs {fallback_match.group(1)}"
375+
)
376+
if problems:
377+
missing.append(
378+
(
379+
dashboard_path,
380+
dashboard_panel.get("id"),
381+
dashboard_panel.get("title"),
382+
problems,
383+
)
384+
)
337385

338386
assert checked >= 40
339387
assert missing == []

tests/grafana_dashboards/conftest.py

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,19 @@
1919

2020
import pytest
2121

22+
# Re-exported so dashboard tests can import everything from one place; the
23+
# definitions live in a plain module to avoid a second conftest import.
24+
from tests.grafana_dashboards.query_info_join import ( # noqa: F401
25+
LABEL_VALUES_CALL,
26+
strip_label_values_calls,
27+
MIN_QUERY_INFO_LOOKBACK_SECONDS,
28+
QUERY_INFO_ANY_REFERENCE,
29+
QUERY_INFO_JOIN_OPERAND,
30+
group_left_label_problems,
31+
join_operand_problems,
32+
promql_duration_seconds,
33+
)
34+
2235
REPO_ROOT = Path(__file__).resolve().parents[2]
2336

2437
# Matches a reference to the db_name template variable in any Grafana syntax,
@@ -47,6 +60,18 @@ def dashboard_paths() -> list[Path]:
4760
return _dashboard_paths()
4861

4962

63+
def unique_dashboard_paths() -> list[Path]:
64+
"""One path per underlying file — the helm tree symlinks into config/."""
65+
seen: set[Path] = set()
66+
unique: list[Path] = []
67+
for path in _dashboard_paths():
68+
resolved = path.resolve()
69+
if resolved not in seen:
70+
seen.add(resolved)
71+
unique.append(path)
72+
return unique
73+
74+
5075
def iter_panels(dashboard: dict) -> Iterable[dict]:
5176
"""Yield every panel in a dashboard, descending into collapsed row panels."""
5277
for p in dashboard.get("panels", []) or []:
Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
"""The canonical pgwatch_query_info join operand, shared by both guards.
2+
3+
pgwatch_query_info arrives in sparse bursts, so joining it as an instant vector
4+
misses its ~5 min lookback and pgss legends degrade to raw labels. Carrying it
5+
forward over days then admits a second risk: one queryid can hold several
6+
displayname* series, which duplicates the joined result. Hence: carry forward,
7+
newest series wins, value normalised back to 1. See #344.
8+
9+
A plain module rather than conftest, so importing it from another test package
10+
does not load conftest a second time alongside pytest's own plugin instance.
11+
"""
12+
from __future__ import annotations
13+
14+
import re
15+
16+
QUERY_INFO_METRIC = "pgwatch_query_info"
17+
PROMQL_DURATION = r"(?:\d+[smhdwy])+"
18+
# Optional label matcher, so a future MR may scope the selector without the
19+
# guards rejecting it out of hand.
20+
QUERY_INFO_SELECTOR = QUERY_INFO_METRIC + r"(?!\w)(?:\{[^}]*\})?"
21+
22+
QUERY_INFO_JOIN_OPERAND = re.compile(
23+
r"\(\s*topk\s+by\s*\(\s*queryid\s*\)\s*\(\s*1\s*,\s*tlast_over_time\("
24+
+ QUERY_INFO_SELECTOR
25+
+ r"\[(" + PROMQL_DURATION + r")\]\)\)\s*>\s*bool\s+0\)"
26+
)
27+
# Any mention of the metric, so the guards can require that every one of them
28+
# is part of a full join operand rather than only rejecting the bare name.
29+
QUERY_INFO_ANY_REFERENCE = re.compile(QUERY_INFO_METRIC + r"(?!\w)")
30+
31+
# Grafana's label_values() takes a selector, not an expression, so a queryid
32+
# picker built on it cannot use the operand and is exempt.
33+
LABEL_VALUES_CALL = re.compile(r"\blabel_values\s*\([^)]*\)")
34+
35+
# Label matcher blocks, blanked out before scanning for join clauses so an
36+
# `on(...)` inside a label *value* cannot be mistaken for a real one.
37+
LABEL_MATCHER_BLOCK = re.compile(r"\{[^{}]*\}")
38+
39+
# What may immediately precede a join operand. The `group_left` form copies
40+
# labels across and is checked; the `unless` form is set exclusion and needs
41+
# none. Anything else — `group_right`, or no modifier at all — means the
42+
# legend's labels never cross the join.
43+
MATCH_CLAUSE = r"\b(on|ignoring)\b\s*\(([^)]*)\)\s*"
44+
GROUP_LEFT_BEFORE_OPERAND = re.compile(MATCH_CLAUSE + r"group_left\s*\(([^)]*)\)\s*$")
45+
_KEYWORD, _MATCH_LABELS, _COPIED_LABELS = 1, 2, 3
46+
UNLESS_BEFORE_OPERAND = re.compile(r"\bunless\s+" + MATCH_CLAUSE + r"$")
47+
48+
_DURATION_UNIT_SECONDS = {
49+
"s": 1, "m": 60, "h": 3600, "d": 86400, "w": 604800, "y": 31536000,
50+
}
51+
52+
# The observed per-series staleness is hours, so anything shorter than an hour
53+
# would satisfy the "is it wrapped?" guards while reinstating the bug.
54+
MIN_QUERY_INFO_LOOKBACK_SECONDS = 3600
55+
56+
57+
def promql_duration_seconds(duration: str) -> int:
58+
"""Convert a PromQL duration ('7d', '1d12h') to seconds."""
59+
parts = re.findall(r"(\d+)([smhdwy])", duration)
60+
assert parts, f"unparseable PromQL duration: {duration!r}"
61+
return sum(int(amount) * _DURATION_UNIT_SECONDS[unit] for amount, unit in parts)
62+
63+
64+
def join_operand_problems(expr: str) -> list[str]:
65+
"""Everything wrong with how one expression references pgwatch_query_info."""
66+
problems: list[str] = []
67+
references = len(QUERY_INFO_ANY_REFERENCE.findall(expr))
68+
if not references:
69+
return problems
70+
71+
operands = QUERY_INFO_JOIN_OPERAND.findall(expr)
72+
if len(operands) != references:
73+
problems.append(
74+
f"{references} reference(s) but {len(operands)} full join operand(s); "
75+
"every reference must be the canonical carry-forward operand"
76+
)
77+
for duration in operands:
78+
seconds = promql_duration_seconds(duration)
79+
if seconds < MIN_QUERY_INFO_LOOKBACK_SECONDS:
80+
problems.append(
81+
f"lookback [{duration}] = {seconds}s is below the "
82+
f"{MIN_QUERY_INFO_LOOKBACK_SECONDS}s staleness floor"
83+
)
84+
return problems
85+
86+
87+
def strip_label_values_calls(expr: str) -> str:
88+
"""Blank out label_values(...) spans, keeping offsets intact.
89+
90+
Grafana's label_values() takes a selector, not an expression, so a queryid
91+
picker built on it cannot carry the metric forward and is exempt — but only
92+
the call itself, not everything else in the same expression.
93+
"""
94+
return LABEL_VALUES_CALL.sub(lambda m: " " * len(m.group(0)), expr)
95+
96+
97+
def group_left_label_problems(expr: str, required_labels: set[str]) -> list[str]:
98+
"""Labels each join must copy across for the legend to resolve.
99+
100+
The operand can be perfect while `group_left()` copies nothing — or is
101+
absent entirely — which renders exactly the raw-label failure the operand
102+
exists to prevent. Checked per join, not per expression, so one correct
103+
join cannot vouch for another.
104+
"""
105+
problems: list[str] = []
106+
if not required_labels or not QUERY_INFO_JOIN_OPERAND.search(expr):
107+
return problems
108+
109+
# Label values can contain anything, including text that looks like a join
110+
# clause, so blank the matcher blocks before reading the clauses.
111+
clauses = LABEL_MATCHER_BLOCK.sub(lambda m: " " * len(m.group(0)), expr)
112+
113+
# Iterate the original expression: blanking is length-preserving, so the
114+
# offsets still line up, but a blanked label matcher would stop the
115+
# operand matching at all and silently switch this guard off.
116+
for operand in QUERY_INFO_JOIN_OPERAND.finditer(expr):
117+
prefix = clauses[: operand.start()].rstrip()
118+
119+
if UNLESS_BEFORE_OPERAND.search(prefix):
120+
# Set exclusion: the operand only decides membership, so it copies
121+
# nothing and needs nothing.
122+
continue
123+
124+
match = GROUP_LEFT_BEFORE_OPERAND.search(prefix)
125+
if match is None:
126+
problems.append(
127+
"a pgwatch_query_info join has neither group_left(...) nor "
128+
"unless on(...) before it, so the label(s) the legend renders "
129+
"never cross it: " + ", ".join(sorted(required_labels))
130+
)
131+
continue
132+
133+
# Labels matched on are already present on the left-hand side.
134+
# `ignoring(...)` names the opposite set, so it exempts nothing.
135+
matched_on: set[str] = set()
136+
if match.group(_KEYWORD) == "on":
137+
matched_on = {
138+
label.strip()
139+
for label in match.group(_MATCH_LABELS).split(",")
140+
if label.strip()
141+
}
142+
copied = {
143+
label.strip()
144+
for label in match.group(_COPIED_LABELS).split(",")
145+
if label.strip()
146+
}
147+
missing = sorted(required_labels - copied - matched_on)
148+
if missing:
149+
problems.append(
150+
"group_left does not copy the label(s) the legend renders: "
151+
+ ", ".join(missing)
152+
)
153+
154+
return problems

0 commit comments

Comments
 (0)