fix(app): list metric names deterministically instead of sampling - #2747
fix(app): list metric names deterministically instead of sampling#2747teeohhem wants to merge 4 commits into
Conversation
The chart editor's metric dropdown discovered names via
getKeyValues({ keys: ['MetricName'], limit: 3000 }), which renders
`groupUniqArray(3000)(MetricName)`. Once a metrics table holds more than
3000 distinct names, groupUniqArray keeps an arbitrary subset — the
survivors follow hash order, not name order — so metrics that exist and are
actively reporting could be unselectable, with no warning and no way to
search for what had been dropped. A full Prometheus scrape of a Kubernetes
cluster clears 3000 distinct gauge names easily, and the reported symptom was
`up` being present in otel_metrics_gauge but absent from the dropdown.
Measured on a deployment with 1209 distinct gauge names, capped at 1000: the
surviving page was not the alphabetically-first 1000 — `metric_00001` was
dropped while `metric_00002` survived.
Adds Metadata.getMetricNames(): a real
`GROUP BY MetricName ... ORDER BY ... LIMIT n+1` with an optional server-side
`MetricName ILIKE` predicate. The extra row makes truncation detectable, so
the dropdown can say the list is incomplete rather than implying otherwise.
Ordering is by relevance to the search pattern (exact, then prefix, then
alphabetical) in SQL rather than in the client. A page ordered purely by name
would still hide a short query like `up` behind the many names that merely
contain it — `group_reads`, `node_uptime_seconds` — which is the original
problem in a new form. Doing it in the query keeps the page returned the page
worth showing, and leaves the component with no ranking logic of its own.
Also restores the chart's time range to the dropdown. useMetricNames accepts
a dateRange and ChartEditorControls already passes one down, but
ChartSeriesEditor never read it, so the window was pinned to the last 24h and
the clamping logic was dead code — any metric last seen over a day ago was
invisible regardless of the range selected.
Three details worth noting for review:
- `timeout_overflow_mode` is pinned to `throw` rather than left unset. `break`
returns a partial aggregate as HTTP 200, i.e. a short list reporting
`truncated: false`, which is the silent incompleteness this replaces, and
the settings spread it sits in is a mutable process-wide bag.
- Mantine mirrors a selected option's label into a searchable Select's input
and reports it through `onSearchChange` exactly like typed text, so it is
explicitly not forwarded as a name pattern; the input is also cleared on
dropdown open so the label cannot be edited into a fragment and searched.
- The query sets `retry: false`, an explicit `staleTime` and no refetch on
window focus. Four of these run per debounced keystroke, and the replaced
MetadataCache path served repeats from memory, so without these the new path
would be busier than the one it replaces.
Metric-name listing had no test coverage anywhere — metadata.int.test.ts was
logs-only and MetricNameSelect.test.ts only unit-tested getMetricOptions.
Adds coverage of the rendered SQL, integration coverage against a table with
more distinct names than one page returns (including that an escaped `_` still
matches, since over-escaping would break every Prometheus metric search), and
a guard on the dateRange wiring so a refactor cannot quietly drop it again.
🦋 Changeset detectedLatest commit: a20d251 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR replaces sampled metric-name discovery with deterministic, server-side searching.
Confidence Score: 4/5The PR should not merge until the metric-name selector restores the exponential-histogram feature gate. The selector still queries exponential-histogram metadata and turns returned names into selectable options regardless of the documented environment gate, so disabled functionality remains exposed. The previously reported transient-failure omission is now visibly disclosed and can recover when the search, range, or source changes. Files Needing Attention: packages/app/src/components/MetricNameSelect.tsx
|
| Filename | Overview |
|---|---|
| packages/common-utils/src/core/metadata.ts | Adds parameterized, deterministic metric-name discovery with relevance ordering and explicit truncation reporting. |
| packages/app/src/hooks/useMetadata.tsx | Adds the React Query wrapper for metric-name discovery with bounded refetch behavior. |
| packages/app/src/components/MetricNameSelect.tsx | Replaces sampled client-side filtering with debounced server-side search and visible completeness status. |
| packages/app/src/components/DBEditTimeChartForm/ChartSeriesEditor.tsx | Forwards the chart date range into metric-name discovery. |
| packages/common-utils/src/tests/metadata.test.ts | Covers generated SQL, wildcard escaping, relevance ordering, timeout behavior, and truncation. |
| packages/common-utils/src/tests/metadata.int.test.ts | Adds ClickHouse integration coverage for deterministic browsing and exact-match search. |
Sequence Diagram
sequenceDiagram
participant U as Chart editor
participant S as MetricNameSelect
participant Q as useGetMetricNames
participant M as Metadata
participant C as ClickHouse
U->>S: date range and metric source
S->>S: Debounce search text
S->>Q: Query each configured metric table
Q->>M: getMetricNames(pattern, range)
M->>C: GROUP BY name, relevance ORDER BY, LIMIT n+1
C-->>M: Ordered names
M-->>Q: names and truncated flag
Q-->>S: Per-kind results
S-->>U: Selectable options and status
Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
E2E Test Results✅ All tests passed • 322 passed • 1 skipped • 1385s
Tests ran across 4 shards in parallel. |
A failed metric-name query previously just vanished from the dropdown. The query is not retried and does not refetch on focus, so a transient error — or one too slow to finish inside the execution cap — silently omitted that kind's metrics from a list that looked perfectly healthy, which is the same class of problem as the sampling it replaced. Reported in the Select's description rather than its `error` slot, since that slot carries form validation for this field and the series editor clears it on focus.
The explicit 15s `max_execution_time` was tighter than what this path had before. The dropdown called getKeyValues with `disableRowLimit: true`, which sends `clickhouse_settings: undefined`, so the client filled in the deployment's `queryTimeout` (60s by default, operator-configurable). Capping at 15s would break enumeration on any metrics table that takes longer than that to aggregate but previously succeeded. Leaving `max_execution_time` unset restores that bound. The row cap stays at `0`: bounding rows underneath `ORDER BY ... LIMIT` is precisely what made the old result an arbitrary subset, so it would reintroduce the bug this replaces. Superseded searches abort through the query's `signal`, so only the latest pattern is ever in flight.
Deep Review✅ No critical issues found. The reported bug ( 🟡 P2 -- recommended
🔵 P3 nitpicks (3)
Reviewers: correctness, security, performance, reliability, adversarial, kieran-typescript, frontend-races, testing, maintainability, api-contract, previous-comments. Verified not issues: The prior Testing gaps: hook-level Coverage note: Findings reflect direct verification of the diff against the surrounding code and the prior-comment history; automated sub-reviewer returns had not landed at synthesis time, so no cross-reviewer corroboration is claimed. |
Not really an issue Mantine repairs this on the next commit —
Both premises are inverted. The replaced call site passed
Four per keystroke is real, but |
|
Since |
…metric-name-listing # Conflicts: # packages/app/src/components/DBEditTimeChartForm/ChartSeriesEditor.tsx # packages/app/src/components/DBEditTimeChartForm/__tests__/DBEditTimeChartForm.test.tsx # packages/app/src/components/MetricNameSelect.tsx
|
@wrn14897 I spoke with Mike about this as well. I prototyped the dictionary lookup and it's not a complete win. It also benchmarks slower: on a 30M row / 29 partition metrics table it still reads the whole index stream (30M rows, ~30ms) while the PR's 3-day-clamped GROUP BY prunes to 3M rows (~13ms). And since tables created before #2545 still have MetricName String (no ALTER migration), it would throw on older deployments anyway. I vote we proceed with the current implementation and evaluate dictionary lookups more broadly when more time passes. |
2 participants
Problem
A customer could not select a metric that exists and is actively reporting: it was missing from the chart editor's dropdown, and typing its name returned nothing. The reported case was
up.The dropdown discovered names with
groupUniqArray(3000)(MetricName), which keeps an arbitrary subset once a source has more than 3000 distinct names — a full Prometheus scrape of a Kubernetes cluster clears that easily. The browser then filtered that already-sampled list client-side, so a name that never left ClickHouse could not be typed into existence.Fix
Metadata.getMetricNames()— a realGROUP BY MetricName … ORDER BY … LIMIT n+1with the search pattern pushed into SQL asMetricName ILIKE. Each debounced keystroke re-queries, so the filter runs over the table rather than over a sample. The extra row makes truncation detectable, so the dropdown can report an incomplete list instead of implying it is whole.Matches are ranked in SQL — exact, then prefix, then alphabetical — because searching
upalso matches names that sort ahead of it:Also forwards the chart's date range to the dropdown.
useMetricNamesalways accepted adateRange, butChartSeriesEditornever passed one, so the listing window was pinned to the last 24h regardless of the range the chart showed.getKeyValuesis untouched — it is shared with the filter UI, so changing its semantics has a much larger blast radius.Not fixed here
Search is now reliable in the query, but not yet end-to-end in the UI:
<Select limit={100}>renders a gauge-first concatenation of the four kinds, so an exact match in a Sum or Histogram table is unreachable once gauge matches alone fill the cap.Neither affects the reported case —
upis a gauge and reports continuously — and both are tracked separately.Tests
Metric-name listing had no coverage anywhere. Added unit coverage of the rendered SQL (grouping, ordering,
LIMIT n+1, wildcard escaping, truncation reporting) and integration coverage against a gauge-shaped table holding more distinct names than one page returns — including the regression itself: an alphabetically-lateupthat a capped page cannot reach comes back first when searched.Fixes: HDX-5007