perf: sample text index key values instead of loading all parts in time range by knudtty · Pull Request #3418 · hyperdxio/hyperdx

knudtty · 2026-10-20T14:53:36Z

Summary

Adds user-configurable intelligent sampling to parts when loading keys to populate filter values.

Screenshots or video

Before	After

References

Linear Issue:
Related PRs:

range

vercel · 2026-10-20T14:53:44Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
hyperdx-oss	Ready	Preview	Oct 20, 2026 5:00pm UTC
hyperdx-storybook	Ready	Preview	Oct 20, 2026 5:00pm UTC

changeset-bot · 2026-10-20T14:53:44Z

🦋 Changeset detected

Latest commit: 8c7169e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@hyperdx/app	Patch
@hyperdx/common-utils	Patch
@hyperdx/api	Patch
@hyperdx/otel-collector	Patch

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

github-actions · 2026-10-20T14:55:02Z

Duplicate code analysis

1 new clone introduced by this PR.

packages/common-utils/src/core/metadata.ts:1728-1735 ↔ packages/common-utils/src/core/metadata.ts:1824-1831 — 8 lines

If one of these duplicates a helper that already exists, import it instead
of redefining it. If the duplication is deliberate, ignore this comment —
the check is advisory and never fails the build.

jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 124 in total (1.62%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

github-actions · 2026-10-20T14:56:44Z

    metadataMVs,
    dateRange: callerDateRange,
    timestampValueExpression,
+    keySampleRatio = 0,


🟠 major — Sampling is now the default for every caller of getMapKeys / getAllFields / getAllKeyValues, not just the filter panel

keySampleRatio = 0 is the default here and in dispatchKeyValueQueries (line 2905), so callers that never opted in now get a subset. That includes packages/api/src/mcp/tools/sources/metricNames.ts:70 (MCP metric-name listing over a text-indexed MetricName), describeSource.ts:293/339/386, controllers/ai.ts:107, and app callers of useAllFields/useGetKeyValues('all') outside the search filters. None of them have a way to ask for more. Fix: default keySampleRatio to 1 (read every part) and pass the sampled ratio only from DBSearchPageFilters. The changeset should also list @hyperdx/api, since its output changes.

_{Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond}

github-actions · 2026-10-20T14:56:44Z

      disableRowLimit,
      maxKeys,
      limit,
+      keySampleRatio,


🔵 minor — Changing the sample ratio in 'exact' mode refetches every facet's values even though exact mode never uses the ratio

DBSearchPageFilters passes keySampleRatio whatever showAllValues is set to, and it goes into the useGetKeyValues query key. In exact mode getKeyValuesWithMVs ignores it, so picking 10%/50%/100% re-runs every exact value query against the raw table for the same result. Add the ratio to the key only when mode === 'all' (or pass undefined from useFacets when the mode is 'exact').

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

github-actions · 2026-10-20T14:56:44Z

        : '';
    const cacheKey = metricName
-      ? `${connectionId}.${databaseName}.${tableName}.${column}.${metricName}.${dateRangeCacheSuffix}.${maxKeys}.keys`
+      ? `${connectionId}.${databaseName}.${tableName}.${column}.${metricName}.${dateRangeCacheSuffix}.${maxKeys}.${keySampleRatio}.keys`


🔵 minor — getMapKeys cache key includes keySampleRatio on the rollup/scan paths, which never use it

Only the text index branches read keySampleRatio. A Map column with no key/kv text index (e.g. ResourceAttributes when only LogAttributes is indexed) still gets a new cache entry for each ratio, so changing the sample re-runs its rollup or full scan. Add the ratio to the cache key only when the text index path is taken, or keep a separate key for the text index result.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

github-actions · 2026-10-20T14:56:44Z

+    if (!timestampValueExpression) return undefined;
+    const [textIndexes, serverVersion] = await Promise.all([
+      this.getMapColumnTextIndexes({ databaseName, tableName, connectionId }),
+      this.getServerVersion({ connectionId }),


🔵 minor — getKeySample ignores native column text indexes, so their sampled values get no indicator and no control

getTextIndexKeyValues now samples parts for native text-indexed columns (e.g. ServiceName, TraceId). But getKeySample returns undefined unless some Map column has a key/kv index, so KeySampleControl is hidden. On a table with only native text indexes, 'all' mode values silently come from a sample, and the user can't read more except through per-key 'Load more'. Count determineKeyValueFetchingStrategy(...).nativeTextIndexLookup too when deciding whether the table is sampled.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

github-actions · 2026-10-20T14:56:44Z

      disableRowLimit,
      maxKeys,
      limit,
+      keySampleRatio,


🔵 minor — useKeySample re-runs the sampling query on every focus/remount, and its count can disagree with the cached keys it describes

The query has no staleTime, and the app QueryClient (packages/app/pages/_app.tsx:58) uses React Query's defaults (staleTime 0, refetchOnWindowFocus). getKeySample is also not memoized in MetadataCache. So every tab focus re-runs the system.parts + mergeTreeIndex cover query, while getMapKeys serves keys from its TTL-less cache built from an older pick of parts. Cache getKeySample in MetadataCache under the same key fields as getMapKeys, or give the query a staleTime that matches the key/value queries (5 min).

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

github-actions · 2026-10-20T14:56:47Z

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.

🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.

Anything about surrounding code, or asking you to widen the change (hoist a helper,
dedupe with another file, fix other call sites): never fix here, whatever the
severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

5 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 4 minor

5 posted as inline comment(s) on the changed lines.

_{Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.}

greptile-apps · 2026-10-20T14:59:32Z

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge, with a non-blocking local metadata-setting issue and two earlier suggestion issues still outstanding.

Findings

Metadata limits ignore resets ▶
Sample controls disappear ▶
Rare fields lose suggestions ▶

Summary

Samples text-index parts when loading filter keys and values, and adds browser-saved query settings in local mode.

Map-key lookups read a sample of data parts by default.
The filter panel lets people choose how broadly to sample keys.
Local mode now has its own Team Settings page.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Filters[Filter panel sample choice] --> Metadata[Metadata lookup]
  Metadata --> Parts[Choose overlapping parts]
  Parts --> Index[Read sampled text indexes]
  More[Load more for one field] --> All[Read all overlapping parts]
  Settings[Local query settings] --> Storage[Browser storage]
  Storage --> Queries[Query settings readers]

_{Reviews (2) · Last reviewed commit: "feat(app): enable Team Settings in local..." · Reviewed by Greptile}

github-actions · 2026-10-20T14:59:34Z

Deep Review

Sampling the text index of a subset of parts (ranked by size/recency plus parts that cover string primary-key values) instead of every part in the time range is a sound, well-commented approach, and the SQL is parameter-bound throughout. I independently verified the highest-risk paths: the ratio=0 edge case still returns the top-ranked part (the cum_rows - est_rows < KEY_SAMPLE_MIN_ROWS clause is true at rank=1), the keepDataAcrossKeySampleChange placeholder correctly compares queryKey.slice(0, -1) with keySampleRatio always last, and the raw-SQL assembly has no injection sink.

✅ No critical issues found.

🟡 P2 — recommended

packages/common-utils/src/__tests__/metadata.int.test.ts:1560 — the only coverage of the part ranking/cover/selection logic in sampledParts.ts lives in integration tests that return early (silent skip, just a console.warn) when the server is below ClickHouse 26.3, while the unit test in metadata.test.ts only string-matches the generated SQL (part_name IN (, row_number() OVER w AS rank) rather than asserting which parts get selected.
- Fix: Add a unit-level assertion over renderSampledPartNames/renderSampledPartCounts output (or a seeded integration path) that exercises ratio=0 min-rows selection, the cover-column branch, and the no-overlapping-parts case, and mark the version-gated tests as skipped rather than silently returning.

🔵 P3 nitpicks (3)

packages/common-utils/src/core/metadata.ts:2093 — getKeySample issues a separate renderSampledPartCounts query to populate the "Sampled from X of Y parts" label, so the displayed counts can diverge from the parts the subsequent read actually covers when parts merge between the two queries (the same race the in-query selection is designed to avoid).
- Fix: Note in the UI copy or a code comment that the counts are an estimate, or derive them from the read query where feasible.
packages/common-utils/src/core/metadata.ts:1177 — keySampleRatio is accepted at the metadata boundary without clamping to [0, 1]; the UI only supplies fixed values and the value is bound as a ClickHouse Float64 param (so there is no injection or crash), but an out-of-range value would silently widen or empty the sample.
- Fix: Clamp keySampleRatio to [0, 1] where it enters keySampleOptions.
packages/app/src/components/DBSearchPageFilters/sampledParts.ts selection — for small part counts the rank <= ratio * total_parts clause floors to zero (e.g. 10% of 6 parts selects no additional parts), so the "10%" level collapses to "Auto" below ~10 parts.
- Fix: Consider ceil(ratio * total_parts) so a non-zero ratio always widens the sample by at least one part.

Reviewers (9): correctness, security, performance, adversarial, kieran-typescript, julik-frontend-races, testing, maintainability, project-standards.

Testing gaps: Core sampledParts.ts selection logic is only verified by version-gated integration tests that skip silently below ClickHouse 26.3; no unit test asserts part-selection behavior or the getKeySampleCoverColumns try/catch fallback.

_{No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond}

greptile-apps · 2026-10-20T14:59:35Z

+    const hasKeyTextIndex = [...textIndexes.values()].some(
+      ({ key, kv }) => key?.indexName || kv?.indexName,
+    );
+    if (!hasKeyTextIndex || !supportsMergeTreeTextIndex(serverVersion)) {
+      return undefined;


Sample controls disappear

getKeySample checks only Map text indexes, but getTextIndexKeyValues now samples native-column text indexes too. For a table with only native text indexes, the panel omits values from unselected parts without showing the sample count or ratio control.

Include native indexes in this check so users can see and enlarge the sample. Per-field “Load more” still works, but does not replace the missing notice and panel-wide control.

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!

greptile-apps · 2026-10-20T14:59:35Z

    metadataMVs,
    dateRange: callerDateRange,
    timestampValueExpression,
+    keySampleRatio = 0,


Rare fields lose suggestions

The new default sample also reaches search autocomplete, which calls useFetchFacets without keySampleRatio. A Map key found only in an older, unselected part disappears from suggestions. Typing the exact key does not load its values because searchField must first exist in the sampled field list.

Choosing 100% in the sidebar does not help because that choice is local to the sidebar. Share the sample choice with autocomplete, or provide a full lookup for a typed key.

Knowledge Base Used: Roll Back Sampled Metric Name Listing

github-actions · 2026-10-20T15:04:18Z

E2E Test Results

✅ All tests passed • 443 passed • 1 skipped • 1472s

Status	Count
✅ Passed	443
❌ Failed	0
⚠️ Flaky	1
⏭️ Skipped	1

Tests ran across 4 shards in parallel.

View full report →

Local mode (Vercel previews, `yarn app:dev:local`) had no way to reach Team Settings, so query settings could not be tuned there. Link to the page and show its Data and Query Settings tabs; the other tabs need the API. Query settings are saved to localStorage, and every reader of `me.team` settings now goes through getTeamClickHouseSettings so they take effect.

greptile-apps · 2026-10-20T16:59:39Z

+  const metadataMaxRowsToRead =
+    getTeamClickHouseSettings(me)?.metadataMaxRowsToRead;


Metadata limits ignore resets

The new local metadata setting ignores 0 and does not restore the default after a reset. useMetadataWithSettings only applies truthy values, while MetadataCache keeps the old limit across pages.

Save a small limit, visit Search, reset it in Team Settings, then return to Search: metadata queries still use the small limit and can return incomplete field and value lists until reload. Choosing unlimited also leaves the default limit in place. Apply 0 explicitly and restore DEFAULT_METADATA_MAX_ROWS_TO_READ when the saved value is removed.

perf: sample text index key values instead of loading all parts in time

a7538c2

range

knudtty marked this pull request as draft October 20, 2026 14:53

vercel Bot deployed to Preview – hyperdx-storybook October 20, 2026 14:55

View deployment

github-actions Bot reviewed Oct 20, 2026

View reviewed changes

vercel Bot deployed to Preview – hyperdx-oss October 20, 2026 14:57

View deployment

greptile-apps Bot reviewed Oct 20, 2026

View reviewed changes

vercel Bot deployed to Preview – hyperdx-storybook October 20, 2026 16:57

View deployment

greptile-apps Bot reviewed Oct 20, 2026

View reviewed changes

vercel Bot deployed to Preview – hyperdx-oss October 20, 2026 17:00

View deployment

		const metadataMaxRowsToRead =
		getTeamClickHouseSettings(me)?.metadataMaxRowsToRead;

Conversation

knudtty commented Oct 20, 2026

Summary

Screenshots or video

References

Uh oh!

vercel Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Duplicate code analysis

Uh oh!

github-actions Bot Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 20, 2026

PR Review

Uh oh!

greptile-apps Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 5/5

Findings

Summary

Diagram

Uh oh!

github-actions Bot commented Oct 20, 2026

Deep Review

🟡 P2 — recommended

Uh oh!

greptile-apps Bot Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

E2E Test Results

Uh oh!

greptile-apps Bot Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

This branch was successfully deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vercel Bot commented Oct 20, 2026 •

edited

Loading

changeset-bot Bot commented Oct 20, 2026 •

edited

Loading

github-actions Bot commented Oct 20, 2026 •

edited

Loading

greptile-apps Bot commented Oct 20, 2026 •

edited

Loading

github-actions Bot commented Oct 20, 2026 •

edited

Loading