mirror of
https://github.com/hyperdxio/hyperdx
synced 2026-04-21 13:37:15 +00:00
fix: Prevent dashboard error when metricName is defined for non-metric source (#1649)
Closes HDX-3236 # Summary This PR fixes an error that occurs when a metricName/metricType is set for a dashboard tile configuration, despite the queried source not being a metric source. 1. Updates in DBEditTimeChartForm prevent us from saving configurations with metricName/metricType for non metric sources 2. Updates in DBDashboardPage ensure that metricName/metricType is ignored for any saved configurations for non-metric sources. ## Demo A new tile would be saved with a metricName/Type incorrectly when 1. Create the tile 2. Select a metric source 3. Select a metric name 4. Switch back to a non-metric source 5. Save And the Dashboard tile would then error: <img width="1288" height="1012" alt="Screenshot 2026-01-23 at 2 39 38 PM" src="https://github.com/user-attachments/assets/4fa4b0bf-355e-47bb-a504-cd03e0dca2d0" /> Now, the configuration is not saved with metricName/Type, and the dashboard does not error for a saved configuration that has a metricName/Type: <img width="769" height="423" alt="Screenshot 2026-01-23 at 2 43 04 PM" src="https://github.com/user-attachments/assets/92af36aa-dd46-47b8-ae59-d0e4bfcb28af" />
This commit is contained in:
parent
cf3ebb4bfc
commit
b2089fa998
3 changed files with 57 additions and 25 deletions
5
.changeset/short-camels-hug.md
Normal file
5
.changeset/short-camels-hug.md
Normal file
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
"@hyperdx/app": patch
|
||||
---
|
||||
|
||||
fix: Prevent dashboard error when metricName is defined for non-metric source
|
||||
|
|
@ -20,6 +20,7 @@ import { convertToDashboardTemplate } from '@hyperdx/common-utils/dist/core/util
|
|||
import {
|
||||
AlertState,
|
||||
DashboardFilter,
|
||||
SourceKind,
|
||||
TSourceUnion,
|
||||
} from '@hyperdx/common-utils/dist/types';
|
||||
import {
|
||||
|
|
@ -182,10 +183,14 @@ const Tile = forwardRef(
|
|||
|
||||
useEffect(() => {
|
||||
if (source != null) {
|
||||
const isMetricSource = source.kind === SourceKind.Metric;
|
||||
|
||||
// TODO: will need to update this when we allow for multiple metrics per chart
|
||||
const firstSelect = chart.config.select[0];
|
||||
const metricType =
|
||||
typeof firstSelect !== 'string' ? firstSelect?.metricType : undefined;
|
||||
isMetricSource && typeof firstSelect !== 'string'
|
||||
? firstSelect?.metricType
|
||||
: undefined;
|
||||
const tableName = getMetricTableName(source, metricType);
|
||||
if (source.connection) {
|
||||
setQueriedConfig({
|
||||
|
|
@ -200,7 +205,7 @@ const Tile = forwardRef(
|
|||
},
|
||||
implicitColumnExpression: source.implicitColumnExpression,
|
||||
filters,
|
||||
metricTables: source.metricTables,
|
||||
metricTables: isMetricSource ? source.metricTables : undefined,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -135,6 +135,29 @@ const getSeriesFieldPath = (
|
|||
return `${namePrefix}${fieldName}` as FieldPath<SavedChartConfigWithSeries>;
|
||||
};
|
||||
|
||||
export function normalizeChartConfig<
|
||||
C extends Pick<
|
||||
SavedChartConfig,
|
||||
'select' | 'having' | 'orderBy' | 'displayType' | 'metricTables'
|
||||
>,
|
||||
>(config: C, source: TSource): C {
|
||||
const isMetricSource = source.kind === SourceKind.Metric;
|
||||
return {
|
||||
...config,
|
||||
// Strip out metric-specific fields for non-metric sources
|
||||
select:
|
||||
!isMetricSource && Array.isArray(config.select)
|
||||
? config.select.map(s => omit(s, ['metricName', 'metricType']))
|
||||
: config.select,
|
||||
metricTables: isMetricSource ? config.metricTables : undefined,
|
||||
// Order By and Having can only be set by the user for table charts
|
||||
having:
|
||||
config.displayType === DisplayType.Table ? config.having : undefined,
|
||||
orderBy:
|
||||
config.displayType === DisplayType.Table ? config.orderBy : undefined,
|
||||
};
|
||||
}
|
||||
|
||||
// Helper function to validate metric names for metric sources
|
||||
const validateMetricNames = (
|
||||
tableSource: TSource | undefined,
|
||||
|
|
@ -684,19 +707,13 @@ export default function EditTimeChartForm({
|
|||
select: isSelectEmpty
|
||||
? tableSource.defaultTableSelectExpression || ''
|
||||
: config.select,
|
||||
// Order By and Having can only be set by the user for table charts
|
||||
having: config.displayType === DisplayType.Table ? config.having : '',
|
||||
orderBy:
|
||||
config.displayType === DisplayType.Table
|
||||
? config.orderBy
|
||||
: undefined,
|
||||
};
|
||||
setQueriedConfigAndSource(
|
||||
// WARNING: DON'T JUST ASSIGN OBJECTS OR DO SPREAD OPERATOR STUFF WHEN
|
||||
// YOUR STATE IS AN OBJECT. YOU'RE COPYING BY REFERENCE WHICH MIGHT
|
||||
// ACCIDENTALLY CAUSE A useQuery SOMEWHERE TO FIRE A REQUEST EVERY TIME
|
||||
// AN INPUT CHANGES. USE structuredClone TO PERFORM A DEEP COPY INSTEAD
|
||||
structuredClone(newConfig),
|
||||
structuredClone(normalizeChartConfig(newConfig, tableSource)),
|
||||
tableSource,
|
||||
);
|
||||
}
|
||||
|
|
@ -734,25 +751,30 @@ export default function EditTimeChartForm({
|
|||
|
||||
const handleSave = useCallback(
|
||||
(v: SavedChartConfigWithSeries) => {
|
||||
// Validate metric sources have metric names selected
|
||||
if (validateMetricNames(tableSource, v.series, setError)) {
|
||||
return;
|
||||
}
|
||||
if (tableSource != null) {
|
||||
// Validate metric sources have metric names selected
|
||||
if (validateMetricNames(tableSource, v.series, setError)) {
|
||||
return;
|
||||
}
|
||||
|
||||
// If the chart type is search, we need to ensure the select is a string
|
||||
if (displayType === DisplayType.Search && typeof v.select !== 'string') {
|
||||
v.select = '';
|
||||
} else if (displayType !== DisplayType.Search) {
|
||||
v.select = v.series;
|
||||
}
|
||||
// If the chart type is search, we need to ensure the select is a string
|
||||
if (
|
||||
displayType === DisplayType.Search &&
|
||||
typeof v.select !== 'string'
|
||||
) {
|
||||
v.select = '';
|
||||
} else if (displayType !== DisplayType.Search) {
|
||||
v.select = v.series;
|
||||
}
|
||||
|
||||
// If the chart is not a table chart, we should clear the having clause
|
||||
if (displayType !== DisplayType.Table) {
|
||||
v.having = undefined;
|
||||
}
|
||||
const normalizedChartConfig = normalizeChartConfig(
|
||||
// Avoid saving the series field. Series should be persisted in the select field.
|
||||
omit(v, ['series']),
|
||||
tableSource,
|
||||
);
|
||||
|
||||
// Avoid saving the series field. Series should be persisted in the select field.
|
||||
onSave?.(omit(v, ['series']));
|
||||
onSave?.(normalizedChartConfig);
|
||||
}
|
||||
},
|
||||
[onSave, displayType, tableSource, setError],
|
||||
);
|
||||
|
|
|
|||
Loading…
Reference in a new issue