Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/deterministic-metric-name-listing.md
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
Expand Up @@ -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`)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = [];
Expand Down Expand Up @@ -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 (
Expand Down Expand Up @@ -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();
Expand Down
183 changes: 99 additions & 84 deletions packages/app/src/components/MetricNameSelect.tsx
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
import { useMemo } from 'react';
import { useMemo, useState } from 'react';
import { addDays, differenceInDays, subDays } from 'date-fns';
import {
DateRange,
MetricsDataType,
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,
Expand Down Expand Up @@ -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,
};
};
Expand All @@ -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,
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Resolved
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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — useMetricNames().isLoading is computed but never consumed

MetricNameSelect destructures only isTruncated/hasError; its own isLoading prop shadows the hook's. Either drop it from the return value or use it — now that every keystroke refetches, the dropdown shows the previous list with no indication a search is in flight (e.g. rightSection={<Loader size="xs"/>}).

query => query.isLoading,
),
};
}

Expand Down Expand Up @@ -195,6 +170,7 @@ export function MetricNameSelect({
isLoading,
isError,
metricSource,
dateRange,
error,
onFocus,
'data-testid': dataTestId,
Expand All @@ -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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 major — Typed search is wiped by Mantine's label mirroring whenever a metric is already selected

Mantine's Select mirrors selectedOption.label into the search value from an effect keyed on the selected option object, and that object is rebuilt every time the data array identity changes — which is exactly what happens when one of the four useGetMetricNames queries resolves. So on an existing series (metricName set), typing cpu fires the debounced query, and when it returns the input text is replaced with up (Gauge); activeSearch then collapses to '' and the next debounce fires a browse query, discarding the search. The activeSearch guard keeps the label out of the SQL but not out of the input or Mantine's own client-side filter. Own the input text here (reset it from a useEffect on [metricName, metricType], or drop to Combobox+InputBase) instead of relying on Select's mirroring, and add a test that types while metricName is set — the new test file only types with metricName={null}, where value === null makes that effect a no-op, so this path is untested.


const {
gaugeMetrics,
histogramMetrics,
sumMetrics,
exponentialHistogramMetrics,
} = useMetricNames(metricSource);
isTruncated,
hasError,
} = useMetricNames(metricSource, dateRange, debouncedSearch);

const options = useMemo(() => {
return getMetricOptions(
Expand Down Expand Up @@ -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={

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — The new description shifts the input down and misaligns the adjacent "Browse metrics" button

Mantine's default inputWrapperOrder renders description above the input, so when the truncation/error text appears the input drops ~20px while the ActionIcon beside it in <Group align="start"> (ChartSeriesEditor.tsx:444) stays put — and in a 220px column the copy wraps to two lines. Render the notice below the control (e.g. inputWrapperOrder={['label','input','description','error']}) or as sibling Text outside the Group so the row stays aligned.

hasError
? 'Some metrics could not be loaded'
: isTruncated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 major — isTruncated reports the server page, not the rendered list, so a cut-off dropdown claims to be complete

DEFAULT_METRIC_NAMES_LIMIT is 500 per kind (up to 2000 options) but limit={100} on line 252 renders only the first 100, gauge-first. A source with 150 gauges and any sums returns truncated: false from all four queries, so no description is shown while the user is in fact browsing a silently truncated, gauge-only list — the same "list looks whole but isn't" failure this PR removes from the SQL layer. Drive the message from what is rendered (e.g. isTruncated || options.length > SELECT_LIMIT) or raise the Select limit to match the server limit.

? 'Too many metrics to list — type to search'
: undefined
}
comboboxProps={{
position: 'bottom-start',
width: 'auto',
Expand Down
Loading
Loading