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
5 changes: 5 additions & 0 deletions .changeset/metric-name-select-not-found.md
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
Expand Up @@ -482,15 +482,13 @@ export function ChartSeriesEditor({
onApply={applyExplorerMetric}
/>
{metricType === 'gauge' && (
<Flex justify="end">
<CheckBoxControlled
control={control}
name={`${namePrefix}isDelta`}
label="Delta"
size="xs"
className="mt-2"
/>
</Flex>
<CheckBoxControlled
control={control}
name={`${namePrefix}isDelta`}
label="Delta"
size="xs"
className="mt-2"
/>
)}
</div>
)}
Expand Down
242 changes: 86 additions & 156 deletions packages/app/src/components/MetricNameSelect.tsx
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,
Expand All @@ -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}`;

Copy link
Copy Markdown
Contributor Author

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

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;
Expand Down Expand Up @@ -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 =
Expand All @@ -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) {

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 — The "no recent data" escape hatch is suppressed whenever any sibling name merely contains the typed text

hasNoMatches (packages/app/src/hooks/useMetricNames.ts:111) is true only when every kind returned zero names, and the server match is a substring ILIKE %pattern% (getMetricNames in packages/common-utils/src/core/metadata.ts:2714). So if the user pastes kube_pod_status_phase (stopped reporting 5 days ago) while kube_pod_status_phase_total still reports, one name comes back, hasNoMatches is false, no offer is appended — and Mantine still cannot commit a value that is not an option, so the exact name stays unchartable. _total/_count/_sum/_bucket siblings make this the common case rather than the exception. Gate the offer on "no returned name equals debouncedSearch case-insensitively" rather than "no returned names at all" — the query already sorts exact matches first, so expose e.g. hasExactMatch from useMetricNames and use !hasExactMatch here.

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 =
Expand All @@ -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={
Comment thread
teeohhem marked this conversation as resolved.
Resolved
Comment thread
teeohhem marked this conversation as resolved.
Resolved
isSearchPending || isSearching
? 'Searching…'
: hasError
? 'Some metrics failed to load'
: activeSearch
? 'No matching metrics'

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 'No matching metrics' branch is unreachable for any source that has a metric table configured

When a search matches nothing on a source with ≥1 configured metricTables entry, hasNoMatches is true and an offer is appended, so the option list is non-empty and Mantine never renders nothingFoundMessage. The remaining ways to reach an empty list are: search still debouncing (→ 'Searching…'), all kinds errored (→ 'Some metrics failed to load'), or all kinds' queries disabled, i.e. no tables at all. The only test covering this string (MetricNameSelectSearch.test.tsx:361) has to blank out all five metricTables entries to hit it, which is why the gap isn't visible. Either drop the branch or make it reachable by showing it alongside the offers (e.g. as the dropdown's leading text) so a real user searching a configured source is told the search didn't match.

: '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={{
Expand Down
Loading
Loading