PMM-15326: Take out the OM plugin's duplicated and dead weight

Review notes from #5817, all of them things the package said twice or
said wrong.

`isRefreshActive` was byte-identical to `isRunActive` - same body, same
signature, both on the public surface. A collection pass and an
inventory refresh are different operations but report the same
`RunStatus`, so "is it still going" is one question. Kept
`isRunActive`, which is named for the type it takes, and pointed the
seven callers at it. `RunStatusBadge` was open-coding the same
comparison inline and now uses it too.

Query keys were the only lower-camel module constants in either hooks
file, three lines below `OM_BASE` and `RUN_POLL_MS`. Uppercased.

`useOmTopology`'s docstring called the topology document "the estate",
which is this package's word for the inventory side - the thing SEP's
probe collects by running a payload on hosts. Using it for both is what
makes the two hook files look interchangeable, and it is the likeliest
reason a reviewer asked why this hook was not on the other one. Said
which is which.

Both triggers sent a body they do not need. `body: "*"` plus
grpc-gateway's decoder treating io.EOF as an empty request means an
unscoped call can omit it; the scoped inventory refresh still sends
`{node_ids: [...]}`, which is what the Hosts row action needs.

`toWireValue` now records why `Number(draft)` is right for a
`timedelta`: SEP annotates those fields with a PlainSerializer to
integer seconds, so the wire carries `1800` rather than an ISO-8601
duration. It is a dependency on the annotation, not on the type, and
nothing else in either repo would have said so.

Signed-off-by: Pawel Lebioda <pawel.lebioda@percona.com>

Author: plebioda

ui/packages/plugins/om/src/HostsPage.tsx

@@ -46,14 +46,13 @@ import { Unavailable } from './components/Unavailable';
 import { formatDuration } from './format';
 import { ageSeconds, isFailing, toHostRows } from './inventory';
 import {
-  isRefreshActive,
   useForgetHost,
   useOmInventoryHosts,
   useInvalidateEstateOnRefreshEnd,
   useOmInventoryRuns,
   useRefreshInventory,
 } from './inventoryHooks';
-import { OmApiError } from './hooks';
+import { isRunActive, OmApiError } from './hooks';
 import type { OmHostRow } from './types';
 
 /** Identifiers and long text the table carries but does not open with. */
