diff --git a/openspec/changes/per-instance-hook-scoping/proposal.md b/openspec/changes/per-instance-hook-scoping/proposal.md new file mode 100644 index 0000000..82018a7 --- /dev/null +++ b/openspec/changes/per-instance-hook-scoping/proposal.md @@ -0,0 +1,93 @@ +# SDD Proposal: Per-Instance Hook Scoping + +**Change:** `per-instance-hook-scoping` +**Phase:** proposal +**Date:** 2026-07-09 + +## 1. Problem / Why Now + +This is a **correctness fix**, not a feature. + +When a user has multiple instances of Alertmanager, Prometheus, or the backups service and opens a specific instance's service page, the **Observability/backup tabs show data for whichever instance resolves first globally — not the instance whose page they're viewing.** The root cause: the frontend hooks (`useAlertmanagerAlerts`, `useAlertmanagerStatus`, `usePrometheusStatus`, `useBackupJobs/Runs/Alerts`) query without a `serviceId`, and the consuming tabs (`AlertsTab`, `MetricsTab`, `JobsTab`) don't pass `instance.id` to them. + +This was documented as a carry-over risk in the `services-as-hub-ia` verify-report (2026-06-26): *"Hooks query globally, not per-instance... JobsTab, AlertsTab, MetricsTab show data for whichever instance the hook resolves as first-configured."* The tabs were deliberately built to accept an `instance` prop and wait for the hooks to catch up — each tab carries an explicit TODO comment to that effect. The fix is now due. + +## 2. Target Users and Situations + +- **Primary users:** Operators with more than one Alertmanager, Prometheus, or backups instance (e.g., a staging + production pair, or per-tenant instances). +- **Workflow moment:** open `/{serviceType}/{instanceId}` to inspect a specific instance → the tabs must reflect THAT instance, not a global/first-resolved one. +- **Urgency:** Medium. Single-instance operators see no bug; multi-instance operators see silently-wrong data. The incorrect-behavior surface grows as more instances are added. + +## 3. Product Outcome + +After this change, an authenticated user viewing a specific service instance's page sees alerts, status, and backup data scoped to **that instance only.** Switching the instance switcher (already present on `ServicePage` when `enabledSiblings.length > 1`) re-scopes the data. The dashboard widgets (which resolve via widget-instance → `service_id`) are unaffected — they were already correct. + +## 4. Scope Boundaries and Non-Goals + +### In scope + +- **Frontend hooks gain a `serviceId` param:** `useAlertmanagerAlerts(serviceId)`, `useAlertmanagerStatus(serviceId)`, `usePrometheusStatus(serviceId)`, `useBackupJobs(serviceId)`, `useBackupRuns(jobId, status, serviceId)`, `useBackupAlerts(jobId, acknowledged, severity, serviceId)`. Each thread it into `queryKey` (so caches are per-instance) and pass it to the fetch function. +- **Fetch functions pass `service_id` as a query param:** the existing `fetch*` functions in `api/client.ts` and `api/backups.ts` gain an optional `serviceId` argument appended to the request URL. +- **Tabs pass `instance.id`:** `AlertsTab`, `MetricsTab`, `JobsTab` call the hooks with their received `instance.id`. +- **Backend backup endpoints gain `service_id`:** `get_backup_jobs`, `get_backup_runs`, `get_backup_alerts`, `get_backup_dashboard` accept an optional `service_id: str | None` query param and thread it into `SettingsStore` query methods (which already have service attribution per the `services-as-hub-ia` Slice 3 work). +- **`get_prometheus_targets` stays global** — it returns Node Exporter scrape targets for *external* Prometheus instances via `http_sd_configs`, not instance-scoped UI data. Out of scope by design. + +### Non-goals (explicitly out of scope) + +- **Widget-level data scoping.** Dashboard widgets (`PrometheusChartWidget`, `AlertmanagerAlertsWidget`, `BackupsWidget`, etc.) already resolve their service via the widget instance's `service_id` binding — they are correct and untouched. +- **`useMonitoringMachines`.** Machines are a global cross-service concept (not per-instance); stays as-is. +- **Per-user scoping.** Scoping is by `service_id` only. +- **Prometheus scrape-target endpoint.** Stays global (see above). +- **Refactoring the hook return shapes or polling intervals.** Only the `serviceId` input is added; outputs and timing are unchanged. +- **Grafana.** Removed by `prometheus-direct-charting`; not present. + +## 5. High-Level Approach + +### 5.1 Backend (small) + +1. `routers/backups.py` — add `service_id: str | None = None` query param to `get_backup_jobs`, `get_backup_runs`, `get_backup_alerts`, `get_backup_dashboard`; thread into `SettingsStore` calls. +2. `services/settings_store.py` — `list_backup_jobs`, `list_backup_runs`, `list_backup_alerts` accept an optional `service_id` filter and add a `WHERE` clause when non-null. (The `backup_jobs`/`backup_runs` tables already carry `service_id` attribution from the earlier change; confirm and add the filter.) +3. The Alertmanager/Prometheus status endpoints already accept `service_id` — no backend change needed there. + +### 5.2 Frontend (most of the work) + +1. `api/client.ts` + `api/backups.ts` — each `fetch*` function gains an optional `serviceId?: string` arg; when provided, append `?service_id=` (or `&service_id=` if other params exist) to the URL. +2. `hooks/useObservability.ts` + `hooks/useBackups.ts` — each hook gains a `serviceId?: string` arg; include it in the `queryKey` (e.g., `["observability", "alerts", serviceId ?? ""]`) so caches don't collide across instances; pass it to the fetch function. +3. `pages/service-tabs/AlertsTab.tsx`, `MetricsTab.tsx`, `JobsTab.tsx` — replace the TODO comments with `useXxx(instance.id)`. +4. Tests: hook tests cover the per-instance queryKey; tab tests cover passing `instance.id`. + +### 5.3 Backward compatibility + +- All new params are optional (`serviceId?: string` / `service_id: str | None = None`). When omitted, behavior is identical to today (first-configured/global). This means the existing dashboard widget callers (which don't pass serviceId because they resolve via widget-instance binding) and any external callers keep working unchanged. + +## 6. Success Criteria / Acceptance Criteria + +1. The three tabs (`AlertsTab`, `MetricsTab`, `JobsTab`) pass `instance.id` to their hooks; the TODO comments are gone. +2. With two instances of a service type configured, viewing instance A's page shows A's data and viewing instance B's page shows B's data (no cross-contamination). +3. The instance switcher on `ServicePage` re-scopes the tab data on switch. +4. Hook `queryKey`s include `serviceId` so React Query caches are per-instance (no stale cross-instance cache hits). +5. Backend backup endpoints accept `service_id` and filter correctly; omitting it returns all (backward-compat). +6. Dashboard widgets render identically before/after (they don't use these hooks — verify no regression). +7. Backend tests (`pytest`) and frontend `npm run build` + `npm run lint` + `npm run test` stay green. + +## 7. Risks and Mitigations + +| Risk | Mitigation | +|------|------------| +| **Cache-key collision** if `serviceId` is forgotten in some hook. | Mandatory: every modified hook includes `serviceId` in `queryKey`; covered by a hook test. | +| **Backup store filter regresses existing callers.** `list_backup_*` with `service_id=None` must return everything. | Add a backend test: `service_id=None` → all rows; `service_id="X"` → only X's rows. | +| **A fetch function drops the param into the URL incorrectly** (encoding, `?` vs `&`). | Use `URLSearchParams` or the existing pattern; one small util if helpful. | +| **Dashboard widget regression** (they use different hooks, but defensive check). | SC6 explicitly verifies widgets render identically; widget tests unchanged. | +| **Review budget (>400 lines).** | Single slice, likely ~250–350 lines (frontend-dominant). If it creeps, split backend-filter from frontend-wiring. | + +## 8. Resolved Questions (no question round needed — investigation settled these) + +- **Q1 — Which hooks?** `useAlertmanagerAlerts`, `useAlertmanagerStatus`, `usePrometheusStatus`, `useBackupJobs`, `useBackupRuns`, `useBackupAlerts`. NOT `usePrometheusTargets` (global scrape config) or `useMonitoringMachines` (global). +- **Q2 — Backend readiness?** Alertmanager/Prometheus status endpoints already take `service_id`. Backup endpoints need it added (data already attributed). +- **Q3 — Backward-compat?** All params optional; omitting = today's behavior. + +## 9. Future Phases + +1. **Per-instance scoping for `usePrometheusTargets`** if it ever becomes a UI concern (today it's an external-scrape endpoint). +2. **A shared `useServiceScopedQuery` helper** if the per-instance `queryKey` pattern repeats beyond these hooks. +3. **Rich PromQL editor / threshold alerting** (separate deferred items, not this change).