Developer
29650ca512
Move to openspec/changes/archive/2026-07-09-per-instance-hook-scoping/ (R100 renames preserved). 9 artifacts. Canonical openspec/specs/ service-instance-scoping/ remains. Resolves multi-instance wrong-data bug (hooks now scope by instance.id; instance switcher re-scopes). Carry-overs: fetchBackupDashboard untouched (design decision 5); subquery scoping for runs/alerts (schema asymmetry).
94 lines
8.4 KiB
Markdown
94 lines
8.4 KiB
Markdown
# 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=<id>` (or `&service_id=<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).
|