@@ -390,7 +389,7 @@ export function HostsPage() {
   // Any active run, matching InventoryPage's button: firing an estate-wide sweep into
   // one already in flight only earns a 409, and the row actions below cannot succeed
   // against a host that sweep already holds.
-  const refreshing = (runs ?? []).some((run) => isRefreshActive(run.status));
+  const refreshing = (runs ?? []).some((run) => isRunActive(run.status));
   const [forgetting, setForgetting] = useState<OmHostRow | null>(null);
   const columns = useColumns();
   const rows = useMemo(() => toHostRows(data), [data]);

ui/packages/plugins/om/src/InventoryPage.tsx

@@ -31,12 +31,11 @@ import {
 import PlayArrowIcon from '@mui/icons-material/PlayArrow';
 import { Table, type MRT_ColumnDef } from '@percona/percona-ui';
 import {
-  isRefreshActive,
   useInvalidateEstateOnRefreshEnd,
   useOmInventoryRuns,
   useRefreshInventory,
 } from './inventoryHooks';
-import { OmApiError } from './hooks';
+import { isRunActive, OmApiError } from './hooks';
 import { ConfigForm } from './components/ConfigForm';
 import { RunStatusBadge } from './components/HealthBadge';
 import { RunEntities } from './components/RunEntities';
@@ -172,7 +171,7 @@ function RefreshButton() {
   // and a narrow one started later can reach a terminal status while a broader one is
   // still probing - `runs[0]` would re-enable this button into a sweep that must
   // conflict with it. Same reason `useOmInventoryRuns` polls on the whole collection.
-  const running = (runs ?? []).some((run) => isRefreshActive(run.status));
+  const running = (runs ?? []).some((run) => isRunActive(run.status));
   // The rows this page's sibling tabs render change when the refresh finishes, not when
   // it is accepted, so the estate is invalidated on that edge rather than on the mutation.
   useInvalidateEstateOnRefreshEnd(runs);
@@ -235,7 +234,7 @@ function LastRun({ run }: { run: OmInventoryRun | undefined }) {
     );
   }
   const age = ageSeconds(run.start_time);
-  const active = isRefreshActive(run.status);
+  const active = isRunActive(run.status);
   return (
     <Stack
       direction="row"

ui/packages/plugins/om/src/components/ConfigForm.tsx

@@ -67,7 +67,19 @@ function inputKind(setting: OmInventorySetting): InputKind {
   return 'text';
 }
 
-/** Parse an edited value back into what the app expects on the wire. */
+/**
+ * Parse an edited value back into what the app expects on the wire.
+ *
+ * `Number(draft)` is right for a `timedelta` because a `timedelta` never reaches this
+ * form as a duration string. SEP declares those fields as
+ * `Annotated[timedelta, PlainSerializer(lambda v: round(v.total_seconds()), int)]`, so
+ * the configuration API dumps `1800`, not `"PT30M"`, and the box holds `"1800"`. The
+ * write goes back as the same integer and the app coerces it.
+ *
+ * That is a load-bearing dependency on SEP's annotation rather than on its type: drop
+ * the serialiser and pydantic's ISO-8601 default takes over, `Number` yields NaN, and
+ * every duration field renders as invalid. Named here because nothing else would say so.
+ */
 function toWireValue(setting: OmInventorySetting, draft: string): unknown {
   switch (inputKind(setting)) {
     case 'bool':

ui/packages/plugins/om/src/components/HealthBadge.tsx

@@ -22,6 +22,7 @@ import {
   SERVICE_STATUS_COLOR,
   SERVICE_STATUS_LABEL,
 } from '../constants';
+import { isRunActive } from '../hooks';
 import type { OmTopologyRunStatus, OmServiceStatus } from '../types';
 
 /**
@@ -49,7 +50,7 @@ export function RunStatusBadge({ status }: { status: OmTopologyRunStatus }) {
       size="small"
       label={RUN_STATUS_LABEL[status] ?? status}
       color={RUN_STATUS_COLOR[status] ?? 'default'}
-      variant={status === 'RUN_STATUS_RUNNING' ? 'outlined' : 'filled'}
+      variant={isRunActive(status) ? 'outlined' : 'filled'}
     />
   );
 }

ui/packages/plugins/om/src/hooks.ts

@@ -64,8 +64,8 @@ const RUN_POLL_MS = 3000;
 // ever shows what it happened to load with.
 const SNAPSHOT_POLL_MS = 30000;
 
-const topologyKey = ['om', 'topology'] as const;
-const runsKey = ['om', 'runs'] as const;
+const TOPOLOGY_KEY = ['om', 'topology'] as const;
+const RUNS_KEY = ['om', 'runs'] as const;
 
 /**
  * One request against pmm-managed.
@@ -118,6 +118,12 @@ export async function request<T>(path: string, init?: RequestInit): Promise<T> {
 /**
  * True while a run has not reached a terminal status.
  *
+ * Covers both kinds. A collection pass and an inventory refresh are very different
+ * operations, but they report the same `RunStatus` enum, so asking "is it still going"
+ * is one question and this is the one place that answers it. `inventoryHooks.ts` used
+ * to carry a byte-identical `isRefreshActive`, which meant two functions to keep in
+ * step with one wire contract.
+ *
  * Typed on the union rather than on `string`, because the compiler is the only thing
  * that would have caught this comparison going stale when the wire values changed.
  */
@@ -126,15 +132,21 @@ export function isRunActive(status: OmTopologyRunStatus | undefined): boolean {
 }
 
 /**
- * The whole estate from the latest terminal snapshot.
+ * The whole topology document from the latest terminal snapshot.
+ *
+ * "Estate" is this package's word for the *inventory* side - what SEP's probe found by
+ * running a payload on the hosts. This is the other thing: pmm-managed's own derivation
+ * from PMM inventory and VictoriaMetrics, which touches no host and is rebuilt per
+ * request. Calling both an estate is what makes the two hook files look interchangeable
+ * when they are not.
  *
  * One request, one document: the API assembles the tree per run, so there is no
  * per-cluster fetch and nothing to paginate. Keeping the previous page in place stops
  * the table collapsing to a spinner while a refetch is in flight.
  */
 export function useOmTopology() {
   return useQuery<OmTopologyResponse>({
-    queryKey: topologyKey,
+    queryKey: TOPOLOGY_KEY,
     placeholderData: keepPreviousData,
     queryFn: () => request<OmTopologyResponse>('/topology'),
     // pmm-managed rebuilds the document on its own timer, so a page left open goes
@@ -292,7 +304,7 @@ export function toEnvironmentSections(
  */
 export function useOmTopologyRuns(limit: number = OM_TOPOLOGY_RUNS_LIMIT) {
   return useQuery<OmTopologyRun[]>({
-    queryKey: [...runsKey, limit],
+    queryKey: [...RUNS_KEY, limit],
     queryFn: async () => {
       const { runs } = await request<{ runs: OmTopologyRun[] }>(
         `/topology/runs?limit=${limit}`
@@ -320,19 +332,21 @@ export function useOmTopologyRuns(limit: number = OM_TOPOLOGY_RUNS_LIMIT) {
 export function useTriggerOmTopologyRun() {
   const queryClient = useQueryClient();
   return useMutation<OmTopologyRunAccepted, Error, void>({
+    // No body. The method declares `body: "*"`, and grpc-gateway's generated decoder
+    // treats io.EOF as an empty request rather than an error, so an empty
+    // TriggerTopologyCollectionRequest is what binds either way.
     mutationFn: () =>
       request<OmTopologyRunAccepted>('/topology/runs:collect', {
         method: 'POST',
-        body: '{}',
       }),
     // onSettled, not onSuccess. A 409 means pmm-managed's own timer holds the lock, so
     // a run is in flight and the document is about to change -- the one case where a
     // refetch is most obviously wanted, and the one the success-only path skipped. The
     // request is cheap and the answer is idempotent, so refetching after a failure that
     // is not a conflict costs nothing either.
     onSettled: () => {
-      queryClient.invalidateQueries({ queryKey: runsKey });
-      queryClient.invalidateQueries({ queryKey: topologyKey });
+      queryClient.invalidateQueries({ queryKey: RUNS_KEY });
+      queryClient.invalidateQueries({ queryKey: TOPOLOGY_KEY });
     },
   });
 }
@@ -341,7 +355,7 @@ export function useTriggerOmTopologyRun() {
 export function useInvalidateOmTopologySnapshot() {
   const queryClient = useQueryClient();
   return () => {
-    queryClient.invalidateQueries({ queryKey: topologyKey });
-    queryClient.invalidateQueries({ queryKey: runsKey });
+    queryClient.invalidateQueries({ queryKey: TOPOLOGY_KEY });
+    queryClient.invalidateQueries({ queryKey: RUNS_KEY });
   };
 }

ui/packages/plugins/om/src/index.ts

@@ -69,7 +69,6 @@ export {
   useRefreshInventory,
   useForgetHost,
   useForgetService,
-  isRefreshActive,
 } from './inventoryHooks';
 export type { OmHostFilters } from './inventoryHooks';
 export {

ui/packages/plugins/om/src/inventoryHooks.ts

@@ -39,21 +39,20 @@ import {
   useQueryClient,
 } from '@tanstack/react-query';
 import { useEffect, useRef } from 'react';
-import { request } from './hooks';
+import { isRunActive, request } from './hooks';
 import type {
   OmInventoryHost,
   OmInventoryRun,
   OmInventoryRunAccepted,
   OmInventoryRunDetail,
   OmInventoryService,
   OmInventorySetting,
-  OmTopologyRunStatus,
 } from './types';
 
-const hostsKey = ['om', 'inventory', 'hosts'] as const;
-const servicesKey = ['om', 'inventory', 'services'] as const;
-const runsKey = ['om', 'inventory', 'runs'] as const;
-const configKey = ['om', 'inventory', 'config'] as const;
+const HOSTS_KEY = ['om', 'inventory', 'hosts'] as const;
+const SERVICES_KEY = ['om', 'inventory', 'services'] as const;
+const RUNS_KEY = ['om', 'inventory', 'runs'] as const;
+const CONFIG_KEY = ['om', 'inventory', 'config'] as const;
 
 /** Poll cadence while a refresh is in flight (ms). */
 const REFRESH_POLL_MS = 3000;
@@ -113,7 +112,7 @@ function toQuery(filters: OmHostFilters): string {
 export function useOmInventoryHosts(filters: OmHostFilters = {}) {
   const query = toQuery(filters);
   return useQuery<OmInventoryHost[]>({
-    queryKey: [...hostsKey, query],
+    queryKey: [...HOSTS_KEY, query],
     placeholderData: keepPreviousData,
     queryFn: async () => {
       const { hosts } = await request<{ hosts: OmInventoryHost[] }>(
@@ -137,7 +136,7 @@ export function useOmInventoryHosts(filters: OmHostFilters = {}) {
  */
 export function useOmInventoryServices() {
   return useQuery<OmInventoryService[]>({
-    queryKey: servicesKey,
+    queryKey: SERVICES_KEY,
     placeholderData: keepPreviousData,
     queryFn: async () => {
       const { services } = await request<{ services: OmInventoryService[] }>(
@@ -149,28 +148,16 @@ export function useOmInventoryServices() {
   });
 }
 
-/**
- * True while a refresh has not reached a terminal status.
- *
- * Typed on the union rather than on `string`, because the compiler is the only thing
- * that would have caught this comparison going stale when the wire values changed.
- */
-export function isRefreshActive(
-  status: OmTopologyRunStatus | undefined
-): boolean {
-  return status === 'RUN_STATUS_RUNNING';
-}
-
 /**
  * Refresh history, newest first.
  *
- * Polls fast while the newest refresh is in flight and slowly otherwise, rather than
- * stopping: the schedule starts sweeps nobody here asked for, and a history that only
- * updated on reload could not show them.
+ * Polls fast while any refresh is in flight and slowly otherwise, rather than stopping:
+ * the schedule starts sweeps nobody here asked for, and a history that only updated on
+ * reload could not show them.
  */
 export function useOmInventoryRuns(limit = 25) {
   return useQuery<OmInventoryRun[]>({
-    queryKey: [...runsKey, limit],
+    queryKey: [...RUNS_KEY, limit],
     queryFn: async () => {
       const { runs } = await request<{ runs: OmInventoryRun[] }>(
         `/inventory/runs?limit=${limit}`
@@ -181,7 +168,7 @@ export function useOmInventoryRuns(limit = 25) {
     // overlap -- and a narrow one started later can finish first, which would leave
     // data[0] terminal while a broader run is still probing.
     refetchInterval: (query) =>
-      (query.state.data ?? []).some((run) => isRefreshActive(run.status))
+      (query.state.data ?? []).some((run) => isRunActive(run.status))
         ? REFRESH_POLL_MS
         : ESTATE_POLL_MS,
   });
@@ -211,12 +198,12 @@ export function useInvalidateEstateOnRefreshEnd(
   const queryClient = useQueryClient();
   const wasActive = useRef(false);
 
-  const anyActive = (runs ?? []).some((run) => isRefreshActive(run.status));
+  const anyActive = (runs ?? []).some((run) => isRunActive(run.status));
 
   useEffect(() => {
     if (wasActive.current && !anyActive) {
-      queryClient.invalidateQueries({ queryKey: hostsKey });
-      queryClient.invalidateQueries({ queryKey: servicesKey });
+      queryClient.invalidateQueries({ queryKey: HOSTS_KEY });
+      queryClient.invalidateQueries({ queryKey: SERVICES_KEY });
     }
     wasActive.current = anyActive;
   }, [anyActive, queryClient]);
@@ -238,15 +225,20 @@ export function useInvalidateEstateOnRefreshEnd(
 export function useRefreshInventory() {
   const queryClient = useQueryClient();
   return useMutation<OmInventoryRunAccepted, Error, string[] | undefined>({
+    // A body only when there is a scope to state. `body: "*"` on the method plus
+    // grpc-gateway tolerating io.EOF means an unscoped sweep needs no `{}` to say
+    // "everything"; the absent body already says it.
     mutationFn: (nodeIds) =>
       request<OmInventoryRunAccepted>('/inventory/runs:trigger', {
         method: 'POST',
-        body: JSON.stringify(nodeIds?.length ? { node_ids: nodeIds } : {}),
+        body: nodeIds?.length
+          ? JSON.stringify({ node_ids: nodeIds })
+          : undefined,
       }),
     // onSettled rather than onSuccess: a 409 means a sweep is in flight and the estate
     // is about to change, which is exactly when a refetch is most wanted.
     onSettled: () => {
-      queryClient.invalidateQueries({ queryKey: runsKey });
+      queryClient.invalidateQueries({ queryKey: RUNS_KEY });
     },
   });
 }
@@ -267,8 +259,8 @@ export function useForgetHost() {
         method: 'DELETE',
       }),
     onSuccess: () => {
-      queryClient.invalidateQueries({ queryKey: hostsKey });
-      queryClient.invalidateQueries({ queryKey: servicesKey });
+      queryClient.invalidateQueries({ queryKey: HOSTS_KEY });
+      queryClient.invalidateQueries({ queryKey: SERVICES_KEY });
     },
   });
 }
@@ -282,8 +274,8 @@ export function useForgetService() {
         method: 'DELETE',
       }),
     onSuccess: () => {
-      queryClient.invalidateQueries({ queryKey: hostsKey });
-      queryClient.invalidateQueries({ queryKey: servicesKey });
+      queryClient.invalidateQueries({ queryKey: HOSTS_KEY });
+      queryClient.invalidateQueries({ queryKey: SERVICES_KEY });
     },
   });
 }
@@ -298,13 +290,13 @@ export function useForgetService() {
  */
 export function useOmInventoryRun(runId: string | undefined) {
   return useQuery<OmInventoryRunDetail>({
-    queryKey: [...runsKey, 'detail', runId],
+    queryKey: [...RUNS_KEY, 'detail', runId],
     enabled: Boolean(runId),
     queryFn: () => request<OmInventoryRunDetail>(`/inventory/runs/${runId}`),
     // A run still going gains entities as its dispatches land, so the open panel
     // follows it; a finished one never changes again.
     refetchInterval: (query) =>
-      isRefreshActive(query.state.data?.run.status) ? REFRESH_POLL_MS : false,
+      isRunActive(query.state.data?.run.status) ? REFRESH_POLL_MS : false,
   });
 }
 
@@ -317,7 +309,7 @@ export function useOmInventoryRun(runId: string | undefined) {
  */
 export function useOmInventoryConfig() {
   return useQuery<OmInventorySetting[]>({
-    queryKey: configKey,
+    queryKey: CONFIG_KEY,
     queryFn: async () => {
       const { settings } = await request<{ settings: OmInventorySetting[] }>(
         '/inventory/config'
@@ -358,10 +350,10 @@ export function useUpdateOmInventoryConfig() {
         body: JSON.stringify(values),
       }),
     onSettled: () => {
-      queryClient.invalidateQueries({ queryKey: configKey });
+      queryClient.invalidateQueries({ queryKey: CONFIG_KEY });
       // The schedule drives when the next refresh runs, so the history's idea of
       // "due" changes with it.
-      queryClient.invalidateQueries({ queryKey: runsKey });
+      queryClient.invalidateQueries({ queryKey: RUNS_KEY });
     },
   });
 }
@@ -375,7 +367,7 @@ export function useResetOmInventoryConfig() {
         method: 'DELETE',
       }),
     onSettled: () => {
-      queryClient.invalidateQueries({ queryKey: configKey });
+      queryClient.invalidateQueries({ queryKey: CONFIG_KEY });
     },
   });
 }