Fix metric loading and empty states (#2779)

* scenarios for mock oxql data

* explicit loading state for timeserieschart

* test it, switch back to project timeseries endpoint

* also test group by error workaround

Author: david-crespo

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'

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}
       />
     </ChartContainer>
   )
@@ -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}
       />
     </ChartContainer>
   )

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 (
       <SkeletonMetric shimmer>
         <MetricsLoadingIndicator />
       </SkeletonMetric>
     )
   }
+  if (!data || data.length === 0) {
+    return (
+      <SkeletonMetric>
+        <MetricsEmpty />
+      </SkeletonMetric>
+    )
+  }
 
   // ResponsiveContainer has default height and width of 100%
   // https://recharts.org/en-US/api/ResponsiveContainer
@@ -281,23 +290,28 @@ export function TimeSeriesChart({
 }
 
 const MetricsLoadingIndicator = () => (
-  <div className="metrics-loading-indicator">
+  <div className="metrics-loading-indicator" aria-label="Chart loading">
     <span></span>
     <span></span>
     <span></span>
   </div>
 )
 
-const MetricsError = () => (
+const MetricsMessage = ({
+  icon,
+  title,
+  description,
+}: {
+  icon?: React.ReactNode
+  title: React.ReactNode
+  description: React.ReactNode
+}) => (
   <>
     <div className="z-10 flex w-52 flex-col items-center justify-center gap-1">
-      <div className="my-2 flex h-8 w-8 items-center justify-center">
-        <div className="absolute h-8 w-8 rounded-full opacity-20 bg-destructive motion-safe:animate-[ping_2s_cubic-bezier(0,0,0.2,1)_infinite]" />
-        <Error12Icon className="relative h-6 w-6 text-error-tertiary" />
-      </div>
-      <div className="text-semi-lg text-center text-raise">Something went wrong</div>
-      <div className="text-center text-sans-md text-secondary">
-        Please try again. If the problem persists, contact your administrator.
+      {icon}
+      <div className="text-semi-lg text-center text-raise">{title}</div>
+      <div className="text-balance text-center text-sans-md text-secondary">
+        {description}
       </div>
     </div>
     <div
@@ -310,6 +324,26 @@ const MetricsError = () => (
   </>
 )
 
+const MetricsError = () => (
+  <MetricsMessage
+    icon={
+      <div className="my-2 flex h-8 w-8 items-center justify-center">
+        <div className="absolute h-8 w-8 rounded-full opacity-20 bg-destructive motion-safe:animate-[ping_2s_cubic-bezier(0,0,0.2,1)_infinite]" />
+        <Error12Icon className="relative h-6 w-6 text-error-tertiary" />
+      </div>
+    }
+    title="Something went wrong"
+    description="Please try again. If the problem persists, contact your administrator."
+  />
+)
+
+const MetricsEmpty = () => (
+  <MetricsMessage
+    // mt-3 is a shameful hack to get it vertically centered in the chart
+    title={<div className="mt-3">No data</div>}
+    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 = {

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 { apiq, OXQL_GROUP_BY_ERROR, 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 { LearnMore } from '~/ui/lib/CardBlock'
 import * as Dropdown from '~/ui/lib/DropdownMenu'
 import { classed } from '~/util/classed'
@@ -51,23 +50,34 @@ 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(
+  const { project } = useProjectSelector()
+  const {
+    data: metrics,
+    error,
+    isLoading,
+  } = useQuery(
     apiq('timeseriesQuery', { body: { query }, query: { project } })
     // 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 === OXQL_GROUP_BY_ERROR
+  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 +113,9 @@ export function OxqlMetric({ title, description, unit, ...queryObj }: OxqlMetric
         unit={unitForSet}
         data={data}
         yAxisTickFormatter={yAxisTickFormatter}
-        hasError={!!error}
+        hasError={hasError}
+        // isLoading only covers first load --- future-proof against the reintroduction of interval refresh
+        loading={isLoading}
       />
     </ChartContainer>
   )

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'
@@ -1614,7 +1614,24 @@ 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
+      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)
+      } else if (body.query.includes('state == "idle"')) {
+        throw OXQL_GROUP_BY_ERROR
+      }
+    }
+
+    return data
   },
   async systemTimeseriesQuery({ cookies, body }) {
     requireFleetViewer(cookies)

mock-api/oxql-metrics.ts

@@ -280,7 +280,9 @@ export const getMockOxqlInstanceData = (
   state?: OxqlVcpuState
 ): Json<OxqlQueryResult> => {
   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 = (
         },
       },
     ],
-  }
+  })
 }

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 }) => {
@@ -41,3 +43,47 @@ 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 }) => {
+  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')
+
+  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()
+
+  // 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()
+})