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 e85711eb6e..0a96bb8d1a 100644 --- a/packages/app/src/components/DBEditTimeChartForm/ChartSeriesEditor.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/ChartSeriesEditor.tsx @@ -454,6 +454,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 846207f6bd..9d0c77a6f1 100644 --- a/packages/app/src/components/DBEditTimeChartForm/__tests__/DBEditTimeChartForm.test.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/__tests__/DBEditTimeChartForm.test.tsx @@ -80,6 +80,9 @@ 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[] = []; + // What the stubbed explorer reports as staged when a metric is applied. The // `mock` prefix is required for a jest.mock factory to close over it. const mockStagedWhere: string[] = []; @@ -133,6 +136,7 @@ jest.mock('@/components/MetricExplorer/MetricExplorerModal', () => ({ jest.mock('../../MetricNameSelect', () => ({ MetricNameSelect: (props: any) => { + metricNameSelectProps.push(props); const { error, onFocus, setMetricName, metricName } = props; const testId = props['data-testid']; return ( @@ -446,6 +450,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 a94b509c93..ddf31cb617 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, }; }; @@ -66,82 +63,60 @@ 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, isLoading: isGaugeLoading } = useGetKeyValues({ - chartConfig: gaugeConfig, - keys: ['MetricName'], - limit: MAX_METRIC_NAME_OPTIONS, - disableRowLimit: true, - }); - const { data: histogramMetrics, isLoading: isHistogramLoading } = - useGetKeyValues({ - chartConfig: histogramConfig, - keys: ['MetricName'], - limit: MAX_METRIC_NAME_OPTIONS, - disableRowLimit: true, - }); - const { data: sumMetrics, isLoading: isSumLoading } = 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, - isLoading: isExponentialHistogramLoading, - } = 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, - isLoading: - isGaugeLoading || - isHistogramLoading || - isSumLoading || - isExponentialHistogramLoading, + 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, + ), + // 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, + ), + isLoading: [gauge, histogram, sum, exponentialHistogram].some( + query => query.isLoading, + ), }; } @@ -195,6 +170,7 @@ export function MetricNameSelect({ isLoading, isError, metricSource, + dateRange, error, onFocus, 'data-testid': dataTestId, @@ -206,16 +182,39 @@ 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, + hasError, + } = useMetricNames(metricSource, dateRange, debouncedSearch); const options = useMemo(() => { return getMetricOptions( @@ -251,6 +250,22 @@ 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)} + // Reported in the description rather than the `error` slot, which belongs + // to form validation for this field. + description={ + hasError + ? 'Some metrics could not be loaded' + : 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..2a2aac2642 --- /dev/null +++ b/packages/app/src/components/__tests__/MetricNameSelectSearch.test.tsx @@ -0,0 +1,174 @@ +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('')); + }); + + // 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, + })); + + 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 521d3370f0..11229094ee 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'; @@ -519,6 +520,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..44b7e37585 100644 --- a/packages/common-utils/src/__tests__/metadata.int.test.ts +++ b/packages/common-utils/src/__tests__/metadata.int.test.ts @@ -971,4 +971,177 @@ 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('ranks mid-string matches by how early the match occurs', async () => { + const result = await metadata.getMetricNames({ + ...baseArgs, + namePattern: 'up', + }); + + expect(result.names).toEqual([ + 'up', // exact + 'group_reads', // position 4 + 'backup_size_bytes', // position 5 + 'node_uptime_seconds', // position 6 + 'mongodb_up', // position 9 + ]); + }); + + 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 8be634c4ba..7e86d83ff1 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -877,6 +877,145 @@ 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 matches then earlier occurrences when searching', async () => { + await metadata.getMetricNames({ ...baseArgs, namePattern: 'up' }); + + const { query } = lastQuery(); + expect(query).toContain('lower(MetricName) = lower('); + expect(query).toContain('positionCaseInsensitive(MetricName,'); + }); + + 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('positionCaseInsensitive'); + }); + + 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', + timeout_overflow_mode: '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 () => { + 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 d9e96e35e0..37309be73b 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -71,6 +71,20 @@ 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; + +// `%` 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 => { @@ -2601,6 +2615,128 @@ 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. Position + // ranks earlier occurrences higher and covers prefix matches (position 1), + // so exact → earliest occurrence → alphabetical. + const orderBy = namePattern + ? chSql`lower(MetricName) = lower(${{ String: namePattern }}) DESC, + positionCaseInsensitive(MetricName, ${{ String: namePattern }}) ASC, + 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(), + // 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', + // 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 deployment 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,