From 0b320166c1c60237975563761437678647e2a9cb Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Wed, 29 Jul 2026 12:59:57 -0400 Subject: [PATCH 1/3] fix(app): list metric names deterministically instead of sampling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../deterministic-metric-name-listing.md | 6 + .../DBEditTimeChartForm/ChartSeriesEditor.tsx | 2 + .../__tests__/DBEditTimeChartForm.test.tsx | 37 ++++ .../app/src/components/MetricNameSelect.tsx | 158 +++++++++--------- .../__tests__/MetricNameSelectSearch.test.tsx | 158 ++++++++++++++++++ packages/app/src/hooks/useMetadata.tsx | 68 ++++++++ packages/app/src/setupTests.tsx | 6 + .../src/__tests__/metadata.int.test.ts | 158 ++++++++++++++++++ .../src/__tests__/metadata.test.ts | 137 +++++++++++++++ packages/common-utils/src/core/metadata.ts | 132 +++++++++++++++ 10 files changed, 787 insertions(+), 75 deletions(-) create mode 100644 .changeset/deterministic-metric-name-listing.md create mode 100644 packages/app/src/components/__tests__/MetricNameSelectSearch.test.tsx diff --git a/.changeset/deterministic-metric-name-listing.md b/.changeset/deterministic-metric-name-listing.md new file mode 100644 index 0000000000..1c1319da91 --- /dev/null +++ b/.changeset/deterministic-metric-name-listing.md @@ -0,0 +1,6 @@ +--- +'@hyperdx/app': patch +'@hyperdx/common-utils': patch +--- + +fix: metric names in the chart editor are now listed deterministically instead of sampled. The dropdown discovered names with `groupUniqArray(3000)(MetricName)`, which keeps an arbitrary subset once a metrics table holds more than 3000 distinct names — 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. Names are now fetched with an ordered, paginated query and matched server-side, ranked so an exact match is always on the first page, and the dropdown says when the list is incomplete. Also fixes the metric list ignoring the chart's selected time range, which pinned it to the last 24 hours. diff --git a/packages/app/src/components/DBEditTimeChartForm/ChartSeriesEditor.tsx b/packages/app/src/components/DBEditTimeChartForm/ChartSeriesEditor.tsx index 137d328cbe..a0e5a23b19 100644 --- a/packages/app/src/components/DBEditTimeChartForm/ChartSeriesEditor.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/ChartSeriesEditor.tsx @@ -104,6 +104,7 @@ export function ChartSeriesEditor({ tableSource, errors, clearErrors, + dateRange, }: ChartSeriesEditorProps) { const aggFn = useWatch({ control, name: `${namePrefix}aggFn` }); const aggConditionLanguage = useWatch({ @@ -360,6 +361,7 @@ export function ChartSeriesEditor({ setValue(`${namePrefix}metricType`, value) } metricSource={tableSource} + dateRange={dateRange} data-testid="metric-name-selector" error={errors?.metricName?.message} onFocus={() => clearErrors(`${namePrefix}metricName`)} diff --git a/packages/app/src/components/DBEditTimeChartForm/__tests__/DBEditTimeChartForm.test.tsx b/packages/app/src/components/DBEditTimeChartForm/__tests__/DBEditTimeChartForm.test.tsx index 7796fdee80..96cb5439df 100644 --- a/packages/app/src/components/DBEditTimeChartForm/__tests__/DBEditTimeChartForm.test.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/__tests__/DBEditTimeChartForm.test.tsx @@ -72,8 +72,12 @@ jest.mock('@/source', () => ({ useSources: jest.fn().mockReturnValue({ data: [] }), })); +// Records every render's props so tests can assert what the form passes down. +const metricNameSelectProps: any[] = []; + jest.mock('../../MetricNameSelect', () => ({ MetricNameSelect: (props: any) => { + metricNameSelectProps.push(props); const { error, onFocus, setMetricName, metricName } = props; const testId = props['data-testid']; return ( @@ -380,6 +384,39 @@ describe('DBEditTimeChartForm - Metric Name Validation', () => { }); }); +describe('DBEditTimeChartForm - Metric name date range wiring', () => { + beforeEach(() => { + jest.clearAllMocks(); + metricNameSelectProps.length = 0; + }); + + // Regression guard: MetricNameSelect only lists metrics that reported inside + // the range it is given, and falls back to the last 24h when the prop is + // missing. That pass-through was silently dropped once already when + // ChartSeriesEditor was split out of this file, which made any metric last + // seen over a day ago unselectable regardless of the chart's own range. + it('passes the chart date range down to the metric name select', () => { + renderComponent(); + + expect(metricNameSelectProps.length).toBeGreaterThan(0); + expect(metricNameSelectProps.at(-1)?.dateRange).toEqual([ + new Date('2024-01-01'), + new Date('2024-01-02'), + ]); + }); + + it('forwards an updated date range', () => { + renderComponent({ + dateRange: [new Date('2024-06-01'), new Date('2024-06-08')], + }); + + expect(metricNameSelectProps.at(-1)?.dateRange).toEqual([ + new Date('2024-06-01'), + new Date('2024-06-08'), + ]); + }); +}); + describe('DBEditTimeChartForm - Save Button Metric Name Validation', () => { beforeEach(() => { jest.clearAllMocks(); diff --git a/packages/app/src/components/MetricNameSelect.tsx b/packages/app/src/components/MetricNameSelect.tsx index 21250c3a42..500f506131 100644 --- a/packages/app/src/components/MetricNameSelect.tsx +++ b/packages/app/src/components/MetricNameSelect.tsx @@ -1,4 +1,4 @@ -import { useMemo } from 'react'; +import { useMemo, useState } from 'react'; import { addDays, differenceInDays, subDays } from 'date-fns'; import { DateRange, @@ -6,14 +6,15 @@ import { TMetricSource, } from '@hyperdx/common-utils/dist/types'; import { Select } from '@mantine/core'; +import { useDebouncedValue } from '@mantine/hooks'; -import { useGetKeyValues } from '@/hooks/useMetadata'; +import { useGetMetricNames } from '@/hooks/useMetadata'; import { capitalizeFirstLetter } from '@/utils'; -const MAX_METRIC_NAME_OPTIONS = 3000; const SEPARATOR = ':::::::'; +const SEARCH_DEBOUNCE_MS = 300; -const chartConfigByMetricType = ({ +const metricNamesQueryArgs = ({ dateRange, metricSource, metricType, @@ -42,16 +43,12 @@ const chartConfigByMetricType = ({ } return { - // metricSource, - from: { - databaseName: metricSource.from.databaseName, - tableName: metricSource.metricTables?.[metricType] ?? '', - }, - where: '', - whereLanguage: 'sql' as const, - select: '', + databaseName: metricSource.from.databaseName, + // Empty when this source has no table for the kind, which disables the query + // rather than emitting `FROM db.``` and failing on every render. + tableName: metricSource.metricTables?.[metricType] ?? '', + connectionId: metricSource.connection, timestampValueExpression: metricSource.timestampValueExpression ?? '', - connection: metricSource.connection, dateRange: _dateRange, }; }; @@ -59,73 +56,51 @@ const chartConfigByMetricType = ({ function useMetricNames( metricSource: TMetricSource, dateRange?: DateRange['dateRange'], + namePattern?: string, ) { - const { - gaugeConfig, - histogramConfig, - sumConfig, - exponentialHistogramConfig, - } = useMemo(() => { - return { - gaugeConfig: chartConfigByMetricType({ - dateRange, - metricSource, - metricType: MetricsDataType.Gauge, - }), - histogramConfig: chartConfigByMetricType({ - dateRange, - metricSource, - metricType: MetricsDataType.Histogram, - }), - sumConfig: chartConfigByMetricType({ - dateRange, - metricSource, - metricType: MetricsDataType.Sum, - }), - exponentialHistogramConfig: chartConfigByMetricType({ - dateRange, - metricSource, - metricType: MetricsDataType.ExponentialHistogram, + const { gaugeArgs, histogramArgs, sumArgs, exponentialHistogramArgs } = + useMemo( + () => ({ + gaugeArgs: metricNamesQueryArgs({ + dateRange, + metricSource, + metricType: MetricsDataType.Gauge, + }), + histogramArgs: metricNamesQueryArgs({ + dateRange, + metricSource, + metricType: MetricsDataType.Histogram, + }), + sumArgs: metricNamesQueryArgs({ + dateRange, + metricSource, + metricType: MetricsDataType.Sum, + }), + exponentialHistogramArgs: metricNamesQueryArgs({ + dateRange, + metricSource, + metricType: MetricsDataType.ExponentialHistogram, + }), }), - }; - }, [metricSource, dateRange]); + [metricSource, dateRange], + ); - const { data: gaugeMetrics } = useGetKeyValues({ - chartConfig: gaugeConfig, - keys: ['MetricName'], - limit: MAX_METRIC_NAME_OPTIONS, - disableRowLimit: true, - }); - const { data: histogramMetrics } = useGetKeyValues({ - chartConfig: histogramConfig, - keys: ['MetricName'], - limit: MAX_METRIC_NAME_OPTIONS, - disableRowLimit: true, - }); - const { data: sumMetrics } = useGetKeyValues({ - chartConfig: sumConfig, - keys: ['MetricName'], - limit: MAX_METRIC_NAME_OPTIONS, - disableRowLimit: true, + const gauge = useGetMetricNames({ ...gaugeArgs, namePattern }); + const histogram = useGetMetricNames({ ...histogramArgs, namePattern }); + const sum = useGetMetricNames({ ...sumArgs, namePattern }); + const exponentialHistogram = useGetMetricNames({ + ...exponentialHistogramArgs, + namePattern, }); - const { data: exponentialHistogramMetrics } = useGetKeyValues( - { - chartConfig: exponentialHistogramConfig, - keys: ['MetricName'], - limit: MAX_METRIC_NAME_OPTIONS, - disableRowLimit: true, - }, - { - enabled: - !!metricSource.metricTables?.[MetricsDataType.ExponentialHistogram], - }, - ); return { - gaugeMetrics: gaugeMetrics?.[0].value, - histogramMetrics: histogramMetrics?.[0].value, - sumMetrics: sumMetrics?.[0].value, - exponentialHistogramMetrics: exponentialHistogramMetrics?.[0].value, + gaugeMetrics: gauge.data?.names, + histogramMetrics: histogram.data?.names, + sumMetrics: sum.data?.names, + exponentialHistogramMetrics: exponentialHistogram.data?.names, + isTruncated: [gauge, histogram, sum, exponentialHistogram].some( + query => query.data?.truncated, + ), }; } @@ -179,6 +154,7 @@ export function MetricNameSelect({ isLoading, isError, metricSource, + dateRange, error, onFocus, 'data-testid': dataTestId, @@ -190,16 +166,38 @@ export function MetricNameSelect({ isLoading?: boolean; isError?: boolean; metricSource: TMetricSource; + dateRange?: DateRange['dateRange']; error?: string; onFocus?: () => void; 'data-testid'?: string; }) { + const [searchValue, setSearchValue] = useState(''); + + // Mantine mirrors the selected option's *label* into a searchable Select's + // input when the selection changes, and reports it through `onSearchChange` + // exactly like typed text. Passing that on would search ClickHouse for + // "up (Gauge)", which matches nothing, so an already-configured chart would + // open to an empty list. Compared case-insensitively because the label for a + // saved exponential-histogram metric differs only in case from the one built + // for a discovered metric. + const selectedLabel = metricName + ? `${metricName} (${capitalizeFirstLetter(metricType)})` + : ''; + const trimmedSearch = searchValue.trim(); + const activeSearch = + trimmedSearch.toLowerCase() === selectedLabel.toLowerCase() + ? '' + : trimmedSearch; + + const [debouncedSearch] = useDebouncedValue(activeSearch, SEARCH_DEBOUNCE_MS); + const { gaugeMetrics, histogramMetrics, sumMetrics, exponentialHistogramMetrics, - } = useMetricNames(metricSource); + isTruncated, + } = useMetricNames(metricSource, dateRange, debouncedSearch); const options = useMemo(() => { return getMetricOptions( @@ -235,6 +233,16 @@ export function MetricNameSelect({ } data={options} limit={100} + searchValue={searchValue} + onSearchChange={setSearchValue} + // Start each browse from an empty input so the full list is offered and the + // mirrored label cannot be partially deleted and searched for; restore it + // on close so a collapsed control still shows its selection. + onDropdownOpen={() => setSearchValue('')} + onDropdownClose={() => setSearchValue(selectedLabel)} + description={ + isTruncated ? 'Too many metrics to list — type to search' : undefined + } comboboxProps={{ position: 'bottom-start', width: 'auto', diff --git a/packages/app/src/components/__tests__/MetricNameSelectSearch.test.tsx b/packages/app/src/components/__tests__/MetricNameSelectSearch.test.tsx new file mode 100644 index 0000000000..54fd3ee94b --- /dev/null +++ b/packages/app/src/components/__tests__/MetricNameSelectSearch.test.tsx @@ -0,0 +1,158 @@ +import { + MetricsDataType, + SourceKind, + TMetricSource, +} from '@hyperdx/common-utils/dist/types'; +import { screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +import { MetricNameSelect } from '@/components/MetricNameSelect'; + +// Comfortably past the component's 300ms search debounce. +const DEBOUNCE_SETTLE_MS = 800; + +const useGetMetricNames = jest.fn(); + +jest.mock('@/hooks/useMetadata', () => ({ + useGetMetricNames: (...args: any[]) => useGetMetricNames(...args), +})); + +const metricSource: TMetricSource = { + id: 'metric-source', + name: 'Metrics', + kind: SourceKind.Metric, + connection: 'conn-1', + from: { databaseName: 'default', tableName: '' }, + timestampValueExpression: 'TimeUnix', + resourceAttributesExpression: 'ResourceAttributes', + // An empty table name is how a source expresses "this kind is not + // configured", which is what disables that kind's query. + metricTables: { + gauge: 'otel_metrics_gauge', + sum: 'otel_metrics_sum', + histogram: '', + summary: '', + 'exponential histogram': '', + }, +}; + +/** Name patterns the gauge query was asked for, oldest first. */ +const gaugePatterns = () => + useGetMetricNames.mock.calls + .filter(([args]) => args.tableName === 'otel_metrics_gauge') + .map(([args]) => args.namePattern); + +const renderSelect = ( + props: Partial> = {}, +) => + renderWithMantine( + , + ); + +beforeEach(() => { + useGetMetricNames.mockReset(); + useGetMetricNames.mockImplementation(({ tableName }: any) => ({ + data: tableName + ? { names: ['group_reads', 'up'], truncated: false } + : undefined, + })); +}); + +describe('MetricNameSelect', () => { + it('passes the clamped chart date range to each metric table query', () => { + renderSelect({ + dateRange: [new Date('2024-06-01'), new Date('2024-06-08')], + }); + + const [gaugeArgs] = useGetMetricNames.mock.calls.find( + ([args]) => args.tableName === 'otel_metrics_gauge', + )!; + // Clamped to the most recent 3 days of the selected range. + expect(gaugeArgs.dateRange).toEqual([ + new Date('2024-06-05'), + new Date('2024-06-08'), + ]); + }); + + // An unconfigured kind resolves to an empty table name, which disables the + // query rather than emitting `FROM db.``` and failing on every render. + it('does not query metric kinds the source has no table for', () => { + renderSelect(); + + const tables = useGetMetricNames.mock.calls.map(([args]) => args.tableName); + + expect(tables).toContain('otel_metrics_gauge'); + expect(tables).not.toContain(undefined); + expect(tables).toContain(''); + }); + + it('sends the typed text as a server-side name pattern', async () => { + renderSelect(); + + await userEvent.type(screen.getByTestId('metric-name-selector'), 'up'); + + await waitFor(() => expect(gaugePatterns()).toContain('up')); + }); + + it('shows the selected metric in the input', async () => { + renderSelect({ metricName: 'up', metricType: MetricsDataType.Gauge }); + + await waitFor(() => + expect(screen.getByTestId('metric-name-selector')).toHaveValue( + 'up (Gauge)', + ), + ); + }); + + // Mantine mirrors the selected option's label into a searchable Select's input + // and reports it through onSearchChange exactly like typed text. Forwarding it + // would search ClickHouse for "up (Gauge)" — which matches nothing — so an + // already-configured chart would open to an empty list. + it('never sends the option label as a name pattern', async () => { + renderSelect({ metricName: 'up', metricType: MetricsDataType.Gauge }); + + // Wait until Mantine has mirrored the label in, then past the debounce, so + // this cannot pass merely by winning a race with the timer. + await waitFor(() => + expect(screen.getByTestId('metric-name-selector')).toHaveValue( + 'up (Gauge)', + ), + ); + await new Promise(resolve => setTimeout(resolve, DEBOUNCE_SETTLE_MS)); + + expect(gaugePatterns()).not.toContain('up (Gauge)'); + }); + + // Clearing on open means the mirrored label cannot be backspaced into a + // fragment like "up (Gauge" and searched for. + it('clears the mirrored label when the dropdown opens', async () => { + renderSelect({ metricName: 'up', metricType: MetricsDataType.Gauge }); + + const input = screen.getByTestId('metric-name-selector'); + await waitFor(() => expect(input).toHaveValue('up (Gauge)')); + + await userEvent.click(input); + + await waitFor(() => expect(input).toHaveValue('')); + }); + + it('tells the user to search when the catalog is truncated', () => { + useGetMetricNames.mockImplementation(({ tableName }: any) => ({ + data: tableName ? { names: ['a'], truncated: true } : undefined, + })); + + renderSelect(); + + expect( + screen.getByText('Too many metrics to list — type to search'), + ).toBeInTheDocument(); + }); +}); diff --git a/packages/app/src/hooks/useMetadata.tsx b/packages/app/src/hooks/useMetadata.tsx index 078ae5b424..9d1af765c0 100644 --- a/packages/app/src/hooks/useMetadata.tsx +++ b/packages/app/src/hooks/useMetadata.tsx @@ -8,6 +8,7 @@ import { } from '@hyperdx/common-utils/dist/clickhouse'; import { Field, + MetricNames, TableConnection, TableMetadata, } from '@hyperdx/common-utils/dist/core/metadata'; @@ -498,6 +499,73 @@ export function useGetKeyValues( ); } +/** + * List metric names for one metrics table, ordered and matched server-side. + * + * Prefer this over `useGetKeyValues({ keys: ['MetricName'] })`, which samples an + * arbitrary subset via `groupUniqArray` and can silently omit metrics on + * high-cardinality sources. + */ +export function useGetMetricNames( + { + databaseName, + tableName, + connectionId, + dateRange, + timestampValueExpression, + namePattern, + }: { + databaseName: string; + tableName: string; + connectionId: string; + dateRange: [Date, Date]; + timestampValueExpression: string; + namePattern?: string; + }, + options?: Partial>, +) { + const metadata = useMetadataWithSettings(); + return useQuery({ + queryKey: [ + 'useMetadata.useGetMetricNames', + { + databaseName, + tableName, + connectionId, + dateRange, + timestampValueExpression, + namePattern, + }, + ], + queryFn: async ({ signal }) => + metadata.getMetricNames({ + databaseName, + tableName, + connectionId, + dateRange, + timestampValueExpression, + namePattern, + signal, + }), + // An empty table name means the source has no table for this metric kind. + enabled: + !!databaseName && + !!tableName && + !!connectionId && + !!timestampValueExpression, + placeholderData: keepPreviousData, + // Four of these run per debounced keystroke, each an unbounded aggregation + // capped only by execution time. Retrying would turn one slow pattern into + // sixteen such scans, and refetching on focus would re-run them all — the + // replaced MetadataCache path served repeats from memory, so without these + // this would be busier than what it replaced. + retry: false, + staleTime: 1000 * 60 * 5, + refetchOnWindowFocus: false, + ...options, + }); +} + export function deduplicate2dArray(array2d: T[][]): T[] { // deduplicate common fields const array: T[] = []; diff --git a/packages/app/src/setupTests.tsx b/packages/app/src/setupTests.tsx index 407e51d24e..26abedda9c 100644 --- a/packages/app/src/setupTests.tsx +++ b/packages/app/src/setupTests.tsx @@ -17,6 +17,12 @@ class ResizeObserver { disconnect() {} } window.ResizeObserver = ResizeObserver; +// jsdom has no scrollIntoView, and Mantine's Combobox calls it on the active +// option when a dropdown opens — from a setTimeout, so the resulting TypeError +// surfaces as an unrelated async test failure. +if (!Element.prototype.scrollIntoView) { + Element.prototype.scrollIntoView = () => undefined; +} Object.defineProperty(window, 'matchMedia', { value: () => ({ matches: false, diff --git a/packages/common-utils/src/__tests__/metadata.int.test.ts b/packages/common-utils/src/__tests__/metadata.int.test.ts index 1d26cc77e2..06e676d0db 100644 --- a/packages/common-utils/src/__tests__/metadata.int.test.ts +++ b/packages/common-utils/src/__tests__/metadata.int.test.ts @@ -971,4 +971,162 @@ describe('Metadata Integration Tests', () => { ); }); }); + // Coverage here was logs-only, which is how a metrics-specific truncation bug + // shipped repeatedly. Runs against a table shaped like the real OTel gauge + // table holding more distinct names than one page can return. + describe('getMetricNames', () => { + let metadata: Metadata; + const GENERATED = 1000; + // Most of these merely *contain* "up". The exact `up` gauge — Prometheus' + // scrape-health metric — sorts last alphabetically, so it is the one a + // name-ordered page cannot reach. + const NOISE = [ + 'backup_size_bytes', + 'group_reads', + 'mongodb_up', + 'node_uptime_seconds', + 'up', + ]; + + const baseArgs = { + databaseName: 'default', + tableName: 'test_metrics_gauge', + connectionId: 'test_connection', + dateRange: [new Date('2023-01-01'), new Date('2025-01-01')] as [ + Date, + Date, + ], + timestampValueExpression: 'TimeUnix', + }; + + beforeAll(async () => { + await client.command({ + query: `CREATE OR REPLACE TABLE default.test_metrics_gauge ( + ServiceName LowCardinality(String), + MetricName String, + TimeUnix DateTime64(9), + Value Float64, + Attributes Map(LowCardinality(String), String) + ) + ENGINE = MergeTree() + ORDER BY (ServiceName, MetricName, toStartOfHour(TimeUnix), cityHash64(Attributes), TimeUnix) + `, + }); + await client.command({ + query: `INSERT INTO default.test_metrics_gauge + SELECT 'svc', concat('metric_', leftPad(toString(number), 5, '0')), + toDateTime64('2024-06-01 12:00:00', 9), 1, map() + FROM numbers(${GENERATED})`, + }); + await client.command({ + query: `INSERT INTO default.test_metrics_gauge + SELECT 'svc', arrayJoin([${NOISE.map(n => `'${n}'`).join(', ')}]), + toDateTime64('2024-06-01 12:00:00', 9), 1, map()`, + }); + }); + + afterAll(async () => { + await client.command({ + query: 'DROP TABLE IF EXISTS default.test_metrics_gauge', + }); + }); + + beforeEach(() => { + metadata = new Metadata(hdxClient, new MetadataCache()); + }); + + it('reports truncation instead of silently dropping names', async () => { + const result = await metadata.getMetricNames({ ...baseArgs, limit: 100 }); + + expect(result.names).toHaveLength(100); + expect(result.truncated).toBe(true); + }); + + it('returns an alphabetically ordered page when browsing', async () => { + const result = await metadata.getMetricNames({ ...baseArgs, limit: 50 }); + + expect(result.names).toEqual([...result.names].sort()); + }); + + // The reported failure mode: `up` is present and healthy, but a capped page + // cannot reach it, and the exact match must outrank the many names that + // merely contain it. + it('surfaces an exact match a capped page could not otherwise reach', async () => { + const browsing = await metadata.getMetricNames({ + ...baseArgs, + limit: 100, + }); + expect(browsing.names).not.toContain('up'); + + const searched = await metadata.getMetricNames({ + ...baseArgs, + limit: 100, + namePattern: 'up', + }); + + expect(searched.names[0]).toBe('up'); + expect(searched.names).toContain('mongodb_up'); + }); + + it('ranks prefix matches ahead of mid-string ones', async () => { + const result = await metadata.getMetricNames({ + ...baseArgs, + namePattern: 'node_', + }); + + expect(result.names[0]).toBe('node_uptime_seconds'); + }); + + it('matches namePattern case-insensitively', async () => { + const result = await metadata.getMetricNames({ + ...baseArgs, + namePattern: 'UP', + }); + + expect([...result.names].sort()).toEqual([...NOISE].sort()); + }); + + // Escaping `_` must still leave it matching a literal underscore. Asserting + // only that wildcards stop matching would also pass if the pattern were + // over-escaped into matching nothing, which would break essentially every + // Prometheus search since those names are full of underscores. + it('still matches underscores after escaping them', async () => { + const result = await metadata.getMetricNames({ + ...baseArgs, + namePattern: 'node_uptime', + }); + + expect(result.names).toEqual(['node_uptime_seconds']); + }); + + it('treats ILIKE wildcards in namePattern as literals', async () => { + const result = await metadata.getMetricNames({ + ...baseArgs, + namePattern: 'metric%000', + }); + + expect(result.names).toEqual([]); + }); + + it('returns every name when the limit exceeds the distinct count', async () => { + const result = await metadata.getMetricNames({ + ...baseArgs, + limit: GENERATED + NOISE.length + 10, + }); + + expect(result.truncated).toBe(false); + expect(result.names).toHaveLength(GENERATED + NOISE.length); + expect(result.names).toContain('up'); + }); + + it('excludes names outside the date range', async () => { + const result = await metadata.getMetricNames({ + ...baseArgs, + dateRange: [new Date('2020-01-01'), new Date('2020-12-31')], + namePattern: 'up', + }); + + expect(result.names).toEqual([]); + }); + }); }); diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index 94d4d38772..a5300b0945 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -798,6 +798,143 @@ describe('Metadata', () => { // Each of the four fetch strategies emits a distinct SQL shape (map-text- // index, native-text-index, metadata-MV, raw-table). We assert against // those shapes rather than exposing the private methods. + // Metric names must be listed deterministically. The previous implementation + // reused getKeyValues, whose `groupUniqArray(limit)(MetricName)` keeps an + // arbitrary subset once a table exceeds `limit` distinct names — that is what + // silently hid metrics such as Prometheus' `up`. + describe('getMetricNames', () => { + const baseArgs = { + databaseName: 'default', + tableName: 'otel_metrics_gauge', + connectionId: 'test_connection', + dateRange: [new Date('2024-01-01'), new Date('2024-01-02')] as [ + Date, + Date, + ], + timestampValueExpression: 'TimeUnix', + }; + + const mockNames = (names: string[]) => { + (mockClickhouseClient.query as jest.Mock).mockResolvedValue({ + json: () => + Promise.resolve({ data: names.map(MetricName => ({ MetricName })) }), + }); + }; + + const lastQuery = () => + (mockClickhouseClient.query as jest.Mock).mock.calls.at(-1)[0]; + + beforeEach(() => { + mockNames([]); + }); + + it('groups and orders by name instead of sampling', async () => { + await metadata.getMetricNames(baseArgs); + + const { query } = lastQuery(); + expect(query).toContain('GROUP BY MetricName'); + expect(query).toContain('MetricName ASC'); + expect(query).not.toContain('groupUniqArray'); + }); + + it('requests one row beyond the limit so truncation is detectable', async () => { + await metadata.getMetricNames({ ...baseArgs, limit: 100 }); + + expect(Object.values(lastQuery().query_params)).toContain(101); + }); + + it('reports truncated and trims to the requested limit', async () => { + mockNames(['a', 'b', 'c']); + + await expect( + metadata.getMetricNames({ ...baseArgs, limit: 2 }), + ).resolves.toEqual({ names: ['a', 'b'], truncated: true }); + }); + + it('reports not truncated when the page is not full', async () => { + mockNames(['a', 'b']); + + await expect( + metadata.getMetricNames({ ...baseArgs, limit: 5 }), + ).resolves.toEqual({ names: ['a', 'b'], truncated: false }); + }); + + it('applies the shared time filter', async () => { + await metadata.getMetricNames(baseArgs); + + expect(timeFilterExpr).toHaveBeenCalledWith( + expect.objectContaining({ + tableName: 'otel_metrics_gauge', + timestampValueExpression: 'TimeUnix', + dateRange: baseArgs.dateRange, + }), + ); + expect(lastQuery().query).toContain('__TIME_FILTER__'); + }); + + it('matches namePattern as a substring, server-side', async () => { + await metadata.getMetricNames({ ...baseArgs, namePattern: 'up' }); + + const { query, query_params } = lastQuery(); + expect(query).toContain('MetricName ILIKE'); + expect(Object.values(query_params)).toContain('%up%'); + }); + + it('escapes ILIKE wildcards in namePattern', async () => { + await metadata.getMetricNames({ + ...baseArgs, + namePattern: 'cpu_%usage', + }); + + expect(Object.values(lastQuery().query_params)).toContain( + '%cpu\\_\\%usage%', + ); + }); + + // Without this, a short query is crowded off the page by the many names that + // merely contain it, which is the original bug in a new form. + it('ranks exact then prefix matches ahead of the rest when searching', async () => { + await metadata.getMetricNames({ ...baseArgs, namePattern: 'up' }); + + const { query } = lastQuery(); + expect(query).toContain('lower(MetricName) = lower('); + expect(query).toContain('startsWith(lower(MetricName), lower('); + }); + + it('adds no name predicate or ranking when browsing', async () => { + await metadata.getMetricNames(baseArgs); + + expect(lastQuery().query).not.toContain('ILIKE'); + expect(lastQuery().query).not.toContain('startsWith'); + }); + + it('excludes empty names in SQL so truncation stays accurate', async () => { + await metadata.getMetricNames(baseArgs); + + expect(lastQuery().query).toContain("MetricName != ''"); + }); + + // `break` returns a partial aggregate as HTTP 200, which reads as a complete + // short list — the silent incompleteness this method replaced. + it('lets a timeout throw rather than returning a partial page', async () => { + await metadata.getMetricNames(baseArgs); + + expect(lastQuery().clickhouse_settings).toMatchObject({ + max_rows_to_read: '0', + max_execution_time: 15, + }); + expect(lastQuery().clickhouse_settings.timeout_overflow_mode).toBe( + 'throw', + ); + }); + + it('rejects a non-positive limit', async () => { + await expect( + metadata.getMetricNames({ ...baseArgs, limit: 0 }), + ).rejects.toThrow('limit must be a positive integer'); + }); + }); + describe('getAllKeyValues (router)', () => { const dateRange: [Date, Date] = [ new Date('2024-01-01'), diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index eddd1675b2..4b2b1a65b1 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -65,6 +65,21 @@ export type KeyValues = { value: string[] | number[]; }; +export type MetricNames = { + names: string[]; + /** True when more names matched than `limit`, so the page is incomplete. */ + truncated: boolean; +}; + +// Metric-name listing. See `getMetricNames`. +export const DEFAULT_METRIC_NAMES_LIMIT = 500; +const METRIC_NAMES_MAX_EXECUTION_SECONDS = 15; + +// `%` and `_` are ILIKE wildcards, so a metric name containing them (or a +// literal backslash) has to be escaped before being wrapped in `%...%`. +const escapeLikePattern = (value: string): string => + value.replace(/[\\%_]/g, char => `\\${char}`); + // See https://github.com/hyperdxio/hyperdx/issues/2163. Inlining a validated // integer literal avoids the `_CAST` wrapper entirely. const inlineNonNegativeInt = (value: number, label: string): string => { @@ -2516,6 +2531,123 @@ export class Metadata { ); } + /** + * List metric names from a single OTel metrics table. + * + * Deliberately does NOT go through `getKeyValues`: that path builds + * `groupUniqArray(limit)(MetricName)`, which keeps an *arbitrary* subset once a + * table has more than `limit` distinct names — the survivors follow hash order, + * not name order, and shift as the data and part layout change. On a + * high-cardinality source (a full Prometheus scrape) that silently hid metrics + * such as `up`, with no way to search for what had been dropped. + * + * Instead: a deterministic page, ordered by relevance to `namePattern` so an + * exact match is always on the first page, plus one extra row so callers can + * tell the list was cut off rather than having to guess. + * + * Not cached in `MetadataCache`: that is an unbounded module-level Map with no + * TTL, and this is the only lookup keyed on free-form user input. Callers + * should cache through react-query, whose `gcTime` bounds it. + */ + async getMetricNames({ + databaseName, + tableName, + connectionId, + dateRange, + timestampValueExpression, + namePattern, + limit = DEFAULT_METRIC_NAMES_LIMIT, + signal, + }: { + databaseName: string; + tableName: string; + connectionId: string; + dateRange: [Date, Date]; + timestampValueExpression: string; + namePattern?: string; + limit?: number; + signal?: AbortSignal; + }): Promise { + if (!Number.isInteger(limit) || limit < 1) { + throw new Error( + `limit must be a positive integer, got: ${String(limit)}`, + ); + } + + // Reuse the shared time filter so `timestampValueExpression` and the + // primary-key/partition pruning optimizations keep applying. + const timeFilter = await timeFilterExpr({ + connectionId, + databaseName, + dateRange, + dateRangeEndInclusive: true, + dateRangeStartInclusive: true, + metadata: this, + tableName, + timestampValueExpression, + }); + + // Excluded in SQL rather than after the fact: an empty name sorts first, so + // filtering it client-side would consume the extra row and under-report + // `truncated`. + const whereParts: ChSql[] = [timeFilter, chSql`MetricName != ''`]; + if (namePattern) { + whereParts.push( + chSql`MetricName ILIKE ${{ + String: `%${escapeLikePattern(namePattern)}%`, + }}`, + ); + } + + // Relevance first when searching, so a short query like `up` cannot be + // crowded off the page by the many names that merely contain it + // (`group_reads`, `node_uptime_seconds`, ...). Ordering here rather than in + // the client keeps the page we return the page worth showing. + const orderBy = namePattern + ? chSql`lower(MetricName) = lower(${{ String: namePattern }}) DESC, + startsWith(lower(MetricName), lower(${{ String: namePattern }})) DESC, + MetricName ASC` + : chSql`MetricName ASC`; + + const sql = chSql` + SELECT MetricName + FROM ${tableExpr({ database: databaseName, table: tableName })} + WHERE ${concatChSql(' AND ', whereParts)} + GROUP BY MetricName + ORDER BY ${orderBy} + LIMIT ${{ Int32: limit + 1 }} + `; + + const names = await this.clickhouseClient + .query<'JSON'>({ + query: sql.sql, + query_params: sql.params, + connectionId, + clickhouse_settings: { + ...this.getClickHouseSettings(), + // `MetricName` is part of the sorting key on the OTel metrics tables, + // but the aggregate still has to scan the time range, so a row cap + // would error instead of returning the page — bound by time instead. + max_rows_to_read: '0', + max_execution_time: METRIC_NAMES_MAX_EXECUTION_SECONDS, + // Pinned, not merely left unset: `break` returns a partial aggregate + // as HTTP 200, i.e. a short list reporting `truncated: false`, which + // is the silent incompleteness this method exists to remove. The + // spread above is a mutable process-wide bag, so relying on absence + // would let a server profile reintroduce it. + timeout_overflow_mode: 'throw', + }, + abort_signal: signal, + }) + .then(res => res.json<{ MetricName: string }>()) + .then(d => d.data.map(row => row.MetricName)); + + return { + names: names.slice(0, limit), + truncated: names.length > limit, + }; + } + async getKeyValuesWithMVs({ chartConfig, keys, From cabb81ee89c32c93755c7baef6382f091186776f Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Wed, 29 Jul 2026 13:26:35 -0400 Subject: [PATCH 2/3] fix(app): surface metric kinds that failed to load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/app/src/components/MetricNameSelect.tsx | 15 ++++++++++++++- .../__tests__/MetricNameSelectSearch.test.tsx | 16 ++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/packages/app/src/components/MetricNameSelect.tsx b/packages/app/src/components/MetricNameSelect.tsx index 500f506131..ea028a0a91 100644 --- a/packages/app/src/components/MetricNameSelect.tsx +++ b/packages/app/src/components/MetricNameSelect.tsx @@ -101,6 +101,12 @@ function useMetricNames( isTruncated: [gauge, histogram, sum, exponentialHistogram].some( query => query.data?.truncated, ), + // Surfaced because a failed kind otherwise just vanishes from the list: the + // query is not retried, so a transient error or a query too slow to finish + // would silently omit those metrics from an apparently healthy dropdown. + hasError: [gauge, histogram, sum, exponentialHistogram].some( + query => query.isError, + ), }; } @@ -197,6 +203,7 @@ export function MetricNameSelect({ sumMetrics, exponentialHistogramMetrics, isTruncated, + hasError, } = useMetricNames(metricSource, dateRange, debouncedSearch); const options = useMemo(() => { @@ -240,8 +247,14 @@ export function MetricNameSelect({ // on close so a collapsed control still shows its selection. onDropdownOpen={() => setSearchValue('')} onDropdownClose={() => setSearchValue(selectedLabel)} + // Reported in the description rather than the `error` slot, which belongs + // to form validation for this field. description={ - isTruncated ? 'Too many metrics to list — type to search' : undefined + hasError + ? 'Some metrics could not be loaded' + : isTruncated + ? 'Too many metrics to list — type to search' + : undefined } comboboxProps={{ position: 'bottom-start', diff --git a/packages/app/src/components/__tests__/MetricNameSelectSearch.test.tsx b/packages/app/src/components/__tests__/MetricNameSelectSearch.test.tsx index 54fd3ee94b..2a2aac2642 100644 --- a/packages/app/src/components/__tests__/MetricNameSelectSearch.test.tsx +++ b/packages/app/src/components/__tests__/MetricNameSelectSearch.test.tsx @@ -144,6 +144,22 @@ describe('MetricNameSelect', () => { await waitFor(() => expect(input).toHaveValue('')); }); + // A failed kind is not retried, so without this its metrics would simply be + // missing from a dropdown that looks perfectly healthy. + it('reports a metric kind that failed to load', () => { + useGetMetricNames.mockImplementation(({ tableName }: any) => + tableName === 'otel_metrics_sum' + ? { data: undefined, isError: true } + : { data: tableName ? { names: ['up'], truncated: false } : undefined }, + ); + + renderSelect(); + + expect( + screen.getByText('Some metrics could not be loaded'), + ).toBeInTheDocument(); + }); + it('tells the user to search when the catalog is truncated', () => { useGetMetricNames.mockImplementation(({ tableName }: any) => ({ data: tableName ? { names: ['a'], truncated: true } : undefined, From 313259d4e0a62ff094cf0df8b5af182f291df9b8 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Wed, 29 Jul 2026 14:00:27 -0400 Subject: [PATCH 3/3] fix(app): inherit the configured query timeout for metric-name listing 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. --- .../common-utils/src/__tests__/metadata.test.ts | 10 ++++++---- packages/common-utils/src/core/metadata.ts | 14 ++++++++------ 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index a5300b0945..19ea5a72fc 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -921,11 +921,13 @@ describe('Metadata', () => { expect(lastQuery().clickhouse_settings).toMatchObject({ max_rows_to_read: '0', - max_execution_time: 15, + timeout_overflow_mode: 'throw', }); - expect(lastQuery().clickhouse_settings.timeout_overflow_mode).toBe( - 'throw', - ); + // Left to the client so the deployment's configured query timeout applies, + // matching the bound this path had before. + expect( + lastQuery().clickhouse_settings.max_execution_time, + ).toBeUndefined(); }); it('rejects a non-positive limit', async () => { diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 4b2b1a65b1..d9dd9027a9 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -73,7 +73,6 @@ export type MetricNames = { // Metric-name listing. See `getMetricNames`. export const DEFAULT_METRIC_NAMES_LIMIT = 500; -const METRIC_NAMES_MAX_EXECUTION_SECONDS = 15; // `%` and `_` are ILIKE wildcards, so a metric name containing them (or a // literal backslash) has to be escaped before being wrapped in `%...%`. @@ -2625,16 +2624,19 @@ export class Metadata { connectionId, clickhouse_settings: { ...this.getClickHouseSettings(), - // `MetricName` is part of the sorting key on the OTel metrics tables, - // but the aggregate still has to scan the time range, so a row cap - // would error instead of returning the page — bound by time instead. + // Bounded by wall clock, not rows: a row cap under `ORDER BY ... LIMIT` + // is what made the old result an arbitrary subset, so capping rows here + // would reintroduce the bug this method removes. `max_execution_time` + // is deliberately left unset so the client applies the deployment's + // configured query timeout — the same effective bound this path had + // before. Superseded searches abort through `signal`, so only the + // latest pattern is ever in flight. max_rows_to_read: '0', - max_execution_time: METRIC_NAMES_MAX_EXECUTION_SECONDS, // Pinned, not merely left unset: `break` returns a partial aggregate // as HTTP 200, i.e. a short list reporting `truncated: false`, which // is the silent incompleteness this method exists to remove. The // spread above is a mutable process-wide bag, so relying on absence - // would let a server profile reintroduce it. + // would let a deployment profile reintroduce it. timeout_overflow_mode: 'throw', }, abort_signal: signal,