From 55e0590b8cd13030e7ffa1c119a213266bc34020 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 3 Apr 2025 13:38:25 -0500 Subject: [PATCH 1/4] scenarios for mock oxql data --- mock-api/msw/handlers.ts | 12 +++++++++++- mock-api/oxql-metrics.ts | 6 ++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index 6f26058b23..c7c52707e6 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -1614,7 +1614,17 @@ export const handlers = makeHandlers({ // timeseries queries are slower than most other queries await delay(1000) - return handleOxqlMetrics(body) + const data = handleOxqlMetrics(body) + + // we use other-project to test certain response cases + if (query.project === 'other-project') { + // 1. return only one data point + const points = Object.values(data.tables[0].timeseries)[0].points + points.timestamps = points.timestamps.slice(0, 2) + points.values = points.values.slice(0, 2) + } + + return data }, async systemTimeseriesQuery({ cookies, body }) { requireFleetViewer(cookies) diff --git a/mock-api/oxql-metrics.ts b/mock-api/oxql-metrics.ts index a65c5a19e7..b63e165f21 100644 --- a/mock-api/oxql-metrics.ts +++ b/mock-api/oxql-metrics.ts @@ -280,7 +280,9 @@ export const getMockOxqlInstanceData = ( state?: OxqlVcpuState ): Json => { const values = state ? mockOxqlVcpuStateValues[state] : mockOxqlValues[name] - return { + // structuredClone lets us mutate data in the calling code without messing up + // the source data + return structuredClone({ tables: [ { name: name, @@ -310,5 +312,5 @@ export const getMockOxqlInstanceData = ( }, }, ], - } + }) } From 55b82f44969d457913bffa580c8930d71adc0c53 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 3 Apr 2025 14:50:52 -0500 Subject: [PATCH 2/4] explicit loading state for timeserieschart --- app/components/SystemMetric.tsx | 6 +++ app/components/TimeSeriesChart.tsx | 52 ++++++++++++++++++---- app/components/oxql-metrics/OxqlMetric.tsx | 42 ++++++++++------- 3 files changed, 75 insertions(+), 25 deletions(-) diff --git a/app/components/SystemMetric.tsx b/app/components/SystemMetric.tsx index 0cfaa28dd6..2bb31bc063 100644 --- a/app/components/SystemMetric.tsx +++ b/app/components/SystemMetric.tsx @@ -100,6 +100,9 @@ export function SiloMetric({ startTime={startTime} endTime={endTime} unit={unit !== 'Count' ? unit : undefined} + // note use of loading, not fetching, which is only true on first fetch. + // otherwise we get loading states on refetches + loading={inRange.isLoading || beforeStart.isLoading} /> ) @@ -168,6 +171,9 @@ export function SystemMetric({ startTime={startTime} endTime={endTime} unit={unit !== 'Count' ? unit : undefined} + // note use of loading, not fetching, which is only true on first fetch. + // otherwise we get loading states on refetches + loading={inRange.isLoading || beforeStart.isLoading} /> ) diff --git a/app/components/TimeSeriesChart.tsx b/app/components/TimeSeriesChart.tsx index 85486a29bf..19ff384224 100644 --- a/app/components/TimeSeriesChart.tsx +++ b/app/components/TimeSeriesChart.tsx @@ -118,6 +118,7 @@ type TimeSeriesChartProps = { unit?: string yAxisTickFormatter?: (val: number) => string hasError?: boolean + loading: boolean } const TICK_COUNT = 6 @@ -173,6 +174,7 @@ export function TimeSeriesChart({ unit, yAxisTickFormatter = (val) => val.toLocaleString(), hasError = false, + loading, }: TimeSeriesChartProps) { // We use the largest data point +20% for the graph scale. !rawData doesn't // mean it's empty (it will never be empty because we fill in artificial 0s at @@ -213,13 +215,20 @@ export function TimeSeriesChart({ ) } - if (!data || data.length === 0) { + if (loading) { return ( ) } + if (!data || data.length === 0) { + return ( + + + + ) + } // ResponsiveContainer has default height and width of 100% // https://recharts.org/en-US/api/ResponsiveContainer @@ -288,16 +297,21 @@ const MetricsLoadingIndicator = () => ( ) -const MetricsError = () => ( +const MetricsMessage = ({ + icon, + title, + description, +}: { + icon?: React.ReactNode + title: React.ReactNode + description: React.ReactNode +}) => ( <>
-
-
- -
-
Something went wrong
-
- Please try again. If the problem persists, contact your administrator. +
{icon}
+
{title}
+
+ {description}
( ) +const MetricsError = () => ( + +
+ + + } + title="Something went wrong" + description="Please try again. If the problem persists, contact your administrator." + /> +) + +const MetricsEmpty = () => ( + } + title="No data" + description="There is no data for this time period." + /> +) export const ChartContainer = classed.div`flex w-full grow flex-col rounded-lg border border-default` type ChartHeaderProps = { diff --git a/app/components/oxql-metrics/OxqlMetric.tsx b/app/components/oxql-metrics/OxqlMetric.tsx index c9b824670a..534d770f19 100644 --- a/app/components/oxql-metrics/OxqlMetric.tsx +++ b/app/components/oxql-metrics/OxqlMetric.tsx @@ -12,15 +12,14 @@ */ import { useQuery } from '@tanstack/react-query' -import { Children, useEffect, useMemo, useState, type ReactNode } from 'react' +import { Children, useMemo, useState, type ReactNode } from 'react' import type { LoaderFunctionArgs } from 'react-router' import { apiq, queryClient } from '@oxide/api' import { CopyCodeModal } from '~/components/CopyCode' import { MoreActionsMenu } from '~/components/MoreActionsMenu' -import { getInstanceSelector, useProjectSelector } from '~/hooks/use-params' -import { useMetricsContext } from '~/pages/project/instances/common' +import { getInstanceSelector } from '~/hooks/use-params' import { LearnMore } from '~/ui/lib/CardBlock' import * as Dropdown from '~/ui/lib/DropdownMenu' import { classed } from '~/util/classed' @@ -51,23 +50,33 @@ export type OxqlMetricProps = OxqlQuery & { } export function OxqlMetric({ title, description, unit, ...queryObj }: OxqlMetricProps) { - // only start reloading data once an intial dataset has been loaded - const { setIsIntervalPickerEnabled } = useMetricsContext() - const { project } = useProjectSelector() const query = toOxqlStr(queryObj) - const { data: metrics, error } = useQuery( - apiq('timeseriesQuery', { body: { query }, query: { project } }) + const { + data: metrics, + error, + isPending, + } = useQuery( + apiq('systemTimeseriesQuery', { body: { query } }) // avoid graphs flashing blank while loading when you change the time // { placeholderData: (x) => x } ) - useEffect(() => { - if (metrics) { - // this is too slow right now; disabling until we can make it faster - // setIsIntervalPickerEnabled(true) - } - }, [metrics, setIsIntervalPickerEnabled]) + + // HACK: omicron has a bug where it blows up on an attempt to group by on + // empty result set because it can't determine whether the data is aligned. + // Most likely it should consider empty data sets trivially aligned and just + // flow the emptiness on through, but in the meantime we have to detect + // this error and pretend it is not an error. + // See https://github.com/oxidecomputer/omicron/issues/7715 + const errorMeansEmpty = error?.message === 'Input tables to a `group_by` must be aligned' + const hasError = !!error && !errorMeansEmpty + const { startTime, endTime } = queryObj - const { chartData, timeseriesCount } = useMemo(() => composeOxqlData(metrics), [metrics]) + const { chartData, timeseriesCount } = useMemo( + () => + errorMeansEmpty ? { chartData: [], timeseriesCount: 0 } : composeOxqlData(metrics), + [metrics, errorMeansEmpty] + ) + const { data, label, unitForSet, yAxisTickFormatter } = useMemo(() => { if (unit === 'Bytes') return getBytesChartProps(chartData) if (unit === 'Count') return getCountChartProps(chartData) @@ -103,7 +112,8 @@ export function OxqlMetric({ title, description, unit, ...queryObj }: OxqlMetric unit={unitForSet} data={data} yAxisTickFormatter={yAxisTickFormatter} - hasError={!!error} + hasError={hasError} + loading={isPending} /> ) From e158149112ce6e3bdb20be481b60c3810742b938 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 3 Apr 2025 14:55:00 -0500 Subject: [PATCH 3/4] test it, switch back to project timeseries endpoint --- app/components/TimeSeriesChart.tsx | 12 +++++----- app/components/oxql-metrics/OxqlMetric.tsx | 10 ++++---- mock-api/msw/handlers.ts | 9 +++++-- test/e2e/instance-metrics.e2e.ts | 28 ++++++++++++++++++++++ 4 files changed, 47 insertions(+), 12 deletions(-) diff --git a/app/components/TimeSeriesChart.tsx b/app/components/TimeSeriesChart.tsx index 19ff384224..7f77c5cf7d 100644 --- a/app/components/TimeSeriesChart.tsx +++ b/app/components/TimeSeriesChart.tsx @@ -290,7 +290,7 @@ export function TimeSeriesChart({ } const MetricsLoadingIndicator = () => ( -
+
@@ -308,7 +308,7 @@ const MetricsMessage = ({ }) => ( <>
-
{icon}
+ {icon}
{title}
{description} @@ -327,10 +327,10 @@ const MetricsMessage = ({ const MetricsError = () => ( +
- +
} title="Something went wrong" description="Please try again. If the problem persists, contact your administrator." @@ -339,8 +339,8 @@ const MetricsError = () => ( const MetricsEmpty = () => ( } - title="No data" + // mt-3 is a shameful hack to get it vertically centered in the chart + title={
No data
} description="There is no data for this time period." /> ) diff --git a/app/components/oxql-metrics/OxqlMetric.tsx b/app/components/oxql-metrics/OxqlMetric.tsx index 534d770f19..48c63753d4 100644 --- a/app/components/oxql-metrics/OxqlMetric.tsx +++ b/app/components/oxql-metrics/OxqlMetric.tsx @@ -19,7 +19,7 @@ import { apiq, queryClient } from '@oxide/api' import { CopyCodeModal } from '~/components/CopyCode' import { MoreActionsMenu } from '~/components/MoreActionsMenu' -import { getInstanceSelector } from '~/hooks/use-params' +import { getInstanceSelector, useProjectSelector } from '~/hooks/use-params' import { LearnMore } from '~/ui/lib/CardBlock' import * as Dropdown from '~/ui/lib/DropdownMenu' import { classed } from '~/util/classed' @@ -51,12 +51,13 @@ export type OxqlMetricProps = OxqlQuery & { export function OxqlMetric({ title, description, unit, ...queryObj }: OxqlMetricProps) { const query = toOxqlStr(queryObj) + const { project } = useProjectSelector() const { data: metrics, error, - isPending, + isLoading, } = useQuery( - apiq('systemTimeseriesQuery', { body: { query } }) + apiq('timeseriesQuery', { body: { query }, query: { project } }) // avoid graphs flashing blank while loading when you change the time // { placeholderData: (x) => x } ) @@ -113,7 +114,8 @@ export function OxqlMetric({ title, description, unit, ...queryObj }: OxqlMetric data={data} yAxisTickFormatter={yAxisTickFormatter} hasError={hasError} - loading={isPending} + // isLoading only covers first load --- future-proof against the reintroduction of interval refresh + loading={isLoading} /> ) diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index c7c52707e6..2fa27cd680 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -1620,8 +1620,13 @@ export const handlers = makeHandlers({ if (query.project === 'other-project') { // 1. return only one data point const points = Object.values(data.tables[0].timeseries)[0].points - points.timestamps = points.timestamps.slice(0, 2) - points.values = points.values.slice(0, 2) + if (body.query.includes('state == "run"')) { + points.timestamps = points.timestamps.slice(0, 2) + points.values = points.values.slice(0, 2) + } else if (body.query.includes('state == "emulation"')) { + points.timestamps = points.timestamps.slice(0, 1) + points.values = points.values.slice(0, 1) + } } return data diff --git a/test/e2e/instance-metrics.e2e.ts b/test/e2e/instance-metrics.e2e.ts index c5f7bfdefd..5a057ae70b 100644 --- a/test/e2e/instance-metrics.e2e.ts +++ b/test/e2e/instance-metrics.e2e.ts @@ -41,3 +41,31 @@ test('Instance metrics work for non-fleet viewer', async ({ browser }) => { // we don't want an error, we want the data! await expect(page.getByText('Something went wrong')).toBeHidden() }) + +test('empty and loading states', async ({ page }) => { + // we have special handling in the API to return special data for this project + await page.goto('/projects/other-project/instances/failed-restarting-soon/metrics/cpu') + + const loading = page.getByLabel('Chart loading') // aria-label on loading indicator + const noData = page.getByText('No data', { exact: true }) + + // default running state returns two data points, which get turned into one by + // the data munging + await expect(loading).toBeVisible() + await expect(loading).toBeHidden() + await expect(noData).toBeHidden() + + const statePicker = page.getByRole('button', { name: 'Choose state' }) + + // emulation state returns one data point + await statePicker.click() + await page.getByRole('option', { name: 'State: Emulation' }).click() + await expect(loading).toBeVisible() + await expect(loading).toBeHidden() + await expect(noData).toBeVisible() + + // make sure it goes away agan for the first one + await statePicker.click() + await page.getByRole('option', { name: 'State: Running' }).click() + await expect(noData).toBeHidden() +}) From 19d99a8a4016ad6294454f777a6d11e1a44d489c Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 3 Apr 2025 16:34:55 -0500 Subject: [PATCH 4/4] also test group by error workaround --- app/api/util.ts | 2 ++ app/components/oxql-metrics/OxqlMetric.tsx | 4 ++-- mock-api/msw/handlers.ts | 4 +++- test/e2e/instance-metrics.e2e.ts | 20 +++++++++++++++++++- 4 files changed, 26 insertions(+), 4 deletions(-) diff --git a/app/api/util.ts b/app/api/util.ts index 4018e4041b..79a06b87dd 100644 --- a/app/api/util.ts +++ b/app/api/util.ts @@ -288,3 +288,5 @@ export function parseIpUtilization({ ipv4, ipv6 }: IpPoolUtilization) { }, } } + +export const OXQL_GROUP_BY_ERROR = 'Input tables to a `group_by` must be aligned' diff --git a/app/components/oxql-metrics/OxqlMetric.tsx b/app/components/oxql-metrics/OxqlMetric.tsx index 48c63753d4..db8c097998 100644 --- a/app/components/oxql-metrics/OxqlMetric.tsx +++ b/app/components/oxql-metrics/OxqlMetric.tsx @@ -15,7 +15,7 @@ import { useQuery } from '@tanstack/react-query' import { Children, useMemo, useState, type ReactNode } from 'react' import type { LoaderFunctionArgs } from 'react-router' -import { apiq, queryClient } from '@oxide/api' +import { apiq, OXQL_GROUP_BY_ERROR, queryClient } from '@oxide/api' import { CopyCodeModal } from '~/components/CopyCode' import { MoreActionsMenu } from '~/components/MoreActionsMenu' @@ -68,7 +68,7 @@ export function OxqlMetric({ title, description, unit, ...queryObj }: OxqlMetric // flow the emptiness on through, but in the meantime we have to detect // this error and pretend it is not an error. // See https://github.com/oxidecomputer/omicron/issues/7715 - const errorMeansEmpty = error?.message === 'Input tables to a `group_by` must be aligned' + const errorMeansEmpty = error?.message === OXQL_GROUP_BY_ERROR const hasError = !!error && !errorMeansEmpty const { startTime, endTime } = queryObj diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index 2fa27cd680..c49e581d3a 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -26,7 +26,7 @@ import { } from '@oxide/api' import { json, makeHandlers, type Json } from '~/api/__generated__/msw-handlers' -import { instanceCan } from '~/api/util' +import { instanceCan, OXQL_GROUP_BY_ERROR } from '~/api/util' import { parseIp } from '~/util/ip' import { commaSeries } from '~/util/str' import { GiB } from '~/util/units' @@ -1626,6 +1626,8 @@ export const handlers = makeHandlers({ } else if (body.query.includes('state == "emulation"')) { points.timestamps = points.timestamps.slice(0, 1) points.values = points.values.slice(0, 1) + } else if (body.query.includes('state == "idle"')) { + throw OXQL_GROUP_BY_ERROR } } diff --git a/test/e2e/instance-metrics.e2e.ts b/test/e2e/instance-metrics.e2e.ts index 5a057ae70b..55020c801a 100644 --- a/test/e2e/instance-metrics.e2e.ts +++ b/test/e2e/instance-metrics.e2e.ts @@ -8,6 +8,8 @@ import { expect, test } from '@playwright/test' +import { OXQL_GROUP_BY_ERROR } from '~/api' + import { getPageAsUser } from './utils' test('Click through instance metrics', async ({ page }) => { @@ -43,6 +45,9 @@ test('Instance metrics work for non-fleet viewer', async ({ browser }) => { }) test('empty and loading states', async ({ page }) => { + const messages: string[] = [] + page.on('console', (e) => messages.push(e.text())) + // we have special handling in the API to return special data for this project await page.goto('/projects/other-project/instances/failed-restarting-soon/metrics/cpu') @@ -64,8 +69,21 @@ test('empty and loading states', async ({ page }) => { await expect(loading).toBeHidden() await expect(noData).toBeVisible() - // make sure it goes away agan for the first one + // idle state returns group_by must be aligned error, treated as empty + const hasGroupByError = () => messages.some((m) => m.includes(OXQL_GROUP_BY_ERROR)) + + expect(hasGroupByError()).toBe(false) // error not in console + await statePicker.click() + await page.getByRole('option', { name: 'State: Idle' }).click() + await expect(loading).toBeVisible() + await expect(loading).toBeHidden() + await expect(page.getByText('Something went wrong')).toBeHidden() + await expect(noData).toBeVisible() + expect(hasGroupByError()).toBe(true) // error present in console + + // make sure empty state goes away again for the first one await statePicker.click() await page.getByRole('option', { name: 'State: Running' }).click() await expect(noData).toBeHidden() + await expect(loading).toBeHidden() })