From e0a648fb0c44854c1abadcd2cac72be137b66abe Mon Sep 17 00:00:00 2001 From: "alexei.dolgolyov" Date: Mon, 30 Mar 2026 11:47:16 +0300 Subject: [PATCH] fix(observability): address final review findings Critical fixes: - Fix StaleContainer frontend type to match nested backend response shape - Guard ContainerID[:12] slice against empty/short IDs in ListAllProxies High-priority fixes: - Support comma-separated severity/source in event log filtering (IN clause) - Eliminate N+1 queries in ListAllProxies and FindStaleInstances (pre-load maps) - Stop leaking internal error messages to API clients (use slog + generic msgs) --- internal/api/proxy.go | 19 +++++++---- internal/api/stale.go | 12 ++++--- internal/api/stats.go | 7 ++-- internal/proxy/manager.go | 33 ++++++++++++++----- internal/stale/scanner.go | 28 ++++++++++++---- internal/store/eventlog.go | 28 +++++++++++++--- .../lib/components/StaleContainerCard.svelte | 10 +++--- web/src/lib/types.ts | 20 +++++++---- 8 files changed, 115 insertions(+), 42 deletions(-) diff --git a/internal/api/proxy.go b/internal/api/proxy.go index 698f7be..e5dd8a0 100644 --- a/internal/api/proxy.go +++ b/internal/api/proxy.go @@ -2,6 +2,7 @@ package api import ( "context" + "log/slog" "net/http" "time" @@ -65,7 +66,8 @@ func (s *Server) createProxy(w http.ResponseWriter, r *http.Request) { p, err := s.proxyManager.CreateProxy(r.Context(), req) if err != nil { - respondError(w, http.StatusInternalServerError, err.Error()) + slog.Error("failed to create proxy", "domain", req.Domain, "error", err) + respondError(w, http.StatusInternalServerError, "failed to create proxy") return } @@ -82,7 +84,8 @@ func (s *Server) listProxies(w http.ResponseWriter, r *http.Request) { proxies, err := s.proxyManager.ListProxies() if err != nil { - respondError(w, http.StatusInternalServerError, err.Error()) + slog.Error("proxy operation failed", "error", err) + respondError(w, http.StatusInternalServerError, "proxy operation failed") return } @@ -104,7 +107,8 @@ func (s *Server) getProxy(w http.ResponseWriter, r *http.Request) { respondNotFound(w, "proxy") return } - respondError(w, http.StatusInternalServerError, err.Error()) + slog.Error("proxy operation failed", "error", err) + respondError(w, http.StatusInternalServerError, "proxy operation failed") return } @@ -145,7 +149,8 @@ func (s *Server) updateProxy(w http.ResponseWriter, r *http.Request) { respondNotFound(w, "proxy") return } - respondError(w, http.StatusInternalServerError, err.Error()) + slog.Error("proxy operation failed", "error", err) + respondError(w, http.StatusInternalServerError, "proxy operation failed") return } @@ -167,7 +172,8 @@ func (s *Server) deleteProxy(w http.ResponseWriter, r *http.Request) { respondNotFound(w, "proxy") return } - respondError(w, http.StatusInternalServerError, err.Error()) + slog.Error("proxy operation failed", "error", err) + respondError(w, http.StatusInternalServerError, "proxy operation failed") return } @@ -184,7 +190,8 @@ func (s *Server) listAllProxies(w http.ResponseWriter, r *http.Request) { views, err := s.proxyManager.ListAllProxies() if err != nil { - respondError(w, http.StatusInternalServerError, err.Error()) + slog.Error("proxy operation failed", "error", err) + respondError(w, http.StatusInternalServerError, "proxy operation failed") return } diff --git a/internal/api/stale.go b/internal/api/stale.go index 26d9c4e..340041e 100644 --- a/internal/api/stale.go +++ b/internal/api/stale.go @@ -22,7 +22,8 @@ func (s *Server) listStaleContainers(w http.ResponseWriter, r *http.Request) { staleInstances, err := s.staleScanner.FindStaleInstances(r.Context()) if err != nil { - respondError(w, http.StatusInternalServerError, "failed to find stale containers: "+err.Error()) + slog.Error("failed to find stale containers", "error", err) + respondError(w, http.StatusInternalServerError, "failed to find stale containers") return } @@ -43,7 +44,8 @@ func (s *Server) cleanupStaleContainer(w http.ResponseWriter, r *http.Request) { respondNotFound(w, "instance") return } - respondError(w, http.StatusInternalServerError, "failed to get instance: "+err.Error()) + slog.Error("failed to get instance", "instance_id", instanceID, "error", err) + respondError(w, http.StatusInternalServerError, "failed to get instance") return } @@ -54,7 +56,8 @@ func (s *Server) cleanupStaleContainer(w http.ResponseWriter, r *http.Request) { } if err := s.cleanupInstance(r, inst); err != nil { - respondError(w, http.StatusInternalServerError, "failed to cleanup instance: "+err.Error()) + slog.Error("failed to cleanup instance", "instance_id", instanceID, "error", err) + respondError(w, http.StatusInternalServerError, "failed to cleanup instance") return } @@ -71,7 +74,8 @@ func (s *Server) bulkCleanupStaleContainers(w http.ResponseWriter, r *http.Reque staleInstances, err := s.staleScanner.FindStaleInstances(r.Context()) if err != nil { - respondError(w, http.StatusInternalServerError, "failed to find stale containers: "+err.Error()) + slog.Error("failed to find stale containers for bulk cleanup", "error", err) + respondError(w, http.StatusInternalServerError, "failed to find stale containers") return } diff --git a/internal/api/stats.go b/internal/api/stats.go index 9f6e088..f1e5ea2 100644 --- a/internal/api/stats.go +++ b/internal/api/stats.go @@ -2,6 +2,7 @@ package api import ( "errors" + "log/slog" "net/http" "github.com/go-chi/chi/v5" @@ -20,7 +21,8 @@ func (s *Server) getInstanceStats(w http.ResponseWriter, r *http.Request) { respondNotFound(w, "instance") return } - respondError(w, http.StatusInternalServerError, "failed to get instance: "+err.Error()) + slog.Error("failed to get instance", "instance_id", instanceID, "error", err) + respondError(w, http.StatusInternalServerError, "failed to get instance") return } @@ -31,7 +33,8 @@ func (s *Server) getInstanceStats(w http.ResponseWriter, r *http.Request) { stats, err := s.docker.GetContainerStats(r.Context(), inst.ContainerID) if err != nil { - respondError(w, http.StatusInternalServerError, "failed to get container stats: "+err.Error()) + slog.Error("failed to get container stats", "container_id", inst.ContainerID, "error", err) + respondError(w, http.StatusInternalServerError, "failed to get container stats") return } diff --git a/internal/proxy/manager.go b/internal/proxy/manager.go index 9a7af02..05189ef 100644 --- a/internal/proxy/manager.go +++ b/internal/proxy/manager.go @@ -233,22 +233,39 @@ func (m *Manager) ListAllProxies() ([]ProxyView, error) { return nil, fmt.Errorf("list instances: %w", err) } + // Pre-load project and stage names to avoid N+1 queries. + allProjects, _ := m.store.GetAllProjects() + projectNames := make(map[string]string, len(allProjects)) + for _, p := range allProjects { + projectNames[p.ID] = p.Name + } + stageNames := make(map[string]string) + for _, p := range allProjects { + stages, _ := m.store.GetStagesByProjectID(p.ID) + for _, s := range stages { + stageNames[s.ID] = s.Name + } + } + for _, inst := range instances { if inst.NpmProxyID <= 0 { continue } - projectName := inst.ProjectID - stageName := inst.StageID - - if proj, err := m.store.GetProjectByID(inst.ProjectID); err == nil { - projectName = proj.Name + projectName := projectNames[inst.ProjectID] + if projectName == "" { + projectName = inst.ProjectID } - if stg, err := m.store.GetStageByID(inst.StageID); err == nil { - stageName = stg.Name + stageName := stageNames[inst.StageID] + if stageName == "" { + stageName = inst.StageID } - destination := fmt.Sprintf("%s:%d", inst.ContainerID[:12], inst.Port) + cid := inst.ContainerID + if len(cid) > 12 { + cid = cid[:12] + } + destination := fmt.Sprintf("%s:%d", cid, inst.Port) if inst.Subdomain != "" { destination = fmt.Sprintf("%s:%d", inst.Subdomain, inst.Port) } diff --git a/internal/stale/scanner.go b/internal/stale/scanner.go index 94063fa..da85e97 100644 --- a/internal/stale/scanner.go +++ b/internal/stale/scanner.go @@ -226,6 +226,20 @@ func (s *Scanner) FindStaleInstances(ctx context.Context) ([]StaleInstance, erro } } + // Pre-load project and stage names to avoid N+1 queries. + allProjects, _ := s.store.GetAllProjects() + projectNames := make(map[string]string, len(allProjects)) + for _, p := range allProjects { + projectNames[p.ID] = p.Name + } + stageNames := make(map[string]string) + for _, p := range allProjects { + stages, _ := s.store.GetStagesByProjectID(p.ID) + for _, st := range stages { + stageNames[st.ID] = st.Name + } + } + now := time.Now().UTC() var result []StaleInstance @@ -254,14 +268,14 @@ func (s *Scanner) FindStaleInstances(ctx context.Context) ([]StaleInstance, erro continue } - // Look up project and stage names. - projectName := inst.ProjectID - stageName := inst.StageID - if proj, err := s.store.GetProjectByID(inst.ProjectID); err == nil { - projectName = proj.Name + // Look up project and stage names from pre-loaded maps. + projectName := projectNames[inst.ProjectID] + if projectName == "" { + projectName = inst.ProjectID } - if stg, err := s.store.GetStageByID(inst.StageID); err == nil { - stageName = stg.Name + stageName := stageNames[inst.StageID] + if stageName == "" { + stageName = inst.StageID } result = append(result, StaleInstance{ diff --git a/internal/store/eventlog.go b/internal/store/eventlog.go index 6c8a458..1414348 100644 --- a/internal/store/eventlog.go +++ b/internal/store/eventlog.go @@ -54,12 +54,32 @@ func (s *Store) ListEvents(filter EventLogFilter) ([]EventLog, error) { var args []any if filter.Severity != "" { - conditions = append(conditions, "severity = ?") - args = append(args, filter.Severity) + parts := strings.Split(filter.Severity, ",") + if len(parts) == 1 { + conditions = append(conditions, "severity = ?") + args = append(args, filter.Severity) + } else { + placeholders := make([]string, len(parts)) + for i, p := range parts { + placeholders[i] = "?" + args = append(args, strings.TrimSpace(p)) + } + conditions = append(conditions, "severity IN ("+strings.Join(placeholders, ",")+")") + } } if filter.Source != "" { - conditions = append(conditions, "source = ?") - args = append(args, filter.Source) + parts := strings.Split(filter.Source, ",") + if len(parts) == 1 { + conditions = append(conditions, "source = ?") + args = append(args, filter.Source) + } else { + placeholders := make([]string, len(parts)) + for i, p := range parts { + placeholders[i] = "?" + args = append(args, strings.TrimSpace(p)) + } + conditions = append(conditions, "source IN ("+strings.Join(placeholders, ",")+")") + } } if filter.Since != "" { conditions = append(conditions, "created_at >= ?") diff --git a/web/src/lib/components/StaleContainerCard.svelte b/web/src/lib/components/StaleContainerCard.svelte index 63dd09b..9e5f12c 100644 --- a/web/src/lib/components/StaleContainerCard.svelte +++ b/web/src/lib/components/StaleContainerCard.svelte @@ -21,7 +21,7 @@ ); const displayName = $derived( - `${container.project_name}-${container.stage_name}-${container.image_tag}` + `${container.project_name}-${container.stage_name}-${container.instance.image_tag}` ); function formatDate(iso: string): string { @@ -59,14 +59,14 @@
- {container.image_tag} + {container.instance.image_tag} - {$t('stale.lastAlive')}: {formatDate(container.last_alive_at)} + {$t('stale.lastAlive')}: {formatDate(container.instance.last_alive_at)} - {container.status} + {container.instance.status}
@@ -75,7 +75,7 @@