feat(app): stream metric names from the primary index - #3025
Conversation
Fill the metric name select from the table's sparse primary index via the mergeTreeIndex table function, so it populates in ~30ms instead of ~770ms on a source reporting ~4,900 gauge metrics, and streams in progressively rather than arriving all at once. Browsing reads the index -- one row per granule mark instead of a full column scan. That list is a subset, weighted towards metrics that carry data, so typing switches to the exhaustive relevance-ranked search added in the parent PR, which reaches anything the index omitted. Browsing also falls back to that search when the index cannot be read at all: a server older than 24.2, a Distributed or non-MergeTree table, or a primary key without MetricName. While the first search for a pattern is in flight the browse list is held and filtered client-side, so the options never blank out mid-keystroke, and the render cap is raised to 500 to match the server-side page size. Adds Metadata.streamDistinctIndexValues, an async generator generic over table and column, and a useStreamingQuery hook that accumulates an async iterable into a React Query cache entry. streamToAsyncIterator moves out of the deprecated session code into common-utils.
🦋 Changeset detectedLatest commit: c5da4b0 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.
|
Greptile SummaryThis PR changes metric-name browsing to stream values from ClickHouse primary indexes while retaining exhaustive queries for typed searches and fallback behavior.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/app/src/hooks/useMetricNames.ts | Coordinates per-kind primary-index browsing, exhaustive typed search, fallback behavior, sorting, and loading state. |
| packages/app/src/hooks/useStreamingQuery.tsx | Adds a React Query wrapper that publishes throttled partial results while accumulating the complete async stream. |
| packages/common-utils/src/core/metadata.ts | Adds guarded streaming of distinct primary-index values for compatible MergeTree tables and server versions. |
| packages/app/src/components/MetricNameSelect.tsx | Integrates streamed metric names, search-oriented messaging, expanded rendering, and streaming feedback into the selector. |
Sequence Diagram
sequenceDiagram
participant U as User
participant S as Metric selector
participant H as useMetricNames
participant I as Primary-index stream
participant Q as Exhaustive query
participant C as ClickHouse
U->>S: Open metric selector
S->>H: Request names without pattern
H->>I: Stream MetricName values
I->>C: Read mergeTreeIndex
C-->>I: Progressive chunks
I-->>H: Partial name lists
H-->>S: Sorted browse options
alt Index read fails
H->>Q: Request exhaustive listing
Q->>C: Grouped metric-name query
C-->>S: Complete fallback options
end
U->>S: Type search text
S->>H: Request names with pattern
H->>Q: Run exhaustive ranked search
Q->>C: Search metric data
C-->>S: Authoritative matches
Reviews (2): Last reviewed commit: "fix(app): make the index read work on an..." | Re-trigger Greptile
| @@ -0,0 +1,330 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
This new 330-line test file exceeds the repository's 300-line limit, making the browse, search, fallback, and streaming cases harder to navigate and extend. Split these behaviors into focused test suites.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Not taking this one. The 300-line guidance in code_style.md sits under Code Organization next to "Single Responsibility: one clear purpose per component/function" — it reads as a rule for source modules, and the repo's own test suite doesn't apply it: 61 of 201 test files in packages/app/src exceed 300 lines, the largest at 2,005 (ChartEditor/__tests__/utils.test.ts). The nearest sibling to this file, useOffsetPaginatedQuery.test.tsx, is 1,176 lines.
At 330 lines this file is smaller than roughly 30% of the existing suite, and its cases share one describe, one mock setup, and one fixture. Splitting them across files would duplicate that scaffolding to satisfy a threshold nothing else in the package observes.
There was a problem hiding this comment.
That's fair. The repository evidence supports treating the 300-line guidance as a source-module heuristic rather than a hard limit for tests, and this file's shared setup and single purpose make splitting it counterproductive. I’m withdrawing this comment.
Deep Review✅ No critical issues found. The streaming/fallback wiring, SQL parameter binding, and error propagation all hold up under review. Findings below are one moderate perf trade-off and a few nits. 🟡 P2 — recommended
🔵 P3 nitpicks (2)
Reviewers (7): maintainability, performance, security, testing, reliability, project-standards, previous-comments. Coverage note: The correctness, adversarial, kieran-typescript, and julik-frontend-races reviewers did not return before synthesis. Their lenses were partially covered by direct verification: SQL values ( Testing gaps:
|
Two ways the index read could come back empty or broken without the fallback noticing. partsOverlapFilter matches on system.parts.min_time, which ClickHouse only populates for time-based partition keys. On a table partitioned by anything else -- or not partitioned at all -- every part sits at the epoch, the predicate excludes all of them, and the query *succeeds* with zero rows. Nothing throws, so streamed.isError stays false and the exhaustive fallback never fires: the picker just shows an empty list. Measured on live ClickHouse: 40 metric names became 0 for both `no PARTITION BY` and `PARTITION BY ServiceName`. Now the parts filter is only applied when the partition key derives from the timestamp column, and skipped otherwise, accepting an unpruned window instead. streamToAsyncIterator assumed a WHATWG ReadableStream, but the node client's stream() returns a Node Readable with no getReader, so streamDistinctIndexValues threw `getReader is not a function` outside the browser. Metadata is platform-agnostic, so the adapter now handles both. Both verified against a live 121M-row instance across three partition schemes, with unit coverage for each.
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Additional context: touches the query rendering engine lightly (30 lines, under the 150-line bar for Tier 4) Review process: Full human review — logic, architecture, edge cases. Stats
|
E2E Test Results✅ All tests passed • 321 passed • 1 skipped • 1377s
Tests ran across 4 shards in parallel. |
|
On the Deep Review P2 about the I benchmarked both against a local 121M-row instance with 4,914 gauge names:
So the ~150ms of main-thread blocking the review cites is real and reproducible. The reason for accepting it: 500 matches Worth noting the suggested fix doesn't address the related problem either: Virtualizing the dropdown is the right long-term answer and I agree with it; it's a larger change than this PR should carry. |
Problem
The metric name select waited on a
groupUniqArrayaggregation over the data before showing anything. On a source reporting ~4,900 gauge metrics that is ~770ms of empty dropdown, paid on every chart editor mount — and the control is mounted per series, on every dashboard tile editor.Fix
Read
MetricNamefrom the table's sparse primary index via themergeTreeIndextable function instead of scanning the data: one row per granule mark. Measured on a 121M-rowotel_metrics_gaugewith 4,914 gauge names:GROUP BYDISTINCTis a streaming transform in ClickHouse, so results also arrive progressively rather than in one block — first options on screen ~30ms after the query starts, and a small spinner replaces the chevron while more land.ORDER BYis deliberately omitted, since the sort would have to complete before the first row; the client sorts.The index returns a subset, and that is the design
The index only records the column's value at each granule boundary, so a metric confined to one granule never appears. On the test dataset that is 1,986 of 4,914 names — and the split is not arbitrary:
The hidden ~2,900 are almost entirely metrics with a dozen datapoints. So browsing shows the metrics that actually carry data, and typing switches to the exhaustive relevance-ranked search from #2747, which reaches anything the index omitted. The placeholder reads
Search metrics...to invite that. Verified end-to-end: a metric absent from the index is found by typing its name, ranked first.Browsing also falls back to that same exhaustive query when the index cannot be read at all — a server older than 24.2, a Distributed or non-MergeTree metric table, or a primary key without
MetricName— so no deployment loses the picker.Two behaviours worth reviewing
The options never blank mid-keystroke.
keepPreviousDataserves the previous pattern's page when the search key changes, and results forhttpcannot match a search forbilling— the client-side filter reduces them to nothing and the dropdown flashes empty for ~700ms. This ignores placeholder pages (isPlaceholderData) and holds the unfiltered browse list instead, which still filters to relevant matches immediately. Measured before/after with aMutationObserveron the option list.Render cap raised 100 → 500 to match
DEFAULT_METRIC_NAMES_LIMIT, so a search not reported astruncatedis fully renderable. Mantine renders options as plain DOM, so this costs ~150ms of keystroke blocking at ~4.9k names that a cap of 100 avoided — an accepted trade, not an oversight.Known limitation, not addressed here
getMetricOptionsconcatenates the four kind lists gauge-first, so kind rather than relevance decides what survives the render cap. Searchinghttpon the test dataset returns 0 of 26 matching Sum metrics behind 325 gauge matches. This predates both PRs (called out in #2747 as tracked separately) and the fix is a relevance-ordered merge across kinds; raising the cap does not fix it, since gauge alone exceeds 500 while browsing.Testing
Metadata.streamDistinctIndexValues: 13 unit tests over a fakedBaseResultSet— chunk-per-chunk yielding, the emitted SQL, parts pruning, and each guard (old server, pointer table, non-MergeTree engine, column outside the primary key, including a column that only appears nested in a key expression).useStreamingQuery: 5 tests — partial publication before completion, throttle-away, error discards partials, disabled, empty stream.useMetricNames: 10 tests — per-kind streaming and sorting, mode switch, the hold, the stale-placeholder guard, the fallback.MetricNameSelectSearch: fix(app): list metric names deterministically instead of sampling #2747's 11 cases retargeted to the two-mode behaviour, all passing.metric-name-streaming.spec.ts): browses from the index, then finds an index-invisible metric by typing. Asserts the seed still has such a metric, so the test cannot pass vacuously.make ci-lintclean,make ci-unit3400 passed. Verified by hand against a 121M-row local instance.Notes
streamToAsyncIteratormoves frompackages/app/src/sessions.ts(markedTO BE DEPRECATED) intocommon-utilsbeside the ClickHouse client.Metadata.streamDistinctIndexValuesis generic over table and column, soServiceNameand other primary-key columns can be listed the same way.🤖 Generated with Claude Code