Repository navigation
fix(app): list metric names deterministically instead of sampling #2747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0b32016
cabb81e
313259d
a20d251
e60f84d
ece4976
ea8e5e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 |
|---|---|---|
| @@ -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, | ||
|
|
@@ -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( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor —
|
||
| 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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
|
||
| 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={ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — The new Mantine's default |
||
| hasError | ||
| ? 'Some metrics could not be loaded' | ||
| : isTruncated | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 major —
|
||
| ? 'Too many metrics to list — type to search' | ||
| : undefined | ||
| } | ||
| comboboxProps={{ | ||
| position: 'bottom-start', | ||
| width: 'auto', | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.