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)
This commit is contained in:
2026-03-30 11:47:16 +03:00
parent 7c57c740b4
commit e0a648fb0c
8 changed files with 115 additions and 42 deletions
+13 -6
View File
@@ -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
}
+8 -4
View File
@@ -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
}
+5 -2
View File
@@ -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
}
+25 -8
View File
@@ -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)
}
+21 -7
View File
@@ -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{
+24 -4
View File
@@ -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 >= ?")
@@ -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 @@
<div class="mt-3 flex flex-wrap items-center gap-x-4 gap-y-1.5 text-xs text-[var(--text-secondary)]">
<span class="inline-flex items-center gap-1">
<IconTag size={12} />
{container.image_tag}
{container.instance.image_tag}
</span>
<span class="inline-flex items-center gap-1">
<IconClock size={12} />
{$t('stale.lastAlive')}: {formatDate(container.last_alive_at)}
{$t('stale.lastAlive')}: {formatDate(container.instance.last_alive_at)}
</span>
<span class="rounded bg-[var(--surface-card-hover)] px-1.5 py-0.5 font-mono text-[10px]">
{container.status}
{container.instance.status}
</span>
</div>
@@ -75,7 +75,7 @@
<button
type="button"
disabled={cleaning}
onclick={() => oncleanup(container.id)}
onclick={() => oncleanup(container.instance.id)}
class="inline-flex items-center gap-1.5 rounded-lg border border-[var(--color-danger)] px-3 py-1.5 text-xs font-medium text-[var(--color-danger)] transition-colors hover:bg-[var(--color-danger-light)] disabled:opacity-50 active:animate-press"
>
<IconTrash size={14} />
+14 -6
View File
@@ -209,15 +209,23 @@ export type ProxyHealthStatus = 'unknown' | 'healthy' | 'unhealthy';
/** A container detected as stale by the backend poller. */
export interface StaleContainer {
id: string;
instance: {
id: string;
stage_id: string;
project_id: string;
container_id: string;
image_tag: string;
subdomain: string;
npm_proxy_id: number;
status: string;
port: number;
last_alive_at: string;
created_at: string;
updated_at: string;
};
project_name: string;
stage_name: string;
image_tag: string;
container_id: string;
status: string;
last_alive_at: string;
days_stale: number;
created_at: string;
}
/** A single step in the validation pipeline. */