Repository navigation
fix(app): UX Enhancements to metrics dropdown #3054
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
ef5308f
61e5ed6
4a4f716
1d8af03
1651193
72faf3c
79cc615
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,5 @@ | ||
| --- | ||
| '@hyperdx/app': patch | ||
| --- | ||
|
|
||
| Fix two problems with the chart editor's metric name picker. The truncation and load-failure notices now render below the input instead of above it, so they no longer push the field down out of alignment with the browse-metrics button beside it, and both were shortened to fit the tile editor's column without wrapping. A search that matches nothing now says so rather than silently hiding the dropdown, and offers the searched name as a selectable option — the catalog only covers the most recent three days of the chart's time range, so a metric that stopped reporting was previously impossible to chart by name. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,4 @@ | ||
| import { useMemo, useState } from 'react'; | ||
| import { addDays, differenceInDays, subDays } from 'date-fns'; | ||
| import { | ||
| DateRange, | ||
| MetricsDataType, | ||
|
|
@@ -8,155 +7,42 @@ import { | |
| import { Select } from '@mantine/core'; | ||
| import { useDebouncedValue } from '@mantine/hooks'; | ||
|
|
||
| import { useGetMetricNames } from '@/hooks/useMetadata'; | ||
| import { capitalizeFirstLetter } from '@/utils'; | ||
| import { useMetricNames } from '@/hooks/useMetricNames'; | ||
| import { | ||
| metricKindLabel, | ||
| QUERYABLE_KINDS, | ||
| type QueryableMetricKind, | ||
| } from '@/utils/metricKinds'; | ||
|
|
||
| const SEPARATOR = ':::::::'; | ||
| const SEARCH_DEBOUNCE_MS = 300; | ||
|
|
||
| const metricNamesQueryArgs = ({ | ||
| dateRange, | ||
| metricSource, | ||
| metricType, | ||
| }: { | ||
| dateRange?: DateRange['dateRange']; | ||
| metricSource: TMetricSource; | ||
| metricType: MetricsDataType; | ||
| }) => { | ||
| // eslint-disable-next-line no-restricted-syntax | ||
| const now = new Date(); | ||
| let _dateRange: DateRange['dateRange'] = dateRange | ||
| ? dateRange | ||
| : [subDays(now, 1), now]; | ||
| const diffInDays = differenceInDays(_dateRange[1], _dateRange[0]); | ||
|
|
||
| if (diffInDays < 1) { | ||
| const nextDay = addDays(_dateRange[0], 1); | ||
| if (nextDay > now) { | ||
| _dateRange = [subDays(_dateRange[1], 1), _dateRange[1]]; | ||
| } else { | ||
| _dateRange = [_dateRange[0], nextDay]; | ||
| } | ||
| } else if (diffInDays > 3) { | ||
| // most recent 3 days | ||
| _dateRange = [subDays(_dateRange[1], 3), _dateRange[1]]; | ||
| } | ||
|
|
||
| return { | ||
| 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 ?? '', | ||
| dateRange: _dateRange, | ||
| }; | ||
| }; | ||
|
|
||
| /** | ||
| * Metric names available on a metric source, one list per queryable kind. | ||
| * | ||
| * Names only, and deliberately so: it backs the always-mounted select, where a | ||
| * grouped scan for unit/description would be too heavy. The explorer pays for | ||
| * that richer catalog itself via `useMetricCatalog`, only while it is open. | ||
| */ | ||
| function useMetricNames( | ||
| metricSource: TMetricSource, | ||
| dateRange?: DateRange['dateRange'], | ||
| namePattern?: string, | ||
| ) { | ||
| 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], | ||
| ); | ||
|
|
||
| const gauge = useGetMetricNames({ ...gaugeArgs, namePattern }); | ||
| const histogram = useGetMetricNames({ ...histogramArgs, namePattern }); | ||
| const sum = useGetMetricNames({ ...sumArgs, namePattern }); | ||
| const exponentialHistogram = useGetMetricNames({ | ||
| ...exponentialHistogramArgs, | ||
| namePattern, | ||
| }); | ||
|
|
||
| return { | ||
| 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, | ||
| ), | ||
| }; | ||
| } | ||
| const metricOptionValue = (metricName: string, metricType: MetricsDataType) => | ||
| `${metricName}${SEPARATOR}${metricType}`; | ||
|
|
||
| export function getMetricOptions( | ||
| gaugeMetrics: string[] | undefined, | ||
| histogramMetrics: string[] | undefined, | ||
| sumMetrics: string[] | undefined, | ||
| exponentialHistogramMetrics: string[] | undefined, | ||
| namesByKind: Record<QueryableMetricKind, string[] | undefined>, | ||
| metricName: string | null | undefined, | ||
| metricType: MetricsDataType, | ||
| ) { | ||
| const metricsFromQuery = [ | ||
| ...(gaugeMetrics?.map(metric => ({ | ||
| value: `${metric}${SEPARATOR}${MetricsDataType.Gauge}`, | ||
| label: `${metric} (Gauge)`, | ||
| })) ?? []), | ||
| ...(histogramMetrics?.map(metric => ({ | ||
| value: `${metric}${SEPARATOR}${MetricsDataType.Histogram}`, | ||
| label: `${metric} (Histogram)`, | ||
| })) ?? []), | ||
| ...(sumMetrics?.map(metric => ({ | ||
| value: `${metric}${SEPARATOR}${MetricsDataType.Sum}`, | ||
| label: `${metric} (Sum)`, | ||
| })) ?? []), | ||
| ...(exponentialHistogramMetrics?.map(metric => ({ | ||
| value: `${metric}${SEPARATOR}${MetricsDataType.ExponentialHistogram}`, | ||
| label: `${metric} (Exponential Histogram)`, | ||
| })) ?? []), | ||
| ]; | ||
| const metricsFromQuery = QUERYABLE_KINDS.flatMap( | ||
| kind => | ||
| namesByKind[kind]?.map(metric => ({ | ||
| value: metricOptionValue(metric, kind), | ||
| label: `${metric} (${metricKindLabel(kind)})`, | ||
| })) ?? [], | ||
| ); | ||
| // if saved metric does not exist in the available options, assume it exists | ||
| // and add it to options | ||
| if ( | ||
| metricName && | ||
| !metricsFromQuery.find( | ||
| metric => metric.value === `${metricName}${SEPARATOR}${metricType}`, | ||
| metric => metric.value === metricOptionValue(metricName, metricType), | ||
| ) | ||
| ) { | ||
| metricsFromQuery.push({ | ||
| value: `${metricName}${SEPARATOR}${metricType}`, | ||
| label: `${metricName} (${capitalizeFirstLetter(metricType)})`, | ||
| value: metricOptionValue(metricName, metricType), | ||
| label: `${metricName} (${metricKindLabel(metricType)})`, | ||
| }); | ||
| } | ||
| return metricsFromQuery; | ||
|
|
@@ -193,11 +79,11 @@ export function MetricNameSelect({ | |
| // 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. | ||
| // open to an empty list. Built from the same label helper as the options, so | ||
| // the two always agree; compared case-insensitively as a cheap guard against | ||
| // that drifting again. | ||
| const selectedLabel = metricName | ||
| ? `${metricName} (${capitalizeFirstLetter(metricType)})` | ||
| ? `${metricName} (${metricKindLabel(metricType)})` | ||
| : ''; | ||
| const trimmedSearch = searchValue.trim(); | ||
| const activeSearch = | ||
|
|
@@ -206,32 +92,62 @@ export function MetricNameSelect({ | |
| : trimmedSearch; | ||
|
|
||
| const [debouncedSearch] = useDebouncedValue(activeSearch, SEARCH_DEBOUNCE_MS); | ||
| // The debounce has not fired yet, so the options on screen answer the | ||
| // previous text and no query is in flight for the current one. | ||
| const isSearchPending = activeSearch !== debouncedSearch; | ||
|
|
||
| const { | ||
| gaugeMetrics, | ||
| histogramMetrics, | ||
| sumMetrics, | ||
| exponentialHistogramMetrics, | ||
| namesByKind, | ||
| isTruncated, | ||
| hasError, | ||
| hasNoMatches, | ||
| isFetching: isSearching, | ||
| } = useMetricNames(metricSource, dateRange, debouncedSearch); | ||
|
|
||
| const options = useMemo(() => { | ||
| return getMetricOptions( | ||
| gaugeMetrics, | ||
| histogramMetrics, | ||
| sumMetrics, | ||
| exponentialHistogramMetrics, | ||
| metricName, | ||
| metricType, | ||
| ); | ||
| const metricOptions = getMetricOptions(namesByKind, metricName, metricType); | ||
| // A name missing from the picker is not necessarily missing from the data: | ||
| // the catalog query covers only the most recent 3 days of the chart's range, | ||
| // and a kind with no table configured is never queried. Offer the searched | ||
| // name so a pasted metric is not stranded — the select has no other way to | ||
| // commit a value that is not already an option. Built from the debounced | ||
| // search, so an offer appears only once the query behind it has answered, | ||
| // and the name is embedded in the label because Mantine filters options by | ||
| // substring against the label and would otherwise drop these. | ||
| // | ||
| // One offer per kind, because the kind decides which table the series is | ||
| // read from and a pasted name carries no kind. Filing it under whatever the | ||
| // last selection happened to use would query the wrong table and chart | ||
| // nothing, with no way to correct it. | ||
| if (debouncedSearch && hasNoMatches) { | ||
|
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 — The "no recent data" escape hatch is suppressed whenever any sibling name merely contains the typed text
|
||
| for (const kind of QUERYABLE_KINDS) { | ||
| // A kind with no table is not queryable on this source, so committing | ||
| // the name under it could never resolve. | ||
| if (!metricSource.metricTables?.[kind]) { | ||
| continue; | ||
| } | ||
| const value = metricOptionValue(debouncedSearch, kind); | ||
| // Committing an offer puts the name in `metricName`, which | ||
| // `getMetricOptions` then synthesizes its own option for while the | ||
| // debounced search still holds the same name — and Mantine throws on a | ||
| // duplicate option value, taking the editor down with it. | ||
| if (metricOptions.some(option => option.value === value)) { | ||
| continue; | ||
| } | ||
| metricOptions.push({ | ||
| value, | ||
| label: `${debouncedSearch} (${metricKindLabel(kind)}, no recent data)`, | ||
| }); | ||
| } | ||
| } | ||
| return metricOptions; | ||
| }, [ | ||
| gaugeMetrics, | ||
| histogramMetrics, | ||
| sumMetrics, | ||
| exponentialHistogramMetrics, | ||
| namesByKind, | ||
| metricName, | ||
| metricType, | ||
| debouncedSearch, | ||
| hasNoMatches, | ||
| metricSource, | ||
| ]); | ||
|
|
||
| const currentValue = | ||
|
|
@@ -257,13 +173,27 @@ export function MetricNameSelect({ | |
| // on close so a collapsed control still shows its selection. | ||
| onDropdownOpen={() => setSearchValue('')} | ||
| onDropdownClose={() => setSearchValue(selectedLabel)} | ||
| // Without a message Mantine sets `hiddenWhenEmpty`, so a search matching | ||
| // nothing hides the whole dropdown and the field just looks broken. | ||
| nothingFoundMessage={ | ||
|
teeohhem marked this conversation as resolved.
Resolved
teeohhem marked this conversation as resolved.
Resolved
|
||
| isSearchPending || isSearching | ||
| ? 'Searching…' | ||
| : hasError | ||
| ? 'Some metrics failed to load' | ||
| : activeSearch | ||
| ? 'No matching metrics' | ||
|
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 When a search matches nothing on a source with ≥1 configured |
||
| : 'No metrics reported recently' | ||
| } | ||
| // Reported in the description rather than the `error` slot, which belongs | ||
| // to form validation for this field. | ||
| // to form validation for this field. Kept below the input: it appears | ||
| // mid-session, and above the input it pushes the field down out of | ||
| // alignment with the browse-metrics button beside it. | ||
| inputWrapperOrder={['label', 'input', 'description', 'error']} | ||
| description={ | ||
| hasError | ||
| ? 'Some metrics could not be loaded' | ||
| ? 'Some metrics failed to load' | ||
| : isTruncated | ||
| ? 'Too many metrics to list — type to search' | ||
| ? 'Type to search all metrics' | ||
| : undefined | ||
| } | ||
| comboboxProps={{ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to a separate hook file